diff --git a/packages/code/CHANGELOG.md b/packages/code/CHANGELOG.md index fb1b52b..9a38cb2 100644 --- a/packages/code/CHANGELOG.md +++ b/packages/code/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- **Hook-fix verification after a successful push**: if HEAD already matches `origin/`, skip the pre-push `--dry-run` (and the follow-up no-op `git push`). Re-running the hook suite can flake — a 30s CLI test timeout previously aborted PR creation for a branch that was already on the remote, with a misleading "didn't amend" error - **CLI argument tests no longer hang on a live tracker host**: parse-only CLI tests point Jira, Linear, and Trello traffic at a closed local port and disable fetch retries (`DEVINTERN_FETCH_MAX_RETRIES=0`), so a slow remote lookup cannot burn the 30s bun timeout and fail pre-push - **Default branch detection**: repository automations now ask the remote for its authoritative default branch before checkout or fetch operations, avoiding failed `master` attempts for repositories whose default is `main` (and supporting custom default branch names). Cached `origin/HEAD` remains an offline fallback - **Structured agent responses**: feasibility checks now accept the valid bare JSON commonly returned by Codex instead of requiring a Markdown code fence. Feasibility, estimation, and auto-review share brace-aware extraction that also tolerates narration and ignores unrelated braces in prose diff --git a/packages/code/src/lib/git-hook-fixer.ts b/packages/code/src/lib/git-hook-fixer.ts index fddc21d..6bb02a2 100644 --- a/packages/code/src/lib/git-hook-fixer.ts +++ b/packages/code/src/lib/git-hook-fixer.ts @@ -35,6 +35,100 @@ export async function isCommitAlreadyComplete(cwd?: string): Promise { return !(await Utils.hasUncommittedChanges(cwd)); } +/** Result of verifying a pre-push hook-fix attempt. */ +export type PushHookFixVerification = { + success: boolean; + alreadyOnRemote: boolean; + amended: boolean; + message: string; +}; + +/** + * Verify a pre-push hook-fix without assuming a failed dry-run means "didn't amend". + * + * If HEAD already matches `origin/`, skip `git push --dry-run`. That + * dry-run re-executes the full pre-push suite and can flake after the agent + * already published the branch, which previously aborted PR creation. + * + * @param options - Working directory and expected branch + */ +export async function verifyPushHookFix(options?: { + cwd?: string; + expectedBranch?: string; +}): Promise { + const cwd = options?.cwd; + const currentBranch = await Utils.getCurrentBranch(cwd); + + if (options?.expectedBranch && currentBranch !== options.expectedBranch) { + return { + success: false, + alreadyOnRemote: false, + amended: false, + message: `worktree HEAD is on '${currentBranch || "unknown"}' but expected '${options.expectedBranch}'`, + }; + } + + const targetBranch = options?.expectedBranch ?? currentBranch ?? undefined; + let amended = false; + + if (await Utils.hasUncommittedChanges(cwd)) { + const stageResult = await Utils.executeGitCommand(["add", "-A"], { cwd }); + if (!stageResult.success) { + return { + success: false, + alreadyOnRemote: false, + amended: false, + message: `Failed to stage changes: ${stageResult.error}`, + }; + } + + const amendResult = await Utils.executeGitCommand( + ["commit", "--amend", "--no-edit", "--no-verify"], + { cwd }, + ); + if (!amendResult.success) { + return { + success: false, + alreadyOnRemote: false, + amended: false, + message: `Failed to amend commit: ${amendResult.error}`, + }; + } + amended = true; + } + + if (targetBranch && (await Utils.remoteTrackingRefMatchesHead(targetBranch, { cwd }))) { + return { + success: true, + alreadyOnRemote: true, + amended, + message: `HEAD already matches origin/${targetBranch}; skipping hook-rerunning dry-run`, + }; + } + + const pushDryRunArgs = targetBranch + ? ["push", "origin", `HEAD:refs/heads/${targetBranch}`, "--dry-run"] + : ["push", "origin", "HEAD", "--dry-run"]; + const pushDryRun = await Utils.executeGitCommand(pushDryRunArgs, { cwd }); + + if (pushDryRun.success) { + return { + success: true, + alreadyOnRemote: false, + amended, + message: "changes are committed and ready to push", + }; + } + + const detail = [pushDryRun.error, pushDryRun.output].filter(Boolean).join("\n").trim(); + return { + success: false, + alreadyOnRemote: false, + amended, + message: detail || "push dry-run failed", + }; +} + /** * Run agent to fix git hook errors. * @@ -275,68 +369,23 @@ ${hookType === "push" ? "- Make sure to amend the commit (git commit --amend --n } } } else { - // For push fix: verify changes are committed and ready to push. - // Target the expected branch explicitly (HEAD:refs/heads/) - // rather than a bare HEAD, so a stray HEAD can never validate a - // push to the wrong/new remote branch. This is a --dry-run, so it - // never publishes anything. - const pushDryRunArgs = expectedBranch - ? ["push", "origin", `HEAD:refs/heads/${expectedBranch}`, "--dry-run"] - : ["push", "origin", "HEAD", "--dry-run"]; - const pushDryRun = await Utils.executeGitCommand(pushDryRunArgs, { cwd }); - - if (pushDryRun.success) { - console.log( - "✅ Verification successful - changes are committed and ready to push!", - ); - resolve(true); - } else { - console.log( - `⚠️ ${harness.displayName} fixed the code but didn't amend - amending manually...`, - ); - - // Check if there are uncommitted changes to amend - const statusCheck = await Utils.executeGitCommand(["status", "--porcelain"], { - cwd, - }); - if (statusCheck.success && statusCheck.output.trim() !== "") { - // Stage all changes (-A: whole tree, not cwd-limited) - const stageResult = await Utils.executeGitCommand(["add", "-A"], { cwd }); - if (!stageResult.success) { - console.log("❌ Failed to stage changes:"); - console.log(` ${stageResult.error}`); - resolve(false); - return; - } - - // Amend the commit - const amendResult = await Utils.executeGitCommand( - ["commit", "--amend", "--no-edit", "--no-verify"], - { cwd }, - ); - if (amendResult.success) { - console.log("✅ Successfully amended commit manually!"); - - // Verify push would work now (explicit branch ref, dry-run) - const retryPush = await Utils.executeGitCommand(pushDryRunArgs, { cwd }); - if (retryPush.success) { - console.log("✅ Verification successful - ready to push!"); - resolve(true); - } else { - console.log("❌ Push would still fail after amend:"); - console.log(` ${retryPush.error || retryPush.output}`); - resolve(false); - } - } else { - console.log("❌ Failed to amend commit:"); - console.log(` ${amendResult.error}`); - resolve(false); - } + const verification = await verifyPushHookFix({ cwd, expectedBranch }); + if (verification.success) { + if (verification.amended) { + console.log("✅ Successfully amended commit manually!"); + } + if (verification.alreadyOnRemote) { + console.log(`✅ Verification successful - ${verification.message}`); } else { - console.log("❌ Push dry-run failed but no uncommitted changes to amend:"); - console.log(` ${pushDryRun.error || pushDryRun.output}`); - resolve(false); + console.log( + "✅ Verification successful - changes are committed and ready to push!", + ); } + resolve(true); + } else { + console.log("❌ Push verification failed:"); + console.log(` ${verification.message}`); + resolve(false); } } } catch (verifyError) { diff --git a/packages/code/src/lib/utils.ts b/packages/code/src/lib/utils.ts index b757c1e..ea506e4 100644 --- a/packages/code/src/lib/utils.ts +++ b/packages/code/src/lib/utils.ts @@ -870,6 +870,35 @@ export class Utils { return "main"; } + /** + * Whether local HEAD is already published at `origin/`. + * + * Uses the remote-tracking ref (no network). A successful `git push` + * updates that ref, so this is a reliable "already pushed" check that + * does not re-run pre-push hooks. + * + * @param branch - Remote branch name without `refs/heads/` + * @param options - Optional working directory + */ + static async remoteTrackingRefMatchesHead( + branch: string, + options?: { cwd?: string }, + ): Promise { + const gitOptions = options?.cwd ? { cwd: options.cwd } : undefined; + const head = await Utils.executeGitCommand(["rev-parse", "HEAD"], gitOptions); + if (!head.success || !head.output.trim()) { + return false; + } + const remote = await Utils.executeGitCommand( + ["rev-parse", "--verify", `refs/remotes/origin/${branch}`], + gitOptions, + ); + if (!remote.success || !remote.output.trim()) { + return false; + } + return head.output.trim() === remote.output.trim(); + } + /** * Push the current branch to `origin`, setting upstream on first push. * @@ -919,6 +948,22 @@ export class Utils { }; } + // If the agent (or a previous attempt) already published this exact + // commit, do not invoke `git push`. A no-op push still runs pre-push + // hooks, and a flaky hook (e.g. a 30s test timeout) would abort PR + // creation for a branch that is already on the remote. + if (await Utils.remoteTrackingRefMatchesHead(currentBranch, { cwd })) { + if (verbose) { + console.log( + `📤 Branch '${currentBranch}' already matches origin/${currentBranch}; skipping push`, + ); + } + return { + success: true, + message: `Branch '${currentBranch}' is already on remote`, + }; + } + if (verbose) { console.log(`📤 Pushing branch '${currentBranch}' to remote...`); } diff --git a/packages/code/tests/git-hook-fixer.test.ts b/packages/code/tests/git-hook-fixer.test.ts index a172bab..fc92ac2 100644 --- a/packages/code/tests/git-hook-fixer.test.ts +++ b/packages/code/tests/git-hook-fixer.test.ts @@ -1,13 +1,15 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { execSync } from "child_process"; -import { existsSync, mkdirSync, rmSync, writeFileSync } from "fs"; +import { chmodSync, existsSync, mkdirSync, rmSync, writeFileSync } from "fs"; import { tmpdir } from "os"; import { join } from "path"; import { isCommitAlreadyComplete, manualHookFixCommitArgs, MANUAL_HOOK_FIX_COMMIT_MESSAGE, + verifyPushHookFix, } from "../src/lib/git-hook-fixer"; +import { Utils } from "../src/lib/utils"; describe("git-hook-fixer", () => { let testDir: string; @@ -79,3 +81,116 @@ describe("git-hook-fixer", () => { expect(log).toBe(MANUAL_HOOK_FIX_COMMIT_MESSAGE); }); }); + +describe("verifyPushHookFix", () => { + let testDir: string; + let repoDir: string; + let remoteDir: string; + + function git(cwd: string, command: string): string { + return execSync(`git ${command}`, { cwd, encoding: "utf8" }).trim(); + } + + function installFailingPrePushHook(cwd: string): void { + const hookPath = join(cwd, ".git", "hooks", "pre-push"); + writeFileSync( + hookPath, + "#!/bin/sh\necho 'pre-push hook declined (intentional test failure)'\nexit 1\n", + "utf8", + ); + chmodSync(hookPath, 0o755); + } + + beforeEach(() => { + testDir = join( + tmpdir(), + `push-hook-verify-${Date.now()}-${Math.random().toString(36).substring(7)}`, + ); + repoDir = join(testDir, "repo"); + remoteDir = join(testDir, "remote.git"); + mkdirSync(repoDir, { recursive: true }); + mkdirSync(remoteDir, { recursive: true }); + + git(remoteDir, "init --bare"); + git(repoDir, "init"); + git(repoDir, "config user.email 'test@test.com'"); + git(repoDir, "config user.name 'Test User'"); + git(repoDir, "config commit.gpgsign false"); + writeFileSync(join(repoDir, "README.md"), "# Test Repo\n", "utf8"); + git(repoDir, "add ."); + git(repoDir, "commit -m 'Initial commit'"); + git(repoDir, "branch -M main"); + git(repoDir, `remote add origin ${remoteDir}`); + git(repoDir, "checkout -b feature/dev-74"); + git(repoDir, "push -u origin feature/dev-74"); + }); + + afterEach(() => { + try { + if (existsSync(testDir)) { + rmSync(testDir, { recursive: true, force: true }); + } + } catch { + // Ignore cleanup errors + } + }); + + test("succeeds without re-running hooks when HEAD is already on origin", async () => { + installFailingPrePushHook(repoDir); + + const result = await verifyPushHookFix({ cwd: repoDir, expectedBranch: "feature/dev-74" }); + + expect(result.success).toBe(true); + expect(result.alreadyOnRemote).toBe(true); + expect(result.amended).toBe(false); + expect(result.message).toContain("skipping hook-rerunning dry-run"); + }); + + test("reports the actual hook error when the commit is not on origin", async () => { + git(repoDir, "commit --allow-empty -m 'local-only commit'"); + installFailingPrePushHook(repoDir); + + const result = await verifyPushHookFix({ cwd: repoDir, expectedBranch: "feature/dev-74" }); + + expect(result.success).toBe(false); + expect(result.alreadyOnRemote).toBe(false); + expect(result.amended).toBe(false); + expect(result.message.toLowerCase()).not.toContain("didn't amend"); + expect(result.message).toMatch(/pre-push|hook declined|failed to push/i); + }); + + test("amends leftover uncommitted changes before verifying", async () => { + // Use an unpushed branch so amend + dry-run is a first push, not a + // non-fast-forward rewrite of an already-published commit. + git(repoDir, "checkout -b feature/unpushed"); + writeFileSync(join(repoDir, "fix.txt"), "hook fix leftover\n", "utf8"); + + const result = await verifyPushHookFix({ cwd: repoDir, expectedBranch: "feature/unpushed" }); + + expect(result.success).toBe(true); + expect(result.amended).toBe(true); + expect(result.alreadyOnRemote).toBe(false); + expect(await Utils.hasUncommittedChanges(repoDir)).toBe(false); + expect(existsSync(join(repoDir, "fix.txt"))).toBe(true); + expect(git(repoDir, "log -1 --pretty=%s")).toBe("Initial commit"); + }); + + test("pushCurrentBranch skips git push when origin already has HEAD", async () => { + installFailingPrePushHook(repoDir); + + const result = await Utils.pushCurrentBranch({ cwd: repoDir }); + + expect(result.success).toBe(true); + expect(result.message).toContain("already on remote"); + }); + + test("remoteTrackingRefMatchesHead is false after a local-only commit", async () => { + expect(await Utils.remoteTrackingRefMatchesHead("feature/dev-74", { cwd: repoDir })).toBe(true); + + git(repoDir, "commit --allow-empty -m 'not pushed'"); + + expect(await Utils.remoteTrackingRefMatchesHead("feature/dev-74", { cwd: repoDir })).toBe( + false, + ); + }); +});