diff --git a/src/upstream/ruleset.ts b/src/upstream/ruleset.ts index 3f7963f378..3a1b2fc5fd 100644 --- a/src/upstream/ruleset.ts +++ b/src/upstream/ruleset.ts @@ -291,9 +291,22 @@ export async function fileUpstreamDriftIssues(env: Env): Promise { @@ -1013,7 +1026,15 @@ function publicDriftReport(report: UpstreamDriftReportRecord): Record { +/** The subset of a resolved existing GitHub drift issue needed to detect whether a fresh PATCH would be a no-op + * (#4503) -- body/labels/assignees are exactly the three fields updateGitHubDriftIssue's payload writes. */ +type ExistingDriftIssue = { number: number; url: string; body: string | null; labels: string[]; assignees: string[] }; + +function githubIssueLabelNames(labels: Array | 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 { const [owner, name] = repo.split("/"); if (!owner || !name) return null; try { @@ -1021,9 +1042,24 @@ async function findGitHubIssueForFingerprint(repo: string, token: string, finger 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, "application/vnd.github+json") }); if (!response.ok) return null; - const issues = (await response.json()) as Array<{ number?: number; html_url?: string; body?: string | null }>; + const issues = (await response.json()) as Array<{ + number?: number; + html_url?: string; + body?: string | null; + labels?: Array; + assignees?: Array<{ login?: string }>; + }>; 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 }; + 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), + }; if (!response.headers.get("link")?.includes('rel="next"')) return null; } } catch { @@ -1057,7 +1093,7 @@ 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<{ number: number; url: string } | 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; const parsedUrl = parseGitHubIssueUrl(report.issueUrl); const [owner, name] = repo.split("/"); @@ -1066,14 +1102,29 @@ async function validateRecordedGitHubIssue(repo: string, token: string, report: try { const response = await timeoutFetch(`https://api.github.com/repos/${owner}/${name}/issues/${report.issueNumber}`, { headers: githubHeaders(token, "application/vnd.github+json") }); if (!response.ok) return null; - const issue = (await response.json()) as { number?: number; html_url?: string; state?: string; body?: string | null; labels?: Array }; + const issue = (await response.json()) as { + number?: number; + html_url?: string; + state?: string; + body?: string | null; + 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; 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; - return { number: report.issueNumber, url: issue.html_url }; + 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), + }; } catch { return null; } @@ -1112,15 +1163,36 @@ export function resolveDriftAssignees(env: Env): string[] { .filter((login) => login.length > 0); } +function githubDriftIssueLabels(report: UpstreamDriftReportRecord): string[] { + return ["signals", "scoring", "data", report.severity === "high" || report.severity === "blocking" ? "high-impact" : "backend"]; +} + function githubDriftIssuePayload(report: UpstreamDriftReportRecord, assignees: string[]): Record { return { title: githubDriftIssueTitle(report), body: githubDriftIssueBody(report), - labels: ["signals", "scoring", "data", report.severity === "high" || report.severity === "blocking" ? "high-impact" : "backend"], + labels: githubDriftIssueLabels(report), assignees, }; } +/** Would a fresh PATCH of `existing` with `report`/`assignees` change anything on GitHub? (#4503) Compares + * against the issue's LIVE state (already fetched by validateRecordedGitHubIssue / findGitHubIssueForFingerprint) + * rather than any locally-stored copy — ground truth, no extra fetch or DB column needed. Body is an exact + * string match: githubDriftIssueBody has no always-changing field (no timestamp), so an unresolved report with + * unchanged content produces a byte-identical body every cycle. Labels/assignees are compared as + * case-insensitive SETS, not ordered arrays — GitHub does not guarantee either the order or the casing it + * echoes back matches what we last sent. */ +function driftIssueUnchanged(existing: ExistingDriftIssue, report: UpstreamDriftReportRecord, assignees: string[]): boolean { + if (existing.body !== githubDriftIssueBody(report)) return false; + const sameSet = (a: string[], b: string[]): boolean => { + const normalizedA = new Set(a.map((value) => value.toLowerCase())); + const normalizedB = new Set(b.map((value) => value.toLowerCase())); + return normalizedA.size === normalizedB.size && [...normalizedA].every((value) => normalizedB.has(value)); + }; + return sameSet(existing.labels, githubDriftIssueLabels(report)) && sameSet(existing.assignees, assignees); +} + function githubDriftIssueBody(report: UpstreamDriftReportRecord): string { return [ ``, diff --git a/test/unit/upstream-ruleset.test.ts b/test/unit/upstream-ruleset.test.ts index 03ca0c3ae7..d6ce7d8c68 100644 --- a/test/unit/upstream-ruleset.test.ts +++ b/test/unit/upstream-ruleset.test.ts @@ -910,6 +910,172 @@ describe("upstream ruleset drift tracking", () => { await expect(fileUpstreamDriftIssues(failingEnv)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, skipped: 1 }); }); + it("INVARIANT (#4503): a second run against an UNCHANGED open drift issue makes zero PATCH calls", async () => { + const env = createTestEnv({ GITTENSORY_AUTO_FILE_DRIFT_ISSUES: "true", GITTENSORY_DRIFT_ISSUE_TOKEN: "token" }); + await upsertUpstreamDriftReport(env, driftReport("stable-fingerprint")); + const createCalls: GitHubIssueFetchCall[] = []; + vi.stubGlobal("fetch", githubIssueFetch({ create: { number: 101, url: "https://github.com/JSONbored/gittensory/issues/101" }, calls: createCalls })); + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 1, updated: 0, unchanged: 0 }); + const posted = createCalls.find((call) => call.method === "POST")?.body; + + // Second run: the "existing" issue's GET reflects EXACTLY what was just posted -- a fresh PATCH would be a no-op. + const secondCalls: GitHubIssueFetchCall[] = []; + vi.stubGlobal( + "fetch", + githubIssueFetch({ + issue: { + number: 101, + url: "https://github.com/JSONbored/gittensory/issues/101", + fingerprint: "stable-fingerprint", + body: String(posted?.body), + labels: posted?.labels as string[], + assignees: posted?.assignees as string[], + }, + calls: secondCalls, + }), + ); + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, unchanged: 1 }); + expect(secondCalls.some((call) => call.method === "PATCH")).toBe(false); + }); + + it("REGRESSION (#4503): two consecutive cycles against an unchanged report only PATCH on the first", async () => { + const env = createTestEnv({ GITTENSORY_AUTO_FILE_DRIFT_ISSUES: "true", GITTENSORY_DRIFT_ISSUE_TOKEN: "token" }); + await upsertUpstreamDriftReport(env, driftReport("cycle-fingerprint", { issueNumber: 202, issueUrl: "https://github.com/JSONbored/gittensory/issues/202" })); + + // Cycle 1: the recorded issue's live body/labels are STALE (drift from before the report's current content) -- + // this cycle must PATCH to bring them into sync. + const firstCalls: GitHubIssueFetchCall[] = []; + vi.stubGlobal( + "fetch", + githubIssueFetch({ + issue: { number: 202, url: "https://github.com/JSONbored/gittensory/issues/202", fingerprint: "cycle-fingerprint", body: "\nstale", labels: ["signals"] }, + calls: firstCalls, + }), + ); + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 0, updated: 1, unchanged: 0 }); + const firstPatchBody = firstCalls.find((call) => call.method === "PATCH")?.body; + + // Cycle 2 (the next 6-hour tick): the live issue now reflects exactly what cycle 1 just wrote -- zero PATCH calls. + const secondCalls: GitHubIssueFetchCall[] = []; + vi.stubGlobal( + "fetch", + githubIssueFetch({ + issue: { + number: 202, + url: "https://github.com/JSONbored/gittensory/issues/202", + fingerprint: "cycle-fingerprint", + body: String(firstPatchBody?.body), + labels: firstPatchBody?.labels as string[], + assignees: firstPatchBody?.assignees as string[], + }, + calls: secondCalls, + }), + ); + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 0, updated: 0, unchanged: 1 }); + expect(secondCalls.filter((call) => call.method === "PATCH")).toHaveLength(0); + }); + + it("negative-path (#4503): a genuinely changed report (severity escalation) still triggers a fresh PATCH", async () => { + const env = createTestEnv({ GITTENSORY_AUTO_FILE_DRIFT_ISSUES: "true", GITTENSORY_DRIFT_ISSUE_TOKEN: "token" }); + await upsertUpstreamDriftReport(env, driftReport("escalating-fingerprint", { severity: "low" })); + const createCalls: GitHubIssueFetchCall[] = []; + vi.stubGlobal("fetch", githubIssueFetch({ create: { number: 303, url: "https://github.com/JSONbored/gittensory/issues/303" }, calls: createCalls })); + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 1, updated: 0, unchanged: 0 }); + const posted = createCalls.find((call) => call.method === "POST")?.body; + + // The SAME fingerprint's report is re-upserted with an ESCALATED severity -- both the body (severity line) and + // the labels ("backend" -> "high-impact") change; the live issue still reflects the OLD (low-severity) content. + await upsertUpstreamDriftReport(env, driftReport("escalating-fingerprint", { severity: "blocking" })); + const secondCalls: GitHubIssueFetchCall[] = []; + vi.stubGlobal( + "fetch", + githubIssueFetch({ + issue: { + number: 303, + url: "https://github.com/JSONbored/gittensory/issues/303", + fingerprint: "escalating-fingerprint", + body: String(posted?.body), + labels: posted?.labels as string[], + }, + calls: secondCalls, + }), + ); + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 0, updated: 1, unchanged: 0 }); + const escalatedPatch = secondCalls.find((call) => call.method === "PATCH")?.body; + expect(escalatedPatch?.labels).toEqual(["signals", "scoring", "data", "high-impact"]); + expect(String(escalatedPatch?.body)).toContain("Severity: blocking"); + }); + + it("REGRESSION (#4503): the list-search fallback tolerates malformed label/assignee shapes (missing name/login) without crashing", async () => { + const env = createTestEnv({ GITTENSORY_AUTO_FILE_DRIFT_ISSUES: "true", GITTENSORY_DRIFT_ISSUE_TOKEN: "token" }); + await upsertUpstreamDriftReport(env, driftReport("malformed-shape-fingerprint")); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit): Promise => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/issues?state=open&labels=signals&per_page=100&page=")) { + return Response.json([ + { + number: 404, + html_url: "https://github.com/JSONbored/gittensory/issues/404", + body: "", + // A nameless label object and a loginless assignee object -- both must be dropped, not crash the map. + labels: [{}, "signals"], + assignees: [{}, { login: "jsonbored" }], + }, + ]); + } + if (url.match(/\/issues\/404$/) && method === "PATCH") return Response.json({ number: 404, html_url: "https://github.com/JSONbored/gittensory/issues/404" }); + return new Response("not found", { status: 404 }); + }); + + // Content genuinely differs (the mocked body is just the fingerprint comment, not a full drift body), so this + // still PATCHes -- the point of this test is that resolving the malformed shapes above never throws. + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 0, updated: 1, skipped: 0 }); + }); + + it("REGRESSION (#4503): validateRecordedGitHubIssue's fast path tolerates a malformed (loginless) assignee without crashing", async () => { + const env = createTestEnv({ GITTENSORY_AUTO_FILE_DRIFT_ISSUES: "true", GITTENSORY_DRIFT_ISSUE_TOKEN: "token" }); + await upsertUpstreamDriftReport(env, driftReport("malformed-assignee-fingerprint", { issueNumber: 505, issueUrl: "https://github.com/JSONbored/gittensory/issues/505" })); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit): Promise => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.match(/\/issues\/505$/) && method === "GET") { + return Response.json({ + number: 505, + html_url: "https://github.com/JSONbored/gittensory/issues/505", + state: "open", + body: "", + labels: ["signals"], + // A loginless assignee object -- must be dropped, not crash the map. + assignees: [{}, { login: "jsonbored" }], + }); + } + if (url.match(/\/issues\/505$/) && method === "PATCH") return Response.json({ number: 505, html_url: "https://github.com/JSONbored/gittensory/issues/505" }); + return new Response("not found", { status: 404 }); + }); + + // Content genuinely differs (the mocked body is just the fingerprint comment), so this still PATCHes -- the + // point of this test is that resolving the malformed assignee shape above never throws. + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 0, updated: 1, skipped: 0 }); + }); + + it("REGRESSION (#4503): validateRecordedGitHubIssue tolerates an issue response with the assignees field entirely absent", async () => { + const env = createTestEnv({ GITTENSORY_AUTO_FILE_DRIFT_ISSUES: "true", GITTENSORY_DRIFT_ISSUE_TOKEN: "token" }); + await upsertUpstreamDriftReport(env, driftReport("no-assignees-field-fingerprint", { issueNumber: 606, issueUrl: "https://github.com/JSONbored/gittensory/issues/606" })); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit): Promise => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.match(/\/issues\/606$/) && method === "GET") { + // No `assignees` key at all -- distinct from an explicit empty array. + return Response.json({ number: 606, html_url: "https://github.com/JSONbored/gittensory/issues/606", state: "open", body: "", labels: ["signals"] }); + } + if (url.match(/\/issues\/606$/) && method === "PATCH") return Response.json({ number: 606, html_url: "https://github.com/JSONbored/gittensory/issues/606" }); + return new Response("not found", { status: 404 }); + }); + + await expect(fileUpstreamDriftIssues(env)).resolves.toMatchObject({ status: "completed", created: 0, updated: 1, skipped: 0 }); + }); + it("assigns filed drift issues to GITTENSORY_DRIFT_ISSUE_ASSIGNEES when a self-host operator sets it", async () => { const env = createTestEnv({ GITTENSORY_AUTO_FILE_DRIFT_ISSUES: "true", GITTENSORY_DRIFT_ISSUE_TOKEN: "token", GITTENSORY_DRIFT_ISSUE_ASSIGNEES: "alice, ,bob" }); await upsertUpstreamDriftReport(env, driftReport("assignee-override")); @@ -1301,7 +1467,7 @@ function githubIssueFetch(options: { create?: { number: number; url: string }; createPayload?: Record; update?: { number: number; url: string }; - issue?: { number: number; url: string; fingerprint: string; state?: string; labels?: Array; body?: string | null }; + issue?: { number: number; url: string; fingerprint: string; state?: string; labels?: Array; body?: string | null; assignees?: string[] }; updatePayload?: Record; issueStatus?: number | undefined; listStatus?: number; @@ -1339,6 +1505,7 @@ function githubIssueFetch(options: { state: options.issue.state ?? "open", body: options.issue.body === undefined ? `` : options.issue.body, labels: options.issue.labels ?? ["signals"], + assignees: (options.issue.assignees ?? []).map((login) => ({ login })), }); } if (issueMatch && method === "PATCH") {