diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ebc3a54..39c25f2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,10 @@ called out in a **Breaking** section. - **An issue graduates into an intent without retyping.** `abcd capture promote ` is the native verb for step 2 of the record walk: one invocation mints an intent draft — slug reused from the issue, body carrying a by-id pointer to the issue rather than a copy, `promoted_from: iss-N` in its frontmatter — and stamps the issue's `promoted_to` with the minted `itd-N`. Promotion works from any status folder and never moves the issue, a second promote is refused with the existing `itd-N`, and a stamp failure after the mint names the orphan draft and its repair: `capture promote --intent `, the stamp-only mode that links an existing draft instead of minting. (itd-119, spc-24; the `promoted_to` half of iss-245) +### Fixed + +- **An uppercase YAML null no longer reads as a superseding handle.** `frontmatter.IsNull` — and its byte-identical lint-side copy `isNull` — now resolve all four YAML nulls (`null` | `Null` | `NULL` | `~`) plus the empty scalar, the same set the memory package's own scalar parser already held. The two uppercase spellings were previously missed, so `disembark pack` over a foreign target repo emitted a live (`status: accepted`) ADR carrying `superseded_by: NULL` as a `superseded-adr` finding into `graveyard/abandoned.json`, quoting the null literal as its evidence; the narrowing also sat behind record-lint's null gate. Both copies are widened together so the two-gate agreement `capture/validate.go` depends on cannot drift. (#290) + ## [0.5.1] - 2026-08-16 ### Added diff --git a/internal/core/frontmatter/frontmatter.go b/internal/core/frontmatter/frontmatter.go index 1b6b9a75..7f19ca45 100644 --- a/internal/core/frontmatter/frontmatter.go +++ b/internal/core/frontmatter/frontmatter.go @@ -63,7 +63,10 @@ func Fields(lines []string) map[string]Field { return fields } -// IsNull treats an empty value and the YAML nulls ""/"null"/"~" as null. +// IsNull treats an empty value and the four YAML nulls ""/"null"/"Null"/"NULL"/"~" +// as null. YAML 1.1 (!!null) and the YAML 1.2 core schema (§10.2.1.1) both resolve +// the three cased spellings and the tilde; the memory package's own scalar parser +// holds the same set (internal/core/memory/yaml.go). func IsNull(v string) bool { - return v == "" || v == "null" || v == "~" + return v == "" || v == "null" || v == "Null" || v == "NULL" || v == "~" } diff --git a/internal/core/frontmatter/frontmatter_test.go b/internal/core/frontmatter/frontmatter_test.go index f444cb95..d5c41fb1 100644 --- a/internal/core/frontmatter/frontmatter_test.go +++ b/internal/core/frontmatter/frontmatter_test.go @@ -59,12 +59,21 @@ func TestFieldsTrimsCarriageReturn(t *testing.T) { } func TestIsNull(t *testing.T) { - for _, v := range []string{"", "null", "~"} { + // The four YAML nulls (YAML 1.1 !!null / YAML 1.2 core schema §10.2.1.1: + // null | Null | NULL | ~) plus the empty scalar all read as null. The + // uppercase spellings were previously missed (iss #290) even though the + // repo's own YAML scalar parser already holds the full set. + for _, v := range []string{"", "null", "Null", "NULL", "~"} { if !IsNull(v) { t.Errorf("IsNull(%q) = false, want true", v) } } - for _, v := range []string{"itd-9", "spc-1", "standalone"} { + // Only the exact YAML null spellings count: near-misses and record handles + // must stay non-null, so a fix can never widen this to a case-fold or a + // substring match. Fields does not strip quotes, so a *quoted* null reaches + // IsNull with its quote characters intact and must stay a string — an + // explicit quoted scalar is not a YAML null. + for _, v := range []string{"itd-9", "spc-1", "standalone", "None", "nil", "NUL", "nullish", `"null"`, `"NULL"`, "'~'"} { if IsNull(v) { t.Errorf("IsNull(%q) = true, want false", v) } diff --git a/internal/core/lifeboat/graveyard_abandoned_test.go b/internal/core/lifeboat/graveyard_abandoned_test.go index c83b77a5..73deff26 100644 --- a/internal/core/lifeboat/graveyard_abandoned_test.go +++ b/internal/core/lifeboat/graveyard_abandoned_test.go @@ -149,6 +149,48 @@ func TestAbandonedAcceptedADRIsNotReported(t *testing.T) { } } +func TestAbandonedAcceptedADRWithUppercaseNullIsNotReported(t *testing.T) { + // Regression (iss #290): superseded_by carrying an uppercase YAML null — + // NULL or Null — must read as "not superseded", exactly as lowercase null + // and ~ do. frontmatter.IsNull previously missed the uppercase spellings, so + // a live (status: accepted) ADR packed from a foreign repo via + // `disembark pack` was silently emitted as a superseded-adr finding, quoting + // `superseded_by: NULL` as its evidence. The status is not-superseded and + // case-folded, so only the null literal decides the finding. + // + // This walks the bare-spelling matrix on the lifeboat path: only the four + // UNQUOTED YAML nulls decide the finding here. A *quoted* null (`"NULL"`, + // `'Null'`) is deliberately NOT in this table: per YAML scalar semantics a + // quoted value is a string, and frontmatter.IsNull — which sees what Fields + // captured, quotes intact — must keep reading it as non-null (asserted by + // TestIsNull's negative controls). gvSupersededADRs currently calls + // gvUnquote BEFORE IsNull, so in the lifeboat path alone a quoted null + // happens to read as absent today; that is quote-insensitive sentinel + // behaviour of gvUnquote, not YAML null semantics, and it is an open + // heuristic decision for lifeboat supersession handling tracked separately — + // this regression pins only the unquoted spellings. A real record handle + // (`adr-9`) is the positive control: widening the null set must not + // suppress a genuine superseding pointer. + nulls := []string{"NULL", "Null", "null", "~"} + for _, nul := range nulls { + dir, write := abandonedWriter(t) + write(".abcd/development/decisions/adrs/0035-live.md", + "---\nid: adr-35\nstatus: accepted\nsuperseded_by: "+nul+"\n---\n\n# Live decision\n") + fs := gvSupersededADRs(abandonedCtx(t, dir)) + if len(fs) != 0 { + t.Fatalf("superseded_by: %s is a YAML null and must not be reported, got %v", nul, fs) + } + } + // Positive control: a real handle still yields a superseded finding. + dir, write := abandonedWriter(t) + write(".abcd/development/decisions/adrs/0035-live.md", + "---\nid: adr-35\nstatus: accepted\nsuperseded_by: adr-9\n---\n\n# Live decision\n") + fs := gvSupersededADRs(abandonedCtx(t, dir)) + if _, ok := gvFindingByID(fs, "adr-35"); !ok { + t.Fatalf("superseded_by: adr-9 is a real handle and must still be reported, got %v", fs) + } +} + func TestAbandonedSupersededADRAcrossBothHomesDedupes(t *testing.T) { dir, write := abandonedWriter(t) // Same ADR id present in the native home AND a conventional home. diff --git a/internal/core/lint/isnull_test.go b/internal/core/lint/isnull_test.go new file mode 100644 index 00000000..78051a4d --- /dev/null +++ b/internal/core/lint/isnull_test.go @@ -0,0 +1,36 @@ +package lint + +import "testing" + +// TestIsNull covers lint's private null predicate, the second copy of +// frontmatter.IsNull that record-lint gates on. It must recognise the same four +// YAML nulls (null | Null | NULL | ~) plus the empty scalar as the frontmatter +// copy, or the two-gate agreement capture/validate.go depends on breaks: an +// `impact: NULL` record would pass one gate and be refused by the other. +// Regression for iss #290 — the uppercase spellings were previously missed. +func TestIsNull(t *testing.T) { + for _, v := range []string{"", "null", "Null", "NULL", "~"} { + if !isNull(v) { + t.Errorf("isNull(%q) = false, want true", v) + } + } + for _, v := range []string{"itd-9", "spc-1", "standalone", "None", "nil", "NUL", "nullish"} { + if isNull(v) { + t.Errorf("isNull(%q) = true, want false", v) + } + } +} + +// TestIsAbsentValueUppercaseNull confirms isAbsentValue, which delegates to +// isNull, inherits the widened set: an uppercase YAML null is an absence, not a +// malformed value. +func TestIsAbsentValueUppercaseNull(t *testing.T) { + for _, v := range []string{"NULL", "Null", "null", "~", "", "[]"} { + if !isAbsentValue(v) { + t.Errorf("isAbsentValue(%q) = false, want true", v) + } + } + if isAbsentValue("adr-9") { + t.Errorf("isAbsentValue(%q) = true, want false", "adr-9") + } +} diff --git a/internal/core/lint/lint.go b/internal/core/lint/lint.go index 2a8211d6..e08b7c77 100644 --- a/internal/core/lint/lint.go +++ b/internal/core/lint/lint.go @@ -2394,8 +2394,11 @@ func contentExempt(rel string, fields map[string]fmField, cfg Config) bool { return false } +// isNull is lint's copy of frontmatter.IsNull; the two must recognise the same +// four YAML nulls (null | Null | NULL | ~) plus the empty scalar so the gates +// they back (record-lint and capture/validate.go) can never disagree on a value. func isNull(v string) bool { - return v == "" || v == "null" || v == "~" + return v == "" || v == "null" || v == "Null" || v == "NULL" || v == "~" } // fenceMask marks lines that are inside (or are a marker for) a triple-backtick diff --git a/internal/surface/cli/guard_shim_test.go b/internal/surface/cli/guard_shim_test.go index e3a33a47..26d1cbe3 100644 --- a/internal/surface/cli/guard_shim_test.go +++ b/internal/surface/cli/guard_shim_test.go @@ -63,7 +63,26 @@ func fakePluginRoot(t *testing.T, script string) string { func runShim(t *testing.T, command, pluginRoot string) (stderr string, code int) { t.Helper() cmd := exec.Command("/bin/sh", "-c", command) - cmd.Env = append(os.Environ(), "CLAUDE_PLUGIN_ROOT="+pluginRoot) + // Hermetic env: the shim's last resort is `command -v abcd`, so a + // developer machine with abcd installed on PATH (this one ships + // ~/.local/bin/abcd) would answer the "binary absent" case with the REAL + // binary — a silent allow, no UNGUARDED notice, and a failure that + // reproduces only on machines that use the tool. PATH keeps the system + // directories the shim's POSIX utilities live in (/bin/sh, find, printf); + // everything user-local is out of reach, and HOME moves off-machine for + // the same reason. Same lesson as iss-219's setupHermetic. + for _, e := range os.Environ() { + switch { + case strings.HasPrefix(e, "PATH="), strings.HasPrefix(e, "HOME="), + strings.HasPrefix(e, "CLAUDE_PLUGIN_ROOT="): + default: + cmd.Env = append(cmd.Env, e) + } + } + cmd.Env = append(cmd.Env, + "PATH=/usr/bin:/bin", + "HOME="+t.TempDir(), + "CLAUDE_PLUGIN_ROOT="+pluginRoot) cmd.Stdin = strings.NewReader(`{"tool_name":"Bash","tool_input":{"command":"ls"}}`) var se strings.Builder cmd.Stderr = &se