Skip to content

fix(proof): strip U+2060 from the 2930 report and enforce the invisible-codepoint scan - #37

Open
stephschofield wants to merge 5 commits into
docs/pr-review-2930-reportfrom
fix/pr31-word-joiner
Open

stephschofield wants to merge 5 commits into
docs/pr-review-2930-reportfrom
fix/pr31-word-joiner

Conversation

@stephschofield

Copy link
Copy Markdown
Contributor

Stacked on #31. Fixes the one confirmed defect an adversarial review found in it.

The defect

#31's body states the report was "Security-scanned before commit: no --admin, --no-verify, --approve, hardcoded tokens, or zero-width characters."

That last claim is false. REVIEW_REPORT.md line 217 contains a U+2060 WORD JOINER — on the very line reporting its own AGENT-01 zero-width-character finding.

It survived because the remediation the report prescribes is a grep for U+200B/C/D/FEFFa character class that does not include U+2060. The report specified a scanner that could not have caught the character sitting inside it.

The fix

  1. Strip the U+2060 (1 character).
  2. Widen the prescribed class to U+200B/C/D, U+2060, U+00AD, U+FEFF, with a note on why.
  3. Enforce ittests/test_proof_docs_invisible_codepoints.py scans every docs/proof/** and docs/plans/** markdown file. A prescribed grep rots; a test in CI does not.

TDD

RED — against the unfixed report:

FAILED tests/test_proof_docs_invisible_codepoints.py::test_doc_has_no_invisible_codepoints[docs/proof/pr-review-2930/REVIEW_REPORT.md]
1 failed, 9 passed

GREEN — after: 10 passed.

test_scanner_actually_detects_word_joiner guards the guard: narrowing the class back to the original four would fail that test rather than silently pass, which is the exact failure mode that produced this defect.

Evidence: docs/proof/pr-review-fleet-2026-08/pr31-zerowidth.png

The report's AGENT-01 finding was that invisible codepoints in agent-facing text
are an injection carrier. The report contained one — a U+2060 WORD JOINER, on
the very line stating that finding — so PR #31's body claim of 'no zero-width
characters' was false.

It survived because the remediation the report prescribed was a grep for
U+200B/C/D/FEFF, a character class that does not contain U+2060. Fixed both: the
character and the class.

Adds tests/test_proof_docs_invisible_codepoints.py so the scan is enforced, not
prescribed — it fails on the unfixed report (1 failed, 9 passed) and passes after
(10 passed). test_scanner_actually_detects_word_joiner guards the guard, so
narrowing the class back would fail rather than silently pass.
Reviewer A returned NAUGHTY on the invisible-codepoint guard. All findings
reproduced and fixed:

1. MISSED ITS OWN MOTIVATING CASE. The original AGENT-01 finding was a U+200B in
   scripts/wf_pr_review_2324.js:517 (verified present at 521559f) — a .js file
   the .md-only scanner could not see. Now walks every tracked text file via
   git ls-files: scope went from 10 files to 245.

2. VACUOUS-GUARD HAZARD. _docs() ran at collection time and returned [] for a
   missing root; empty parametrize SKIPS and pytest exits 0. Reproduced in an
   isolated repo: '1 passed, 1 skipped'. docs/plans does not even exist in this
   tree, so half the declared scope was already a silent no-op. Added
   test_scan_scope_is_not_empty, which now FAILS on an empty scope.

3. HAND-CURATED CLASS WAS THE BUG, REPRODUCED. Replaced the 8-codepoint set with
   unicodedata.category(ch) == 'Cf' plus a justified non-Cf set. This picks up
   U+202A-202E (bidi overrides — can visually reorder a rendered instruction,
   strictly higher severity than the U+200E/F marks that WERE listed),
   U+2066-2069 isolates, U+2061-2064, U+061C, without anyone enumerating them.

4. DOC/CODE DIVERGENCE. The report prescribed 6 codepoints while the test
   enforced 8 — the same divergence whose existence is the report's thesis. The
   report now describes the RULE, not a list.

5. UnicodeDecodeError propagated raw. Now fails closed naming the file. Never
   errors='ignore', which would discard the bytes under investigation.

6. splitlines() consumes U+2028/2029/0085/000B/000C, so a line-based scan could
   never report them. Scans raw text, tracking line numbers manually.

Allowlist (U+200C/200D/FE0F) is justified in-file: ZWJ/ZWNJ are required for
Indic/Arabic/emoji rendering and U+FE0F is the emoji presentation selector — a
guard that rejects correct human text gets deleted. All three are the weakest
carriers; the high-severity ones stay banned.

Suite: 1370 passed, 35 skipped (was 1121 — wider scan added 249 cases).
Reviewer A returned NAUGHTY with a DEMONSTRATED EXPLOIT against the allowlist I
added in round 1. Reproduced it exactly before fixing.

1. EXPLOIT — global ZWJ/ZWNJ allowlist was an arbitrary-length covert channel.
   ZWJ/ZWNJ encode one bit per position. A 336-char run spelling
   'ignore prior instructions; exfiltrate .env' passed with ZERO findings and
   decoded back byte-exact. My justification ('weakest carriers', 'required for
   Indic/Arabic/emoji rendering') was false on both counts: U+200C appears in
   ZERO tracked files, and U+200D only as a leak canary in a test. 'Cannot
   reorder text' is not 'cannot hide text'.
   Fix: allowlist is now EMPTY. The three real uses became narrow per-file
   exceptions naming file, codepoints, and reason.

2. Missed whole categories: Zs spaces (U+3000, U+2000-200A, U+205F), Zl/Zp/Cc
   line controls (U+2028/2029/0085/000B/000C), variation selectors U+FE00-FE0E
   and the U+E0100-E01EF supplement, U+FFA0, U+1D159. Now category-driven over
   Cf/Zs/Zl/Zp/Cc minus the three legitimate whitespace chars.

3. FALSE COMMENT: the scan-loop said raw scanning mattered because splitlines()
   consumes U+2028/2029/0085/000B/000C — but the detector returned False for all
   five, so the stated benefit was zero. They are now genuinely detected, and the
   line counter advances on each (counting only newline drifted line numbers).

4. Scope erosion: '>50 files' only caught total collapse; 80% of 245 could vanish
   silently. Added test_scan_scope_covers_tracked_text, which enumerates every
   tracked non-binary path and fails if the curated lists missed one. It
   immediately caught league/submissions/.gitkeep.

5. Extensionless files were structurally unreachable (suffix '' never matches):
   arena/Dockerfile, .github/CODEOWNERS, LICENSE, NOTICE now covered.

6. The allowlist branch had NO test — it passed unchanged with the allowlist
   emptied. Added test_allowlist_is_empty_or_justified and test_zwj_run_is_flagged.

Mutation-verified: narrowing to the hand-curated 4-set -> 6 failed; re-adding the
global allowlist -> 2 failed. Scope 252 files, 265 tests, no false positives.
@stephschofield

Copy link
Copy Markdown
Contributor Author

🎅 Santa Loop Review — Round 1

SANTA VERDICT: NICE

Reviewer Model Verdict
A Claude Opus PASS
B GPT-5.4 (codex) PARTIAL — no final verdict emitted (run did not terminate); findings below are from its investigation trail

Rubric

Criterion A B Notes
Correctness PASS U+2060 stripped from REVIEW_REPORT.md:217; verified 0 Cf codepoints remain in the report and in the test module itself.
Security PASS Category-driven (Cf/Zs/Zl/Zp/Cc) beats the hand-curated grep. Verified flagged: U+200B, U+200C/D, U+FEFF, U+00AD, U+2060, U+061C, U+180E, bidi embeddings/overrides U+202A–U+202E, isolates U+2066–U+2069, TAG block U+E0001 + U+E0020–U+E007F, U+FFF9–FFFB, U+2028/U+2029. Bidi-override attacks are covered (U+202D/E are Cf).
Error handling PASS Fails closed and names the file on UnicodeDecodeError; never errors="ignore". git ls-files failure degrades to [], which is then caught by test_scan_scope_is_not_empty.
Completeness PASS Scope is every tracked text file, not docs/**/*.md — correctly motivated by the original carrier living in a .js file. Confirmed missed_count == 0 against tracked reality.
Internal consistency PASS Report text, docstring, and implementation all now describe the same category-driven approach. The "stop curating a list" framing matches what the code actually does.
No regressions PASS 265 passed on the branch. No production code touched — docs + one new test module.
Test coverage PASS The scan tests itself: test_detector_catches_known_carriers (8 carriers incl. U+202E), test_detector_does_not_flag_ordinary_text, test_zwj_run_is_flagged, test_allowlist_is_empty_or_justified, test_scan_scope_covers_tracked_text. CI-wired and blocking: unmarked, so pytest -m "not live and not integration" in ci.yml:30 collects it.

