Development record
2026-08-05 — bug-hunt loop round 1: iss-184 (critical, guard tokenizer heredoc misparse) fixed at root cause, in two passes after a pre-PR adversarial security review caught the first pass narrowing the hole rather than closing it. `internal/core/guard/tokenize.go`'s `<<` handling already special-cased the literal-digit arithmetic-shift form (`$((1<<20))` is not a heredoc), but `isDelimStart` accepts any identifier-shaped word, so `$((1<<shift))` — an identifier operand — still read as a heredoc delimiter, and `skipHeredocBodies` then silently consumed every remaining line of the command looking for a "shift" terminator line that never comes, dropping later commands (a force-push, an `rm -rf`, anything) from ever reaching command position — a silent allow, not the loud fail-open the hook's own doc comment promises for anything the tokenizer cannot answer confidently. The first pass made `skipHeredocBodies` report whether it actually found each pending heredoc's terminator, turning a miss into `ErrUnparsableCommand`. The security review then constructed the exploit this missed: an attacker supplying a later line that happens to equal the misread "delimiter" (e.g. appending a bare `shift`) still finds a match, so the swallow still succeeds silently — the fix only closed the *unterminated* half of the class, not the *coincidentally-terminated* half, which is the one an attacker actually controls. The real fix is classification, not detection-of-failure: a heredoc delimiter word immediately followed by a bare `(` or `)` with no separator is never a real heredoc — its body and terminator line have to come first, so nothing legitimate places a paren directly against the delimiter word, and `$((expr<<ident))` produces exactly that shape. `<<` in that position is now read as the arithmetic operator at tokenize time, same as the literal-digit case, so the guarded line is reached and matched normally regardless of what any later line contains. The unterminated-body-to-error fix from the first pass is kept as defense in depth for genuinely malformed heredocs (`cat <<EOF` with no closing `EOF` line), a distinct, narrower gap the same swallow also covered — an existing test that pinned that silent-swallow as intended behaviour is updated to assert the corrected `ErrUnparsableCommand` contract. Detectors: `TestArithmeticShiftByIdentifierIsNotAHeredoc`, `TestArithmeticShiftCoincidentalDelimiterStillBlocks` (the adversarial payload the security review demonstrated), and `TestTokenizeRejectsUnterminatedHeredoc` — each watched failing on pre-fix code for the claimed reason and passing after. Three more bugs surfaced by the same round's multi-angle sweep and independently verified with failing tests are captured but not fixed here — iss-185 (critical, scanner leaves a second back-to-back secret token undetected and unredacted, defeating history's fail-closed residual re-scan), iss-186 (minor, a failed source-unlink in a capture ledger transition can permanently strand an issue id across two status directories), iss-187 (minor, `rules.Merge` panics on a nil-Domains base — currently unreachable through any live caller, so a latent exported-API contract defect) — left open for a future round, highest severity first.
2026-08-05 — bug-hunt loop round 1: iss-184 (critical, guard tokenizer heredoc misparse) fixed at root cause, in two passes after a pre-PR adversarial security review caught the first pass narrowing the hole rather than closing it. internal/core/guard/tokenize.go's << handling already special-cased the literal-digit arithmetic-shift form ($((1<<20)) is not a heredoc), but isDelimStart accepts any identifier-shaped word, so $((1<<shift)) — an identifier operand — still read as a heredoc delimiter, and skipHeredocBodies then silently consumed every remaining line of the command looking for a "shift" terminator line that never comes, dropping later commands (a force-push, an rm -rf, anything) from ever reaching command position — a silent allow, not the loud fail-open the hook's own doc comment promises for anything the tokenizer cannot answer confidently. The first pass made skipHeredocBodies report whether it actually found each pending heredoc's terminator, turning a miss into ErrUnparsableCommand. The security review then constructed the exploit this missed: an attacker supplying a later line that happens to equal the misread "delimiter" (e.g. appending a bare shift) still finds a match, so the swallow still succeeds silently — the fix only closed the unterminated half of the class, not the coincidentally-terminated half, which is the one an attacker actually controls. The real fix is classification, not detection-of-failure: a heredoc delimiter word immediately followed by a bare ( or ) with no separator is never a real heredoc — its body and terminator line have to come first, so nothing legitimate places a paren directly against the delimiter word, and $((expr<<ident)) produces exactly that shape. << in that position is now read as the arithmetic operator at tokenize time, same as the literal-digit case, so the guarded line is reached and matched normally regardless of what any later line contains. The unterminated-body-to-error fix from the first pass is kept as defense in depth for genuinely malformed heredocs (cat <<EOF with no closing EOF line), a distinct, narrower gap the same swallow also covered — an existing test that pinned that silent-swallow as intended behaviour is updated to assert the corrected ErrUnparsableCommand contract. Detectors: TestArithmeticShiftByIdentifierIsNotAHeredoc, TestArithmeticShiftCoincidentalDelimiterStillBlocks (the adversarial payload the security review demonstrated), and TestTokenizeRejectsUnterminatedHeredoc — each watched failing on pre-fix code for the claimed reason and passing after. Three more bugs surfaced by the same round's multi-angle sweep and independently verified with failing tests are captured but not fixed here — iss-185 (critical, scanner leaves a second back-to-back secret token undetected and unredacted, defeating history's fail-closed residual re-scan), iss-186 (minor, a failed source-unlink in a capture ledger transition can permanently strand an issue id across two status directories), iss-187 (minor, rules.Merge panics on a nil-Domains base — currently unreachable through any live caller, so a latent exported-API contract defect) — left open for a future round, highest severity first.
Frontmatter
| id | dcn-2608050000000006 |
|---|---|
| type | decision |
| date | 2026-08-05 |
.abcd/work/decisions/dcn-2608050000000006-bug-hunt-loop-round-1-iss-184-critical.md
open on GitHub ↗ · commit history ↗
mentions4
- iss-184guard tokenizer heredoc misparse: an unquoted arithmetic left-shift with an identifier operand (e.g. `$((1<<shift))`) is misparsed as a here-document start by isDelimStart (tokenize.go:206, accepts any word starting with a letter/underscore) plus readHeredocDelim (tokenize.go:249, terminates the delimiter word on ')'). skipHeredocBodies (tokenize.go:264) then scans for a line equal to the bogus delimiter, finds none, and silently swallows every subsequent line of the command before it reaches command position. A dangerous command on a later line (git push --force, rm -rf, gh repo delete, etc.) is never seen by Registry.Check and the guard returns VerdictAllow with no error — a silent fail-open, not the loud could-not-answer path the shim reserves for unparsable input. Confirmed live via both CLI front doors (internal/surface/cli/guard.go: guard check and guard hook). The literal-digit form (1<<20) is unaffected; only identifier/variable operands trip it. Reproducing test: internal/core/guard/tokenize_test.go, TestArithmeticShiftByIdentifierIsNotAHeredoc.
- iss-185scanner leaves a second back-to-back secret token undetected and unredacted, and the fail-closed residual re-scan reports clean anyway. Fixed-length token patterns (e.g. github_pat_ at patterns.go:93) require a leading \b; when two same-family tokens are concatenated with no separator, the first regex match consumes bytes up through the second token's first byte, leaving word/word at the junction so the second token's \b can never match. ScanText (scanner.go:274) therefore returns only one finding. Redact's fingerprintSpan keeps 2 raw alnum bytes at the tail of the masked span (scanner.go:393), so the junction stays word/word after redaction too — the second token survives completely raw in the output. Because the boundary never recovers, history.Capture's stage-two residual check (history.go:159, blockingResidual) re-scans the redacted text, finds nothing, and writes the record to disk with a live secret still in it, directly violating history's documented fail-closed guarantee ('a stored record can never contain a live secret'). Distinct from iss-65 (Finding.MarshalJSON snippet cross-leak in serialized findings, already fixed) — this is a ScanText/Redact detection gap in the scan pass itself. Reproducing test: internal/adapter/scanner/, TestConcatenatedSecretsBothDetected.
- iss-186capture.commitTransition writes the destination file before removing the source (workflow.go:255-273); a non-ENOENT os.Remove(src) failure (EPERM/EROFS/EIO — e.g. a read-only remount or restrictive ACL on the source status dir) returns an error after dst already landed, with no rollback of dst. The same issue id is now present in two status directories (e.g. both open/ and resolved/). findIssue (alloc.go:383) then rejects any further transition on that id as ErrDuplicateIssueID, so it can never again be resolved or wontfixed on a healthy filesystem — no repair verb exists; recovery requires manually deleting one copy. scanLedger-based reads (List/Status) do not dedupe either, so the id also double-counts. Confirmed with a real non-ENOENT unlink failure (ext4 immutable attribute on the source dir, which blocks unlink even as root, unlike chmod). Reproducing test: internal/core/capture/workflow_test.go, TestTransitionRemoveFailureDoesNotStrandIssueInTwoDirs.
- iss-187rules.Merge panics with 'assignment to entry in nil map' (rules.go:193) when the base RuleSet's Domains field is nil and the overlay has at least one domain key. cloneRuleSet (rules.go:524) only allocates out.Domains when the source map is already non-nil, so a base like RuleSet{SchemaVersion: 1} (a Validate-accepted, valid zero-ish value) crashes on merge, contradicting Merge's own doc comment ('New domain keys are added') which promises no such precondition. The sibling loader guard.Merge (internal/core/guard/config.go:77-79) gets this right, explicitly allocating out.Entries when nil before the same kind of loop — Merge is simply missing that guard. Not reachable today: RuleSet.Load's only call site passes Merge(Defaults(), over), and Defaults() always has non-nil Domains, so this is a latent exported-API contract defect rather than a live crash — it becomes live the moment any caller merges onto a non-Defaults() base (e.g. a future multi-tier overlay starting from an empty RuleSet). Reproducing test (recover-guarded): internal/core/rules/, TestMergeNilBaseDomainsAddsNewKeys.