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( 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. } 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/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)", () => { 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 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