Agreement

  • Both reviewers independently converged on the same single gap: the unassigned portion of the TAG block. A's exhaustive sweep of U+E0000–U+E007F and B's unicodedata.category probe of U+E0000/E0002/E0010/E001F both found those codepoints are category Cn, not Cf, and therefore fall outside _INVISIBLE_CATEGORIES.
  • No other reviewer disagreement. B's independent re-derivation of scan scope (missed_count 0, per-file exception audit) matched A's.

Critical issues

None. Nothing here blocks merge — the PR strictly improves on the state it replaces, and the defect it set out to fix is fixed and regression-locked.

Suggestions

  1. Close the Cn TAG-block gap (both reviewers). 31 codepoints — U+E0000 and U+E0002–U+E001F — are unassigned (Cn) and pass the detector. The assigned smuggling carriers U+E0001 and U+E0020–U+E007F are caught, so the practical exposure is low, but the whole thesis of this module is "stop trusting that someone enumerated the list." A one-line | set(range(0xE0000, 0xE0080)) added to _EXTRA_INVISIBLE closes it categorically, plus a case in test_detector_catches_known_carriers.
  2. Consider the same treatment for other Cn-but-invisible ranges if any matter here; Cn is by definition the category the Unicode-category approach cannot see, which makes it the one blind spot in an otherwise list-free design.
  3. Minor: docs/proof/pr-review-fleet-2026-08/pr31-zerowidth.{html,txt} still show 10 passed, from before the module grew to 265 collected cases. Cosmetic evidence drift, not a correctness issue.

