From 733071f8ef622a023a723a36149c61a42383716d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 20 Jul 2026 17:53:11 -0700 Subject: [PATCH 1/2] fix(github): log discarded review-files fetch warnings (#7602) fetchAndStorePullRequestFilesForReview built a warnings array from fetchPullRequestFiles but never read it, so a REST+GraphQL double failure during the live-review inline file fetch degraded to an empty file list with zero observability. Log it (matching the console.error(JSON.stringify({level, event, ...})) convention used elsewhere in src/) without changing the fail-safe return-[] behavior. --- src/github/backfill.ts | 6 ++++++ test/unit/backfill-2.test.ts | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 02ccec4200..cbc18c8d33 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2442,6 +2442,9 @@ const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout * Fail-safe by construction: a fetch failure returns `[]` (never throws), so the review degrades to the same * empty-diff state it has today rather than breaking. The persist is best-effort and only runs when the fetch * actually returned files (a failed REST+GraphQL fetch must not wipe a row another sync just wrote). + * + * A REST+GraphQL double failure is logged (#7602) even though it stays fail-safe -- that combination used to + * discard its `warnings` entry silently, leaving no trace of how often the double-failure case actually happens. */ export async function fetchAndStorePullRequestFilesForReview( env: Env, @@ -2457,6 +2460,9 @@ export async function fetchAndStorePullRequestFilesForReview( await sleep(REVIEW_FILES_EMPTY_RETRY_DELAY_MS); files = await fetchOnce(); } + if (warnings.length > 0) { + console.error(JSON.stringify({ level: "warn", event: "review_files_fetch_failed", repoFullName, pullNumber, warnings })); + } if (files.length === 0) return []; const records = files.map((file) => toPullRequestFileRecordFromGitHub(repoFullName, pullNumber, file)); // Persist so the AI review, grounding, gate, check-run, and unified-comment reads in THIS run (and any later diff --git a/test/unit/backfill-2.test.ts b/test/unit/backfill-2.test.ts index 70a9f8f002..7811da927d 100644 --- a/test/unit/backfill-2.test.ts +++ b/test/unit/backfill-2.test.ts @@ -224,6 +224,38 @@ describe("GitHub backfill", () => { await expect(fetchAndStorePullRequestFilesForReview(env, "JSONbored/gittensory", 56, "public-token")).resolves.toEqual([]); expect(filesCalls).toBe(2); }); + + it("logs a structured warning (#7602) when both REST and GraphQL file fetches fail -- this used to be totally silent", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async () => new Response("boom", { status: 500 })); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + await expect(fetchAndStorePullRequestFilesForReview(env, "JSONbored/gittensory", 100, "public-token")).resolves.toEqual([]); + const traceLine = errorSpy.mock.calls.map((c) => String(c[0])).find((line) => line.includes("review_files_fetch_failed")); + expect(traceLine).toBeDefined(); + const parsed = JSON.parse(traceLine as string) as Record; + expect(parsed).toMatchObject({ level: "warn", event: "review_files_fetch_failed", repoFullName: "JSONbored/gittensory", pullNumber: 100 }); + // Both fetchOnce() attempts hit the REST+GraphQL double-failure path, so the warning is recorded twice. + expect(parsed.warnings).toEqual([ + "File sync failed for #100: GitHub REST and GraphQL detail fetches failed.", + "File sync failed for #100: GitHub REST and GraphQL detail fetches failed.", + ]); + } finally { + errorSpy.mockRestore(); + } + }); + + it("never logs a warning when GitHub returns a real, successful empty files list (not a fetch failure)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async () => Response.json([])); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + await expect(fetchAndStorePullRequestFilesForReview(env, "JSONbored/gittensory", 101, "public-token")).resolves.toEqual([]); + expect(errorSpy).not.toHaveBeenCalled(); + } finally { + errorSpy.mockRestore(); + } + }); }); describe("fetchLiveCiAggregate", () => { From 0cfc746b5565be4c1c4d99c7328dc0c34245a70e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 20 Jul 2026 18:00:55 -0700 Subject: [PATCH 2/2] fix(deps): clear adm-zip and body-parser npm audit findings (#7607) adm-zip <0.6.0 (GHSA-xcpc-8h2w-3j85, high) is a transitive dependency of github-actionlint (adm-zip: ^0.5.16), used via extractAllTo in its binary download/extract step. adm-zip@0.6.0 is the first patched version; force it with a package.json override (the same mechanism already used here for tar/esbuild/ws/js-yaml). Verified against a cleared ~/.github-actionlint cache -- forcing a real re-extraction through the new adm-zip code path, not a cached hit -- that actionlint still passes. body-parser 2.0.0-2.2.2 (GHSA-v422-hmwv-36x6, low) is transitive via @modelcontextprotocol/sdk -> express. body-parser@2.3.0 satisfies the existing declared range, so `npm update body-parser` resolves it with a lockfile-only change (same shape as #7569's brace-expansion fix). npm audit --audit-level=moderate: 0 vulnerabilities. --- package-lock.json | 35 +++++++++++++---------------------- package.json | 1 + 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/package-lock.json b/package-lock.json index 6fa0ae9d41..cdefbb610d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9285,13 +9285,13 @@ } }, "node_modules/adm-zip": { - "version": "0.5.17", - "resolved": "https://registry.npmjs.org/adm-zip/-/adm-zip-0.5.17.tgz", - "integrity": "sha512-+Ut8d9LLqwEvHHJl1+PIHqoyDxFgVN847JTVM3Izi3xHDWPE4UtzzXysMZQs64DMcrJfBeS/uoEP4AD3HQHnQQ==", + "version": "0.6.0", + "resolved": "https://registry.npmjs.org/adm-zip/-/adm-zip-0.6.0.tgz", + "integrity": "sha512-XleryMhbuksdKtofnWZ9Sk+4CUTbms4Mb/EU32SZwToAyZ5RgVos/ki8n+yr0LWHOGKuakbXTuuYNHLQjhddgg==", "dev": true, "license": "MIT", "engines": { - "node": ">=12.0" + "node": ">=14.0" } }, "node_modules/agent-base": { @@ -10177,20 +10177,20 @@ "license": "MIT" }, "node_modules/body-parser": { - "version": "2.2.2", - "resolved": "https://registry.npmjs.org/body-parser/-/body-parser-2.2.2.tgz", - "integrity": "sha512-oP5VkATKlNwcgvxi0vM0p/D3n2C3EReYVX+DNYs5TjZFn/oQt2j+4sVJtSMr18pdRr8wjTcBl6LoV+FUwzPmNA==", + "version": "2.3.0", + "resolved": "https://registry.npmjs.org/body-parser/-/body-parser-2.3.0.tgz", + "integrity": "sha512-2cGmJupaNgg+QUwVLAucDuWuoMZ6EX9iHDRswZ5lsNYEmwPaRknMPCLZz07yTzVq/83p4o/wzbDZbBrTvGGTIw==", "license": "MIT", "dependencies": { "bytes": "^3.1.2", - "content-type": "^1.0.5", + "content-type": "^2.0.0", "debug": "^4.4.3", - "http-errors": "^2.0.0", - "iconv-lite": "^0.7.0", + "http-errors": "^2.0.1", + "iconv-lite": "^0.7.2", "on-finished": "^2.4.1", - "qs": "^6.14.1", - "raw-body": "^3.0.1", - "type-is": "^2.0.1" + "qs": "^6.15.2", + "raw-body": "^3.0.2", + "type-is": "^2.1.0" }, "engines": { "node": ">=18" @@ -10200,15 +10200,6 @@ "url": "https://opencollective.com/express" } }, - "node_modules/body-parser/node_modules/content-type": { - "version": "1.0.5", - "resolved": "https://registry.npmjs.org/content-type/-/content-type-1.0.5.tgz", - "integrity": "sha512-nTjqfcBFEipKdXCv4YDQWCfmcLZKm81ldF0pAopTvyrFGVbcR6P/VAAd5G7N+0tTr8QqiU0tFadD6FK4NtJwOA==", - "license": "MIT", - "engines": { - "node": ">= 0.6" - } - }, "node_modules/brace-expansion": { "version": "1.1.16", "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.16.tgz", diff --git a/package.json b/package.json index c69c522973..2b35c3a98c 100644 --- a/package.json +++ b/package.json @@ -168,6 +168,7 @@ "ws": "^8.21.0", "tar": "^7.5.19", "js-yaml": "^4.3.0", + "adm-zip": "^0.6.0", "lovable-tagger@1.2.0": { "esbuild": "^0.28.1" },