diff --git a/src/work.mjs b/src/work.mjs index f1c971b5..668d9914 100644 --- a/src/work.mjs +++ b/src/work.mjs @@ -426,9 +426,16 @@ async function dropTree(repo, dir, deps) { /** Everything the attempt changed against the base, as one binary-safe patch. */ async function collectPatch(dir, base, linked, deps) { - const excludes = linked.map((p) => `:(exclude)${p}`); - const add = await deps.git(dir, ["add", "-A", "--", ".", ...excludes]); + // Add everything, then take the links back out. Naming a link in the add + // itself (an exclude pathspec) makes git refuse the whole add when the + // repository's .gitignore already ignores it, which `node_modules` without + // a trailing slash does. + const add = await deps.git(dir, ["add", "-A"]); if (add.code !== 0) return { ok: false, error: `git add failed: ${tail(add.stderr, 400)}` }; + if (linked.length) { + const unstage = await deps.git(dir, ["rm", "-r", "-q", "--cached", "--ignore-unmatch", "--", ...linked]); + if (unstage.code !== 0) return { ok: false, error: `git rm --cached failed: ${tail(unstage.stderr, 400)}` }; + } const diff = await deps.git(dir, ["diff", "--cached", "--binary", base]); if (diff.code !== 0) return { ok: false, error: `git diff failed: ${tail(diff.stderr, 400)}` }; const names = await deps.git(dir, ["diff", "--cached", "--name-only", base]); @@ -604,6 +611,11 @@ export async function runWork(options, { tally.done++; const files = result.files?.length ? ` (${result.files.length} file${result.files.length === 1 ? "" : "s"})` : ""; write(` ✓ ${task.id} passed on attempt ${n}${files}: ${task.title}`); + } else if (passed) { + // Verify passed and the change could not be collected: another attempt + // would pass again and fail the same way. The kept tree has the work. + keepTree = Boolean(tree); + toInbox(task, `verify passed, but the change could not be collected: ${row.error}`, { attempts, tree: tree?.dir }); } else if (n < task.maxAttempts) { finish(task, "pending", { attempts }); tally.retried++; diff --git a/test/work.test.mjs b/test/work.test.mjs index 20ad498a..9497799f 100644 --- a/test/work.test.mjs +++ b/test/work.test.mjs @@ -335,3 +335,43 @@ test("every package's node_modules is linked into the tree, and none of them rea assert.doesNotMatch(fs.readFileSync(task.result.patch, "utf8"), /node_modules/); } finally { s.cleanup(); } }); + +test("a repo that ignores node_modules (no slash) still has its passing change collected", async () => { + const s = sandbox(); + try { + // `node_modules` without a trailing slash also matches the symlink, and + // naming an ignored path in `git add` made git refuse the whole add. + fs.writeFileSync(path.join(s.repo, ".gitignore"), "node_modules\n"); + spawnSync("git", ["-c", "user.name=t", "-c", "user.email=t@t", "add", "-A"], { cwd: s.repo }); + spawnSync("git", ["-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", "ignore"], { cwd: s.repo }); + fs.mkdirSync(path.join(s.repo, "node_modules/.bin"), { recursive: true }); + const { id } = work.addTask("q", { prompt: "make add add", verify: VERIFY, cwd: s.repo }, { env: s.env }); + const calls = []; + const r = await work.runWork({ queue: "q" }, { env: s.env, deps: depsWith(s.env, fixAdd, calls), engineOf: claude }); + assert.equal(r.done, 1, JSON.stringify(work.findTask("q", id, s.env).task)); + assert.equal(calls.length, 1, "one attempt: a pass is not retried"); + const { task } = work.findTask("q", id, s.env); + assert.deepEqual(task.result.files, ["src/add.mjs"]); + } finally { s.cleanup(); } +}); + +test("verify passed but the change cannot be collected: the inbox at once, not a retry", async () => { + const s = sandbox(); + try { + const { id } = work.addTask("q", { prompt: "make add add", verify: VERIFY, cwd: s.repo }, { env: s.env }); + const live = depsWith(s.env, fixAdd); + const calls = []; + const deps = { + ...live, + engine: (...a) => { calls.push(a); return live.engine(...a); }, + git: (cwd, args) => (args[0] === "add" ? Promise.resolve({ code: 1, stdout: "", stderr: "boom", error: null }) : live.git(cwd, args)), + }; + const r = await work.runWork({ queue: "q" }, { env: s.env, deps, engineOf: claude }); + assert.equal(r.inbox, 1); + assert.equal(calls.length, 1); + const { state, task } = work.findTask("q", id, s.env); + assert.equal(state, "inbox"); + assert.match(task.inbox.reason, /verify passed, but the change could not be collected: git add failed: boom/); + assert.ok(fs.existsSync(task.inbox.tree), "the tree with the work in it is kept"); + } finally { s.cleanup(); } +});