Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/services/ai-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,14 @@ type AiRunCorrelation = {
anthropicModel?: string | undefined;
};

/** True for the self-host CLI adapter's own non-transient timeout signal (`src/selfhost/ai.ts`'s
* `throw new Error("subscription_cli_timeout")`, thrown after a `claude-code`/`codex` subprocess is SIGKILLed
* at its effort-based deadline). Distinguishes it from a genuinely transient failure (a dropped connection, a
* malformed response) that's still worth retrying up to the full budget. */
function isSubscriptionCliTimeout(error: unknown): boolean {
return error instanceof Error && error.message === "subscription_cli_timeout";
}

/** One reviewer opinion (whichever provider `env.AI` resolves to — self-host Codex/Claude Code/etc, or the
* legacy Workers-AI pair) with a per-slot reliable fallback and a 3× retry on the primary. */
async function runWorkersOpinion(
Expand Down Expand Up @@ -976,6 +984,12 @@ async function runWorkersOpinion(
}),
);
lastError = error;
// A CLI timeout is not transient -- the same model retrying the same oversized/complex diff will almost
// certainly time out again. Stop retrying THIS model (the fallback below still gets its own full retry
// budget, since a different model/config may not share the same timeout) instead of burning up to 3x
// the full effort-timeout in subprocess time for zero additional chance of success (#gaming-tactic-draft-cycle
// audit finding: this inner retry count is distinct from c7073949's outer cross-sweep-tick cap).
if (isSubscriptionCliTimeout(error)) break;
}
}
}
Expand Down Expand Up @@ -1616,6 +1630,8 @@ async function runDualAiTieBreakJudgeCall(
status: "provider_error",
error: errorMessage(error),
});
// See runWorkersOpinion's identical guard: a CLI timeout will not resolve by retrying the same model.
if (isSubscriptionCliTimeout(error)) break;
}
}
}
Expand Down
43 changes: 43 additions & 0 deletions test/unit/ai-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2675,6 +2675,21 @@ describe("pure helpers", () => {
expect(diagnostics.some((d) => d.status === "provider_error")).toBe(true);
});

it("runDualAiTieBreakJudgeCall stops retrying a model after ONE subscription_cli_timeout, but the fallback still gets its full retry budget (#gaming-tactic-draft-cycle)", async () => {
let primaryAttempts = 0;
const run = vi.fn(async (model: string) => {
if (model === "fallback") return { response: '{"favored":"reviewer_1"}' };
primaryAttempts += 1;
throw new Error("subscription_cli_timeout");
});
const env = createTestEnv({ AI: { run } as unknown as Ai });
const diagnostics: Array<{ status: string; model: string }> = [];
const parsed = await runDualAiTieBreakJudgeCall(env, "primary", "fallback", blockedA, clean, false, diagnostics as never);
expect(parsed?.verdict).toBe("reviewer_1");
expect(primaryAttempts).toBe(1); // NOT 3 -- the timeout short-circuits further retries of this model.
expect(run).toHaveBeenCalledTimes(2); // 1 primary (timed out) + 1 fallback (succeeded on its first try).
});

it("resolveDualAiTieBreakWithOrderStability returns inconclusive when judge output never parses", async () => {
const run = vi.fn(async () => ({ response: "not-json" }));
const env = createTestEnv({ AI: { run } as unknown as Ai });
Expand Down Expand Up @@ -2793,6 +2808,34 @@ describe("pure helpers", () => {
expect(run).toHaveBeenCalledTimes(1);
});

it("runWorkersOpinion stops retrying a model after ONE subscription_cli_timeout, but the fallback still gets its full retry budget (#gaming-tactic-draft-cycle)", async () => {
let primaryAttempts = 0;
const run = vi.fn(async (model: string) => {
if (model === "fallback") return { response: reviewJson() };
primaryAttempts += 1;
throw new Error("subscription_cli_timeout");
});
const env = createTestEnv({ AI: { run } as unknown as Ai });
const diagnostics: Array<{ status: string; model: string }> = [];
const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never);
expect(parsed.review?.assessment).toContain("reasonable");
expect(primaryAttempts).toBe(1); // NOT 3 -- the timeout short-circuits further retries of this model.
expect(run).toHaveBeenCalledTimes(2); // 1 primary (timed out) + 1 fallback (succeeded on its first try).
});

it("runWorkersOpinion still retries a genuinely transient (non-timeout) error up to the full budget", async () => {
let attempts = 0;
const run = vi.fn(async () => {
attempts += 1;
if (attempts < 3) throw new Error("connection reset");
return { response: reviewJson() };
});
const env = createTestEnv({ AI: { run } as unknown as Ai });
const parsed = await runWorkersOpinion(env, "m", "m", "sys", "user", 256);
expect(parsed.review?.assessment).toContain("reasonable");
expect(attempts).toBe(3);
});

it("forwards correlation + self-host ai_model override fields into ai.run's options, omitting absent ones (#selfhost-ai-model-override)", async () => {
let seenOptions: Record<string, unknown> = {};
const run = vi.fn(async (_model: string, options: Record<string, unknown>) => {
Expand Down