diff --git a/mcp/git.test.ts b/mcp/git.test.ts index 96c2052..a913091 100644 --- a/mcp/git.test.ts +++ b/mcp/git.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { classifyPushError } from "./git.ts"; // re-export the normalizeUrl function for testing // note: in a real scenario, we'd export this from git.ts or move to a shared utils file @@ -61,3 +62,123 @@ describe("push URL validation", () => { expect(pushUrlNormalized).toBe(actualUrlNormalized); }); }); + +describe("classifyPushError", () => { + describe("concurrent-push", () => { + it("matches client-side non-fast-forward (`fetch first`)", () => { + const msg = + "git push failed (exit 1): To https://github.com/o/r.git\n" + + " ! [rejected] feature -> feature (fetch first)\n" + + "error: failed to push some refs to 'https://github.com/o/r.git'\n" + + "hint: Updates were rejected because the remote contains work"; + expect(classifyPushError(msg)).toBe("concurrent-push"); + }); + + it("matches client-side `non-fast-forward` wording", () => { + const msg = "! [rejected] main -> main (non-fast-forward)"; + expect(classifyPushError(msg)).toBe("concurrent-push"); + }); + + it("matches server-side `cannot lock ref` (the case from #571)", () => { + const msg = + "remote: error: cannot lock ref 'refs/heads/feature': is at " + + "abc123 but expected def456\n" + + " ! [remote rejected] feature -> feature (cannot lock ref ...)"; + expect(classifyPushError(msg)).toBe("concurrent-push"); + }); + }); + + describe("transient", () => { + it("matches RPC failed with HTTP 502", () => { + expect( + classifyPushError( + "fatal: unable to access 'https://github.com/o/r.git/': The requested URL returned error: 502" + ) + ).toBe("transient"); + }); + + it("matches early EOF mid-pack", () => { + expect( + classifyPushError("fatal: the remote end hung up unexpectedly\nfatal: early EOF") + ).toBe("transient"); + }); + + it("matches RPC failed", () => { + expect( + classifyPushError("fatal: RPC failed; curl 56 OpenSSL SSL_read: Connection reset by peer") + ).toBe("transient"); + }); + + it("matches HTTP/2 stream not closed cleanly", () => { + expect( + classifyPushError("fatal: HTTP/2 stream 7 was not closed cleanly: PROTOCOL_ERROR (err 1)") + ).toBe("transient"); + }); + + it("matches DNS resolution failure", () => { + expect(classifyPushError("fatal: Could not resolve host: github.com")).toBe("transient"); + }); + + it("matches unexpected disconnect during sideband read", () => { + expect(classifyPushError("fatal: unexpected disconnect while reading sideband packet")).toBe( + "transient" + ); + }); + + it("classifies HTTP 429 (rate-limit / abuse detection) as transient", () => { + // 429 is the documented exception to the otherwise-permanent 4xx class — + // GitHub's abuse detection occasionally surfaces it on git push. + expect( + classifyPushError( + "fatal: unable to access 'https://github.com/o/r.git/': The requested URL returned error: 429" + ) + ).toBe("transient"); + expect(classifyPushError("remote: HTTP 429: too many requests")).toBe("transient"); + }); + }); + + describe("unknown", () => { + it("does NOT classify auth/403 as transient", () => { + // permission denied is permanent within a run — retrying just wastes + // time. must NOT match the HTTP-5xx regex. + expect( + classifyPushError( + "remote: Permission to o/r.git denied to bot.\n" + + "fatal: unable to access 'https://github.com/o/r.git/': The requested URL returned error: 403" + ) + ).toBe("unknown"); + }); + + it("does NOT classify protected-branch rejection as concurrent-push", () => { + expect( + classifyPushError( + " ! [remote rejected] main -> main (push declined due to repository rule violations)" + ) + ).toBe("unknown"); + }); + + it("does NOT classify 404 as transient", () => { + expect( + classifyPushError( + "fatal: unable to access 'https://github.com/o/r.git/': The requested URL returned error: 404" + ) + ).toBe("unknown"); + }); + + it("returns unknown for an empty message", () => { + expect(classifyPushError("")).toBe("unknown"); + }); + }); + + describe("ordering", () => { + it("prefers concurrent-push over transient when both signals appear", () => { + // a server-side cannot-lock-ref response that also includes an HTTP + // 5xx in the libcurl envelope should still route to the recovery + // path, not a blind retry. + const msg = + "remote: error: cannot lock ref 'refs/heads/feature': is at A but expected B\n" + + "fatal: unable to access ...: The requested URL returned error: 500"; + expect(classifyPushError(msg)).toBe("concurrent-push"); + }); + }); +}); diff --git a/mcp/git.ts b/mcp/git.ts index 5c15e5a..f828747 100644 --- a/mcp/git.ts +++ b/mcp/git.ts @@ -153,6 +153,62 @@ export const PushBranch = type({ force: type.boolean.describe("Force push (use with caution)").default(false), }); +// classify an error from `$git("push", ...)` to decide retry vs. recovery +// vs. rethrow. exported for tests. +// +// - `concurrent-push`: server-side compare-and-swap failed because the ref +// advanced between fetch and push. recovery is fetch + integrate + retry. +// matches both the client-side detection (`fetch first` / +// `non-fast-forward`) and the server-side detection (`cannot lock ref` +// with `is at but expected `). +// - `transient`: network or upstream server hiccup (RPC failed mid-stream, +// HTTP 5xx, early EOF, reset, timeout, dns flake). push is idempotent so +// verbatim retry with backoff is safe. +// - `unknown`: anything else (including auth/permission/protected-branch +// rejections). retrying these wastes time; surface to the caller. +// +// kept conservative: a misclassification of `unknown` -> `transient` would +// cause two extra round-trips on a permanently-failing push, while the +// reverse (true transient labeled `unknown`) just falls back to current +// behavior. so we only mark as transient when the error string is +// unambiguously a network/server-side fault, not a refusal. +export type PushErrorKind = "concurrent-push" | "transient" | "unknown"; + +const CONCURRENT_PUSH_PATTERNS = ["fetch first", "non-fast-forward", "cannot lock ref"] as const; + +const TRANSIENT_PATTERNS: RegExp[] = [ + /RPC failed/i, + /early EOF/, + /the remote end hung up unexpectedly/, + /Connection reset/i, + /Could not resolve host/i, + /Operation timed out/i, + /HTTP\/2 stream \d+ was not closed cleanly/i, + /unexpected disconnect while reading sideband packet/i, + // libcurl HTTP 5xx surfaced by git over https. matches both the + // libcurl-style "The requested URL returned error: 502" and the more + // recent "HTTP 502" wording. most 4xx is intentionally excluded — + // 401/403/404 indicate auth/permission problems that are not + // retry-safe — but 429 (rate-limited / abuse detection) IS retry-safe + // and GitHub occasionally surfaces it on git push, so it's included + // explicitly below. + /HTTP 5\d\d/, + /returned error: 5\d\d/i, + /HTTP 429/, + /returned error: 429/i, +]; + +export function classifyPushError(msg: string): PushErrorKind { + if (CONCURRENT_PUSH_PATTERNS.some((p) => msg.includes(p))) return "concurrent-push"; + if (TRANSIENT_PATTERNS.some((p) => p.test(msg))) return "transient"; + return "unknown"; +} + +// backoff delays before retry attempts 2 and 3. attempt 1 is the original +// push. total worst-case added latency: ~7s. small enough that the agent +// rarely notices, large enough to ride out most upstream hiccups. +const TRANSIENT_RETRY_DELAYS_MS = [2000, 5000]; + export function PushBranchTool(ctx: ToolContext) { const defaultBranch = ctx.repo.data.default_branch || "main"; const pushPermission = ctx.payload.push; @@ -234,30 +290,65 @@ export function PushBranchTool(ctx: ToolContext) { log.warning(`force pushing - this will overwrite remote history`); } - try { - await $git("push", pushArgs, { - token: ctx.gitToken, - }); - } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - if (msg.includes("fetch first") || msg.includes("non-fast-forward")) { - // git rebase is blocked through the MCP tool when shell is disabled - // (rebase --exec can execute arbitrary code). merge always works and - // integrates remote changes cleanly, so suggest it as the default. - const integrateStep = - ctx.payload.shell === "disabled" - ? `2. use the git tool to merge the remote branch into yours: git({ command: "merge", args: ["origin/${pushDest.remoteBranch}"] })` - : `2. use the git tool to rebase or merge your changes on top: git({ command: "merge", args: ["origin/${pushDest.remoteBranch}"] }) (or 'rebase')`; - throw new Error( - `push rejected: the remote branch '${pushDest.remoteBranch}' has new commits you don't have locally.\n\n` + - `to resolve this:\n` + - `1. use git_fetch to fetch the remote branch: git_fetch({ ref: "${pushDest.remoteBranch}" })\n` + - `${integrateStep}\n` + - `3. resolve any merge conflicts if needed\n` + - `4. retry push_branch` - ); + // retry transient network/server errors (RPC failed, early EOF, 5xx, + // connection reset, etc) with backoff. push is idempotent: if the remote + // never received the pack, retry creates the ref; if it did, the retry + // is a no-op fast-forward to the same SHA. concurrent-push rejections + // and permission errors are NOT retried — they need user intervention. + let lastErr: unknown; + let pushed = false; + for (let attempt = 0; attempt <= TRANSIENT_RETRY_DELAYS_MS.length; attempt++) { + try { + await $git("push", pushArgs, { + token: ctx.gitToken, + }); + if (attempt > 0) { + log.info(`push succeeded on attempt ${attempt + 1}`); + } + pushed = true; + break; + } catch (err) { + lastErr = err; + const msg = err instanceof Error ? err.message : String(err); + const kind = classifyPushError(msg); + + if (kind === "concurrent-push") { + // git rebase is blocked through the MCP tool when shell is disabled + // (rebase --exec can execute arbitrary code). merge always works and + // integrates remote changes cleanly, so suggest it as the default. + const integrateStep = + ctx.payload.shell === "disabled" + ? `2. use the git tool to merge the remote branch into yours: git({ command: "merge", args: ["origin/${pushDest.remoteBranch}"] })` + : `2. use the git tool to rebase or merge your changes on top: git({ command: "merge", args: ["origin/${pushDest.remoteBranch}"] }) (or 'rebase')`; + throw new Error( + `push rejected: the remote branch '${pushDest.remoteBranch}' has new commits you don't have locally (often a concurrent push to the same branch).\n\n` + + `to resolve this:\n` + + `1. use git_fetch to fetch the remote branch: git_fetch({ ref: "${pushDest.remoteBranch}" })\n` + + `${integrateStep}\n` + + `3. resolve any merge conflicts if needed\n` + + `4. retry push_branch` + ); + } + + if (kind === "transient" && attempt < TRANSIENT_RETRY_DELAYS_MS.length) { + // jitter avoids lockstep retries when several agents are hit by the + // same upstream blip simultaneously — without it, all retries land + // on the same recovering server at the same instant. + const baseDelay = TRANSIENT_RETRY_DELAYS_MS[attempt] ?? 5000; + const delay = Math.round(baseDelay * (0.75 + Math.random() * 0.5)); + log.info( + `push attempt ${attempt + 1} failed (transient), retrying in ${delay}ms: ${msg.slice(0, 300)}` + ); + await new Promise((r) => setTimeout(r, delay)); + continue; + } + + throw err; } - throw err; + } + if (!pushed) { + // safety net — loop should always either break with success or throw. + throw lastErr instanceof Error ? lastErr : new Error(String(lastErr)); } return { diff --git a/utils/gitAuth.ts b/utils/gitAuth.ts index c95cb8f..8d79bac 100644 --- a/utils/gitAuth.ts +++ b/utils/gitAuth.ts @@ -154,8 +154,19 @@ export async function $git( if (result.exitCode !== 0) { const stderr = result.stderr.trim(); - log.info(`git ${subcommand} failed: ${stderr}`); - throw new Error(`git ${subcommand} failed: ${stderr}`); + const stdout = result.stdout.trim(); + // stderr is the primary channel for git diagnostics, but in rare cases + // (e.g. some HTTPS smart-protocol failures) the only useful detail is + // on stdout — without it the agent / operator sees an empty error. + // include exit code so we can distinguish e.g. signal-killed (1 with + // empty output) from a genuine git-level rejection. + const detail = + stderr && stdout + ? `${stderr}\n--- stdout ---\n${stdout}` + : stderr || stdout || "(no output)"; + const message = `git ${subcommand} failed (exit ${result.exitCode}): ${detail}`; + log.info(message); + throw new Error(message); } return {