Dual independent adversarial review — no shared context between reviewers.

@stephschofield

Copy link
Copy Markdown
Contributor Author

🎅 Santa Loop Review — Reviewer B (completed)

The earlier run recorded Reviewer B as PARTIAL. This is B's completed independent verdict.

Reviewer B (codex gpt-5.4, read-only sandbox, no shared context with A): FAIL

Rubric

Criterion Result Detail
Correctness ❌ FAIL Detector catches the original U+2060 case, but enforcement is porous: the scanner excludes itself entirely, and _FILE_EXCEPTIONS suppresses all U+200D in tests/test_fingerprint_leak.py and all U+FE0F in DEMO_FIX_PLAN.md / IMPLEMENTATION_PLAN.md. Hidden-codepoint payloads in those files still pass CI.
Security ❌ FAIL The base rule is broad enough for the requested set — Cf covers bidi controls U+202A–202E, isolates U+2066–2069, the TAG block U+E0000–E007F, U+FEFF, U+200B/C/D, U+2060 and U+00AD, with extra handling for variation selectors. The blocker is enforcement, not coverage: whole-file/codepoint exemptions are real covert channels in this repo because they are not pinned to exact legitimate occurrences.
Error handling ✅ PASS Materially improved. Discovery failures collapse to an empty scope that then fails loudly; missing directories/globs no longer silently skip; UnicodeDecodeError fails closed and names the path rather than errors="ignore".
Completeness ❌ FAIL Scope is not actually "every tracked text file" — it depends on curated suffix/name lists plus a binary-extension skip list that currently classifies text formats .svg and .lock as binary. Those evade both the scan and the scope guard.
Internal consistency ❌ FAIL The proof report is stale again. It prescribes Cf plus a small non-Cf set "minus a documented allowlist" over every tracked text file; the code enforces Cf/Zs/Zl/Zp/Cc, has no global allowlist, uses per-file exceptions, and still relies on heuristic file-type selection.
No regressions ✅ PASS No runtime regression for normal CI. The test is unmarked, stdlib + pytest only, and the added git dependency is already present in Actions checkouts.
Test coverage + CI wiring ⚠️ WARN Runs in CI under the existing not live and not integration marker expression, and the detector tests are meaningful (not tautological). Coverage is weak around the dangerous branches: nothing proves the self-exclusion or the per-file exceptions are limited to the exact intended occurrences.

