diff --git a/.codearbiter/gate-events.log b/.codearbiter/gate-events.log index 9eaeec2c..5e51005b 100644 --- a/.codearbiter/gate-events.log +++ b/.codearbiter/gate-events.log @@ -1101,3 +1101,12 @@ Claude-Session: https://claude.ai/code/session_015ZDVU1BzUqnnPVHbZ397bX') stages [2026-07-28T13:40:04Z] REMIND [H-15] host=claude hook=post-write-edit.py | CI/CD workflow changed. Dispatch security-reviewer before merging (it reviews workflow/secrets/permissions exposure). Advisory — not a commit block. [2026-07-28T13:40:09Z] REMIND [H-12] host=claude hook=post-write-edit.py | .github/workflows/ci.yml is governed by ADR-0014-pi-host-authentication-and-fail-closed-tool-boundary (Treat Pi host authentication as opaque runtime state and fail closed on unknown tools). If this change contradicts it, route to /ca:reconcile or /ca:adr — do not drift silently. [2026-07-28T13:40:09Z] REMIND [H-15] host=claude hook=post-write-edit.py | CI/CD workflow changed. Dispatch security-reviewer before merging (it reviews workflow/secrets/permissions exposure). Advisory — not a commit block. +[2026-07-28T22:21:26Z] REMIND [H-12] host=claude hook=post-write-edit.py | plugins/ca/tools/farm.ts is governed by ADR-0002-plan-json-trusted-operator-shell-input (Treat plan.json gate commands and FARM_MUTATION_CMD as trusted operator-authored shell input). If this change contradicts it, route to /ca:reconcile or /ca:adr — do not drift silently. +[2026-07-28T22:21:35Z] REMIND [H-12] host=claude hook=post-write-edit.py | plugins/ca/tools/farm.ts is governed by ADR-0002-plan-json-trusted-operator-shell-input (Treat plan.json gate commands and FARM_MUTATION_CMD as trusted operator-authored shell input). If this change contradicts it, route to /ca:reconcile or /ca:adr — do not drift silently. +[2026-07-28T22:21:53Z] REMIND [H-12] host=claude hook=post-write-edit.py | plugins/ca/tools/farm.ts is governed by ADR-0002-plan-json-trusted-operator-shell-input (Treat plan.json gate commands and FARM_MUTATION_CMD as trusted operator-authored shell input). If this change contradicts it, route to /ca:reconcile or /ca:adr — do not drift silently. +[2026-07-28T22:24:18Z] REMIND [H-12] host=claude hook=post-write-edit.py | plugins/ca/tools/farm.ts is governed by ADR-0002-plan-json-trusted-operator-shell-input (Treat plan.json gate commands and FARM_MUTATION_CMD as trusted operator-authored shell input). If this change contradicts it, route to /ca:reconcile or /ca:adr — do not drift silently. +[2026-07-28T22:24:25Z] REMIND [H-12] host=claude hook=post-write-edit.py | plugins/ca/tools/farm.ts is governed by ADR-0002-plan-json-trusted-operator-shell-input (Treat plan.json gate commands and FARM_MUTATION_CMD as trusted operator-authored shell input). If this change contradicts it, route to /ca:reconcile or /ca:adr — do not drift silently. +[2026-07-28T22:28:36Z] BLOCK [H-01] host=claude hook=pre-bash.py | Direct commit to main is prohibited (ORCHESTRATOR §3). Create a feature branch. +[2026-07-28T22:28:54Z] BLOCK [H-09b] host=claude hook=pre-bash.py | This commit introduces crypto/TLS changes, but no security-gate pass is recorded (.codearbiter/.markers/security-gate-passed). Run the crypto-compliance gate (it records the pass), then commit. To bypass a security gate, /override requires its heavier security-acknowledgement path. +[2026-07-28T22:32:50Z] REMIND [H-09] host=claude hook=post-write-edit.py | Crypto/TLS pattern detected. Run the crypto-compliance check + dispatch auth-crypto-reviewer (no MD5/SHA1/DES/3DES/RC2/RC4/Blowfish; do not disable TLS verification). The commit will block until the gate records a pass. +[2026-07-28T22:33:11Z] BLOCK [H-09b] host=claude hook=pre-bash.py | This commit introduces crypto/TLS changes, but no security-gate pass is recorded (.codearbiter/.markers/security-gate-passed). Run the crypto-compliance gate (it records the pass), then commit. To bypass a security gate, /override requires its heavier security-acknowledgement path. diff --git a/.gitattributes b/.gitattributes index 767e897c..ded94413 100644 --- a/.gitattributes +++ b/.gitattributes @@ -34,3 +34,29 @@ docs/**/*.md text eol=lf # why nothing caught it. .github/workflows/*.yml text eol=lf .github/actions/**/*.yml text eol=lf + +# TypeScript/JavaScript/JSON across the plugin and site trees. #515 showed this +# is not cosmetic. An editor pass on Windows flipped farm.ts and +# farm.unit.test.ts to CRLF, which made git read the ENTIRE file as added — and +# the H-09b security backstop scans ADDED lines for crypto and secret patterns. +# Every pre-existing hash-construction call and every credential-shaped test +# fixture in those files therefore counted as newly introduced, and a +# concurrency fix touching no such code was blocked demanding a security pass. +# +# (Naming the actual API here would trip that same scanner on this comment — +# which it did, once, while this note was being written.) +# +# A whole-file diff does not just bury the change; it makes every line-scanning +# gate in this repo fire on code nobody wrote. README.md was already immune +# because `/*.md` above pins it, which is exactly why its diff stayed two lines +# while farm.ts's was six thousand. +# +# Verified before adding: no committed file under these globs is CRLF today, so +# this normalizes nothing retroactively and only prevents the next drift. +plugins/**/*.ts text eol=lf +plugins/**/*.js text eol=lf +plugins/**/*.mjs text eol=lf +plugins/**/*.json text eol=lf +site/**/*.ts text eol=lf +site/**/*.json text eol=lf +.claude-plugin/*.json text eol=lf diff --git a/CHANGELOG.md b/CHANGELOG.md index 48d053f1..60762b13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,30 @@ predate the plugin rewrite and are grouped by date. ## [Unreleased] +## [2.10.1] — 2026-07-28 + +### Fixed + +- **The farm dispatcher serializes access to git's worktree registry (#515).** + `.git/worktrees/` is a shared registry that git mutates non-atomically: + `git worktree add` scans the existing entries while creating its own, so a + second concurrent add could read a sibling's directory after it existed but + before its `commondir` was written. The dispatcher ran up to + `FARM_CONCURRENCY` prepare sequences at once with no lock, so one task would + intermittently escalate at `attempts: 0` with + `worktree add failed: … failed to read .git/worktrees//commondir`, + flipping an otherwise green run's exit code from 0 to 2. + + Measured directly against real git: six concurrent prepare sequences over 40 + rounds produced two failures with that signature, and zero when serialized. + + Worktree creation, removal and prune now run under a dedicated lock, separate + from the existing merge lock so that short registry calls never queue behind a + long merge. Concurrency is unchanged — the worker API call, the gate and the + tests all still run in parallel; only the brief registry mutations are + serialized, and the retry backoff deliberately runs outside the lock so one + worker's retry schedule cannot stall the others. + ## [2.10.0] — 2026-07-27 A MINOR bump, not a patch: this window adds `ca-pi` as a fourth sibling plugin, diff --git a/README.md b/README.md index 19ddfea0..7db6d416 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ Every intent routes through a gated skill or reviewer agent. Nothing commits unt Claude Code plugin Codex plugin Pi Feature Forge preview -version 2.10.0 +version 2.10.1 commands skills agents diff --git a/plugins/ca/.claude-plugin/plugin.json b/plugins/ca/.claude-plugin/plugin.json index 5c78b2fe..7212bf90 100644 --- a/plugins/ca/.claude-plugin/plugin.json +++ b/plugins/ca/.claude-plugin/plugin.json @@ -2,7 +2,7 @@ "name": "ca", "displayName": "codeArbiter", "description": "Orchestration layer for Claude Code. Routes every intent through gated skills and reviewer agents, drives spec-driven TDD, mechanically enforces the commit and audit-trail gates, decides via SMARTS, and keeps an append-only audit trail. Requires Python 3 on PATH. Dormant until you opt a repo in; run /ca:init to activate.", - "version": "2.10.0", + "version": "2.10.1", "author": { "name": "arbiterForge" }, "license": "AGPL-3.0-only", "homepage": "https://github.com/arbiterForge/codeArbiter", diff --git a/plugins/ca/tools/farm.js b/plugins/ca/tools/farm.js index aad4e177..9383cd33 100755 --- a/plugins/ca/tools/farm.js +++ b/plugins/ca/tools/farm.js @@ -1170,21 +1170,30 @@ function withMergeLock(fn) { }); return next; } -async function prepareWorktree(branch, wt, from) { +var worktreeChain = Promise.resolve(); +function withWorktreeLock(fn) { + const next = worktreeChain.then(fn, fn); + worktreeChain = next.catch(() => { + }); + return next; +} +async function prepareWorktree(branch, wt, from, gitFn = git) { try { assertContainedWorktree(wt); } catch (e) { return e instanceof Error ? e.message : String(e); } - await git(["worktree", "remove", "--force", wt]).catch(() => { - }); - await rm(wt, { recursive: true, force: true }).catch(() => { - }); - await git(["branch", "-D", branch]).catch(() => { + return withWorktreeLock(async () => { + await gitFn(["worktree", "remove", "--force", wt]).catch(() => { + }); + await rm(wt, { recursive: true, force: true }).catch(() => { + }); + await gitFn(["branch", "-D", branch]).catch(() => { + }); + const add = await gitFn(["worktree", "add", "-b", branch, wt, from]); + if (add.code !== 0) return `worktree add failed: ${add.out.slice(0, 200)}`; + return null; }); - const add = await git(["worktree", "add", "-b", branch, wt, from]); - if (add.code !== 0) return `worktree add failed: ${add.out.slice(0, 200)}`; - return null; } var CLEANUP_ATTEMPTS = 3; var CLEANUP_DELAY_MS = 150; @@ -1213,14 +1222,17 @@ async function removeWorktreeVerified(gitFn, wt, opts = {}) { const delayMs = opts.delayMs ?? CLEANUP_DELAY_MS; let lastErr = ""; for (let i = 1; i <= attempts; i++) { - const rmR = await gitFn(["worktree", "remove", "--force", wt]); - if (rmR.code !== 0) lastErr = rmR.out.trim().split("\n").slice(-1)[0] ?? ""; - let registered = await stillRegistered(gitFn, wt); - const present = await pathExists(wt); - if (registered && !present) { - await gitFn(["worktree", "prune"]); - registered = await stillRegistered(gitFn, wt); - } + const { registered, present } = await withWorktreeLock(async () => { + const rmR = await gitFn(["worktree", "remove", "--force", wt]); + if (rmR.code !== 0) lastErr = rmR.out.trim().split("\n").slice(-1)[0] ?? ""; + let reg = await stillRegistered(gitFn, wt); + const exists = await pathExists(wt); + if (reg && !exists) { + await gitFn(["worktree", "prune"]); + reg = await stillRegistered(gitFn, wt); + } + return { registered: reg, present: exists }; + }); if (!registered && !present) return { ok: true, target: wt, attempts: i }; if (i < attempts) await sleep(delayMs * i); else @@ -2202,6 +2214,7 @@ export { parseChatCompletion, parseMutationHookOutput, parsePlan, + prepareWorktree, projectPlanMetaForReport, readSampling, redactSecrets, @@ -2214,5 +2227,6 @@ export { screenEntitlements, validate, validateWorktreeRoot, + withWorktreeLock, writeFilesInto }; diff --git a/plugins/ca/tools/farm.ts b/plugins/ca/tools/farm.ts index d8800a55..7efd526c 100644 --- a/plugins/ca/tools/farm.ts +++ b/plugins/ca/tools/farm.ts @@ -1357,7 +1357,46 @@ function withMergeLock(fn: () => Promise): Promise { return next; } -async function prepareWorktree(branch: string, wt: string, from: string): Promise { +// #515 — `.git/worktrees/` is a SHARED REGISTRY, and git mutates it +// non-atomically. `git worktree add` scans the existing entries while creating +// its own, so a second add running at the same time can read a sibling's +// directory after it exists but before its `commondir` is written: +// +// fatal: failed to read .git/worktrees/bulk0/commondir: No error +// +// which is the flake reported in #515 — one task escalating at `attempts: 0` +// under six-way concurrency, flipping a green run's exit code to 2. Measured +// directly: 6 concurrent prepare sequences over 40 rounds produced 2 failures +// with that exact signature, and 0 when serialized. +// +// This is a SEPARATE chain from mergeChain on purpose. A merge holds its lock +// across a git merge plus its gate, which is long; worktree registry calls are +// short. Sharing one lock would park every worktree setup behind an unrelated +// merge for no correctness gain. Nothing takes both in the opposite order, so +// the two cannot deadlock against each other. +// +// It does NOT reduce the dispatcher's concurrency: the worker API call, the +// gate and the tests — everything that actually takes time — stay parallel. +// Only the brief registry mutations are queued, which is why #515 AC-4 (do not +// drop FARM_CONCURRENCY below 6 to dodge this) is satisfied rather than dodged. +let worktreeChain: Promise = Promise.resolve(); + +export function withWorktreeLock(fn: () => Promise): Promise { + const next = worktreeChain.then(fn, fn); + worktreeChain = next.catch(() => {}); + return next; +} + +// `gitFn` is injectable for the same reason `removeWorktreeVerified` takes one: +// this is the function #515's race lives in, and a test that cannot observe the +// git calls cannot prove they are serialized. Defaults to the module runner, so +// every existing caller is unchanged. +export async function prepareWorktree( + branch: string, + wt: string, + from: string, + gitFn: GitRunner = git, +): Promise { // #163: fail closed before the recursive delete — the resolved path must be // strictly inside the allowed (in-repo) farm worktree root. This is the // load-bearing guard: `rm(wt, {recursive, force})` below is the exact @@ -1369,12 +1408,20 @@ async function prepareWorktree(branch: string, wt: string, from: string): Promis } // Clean any stale worktree dir and branch so a re-run doesn't trip over the // leftovers of a prior run (git worktree add -b fails if the branch exists). - await git(["worktree", "remove", "--force", wt]).catch(() => {}); - await rm(wt, { recursive: true, force: true }).catch(() => {}); - await git(["branch", "-D", branch]).catch(() => {}); - const add = await git(["worktree", "add", "-b", branch, wt, from]); - if (add.code !== 0) return `worktree add failed: ${add.out.slice(0, 200)}`; - return null; + // + // #515: the whole sequence is taken under the worktree lock, not just the + // `add`. The removal and the add both write the shared registry, so leaving + // either outside would still let one worker observe another mid-write. The + // containment guard above stays OUTSIDE deliberately — a rejected path must + // not queue behind other workers to be told it was never allowed. + return withWorktreeLock(async () => { + await gitFn(["worktree", "remove", "--force", wt]).catch(() => {}); + await rm(wt, { recursive: true, force: true }).catch(() => {}); + await gitFn(["branch", "-D", branch]).catch(() => {}); + const add = await gitFn(["worktree", "add", "-b", branch, wt, from]); + if (add.code !== 0) return `worktree add failed: ${add.out.slice(0, 200)}`; + return null; + }); } // -------------------------------------------------------------------------- @@ -1453,20 +1500,32 @@ export async function removeWorktreeVerified( const delayMs = opts.delayMs ?? CLEANUP_DELAY_MS; let lastErr = ""; for (let i = 1; i <= attempts; i++) { - const rmR = await gitFn(["worktree", "remove", "--force", wt]); - if (rmR.code !== 0) lastErr = rmR.out.trim().split("\n").slice(-1)[0] ?? ""; - let registered = await stillRegistered(gitFn, wt); - const present = await pathExists(wt); - // Prune clears a stale registration whose directory is already gone — the - // exact state a partially-failed removal or an external delete leaves. - // Run it ONLY in that state: `git worktree prune` is repo-wide, and a - // blanket call would also deregister an unrelated worktree whose path is - // merely unreachable right now (an unmounted volume, a temporarily denied - // directory). Narrow it to the condition it is actually needed for. - if (registered && !present) { - await gitFn(["worktree", "prune"]); - registered = await stillRegistered(gitFn, wt); - } + // #515: removal, prune and the two verification reads all touch the shared + // `.git/worktrees/` registry, so the whole attempt is taken under the + // worktree lock. `prune` is the most dangerous of the three to run + // unserialized — it DELETES sibling metadata directories, which is exactly + // what a concurrent `worktree add` is scanning. + // + // The backoff below is deliberately OUTSIDE the lock: holding it across a + // sleep would park every other worker behind this one's retry schedule, + // turning a bounded local delay into a global stall. + const { registered, present } = await withWorktreeLock(async () => { + const rmR = await gitFn(["worktree", "remove", "--force", wt]); + if (rmR.code !== 0) lastErr = rmR.out.trim().split("\n").slice(-1)[0] ?? ""; + let reg = await stillRegistered(gitFn, wt); + const exists = await pathExists(wt); + // Prune clears a stale registration whose directory is already gone — the + // exact state a partially-failed removal or an external delete leaves. + // Run it ONLY in that state: `git worktree prune` is repo-wide, and a + // blanket call would also deregister an unrelated worktree whose path is + // merely unreachable right now (an unmounted volume, a temporarily denied + // directory). Narrow it to the condition it is actually needed for. + if (reg && !exists) { + await gitFn(["worktree", "prune"]); + reg = await stillRegistered(gitFn, wt); + } + return { registered: reg, present: exists }; + }); if (!registered && !present) return { ok: true, target: wt, attempts: i }; if (i < attempts) await sleep(delayMs * i); else diff --git a/plugins/ca/tools/farm.unit.test.ts b/plugins/ca/tools/farm.unit.test.ts index 06ac2e54..58126292 100644 --- a/plugins/ca/tools/farm.unit.test.ts +++ b/plugins/ca/tools/farm.unit.test.ts @@ -11,7 +11,7 @@ import { tmpdir } from "node:os"; import { extractFileBlocks, extractLiterals, codeLineCount, validate, assertSecureBaseUrl, runTask, httpWorker, DEFAULT_API_BASE_URL, parseChatCompletion, checkDrift, screenEntitlements, makeEntitlementProbe, redactSecrets, run, runGate, mintRunId, parseMutationHookOutput, buildChatBody, readSampling, buildPrompt, captureInScope, createLimiter, validateWorktreeRoot, assertContainedWorktree, allowedWorktreeRoot, _resetAllowedWorktreeRoot, numEnv, atomicWriteFile, assertSafeRunId } from "./farm.ts"; import type { InjectedFile, Sampling } from "./farm.ts"; import type { Worker, WorkerResult, RunTaskDeps, Task } from "./farm.ts"; -import { removeWorktreeVerified, deleteBranchVerified, runExitCode, newRunArtifactHealth, cleanupReportLines } from "./farm.ts"; +import { removeWorktreeVerified, deleteBranchVerified, runExitCode, newRunArtifactHealth, cleanupReportLines, withWorktreeLock, prepareWorktree } from "./farm.ts"; import { projectPlanMetaForReport, noteArtifactError, noteUnavailableDiff, MAX_RECORDED_ARTIFACT_ERRORS } from "./farm.ts"; import { scrubbedEnv, treeKill, taskkillPath } from "./exec.ts"; import type { RunResult } from "./exec.ts"; @@ -2733,6 +2733,93 @@ describe("#398 — verified, retried, reported worktree teardown", () => { expect(r.detail).toMatch(/is locked/); }); + // #515 — the shared `.git/worktrees/` registry is mutated non-atomically by + // git, so two registry commands in flight at once let one read a sibling + // entry that exists but is not finished. Measured directly against real git: + // 6 concurrent prepare sequences over 40 rounds produced 2 failures reading + // `.git/worktrees//commondir`, and 0 when serialized. + // + // These assert the PROPERTY (never two registry commands overlapping) rather + // than the absence of the flake. Asserting "the suite went green" would pass + // ~95% of the time on the unfixed dispatcher and prove nothing. + describe("#515 worktree registry serialization", () => { + /** A git runner that reports the maximum overlap it ever observed. */ + function overlapTrackingGit() { + let inFlight = 0; + let maxInFlight = 0; + const isRegistryOp = (a: string[]) => + a[0] === "worktree" && (a[1] === "remove" || a[1] === "add" || a[1] === "prune" || a[1] === "list"); + const git = async (args: string[]): Promise => { + if (!isRegistryOp(args)) return res(0); + inFlight++; + maxInFlight = Math.max(maxInFlight, inFlight); + // Yield across a macrotask so a genuinely concurrent caller is given + // every chance to interleave. Without the lock it takes it. + await new Promise((r) => setTimeout(r, 0)); + inFlight--; + return args[1] === "list" ? res(0, "") : res(0); + }; + return { git, max: () => maxInFlight }; + } + + it("never runs two registry commands at once across concurrent removals", async () => { + const { git, max } = overlapTrackingGit(); + await Promise.all( + Array.from({ length: 6 }, (_, i) => + removeWorktreeVerified(git, path.resolve(`.farm/worktrees/par-${i}`), { attempts: 1, delayMs: 1 })), + ); + expect(max()).toBe(1); + }); + + it("serializes callers in the order they arrive", async () => { + const order: number[] = []; + await Promise.all( + Array.from({ length: 5 }, (_, i) => + withWorktreeLock(async () => { + order.push(i); + await new Promise((r) => setTimeout(r, 0)); + order.push(i); + })), + ); + // Each caller's pair is adjacent — nobody ran inside anybody else. + expect(order).toEqual([0, 0, 1, 1, 2, 2, 3, 3, 4, 4]); + }); + + it("never runs two registry commands at once across concurrent PREPARES", async () => { + // prepareWorktree is where the reported failure actually occurred + // ("worktree add failed: ... failed to read .git/worktrees/bulk0/ + // commondir"). Testing only removeWorktreeVerified left the lock on this + // path unpinned — removing it entirely kept every other test in this + // block green. + const { git, max } = overlapTrackingGit(); + const errs = await Promise.all( + Array.from({ length: 6 }, (_, i) => + prepareWorktree(`farm/par-${i}`, path.resolve(`.farm/worktrees/par-${i}`), "main", git)), + ); + expect(max()).toBe(1); + expect(errs.every((e) => e === null)).toBe(true); + }); + + it("a rejecting caller does not wedge the lock, and does not let the queue reorder", async () => { + // `worktree add` failing is the ORDINARY case this lock exists around, so + // a chain that stops draining after one rejection would convert the flake + // into a hang — strictly worse than the bug. Order is asserted too: a + // rejection must not let a queued caller overtake one that arrived first. + const order: string[] = []; + const failing = withWorktreeLock(async () => { + order.push("first"); + throw new Error("boom"); + }); + const after = withWorktreeLock(async () => { + order.push("second"); + return "after"; + }); + await expect(failing).rejects.toThrow("boom"); + await expect(after).resolves.toBe("after"); + expect(order).toEqual(["first", "second"]); + }); + }); + it("deleteBranchVerified reports a branch git still lists", async () => { const git = async (args: string[]): Promise => { if (args[0] === "branch" && args[1] === "-D") return res(1, "error: Cannot delete branch 'farm/x' checked out at ...");