diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b11706c..e6376686 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,36 @@ predate the plugin rewrite and are grouped by date. ## [Unreleased] +## [2.10.4] — 2026-07-29 + +### Fixed + +- **The symlink-safe worktree writer accepts an absolute destination spelled + through a link (#541).** `writeWorktreeFile` canonicalized the worktree ROOT + but resolved the caller's absolute path as-is, then compared the two — so a + caller building a destination from the root it had itself passed in was + refused for writing inside its own worktree. Same root cause as #539, one + module down, and it never reproduced on a host whose paths are already + canonical. + + Only the ANCESTOR chain is canonicalized; the final component stays literal, + so a symlinked destination is still refused rather than written through. Like + #539 this tightens the guard: a path whose ancestors traverse a link out of + the root is now caught by containment instead of relying on a later check. + +- **A timed-out farm test no longer abandons a live subprocess (#542).** Every + launch in `farm.test.ts` is a real child process and none were tracked, so a + vitest timeout left one running with the temp directory as its cwd and the + teardown then failed `EBUSY` — reporting the cleanup instead of the timeout + that caused it. Children are now reaped before the directory is removed, and + the removal tolerates Windows' asynchronous handle release. + + The heaviest case (12 tasks at `FARM_CONCURRENCY` 6) also gets a budget drawn + from measurement rather than a nudge: 3553 ms isolated on a developer box + against a 5000 ms default, 5331 ms under load on a CI runner. It is now 30 s — + bounded, so a genuine hang still fails. The case itself is unchanged; + concurrency stays at 6 and every assertion stands. + ## [2.10.3] — 2026-07-28 ### Fixed diff --git a/README.md b/README.md index d91eeec5..7f28e36d 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.3 +version 2.10.4 commands skills agents diff --git a/plugins/ca/.claude-plugin/plugin.json b/plugins/ca/.claude-plugin/plugin.json index f6366ac1..547d65e7 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.3", + "version": "2.10.4", "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 a2039dc4..ab5064e7 100755 --- a/plugins/ca/tools/farm.js +++ b/plugins/ca/tools/farm.js @@ -286,6 +286,24 @@ function contained(root, candidate) { const relative = path2.relative(root, candidate); return relative === "" || !relative.startsWith(`..${path2.sep}`) && relative !== ".." && !path2.isAbsolute(relative); } +async function canonicalizeAncestors(target, resolvePath = realpath) { + const resolved = path2.resolve(target); + const leaf = path2.basename(resolved); + const missing = []; + let cursor = path2.dirname(resolved); + for (; ; ) { + try { + const real = await resolvePath(cursor); + return path2.join(real, ...missing.slice().reverse(), leaf); + } catch (error) { + if (!isMissing(error)) throw error; + const parent = path2.dirname(cursor); + if (parent === cursor) return resolved; + missing.push(path2.basename(cursor)); + cursor = parent; + } + } +} function sameFile(left, right) { return left.dev === right.dev && left.ino === right.ino; } @@ -308,7 +326,8 @@ async function writeWorktreeFile(worktree, relPath, contents, testHooks = {}) { if (!contained(canonicalRoot, await realpath(verified.path))) unsafe(); } }; - const target = path2.resolve(canonicalRoot, relPath); + const requested = path2.isAbsolute(relPath) ? await canonicalizeAncestors(relPath) : relPath; + const target = path2.resolve(canonicalRoot, requested); const relative = path2.relative(canonicalRoot, target); if (relative === "" || relative === ".." || relative.startsWith(`..${path2.sep}`) || path2.isAbsolute(relative)) unsafe(); const segments = relative.split(path2.sep); diff --git a/plugins/ca/tools/farm.test.ts b/plugins/ca/tools/farm.test.ts index c6aa2193..2f58cf79 100644 --- a/plugins/ca/tools/farm.test.ts +++ b/plugins/ca/tools/farm.test.ts @@ -72,6 +72,55 @@ function createTempRepo(dir: string) { // -------------------------------------------------------------------------- // Run farm.ts via tsx (dev path) against the temp repo // -------------------------------------------------------------------------- +// #542 — every farm launch here is a real subprocess, and nothing tracked it. +// When vitest abandons a case on timeout the child keeps running, keeps its cwd +// open, and the afterEach `rmSync(tmpDir)` then fails with EBUSY on Windows. The +// teardown error is louder than the timeout that caused it, so the log names the +// wrong problem. +// +// Track every live child so teardown can release the directory before deleting +// it. A test that times out is already failing; it must not also corrupt the +// diagnosis of the next one. +const liveChildren = new Set>(); + +function trackChild(child: ReturnType): void { + liveChildren.add(child); + child.on("close", () => liveChildren.delete(child)); +} + +/** Kill any subprocess still running, and wait for the OS to release its handles. */ +async function reapStrayChildren(): Promise { + if (liveChildren.size === 0) return; + for (const child of liveChildren) { + try { + child.kill("SIGKILL"); + } catch { + /* already gone */ + } + } + // Windows releases the cwd handle asynchronously after the process dies, so a + // kill alone is not enough to make rmSync succeed on the next line. + for (let i = 0; i < 40 && liveChildren.size > 0; i++) { + await new Promise((r) => setTimeout(r, 25)); + } + liveChildren.clear(); +} + +/** `rmSync` that tolerates Windows' asynchronous handle release. */ +function rmWithRetry(target: string): void { + for (let attempt = 1; attempt <= 10; attempt++) { + try { + rmSync(target, { recursive: true, force: true }); + return; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if ((code !== "EBUSY" && code !== "EPERM" && code !== "ENOTEMPTY") || attempt === 10) throw error; + const until = Date.now() + 50 * attempt; + while (Date.now() < until) { /* brief spin; afterEach is sync */ } + } + } +} + function runFarm( repoDir: string, planPath: string, @@ -101,6 +150,7 @@ function runFarm( }, }, ); + trackChild(child); let out = ""; child.stdout.on("data", (d: Buffer) => (out += d)); child.stderr.on("data", (d: Buffer) => (out += d)); @@ -141,6 +191,7 @@ function runFarmWithArgs( ["--import", TSX_LOADER, farmTs, ...extraArgs, planPath], { cwd: repoDir, env: spawnEnv }, ); + trackChild(child); let out = ""; child.stdout.on("data", (d: Buffer) => (out += d)); child.stderr.on("data", (d: Buffer) => (out += d)); @@ -181,9 +232,12 @@ describe("farm.ts smoke tests", () => { createTempRepo(tmpDir); }); - afterEach(() => { + afterEach(async () => { mockServer?.close(); - rmSync(tmpDir, { recursive: true, force: true }); + // #542, same reasoning as the artifact block: this suite launches the same + // real subprocesses, so it inherits the same abandoned-child teardown. + await reapStrayChildren(); + rmWithRetry(tmpDir); // Clean up any leftover farm worktrees rmSync(join(tmpDir, "../.codearbiter-farm"), { recursive: true, force: true }); }); @@ -1483,9 +1537,13 @@ describe("farm artifact publication (#397 / #387)", () => { createTempRepo(tmpDir); }); - afterEach(() => { + afterEach(async () => { mockServer?.close(); - rmSync(tmpDir, { recursive: true, force: true }); + // #542: release any subprocess a timed-out case abandoned BEFORE deleting + // its cwd, or the delete fails EBUSY and reports the teardown instead of + // the timeout that caused it. + await reapStrayChildren(); + rmWithRetry(tmpDir); }); // ------------------------------------------------------------------------- @@ -1727,7 +1785,20 @@ describe("farm artifact publication (#397 / #387)", () => { expect(result.out).toContain(join(".farm", "runs", "mdonly")); }); - it("#387: the unavailable-diff list is bounded and still reports the true total", async () => { + // #542 — MEASURED, not nudged. This is the heaviest case in the file: 12 tasks + // at FARM_CONCURRENCY 6, each spawning a real gate subprocess. Isolated on a + // developer Windows box it takes 3553ms against the 5000ms default — 29% + // headroom before any load at all. Under full-suite load on a windows-latest + // runner it took 5331ms and timed out, which then abandoned a live child and + // turned the teardown into a second, louder EBUSY failure. + // + // 30s is ~6x the slowest observed run: comfortable for a loaded runner, still + // bounded, so a genuine hang fails rather than sitting until the job timeout. + // The case is NOT made cheaper - FARM_CONCURRENCY stays 6 and the bounded-at-11 + // / true-total-12 assertions are untouched (#515 AC-4, #542 AC-2). The six-way + // path is the thing under test; buying headroom by shrinking it would delete + // the coverage this case exists for. + it("#387: the unavailable-diff list is bounded and still reports the true total", { timeout: 30_000 }, async () => { ({ server: mockServer, port } = await startMockServer(greenHandler())); const ids = Array.from({ length: 12 }, (_, i) => `bulk${i}`); const planPath = writePlan("plan.json", planFor("bounded", ids, port)); diff --git a/plugins/ca/tools/worktree-fs.test.ts b/plugins/ca/tools/worktree-fs.test.ts index 8cd159ae..7fc3d94b 100644 --- a/plugins/ca/tools/worktree-fs.test.ts +++ b/plugins/ca/tools/worktree-fs.test.ts @@ -1,9 +1,9 @@ -import { link, lstat, mkdir, mkdtemp, readdir, readFile, rm, symlink, writeFile } from "node:fs/promises"; +import { link, lstat, mkdir, mkdtemp, readdir, readFile, realpath, rm, symlink, writeFile } from "node:fs/promises"; import { existsSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; -import { isUnsafeWorktreePathError, writeWorktreeFile } from "./worktree-fs.ts"; +import { canonicalizeAncestors, isUnsafeWorktreePathError, writeWorktreeFile } from "./worktree-fs.ts"; type WorktreeFs = typeof import("./worktree-fs.ts"); @@ -133,11 +133,15 @@ describe("symlink-safe worktree writer", () => { * byte and 40 concurrent calls reached three of them through the public API with * no seam at all. * - * Platform note: CI runs the tools job on ubuntu-latest ONLY, so Linux is the - * platform that matters for these numbers. Windows and Linux do not agree here — - * an over-long path segment reaches the mkdir failure arm on Windows but fails - * earlier at `lstat` with ENAMETOOLONG on Linux, so that case bought nothing on - * CI and is not in this block. + * Platform note: this tree now runs on BOTH ubuntu-latest and windows-latest — + * the tools job is still ubuntu-only, but #521's coverage-union cells execute + * the suite on each host and merge the reports, so Linux is no longer the only + * platform whose numbers count. Windows and Linux still do not agree here: an + * over-long path segment reaches the mkdir failure arm on Windows but fails + * earlier at `lstat` with ENAMETOOLONG on Linux. That divergence is the reason + * the union exists rather than a reason to ignore one side, and it is why #541 + * — an absolute destination refused on Windows alone — went unseen until a + * second host ran this file. */ describe("symlink-safe worktree writer — refusal paths", () => { const MESSAGE = "unsafe worktree path rejected"; @@ -234,6 +238,133 @@ describe("symlink-safe worktree writer — refusal paths", () => { } }); + // #541 — the absolute-destination check compared the caller's spelling of the + // root against `realpath(worktree)`. Those are the same directory and + // different strings whenever a link or a Windows 8.3 short name is on the + // path, so a caller writing INSIDE its own worktree was refused. Same root + // cause as #539, one module down; it never reproduced on a developer box + // whose tmpdir is already canonical, and surfaced only when #521's coverage + // union first ran this tree on Windows CI. + // + // A junction gives two real spellings of one directory on any platform. + it("accepts an absolute destination spelled through a link into the same root", async () => { + const base = await realpath(await mkdtemp(path.join(tmpdir(), "farm-541-"))); + try { + const real = path.join(base, "real"); + const root = path.join(real, "wt"); + await mkdir(root, { recursive: true }); + const link = path.join(base, "link"); + try { + await symlink(real, link, "junction"); + } catch { + return; // unprivileged Windows without developer mode + } + const rootViaLink = path.join(link, "wt"); + + // Precondition: the two spellings are NOT lexically comparable, which is + // the whole reason this case exists. + expect(path.relative(root, rootViaLink).startsWith("..")).toBe(true); + + // The caller passes its own spelling as the worktree AND builds the + // destination from it — the natural thing to do. + await writeWorktreeFile(rootViaLink, path.join(rootViaLink, "inside.txt"), "accepted\n"); + expect(await readFile(path.join(root, "inside.txt"), "utf8")).toBe("accepted\n"); + + // Mirror: canonical root, destination spelled through the link. + await writeWorktreeFile(root, path.join(rootViaLink, "mirror.txt"), "accepted\n"); + expect(await readFile(path.join(root, "mirror.txt"), "utf8")).toBe("accepted\n"); + + // Deep, not-yet-created ancestors through the link still land inside. + await writeWorktreeFile(rootViaLink, path.join(rootViaLink, "a", "b", "deep.txt"), "deep\n"); + expect(await readFile(path.join(root, "a", "b", "deep.txt"), "utf8")).toBe("deep\n"); + } finally { + await rm(base, { recursive: true, force: true }); + } + }); + + it("does NOT pre-resolve the leaf, so a symlinked destination is still refused", async () => { + // The canonicalization added for #541 walks the ANCESTOR chain only. If it + // resolved the final component too, a symlink AT the destination pointing + // to another file INSIDE the root would resolve to a contained path, pass + // containment, and be written THROUGH — silently overwriting the target the + // per-segment checks exist to protect. Refusing symlinked destinations is + // this module's contract, and canonicalizing must not quietly undo it. + const base = await realpath(await mkdtemp(path.join(tmpdir(), "farm-541leaf-"))); + try { + const root = path.join(base, "wt"); + await mkdir(root, { recursive: true }); + await writeFile(path.join(root, "real.txt"), "original\n", "utf8"); + try { + await symlink(path.join(root, "real.txt"), path.join(root, "alias.txt"), "file"); + } catch { + return; // unprivileged Windows without developer mode + } + await expectRefused(writeWorktreeFile(root, path.join(root, "alias.txt"), "written-through\n")); + expect(await readFile(path.join(root, "real.txt"), "utf8")).toBe("original\n"); + } finally { + await rm(base, { recursive: true, force: true }); + } + }); + + it("REFUSES when an ancestor cannot be canonicalized for a reason other than absence", async () => { + // EACCES and ELOOP are not portably inducible, so the failure is injected. + // Without this the refusal branch never runs and the fail-closed stance is + // a comment rather than behaviour. + const err = (code: string) => { + const e = new Error(`simulated ${code}`) as NodeJS.ErrnoException; + e.code = code; + return e; + }; + await expect( + canonicalizeAncestors(path.join(path.sep, "repo", "wt", "f.txt"), async () => { throw err("EACCES"); }), + ).rejects.toThrow(/EACCES/); + await expect( + canonicalizeAncestors(path.join(path.sep, "repo", "wt", "f.txt"), async () => { throw err("ELOOP"); }), + ).rejects.toThrow(/ELOOP/); + }); + + it("treats a missing ancestor as normal and walks up to the deepest existing one", async () => { + const seen: string[] = []; + const target = path.join(path.sep, "a", "b", "c", "f.txt"); + const out = await canonicalizeAncestors(target, async (p) => { + seen.push(p); + if (p === path.resolve(path.sep, "a")) return path.resolve(path.sep, "REAL"); + const e = new Error("missing") as NodeJS.ErrnoException; + e.code = "ENOENT"; + throw e; + }); + expect(out).toBe(path.join(path.resolve(path.sep, "REAL"), "b", "c", "f.txt")); + expect(seen.length).toBeGreaterThan(1); + }); + + it("still refuses an absolute destination whose ancestors link OUT of the root", async () => { + // The tightening half. Canonicalizing the ancestor chain means a directory + // inside the root that links outside is caught HERE, by containment, rather + // than relying on a later check to notice. + const base = await realpath(await mkdtemp(path.join(tmpdir(), "farm-541e-"))); + try { + const root = path.join(base, "wt"); + const outside = path.join(base, "outside"); + await mkdir(root, { recursive: true }); + await mkdir(outside, { recursive: true }); + await writeFile(path.join(outside, "sentinel.txt"), "outside-must-survive", "utf8"); + + const escape = path.join(root, "escape"); + try { + await symlink(outside, escape, "junction"); + } catch { + return; + } + // Lexically this looks contained — that is what makes it worth refusing. + expect(path.relative(root, escape).startsWith("..")).toBe(false); + + await expectRefused(writeWorktreeFile(root, path.join(escape, "sentinel.txt"), "overwrite")); + expect(await readFile(path.join(outside, "sentinel.txt"), "utf8")).toBe("outside-must-survive"); + } finally { + await rm(base, { recursive: true, force: true }); + } + }); + it("refuses a NUL byte in the destination segment", async () => { // Reaches the non-ENOENT arm of the destination lstat: the error is neither // "missing" nor an `unsafe()` call, so the catch has to funnel it. An diff --git a/plugins/ca/tools/worktree-fs.ts b/plugins/ca/tools/worktree-fs.ts index eb6d5087..4911563f 100644 --- a/plugins/ca/tools/worktree-fs.ts +++ b/plugins/ca/tools/worktree-fs.ts @@ -35,6 +35,49 @@ function contained(root: string, candidate: string): boolean { return relative === "" || (!relative.startsWith(`..${path.sep}`) && relative !== ".." && !path.isAbsolute(relative)); } +// #541 — an ABSOLUTE destination is compared against `realpath(worktree)`, but +// resolved from the caller's own spelling of the root. Those are the same +// directory and different strings whenever a link or a Windows 8.3 short name +// sits on the path: a GitHub runner hands out `C:\Users\RUNNER~1\...` while +// realpath returns `C:\Users\runneradmin\...`, so a caller building an absolute +// path from the root IT passed in was refused for writing inside the worktree. +// Same root cause as #539, one module down. +// +// Only the ANCESTOR chain is canonicalized; the final component is kept +// literal. Resolving the leaf would follow a symlink AT the destination, which +// is precisely what the per-segment lstat checks below exist to catch — this +// must not quietly pre-resolve the thing they are guarding. +// +// Canonicalizing tightens rather than loosens: a path whose ancestors traverse +// a symlink OUT of the root now fails `contained` here instead of relying on a +// later check. A missing ancestor is normal (directories are created on +// demand), so walk up to the deepest existing one; any non-ENOENT failure +// propagates and refuses rather than falling through. +// `resolvePath` is injectable for the same reason #539's canonicalize is: an +// EACCES or ELOOP cannot be induced portably, and a guard whose refusal path +// never executes is a fail-closed stance asserted rather than proven. +export async function canonicalizeAncestors( + target: string, + resolvePath: (p: string) => Promise = realpath, +): Promise { + const resolved = path.resolve(target); + const leaf = path.basename(resolved); + const missing: string[] = []; + let cursor = path.dirname(resolved); + for (;;) { + try { + const real = await resolvePath(cursor); + return path.join(real, ...missing.slice().reverse(), leaf); + } catch (error) { + if (!isMissing(error)) throw error; + const parent = path.dirname(cursor); + if (parent === cursor) return resolved; + missing.push(path.basename(cursor)); + cursor = parent; + } + } +} + function sameFile(left: Stats, right: Stats): boolean { return left.dev === right.dev && left.ino === right.ino; } @@ -70,7 +113,12 @@ export async function writeWorktreeFile( if (!contained(canonicalRoot, await realpath(verified.path))) unsafe(); } }; - const target = path.resolve(canonicalRoot, relPath); + // #541: an absolute destination is canonicalized against the same reality + // `canonicalRoot` came from, so the two are comparable. A relative one is + // resolved against `canonicalRoot`, which is already canonical, and is + // therefore untouched. + const requested = path.isAbsolute(relPath) ? await canonicalizeAncestors(relPath) : relPath; + const target = path.resolve(canonicalRoot, requested); const relative = path.relative(canonicalRoot, target); if (relative === "" || relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) unsafe();