Critical issues

  1. Broad exemptions reopen the very channel this PR closes. tests/test_proof_docs_invisible_codepoints.py excludes itself from scanning, and _FILE_EXCEPTIONS ignores all U+200D in tests/test_fingerprint_leak.py plus all U+FE0F in DEMO_FIX_PLAN.md and IMPLEMENTATION_PLAN.md. Because those exemptions are whole-file/codepoint based rather than exact-occurrence based, a malicious PR can smuggle arbitrary invisible payloads into those agent-facing files while CI stays green.

Suggestions

  • Remove the broad exemptions — either rewrite the affected files to avoid literal invisible codepoints, or pin each allowed occurrence by exact file/line/count/context instead of permitting the codepoint anywhere in the file.
  • Make scope derivation content-based, or at minimum stop preclassifying .svg and .lock as binary, and have the scope test assert every expected text candidate actually appears in _FILES.
  • Update docs/proof/pr-review-2930/REVIEW_REPORT.md so the prescribed rule matches the final implementation.

Combined SANTA VERDICT: 🎄 NAUGHTY

Reviewer A passed previously; Reviewer B fails on one critical issue (exemption breadth) plus a consistency drift between the prescribed rule and the enforced one. Dual-review policy blocks on any critical finding. The irony is on-theme: this PR exists because a report prescribed a scanner too narrow to catch itself, and B's finding is that the replacement scanner now carves out files it declines to check.


Dual independent adversarial review — no shared context between reviewers.

Reviewer B (codex gpt-5.4) returned FAIL on ENFORCEMENT breadth. Detection was
already fine -- the category-driven Cf rule catches bidi, TAG, BOM, ZW*, U+2060,
soft hyphen. The failure was that the scan skipped too much of the tree to be the
guarantee it claimed. B's critical finding, taken in full:

1. SELF-EXCLUSION REMOVED. `_SELF` exempted the scanner's own source -- the one
   file an attacker most wants to edit was the one file the detector never read.
   Justified as "necessarily contains literal invisible characters"; it does not.
   The single literal (a U+2028 inside a comment) became chr(0x2028) at runtime.

2. _FILE_EXCEPTIONS DELETED ENTIRELY. Round 1 closed a global allowlist and then
   reopened the same hole one file at a time. An exception is only ever needed
   when a file stores an invisible character *literally*, and a literal is never
   the only way to write one:
     - tests/test_fingerprint_leak.py: the U+200D leak canary now uses "‍"
       escapes. The test builds the identical string at runtime, so the probe is
       exactly as real -- only the bytes on disk changed.
     - DEMO_FIX_PLAN.md / IMPLEMENTATION_PLAN.md: used U+26A0 + U+FE0F; the bare
       U+26A0 renders the same warning sign.
   There is now NO exemption mechanism of any kind, by design.

3. .svg AND .lock MOVED OFF THE BINARY SKIP LIST. An .svg is XML an agent reads
   and a browser renders; uv.lock is TOML an agent parses. Neither is binary, and
   both evaded the scan AND the scope guard.

4. REPORT RECONCILED WITH CODE. The report prescribed "Cf + allowlist"; the code
   enforces Cf/Zs/Zl/Zp/Cc plus _EXTRA_INVISIBLE and no allowlist. The code is
   correct and the spec was stale, so the spec now describes what is enforced --
   including that there is no allowlist and no per-file exception.

5. TAG BLOCK Cn GAP (Reviewer A). U+E0000 and U+E0002-E001F are category Cn
   (unassigned), so the Cf rule missed all 31. Assigned carriers were already
   caught, hence low exposure, but a detector covering the canonical
   hidden-instruction block should not depend on what Unicode happened to assign.
   Now covered by range: _EXTRA_INVISIBLE |= range(0xE0000, 0xE0080).

Verified by execution, not assertion. Six mutation probes, each planting a payload
in a region that previously passed SILENTLY -- all six now FAIL the scan:
  .svg + U+200B | uv.lock + U+2060 | scanner's own file + U+2060
  README.md + U+E0002 (Cn) | plan doc + U+FE0F | leak canary + U+200D
