diff --git a/packages/runtime/src/workspace-handoff.ts b/packages/runtime/src/workspace-handoff.ts index d59bee659..1a9d6b469 100644 --- a/packages/runtime/src/workspace-handoff.ts +++ b/packages/runtime/src/workspace-handoff.ts @@ -130,12 +130,113 @@ function parseChangedPaths(raw: string): WorkspacePatchFile[] { return [...unique.values()].sort((left, right) => left.path.localeCompare(right.path)); } +/** + * Stage every worktree change except the runtime roots. + * + * Three earlier shapes were all wrong, each established by measurement rather than argument: + * + * - `git add -A -- . :(exclude)/**` — a negative pathspec makes `git add` report an ignored + * path as an error, so an ignored `node_modules` aborted capture (issue #281, killed a live run). + * - bare `git add -A -- .` — avoids that error but descends into the roots, hashing them into + * unreachable objects and aborting on any unreadable file under `artifacts/`. `core.excludesFile` + * does not prevent the descent either: it is the lowest-precedence ignore source, so a repository + * `.gitignore` negation re-admits the path. + * - naming top-level entry names as positive pathspecs — aborts on ANY ignored top-level entry, not + * just a runtime root. Foundry ignores `cache/` and `out/`, which `forge build` creates, so that + * was a wider regression than the bug it fixed. + * + * Naming the exact paths git reports avoids all three. `--exclude-standard` drops ignored untracked + * paths so none is ever named, and `--cached` keeps tracked paths, including ones deleted from the + * worktree, so deletions are still captured. The roots are excluded in `ls-files` itself rather than + * filtered afterwards: unlike `git add`, `ls-files` tolerates negative pathspecs and prunes the + * traversal, which keeps a large `artifacts/` or `.smithers/` from producing path text that would + * overflow the output buffer. + * + * `--force` is required, and is safe because of `--exclude-standard`: the enumerated set is exactly + * `tracked-in-index` plus `non-ignored-untracked`, which is what a plain `git add -A -- .` would + * stage, so suppressing the ignored-path check cannot admit anything new. Without it, naming a + * tracked file whose parent directory is ignored still trips the ignored-path error. + * + * Paths stay as bytes: a non-UTF-8 filename decoded through a string returns replacement characters + * and matches nothing. They are fed over stdin so a wide repository cannot hit `E2BIG`, and + * `--literal-pathspecs` on the `add` stops a name containing glob or `:` magic being reinterpreted. + * + * No `reset` of the roots is needed afterwards. `read-tree ` already put exactly `treeish` + * in the index and nothing here ever names a root path, so their entries are `treeish` by + * construction. + */ function stageWorkspaceTree(workspaceRoot: string, index: string): void { - runGit( + // A listed path can vanish before it is staged — agent subprocesses are still running during + // capture — and `git add --pathspec-from-file` fails the whole invocation when a name matches + // nothing (`--ignore-errors` does not suppress it). Re-list and retry rather than aborting the + // run, which is the failure this whole function exists to avoid. + for (let attempt = 0; ; attempt += 1) { + const pathspecs = stageableWorkspacePaths(workspaceRoot, index); + if (pathspecs.length === 0) return; + try { + runGit( + workspaceRoot, + ["--literal-pathspecs", "add", "-A", "--force", "--pathspec-from-file=-", "--pathspec-file-nul"], + index, + Buffer.concat(pathspecs.flatMap((entry) => [entry, NUL])) + ); + return; + } catch (error) { + const vanished = /did not match any files/u.test(error instanceof Error ? error.message : String(error)); + if (!vanished || attempt >= WORKSPACE_STAGE_ATTEMPTS - 1) throw error; + } + } +} + +const WORKSPACE_STAGE_ATTEMPTS = 3; + +/** Every path git would stage, minus the runtime roots and any directory entry. */ +function stageableWorkspacePaths(workspaceRoot: string, index: string): Buffer[] { + const listed = runGitBuffer( workspaceRoot, - ["add", "-A", "--", ".", ...WORKSPACE_RUNTIME_ROOTS.map((root) => `:(exclude)${root}/**`)], + [ + "ls-files", + "-z", + "--cached", + "--others", + "--exclude-standard", + "--", + ".", + ...WORKSPACE_RUNTIME_ROOTS.map((root) => `:(exclude)${root}/**`) + ], index ); + const runtimeRoots = new Set(WORKSPACE_RUNTIME_ROOTS); + const pathspecs: Buffer[] = []; + for (const entry of splitNulBuffer(listed)) { + // `ls-files --others` reports an untracked nested repository as a directory. Naming it aborts + // `git add` when it has no commit checked out — an interrupted `forge install` or clone produces + // exactly that — and when it does have one, git records a gitlink pointing at an object that + // lives only in the nested repository, which `assertPatchPathsMatchManifest` later refuses. So a + // captured baseline containing one could never yield an applicable patch. + if (entry[entry.length - 1] === 0x2f) continue; + const separator = entry.indexOf(0x2f); + // Runtime-root names are ASCII and Node rejects overlong encodings, so no non-UTF-8 sequence can + // decode into one. Covers a top-level *file* named like a root, which the pathspecs above do not. + const top = (separator < 0 ? entry : entry.subarray(0, separator)).toString("utf8"); + if (top === ".git" || runtimeRoots.has(top)) continue; + pathspecs.push(entry); + } + return pathspecs; +} + +const NUL = Buffer.from([0]); + +function splitNulBuffer(raw: Buffer): Buffer[] { + const entries: Buffer[] = []; + let start = 0; + for (let index = 0; index < raw.length; index += 1) { + if (raw[index] !== 0) continue; + if (index > start) entries.push(raw.subarray(start, index)); + start = index + 1; + } + if (raw.length > start) entries.push(raw.subarray(start)); + return entries; } function assertPatchPathsMatchManifest( @@ -284,7 +385,7 @@ function withTemporaryIndex(workspaceRoot: string, callback: (index: string) } } -function runGit(workspaceRoot: string, args: string[], index?: string, input?: string): string { +function runGit(workspaceRoot: string, args: string[], index?: string, input?: string | Buffer): string { const env = index === undefined ? undefined : { ...process.env, GIT_INDEX_FILE: index }; return execFileSync("git", args, { cwd: workspaceRoot, @@ -294,3 +395,9 @@ function runGit(workspaceRoot: string, args: string[], index?: string, input?: s maxBuffer: MAX_PATCH_BYTES * 2 }); } + +/** Byte-exact git output, for path lists that may not be valid UTF-8. */ +function runGitBuffer(workspaceRoot: string, args: string[], index?: string): Buffer { + const env = index === undefined ? undefined : { ...process.env, GIT_INDEX_FILE: index }; + return execFileSync("git", args, { cwd: workspaceRoot, env, maxBuffer: MAX_PATCH_BYTES * 2 }); +} diff --git a/packages/runtime/test/workspace-handoff.test.ts b/packages/runtime/test/workspace-handoff.test.ts index 8cab85f44..730801aaf 100644 --- a/packages/runtime/test/workspace-handoff.test.ts +++ b/packages/runtime/test/workspace-handoff.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; -import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import fs, { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import os from "node:os"; import path from "node:path"; import test from "node:test"; @@ -55,6 +55,279 @@ test("captures tracked and untracked setup changes relative to the dependency ba } }); +// R44's `property-specification-a16z` node died here (issue #281). Any negative pathspec makes +// `git add` report an ignored path as an error instead of skipping it, so an ignored `node_modules` +// aborted capture outright — `:(exclude)node_modules`, `:!node_modules` and the `/**` form all fail +// identically, and the glob is irrelevant. It only reproduces when the agent actually installed +// dependencies in that worktree, which is why most nodes survive. +test("captures a workspace containing ignored runtime roots", () => { + const root = fixture(); + try { + writeFileSync(path.join(root, ".gitignore"), "node_modules\ncache/\nout/\n"); + git(root, ["add", ".gitignore"]); + git(root, ["commit", "--quiet", "-m", "ignore node_modules"]); + const baseline = captureWorkspaceTree(root); + + // Ignored top-level entries that are NOT runtime roots. Naming these as pathspecs aborts + // `git add`, and Foundry's own .gitignore lists both, so `forge build` creates them on every + // real target. An earlier attempt at this fix passed its tests only because the fixture never + // created them. + for (const ignoredRoot of ["cache", "out"]) { + mkdirSync(path.join(root, ignoredRoot), { recursive: true }); + writeFileSync(path.join(root, ignoredRoot, "build.json"), "{}\n"); + } + // Every runtime root the stager is supposed to skip, present and ignored. + for (const runtimeRoot of ["node_modules", ".ultrafuzz", ".smithers", "artifacts"]) { + mkdirSync(path.join(root, runtimeRoot, "nested"), { recursive: true }); + writeFileSync(path.join(root, runtimeRoot, "nested", "payload.json"), "runtime\n"); + } + writeFileSync(path.join(root, "UltrafuzzSmoke.t.sol"), "contract UltrafuzzSmoke {}\n"); + + const captured = captureWorkspacePatch(root, baseline); + // The real setup change is captured and no runtime root leaks into the patch. + assert.deepEqual( + captured.manifest.files.map((entry) => entry.path), + ["UltrafuzzSmoke.t.sol"] + ); + // Assert against the captured tree rather than substring-matching the patch text. + const staged = git(root, ["ls-tree", "-r", "--name-only", captured.manifest.result_tree]); + for (const skipped of ["node_modules", ".ultrafuzz", ".smithers", "artifacts", "cache", "out"]) { + assert.equal( + staged.split("\n").some((entry) => entry === skipped || entry.startsWith(`${skipped}/`)), + false, + `${skipped} leaked into the captured tree: ${staged}` + ); + } + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +// A bare `git add -A -- .` avoids the ignored-path error but descends into the runtime roots, so any +// unreadable file under `artifacts/` aborts capture instead — trading #281 for the same failure class. +// The runtime writes `artifacts//` into the workspace while agent subprocesses are still +// running, so this is reachable. Skipped as root, which can read a 0000-mode file regardless. +test("captures a workspace with an unreadable file under a runtime root", { skip: process.getuid?.() === 0 }, () => { + const root = fixture(); + const unreadable = path.join(root, "artifacts", "attempt", "opaque.json"); + try { + mkdirSync(path.dirname(unreadable), { recursive: true }); + writeFileSync(unreadable, "agent output\n"); + chmodSync(unreadable, 0o000); + writeFileSync(path.join(root, "UltrafuzzSmoke.t.sol"), "contract UltrafuzzSmoke {}\n"); + + const tree = captureWorkspaceTree(root); + const staged = git(root, ["ls-tree", "-r", "--name-only", tree]); + assert.doesNotMatch(staged, /artifacts/u); + assert.match(staged, /UltrafuzzSmoke\.t\.sol/u); + } finally { + try { + chmodSync(unreadable, 0o644); + } catch { + // best effort so the fixture can be removed + } + rmSync(root, { recursive: true, force: true }); + } +}); + +// `core.excludesFile` is the lowest-precedence ignore source, so a repo .gitignore negation re-admits +// the path. Naming only the other top-level entries as positive pathspecs is immune to that. +test("keeps runtime roots out even when the repository un-ignores them", () => { + const root = fixture(); + try { + writeFileSync(path.join(root, ".gitignore"), "!artifacts/\n!artifacts/**\n!node_modules\n"); + git(root, ["add", ".gitignore"]); + git(root, ["commit", "--quiet", "-m", "un-ignore runtime roots"]); + mkdirSync(path.join(root, "artifacts", "attempt"), { recursive: true }); + writeFileSync(path.join(root, "artifacts", "attempt", "output.json"), "agent output\n"); + writeFileSync(path.join(root, "UltrafuzzSmoke.t.sol"), "contract UltrafuzzSmoke {}\n"); + + const tree = captureWorkspaceTree(root); + const staged = git(root, ["ls-tree", "-r", "--name-only", tree]); + assert.doesNotMatch(staged, /artifacts/u); + assert.match(staged, /UltrafuzzSmoke\.t\.sol/u); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +// Staging names an explicit entry list, so it must include tracked paths that no longer exist in the +// worktree or a deletion would silently go uncaptured. +test("captures deletion of a tracked top-level path", () => { + const root = fixture(); + try { + writeFileSync(path.join(root, "Removed.t.sol"), "contract Removed {}\n"); + git(root, ["add", "Removed.t.sol"]); + git(root, ["commit", "--quiet", "-m", "add a file to remove"]); + const baseline = captureWorkspaceTree(root); + rmSync(path.join(root, "Removed.t.sol")); + + const captured = captureWorkspacePatch(root, baseline); + assert.deepEqual( + captured.manifest.files.map((entry) => entry.path), + ["Removed.t.sol"] + ); + assert.doesNotMatch(git(root, ["ls-tree", "-r", "--name-only", captured.manifest.result_tree]), /Removed/u); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +// A tracked file whose parent directory is gitignored is still listed by `ls-files --cached`, and +// naming it trips the ignored-path error without `--force`. It must be captured, not dropped: it is +// not under a runtime root, so nothing else would restore it. +test("captures a tracked file inside an ignored directory", () => { + const root = fixture(); + try { + writeFileSync(path.join(root, ".gitignore"), "out/\n"); + mkdirSync(path.join(root, "out"), { recursive: true }); + writeFileSync(path.join(root, "out", "kept.json"), "baseline\n"); + git(root, ["add", ".gitignore"]); + git(root, ["add", "--force", "out/kept.json"]); + git(root, ["commit", "--quiet", "-m", "force-track a file under an ignored directory"]); + const baseline = captureWorkspaceTree(root); + + writeFileSync(path.join(root, "out", "kept.json"), "mutated\n"); + + const captured = captureWorkspacePatch(root, baseline); + assert.deepEqual( + captured.manifest.files.map((entry) => entry.path), + ["out/kept.json"] + ); + assert.equal(git(root, ["show", `${captured.manifest.result_tree}:out/kept.json`]), "mutated\n"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +// A listed path can vanish before it is staged, because agent subprocesses are still running during +// capture. `git add --pathspec-from-file` fails the whole invocation when a name matches nothing, and +// `--ignore-errors` does not suppress it, so capture must re-list and retry instead of aborting the +// run. Simulated with a git wrapper that deletes a listed file on the first `add` only. +test("recovers when a listed path vanishes before it is staged", () => { + const root = fixture(); + const binDir = mkdtempSync(path.join(os.tmpdir(), "ultrafuzz-git-shim-")); + const previousPath = process.env.PATH; + try { + writeFileSync(path.join(root, "transient.tmp"), "written by a still-running subprocess\n"); + writeFileSync(path.join(root, "UltrafuzzSmoke.t.sol"), "contract UltrafuzzSmoke {}\n"); + const marker = path.join(binDir, "fired"); + const realGit = execFileSync("which", ["git"], { encoding: "utf8" }).trim(); + // Shim only the first `add`: delete the transient path so git reports it as unmatched. + writeFileSync( + path.join(binDir, "git"), + [ + "#!/bin/sh", + 'for arg in "$@"; do', + ' if [ "$arg" = "add" ] && [ ! -f ' + JSON.stringify(marker) + " ]; then", + " : > " + JSON.stringify(marker), + " rm -f " + JSON.stringify(path.join(root, "transient.tmp")), + " fi", + "done", + `exec ${JSON.stringify(realGit)} "$@"`, + "" + ].join("\n"), + { mode: 0o755 } + ); + process.env.PATH = `${binDir}${path.delimiter}${previousPath ?? ""}`; + + const tree = captureWorkspaceTree(root); + assert.equal(fs.existsSync(marker), true, "the shim never intercepted an add"); + const staged = git(root, ["ls-tree", "-r", "--name-only", tree]); + assert.match(staged, /UltrafuzzSmoke\.t\.sol/u); + assert.doesNotMatch(staged, /transient\.tmp/u); + } finally { + process.env.PATH = previousPath; + rmSync(binDir, { recursive: true, force: true }); + rmSync(root, { recursive: true, force: true }); + } +}); + +// `ls-files --others` reports an untracked nested repository as a DIRECTORY. Naming it makes `git add` +// fail hard when it has no commit checked out — reachable whenever `forge install` or a clone is +// interrupted, which would kill the node the same way #281 did. +test("captures a workspace containing an untracked nested repository", () => { + const root = fixture(); + try { + // No commit checked out: the interrupted-clone shape. + mkdirSync(path.join(root, "lib", "dep"), { recursive: true }); + git(path.join(root, "lib", "dep"), ["init", "--quiet"]); + writeFileSync(path.join(root, "UltrafuzzSmoke.t.sol"), "contract UltrafuzzSmoke {}\n"); + + const tree = captureWorkspaceTree(root); + const staged = git(root, ["ls-tree", "-r", "--name-only", tree]); + assert.match(staged, /UltrafuzzSmoke\.t\.sol/u); + assert.doesNotMatch(staged, /lib\/dep/u); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +// `--literal-pathspecs` on the `add` is load-bearing: without it a filename that looks like pathspec +// magic is reinterpreted and `git add` dies with "did not match any files". +test("captures filenames that look like pathspec magic or globs", () => { + const root = fixture(); + try { + const names = [":(icase)magic.sol", "[abc].sol", "star*.sol", "q?b.sol", "-dash.sol"]; + for (const name of names) writeFileSync(path.join(root, name), "contract C {}\n"); + + const tree = captureWorkspaceTree(root); + const staged = git(root, ["ls-tree", "-r", "--name-only", tree]); + for (const name of names) { + assert.equal(staged.split("\n").includes(name), true, `${name} missing from ${staged}`); + } + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +// Paths are carried as bytes end to end. Decoding through a string would return replacement +// characters, and the resulting pathspec would match nothing. +test("captures a filename that is not valid UTF-8", () => { + const root = fixture(); + try { + const raw = Buffer.concat([Buffer.from(`${root}/caf`), Buffer.from([0xe9]), Buffer.from(".sol")]); + writeFileSync(raw, "contract C {}\n"); + writeFileSync(path.join(root, "Ok.sol"), "contract Ok {}\n"); + + const tree = captureWorkspaceTree(root); + const staged = git(root, ["ls-tree", "-r", "--name-only", "-z", tree]).split("\0").filter(Boolean); + assert.equal(staged.includes("Ok.sol"), true, JSON.stringify(staged)); + // Three entries: foundry.toml, Ok.sol, and the non-UTF-8 name git reports with an escape. + assert.equal(staged.length, 3, JSON.stringify(staged)); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +// A tracked file under a runtime root is never named for staging, so its index entry stays exactly as +// `read-tree` left it. This pins that a worktree mutation of such a file does not leak into the +// captured tree and, just as importantly, is not recorded as a deletion. +test("leaves tracked runtime-root files at the baseline", () => { + const root = fixture(); + try { + mkdirSync(path.join(root, "artifacts"), { recursive: true }); + writeFileSync(path.join(root, "artifacts", "keep.txt"), "baseline\n"); + git(root, ["add", "artifacts/keep.txt"]); + git(root, ["commit", "--quiet", "-m", "track a runtime-root file"]); + const baseline = captureWorkspaceTree(root); + + writeFileSync(path.join(root, "artifacts", "keep.txt"), "mutated by the agent\n"); + writeFileSync(path.join(root, "UltrafuzzSmoke.t.sol"), "contract UltrafuzzSmoke {}\n"); + + const captured = captureWorkspacePatch(root, baseline); + assert.deepEqual( + captured.manifest.files.map((entry) => entry.path), + ["UltrafuzzSmoke.t.sol"] + ); + // The tracked file is still present in the captured tree, holding its baseline content. + const blob = git(root, ["show", `${captured.manifest.result_tree}:artifacts/keep.txt`]); + assert.equal(blob, "baseline\n"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + test("applies a validated setup patch and rejects a base-tree mismatch", () => { const source = fixture(); // The patch contract is keyed to the exact Git tree. Build the downstream