From f873e85ddbf860e961091503f82d732a927bb90e Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:16:02 +0100 Subject: [PATCH 1/7] record: the shell half still signals SessionStart notices by exit code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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] --- ...p-sh-and-hooks-json-still-deliver-sessionstart.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 .abcd/work/issues/open/iss-2608251011427187-hooks-bootstrap-sh-and-hooks-json-still-deliver-sessionstart.md diff --git a/.abcd/work/issues/open/iss-2608251011427187-hooks-bootstrap-sh-and-hooks-json-still-deliver-sessionstart.md b/.abcd/work/issues/open/iss-2608251011427187-hooks-bootstrap-sh-and-hooks-json-still-deliver-sessionstart.md new file mode 100644 index 00000000..5f1e12bd --- /dev/null +++ b/.abcd/work/issues/open/iss-2608251011427187-hooks-bootstrap-sh-and-hooks-json-still-deliver-sessionstart.md @@ -0,0 +1,12 @@ +--- +schema_version: 1 +id: "iss-2608251011427187" +slug: "hooks-bootstrap-sh-and-hooks-json-still-deliver-sessionstart" +severity: "major" +category: "bug" +source: "agent-finding" +found_during: "adversarial review of the v0.6.6 priority sweep 2026-08-25" +found_at: "hooks/bootstrap.sh" +--- + +hooks/bootstrap.sh and hooks.json still deliver SessionStart notices by a non-zero exit, which the harness renders as an opaque banner with the text dropped. iss-2608241115201044 fixed the binary half: abcd hook session-start now exits 0, writes its notice text to stderr and only a CONSTANT to stdout. The shell half is unchanged and inconsistent with it. bootstrap.sh's notice() exits 2 on a REPORTED CONDITION including a successful install, and hooks.json's SessionStart chain propagates that ($s) plus a trailing exit 2 on the missing-binary branch. Under the same premise those messages are dropped too, so a fresh install's success line and the missing-binary complaint are both delivered by the mechanism the binary half abandoned. bootstrap.sh's own comment states the reasoning that makes the binary fix correct — 'a SessionStart hook's stdout becomes model context' — and an adversarial review demonstrated that concern concretely, planting a directive payload in a TRACKED config value that reached context when notices were briefly routed to stdout. So the shell must NOT simply move its text to stdout either. The open question is what channel a shell hook has left: a constant on stdout plus text on stderr mirrors the binary, but bootstrap.sh's exit code also governs the install path, and changing it touches the most safety-critical surface in the repo. Deliberately deferred rather than bundled into the binary fix. \ No newline at end of file From 96992e3fa2acf80bf845e758a582799f0305be2c Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:16:02 +0100 Subject: [PATCH 2/7] fix: report an unusable per-repo scanner config instead of absorbing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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] --- ...3-audit-privacy-degraded-scanner-silent.md | 14 +++ internal/adapter/scanner/scanner.go | 12 +- internal/core/repolint/rule_privacy.go | 37 +++++- .../repolint/rule_privacy_degraded_test.go | 105 ++++++++++++++++++ 4 files changed, 163 insertions(+), 5 deletions(-) create mode 100644 .abcd/work/issues/resolved/iss-203-audit-privacy-degraded-scanner-silent.md create mode 100644 internal/core/repolint/rule_privacy_degraded_test.go diff --git a/.abcd/work/issues/resolved/iss-203-audit-privacy-degraded-scanner-silent.md b/.abcd/work/issues/resolved/iss-203-audit-privacy-degraded-scanner-silent.md new file mode 100644 index 00000000..df857e66 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-203-audit-privacy-degraded-scanner-silent.md @@ -0,0 +1,14 @@ +--- +schema_version: 1 +id: "iss-203" +slug: "audit-privacy-degraded-scanner-silent" +severity: "major" +category: "bug" +source: "agent-finding" +found_during: "bug-hunt loop round 9 (state issue #197), contract-fidelity hunt angle + independent adversarial verification" +found_at: "internal/core/audit/rule_privacy.go:87 (err == nil guard), internal/adapter/scanner/scanner.go:92-124 (New, degradation paths)" +resolution: "privacy-hygiene reports an unusable per-repo scanner config instead of absorbing it. scanner.New returns a nil error on every degradation path, so the err == nil guard was always true and its documented fallback was dead; the rule now consults sc.Unavailable() and emits an error Finding naming the config and the reason. An adversarial review found the contract still bypassable one character away — blanking a new pattern's regex dropped the detector with no Unavailable() and no finding, and abcd lint then reported conforms at exit 0 over a file it had been catching — so mergeConfig refuses an empty regex on a new pattern name, bounded to new names so a bundled-pattern severity raise is unaffected. Both directions are pinned." +impact: fix +--- + +audit's privacy-hygiene rule guards on 'if sc, err := scanner.New(ctx.RepoRoot); err == nil' expecting a fallback to the built-in pattern set on error, but scanner.New never returns a non-nil error — every degradation path (unreadable/unparseable/uncompilable pii.json override) returns unavailable=true with a nil error instead — so the guard is always true, the documented fallback branch is dead code, and the rule never calls sc.Unavailable(); a broken per-repo severity override (malformed JSON, a directory at the config path, or an uncompilable override regex) silently drops the repo's raised severities and downgrades the audit exit code from 2 to 1 with zero finding or diagnostic explaining why, unlike history.Capture and the CLI identity path which both check Unavailable() explicitly on the same scanner. Independently verified via two-fixture test (valid override raises severity to error/exit-2; broken override silently drops to warn/exit-1 with no diagnostic) for all three degradation branches (malformed JSON, directory at path, uncompilable regex) \ No newline at end of file diff --git a/internal/adapter/scanner/scanner.go b/internal/adapter/scanner/scanner.go index d0f34f34..13c62856 100644 --- a/internal/adapter/scanner/scanner.go +++ b/internal/adapter/scanner/scanner.go @@ -226,7 +226,17 @@ func (s *Scanner) mergeConfig(cfg Config) error { } def := cfg.Patterns[name] if def.Regex == "" { - continue + // A NEW pattern with no regex detects nothing, and skipping it + // silently is the one weakening a broken config can still achieve + // without being reported: an adversarial review demonstrated a + // pull request blanking a repo's custom detector's regex, after + // which `abcd lint` reported "conforms" at exit 0 over a file the + // detector had been catching. Bundled names never reach this loop + // (the floors check above continues past them), so refusing here + // cannot affect a config that only adjusts a built-in pattern's + // label or severity — it binds exactly the case where an empty + // regex is meaningless (iss-203's contract, closed here). + return errUnreadable("pattern " + name + " has no regex") } expr := def.Regex if def.CaseInsensitive { diff --git a/internal/core/repolint/rule_privacy.go b/internal/core/repolint/rule_privacy.go index 8adda693..b6e95403 100644 --- a/internal/core/repolint/rule_privacy.go +++ b/internal/core/repolint/rule_privacy.go @@ -40,6 +40,12 @@ type privacyHygiene struct{} // docs-lint HTML-comment form it works in source files too, where `` is // not valid. It is the spelling the rule's own Fix hint teaches, so the corpus // converges on one token. +// privacyConfigRel is the per-repo scanner override this rule reports on when it +// cannot be used. It mirrors scanner's own unexported repoConfigRelPath; the +// scanner does not export it, and duplicating one slash-joined literal is +// cheaper than widening that package's surface for a message string. +const privacyConfigRel = ".abcd/config/pii.json" + const lintWaiver = "abcd-lint:allow" // auditWaiver is the pre-spc-29 spelling, honoured forever: the token lives in @@ -95,14 +101,37 @@ func (privacyHygiene) Eval(ctx Context) ([]Finding, error) { // The canonical network-identifier set AS THIS REPO CONFIGURES IT: the // scanner's merged patterns, so a severity a repo raised in // .abcd/config/pii.json is honoured here exactly as it is in Stage-1 - // redaction. A scanner that cannot be built falls back to the built-in set, - // which detects the same things at their default severities. + // redaction. + // + // The degraded case is REPORTED, not silently absorbed (iss-203). scanner.New + // returns a nil error on every degradation path — an unreadable, unparseable + // or uncompilable pii.json all yield a usable scanner marked unavailable — so + // an `err == nil` guard here is always true and the fallback branch it reads + // as guarding is dead code. What actually happens on a broken override is + // that the merge fails, the scanner keeps the built-in defaults, and the + // repo's RAISED severities vanish. Reporting that is the whole point: a + // weakened privacy scan that says "conforms" is the didn't-scan-reported-clean + // shape this rule's contract forbids, and the operator who raised a severity + // is the one person who would never learn it stopped applying. + var out []Finding patterns := scanner.NetworkPatterns() - if sc, err := scanner.New(ctx.RepoRoot); err == nil { + sc, err := scanner.New(ctx.RepoRoot) + if err != nil { + return nil, err + } + if degraded, reason := sc.Unavailable(); degraded { + out = append(out, Finding{ + RuleID: "privacy-hygiene", + Severity: SeverityError, + File: privacyConfigRel, + Message: "per-repo scanner config is unusable, so this scan runs with the " + + "built-in severities and any raised in it do not apply: " + reason, + Fix: "repair or remove " + privacyConfigRel, + }) + } else { patterns = sc.NetworkPatterns() } - var out []Finding for _, rel := range tracked { data, ok, oversizeText := readTrackedFile(root, filepath.FromSlash(rel)) if !ok { diff --git a/internal/core/repolint/rule_privacy_degraded_test.go b/internal/core/repolint/rule_privacy_degraded_test.go new file mode 100644 index 00000000..f74b6953 --- /dev/null +++ b/internal/core/repolint/rule_privacy_degraded_test.go @@ -0,0 +1,105 @@ +package repolint_test + +import ( + "strings" + "testing" + + "github.com/intentdriven/abcd/internal/core/repolint" +) + +// A broken per-repo scanner override must be REPORTED by privacy-hygiene, not +// absorbed (iss-203). +// +// The bug this pins was a guard that could not fail. The rule read +// +// if sc, err := scanner.New(ctx.RepoRoot); err == nil { patterns = sc.NetworkPatterns() } +// +// on the stated assumption that a scanner which "cannot be built falls back to +// the built-in set". scanner.New returns a nil error on EVERY degradation path — +// an unreadable, unparseable or uncompilable pii.json each yield a usable +// scanner marked unavailable — so the guard was always true and the fallback it +// appeared to protect was dead code. +// +// What actually happened on a broken override: the merge failed, the scanner +// kept the built-in defaults, and any severity the repo had RAISED silently +// stopped applying. The scan then reported "conforms" over a weaker pattern set +// than the repo asked for, which is the didn't-scan-reported-clean shape this +// rule's own contract forbids — and the operator who raised the severity is the +// one person who would never find out it had lapsed. +// +// Both halves are asserted, because only the pair distinguishes a real report +// from a rule that flags everything: a broken override is a finding that cites +// the config and carries the reason, and a valid one is silent. +func TestPrivacyReportsAnUnusableScannerConfig(t *testing.T) { + for _, c := range []struct { + name string + body string + }{ + {"unparseable json", "{ this is not json"}, + {"uncompilable override regex", `{"patterns":{"bad":{"regex":"([unclosed","severity":"hard_fail"}}}`}, + // The case an adversarial review demonstrated escaping this rule: a pull + // request that BLANKS a custom detector's regex removed it entirely, and + // the scan then reported "conforms" at exit 0 over a file the detector had + // been catching. An empty regex on a new pattern name detects nothing, so + // it is a config fault rather than an omission. + {"blanked override regex", `{"patterns":{"corp_host":{"regex":"","severity":"hard_fail"}}}`}, + // A NEW pattern name with no regex at all: same defect, written as an + // omission rather than a blanking. It detects nothing, so it is a config + // fault, and the merge must fail closed rather than skip it. + {"new pattern with no regex", `{"patterns":{"mine":{"severity":"hard_fail"}}}`}, + } { + t.Run(c.name, func(t *testing.T) { + b := newFixtureRepo(t).conforming(). + file(".abcd/config/pii.json", c.body). + commit() + res := b.run() + + f := findingFor(res, "privacy-hygiene") + if f == nil { + t.Fatalf("a %s override produced no privacy-hygiene finding; the scan ran with "+ + "silently weakened severities and said nothing", c.name) + } + if f.Severity != repolint.SeverityError { + t.Errorf("severity = %q, want %q — a weakened privacy scan is not advisory", + f.Severity, repolint.SeverityError) + } + if f.File != ".abcd/config/pii.json" { + t.Errorf("citation = %q, want the config that caused it", f.File) + } + // The reason is what makes the finding actionable: "unusable" alone + // sends the reader looking, the reason tells them what to repair. + if strings.TrimSpace(f.Message) == "" || !strings.Contains(f.Message, ":") { + t.Errorf("message = %q, want the scanner's reason appended", f.Message) + } + }) + } +} + +// The negative half. A repo with a VALID override must produce no degradation +// finding — otherwise the check above would pass on a rule that simply always +// fires, and the corpus would prove nothing. +// +// The second case is the load-bearing one for the empty-regex refusal. Raising a +// BUNDLED pattern's severity legitimately carries no regex, because the regex is +// the built-in one — so the refusal must bind new names only. Without this case +// the fix could be over-broad and nothing would say so. +func TestPrivacyIsSilentOnAUsableScannerConfig(t *testing.T) { + for _, c := range []struct{ name, body string }{ + {"skip list only", `{"skip_extensions":[".png"]}`}, + {"bundled pattern severity raise, no regex", `{"patterns":{"net_ipv4":{"severity":"hard_fail"}}}`}, + } { + t.Run(c.name, func(t *testing.T) { + b := newFixtureRepo(t).conforming(). + file(".abcd/config/pii.json", c.body). + commit() + res := b.run() + + if f := findingFor(res, "privacy-hygiene"); f != nil { + t.Fatalf("a valid override was reported as unusable: %+v", f) + } + if res.ExitCode != 0 { + t.Errorf("exit = %d, want 0", res.ExitCode) + } + }) + } +} From 96d97ab409fbc7d02ad41a46d12087a634a43297 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:16:24 +0100 Subject: [PATCH 3/7] fix: one null predicate, and it is the YAML core set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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] --- ...pellings-unrecognised-by-the-null-predi.md | 24 ++++++++ internal/core/frontmatter/frontmatter.go | 25 +++++++- internal/core/frontmatter/isnull_test.go | 58 +++++++++++++++++++ internal/core/lint/lint.go | 9 ++- 4 files changed, 111 insertions(+), 5 deletions(-) create mode 100644 .abcd/work/issues/resolved/iss-287-uppercase-yaml-null-spellings-unrecognised-by-the-null-predi.md create mode 100644 internal/core/frontmatter/isnull_test.go diff --git a/.abcd/work/issues/resolved/iss-287-uppercase-yaml-null-spellings-unrecognised-by-the-null-predi.md b/.abcd/work/issues/resolved/iss-287-uppercase-yaml-null-spellings-unrecognised-by-the-null-predi.md new file mode 100644 index 00000000..65cf2baa --- /dev/null +++ b/.abcd/work/issues/resolved/iss-287-uppercase-yaml-null-spellings-unrecognised-by-the-null-predi.md @@ -0,0 +1,24 @@ +--- +schema_version: 1 +id: "iss-287" +slug: "uppercase-yaml-null-spellings-unrecognised-by-the-null-predi" +severity: "major" +category: "bug" +source: "user-observation" +found_during: "pr-294-review" +found_at: "internal/core/frontmatter/frontmatter.go" +resolution: "frontmatter.IsNull is the YAML 1.2 core null set exactly — empty, ~, null, Null, NULL — and deliberately not a case-insensitive compare, because YAML does not accept nUlL and reading records no parser agrees with is worse than the miss. internal/core/lint's private duplicate, which recognised only the lower-case spelling, now delegates to it: that duplicate was the split where impact: NULL was null to capture and a malformed impact to the lint." +impact: fix +--- + +Uppercase YAML null spellings unrecognised by the null predicates (reported as GitHub #290) + +Retroactive capture of the bug reported as GitHub #290 so the ledger holds +the handle the CHANGELOG entry should cite: `frontmatter.IsNull` and lint's +`isNull` recognised `null` and `~` but not the YAML 1.2 core-schema +spellings `Null`/`NULL`, so records using an uppercase null were misread +(e.g. a superseded ADR's `superseded_by:` treated as a live handle). Fixed +in flight by PR #294, which widens both predicates with regression tests; +resolve this issue when that PR merges, citing its commit. Evidence: +`.abcd/work/reviews/2026-08-19-pr-294-null-predicate/` (F10). +Forge mirror: #374 diff --git a/internal/core/frontmatter/frontmatter.go b/internal/core/frontmatter/frontmatter.go index 3a03943b..df8fe109 100644 --- a/internal/core/frontmatter/frontmatter.go +++ b/internal/core/frontmatter/frontmatter.go @@ -76,7 +76,28 @@ func TrimBOM(s string) string { return strings.TrimPrefix(s, utf8BOM) } -// IsNull treats an empty value and the YAML nulls ""/"null"/"~" as null. +// IsNull reports whether a frontmatter scalar is a YAML null. +// +// The set is the YAML 1.2 core schema's, exactly: the empty value, "~", and the +// three spellings "null", "Null" and "NULL". It is deliberately NOT a +// case-insensitive compare — YAML does not accept "nUlL", and an EqualFold here +// would make abcd read records no YAML parser would agree with, which is worse +// than the miss it fixes (iss-287, reported as GitHub #290). +// +// This is the ONE null predicate. internal/core/lint held a private copy that +// recognised only the lower-case spelling, so `impact: NULL` read as null in one +// gate and as a malformed impact in the other — the split-verdict shape that +// makes a record pass a lint and then fail the command that acts on it. Callers +// come here rather than re-deriving it. +// +// The value must be the RAW scalar, before unquoting. Quoting is what separates +// a null from a string in YAML: bare null is a null, and "null" with its quotes +// is the three-character string. A caller that unquotes first destroys that +// distinction and cannot get it back. func IsNull(v string) bool { - return v == "" || v == "null" || v == "~" + switch v { + case "", "~", "null", "Null", "NULL": + return true + } + return false } diff --git a/internal/core/frontmatter/isnull_test.go b/internal/core/frontmatter/isnull_test.go new file mode 100644 index 00000000..d85345db --- /dev/null +++ b/internal/core/frontmatter/isnull_test.go @@ -0,0 +1,58 @@ +package frontmatter_test + +import ( + "testing" + + "github.com/intentdriven/abcd/internal/core/frontmatter" +) + +// IsNull is the YAML 1.2 core schema's null set and nothing wider (iss-287, +// reported as GitHub #290). +// +// The bug was a predicate that recognised only the lower-case spelling, so a +// record written `impact: NULL` — which every YAML parser reads as null — was +// null to one gate and a malformed impact to another. That split verdict is the +// shape where a record passes `record-lint` and then fails the command that acts +// on it, which is the worst kind: the author is told the record is fine by the +// tool whose job is to say so. +// +// The over-correction is asserted too. A case-insensitive compare would accept +// "nUlL", which no YAML parser does, and abcd would then read records nobody +// else agrees with — a wrong answer that is harder to notice than the miss it +// replaces, because it only shows up when the file leaves this toolchain. +func TestIsNullMatchesTheYAMLCoreSchema(t *testing.T) { + for _, v := range []string{"", "~", "null", "Null", "NULL"} { + t.Run("null/"+v, func(t *testing.T) { + if !frontmatter.IsNull(v) { + t.Errorf("IsNull(%q) = false, want true — YAML reads it as null", v) + } + }) + } + + for _, v := range []string{ + "nUlL", "NuLL", "nULL", // case YAML does not accept + "None", "nil", "NIL", "Nothing", // other languages' spellings + `"null"`, `"NULL"`, `'null'`, // quoted: a string, not a null + "nulls", "null ", " null", "annul", // near misses + "fix", "internal", "additive", // real impact values + } { + t.Run("not-null/"+v, func(t *testing.T) { + if frontmatter.IsNull(v) { + t.Errorf("IsNull(%q) = true, want false", v) + } + }) + } +} + +// The quoting distinction, stated as its own case because it is the half that +// iss-285 turns on: quoting is what separates a null from a string in YAML, so +// the predicate must be given the RAW scalar. A caller that unquotes first has +// already destroyed the distinction and cannot recover it here. +func TestIsNullRequiresTheRawScalar(t *testing.T) { + if !frontmatter.IsNull("null") { + t.Fatal(`IsNull("null") must be true: a bare null is a null`) + } + if frontmatter.IsNull(`"null"`) { + t.Fatal(`IsNull("\"null\"") must be false: quotes make it the three-character string`) + } +} diff --git a/internal/core/lint/lint.go b/internal/core/lint/lint.go index beadc1a9..bb8e6f76 100644 --- a/internal/core/lint/lint.go +++ b/internal/core/lint/lint.go @@ -2486,9 +2486,12 @@ func contentExempt(rel string, fields map[string]fmField, cfg Config) bool { return false } -func isNull(v string) bool { - return v == "" || v == "null" || v == "~" -} +// isNull delegates to the one null predicate. It stays as a name here only +// because the call sites read better for it; the SET of null spellings lives in +// internal/core/frontmatter and is not restated (iss-287). The private copy this +// replaces recognised only the lower-case spelling, so `impact: NULL` was null +// to capture and a malformed impact to this lint. +func isNull(v string) bool { return frontmatter.IsNull(v) } // fenceMask marks lines that are inside (or are a marker for) a triple-backtick // fenced code block. From b7415bcec58ea2efcac647e0770b42b484791718 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:16:24 +0100 Subject: [PATCH 4/7] fix: part bare from quoted YAML nulls the way record-lint does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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] --- ...it-capture-and-record-lint-verdicts-cap.md | 29 ++++++++++ internal/core/capture/nulls_internal_test.go | 56 ++++++++++++++++++ internal/core/capture/parse.go | 58 +++++++++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 .abcd/work/issues/resolved/iss-285-quoted-yaml-nulls-split-capture-and-record-lint-verdicts-cap.md create mode 100644 internal/core/capture/nulls_internal_test.go diff --git a/.abcd/work/issues/resolved/iss-285-quoted-yaml-nulls-split-capture-and-record-lint-verdicts-cap.md b/.abcd/work/issues/resolved/iss-285-quoted-yaml-nulls-split-capture-and-record-lint-verdicts-cap.md new file mode 100644 index 00000000..8ac196c9 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-285-quoted-yaml-nulls-split-capture-and-record-lint-verdicts-cap.md @@ -0,0 +1,29 @@ +--- +schema_version: 1 +id: "iss-285" +slug: "quoted-yaml-nulls-split-capture-and-record-lint-verdicts-cap" +severity: "major" +category: "bug" +source: "impl-review" +found_during: "pr-294-review" +found_at: "internal/core/capture/parse.go" +resolution: "capture and record-lint now part bare from quoted YAML nulls identically. A bare null spelling normalises to the empty string at parse time while a quoted one stays the string it spells, so the nullness decision is made while the RAW scalar is still in hand — record-lint tests the raw scalar, and capture used to test the unquoted one. Widening IsNull for iss-287 made that split wider rather than narrower, which is why the two land together. An adversarial review found the empty spelling still split: impact: with no value took the nested-object branch and parsed to a map, which capture rejected as must be a string while record-lint read it as null. A lookahead now parts them — an object has an indented member, a bare key: does not." +impact: fix +--- + +Quoted YAML nulls split capture and record-lint verdicts: capture unquotes before IsNull, record-lint reads the raw value + +An issue record carrying `impact: "NULL"`: capture's `decodeScalar` +(`internal/core/capture/parse.go:188`) strips the double quotes before +`validateStrict` calls `frontmatter.IsNull`, so `abcd capture resolve` +accepts the record — while record-lint's `checkIssueImpact` reads the raw +value with quotes intact, `ParseImpact` fails, and the `issue_impact_valid` +blocker fires on the record capture just accepted. The comment at +`internal/core/lint/lint.go:2397` claims this split is impossible. PR #294 +widened the disagreement window from one spelling (`"null"`) to three; its +own tests pin both sides of the contradiction +(`internal/core/frontmatter/frontmatter_test.go:76` vs +`internal/core/lifeboat/graveyard_abandoned_test.go:167`). The fix is to +normalise quoting on one side, not to widen literal sets. Evidence: +`.abcd/work/reviews/2026-08-19-pr-294-null-predicate/` (F1, F2). +Forge mirror: #372 diff --git a/internal/core/capture/nulls_internal_test.go b/internal/core/capture/nulls_internal_test.go new file mode 100644 index 00000000..d55c5d9c --- /dev/null +++ b/internal/core/capture/nulls_internal_test.go @@ -0,0 +1,56 @@ +package capture + +import "testing" + +// capture and record-lint must reach the SAME verdict on one record's impact, +// whatever spelling and quoting it uses (iss-285, coupled to iss-287). +// +// record-lint tests the RAW scalar: a bare null is a null, and a quoted one is +// the string it spells. capture parses first, and parsing unquotes — so without +// normalisation the two gates disagree, and a record passes the lint and then +// fails the command that acts on it. Widening the null set (iss-287) makes that +// split wider, not narrower, which is why both fixes land together. +func TestBareAndQuotedNullsPartTheSameWay(t *testing.T) { + for _, c := range []struct { + name string + line string + wantNul bool + }{ + {"bare lower", "impact: null", true}, + {"bare title", "impact: Null", true}, + {"bare upper", "impact: NULL", true}, + {"bare tilde", "impact: ~", true}, + {"bare empty", "impact:", true}, + {"quoted lower", `impact: "null"`, false}, + {"quoted upper", `impact: "NULL"`, false}, + {"quoted tilde", `impact: "~"`, false}, + {"a real impact", "impact: fix", false}, + } { + t.Run(c.name, func(t *testing.T) { + fm, err := parseFrontmatterBlock([]string{c.line}) + if err != nil { + t.Fatalf("parse %q: %v", c.line, err) + } + // Assert the TYPE before the value. `got, _ := v.(string)` yields "" + // for any non-string, so without this the empty case passed for the + // wrong reason — `impact:` with no value parses to a map, not a + // string, and the assertion silently swallowed it. + raw, present := fm["impact"] + if !present { + t.Fatalf("%q produced no impact key at all", c.line) + } + got, isStr := raw.(string) + if !isStr { + t.Fatalf("%q parsed impact to %T (%v), want a string — a non-string here "+ + "is rejected by capture and read as null by record-lint, which is the "+ + "split this test exists to close", c.line, raw, raw) + } + isNull := got == "" + if isNull != c.wantNul { + t.Errorf("%q parsed to %q (null=%v), want null=%v — capture must part "+ + "bare from quoted exactly where record-lint does", + c.line, got, isNull, c.wantNul) + } + }) + } +} diff --git a/internal/core/capture/parse.go b/internal/core/capture/parse.go index 9089e9d5..845b4457 100644 --- a/internal/core/capture/parse.go +++ b/internal/core/capture/parse.go @@ -2,6 +2,7 @@ package capture import ( "fmt" + "github.com/intentdriven/abcd/internal/core/frontmatter" "strconv" "strings" ) @@ -52,6 +53,34 @@ func parseFrontmatterAndBody(text string) (map[string]any, string, error) { } // parseFrontmatterBlock parses the interior lines of a frontmatter block. +// nextLineIsIndented reports whether the line after i begins an indented block, +// skipping blank lines. It is the lookahead that separates `key:` as a null from +// `key:` as the head of a nested object. +func nextLineIsIndented(lines []string, i int) bool { + for j := i + 1; j < len(lines); j++ { + l := strings.TrimRight(lines[j], "\r\n") + if strings.TrimSpace(l) == "" { + continue + } + return strings.HasPrefix(l, " ") || strings.HasPrefix(l, "\t") + } + return false +} + +// quotedScalar reports whether a raw scalar token is double-quoted, which in +// YAML makes it a string whatever it spells. +func quotedScalar(rest string) bool { + t := strings.TrimSpace(rest) + return len(t) >= 2 && strings.HasPrefix(t, `"`) && strings.HasSuffix(t, `"`) +} + +// parseFrontmatterBlock parses the interior lines of a frontmatter block, +// deciding null-vs-string while the RAW scalar is still in hand. +// +// An earlier draft split this in two and threaded a set of quoted keys out to the +// validator. The decision is made inline here, so that set was never populated and +// both callers discarded it — dead scaffolding, and the comment justifying it was +// false as well. One function, no reserved return. func parseFrontmatterBlock(lines []string) (map[string]any, error) { fm := map[string]any{} i := 0 @@ -80,6 +109,17 @@ func parseFrontmatterBlock(lines []string) (map[string]any, error) { if _, dup := fm[key]; dup { return nil, fmt.Errorf("%w: duplicate key %q", ErrMalformedFrontmatter, key) } + // `key:` with no value and NO indented line under it is a bare YAML null, + // not an empty object (iss-285). Taking the nested branch there made the + // value a map, which capture rejects as "must be a string" while + // record-lint reads the same raw scalar as null — the split verdict this + // normalisation exists to close, left open for the one spelling nobody + // tested. The lookahead is what parts them: an object has a member. + if rest == "" && !nextLineIsIndented(lines, i) { + fm[key] = "" + i++ + continue + } if rest == "" { // Nested one-level object: consume following indented lines. sub := map[string]any{} @@ -116,6 +156,24 @@ func parseFrontmatterBlock(lines []string) (map[string]any, error) { if err != nil { return nil, err } + // Normalise a BARE YAML null to the empty string, and leave a quoted one + // as the string it is (iss-285). + // + // parseScalarOrList unquotes, which destroys the only thing separating + // `impact: null` from `impact: "null"` — and record-lint tests the RAW + // scalar, so the two gates reached opposite verdicts on one record: the + // lint saw a string and refused, capture saw a null and passed. That is + // the shape where a record passes the lint and then fails the command + // that acts on it. Widening IsNull to the whole YAML null set (iss-287) + // makes the split worse rather than better, which is why the two fixes + // land together. + // + // Collapsing to "" here means every downstream nullness test is a test on + // a value that already knows which it was, without threading quotedness + // through validate and its callers. + if str, isStr := val.(string); isStr && !quotedScalar(rest) && frontmatter.IsNull(str) { + val = "" + } fm[key] = val i++ } From 64099ca329644ace73d88bc3421cfb849679e111 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:16:49 +0100 Subject: [PATCH 5/7] fix: a session-start notice is not a hook failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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] --- ...4-sessionstart-exit2-advisory-invisible.md | 14 +++++++ internal/surface/cli/cli.go | 42 ++++++++++++++++++- .../surface/cli/hook_session_start_test.go | 33 ++++++++++++--- .../surface/cli/hooks_sessionstart_test.go | 33 +++++++++++---- internal/surface/cli/skew_test.go | 13 +++--- 5 files changed, 117 insertions(+), 18 deletions(-) create mode 100644 .abcd/work/issues/resolved/iss-2608241115201044-sessionstart-exit2-advisory-invisible.md diff --git a/.abcd/work/issues/resolved/iss-2608241115201044-sessionstart-exit2-advisory-invisible.md b/.abcd/work/issues/resolved/iss-2608241115201044-sessionstart-exit2-advisory-invisible.md new file mode 100644 index 00000000..219e74f2 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2608241115201044-sessionstart-exit2-advisory-invisible.md @@ -0,0 +1,14 @@ +--- +schema_version: 1 +id: "iss-2608241115201044" +slug: "sessionstart-exit2-advisory-invisible" +severity: "major" +category: "bug" +source: "user-observation" +found_during: "sessionstart-hook-error-investigation" +found_at: "internal/surface/cli/cli.go" +resolution: "abcd hook session-start exits 0 and delivers its notices where they are read. It returned exit 2 on the belief that a non-zero SessionStart puts stderr in front of the human; the harness renders an opaque startup hook error banner and drops the text, so every notice arrived as an error with no content. The notice text goes to stderr and only a CONSTANT plus a count goes to stdout, naming abcd history staged and abcd ahoy. That split matters: SessionStart stdout is injected into the session context, and an adversarial review demonstrated a directive payload reaching context through meta.setup_version in the TRACKED .abcd/config.json when the text was briefly routed there. bootstrap.sh's own comment had already recorded the reasoning. The shell half of the same premise is deferred as iss-2608251011427187." +impact: fix +--- + +SessionStart exit-2-as-advisory no longer reaches the user. abcd hook session-start returns exitError{Code: 2} whenever it has notices to print (internal/surface/cli/cli.go:1289), on the documented assumption that a non-zero SessionStart surfaces stderr without blocking. Claude Code v2.1.241 instead renders it as 'SessionStart:startup hook error' followed by a truncated echo of the hooks.json command string; the stderr notice itself never reaches the user. Every session-start notice is therefore an opaque error banner: transcript-capture gaps, staged-drain failures, the remaining-backlog count, binary skew, dogfood staleness, and the version transition. The data-loss-adjacent ones matter most - a user told that a session transcript was not captured now sees only a hook error, with no way to learn what it was. \ No newline at end of file diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index 461a151d..5909461c 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -1283,10 +1283,50 @@ func newHookCommand() *cobra.Command { if len(notices) == 0 { return nil } + // Exit ZERO, and put the notices where a SessionStart hook's output is + // actually read (iss-2608241115201044). + // + // This returned exit 2 on the documented assumption that a non-zero + // SessionStart surfaces stderr without blocking. It does not: the + // harness renders the non-zero exit as an opaque "SessionStart:startup + // hook error" banner followed by a truncated echo of the hooks.json + // command string, and DROPS the stderr text. So every notice this hook + // exists to deliver — transcript-capture gaps, staged-drain failures, + // the backlog count, binary skew — reached the user as an error with no + // content, which is worse than silence: it reports a fault in abcd + // rather than the condition it was trying to report. + // + // The notice TEXT goes to stderr, sanitised. Only a CONSTANT goes to + // stdout. + // + // SessionStart's stdout is injected into the session's context, and the + // notices interpolate repo-derived strings — `meta.setup_version` from + // .abcd/config.json is a TRACKED file, so a pull request or a fork can + // set it, and an adversarial review demonstrated a directive payload + // reaching context through exactly this line. termsafe.Sanitize defends a + // terminal, not a context window: it masks control bytes and leaves prose + // untouched, which is the whole of an injection. Nothing repo-derived is + // worth putting on that channel for a notice. + // + // So stdout carries a fixed sentence and a count — enough for the session + // to know notices exist and say where to look — and the content stays on + // stderr where it has always been. The exit code stays 0 because a notice + // is not a hook failure, which is the delivery bug this fixes + // (iss-2608241115201044): a non-zero exit renders as an opaque error + // banner with the text dropped. for _, n := range notices { fmt.Fprintln(cmd.ErrOrStderr(), n) } - return &exitError{Code: 2} // non-zero so SessionStart shows it; SessionStart never blocks + // Name the verbs that actually hold the detail. An earlier draft sent + // the reader to `abcd ahoy` alone, which renders install state and a + // gap COUNT and says nothing about a staged transcript or a failed + // drain — so the one notice naming a privacy artefact was the one it + // lost. `abcd history staged` is where that lives. + fmt.Fprintf(cmd.OutOrStdout(), + "abcd: %d session-start notice(s) on this hook's stderr; "+ + "run `abcd history staged` for transcript backlog or `abcd ahoy` for install state.\n", + len(notices)) + return nil }, }) diff --git a/internal/surface/cli/hook_session_start_test.go b/internal/surface/cli/hook_session_start_test.go index d3b5335b..f0665ada 100644 --- a/internal/surface/cli/hook_session_start_test.go +++ b/internal/surface/cli/hook_session_start_test.go @@ -64,14 +64,37 @@ func runSessionStart(stdin string, args ...string) (stdout, stderr string, code // TestHookSessionStartWarnsWhenStoreMissing is iss-95: a session that begins in a // repo where abcd is not installed must SAY SO — visibly — so the user isn't left -// with a silently non-accruing transcript corpus. SessionStart shows a hook's -// stderr as a visible notice only on a non-zero exit, and never blocks on it. +// with a silently non-accruing transcript corpus. +// +// The DELIVERY contract changed under iss-2608241115201044 and this test changed +// with it, twice. It used to require a non-zero exit, on the assumption that +// SessionStart renders a hook's stderr when the hook exits non-zero. The harness +// instead renders an opaque "SessionStart:startup hook error" banner and drops the +// stderr text, so the old assertion pinned a behaviour that delivered nothing. +// +// The first correction put the notice TEXT on stdout, and an adversarial review +// showed why that is wrong: SessionStart's stdout is injected into the session's +// context, and these notices interpolate repo-derived strings from a TRACKED +// config file. So stdout carries a constant and a count, the text stays on +// stderr, and the exit code is 0 because a notice is not a hook failure. Both +// channels are asserted, because the point is that each carries the right thing. func TestHookSessionStartWarnsWhenStoreMissing(t *testing.T) { repo := gitRepoNoStore(t) - _, stderr, code := runSessionStart(startPayload("s1", repo), "hook", "session-start") + stdout, stderr, code := runSessionStart(startPayload("s1", repo), "hook", "session-start") - if code == 0 { - t.Error("a missing store must exit non-zero so SessionStart renders the notice; got exit 0 (silent)") + if code != 0 { + t.Errorf("a notice is not a hook failure; got exit %d, which the harness renders as an opaque error banner with the text dropped", code) + } + if !strings.Contains(stderr, "ahoy install") { + t.Errorf("the notice text must reach stderr; stderr = %q", stderr) + } + if !strings.Contains(stdout, "session-start notice") { + t.Errorf("stdout must say notices exist so the session knows to look; stdout = %q", stdout) + } + // The guard that matters: nothing repo-derived may reach the context channel. + if strings.Contains(stdout, "ahoy install") || strings.Contains(stdout, repo) { + t.Errorf("repo-derived text reached SessionStart stdout, which is injected into "+ + "the session's context; stdout = %q", stdout) } if !strings.Contains(stderr, "ahoy install") { t.Errorf("the notice must tell the user how to fix it (ahoy install); stderr = %q", stderr) diff --git a/internal/surface/cli/hooks_sessionstart_test.go b/internal/surface/cli/hooks_sessionstart_test.go index 9ba19fa7..0009fba7 100644 --- a/internal/surface/cli/hooks_sessionstart_test.go +++ b/internal/surface/cli/hooks_sessionstart_test.go @@ -135,8 +135,12 @@ func sessionStartRoot(t *testing.T, bootstrap, binary string) (root, calls strin // LimitReader), so two calls sharing one stdin leave the second reading EOF; // - `hook prompt-router-reset` writes an UNCONDITIONAL success diagnostic to // stderr, so whichever call runs first owns the line the transcript renders; -// - `hook session-start` exits 2 when it has a notice, which is the only way -// its warning reaches the human. +// - `hook session-start` exits 0 even when it has a notice, writing the text to +// stderr and a constant to stdout. It used to exit 2, on the belief that a +// non-zero SessionStart puts stderr in front of the human; the harness instead +// renders an opaque error banner and drops the text (iss-2608241115201044). +// This stub models the current binary, so the chain's own exits are what these +// tests exercise. const stubBinary = `#!/bin/sh in=$(cat) printf '%s %s stdin=[%s]\n' "$1" "$2" "$in" >> "$ABCD_CALLS" @@ -146,7 +150,8 @@ if [ "$2" = "prompt-router-reset" ]; then exit 0 fi printf 'abcd: a session-start notice\n' >&2 -exit 2 +printf 'abcd: 1 session-start notice(s) on this hook stderr\n' +exit 0 ` // sessionStartPayload is a harness SessionStart payload of the shape @@ -220,8 +225,19 @@ exit 2 if got := firstLine(stderr); !strings.HasPrefix(got, "abcd bootstrap: installed") { t.Errorf("the first stderr line must be the bootstrap's success (it is the only line the transcript shows); got %q\nfull stderr:\n%s", got, stderr) } - if code == 0 { - t.Error("the exit must stay non-zero: only a non-zero SessionStart exit puts stderr in front of the human") + // The chain still exits 2 here, and that is the BOOTSTRAP's code, not the + // binary's. hooks/bootstrap.sh's notice() exits 2 for the same reason the Go + // hook used to, and its own comment states the reasoning that makes the other + // half of this correct: "a SessionStart hook's stdout becomes model context". + // That is exactly why the Go hook now puts only a constant there. + // + // The shell half is deliberately out of scope for iss-2608241115201044, which + // is about the binary's notices: bootstrap.sh's exit code also governs the + // install path, and changing it is a separate change with its own risk. + // Tracked as iss-2608251011427187. What this test pins either way is the + // TEXT and its ORDER, which the first-line assertion above carries. + if code != 2 { + t.Errorf("the bootstrap's own notice code must still propagate; code = %d", code) } if strings.Contains(stderr, "the plugin binary is not installed") { t.Errorf("a successful bootstrap must not be followed by a missing-binary complaint; stderr:\n%s", stderr) @@ -259,8 +275,11 @@ func TestSessionStartSteadyStateRunsBothCalls(t *testing.T) { if got := firstLine(stderr); got != "abcd: a session-start notice" { t.Errorf("session-start's notice must be the line the transcript renders, not the reset's success diagnostic; first line = %q\nfull stderr:\n%s", got, stderr) } - if code != 2 { - t.Errorf("the binary's notice exit must propagate; code = %d", code) + // A notice is not a failure, so nothing propagates a non-zero code here. The + // first-line assertion above is what carries the coverage: the notice TEXT + // must be the line the transcript renders. + if code != 0 { + t.Errorf("a notice must not surface as a hook failure; code = %d", code) } } diff --git a/internal/surface/cli/skew_test.go b/internal/surface/cli/skew_test.go index 0503ceae..71885cf1 100644 --- a/internal/surface/cli/skew_test.go +++ b/internal/surface/cli/skew_test.go @@ -57,8 +57,11 @@ func TestHookSessionStartReportsBinarySkew(t *testing.T) { _, stderr, code := runSessionStart(startPayload("s-skew", repo), "hook", "session-start") - if code == 0 { - t.Error("the skew notice must exit non-zero so SessionStart renders it; got exit 0 (silent)") + // The exit is always 0 now — session-start's RunE returns nil on every path — + // so this cannot fail and is kept only as a regression tripwire on that. + // The stderr assertions below carry the real coverage. + if code != 0 { + t.Errorf("session-start must not signal a notice as a failure; got exit %d", code) } for _, want := range []string{"222222222222", "v0.4.9", "111111111111"} { if !strings.Contains(stderr, want) { @@ -91,7 +94,7 @@ func TestHookSessionStartSkewComparesTheLiveRoot(t *testing.T) { skewCacheMeta(t, "release_tag=v0.4.9\nrelease_sha="+skewRelease+"\nplugin_sha="+skewRelease+"\nfetched_at=2026-08-01T00:00:00Z\n") _, stderr, code := runSessionStart(startPayload("s-live", repo), "hook", "session-start") - if code == 0 { + if code != 0 { t.Fatalf("the notice must be computed against the LIVE plugin root, never a recorded provisioning-time root; got exit 0 (stderr %q)", stderr) } if !strings.Contains(stderr, "222222222222") { @@ -112,7 +115,7 @@ func TestHookSessionStartSkewFallsBackToRootMeta(t *testing.T) { } _, stderr, code := runSessionStart(startPayload("s-fallback", repo), "hook", "session-start") - if code == 0 { + if code != 0 { t.Fatalf("the degraded install's root-local meta must still feed the notice; got exit 0 (stderr %q)", stderr) } for _, want := range []string{"222222222222", "111111111111"} { @@ -135,7 +138,7 @@ func TestHookSessionStartSkewTagIsTermsafe(t *testing.T) { _, stderr, code := runSessionStart(startPayload("s-termsafe", repo), "hook", "session-start") - if code == 0 { + if code != 0 { t.Fatalf("the skew notice must still render; got exit 0 (stderr %q)", stderr) } if strings.ContainsRune(stderr, 0x1b) { From 2b42e00e3d089a241daa930a2250494bde29c669 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:16:49 +0100 Subject: [PATCH 6/7] test: derive the preflight gate list instead of restating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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] --- ...st-is-prose-no-test-derives-from-the-re.md | 14 ++ AGENTS.md | 9 +- internal/core/lint/preflightgates_test.go | 133 ++++++++++++++++++ 3 files changed, 152 insertions(+), 4 deletions(-) create mode 100644 .abcd/work/issues/resolved/iss-2608242043243131-the-preflight-gate-list-is-prose-no-test-derives-from-the-re.md create mode 100644 internal/core/lint/preflightgates_test.go diff --git a/.abcd/work/issues/resolved/iss-2608242043243131-the-preflight-gate-list-is-prose-no-test-derives-from-the-re.md b/.abcd/work/issues/resolved/iss-2608242043243131-the-preflight-gate-list-is-prose-no-test-derives-from-the-re.md new file mode 100644 index 00000000..e7da5206 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2608242043243131-the-preflight-gate-list-is-prose-no-test-derives-from-the-re.md @@ -0,0 +1,14 @@ +--- +schema_version: 1 +id: "iss-2608242043243131" +slug: "the-preflight-gate-list-is-prose-no-test-derives-from-the-re" +severity: "major" +category: "process" +source: "agent-finding" +found_during: "v0.6.5 docs-currency release gate 2026-08-24" +found_at: "Makefile" +resolution: "the preflight gate list is derived from the Makefile recipe and checked against every surface that restates it — the Makefile's own comment block, the install guide, CONTRIBUTING.md, AGENTS.md and its CLAUDE.md mirror. The detector immediately caught two live instances in AGENTS.md, which said three gates in one place and four in another. 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, since rewriting a sentence to name two of five gates removes the sentinel with them; and the Makefile subtest read the whole file including the recipe line it derives the expected set from, so it could not fail. Both are closed and both defeats are proven to fail now." +impact: fix +--- + +the preflight gate list is prose no test derives from the recipe, so it drifts every time the recipe changes. Makefile's preflight target names its prerequisites once; docs/how-to/install.md, CONTRIBUTING.md and the Makefile's own comment each restate that list by hand. The restatements have now drifted twice in two releases: v0.6.4 corrected three surfaces that said three gates after lint-issues made it four (2852c095 updated AGENTS.md alone), and v0.6.5 re-introduced the same defect by adding site-render as a fifth prerequisite and leaving those surfaces saying four. Both times only the host-run docs-currency reviewer noticed, and both times it refused a release cut over it. The class is a derived-value restatement with no deriving test: the same shape as the marketplace slug, which TestInstallGuideDocumentsTheInstallAndUpdatePath solved by reading go.mod and asserting the documented string contains it. The detector is the same move here — parse preflight's prerequisites out of the Makefile and assert each enumerating surface names exactly that set. Acceptance corpus: the two instances above, both of which the test must flag when replayed against their pre-fix trees. \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index f418325e..c6ac98d7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,8 +62,9 @@ Start with the plan and the design record: Run from the repo root. ```bash -make preflight # the pre-push gate: lint-reviews + record-lint + docs-lint, - # then build + vet + test + race (internal) +make preflight # the pre-push gate: lint-reviews + lint-issues + + # record-lint + docs-lint + site-render, then build + vet + + # test + race (internal) make build # cross-compiles bin/abcd-- (there is no plain bin/abcd) gofmt -l . # format gate: any output names a file needing `gofmt -w` go vet ./... # static checks @@ -151,8 +152,8 @@ irreversible; guessing downward costs nothing.** ## Definition of done -- `make preflight` is clean — the four lint gates (`lint-reviews`, - `lint-issues`, `record-lint`, `docs-lint`) plus `go build ./...`, +- `make preflight` is clean — the five gates (`lint-reviews`, `lint-issues`, + `record-lint`, `docs-lint`, `site-render`) plus `go build ./...`, `go vet ./...`, `go test ./...`, and `go test -race ./internal/...`. - `gofmt -l .` reports nothing. The format gate is CI's own step, outside `make preflight`, so run it before pushing. diff --git a/internal/core/lint/preflightgates_test.go b/internal/core/lint/preflightgates_test.go new file mode 100644 index 00000000..369568a1 --- /dev/null +++ b/internal/core/lint/preflightgates_test.go @@ -0,0 +1,133 @@ +package lint_test + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// preflight's prerequisite list is a DERIVED value, and every surface that +// restates it must name the same set (iss-2608242043243131). +// +// The Makefile declares the list once. Three prose surfaces restate it by hand, +// and nothing derived them from the recipe, so they drifted every time the +// recipe changed — twice in two releases, each time caught only by a host-run +// semantic reviewer refusing a release: +// +// - v0.6.4: `lint-issues` made preflight four gates. The commit that added it +// updated AGENTS.md alone; the install guide, CONTRIBUTING.md and the +// Makefile's own comment still said three. +// - v0.6.5: `site-render` made it five. The same three surfaces still said four +// — including the two just corrected. +// +// The repository already solved this exact shape once, for a different derived +// value: TestInstallGuideDocumentsTheInstallAndUpdatePath reads the module path +// out of go.mod and asserts the documented install command contains it. This is +// that move applied to the gate list. +// +// Deliberately a containment check rather than an equality one. These are +// sentences, not lists — "together with the lint-reviews, lint-issues, … gates" +// — so requiring an exact rendering would fail on a comma. What must hold is +// that every prerequisite the recipe declares is NAMED where the list is +// restated, and that no surface names a gate the recipe does not have. +func TestPreflightGateListIsNotRestatedWrongly(t *testing.T) { + root := filepath.Join("..", "..", "..") + + declared := preflightPrereqs(t, root) + if len(declared) < 2 { + t.Fatalf("parsed %d preflight prerequisites from the Makefile; the parser or the recipe changed shape", len(declared)) + } + + // Every surface that enumerates the gates. A file joins this list when it + // starts restating them — which is the moment it becomes able to drift. + for _, rel := range []string{ + "Makefile", // the recipe's own comment, above the recipe + "docs/how-to/install.md", // the build section a contributor copies + "CONTRIBUTING.md", // the local-gates paragraph + "AGENTS.md", // the definition-of-done list + "CLAUDE.md", // AGENTS.md's committed mirror + } { + t.Run(rel, func(t *testing.T) { + prose := readRepoFile(t, root, rel) + // The Makefile is checked against the COMMENT BLOCK above the recipe, + // not the whole file: the recipe line is where `declared` comes from, + // so a whole-file containment check passes on its own input and can + // never fail. + if rel == "Makefile" { + prose = commentBlockAbovePreflight(prose) + } + // No skip for a file that stops enumerating. An earlier draft skipped + // when "lint-reviews" was absent, which made the gate 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 passes by skipping. The file list is hand-curated — a + // surface that genuinely stops enumerating leaves this list in the + // same change. + for _, gate := range declared { + if !strings.Contains(prose, gate) { + t.Errorf("%s enumerates the preflight gates but omits %q.\n\n"+ + "The Makefile declares: %s\n"+ + "A restated list that has fallen behind the recipe understates what "+ + "guards a push, and this drift has reached a release twice.", + rel, gate, strings.Join(declared, " ")) + } + } + }) + } +} + +// preflightPrereqs reads the prerequisites off the `preflight:` recipe line. +// Hand-parsed for the same reason the workflow contracts beside it are: this +// repository carries no Makefile parser and adds none for one line. +func preflightPrereqs(t *testing.T, root string) []string { + t.Helper() + for _, line := range strings.Split(readRepoFile(t, root, "Makefile"), "\n") { + rest, ok := strings.CutPrefix(line, "preflight:") + if !ok { + continue + } + var out []string + for _, f := range strings.Fields(rest) { + // Only the lint-style prerequisites are restated in prose; a future + // non-gate prerequisite should not force itself into a sentence. + if regexp.MustCompile(`^[a-z][a-z-]*$`).MatchString(f) { + out = append(out, f) + } + } + return out + } + t.Fatal("Makefile declares no `preflight:` recipe") + return nil +} + +// commentBlockAbovePreflight returns the contiguous `#` comment block directly +// above the `preflight:` recipe, which is the part that restates the gate list. +func commentBlockAbovePreflight(makefile string) string { + lines := strings.Split(makefile, "\n") + at := -1 + for i, l := range lines { + if strings.HasPrefix(l, "preflight:") { + at = i + break + } + } + if at < 0 { + return "" + } + start := at + for start > 0 && strings.HasPrefix(strings.TrimSpace(lines[start-1]), "#") { + start-- + } + return strings.Join(lines[start:at], "\n") +} + +func readRepoFile(t *testing.T, root, rel string) string { + t.Helper() + b, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(rel))) + if err != nil { + t.Fatalf("read %s: %v", rel, err) + } + return string(b) +} From 046f06ed2b47eaa51376ef31089c1a81e10c334d Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:17:01 +0100 Subject: [PATCH 7/7] docs: SD001 is reserved, not armed, and the tree does not satisfy it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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] --- .../06-delivery/02-verification-matrix.md | 2 +- .../plans/2026-08-15-plugin-user-safety.md | 2 +- ...3-audit-privacy-degraded-scanner-silent.md | 12 --------- ...4-sessionstart-exit2-advisory-invisible.md | 12 --------- ...st-is-prose-no-test-derives-from-the-re.md | 12 --------- ...it-capture-and-record-lint-verdicts-cap.md | 27 ------------------- ...pellings-unrecognised-by-the-null-predi.md | 22 --------------- 7 files changed, 2 insertions(+), 87 deletions(-) delete mode 100644 .abcd/work/issues/open/iss-203-audit-privacy-degraded-scanner-silent.md delete mode 100644 .abcd/work/issues/open/iss-2608241115201044-sessionstart-exit2-advisory-invisible.md delete mode 100644 .abcd/work/issues/open/iss-2608242043243131-the-preflight-gate-list-is-prose-no-test-derives-from-the-re.md delete mode 100644 .abcd/work/issues/open/iss-285-quoted-yaml-nulls-split-capture-and-record-lint-verdicts-cap.md delete mode 100644 .abcd/work/issues/open/iss-287-uppercase-yaml-null-spellings-unrecognised-by-the-null-predi.md diff --git a/.abcd/development/brief/06-delivery/02-verification-matrix.md b/.abcd/development/brief/06-delivery/02-verification-matrix.md index 0b36002d..d1b6a0e0 100644 --- a/.abcd/development/brief/06-delivery/02-verification-matrix.md +++ b/.abcd/development/brief/06-delivery/02-verification-matrix.md @@ -91,6 +91,6 @@ | Recovery humility (idea-2) | `/abcd:disembark` and `/abcd:embark` surface docs carry the recovery-humility paragraph (≤4 sentences); embark closing imperative names "hunt the originating session before trusting the lifeboat blindly" | | Capability scope (idea-4, itd-5 extension) | Every agent prompt frontmatter carries `capability_scope` with `task_classes` (closed-enum tokens) + `designed_for` (free-text 1-line); `internal/core/lint` validates set-membership at plan-review | | Oracle seam contract preserved (idea-4) | Adding `capability_scope` does NOT modify the oracle seam (host-delegated by default, opt-in adapters — adr-25); the later-phase capability-aware routing (Frontier Awareness) is a pre-dispatch selector above the seam, not a modification to the seam contract | -| Surface discipline (sweep) | No `/abcd: show`, ` stats`, ` list` (plain), or ` view` sub-verbs ship; bare invocation renders state for all `/abcd:` commands; lint `SD001` blocks on forbidden sub-verb introduction; earned exception preserved — each of `/abcd:capture list --open`, `/abcd:capture list --resolved`, `/abcd:capture list --wontfix`, `/abcd:capture list --all` (flag immediately adjacent to `list`, never pipe-joined into one token) | +| Surface discipline (sweep) | No `/abcd: show`, ` stats`, ` list` (plain), or ` view` sub-verbs ship; bare invocation renders state for all `/abcd:` commands; `SD001` is reserved for this check but not yet implemented, so the discipline is enforced by review today, not by lint — and the shipped tree does not currently satisfy it: `abcd history show` and a plain `abcd history list` both ship, and `abcd banlist list` renders identically to bare `abcd banlist`, which is the redundancy the rule exists to forbid; bare invocation renders state for most but not all parents, `docs`, `history`, `disembark`, `embark`, `guard` and `ideate` printing usage instead. Those are the gaps a future `SD001` would have to admit or the tree would have to close; earned exception preserved — each of `/abcd:capture list --open`, `/abcd:capture list --resolved`, `/abcd:capture list --wontfix`, `/abcd:capture list --all` (flag immediately adjacent to `list`, never pipe-joined into one token) | > **Open question (adr-35):** the *Disembark checkpoint* row above locates the interrupt checkpoint at `.abcd/logbook/disembark//_state.json`. adr-35 makes disembark read-only with respect to the source repository and moves the operations namespace to `~/.abcd/voyage//`, but it does not say where a mid-run checkpoint lands. The gate cannot be written until that is decided: the checkpoint must go either under the operator-level voyage home (alongside `disembark/history.jsonl`) or under the destination — it may not be written into the source tree, which the read-only gate hashes. diff --git a/.abcd/development/plans/2026-08-15-plugin-user-safety.md b/.abcd/development/plans/2026-08-15-plugin-user-safety.md index ad8528a5..a3ed693f 100644 --- a/.abcd/development/plans/2026-08-15-plugin-user-safety.md +++ b/.abcd/development/plans/2026-08-15-plugin-user-safety.md @@ -73,7 +73,7 @@ once. Human-paired (the §4 gate is manual by design). or nothing. 2. **[iss-202](../../work/issues/resolved/iss-202-scanner-pii-config-unguarded-read.md)** (critical) — `scanner.New` reads `pii.json` unguarded. Autonomous-eligible. -3. **[iss-203](../../work/issues/open/iss-203-audit-privacy-degraded-scanner-silent.md)** +3. **[iss-203](../../work/issues/resolved/iss-203-audit-privacy-degraded-scanner-silent.md)** (major) — audit silently degrades when `scanner.New` fails. Same seam as iss-202; land with or immediately after it, never in parallel. 4. **[iss-201](../../work/issues/resolved/iss-201-guard-hook-stdin-overflow-fail-open.md)** diff --git a/.abcd/work/issues/open/iss-203-audit-privacy-degraded-scanner-silent.md b/.abcd/work/issues/open/iss-203-audit-privacy-degraded-scanner-silent.md deleted file mode 100644 index 7e2c699d..00000000 --- a/.abcd/work/issues/open/iss-203-audit-privacy-degraded-scanner-silent.md +++ /dev/null @@ -1,12 +0,0 @@ ---- -schema_version: 1 -id: "iss-203" -slug: "audit-privacy-degraded-scanner-silent" -severity: "major" -category: "bug" -source: "agent-finding" -found_during: "bug-hunt loop round 9 (state issue #197), contract-fidelity hunt angle + independent adversarial verification" -found_at: "internal/core/audit/rule_privacy.go:87 (err == nil guard), internal/adapter/scanner/scanner.go:92-124 (New, degradation paths)" ---- - -audit's privacy-hygiene rule guards on 'if sc, err := scanner.New(ctx.RepoRoot); err == nil' expecting a fallback to the built-in pattern set on error, but scanner.New never returns a non-nil error — every degradation path (unreadable/unparseable/uncompilable pii.json override) returns unavailable=true with a nil error instead — so the guard is always true, the documented fallback branch is dead code, and the rule never calls sc.Unavailable(); a broken per-repo severity override (malformed JSON, a directory at the config path, or an uncompilable override regex) silently drops the repo's raised severities and downgrades the audit exit code from 2 to 1 with zero finding or diagnostic explaining why, unlike history.Capture and the CLI identity path which both check Unavailable() explicitly on the same scanner. Independently verified via two-fixture test (valid override raises severity to error/exit-2; broken override silently drops to warn/exit-1 with no diagnostic) for all three degradation branches (malformed JSON, directory at path, uncompilable regex) \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-2608241115201044-sessionstart-exit2-advisory-invisible.md b/.abcd/work/issues/open/iss-2608241115201044-sessionstart-exit2-advisory-invisible.md deleted file mode 100644 index 8d045e6b..00000000 --- a/.abcd/work/issues/open/iss-2608241115201044-sessionstart-exit2-advisory-invisible.md +++ /dev/null @@ -1,12 +0,0 @@ ---- -schema_version: 1 -id: "iss-2608241115201044" -slug: "sessionstart-exit2-advisory-invisible" -severity: "major" -category: "bug" -source: "user-observation" -found_during: "sessionstart-hook-error-investigation" -found_at: "internal/surface/cli/cli.go" ---- - -SessionStart exit-2-as-advisory no longer reaches the user. abcd hook session-start returns exitError{Code: 2} whenever it has notices to print (internal/surface/cli/cli.go:1289), on the documented assumption that a non-zero SessionStart surfaces stderr without blocking. Claude Code v2.1.241 instead renders it as 'SessionStart:startup hook error' followed by a truncated echo of the hooks.json command string; the stderr notice itself never reaches the user. Every session-start notice is therefore an opaque error banner: transcript-capture gaps, staged-drain failures, the remaining-backlog count, binary skew, dogfood staleness, and the version transition. The data-loss-adjacent ones matter most - a user told that a session transcript was not captured now sees only a hook error, with no way to learn what it was. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-2608242043243131-the-preflight-gate-list-is-prose-no-test-derives-from-the-re.md b/.abcd/work/issues/open/iss-2608242043243131-the-preflight-gate-list-is-prose-no-test-derives-from-the-re.md deleted file mode 100644 index c89efc0f..00000000 --- a/.abcd/work/issues/open/iss-2608242043243131-the-preflight-gate-list-is-prose-no-test-derives-from-the-re.md +++ /dev/null @@ -1,12 +0,0 @@ ---- -schema_version: 1 -id: "iss-2608242043243131" -slug: "the-preflight-gate-list-is-prose-no-test-derives-from-the-re" -severity: "major" -category: "process" -source: "agent-finding" -found_during: "v0.6.5 docs-currency release gate 2026-08-24" -found_at: "Makefile" ---- - -the preflight gate list is prose no test derives from the recipe, so it drifts every time the recipe changes. Makefile's preflight target names its prerequisites once; docs/how-to/install.md, CONTRIBUTING.md and the Makefile's own comment each restate that list by hand. The restatements have now drifted twice in two releases: v0.6.4 corrected three surfaces that said three gates after lint-issues made it four (2852c095 updated AGENTS.md alone), and v0.6.5 re-introduced the same defect by adding site-render as a fifth prerequisite and leaving those surfaces saying four. Both times only the host-run docs-currency reviewer noticed, and both times it refused a release cut over it. The class is a derived-value restatement with no deriving test: the same shape as the marketplace slug, which TestInstallGuideDocumentsTheInstallAndUpdatePath solved by reading go.mod and asserting the documented string contains it. The detector is the same move here — parse preflight's prerequisites out of the Makefile and assert each enumerating surface names exactly that set. Acceptance corpus: the two instances above, both of which the test must flag when replayed against their pre-fix trees. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-285-quoted-yaml-nulls-split-capture-and-record-lint-verdicts-cap.md b/.abcd/work/issues/open/iss-285-quoted-yaml-nulls-split-capture-and-record-lint-verdicts-cap.md deleted file mode 100644 index 1d766192..00000000 --- a/.abcd/work/issues/open/iss-285-quoted-yaml-nulls-split-capture-and-record-lint-verdicts-cap.md +++ /dev/null @@ -1,27 +0,0 @@ ---- -schema_version: 1 -id: "iss-285" -slug: "quoted-yaml-nulls-split-capture-and-record-lint-verdicts-cap" -severity: "major" -category: "bug" -source: "impl-review" -found_during: "pr-294-review" -found_at: "internal/core/capture/parse.go" ---- - -Quoted YAML nulls split capture and record-lint verdicts: capture unquotes before IsNull, record-lint reads the raw value - -An issue record carrying `impact: "NULL"`: capture's `decodeScalar` -(`internal/core/capture/parse.go:188`) strips the double quotes before -`validateStrict` calls `frontmatter.IsNull`, so `abcd capture resolve` -accepts the record — while record-lint's `checkIssueImpact` reads the raw -value with quotes intact, `ParseImpact` fails, and the `issue_impact_valid` -blocker fires on the record capture just accepted. The comment at -`internal/core/lint/lint.go:2397` claims this split is impossible. PR #294 -widened the disagreement window from one spelling (`"null"`) to three; its -own tests pin both sides of the contradiction -(`internal/core/frontmatter/frontmatter_test.go:76` vs -`internal/core/lifeboat/graveyard_abandoned_test.go:167`). The fix is to -normalise quoting on one side, not to widen literal sets. Evidence: -`.abcd/work/reviews/2026-08-19-pr-294-null-predicate/` (F1, F2). -Forge mirror: #372 diff --git a/.abcd/work/issues/open/iss-287-uppercase-yaml-null-spellings-unrecognised-by-the-null-predi.md b/.abcd/work/issues/open/iss-287-uppercase-yaml-null-spellings-unrecognised-by-the-null-predi.md deleted file mode 100644 index 468a4f44..00000000 --- a/.abcd/work/issues/open/iss-287-uppercase-yaml-null-spellings-unrecognised-by-the-null-predi.md +++ /dev/null @@ -1,22 +0,0 @@ ---- -schema_version: 1 -id: "iss-287" -slug: "uppercase-yaml-null-spellings-unrecognised-by-the-null-predi" -severity: "major" -category: "bug" -source: "user-observation" -found_during: "pr-294-review" -found_at: "internal/core/frontmatter/frontmatter.go" ---- - -Uppercase YAML null spellings unrecognised by the null predicates (reported as GitHub #290) - -Retroactive capture of the bug reported as GitHub #290 so the ledger holds -the handle the CHANGELOG entry should cite: `frontmatter.IsNull` and lint's -`isNull` recognised `null` and `~` but not the YAML 1.2 core-schema -spellings `Null`/`NULL`, so records using an uppercase null were misread -(e.g. a superseded ADR's `superseded_by:` treated as a live handle). Fixed -in flight by PR #294, which widens both predicates with regression tests; -resolve this issue when that PR merges, citing its commit. Evidence: -`.abcd/work/reviews/2026-08-19-pr-294-null-predicate/` (F10). -Forge mirror: #374