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
9 changes: 6 additions & 3 deletions src/review/planner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
// legacy Workers-AI pair); the output is public-safe-sanitized before posting; any model/error degrades to
// a no-plan no-op.

import { type AiReviewActualUsage, BEST_REVIEW_MODELS, clampNumber, coerceAiText, coerceAiUsage, estimateNeurons, RELIABLE_FALLBACK_MODELS, utcDayStartIso } from "../services/ai-review";
import { type AiReviewActualUsage, BEST_REVIEW_MODELS, clampNumber, coerceAiText, coerceAiUsage, estimateNeurons, isRateLimitError, RELIABLE_FALLBACK_MODELS, utcDayStartIso } from "../services/ai-review";
import { recordAiUsageEvent, sumAiEstimatedNeuronsSince } from "../db/repositories";
import { sanitizePublicComment } from "../github/commands";
import { AGENT_COMMAND_COMMENT_MARKER } from "../github/comments";
Expand Down Expand Up @@ -123,8 +123,11 @@ async function runPlannerModel(env: Env, system: string, user: string): Promise<
const result = await ai.run(model, { max_tokens: PLANNER_MAX_TOKENS, temperature: 0.2, messages: [{ role: "system", content: system }, { role: "user", content: user }], finalAttempt: attempt === 1 && modelIndex === models.length - 1 }, extra);
const text = coerceAiText(result).trim();
if (text) return { text, usage: coerceAiUsage(result) };
} catch {
/* retry, then fall through to the fallback model */
} catch (error) {
// #5385-sentry (GITTENSORY-K/8): a 429 will not have cleared by the next attempt a few hundred ms
// later, so retrying THIS model burns the remaining budget for zero additional chance of success --
// move straight to the fallback model instead (same guard as runWorkersOpinion in ai-review.ts).
if (isRateLimitError(error)) break;
}
}
}
Expand Down
23 changes: 20 additions & 3 deletions src/services/ai-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1007,6 +1007,19 @@ function isSubscriptionCliTimeout(error: unknown): boolean {
return error instanceof Error && error.message === "subscription_cli_timeout";
}

/** True for a provider's own HTTP-429 signal (`src/selfhost/ai.ts`'s `claude_code_error_429` /
* `ai_http_429` / `anthropic_http_429`, and the generic Workers-AI equivalent). #5385-sentry
* (GITTENSORY-K/8): an immediate same-model retry against a rate limit that is still in its window has
* near-zero chance of success -- unlike a transient network blip, a 429 will not clear in the handful of
* milliseconds between attempts. Mirrors {@link isSubscriptionCliTimeout}'s identical non-transient-error
* short-circuit: stop burning the remaining per-model retry budget and move straight to the fallback model
* (which may be a different provider/account entirely, and so isn't necessarily still rate-limited).
* Exported so every independent AI-calling retry loop (ai-slop.ts, planner.ts) can share this one
* definition instead of each re-deriving its own copy of the error-shape regex. */
export function isRateLimitError(error: unknown): boolean {
return error instanceof Error && /_(?:http|error)_429$/.test(error.message);
}

/** Cap on the diagnostic prefix logged for an unparseable model response (#observability-unparseable) -- long
* enough to tell a markdown-fenced/truncated-mid-JSON/plain-prose response apart, short enough to never dump
* a large chunk of model output into Sentry/audit context. */
Expand Down Expand Up @@ -1131,7 +1144,11 @@ async function runWorkersOpinion(
// 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;
// A 429 is the same story (#5385-sentry, GITTENSORY-K/8): the rate-limit window that just rejected
// this attempt will not have cleared by the next attempt a few hundred ms later, so an immediate
// same-model retry burns the remaining budget for zero additional chance of success -- move straight
// to the fallback model instead, which may be on a different account/provider entirely.
if (isSubscriptionCliTimeout(error) || isRateLimitError(error)) break;
}
}
}
Expand Down Expand Up @@ -1842,8 +1859,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;
// See runWorkersOpinion's identical guard: a CLI timeout or 429 will not resolve by retrying the same model.
if (isSubscriptionCliTimeout(error) || isRateLimitError(error)) break;
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/services/ai-slop.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
coerceAiUsage,
estimateNeurons,
isEnabled,
isRateLimitError,
toPublicSafe,
utcDayStartIso,
} from "./ai-review";
Expand Down Expand Up @@ -165,8 +166,11 @@ async function runWorkersSlopOpinion(env: Env, system: string, user: string, max
);
const parsed = parseSlopOpinion(coerceAiText(result));
if (parsed) return { opinion: parsed, usage: coerceAiUsage(result) };
} catch {
/* retry / fall through to fallback */
} catch (error) {
// #5385-sentry (GITTENSORY-K/8): a 429 will not have cleared by the next attempt a few hundred ms
// later, so retrying THIS model burns the remaining budget for zero additional chance of success --
// move straight to the fallback model instead (same guard as runWorkersOpinion in ai-review.ts).
if (isRateLimitError(error)) break;
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion src/services/linked-issue-satisfaction-run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
coerceAiUsage,
estimateNeurons,
isEnabled,
isRateLimitError,
utcDayStartIso,
} from "./ai-review";

