From 6c7994cea966436535e794148194fdf9a788d260 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:01:20 -0700 Subject: [PATCH 1/5] test(gate): cover the fail-safe marker-clear path (#9462) Mirrors the sibling visual-capture-satisfied fail-safe test: a rejecting clear must not throw out of the pass, since a failed clear only means the gate stays deferred until the head moves. (cherry picked from commit efa64726027177a15d30f353cea779a984952a49) --- test/unit/queue-3.test.ts | 58 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index b17e54f0da..4bdb3606e6 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -2339,6 +2339,64 @@ describe("queue processors", () => { expect((await getPullRequest(env, "JSONbored/gittensory", 62))?.visualCaptureRetryPendingSha).toBeNull(); }); + // #9462: the clear is best-effort, mirroring its sibling mark write -- a failed clear means the gate stays + // deferred until the head moves, which is strictly better than letting a cleanup failure crash the pass. + it("screenshot-table gate (#9462): a failed marker clear is swallowed (fail-safe) -- the pass still completes", async () => { + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockRejectedValue(new Error("browserless connection refused")); + const clearSpy = vi.spyOn(repositoriesModule, "clearPullRequestVisualCaptureRetryPending").mockRejectedValue(new Error("D1 write failed")); + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + LOOPOVER_REVIEW_SCREENSHOTS: "true", + }); + env.JOBS = { async send() {} } as unknown as Queue; + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 63, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis63" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/63/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/63/reviews")) return Response.json([]); + if (url.includes("/pulls/63/commits")) return Response.json([]); + if (url.endsWith("/pulls/63") && method === "PATCH") return Response.json({ number: 63, state: "closed" }); + if (url.endsWith("/pulls/63")) return Response.json({ number: 63, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis63" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis63/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis63/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis63/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/63/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/63/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 903 }, { status: 201 }); + if (url.includes("/check-runs/903") && method === "PATCH") return Response.json({ id: 903 }); + return new Response("not found", { status: 404 }); + }); + + try { + // Must not throw even though the clear rejects. + await expect( + processJob(env, { type: "recapture-preview", deliveryId: "screenshot-marker-clear-failed", repoFullName: "JSONbored/gittensory", prNumber: 63, installationId: 123, attempt: MAX_PREVIEW_POLL_ATTEMPTS }), + ).resolves.not.toThrow(); + expect(clearSpy).toHaveBeenCalled(); + } finally { + clearSpy.mockRestore(); + buildCaptureSpy.mockRestore(); + } + }); + describe("live migrations/** collision recheck (#2550)", () => { // Full merge-eligible stub set (clean + green + approved), reused across scenarios — a positive test proves // the collision hold actually suppresses what would otherwise merge; a negative test proves the check From 0db1f752b77dc055cbf6d937e51b798124bec1d0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:29:22 -0700 Subject: [PATCH 2/5] refactor(gate): drop an unreachable spread arm and annotate the defensive head-SHA guard (#9462) screenshotTableEvidenceUnresolved is computed as a plain boolean, so the undefined arm of its conditional spread was dead code -- pass it directly. The head-SHA guard in the exhausted branch is genuinely defensive (a recapture-preview job is only minted for a PR that had one), matching the sibling mark write, so it carries a reason rather than a contrived test. (cherry picked from commit 4f59499e954ec4ed0b25f5c4699ded816fb5eb88) --- src/queue/processors.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 701e25c167..8f8626b39d 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2859,7 +2859,7 @@ function buildAgentMaintenancePlanInput(args: { : {}), copycatGateMode: settings.copycatGateMode, ...(screenshotTableMatch !== undefined ? { screenshotTableMatch } : {}), - ...(screenshotTableEvidenceUnresolved !== undefined ? { screenshotTableEvidenceUnresolved } : {}), + screenshotTableEvidenceUnresolved, ...(contributorCapMatch !== undefined ? { contributorCapMatch } : {}), // Always threaded (the DB layer populates it, default "over-contributor-limit"); the planner applies its // own fallback. @@ -9491,6 +9491,8 @@ async function scheduleVisualCaptureRetry( // forever (the sole other clear needs a successful capture, which by definition never came), which silently // and permanently disabled the screenshotTableGate's close for that head. Best-effort, matching the mark // write below: a failed clear only means the gate stays deferred until the head moves, never a crash. + /* v8 ignore next -- a recapture-preview job is only ever minted for a PR that had a head SHA, so the + falsy arm is defensive; the mark write below carries the identical guard for the same reason. */ if (args.pr.headSha) { await clearPullRequestVisualCaptureRetryPending(env, args.repoFullName, args.pr.number, args.pr.headSha).catch((error) => { console.log( From c3692bc8a7e60689fe292e4878c35e847f99b988 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:59:32 -0700 Subject: [PATCH 3/5] test(ai): exclude two unreachable best-effort cleanup arms from coverage (#9479) rm with force:true does not throw for a missing path, so the catch needs a filesystem-level failure no unit test can portably induce; the unset-cwd guard is only reachable when mkdtemp itself threw, i.e. no directory was ever created. Both annotated with their reason, matching the file's existing convention. (cherry picked from commit bf3efe663b69a2b9e0592f54c79ae32b344fcff2) --- src/selfhost/ai.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index f9cc55ad54..ee672a8cdb 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -604,10 +604,14 @@ async function isolatedCliCwd(): Promise { * instructions on disk indefinitely. Best-effort by design: a cleanup failure must never turn a completed * review into a thrown error, and the next container recreation still collects anything missed. */ async function removeIsolatedCliCwd(cwd: string | undefined): Promise { + /* v8 ignore next -- the finally runs with cwd unset only when the mkdtemp itself threw, i.e. the temp dir was + never created and there is nothing to remove; unreachable from a test that gets far enough to spawn. */ if (!cwd) return; try { const { rm } = await import("node:fs/promises"); await rm(cwd, { recursive: true, force: true }); + /* v8 ignore next 3 -- best-effort cleanup: rm with force:true does not throw for a missing path, so this + arm needs a filesystem-level failure (permissions, EBUSY) that no unit test can portably induce. */ } catch { // best-effort -- see the doc comment. } From 2e46181efd692cc354047004cb7938a02d1ae778 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:31:44 -0700 Subject: [PATCH 4/5] test(ai): drive the real subprocess EPIPE path and the non-Error stall guard (#9476, #9479) The EPIPE regression uses the existing real-subprocess harness rather than a stubbed spawn, so the stdin error listener is genuinely exercised: a fake CLI exits immediately without draining stdin, and the test asserts no uncaughtException reaches the process. Verified to fail without the listener (it captures an uncaught EPIPE), which is precisely the crash that took down every in-flight queue job in the container. Also covers isStalledNoOutput rejecting a non-Error throw -- a provider adapter can reject with a string, and misclassifying that as a deadline signal would silently skip the retry budget. (cherry picked from commit c770cc1152f7f3101e52d538875707368be31afb) --- test/unit/ai-review.test.ts | 15 +++++++++++++++ test/unit/selfhost-ai.test.ts | 30 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 4590245bb0..68856c9a6d 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -3540,6 +3540,21 @@ describe("pure helpers", () => { expect(run).toHaveBeenCalledTimes(2); // 1 primary (stalled) + 1 fallback (succeeded on its first try). }); + it("REGRESSION (#9476): a non-Error throw is not mistaken for a stall and still uses the full budget", async () => { + // isStalledNoOutput must reject a non-Error value rather than throwing on `.message` -- a provider adapter + // can reject with a string, and misclassifying that as a deadline signal would silently skip retries. + let primaryAttempts = 0; + const run = vi.fn(async (model: string) => { + if (model === "fallback") return { response: reviewJson() }; + primaryAttempts += 1; + throw "claude_stalled_no_output: not an Error instance"; + }); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const diagnostics: Array<{ status: string; model: string }> = []; + await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + expect(primaryAttempts).toBe(3); + }); + it("REGRESSION (#9476): a genuinely transient error still gets the FULL retry budget (the break is narrow)", async () => { // Guards against over-broadening the break: only the non-transient deadline/rate-limit/config signals // short-circuit. A dropped connection must still be retried up to the budget. diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index cc63bd1088..422e801d54 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/test/unit/selfhost-ai.test.ts @@ -1780,6 +1780,36 @@ describe("subscription CLI helpers + fail-safe", () => { } }); + // #9479 REGRESSION: child.on("error") catches SPAWN failures only -- it never receives stdio stream errors. + // A CLI that exits before draining stdin (an unknown flag on an upgraded binary, an immediate auth abort, an + // OOM kill) made the ~250KB stdin write fail with EPIPE on an emitter with no "error" listener, which Node + // escalates to an UNCAUGHT exception -> installSelfHostCrashHandlers -> exit(1), taking down every in-flight + // queue job in the container. This drives the real subprocess so the listener is genuinely exercised: the + // fake exits immediately without reading stdin, and the write must be swallowed rather than crash the run. + it("REAL subprocess: a CLI that exits without draining stdin does not crash the worker (#9479)", async () => { + const dir = mkdtempSync(join(tmpdir(), "fakecli-")); + const fake = join(dir, "claude"); + // Exits at once, never reading stdin -> the parent's write lands on a closed pipe (EPIPE). + writeFileSync(fake, "#!/usr/bin/env node\nprocess.exit(2);\n"); + chmodSync(fake, 0o755); + const origPath = process.env.PATH; + const uncaught: unknown[] = []; + const onUncaught = (err: unknown) => uncaught.push(err); + process.on("uncaughtException", onUncaught); + try { + // A large input makes the write span multiple chunks, so it cannot complete before the child is gone. + await expect( + createClaudeCodeAi({ PATH: `${dir}:${origPath ?? ""}`, CLAUDE_CODE_OAUTH_TOKEN: "t" }).run("sonnet", { + prompt: "x".repeat(300_000), + }), + ).rejects.toThrow(); // surfaces as a normal provider error via the exit-code guard... + } finally { + process.off("uncaughtException", onUncaught); + process.env.PATH = origPath; + } + expect(uncaught).toEqual([]); // ...and never as an uncaught EPIPE, which is what killed the container + }); + // REGRESSION (GITTENSORY-K/M/8/Z, #4994): the real defaultSpawn fast-fail path against a genuinely-hung fake // `claude` that writes nothing to either stream and never exits — mirrors the identical codex real-subprocess // test below, proving createClaudeCodeAi's plumbing (not just a stubbed spawn) actually wires From 3f8f2b5f9d9251dec184e709a09a955a1ca34f5c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:02:36 -0700 Subject: [PATCH 5/5] test(rate-limit): cover the inline-retry break for an over-budget Retry-After (#9494) Asserts the attempt COUNT, not just the classification: a 60s Retry-After yields exactly one attempt instead of four, and a short one still uses the full inline budget so the break stays narrow. (cherry picked from commit 92813bf195343f02059835c74d8a779b3fa161c4) --- test/unit/github-app.test.ts | 42 ++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index b2708e748c..13e2a8d791 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -3123,6 +3123,48 @@ describe("GitHub rate-limit handling (#ratelimit-resilience)", () => { await expect(createInstallationToken(env, 5252)).rejects.toThrow(); expect(calls).toBe(4); // initial + GITHUB_RATE_LIMIT_MAX_RETRIES (3) }); + + // #9494 regression: a secondary-limit Retry-After is typically 60s against an 8s inline cap. The old code + // took Math.min(retryAfter, cap) and retried up to 3 times INSIDE the window GitHub explicitly asked us to + // stay out of -- which its own docs warn can extend or escalate a secondary block. Surface it instead: the + // queue honors the full Retry-After with jitter and defers sibling jobs for the same admission target. + it("timeoutFetch stops retrying inline when Retry-After exceeds the inline budget (#9494)", async () => { + const privateKey = await generatePrivateKeyPem(); + clearInstallationTokenCacheForTest(); + let calls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) { + calls += 1; + return new Response("secondary rate limit", { + status: 403, + headers: { "retry-after": "60" }, // far beyond GITHUB_RATE_LIMIT_MAX_DELAY_MS + }); + } + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + await expect(createInstallationToken(env, 5353)).rejects.toThrow(); + expect(calls).toBe(1); // ONE attempt -- not 4. The wait belongs to the queue, not this loop. + }); + + it("timeoutFetch still uses its full inline budget when Retry-After fits within it (#9494)", async () => { + // Guards against over-broadening: a short instructed wait is exactly what the inline retries are for. + const privateKey = await generatePrivateKeyPem(); + clearInstallationTokenCacheForTest(); + let calls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) { + calls += 1; + return new Response("secondary rate limit", { status: 403, headers: { "retry-after": "0" } }); + } + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + await expect(createInstallationToken(env, 5454)).rejects.toThrow(); + expect(calls).toBe(4); + }); }); describe("repoFullName segment-count + whitespace guard (#8311)", () => {