diff --git a/packages/code/src/lib/conflict-resolver.ts b/packages/code/src/lib/conflict-resolver.ts index f75b4e9a..bc7e7ea7 100644 --- a/packages/code/src/lib/conflict-resolver.ts +++ b/packages/code/src/lib/conflict-resolver.ts @@ -111,6 +111,61 @@ Instructions: When you are done, every conflict must be resolved and the merge commit created.`; } +/** How many times a failed pre-push hook may be handed to the agent for fixing. */ +const MAX_HOOK_FIX_ATTEMPTS = 2; + +/** Characters of raw hook error output included in the agent's fix prompt. */ +const HOOK_ERROR_PROMPT_LENGTH = 1500; + +/** Build the agent prompt for fixing a failed pre-push hook on the merge commit. */ +export function buildHookFixPrompt(options: { branch: string; hookError: string }): string { + const rawError = options.hookError.trim(); + const excerpt = + rawError.length > HOOK_ERROR_PROMPT_LENGTH + ? `…${rawError.slice(-HOOK_ERROR_PROMPT_LENGTH)}` + : rawError; + return `# Fix Pre-Push Hook Failures + +A \`git push\` of \`${options.branch}\` was rejected by a pre-push hook. The branch +carries a merge commit that has NOT been published yet. + +Hook error output (may be truncated): +\`\`\` +${excerpt || "(no output captured — run the project's check scripts to reproduce)"} +\`\`\` + +Instructions: +1. Reproduce the failing checks with the project's own scripts (lint, format, + typecheck, tests) and fix the underlying issues. Change as little as possible. +2. Stage everything with \`git add -A\`. +3. Fold the fixes into the existing commit with \`git commit --amend --no-edit\` + (only if there is anything to fold in — the commit is a merge commit and must + keep both parents). +4. Do NOT push. Do NOT rebase, reset, or create new commits beyond the amend. +5. Do NOT abort or undo the merge. + +When you are done, the failing checks must pass locally and the working tree +must be clean.`; +} + +/** + * Fold leftover working-tree changes from a hook-fix agent run into the merge + * commit (amend keeps both parents). A no-op when the tree is already clean. + * + * @returns `true` when changes were found and successfully amended in. + */ +async function amendHookFixIntoMergeCommit(workDir: string): Promise { + const status = await Utils.executeGitCommand(["status", "--porcelain"], { cwd: workDir }); + if (!status.success || status.output.trim() === "") { + return false; + } + await Utils.executeGitCommand(["add", "-A"], { cwd: workDir }); + const amend = await Utils.executeGitCommand(["commit", "--amend", "--no-edit"], { + cwd: workDir, + }); + return amend.success; +} + /** * How many times the merge+push cycle may restart when a push is rejected * because the branch moved underneath us (or was rejected transiently). @@ -520,12 +575,59 @@ export async function resolveConflictsOnPr( // The exact lease makes the head check above atomic with the push. The // push helper also verifies that HEAD descends from the leased commit. - const push = await Utils.pushCurrentBranch({ - cwd: workDir, - expectedBranch: branch, - expectedRemoteSha: leaseSha ?? remoteTip, - verbose, - }); + const pushOnce = () => + Utils.pushCurrentBranch({ + cwd: workDir, + expectedBranch: branch, + expectedRemoteSha: leaseSha ?? remoteTip, + verbose, + }); + let push = await pushOnce(); + + // The PR repo's pre-push hooks run inside this ephemeral worktree and + // can fail for fixable reasons (lint, formatting, types). Hand the + // failure to the agent and retry, mirroring the review flow's hook + // fixer; without this the resolved merge never lands on the PR. A + // branch race is not fixable by the agent, so the lease is re-checked + // first to keep the existing defer/retry handling authoritative. + let hookFixAttempt = 0; + while (!push.success && push.hookError && hookFixAttempt < MAX_HOOK_FIX_ATTEMPTS) { + if (leaseSha) { + const refreshed = await Utils.executeGitCommand( + ["fetch", "origin", `+refs/heads/${branch}:refs/remotes/origin/${branch}`], + { cwd: workDir, verbose }, + ); + const moved = refreshed.success + ? await Utils.executeGitCommand(["rev-parse", `origin/${branch}`], { cwd: workDir }) + : null; + const newTip = moved?.success ? moved.output.trim() : null; + if (newTip && newTip !== leaseSha) { + break; // the post-push handling below defers or retries on races + } + } + hookFixAttempt++; + console.log( + `⚠️ Pre-push hook failed (attempt ${hookFixAttempt}/${MAX_HOOK_FIX_ATTEMPTS}); handing to the agent`, + ); + let fixResult: { success: boolean; output: string }; + try { + fixResult = await agentRunner( + buildHookFixPrompt({ branch, hookError: push.hookError }), + workDir, + verbose, + ); + } catch (error) { + fixResult = { success: false, output: (error as Error).message }; + } + // Trust the tree, not the agent's word: fold any leftover changes into + // the merge commit so the retry pushes a complete tree. + const amended = await amendHookFixIntoMergeCommit(workDir); + if (!fixResult.success && !amended) { + console.warn("⚠️ Hook-fix agent run failed; retrying the push anyway"); + } + push = await pushOnce(); + } + if (!push.success) { if (leaseSha) { const refreshed = await Utils.executeGitCommand( diff --git a/packages/code/src/lib/utils.ts b/packages/code/src/lib/utils.ts index 29364b8b..2537e386 100644 --- a/packages/code/src/lib/utils.ts +++ b/packages/code/src/lib/utils.ts @@ -1,6 +1,6 @@ import { fetchWithRetry as sharedFetchWithRetry } from "@devintern/utils"; import { spawn } from "child_process"; -import { existsSync, mkdirSync, readdirSync, rmSync } from "fs"; +import { chmodSync, copyFileSync, existsSync, mkdirSync, readdirSync, rmSync, statSync } from "fs"; import { basename, dirname, join } from "path"; /** @@ -1788,6 +1788,10 @@ export class Utils { if (verbose) { console.log(`📦 Installing dependencies...`); } + // Confine hook rewrites by dependency postinstalls (lefthook) + // to this worktree, before `bun install` gets a chance to + // touch the shared `.git/hooks`. + await Utils.isolateWorktreeHooks(worktreePath, { verbose }); const installResult = await Utils.installDependencies(worktreePath, { verbose }); if (!installResult.success) { @@ -1969,6 +1973,10 @@ export class Utils { if (verbose) { console.log(`📦 Installing dependencies...`); } + // Confine hook rewrites by dependency postinstalls (lefthook) to this + // worktree, before `bun install` gets a chance to touch the shared + // `.git/hooks`. + await Utils.isolateWorktreeHooks(worktreePath, { verbose }); const installResult = await Utils.installDependencies(worktreePath, { verbose, }); @@ -2094,6 +2102,85 @@ export class Utils { await Utils.executeGitCommand(["worktree", "prune"], { verbose: false, cwd }); } + /** + * Point a linked review worktree's `core.hooksPath` at a private directory + * (via per-worktree git config) seeded with copies of the shared hooks. + * + * A linked worktree shares `.git/hooks` with the user's checkout. Dependency + * postinstalls that rewrite hooks (lefthook's `lefthook install`) would + * therefore clobber the user's real hooks with scripts hardcoding this + * ephemeral worktree's `node_modules` path — breaking every later push once + * the worktree is removed. Redirecting hooks first confines those rewrites + * to the private directory, where they stay valid for this run's pushes and + * vanish together with the worktree. + * + * The private directory lives in the worktree's git admin area + * (`/.git/worktrees//hooks`), which keeps it outside the working + * tree (invisible to `git status`, `git clean`, and `git add -A`) and lets + * `git worktree remove` clean it up automatically. The per-worktree config + * lives in the same admin area; `extensions.worktreeConfig` stays enabled in + * the shared config, which is harmless. + */ + static async isolateWorktreeHooks( + worktreePath: string, + options?: { verbose?: boolean }, + ): Promise { + const verbose = options?.verbose ?? false; + + const gitDir = await Utils.executeGitCommand(["rev-parse", "--absolute-git-dir"], { + cwd: worktreePath, + }); + if (!gitDir.success || !gitDir.output.trim()) { + console.warn(`⚠️ Could not locate worktree git dir; hooks stay shared: ${gitDir.error}`); + return; + } + const hooksDir = join(gitDir.output.trim(), "hooks"); + + // Existing shared hooks (e.g. plain scripts installed outside a package's + // postinstall) keep working in the worktree: copy them into the isolated + // directory. This must run before `core.hooksPath` is set, because + // `git rev-parse --git-path hooks` resolves through it. + const sharedHooks = await Utils.executeGitCommand(["rev-parse", "--git-path", "hooks"], { + cwd: worktreePath, + }); + if (sharedHooks.success && sharedHooks.output.trim() && existsSync(sharedHooks.output.trim())) { + const sharedDir = sharedHooks.output.trim(); + try { + Utils.ensureDirectoryExists(hooksDir); + for (const entry of readdirSync(sharedDir)) { + if (entry.endsWith(".sample")) continue; + const source = join(sharedDir, entry); + if (!statSync(source).isFile()) continue; + copyFileSync(source, join(hooksDir, entry)); + chmodSync(join(hooksDir, entry), 0o755); + } + } catch (error) { + console.warn( + `⚠️ Could not copy shared git hooks into the worktree: ${(error as Error).message}`, + ); + } + } + + const enable = await Utils.executeGitCommand(["config", "extensions.worktreeConfig", "true"], { + cwd: worktreePath, + }); + if (!enable.success) { + console.warn(`⚠️ Could not enable per-worktree git config: ${enable.error}`); + return; + } + const setPath = await Utils.executeGitCommand( + ["config", "--worktree", "core.hooksPath", hooksDir], + { cwd: worktreePath }, + ); + if (!setPath.success) { + console.warn(`⚠️ Could not redirect worktree git hooks: ${setPath.error}`); + return; + } + if (verbose) { + console.log(` 🔒 Git hooks isolated to ${hooksDir}`); + } + } + /** * Auto-detect package managers and install project dependencies in a worktree. * diff --git a/packages/code/tests/conflict-resolver.test.ts b/packages/code/tests/conflict-resolver.test.ts index 10178c8c..5a47cbb1 100644 --- a/packages/code/tests/conflict-resolver.test.ts +++ b/packages/code/tests/conflict-resolver.test.ts @@ -6,9 +6,11 @@ import { join } from "path"; import { buildConflictPrompt, + buildHookFixPrompt, resolveConflictsOnPr, sanitizeErrorForPublicComment, } from "../src/lib/conflict-resolver"; +import { Utils } from "../src/lib/utils"; import type { PullRequestInfo } from "../src/lib/github-reviews"; const PR_URL = "https://github.com/acme/widgets/pull/7"; @@ -499,7 +501,11 @@ describe("resolveConflictsOnPr", () => { comments.push(body); }, fetchPr: async () => prInfo(), - agentRunner: async (_prompt, workDir) => { + agentRunner: async (prompt, workDir) => { + if (prompt.includes("Fix Pre-Push Hook Failures")) { + // The hook is permanently broken here; the agent cannot fix it. + return { success: true, output: "cannot fix" }; + } writeFileSync(join(workDir, "greeting.txt"), "resolved\n"); git(workDir, "add -A"); git(workDir, "commit --no-edit"); @@ -540,7 +546,11 @@ exit 1 comments.push(body); }, fetchPr: async () => prInfo(), - agentRunner: async (_prompt, workDir) => { + agentRunner: async (prompt, workDir) => { + if (prompt.includes("Fix Pre-Push Hook Failures")) { + // The hook always leaks and always fails; nothing to fix. + return { success: true, output: "cannot fix" }; + } writeFileSync(join(workDir, "greeting.txt"), "resolved\n"); git(workDir, "add -A"); git(workDir, "commit --no-edit"); @@ -948,6 +958,84 @@ exit 1 rmSync(shipped, { recursive: true, force: true }); }); + test("hands a failed pre-push hook to the agent and pushes once fixed", async () => { + const untouchedHead = git(seedDir, "rev-parse origin/feature/change").trim(); + const marker = join(testDir, "hook-fixed"); + const hookPath = join(repoDir, ".git", "hooks", "pre-push"); + writeFileSync( + hookPath, + `#!/bin/sh\nif [ ! -f "${marker}" ]; then echo 'typecheck failed' >&2; exit 1; fi\n`, + { mode: 0o755 }, + ); + + const prompts: string[] = []; + const result = await resolveConflictsOnPr(PR_URL, { + cwd: repoDir, + noComment: true, + fetchPr: async () => prInfo(), + agentRunner: async (prompt, workDir) => { + prompts.push(prompt); + if (prompt.includes("Fix Pre-Push Hook Failures")) { + // Simulate the agent fixing the underlying hook failure. + writeFileSync(marker, "ok\n"); + return { success: true, output: "fixed" }; + } + writeFileSync(join(workDir, "greeting.txt"), "hello from main and the branch\n"); + git(workDir, "add -A"); + git(workDir, "commit --no-edit"); + return { success: true, output: "done" }; + }, + }); + + expect(result.outcome).toBe("resolved"); + expect(prompts.length).toBe(2); + expect(prompts[1]).toContain("Fix Pre-Push Hook Failures"); + expect(prompts[1]).toContain("typecheck failed"); + expect(prompts[1]).toContain("--amend"); + + // The merge landed on origin's feature branch once the hook passed. + expect( + git(testDir, `--git-dir=${originDir} rev-parse refs/heads/feature/change`).trim(), + ).not.toBe(untouchedHead); + }); + + test("folds hook-fix changes into the merge commit before retrying the push", async () => { + // The hook fails exactly once, so the retry (after the amend) can land. + const marker = join(testDir, "hook-failed-once"); + const hookPath = join(repoDir, ".git", "hooks", "pre-push"); + writeFileSync( + hookPath, + `#!/bin/sh\nif [ ! -f "${marker}" ]; then touch "${marker}"; echo 'formatting drifted' >&2; exit 1; fi\n`, + { mode: 0o755 }, + ); + + const result = await resolveConflictsOnPr(PR_URL, { + cwd: repoDir, + noComment: true, + fetchPr: async () => prInfo(), + agentRunner: async (prompt, workDir) => { + if (prompt.includes("Fix Pre-Push Hook Failures")) { + // The agent fixed the code but forgot to amend; the resolver must + // fold the change into the merge commit itself. + writeFileSync(join(workDir, "hook-fix.txt"), "formatted\n"); + return { success: true, output: "fixed" }; + } + writeFileSync(join(workDir, "greeting.txt"), "hello from main and the branch\n"); + git(workDir, "add -A"); + git(workDir, "commit --no-edit"); + return { success: true, output: "done" }; + }, + }); + + expect(result.outcome).toBe("resolved"); + + // The amend happened: the pushed merge commit carries the hook fix. + const shipped = mkdtempSync(join(tmpdir(), "devintern-conflict-amend-")); + execSync(`git clone -b feature/change ${originDir} ${shipped}/clone`, { stdio: "ignore" }); + expect(existsSync(join(shipped, "clone", "hook-fix.txt"))).toBe(true); + rmSync(shipped, { recursive: true, force: true }); + }); + test("an already-up-to-date branch stays quiet and posts nothing", async () => { // Make the branch genuinely contain main: a real (resolved) merge commit. const fix = join(testDir, "uptodate"); @@ -985,3 +1073,107 @@ exit 1 expect(comments).toEqual([]); }); }); + +describe("isolateWorktreeHooks", () => { + let testDir: string; + let repoDir: string; + let sharedHookPath: string; + + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "devintern-hooks-")); + process.env.GIT_CONFIG_GLOBAL = "/dev/null"; + execSync( + [ + "set -e", + "git init -q bare-origin", + "git clone -q bare-origin repo", + "cd repo", + "git config user.email t@t.co", + "git config user.name T", + "printf 'x\\n' > f.txt", + "git add .", + "git commit -qm init", + "git push -qu origin HEAD:main", + ].join("\n"), + { cwd: testDir, encoding: "utf8", stdio: "ignore" }, + ); + repoDir = join(testDir, "repo"); + sharedHookPath = join(repoDir, ".git", "hooks", "pre-push"); + }); + + afterEach(() => { + if (process.env.GIT_CONFIG_GLOBAL === "/dev/null") delete process.env.GIT_CONFIG_GLOBAL; + rmSync(testDir, { recursive: true, force: true }); + }); + + test("redirects core.hooksPath into the worktree admin dir and copies shared hooks", async () => { + writeFileSync(sharedHookPath, "#!/bin/sh\necho shared-hook\n", { mode: 0o755 }); + const worktreePath = join(testDir, "hooks-worktree"); + execSync(`git worktree add -q -b hooks-branch ${worktreePath} origin/main`, { + cwd: repoDir, + stdio: "ignore", + }); + + await Utils.isolateWorktreeHooks(worktreePath); + + // `core.hooksPath` resolves (per-worktree) into the worktree's own git + // admin area, not the shared `.git/hooks`. + const hooksPath = git(worktreePath, "config core.hooksPath").trim(); + expect(hooksPath).toContain(join(".git", "worktrees")); + expect(hooksPath).not.toBe(join(repoDir, ".git", "hooks")); + // The main checkout's config is untouched (`git config` exits 1 and prints + // nothing when the key is unset in the shared config). + expect( + execSync("git config core.hooksPath || true", { cwd: repoDir, encoding: "utf8" }).trim(), + ).toBe(""); + + // Shared hooks were copied into the isolated directory. + expect(readFileSync(join(hooksPath, "pre-push"), "utf8")).toContain("shared-hook"); + }); + + test("shields the shared hooks from postinstall-style rewrites", async () => { + writeFileSync(sharedHookPath, "#!/bin/sh\necho shared-hook\n", { mode: 0o755 }); + const worktreePath = join(testDir, "hooks-worktree"); + execSync(`git worktree add -q -b hooks-branch ${worktreePath} origin/main`, { + cwd: repoDir, + stdio: "ignore", + }); + + await Utils.isolateWorktreeHooks(worktreePath); + + // Simulate what lefthook's postinstall does: rewrite the hooks it finds + // via `git config core.hooksPath` (which now points inside the worktree). + const hooksPath = git(worktreePath, "config core.hooksPath").trim(); + writeFileSync(join(hooksPath, "pre-push"), "#!/bin/sh\necho rewritten\n", { mode: 0o755 }); + + expect(readFileSync(sharedHookPath, "utf8")).toContain("shared-hook"); + expect(readFileSync(join(hooksPath, "pre-push"), "utf8")).toContain("rewritten"); + }); + + test("runs the isolated hook (not the shared one) on push from the worktree", async () => { + const isolatedStamp = join(testDir, "isolated-hook-ran"); + const sharedStamp = join(testDir, "shared-hook-ran"); + writeFileSync(sharedHookPath, `#!/bin/sh\necho ran > "${sharedStamp}"\n`, { mode: 0o755 }); + const worktreePath = join(testDir, "hooks-worktree"); + execSync(`git worktree add -q -b hooks-branch ${worktreePath} origin/main`, { + cwd: repoDir, + stdio: "ignore", + }); + + await Utils.isolateWorktreeHooks(worktreePath); + + // After isolation, the hook copied into the isolated dir writes elsewhere. + const hooksPath = git(worktreePath, "config core.hooksPath").trim(); + writeFileSync(join(hooksPath, "pre-push"), `#!/bin/sh\necho ran > "${isolatedStamp}"\n`, { + mode: 0o755, + }); + + writeFileSync(join(worktreePath, "new.txt"), "new\n"); + git(worktreePath, "add -A"); + git(worktreePath, "commit -qm new"); + git(worktreePath, "push -q origin hooks-branch"); + + expect(existsSync(isolatedStamp)).toBe(true); + expect(existsSync(sharedStamp)).toBe(false); + }); +});