Expand Down Expand Up @@ -86,7 +87,8 @@ async function runWorkersSatisfactionOpinion(
);
const result = buildLinkedIssueSatisfactionResult(issueText, coerceAiText(raw));
if (result) return { result, usage: coerceAiUsage(raw) };
} catch {
} catch (error) {
if (isRateLimitError(error)) break;
/* retry / fall through to fallback */
}
}
Expand Down
32 changes: 31 additions & 1 deletion test/unit/ai-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2862,6 +2862,21 @@ describe("pure helpers", () => {
expect(run).toHaveBeenCalledTimes(2); // 1 primary (timed out) + 1 fallback (succeeded on its first try).
});

it("REGRESSION (#5385-sentry, GITTENSORY-K/8): runDualAiTieBreakJudgeCall stops retrying a model after ONE 429 rate-limit error, same as a CLI timeout", async () => {
let primaryAttempts = 0;
const run = vi.fn(async (model: string) => {
if (model === "fallback") return { response: '{"favored":"reviewer_1"}' };
primaryAttempts += 1;
throw new Error("claude_code_error_429");
});
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 429 short-circuits further retries of this model.
expect(run).toHaveBeenCalledTimes(2); // 1 primary (rate-limited) + 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 @@ -3013,7 +3028,22 @@ describe("pure helpers", () => {
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 () => {
it("REGRESSION (#5385-sentry, GITTENSORY-K/8): runWorkersOpinion stops retrying a model after ONE 429 rate-limit error, same as a CLI timeout", async () => {
let primaryAttempts = 0;
const run = vi.fn(async (model: string) => {
if (model === "fallback") return { response: reviewJson() };
primaryAttempts += 1;
throw new Error("claude_code_error_429");
});
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 429 short-circuits further retries of this model.
expect(run).toHaveBeenCalledTimes(2); // 1 primary (rate-limited) + 1 fallback (succeeded on its first try).
});

it("runWorkersOpinion still retries a genuinely transient (non-timeout, non-429) error up to the full budget", async () => {
let attempts = 0;
const run = vi.fn(async () => {
attempts += 1;
Expand Down
14 changes: 14 additions & 0 deletions test/unit/ai-slop.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,20 @@ describe("runGittensoryAiSlopAdvisory gating + fail-safe", () => {
expect(run).toHaveBeenCalled(); // it tried (3× primary + fallback) and gave up cleanly
});

it("REGRESSION (#5385-sentry, GITTENSORY-K/8): stops retrying a model after ONE 429 rate-limit error instead of burning all 3 attempts, unlike a genuinely transient error", async () => {
const run = vi.fn(async () => {
throw new Error("claude_code_error_429");
});
const result = await runGittensoryAiSlopAdvisory(enabledEnv(run), baseInput);
expect(result.status).toBe("ok");
if (result.status !== "ok") throw new Error("unreachable");
expect(result.finding).toBeNull();
// 1 attempt per model (2 models), NOT the full 6-call budget a non-429 error would burn (see the
// "is fail-safe: a throwing model" test above, which uses toHaveBeenCalled() precisely because it burns
// the whole budget) -- the 429 short-circuits each model's remaining retries.
expect(run).toHaveBeenCalledTimes(2);
});

it("falls back to the reliable model when the primary keeps returning garbage", async () => {
const run = vi.fn(async (model: string) => ({ response: model.includes("gpt-oss") ? "not json" : slopJson({ band: "low" }) }));
const result = await runGittensoryAiSlopAdvisory(enabledEnv(run), baseInput);
Expand Down
11 changes: 11 additions & 0 deletions test/unit/linked-issue-satisfaction-run.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,17 @@ describe("runGittensoryLinkedIssueSatisfaction gating + fail-safe", () => {
expect(run).toHaveBeenCalled();
});

it("REGRESSION (#5385-sentry, GITTENSORY-K/8): stops retrying a model after ONE 429 rate-limit error instead of burning its full attempt budget", async () => {
const run = vi.fn(async () => {
throw new Error("claude_code_error_429");
});
const result = await runGittensoryLinkedIssueSatisfaction(enabledEnv(run), baseInput);
expect(result.status).toBe("ok");
if (result.status !== "ok") throw new Error("unreachable");
expect(result.result).toBeNull();
expect(run).toHaveBeenCalledTimes(2); // 1 attempt per model (2 models), not the full 6-call budget
});

it("falls back to the reliable model when the primary keeps returning garbage", async () => {
const run = vi.fn(async (model: string) => ({ response: model.includes("gpt-oss") ? "not json" : satisfactionJson({ status: "partial" }) }));
const result = await runGittensoryLinkedIssueSatisfaction(enabledEnv(run), baseInput);
Expand Down
10 changes: 10 additions & 0 deletions test/unit/planner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@ describe("generateIssuePlan (#issue-coding-plan)", () => {
throw new Error("ai down");
});
expect(await generateIssuePlan(createTestEnv({ AI: { run: throwRun } as unknown as Ai }), { title: "T", body: "B" })).toBeNull();
expect(throwRun).toHaveBeenCalledTimes(4); // 2 models x 2 attempts each -- a non-429 error burns the full budget.
});

it("REGRESSION (#5385-sentry, GITTENSORY-K/8): stops retrying a model after ONE 429 rate-limit error instead of burning its full attempt budget", async () => {
const throwRun = vi.fn(async () => {
throw new Error("claude_code_error_429");
});
const env = createTestEnv({ AI: { run: throwRun } as unknown as Ai });
expect(await generateIssuePlan(env, { title: "T", body: "B" })).toBeNull();
expect(throwRun).toHaveBeenCalledTimes(2); // 1 attempt per model (2 models), not the full 4-call budget.
});
});

Expand Down