From cb0dbcd371728dc29983ce9d5b1ff7812f492a79 Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Wed, 20 May 2026 02:43:23 +0000 Subject: [PATCH] feat(action): make prepush hook non-blocking after one failure (#777) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(action): make prepush hook non-blocking after one failure push_branch now treats the repository's prepush hook as best-effort: it runs at most once per run, surfaces the failure output if the script exits non-zero, and every subsequent push_branch call this run skips the hook so the agent isn't blocked by failures unrelated to its change. The agent can iterate by running the hook command itself via the shell tool when shell access is available; push_branch will not re-run the hook automatically after a failure. Why: a one-line OSS-allowlist change took 9 minutes (#776) because the agent retried push_branch six times against a prepush hook that was failing for env-leak and missing-build-artifact reasons unrelated to the change. CI catches the same checks on the GitHub side; the local prepush gate was duplicating work and blocking unrelated fixes. - ToolState: new prepushFailureCount counter (per-run, never resets) - executeLifecycleHook: returns structured failure (kind/output/exitCode) so prepush can compose its own agent-facing message instead of inheriting the generic retry/no-retry advice meant for setup - push_branch: composes a shell-mode-aware error message; surfaces prepushSkipped on the success payload + appends a note to the message - instructions.ts + wiki/prompt.md + docs/comparisons.mdx: updated to reflect best-effort semantics * fix(action): clarify prepush latch semantics + soften static guidance review fixes from PR #777: - toolState comment, instructions, success message, tool description: replace "runs at most once per run" / "first call only" wording with the actual semantic — successful prepush keeps running on later push_branch calls; only a hook FAILURE latches the bypass. - tool description: drop hardcoded "via the shell tool" guidance so the static description doesn't mislead in shell:disabled runs (the dynamic agent prompt in instructions.ts already does shell-conditional messaging). - LifecycleHookFailure.output JSDoc: match the implementation (stderr-preferred fallback to stdout, empty for timeout/spawn). * fix(action): shorten prepush-skip log to terse operator telemetry the previous log line tried to address the agent ("re-run the hook command yourself via shell"), but log.info writes to the action runtime's stdout — the agent never sees it. agent-facing skip guidance already lives in the error message from buildPrepushFailureMessage, the success message when bypassed, and the system prompt in instructions.ts. log line is now just operator telemetry. * refactor(action): drop slop from prepush soft-fail self-audit pass after the previous review-fix round. removed duplication between code-level comment and the five other places that already explain the same behavior, tightened verbose JSDoc, and collapsed redundant clauses in agent-facing strings. - LifecycleHookFailure → discriminated union. drops the optional exitCode/spawnError fields (and the empty-output sentinel for timeout/spawn) plus the corresponding ?? fallbacks in the helper. - PushBranchTool: 7-line code comment above the latch removed (toolState field comment + tool description + error message + success message + system prompt all already cover it). tool description third sentence dropped (restated the second). success message tightened to a parenthetical. - buildPrepushFailureMessage: 4-line JSDoc → 1 line. shared "if you think the failure could indicate a real bug in your code" prefix factored out across the shell-conditional branches. - ToolState.prepushFailureCount comment: 8 lines → 3. the "what" is in git.ts; comment now only documents the invariant (never decremented within a run). - instructions.ts prepush guidance: collapsed nested bullets + ternary into one paragraph; dropped the "so re-running via shell is the only way…" tail that restated "push_branch will NOT re-run it". * fix(action): hint prepush bypass on dirty tree after hook failure When push_branch blocks on a dirty working tree and the prepush latch is already set, tell the agent the hook will be skipped once the tree is clean. * fix(test): narrow CI matrix for lifecycle and toolState changes Remove lifecycle.ts from ALWAYS_RUN_ALL and add lifecycle.ts + toolState.ts to push/git agnostic test coverage so PRs touching prepush latch logic run targeted tests instead of the full matrix. --- agents/postRun.test.ts | 1 + mcp/git.ts | 87 ++++++++++++++++++++++++--------- test/agnostic/gitPerms.ts | 2 + test/agnostic/pushDisabled.ts | 2 + test/agnostic/pushEnabled.ts | 2 + test/agnostic/pushRestricted.ts | 2 + test/coverage.ts | 1 - toolState.ts | 5 ++ utils/instructions.ts | 2 +- utils/lifecycle.ts | 24 +++++++-- 10 files changed, 99 insertions(+), 29 deletions(-) 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.`,