From dfbfdf158ccac075f65e21ca7b7756893d2da47e Mon Sep 17 00:00:00 2001
From: carlh171112 <carlh171112@gmail.com>
Date: Sun, 12 Jul 2026 16:48:13 -0700
Subject: [PATCH] test(ai-review): add unit tests for isRateLimitError function

Introduces a new test suite for the isRateLimitError function to ensure it correctly identifies rate limit errors (HTTP 429) across various provider error messages. The tests cover matching valid 429 errors, rejecting non-429 errors, and validating the function's behavior with non-Error values. This addition helps prevent future regressions in error handling related to rate limits.
---
 test/unit/ai-review-rate-limit-error.test.ts | 56 ++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 test/unit/ai-review-rate-limit-error.test.ts

diff --git a/test/unit/ai-review-rate-limit-error.test.ts b/test/unit/ai-review-rate-limit-error.test.ts
new file mode 100644
index 0000000000..c4483bc799
--- /dev/null
+++ b/test/unit/ai-review-rate-limit-error.test.ts
@@ -0,0 +1,56 @@
+import { describe, expect, it } from "vitest";
+import { isRateLimitError } from "../../src/services/ai-review";
+
+// Direct contract test for isRateLimitError (#5385-sentry / GITTENSORY-K/8, added in #5481). The helper is the
+// SHARED short-circuit that four independent AI-calling retry loops now depend on — runWorkersOpinion +
+// runDualAiTieBreakJudgeCall (services/ai-review.ts), planner.ts, ai-slop.ts, and
+// linked-issue-satisfaction-run.ts all `break` out of the same-model retry on a 429 instead of burning the
+// remaining per-model budget on a rate-limit window that will not have cleared a few hundred ms later. It
+// shipped wired into those loops but with no test of its own; this pins the exact set of error shapes that count
+// as a rate limit so a future tweak to the `/_(?:http|error)_429$/` matcher can't silently regress the
+// short-circuit across every one of those call sites at once.
+
+describe("isRateLimitError — the shared 429 retry short-circuit (#5481)", () => {
+  it("matches every provider 429 shape src/selfhost/ai.ts actually throws", () => {
+    // ai.ts line 344 `ai_http_${status}`, line 392 `anthropic_http_${status}`, line 990
+    // `claude_code_error_${errStatus}`, and the embeddings path's `ai_embed_http_${status}`.
+    for (const message of ["ai_http_429", "anthropic_http_429", "claude_code_error_429", "ai_embed_http_429"]) {
+      expect(isRateLimitError(new Error(message))).toBe(true);
+    }
+  });
+
+  it("does NOT match a non-429 provider status (a 4xx/5xx that a retry might legitimately clear)", () => {
+    for (const message of ["claude_code_error_404", "ai_http_400", "anthropic_http_500", "ai_http_503"]) {
+      expect(isRateLimitError(new Error(message))).toBe(false);
+    }
+  });
+
+  it("does NOT match the sibling non-transient error that has its own separate short-circuit", () => {
+    // runWorkersOpinion breaks on `isSubscriptionCliTimeout(error) || isRateLimitError(error)` — the two guards
+    // are distinct, so a CLI timeout must not be absorbed by the 429 matcher.
+    expect(isRateLimitError(new Error("subscription_cli_timeout"))).toBe(false);
+    expect(isRateLimitError(new Error("codex_exit_1: unknown model"))).toBe(false);
+    expect(isRateLimitError(new Error("claude_code_empty_output"))).toBe(false);
+  });
+
+  it("anchors on 429 as the FULL suffix — a longer status or a trailing detail is not a 429", () => {
+    // The `$` anchor is deliberate: only a bare `..._http_429` / `..._error_429` is the rate-limit signal.
+    expect(isRateLimitError(new Error("ai_http_4291"))).toBe(false);
+    expect(isRateLimitError(new Error("claude_code_error_429_retryable"))).toBe(false);
+    expect(isRateLimitError(new Error("ai_http_429: too many requests"))).toBe(false);
+  });
+
+  it("requires the `_http`/`_error` delimiter, so a bare '429' substring never trips it", () => {
+    expect(isRateLimitError(new Error("http_429"))).toBe(false); // no leading `_` before `http`
+    expect(isRateLimitError(new Error("429"))).toBe(false);
+    expect(isRateLimitError(new Error("rate limited (429)"))).toBe(false);
+  });
+
+  it("is false for any non-Error value (the `error instanceof Error` guard)", () => {
+    expect(isRateLimitError("ai_http_429")).toBe(false); // a matching string, but not an Error instance
+    expect(isRateLimitError(null)).toBe(false);
+    expect(isRateLimitError(undefined)).toBe(false);
+    expect(isRateLimitError(429)).toBe(false);
+    expect(isRateLimitError({ message: "ai_http_429" })).toBe(false);
+  });
+});