diff --git a/.github/workflows/control-bytes.yml b/.github/workflows/control-bytes.yml new file mode 100644 index 0000000000..3a1e0222e2 --- /dev/null +++ b/.github/workflows/control-bytes.yml @@ -0,0 +1,53 @@ +name: Control Bytes + +# Why this is its own workflow instead of a job in `ci.yml` or `lint.yml`: both +# of those list `'**/*.md'`, `content/**`, `docs/**` and `.changeset/**` under +# `paths-ignore`, and GitHub has no per-job path filter. A raw control byte lands +# in markdown exactly as easily as in TypeScript — objectstack#4890 was a NUL in +# a `.claude/` skill file, emitted by the very PR that was writing the rule +# against it — so a gate that cannot see a markdown-only PR rebuilds the hole it +# exists to close. `changeset-guard.yml` sits in this repo for the same reason +# and says so in its own header. +# +# Hence: no `paths` and no `paths-ignore` here, deliberately. +# `scripts/__tests__/check-control-bytes.test.ts` fails if either is ever added. +# +# It needs no install and no build — a checkout plus one `node` call over +# `git ls-files`, a few seconds — so keep it that way if you add checks to it. + +on: + pull_request: + branches: [main, develop] + push: + branches: [main, develop] + workflow_dispatch: + +concurrency: + group: control-bytes-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + control-bytes: + name: Control Byte Scan + runs-on: ubuntu-latest + timeout-minutes: 5 + + steps: + - name: Checkout code + uses: actions/checkout@v7 + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: '22.x' + + # A single raw U+0000 makes grep and ripgrep classify the whole file as + # binary and print no matching lines at all, so the file drops out of code + # search and out of every grep-based lint — with no error to say so. git + # will not warn either: it decides binary-ness from the first 8000 bytes + # only. Reads `git ls-files`, so no install is required. + - name: Scan tracked text files for raw control bytes + run: node scripts/check-control-bytes.mjs diff --git a/content/docs/guide/ci-cd-pipeline.md b/content/docs/guide/ci-cd-pipeline.md index 37d8dbb360..3fc994f178 100644 --- a/content/docs/guide/ci-cd-pipeline.md +++ b/content/docs/guide/ci-cd-pipeline.md @@ -26,6 +26,7 @@ one has its own section below. | `ci.yml` | CI | Push / PR to `main`, `develop` | **Yes** — 6 of its 7 jobs run on PRs | | `lint.yml` | Lint | Push / PR to `main`, `develop`; manual | **Yes** — ESLint **errors** only | | `changeset-guard.yml` | Changeset Bump Policy | PR / push touching `.changeset/**` | **Yes** | +| `control-bytes.yml` | Control Byte Scan | Push / PR to `main`, `develop` — **no path filter**; manual | **Yes** | | `performance-budget.yml` | Bundle Analysis | Push / PR touching `packages/**`, `apps/console/**`, `pnpm-lock.yaml` | **Yes** — the console entry gzip budget | | `live-e2e.yml` | Live E2E (informational) | PR to `main`, `develop` (code paths); nightly cron `30 6 * * *`; manual | No — informational lane, `continue-on-error` | | `labeler.yml` | Auto Label PRs | PR `opened`, `synchronize`, `reopened` | No | @@ -97,6 +98,54 @@ the rules `eslint.config.js` sets to `error` — including the custom `object-ui this workflow was `workflow_dispatch`-only, so every one of those `error` ratchets was inert: each was written specifically to fail CI, and nothing ran them. +## Control Bytes (`control-bytes.yml`) + +**Triggers:** Push and PR to `main`/`develop`, plus manual dispatch — with **no path filter at +all**, which is the point of the workflow. It appears in the checks list as **Control Byte Scan**. + +Runs `scripts/check-control-bytes.mjs`, which reads `git ls-files` and rejects raw control +characters in every tracked **text** file: the C0 range apart from tab, line feed and carriage +return, plus U+007F. No install, no build — a checkout and one `node` call. + +**Why it blocks a merge.** A single raw U+0000 makes grep and ripgrep classify the *entire* file +as binary: they print `binary file matches` and no matching line, so the file silently drops out +of code search and out of every grep-based lint. Nothing else catches it — git decides +binary-ness from the first 8000 bytes only, so a control byte past that offset keeps diffing as +ordinary text, and review cannot see a character that renders as nothing. objectui had no such +guard until objectstack#5425, by which time five files had accumulated the defect. + +The two byte classes carry different harms and the report says which: + +| Byte | Harm | Measured behaviour | +|---|---|---| +| U+0000 | Code-search outage | GNU grep 3.11 and ripgrep 14 both refuse to print matching lines | +| Every other control byte | Invisible, unreviewable literal | Both tools print the line normally | + +Covering only U+0000 would reproduce a known miss: objectstack#5140 shipped a NUL *and* a U+0001 +fourteen bytes away, and the NUL-only scanner reported OK on the second one (objectstack#5157). + +**Why it is a separate workflow.** `ci.yml` and `lint.yml` both list `'**/*.md'`, `content/**`, +`docs/**` and `.changeset/**` under `paths-ignore`, and GitHub has no per-job path filter. Markdown +is exactly the carrier the worst instance of this bug used — objectstack#4890 was a raw NUL in a +`.claude/` skill file, emitted by the PR that was writing the rule forbidding it, leaving the agent +instructions unfindable by `grep -r` with no signal that anything was missing. A path-filtered gate +could not have seen that PR. `scripts/__tests__/check-control-bytes.test.ts` fails if a `paths` or +`paths-ignore` key is ever added here. + +**If it fails:** write the escape sequence (backslash, lowercase `u`, four zeroes) instead of the +byte — the resulting string is byte-identical at runtime. Better still, if the byte was only ever +"a character the data cannot contain" (a join/split separator, a sentinel), use something a reader +can verify: a newline, a comma, or `JSON.stringify`, which needs no impossible character at all. +When writing *about* these bytes in prose or in a tool payload, name them as `U+0000` — a backslash +escape typed into an agent's tool payload gets decoded into the real byte before it reaches disk, +which is how two of the five incidents in this family happened. + +**Known pre-existing offenders.** `KNOWN_OFFENDERS` in the script baselines the files that already +carried a control byte when the gate landed, so it could be switched on as a ratchet. It is not a +skip-list: the scan fails on an entry whose file has been cleaned or deleted, so a fix that forgets +to remove its entry is as red as a new offender. Entries carry the issue tracking their removal +(objectstack#5450) and the map is expected to reach empty and stay there. + ## Performance Budget (`performance-budget.yml`) **Triggers:** Push and PR when changes touch `packages/`, `apps/console/`, or `pnpm-lock.yaml`. diff --git a/package.json b/package.json index 90aa8f18d4..69784b520e 100644 --- a/package.json +++ b/package.json @@ -34,6 +34,7 @@ "type-check": "turbo run type-check", "type-check:coverage": "node scripts/check-type-check-coverage.mjs", "check:spec-symbols": "node scripts/check-spec-symbol-derivation.mjs", + "check:control-bytes": "node scripts/check-control-bytes.mjs", "cli": "node packages/cli/dist/cli.js", "objectui": "node packages/cli/dist/cli.js", "create-plugin": "node packages/create-plugin/dist/index.js", diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/useDatasetFields.catalog.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/useDatasetFields.catalog.test.tsx new file mode 100644 index 0000000000..7db81a0b04 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/useDatasetFields.catalog.test.tsx @@ -0,0 +1,172 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { renderHook, waitFor, cleanup } from '@testing-library/react'; + +/** + * objectstack#5425. `useDatasetFieldCatalog` collapses its `include: string[]` + * prop into one scalar so it can sit in a `React.useEffect` dependency array, + * then expands it again inside the effect. That encode/decode pair used to be + * `join` / `split` on a raw U+0000 byte — a byte that made this entire file + * binary to grep, so nothing in it was findable by content search. + * + * The pair is now `JSON.stringify` / `JSON.parse`, which needs no "character the + * data cannot contain" at all. These tests pin the behaviour that round-trip is + * responsible for, because it is the only thing the swap could have broken: + * which paths get walked, and when the effect re-runs. + * + * No control byte appears in this file, and none is needed to test the fix — + * that is rather the point of the change. + */ + +/** + * The client identity must be STABLE across renders. The real + * `useMetadataClient` memoizes with `useMemo`, and the effect under test lists + * `client` in its dependency array — so a mock that returns a fresh object per + * render re-runs the effect on every render, whose `setState` triggers the next + * render. That is an unbounded loop that OOMs the worker rather than failing an + * assertion, so the faithful mock is the only usable one here. + */ +const { get, client } = vi.hoisted(() => { + const get = vi.fn(); + return { get, client: { get } }; +}); + +vi.mock('../useMetadata', () => ({ + useMetadataClient: () => client, +})); + +import { useDatasetFieldCatalog } from './useDatasetFields'; + +/** Minimal object docs: an opportunity that looks up an account, which looks up a region. */ +const DOCS: Record> = { + opportunity: { + label: 'Opportunity', + fields: { + amount: { type: 'currency', label: 'Amount' }, + account: { type: 'lookup', reference_to: 'account', label: 'Account' }, + }, + }, + account: { + label: 'Account', + fields: { + name: { type: 'text', label: 'Account Name' }, + region: { type: 'lookup', reference_to: 'region', label: 'Region' }, + }, + }, + region: { + label: 'Region', + fields: { code: { type: 'text', label: 'Region Code' } }, + }, +}; + +beforeEach(() => { + get.mockReset(); + get.mockImplementation(async (_type: string, name: string) => DOCS[name] ?? null); +}); + +afterEach(cleanup); + +describe('useDatasetFieldCatalog — the include round-trip', () => { + it('walks a single-hop include path and groups its fields', async () => { + const { result } = renderHook(() => useDatasetFieldCatalog('opportunity', ['account'])); + + await waitFor(() => expect(result.current.loading).toBe(false)); + + const values = result.current.fieldOptions.map((o) => o.value); + expect(values).toContain('amount'); + expect(values).toContain('account.name'); + // The heading is every hop's relationship label plus the target object's + // own label, so a single hop through "Account" onto the Account object + // reads "Account → Account" (buildFieldOptions). + expect(result.current.fieldOptions.find((o) => o.value === 'account.name')?.group).toBe('Account → Account'); + }); + + it('walks a MULTI-hop path — the case the encode/decode pair exists for', async () => { + // `a.b.field` is the ADR-0071 generalisation. If the round-trip mangled the + // path (the real risk when a separator collides with the data), this is + // where it shows: the second hop would never be fetched. + const { result } = renderHook(() => useDatasetFieldCatalog('opportunity', ['account.region'])); + + await waitFor(() => expect(result.current.loading).toBe(false)); + + const values = result.current.fieldOptions.map((o) => o.value); + expect(values).toContain('account.region.code'); + expect(result.current.fieldOptions.find((o) => o.value === 'account.region.code')?.group).toBe( + 'Account → Region → Region', + ); + expect(get).toHaveBeenCalledWith('object', 'region'); + }); + + it('keeps several include paths distinct', async () => { + const { result } = renderHook(() => useDatasetFieldCatalog('opportunity', ['account', 'account.region'])); + + await waitFor(() => expect(result.current.loading).toBe(false)); + + const values = result.current.fieldOptions.map((o) => o.value); + expect(values).toContain('account.name'); + expect(values).toContain('account.region.code'); + }); + + it('treats an empty include list as "base object fields only"', async () => { + // The old code guarded this with a ternary on the joined string, because + // ''.split(sep) yields [''] rather than []. JSON.parse('[]') needs no guard, + // but the observable behaviour must be identical. + const { result } = renderHook(() => useDatasetFieldCatalog('opportunity', [])); + + await waitFor(() => expect(result.current.loading).toBe(false)); + + expect(result.current.fieldOptions.map((o) => o.value)).toEqual(['amount', 'account']); + expect(get).toHaveBeenCalledTimes(1); + }); + + it('re-runs when include CHANGES but not when it is merely a new array', async () => { + // The whole reason the scalar key exists: `include` is a fresh array on + // every render, so keying on it directly would refetch forever. + const { result, rerender } = renderHook(({ include }) => useDatasetFieldCatalog('opportunity', include), { + initialProps: { include: ['account'] }, + }); + await waitFor(() => expect(result.current.loading).toBe(false)); + const afterFirst = get.mock.calls.length; + + // Same contents, new array identity -> no refetch. + rerender({ include: ['account'] }); + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(get.mock.calls.length).toBe(afterFirst); + + // Different contents -> refetch, and the new path resolves. + rerender({ include: ['account.region'] }); + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(get.mock.calls.length).toBeGreaterThan(afterFirst); + expect(result.current.fieldOptions.map((o) => o.value)).toContain('account.region.code'); + }); + + it('still resolves a path whose segment names contain a comma', async () => { + // A regression guard aimed at the fix itself. The obvious repair for the + // U+0000 separator is "use a printable character no name contains" — which + // just relocates the collision. JSON has no such assumption, so a name that + // contains the would-be separator must still round-trip. + const docs = { + ...DOCS, + opportunity: { + label: 'Opportunity', + fields: { 'odd,name': { type: 'lookup', reference_to: 'account', label: 'Odd' } }, + }, + }; + get.mockImplementation(async (_t: string, name: string) => docs[name as keyof typeof docs] ?? null); + + const { result } = renderHook(() => useDatasetFieldCatalog('opportunity', ['odd,name'])); + await waitFor(() => expect(result.current.loading).toBe(false)); + + expect(result.current.fieldOptions.map((o) => o.value)).toContain('odd,name.name'); + }); + + it('falls back to an empty catalog when the base object cannot be read', async () => { + get.mockRejectedValue(new Error('boom')); + const { result } = renderHook(() => useDatasetFieldCatalog('opportunity', ['account'])); + + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.fieldOptions).toEqual([]); + expect(result.current.relationships).toEqual([]); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/useDatasetFields.ts b/packages/app-shell/src/views/metadata-admin/inspectors/useDatasetFields.ts index 37a27efa4d..2ebc76866b 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/useDatasetFields.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/useDatasetFields.ts @@ -243,7 +243,13 @@ export function useDatasetFieldCatalog( include: string[], ): DatasetFieldCatalog { const client = useMetadataClient(); - const includeKey = include.join(''); + // Serialized rather than joined on a separator: this value is only a React + // dependency key, and the separator used to be a raw U+0000 byte picked as + // "a character no path can contain" -- which made this entire file binary to + // grep, so no content search could see it (objectstack#5425). JSON needs no + // impossible character at all, so the round-trip below cannot be broken by + // any path value, and the source stays plain ASCII. + const includeKey = JSON.stringify(include); const [state, setState] = React.useState({ relationships: [], fieldOptions: [], @@ -262,7 +268,7 @@ export function useDatasetFieldCatalog( const baseDoc = await client.get>('object', object); if (cancelled) return; const base = normalizeObject(baseDoc, object); - const includeList = includeKey ? includeKey.split('') : []; + const includeList: string[] = JSON.parse(includeKey); // Walk each included PATH hop-by-hop, fetching every object along the // chain (memoized by name) so multi-hop `a.b.field` paths resolve // (ADR-0021 single-hop, generalized by ADR-0071). Hops can't be fetched @@ -314,7 +320,7 @@ export function useDatasetFieldCatalog( cancelled = true; }; // includeKey captures the include array by value; eslint-disable the - // exhaustive-deps array-identity warning since we key on the joined string. + // exhaustive-deps array-identity warning since we key on the serialized string. }, [client, object, includeKey]); return state; diff --git a/scripts/__tests__/check-control-bytes.test.ts b/scripts/__tests__/check-control-bytes.test.ts new file mode 100644 index 0000000000..24c307df77 --- /dev/null +++ b/scripts/__tests__/check-control-bytes.test.ts @@ -0,0 +1,330 @@ +import { describe, expect, it } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// @ts-expect-error — plain-JS CI helper, intentionally untyped +import { classify, scan, locate, SCANNED_BYTES, KNOWN_OFFENDERS } from '../check-control-bytes.mjs'; + +/** + * objectstack#5425. Two raw U+0000 bytes in + * `packages/app-shell/src/views/metadata-admin/inspectors/useDatasetFields.ts` + * made the whole file binary to grep, so a content search for anything in it + * returned nothing at all — and objectui had no gate that could ever have said + * so. `scripts/check-control-bytes.mjs` is that gate; this is its test. + * + * ## Byte discipline + * + * Every control byte below is produced from a NUMBER (`Buffer.from([0x00])`). + * None is written as a literal, and none is written as a backslash escape: an + * escape typed into an agent's tool payload is decoded into the real byte + * before it reaches disk, which is exactly how objectstack#4763 and #4890 + * happened — both while their author was writing the rule forbidding it. This + * file is inside the gate's own scan scope, so a slip here turns the suite red + * on itself, which is the intended safety net. + */ + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); + +/** A one-byte buffer, built from a number — never a literal. */ +const byte = (n: number) => Buffer.from([n]); + +const NUL = 0x00; +const SOH = 0x01; // the byte objectstack#5140 shipped alongside a NUL (#5157) +const TAB = 0x09; +const LF = 0x0a; +const CR = 0x0d; +const DEL = 0x7f; + +/** Builds a throwaway git repo and runs the REAL `scan()` over it. */ +function withTempRepo(build: (write: (rel: string, contents: Buffer | string) => void, dir: string) => void, run: (dir: string) => T): T { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'check-control-bytes-')); + const write = (rel: string, contents: Buffer | string) => { + const full = path.join(dir, rel); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, contents); + }; + try { + execFileSync('git', ['init', '-q'], { cwd: dir }); + build(write, dir); + execFileSync('git', ['add', '-A', '-f'], { cwd: dir }); + return run(dir); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +describe('classify — text or binary, judged by content alone', () => { + it('never lets a control byte be its own alibi', () => { + // The circularity this guard exists to break: "the file has a NUL, therefore + // it is binary, therefore we do not check it for NULs" is what git does. + expect(classify(Buffer.concat([Buffer.from('plain text'), byte(NUL)]))).toBe('text'); + expect(classify(Buffer.concat([Buffer.from('plain text'), byte(SOH)]))).toBe('text'); + }); + + it('calls genuinely invalid UTF-8 binary', () => { + expect(classify(Buffer.from([0xc0, 0x80, 0x41, 0xf8]))).toBe('binary'); + }); + + it('treats an empty file as text', () => { + expect(classify(Buffer.from(''))).toBe('text'); + }); + + it('leaves UTF-16/UTF-32 alone — their NULs are structural, not a defect', () => { + expect(classify(Buffer.concat([Buffer.from([0xff, 0xfe]), Buffer.from('hi', 'utf16le')]))).toBe('wide-encoding'); + expect(classify(Buffer.from([0x00, 0x00, 0xfe, 0xff, 0x00, 0x41]))).toBe('wide-encoding'); + }); + + it('does not misread a long multi-byte UTF-8 file as binary', () => { + // A leading-window probe truncates a multi-byte character and calls the file + // binary; the decode must read the whole buffer. + expect(classify(Buffer.from(`# Long\n\n${'中文段落,用于跨越任何前缀窗口。'.repeat(4000)}\n`))).toBe('text'); + }); +}); + +describe('SCANNED_BYTES — the covered set', () => { + it('covers the C0 range apart from tab, line feed and carriage return', () => { + for (let b = 0x00; b <= 0x1f; b++) { + const structural = b === TAB || b === LF || b === CR; + expect(SCANNED_BYTES.has(b), `${b.toString(16)} structural=${structural}`).toBe(!structural); + } + }); + + it('covers U+007F and nothing printable', () => { + expect(SCANNED_BYTES.has(DEL)).toBe(true); + for (const b of [0x20, 0x41, 0x7e]) expect(SCANNED_BYTES.has(b)).toBe(false); + }); + + it('goes beyond U+0000 — the objectstack#5157 lesson', () => { + // #5140 shipped a NUL and a U+0001 fourteen bytes apart; the NUL-only + // scanner reported OK on the second. A gate that only knows about U+0000 + // reproduces that miss by construction. + expect(SCANNED_BYTES.has(NUL)).toBe(true); + expect(SCANNED_BYTES.has(SOH)).toBe(true); + }); +}); + +describe('scan — what gets flagged', () => { + it('flags control bytes by carrier, not by file extension', () => { + const { offenders } = withTempRepo( + (write) => { + // The original case: a NUL in a TS source. + write('packages/x/src/protocol.ts', Buffer.concat([Buffer.from("const sep = '"), byte(NUL), Buffer.from("';\n")])); + // objectstack#4890: agent instructions under `.claude/`, markdown, and + // the byte sits well past git's 8000-byte binary sniff window. + write( + '.claude/skills/demo/SKILL.md', + Buffer.concat([Buffer.from(`# Demo skill\n\n${'filler prose. '.repeat(700)}\nsep: `), byte(NUL), Buffer.from('\n')]), + ); + // An extension nobody has seen before must still be scanned — that is + // the property an allow-list cannot have. + write('config/weird.frobnicate', Buffer.concat([Buffer.from('key='), byte(NUL), Buffer.from('\n')])); + // A YAML workflow, same reasoning. + write('.github/workflows/ci.yml', Buffer.concat([Buffer.from('name: ci # '), byte(SOH), Buffer.from('\n')])); + }, + (dir) => scan(dir, new Map()), + ); + + expect(offenders.map((o: { file: string }) => o.file).sort()).toEqual([ + '.claude/skills/demo/SKILL.md', + '.github/workflows/ci.yml', + 'config/weird.frobnicate', + 'packages/x/src/protocol.ts', + ]); + }); + + it('reports which byte it found, not just that something was wrong', () => { + const { offenders } = withTempRepo( + (write) => { + write('a.ts', Buffer.concat([Buffer.from('const a = 1;'), byte(SOH), Buffer.from('\n')])); + }, + (dir) => scan(dir, new Map()), + ); + // The two classes carry different harms, so the report must distinguish them. + expect(offenders[0].bytes).toEqual([SOH]); + expect(offenders[0].bytes).not.toContain(NUL); + }); + + it('leaves clean text of every shape green, tabs and CRLF included', () => { + const { offenders, scanned } = withTempRepo( + (write) => { + write('docs/clean.md', '# Clean\n\n中文说明,带 emoji 🚀 —— 完全合法的 UTF-8。\n'); + write('src/tabs.ts', Buffer.concat([Buffer.from('function f() {'), byte(TAB), Buffer.from('return 1; }\n')])); + write('src/crlf.ts', Buffer.concat([Buffer.from('const a = 1;'), byte(CR), byte(LF)])); + write('.github/workflows/ci.yml', 'name: ci\non: [push]\n'); + }, + (dir) => scan(dir, new Map()), + ); + expect(offenders).toEqual([]); + expect(scanned).toBe(4); + }); + + it('skips real binary assets, wide encodings, dangling symlinks and build output', () => { + const { offenders, skipped } = withTempRepo( + (write, dir) => { + write('assets/pic.png', Buffer.concat([Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), byte(NUL), Buffer.from([0xff, 0xd8, 0xc0, 0x80])])); + write('docs/utf16.txt', Buffer.concat([Buffer.from([0xff, 0xfe]), Buffer.from('hi', 'utf16le')])); + write('packages/x/dist/bundle.js', Buffer.concat([Buffer.from('var a='), byte(NUL), Buffer.from(';\n')])); + fs.symlinkSync('/nonexistent/target', path.join(dir, 'dangling')); + }, + (dir) => scan(dir, new Map()), + ); + expect(offenders).toEqual([]); + expect(skipped.binary).toEqual(['assets/pic.png']); + expect(skipped['wide-encoding']).toEqual(['docs/utf16.txt']); + expect(skipped.unreadable).toEqual(['dangling']); + }); + + it('locates a byte past git’s 8000-byte sniff window', () => { + const { offenders } = withTempRepo( + (write) => { + write('long.md', Buffer.concat([Buffer.from(`# Long\n\n${'filler prose. '.repeat(700)}\nsep: `), byte(NUL), Buffer.from('\n')])); + }, + (dir) => scan(dir, new Map()), + ); + expect(offenders[0].offset).toBeGreaterThan(8000); + expect(offenders[0].line).toBe(4); + expect(offenders[0].column).toBe(6); + }); + + it('counts every occurrence, not just the first', () => { + const { offenders } = withTempRepo( + (write) => { + write('a.ts', Buffer.concat([Buffer.from('a'), byte(NUL), Buffer.from('b'), byte(NUL), Buffer.from('c\n')])); + }, + (dir) => scan(dir, new Map()), + ); + expect(offenders[0].count).toBe(2); + }); +}); + +describe('locate — byte offset to line:column', () => { + it('counts lines from 1 and columns in characters, not bytes', () => { + const buf = Buffer.from('one\n中文x'); + expect(locate(buf, buf.indexOf(0x78))).toEqual({ line: 2, column: 3 }); + }); +}); + +describe('scan — the KNOWN_OFFENDERS baseline is a ratchet, not a skip-list', () => { + const baselineFor = (file: string, bytes: number[]) => new Map([[file, { bytes, issue: 'objectstack#5450' }]]); + + it('lets a declared pre-existing offender through without hiding it', () => { + const { offenders, baselined } = withTempRepo( + (write) => write('legacy.ts', Buffer.concat([Buffer.from('const s = "'), byte(NUL), Buffer.from('";\n')])), + (dir) => scan(dir, baselineFor('legacy.ts', [NUL])), + ); + expect(offenders).toEqual([]); + // Still reported, with its issue — baselined is not the same as invisible. + expect(baselined).toHaveLength(1); + expect(baselined[0].file).toBe('legacy.ts'); + expect(baselined[0].issue).toBe('objectstack#5450'); + }); + + it('does NOT license a byte the entry never declared', () => { + // One entry must not become blanket permission for the whole file — a file + // that gains a new kind of control byte is a new defect. + const { offenders } = withTempRepo( + (write) => write('legacy.ts', Buffer.concat([Buffer.from('const s = "'), byte(NUL), byte(SOH), Buffer.from('";\n')])), + (dir) => scan(dir, baselineFor('legacy.ts', [NUL])), + ); + expect(offenders).toHaveLength(1); + expect(offenders[0].bytes).toEqual([NUL, SOH]); + }); + + it('goes red when a baselined file has been cleaned', () => { + // Without this, the map degrades into a permanent skip-list nobody dares + // delete from, because nobody knows which entries still apply. + const { stale, offenders } = withTempRepo( + (write) => write('legacy.ts', 'const s = "clean";\n'), + (dir) => scan(dir, baselineFor('legacy.ts', [NUL])), + ); + expect(offenders).toEqual([]); + expect(stale).toEqual(['legacy.ts']); + }); + + it('goes red when a baselined file no longer exists', () => { + const { stale } = withTempRepo( + (write) => write('other.ts', 'const s = 1;\n'), + (dir) => scan(dir, baselineFor('deleted.ts', [NUL])), + ); + expect(stale).toEqual(['deleted.ts']); + }); +}); + +describe('repo state — the gate is green on this tree', () => { + const result = scan(repoRoot); + + it('has no un-baselined control byte anywhere in the tree', () => { + expect( + result.offenders.map((o: { file: string; line: number; bytes: number[] }) => `${o.file}:${o.line}`), + 'Run `pnpm check:control-bytes` for the full report and the fix guidance.', + ).toEqual([]); + }); + + it('has no stale baseline entries', () => { + expect( + result.stale, + 'These files are listed in KNOWN_OFFENDERS but are now clean — delete their entries.', + ).toEqual([]); + }); + + it('actually scanned the tree rather than silently matching nothing', () => { + // A scan that finds no files would pass both assertions above for the wrong + // reason — the empty-verdict trap. + expect(result.scanned).toBeGreaterThan(1000); + }); + + it('keeps every baseline entry attached to an issue', () => { + for (const [file, entry] of KNOWN_OFFENDERS as Map) { + expect(entry.issue, `KNOWN_OFFENDERS[${file}] must name the issue tracking its removal`).toMatch(/#\d+/); + expect(entry.bytes.length, `KNOWN_OFFENDERS[${file}] must declare which bytes it covers`).toBeGreaterThan(0); + } + }); +}); + +describe('objectstack#5425 — the file that started this is readable again', () => { + const target = 'packages/app-shell/src/views/metadata-admin/inspectors/useDatasetFields.ts'; + + it('carries no control byte at all', () => { + const buf = fs.readFileSync(path.join(repoRoot, target)); + const found = [...buf].filter((b) => SCANNED_BYTES.has(b)); + expect(found).toEqual([]); + }); + + it('is visible to a content search — the harm the issue actually reported', () => { + // The regression this pins is not "the byte is gone", it is "grep can see + // the file". grep exits 0 and prints the line; before the fix it printed + // `binary file matches` and no line at all. + const out = execFileSync('grep', ['-n', 'includeKey', target], { cwd: repoRoot, encoding: 'utf8' }); + expect(out).toMatch(/includeKey/); + expect(out).not.toMatch(/binary file matches/); + }); +}); + +describe('wiring — the gate is actually reachable and actually runs', () => { + const pkg = JSON.parse(fs.readFileSync(path.join(repoRoot, 'package.json'), 'utf8')); + const workflowPath = path.join(repoRoot, '.github/workflows/control-bytes.yml'); + + it('is exposed as a root package script', () => { + expect(pkg.scripts['check:control-bytes']).toBe('node scripts/check-control-bytes.mjs'); + }); + + it('has a workflow that gates pull requests', () => { + expect(fs.existsSync(workflowPath), 'a check nothing runs is not a gate').toBe(true); + expect(fs.readFileSync(workflowPath, 'utf8')).toMatch(/pull_request:/); + }); + + it('does NOT filter that workflow by path', () => { + // This is the whole reason it is its own workflow rather than a step in + // ci.yml or lint.yml: both of those `paths-ignore` markdown, `content/**`, + // `docs/**` and `.changeset/**`. Markdown is precisely the carrier + // objectstack#4890 was found in, so a path filter here would rebuild the + // hole this guard exists to close. `changeset-guard.yml` is in the repo for + // the same reason and says so in its own header. + const workflow = fs.readFileSync(workflowPath, 'utf8'); + expect(workflow).not.toMatch(/paths-ignore:/); + expect(workflow).not.toMatch(/^\s+paths:/m); + }); +}); diff --git a/scripts/check-control-bytes.mjs b/scripts/check-control-bytes.mjs new file mode 100644 index 0000000000..3a874e1008 --- /dev/null +++ b/scripts/check-control-bytes.mjs @@ -0,0 +1,371 @@ +#!/usr/bin/env node +/** + * Rejects raw control characters in every tracked TEXT file. + * + * Run: node scripts/check-control-bytes.mjs + * node scripts/check-control-bytes.mjs --list # what got scanned / skipped + * Exit: 0 = OK, 1 = a control byte is present, or the baseline below is stale + * + * ## Why this exists + * + * objectui#3425 (objectstack#5425): two raw U+0000 bytes sat in + * `packages/app-shell/src/views/metadata-admin/inspectors/useDatasetFields.ts` + * as a `join`/`split` separator. One of them makes grep and ripgrep classify the + * WHOLE file as binary, so a content search returns nothing at all: + * + * grep -rn "includeKey" packages/app-shell/src/.../inspectors/ + * => grep: .../useDatasetFields.ts: binary file matches (zero lines) + * + * Not a cosmetic problem. Every agent and every human who searches this repo by + * content is blind to that file, and so is every grep-based lint. git will not + * warn either: it decides binary-ness from the first 8000 bytes only, so a + * control byte past that offset keeps diffing as ordinary text. + * + * objectstack has carried the equivalent guard since its own #3127/#4890; this + * repo had nothing, which is why four more files accumulated the same defect + * before anyone looked (objectstack#5450). + * + * ## Two harms, not one — and they are not the same byte set + * + * This distinction is measured, not assumed. Writing one control byte into a + * fixture file and grepping for a literal on a different line, on GNU grep 3.11 + * and ripgrep 14: + * + * - U+0000 is the ONLY byte that triggers binary classification. Both tools + * refuse to print the matching line. + * - U+0001, U+0002, U+0007, U+0008, U+000B, U+000C, U+000E, U+001A, U+001B, + * U+001F and U+007F all print the matching line normally. + * + * So the two classes fail for different reasons, and the report says which: + * + * - U+0000 -> a code-search outage. The file drops out of grep, ripgrep, and + * every tool built on them. + * - every other control byte -> not a search outage, but an INVISIBLE byte in + * a string literal. No reviewer can see it, no diff renders it, and it is + * indistinguishable from its neighbours. It is never intentional in source. + * + * Covering only U+0000 would be the documented mistake: objectstack#5140 shipped + * a NUL *and* a U+0001 fourteen bytes away, and the NUL-only scanner reported OK + * on the second one (objectstack#5157). Hence the wider set — with an honest + * message per class rather than one overstated claim for both. + * + * ## Scope: the carrier, not the use (objectstack#4890) + * + * Deliberately NOT an extension allow-list. The harm named above is a property + * of *grep*, not of JavaScript: it lands identically on a markdown file, a YAML + * workflow, or an extension nobody has invented yet. objectstack's guard began + * as a JS/TS scan and the hole showed up exactly where an allow-list always puts + * it — the PR *writing the rule* "never emit a raw NUL" emitted one into a + * `.claude/` skill file, and the check reported OK. An agent then cannot grep + * the instructions it is supposed to follow, with no signal that anything is + * missing. + * + * A tracked blob is therefore scanned unless, in order: + * + * 1. it is not a regular file (symlink, submodule gitlink) or is unreadable; + * 2. it starts with a UTF-16/UTF-32 byte-order mark — those encodings are text + * whose NULs are STRUCTURAL, so this guard has nothing to say about them; + * 3. its bytes, with the scanned control bytes removed, are not valid UTF-8. + * + * Rule 3 is the whole criterion. The control bytes are stripped BEFORE the + * judgement so that a control byte can never be its own alibi — "the file has a + * NUL, therefore it is binary, therefore we do not check it for NULs" is exactly + * the circularity git falls into, and breaking it is the point. The decode reads + * the ENTIRE file, never a leading window, for the 8000-byte reason above. + * + * ## Byte discipline inside this file + * + * This file is in its own scope, so it must contain no control byte itself. + * Every one below is produced at runtime from a NUMBER. None is written as a + * literal, and none is written as a backslash escape either: a backslash escape + * typed into an agent's tool payload gets decoded into the real byte before it + * ever reaches disk, which is how two of the five incidents in this family + * happened *while their author was writing the rule* (objectstack#4763, #4890). + * Prose here names bytes in `U+XXXX` form for the same reason. + */ + +import { execFileSync } from 'node:child_process'; +import { lstatSync, readFileSync } from 'node:fs'; +import { dirname, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * Control characters that are never legitimate in a text file: the C0 range + * minus the three that are structural in text (U+0009 tab, U+000A line feed, + * U+000D carriage return), plus U+007F delete. + * + * Built from numbers on purpose — see "Byte discipline" above. + */ +export const SCANNED_BYTES = new Set([ + ...Array.from({ length: 0x20 }, (_, i) => i).filter((b) => b !== 0x09 && b !== 0x0a && b !== 0x0d), + 0x7f, +]); + +/** The one byte that makes grep/ripgrep give up on the whole file. */ +const NUL = 0x00; + +/** `U+0000` etc, for messages. Never the byte, never a backslash escape. */ +function nameOf(byte) { + return `U+${byte.toString(16).toUpperCase().padStart(4, '0')}`; +} + +/** + * The six-character escape authors should write instead, assembled from a char + * code so that this source file contains no backslash-u sequence to materialize. + */ +const ESCAPE_HINT = `${String.fromCharCode(0x5c)}u0000`; + +/** + * Belt-and-braces: git already ignores these, so nothing matches today. Kept so + * a future vendored or committed artifact directory cannot quietly turn this + * red — a control byte in a build artifact is that toolchain's business. + */ +const EXCLUDED = /(^|\/)(node_modules|dist|build|\.next|\.turbo|\.wt-[^/]*)\//; + +/** UTF-16/UTF-32 byte-order marks, where NUL bytes are structural, not a bug. */ +const WIDE_BOMS = [ + [0x00, 0x00, 0xfe, 0xff], // UTF-32BE + [0xff, 0xfe, 0x00, 0x00], // UTF-32LE + [0xfe, 0xff], // UTF-16BE + [0xff, 0xfe], // UTF-16LE +]; + +/** + * `path -> { bytes, issue }` — files that ALREADY carry a control byte on the + * day this guard landed, so it could be switched on as a ratchet instead of + * waiting for unrelated packages to be fixed first. + * + * This is a BASELINE, not an escape hatch. It cannot rot, because `scan()` + * fails on an entry whose file is gone or has been cleaned — so fixing a file + * without deleting its entry here is as red as adding a new offender. Adding an + * entry is a deliberate admission that a file ships an unreadable byte: do it + * with an issue number, and treat it as temporary. + * + * objectui's own `check-lint-coverage.mjs` / `check-type-check-coverage.mjs` + * use the same shape (`DEBT`, known gaps with issue numbers). objectstack's + * `check-nul-bytes.mjs` deliberately has no such map — it was introduced into a + * tree that was already clean, which is a luxury this repo does not have today. + * The map is expected to reach empty and stay there. + */ +export const KNOWN_OFFENDERS = new Map([ + ['packages/core/src/evaluator/listConditional.ts', { bytes: [NUL], issue: 'objectstack#5450' }], + ['packages/core/src/utils/record-title.ts', { bytes: [NUL], issue: 'objectstack#5450' }], + ['packages/fields/src/widgets/PeoplePicker.tsx', { bytes: [NUL], issue: 'objectstack#5450' }], + ['packages/plugin-dashboard/src/DatasetWidget.tsx', { bytes: [0x01], issue: 'objectstack#5450' }], +]); + +function hasWideBom(buf) { + return WIDE_BOMS.some((bom) => bom.length <= buf.length && bom.every((b, i) => buf[i] === b)); +} + +/** + * Text or binary, judged by content alone. + * + * @returns {'text' | 'binary' | 'wide-encoding'} + */ +export function classify(buf) { + if (hasWideBom(buf)) return 'wide-encoding'; + // Strip the bytes under investigation first: none of them can appear inside a + // multi-byte UTF-8 sequence (lead bytes are 0xC2+, continuations 0x80-0xBF), + // so removing them cannot break an otherwise-valid sequence. + const dirty = buf.some((b) => SCANNED_BYTES.has(b)); + const probe = dirty ? buf.filter((b) => !SCANNED_BYTES.has(b)) : buf; + try { + new TextDecoder('utf8', { fatal: true }).decode(probe); + return 'text'; + } catch { + return 'binary'; + } +} + +/** + * Byte offset -> line:column, so the author can jump straight to a byte their + * editor renders as nothing and grep refuses to look for. + */ +export function locate(buf, offset) { + let line = 1; + let lineStart = 0; + for (let i = 0; i < offset; i++) { + if (buf[i] === 0x0a) { + line++; + lineStart = i + 1; + } + } + const column = buf.subarray(lineStart, offset).toString('utf8').length + 1; + return { line, column }; +} + +/** + * The one scan. `main()`, `--list` and the test suite all go through here, so + * the tests exercise the real code path rather than a parallel imitation. + * + * @param root repository root to scan + * @param baseline `path -> { bytes, issue }` map of pre-existing offenders + */ +export function scan(root, baseline = KNOWN_OFFENDERS) { + const files = execFileSync('git', ['ls-files', '-z'], { + cwd: root, + encoding: 'buffer', + maxBuffer: 64 * 1024 * 1024, + }) + // `-z` gives NUL-delimited output: the one context where this byte is + // load-bearing rather than a bug. Split on the number, not on a literal. + .toString('utf8') + .split(String.fromCharCode(NUL)) + .filter(Boolean) + .filter((f) => !EXCLUDED.test(f)); + + const offenders = []; + const baselined = []; + const skipped = { binary: [], 'wide-encoding': [], unreadable: [] }; + let scanned = 0; + + for (const file of files) { + const full = join(root, file); + let stat; + try { + // lstat, not stat: a tracked symlink must not be followed (a broken one + // would throw), and a submodule gitlink is a directory here. + stat = lstatSync(full); + } catch { + skipped.unreadable.push(file); + continue; + } + if (!stat.isFile()) { + skipped.unreadable.push(file); + continue; + } + + const buf = readFileSync(full); + const kind = classify(buf); + if (kind !== 'text') { + skipped[kind].push(file); + continue; + } + scanned++; + + /** @type {{ byte: number, offset: number }[]} */ + const hits = []; + for (let i = 0; i < buf.length; i++) { + if (SCANNED_BYTES.has(buf[i])) hits.push({ byte: buf[i], offset: i }); + } + if (hits.length === 0) continue; + + const bytes = [...new Set(hits.map((h) => h.byte))].sort((a, b) => a - b); + const first = hits[0]; + const { line, column } = locate(buf, first.offset); + const record = { file, bytes, count: hits.length, line, column, offset: first.offset }; + + const allowed = baseline.get(file); + // A baseline entry covers a file only for the byte values it declares. A + // file that gains a NEW kind of control byte is a new defect, not covered + // debt — otherwise one entry silently licenses everything else in the file. + if (allowed && bytes.every((b) => allowed.bytes.includes(b))) { + baselined.push({ ...record, issue: allowed.issue }); + continue; + } + offenders.push(record); + } + + // Stale baseline entries: the file was cleaned (or deleted) but its entry + // stayed. Left unchecked, the map degrades into a permanent skip-list that + // nobody dares delete from because nobody knows which lines still apply. + const stillDirty = new Set(baselined.map((b) => b.file)); + const stale = [...baseline.keys()].filter((f) => !stillDirty.has(f)); + + return { offenders, baselined, stale, scanned, skipped, tracked: files.length }; +} + +function repoRoot() { + return resolve(dirname(fileURLToPath(import.meta.url)), '..'); +} + +function summarise({ scanned, skipped }) { + const parts = []; + if (skipped.binary.length) parts.push(`${skipped.binary.length} binary`); + if (skipped['wide-encoding'].length) parts.push(`${skipped['wide-encoding'].length} UTF-16/32`); + if (skipped.unreadable.length) parts.push(`${skipped.unreadable.length} non-regular`); + const tail = parts.length ? `; skipped ${parts.join(', ')}` : ''; + return `scanned ${scanned} tracked text file(s)${tail}`; +} + +function describe(record) { + const names = record.bytes.map(nameOf).join(', '); + const times = record.count === 1 ? '1 occurrence' : `${record.count} occurrences`; + return `${record.file}:${record.line}:${record.column} — ${names} (${times}, first at byte offset ${record.offset})`; +} + +function main() { + const result = scan(repoRoot()); + const { offenders, baselined, stale } = result; + + if (offenders.length === 0 && stale.length === 0) { + const debt = baselined.length ? `; ${baselined.length} baselined, see objectstack#5450` : ''; + console.log(`✅ check-control-bytes: OK (${summarise(result)}${debt}).`); + process.exit(0); + } + + if (offenders.length > 0) { + const plural = offenders.length === 1 ? 'file contains' : 'files contain'; + console.error(`❌ check-control-bytes: ${offenders.length} ${plural} a raw control character\n`); + + const blind = offenders.filter((o) => o.bytes.includes(NUL)); + const invisible = offenders.filter((o) => !o.bytes.includes(NUL)); + + if (blind.length) { + console.error(` ${nameOf(NUL)} — INVISIBLE TO CODE SEARCH. grep and ripgrep classify the`); + console.error(' entire file as binary and print no matching lines at all:\n'); + for (const o of blind) console.error(` • ${describe(o)}`); + console.error(''); + } + if (invisible.length) { + console.error(' Other control characters — these do NOT make grep give up (measured on'); + console.error(' GNU grep 3.11 and ripgrep 14), but they are invisible in every editor and'); + console.error(' every diff, so nobody can review what the literal actually contains:\n'); + for (const o of invisible) console.error(` • ${describe(o)}`); + console.error(''); + } + + console.error(`Write the escape sequence instead of the byte — ${ESCAPE_HINT} for ${nameOf(NUL)}. +The resulting string is byte-identical at runtime, so behaviour does not change. +Better still, if the byte was only ever "a character the data cannot contain" +(a join/split separator, a sentinel), say so in a way a reader can check: a +newline, a comma, or JSON.stringify — which needs no impossible character at all. + +git will not catch this for you: it decides binary-ness from the first 8000 +bytes only, so a control byte past that offset keeps diffing as ordinary text. + +In prose (markdown, agent instructions, PR bodies) write ${nameOf(NUL)} or the words +"NUL byte" — never the byte, and never a bare backslash escape in a tool +payload, which gets decoded into the real byte before it reaches disk.`); + } + + if (stale.length > 0) { + console.error(`\n❌ check-control-bytes: ${stale.length} stale KNOWN_OFFENDERS entr(y/ies) in scripts/check-control-bytes.mjs:\n`); + for (const f of stale) console.error(` • ${f}`); + console.error(` +These files are listed as pre-existing debt but are now clean (or gone). Delete +their entries from KNOWN_OFFENDERS — the baseline is a ratchet, and an entry +nobody removes is how a baseline turns into a permanent skip-list.`); + } + + process.exit(1); +} + +// Run only when invoked directly — the test suite imports `scan`/`classify` +// from here and must not trigger a repo scan (or a process.exit) on import. +// Same guard shape as scripts/check-changeset-no-major.mjs. +const invokedDirectly = process.argv[1] && resolve(process.argv[1]) === fileURLToPath(import.meta.url); + +if (invokedDirectly) { + if (process.argv.includes('--list')) { + const result = scan(repoRoot()); + for (const f of result.skipped.binary) console.log(`binary ${f}`); + for (const f of result.skipped['wide-encoding']) console.log(`wide-encoding ${f}`); + for (const f of result.skipped.unreadable) console.log(`non-regular ${f}`); + for (const b of result.baselined) console.log(`baselined ${describe(b)} [${b.issue}]`); + console.log(`\n${summarise(result)} (of ${result.tracked} tracked path(s))`); + } else { + main(); + } +}