chore(repo): replace four literal NUL bytes with the escape, gate the class (#787)

[Samhit21]

Summary

Four source files carried a raw 0x00 where the six-character JS escape belongs. The reasoning was sound every time — a NUL cannot occur in a résumé field, so a composite key joined with it is collision-free by construction — and the runtime string is identical either way. Only the encoding was wrong.

File	Line	Byte offset
src/components/features/JobRepostArchiveDialog.tsx	79	4149
src/hooks/useJobSearch.ts	198, 203	8382
src/lib/heuristics/fixture-match.ts	321	14083
src/lib/heuristics/extract/education.ts	562	28911

Encoding-only, so no test moved. A full git ls-files sweep confirms the only NULs left in the repo are the two Poppins faces under src/assets/fonts/ and the 58 PDF fixtures.

The gate

scripts/check-no-literal-nul.mjs (+ 13 unit tests), wired into verify. Without it this recurs: #786 shipped a fifth instance through review and #125 fixed a sixth in June. The idiom is individually correct each time someone reaches for it, so "remember not to do this" is not a fix.

The one design call worth reviewing is the skip list. It is by declared file extension, matched on the path — deliberately not content sniffing. Every binary-detection heuristic keys on the presence of a NUL byte, so the obvious "skip files that look binary" implementation skips exactly the files the gate exists to catch and passes forever. A new binary asset type is therefore a deliberate edit to BINARY_EXTENSIONS, which is the right amount of friction for teaching a correctness gate to ignore a file. The reasoning is in the docblock so it survives the next simplification pass.

Fail-before evidence

Reintroducing the byte into one file:

✗ src/hooks/useJobSearch.ts:198:67 — literal NUL (byte offset 8971)

1 literal NUL byte(s) in 1 of 1007 scanned file(s).
exit=1

Clean: ✓ no literal NUL bytes: 1009 tracked text file(s) scanned (62 binary path(s) skipped by extension).

The test suite also asserts the acceptance criterion directly — scanPaths(trackedFiles()) must be empty — so the class goes red in CI whether or not anyone ran npm run check:nul.

Review focus

• check:nul is wired into verify:quick as well as verify. The issue only asked for verify. I added both because verify:quick's stated criterion is "only what a bad edit actually breaks": the check costs ~50ms and all four occurrences were .ts under src/, which is precisely what the Stop sentinel fires on. Say the word and I will drop it back to verify only.
• BINARY_EXTENSIONS covers more formats than the repo currently commits (.mp4, .wasm, .zip, …). Defensive, so the first .png someone adds does not fail the build for an unrelated reason. Trim if you would rather the list describe only what exists.

One acceptance criterion is only partly satisfiable here

JobRepostArchiveDialog.tsx renders a normal textual diff on GitHub after the change.

It will render Bin 7426 -> 7431 in this PR, and that is unavoidable: git sniffs both sides of a diff and the old blob is the binary one. The property does hold from the next change onward. Verified by committing the fixed blob to a scratch repo and editing it:

 f.tsx | 2 ++
 1 file changed, 2 insertions(+)

So this one file is unreviewable inline in this PR only. Its change is the single separator on line 79.

Test plan

• OFFLINECV_FULL_TESTS=1 npm run verify — 364 test files, 5918 passed / 10 skipped, build 5.55s, check:core publishable, fallow ✓ No issues in 7 changed files
• scripts/check-no-literal-nul.test.mjs — 13 passed
• Gate fails on a deliberately reintroduced NUL (above), passes on the fixed tree
• Byte-verified with count(b"\x00") rather than by eye, per the issue's warning

Note for anyone running verify locally on a stale install: check:core fails with Cannot find package 'rollup-plugin-dts' — declared in both manifests but absent from node_modules. npm install fixes it, lockfile unchanged. It reproduces on main with this branch stashed, so it is not from these changes.

The escape-decoding hazard the issue warns about is real and fired during this work: writing the commit message, a typed escape was decoded into an actual NUL byte in COMMIT_EDITMSG. Caught by byte-counting, not by reading. Worth knowing it applies to commit messages too, not only to source files.

Closes #787

[cloudflare-workers-and-pages]

Deploying offlinecv with    Cloudflare Pages

Latest commit:	835c742
Status:	✅  Deploy successful!
Preview URL:	https://a11d8cd5.offlinecv.pages.dev
Branch Preview URL:	https://gh-787-literal-nul-bytes.offlinecv.pages.dev

View logs

[s-annam]

Approving. 0 Blocking, 4 Secondary, 4 Nits — verdict rule: 0 Blocking → APPROVE, and nits never soft-gate. Reviewed at 835c742.

This is the right shape for the problem: the four substitutions are encoding-only, and the gate is designed against the trap that would have made it worthless. I checked the skip-list reasoning specifically, since it is the one thing the body asks for — skipping by declared path extension rather than by content sniffing is correct, and the docblock's statement of why (every binary-detection heuristic keys on the NUL, so a sniffing gate skips exactly the files it exists to catch) is the part worth keeping through the next simplification pass.

What I verified rather than took on trust

• The one file nobody can review inline. JobRepostArchiveDialog.tsx renders as Bin 7426 -> 7431, so I reconstructed both blobs and normalised the separator in each direction (tr -d '\000' on the old, s/\\u0000//g on the new). They are byte-identical. The change really is the single separator on line 79 and nothing else.
• Zero NULs remain. Independent sweep over git ls-files -z outside the extension list: offenders: []. The only NUL-carrying tracked paths are the two Poppins faces (15400 / 15451 NULs) and the 60 PDF fixtures.
• The gate fails when fed one. Wrote a real 0x00 into a staged scratch file: ✗ scripts/__nul-probe.tmp.mjs:1:12 — literal NUL (byte offset 11), exit=1. Removed afterwards; tree clean.
• npm run verify is green here — 364 test files, 5918 passed / 10 skipped, build 3.87s, check:core fine (I did not hit the rollup-plugin-dts problem the body notes), VERIFY_EXIT=0. Machine load average 2.35 on 10 cores at the start of the run.
• The gate costs 90ms, measured three times at real 0.09 for the script alone — comfortably inside what verify:quick should carry, so the Review-focus call to wire it into both is right. npm run check:nul reports 1009 tracked text file(s) scanned (62 binary path(s) skipped).
• The new suite really runs. vite.config.ts test.include covers scripts/**/*.test.mjs, and select-tests.mjs falls back to the full suite on any scripts/ change — so describe("the repository itself") is what carries the acceptance criterion into CI. That is load-bearing; see Secondary 1.

Acceptance criteria (#787)

AC	Result
Zero literal 0x00 outside known-binary extensions	✅ verified independently, not via the gate
npm run test green with no test changes	✅ 5918 passed; no pre-existing test file is in the diff
A gate that fails verify on a new literal NUL, with fail-before evidence	✅ reproduced at exit 1
JobRepostArchiveDialog.tsx renders a normal textual diff	⚠️ not in this PR, and cannot be — git sniffs both sides and the old blob is the binary one. The body says so plainly and demonstrates the property holds from the next change. Correct disclosure of an AC that is unsatisfiable inside the PR that fixes it; not held against the change.

Secondary

1. check:nul is not mirrored into the required CI job. .github/workflows/ci.yml deliberately enumerates each gate (check:fixtures L49, check:baselines L57, check:core L70) instead of calling npm run verify, and L60–69 spells out why: "branch protection requires THIS job… without this line a push that skips the pre-push hook merges green." The new gate went into verify / verify:quick only. The class is still caught in CI — describe("the repository itself") runs under npm run test:coverage — so this is not a hole, and the body says as much. What is lost is the message: a merge-queue failure surfaces as expected [ { relPath: … } ] to deeply equal [] rather than ✗ file:line:col — literal NUL plus the "verify by counting bytes, not by eye" remediation text, which is the exact guidance the docblock was written to deliver at the moment someone needs it. One line beside the other three closes it.

2. scripts/check-no-literal-nul.mjs is missing from vite.config.ts coverage.include. The four other tested gates under scripts/ are enumerated there (L336–339), and the comment above the list says the enumeration exists so that "forget to add a new tested gate here and fallow merely scores it 0% and complains, loudly." This is the fifth tested gate and the first not listed. Honest bound: .fallowrc.jsonc health.ignore already carries scripts/**, so fallow's complexity/CRAP pass skips it either way — I confirmed the audit reports nothing from scripts/ on this branch. So this is consistency, not a live failure. It is still a live convention: select-tests.mjs was added to that list three days ago in #828, with health.ignore already in place.

3. The verify:quick change leaves its two descriptions stale, and one of them is a failure message. scripts/hooks/lint_and_test.sh:59 still prints verify:quick failed (typecheck/lint/tests) and its docblock (L6) still says "typecheck, lint, and the change-scoped test run"; CLAUDE.md:72 still reads inner-loop gate: typecheck → lint → change-scoped tests. Concretely: an agent edit reintroduces a NUL, Stop fires, and the developer reads a message naming three things none of which is what failed. CLAUDE.md:78's paragraph enumerating what verify:quick skips is still accurate as written (check:nul is not in that list), so only the two summary lines need to move. See the inline comment on package.json.

4. The test plan quotes a fallow result that does not reproduce. The box reads fallow ✓ No issues in 7 changed files. Running npm run verify on this branch, the fallow step prints:

✗ 106 lines (0.1%) duplicated across 2 files
✗ 3 above threshold · 90 analyzed
✗ complexity: 3 findings · duplication: 2 clone groups · 7 changed files
  audit gate excluded 3 inherited findings (run with --gate all to enforce)
fallow audit exited non-zero (report-only, ignored)

The string No issues appears nowhere in the run. Nothing is wrong with the change — all three complexity findings are pre-existing (education.ts, useJobSearch.ts), both clone groups are the pre-existing JobRepostArchiveDialog.tsx ↔ JobArchiveSweepDialog.tsx overlap, every one is in fallow's excluded-inherited bucket, fallow is report-only, and verify exits 0, which I reproduced. The ticked box is legitimately ticked; it is the quoted line inside it that has to round-trip, and this repo holds quoted gate output to that standard. Worth correcting before the squash, since this body is what a bisecting reader trusts later.

Nits

Four, all inline, all non-blocking: a docblock count that is off by two on the day it lands (with a one-click suggestion), the run-as-script guard that no-ops through a symlinked path, git ls-files not covering untracked files at the Stop-hook layer, and one note on the fail-before evidence block below.

On the fail-before evidence: the failing run reports 1 of 1007 scanned file(s) and the clean run 1009 tracked text file(s). Those two are from different trees — 1007 is this branch minus the gate and its test — so the block reads as one demonstration when it is two. It does not weaken the claim; I reproduced the failure on the current tree and the gate exits 1 as advertised. Worth a sentence saying so, or a re-run of both halves against the same tree.

Why this review pushed nothing

I had four small non-behavioural fixes in hand and deliberately did not commit them. This is a named contributor's in-repo branch, which means an auto-fix push is allowed but a follow-up collapse is not — force-pushing your branch would destroy your local copy and rewrite your authorship. The branch is at exactly one commit today; a fix commit from me would make it two with no way for me to restore the invariant, which trades a hard rule for two comment edits. So everything landed as review comments instead, with suggestion blocks where the fix is a literal line replacement — apply those in one click and the commits stay yours. Amend rather than adding a commit, so the branch stays at one.

Gates

Gate	Result
/code-review generic pass (high)	ran — no blocking correctness findings; its ci.yml, coverage-include, hook-message and symlink-guard findings are folded in above after I re-verified each
3a fixture PII	skipped — no fixture added or changed. npm run check:fixtures run anyway: ✓ 60 PDFs and 16 ground-truth sidecars — all personas synthetic
3b design-system / reuse	skipped — no new file under src/components/, no raw interactive elements added
3c style tokens	✅ clean — no hex, no raw palette class, no manual dark: in the + lines
3d fallow	✅ no new findings; 3 complexity + 2 clone groups all inherited
3e command-level bugs in script files	✅ ran — one Nit (the symlink guard), nothing that fires on normal invocation. No flag inventions, no word-splitting, no swallowed errors; the gate fails closed if git is unavailable and uses process.exitCode rather than process.exit, so its output flushes
3f description accuracy	overclaims in one place (Secondary 4); the line-number table, the sweep result, the 13-test count and the verify numbers all round-trip exactly

---

Reviewed by: Claude Opus 5 (high)

[s-annam]

Secondary — the two places that describe verify:quick are now stale.

Adding check:nul here is the right call and I agree with the Review-focus reasoning: the check is 90ms measured (real 0.09, three runs) and all four occurrences were .ts under src/, which is exactly the Stop sentinel. Keep it.

What did not move with it:

• scripts/hooks/lint_and_test.sh:59 prints verify:quick failed (typecheck/lint/tests) — an agent edit that reintroduces a NUL now fails Stop with a message naming three things, none of which is the cause.
• scripts/hooks/lint_and_test.sh:6 — "typecheck, lint, and the change-scoped test run."
• CLAUDE.md:72 — inner-loop gate: typecheck → lint → change-scoped tests.

CLAUDE.md:78's paragraph about what verify:quick skips is still correct as written, since check:nul is not in that list — so this is two summary lines and one failure message, not a doc rewrite.

[s-annam]

Nit — off by two on the day it lands.

git ls-files 'tests/fixtures/pdfs/**/*.pdf' returns 60, and npm run check:fixtures on this branch prints ✓ fixture PII: 60 PDFs and 16 ground-truth sidecars. The gate's own clean run agrees: 62 binary path(s) skipped = 60 PDFs + the 2 Poppins faces this same sentence names, so the docblock contradicts its own arithmetic.

The 58 is inherited from a stale figure in CLAUDE.md:95 (written at #829; two more fixtures landed in #842). Correcting the count here rather than dropping it, since the sentence is doing real work — it tells the next reader which live paths the skip list is actually protecting.

Suggested change

	* (~15k NULs each) and the 58 PDF fixtures under `tests/fixtures/pdfs/`; the
	* (~15k NULs each) and the 60 PDF fixtures under `tests/fixtures/pdfs/`; the

[s-annam]

Nit — git ls-files is the index, so a brand-new file is invisible to the fast layer.

An agent writes a new .ts with a NUL, Stop fires, verify:quick runs check:nul, and the file is untracked so it is not scanned. Caught later at pre-push (verify, after git add) and in CI, so nothing merges — but the inner loop is exactly where an agent-authored NUL appears, and it is the layer this PR deliberately wired the gate into.

Matching the issue's AC verbatim ("any git ls-files path"), so this is a scope observation, not a defect against spec. If you want it: select-tests.mjs:143 already does the untracked half with git ls-files --others --exclude-standard, and I checked that returns 0 paths in this repo — no node_modules noise to filter, unlike some repos where .git/info/exclude un-ignores it. Three lines beside the existing call.

[s-annam]

Nit — pre-existing house idiom, but this gate is the one where it stings.

fileURLToPath(import.meta.url) is realpath'd by Node for the ESM main; process.argv[1] is not. Reach the script through a symlinked path and the two differ, main() never runs, and the gate exits 0 having scanned nothing — a silent pass, which is the one failure mode a correctness gate cannot have. Verified:

$ ln -s /Users/annam/offlinecv2 repolink
$ node repolink/scripts/check-no-literal-nul.mjs
exit=0   # no output at all

Not introduced here — check-fixture-pii.mjs, check-known-failures.mjs and select-tests.mjs carry the identical guard and no-op the same way (I ran all three through the symlink). So this is arguably its own change across four files rather than yours to carry, and npm run check:nul from a real path is fine, which is every path CI and the hooks take. Flagging it because this gate's whole premise is failing loudly, and because a reader will copy this guard into the sixth one. realpathSync on both sides closes it.