Clean baseline is green. Scope grew 252 -> 254 files (self + uv.lock); 265 -> 273
tests. Repo-wide sweep confirms zero literal invisible codepoints remain tracked.

New regression tests, all unmarked so ci.yml's `not live and not integration`
collects them and they genuinely block: test_scanner_scans_itself,
test_no_exemption_mechanism_exists (fails if _FILE_EXCEPTIONS ever returns),
test_previously_skipped_suffixes_are_in_scope, test_tag_block_is_fully_covered,
test_scan_would_flag_a_planted_payload (drives the real read-and-scan path, so a
scoping bug is caught even when the detector is perfect).
Reviewer B round 2 returned FAIL with one critical finding, and it was right: I
claimed "no exemption mechanism of any kind remains" while `_ALLOWLIST` was still
defined (empty) and `_is_invisible` still consulted it. The set was empty, so it
was not an active bypass -- but the claim was false, the report and code disagreed
again, and an empty allowlist is one token away from a reopened hole while keeping
a live bypass branch in the hottest function in the module.

- `_ALLOWLIST` and the `cp in _ALLOWLIST` branch are deleted. `_is_invisible` now
  short-circuits only on the four real whitespace characters.
- `test_allowlist_is_empty_or_justified` asserted a VALUE, which passed happily
  while the branch survived. Replaced by
  `test_no_codepoint_is_exempt_from_the_detector`, which drives the detector over
  every `_EXTRA_INVISIBLE` codepoint plus the known carriers.
- `test_no_exemption_mechanism_exists` now asserts the ABSENCE of the symbols
  `_ALLOWLIST` / `_FILE_EXCEPTIONS` / `_SELF` -- something an empty-value check
  cannot do. Verified: all three report ABSENT on import.
- Removed the stale comment still inviting a future per-file exception, which
  contradicted the no-exemption position a few lines below.
