From d2a14b88507d8ce217c0fde1b8548081b518d7f3 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 25 Jul 2026 23:21:01 -0700 Subject: [PATCH] fix(ai-review): mode-aware inconclusive copy + structural classification for claude-code auth errors (#8791) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two alignment gaps from the 2026-07-26 inconclusive-verdict trace: - The inconclusive finding unconditionally said "dual-model AI review" — on a single-reviewer-with-fallback deployment (the AI_PROVIDER=a,b default without AI_DUAL_REVIEW) that misdescribed a provider-chain outage as a two-model disagreement and sent the incident's debugging down the wrong path. The ok-result now carries plannedReviewerCount (the resolved plan's independent slots, distinct from reviewerCount's produced-output count) and the copy matches what actually ran. - isStructuralProviderConfigError only matched codex_* auth shapes, so an expired Claude Code token (claude_code_no_oauth_token, claude_code_error_401/403) burned the full retry budget every review instead of earning the structural circuit-breaker cooldown. The claude-code shapes now join the codex ones; 429s and exit/stall errors stay transient. --- src/queue/ai-review-orchestration.ts | 8 ++++++- src/services/ai-review.ts | 12 +++++++++- test/unit/ai-review-advisory.test.ts | 34 ++++++++++++++++++++++++++++ test/unit/ai-review.test.ts | 12 ++++++++++ 4 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index 364d9d54cc..0b447c2249 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -795,12 +795,18 @@ export async function runAiReviewForAdvisory( } else if (result.inconclusive) { // Fail-CLOSED (#ai-fail-closed): block-mode AI could not return a usable verdict. Hold the PR for a human // (an evaluation-blocker code → neutral gate) rather than letting it pass to auto-merge uncertified. + // #8791: the copy reflects the plan that ACTUALLY ran. This text unconditionally claimed "dual-model" — + // on a single-reviewer-with-fallback deployment (the AI_PROVIDER=a,b default without AI_DUAL_REVIEW) + // that misdescribed a provider-chain outage as a two-model disagreement and sent the 2026-07-26 + // incident's debugging down the wrong path. findings.push({ code: "ai_review_inconclusive", severity: "warning", title: "AI review could not be completed", detail: - "The dual-model AI review did not return a usable verdict for this change.", + result.plannedReviewerCount === 2 + ? "The dual-model AI review did not return a usable verdict for this change." + : "The AI review (one reviewer, including its provider fallback chain) did not return a usable verdict for this change.", action: "The gate is held for a human reviewer rather than passed automatically; it re-evaluates on the next update.", }); diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index b3fa7ea33b..964396231e 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -416,6 +416,11 @@ export type LoopOverAiReviewResult = inconclusive: boolean; estimatedNeurons: number; reviewerCount: number; + /** #8791: how many INDEPENDENT reviewer slots the resolved plan ran (2 = genuine dual review, 1 = + * single reviewer, possibly with a same-slot provider fallback). Distinct from reviewerCount (the + * count that actually produced output — 0 on a fully-failed run) so the inconclusive finding's copy + * can describe what actually happened instead of unconditionally claiming "dual-model". */ + plannedReviewerCount: 1 | 2; /** Per-reviewer stances for the provider track records (#8229 stage 0). Attribution attaches at leg * PRODUCTION time (a.review ↔ primary.model, b.review ↔ secondary.model), so the tie-break judge's * order-swap — which operates downstream on copies — can never misattribute a vote. Block-mode only @@ -1179,9 +1184,13 @@ export function isRateLimitError(error: unknown): boolean { * short-circuit. Exported so `src/selfhost/ai.ts`'s circuit breaker can give this failure class a much longer * cooldown than a genuinely transient one. */ export function isStructuralProviderConfigError(error: unknown): boolean { + // #8791: the claude-code CLI's own auth-failure shapes join the codex ones — an expired/missing OAuth token + // (claude_code_no_oauth_token) or a 401/403 API rejection is exactly as deterministic as codex_no_auth: the + // same model fails the identical way on every retry until a human rotates the credential, so it deserves the + // same long structural circuit-breaker cooldown instead of burning the full retry budget on every review. return ( error instanceof Error && - /^codex_(?:auth_not_configured|no_auth|credential_isolation_required)(?::|$)/.test(error.message) + /^(?:codex_(?:auth_not_configured|no_auth|credential_isolation_required)|claude_code_(?:no_oauth_token|error_40[13]))(?::|$)/.test(error.message) ); } @@ -2661,6 +2670,7 @@ export async function runLoopOverAiReview( inconclusive, estimatedNeurons, reviewerCount: Math.max(reviewsForNotes.length, fallbackNotes.length), + plannedReviewerCount: dual ? 2 : 1, inlineFindings, valueAssessment, reviewDiagnostics, diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index 524ad3ba4a..2eb0d80325 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -803,6 +803,40 @@ describe("runAiReviewForAdvisory", () => { expect(aiCalls).toBe(12); // 3 attempts x the same 4 model slots — pre-#8790 behavior pinned for varying output }); + it("#8791: a SINGLE-reviewer plan's inconclusive finding says one-reviewer-with-fallback, never 'dual-model'", async () => { + const adv = advisory(); + const env = aiEnv(async () => ({ response: "### prose, never JSON" })); + // combine: "single" resolves dual=false (one reviewer slot) regardless of provider count. + const result = await runAiReviewForAdvisory(env, { + mode: "live", + settings: { aiReviewMode: "block", aiReviewCombine: "single" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + const finding = result?.findings.find((f) => f.code === "ai_review_inconclusive"); + expect(finding?.detail).toContain("one reviewer, including its provider fallback chain"); + expect(finding?.detail).not.toContain("dual-model"); + }); + + it("#8791: the default dual plan's inconclusive finding still says 'dual-model'", async () => { + const adv = advisory(); + const env = aiEnv(async () => ({ response: "### prose, never JSON" })); + const result = await runAiReviewForAdvisory(env, { + mode: "live", + settings: { aiReviewMode: "block" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + const finding = result?.findings.find((f) => f.code === "ai_review_inconclusive"); + expect(finding?.detail).toContain("dual-model"); + }); + it("uses the non-cacheable block-mode inconclusive note when no reviewer returns public text", async () => { const adv = advisory(); const result = await runAiReviewForAdvisory(aiEnv(async () => ({ response: "" })), { diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 7678406901..f104824406 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -3372,6 +3372,18 @@ describe("pure helpers", () => { // Prefix-anchored, but must not match a longer look-alike token that merely starts with the same characters. expect(isStructuralProviderConfigError(new Error("codex_no_auth_pending"))).toBe(false); expect(isStructuralProviderConfigError(new Error("connection reset"))).toBe(false); + }); + + it("#8791: claude-code's own auth-failure shapes are structural too — expired tokens are as deterministic as codex_no_auth", () => { + expect(isStructuralProviderConfigError(new Error("claude_code_no_oauth_token"))).toBe(true); + expect(isStructuralProviderConfigError(new Error("claude_code_no_oauth_token: CLAUDE_CODE_OAUTH_TOKEN not set"))).toBe(true); + expect(isStructuralProviderConfigError(new Error("claude_code_error_401: authentication_error"))).toBe(true); + expect(isStructuralProviderConfigError(new Error("claude_code_error_403"))).toBe(true); + // A 429 is rate-limiting, not structural — it clears on its own and already has its own same-model break. + expect(isStructuralProviderConfigError(new Error("claude_code_error_429: rate_limit_error"))).toBe(false); + // Other claude_code error shapes (exit codes, stalls) stay transient. + expect(isStructuralProviderConfigError(new Error("claude_code_exit_1: something broke"))).toBe(false); + expect(isStructuralProviderConfigError(new Error("claude_stalled_no_output: no stdout within firstOutputTimeoutMs"))).toBe(false); // Anchored ("^codex_...") -- a wrapped/rethrown message doesn't match, only the exact provider-level throw does. expect(isStructuralProviderConfigError(new Error("wrapped: codex_auth_not_configured: nested"))).toBe(false); expect(isStructuralProviderConfigError("codex_auth_not_configured: not an Error instance")).toBe(false);