From e83edd98a2739b68d515557599bb32c24635fe3c Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 10:02:37 +0100 Subject: [PATCH 1/3] refactor: extract withLedgerLock from reservePath (capture) Lift the flock-acquire/release primitive (ensure dirs, open lock with the symlink/regular-file guards, acquire within lockTimeout, release) out of reservePath into a reusable withLedgerLock helper so a second ledger mutator (status transitions) can serialize on the same lock without a duplicate copy. Behaviour-preserving; the capture suite stays green. Assisted-by: Claude:claude-opus-4-8 --- internal/core/capture/alloc.go | 81 ++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 32 deletions(-) diff --git a/internal/core/capture/alloc.go b/internal/core/capture/alloc.go index b036b8b8..e252bc54 100644 --- a/internal/core/capture/alloc.go +++ b/internal/core/capture/alloc.go @@ -65,56 +65,73 @@ func safeMkdirLeaf(target string) error { return nil } -// 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) { +// withLedgerLock runs fn while holding the exclusive allocator flock, so every +// ledger mutation — id allocation AND status transitions — serializes on one +// lock. It creates the ledger dirs, opens the lock with the symlink/regular-file +// guards, acquires the flock within lockTimeout, runs fn, then releases. +func withLedgerLock(issuesRoot string, fn func() error) error { if err := ensureLedgerDirs(issuesRoot); err != nil { - return "", "", err + return err } lockPath := filepath.Join(issuesRoot, lockFilename) lockFd, err := safeOpenLockFd(lockPath) if err != nil { - return "", "", err + return err } defer syscall.Close(lockFd) if err := acquireFlock(lockFd, lockTimeout); err != nil { - return "", "", err + return err } defer syscall.Flock(lockFd, syscall.LOCK_UN) - if forceID != "" { - if issPresent(issuesRoot, forceID) { - return "", "", fmt.Errorf("%w: %s already exists in the ledger", ErrDuplicateIssueID, forceID) - } - target := filepath.Join(issuesRoot, "open", forceID+"-"+slug+".md") - fd, cErr := createPlaceholder(target) - if cErr != nil { - if os.IsExist(cErr) { - return "", "", fmt.Errorf("%w: %s appeared between scan and create", ErrDuplicateIssueID, forceID) + return fn() +} + +// 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) { + var resID, resTarget string + err := withLedgerLock(issuesRoot, func() error { + if forceID != "" { + if issPresent(issuesRoot, forceID) { + return fmt.Errorf("%w: %s already exists in the ledger", ErrDuplicateIssueID, forceID) } - return "", "", cErr + target := filepath.Join(issuesRoot, "open", forceID+"-"+slug+".md") + fd, cErr := createPlaceholder(target) + if cErr != nil { + if os.IsExist(cErr) { + return fmt.Errorf("%w: %s appeared between scan and create", ErrDuplicateIssueID, forceID) + } + return cErr + } + syscall.Close(fd) + resID, resTarget = forceID, target + return nil } - syscall.Close(fd) - return forceID, target, nil - } - maxN := maxIssN(issuesRoot) - for attempt := 0; attempt < placeholderRetryBudget; attempt++ { - issID := fmt.Sprintf("iss-%d", maxN+1+attempt) - target := filepath.Join(issuesRoot, "open", issID+"-"+slug+".md") - fd, cErr := createPlaceholder(target) - if cErr != nil { - if os.IsExist(cErr) { - continue + maxN := maxIssN(issuesRoot) + for attempt := 0; attempt < placeholderRetryBudget; attempt++ { + issID := fmt.Sprintf("iss-%d", maxN+1+attempt) + target := filepath.Join(issuesRoot, "open", issID+"-"+slug+".md") + fd, cErr := createPlaceholder(target) + if cErr != nil { + if os.IsExist(cErr) { + continue + } + return cErr } - return "", "", cErr + syscall.Close(fd) + resID, resTarget = issID, target + return nil } - syscall.Close(fd) - return issID, target, nil + return fmt.Errorf("%w: could not allocate iss-N after %d retries", ErrAllocatorContention, placeholderRetryBudget) + }) + if err != nil { + return "", "", err } - return "", "", fmt.Errorf("%w: could not allocate iss-N after %d retries", ErrAllocatorContention, placeholderRetryBudget) + return resID, resTarget, nil } // safeOpenLockFd opens the allocator lock with O_NOFOLLOW and verifies it is a From e39aa8b1fb4f2c2eb239a4b902eafb0f8cefdab2 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 10:06:32 +0100 Subject: [PATCH 2/3] fix: serialize ledger transitions + validate ForceID (iss-71) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ledger-mutation-safety fixes: - C4: abcd capture resolve/wontfix ran their find->read->move with no ledger lock (only id allocation locked). Two concurrent conflicting transitions on one issue (a resolve and a wontfix) could both pass commitTransition's checksum re-read before either removed the source, landing the issue in TWO status dirs — permanent split-brain (findIssue then always ErrDuplicateIssueID). The find->move critical section now runs under withLedgerLock, the same lock allocation takes, so the second transition sees the issue already moved and conflicts cleanly. Deterministic test: with the lock held, Resolve returns ErrAllocatorContention and leaves the issue in open/. - P13: reservePath built a path from and O_EXCL-created a placeholder for a migrator-supplied ForceID before validating it. A traversal id (../../evil) escaped the ledger open/ dir (swept only on the later validation failure). ForceID is now validated against ^iss-[0-9]+$ before any filesystem op. Assisted-by: Claude:claude-opus-4-8 --- CHANGELOG.md | 6 +++ internal/core/capture/alloc.go | 11 +++- internal/core/capture/workflow.go | 69 +++++++++++++++----------- internal/core/capture/workflow_test.go | 64 ++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4985c7dd..ef4dd1e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -152,6 +152,12 @@ called out in a **Breaking** section. misclassifies a valid UTF-8 file as binary when a multibyte rune straddles the 8 KB boundary (which would have skipped scanning it), and a bundle file that cannot be read is now surfaced in `unscanned` rather than silently dropped. +- **Issue-ledger transition hardening** (iss-71). `abcd capture resolve`/`wontfix` + now run their find→move under the same ledger lock id allocation uses, so two + concurrent conflicting transitions on one issue can no longer land it in two + status directories at once. A migrator-supplied `ForceID` is validated against + the `iss-N` shape before any path is built, so a traversal id cannot touch the + filesystem outside the ledger. ## [v0.1.0] - 2026-07-07 diff --git a/internal/core/capture/alloc.go b/internal/core/capture/alloc.go index e252bc54..c5a6a3f6 100644 --- a/internal/core/capture/alloc.go +++ b/internal/core/capture/alloc.go @@ -18,8 +18,9 @@ const placeholderRetryBudget = 8 // sweep removes it. const orphanAgeThreshold = 60 * time.Second -// lockTimeout is the default flock acquisition budget. -const lockTimeout = 5 * time.Second +// lockTimeout is the default flock acquisition budget. A var (not const) so a +// test can shorten it to exercise contention without a multi-second wait. +var lockTimeout = 5 * time.Second var rePlaceholderName = regexp.MustCompile(`^iss-[0-9]+(-[a-z0-9]+(-[a-z0-9]+)*)?\.md$`) var reMaxIssN = regexp.MustCompile(`^iss-([0-9]+)(?:-[a-z0-9-]+)?\.md$`) @@ -92,6 +93,12 @@ func withLedgerLock(issuesRoot string, fn func() error) error { // 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) { + // 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. + if forceID != "" && !reIssID.MatchString(forceID) { + return "", "", fmt.Errorf("%w: ForceID %q must match ^iss-[0-9]+$", ErrPathUnsafe, forceID) + } var resID, resTarget string err := withLedgerLock(issuesRoot, func() error { if forceID != "" { diff --git a/internal/core/capture/workflow.go b/internal/core/capture/workflow.go index 5da14259..068ae45c 100644 --- a/internal/core/capture/workflow.go +++ b/internal/core/capture/workflow.go @@ -142,39 +142,52 @@ func transition(repoRoot, issuesRoot, issID, field, note string, target State) ( return TransitionResult{}, fmt.Errorf("%s must be a non-empty string", field) } - src, status, err := findIssue(ir, issID) - if err != nil { - return TransitionResult{}, err - } - if status != StateOpen { - return TransitionResult{}, fmt.Errorf("%w: %s already in %s", ErrTransitionConflict, issID, status) - } + // The find→read→move critical section runs under the ledger lock, the SAME + // lock id allocation takes, so two concurrent conflicting transitions (a + // resolve and a wontfix on one issue) serialize: the second sees the issue + // already moved out of open/ and conflicts, instead of both passing the + // checksum re-read and landing the issue in two status dirs (split-brain). + var result TransitionResult + err = withLedgerLock(ir, func() error { + src, status, err := findIssue(ir, issID) + if err != nil { + return err + } + if status != StateOpen { + return fmt.Errorf("%w: %s already in %s", ErrTransitionConflict, issID, status) + } - content, checksum, err := readWithChecksum(src) - if err != nil { - return TransitionResult{}, err - } - newContent, err := setScalarField(content, field, note) - if err != nil { - return TransitionResult{}, err - } + content, checksum, err := readWithChecksum(src) + if err != nil { + return err + } + newContent, err := setScalarField(content, field, note) + if err != nil { + return err + } - dst := filepath.Join(ir, statusDirName[target], filepath.Base(src)) - fm, _, err := parseFrontmatterAndBody(newContent) - if err != nil { - return TransitionResult{}, err - } - if err := validateStrict(fm); err != nil { - return TransitionResult{}, err - } - if err := validateInvariants(fm, target, dst); err != nil { - return TransitionResult{}, err - } + dst := filepath.Join(ir, statusDirName[target], filepath.Base(src)) + fm, _, err := parseFrontmatterAndBody(newContent) + if err != nil { + return err + } + if err := validateStrict(fm); err != nil { + return err + } + if err := validateInvariants(fm, target, dst); err != nil { + return err + } - if err := commitTransition(src, dst, newContent, checksum); err != nil { + if err := commitTransition(src, dst, newContent, checksum); err != nil { + return err + } + result = TransitionResult{ID: issID, Path: dst, FromStatus: StateOpen, ToStatus: target} + return nil + }) + if err != nil { return TransitionResult{}, err } - return TransitionResult{ID: issID, Path: dst, FromStatus: StateOpen, ToStatus: target}, nil + return result, nil } // commitTransition re-verifies the source's checksum, writes the destination diff --git a/internal/core/capture/workflow_test.go b/internal/core/capture/workflow_test.go index 9a206675..9143ee33 100644 --- a/internal/core/capture/workflow_test.go +++ b/internal/core/capture/workflow_test.go @@ -6,9 +6,73 @@ import ( "path/filepath" "strconv" "strings" + "syscall" "testing" + "time" ) +// TestTransitionSerializesOnLedgerLock (iss-71 C4) proves a status transition +// acquires the same allocator flock id allocation uses, so two concurrent +// conflicting transitions cannot split-brain an issue across two status dirs. +// It holds the ledger lock externally and asserts Resolve fails to acquire it +// (rather than proceeding lock-free, which is how the split-brain arises). +func TestTransitionSerializesOnLedgerLock(t *testing.T) { + repo, ir := ledger(t) + res, err := Capture(CaptureRequest{ + RepoRoot: repo, IssuesRoot: ir, Text: "b", Severity: SeverityMinor, + Category: "bug", Source: "user-observation", FoundDuring: "t", Slug: "note", + }) + if err != nil { + t.Fatal(err) + } + + // Hold the allocator lock on a separate fd, as a competing mutator would. + lockPath := filepath.Join(ir, lockFilename) + fd, err := syscall.Open(lockPath, syscall.O_CREAT|syscall.O_RDWR|syscall.O_NOFOLLOW, 0o644) + if err != nil { + t.Fatal(err) + } + defer syscall.Close(fd) + if err := syscall.Flock(fd, syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + t.Fatalf("could not take the ledger lock for the test: %v", err) + } + defer syscall.Flock(fd, syscall.LOCK_UN) + + orig := lockTimeout + lockTimeout = 200 * time.Millisecond + defer func() { lockTimeout = orig }() + + _, err = Resolve(ResolveRequest{RepoRoot: repo, IssuesRoot: ir, ID: res.ID, Resolution: "x"}) + if !errors.Is(err, ErrAllocatorContention) { + t.Fatalf("transition must serialize on the ledger lock (expect contention while held), got err=%v", err) + } + // The issue must remain untouched in open/ — no move happened. + if _, status, ferr := findIssue(ir, res.ID); ferr != nil || status != StateOpen { + t.Fatalf("a lock-blocked transition must not move the issue: status=%s err=%v", status, ferr) + } +} + +// TestReservePathRejectsUnsafeForceID (iss-71 P13) proves reservePath validates +// a ForceID against the iss-N shape BEFORE building any path or creating a +// placeholder, so a traversal id cannot touch the filesystem outside the ledger. +func TestReservePathRejectsUnsafeForceID(t *testing.T) { + repo, ir := ledger(t) + if err := ensureLedgerDirs(ir); err != nil { + 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) + if err == nil { + t.Fatalf("reservePath must reject unsafe ForceID %q before any fs op (got id=%q target=%q)", bad, id, target) + } + } + // The `../../evil` id would have escaped to repo/.abcd/work/evil-note.md. + escaped := filepath.Join(repo, ".abcd", "work", "evil-note.md") + if _, err := os.Lstat(escaped); err == nil { + t.Fatalf("a traversal ForceID created a file outside the ledger: %s", escaped) + } +} + // ledger returns (repoRoot, issuesRoot) rooted in a temp dir, avoiding git // discovery by supplying both explicitly (resolveRoots contract B). func ledger(t *testing.T) (string, string) { From ddab8568197656a4b635941fc564ebb680d76ea2 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 10:10:08 +0100 Subject: [PATCH 3/3] chore: resolve iss-71 in the ledger Ledger move trailing the fix in this branch, so resolution lands atomically on merge (no separate chore PR). Assisted-by: Claude:claude-opus-4-8 --- .../{open => resolved}/iss-71-capture-concurrency-and-forceid.md | 1 + 1 file changed, 1 insertion(+) rename .abcd/work/issues/{open => resolved}/iss-71-capture-concurrency-and-forceid.md (74%) diff --git a/.abcd/work/issues/open/iss-71-capture-concurrency-and-forceid.md b/.abcd/work/issues/resolved/iss-71-capture-concurrency-and-forceid.md similarity index 74% rename from .abcd/work/issues/open/iss-71-capture-concurrency-and-forceid.md rename to .abcd/work/issues/resolved/iss-71-capture-concurrency-and-forceid.md index 5f563969..d5ca762c 100644 --- a/.abcd/work/issues/open/iss-71-capture-concurrency-and-forceid.md +++ b/.abcd/work/issues/resolved/iss-71-capture-concurrency-and-forceid.md @@ -7,6 +7,7 @@ category: "bug" source: "agent-finding" found_during: "clean-slate-sweep" found_at: "internal/core/capture/workflow.go" +resolution: "Ledger transitions now serialize under withLedgerLock (no split-brain across status dirs); migrator ForceID validated against iss-N before any fs op. Refactor (withLedgerLock extraction) + fix in separate commits. ruthless SHIP + security PASS; full capture pkg green under -race." --- capture ledger concurrency + unvalidated ForceID: transition() (resolve/wontfix) takes no flock (only reservePath does) so two concurrent conflicting transitions on the same iss split-brain it across two status dirs — permanent corruption, findIssue then always ErrDuplicateIssueID (workflow.go:129, C4); reservePath builds a path from and O_EXCL-creates a placeholder for ForceID BEFORE validateStrict checks it against the iss-id regex, a pre-validation filesystem touch/traversal (alloc.go:91, P13). Detector: hold the ledger flock across findIssue+commitTransition; reject a non-matching ForceID before any filesystem op. Corpus: C4, P13. \ No newline at end of file