fix: serialize issue-ledger transitions + validate ForceID (iss-71)

.abcd/work/issues/open/iss-71-capture-concurrency-and-forceid.md → .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.

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
 

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$`)
@@ -65,56 +66,79 @@ 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) {
+	// 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 != "" {
+			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

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

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) {