From f2e56db204bcec984b570642b3c07bdf7c73023d Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sat, 25 Jul 2026 15:06:00 +0100 Subject: [PATCH 1/3] fix: mint record ids across git refs and add a spec-id uniqueness lint Sequential record ids (iss-N, itd-N, spc-N) were minted from the local working tree only, so two branches cut from the same base silently minted the same next id (iss-115, iss-120): invisible on each branch, surfacing only at merge where it breaks the bidirectional intent-spec link. Consolidate the git-ref side of the maximum onto one canonical primitive, recordid.MaxAcrossRefs, and fold it into all three allocators (capture's reservePath, intent's nextIntentID, spec's NextID) so minting is max+1 over the union of the working tree and every local/remote-tracking branch tip. Once one branch commits an id, the other mints past it. A repository git cannot read degrades loudly to working-tree-only minting on stderr (never a silent fallback); a non-repository directory has no refs to collide with and mints quietly. The residual window (two branches minting before either commits) is accepted and caught by the armed record-lint uniqueness rules on the merged PR union. Complete the class for detection: add the spec_id_unique record-lint rule through the shared validateIDUnique primitive (mirroring issue_id_unique and the intent-id guard), flagging every file that claims a duplicate spc-N. Assisted-by: Claude:claude-opus-4-8 --- .abcd/record-lint.json | 307 +++++++++++++++++++++--- .abcd/work/DECISIONS.md | 13 + CHANGELOG.md | 14 ++ internal/core/capture/alloc.go | 12 +- internal/core/capture/alloc_test.go | 2 +- internal/core/capture/capture.go | 14 +- internal/core/capture/workflow.go | 15 +- internal/core/capture/workflow_test.go | 2 +- internal/core/intent/create.go | 36 ++- internal/core/intent/create_test.go | 8 +- internal/core/intent/impact_test.go | 6 +- internal/core/intent/intent.go | 9 +- internal/core/intent/intent_test.go | 2 +- internal/core/intent/lifecycle.go | 5 +- internal/core/lint/lint.go | 65 +++++ internal/core/lint/lint_test.go | 40 +++ internal/core/recordid/recordid.go | 169 +++++++++++++ internal/core/recordid/recordid_test.go | 71 ++++++ internal/core/spec/refunion_test.go | 41 ++++ internal/core/spec/store.go | 42 +++- internal/core/spec/store_test.go | 24 +- internal/surface/cli/cli.go | 16 +- 22 files changed, 824 insertions(+), 89 deletions(-) create mode 100644 internal/core/recordid/recordid.go create mode 100644 internal/core/recordid/recordid_test.go create mode 100644 internal/core/spec/refunion_test.go diff --git a/.abcd/record-lint.json b/.abcd/record-lint.json index a335e02..8fc7f0d 100644 --- a/.abcd/record-lint.json +++ b/.abcd/record-lint.json @@ -1,37 +1,282 @@ { - "roots": [".abcd/development"], + "roots": [ + ".abcd/development" + ], "banned_tokens": [ - {"id":"moved-intents-path","pattern":"roadmap/intents","message":"intents live at intents/ (record IA); update the path","severity":"blocker","successor":"intents/","allow_context":["historical"]}, - {"id":"flow-next-spec-prefix","pattern":"\\bfn-","message":"fn- is the flow-next spec prefix (dropped tool); abcd specs use spc-","severity":"blocker","successor":"spc-","allow_context":["historical"]}, - {"id":"python-lint-names","pattern":"\\bintent_lint\\b|inventory\\.py","message":"Python tool name; the lint engine is internal/core/lint (Go)","severity":"blocker","successor":"internal/core/lint","allow_context":["historical","spc-15"]}, - {"id":"retired-activity-dir","pattern":"development/activity","message":"development/activity is retired; working material lives in .abcd/work/ (shared) or .abcd/.work.local/ (local)","severity":"blocker","successor":".abcd/work/ or .abcd/.work.local/","allow_context":["historical"]}, - {"id":"retired-work-root","pattern":"\\.work/","message":".work/ is retired as a root path; use .abcd/work/ or .abcd/.work.local/","severity":"blocker","successor":".abcd/work/ or .abcd/.work.local/","allow_context":["historical","\\.abcd/\\.work\\.local"]}, - {"id":"retired-scripts-dir","pattern":"scripts/abcd/","message":"scripts/abcd/ is retired; behaviour lives in the Go binary (internal/core)","severity":"blocker","successor":"internal/core","allow_context":["historical"]}, - {"id":"flow-next-dir","pattern":"\\.flow/","message":".flow/ belongs to the dropped flow-next tool","severity":"blocker","successor":".abcd/","allow_context":["historical"]}, - {"id":"retired-workflow-files","pattern":"workflows/lint\\.yml|lint-corpus\\.yml","message":"retired workflow file; CI is .github/workflows/ci.yml","severity":"blocker","successor":".github/workflows/ci.yml","allow_context":["historical"]}, - {"id":"retired-since-staged-flag","pattern":"--since-staged","message":"--since-staged is a retired flag spelling","severity":"blocker","successor":"--since","allow_context":["historical"]}, - {"id":"dev-repo-name","pattern":"\\babcdDev\\b","message":"single-repo (adr-28): use 'the frozen reference implementation' / abcd-cli","severity":"blocker","successor":"abcd-cli","allow_context":["historical"]}, - {"id":"workspaces-json","pattern":"workspaces\\.json","message":"the workspace concept is dropped; only a negation may mention it","severity":"blocker","successor":"index.json (the sole user-scope history registry, keyed by root_commit)","allow_context":["no ","not ","there is no","sole user-scope","dropped","without a"]}, - {"id":"meta-json-file","pattern":"\\bmeta\\.json\\b","message":"meta.json is abolished as a repo file; setup metadata is config.json[\"meta\"]","severity":"blocker","successor":"config.json[\"meta\"]","allow_context":["history","root-sha","root-commit","per-root","index.json","identity","no ","there is no","separate","abolished","not ","without"]}, - {"id":"presidio-default","pattern":"Presidio","message":"native Go PII engine is the default; Presidio is only one opt-in scanner adapter","severity":"warn","successor":"the native Go PII engine","allow_context":["gitleaks","opt-in","adapter","seam"]} + { + "id": "moved-intents-path", + "pattern": "roadmap/intents", + "message": "intents live at intents/ (record IA); update the path", + "severity": "blocker", + "successor": "intents/", + "allow_context": [ + "historical" + ] + }, + { + "id": "flow-next-spec-prefix", + "pattern": "\\bfn-", + "message": "fn- is the flow-next spec prefix (dropped tool); abcd specs use spc-", + "severity": "blocker", + "successor": "spc-", + "allow_context": [ + "historical" + ] + }, + { + "id": "python-lint-names", + "pattern": "\\bintent_lint\\b|inventory\\.py", + "message": "Python tool name; the lint engine is internal/core/lint (Go)", + "severity": "blocker", + "successor": "internal/core/lint", + "allow_context": [ + "historical", + "spc-15" + ] + }, + { + "id": "retired-activity-dir", + "pattern": "development/activity", + "message": "development/activity is retired; working material lives in .abcd/work/ (shared) or .abcd/.work.local/ (local)", + "severity": "blocker", + "successor": ".abcd/work/ or .abcd/.work.local/", + "allow_context": [ + "historical" + ] + }, + { + "id": "retired-work-root", + "pattern": "\\.work/", + "message": ".work/ is retired as a root path; use .abcd/work/ or .abcd/.work.local/", + "severity": "blocker", + "successor": ".abcd/work/ or .abcd/.work.local/", + "allow_context": [ + "historical", + "\\.abcd/\\.work\\.local" + ] + }, + { + "id": "retired-scripts-dir", + "pattern": "scripts/abcd/", + "message": "scripts/abcd/ is retired; behaviour lives in the Go binary (internal/core)", + "severity": "blocker", + "successor": "internal/core", + "allow_context": [ + "historical" + ] + }, + { + "id": "flow-next-dir", + "pattern": "\\.flow/", + "message": ".flow/ belongs to the dropped flow-next tool", + "severity": "blocker", + "successor": ".abcd/", + "allow_context": [ + "historical" + ] + }, + { + "id": "retired-workflow-files", + "pattern": "workflows/lint\\.yml|lint-corpus\\.yml", + "message": "retired workflow file; CI is .github/workflows/ci.yml", + "severity": "blocker", + "successor": ".github/workflows/ci.yml", + "allow_context": [ + "historical" + ] + }, + { + "id": "retired-since-staged-flag", + "pattern": "--since-staged", + "message": "--since-staged is a retired flag spelling", + "severity": "blocker", + "successor": "--since", + "allow_context": [ + "historical" + ] + }, + { + "id": "dev-repo-name", + "pattern": "\\babcdDev\\b", + "message": "single-repo (adr-28): use 'the frozen reference implementation' / abcd-cli", + "severity": "blocker", + "successor": "abcd-cli", + "allow_context": [ + "historical" + ] + }, + { + "id": "workspaces-json", + "pattern": "workspaces\\.json", + "message": "the workspace concept is dropped; only a negation may mention it", + "severity": "blocker", + "successor": "index.json (the sole user-scope history registry, keyed by root_commit)", + "allow_context": [ + "no ", + "not ", + "there is no", + "sole user-scope", + "dropped", + "without a" + ] + }, + { + "id": "meta-json-file", + "pattern": "\\bmeta\\.json\\b", + "message": "meta.json is abolished as a repo file; setup metadata is config.json[\"meta\"]", + "severity": "blocker", + "successor": "config.json[\"meta\"]", + "allow_context": [ + "history", + "root-sha", + "root-commit", + "per-root", + "index.json", + "identity", + "no ", + "there is no", + "separate", + "abolished", + "not ", + "without" + ] + }, + { + "id": "presidio-default", + "pattern": "Presidio", + "message": "native Go PII engine is the default; Presidio is only one opt-in scanner adapter", + "severity": "warn", + "successor": "the native Go PII engine", + "allow_context": [ + "gitleaks", + "opt-in", + "adapter", + "seam" + ] + } ], "rules": { - "no_git_metadata": {"enabled": true, "severity": "blocker", "fields": ["created","updated","author","maintained_by","version"]}, - "links_resolve": {"enabled": true, "severity": "blocker"}, - "no_brittle_line_refs": {"enabled": true, "severity": "warn"}, - "directory_coverage": {"enabled": true, "severity": "warn", "exempt": []}, - "intent_lifecycle": {"enabled": true, "severity": "blocker", "intents_dir": "intents"}, - "intent_impact_valid": {"enabled": true, "severity": "blocker", "intents_dir": "intents"}, - "issue_id_unique": {"enabled": true, "severity": "blocker", "issues_dir": ".abcd/work/issues"}, - "issue_impact_valid": {"enabled": true, "severity": "blocker", "issues_dir": ".abcd/work/issues"}, - "spec_lifecycle": {"enabled": true, "severity": "blocker", "specs_dir": "specs", "intents_dir": "intents"}, - "persona_registry": {"enabled": true, "severity": "blocker", "registry": ".abcd/development/personas.json"}, - "context_status_free": {"enabled": true, "severity": "blocker", "target": ".abcd/work/CONTEXT.md"}, - "surface_coverage": {"enabled": true, "severity": "blocker", "commands_dir": "commands/abcd", "skills_dir": "skills", "registry": ".abcd/development/brief/04-surfaces/README.md"}, - "receipt_gate": {"enabled": false, "severity": "blocker", "receipts_dir": ".abcd/work/reviews", "required_gates": ["docs-currency-reviewer", "iss35-brief-surface-crosscheck"]}, - "gate_lockstep": {"enabled": true, "severity": "blocker", "runbook": ".abcd/development/release-gate/README.md", "workflow": ".github/workflows/release.yml", "job": "verify", "ignore_steps": ["Check out the pushed commit", "Set up Go"], "min_gates": 9}, - "forbidden_synonyms": {"enabled": true, "severity": "blocker", "glossary_dir": ".abcd/development/brief/glossary", "enforce": ["epic"], "exempt_prefixes": [".abcd/development/research/", ".abcd/development/decisions/", ".abcd/development/plans/", ".abcd/development/intents/shipped/", ".abcd/development/intents/superseded/"], "allow_context": ["epic-review", "epic->spec", "epic→spec", "epic-to-spec"]} + "no_git_metadata": { + "enabled": true, + "severity": "blocker", + "fields": [ + "created", + "updated", + "author", + "maintained_by", + "version" + ] + }, + "links_resolve": { + "enabled": true, + "severity": "blocker" + }, + "no_brittle_line_refs": { + "enabled": true, + "severity": "warn" + }, + "directory_coverage": { + "enabled": true, + "severity": "warn", + "exempt": [] + }, + "intent_lifecycle": { + "enabled": true, + "severity": "blocker", + "intents_dir": "intents" + }, + "intent_impact_valid": { + "enabled": true, + "severity": "blocker", + "intents_dir": "intents" + }, + "issue_id_unique": { + "enabled": true, + "severity": "blocker", + "issues_dir": ".abcd/work/issues" + }, + "issue_impact_valid": { + "enabled": true, + "severity": "blocker", + "issues_dir": ".abcd/work/issues" + }, + "spec_lifecycle": { + "enabled": true, + "severity": "blocker", + "specs_dir": "specs", + "intents_dir": "intents" + }, + "spec_id_unique": { + "enabled": true, + "severity": "blocker", + "specs_dir": "specs", + "intents_dir": "intents" + }, + "persona_registry": { + "enabled": true, + "severity": "blocker", + "registry": ".abcd/development/personas.json" + }, + "context_status_free": { + "enabled": true, + "severity": "blocker", + "target": ".abcd/work/CONTEXT.md" + }, + "surface_coverage": { + "enabled": true, + "severity": "blocker", + "commands_dir": "commands/abcd", + "skills_dir": "skills", + "registry": ".abcd/development/brief/04-surfaces/README.md" + }, + "receipt_gate": { + "enabled": false, + "severity": "blocker", + "receipts_dir": ".abcd/work/reviews", + "required_gates": [ + "docs-currency-reviewer", + "iss35-brief-surface-crosscheck" + ] + }, + "gate_lockstep": { + "enabled": true, + "severity": "blocker", + "runbook": ".abcd/development/release-gate/README.md", + "workflow": ".github/workflows/release.yml", + "job": "verify", + "ignore_steps": [ + "Check out the pushed commit", + "Set up Go" + ], + "min_gates": 9 + }, + "forbidden_synonyms": { + "enabled": true, + "severity": "blocker", + "glossary_dir": ".abcd/development/brief/glossary", + "enforce": [ + "epic" + ], + "exempt_prefixes": [ + ".abcd/development/research/", + ".abcd/development/decisions/", + ".abcd/development/plans/", + ".abcd/development/intents/shipped/", + ".abcd/development/intents/superseded/" + ], + "allow_context": [ + "epic-review", + "epic->spec", + "epic\u2192spec", + "epic-to-spec" + ] + } }, - "exempt_paths": [".abcd/development/research/", ".abcd/development/intents/superseded/"], - "exempt_if_status": ["superseded"] + "exempt_paths": [ + ".abcd/development/research/", + ".abcd/development/intents/superseded/" + ], + "exempt_if_status": [ + "superseded" + ] } diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 90c3576..63c9b00 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -749,3 +749,16 @@ parallel-agent merge contention bites. itd-28 implements in the following run against the adapter decision. Queue: plans/2026-07-24-next-run-queue.md (supersedes the 2026-07-18 queue file's pick-up role). + +- 2026-07-25 — Record-id collisions across parallel branches (iss-115, iss-120) + fixed as a class: one canonical allocator primitive (recordid.MaxAcrossRefs) + mints max+1 over the union of the working tree AND every git ref, so a + committed id on another branch is seen; git-unreadable-over-a-repo degrades + loudly to tree-only, a non-repo mints quietly (no refs to collide). Detection + completes the class — a new spec_id_unique record-lint rule via the shared + validateIDUnique primitive (ADR ids left out of scope: this directive is + iss-/itd-/spc- only). Rejected: id-range leases per programme, mint-at-merge, + and non-sequential/hash ids — all trade away human-readable sequential ids + without a maintainer mandate. The residual uncommitted-mint window (both + branches mint before either commits) is accepted behind the armed detectors on + the merged PR union. diff --git a/CHANGELOG.md b/CHANGELOG.md index d768733..a318961 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,20 @@ called out in a **Breaking** section. ### Added +- **Record-id minting now sees every branch, and a spec-id uniqueness lint closes + the class** (iss-115, iss-120). Sequential ids (`iss-N`, `itd-N`, `spc-N`) were + minted from the local working tree only, so two branches cut from the same base + silently minted the same next id — invisible on each branch and surfacing only + at merge. Minting now folds in the highest id committed on every local and + remote-tracking branch (a single canonical refs-union scan), so once one branch + commits an id, the other mints past it. When git cannot be read over a present + repository the mint degrades to working-tree-only and says so loudly on stderr + (never a silent fallback); a directory that is not a repository has no branches + to collide with and mints quietly. The residual window — two branches that both + mint before either commits — is caught by the record-lint uniqueness rules on + the merged pull request, which now cover spec ids too: the new `spec_id_unique` + rule flags every file claiming a duplicate `spc-N`, mirroring the existing + `issue_id_unique` and intent-id guards. - **`abcd capture resolve` and `abcd intent ""` can now stamp a product `impact`** (iss-117). A resolved issue and a shipped intent are in the release set, so the `issue_impact_valid` and `intent_impact_valid` record-lint blockers diff --git a/internal/core/capture/alloc.go b/internal/core/capture/alloc.go index fdfb358..84bc78c 100644 --- a/internal/core/capture/alloc.go +++ b/internal/core/capture/alloc.go @@ -94,7 +94,14 @@ func withLedgerLock(issuesRoot string, fn func() error) error { // reservePath reserves an iss-N id and creates a zero-byte placeholder under // open/, mirroring reserve_issue_path: flock -> scan max N -> O_EXCL create // with bump-retry. When forceID is non-empty it demands that exact id. -func reservePath(issuesRoot, slug, forceID string) (string, string, error) { +// +// refFloor is the highest iss-N observed across other git refs +// (recordid.MaxAcrossRefs), computed by the caller before the lock: the reserved +// id starts at max(local max N, refFloor)+1, so a branch that has already +// committed a higher id is not re-minted (iss-115, iss-120). It is a floor, not a +// substitute for the local scan — the local scan alone sees uncommitted mints in +// this worktree, and the O_EXCL bump-retry still resolves any residual clash. +func reservePath(issuesRoot, slug, forceID string, refFloor int) (string, string, error) { // Validate a caller-supplied ForceID against the iss-N shape BEFORE it is used // to build a path or create a placeholder — a traversal id (../../evil) must // never touch the filesystem outside the ledger, even transiently. @@ -121,6 +128,9 @@ func reservePath(issuesRoot, slug, forceID string) (string, string, error) { } maxN := maxIssN(issuesRoot) + if refFloor > maxN { + maxN = refFloor + } // Guard the id arithmetic below (maxN+1+attempt) against int overflow: a // hand-crafted MaxInt-adjacent filename would otherwise wrap to a negative // "iss--N" that fails reIssID, creating a bogus placeholder that only fails diff --git a/internal/core/capture/alloc_test.go b/internal/core/capture/alloc_test.go index 0d645f1..8e0f3fe 100644 --- a/internal/core/capture/alloc_test.go +++ b/internal/core/capture/alloc_test.go @@ -23,7 +23,7 @@ func TestReservePathRefusesOverflowingCounter(t *testing.T) { if err := os.WriteFile(filepath.Join(ir, "open", name), nil, 0o644); err != nil { t.Fatal(err) } - id, target, err := reservePath(ir, "note", "") + id, target, err := reservePath(ir, "note", "", 0) if !errors.Is(err, ErrAllocatorContention) { t.Fatalf("reservePath must refuse an overflowing counter, got id=%q target=%q err=%v", id, target, err) } diff --git a/internal/core/capture/capture.go b/internal/core/capture/capture.go index f4698dc..a3b095e 100644 --- a/internal/core/capture/capture.go +++ b/internal/core/capture/capture.go @@ -119,12 +119,16 @@ type CaptureRequest struct { ForceID string // migrator-only; "" = auto-allocate } -// CaptureResult is the outcome of a successful Capture. +// CaptureResult is the outcome of a successful Capture. MintWarning is the +// loud-degrade note from the iss-id refs-union scan (empty when the scan +// completed) — the surface MUST render it so a degrade to working-tree-only +// minting is never silent. type CaptureResult struct { - ID string `json:"id"` - Slug string `json:"slug"` - Path string `json:"path"` - Status State `json:"status"` // always "open" + ID string `json:"id"` + Slug string `json:"slug"` + Path string `json:"path"` + Status State `json:"status"` // always "open" + MintWarning string `json:"mint_warning,omitempty"` } // ResolveRequest moves an open issue to resolved/. diff --git a/internal/core/capture/workflow.go b/internal/core/capture/workflow.go index 0a8f814..5325321 100644 --- a/internal/core/capture/workflow.go +++ b/internal/core/capture/workflow.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/REPPL/abcd-cli/internal/core/changelog" + "github.com/REPPL/abcd-cli/internal/core/recordid" "github.com/REPPL/abcd-cli/internal/fsutil" ) @@ -42,7 +43,18 @@ func Capture(req CaptureRequest) (CaptureResult, error) { return CaptureResult{}, err } - issID, placeholder, err := reservePath(issuesRoot, slugNorm, req.ForceID) + // Fold in the highest iss-N committed on any other git ref so two parallel + // branches cannot re-mint the same id once one has committed it (iss-115, + // iss-120). The scan runs outside the ledger lock (refs are immutable during + // this op) and yields a floor for reservePath; a degrade to working-tree-only + // is surfaced via MintWarning, never swallowed. A ledger outside the work tree + // (a non-default IssuesRoot) has no ref coverage, so it keeps tree-only minting. + scan := recordid.RefScan{} + if rel, relErr := filepath.Rel(repoRoot, issuesRoot); relErr == nil && !strings.HasPrefix(rel, "..") { + scan = recordid.MaxAcrossRefs(repoRoot, "iss", []string{filepath.ToSlash(rel)}) + } + + issID, placeholder, err := reservePath(issuesRoot, slugNorm, req.ForceID, scan.Max) if err != nil { return CaptureResult{}, err } @@ -55,6 +67,7 @@ func Capture(req CaptureRequest) (CaptureResult, error) { // Machine output carries a repo-relative locator, never an absolute // developer-identity path (iss-81). result.Path = fsutil.RepoRel(repoRoot, result.Path) + result.MintWarning = scan.Warning() return result, nil } diff --git a/internal/core/capture/workflow_test.go b/internal/core/capture/workflow_test.go index f372b6b..4892317 100644 --- a/internal/core/capture/workflow_test.go +++ b/internal/core/capture/workflow_test.go @@ -61,7 +61,7 @@ func TestReservePathRejectsUnsafeForceID(t *testing.T) { t.Fatal(err) } for _, bad := range []string{"../../evil", "iss-1/x", "iss-1 ", "not-an-id", "iss-1/../../evil"} { - id, target, err := reservePath(ir, "note", bad) + id, target, err := reservePath(ir, "note", bad, 0) if err == nil { t.Fatalf("reservePath must reject unsafe ForceID %q before any fs op (got id=%q target=%q)", bad, id, target) } diff --git a/internal/core/intent/create.go b/internal/core/intent/create.go index 91cad29..e2aa53d 100644 --- a/internal/core/intent/create.go +++ b/internal/core/intent/create.go @@ -11,6 +11,7 @@ import ( "time" "github.com/REPPL/abcd-cli/internal/core/changelog" + "github.com/REPPL/abcd-cli/internal/core/recordid" "github.com/REPPL/abcd-cli/internal/fsutil" ) @@ -36,10 +37,10 @@ const maxSlugLen = 60 // null and whose spec_id is null) and passes Validate; a human expands it, then // `abcd intent plan` schedules it. This is the quoted-text create path itd-46 // delivers — the create half of what spc-6 AC3 (promote) needs. -func CreateFromText(repoRoot, text, impact string) (Intent, error) { +func CreateFromText(repoRoot, text, impact string) (Intent, string, error) { trimmed := strings.TrimSpace(text) if trimmed == "" { - return Intent{}, fmt.Errorf("intent: refusing to create from empty text") + return Intent{}, "", fmt.Errorf("intent: refusing to create from empty text") } // impact is optional on a draft (intent_impact_valid gates the move into // shipped/, not the seed), but when set it must be a legal, non-internal @@ -49,23 +50,25 @@ func CreateFromText(repoRoot, text, impact string) (Intent, error) { if impact != "" { imp, err := changelog.ParseImpact(impact) if err != nil { - return Intent{}, fmt.Errorf("intent: %w", err) + return Intent{}, "", fmt.Errorf("intent: %w", err) } if imp == changelog.ImpactInternal { - return Intent{}, fmt.Errorf("intent: impact must not be internal on an intent — a press-release-first intent is user-facing by definition; declare one of additive|breaking|fix, or record the work as an issue instead") + return Intent{}, "", fmt.Errorf("intent: impact must not be internal on an intent — a press-release-first intent is user-facing by definition; declare one of additive|breaking|fix, or record the work as an issue instead") } } slug, err := deriveIntentSlug(trimmed) if err != nil { - return Intent{}, err + return Intent{}, "", err } var created Intent + var mintWarning string err = withIntentMintLock(repoRoot, func() error { - id, err := nextIntentID(repoRoot) + id, warn, err := nextIntentID(repoRoot) if err != nil { return err } + mintWarning = warn draftsDirAbs := filepath.Join(repoRoot, IntentsRelDir, BucketDrafts) if err := ensureRealDir(draftsDirAbs, filepath.Join(IntentsRelDir, BucketDrafts)); err != nil { return err @@ -92,9 +95,9 @@ func CreateFromText(repoRoot, text, impact string) (Intent, error) { return nil }) if err != nil { - return Intent{}, err + return Intent{}, "", err } - return created, Validate(created) + return created, mintWarning, Validate(created) } // deriveIntentSlug lowercases the text, collapses non-[a-z0-9] runs to a single @@ -119,9 +122,14 @@ func deriveIntentSlug(text string) (string, error) { var slugNonAlnumRe = regexp.MustCompile(`[^a-z0-9]+`) // nextIntentID returns the next free itd-N: max N over every intent file in every -// bucket, plus one. Called under the mint lock so the scan and the subsequent -// write are one critical section (no two concurrent creates observe the same max). -func nextIntentID(repoRoot string) (string, error) { +// bucket AND over intent filenames on every other git ref, plus one. Called under +// the mint lock so the working-tree scan and the subsequent write are one critical +// section (no two concurrent creates in the same worktree observe the same max). +// Folding in recordid.MaxAcrossRefs is what stops two parallel branches from +// re-minting the same itd-N once one has committed it (iss-115, iss-120). The +// returned mintWarning is non-empty (and MUST be surfaced) when the ref scan +// degraded to working-tree-only, so the fallback is never silent. +func nextIntentID(repoRoot string) (id, mintWarning string, err error) { max := 0 for _, bucket := range Buckets { dir := filepath.Join(repoRoot, IntentsRelDir, bucket) @@ -146,7 +154,11 @@ func nextIntentID(repoRoot string) (string, error) { } } } - return fmt.Sprintf("itd-%d", max+1), nil + scan := recordid.MaxAcrossRefs(repoRoot, "itd", []string{IntentsRelDir}) + if scan.Max > max { + max = scan.Max + } + return fmt.Sprintf("itd-%d", max+1), scan.Warning(), nil } // seedDraft renders the canonical draft skeleton: the full draft frontmatter set diff --git a/internal/core/intent/create_test.go b/internal/core/intent/create_test.go index 597ddc7..7324a27 100644 --- a/internal/core/intent/create_test.go +++ b/internal/core/intent/create_test.go @@ -16,7 +16,7 @@ import ( func TestCreateFromTextSeedsDraft(t *testing.T) { root := t.TempDir() - it, err := CreateFromText(root, "I want users to feel the card respects their time", "") + it, _, err := CreateFromText(root, "I want users to feel the card respects their time", "") if err != nil { t.Fatalf("CreateFromText: %v", err) } @@ -59,7 +59,7 @@ func TestCreateFromTextAllocatesNextID(t *testing.T) { writeFile(t, root, plannedDir+"/itd-9-beta.md", "---\nid: itd-9\nslug: beta\nspec_id: spc-1\nkind: standalone\n---\n# beta\n") - it, err := CreateFromText(root, "another product intent", "") + it, _, err := CreateFromText(root, "another product intent", "") if err != nil { t.Fatalf("CreateFromText: %v", err) } @@ -73,7 +73,7 @@ func TestCreateFromTextAllocatesNextID(t *testing.T) { func TestCreateFromTextRefusesEmpty(t *testing.T) { root := t.TempDir() for _, in := range []string{"", " ", "\t\n"} { - if _, err := CreateFromText(root, in, ""); err == nil { + if _, _, err := CreateFromText(root, in, ""); err == nil { t.Fatalf("CreateFromText(%q) must be refused", in) } } @@ -87,7 +87,7 @@ func TestCreateFromTextRefusesEmpty(t *testing.T) { // over a freshly seeded draft — the "abcd audit stays green" guarantee. func TestCreateFromTextPassesRecordLint(t *testing.T) { root := t.TempDir() - if _, err := CreateFromText(root, "seeded from a quoted-text capture", ""); err != nil { + if _, _, err := CreateFromText(root, "seeded from a quoted-text capture", ""); err != nil { t.Fatalf("CreateFromText: %v", err) } cfg := lint.Config{ diff --git a/internal/core/intent/impact_test.go b/internal/core/intent/impact_test.go index c4ff2db..eafc96e 100644 --- a/internal/core/intent/impact_test.go +++ b/internal/core/intent/impact_test.go @@ -33,7 +33,7 @@ func lintIntentImpact(t *testing.T, root string) []lint.Finding { // intent the tool can never ship past its own blocker. func TestCreateFromTextStampsImpact(t *testing.T) { root := t.TempDir() - it, err := CreateFromText(root, "a user-facing improvement worth shipping", "additive") + it, _, err := CreateFromText(root, "a user-facing improvement worth shipping", "additive") if err != nil { t.Fatalf("CreateFromText: %v", err) } @@ -67,7 +67,7 @@ func TestCreateFromTextStampsImpact(t *testing.T) { func TestCreateFromTextRejectsBadImpact(t *testing.T) { root := t.TempDir() for _, bad := range []string{"additiv", "Additive", "internal", `"fix"`} { - if _, err := CreateFromText(root, "some intent text", bad); err == nil { + if _, _, err := CreateFromText(root, "some intent text", bad); err == nil { t.Fatalf("CreateFromText with impact %q must be refused", bad) } } @@ -82,7 +82,7 @@ func TestCreateFromTextRejectsBadImpact(t *testing.T) { // before, so the existing capture->intent promotion flow is unchanged. func TestCreateFromTextImpactOptional(t *testing.T) { root := t.TempDir() - it, err := CreateFromText(root, "seeded without a judgement yet", "") + it, _, err := CreateFromText(root, "seeded without a judgement yet", "") if err != nil { t.Fatalf("CreateFromText: %v", err) } diff --git a/internal/core/intent/intent.go b/internal/core/intent/intent.go index 4b13241..93fe530 100644 --- a/internal/core/intent/intent.go +++ b/internal/core/intent/intent.go @@ -182,10 +182,13 @@ func sortStrings(s []string) { } // PlanResult reports a completed Plan: the updated planned intent and the spec -// minted to realise it. +// minted to realise it. MintWarning is the loud-degrade note from the spec-id +// refs-union scan (empty when the scan completed) — the surface MUST render it so +// a degrade to working-tree-only minting is never silent. type PlanResult struct { - Intent Intent `json:"intent"` - Spec spec.Spec `json:"spec"` + Intent Intent `json:"intent"` + Spec spec.Spec `json:"spec"` + MintWarning string `json:"mint_warning,omitempty"` } // LinkResult reports a completed Link: the updated intent and the spec it now diff --git a/internal/core/intent/intent_test.go b/internal/core/intent/intent_test.go index 3f63dc6..9f2199a 100644 --- a/internal/core/intent/intent_test.go +++ b/internal/core/intent/intent_test.go @@ -213,7 +213,7 @@ func TestPlanReusesExistingSpecForIntent(t *testing.T) { root := t.TempDir() writeFile(t, root, draftsDir+"/itd-10-alpha.md", draftWithAC("itd-10", "alpha")) // Pre-create the spec for this intent; the draft is still an unlinked draft. - sp, err := spec.Create(root, "itd-10", "alpha") + sp, _, err := spec.Create(root, "itd-10", "alpha") if err != nil { t.Fatal(err) } diff --git a/internal/core/intent/lifecycle.go b/internal/core/intent/lifecycle.go index b602792..4a52e94 100644 --- a/internal/core/intent/lifecycle.go +++ b/internal/core/intent/lifecycle.go @@ -138,8 +138,9 @@ func Plan(repoRoot, intentID string) (PlanResult, error) { return PlanResult{}, err } sp, ok := store.ByIntent(intentID) + var mintWarning string if !ok { - sp, err = spec.Create(repoRoot, intentID, it.Slug) + sp, mintWarning, err = spec.Create(repoRoot, intentID, it.Slug) if err != nil { return PlanResult{}, err } @@ -184,7 +185,7 @@ func Plan(repoRoot, intentID string) (PlanResult, error) { it.SpecID = sp.ID it.Bucket = BucketPlanned it.Path = plannedRel - return PlanResult{Intent: it, Spec: sp}, nil + return PlanResult{Intent: it, Spec: sp, MintWarning: mintWarning}, nil } // Link retroactively writes the derived spec_id link on an existing planned diff --git a/internal/core/lint/lint.go b/internal/core/lint/lint.go index 06b7958..ea54ff1 100644 --- a/internal/core/lint/lint.go +++ b/internal/core/lint/lint.go @@ -214,6 +214,14 @@ func Lint(cfg Config, repoRoot string) ([]Finding, error) { findings = append(findings, sl...) } + if suCfg, ok := cfg.Rules["spec_id_unique"]; ok && suCfg.Enabled { + su, err := checkSpecIDUnique(repoRoot, rootAbs, suCfg, cfg) + if err != nil { + return nil, err + } + findings = append(findings, su...) + } + if fsCfg, ok := cfg.Rules["forbidden_synonyms"]; ok && fsCfg.Enabled { fs, err := checkForbiddenSynonyms(repoRoot, rootAbs, fsCfg) if err != nil { @@ -1603,6 +1611,63 @@ func checkSpecLifecycle(repoRoot, rootAbs string, cfg RuleConfig, top Config) ([ return out, nil } +// checkSpecIDUnique flags any spc-N id claimed by two or more spec-store files +// across specs/{open,closed}/. The spec mint allocator now folds every git ref +// into its max (recordid.MaxAcrossRefs), which closes the common +// commit-then-branch collision, but two branches that both mint before either +// commits still race (iss-115, iss-120) — and a hand-added spec file bypasses the +// allocator entirely. This is the record-lint backstop that CI runs on the merged +// PR's union, mirroring issue_id_unique/intent_lifecycle and sharing the one +// validateIDUnique primitive. A malformed or absent id is spec_lifecycle's +// concern, not this rule's, so only well-formed spc-N ids are compared; a +// content-exempt spec is skipped exactly as spec_lifecycle skips it. +func checkSpecIDUnique(repoRoot, rootAbs string, cfg RuleConfig, top Config) ([]Finding, error) { + specsDir := cfg.SpecsDir + if specsDir == "" { + specsDir = "specs" + } + if _, err := os.Stat(filepath.Join(rootAbs, specsDir)); err != nil { + return nil, nil // missing specs/ is soft, mirroring spec_lifecycle + } + rootRel, err := filepath.Rel(repoRoot, rootAbs) + if err != nil { + return nil, err + } + idx, err := ScanSpecLinks(repoRoot, + filepath.ToSlash(filepath.Join(rootRel, intentsDirOf(cfg))), + filepath.ToSlash(filepath.Join(rootRel, specsDir)), top) + if err != nil { + return nil, err + } + + // Track every file each well-formed spc-N id claims, then flag every member of + // a set of size > 1 via the shared primitive. + idFiles := map[string][]string{} + for _, spec := range idx.Specs { + if spec.exempt { + continue + } + id := spec.fields["id"].value + if !specIDFullRe.MatchString(id) { + continue + } + idFiles[id] = append(idFiles[id], filepath.Join(repoRoot, spec.Path)) + } + + var out []Finding + for _, spec := range idx.Specs { + if spec.exempt { + continue + } + id := spec.fields["id"].value + if !specIDFullRe.MatchString(id) { + continue + } + out = append(out, validateIDUnique(repoRoot, spec.Path, id, "spec", "spec_id_unique", cfg.Severity, spec.fields, idFiles)...) + } + return out, nil +} + func validateSpec(rel string, fields map[string]fmField, knownIntent map[string]bool, intentSpecID map[string]string, severity string) []Finding { var out []Finding add := func(line int, msg string) { diff --git a/internal/core/lint/lint_test.go b/internal/core/lint/lint_test.go index bf7ab0e..0de37bb 100644 --- a/internal/core/lint/lint_test.go +++ b/internal/core/lint/lint_test.go @@ -423,6 +423,46 @@ func TestIssueIDUniqueDuplicate(t *testing.T) { } } +func TestSpecIDUniqueDuplicate(t *testing.T) { + root := t.TempDir() + base := "rec/specs" + + // The real collision shape: two branches each minted spc-10 with a different + // slug and different linked intent, then merged. Both files claim spc-10. + writeFile(t, root, base+"/open/spc-10-alpha.md", "---\nid: spc-10\nslug: alpha\nintent: itd-70\n---\n# a\n") + writeFile(t, root, base+"/closed/spc-10-beta.md", "---\nid: spc-10\nslug: beta\nintent: itd-80\n---\n# b\n") + // A unique id must stay clean under this rule. + writeFile(t, root, base+"/open/spc-11-solo.md", "---\nid: spc-11\nslug: solo\nintent: itd-90\n---\n# c\n") + + cfg := Config{ + Roots: []string{"rec"}, + Rules: map[string]RuleConfig{ + "spec_id_unique": {Enabled: true, Severity: "blocker", SpecsDir: "specs", IntentsDir: "intents"}, + }, + } + fs, err := Lint(cfg, root) + if err != nil { + t.Fatal(err) + } + + // Every file in the colliding set is flagged — flagging only one would imply + // the other is authoritative, which the linter cannot know. + dupes := []string{ + filepath.Join(base, "open", "spc-10-alpha.md"), + filepath.Join(base, "closed", "spc-10-beta.md"), + } + for _, f := range dupes { + if !hasFinding(fs, f, "spec_id_unique", 2) { // the id: line + t.Errorf("expected duplicate-id finding on %s:2; got %+v", f, fs) + } + } + for _, f := range fs { + if filepath.Base(f.File) == "spc-11-solo.md" && f.RuleID == "spec_id_unique" { + t.Errorf("unexpected spec_id_unique finding on unique spec: %+v", f) + } + } +} + func TestExemptions(t *testing.T) { root := t.TempDir() // A banned token in an exempt_paths file → no finding. diff --git a/internal/core/recordid/recordid.go b/internal/core/recordid/recordid.go new file mode 100644 index 0000000..ca26442 --- /dev/null +++ b/internal/core/recordid/recordid.go @@ -0,0 +1,169 @@ +// Package recordid is abcd's canonical support for minting sequential record +// ids (iss-N, itd-N, spc-N) that do not collide across parallel branches. +// +// Every ledger mints "max observed N, plus one". Historically each ledger looked +// only at ITS OWN WORKING TREE, so two branches cut from the same base silently +// minted the same next id — invisible on each branch, surfacing only at merge +// (iss-115, iss-120: two programmes both minted spc-10/spc-11, iss-110/iss-111). +// +// MaxAcrossRefs is the one home for the git-ref side of that maximum: it scans +// every local branch and remote-tracking ref for a family's highest id, so a +// branch that has ALREADY committed a higher id is seen even though its file is +// absent from the current working tree. Each ledger keeps its own working-tree +// scan (which alone sees uncommitted mints) and folds this in — union, then +1. +// +// RESIDUAL WINDOW (accepted, documented): a ref only carries a mint once it is +// committed. Two branches that BOTH mint before either commits still collide — +// the ref scan cannot see an uncommitted file on another branch. That window is +// left to the armed record-lint uniqueness rules (issue_id_unique, +// intent_lifecycle, spec_id_unique), which run on the merged PR's union in CI and +// flag every colliding claimant. Refs-union minting closes the common +// commit-then-branch case; the detectors close the rest. +package recordid + +import ( + "os" + "path" + "path/filepath" + "regexp" + "strconv" + "strings" + + "github.com/REPPL/abcd-cli/internal/gitutil" +) + +// refScanMaxBytes caps the stdout buffered from a single git query. A hostile or +// pathological repository cannot make one for-each-ref / ls-tree exhaust memory; +// a truncated tail can only UNDER-count the max, which the loud degrade covers. +const refScanMaxBytes = 8 << 20 // 8 MiB + +// maxRefs bounds how many distinct ref tips the scan visits, keeping the cost +// O(branches) even in a repo with a pathological number of refs. Hitting the cap +// degrades loudly rather than scanning unboundedly. +const maxRefs = 4096 + +// RefScan is the outcome of scanning git refs for a family's highest id N. +// +// Max is the best-effort highest N observed (0 when none, or when the scan could +// not run). Degraded is true when a repository is present but git could not read +// it (git absent/broken over a real repo) or a ref query failed — in which case +// Max reflects only what was seen before the failure and the caller mints from +// the working tree essentially alone, so it MUST surface Warning() rather than +// fall back silently. A directory that is simply NOT a repository is not degraded +// (there are no refs to collide against): Max is 0 and Warning() is empty. +type RefScan struct { + Max int + Degraded bool + Reason string +} + +// Warning renders the loud-degrade note for a surface to print (stderr / result +// envelope), or "" when the scan completed. It never leaks a path — Reason is a +// short, path-free cause. +func (s RefScan) Warning() string { + if !s.Degraded { + return "" + } + return "record-id minting saw the working tree only (" + s.Reason + + "): a parallel branch that has already committed a higher id can collide, " + + "and record-lint on the merged PR is the backstop" +} + +// MaxAcrossRefs returns the highest N in filenames of the form +// -[-].md under any of dirs (repo-relative, slash-separated), +// across every local branch (refs/heads) and remote-tracking ref (refs/remotes) +// of the repository at repoRoot. +// +// It NEVER returns an error: a git-absent host, a repoRoot outside a work tree, +// or a failed ref query degrade LOUDLY (Degraded=true, a path-free Reason) and +// the best-effort Max seen so far is returned. The caller then mints +// max(workingTreeMax, RefScan.Max)+1 and surfaces Warning() — so a degraded scan +// never silently narrows the id space it can see. +// +// Only branch TIPS are scanned (a committed id lives in the tip's tree), so the +// cost is O(distinct branch tips), not O(history). Uncommitted files on other +// branches are deliberately out of scope — see the package doc's residual window. +func MaxAcrossRefs(repoRoot, prefix string, dirs []string) RefScan { + if len(dirs) == 0 { + return RefScan{} + } + if !gitutil.InRepo(repoRoot) { + // A directory with no repository at all has no refs to collide against, so + // working-tree-only minting is COMPLETE, not degraded — stay quiet (the + // common non-repo / test case). But a .git that git could NOT read (git + // absent or broken over a real repo that may hold parallel branches) is the + // dangerous silent-collision case the loud degrade exists to catch: warn. + if gitDirPresent(repoRoot) { + return RefScan{Degraded: true, Reason: "git could not read the repository (git absent or unreadable)"} + } + return RefScan{} + } + + out, err := gitutil.RunLimited(repoRoot, refScanMaxBytes, + "for-each-ref", "--format=%(objectname)", "refs/heads/", "refs/remotes/") + if err != nil { + return RefScan{Degraded: true, Reason: "git for-each-ref failed"} + } + + re := idRe(prefix) + scan := RefScan{} + seen := map[string]bool{} + count := 0 + for _, obj := range strings.Fields(out) { + if seen[obj] { + continue + } + seen[obj] = true + count++ + if count > maxRefs { + scan.Degraded = true + scan.Reason = "more than " + strconv.Itoa(maxRefs) + " refs; scan truncated" + break + } + args := make([]string, 0, 5+len(dirs)) + args = append(args, "ls-tree", "-r", "--name-only", obj, "--") + args = append(args, dirs...) + treeOut, terr := gitutil.RunLimited(repoRoot, refScanMaxBytes, args...) + if terr != nil { + // A single bad ref (e.g. a tag ref that is not a commit, an object-store + // race) degrades loudly but does not abort the scan: the maxima from the + // other refs still tighten the floor. + scan.Degraded = true + scan.Reason = "git ls-tree failed for a ref" + continue + } + for _, line := range strings.Split(treeOut, "\n") { + if line == "" { + continue + } + m := re.FindStringSubmatch(path.Base(line)) + if m == nil { + continue + } + n, err := strconv.Atoi(m[1]) + if err != nil { + continue + } + if n > scan.Max { + scan.Max = n + } + } + } + return scan +} + +// gitDirPresent reports whether repoRoot carries a .git entry (a directory for a +// normal clone, a file for a worktree/submodule). It distinguishes "no repo here, +// nothing to scan" from "a repo is here but git could not read it", so the loud +// degrade fires only for the latter. +func gitDirPresent(repoRoot string) bool { + _, err := os.Stat(filepath.Join(repoRoot, ".git")) + return err == nil +} + +// idRe matches a record filename -[-].md and captures N. The +// prefix is a fixed family tag (iss/itd/spc); it is regexp-quoted defensively so +// a caller can never inject metacharacters through it. +func idRe(prefix string) *regexp.Regexp { + return regexp.MustCompile(`^` + regexp.QuoteMeta(prefix) + `-([0-9]+)(?:-[a-z0-9-]+)?\.md$`) +} diff --git a/internal/core/recordid/recordid_test.go b/internal/core/recordid/recordid_test.go new file mode 100644 index 0000000..d3bdd8c --- /dev/null +++ b/internal/core/recordid/recordid_test.go @@ -0,0 +1,71 @@ +package recordid + +import ( + "os" + "path/filepath" + "testing" + + "github.com/REPPL/abcd-cli/internal/gittest" +) + +// TestMaxAcrossRefsSeesOtherBranches proves the ref scan finds an id committed on +// a branch whose file is absent from the current working tree — the core of the +// parallel-branch collision fix. +func TestMaxAcrossRefsSeesOtherBranches(t *testing.T) { + r := gittest.NewRepo(t) + r.Write("README.md", "# base\n") + r.Commit("base") + + // main carries spc-1 and spc-3; a gap at spc-2 must not lower the max. + r.Write(".abcd/development/specs/open/spc-1-alpha.md", "---\nid: spc-1\n---\n") + r.Write(".abcd/development/specs/closed/spc-3-gamma.md", "---\nid: spc-3\n---\n") + r.Commit("specs on main") + + // A branch cut from base has none of them in its working tree. + r.Git("checkout", "-b", "feature", "HEAD~1") + + scan := MaxAcrossRefs(r.Root(), "spc", []string{".abcd/development/specs"}) + if scan.Degraded { + t.Fatalf("scan degraded unexpectedly: %s", scan.Reason) + } + if scan.Max != 3 { + t.Fatalf("MaxAcrossRefs = %d, want 3 (highest id committed on refs/heads/main)", scan.Max) + } +} + +// TestMaxAcrossRefsLoudWhenGitUnreadable proves the loud-degrade path: a +// repository is present (.git exists) but git cannot be run (PATH emptied, so the +// binary is unfindable) — the dangerous silent-collision case. The scan must +// degrade LOUDLY rather than fall back quietly. +func TestMaxAcrossRefsLoudWhenGitUnreadable(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".git"), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", "") // git becomes unfindable for exec.LookPath + + scan := MaxAcrossRefs(dir, "spc", []string{".abcd/development/specs"}) + if !scan.Degraded { + t.Fatal("a present repository git cannot read must degrade loudly, not fall back silently") + } + if scan.Warning() == "" { + t.Fatal("a degraded scan must produce a non-empty Warning() for the surface to render") + } +} + +// TestMaxAcrossRefsQuietOutsideRepo proves a non-repository directory is NOT a +// loud degrade: there are no refs to collide against, so the scan stays quiet and +// the caller mints from the working tree alone. +func TestMaxAcrossRefsQuietOutsideRepo(t *testing.T) { + dir := t.TempDir() + scan := MaxAcrossRefs(dir, "spc", []string{".abcd/development/specs"}) + if scan.Degraded { + t.Fatalf("non-repo directory must not degrade loudly; got reason %q", scan.Reason) + } + if w := scan.Warning(); w != "" { + t.Fatalf("non-repo directory must emit no warning; got %q", w) + } + if scan.Max != 0 { + t.Fatalf("non-repo Max = %d, want 0", scan.Max) + } +} diff --git a/internal/core/spec/refunion_test.go b/internal/core/spec/refunion_test.go new file mode 100644 index 0000000..3f9ef76 --- /dev/null +++ b/internal/core/spec/refunion_test.go @@ -0,0 +1,41 @@ +package spec + +import ( + "testing" + + "github.com/REPPL/abcd-cli/internal/gittest" +) + +// TestCreateMintsPastACommittedBranch reproduces the parallel-branch id-minting +// race (iss-115, iss-120): two branches cut from one base each mint a spec, and +// without the refs-union scan branch B re-mints the id branch A has already +// committed. After the fix, branch B mints past A's committed id. +func TestCreateMintsPastACommittedBranch(t *testing.T) { + r := gittest.NewRepo(t) + r.Write("README.md", "# base\n") + r.Commit("base") + + // Branch A (main): mint spc-1 and commit it. + spA, _, err := Create(r.Root(), "itd-1", "alpha") + if err != nil { + t.Fatalf("branch A Create: %v", err) + } + if spA.ID != "spc-1" { + t.Fatalf("branch A minted %s, want spc-1", spA.ID) + } + r.Commit("A: " + spA.ID) + + // Branch B, cut from the base BEFORE A's commit: its working tree carries no + // spec, so a tree-only mint would hand out spc-1 again. + r.Git("checkout", "-b", "branch-b", "HEAD~1") + + spB, _, err := Create(r.Root(), "itd-2", "beta") + if err != nil { + t.Fatalf("branch B Create: %v", err) + } + // The class bug: spB.ID == "spc-1" (collides with A). The fix: the ref scan + // sees A's committed spc-1 on refs/heads/main and mints spc-2. + if spB.ID != "spc-2" { + t.Fatalf("branch B minted %s, want spc-2 (collided with branch A's committed id)", spB.ID) + } +} diff --git a/internal/core/spec/store.go b/internal/core/spec/store.go index 1324845..6c0e280 100644 --- a/internal/core/spec/store.go +++ b/internal/core/spec/store.go @@ -11,6 +11,7 @@ import ( "time" "github.com/REPPL/abcd-cli/internal/core/frontmatter" + "github.com/REPPL/abcd-cli/internal/core/recordid" "github.com/REPPL/abcd-cli/internal/fsutil" ) @@ -90,17 +91,28 @@ func parseSpec(relPath, content, bucket string) (Spec, error) { // NextID mints the next spec id. The rule is: // // max(N over existing spec-store files ∪ N over every intent's spec_id -// frontmatter across .abcd/development/intents/**) + 1 +// frontmatter across .abcd/development/intents/** ∪ N over spec-store filenames +// on every other git ref) + 1 // // Scanning the intents is what keeps a freshly minted spec from colliding with // a reservation: itd-3 shipped with spec_id: spc-1 but has no spec-store file, // so a spec-only scan would hand out spc-1 again. Folding intent reservations in // means the first minted id is spc-2 while that reservation stands. -func NextID(repoRoot string) (string, error) { +// +// Scanning every git ref (recordid.MaxAcrossRefs) is what keeps two parallel +// branches from re-minting the same spc-N: once branch A commits spc-10, branch +// B's ref scan sees it even though the file is absent from B's working tree +// (iss-115, iss-120). The returned mintWarning is non-empty (and MUST be +// surfaced) when that ref scan degraded to working-tree-only — git absent, not a +// repo, or a failed ref query — so the fallback is never silent. The cross-ref +// scan reads spec-store FILENAMES only; a spec_id reservation committed in an +// intent's frontmatter on another branch is not folded in (that narrower window +// is left to the record-lint uniqueness rules on the merged PR). +func NextID(repoRoot string) (id, mintWarning string, err error) { max := 0 store, err := Load(repoRoot) if err != nil { - return "", err + return "", "", err } for _, sp := range store.Specs { if n := specNum(sp.ID); n > max { @@ -109,12 +121,16 @@ func NextID(repoRoot string) (string, error) { } reserved, err := maxIntentSpecNum(repoRoot) if err != nil { - return "", err + return "", "", err } if reserved > max { max = reserved } - return fmt.Sprintf("spc-%d", max+1), nil + scan := recordid.MaxAcrossRefs(repoRoot, "spc", []string{SpecsRelDir}) + if scan.Max > max { + max = scan.Max + } + return fmt.Sprintf("spc-%d", max+1), scan.Warning(), nil } // maxIntentSpecNum returns the highest N across every intent's spec_id @@ -173,12 +189,14 @@ func maxIntentSpecNum(repoRoot string) (int, error) { // Create mints an id via NextID and writes specs/open/spc-N-.md with the // intent link in frontmatter. Both the intent id and the slug are validated // before any path is built (the slug becomes a filename). The write is atomic. -func Create(repoRoot, intentID, slug string) (Spec, error) { +// The returned mintWarning is non-empty when the refs-union scan degraded to +// working-tree-only minting; the caller MUST surface it (never swallow it). +func Create(repoRoot, intentID, slug string) (Spec, string, error) { if !intentIDRe.MatchString(intentID) { - return Spec{}, fmt.Errorf("spec: intent id %q must match ^itd-[0-9]+$", intentID) + return Spec{}, "", fmt.Errorf("spec: intent id %q must match ^itd-[0-9]+$", intentID) } if !slugRe.MatchString(slug) { - return Spec{}, fmt.Errorf("spec: slug %q must be kebab-case", slug) + return Spec{}, "", fmt.Errorf("spec: slug %q must be kebab-case", slug) } // Mint and write under the exclusive mint lock: NextID scans the store for // max N and this writes spc-N-.md, so without serialization two @@ -187,11 +205,13 @@ func Create(repoRoot, intentID, slug string) (Spec, error) { // clobber guard detects it. Holding the lock across the scan+write makes the // second run see the first's file and mint N+1. var sp Spec + var mintWarning string err := withMintLock(repoRoot, func() error { - id, err := NextID(repoRoot) + id, warn, err := NextID(repoRoot) if err != nil { return err } + mintWarning = warn openDir := filepath.Join(repoRoot, SpecsRelDir, StatusOpen) if err := ensureDir(openDir, filepath.Join(SpecsRelDir, StatusOpen)); err != nil { return err @@ -211,9 +231,9 @@ func Create(repoRoot, intentID, slug string) (Spec, error) { return nil }) if err != nil { - return Spec{}, err + return Spec{}, "", err } - return sp, Validate(sp) + return sp, mintWarning, Validate(sp) } // withMintLock runs fn while holding an exclusive advisory lock over the spec diff --git a/internal/core/spec/store_test.go b/internal/core/spec/store_test.go index b7fa7b7..95b8bf7 100644 --- a/internal/core/spec/store_test.go +++ b/internal/core/spec/store_test.go @@ -19,7 +19,7 @@ const ( func TestNextIDEmptyRepo(t *testing.T) { root := t.TempDir() - got, err := NextID(root) + got, _, err := NextID(root) if err != nil { t.Fatal(err) } @@ -35,7 +35,7 @@ func TestNextIDReservedByIntent(t *testing.T) { writeFile(t, root, intentsBase+"/shipped/itd-3-rules-loader.md", "---\nid: itd-3\nslug: rules-loader\nspec_id: spc-1\nkind: standalone\n---\n# ok\n") - got, err := NextID(root) + got, _, err := NextID(root) if err != nil { t.Fatal(err) } @@ -54,7 +54,7 @@ func TestNextIDMaxAcrossSpecsAndIntents(t *testing.T) { writeFile(t, root, intentsBase+"/planned/itd-20-x.md", "---\nid: itd-20\nslug: x\nspec_id: spc-2-thing\nkind: standalone\n---\n# ok\n") - got, err := NextID(root) + got, _, err := NextID(root) if err != nil { t.Fatal(err) } @@ -65,7 +65,7 @@ func TestNextIDMaxAcrossSpecsAndIntents(t *testing.T) { func TestCreateRoundTrip(t *testing.T) { root := t.TempDir() - sp, err := Create(root, "itd-9", "my-feature") + sp, _, err := Create(root, "itd-9", "my-feature") if err != nil { t.Fatal(err) } @@ -97,20 +97,20 @@ func TestCreateRoundTrip(t *testing.T) { func TestCreateRejectsBadIntent(t *testing.T) { root := t.TempDir() - if _, err := Create(root, "itd-../../etc", "slug"); err == nil { + if _, _, err := Create(root, "itd-../../etc", "slug"); err == nil { t.Fatal("Create with traversal intent id must fail") } - if _, err := Create(root, "spc-1", "slug"); err == nil { + if _, _, err := Create(root, "spc-1", "slug"); err == nil { t.Fatal("Create with non-itd intent id must fail") } } func TestCreateRejectsBadSlug(t *testing.T) { root := t.TempDir() - if _, err := Create(root, "itd-9", "../../etc"); err == nil { + if _, _, err := Create(root, "itd-9", "../../etc"); err == nil { t.Fatal("Create with traversal slug must fail") } - if _, err := Create(root, "itd-9", "Bad Slug"); err == nil { + if _, _, err := Create(root, "itd-9", "Bad Slug"); err == nil { t.Fatal("Create with non-kebab slug must fail") } } @@ -146,7 +146,7 @@ func TestLoadRejectsTraversalID(t *testing.T) { func TestCloseMovesOpenToClosed(t *testing.T) { root := t.TempDir() - if _, err := Create(root, "itd-9", "my-feature"); err != nil { + if _, _, err := Create(root, "itd-9", "my-feature"); err != nil { t.Fatal(err) } @@ -179,7 +179,7 @@ func TestCloseMovesOpenToClosed(t *testing.T) { // clobbering a same-name spec already sitting in closed/. func TestCloseRefusesWhenClosedTargetExists(t *testing.T) { root := t.TempDir() - if _, err := Create(root, "itd-9", "my-feature"); err != nil { + if _, _, err := Create(root, "itd-9", "my-feature"); err != nil { t.Fatal(err) } // A same-name spec already occupies closed/. @@ -227,7 +227,7 @@ func TestNextIDRejectsUnreservableSpecID(t *testing.T) { root := t.TempDir() writeFile(t, root, intentsBase+"/planned/itd-20-x.md", "---\nid: itd-20\nslug: x\nspec_id: spc-oops\nkind: standalone\n---\n# ok\n") - if _, err := NextID(root); err == nil { + if _, _, err := NextID(root); err == nil { t.Fatal("NextID must fail closed on a spec_id with no reservable number, not silently drop it") } } @@ -277,7 +277,7 @@ func TestCreateConcurrentMintsDistinctIDs(t *testing.T) { go func(i int) { defer wg.Done() <-start // release all goroutines together to maximise the collision window - sp, err := Create(root, "itd-1", fmt.Sprintf("slug-%d", i)) + sp, _, err := Create(root, "itd-1", fmt.Sprintf("slug-%d", i)) mu.Lock() defer mu.Unlock() if err != nil { diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index 94c5f97..9f69f03 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -1154,6 +1154,7 @@ func newIntentCommand(asJSON *bool) *cobra.Command { if err != nil { return &exitError{Code: 2, Msg: "abcd intent plan: " + err.Error()} } + emitMintWarning(cmd, res.MintWarning) return render(cmd.OutOrStdout(), *asJSON, res, func(w io.Writer) { fmt.Fprintf(w, "abcd intent plan — %s drafts -> planned, linked %s\n", res.Intent.ID, res.Spec.ID) fmt.Fprintf(w, " intent: %s\n", res.Intent.Path) @@ -1241,15 +1242,27 @@ const ledgerDecisionRule = " which ledger? half-formed observation, question, o // engine refuses empty/whitespace text and mints the id under the store lock, so // this surface stays a thin marshaller. func createIntentFromText(cmd *cobra.Command, cwd, text, impact string, asJSON bool) error { - it, err := intent.CreateFromText(cwd, text, impact) + it, mintWarning, err := intent.CreateFromText(cwd, text, impact) if err != nil { return &exitError{Code: 2, Msg: "abcd intent: " + err.Error()} } + emitMintWarning(cmd, mintWarning) return render(cmd.OutOrStdout(), asJSON, it, func(w io.Writer) { fmt.Fprintf(w, "created %s (%s) — %s\n", it.ID, it.Bucket, it.Path) }) } +// emitMintWarning prints a record-id mint degrade note to stderr (loud-staging: +// a stage that degraded to working-tree-only minting must say so, never silently +// fall back). The note is engine-produced and path-free; it is sanitised anyway +// before it touches the terminal. Empty warnings emit nothing. +func emitMintWarning(cmd *cobra.Command, warning string) { + if warning == "" { + return + } + fmt.Fprintln(cmd.ErrOrStderr(), "warning: "+termsafe.Sanitize(warning)) +} + // newIntentReviewCommand builds `abcd intent review`: `ingest --verdict-json` // applies a host-produced intent-fidelity verdict to the shipped intent's Audit // Notes (fail-closed: ingested | dead_letter | noop); bare `review ` @@ -1742,6 +1755,7 @@ func newCaptureCommand(asJSON *bool) *cobra.Command { if err != nil { return err } + emitMintWarning(cmd, res.MintWarning) return render(cmd.OutOrStdout(), *asJSON, res, func(w io.Writer) { fmt.Fprintf(w, "captured %s (%s) — %s\n", res.ID, res.Status, res.Path) }) From adc81ad5d427fa1d2042f996b96baf8ac327c352 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sat, 25 Jul 2026 15:06:01 +0100 Subject: [PATCH 2/3] chore: resolve iss-115 and iss-120 (record-id allocator class) Both close on the refs-union allocator + spec_id_unique lint landed in the preceding commit. Resolved with --impact fix (a bug-class correctness fix). Assisted-by: Claude:claude-opus-4-8 --- ...re-is-no-uniqueness-lint-for-spec-ids-so-two-concurrent-b.md | 2 ++ ...120-local-tree-id-minting-races-under-parallel-programmes.md | 2 ++ 2 files changed, 4 insertions(+) rename .abcd/work/issues/{open => resolved}/iss-115-there-is-no-uniqueness-lint-for-spec-ids-so-two-concurrent-b.md (83%) rename .abcd/work/issues/{open => resolved}/iss-120-local-tree-id-minting-races-under-parallel-programmes.md (77%) diff --git a/.abcd/work/issues/open/iss-115-there-is-no-uniqueness-lint-for-spec-ids-so-two-concurrent-b.md b/.abcd/work/issues/resolved/iss-115-there-is-no-uniqueness-lint-for-spec-ids-so-two-concurrent-b.md similarity index 83% rename from .abcd/work/issues/open/iss-115-there-is-no-uniqueness-lint-for-spec-ids-so-two-concurrent-b.md rename to .abcd/work/issues/resolved/iss-115-there-is-no-uniqueness-lint-for-spec-ids-so-two-concurrent-b.md index 57a8554..10827fb 100644 --- a/.abcd/work/issues/open/iss-115-there-is-no-uniqueness-lint-for-spec-ids-so-two-concurrent-b.md +++ b/.abcd/work/issues/resolved/iss-115-there-is-no-uniqueness-lint-for-spec-ids-so-two-concurrent-b.md @@ -7,6 +7,8 @@ category: "bug" source: "impl-review" found_during: "itd-95/itd-96 merge to main" found_at: ".abcd/development/specs" +resolution: "No spec-id uniqueness lint and local-tree-only minting fixed as a class: refs-union allocator (recordid.MaxAcrossRefs) folded into all three id families, plus the new spec_id_unique record-lint rule." +impact: fix --- There is no uniqueness lint for spec ids, so two concurrent branches always mint the same spc-N. abcd mints the next free id from the local tree, so any two branches cut from the same base collide silently: the itd-73/itd-67 programme and the itd-95/itd-96 programme both minted spc-10 and spc-11 on 2026-07-21. The collision is invisible on each branch and only appears on merge, where it breaks the bidirectional intent-spec link in BOTH directions at once — 'itd-95 names spc-10, but spc-10 claims itd-73' and 'itd-67 names spc-11, but spc-11 claims itd-96' — because whichever spec wins the store lookup leaves the other intent's link dangling. iss-74 added an issue-id uniqueness lint; specs need the same guard, and record-lint is the natural home so CI catches it on the PR rather than a human catching it at merge. Consider also whether minting should reserve ids in a way concurrent branches cannot duplicate. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-120-local-tree-id-minting-races-under-parallel-programmes.md b/.abcd/work/issues/resolved/iss-120-local-tree-id-minting-races-under-parallel-programmes.md similarity index 77% rename from .abcd/work/issues/open/iss-120-local-tree-id-minting-races-under-parallel-programmes.md rename to .abcd/work/issues/resolved/iss-120-local-tree-id-minting-races-under-parallel-programmes.md index 52e96b0..a0b3d3a 100644 --- a/.abcd/work/issues/open/iss-120-local-tree-id-minting-races-under-parallel-programmes.md +++ b/.abcd/work/issues/resolved/iss-120-local-tree-id-minting-races-under-parallel-programmes.md @@ -7,6 +7,8 @@ category: "tech-debt" source: "user-observation" found_during: "post-merge retrospective" found_at: "internal/core/capture/capture.go" +resolution: "Local-tree id minting no longer races under parallel programmes: minting is now max+1 over the working tree AND every git ref, with loud degrade when git is unreadable and the armed record-lint detectors as the residual-window backstop." +impact: fix --- Sequential record ids are minted from the local tree, so two parallel programmes mint the same next id — both took spc-10, spc-11, iss-110 and iss-111, costing four renumber commits across the episode. This generalises iss-115 (spec ids lack a uniqueness lint): issue and intent ids have armed detectors via the shared validateIDUnique primitive, spec and ADR ids have none, and detection-after-collision is the only mitigation anywhere. The design call is whether detection everywhere is enough or minting itself should be made collision-free (id-range leases per programme, mint-at-merge), which trades away human-readable sequential ids. \ No newline at end of file From 22f50312232b7716fe6baec5b46d5d1763e50b76 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sat, 25 Jul 2026 15:19:29 +0100 Subject: [PATCH 3/3] fix: guard itd/spc id minting against integer-ceiling overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The iss allocator already refuses near math.MaxInt, but the itd and spc mints computed max+1 with no guard. A hand-crafted MaxInt filename on any scanned source — a local file or a fetched remote-tracking ref carrying itd--x.md / spc--x.md — parses to math.MaxInt with no error, so max+1 wrapped to math.MinInt and minted a negative id (itd--… / spc--…). That malformed record was persisted by WriteFileAtomic BEFORE Validate ran, and the family's mint was DoS'd while the hostile source was present. Mirror the capture allocator's ceiling guard in both nextIntentID and spec NextID: refuse loudly ("counter near the integer ceiling; refusing to allocate") before the arithmetic, so nothing is written. Also restore .abcd/record-lint.json to its original compact formatting (a prior commit reflowed it pretty via a one-off serializer, inflating a 1-line semantic change); the spec_id_unique rule addition is preserved. Assisted-by: Claude:claude-opus-4-8 --- .abcd/record-lint.json | 308 +++----------------------- internal/core/intent/create.go | 10 + internal/core/intent/overflow_test.go | 44 ++++ internal/core/spec/overflow_test.go | 44 ++++ internal/core/spec/store.go | 10 + 5 files changed, 140 insertions(+), 276 deletions(-) create mode 100644 internal/core/intent/overflow_test.go create mode 100644 internal/core/spec/overflow_test.go diff --git a/.abcd/record-lint.json b/.abcd/record-lint.json index 8fc7f0d..8609568 100644 --- a/.abcd/record-lint.json +++ b/.abcd/record-lint.json @@ -1,282 +1,38 @@ { - "roots": [ - ".abcd/development" - ], + "roots": [".abcd/development"], "banned_tokens": [ - { - "id": "moved-intents-path", - "pattern": "roadmap/intents", - "message": "intents live at intents/ (record IA); update the path", - "severity": "blocker", - "successor": "intents/", - "allow_context": [ - "historical" - ] - }, - { - "id": "flow-next-spec-prefix", - "pattern": "\\bfn-", - "message": "fn- is the flow-next spec prefix (dropped tool); abcd specs use spc-", - "severity": "blocker", - "successor": "spc-", - "allow_context": [ - "historical" - ] - }, - { - "id": "python-lint-names", - "pattern": "\\bintent_lint\\b|inventory\\.py", - "message": "Python tool name; the lint engine is internal/core/lint (Go)", - "severity": "blocker", - "successor": "internal/core/lint", - "allow_context": [ - "historical", - "spc-15" - ] - }, - { - "id": "retired-activity-dir", - "pattern": "development/activity", - "message": "development/activity is retired; working material lives in .abcd/work/ (shared) or .abcd/.work.local/ (local)", - "severity": "blocker", - "successor": ".abcd/work/ or .abcd/.work.local/", - "allow_context": [ - "historical" - ] - }, - { - "id": "retired-work-root", - "pattern": "\\.work/", - "message": ".work/ is retired as a root path; use .abcd/work/ or .abcd/.work.local/", - "severity": "blocker", - "successor": ".abcd/work/ or .abcd/.work.local/", - "allow_context": [ - "historical", - "\\.abcd/\\.work\\.local" - ] - }, - { - "id": "retired-scripts-dir", - "pattern": "scripts/abcd/", - "message": "scripts/abcd/ is retired; behaviour lives in the Go binary (internal/core)", - "severity": "blocker", - "successor": "internal/core", - "allow_context": [ - "historical" - ] - }, - { - "id": "flow-next-dir", - "pattern": "\\.flow/", - "message": ".flow/ belongs to the dropped flow-next tool", - "severity": "blocker", - "successor": ".abcd/", - "allow_context": [ - "historical" - ] - }, - { - "id": "retired-workflow-files", - "pattern": "workflows/lint\\.yml|lint-corpus\\.yml", - "message": "retired workflow file; CI is .github/workflows/ci.yml", - "severity": "blocker", - "successor": ".github/workflows/ci.yml", - "allow_context": [ - "historical" - ] - }, - { - "id": "retired-since-staged-flag", - "pattern": "--since-staged", - "message": "--since-staged is a retired flag spelling", - "severity": "blocker", - "successor": "--since", - "allow_context": [ - "historical" - ] - }, - { - "id": "dev-repo-name", - "pattern": "\\babcdDev\\b", - "message": "single-repo (adr-28): use 'the frozen reference implementation' / abcd-cli", - "severity": "blocker", - "successor": "abcd-cli", - "allow_context": [ - "historical" - ] - }, - { - "id": "workspaces-json", - "pattern": "workspaces\\.json", - "message": "the workspace concept is dropped; only a negation may mention it", - "severity": "blocker", - "successor": "index.json (the sole user-scope history registry, keyed by root_commit)", - "allow_context": [ - "no ", - "not ", - "there is no", - "sole user-scope", - "dropped", - "without a" - ] - }, - { - "id": "meta-json-file", - "pattern": "\\bmeta\\.json\\b", - "message": "meta.json is abolished as a repo file; setup metadata is config.json[\"meta\"]", - "severity": "blocker", - "successor": "config.json[\"meta\"]", - "allow_context": [ - "history", - "root-sha", - "root-commit", - "per-root", - "index.json", - "identity", - "no ", - "there is no", - "separate", - "abolished", - "not ", - "without" - ] - }, - { - "id": "presidio-default", - "pattern": "Presidio", - "message": "native Go PII engine is the default; Presidio is only one opt-in scanner adapter", - "severity": "warn", - "successor": "the native Go PII engine", - "allow_context": [ - "gitleaks", - "opt-in", - "adapter", - "seam" - ] - } + {"id":"moved-intents-path","pattern":"roadmap/intents","message":"intents live at intents/ (record IA); update the path","severity":"blocker","successor":"intents/","allow_context":["historical"]}, + {"id":"flow-next-spec-prefix","pattern":"\\bfn-","message":"fn- is the flow-next spec prefix (dropped tool); abcd specs use spc-","severity":"blocker","successor":"spc-","allow_context":["historical"]}, + {"id":"python-lint-names","pattern":"\\bintent_lint\\b|inventory\\.py","message":"Python tool name; the lint engine is internal/core/lint (Go)","severity":"blocker","successor":"internal/core/lint","allow_context":["historical","spc-15"]}, + {"id":"retired-activity-dir","pattern":"development/activity","message":"development/activity is retired; working material lives in .abcd/work/ (shared) or .abcd/.work.local/ (local)","severity":"blocker","successor":".abcd/work/ or .abcd/.work.local/","allow_context":["historical"]}, + {"id":"retired-work-root","pattern":"\\.work/","message":".work/ is retired as a root path; use .abcd/work/ or .abcd/.work.local/","severity":"blocker","successor":".abcd/work/ or .abcd/.work.local/","allow_context":["historical","\\.abcd/\\.work\\.local"]}, + {"id":"retired-scripts-dir","pattern":"scripts/abcd/","message":"scripts/abcd/ is retired; behaviour lives in the Go binary (internal/core)","severity":"blocker","successor":"internal/core","allow_context":["historical"]}, + {"id":"flow-next-dir","pattern":"\\.flow/","message":".flow/ belongs to the dropped flow-next tool","severity":"blocker","successor":".abcd/","allow_context":["historical"]}, + {"id":"retired-workflow-files","pattern":"workflows/lint\\.yml|lint-corpus\\.yml","message":"retired workflow file; CI is .github/workflows/ci.yml","severity":"blocker","successor":".github/workflows/ci.yml","allow_context":["historical"]}, + {"id":"retired-since-staged-flag","pattern":"--since-staged","message":"--since-staged is a retired flag spelling","severity":"blocker","successor":"--since","allow_context":["historical"]}, + {"id":"dev-repo-name","pattern":"\\babcdDev\\b","message":"single-repo (adr-28): use 'the frozen reference implementation' / abcd-cli","severity":"blocker","successor":"abcd-cli","allow_context":["historical"]}, + {"id":"workspaces-json","pattern":"workspaces\\.json","message":"the workspace concept is dropped; only a negation may mention it","severity":"blocker","successor":"index.json (the sole user-scope history registry, keyed by root_commit)","allow_context":["no ","not ","there is no","sole user-scope","dropped","without a"]}, + {"id":"meta-json-file","pattern":"\\bmeta\\.json\\b","message":"meta.json is abolished as a repo file; setup metadata is config.json[\"meta\"]","severity":"blocker","successor":"config.json[\"meta\"]","allow_context":["history","root-sha","root-commit","per-root","index.json","identity","no ","there is no","separate","abolished","not ","without"]}, + {"id":"presidio-default","pattern":"Presidio","message":"native Go PII engine is the default; Presidio is only one opt-in scanner adapter","severity":"warn","successor":"the native Go PII engine","allow_context":["gitleaks","opt-in","adapter","seam"]} ], "rules": { - "no_git_metadata": { - "enabled": true, - "severity": "blocker", - "fields": [ - "created", - "updated", - "author", - "maintained_by", - "version" - ] - }, - "links_resolve": { - "enabled": true, - "severity": "blocker" - }, - "no_brittle_line_refs": { - "enabled": true, - "severity": "warn" - }, - "directory_coverage": { - "enabled": true, - "severity": "warn", - "exempt": [] - }, - "intent_lifecycle": { - "enabled": true, - "severity": "blocker", - "intents_dir": "intents" - }, - "intent_impact_valid": { - "enabled": true, - "severity": "blocker", - "intents_dir": "intents" - }, - "issue_id_unique": { - "enabled": true, - "severity": "blocker", - "issues_dir": ".abcd/work/issues" - }, - "issue_impact_valid": { - "enabled": true, - "severity": "blocker", - "issues_dir": ".abcd/work/issues" - }, - "spec_lifecycle": { - "enabled": true, - "severity": "blocker", - "specs_dir": "specs", - "intents_dir": "intents" - }, - "spec_id_unique": { - "enabled": true, - "severity": "blocker", - "specs_dir": "specs", - "intents_dir": "intents" - }, - "persona_registry": { - "enabled": true, - "severity": "blocker", - "registry": ".abcd/development/personas.json" - }, - "context_status_free": { - "enabled": true, - "severity": "blocker", - "target": ".abcd/work/CONTEXT.md" - }, - "surface_coverage": { - "enabled": true, - "severity": "blocker", - "commands_dir": "commands/abcd", - "skills_dir": "skills", - "registry": ".abcd/development/brief/04-surfaces/README.md" - }, - "receipt_gate": { - "enabled": false, - "severity": "blocker", - "receipts_dir": ".abcd/work/reviews", - "required_gates": [ - "docs-currency-reviewer", - "iss35-brief-surface-crosscheck" - ] - }, - "gate_lockstep": { - "enabled": true, - "severity": "blocker", - "runbook": ".abcd/development/release-gate/README.md", - "workflow": ".github/workflows/release.yml", - "job": "verify", - "ignore_steps": [ - "Check out the pushed commit", - "Set up Go" - ], - "min_gates": 9 - }, - "forbidden_synonyms": { - "enabled": true, - "severity": "blocker", - "glossary_dir": ".abcd/development/brief/glossary", - "enforce": [ - "epic" - ], - "exempt_prefixes": [ - ".abcd/development/research/", - ".abcd/development/decisions/", - ".abcd/development/plans/", - ".abcd/development/intents/shipped/", - ".abcd/development/intents/superseded/" - ], - "allow_context": [ - "epic-review", - "epic->spec", - "epic\u2192spec", - "epic-to-spec" - ] - } + "no_git_metadata": {"enabled": true, "severity": "blocker", "fields": ["created","updated","author","maintained_by","version"]}, + "links_resolve": {"enabled": true, "severity": "blocker"}, + "no_brittle_line_refs": {"enabled": true, "severity": "warn"}, + "directory_coverage": {"enabled": true, "severity": "warn", "exempt": []}, + "intent_lifecycle": {"enabled": true, "severity": "blocker", "intents_dir": "intents"}, + "intent_impact_valid": {"enabled": true, "severity": "blocker", "intents_dir": "intents"}, + "issue_id_unique": {"enabled": true, "severity": "blocker", "issues_dir": ".abcd/work/issues"}, + "issue_impact_valid": {"enabled": true, "severity": "blocker", "issues_dir": ".abcd/work/issues"}, + "spec_lifecycle": {"enabled": true, "severity": "blocker", "specs_dir": "specs", "intents_dir": "intents"}, + "spec_id_unique": {"enabled": true, "severity": "blocker", "specs_dir": "specs", "intents_dir": "intents"}, + "persona_registry": {"enabled": true, "severity": "blocker", "registry": ".abcd/development/personas.json"}, + "context_status_free": {"enabled": true, "severity": "blocker", "target": ".abcd/work/CONTEXT.md"}, + "surface_coverage": {"enabled": true, "severity": "blocker", "commands_dir": "commands/abcd", "skills_dir": "skills", "registry": ".abcd/development/brief/04-surfaces/README.md"}, + "receipt_gate": {"enabled": false, "severity": "blocker", "receipts_dir": ".abcd/work/reviews", "required_gates": ["docs-currency-reviewer", "iss35-brief-surface-crosscheck"]}, + "gate_lockstep": {"enabled": true, "severity": "blocker", "runbook": ".abcd/development/release-gate/README.md", "workflow": ".github/workflows/release.yml", "job": "verify", "ignore_steps": ["Check out the pushed commit", "Set up Go"], "min_gates": 9}, + "forbidden_synonyms": {"enabled": true, "severity": "blocker", "glossary_dir": ".abcd/development/brief/glossary", "enforce": ["epic"], "exempt_prefixes": [".abcd/development/research/", ".abcd/development/decisions/", ".abcd/development/plans/", ".abcd/development/intents/shipped/", ".abcd/development/intents/superseded/"], "allow_context": ["epic-review", "epic->spec", "epic→spec", "epic-to-spec"]} }, - "exempt_paths": [ - ".abcd/development/research/", - ".abcd/development/intents/superseded/" - ], - "exempt_if_status": [ - "superseded" - ] + "exempt_paths": [".abcd/development/research/", ".abcd/development/intents/superseded/"], + "exempt_if_status": ["superseded"] } diff --git a/internal/core/intent/create.go b/internal/core/intent/create.go index e2aa53d..92cc1e6 100644 --- a/internal/core/intent/create.go +++ b/internal/core/intent/create.go @@ -2,6 +2,7 @@ package intent import ( "fmt" + "math" "os" "path/filepath" "regexp" @@ -158,6 +159,15 @@ func nextIntentID(repoRoot string) (id, mintWarning string, err error) { if scan.Max > max { max = scan.Max } + // Guard the max+1 below against int overflow: a hand-crafted MaxInt itd-N + // (a local file or a fetched remote-tracking ref carrying itd--x.md) + // parses to math.MaxInt with no error, so max+1 would wrap to math.MinInt and + // mint itd--9223372036854775808 — a malformed draft WriteFileAtomic persists + // before Validate runs, plus a mint DoS for the family. Refuse clearly instead, + // mirroring the capture allocator's ceiling guard. + if max >= math.MaxInt { + return "", "", fmt.Errorf("intent: itd-N counter near the integer ceiling (highest observed %d); refusing to allocate", max) + } return fmt.Sprintf("itd-%d", max+1), scan.Warning(), nil } diff --git a/internal/core/intent/overflow_test.go b/internal/core/intent/overflow_test.go new file mode 100644 index 0000000..ee34f0c --- /dev/null +++ b/internal/core/intent/overflow_test.go @@ -0,0 +1,44 @@ +package intent + +import ( + "os" + "path/filepath" + "strconv" + "testing" +) + +// TestCreateFromTextRefusesAtIntegerCeiling proves the itd-N mint refuses cleanly +// when the observed max is at the integer ceiling, instead of wrapping max+1 to a +// negative id and persisting a malformed draft (a scanned ref or a local file +// carrying itd--x.md is the attack path). It must return a loud error and +// write NO new draft. +func TestCreateFromTextRefusesAtIntegerCeiling(t *testing.T) { + root := t.TempDir() + maxID := "itd-" + strconv.Itoa(int(^uint(0)>>1)) // itd- + draftsDir := filepath.Join(root, IntentsRelDir, BucketDrafts) + if err := os.MkdirAll(draftsDir, 0o755); err != nil { + t.Fatal(err) + } + ceiling := filepath.Join(draftsDir, maxID+"-huge.md") + if err := os.WriteFile(ceiling, []byte("---\nid: "+maxID+"\n---\n# huge\n"), 0o644); err != nil { + t.Fatal(err) + } + + _, _, err := CreateFromText(root, "a fresh intent worth shipping", "") + if err == nil { + t.Fatal("CreateFromText must refuse at the integer ceiling, not mint a wrapped negative id") + } + + // No new draft may have been written — only the ceiling file remains. + entries, rerr := os.ReadDir(draftsDir) + if rerr != nil { + t.Fatal(rerr) + } + if len(entries) != 1 || entries[0].Name() != filepath.Base(ceiling) { + var names []string + for _, e := range entries { + names = append(names, e.Name()) + } + t.Fatalf("a draft was written despite the ceiling refusal: %v", names) + } +} diff --git a/internal/core/spec/overflow_test.go b/internal/core/spec/overflow_test.go new file mode 100644 index 0000000..2171ff4 --- /dev/null +++ b/internal/core/spec/overflow_test.go @@ -0,0 +1,44 @@ +package spec + +import ( + "os" + "path/filepath" + "strconv" + "testing" +) + +// TestCreateRefusesAtIntegerCeiling proves the spc-N mint refuses cleanly when +// the observed max is at the integer ceiling, instead of wrapping max+1 to a +// negative id and persisting a malformed record (a scanned ref or a local file +// carrying spc--x.md is the attack path). It must return a loud error and +// write NO new spec file. +func TestCreateRefusesAtIntegerCeiling(t *testing.T) { + root := t.TempDir() + maxID := "spc-" + strconv.Itoa(int(^uint(0)>>1)) // spc- + openDir := filepath.Join(root, SpecsRelDir, StatusOpen) + if err := os.MkdirAll(openDir, 0o755); err != nil { + t.Fatal(err) + } + ceiling := filepath.Join(openDir, maxID+"-huge.md") + if err := os.WriteFile(ceiling, []byte("---\nid: "+maxID+"\nslug: huge\nintent: itd-1\n---\n# huge\n"), 0o644); err != nil { + t.Fatal(err) + } + + _, _, err := Create(root, "itd-2", "new-one") + if err == nil { + t.Fatal("Create must refuse at the integer ceiling, not mint a wrapped negative id") + } + + // No new spec file may have been written — only the ceiling file remains. + entries, rerr := os.ReadDir(openDir) + if rerr != nil { + t.Fatal(rerr) + } + if len(entries) != 1 || entries[0].Name() != filepath.Base(ceiling) { + var names []string + for _, e := range entries { + names = append(names, e.Name()) + } + t.Fatalf("a spec file was written despite the ceiling refusal: %v", names) + } +} diff --git a/internal/core/spec/store.go b/internal/core/spec/store.go index 6c0e280..b6ebb34 100644 --- a/internal/core/spec/store.go +++ b/internal/core/spec/store.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "io" + "math" "os" "path/filepath" "strings" @@ -130,6 +131,15 @@ func NextID(repoRoot string) (id, mintWarning string, err error) { if scan.Max > max { max = scan.Max } + // Guard the max+1 below against int overflow: a hand-crafted MaxInt spc-N + // (a local file or a fetched remote-tracking ref carrying spc--x.md) + // parses to math.MaxInt with no error, so max+1 would wrap to math.MinInt and + // mint spc--9223372036854775808 — a malformed record WriteFileAtomic persists + // before Validate runs, plus a mint DoS for the family. Refuse clearly instead, + // mirroring the capture allocator's ceiling guard. + if max >= math.MaxInt { + return "", "", fmt.Errorf("spec: spc-N counter near the integer ceiling (highest observed %d); refusing to allocate", max) + } return fmt.Sprintf("spc-%d", max+1), scan.Warning(), nil }