diff --git a/.abcd/record-lint.json b/.abcd/record-lint.json index a335e02..8609568 100644 --- a/.abcd/record-lint.json +++ b/.abcd/record-lint.json @@ -25,6 +25,7 @@ "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"}, 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/.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 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..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" @@ -11,6 +12,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 +38,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 +51,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 +96,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 +123,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 +155,20 @@ 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 + } + // 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 } // 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/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/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/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/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..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" @@ -11,6 +12,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 +92,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 +122,25 @@ 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 + } + // 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 } // maxIntentSpecNum returns the highest N across every intent's spec_id @@ -173,12 +199,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 +215,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 +241,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) })