diff --git a/agents/postRun.test.ts b/agents/postRun.test.ts index d05a10f..c8fe303 100644 --- a/agents/postRun.test.ts +++ b/agents/postRun.test.ts @@ -6,6 +6,7 @@ function makeToolState(overrides: Partial = {}): ToolState { return { progressComment: undefined, hadProgressComment: true, + prepushFailureCount: 0, backgroundProcesses: new Map(), usageEntries: [], ...overrides, diff --git a/mcp/git.ts b/mcp/git.ts index 3aa697d..e0d33c3 100644 --- a/mcp/git.ts +++ b/mcp/git.ts @@ -3,7 +3,7 @@ import { type } from "arktype"; import type { StoredPushDest } from "../toolState.ts"; import { log } from "../utils/cli.ts"; import { $git, $gitFetchWithDeepen } from "../utils/gitAuth.ts"; -import { executeLifecycleHook } from "../utils/lifecycle.ts"; +import { executeLifecycleHook, type LifecycleHookFailure } from "../utils/lifecycle.ts"; import { $ } from "../utils/shell.ts"; import type { ToolContext } from "./server.ts"; import { execute, tool } from "./shared.ts"; @@ -221,7 +221,7 @@ export function PushBranchTool(ctx: ToolContext) { 'Example: `push_branch({})` to push the current branch. Example: `push_branch({ branchName: "pr-1" })` to push a specific local branch. ' + "If specifying branchName, use the LOCAL branch name (e.g., 'pr-1'), not the remote branch name. " + "The correct remote and remote branch are determined automatically from branch config set by checkout_pr. " + - "Requires a clean working tree. Runs the repository prepush hook (if configured) before the network push — hook failure means tests/lint or similar in that script failed, not necessarily a Pullfrog timeout. " + + "Requires a clean working tree. Runs the repository prepush hook (if configured) — best-effort. If the hook fails, the tool returns the failure output and every subsequent call this run skips the hook. " + "Never force push unless explicitly requested. Pushes to the default branch are blocked in restricted mode. " + "If the response reports a timeout, the underlying push may have actually succeeded — verify with `git log origin/` (or this tool with command 'log') before retrying, otherwise you'll push a duplicate.", parameters: PushBranch, @@ -243,7 +243,10 @@ export function PushBranchTool(ctx: ToolContext) { if (status) { throw new Error( `push blocked: working tree is not clean (tracked changes and/or untracked files). commit, discard, or remove stray artifacts before pushing.\n\n` + - `git status:\n${status}` + `git status:\n${status}` + + (ctx.toolState.prepushFailureCount > 0 + ? "\n\nnote: the prepush hook failed earlier this run — once the working tree is clean, push_branch will skip the hook." + : "") ); } @@ -265,27 +268,31 @@ export function PushBranchTool(ctx: ToolContext) { ? ["--force", "-u", pushDest.remoteName, refspec] : ["-u", pushDest.remoteName, refspec]; - // prepush failure should block the push — a passing hook is the gate - // that protects main from bad pushes. - const prepushHook = await executeLifecycleHook({ - event: "prepush", - script: ctx.prepushScript, - }); - if (prepushHook.warning) { - throw new Error(prepushHook.warning); - } + const prepushSkipped = ctx.toolState.prepushFailureCount > 0; + if (prepushSkipped) { + log.info(`» skipping prepush hook (failed earlier this run)`); + } else if (ctx.prepushScript) { + const prepushHook = await executeLifecycleHook({ + event: "prepush", + script: ctx.prepushScript, + }); + if (prepushHook.failure) { + ctx.toolState.prepushFailureCount += 1; + throw new Error(buildPrepushFailureMessage(prepushHook.failure, ctx.payload.shell)); + } - // re-verify clean working tree after prepush. a hook that writes tracked - // files (formatter, type generator, build artifacts) would leave those - // changes uncommitted — pushing now would silently drop them, and the - // agent would report a "successful push" of code the hook had expected - // to be included. - const postHookStatus = $("git", ["status", "--porcelain"], { log: false }); - if (postHookStatus) { - throw new Error( - `push blocked: the prepush hook modified the working tree. those changes are not included in the push. commit or discard them (or change the hook to not mutate tracked files) before retrying.\n\n` + - `git status:\n${postHookStatus}` - ); + // re-verify clean working tree after prepush. a hook that writes tracked + // files (formatter, type generator, build artifacts) would leave those + // changes uncommitted — pushing now would silently drop them, and the + // agent would report a "successful push" of code the hook had expected + // to be included. + const postHookStatus = $("git", ["status", "--porcelain"], { log: false }); + if (postHookStatus) { + throw new Error( + `push blocked: the prepush hook modified the working tree. those changes are not included in the push. commit or discard them (or change the hook to not mutate tracked files) before retrying.\n\n` + + `git status:\n${postHookStatus}` + ); + } } log.debug(`pushing ${branch} to ${pushDest.remoteName}/${pushDest.remoteBranch}`); @@ -359,18 +366,50 @@ export function PushBranchTool(ctx: ToolContext) { `» pushed branch ${branch} to ${pushDest.remoteName}/${pushDest.remoteBranch} (sha ${pushedSha})` ); + const baseMsg = `successfully pushed ${branch} to ${pushDest.remoteName}/${pushDest.remoteBranch}`; + const message = prepushSkipped + ? `${baseMsg} (prepush hook skipped — failed earlier this run).` + : baseMsg; + return { success: true, branch, remoteBranch: pushDest.remoteBranch, remote: pushDest.remoteName, force, - message: `successfully pushed ${branch} to ${pushDest.remoteName}/${pushDest.remoteBranch}`, + prepushSkipped, + message, }; }), }); } +/** agent-facing prepush failure message: script output + bypass guidance, + * with no generic lifecycle retry advice (which would conflict). */ +function buildPrepushFailureMessage( + failure: LifecycleHookFailure, + shell: ToolContext["payload"]["shell"] +): string { + const header = + failure.kind === "exit" + ? `prepush hook failed with exit code ${failure.exitCode}.\n\nscript output:\n${failure.output || "(empty)"}` + : failure.kind === "timeout" + ? `prepush hook timed out — the script is hung or doing too much work.` + : `prepush hook failed to spawn: ${failure.spawnError}.`; + + const ifRealBug = + shell === "disabled" + ? `fix it before pushing again — shell access is disabled in this run, so you can't re-run the hook command yourself.` + : `run the hook command yourself via the shell tool to iterate (push_branch will NOT re-run it).`; + + return ( + `${header}\n\n` + + `this repo's prepush hook is best-effort: the next push_branch call will SKIP the hook and proceed. ` + + `if the failure is unrelated to your changes (pre-existing breakage, flaky check), just call push_branch again. ` + + `if it could be a real bug in your code, ${ifRealBug}` + ); +} + // commands that require authentication - redirect to dedicated tools. // exported so tests can exercise the same table the runtime uses. // diff --git a/test/agnostic/gitPerms.ts b/test/agnostic/gitPerms.ts index ec2f07c..85c3343 100644 --- a/test/agnostic/gitPerms.ts +++ b/test/agnostic/gitPerms.ts @@ -107,6 +107,8 @@ export const test: TestRunnerOptions = { coverage: [ "action/utils/gitAuth.ts", "action/utils/gitAuthServer.ts", + "action/utils/lifecycle.ts", + "action/toolState.ts", "action/mcp/git.ts", "action/mcp/checkout.ts", ], diff --git a/test/agnostic/pushDisabled.ts b/test/agnostic/pushDisabled.ts index b3b7c6c..399ae28 100644 --- a/test/agnostic/pushDisabled.ts +++ b/test/agnostic/pushDisabled.ts @@ -65,6 +65,8 @@ export const test: TestRunnerOptions = { coverage: [ "action/utils/gitAuth.ts", "action/utils/gitAuthServer.ts", + "action/utils/lifecycle.ts", + "action/toolState.ts", "action/mcp/git.ts", "action/mcp/checkout.ts", ], diff --git a/test/agnostic/pushEnabled.ts b/test/agnostic/pushEnabled.ts index 191c4f3..3d8a688 100644 --- a/test/agnostic/pushEnabled.ts +++ b/test/agnostic/pushEnabled.ts @@ -77,6 +77,8 @@ export const test: TestRunnerOptions = { coverage: [ "action/utils/gitAuth.ts", "action/utils/gitAuthServer.ts", + "action/utils/lifecycle.ts", + "action/toolState.ts", "action/mcp/git.ts", "action/mcp/checkout.ts", ], diff --git a/test/agnostic/pushRestricted.ts b/test/agnostic/pushRestricted.ts index 17011fb..a25dbdf 100644 --- a/test/agnostic/pushRestricted.ts +++ b/test/agnostic/pushRestricted.ts @@ -70,6 +70,8 @@ export const test: TestRunnerOptions = { coverage: [ "action/utils/gitAuth.ts", "action/utils/gitAuthServer.ts", + "action/utils/lifecycle.ts", + "action/toolState.ts", "action/mcp/git.ts", "action/mcp/checkout.ts", ], diff --git a/test/coverage.ts b/test/coverage.ts index 48ddc9a..085da40 100644 --- a/test/coverage.ts +++ b/test/coverage.ts @@ -37,7 +37,6 @@ export const ALWAYS_RUN_ALL: string[] = [ "action/index.ts", "action/cli.ts", "action/utils/setup.ts", - "action/utils/lifecycle.ts", "action/utils/install.ts", "action/utils/runFixture.ts", "action/utils/globals.ts", diff --git a/toolState.ts b/toolState.ts index f73224a..73e137d 100644 --- a/toolState.ts +++ b/toolState.ts @@ -89,6 +89,10 @@ export interface ToolState { // then from checkoutSha when review.ts detects new commits mid-review beforeSha?: string; selectedMode?: string; + // number of prepush hook failures this run. push_branch runs the hook + // while this is 0 and skips it once non-zero; never decremented within + // a run. + prepushFailureCount: number; backgroundProcesses: Map; browserDaemon?: BrowserDaemon | undefined; review?: { @@ -186,6 +190,7 @@ export function initToolState(params: InitToolStateParams): ToolState { return { progressComment: resolved, hadProgressComment: !!resolved, + prepushFailureCount: 0, backgroundProcesses: new Map(), usageEntries: [], }; diff --git a/utils/instructions.ts b/utils/instructions.ts index d1b1a83..78030a7 100644 --- a/utils/instructions.ts +++ b/utils/instructions.ts @@ -269,7 +269,7 @@ Rules: - Never push commits directly to the default branch or any protected branch (commonly: main, master, production, develop, staging). Always create a feature branch following the pattern: \`pullfrog/-\` (e.g., \`pullfrog/123-fix-login-bug\`). - Never add co-author trailers (e.g., "Co-authored-by" or "Co-Authored-By") to commit messages. - Untracked files from tests or tooling (e.g. \`coverage/\`) often remain *after* your last commit and still block \`${t("push_branch")}\` — delete them, extend \`.gitignore\`, or only add files that truly belong in the repo. -- \`${t("push_branch")}\` runs the repository's optional **prepush** hook before the network push. If the error includes \`lifecycle hook 'prepush' failed\` (with an exit code and script output after it), the hook script exited non-zero (commonly tests or lint). Fix that or change the hook — do not describe it as an infrastructure "timeout" unless the tool output or logs clearly show a timeout. +- \`${t("push_branch")}\` runs the repository's optional **prepush** hook (commonly tests or lint) — best-effort. On failure the output is returned, the hook is latched off, and every subsequent \`${t("push_branch")}\` call this run skips it. If the failure is unrelated to your changes (pre-existing breakage, env-dependent test, flaky check), just call \`${t("push_branch")}\` again. If it could be a real bug in your code, ${ctx.payload.shell === "disabled" ? `fix it from the failure output (shell is disabled, so you can't re-run the hook)` : `re-run the hook via the shell tool to iterate — \`${t("push_branch")}\` itself won't re-run it`}. Don't describe the failure as an infrastructure "timeout" unless the tool output clearly shows one. - If push or PR creation fails, \`${t("report_progress")}\` must summarize using the **actual** error from the tool. Do not substitute vague causes unless they match what failed. ### GitHub diff --git a/utils/lifecycle.ts b/utils/lifecycle.ts index d5a64a3..6112333 100644 --- a/utils/lifecycle.ts +++ b/utils/lifecycle.ts @@ -12,22 +12,37 @@ export interface ExecuteLifecycleHookParams { script: string | null; } +/** structured failure info — `output` on the `exit` variant is trimmed + * stderr, falling back to stdout when stderr is empty. */ +export type LifecycleHookFailure = + | { kind: "exit"; exitCode: number; output: string } + | { kind: "timeout" } + | { kind: "spawn"; spawnError: string }; + export interface LifecycleHookResult { /** * human-readable warning when the hook failed. includes retry guidance: * transient spawn/exit errors are worth retrying, timeouts and * persistent failures are not. absent when the hook succeeded or was - * skipped. + * skipped. setup/post-checkout callers surface this verbatim; prepush + * builds its own message from `failure` instead. */ warning?: string; + /** + * structured failure info — undefined when the hook succeeded or was + * skipped. lets callers compose their own messaging without parsing the + * `warning` string. + */ + failure?: LifecycleHookFailure; } /** * execute a lifecycle hook script if one is configured. * * soft-fails: instead of throwing on hook errors, returns a warning string - * so callers can choose whether to surface it (mcp tools) or upgrade it to - * a fatal error (setup/prepush). timeouts are flagged as non-retryable. + * (and structured failure info) so callers can choose whether to surface + * it (mcp tools) or upgrade it to a fatal error (setup). timeouts are + * flagged as non-retryable in the warning text. */ export async function executeLifecycleHook( params: ExecuteLifecycleHookParams @@ -50,6 +65,7 @@ export async function executeLifecycleHook( if (result.exitCode !== 0) { const output = (result.stderr || result.stdout).trim(); return { + failure: { kind: "exit", output, exitCode: result.exitCode }, warning: `lifecycle hook '${params.event}' failed with exit code ${result.exitCode}. ` + `output: ${output || "(empty)"}. ` + @@ -67,6 +83,7 @@ export async function executeLifecycleHook( if (isTimeout) { const minutes = Math.round(LIFECYCLE_HOOK_TIMEOUT_MS / 60000); return { + failure: { kind: "timeout" }, warning: `lifecycle hook '${params.event}' timed out after ${minutes}min. ` + `do NOT retry — the script is likely hung or doing too much work. ` + @@ -75,6 +92,7 @@ export async function executeLifecycleHook( } const msg = err instanceof Error ? err.message : String(err); return { + failure: { kind: "spawn", spawnError: msg }, warning: `lifecycle hook '${params.event}' failed to spawn: ${msg}. ` + `this is likely a transient failure — retry the operation.`,