fix(push_branch): retry transient push errors and surface full stderr/stdout (#573)

* fix(push_branch): retry transient push errors and surface full stderr/stdout

issue #571 motivated three small improvements to `mcp__pullfrog__push_branch`:

1. classify push errors into `concurrent-push` / `transient` / `unknown`.
   - `concurrent-push` extends the existing `fetch first` / `non-fast-forward`
     matcher to also catch the server-side `cannot lock ref` form (the case
     #571 reports). all three route to the same fetch + integrate + retry
     recovery message; copy now mentions concurrent push as a likely cause.
   - `transient` covers RPC failed, early EOF, connection reset, dns flake,
     HTTP 5xx, HTTP/2 stream not closed, and unexpected sideband disconnect.
     these are retried in-tool with 2s + 5s backoff before surfacing the
     error. push is idempotent so verbatim retry is safe.
   - `unknown` (auth/permission/protected-branch/4xx) is rethrown unchanged —
     retrying these wastes time and noise.

2. surface stdout alongside stderr in `$git` failure messages and include the
   exit code. previously only `stderr.trim()` was forwarded, which could be
   empty in rare HTTPS failure modes (the agent on issue #571's run saw a
   one-line `failed to push some refs` and had nothing to diagnose with).

3. unit tests for the classifier covering all three branches plus the
   concurrent-push-wins-over-transient ordering.

does not introduce auto fetch+rebase+retry inside the tool — that path is
blocked under shell=disabled, can leave the working tree mid-conflict, and
would create unwanted merge commits. the recovery message keeps the agent
in the loop.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(push_branch): retry 429, jitter backoff, downgrade retry log to info

- treat HTTP 429 (rate-limit / abuse detection) as transient — GitHub
  occasionally surfaces it on git push, where it is retry-safe unlike
  401/403/404
- add ±25% jitter to backoff so concurrent agents hit by the same
  upstream blip don't retry in lockstep
- log retries with log.info instead of log.warning to match retry.ts
  convention; a successful retry shouldn't leave a yellow GHA annotation
  behind in the job summary

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>

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");
+    });
+  });
+});

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 <SHA1> but expected <SHA2>`).
+// - `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 {

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 {