Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/code/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Fixed

- **Hook-fix verification after a successful push**: if HEAD already matches `origin/<branch>`, 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
Expand Down
169 changes: 109 additions & 60 deletions packages/code/src/lib/git-hook-fixer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,100 @@ export async function isCommitAlreadyComplete(cwd?: string): Promise<boolean> {
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/<branch>`, 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<PushHookFixVerification> {
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.
*
Expand Down Expand Up @@ -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/<branch>)
// 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) {
Expand Down
45 changes: 45 additions & 0 deletions packages/code/src/lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,35 @@ export class Utils {
return "main";
}

/**
* Whether local HEAD is already published at `origin/<branch>`.
*
* 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<boolean> {
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.
*
Expand Down Expand Up @@ -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...`);
}
Expand Down
117 changes: 116 additions & 1 deletion packages/code/tests/git-hook-fixer.test.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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,
);
});
});
Loading