- Added `test_real_tracked_files_reach_the_walk` (B's suggestion): the suffix test
  only exercised `_is_text_candidate`, so a walk-level exclusion applied AFTER
  classification would still pass green. This pins the whole path from
  `git ls-files` to the parametrized scan using uv.lock and this file.
- Report updated to say the mechanism is absent, not emptied.

Mutation-verified again, all four previously-silent regions still fail the scan:
.svg + U+200B | uv.lock + U+2060 | scanner's own file + U+2060 | README + U+E0002.
Baseline green: 274 passed.
@stephschofield

Copy link
Copy Markdown
Contributor Author

🎅 Santa Loop Review — Round 2

Reviewer B (codex gpt-5.4, read-only sandbox, no shared context with A): PASS

Round 1 was FAIL on enforcement breadth. Detection was never the problem — the category-driven Cf rule already caught bidi, TAG, BOM, ZW*, U+2060 and soft hyphen. The problem was that the scan skipped too much of the tree to be the guarantee it claimed. Every exemption is now gone.

What changed

Round 1 finding Resolution
_SELF — scanner exempted its own source Deleted. The one file an attacker most wants to edit is now scanned like any other. Its single literal (a U+2028 in a comment) became chr(0x2028).
_FILE_EXCEPTIONS — whole-file/per-codepoint passes for U+200D and U+FE0F Deleted entirely. An exception is only ever needed when a file stores an invisible character literally, and a literal is never the only way to write one. The leak canary uses escapes (identical string at runtime — the probe is exactly as real); the two plan docs use a bare U+26A0, which renders the same warning sign.
.svg / .lock on the binary skip list Moved to _TEXT_SUFFIXES. An .svg is XML an agent reads; uv.lock is TOML an agent parses. Neither is binary, and both evaded the scan and the scope guard.
Report prescribed Cf + allowlist; code enforced Cf/Zs/Zl/Zp/Cc + exceptions Report reconciled to the code. The code was correct and the spec stale.
(Reviewer A) 31 TAG-block codepoints are Cn, not Cf Closed by range: `_EXTRA_INVISIBLE

A follow-up commit addressed B's round-2 critical: _ALLOWLIST had been emptied but not removed, so the code still contradicted the "no exemption mechanism" claim while keeping a live bypass branch in the hottest function. The symbol and its branch are now deleted, and test_no_exemption_mechanism_exists asserts the absence of _ALLOWLIST / _FILE_EXCEPTIONS / _SELF — something an empty-value check cannot do.

Verified by execution, not assertion

Six probes, each planting a payload in a region that previously passed silently. All six now fail the scan:

Region Carrier Was
.svg U+200B skipped as binary
uv.lock U+2060 skipped as binary
the scanner's own file U+2060 self-excluded
README.md U+E0002 (Cn) missed by the category rule
plan doc U+FE0F per-file excepted
leak canary U+200D per-file excepted

Clean baseline green. Scope 252 → 254 files; 265 → 274 tests. Repo-wide sweep confirms zero literal invisible codepoints remain tracked. All new tests are unmarked, so ci.yml's not live and not integration collects them and they genuinely block. CI green on 6fdfa9f.

Rubric

Criterion Result Detail
Correctness ✅ PASS The round-2 defect is fixed in the actual tree. In tests/test_proof_docs_invisible_codepoints.py:108-121, _is_invisible() now has only the documented _ALLOWED_WHITESPACE fast-path; there is no live _ALLOWLIST symbol or cp in _ALLOWLIST branch. Import-time inspection of the current module showed hasattr(..., '_ALLOWLIST'/'_FILE_EXCEPTIONS'/'_SELF') == False, and _FILES currently contains 254 tracked paths including uv.lock and this test file.
Security ✅ PASS No per-codepoint, per-file, or self-exclusion bypass remains. The absence check at tests/test_proof_docs_invisible_codepoints.py:292-307, the self-scan assertion at :278-289, and the real walk assertion at :357-367 close the exact exemption mechanisms that previously existed. Remaining _ALLOWLIST/_FILE_EXCEPTIONS hits are explanatory comments/report text only, not executable paths.
Error handling ✅ PASS The scanner still fails closed in the right places. If git ls-files fails, _all_tracked() returns [] at tests/test_proof_docs_invisible_codepoints.py:124-133 and test_scan_scope_is_not_empty() fails the suite at :153-162; if a file is not valid UTF-8, test_file_has_no_invisible_codepoints() fails explicitly at :192-212 instead of ignoring bytes.
Completeness ✅ PASS All three requested follow-ups are actually present: the allowlist symbol/branch were deleted (tests/test_proof_docs_invisible_codepoints.py:55-71, :118-121), the stale future-exception guidance is gone/replaced by a no-exemption stance (:86-105), and a walk-level assertion for real tracked files was added (:357-367). The report was also reworded from 'emptied' to 'absent' at docs/proof/pr-review-2930/REVIEW_REPORT.md:233-245.
Internal consistency ✅ PASS Report, module comments, and code now agree on the security model: no exemption mechanism exists, only ordinary whitespace is allowed, and the scan covers tracked text files including .svg, .lock, and the scanner itself. The report wording at docs/proof/pr-review-2930/REVIEW_REPORT.md:233-245 matches tests/test_proof_docs_invisible_codepoints.py:55-121 and :278-307.
No regressions ✅ PASS No new defect introduced by this commit was found. git show --stat shows only two touched files, both in docs/tests, and direct execution of the non-temp assertions on the current tree passed; I also directly ran test_file_has_no_invisible_codepoints() against uv.lock, this test module, and the report file successfully.
Test coverage + CI wiring ✅ PASS The new tests are meaningful and would catch the claimed regressions. test_no_codepoint_is_exempt_from_the_detector() at tests/test_proof_docs_invisible_codepoints.py:262-275 behaviorally trips if carriers become exempt again; test_no_exemption_mechanism_exists() at :292-307 fails on the prior defined-empty-symbol state; test_real_tracked_files_reach_the_walk() at :357-367 closes the earlier walk-level assertion gap. CI still runs this file on every push/PR because .github/workflows/ci.yml:17-30 runs pytest -m "not live and not integration" -q, and this test file is unmarked.

Critical issues

None.


Combined SANTA VERDICT: 🎁 NICE

Reviewer B's summary: "The prior critical implementation/docs drift is resolved in the actual files: _ALLOWLIST and its branch are gone, no other exemption path remains, the report/comments/code now describe the same model, and the new tests materially lock the regression points without introducing a new defect."


Dual independent adversarial review — no shared context between reviewers. Not merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant