From 11b145136bc61239eb19fd0fe7f5b11316aa2ec6 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Fri, 31 Jul 2026 08:53:21 +0000 Subject: [PATCH] fix(upstream): treat a failed drift-issue fingerprint search as inconclusive, not absent Both the recorded-issue fast path and the fingerprint-search fallback returned null for a read failure (non-OK response, thrown fetch) exactly as they did for a genuine no-match, so a transient GitHub rate-limit or 5xx during the drift reconcile fell straight through to filing a brand-new issue for a fingerprint that may already have one. Both lookups now return an { existing, errored } pair (mirroring LastCloserResult.errored) so the caller can tell "found nothing" apart from "learned nothing" and skip instead of creating a duplicate on the errored path, leaving the report's stored issue reference untouched. The fingerprint-search pagination walk is also bounded to 10 pages, matching every other list walk in this file, with an exhausted cap now treated as a read failure rather than proof no issue exists. --- src/upstream/ruleset.ts | 107 ++++++++++++++++++++--------- test/unit/upstream-ruleset.test.ts | 90 ++++++++++++++++++++++-- 2 files changed, 159 insertions(+), 38 deletions(-) diff --git a/src/upstream/ruleset.ts b/src/upstream/ruleset.ts index dec422018b..9e5a060178 100644 --- a/src/upstream/ruleset.ts +++ b/src/upstream/ruleset.ts @@ -347,7 +347,19 @@ export async function fileUpstreamDriftIssues(env: Env, manifestOverride?: Upstr let skipped = 0; let unchanged = 0; for (const report of reports) { - const existing = (await validateRecordedGitHubIssue(repo, token, report)) ?? (await findGitHubIssueForFingerprint(repo, token, report.fingerprint)); + const recorded = await validateRecordedGitHubIssue(repo, token, report); + let existing = recorded.existing; + // #10027: a read failure on EITHER lookup (rate-limit, 5xx, thrown fetch, exhausted page cap) means we + // learned nothing about whether an issue already exists for this fingerprint -- falling through to + // createGitHubDriftIssue in that state is exactly what files a duplicate on every failing cron tick. Still + // attempt the fingerprint search when the fast path came back empty (mirrors the original `??` fallback), but + // OR the two errored flags together so either lookup's failure blocks the create below. + let readFailure = recorded.errored; + if (!existing) { + const searched = await findGitHubIssueForFingerprint(repo, token, report.fingerprint); + existing = searched.existing; + readFailure = readFailure || searched.errored; + } if (existing) { // Keep the recorded issue reference correct even when the content below turns out unchanged -- this is a // local D1 write (no GitHub API cost), and it is what lets validateRecordedGitHubIssue's fast path replace @@ -370,6 +382,13 @@ export async function fileUpstreamDriftIssues(env: Env, manifestOverride?: Upstr updated += 1; continue; } + if (readFailure) { + // Neither lookup could confirm an issue is absent -- leave the report's stored issueNumber/issueUrl + // untouched and count it skipped rather than risk filing a duplicate for a fingerprint that may already + // have one. + skipped += 1; + continue; + } const issue = await createGitHubDriftIssue(repo, token, report, assignees); if (!issue) { skipped += 1; @@ -1088,18 +1107,33 @@ function publicDriftReport(report: UpstreamDriftReportRecord): Record | undefined): string[] { return (labels ?? []).map((label) => (typeof label === "string" ? label : (label.name ?? ""))).filter((name) => name.length > 0); } -async function findGitHubIssueForFingerprint(repo: string, token: string, fingerprint: string): Promise { +async function findGitHubIssueForFingerprint(repo: string, token: string, fingerprint: string): Promise { const [owner, name] = repo.split("/"); - if (!owner || !name) return null; + if (!owner || !name) return { existing: null, errored: false }; try { - for (let page = 1; ; page += 1) { + for (let page = 1; page <= FINGERPRINT_SEARCH_PAGE_LIMIT; page += 1) { const url = `https://api.github.com/repos/${owner}/${name}/issues?state=open&labels=signals&per_page=100&page=${page}`; const response = await timeoutFetch(url, { headers: githubHeaders({ token, accept: "application/vnd.github+json" }) }); - if (!response.ok) return null; + if (!response.ok) return { existing: null, errored: true }; const issues = (await response.json()) as Array<{ number?: number; html_url?: string; @@ -1110,18 +1144,24 @@ async function findGitHubIssueForFingerprint(repo: string, token: string, finger const match = issues.find((issue) => issue.body?.includes(`gittensory-upstream-drift:${fingerprint}`)); if (match?.number && match.html_url) return { - number: match.number, - url: match.html_url, - /* v8 ignore next -- unreachable: `match` only exists when `issue.body?.includes(...)` was truthy above, - * which already requires match.body to be a defined, non-empty string. */ - body: match.body ?? null, - labels: githubIssueLabelNames(match.labels), - assignees: (match.assignees ?? []).map((assignee) => assignee.login ?? "").filter((login) => login.length > 0), + existing: { + number: match.number, + url: match.html_url, + /* v8 ignore next -- unreachable: `match` only exists when `issue.body?.includes(...)` was truthy above, + * which already requires match.body to be a defined, non-empty string. */ + body: match.body ?? null, + labels: githubIssueLabelNames(match.labels), + assignees: (match.assignees ?? []).map((assignee) => assignee.login ?? "").filter((login) => login.length > 0), + }, + errored: false, }; - if (!response.headers.get("link")?.includes('rel="next"')) return null; + if (!response.headers.get("link")?.includes('rel="next"')) return { existing: null, errored: false }; } + // Exhausted the page cap without a match or an absent-rel="next" signal -- a truncated search is not proof + // that no issue exists, so this reports a read failure rather than "nothing found". #10027 + return { existing: null, errored: true }; } catch { - return null; + return { existing: null, errored: true }; } } @@ -1151,15 +1191,15 @@ async function updateGitHubDriftIssue(repo: string, token: string, issueNumber: return payload.number && payload.html_url ? { number: payload.number, url: payload.html_url } : null; } -async function validateRecordedGitHubIssue(repo: string, token: string, report: UpstreamDriftReportRecord): Promise { - if (!Number.isInteger(report.issueNumber) || !report.issueNumber || report.issueNumber <= 0 || !report.issueUrl) return null; +async function validateRecordedGitHubIssue(repo: string, token: string, report: UpstreamDriftReportRecord): Promise { + if (!Number.isInteger(report.issueNumber) || !report.issueNumber || report.issueNumber <= 0 || !report.issueUrl) return { existing: null, errored: false }; const parsedUrl = parseGitHubIssueUrl(report.issueUrl); const [owner, name] = repo.split("/"); - if (!owner || !name || !parsedUrl || parsedUrl.number !== report.issueNumber) return null; - if (parsedUrl.owner.toLowerCase() !== owner.toLowerCase() || parsedUrl.name.toLowerCase() !== name.toLowerCase()) return null; + if (!owner || !name || !parsedUrl || parsedUrl.number !== report.issueNumber) return { existing: null, errored: false }; + if (parsedUrl.owner.toLowerCase() !== owner.toLowerCase() || parsedUrl.name.toLowerCase() !== name.toLowerCase()) return { existing: null, errored: false }; try { const response = await timeoutFetch(`https://api.github.com/repos/${owner}/${name}/issues/${report.issueNumber}`, { headers: githubHeaders({ token, accept: "application/vnd.github+json" }) }); - if (!response.ok) return null; + if (!response.ok) return { existing: null, errored: true }; const issue = (await response.json()) as { number?: number; html_url?: string; @@ -1168,23 +1208,26 @@ async function validateRecordedGitHubIssue(repo: string, token: string, report: labels?: Array; assignees?: Array<{ login?: string }>; }; - if (issue.number !== report.issueNumber || !issue.html_url || issue.state !== "open") return null; - if (!issue.body?.includes(`gittensory-upstream-drift:${report.fingerprint}`)) return null; - if (!issue.labels?.some((label) => (typeof label === "string" ? label : label.name)?.toLowerCase() === "signals")) return null; + if (issue.number !== report.issueNumber || !issue.html_url || issue.state !== "open") return { existing: null, errored: false }; + if (!issue.body?.includes(`gittensory-upstream-drift:${report.fingerprint}`)) return { existing: null, errored: false }; + if (!issue.labels?.some((label) => (typeof label === "string" ? label : label.name)?.toLowerCase() === "signals")) return { existing: null, errored: false }; const issueUrl = parseGitHubIssueUrl(issue.html_url); - if (!issueUrl || issueUrl.number !== report.issueNumber) return null; - if (issueUrl.owner.toLowerCase() !== owner.toLowerCase() || issueUrl.name.toLowerCase() !== name.toLowerCase()) return null; + if (!issueUrl || issueUrl.number !== report.issueNumber) return { existing: null, errored: false }; + if (issueUrl.owner.toLowerCase() !== owner.toLowerCase() || issueUrl.name.toLowerCase() !== name.toLowerCase()) return { existing: null, errored: false }; return { - number: report.issueNumber, - url: issue.html_url, - /* v8 ignore next -- unreachable: `issue.body?.includes(...)` above already required issue.body to be a - * defined, non-empty string, or this function would have returned null before reaching here. */ - body: issue.body ?? null, - labels: githubIssueLabelNames(issue.labels), - assignees: (issue.assignees ?? []).map((assignee) => assignee.login ?? "").filter((login) => login.length > 0), + existing: { + number: report.issueNumber, + url: issue.html_url, + /* v8 ignore next -- unreachable: `issue.body?.includes(...)` above already required issue.body to be a + * defined, non-empty string, or this function would have returned null before reaching here. */ + body: issue.body ?? null, + labels: githubIssueLabelNames(issue.labels), + assignees: (issue.assignees ?? []).map((assignee) => assignee.login ?? "").filter((login) => login.length > 0), + }, + errored: false, }; } catch { - return null; + return { existing: null, errored: true }; } } diff --git a/test/unit/upstream-ruleset.test.ts b/test/unit/upstream-ruleset.test.ts index ef2506998c..71c4ed23a2 100644 --- a/test/unit/upstream-ruleset.test.ts +++ b/test/unit/upstream-ruleset.test.ts @@ -902,6 +902,12 @@ describe("upstream ruleset drift tracking", () => { await upsertUpstreamDriftReport(invalidRepoEnv, driftReport("invalid-repo")); await expect(fileUpstreamDriftIssues(invalidRepoEnv)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, skipped: 1 }); + // A leading-slash repo splits to an empty owner segment (as opposed to invalidRepoEnv's missing name + // segment above) -- exercises the other arm of findGitHubIssueForFingerprint's `!owner || !name` guard. + const emptyOwnerRepoEnv = driftEnv({ LOOPOVER_AUTO_FILE_DRIFT_ISSUES: "true", LOOPOVER_DRIFT_ISSUE_TOKEN: "token", LOOPOVER_DRIFT_ISSUE_REPO: "/bad-repo" }); + await upsertUpstreamDriftReport(emptyOwnerRepoEnv, driftReport("empty-owner-repo")); + await expect(fileUpstreamDriftIssues(emptyOwnerRepoEnv)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, skipped: 1 }); + const createEnv = driftEnv({ LOOPOVER_AUTO_FILE_DRIFT_ISSUES: "true", LOOPOVER_DRIFT_ISSUE_TOKEN: "token" }); await upsertUpstreamDriftReport(createEnv, driftReport("create-fingerprint")); vi.stubGlobal("fetch", githubIssueFetch({ create: { number: 77, url: "https://github.com/JSONbored/gittensory/issues/77" } })); @@ -1147,10 +1153,13 @@ describe("upstream ruleset drift tracking", () => { vi.stubGlobal("fetch", githubIssueFetch({ createPayload: {} })); await expect(fileUpstreamDriftIssues(missingPayloadEnv)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, skipped: 1 }); + // #10027: a thrown fingerprint search is a read failure, not "no match" -- it must NOT fall through to create. const throwingListEnv = driftEnv({ LOOPOVER_AUTO_FILE_DRIFT_ISSUES: "true", LOOPOVER_DRIFT_ISSUE_TOKEN: "token" }); await upsertUpstreamDriftReport(throwingListEnv, driftReport("throwing-list")); - vi.stubGlobal("fetch", githubIssueFetch({ throwOnList: true, create: { number: 92, url: "https://github.com/JSONbored/gittensory/issues/92" } })); - await expect(fileUpstreamDriftIssues(throwingListEnv)).resolves.toMatchObject({ status: "completed", created: 1, updated: 0, skipped: 0 }); + const throwingListCalls: GitHubIssueFetchCall[] = []; + vi.stubGlobal("fetch", githubIssueFetch({ throwOnList: true, create: { number: 92, url: "https://github.com/JSONbored/gittensory/issues/92" }, calls: throwingListCalls })); + await expect(fileUpstreamDriftIssues(throwingListEnv)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, skipped: 1 }); + expect(throwingListCalls.some((call) => call.method === "POST")).toBe(false); const linkedEnv = driftEnv({ LOOPOVER_AUTO_FILE_DRIFT_ISSUES: "true", LOOPOVER_DRIFT_ISSUE_TOKEN: "token" }); await upsertUpstreamDriftReport(linkedEnv, driftReport("linked-fingerprint", { issueNumber: 93, issueUrl: "https://github.com/JSONbored/gittensory/issues/93" })); @@ -1211,25 +1220,36 @@ describe("upstream ruleset drift tracking", () => { expect.not.arrayContaining([expect.objectContaining({ method: "PATCH", url: "https://api.github.com/repos/JSONbored/gittensory/issues/125" })]), ); + // #10027: a thrown GET on the recorded-issue fast path is a read failure -- the search fallback still runs + // (mirroring the original `??` fallback) but finding nothing there must not create a duplicate either. const throwingLinkedEnv = driftEnv({ LOOPOVER_AUTO_FILE_DRIFT_ISSUES: "true", LOOPOVER_DRIFT_ISSUE_TOKEN: "token" }); await upsertUpstreamDriftReport(throwingLinkedEnv, driftReport("throwing-linked", { issueNumber: 127, issueUrl: "https://github.com/JSONbored/gittensory/issues/127" })); const throwingLinkedCalls: GitHubIssueFetchCall[] = []; vi.stubGlobal("fetch", githubIssueFetch({ throwOnIssueGet: true, create: { number: 128, url: "https://github.com/JSONbored/gittensory/issues/128" }, calls: throwingLinkedCalls })); - await expect(fileUpstreamDriftIssues(throwingLinkedEnv)).resolves.toMatchObject({ status: "completed", created: 1, updated: 0, skipped: 0 }); + await expect(fileUpstreamDriftIssues(throwingLinkedEnv)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, skipped: 1 }); expect(throwingLinkedCalls).toEqual( expect.not.arrayContaining([expect.objectContaining({ method: "PATCH", url: "https://api.github.com/repos/JSONbored/gittensory/issues/127" })]), ); + expect(throwingLinkedCalls.some((call) => call.method === "POST")).toBe(false); for (const scenario of [ { fingerprint: "wrong-host-linked", issueNumber: 130, issueUrl: "https://example.com/JSONbored/gittensory/issues/130" }, { fingerprint: "wrong-path-linked", issueNumber: 131, issueUrl: "https://github.com/JSONbored/gittensory/pull/131" }, - { fingerprint: "lookup-status-linked", issueNumber: 132, issueUrl: "https://github.com/JSONbored/gittensory/issues/132", issueStatus: 500 }, - { fingerprint: "wrong-number-linked", issueNumber: 133, issueUrl: "https://github.com/JSONbored/gittensory/issues/133", issue: { number: 134, url: "https://github.com/JSONbored/gittensory/issues/133", fingerprint: "wrong-number-linked" } }, + // #10027: a non-OK GET (rate-limit/5xx) on the recorded-issue fast path is a READ FAILURE, not "not + // found" -- it must skip rather than fall through to filing a duplicate. + { fingerprint: "lookup-status-linked", issueNumber: 132, issueUrl: "https://github.com/JSONbored/gittensory/issues/132", issueStatus: 500, readFailure: true }, + // The mock 404s here because its own number-in-URL match fails (it never reaches ruleset.ts's in-body + // number check) -- same non-OK-response read-failure path as the case above. + { fingerprint: "wrong-number-linked", issueNumber: 133, issueUrl: "https://github.com/JSONbored/gittensory/issues/133", issue: { number: 134, url: "https://github.com/JSONbored/gittensory/issues/133", fingerprint: "wrong-number-linked" }, readFailure: true }, { fingerprint: "closed-linked", issueNumber: 135, issueUrl: "https://github.com/JSONbored/gittensory/issues/135", issue: { number: 135, url: "https://github.com/JSONbored/gittensory/issues/135", fingerprint: "closed-linked", state: "closed" } }, { fingerprint: "missing-body-linked", issueNumber: 136, issueUrl: "https://github.com/JSONbored/gittensory/issues/136", issue: { number: 136, url: "https://github.com/JSONbored/gittensory/issues/136", fingerprint: "missing-body-linked", body: null } }, { fingerprint: "missing-label-linked", issueNumber: 137, issueUrl: "https://github.com/JSONbored/gittensory/issues/137", issue: { number: 137, url: "https://github.com/JSONbored/gittensory/issues/137", fingerprint: "missing-label-linked", labels: [{ name: "triage" }] } }, { fingerprint: "nameless-label-linked", issueNumber: 141, issueUrl: "https://github.com/JSONbored/gittensory/issues/141", issue: { number: 141, url: "https://github.com/JSONbored/gittensory/issues/141", fingerprint: "nameless-label-linked", labels: [{}] } }, { fingerprint: "returned-url-linked", issueNumber: 138, issueUrl: "https://github.com/JSONbored/gittensory/issues/138", issue: { number: 138, url: "https://github.com/other/repo/issues/138", fingerprint: "returned-url-linked" } }, + // The returned html_url parses fine and matches the expected owner/repo, but its OWN embedded issue + // number disagrees with the recorded report.issueNumber (distinct from "wrong-number-linked" above, which + // never gets past the fetch itself) -- still a not-found, not a read failure. + { fingerprint: "mismatched-url-number-linked", issueNumber: 142, issueUrl: "https://github.com/JSONbored/gittensory/issues/142", issue: { number: 142, url: "https://github.com/JSONbored/gittensory/issues/999", fingerprint: "mismatched-url-number-linked" } }, ]) { const rejectedLinkedEnv = driftEnv({ LOOPOVER_AUTO_FILE_DRIFT_ISSUES: "true", LOOPOVER_DRIFT_ISSUE_TOKEN: "token" }); await upsertUpstreamDriftReport(rejectedLinkedEnv, driftReport(scenario.fingerprint, { issueNumber: scenario.issueNumber, issueUrl: scenario.issueUrl })); @@ -1243,10 +1263,13 @@ describe("upstream ruleset drift tracking", () => { calls: rejectedLinkedCalls, }), ); - await expect(fileUpstreamDriftIssues(rejectedLinkedEnv)).resolves.toMatchObject({ status: "completed", created: 1, updated: 0, skipped: 0 }); + await expect(fileUpstreamDriftIssues(rejectedLinkedEnv)).resolves.toMatchObject( + scenario.readFailure ? { status: "completed", created: 0, updated: 0, skipped: 1 } : { status: "completed", created: 1, updated: 0, skipped: 0 }, + ); expect(rejectedLinkedCalls).toEqual( expect.not.arrayContaining([expect.objectContaining({ method: "PATCH", url: `https://api.github.com/repos/JSONbored/gittensory/issues/${scenario.issueNumber}` })]), ); + if (scenario.readFailure) expect(rejectedLinkedCalls.some((call) => call.method === "POST")).toBe(false); } const failingLinkedEnv = driftEnv({ LOOPOVER_AUTO_FILE_DRIFT_ISSUES: "true", LOOPOVER_DRIFT_ISSUE_TOKEN: "token" }); @@ -1298,6 +1321,61 @@ describe("upstream ruleset drift tracking", () => { expect(calls.every((call) => !call.startsWith("POST "))).toBe(true); }); + it("REGRESSION (#10027): a failed fingerprint search must not file a duplicate drift issue", async () => { + const env = driftEnv({ LOOPOVER_AUTO_FILE_DRIFT_ISSUES: "true", LOOPOVER_DRIFT_ISSUE_TOKEN: "token" }); + await upsertUpstreamDriftReport(env, driftReport("duplicate-guard-fingerprint")); + const calls: GitHubIssueFetchCall[] = []; + // A 403 on page 1 of the search (the shape a shared REST rate-limit produces) used to fall straight through + // to createGitHubDriftIssue -- assert zero POSTs and the report's stored issue reference stays untouched. + vi.stubGlobal("fetch", githubIssueFetch({ listStatus: 403, create: { number: 999, url: "https://github.com/JSONbored/gittensory/issues/999" }, calls })); + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, skipped: 1 }); + expect(calls.some((call) => call.method === "POST")).toBe(false); + await expect(listUpstreamDriftReports(env)).resolves.toEqual([expect.objectContaining({ issueNumber: null, issueUrl: null })]); + }); + + it("REGRESSION (#10027): a non-OK response on the fingerprint search's page 2 is a read failure, not a truncated match", async () => { + const env = driftEnv({ LOOPOVER_AUTO_FILE_DRIFT_ISSUES: "true", LOOPOVER_DRIFT_ISSUE_TOKEN: "token" }); + await upsertUpstreamDriftReport(env, driftReport("page-2-failure-fingerprint")); + const calls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push(`${method} ${url}`); + if (url.includes("/issues?state=open&labels=signals&per_page=100&page=1")) { + return Response.json( + [{ number: 1, html_url: "https://github.com/JSONbored/gittensory/issues/1", body: "" }], + { headers: { link: '; rel="next"' } }, + ); + } + if (url.includes("/issues?state=open&labels=signals&per_page=100&page=2")) return new Response("rate limited", { status: 403 }); + return new Response("not found", { status: 404 }); + }); + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, skipped: 1 }); + expect(calls.every((call) => !call.startsWith("POST "))).toBe(true); + }); + + it("REGRESSION (#10027): the fingerprint search is bounded to 10 pages, and exhausting the cap is a read failure", async () => { + const env = driftEnv({ LOOPOVER_AUTO_FILE_DRIFT_ISSUES: "true", LOOPOVER_DRIFT_ISSUE_TOKEN: "token" }); + await upsertUpstreamDriftReport(env, driftReport("page-cap-fingerprint")); + const calls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + calls.push(`${init?.method ?? "GET"} ${url}`); + if (url.includes("/issues?state=open&labels=signals&per_page=100&page=")) { + // Every page reports a next page AND never matches -- a pathological repo with more than 1000 open + // `signals`-labeled issues, or an endlessly-looping stub. The cap must stop the walk at page 10. + return Response.json([{ number: 1, html_url: "https://github.com/JSONbored/gittensory/issues/1", body: "" }], { + headers: { link: '; rel="next"' }, + }); + } + return new Response("not found", { status: 404 }); + }); + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, skipped: 1 }); + const listCalls = calls.filter((call) => call.includes("/issues?state=open&labels=signals")); + expect(listCalls).toHaveLength(10); + expect(calls.every((call) => !call.startsWith("POST "))).toBe(true); + }); + it("publishes null report references in upstream status safely", async () => { const env = driftEnv(); await persistUpstreamRulesetSnapshot(env, ruleset("current", "current-hash", "pending_saturation_model", 1, 0.01, new Date().toISOString()));