fix: seven priority issues, with the detectors the reviews demanded - #504
Merged
Conversation
hooks/bootstrap.sh's notice() and hooks.json's SessionStart chain still exit non-zero to surface a message, which is the mechanism iss-2608241115201044 found broken for the binary. Deferred rather than bundled: bootstrap.sh's exit code also governs the install path. Its own comment carries the reasoning that makes the binary fix correct — a SessionStart hook's stdout becomes model context — which an adversarial review then demonstrated concretely. Assisted-by: Claude:claude-opus-5[1m]
privacy-hygiene guarded on `if sc, err := scanner.New(...); err == nil`, expecting a fallback to the built-in patterns on error. scanner.New never returns a non-nil error: an unreadable, unparseable, symlinked or uncompilable .abcd/config/pii.json each yield a usable scanner marked unavailable. So the guard was always true, the fallback it appeared to protect was dead code, and a broken override silently dropped the repo's RAISED severities while the scan reported "conforms" — the didn't-scan-reported-clean shape this rule's own contract forbids, invisible to the one person who had raised a severity. The rule now consults sc.Unavailable() and emits an error Finding naming the config and the reason. An adversarial review then showed the contract was still bypassable one character away: BLANKING a new pattern's regex dropped the detector with no Unavailable() and no finding, and `abcd lint` reported conforms at exit 0 over a file that detector had been catching. mergeConfig now refuses an empty regex, bounded to NEW pattern names — bundled names never reach that loop, so raising a built-in pattern's severity, which legitimately carries no regex, is unaffected. Both directions are pinned, and the negative case is what caught an over-broad first attempt. The fixtures are worth a note: written first as a JSON array, they parsed as invalid JSON and passed for that reason rather than exercising the empty-regex path at all. `patterns` is a map. A test that passes for the wrong reason is the recurring defect of this whole sweep. Resolves: iss-203 Assisted-by: Claude:claude-opus-5[1m]
`IsNull` recognised `""`, `null` and `~` only, so `impact: NULL` — which every YAML parser reads as null — was a value. internal/core/lint carried a private copy of the same three, which is where it bit: capture read one spelling as null and the lint read it as a malformed impact, so a record could pass record-lint and then fail the command that acts on it. The set is now the YAML 1.2 core schema's exactly — `""`, `~`, `null`, `Null`, `NULL` — and deliberately NOT a case-insensitive compare. `EqualFold` would accept `nUlL`, which no YAML parser does, and abcd would then read records nobody else agrees with: a wrong answer that only surfaces once the file leaves this toolchain, which is harder to notice than the miss it replaces. The test asserts both halves. The lint's copy delegates rather than restating the set. An adversarial review enumerated all 18 call sites — spec ids, kinds, statuses, slugs, superseded_by, impact — and confirmed none can legally hold `NULL`, and that `frontmatter.Fields` returns the raw trimmed scalar with no unquoting, so the raw-scalar contract the doc now states actually holds at every caller but one: lifeboat's graveyard unquotes first, which is recorded rather than changed here. Resolves: iss-287 Assisted-by: Claude:claude-opus-5[1m]
Quoting is what separates a null from a string in YAML: bare `null` is a null, `"null"` is the four-character string. record-lint tests the RAW scalar and gets that right. capture parsed first, and parsing unquotes, so it got it wrong — and widening the null set for iss-287 made the split WIDER, not narrower, which is why these land together. A bare null spelling now normalises to `""` at parse time while a quoted one stays the string it spells, so every downstream nullness test runs on a value that already knows which it was. That avoids threading quotedness through validate and its callers. An adversarial review found the empty spelling still split, and the test that should have caught it passing for the wrong reason: `impact:` with no value took the nested-object branch and parsed to a `map[string]any`, which capture rejects as "must be a string" while record-lint reads the same raw scalar as null. The test's `got, _ := v.(string)` swallowed the non-string and yielded `""`, so it would have passed for any type. It now asserts the type first, and a lookahead parts the two cases: an object has an indented member, a bare `key:` does not. Also removed: a `quoted` map returned by the parser, populated nowhere and discarded by both callers, with a doc comment describing plumbing that did not exist. Dead scaffolding against "wired or it isn't done", and the justification for keeping it was false as well. Resolves: iss-285 Assisted-by: Claude:claude-opus-5[1m]
`abcd hook session-start` returned exit 2 whenever it had notices, on the
documented belief that a non-zero SessionStart puts a hook's stderr in front of
the human without blocking. The harness instead renders an opaque
"SessionStart:startup hook error" banner followed by a truncated echo of the
hooks.json command string, and DROPS the stderr text. Every notice the hook
exists to deliver — transcript-capture gaps, staged-drain failures, the backlog
count, binary skew — arrived as an error with no content, which is worse than
silence: it reports a fault in abcd rather than the condition it was reporting.
The exit is now 0. The notice TEXT goes to stderr. Only a CONSTANT and a count
go to stdout.
That split is the load-bearing part, and a first attempt got it wrong by putting
the text on stdout. SessionStart's stdout is injected into the session's context,
and the notices interpolate repo-derived strings: an adversarial review planted a
directive payload in `meta.setup_version` in `.abcd/config.json` — a TRACKED file
a pull request or fork can set — and watched it reach context verbatim.
termsafe.Sanitize defends a terminal, not a context window; it masks control
bytes and leaves prose untouched, and prose is the whole of an injection.
hooks/bootstrap.sh's own comment had already recorded the reasoning ("a
SessionStart hook's stdout becomes model context"), which I had not read before
routing notices there.
The constant names `abcd history staged` and `abcd ahoy` rather than `ahoy`
alone: ahoy renders install state and a gap COUNT and says nothing about a
staged transcript, so the one notice naming a privacy artefact was the one it
lost.
Five tests moved off the abandoned contract, and the stub binary with them — it
hard-coded `exit 2`, so the fixture modelled a contract the binary no longer
has. One assertion deliberately still expects exit 2: that is the BOOTSTRAP's
code, and the shell half of the same premise is out of scope here because
bootstrap.sh's exit code also governs the install path. Tracked as
iss-2608251011427187.
Resolves: iss-2608241115201044
Assisted-by: Claude:claude-opus-5[1m]
`preflight`'s prerequisite list is a derived value that three prose surfaces restate by hand, and nothing derived them from the recipe. It drifted twice in two releases, each time caught only by a host-run semantic reviewer refusing a release: `lint-issues` made it four gates and one file was updated, then `site-render` made it five and the same three files still said four. The test reads the prerequisites off the `preflight:` recipe and asserts every surface that enumerates them names the same set. It is the move TestInstallGuideDocumentsTheInstallAndUpdatePath already makes for the marketplace slug, applied to the gate list. It immediately caught two live instances in AGENTS.md, which said three gates in one passage and four in another — contradicting the recipe and itself. Both are corrected here; they were the detector's founding corpus. An adversarial review then found two holes in the detector itself: - A skip-when-absent sentinel made it defeatable by exactly the drift it targets. Rewriting a sentence to name two of five gates removes the sentinel along with the gates, and the subtest passed by skipping. The file list is hand-curated, so a surface that stops enumerating leaves the list in the same change instead. - The Makefile subtest read the whole file, including the recipe line the expected set is derived from, so containment held against its own input and it could not fail. It now reads only the comment block above the recipe. Both defeats are proven to fail now, and adding a sixth prerequisite is proven to fail all four enumerating surfaces. Resolves: iss-2608242043243131 Assisted-by: Claude:claude-opus-5[1m]
The verification matrix's surface-discipline row claimed "lint `SD001` blocks on forbidden sub-verb introduction". No SD001 rule exists: the naming chapter calls it a reserved code, `abcd lint` emits no such finding, and the only occurrences in the Go tree are two explanatory comments. The adjacent vocabulary row IS caveated as reserved-but-unimplemented; this one was not, so it read as a live gate — the phantom-gate shape enforcement-claims-are-facts exists to prevent, where a reader believing a check exists stops compensating for its absence. The row now carries the same caveat, and says what the previous wording hid: the shipped tree does not currently satisfy the criterion either. `abcd history show` and a plain `abcd history list` both ship, `abcd banlist list` renders byte-identically to bare `abcd banlist` — the redundancy the rule exists to forbid — and bare invocation renders state for most parents but not all, six of them printing usage instead. Every claim in the replacement was verified against a built binary by an adversarial review, including that the list of six is complete. Assisted-by: Claude:claude-opus-5[1m]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seven priority issues, each with a detector proven both ways, and two adversarial reviews that between them returned BLOCK and FIX FIRST.
The reviews were the valuable part. They found six defects in the fixes, and the worst of them was mine.
What landed
fix:pii.jsoninstead of scanning with silently weakened severitiesfix:lint's private duplicate collapsed into itfix:record-lintparts themtest:fix:docs:SD001is reserved, not armed, and the tree does not satisfy itrecord:The security review returned BLOCK, and it was right
I opened a prompt-injection channel while fixing a delivery bug. My first cut routed session-start notice text to stdout, which the harness injects into the model's context. The notices interpolate
meta.setup_versionfrom.abcd/config.json— a tracked file a pull request or fork can set. The reviewer planted a directive payload and watched it arrive in context verbatim.Its framing is the lesson: "Sanitising for a terminal is not the same as sanitising for a context window, and this line changed which of those two the string is destined for."
termsafe.Sanitizemasks control bytes and leaves prose alone — prose is the whole of an injection.hooks/bootstrap.sh's own comment already said so: "a SessionStart hook's stdout becomes model context". The hazard was documented in the file beside the one I was editing.Now: text to stderr, a constant to stdout, exit 0. The test asserts the negative — that repo-derived text does not reach stdout.
Second blocker: my iss-203 fix claimed more than it delivered. Blanking a custom detector's regex still dropped it silently — no
Unavailable(), no finding,abcd lintreporting conforms at exit 0 over a file it had been catching. One character from the case I did catch. Closed inmergeConfig, bounded to new pattern names so a bundled-severity raise is unaffected — and the negative case caught an over-broad first attempt.The ruthless review found the detectors were weaker than claimed
abcd ahoyrenders a gap count, not staged transcripts) — so the one notice naming a privacy artefact was the one it lost.parseFrontmatterBlockQuoted's second return was always nil, discarded by both callers, with a doc comment justifying it that was false.All fixed; every defeat is proven to fail now.
Three of my tests passed for the wrong reason
Worth naming, because it is the recurring defect of the sweep:
{"bare empty", "impact:", true}passed becausegot, _ := v.(string)swallows a non-string and yields"". Asserting the type first exposed a real split still open:impact:with no value parsed to a map, which capture rejects and record-lint reads as null. Fixed with a lookahead.patternsis a map, so they exercised "invalid JSON" rather than the empty-regex path they named.Deliberately not done
emit.goto testresolved_by.commitreachability, but six swept records carry no stamp, so the check changes what a release contains. Its own change, its own review.bootstrap.shexits 2 on success for the same disproven reason, but its exit code also governs the install path. Captured as iss-2608251011427187, and the test documents the scope boundary rather than hiding it.Gates
make preflightexit 0,gofmt -l .clean,record-lintanddocs-lint0 blockers,make lint-issues→RS001 checked iss-2608242043243131 iss-2608241115201044 iss-285 iss-287 iss-203 — OK.Assisted-by: Claude:claude-opus-5[1m]