Skip to content

Commit f50d010

Browse files
authored
fix(github): reuse an existing repo-docs branch ref instead of failing (#8310) (#8481)
openRepoDocPullRequest always targets the fixed loopover/repo-docs branch, but only looks for an OPEN pull request on it before deciding to create the ref. A maintainer who closes the repo-doc PR without deleting its branch (GitHub never forces that, and an API/bot close deletes nothing) leaves the ref behind: the open-only lookup finds nothing, POST /git/refs then 422s "Reference already exists", and the outer catch turns every later refresh into { opened: false }, permanently. When the create reports that specific failure, fall back to PATCH /git/refs/{ref} with { sha, force: true } and continue into the existing PR-open path unchanged. The branch is owned solely by this feature (per the file's own header comment), never shared with contributor work, so force- updating it to the freshly built commit is safe. Deliberately narrow: - Only a 422 whose message says "Reference already exists" recovers; any other create-ref failure still propagates to the outer catch's fail-safe, so a bad sha is never force-pushed over. - The "reuse only an OPEN PR" behaviour is untouched -- a closed PR still results in a NEW pull request, only the underlying ref is reused. - Reuses the existing githubErrorStatus and errorMessage helpers rather than hand-rolling status/message extraction. Adds two tests: the 422 path force-updates the ref and still opens a fresh PR (reused: false), and an unrelated create-ref 422 issues no PATCH and degrades to opened: false.
1 parent df753da commit f50d010

2 files changed

Lines changed: 88 additions & 1 deletion

File tree

src/github/repo-doc-pr.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
// module's own design intent), so a skill-only content change can still open a PR even when AGENTS.md itself
2626
// is unchanged, and a skill-file conflict (manual-review-required without the overwrite opt-in) only excludes
2727
// the skill from this run rather than blocking the AGENTS.md refresh it rode in with.
28+
import { errorMessage } from "../utils/json";
2829
import { githubErrorStatus, withInstallationTokenRetry } from "./app";
2930
import { githubRateLimitAdmissionKeyForInstallation, makeInstallationOctokit } from "./client";
3031
import { LOOPOVER_SITE_URL } from "./footer";
@@ -59,6 +60,13 @@ function splitRepo(repoFullName: string): { owner: string; repo: string } {
5960
type DocTreeEntry = { path: string; mode: "100644" | "120000"; type: "blob"; content: string };
6061
type Octokit = ReturnType<typeof makeInstallationOctokit>;
6162

63+
/** #8310: GitHub answers a create-ref for an existing branch with 422 "Reference already exists". Matched on
64+
* BOTH the status and the message so an unrelated 422 (e.g. a bad sha) still fails loudly instead of being
65+
* silently force-updated. */
66+
function isRefAlreadyExistsError(error: unknown): boolean {
67+
return githubErrorStatus(error) === 422 && /reference already exists/i.test(errorMessage(error));
68+
}
69+
6270
// GitHub's Contents API base64-encodes the file's raw bytes (with line-wrapped whitespace); decoding through
6371
// atob + TextDecoder (rather than a naive charCodeAt reassembly) is what makes this correct for non-ASCII
6472
// manual content a maintainer added outside the generated markers.
@@ -224,7 +232,18 @@ export async function openRepoDocPullRequest(env: Env, repoFullName: string, mod
224232
const commit = await octokit.request("POST /repos/{owner}/{repo}/git/commits", { owner, repo, message: PR_TITLE, tree: treeSha, parents: [baseCommitSha] });
225233
const commitSha = (commit.data as { sha: string }).sha;
226234

227-
await octokit.request("POST /repos/{owner}/{repo}/git/refs", { owner, repo, ref: `refs/heads/${REPO_DOC_BRANCH_NAME}`, sha: commitSha });
235+
// #8310: the ref can already exist with NO open PR on it -- a maintainer closed the previous repo-doc PR
236+
// without deleting its branch (GitHub never forces that, and an API/bot close deletes nothing). The
237+
// open-only PR lookup above then finds nothing, so we reach here and POST /git/refs 422s "Reference
238+
// already exists", failing the whole refresh forever. This branch is exclusively owned by this feature
239+
// (see the header comment), never shared with contributor work, so force-updating it to the freshly
240+
// built commit is safe. Any OTHER failure still propagates to the outer catch's fail-safe.
241+
try {
242+
await octokit.request("POST /repos/{owner}/{repo}/git/refs", { owner, repo, ref: `refs/heads/${REPO_DOC_BRANCH_NAME}`, sha: commitSha });
243+
} catch (refError) {
244+
if (!isRefAlreadyExistsError(refError)) throw refError;
245+
await octokit.request("PATCH /repos/{owner}/{repo}/git/refs/{ref}", { owner, repo, ref: `heads/${REPO_DOC_BRANCH_NAME}`, sha: commitSha, force: true });
246+
}
228247

229248
const pr = await octokit.request("POST /repos/{owner}/{repo}/pulls", {
230249
owner,

test/unit/repo-doc-pr.test.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,74 @@ describe("openRepoDocPullRequest (#3000)", () => {
212212
expect(prCall?.body.body as string).toContain("LoopOver opened this pull request");
213213
});
214214

215+
// #8310: a maintainer closing the repo-doc PR WITHOUT deleting its branch left the ref behind. The open-only
216+
// PR lookup then finds nothing, create-ref 422s "Reference already exists", and every later refresh failed
217+
// permanently. The branch is owned solely by this feature, so force-updating it is safe.
218+
it("force-updates the existing branch ref and still opens a fresh PR when the branch survived a closed PR", async () => {
219+
const env = envWithKey();
220+
await seedInstalledRepo(env, { defaultBranch: "main" });
221+
await seedProfileData(env);
222+
await seedRepoDocGenerationConfig(env, REPO);
223+
const calls: Array<{ method: string; url: string; body: Record<string, unknown> }> = [];
224+
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
225+
const url = input.toString();
226+
if (TOKEN_URL.test(url)) return Response.json({ token: "t" });
227+
const method = init?.method ?? "GET";
228+
calls.push({ method, url, body: init?.body ? JSON.parse(String(init.body)) : {} });
229+
if (url.includes("/pulls?") && method === "GET") return Response.json([]); // the closed PR is not returned
230+
if (url.includes("/contents/AGENTS.md") && method === "GET") return new Response("not found", { status: 404 });
231+
if (url.includes("/contents/CLAUDE.md") && method === "GET") return new Response("not found", { status: 404 });
232+
if (url.endsWith("/branches/main")) return Response.json({ commit: { sha: "base-commit-sha", commit: { tree: { sha: "base-tree-sha" } } } });
233+
if (url.endsWith("/git/trees") && method === "POST") return Response.json({ sha: "new-tree-sha" });
234+
if (url.endsWith("/git/commits") && method === "POST") return Response.json({ sha: "new-commit-sha" });
235+
if (url.endsWith("/git/refs") && method === "POST") {
236+
return Response.json({ message: "Reference already exists" }, { status: 422 });
237+
}
238+
if (method === "PATCH" && url.includes("/git/refs/")) return Response.json({ ref: "refs/heads/loopover/repo-docs" });
239+
if (url.endsWith("/repos/owner/widgets/pulls") && method === "POST") return Response.json({ number: 43, html_url: "https://github.com/owner/widgets/pull/43" });
240+
return new Response("unexpected", { status: 500 });
241+
});
242+
243+
const result = await openRepoDocPullRequest(env, REPO, "live");
244+
245+
// A brand-new PR is opened (reused: false) -- a CLOSED PR is never silently revived.
246+
expect(result).toMatchObject({ opened: true, reused: false, pullNumber: 43 });
247+
const patchCall = calls.find((c) => c.method === "PATCH" && c.url.includes("/git/refs/"));
248+
expect(patchCall, "expected a force ref-update after the create 422'd").toBeTruthy();
249+
expect(patchCall?.body).toMatchObject({ sha: "new-commit-sha", force: true });
250+
});
251+
252+
// #8310 (other arm): only "Reference already exists" is recovered. Any other create-ref failure must still
253+
// degrade through the existing fail-safe rather than being force-pushed over.
254+
it("does NOT force-update the ref for an unrelated create-ref failure, degrading to opened:false", async () => {
255+
const env = envWithKey();
256+
await seedInstalledRepo(env, { defaultBranch: "main" });
257+
await seedProfileData(env);
258+
await seedRepoDocGenerationConfig(env, REPO);
259+
const calls: Array<{ method: string; url: string; body: Record<string, unknown> }> = [];
260+
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
261+
const url = input.toString();
262+
if (TOKEN_URL.test(url)) return Response.json({ token: "t" });
263+
const method = init?.method ?? "GET";
264+
calls.push({ method, url, body: init?.body ? JSON.parse(String(init.body)) : {} });
265+
if (url.includes("/pulls?") && method === "GET") return Response.json([]);
266+
if (url.includes("/contents/AGENTS.md") && method === "GET") return new Response("not found", { status: 404 });
267+
if (url.includes("/contents/CLAUDE.md") && method === "GET") return new Response("not found", { status: 404 });
268+
if (url.endsWith("/branches/main")) return Response.json({ commit: { sha: "base-commit-sha", commit: { tree: { sha: "base-tree-sha" } } } });
269+
if (url.endsWith("/git/trees") && method === "POST") return Response.json({ sha: "new-tree-sha" });
270+
if (url.endsWith("/git/commits") && method === "POST") return Response.json({ sha: "new-commit-sha" });
271+
if (url.endsWith("/git/refs") && method === "POST") {
272+
return Response.json({ message: "Invalid request. sha is not a valid commit" }, { status: 422 });
273+
}
274+
return new Response("unexpected", { status: 500 });
275+
});
276+
277+
const result = await openRepoDocPullRequest(env, REPO, "live");
278+
279+
expect(result.opened).toBe(false);
280+
expect(calls.some((c) => c.method === "PATCH")).toBe(false);
281+
});
282+
215283
// #4613: a self-hoster's PUBLIC_SITE_ORIGIN reaches the generated AGENTS.md's attribution link instead
216284
// of LOOPOVER_SITE_URL. NOTE: createTestEnv() defaults PUBLIC_SITE_ORIGIN to a truthy value (matching
217285
// LOOPOVER_SITE_URL's own string), so envWithKey() alone does NOT exercise the `??` fallback's nullish

0 commit comments

Comments
 (0)