From aacaa012e141b16732c6eefbab6a42af602e2d0a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 12:47:17 +0000 Subject: [PATCH] chore(scripts): the raw-byte gate scans the whole C0 control set, not only NUL (#5157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:nul-bytes` shipped from #4890 scanning 0x00 alone, because 0x00 was the byte whose harm its own failure message could prove: a raw NUL makes grep and ripgrep classify the whole file as binary and silently return zero matches. Measured, that argument really is NUL-specific — GNU grep 3.11 and ripgrep 14.1 report "binary file matches" for a file carrying 0x00 and keep matching normally for one carrying 0x01 or 0x03 — so this change is not that argument extended by assertion. It is a second harm that lands on the whole C0 set. The other C0 controls render as NOTHING wherever a human reads the code. Both specimens this commit removed from the tree read as an empty string while being load-bearing: const key = keyParts.join('<0x01>'); // shows as: keyParts.join('') return `${object}<0x01>${recordId}`; // shows as: plain concatenation grep prints the match, the diff prints the line, and review sees `join('')` — an obviously pointless call a later reader is invited to delete, or a separator-less composite key a later reader is invited to "fix". Code that lies to every reader is worse than code grep cannot find, because nothing signals a second reading exists. Nor can the author search for it: not the escape text (the file holds a byte) and not the byte (nobody can type it). And the accident source does not pick byte values — every occurrence in this repo came from an editing tool materialising an escape while someone wrote ABOUT the byte (#4763, #4890, and PR #5140, where the caught NUL was fixed and a 0x01 fourteen bytes away was not). Scanned set is now `[\x00-\x08\x0b\x0c\x0e-\x1f]`: the C0 range minus tab, LF and CR — the pattern #4890's own manual sweep used before the gate narrowed to NUL. The binary probe widens with it, and that step is load-bearing rather than cosmetic tidiness. A control byte CAN break an otherwise-valid multi-byte sequence: `E4 B8 01 AD` is 中 with a 0x01 spliced into it, and stripping only NUL leaves that undecodable, so the file is skipped as binary and the 0x01 becomes its own alibi — the exact circularity this gate exists to break, one byte value over. Widening cannot err the other way: every scanned byte is <= 0x1f while valid UTF-8 multi-byte sequences are built only from bytes >= 0x80. Measured over all 5448 tracked paths, the widened stripping moves zero files between text and binary. Six raw control bytes already in the tree are escaped here; the NUL-only gate was green over all of them. `login.ts` / `register.ts` carried a 0x03 Ctrl+C case label, `cross-object-rebucket.ts` a 0x01 bucket-key separator (in the comment and in the `join`), `verify-file-references.ts` two 0x01 in `slotKey`. Escaped strings are byte-identical at runtime, so no behaviour changes and nothing ships. The script and its `pnpm check:nul-bytes` command keep their historical names: those strings are referenced from CI, from other gates' comments and from agent instruction files, several owned by other in-flight work, and a rename buys a more accurate name at the price of a half-applied one. The widened semantics are stated in the script header, the failure message and the CI step instead. `--self-test` goes 16 -> 34 assertions. The new ones pin the widening in both directions: every new specimen file contains no NUL anywhere, so the pre-#5157 `buf.indexOf(0)` scan had nothing to find, and reverting the widening turns the self-test red with 8 failures rather than leaving it quietly green. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GX3sL71LFq8m2usg6VqTSE --- .changeset/control-byte-gate-scans-all-c0.md | 18 + .github/workflows/lint.yml | 27 +- packages/cli/src/commands/login.ts | 2 +- packages/cli/src/commands/register.ts | 2 +- .../src/strategies/cross-object-rebucket.ts | 4 +- .../src/verify-file-references.ts | 2 +- scripts/check-nul-bytes.mjs | 334 ++++++++++++++---- 7 files changed, 316 insertions(+), 73 deletions(-) create mode 100644 .changeset/control-byte-gate-scans-all-c0.md diff --git a/.changeset/control-byte-gate-scans-all-c0.md b/.changeset/control-byte-gate-scans-all-c0.md new file mode 100644 index 0000000000..152b5225f4 --- /dev/null +++ b/.changeset/control-byte-gate-scans-all-c0.md @@ -0,0 +1,18 @@ +--- +--- + +chore(scripts): `check:nul-bytes` 扫描面从 NUL 扩到整个 C0 控制字符集,并清掉仓内既存的 6 枚裸控制字节 (#5157) + +这道门禁 #4890 落地时只扫 `0x00`,理由是它的报错文案只论证得了 NUL 的后果(grep/ripgrep 把整个文件当二进制、静默返回零匹配)。这个理由实测确实是 NUL 专属的:GNU grep 3.11 与 ripgrep 14.1 对含 `0x00` 的文件报 "binary file matches",对含 `0x01`/`0x03` 的文件照常匹配。所以本次扩面**不是**把 NUL 的论证外推,而是另一条独立的、落在整个 C0 集上的危害: + +- **这些字节在任何人类阅读的地方都渲染成"什么都没有"**。本次从仓里清掉的两个真实样本,读起来都是空串,而它们是承重的:`const key = keyParts.join('<0x01>')` 在 grep 输出、diff、code review 里都显示成 `keyParts.join('')` —— 一个"显然多余、下一个读者会顺手删掉"的调用;`return \`${object}<0x01>${recordId}<0x01>${field}\`` 显示成三段直接拼接,即"没有分隔符的复合键"这一经典碰撞 bug 的样子,读者会去"修"一个并不存在的缺陷。**对每一个读者说谎的代码,比 grep 找不到的代码更糟**,因为没有任何信号提示还存在第二种读法。 +- **两种拼写都搜不到**。本意写 `\u0001` 的作者,既不能 grep `\u0001`(文件里是字节,不是这段文本),也没法把那个字节敲进搜索框。 +- **事故源不挑字节**。本仓每一例都来自"作者正在写关于这个字节的内容时,编辑工具把转义落成了真字节":#4763(派发文)、#4890(写「不要写裸 NUL」这条规则的过程中,一个裸 NUL 落进 SKILL.md)、PR #5140 —— 也正是催生本单的那次:被门禁抓到的 NUL 修好了,14 字节外的一个 `0x01` 从 NUL-only 的修复下溜走。 + +扫描面现在是 `[\x00-\x08\x0b\x0c\x0e-\x1f]`(C0 全集,排除 tab/LF/CR),与 #4890 议题里当年那次手工扫描的模式一致 —— 门禁当初正是在这一步收窄成了 NUL-only。 + +**二进制判据同步扩面,而且这一步是承重的而非顺手对齐。** 原判据是"剔除 NUL 后整文件 UTF-8 严格解码,解不通才算二进制";现在剔除的是整个扫描集。原因是控制字节**能**打断一个本来合法的多字节序列:`E4 B8 01 AD` 是「中」被塞进一个 `0x01`,只剔 NUL 的话它解码失败 → 文件被判二进制 → 跳过 → **那个 `0x01` 成了自己的不在场证明**,正是本门禁要打破的那个循环(git 对 NUL 掉进去的那个),只是换了一个字节值。反方向不会出错:扫描集全部 `<= 0x1f`,而合法 UTF-8 多字节序列只由 `>= 0x80` 的字节构成,所以剔除它们永远不会破坏一个本来合法的序列。实测全部 5448 个受追踪路径:扩面后文本/二进制的判定**零变化**(仍是 4 个 PNG + 1 个 ICO 跳过),即这条反循环性质是白拿的。 + +**仓内既存的 6 枚裸控制字节一并转义**(改前 NUL-only 门禁全绿,改后判红 4 个文件):`packages/cli/src/commands/login.ts`、`register.ts` 的 `case '<0x03>': // Ctrl+C`(各 1 枚),`packages/services/service-analytics/.../cross-object-rebucket.ts` 的分桶键分隔符(注释 + `join()` 各 1 枚),`packages/services/service-storage/src/verify-file-references.ts` 的 `slotKey()`(2 枚)。转义后的字符串在运行时**逐字节相同**,行为不变,因此不发版。 + +脚本名与 `pnpm check:nul-bytes` 命令名保持不变:这两个字符串被 CI、其他门禁的注释、以及若干 agent 指令文件引用,其中一部分正被其他在飞工作占用;改名换来的是名字更准,代价是一次跨文件、只改了一半的重命名。语义变化写在脚本头、报错文案和 CI 步骤三处。`--self-test` 断言数 16 → 34,新增断言把"改前绿/改后红"钉在代码旁边(每个新样本文件**整个文件都不含 NUL**,所以旧的 `buf.indexOf(0)` 扫描确实无事可做);把扩面回退成 NUL-only,自测立刻 8 条失败。工具链改动,不发版。 diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 591cee535f..a599cfc5c1 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -76,14 +76,25 @@ jobs: - name: Slot-lookup ratchet run: pnpm check:slot-lookup - # Raw NUL guard (#3127): one literal U+0000 byte makes grep/ripgrep treat - # the whole file as binary and silently return ZERO matches — the file drops - # out of code search and out of every grep-based lint, with no error saying - # so. Nothing else catches it: git sniffs only the first 8000 bytes to decide - # binary-ness, and protocol.ts carried its NUL at offset 147230, so it kept - # diffing as ordinary text through review. That blind spot let six files - # accumulate the same defect. Authors must write the unicode escape instead. - - name: Raw NUL byte guard + # Raw control-byte guard (#3127 / #4890 / #5157). Scans every tracked TEXT + # file for a raw C0 control byte — 0x00-0x08, 0x0b, 0x0c, 0x0e-0x1f, i.e. + # everything except tab/LF/CR. Two distinct harms, one gate: + # • A literal U+0000 makes grep/ripgrep treat the whole file as binary and + # silently return ZERO matches — the file drops out of code search and + # out of every grep-based lint, with no error saying so. Nothing else + # catches it: git sniffs only the first 8000 bytes to decide binary-ness, + # and protocol.ts carried its NUL at offset 147230, so it kept diffing as + # ordinary text through review. That blind spot let six files accumulate + # the same defect. + # • The other C0 controls still match in grep but RENDER AS NOTHING, so a + # load-bearing separator reads as an empty string in the diff and in + # review: `keyParts.join('<0x01>')` shows up as `keyParts.join('')`. + # Four tracked source files carried those past the NUL-only gate until + # #5157 widened the scan surface; PR #5140 is the case that found it, + # when a 0x01 sitting 14 bytes from a caught NUL went unfixed. + # The command name stays `check:nul-bytes` for continuity — see the script's + # header for why. Authors must write the unicode escape instead of the byte. + - name: Raw control-byte guard run: pnpm check:nul-bytes # Docs/skills authoring guard (#2035 / ADR-0059): TS code blocks in diff --git a/packages/cli/src/commands/login.ts b/packages/cli/src/commands/login.ts index 2c3ee1ccfc..c7da6466c7 100644 --- a/packages/cli/src/commands/login.ts +++ b/packages/cli/src/commands/login.ts @@ -35,7 +35,7 @@ async function promptPassword(promptText: string): Promise { const handler = (char: string) => { switch (char) { - case '': // Ctrl+C + case '\u0003': // Ctrl+C cleanup(); process.kill(process.pid, 'SIGINT'); break; diff --git a/packages/cli/src/commands/register.ts b/packages/cli/src/commands/register.ts index 9ed15c4f70..892ac0356d 100644 --- a/packages/cli/src/commands/register.ts +++ b/packages/cli/src/commands/register.ts @@ -31,7 +31,7 @@ async function promptPassword(promptText: string): Promise { const handler = (char: string) => { switch (char) { - case '': // Ctrl+C + case '\u0003': // Ctrl+C cleanup(); process.kill(process.pid, 'SIGINT'); break; diff --git a/packages/services/service-analytics/src/strategies/cross-object-rebucket.ts b/packages/services/service-analytics/src/strategies/cross-object-rebucket.ts index a982e0d2f9..f1af8e6ab2 100644 --- a/packages/services/service-analytics/src/strategies/cross-object-rebucket.ts +++ b/packages/services/service-analytics/src/strategies/cross-object-rebucket.ts @@ -118,7 +118,7 @@ export function rebucketCrossObject( resolved[cd.outputName] = cd.fkToAttr.has(fk) ? cd.fkToAttr.get(fk) : RESTRICTED_BUCKET; } - // Bucket key = base dims (unchanged) + resolved attributes. `` is a + // Bucket key = base dims (unchanged) + resolved attributes. `\u0001` is a // separator no group value contains, matching the engine's own convention. const keyParts: string[] = []; // JSON-encoded, so the empty bucket (`null` on both aggregation paths since @@ -128,7 +128,7 @@ export function rebucketCrossObject( // bucket keeps the row's own value verbatim below. for (const f of baseDimFields) keyParts.push(`${f}=${JSON.stringify(row[f] ?? null)}`); for (const cd of crossDims) keyParts.push(`${cd.outputName}=${String(resolved[cd.outputName])}`); - const key = keyParts.join(''); + const key = keyParts.join('\u0001'); let bucket = buckets.get(key); if (!bucket) { diff --git a/packages/services/service-storage/src/verify-file-references.ts b/packages/services/service-storage/src/verify-file-references.ts index 4ee1179f7b..abdb95ca29 100644 --- a/packages/services/service-storage/src/verify-file-references.ts +++ b/packages/services/service-storage/src/verify-file-references.ts @@ -104,7 +104,7 @@ export interface VerifyReferencesOptions { } function slotKey(object: string, recordId: string, field: string): string { - return `${object}${recordId}${field}`; + return `${object}\u0001${recordId}\u0001${field}`; } function fileFieldsOf(engine: VerifyReferencesEngine, objectName: string): string[] { diff --git a/scripts/check-nul-bytes.mjs b/scripts/check-nul-bytes.mjs index 0428e32074..a6510ad1c9 100644 --- a/scripts/check-nul-bytes.mjs +++ b/scripts/check-nul-bytes.mjs @@ -1,7 +1,26 @@ #!/usr/bin/env node // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. // -// check-nul-bytes -- rejects raw NUL (0x00) bytes in every tracked TEXT file. +// check-nul-bytes -- rejects raw C0 control bytes in every tracked TEXT file. +// +// Scanned set (#5157): 0x00-0x08, 0x0b, 0x0c, 0x0e-0x1f -- the whole C0 control +// range except the three bytes that ARE ordinary text structure: tab (0x09), +// LF (0x0a), CR (0x0d). Equivalently `[\x00-\x08\x0b\x0c\x0e-\x1f]`, the exact +// pattern #4890's own manual sweep used before this gate narrowed to NUL. +// +// node scripts/check-nul-bytes.mjs +// node scripts/check-nul-bytes.mjs --self-test # verify the checker itself +// node scripts/check-nul-bytes.mjs --list # what got scanned / skipped +// +// The script keeps its historical file name and its `pnpm check:nul-bytes` +// command name deliberately: those strings are referenced from CI, from other +// gates' comments and from agent instruction files, several of them owned by +// other in-flight work. A rename would buy accuracy in the name at the price of +// a half-applied rename across files this change must not touch -- so the name +// stays historical and the SCOPE is stated here, in the failure message, and at +// the CI step. +// +// ## Why a raw NUL (0x00) is rejected -- the original case // // A single raw NUL makes grep/ripgrep classify the WHOLE file as binary and // silently return zero matches. `grep -n saveMetaItem @@ -17,9 +36,42 @@ // six separate files accumulated the same defect before #3127 fixed them. This // guard is what keeps them from coming back. // -// node scripts/check-nul-bytes.mjs -// node scripts/check-nul-bytes.mjs --self-test # verify the checker itself -// node scripts/check-nul-bytes.mjs --list # what got scanned / skipped +// ## Why the OTHER C0 controls are rejected too (#5157) +// +// The gate first shipped scanning 0x00 only, because 0x00 was the byte whose +// harm its failure message could prove. Measured, that harm really is +// NUL-specific: GNU grep 3.11 and ripgrep 14.1 report "binary file matches" for +// a file carrying 0x00, and keep matching normally for one carrying 0x01 or +// 0x03. So this widening is NOT the NUL argument extended by assertion -- it is +// a second, different harm that lands on the whole C0 set: +// +// 1. The byte RENDERS AS NOTHING, everywhere a human reads the code. Both +// real specimens this change removed from the tree read as an empty string +// while being load-bearing: +// +// const key = keyParts.join('<0x01>'); // shows as: keyParts.join('') +// case '<0x03>': // Ctrl+C // shows as: case '': +// +// grep prints the match, the diff prints the line, and review sees +// `join('')` -- an obviously pointless call a later reader is invited to +// "clean up", collapsing a composite key into an ambiguous concatenation. +// Code that lies to every reader is worse than code grep cannot find, +// because nothing signals that a second reading exists. +// 2. Nobody can search for it. The author who meant \u0001 cannot grep for +// \u0001 (the file holds a byte, not that text) and cannot type the byte +// into a search box either. The occurrence is unfindable in BOTH spellings. +// 3. The accident source does not pick bytes. Every occurrence in this repo's +// history came from an editing tool materialising an escape sequence into +// the real byte while an author was writing ABOUT the byte: #4763 (a +// dispatch prompt), #4890 (a raw NUL landed in SKILL.md while the rule +// "never emit a raw NUL" was being written), and PR #5140 -- the case that +// produced this widening: the NUL this gate caught got fixed, and a 0x01 +// sitting 14 bytes away walked straight past the NUL-only scan. +// +// A byte in this set has no legitimate reason to appear in a text source file. +// Where the VALUE is genuinely wanted (a key separator, a Ctrl-key literal) the +// escape sequence is byte-identical at runtime and is the only spelling that a +// reviewer, a grep and a diff can all see. // // ## Scope: the carrier, not the use (#4890) // @@ -54,40 +106,78 @@ // 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; // the repo has zero such files today (belt-and-braces). -// 3. Its bytes, with NULs removed, are not valid UTF-8. +// 3. Its bytes, with every SCANNED control byte removed, are not valid UTF-8. // -// Rule 3 is the whole criterion, and two properties of it matter: +// Rule 3 is the whole criterion, and three properties of it matter: // -// - NUL is stripped BEFORE the judgement, so a raw NUL 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 it is -// what this guard exists to break. +// - The scanned bytes are stripped BEFORE the judgement, so a byte under +// investigation 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 it is what this guard exists to break. +// - #5157 widened that stripping from NUL to the whole scanned set, and the +// widening is load-bearing rather than cosmetic: a control byte CAN break an +// otherwise-valid multi-byte sequence, which NUL-only stripping would then +// read as "binary". `E4 B8 01 AD` is the character 中 with a stray 0x01 +// dropped into the middle of it -- strip only NUL and that decodes as +// invalid UTF-8, the file is skipped as binary, and the 0x01 is its own +// alibi, one byte value over from the circularity this guard was built to +// break. Widening cannot go wrong in the other direction either: every +// scanned byte is <= 0x1f, while valid UTF-8 multi-byte sequences are built +// exclusively from bytes >= 0x80, so removing them can never break an +// otherwise-valid sequence. // - The decode reads the ENTIRE file, not a leading window. git's 8000-byte // sniff is the documented blind spot above (protocol.ts hid a NUL at byte // 147230); reusing it here would reproduce it. // // A new text file with an extension nobody has seen before therefore gets // scanned by default -- it decodes as UTF-8, so it is text. Only real binary -// assets (the repo's 4 PNGs and 1 ICO today) fail rule 3 and drop out. +// assets (the repo's 4 PNGs and 1 ICO today) fail rule 3 and drop out. Measured +// over all 5448 tracked paths when #5157 landed: the widened stripping moves +// exactly zero files between text and binary, so it buys the anti-circularity +// property above at no cost in false positives. // // There is intentionally NO per-file exemption hatch. No tracked file in this -// repo carries a legitimate raw NUL; if one ever genuinely needs to, that is a -// decision to take in the open, not a line to add to a skip-list. +// repo carries a legitimate raw control byte; if one ever genuinely needs to, +// that is a decision to take in the open, not a line to add to a skip-list. import { execFileSync } from 'node:child_process'; import { lstatSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; -// The escape sequence authors should write instead, and the in-repo precedent. -// Written as an escape, never as the byte -- this file is itself in scope, so a -// literal NUL here would make the guard fail on itself. -const ESCAPE = '\\u0000'; +/** + * The scanned set as a 256-entry lookup: every C0 control except tab, LF and CR. + * A table rather than a regex, because the scan is one pass over BYTES and must + * never have to decode the file first -- a file this gate is interested in is + * precisely one that may not decode cleanly. + */ +const IS_SCANNED = new Uint8Array(256); +for (let b = 0x00; b <= 0x1f; b++) IS_SCANNED[b] = 1; +IS_SCANNED[0x09] = 0; // tab -- ordinary text structure +IS_SCANNED[0x0a] = 0; // LF +IS_SCANNED[0x0d] = 0; // CR + +/** + * The escape an author should have written for a byte, as TEXT. + * Built from the byte value rather than spelled out, because this file is in its + * own scan surface: an exact literal would have to BE the byte, and the byte + * would make the guard fail on itself. + */ +function escapeFor(byteValue) { + return `\\u${byteValue.toString(16).padStart(4, '0')}`; +} + +/** Hex spelling for the failure report, e.g. 0x01. */ +function hex(byteValue) { + return `0x${byteValue.toString(16).padStart(2, '0')}`; +} + +// The in-repo precedent for the NUL case, cited in the failure message. const CONVENTION = 'packages/rest/src/rest-server.ts:1065'; // 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 NUL in a build artifact is that toolchain's business, not ours. +// -- a control byte in a build artifact is that toolchain's business, not ours. const EXCLUDED = /(^|\/)(node_modules|dist|build|\.next|\.turbo)\//; /** UTF-16/UTF-32 byte-order marks, where NUL bytes are structural, not a bug. */ @@ -102,17 +192,31 @@ function hasWideBom(buf) { return WIDE_BOMS.some((bom) => bom.length <= buf.length && bom.every((b, i) => buf[i] === b)); } +/** Every offset in `buf` holding a scanned control byte, in order. One pass. */ +export function findControlBytes(buf) { + const offsets = []; + for (let i = 0; i < buf.length; i++) { + if (IS_SCANNED[buf[i]] === 1) offsets.push(i); + } + return offsets; +} + /** * Text or binary, judged by content alone. * + * @param {Buffer} buf + * @param {number[]} [offsets] precomputed `findControlBytes(buf)`, so a caller + * that already has it does not pay for a second pass. * @returns {'text' | 'binary' | 'wide-encoding'} */ -export function classify(buf) { +export function classify(buf, offsets) { if (hasWideBom(buf)) return 'wide-encoding'; - // Strip NULs first: the byte under investigation must never be the reason we - // decline to investigate. Multi-byte UTF-8 sequences never contain 0x00, so - // removing NULs cannot break an otherwise-valid sequence. - const probe = buf.includes(0) ? buf.filter((b) => b !== 0) : buf; + // Strip the scanned bytes first: a byte under investigation must never be the + // reason we decline to investigate. See the header -- a stray 0x01 spliced + // into a multi-byte sequence would otherwise make the file "binary" and hide + // itself, the same circularity git falls into with NUL. + const found = offsets ?? findControlBytes(buf); + const probe = found.length > 0 ? buf.filter((b) => IS_SCANNED[b] === 0) : buf; try { new TextDecoder('utf8', { fatal: true }).decode(probe); return 'text'; @@ -123,7 +227,7 @@ export function classify(buf) { /** * Byte offset -> line:column, so the author can jump straight to a byte their - * editor renders as nothing and grep refuses to look for. + * editor renders as nothing and grep cannot be asked to look for. */ function locate(buf, offset) { let line = 1; @@ -175,18 +279,18 @@ export function scan(root) { } const buf = readFileSync(full); - const kind = classify(buf); + const offsets = findControlBytes(buf); + const kind = classify(buf, offsets); if (kind !== 'text') { skipped[kind].push(file); continue; } scanned++; - const offsets = []; - for (let i = buf.indexOf(0); i !== -1; i = buf.indexOf(0, i + 1)) offsets.push(i); if (offsets.length === 0) continue; const { line, column } = locate(buf, offsets[0]); - offenders.push({ file, line, column, offset: offsets[0], count: offsets.length }); + const bytes = [...new Set(offsets.map((o) => buf[o]))].sort((a, b) => a - b); + offenders.push({ file, line, column, offset: offsets[0], count: offsets.length, bytes }); } return { offenders, scanned, skipped, tracked: files.length }; @@ -210,52 +314,78 @@ function main() { const { offenders } = result; if (offenders.length === 0) { - console.log(`check-nul-bytes: OK (${summarise(result)}; no raw NUL bytes).`); + console.log(`check-nul-bytes: OK (${summarise(result)}; no raw C0 control bytes).`); process.exit(0); } const plural = offenders.length === 1 ? 'file contains' : 'files contain'; - console.error(`check-nul-bytes: ${offenders.length} ${plural} a raw NUL byte (0x00)\n`); + console.error(`check-nul-bytes: ${offenders.length} ${plural} a raw C0 control byte\n`); for (const o of offenders) { const times = o.count === 1 ? '1 occurrence' : `${o.count} occurrences`; - console.error(` • ${o.file}:${o.line}:${o.column} -- ${times}, first at byte offset ${o.offset}`); + const which = o.bytes.map(hex).join(', '); + console.error(` • ${o.file}:${o.line}:${o.column} -- ${times} of ${which}, first at byte offset ${o.offset}`); } - console.error(` -A raw NUL makes grep/ripgrep treat the entire file as binary and silently return -ZERO matches, so the file drops out of code search and out of every grep-based -lint. git will not warn you: it only scans the first 8000 bytes to decide -binary-ness, so a NUL past that offset keeps diffing as ordinary text. - -That harm is grep's behaviour, not any one language's, so this guard covers every -tracked TEXT file -- markdown and agent instructions under .claude/ included -(#4890), not just JS/TS sources. -Write the escape sequence ${ESCAPE} instead of the byte. The resulting string is -byte-identical at runtime, so behaviour does not change. Existing convention -- -${CONVENTION}: + const seen = [...new Set(offenders.flatMap((o) => o.bytes))].sort((a, b) => a - b); + console.error('\nWrite the escape sequence instead of the byte:\n'); + for (const byteValue of seen) console.error(` ${hex(byteValue)} -> ${escapeFor(byteValue)}`); - const key = environmentId ?? '${ESCAPE}default'; - -Prefer ${ESCAPE} over \\0, which becomes a legacy octal escape error if it is -ever followed by a digit. In prose (markdown, agent instructions), write the -words "NUL byte" or the escape text -- never the byte itself.`); + console.error(` +The resulting string is byte-identical at runtime, so behaviour does not change. + +Why every C0 control byte and not only NUL (#5157): + + • A raw NUL makes grep/ripgrep treat the entire file as binary and silently + return ZERO matches, so the file drops out of code search and out of every + grep-based lint. git will not warn you: it decides binary-ness from the + first 8000 bytes only, so a NUL past that offset keeps diffing as text. + • The other C0 controls keep matching in grep -- and read worse. They render + as NOTHING, so \`keyParts.join('${escapeFor(0x01)}')\` appears in grep output, in the + diff and in review as \`keyParts.join('')\`: a load-bearing separator that + reads as an empty string, which a later reader is invited to delete. And + the author who meant ${escapeFor(0x01)} can grep for neither spelling -- not the + escape text (the file holds a byte) and not the byte (nobody can type it). + • Every occurrence in this repo came from an editing tool materialising an + escape into the real byte while someone was writing ABOUT the byte (#4763, + #4890, PR #5140). That slip does not pick byte values, so neither does this + gate. + +That harm is not any one language's, so this guard covers every tracked TEXT +file -- markdown and agent instructions under .claude/ included (#4890), not +just JS/TS sources. + +Existing convention for the NUL case -- ${CONVENTION}: + + const key = environmentId ?? '${escapeFor(0x00)}default'; + +Prefer ${escapeFor(0x00)} over \\0, which becomes a legacy octal escape error if it +is ever followed by a digit. In prose (markdown, agent instructions), write the +byte's name or the escape TEXT -- never the byte itself.`); process.exit(1); } // ── Self-test ──────────────────────────────────────────────────────────────── // // Builds a throwaway git repo in a temp dir and runs `scan()` -- the SAME -// function main() calls -- over it. Every NUL below is produced at runtime from -// a byte value; none is written as a literal, because this file is in its own -// scope and a literal would make the guard fail on itself. +// function main() calls -- over it. Every control byte below is produced at +// runtime from a byte value; none is written as a literal, because this file is +// in its own scan surface and a literal would make the guard fail on itself. function selfTest() { const failures = []; + // Counted rather than written down: some assertions run inside a loop, and a + // hand-kept total in the success line is exactly the kind of number that + // drifts silently once someone adds a case. + let checked = 0; const assert = (cond, msg) => { + checked++; if (!cond) failures.push(msg); }; - const NUL = Buffer.from([0x00]); + const byte = (v) => Buffer.from([v]); + const NUL = byte(0x00); + const SOH = byte(0x01); // the PR #5140 specimen + const ETX = byte(0x03); // Ctrl+C, as a CLI key literal const dir = mkdtempSync(join(tmpdir(), 'check-nul-bytes-selftest-')); const write = (rel, contents) => { const full = join(dir, rel); @@ -278,6 +408,34 @@ function selfTest() { ); // The historical case: a NUL in a TS source. write('packages/x/src/protocol.ts', Buffer.concat([Buffer.from("const sep = '"), NUL, Buffer.from("';\n")])); + // #5157 specimen 1: a 0x01 composite-key separator, with NO NUL anywhere in + // the file. This is the shape PR #5140 found 14 bytes from a NUL, and the + // shape two files in this repo carried past the NUL-only gate for months. + write('packages/x/src/key.ts', Buffer.concat([Buffer.from("const key = parts.join('"), SOH, Buffer.from("');\n")])); + // #5157 specimen 2: a Ctrl-key literal in a CLI prompt loop. + write('packages/cli/src/login.ts', Buffer.concat([Buffer.from(" case '"), ETX, Buffer.from("': // Ctrl+C\n")])); + // #5157 specimen 3: the far end of the range, plus the two vertical-space + // controls that sit between the exempt ones. + write( + 'docs/range.md', + Buffer.concat([ + Buffer.from('unit sep '), + byte(0x1f), + Buffer.from(' vt '), + byte(0x0b), + Buffer.from(' ff '), + byte(0x0c), + Buffer.from('\n'), + ]), + ); + // #5157, the anti-circularity case: a stray 0x01 spliced INSIDE a multi-byte + // sequence. E4 B8 AD is 中; with the 0x01 in the middle, NUL-only stripping + // leaves invalid UTF-8, the file reads as "binary", and the 0x01 becomes its + // own alibi. Stripping the whole scanned set is what keeps it visible. + write( + 'docs/split-sequence.md', + Buffer.concat([Buffer.from('head '), byte(0xe4), byte(0xb8), SOH, byte(0xad), Buffer.from(' tail\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='), NUL, Buffer.from('\n')])); @@ -288,10 +446,22 @@ function selfTest() { write('docs/long.md', `# Long\n\n${'中文段落,用于跨越任何前缀窗口。'.repeat(4000)}\n`); write('src/clean.ts', "export const sep = '\\u0000';\n"); write('.github/workflows/ci.yml', 'name: ci\non: [push]\n'); - // Real binary assets: a PNG header and an ICO header, both carrying NULs. + // The three exempt controls are ordinary text structure and must stay green, + // CRLF endings included. DEL (0x7f) rides along: it is not a C0 control, so + // it is deliberately outside this gate's set and must not be flagged either. + write( + 'src/whitespace.ts', + Buffer.concat([Buffer.from('const a\t= 1;\r\nconst b = 2;\r\n'), byte(0x7f), Buffer.from('\n')]), + ); + // Real binary assets: a PNG header and an ICO header, both carrying NULs and + // other control bytes -- they must stay binary under the WIDENED stripping. write( 'assets/pic.png', - Buffer.concat([Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), NUL, Buffer.from([0xff, 0xd8, 0xc0, 0x80])]), + Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), + NUL, + Buffer.from([0xff, 0xd8, 0xc0, 0x80]), + ]), ); write('assets/icon.ico', Buffer.from([0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x10, 0x10, 0xff, 0xfe, 0xc0])); // UTF-16LE text: valid text whose NULs are structural, not a defect. @@ -304,12 +474,53 @@ function selfTest() { execFileSync('git', ['add', '-A', '-f'], { cwd: dir }); const { offenders, scanned, skipped } = scan(dir); - const flagged = new Set(offenders.map((o) => o.file)); + const flagged = new Map(offenders.map((o) => [o.file, o])); assert(flagged.has('.claude/skills/demo/SKILL.md'), '#4890: markdown under .claude/ must be flagged'); assert(flagged.has('packages/x/src/protocol.ts'), 'the original JS/TS case must still be flagged'); assert(flagged.has('config/weird.frobnicate'), 'an unknown extension holding text must still be scanned'); + // ── #5157: the widening, proved in both directions ────────────────────── + // + // Forward -- each specimen is flagged now. + assert(flagged.has('packages/x/src/key.ts'), '#5157: a 0x01 key separator must be flagged'); + assert(flagged.has('packages/cli/src/login.ts'), '#5157: a 0x03 Ctrl-key literal must be flagged'); + assert(flagged.has('docs/range.md'), '#5157: 0x0b / 0x0c / 0x1f must be flagged'); + assert(flagged.has('docs/split-sequence.md'), '#5157: a 0x01 inside a multi-byte sequence must be flagged'); + // + // Reverse -- the SAME fixtures were green under the NUL-only gate, and not + // by accident of fixture construction: none of them contains a NUL at all, + // so the pre-#5157 scan (`buf.indexOf(0)`) had literally nothing to find. + // That is the "green before / red after" proof, recorded next to the code it + // is about instead of in a PR description that cannot fail. + const nulOnlyGateWouldFlag = (rel) => readFileSync(join(dir, rel)).includes(0x00); + for (const rel of ['packages/x/src/key.ts', 'packages/cli/src/login.ts', 'docs/range.md', 'docs/split-sequence.md']) { + assert(!nulOnlyGateWouldFlag(rel), `#5157 reverse: ${rel} carries no NUL, so the NUL-only gate passed it`); + } + // ...and the anti-circularity fixture would not even have been SCANNED + // before: strip NUL only, and `head E4 B8 01 AD tail` fails to decode. + const splitSeq = readFileSync(join(dir, 'docs/split-sequence.md')); + let nulOnlyProbeDecodes = true; + try { + new TextDecoder('utf8', { fatal: true }).decode(splitSeq.filter((b) => b !== 0x00)); + } catch { + nulOnlyProbeDecodes = false; + } + assert(!nulOnlyProbeDecodes, '#5157: NUL-only stripping would misread the split-sequence file as binary'); + assert(classify(splitSeq) === 'text', '#5157: widened stripping keeps the split-sequence file scannable'); + + // Which byte it was is reported, so the prescription can name the escape. + assert( + flagged.get('packages/x/src/key.ts')?.bytes.join() === '1', + `the offending byte value is reported, got ${flagged.get('packages/x/src/key.ts')?.bytes}`, + ); + assert( + flagged.get('docs/range.md')?.bytes.join() === [0x0b, 0x0c, 0x1f].join(), + `all distinct offending bytes are reported, got ${flagged.get('docs/range.md')?.bytes}`, + ); + assert(escapeFor(0x01) === '\\u0001' && escapeFor(0x00) === '\\u0000', 'the prescribed escape is per-byte'); + assert(hex(0x0b) === '0x0b', 'the reported hex is two-digit'); + assert(!flagged.has('assets/pic.png'), 'a real binary asset must not be flagged'); assert(!flagged.has('assets/icon.ico'), 'an ICO must not be flagged'); assert(skipped.binary.length === 2, `exactly the 2 binary assets skip, got ${skipped.binary.length}`); @@ -320,24 +531,27 @@ function selfTest() { ['docs/clean.md', 'docs/long.md', 'src/clean.ts', '.github/workflows/ci.yml'].every((f) => !flagged.has(f)), 'clean text of every shape stays green', ); + assert(!flagged.has('src/whitespace.ts'), 'tab / CR / LF / DEL are outside the scanned set and stay green'); assert( !skipped.binary.includes('docs/long.md'), 'a long multi-byte UTF-8 file must not be misread as binary (leading-window truncation)', ); - assert(scanned >= 7, `every text fixture is actually scanned, got ${scanned}`); + assert(scanned >= 11, `every text fixture is actually scanned, got ${scanned}`); - // The location report points at the NUL, not at byte 0. - const skill = offenders.find((o) => o.file === '.claude/skills/demo/SKILL.md'); + // The location report points at the byte, not at byte 0. + const skill = flagged.get('.claude/skills/demo/SKILL.md'); assert(skill && skill.offset > 8000, "a NUL past git's 8000-byte sniff window is still located"); assert( skill && skill.line === 4 && skill.column === 6, - `line:col points at the NUL, got ${skill?.line}:${skill?.column}`, + `line:col points at the byte, got ${skill?.line}:${skill?.column}`, ); // classify() is the criterion; state it directly too. assert(classify(Buffer.concat([Buffer.from('plain text'), NUL])) === 'text', 'a NUL alone never makes a file binary'); + assert(classify(Buffer.concat([Buffer.from('plain text'), SOH])) === 'text', 'a 0x01 alone never makes a file binary'); assert(classify(Buffer.from([0xc0, 0x80, 0x41, 0xf8])) === 'binary', 'invalid UTF-8 is binary'); assert(classify(Buffer.from('')) === 'text', 'an empty file is text'); + assert(findControlBytes(Buffer.from('a\tb\r\nc\n')).length === 0, 'tab / CR / LF are not control-byte hits'); } finally { rmSync(dir, { recursive: true, force: true }); } @@ -347,7 +561,7 @@ function selfTest() { for (const f of failures) console.error(` • ${f}`); process.exit(1); } - console.log('✓ check-nul-bytes --self-test: 16 assertions over a temp git repo (real scan() path)'); + console.log(`✓ check-nul-bytes --self-test: ${checked} assertions over a temp git repo (real scan() path)`); } if (process.argv.includes('--self-test')) {