b6e2c61d30
* 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>
185 lines
6.7 KiB
TypeScript
185 lines
6.7 KiB
TypeScript
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
|
|
function normalizeUrl(url: string): string {
|
|
return url.replace(/\.git$/, "").toLowerCase();
|
|
}
|
|
|
|
describe("normalizeUrl", () => {
|
|
it("removes .git suffix", () => {
|
|
expect(normalizeUrl("https://github.com/owner/repo.git")).toBe("https://github.com/owner/repo");
|
|
});
|
|
|
|
it("lowercases URL", () => {
|
|
expect(normalizeUrl("https://github.com/Owner/Repo")).toBe("https://github.com/owner/repo");
|
|
});
|
|
|
|
it("handles URL without .git suffix", () => {
|
|
expect(normalizeUrl("https://github.com/owner/repo")).toBe("https://github.com/owner/repo");
|
|
});
|
|
|
|
it("handles combined case and .git suffix", () => {
|
|
expect(normalizeUrl("https://github.com/OWNER/REPO.git")).toBe("https://github.com/owner/repo");
|
|
});
|
|
});
|
|
|
|
describe("push URL validation", () => {
|
|
// these tests document the expected behavior
|
|
// actual integration testing happens via the agent test suite
|
|
|
|
it("should block push when actual URL differs from pushUrl", () => {
|
|
// pushUrl is set by setupGit (base repo) or checkout_pr (fork repo)
|
|
const pushUrl = "https://github.com/fork-owner/repo.git";
|
|
const actualUrl = "https://github.com/base-owner/repo.git"; // different repo
|
|
|
|
const pushUrlNormalized = normalizeUrl(pushUrl);
|
|
const actualUrlNormalized = normalizeUrl(actualUrl);
|
|
|
|
expect(pushUrlNormalized).not.toBe(actualUrlNormalized);
|
|
// in real code, this mismatch would throw an error
|
|
});
|
|
|
|
it("should allow push when actual URL matches pushUrl", () => {
|
|
const pushUrl = "https://github.com/fork-owner/repo.git";
|
|
const actualUrl = "https://github.com/fork-owner/repo"; // same repo, no .git
|
|
|
|
const pushUrlNormalized = normalizeUrl(pushUrl);
|
|
const actualUrlNormalized = normalizeUrl(actualUrl);
|
|
|
|
expect(pushUrlNormalized).toBe(actualUrlNormalized);
|
|
// in real code, this would allow the push
|
|
});
|
|
|
|
it("should handle case differences in URLs", () => {
|
|
const pushUrl = "https://github.com/Owner/Repo.git";
|
|
const actualUrl = "https://github.com/owner/repo";
|
|
|
|
const pushUrlNormalized = normalizeUrl(pushUrl);
|
|
const actualUrlNormalized = normalizeUrl(actualUrl);
|
|
|
|
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");
|
|
});
|
|
});
|
|
});
|