fix(proxy): rectifier 不应绕过 maxRetryAttempts by tesgth032 · Pull Request #924 · ding113/claude-code-hub

tesgth032 · 2026-03-15T12:38:27Z

Summary

Fix thinking signature and thinking budget rectifiers to respect the provider-level maxRetryAttempts configuration. Previously, these rectifiers would bypass maxRetryAttempts by using Math.max() to force an additional retry, making configurations like maxRetryAttempts=1 ineffective.

Problem

Single-provider max retry attempts (providers.max_retry_attempts) can be bypassed in thinking signature / thinking budget rectifier scenarios: even when configured to 1, an additional request to the same provider would still be made.

Related Issues:

Related to feat: thinking signature rectifier #576 - The original thinking signature rectifier feature this bug affects

Root Cause

ProxyForwarder rectifier branch was forcibly expanding maxAttemptsPerProvider using Math.max(), causing configured lower values to have no effect.

Solution

When rectifier hits, strictly follow maxRetryAttempts:
- attemptCount < maxRetryAttempts: continue retrying with the same provider
- Otherwise: directly enter provider failover (switch vendor)
When failover is triggered due to reaching retry limit and no alternative provider available: throw original error (typically 400) instead of returning generic 503, avoiding masking client input issues

Changes

Core Changes

src/app/v1/_lib/proxy/forwarder.ts (+80/-27)
- Added lastRectifierSwitchError to preserve original error when failover triggered
- Check hasRetryBudget = attemptCount < maxAttemptsPerProvider before retrying
- When no retry budget remains, break to provider switch loop instead of forcing retry
- If no alternative provider available after failover trigger, throw original 400 error instead of 503

Supporting Changes

tests/unit/proxy/proxy-forwarder-retry-limit.test.ts (+437/-0)
- Added comprehensive unit tests covering:
  - maxRetryAttempts=1 should switch provider (not bypass)
  - maxRetryAttempts=1 + no alternative should throw original 400 (not 503)
  - maxRetryAttempts=2 should still retry same provider

Testing

Automated Tests

Unit tests added for maxRetryAttempts rectifier behavior
bun run test passes
bun run typecheck passes
bun run build passes

Checklist

Code follows project conventions
Self-review completed
Tests pass locally
Documentation updated (if needed)

Description enhanced by Claude AI

Greptile Summary

This PR fixes the Math.max() bypass in the thinking-signature and thinking-budget rectifiers — previously, even a maxRetryAttempts=1 config would silently get an extra same-provider attempt. The fix introduces a hasRetryBudget check that either retries the same provider (if budget permits) or breaks to provider failover, throwing the original 400 error when no alternative exists. A parallel set of changes adds the same rectifier logic to the streamingHedge path, including a withRectifierLock mutex, providerAttemptCounts tracking, and stale-request version detection.

Key changes:

forwarder.ts (send): Replaced Math.max(maxAttemptsPerProvider, attemptCount + 1) with hasRetryBudget guard; introduced lastRectifierSwitchError to propagate the original 400 instead of a 503 when no alternative provider exists.
forwarder.ts (streamingHedge): Added ~300 lines of new rectifier handling mirroring the send() fix, with a withRectifierLock mutex to prevent concurrent rectification, providerAttemptCounts for per-provider attempt tracking, and stale-request-version detection.
special-settings.ts / types/special-settings.ts: retryAttemptNumber is now optional; buildSettingKey updated accordingly.
Tests: Comprehensive new cases for both send() and streamingHedge, including same-provider retry, provider switch, no-alternative 400 throw, and concurrent stale-attempt scenarios.

Issues found:

In streamingHedge's startAttempt, providerAttemptCounts is incremented before the async endpoint selection. If endpoint selection returns null, the phantom increment inflates subsequent stale-path hasRetryBudget checks and can trigger redundant launchAlternative() calls.
When a stale attempt finds hasProviderInFlight = true, the function returns true without calling session.addProviderToChain(...). Every other error-exit path from the rectifier block records a chain entry; this omission means concurrent hedge failures during rectifier handling are invisible in audit logs.
The stale-retry path (hasProviderInFlight = false, hasRetryBudget = true) starts a new attempt without adding the provider to signatureRectifierRetriedProviderIds / budgetRectifierRetriedProviderIds. If that retry also fails with a rectifier-triggerable error, the guard is bypassed, causing a redundant rectifyAnthropicRequestMessage call (applied=false), an extraneous hit: false audit entry, and an additional unnecessary selectAlternative call.

Confidence Score: 2/5

Not safe to merge — the PR is explicitly marked [未完成] (incomplete) and contains logic bugs in the new streamingHedge rectifier path.
The send() path fix is solid and well-tested. However, the new streamingHedge rectifier implementation (~300 lines) has three issues: providerAttemptCounts being incremented before endpoint validation can cause inflated budget checks and redundant failovers; stale-concurrent failures are silently dropped from the provider chain audit; and the stale retry path skips updating the rectifier-retried Sets, creating a guard bypass on subsequent failures. The PR also self-declares it is incomplete.
src/app/v1/_lib/proxy/forwarder.ts — specifically the new streamingHedge rectifier block (~lines 3152–3445) and the startAttempt function (~lines 3572–3640).

Important Files Changed

Filename	Overview
src/app/v1/_lib/proxy/forwarder.ts	Core fix replacing `Math.max()` bypass with proper `hasRetryBudget` checks; also adds ~300 lines of new rectifier handling to `streamingHedge` (including a mutex, `providerAttemptCounts`, stale-request detection). Several issues found: `providerAttemptCounts` incremented before endpoint validation, stale in-flight sibling failures dropped from audit chain, and stale retry path not marking providers in the rectifier-retried Sets.
tests/unit/proxy/proxy-forwarder-retry-limit.test.ts	Substantial new test coverage for both `send()` and `streamingHedge` rectifier paths including maxRetryAttempts=1 switch, no-alternative 400 throw, maxRetryAttempts=2 same-provider retry, and concurrent stale-attempt scenarios.
src/types/special-settings.ts	Makes `retryAttemptNumber` optional on both rectifier setting types to accurately reflect that no retry is planned when the budget is exhausted; well-documented with JSDoc comments.
src/lib/utils/special-settings.ts	Updates `buildSettingKey` to use `?? null` for the now-optional `retryAttemptNumber` field, keeping deduplication logic consistent.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Attempt fails with Anthropic 400] --> B{Is Anthropic provider?}
    B -- No --> Z[Normal error handling]
    B -- Yes --> C{Signature or Budget\nrectifier trigger?}
    C -- No --> Z
    C -- Yes --> D[Set errorCategory =\nNON_RETRYABLE_CLIENT_ERROR]
    D --> E{Acquire rectifierLock}
    E --> F{settled or\nwinnerCommitted?}
    F -- Yes --> R[return false\nnot handled]
    F -- No --> G{attempt.requestVersion\n< requestVersion?}

    G -- Yes stale --> H{hasProviderInFlight\nwith current version?}
    H -- Yes --> I[return true\nstale, sibling handles it]
    H -- No --> J{currentAttemptNumber\n< maxAttemptsPerProvider?}
    J -- Yes --> K[startAttempt same provider\nallowDuplicate]
    J -- No --> L[launchAlternative\nfinishIfExhausted]
    K --> M[return true handled]
    L --> M

    G -- No fresh --> N{providerID already\nin rectifierRetriedSet?}
    N -- Yes --> R
    N -- No --> O[rectifyAnthropicRequestMessage\nmutate session.request.message]
    O --> P{rectified.applied?}
    P -- No --> R
    P -- Yes --> Q[requestVersion++\naddProviderToChain reason=retry_failed]
    Q --> S{attempt.providerAttemptNumber\n< maxAttemptsPerProvider?\nhasRetryBudget}
    S -- Yes --> T[add to rectifierRetriedSet\nstartAttempt same provider]
    S -- No --> U[launchAlternative\nfinishIfExhausted]
    T --> M
    U --> M

Comments Outside Diff (3)

src/app/v1/_lib/proxy/forwarder.ts, line 1108-1118 (link)

retryAttemptNumber audit field is misleading in the provider-switch path

retryAttemptNumber: attemptCount + 1 is recorded unconditionally in addSpecialSetting, before the hasRetryBudget check lower in the same block. When hasRetryBudget is false, no retry will occur on this provider — the code breaks out and either switches provider or throws. The audit record therefore claims a retry at attemptCount + 1 that never happens on this provider, which could confuse any tooling or monitoring that consumes these records.

Consider computing and passing retryAttemptNumber conditionally (or using a separate field like switchedProvider: true for the no-budget branch), or populating the field only after determining whether a retry will actually take place. The same issue applies to the thinking budget rectifier's addSpecialSetting call at the equivalent location in section 2.6.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/forwarder.ts
Line: 1108-1118

Comment:
**`retryAttemptNumber` audit field is misleading in the provider-switch path**

`retryAttemptNumber: attemptCount + 1` is recorded unconditionally in `addSpecialSetting`, before the `hasRetryBudget` check lower in the same block. When `hasRetryBudget` is `false`, no retry will occur on this provider — the code breaks out and either switches provider or throws. The audit record therefore claims a retry at `attemptCount + 1` that never happens on this provider, which could confuse any tooling or monitoring that consumes these records.

Consider computing and passing `retryAttemptNumber` conditionally (or using a separate field like `switchedProvider: true` for the no-budget branch), or populating the field only after determining whether a retry will actually take place. The same issue applies to the thinking budget rectifier's `addSpecialSetting` call at the equivalent location in section 2.6.

How can I resolve this? If you propose a fix, please make it concise.

tests/unit/proxy/proxy-forwarder-retry-limit.test.ts, line 993-1059 (link)

Missing streaming hedge + budget rectifier maxRetryAttempts=2 test

The streaming hedge tests cover the signature rectifier for both maxRetryAttempts=1 (switch provider, line 859) and maxRetryAttempts=2 (retry same provider, line 931), and the budget rectifier for maxRetryAttempts=1 (line 993). However, there is no corresponding test for streaming hedge + thinking budget rectifier: maxRetryAttempts=2 should retry same provider.

Without this test, the budget rectifier's per-provider retry path (hasRetryBudget=true → budgetRectifierRetriedProviderIds.add → re-launch via startAttempt(…, { allowDuplicateProvider: true })) in streamingHedge has no coverage, leaving an asymmetry with the signature rectifier path that IS covered. Consider adding a parallel test:

test("streaming hedge + thinking budget rectifier: maxRetryAttempts=2 should retry same provider", async () => {
  vi.useFakeTimers();
  try {
    const session = createSession();
    (session.request.message as any).stream = true;
    const providerA = createProvider({ id: 1, name: "p1", providerType: "claude", providerVendorId: null, maxRetryAttempts: 2 });
    session.setProvider(providerA);
    (session.request.message as any).thinking = { type: "enabled", budget_tokens: 1 };
    delete (session.request.message as any).max_tokens;

    const doForward = vi.spyOn(ProxyForwarder as any, "doForward");
    const selectAlternative = vi.spyOn(ProxyForwarder as any, "selectAlternative");

    doForward.mockImplementationOnce(async () => {
      throw new ProxyError("thinking.enabled.budget_tokens: Input should be greater than or equal to 1024", 400);
    });
    doForward.mockResolvedValueOnce(new Response("data", { status: 200, headers: { "content-type": "text/event-stream" } }));

    const response = await ProxyForwarder.send(session);
    expect(response.status).toBe(200);
    expect(doForward).toHaveBeenCalledTimes(2);
    expect(selectAlternative).not.toHaveBeenCalled();
    expect(doForward.mock.calls.map((c) => (c[1] as Provider).id)).toEqual([1, 1]);
    expect(doForward.mock.calls.map((c) => c[4] as number)).toEqual([1, 2]);
  } finally {
    vi.useRealTimers();
  }
});

Prompt To Fix With AI

This is a comment left during a code review.
Path: tests/unit/proxy/proxy-forwarder-retry-limit.test.ts
Line: 993-1059

Comment:
**Missing streaming hedge + budget rectifier maxRetryAttempts=2 test**

The streaming hedge tests cover the signature rectifier for both `maxRetryAttempts=1` (switch provider, line 859) and `maxRetryAttempts=2` (retry same provider, line 931), and the budget rectifier for `maxRetryAttempts=1` (line 993). However, there is no corresponding test for `streaming hedge + thinking budget rectifier: maxRetryAttempts=2 should retry same provider`.

Without this test, the budget rectifier's per-provider retry path (`hasRetryBudget=true` → `budgetRectifierRetriedProviderIds.add` → re-launch via `startAttempt(…, { allowDuplicateProvider: true })`) in `streamingHedge` has no coverage, leaving an asymmetry with the signature rectifier path that IS covered. Consider adding a parallel test:

```typescript
test("streaming hedge + thinking budget rectifier: maxRetryAttempts=2 should retry same provider", async () => {
  vi.useFakeTimers();
  try {
    const session = createSession();
    (session.request.message as any).stream = true;
    const providerA = createProvider({ id: 1, name: "p1", providerType: "claude", providerVendorId: null, maxRetryAttempts: 2 });
    session.setProvider(providerA);
    (session.request.message as any).thinking = { type: "enabled", budget_tokens: 1 };
    delete (session.request.message as any).max_tokens;

    const doForward = vi.spyOn(ProxyForwarder as any, "doForward");
    const selectAlternative = vi.spyOn(ProxyForwarder as any, "selectAlternative");

    doForward.mockImplementationOnce(async () => {
      throw new ProxyError("thinking.enabled.budget_tokens: Input should be greater than or equal to 1024", 400);
    });
    doForward.mockResolvedValueOnce(new Response("data", { status: 200, headers: { "content-type": "text/event-stream" } }));

    const response = await ProxyForwarder.send(session);
    expect(response.status).toBe(200);
    expect(doForward).toHaveBeenCalledTimes(2);
    expect(selectAlternative).not.toHaveBeenCalled();
    expect(doForward.mock.calls.map((c) => (c[1] as Provider).id)).toEqual([1, 1]);
    expect(doForward.mock.calls.map((c) => c[4] as number)).toEqual([1, 2]);
  } finally {
    vi.useRealTimers();
  }
});
```

How can I resolve this? If you propose a fix, please make it concise.

tests/unit/proxy/proxy-forwarder-retry-limit.test.ts, line 1556-1599 (link)

Streaming hedge tests missing getPreferredProviderEndpoints setup

The streaming hedge tests (starting here) set stream = true and rely on the streamingHedge code path, which calls getPreferredProviderEndpoints internally for endpoint selection inside startAttempt. However, unlike the "raw passthrough policy parity" describe block (line ~979), there is no explicit mocks.getPreferredProviderEndpoints.mockResolvedValue(...) call inside these streaming tests.

Because vi.clearAllMocks() in beforeEach clears recorded calls but does not reset implementations, these tests implicitly depend on whatever state getPreferredProviderEndpoints was left in by a previously-run test. If test execution order changes (e.g., test file isolation, --randomize flag, or future test additions), getPreferredProviderEndpoints could return undefined or [], causing startAttempt to bail out early on endpoint selection failure before doForward is even called — silently invalidating these tests.

Add an explicit mock setup at the top of each streaming hedge test:

mocks.getPreferredProviderEndpoints.mockResolvedValue([
  makeEndpoint({ id: 1, vendorId: 0, providerType: "claude", url: "https://ep.example.com" }),
]);

Prompt To Fix With AI

This is a comment left during a code review.
Path: tests/unit/proxy/proxy-forwarder-retry-limit.test.ts
Line: 1556-1599

Comment:
**Streaming hedge tests missing `getPreferredProviderEndpoints` setup**

The streaming hedge tests (starting here) set `stream = true` and rely on the `streamingHedge` code path, which calls `getPreferredProviderEndpoints` internally for endpoint selection inside `startAttempt`. However, unlike the `"raw passthrough policy parity"` describe block (line ~979), there is no explicit `mocks.getPreferredProviderEndpoints.mockResolvedValue(...)` call inside these streaming tests.

Because `vi.clearAllMocks()` in `beforeEach` clears recorded calls but does **not** reset implementations, these tests implicitly depend on whatever state `getPreferredProviderEndpoints` was left in by a previously-run test. If test execution order changes (e.g., test file isolation, `--randomize` flag, or future test additions), `getPreferredProviderEndpoints` could return `undefined` or `[]`, causing `startAttempt` to bail out early on endpoint selection failure before `doForward` is even called — silently invalidating these tests.

Add an explicit mock setup at the top of each streaming hedge test:
```typescript
mocks.getPreferredProviderEndpoints.mockResolvedValue([
  makeEndpoint({ id: 1, vendorId: 0, providerType: "claude", url: "https://ep.example.com" }),
]);
```

How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/forwarder.ts
Line: 3152-3165

Comment:
**Stale attempt with in-flight sibling silently drops from audit chain**

When `hasProviderInFlight` is `true`, the function returns `true` immediately without adding a `addProviderToChain` entry for the stale failure. Every other failure exit from the rectifier block (the `!hasProviderInFlight` stale path and the fresh path) calls `session.addProviderToChain(...)` before proceeding. Silently swallowing this failure means concurrent streaming-hedge failures during rectifier handling are invisible to audit logs — operators will see fewer chain entries than actual upstream requests were made.

Consider adding a chain entry before the early return:
```typescript
if (hasProviderInFlight) {
  // Record the stale failure so the audit chain reflects every request made.
  session.addProviderToChain(attempt.provider, {
    ...attempt.endpointAudit,
    reason: "retry_failed",
    attemptNumber: attempt.sequence,
    statusCode,
    errorMessage,
    circuitState: getCircuitState(attempt.provider.id),
  });
  return true;
}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/forwarder.ts
Line: 3585-3607

Comment:
**`providerAttemptCounts` incremented before endpoint validation succeeds**

`providerAttemptCounts.set(provider.id, providerAttemptNumber)` is executed synchronously before the async endpoint selection. If `endpointSelection` is `null` (no endpoints available), the function exits early — but the count is already bumped. In subsequent stale-path checks for the same provider:

```typescript
const currentAttemptNumber =
  providerAttemptCounts.get(attempt.provider.id) ?? attempt.providerAttemptNumber;
const hasRetryBudget = currentAttemptNumber < attempt.maxAttemptsPerProvider;
```

`currentAttemptNumber` will reflect the phantom increment, making `hasRetryBudget` appear exhausted one step earlier than it truly is. Worse, if `hasRetryBudget` is still `true`, the stale path calls `startAttempt` again, which increments the count a second time and hits endpoint selection failure once more — producing a redundant `launchAlternative()` chain.

Move the `providerAttemptCounts.set` call to after a successful endpoint resolution (alongside `startedProviderIds.add`):

```typescript
// After: if (!endpointSelection) { ... return; }
startedProviderIds.add(provider.id);
providerAttemptCounts.set(provider.id, providerAttemptNumber);
launchedProviderCount += 1;
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/forwarder.ts
Line: 3199-3215

Comment:
**Stale retry path does not mark provider in `signatureRectifierRetriedProviderIds`**

When the stale path decides `hasRetryBudget = true` and calls `startAttempt`, it never adds `attempt.provider.id` to `signatureRectifierRetriedProviderIds`. If that stale retry subsequently fails with another signature error, its `requestVersion` will equal the current `requestVersion` (fresh path), the guard `signatureRectifierRetriedProviderIds.has(attempt.provider.id)` returns `false`, and the code calls `rectifyAnthropicRequestMessage` a second time. Since the message is already clean, `rectified.applied = false` and the handler returns `false`. This causes the failure to fall through to the normal error-handling path, which calls `launchAlternative()` again.

While not catastrophic (idempotent rectifier + `NON_RETRYABLE_CLIENT_ERROR` prevents circuit-breaker writes), it produces an extraneous audit entry with `hit: false` and an unnecessary `selectAlternative` call.

Add the provider to the set before launching the stale retry:
```typescript
if (hasRetryBudget) {
  signatureRectifierRetriedProviderIds.add(attempt.provider.id);
  await startAttempt(attempt.provider, attempt.provider.id === initialProvider.id, {
    allowDuplicateProvider: true,
  });
  return true;
}
```

The same pattern applies to the budget rectifier stale path.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 6f6daf6}

Greptile also left 3 inline comments on this PR.

coderabbitai · 2026-03-15T12:38:41Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

在外层每次迭代中新增整流器切换错误追踪（lastRectifierSwitchError）；将单个 provider 的重试决策改为基于思考整流器预算（thinking-signature / thinking-budget）；在预算耗尽时记录切换并在无替代 provider 时重新抛出原始错误；新增/调整大量单元测试并导出两个思考预算常量；对序列/尝试计数与端点选择逻辑做出多处内部状态与日志增强。

Changes

Cohort / File(s)	Summary
整流器预算与失败追踪 `src/app/v1/_lib/proxy/forwarder.ts`	扩展 per-iteration 与 per-provider 状态（如 `lastRectifierSwitchError`, `startedProviderIds`, `providerAttemptCounts`, `providerAttemptNumber`, `maxAttemptsPerProvider`）；将重试决策改为基于是否存在 retry 预算（hasRetryBudget），在预算耗尽时记录切换并在无替代 provider 时清理会话绑定后抛出原始错误；调整 hedge-winner 判定为基于唯一 provider 数量；更新 startAttempt 签名以支持去重与 per-provider 计数。
思维预算常量导出 `src/app/v1/_lib/proxy/thinking-budget-rectifier.ts`	将内部常量 `MAX_THINKING_BUDGET` 与 `MAX_TOKENS_VALUE` 改为导出（`export const`），不改变数值。
特殊设置键序列化微调 `src/lib/utils/special-settings.ts`	在构建 thinking rectifier 的 key 时将 `retryAttemptNumber` 使用 `?? null` 明确为 null（当未定义时），避免在序列化数组中出现 undefined。
类型调整：重试次数可选 `src/types/special-settings.ts`	将 `ThinkingSignatureRectifierSpecialSetting.retryAttemptNumber` 与 `ThinkingBudgetRectifierSpecialSetting.retryAttemptNumber` 从必需字段改为可选（`?: number`），并补充 JSDoc 说明。
单元测试：重试限制与整流器交互 `tests/unit/proxy/proxy-forwarder-retry-limit.test.ts`	新增/修改大量测试与 mocks（包含对 `getCachedSystemSettings`、`isHttp2Enabled` 的测试时覆盖）；新增测试套件验证 `maxRetryAttempts` 与 thinking-signature/ thinking-budget 整流器的交互、预算耗尽时的 provider 切换与回退行为、端点进阶/粘性/延迟排序与重试计数断言；调整定时器控制与清理。

Estimated code review effort

🎯 3 (中等) | ⏱️ ~25 分钟

Possibly related PRs

fix(proxy): implement endpoint stickiness on retries #651 — 修改 src/app/v1/_lib/proxy/forwarder.ts 中的重试与端点粘性逻辑，与本次对 retry/endpoint 选择与切换的改动有直接重合。
feat: thinking signature rectifier #576 — 对 thinking-signature 整流器相关逻辑做出调整，与本 PR 在整流器驱动的重试/预算交互方面存在代码层面关联。

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description check	✅ Passed	The pull request description clearly explains the problem (thinking rectifiers bypassing maxRetryAttempts via Math.max), root cause, and solution with specific code changes.
Title check	✅ Passed	标题准确地总结了拉取请求的核心变更：修复思维签名和预算校正器不应该通过 Math.max() 绕过 maxRetryAttempts 限制。标题与代码改动（在 forwarder.ts 中移除 Math.max 绕过、添加 lastRectifierSwitchError 等）高度相关，清晰地表示了主要修复内容。

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📝 Coding Plan

Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

gemini-code-assist · 2026-03-15T12:38:45Z

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

此拉取请求旨在解决代理转发器中一个关键的重试逻辑缺陷。此前，思考签名和思考预算整流器在处理某些错误时，会无视单个供应商配置的最大重试次数，导致不必要的额外请求。本次修复确保了所有重试机制都严格遵循配置的重试限制，并在无法找到替代供应商时，通过抛出更准确的原始错误来改善错误诊断，从而提升了系统的稳定性和可预测性。

Highlights

整流器重试逻辑修复: 修复了整流器（thinking signature / thinking budget rectifier）在特定场景下会绕过 maxRetryAttempts 配置的问题。
严格遵守重试限制: 现在整流器会严格遵守 maxRetryAttempts，只有在有重试预算时才会在同一供应商上重试，否则会尝试切换供应商。
改进错误处理: 当整流器触发供应商切换，但因达到重试上限且没有可用替代供应商时，将抛出原始错误（通常是 400），而不是统一返回 503，以避免掩盖客户端输入问题。
新增单元测试: 新增了全面的单元测试，以验证整流器在不同 maxRetryAttempts 配置下的行为，包括供应商切换和错误回退逻辑。

Changelog

src/app/v1/_lib/proxy/forwarder.ts
- 新增 lastRectifierSwitchError 变量，用于在整流器触发切换但无替代供应商时保留原始错误。
- 修改了思考签名和思考预算整流器的逻辑，使其在决定是否重试或切换供应商时，严格检查 attemptCount 是否小于 maxAttemptsPerProvider。
- 更新了相关日志信息，以更清晰地反映整流器是重试还是切换供应商。
- 在没有可用替代供应商的情况下，如果存在 lastRectifierSwitchError，则抛出该错误。
tests/integration/usage-ledger.test.ts
- 调整了 test 块的格式，将配置对象作为第二个参数传递。
tests/unit/proxy/proxy-forwarder-retry-limit.test.ts
- 为 getCachedSystemSettings 和 isHttp2Enabled 添加了模拟实现。
- 新增了一组测试用例，专门验证整流器在不同 maxRetryAttempts 设置下的行为。
- 测试了当 maxRetryAttempts=1 时，思考签名和思考预算整流器应切换供应商而非在同一供应商上重试。
- 测试了当 maxRetryAttempts=1 且没有替代供应商时，应抛出原始的 400 错误。
- 测试了当 maxRetryAttempts=2 时，思考签名和思考预算整流器应在同一供应商上重试。

Activity

作者已新增单元测试，覆盖了 maxRetryAttempts=1 时整流器应切换供应商、无替代供应商时回退抛出 400 错误，以及 maxRetryAttempts=2 时仍会在同供应商重试的场景。
作者已确认 bun run test、bun run typecheck 和 bun run build 命令均已通过。

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

The pull request refactors the ProxyForwarder's 'Thinking signature rectifier' and 'Thinking budget rectifier' logic to strictly adhere to maxRetryAttempts per provider. This ensures that if a rectifier is triggered and the current provider has exhausted its retry budget, the system will attempt to switch to an alternative provider instead of retrying with the same one. Additionally, if no alternative provider is available after a rectifier-triggered switch, the original client-side error (e.g., 400) is now propagated instead of a generic 503, preventing the masking of client input issues. New unit tests were added to validate this behavior for both rectifiers under various maxRetryAttempts scenarios, including cases with and without alternative providers. A minor formatting change was also applied to an integration test file.

coderabbitai

🧹 Nitpick comments (1)

tests/unit/proxy/proxy-forwarder-retry-limit.test.ts (1)
470-473: 考虑导出常量，在测试中使用而非硬编码数值。

测试中硬编码了 32000 和 64000 作为期望值。这些值与 thinking-budget-rectifier.ts 中的 MAX_THINKING_BUDGET 和 MAX_TOKENS_VALUE 常量相匹配。然而，这些常量目前未导出，使得在测试中引入它们不可行。如果这两个常量被导出，可以在测试中导入并使用它们，这样在常量值变化时，测试会随之自动调整。
可选改进：导出常量后在测试中使用

在 thinking-budget-rectifier.ts 中导出常量：
+export const MAX_THINKING_BUDGET = 32000;
+export const MAX_TOKENS_VALUE = 64000;
然后在测试中导入：
+import {
+  MAX_THINKING_BUDGET,
+  MAX_TOKENS_VALUE,
+} from "@/app/v1/_lib/proxy/thinking-budget-rectifier";

-expect(thinking.budget_tokens).toBe(32000);
-expect((session.request.message as any).max_tokens).toBe(64000);
+expect(thinking.budget_tokens).toBe(MAX_THINKING_BUDGET);
+expect((session.request.message as any).max_tokens).toBe(MAX_TOKENS_VALUE);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts` around lines 470 - 473,
Export the constants MAX_THINKING_BUDGET and MAX_TOKENS_VALUE from
thinking-budget-rectifier.ts and update the test to import and use those exports
instead of the hardcoded numbers; specifically, add exports for
MAX_THINKING_BUDGET and MAX_TOKENS_VALUE in the module that defines them and
replace the literal 32000 and 64000 in proxy-forwarder-retry-limit.test.ts with
the imported MAX_THINKING_BUDGET and MAX_TOKENS_VALUE so the test follows the
canonical source of truth.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts`:
- Around line 470-473: Export the constants MAX_THINKING_BUDGET and
MAX_TOKENS_VALUE from thinking-budget-rectifier.ts and update the test to import
and use those exports instead of the hardcoded numbers; specifically, add
exports for MAX_THINKING_BUDGET and MAX_TOKENS_VALUE in the module that defines
them and replace the literal 32000 and 64000 in
proxy-forwarder-retry-limit.test.ts with the imported MAX_THINKING_BUDGET and
MAX_TOKENS_VALUE so the test follows the canonical source of truth.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 18bcbbb9-cf12-4ae8-af5a-87ccd7dba2ef

📥 Commits

Reviewing files that changed from the base of the PR and between 412c523 and 13810df.

📒 Files selected for processing (3)

src/app/v1/_lib/proxy/forwarder.ts
tests/integration/usage-ledger.test.ts
tests/unit/proxy/proxy-forwarder-retry-limit.test.ts

github-actions

Code Review Summary

This PR fixes a bug where max_retry_attempts configuration was being bypassed by the thinking signature and budget rectifiers. The fix ensures rectifiers respect the configured retry limits and properly fallback to throwing the original 400 error (instead of 503) when no alternative provider exists.

PR Size: M

Lines changed: 556 additions, 68 deletions
Files changed: 3

Issues Found

Category	Critical	High	Medium	Low
Logic/Bugs	0	0	0	0
Security	0	0	0	0
Error Handling	0	0	0	0
Types	0	0	0	0
Comments/Docs	0	0	0	0
Tests	0	0	0	0
Simplification	0	0	0	0

Critical Issues (Must Fix)

None identified.

High Priority Issues (Should Fix)

None identified.

Review Coverage

Logic and correctness - Clean
Security (OWASP Top 10) - Clean
Error handling - Clean
Type safety - Clean
Documentation accuracy - Clean
Test coverage - Adequate (8 new comprehensive unit tests)
Code clarity - Good

Test Summary

The PR includes 8 new unit tests covering:

Thinking signature rectifier with maxRetryAttempts=1 (should switch provider, not retry same)
Thinking budget rectifier with maxRetryAttempts=1 (should switch provider, not retry same)
No alternative provider should throw original 400 (not 503) - for both rectifiers
maxRetryAttempts=2 should retry same provider (not switch) - for both rectifiers

All tests properly validate the fix using mocked dependencies and fake timers.

Automated review by Claude AI

greptile-apps · 2026-03-15T12:52:20Z

+        if (lastRectifierSwitchError) {
+          throw lastRectifierSwitchError;
+        }


Missing session provider binding cleanup

When lastRectifierSwitchError is thrown here, execution exits the outer while loop via throw, completely bypassing the clearSessionProviderBinding(session) call at line 1854. Every other error-exit path in this method calls clearSessionProviderBinding before propagating an error (e.g. client-abort at line 1053, and the all-providers-failed path at line 1854). Skipping it here means SessionManager.clearSessionProvider(session.sessionId) is never invoked, potentially leaving a stale provider binding in the session store that could affect future requests on the same session.

Suggested change

if (lastRectifierSwitchError) {

throw lastRectifierSwitchError;

}

if (!alternativeProvider) {

if (lastRectifierSwitchError) {

await ProxyForwarder.clearSessionProviderBinding(session);

throw lastRectifierSwitchError;

}

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 1817-1819 Comment: **Missing session provider binding cleanup** When `lastRectifierSwitchError` is thrown here, execution exits the outer `while` loop via `throw`, completely bypassing the `clearSessionProviderBinding(session)` call at line 1854. Every other error-exit path in this method calls `clearSessionProviderBinding` before propagating an error (e.g. client-abort at line 1053, and the all-providers-failed path at line 1854). Skipping it here means `SessionManager.clearSessionProvider(session.sessionId)` is never invoked, potentially leaving a stale provider binding in the session store that could affect future requests on the same session. ```suggestion if (!alternativeProvider) { if (lastRectifierSwitchError) { await ProxyForwarder.clearSessionProviderBinding(session); throw lastRectifierSwitchError; } ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T12:52:25Z

+      expect(doForward).toHaveBeenCalledTimes(1);
+      expect(selectAlternative).toHaveBeenCalledTimes(1);
+    } finally {
+      vi.useRealTimers();
+    }
+  });
+
+  test("thinking budget rectifier: maxRetryAttempts=1 + no alternative provider should throw original 400 (not 503)", async () => {
+    vi.useFakeTimers();
+
+    try {
+      const session = createSession();


Potentially unreliable caught variable check after vi.runAllTimersAsync()

The test attaches a .catch() callback, awaits vi.runAllTimersAsync(), and then inspects the caught variable. The .catch() callback itself is resolved as a microtask when sendPromise rejects; there is no guarantee it has run by the time the expect line executes — this is a subtle but well-known race in promise-based test patterns.

The same pattern is used in three other tests in this file (the two "no alternative provider should throw" cases and the budget rectifier variant).

Prefer the standard Vitest idiom that properly awaits the full promise rejection:

// Instead of: const sendPromise = ProxyForwarder.send(session); let caught: unknown = null; sendPromise.catch((error) => { caught = error; }); await vi.runAllTimersAsync(); expect(caught).toBeInstanceOf(ProxyError); expect((caught as ProxyError).statusCode).toBe(400); // Use: const sendPromise = ProxyForwarder.send(session); vi.runAllTimersAsync(); // don't await — let the underlying async work progress await expect(sendPromise).rejects.toBeInstanceOf(ProxyError); await expect(sendPromise).rejects.toMatchObject({ statusCode: 400 });

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/unit/proxy/proxy-forwarder-retry-limit.test.ts Line: 537-548 Comment: **Potentially unreliable `caught` variable check after `vi.runAllTimersAsync()`** The test attaches a `.catch()` callback, awaits `vi.runAllTimersAsync()`, and then inspects the `caught` variable. The `.catch()` callback itself is resolved as a microtask when `sendPromise` rejects; there is no guarantee it has run by the time the `expect` line executes — this is a subtle but well-known race in promise-based test patterns. The same pattern is used in three other tests in this file (the two "no alternative provider should throw" cases and the budget rectifier variant). Prefer the standard Vitest idiom that properly awaits the full promise rejection: ```typescript // Instead of: const sendPromise = ProxyForwarder.send(session); let caught: unknown = null; sendPromise.catch((error) => { caught = error; }); await vi.runAllTimersAsync(); expect(caught).toBeInstanceOf(ProxyError); expect((caught as ProxyError).statusCode).toBe(400); // Use: const sendPromise = ProxyForwarder.send(session); vi.runAllTimersAsync(); // don't await — let the underlying async work progress await expect(sendPromise).rejects.toBeInstanceOf(ProxyError); await expect(sendPromise).rejects.toMatchObject({ statusCode: 400 }); ``` How can I resolve this? If you propose a fix, please make it concise.

coderabbitai

🧹 Nitpick comments (1)

src/app/v1/_lib/proxy/forwarder.ts (1)

1162-1236: 建议抽取 rectifier 重试预算处理的公共逻辑。

signature / budget 两段分支的预算判定、日志与切换路径几乎一致，后续容易出现单侧修复遗漏。建议抽成一个内部 helper，降低维护风险。

♻️ 可选重构示例

+type RectifierBudgetDecision = "retry_same_provider" | "switch_provider";
+
+function decideRectifierBudgetAction(
+  attemptCount: number,
+  maxAttemptsPerProvider: number
+): RectifierBudgetDecision {
+  return attemptCount < maxAttemptsPerProvider ? "retry_same_provider" : "switch_provider";
+}

-const hasRetryBudget = attemptCount < maxAttemptsPerProvider;
-if (hasRetryBudget) {
-  // retry logs...
-  thinkingBudgetRectifierRetried = true;
-  continue;
-}
-lastRectifierSwitchError = lastError;
-failedProviderIds.push(currentProvider.id);
-break;
+const action = decideRectifierBudgetAction(attemptCount, maxAttemptsPerProvider);
+if (action === "retry_same_provider") {
+  // retry logs...
+  thinkingBudgetRectifierRetried = true;
+  continue;
+}
+lastRectifierSwitchError = lastError;
+failedProviderIds.push(currentProvider.id);
+break;

Also applies to: 1316-1393

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 1162 - 1236, The
retry-budget and switch logic for the rectifier blocks is duplicated; extract a
helper (e.g., handleRetryBudgetForRectifier) that accepts parameters
(currentProvider, attemptCount, maxAttemptsPerProvider, rectifierTrigger,
lastError, endpointAudit, requestDetailsBeforeRectify, getCircuitState, session)
and encapsulates: computing hasRetryBudget, emitting the two logger.info
variants, recording the provider via session.addProviderToChain (handling
ProxyError vs generic error shapes), and returning a control signal (retry |
switch) plus any metadata (e.g., lastRectifierSwitchError candidate,
failedProviderId). Replace the duplicated branches around
thinkingSignatureRectifierRetried/continue and
lastRectifierSwitchError/failedProviderIds with calls to this helper in both the
thinking signature and signature rectifier places to centralize behavior and
reduce drift.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 1162-1236: The retry-budget and switch logic for the rectifier
blocks is duplicated; extract a helper (e.g., handleRetryBudgetForRectifier)
that accepts parameters (currentProvider, attemptCount, maxAttemptsPerProvider,
rectifierTrigger, lastError, endpointAudit, requestDetailsBeforeRectify,
getCircuitState, session) and encapsulates: computing hasRetryBudget, emitting
the two logger.info variants, recording the provider via
session.addProviderToChain (handling ProxyError vs generic error shapes), and
returning a control signal (retry | switch) plus any metadata (e.g.,
lastRectifierSwitchError candidate, failedProviderId). Replace the duplicated
branches around thinkingSignatureRectifierRetried/continue and
lastRectifierSwitchError/failedProviderIds with calls to this helper in both the
thinking signature and signature rectifier places to centralize behavior and
reduce drift.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1b21a613-f5da-4ca5-bb03-65ab79e6a9c2

📥 Commits

Reviewing files that changed from the base of the PR and between 13810df and e59ff5a.

📒 Files selected for processing (2)

src/app/v1/_lib/proxy/forwarder.ts
src/app/v1/_lib/proxy/thinking-budget-rectifier.ts

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts`:
- Around line 17-56: The mock for getCachedSystemSettings is missing the
required SystemSettings field enableResponseInputRectifier; update the async
mock return object in getCachedSystemSettings to include
enableResponseInputRectifier: true (matching DEFAULT_SETTINGS) so the mocked
value conforms to the SystemSettings interface and mirrors DEFAULT_SETTINGS.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 55f4d5dd-79a1-454f-839a-937dc642a23a

📥 Commits

Reviewing files that changed from the base of the PR and between e59ff5a and 9fcc17a.

📒 Files selected for processing (2)

src/app/v1/_lib/proxy/forwarder.ts
tests/unit/proxy/proxy-forwarder-retry-limit.test.ts

greptile-apps · 2026-03-15T13:05:31Z

                  providerName: currentProvider.name,
                  trigger: rectifierTrigger,
                  attemptNumber: attemptCount,
-                  retryAttemptNumber: attemptCount + 1,
+                  retryAttemptNumber: hasRetryBudget ? attemptCount + 1 : attemptCount,


retryAttemptNumber is misleading when no retry is planned

When !hasRetryBudget, retryAttemptNumber is set to attemptCount — the same value as attemptNumber. This makes the audit record look as if a retry was attempted at the current attempt number, rather than clearly communicating "no retry occurred." A reader scanning the audit log has no way to distinguish "retry happened at attempt N" from "no retry, field is a fallback."

Consider using a sentinel value such as null (or omitting the field) when no retry will be performed:

Suggested change

providerName: currentProvider.name,

trigger: rectifierTrigger,

attemptNumber: attemptCount,

retryAttemptNumber: attemptCount + 1,

retryAttemptNumber: hasRetryBudget ? attemptCount + 1 : attemptCount,

attemptNumber: attemptCount,

retryAttemptNumber: hasRetryBudget ? attemptCount + 1 : null,

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 1112-1115 Comment: **`retryAttemptNumber` is misleading when no retry is planned** When `!hasRetryBudget`, `retryAttemptNumber` is set to `attemptCount` — the same value as `attemptNumber`. This makes the audit record look as if a retry was attempted at the current attempt number, rather than clearly communicating "no retry occurred." A reader scanning the audit log has no way to distinguish "retry happened at attempt N" from "no retry, field is a fallback." Consider using a sentinel value such as `null` (or omitting the field) when no retry will be performed: ```suggestion attemptNumber: attemptCount, retryAttemptNumber: hasRetryBudget ? attemptCount + 1 : null, ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T13:05:33Z

                  trigger: budgetRectifierTrigger,
                  attemptNumber: attemptCount,
-                  retryAttemptNumber: attemptCount + 1,
+                  retryAttemptNumber: hasRetryBudget ? attemptCount + 1 : attemptCount,


Same retryAttemptNumber ambiguity in budget rectifier audit

The thinking budget rectifier has the same semantic issue. When !hasRetryBudget, retryAttemptNumber: attemptCount is indistinguishable in the audit log from a retry that actually happened at attempt N. Using null (or omitting) when no retry is planned would make the audit semantics unambiguous.

Suggested change

trigger: budgetRectifierTrigger,

attemptNumber: attemptCount,

retryAttemptNumber: attemptCount + 1,

retryAttemptNumber: hasRetryBudget ? attemptCount + 1 : attemptCount,

attemptNumber: attemptCount,

retryAttemptNumber: hasRetryBudget ? attemptCount + 1 : null,

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 1268-1270 Comment: **Same `retryAttemptNumber` ambiguity in budget rectifier audit** The thinking budget rectifier has the same semantic issue. When `!hasRetryBudget`, `retryAttemptNumber: attemptCount` is indistinguishable in the audit log from a retry that actually happened at attempt N. Using `null` (or omitting) when no retry is planned would make the audit semantics unambiguous. ```suggestion attemptNumber: attemptCount, retryAttemptNumber: hasRetryBudget ? attemptCount + 1 : null, ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T13:05:35Z

      if (!alternativeProvider) {
+        if (lastRectifierSwitchError) {
+          await ProxyForwarder.clearSessionProviderBinding(session);
+          throw lastRectifierSwitchError;
+        }


lastRectifierSwitchError is reset before each alternative provider is tried, silently discarding the original 400 in chains of ≥ 3 providers

lastRectifierSwitchError is nulled out at line 548 (lastRectifierSwitchError = null) at the top of every outer-loop iteration. This means:

Provider A — rectifier fires, !hasRetryBudget → lastRectifierSwitchError = 400 error, break, select provider B.

Provider B outer iteration starts → lastRectifierSwitchError = null (reset).

Provider B fails for any non-rectifier reason → break, selectAlternative returns null.

lastRectifierSwitchError is null → falls through to buildAllProvidersUnavailableError() → 503 thrown, even though the root cause was a client 400.

For single-provider or two-provider setups this is fine (and is covered by the new tests). However, in deployments with ≥ 3 providers configured, the original 400 can be silently swallowed if an intermediate provider also exhausts its attempts.

It may be acceptable to preserve only the most-recent rectifier error (current behavior) rather than the earliest one, but the discarding is invisible and there is no test exercising this path. Adding a test (or a comment explaining the design choice) would prevent future regressions.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 1814-1818 Comment: **`lastRectifierSwitchError` is reset before each alternative provider is tried, silently discarding the original 400 in chains of ≥ 3 providers** `lastRectifierSwitchError` is nulled out at line 548 (`lastRectifierSwitchError = null`) at the top of every outer-loop iteration. This means: 1. Provider A — rectifier fires, `!hasRetryBudget` → `lastRectifierSwitchError = 400 error`, break, select provider B. 2. Provider B outer iteration starts → `lastRectifierSwitchError = null` (reset). 3. Provider B fails for any non-rectifier reason → break, `selectAlternative` returns `null`. 4. `lastRectifierSwitchError` is `null` → falls through to `buildAllProvidersUnavailableError()` → **503 thrown**, even though the root cause was a client 400. For single-provider or two-provider setups this is fine (and is covered by the new tests). However, in deployments with ≥ 3 providers configured, the original 400 can be silently swallowed if an intermediate provider also exhausts its attempts. It may be acceptable to preserve only the most-recent rectifier error (current behavior) rather than the earliest one, but the discarding is invisible and there is no test exercising this path. Adding a test (or a comment explaining the design choice) would prevent future regressions. How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T13:05:36Z

+      const sendPromise = ProxyForwarder.send(session);
+      void vi.runAllTimersAsync();
+      await expect(sendPromise).rejects.toBeInstanceOf(ProxyError);
+      await expect(sendPromise).rejects.toMatchObject({ statusCode: 400 });


void vi.runAllTimersAsync() is inconsistent with the success-path test pattern

The success-path tests (e.g., the "maxRetryAttempts=1 should switch provider" tests above) use await vi.runAllTimersAsync(), which guarantees all fake timers have advanced before the next assertion. Here the "no-alternative" tests use void (fire-and-forget) and immediately await expect(sendPromise).rejects....

This works in practice because the rejection does not depend on any timers, but it creates an inconsistency that may confuse future test authors. For uniformity and clarity, prefer:

const sendPromise = ProxyForwarder.send(session); await vi.runAllTimersAsync(); await expect(sendPromise).rejects.toBeInstanceOf(ProxyError); await expect(sendPromise).rejects.toMatchObject({ statusCode: 400 });

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/unit/proxy/proxy-forwarder-retry-limit.test.ts Line: 528-531 Comment: **`void vi.runAllTimersAsync()` is inconsistent with the success-path test pattern** The success-path tests (e.g., the "maxRetryAttempts=1 should switch provider" tests above) use `await vi.runAllTimersAsync()`, which guarantees all fake timers have advanced before the next assertion. Here the "no-alternative" tests use `void` (fire-and-forget) and immediately `await expect(sendPromise).rejects...`. This works in practice because the rejection does not depend on any timers, but it creates an inconsistency that may confuse future test authors. For uniformity and clarity, prefer: ```typescript const sendPromise = ProxyForwarder.send(session); await vi.runAllTimersAsync(); await expect(sendPromise).rejects.toBeInstanceOf(ProxyError); await expect(sendPromise).rejects.toMatchObject({ statusCode: 400 }); ``` How can I resolve this? If you propose a fix, please make it concise.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

coderabbitai

🧹 Nitpick comments (3)

tests/unit/proxy/proxy-forwarder-retry-limit.test.ts (3)

529-534: 建议使用 await 替代 void 以避免潜在的竞态条件

第 530 行使用 void vi.runAllTimersAsync() 而非 await vi.runAllTimersAsync()。由于 runAllTimersAsync 返回 Promise，使用 void 忽略返回值可能导致定时器未完全推进就开始执行断言，从而产生不稳定的测试结果。

文件中其他测试（如第 386、463 行）都正确使用了 await vi.runAllTimersAsync()。

♻️ 建议修复

       const sendPromise = ProxyForwarder.send(session);
-      void vi.runAllTimersAsync();
+      await vi.runAllTimersAsync();
       await expect(sendPromise).rejects.toBeInstanceOf(ProxyError);
       await expect(sendPromise).rejects.toMatchObject({ statusCode: 400 });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts` around lines 529 - 534,
The test uses "void vi.runAllTimersAsync()" after calling
ProxyForwarder.send(session), which discards the promise and can produce race
conditions; replace the void usage with "await vi.runAllTimersAsync()" so timers
are fully advanced before asserting on sendPromise (references:
ProxyForwarder.send and vi.runAllTimersAsync in the failing test block).

574-579: 同上，建议使用 await 替代 void

第 575 行存在与第 530 行相同的问题。建议统一使用 await vi.runAllTimersAsync() 以确保测试稳定性。

♻️ 建议修复

       const sendPromise = ProxyForwarder.send(session);
-      void vi.runAllTimersAsync();
+      await vi.runAllTimersAsync();
       await expect(sendPromise).rejects.toBeInstanceOf(ProxyError);
       await expect(sendPromise).rejects.toMatchObject({ statusCode: 400 });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts` around lines 574 - 579,
The test uses "void vi.runAllTimersAsync()" which can hide promise rejections
and cause flakiness; replace it with "await vi.runAllTimersAsync()" in the test
that calls ProxyForwarder.send(session) so the timers finish before assertions.
Update the invocation at the block containing ProxyForwarder.send(session) (and
the related assertions on ProxyError, statusCode, doForward and
selectAlternative) to await vi.runAllTimersAsync() to ensure the promise settles
before the expects.

844-865: 冗余的 mockImplementation 可能导致理解困惑

第 844-846 行的 mockImplementation 设置了默认行为，但由于后续添加了恰好 5 个排队的实现（4 个 mockImplementationOnce + 1 个 mockResolvedValueOnce），且测试预期正好 5 次调用，该默认实现永远不会被执行。

建议移除该冗余设置以提高代码可读性。

♻️ 建议修复

-      // All attempts fail except the last one
-      doForward.mockImplementation(async () => {
-        throw networkError;
-      });
-      // 5th attempt succeeds
-      doForward.mockImplementationOnce(async () => {
+      // First 4 attempts fail, 5th succeeds
+      doForward.mockImplementationOnce(async () => {
         throw networkError;
       });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts` around lines 844 - 865,
删除冗余的默认 mock 行：移除对 doForward.mockImplementation(async () => { throw
networkError; }); 的调用，只保留后续用到的 doForward.mockImplementationOnce(...) 和
doForward.mockResolvedValueOnce(...) 排队实现，以确保测试使用正好 5
次排队的返回行为（函数名：doForward，方法：mockImplementation、mockImplementationOnce、mockResolvedValueOnce）。

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts`:
- Around line 529-534: The test uses "void vi.runAllTimersAsync()" after calling
ProxyForwarder.send(session), which discards the promise and can produce race
conditions; replace the void usage with "await vi.runAllTimersAsync()" so timers
are fully advanced before asserting on sendPromise (references:
ProxyForwarder.send and vi.runAllTimersAsync in the failing test block).
- Around line 574-579: The test uses "void vi.runAllTimersAsync()" which can
hide promise rejections and cause flakiness; replace it with "await
vi.runAllTimersAsync()" in the test that calls ProxyForwarder.send(session) so
the timers finish before assertions. Update the invocation at the block
containing ProxyForwarder.send(session) (and the related assertions on
ProxyError, statusCode, doForward and selectAlternative) to await
vi.runAllTimersAsync() to ensure the promise settles before the expects.
- Around line 844-865: 删除冗余的默认 mock 行：移除对 doForward.mockImplementation(async ()
=> { throw networkError; }); 的调用，只保留后续用到的 doForward.mockImplementationOnce(...)
和 doForward.mockResolvedValueOnce(...) 排队实现，以确保测试使用正好 5
次排队的返回行为（函数名：doForward，方法：mockImplementation、mockImplementationOnce、mockResolvedValueOnce）。

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c493e1f0-bd20-4315-bf3b-f5b9805ab337

📥 Commits

Reviewing files that changed from the base of the PR and between 9fcc17a and 02d2a40.

📒 Files selected for processing (1)

tests/unit/proxy/proxy-forwarder-retry-limit.test.ts

greptile-apps · 2026-03-15T13:44:40Z

@@ -1207,15 +1223,23 @@ export class ProxyForwarder {
                    });
                  }

-                  // 确保即使 maxAttemptsPerProvider=1 也能完成一次额外重试
-                  maxAttemptsPerProvider = Math.max(maxAttemptsPerProvider, attemptCount + 1);
-                  continue;
+                  // 尊重“单供应商最大尝试次数”：只在还有预算时才对同一供应商重试；否则直接进入供应商切换逻辑。
+                  if (hasRetryBudget) {
+                    thinkingSignatureRectifierRetried = true;
+                    continue;
+                  }
+
+                  // 记录原始错误：若无可用替代供应商，回退抛出该错误而非 503，避免掩盖客户端输入问题。
+                  lastRectifierSwitchError = lastError;
+                  failedProviderIds.push(currentProvider.id);
+                  break;


reason: "retry_failed" is incorrect when no retry is performed

The addProviderToChain call at this block unconditionally uses reason: "retry_failed" for the audit record, but this block is shared between two semantically different outcomes:

hasRetryBudget == true → a retry actually happens (continue) → "retry_failed" is accurate

hasRetryBudget == false → no retry happens, we immediately break to switch providers → "retry_failed" is misleading

When the !hasRetryBudget path is taken, the audit chain records "retry_failed" even though only a single attempt was made before the provider switch. This creates incorrect audit data: an operator analyzing the chain will see "retry_failed" implying two attempts were made, when in fact only one occurred.

The same issue exists in the budget rectifier block at ~line 1342–1380.

A dedicated reason — e.g. "rectifier_triggered_failover" — should be used when !hasRetryBudget, to accurately reflect that rectification was applied but budget was exhausted, triggering a provider switch rather than a retry:

// After audit/chain recording, before the hasRetryBudget branch if (hasRetryBudget) { // reason "retry_failed" is correct: we recorded the failure and are about to retry thinkingSignatureRectifierRetried = true; continue; } // Change the already-recorded chain entry's reason, or record a separate entry // with a distinct reason like "rectifier_triggered_failover" lastRectifierSwitchError = lastError; failedProviderIds.push(currentProvider.id); break;

The same fix should be applied to the budget rectifier at ~line 1342.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 1185-1235 Comment: **`reason: "retry_failed"` is incorrect when no retry is performed** The `addProviderToChain` call at this block unconditionally uses `reason: "retry_failed"` for the audit record, but this block is shared between two semantically different outcomes: 1. `hasRetryBudget == true` → a retry actually happens (`continue`) → `"retry_failed"` is accurate 2. `hasRetryBudget == false` → **no retry happens**, we immediately break to switch providers → `"retry_failed"` is misleading When the `!hasRetryBudget` path is taken, the audit chain records `"retry_failed"` even though only a single attempt was made before the provider switch. This creates incorrect audit data: an operator analyzing the chain will see `"retry_failed"` implying two attempts were made, when in fact only one occurred. The same issue exists in the budget rectifier block at ~line 1342–1380. A dedicated reason — e.g. `"rectifier_triggered_failover"` — should be used when `!hasRetryBudget`, to accurately reflect that rectification was applied but budget was exhausted, triggering a provider switch rather than a retry: ```typescript // After audit/chain recording, before the hasRetryBudget branch if (hasRetryBudget) { // reason "retry_failed" is correct: we recorded the failure and are about to retry thinkingSignatureRectifierRetried = true; continue; } // Change the already-recorded chain entry's reason, or record a separate entry // with a distinct reason like "rectifier_triggered_failover" lastRectifierSwitchError = lastError; failedProviderIds.push(currentProvider.id); break; ``` The same fix should be applied to the budget rectifier at ~line 1342. How can I resolve this? If you propose a fix, please make it concise.

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Line 1115: The code currently always sets retryAttemptNumber (in the object
created near retryAttemptNumber: hasRetryBudget ? attemptCount + 1 :
attemptCount) even when hasRetryBudget is false, which incorrectly records a
non-existent same-provider retry; change the logic so that when hasRetryBudget
=== false the retryAttemptNumber property is omitted (or set to undefined)
instead of being populated with attemptCount, and apply the same fix to the
other occurrence that mirrors this logic (the second similar assignment around
retryAttemptNumber). Locate the object construction using retryAttemptNumber,
the variables hasRetryBudget and attemptCount, and update it to only include
retryAttemptNumber when hasRetryBudget is true (or explicitly record a
provider-switch field if you need to represent failover separately).
- Around line 1075-1082: The rectifier (thinking-signature/thinking-budget) and
failover decision is only applied in send()'s retry loop, so streaming requests
via sendStreamingWithHedge() (which uses handleAttemptFailure() and
categorizeErrorAsync()) skip it; move the rectifier/failover logic into the
shared failure-handling path used by both hedge and non-hedge flows (e.g., unify
into handleAttemptFailure() or a new helper called from both send() and
sendStreamingWithHedge()), ensure the logic checks isAnthropicProvider and
respects maxRetryAttempts there, and add an explicit branch to handle stream:
true so streaming Claude calls get the same rectifier + failover behavior as
non-streaming calls.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b756f5b2-7d23-4b11-814d-9220c98c96b2

📥 Commits

Reviewing files that changed from the base of the PR and between 02d2a40 and 4149ed5.

📒 Files selected for processing (2)

src/app/v1/_lib/proxy/forwarder.ts
tests/unit/proxy/proxy-forwarder-retry-limit.test.ts

greptile-apps · 2026-03-15T14:40:03Z

  recordFailure: mocks.recordFailure,
 }));

+vi.mock("@/lib/config", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("@/lib/config")>();
+  return {
+    ...actual,
+    getCachedSystemSettings: mocks.getCachedSystemSettings,
+    isHttp2Enabled: mocks.isHttp2Enabled,
+  };


vi.clearAllMocks() does not reset mock implementations — stale return values may leak between tests

vi.clearAllMocks() (called in each beforeEach) only clears recorded calls, instances, and results. It does not reset implementations set via .mockResolvedValue() or .mockImplementation(). Because vi.spyOn(..., "selectAlternative") is created freshly inside each test body this is benign today — the fresh spyOn replaces the previous spy on the class and has no default implementation. However, doForward spies created inside each test that use .mockImplementationOnce() also benefit from fresh spies, so there's no bleed-over.

The concern is that a future test author might introduce a spy at describe-block scope (rather than inside the test body) and then rely on vi.clearAllMocks() to reset its return value, which would not work. Consider adding a comment or using vi.resetAllMocks() instead of vi.clearAllMocks() in the beforeEach of this new describe block to make the intent explicit.

Suggested change

recordFailure: mocks.recordFailure,

}));

vi.mock("@/lib/config", async (importOriginal) => {

const actual = await importOriginal<typeof import("@/lib/config")>();

return {

...actual,

getCachedSystemSettings: mocks.getCachedSystemSettings,

isHttp2Enabled: mocks.isHttp2Enabled,

};

beforeEach(() => {

vi.resetAllMocks();

});

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/unit/proxy/proxy-forwarder-retry-limit.test.ts Line: 90-99 Comment: **`vi.clearAllMocks()` does not reset mock implementations — stale return values may leak between tests** `vi.clearAllMocks()` (called in each `beforeEach`) only clears recorded calls, instances, and results. It does **not** reset implementations set via `.mockResolvedValue()` or `.mockImplementation()`. Because `vi.spyOn(..., "selectAlternative")` is created freshly inside each test body this is benign today — the fresh `spyOn` replaces the previous spy on the class and has no default implementation. However, `doForward` spies created inside each test that use `.mockImplementationOnce()` also benefit from fresh spies, so there's no bleed-over. The concern is that a future test author might introduce a spy at describe-block scope (rather than inside the test body) and then rely on `vi.clearAllMocks()` to reset its return value, which would not work. Consider adding a comment or using `vi.resetAllMocks()` instead of `vi.clearAllMocks()` in the `beforeEach` of this new describe block to make the intent explicit. ```suggestion beforeEach(() => { vi.resetAllMocks(); }); ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T14:40:04Z

+                  // 尊重“单供应商最大尝试次数”：只在还有预算时才对同一供应商重试；否则直接进入供应商切换逻辑。
+                  if (hasRetryBudget) {
+                    thinkingSignatureRectifierRetried = true;
+                    continue;
+                  }
+
+                  // 记录原始错误：若无可用替代供应商，回退抛出该错误而非 503，避免掩盖客户端输入问题。
+                  lastRectifierSwitchError = lastError;
+                  failedProviderIds.push(currentProvider.id);
+                  break;


Multi-endpoint provider skips remaining endpoints on rectifier failover

When !hasRetryBudget causes the inner loop to break, only the first endpoint of currentProvider has been tried. For providers configured with multiple endpoints (e.g. a vendor with a latency-sorted endpoint pool), the remaining endpoints for that provider are silently skipped.

This is appropriate in this case because the 400 error is a client-input issue (invalid thinking signature/budget) — trying a different endpoint on the same provider would not help. A brief comment here (and in the analogous budget-rectifier block at ~line 1382) would make this intent explicit for future readers, since elsewhere in the file "break" from the inner loop implies "all endpoints for this provider have been exhausted".

Suggested change

// 尊重“单供应商最大尝试次数”：只在还有预算时才对同一供应商重试；否则直接进入供应商切换逻辑。

if (hasRetryBudget) {

thinkingSignatureRectifierRetried = true;

continue;

}

// 记录原始错误：若无可用替代供应商，回退抛出该错误而非 503，避免掩盖客户端输入问题。

lastRectifierSwitchError = lastError;

failedProviderIds.push(currentProvider.id);

break;

// NOTE: We intentionally skip remaining endpoints for this provider (if any).

// The 400 error is a client-input issue that would reproduce on every endpoint,

// so endpoint-level failover would not help.

lastRectifierSwitchError = lastError;

failedProviderIds.push(currentProvider.id);

break;

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 1226-1235 Comment: **Multi-endpoint provider skips remaining endpoints on rectifier failover** When `!hasRetryBudget` causes the inner loop to `break`, only the **first** endpoint of `currentProvider` has been tried. For providers configured with multiple endpoints (e.g. a vendor with a latency-sorted endpoint pool), the remaining endpoints for that provider are silently skipped. This is appropriate in this case because the 400 error is a client-input issue (invalid thinking signature/budget) — trying a different endpoint on the same provider would not help. A brief comment here (and in the analogous budget-rectifier block at ~line 1382) would make this intent explicit for future readers, since elsewhere in the file "break" from the inner loop implies "all endpoints for this provider have been exhausted". ```suggestion // NOTE: We intentionally skip remaining endpoints for this provider (if any). // The 400 error is a client-input issue that would reproduce on every endpoint, // so endpoint-level failover would not help. lastRectifierSwitchError = lastError; failedProviderIds.push(currentProvider.id); break; ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T14:58:36Z

+      doForward.mockImplementationOnce(async () => {
+        throw new ProxyError("invalid signature in thinking block", 400);
+      });
+      doForward.mockResolvedValueOnce(


mockResolvedValue (persistent) instead of mockResolvedValueOnce

selectAlternative.mockResolvedValue(providerB) is a permanent/sticky mock — every call to selectAlternative will return providerB for the lifetime of the test and potentially beyond (until the spy is replaced or cleared). For a test that expects selectAlternative to be called exactly once (expect(selectAlternative).toHaveBeenCalledTimes(1)), using mockResolvedValueOnce would be more precise and defensive. If selectAlternative is called unexpectedly a second time (e.g. due to a logic error), the sticky mock silently returns providerB and the test may still pass, masking the bug.

The same applies to the analogous budget rectifier test at line ~448.

Suggested change

doForward.mockResolvedValueOnce(

selectAlternative.mockResolvedValueOnce(providerB);

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/unit/proxy/proxy-forwarder-retry-limit.test.ts Line: 374 Comment: **`mockResolvedValue` (persistent) instead of `mockResolvedValueOnce`** `selectAlternative.mockResolvedValue(providerB)` is a permanent/sticky mock — every call to `selectAlternative` will return `providerB` for the lifetime of the test and potentially beyond (until the spy is replaced or cleared). For a test that expects `selectAlternative` to be called exactly once (`expect(selectAlternative).toHaveBeenCalledTimes(1)`), using `mockResolvedValueOnce` would be more precise and defensive. If `selectAlternative` is called unexpectedly a second time (e.g. due to a logic error), the sticky mock silently returns `providerB` and the test may still pass, masking the bug. The same applies to the analogous budget rectifier test at line ~448. ```suggestion selectAlternative.mockResolvedValueOnce(providerB); ``` How can I resolve this? If you propose a fix, please make it concise.

tesgth032 · 2026-03-15T15:48:33Z

已按 CodeRabbit 的 CHANGES_REQUESTED 修复并补测：

修正审计字段：当已达到 maxRetryAttempts 而直接进入供应商切换时，specialSettings 不再写入误导性的 retryAttemptNumber；并将其调整为可选字段，buildSettingKey 做兼容。
补齐 streaming hedge 路径 rectifier：stream=true 且启用 first-byte hedge 时，thinking signature / thinking budget rectifier 也会生效，并严格遵守 maxRetryAttempts（有预算则同供应商重试；无预算则进入 provider failover）。

同时补齐/加固单测：streaming hedge 场景 + maxRetryAttempts=2 连续失败边界；并将 selectAlternative mock 收敛为 mockResolvedValueOnce。

@coderabbitai review

coderabbitai · 2026-03-15T15:48:53Z

@tesgth032 感谢详细说明修复内容！我将立即对最新变更进行全面审查，重点关注：

审计字段修正（retryAttemptNumber 可选化及 buildSettingKey 兼容性）
Streaming hedge 路径的 rectifier 整合及 maxRetryAttempts 遵守情况
新增单元测试的覆盖完整性

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

🧹 Nitpick comments (3)

src/app/v1/_lib/proxy/forwarder.ts (1)
1124-1151: 建议收敛 special settings 持久化逻辑，减少分支漂移。

同一段持久化代码在同步与 streaming hedge 的 signature/budget 两条路径重复出现，后续改动容易漏改。建议统一复用 persistSpecialSettings(...)（在 hedge 分支传入主 session），降低维护风险。

Also applies to: 1280-1307, 3215-3242, 3334-3361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 1124 - 1151, Duplicate
persistence logic for specialSettings appears in multiple paths (including the
shown block using SessionManager.storeSessionSpecialSettings and
updateMessageRequestDetails); extract and consolidate this into a single helper
like persistSpecialSettings(sessionOrMainSession, messageRequestId?) and call it
from all branches (for hedge streaming pass the main session as the first
argument and provide messageRequestId when available) so both session
persistence (SessionManager.storeSessionSpecialSettings) and message request
persistence (updateMessageRequestDetails) are centralized; update all usages
(including the other occurrences noted) to call the new persistSpecialSettings
helper and remove the duplicated try/catch blocks.
tests/unit/proxy/proxy-forwarder-retry-limit.test.ts (2)
126-129: 建议降低测试对生产内部常量导出的耦合。

这里直接依赖 MAX_THINKING_BUDGET / MAX_TOKENS_VALUE 会把测试需求反向施加到生产导出面。若这两个常量并非明确公共 API，建议改为通过行为断言或测试侧 fixture 常量来验证。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts` around lines 126 - 129,
The test currently imports MAX_THINKING_BUDGET and MAX_TOKENS_VALUE from
production code; instead remove that import and decouple the test by either
using local fixture constants (define test-side constants like
expectedThinkingBudget / expectedMaxTokens) or by asserting behavior (e.g.,
assert that the forwarder enforces the retry/token limits via observable
outcomes rather than mirroring exported values). Update the test that references
MAX_THINKING_BUDGET and MAX_TOKENS_VALUE to use these test fixtures or behavior
assertions so the test no longer depends on those production exports.
328-407: 建议补充 no-budget 路径的审计字段断言。

当前用例已覆盖“重试/切换”行为，但还缺少对审计语义的直接保护：在 maxRetryAttempts=1 的切换路径中，建议断言 retryAttemptNumber 不存在（或为 undefined），避免未来回归到“记录了不存在的同供应商重试”。

Also applies to: 409-485, 487-583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts` around lines 328 - 407,
在 maxRetryAttempts=1 的切换路径测试 ("thinking signature rectifier: maxRetryAttempts=1
should switch provider...") 中补充对审计字段的断言：在调用 ProxyForwarder.send(session)
后，检查产生的审计条目（inspect session 的审计存储点，例如 session.auditEntries /
session.request.audit... 或项目中实际存放审计记录的集合）并断言该“no-budget 切换”条目上不包含
retryAttemptNumber（即为 undefined 或不存在）；在同样的其它两个测试（行 409-485, 487-583
区域）也加上相同断言以防回归，并将断言位置与现有对 provider chain / doForward 调用次数的断言一起放置以便可读性（参照
symbols: ProxyForwarder.send, session, doForward, selectAlternative）。

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 1124-1151: Duplicate persistence logic for specialSettings appears
in multiple paths (including the shown block using
SessionManager.storeSessionSpecialSettings and updateMessageRequestDetails);
extract and consolidate this into a single helper like
persistSpecialSettings(sessionOrMainSession, messageRequestId?) and call it from
all branches (for hedge streaming pass the main session as the first argument
and provide messageRequestId when available) so both session persistence
(SessionManager.storeSessionSpecialSettings) and message request persistence
(updateMessageRequestDetails) are centralized; update all usages (including the
other occurrences noted) to call the new persistSpecialSettings helper and
remove the duplicated try/catch blocks.

In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts`:
- Around line 126-129: The test currently imports MAX_THINKING_BUDGET and
MAX_TOKENS_VALUE from production code; instead remove that import and decouple
the test by either using local fixture constants (define test-side constants
like expectedThinkingBudget / expectedMaxTokens) or by asserting behavior (e.g.,
assert that the forwarder enforces the retry/token limits via observable
outcomes rather than mirroring exported values). Update the test that references
MAX_THINKING_BUDGET and MAX_TOKENS_VALUE to use these test fixtures or behavior
assertions so the test no longer depends on those production exports.
- Around line 328-407: 在 maxRetryAttempts=1 的切换路径测试 ("thinking signature
rectifier: maxRetryAttempts=1 should switch provider...") 中补充对审计字段的断言：在调用
ProxyForwarder.send(session) 后，检查产生的审计条目（inspect session 的审计存储点，例如
session.auditEntries / session.request.audit... 或项目中实际存放审计记录的集合）并断言该“no-budget
切换”条目上不包含 retryAttemptNumber（即为 undefined 或不存在）；在同样的其它两个测试（行 409-485, 487-583
区域）也加上相同断言以防回归，并将断言位置与现有对 provider chain / doForward 调用次数的断言一起放置以便可读性（参照
symbols: ProxyForwarder.send, session, doForward, selectAlternative）。

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 395e3013-daf3-47d2-83ff-ea13300cd6bd

📥 Commits

Reviewing files that changed from the base of the PR and between 02d2a40 and deda790.

📒 Files selected for processing (4)

src/app/v1/_lib/proxy/forwarder.ts
src/lib/utils/special-settings.ts
src/types/special-settings.ts
tests/unit/proxy/proxy-forwarder-retry-limit.test.ts

greptile-apps · 2026-03-15T15:55:50Z

+describe("ProxyForwarder - maxRetryAttempts should not be bypassed by thinking rectifiers", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
+  test("thinking signature rectifier: maxRetryAttempts=1 should switch provider (not retry same provider)", async () => {
+    vi.useFakeTimers();
+


No test coverage for the new streamingHedge rectifier logic

The new describe block "ProxyForwarder - maxRetryAttempts should not be bypassed by thinking rectifiers" exclusively calls ProxyForwarder.send(). However, this PR also introduces ~250 lines of new rectifier handling inside streamingHedge (lines ~3178–3415 of forwarder.ts) that is completely untested.

The streaming-hedge path differs significantly from the send() path:

It uses signatureRectifierRetriedProviderIds / budgetRectifierRetriedProviderIds Sets (vs the boolean flags in send())

It calls launchAlternative() + finishIfExhausted() for the !hasRetryBudget case instead of the lastRectifierSwitchError mechanism

It relies on attempt.providerAttemptNumber tracked through providerAttemptCounts

Consider adding parallel test cases that exercise streamingHedge for at least:

maxRetryAttempts=1 → should switch provider, not retry same

maxRetryAttempts=1 + no alternative → should settle with original 400 (via resolveHedgeTerminalError)

maxRetryAttempts=2 → should retry same provider

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/unit/proxy/proxy-forwarder-retry-limit.test.ts Line: 323-330 Comment: **No test coverage for the new `streamingHedge` rectifier logic** The new describe block `"ProxyForwarder - maxRetryAttempts should not be bypassed by thinking rectifiers"` exclusively calls `ProxyForwarder.send()`. However, this PR also introduces ~250 lines of new rectifier handling inside `streamingHedge` (lines ~3178–3415 of `forwarder.ts`) that is **completely untested**. The streaming-hedge path differs significantly from the `send()` path: - It uses `signatureRectifierRetriedProviderIds` / `budgetRectifierRetriedProviderIds` Sets (vs the `boolean` flags in `send()`) - It calls `launchAlternative()` + `finishIfExhausted()` for the `!hasRetryBudget` case instead of the `lastRectifierSwitchError` mechanism - It relies on `attempt.providerAttemptNumber` tracked through `providerAttemptCounts` Consider adding parallel test cases that exercise `streamingHedge` for at least: - `maxRetryAttempts=1` → should switch provider, not retry same - `maxRetryAttempts=1` + no alternative → should settle with original 400 (via `resolveHedgeTerminalError`) - `maxRetryAttempts=2` → should retry same provider How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T15:55:51Z

+        // Align streaming-hedge behavior with send(): rectify specific Anthropic 400s and then retry/failover,
+        // while strictly respecting provider-level maxRetryAttempts.
+        const signatureTrigger = detectThinkingSignatureRectifierTrigger(errorMessage);
+        if (signatureTrigger) {
+          const settings = await getCachedSystemSettings();
+          const enabled = settings.enableThinkingSignatureRectifier ?? true;
+
+          if (enabled) {
+            // This is a client input / request-shape issue, and must not be counted into circuit breakers.
+            // Also, keep NON_RETRYABLE_CLIENT_ERROR so that if no alternative provider exists,
+            // resolveHedgeTerminalError() returns the original error instead of a generic 503.
+            errorCategory = ErrorCategory.NON_RETRYABLE_CLIENT_ERROR;
+            lastErrorCategory = errorCategory;
+
+            if (!signatureRectifierRetriedProviderIds.has(attempt.provider.id)) {
+              const requestDetailsBeforeRectify = buildRequestDetails(session);
+              const rectified = rectifyAnthropicRequestMessage(
+                session.request.message as Record<string, unknown>
+              );
+              const hasRetryBudget = attempt.providerAttemptNumber < attempt.maxAttemptsPerProvider;
+
+              session.addSpecialSetting({
+                type: "thinking_signature_rectifier",
+                scope: "request",
+                hit: rectified.applied,
+                providerId: attempt.provider.id,
+                providerName: attempt.provider.name,
+                trigger: signatureTrigger,
+                attemptNumber: attempt.providerAttemptNumber,
+                ...(hasRetryBudget
+                  ? { retryAttemptNumber: attempt.providerAttemptNumber + 1 }
+                  : {}),
+                removedThinkingBlocks: rectified.removedThinkingBlocks,
+                removedRedactedThinkingBlocks: rectified.removedRedactedThinkingBlocks,
+                removedSignatureFields: rectified.removedSignatureFields,
+              });
+
+              const specialSettings = session.getSpecialSettings();
+              if (specialSettings && session.sessionId) {
+                try {
+                  await SessionManager.storeSessionSpecialSettings(
+                    session.sessionId,
+                    specialSettings,


~200-line duplication between signature and budget rectifier blocks in streamingHedge

The signature rectifier block (lines ~3178–3300) and the budget rectifier block (lines ~3305–3415) are structurally nearly identical — both:

Check isAnthropicProvider

Detect trigger

Fetch and check settings enable flag

Set errorCategory = NON_RETRYABLE_CLIENT_ERROR

Guard with !<rectifierRetriedProviderIds>.has(attempt.provider.id)

Rectify the message in-place

Compute hasRetryBudget

Add special setting, persist to session/DB

Conditionally add to provider chain with reason: "retry_failed"

Branch on hasRetryBudget: retry vs. launchAlternative + finishIfExhausted

The only differences are the detector function, the rectifier function, the special-setting type, and the set tracking variable. This duplication is ~200 lines and carries a high maintenance risk — a future fix to one block (e.g. correcting the reason field, updating persistence logic) is easy to miss in the other.

Consider extracting a shared helper:

async function handleRectifier( attempt: StreamingHedgeAttempt, trigger: RectifierTrigger, rectify: (msg: Record<string, unknown>) => RectifiedResult, retriedSet: Set<number>, settingType: "thinking_signature_rectifier" | "thinking_budget_rectifier" ): Promise<boolean /* handled */> { ... }

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 3178-3220 Comment: **~200-line duplication between signature and budget rectifier blocks in `streamingHedge`** The signature rectifier block (lines ~3178–3300) and the budget rectifier block (lines ~3305–3415) are structurally nearly identical — both: 1. Check `isAnthropicProvider` 2. Detect trigger 3. Fetch and check settings enable flag 4. Set `errorCategory = NON_RETRYABLE_CLIENT_ERROR` 5. Guard with `!<rectifierRetriedProviderIds>.has(attempt.provider.id)` 6. Rectify the message in-place 7. Compute `hasRetryBudget` 8. Add special setting, persist to session/DB 9. Conditionally add to provider chain with `reason: "retry_failed"` 10. Branch on `hasRetryBudget`: retry vs. `launchAlternative` + `finishIfExhausted` The only differences are the detector function, the rectifier function, the special-setting type, and the set tracking variable. This duplication is ~200 lines and carries a high maintenance risk — a future fix to one block (e.g. correcting the `reason` field, updating persistence logic) is easy to miss in the other. Consider extracting a shared helper: ```typescript async function handleRectifier( attempt: StreamingHedgeAttempt, trigger: RectifierTrigger, rectify: (msg: Record<string, unknown>) => RectifiedResult, retriedSet: Set<number>, settingType: "thinking_signature_rectifier" | "thinking_budget_rectifier" ): Promise<boolean /* handled */> { ... } ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T16:14:43Z

        return;
      }

+      const isAnthropicProvider =
+        attempt.provider.providerType === "claude" ||
+        attempt.provider.providerType === "claude-auth";
+      if (isAnthropicProvider) {
+        // Align streaming-hedge behavior with send(): rectify specific Anthropic 400s and then retry/failover,


Rectifier failover labels alternative provider success as hedge_winner

When a rectifier's !hasRetryBudget path calls launchAlternative(), the alternative provider is added to startedProviderIds. When that alternative succeeds in settleSuccess, uniqueProvidersAttempted = startedProviderIds.size = 2, so isActualHedgeWin = true and reason = "hedge_winner" is written to the audit chain.

However, this is a rectifier-triggered failover — provider A was exhausted due to a client input error (400), not a timed race. Labeling the result "hedge_winner" implies a simultaneous hedge race occurred, which can mislead operators scanning audit logs. In a real simultaneous hedge (e.g., latency racing), both providers would have been launched at the start; here B is launched only after A's budget is exhausted.

Consider extending the reason enum with a dedicated value (e.g., "rectifier_failover_success") to distinguish this scenario, or at minimum document this semantic overlap in a comment adjacent to the isActualHedgeWin check:

// NOTE: isActualHedgeWin is also true for rectifier-triggered failovers where // launchAlternative() was called from the !hasRetryBudget path. This is by design: // the semantics of "multiple providers participated" is preserved, even though // no simultaneous hedge race occurred. const isActualHedgeWin = uniqueProvidersAttempted > 1;

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 3171-3178 Comment: **Rectifier failover labels alternative provider success as `hedge_winner`** When a rectifier's `!hasRetryBudget` path calls `launchAlternative()`, the alternative provider is added to `startedProviderIds`. When that alternative succeeds in `settleSuccess`, `uniqueProvidersAttempted = startedProviderIds.size = 2`, so `isActualHedgeWin = true` and `reason = "hedge_winner"` is written to the audit chain. However, this is a **rectifier-triggered failover** — provider A was exhausted due to a client input error (400), not a timed race. Labeling the result `"hedge_winner"` implies a simultaneous hedge race occurred, which can mislead operators scanning audit logs. In a real simultaneous hedge (e.g., latency racing), both providers would have been launched at the start; here B is launched only after A's budget is exhausted. Consider extending the `reason` enum with a dedicated value (e.g., `"rectifier_failover_success"`) to distinguish this scenario, or at minimum document this semantic overlap in a comment adjacent to the `isActualHedgeWin` check: ```typescript // NOTE: isActualHedgeWin is also true for rectifier-triggered failovers where // launchAlternative() was called from the !hasRetryBudget path. This is by design: // the semantics of "multiple providers participated" is preserved, even though // no simultaneous hedge race occurred. const isActualHedgeWin = uniqueProvidersAttempted > 1; ``` How can I resolve this? If you propose a fix, please make it concise.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

greptile-apps · 2026-03-15T17:05:08Z

+      const uniqueProvidersAttempted = startedProviderIds.size;
+      const didInitialProviderStart = startedProviderIds.has(initialProvider.id);
+      const isHedgeWin = hedgeTriggered && uniqueProvidersAttempted > 1;
+      const isRetrySuccess =
+        !isHedgeWin && (uniqueProvidersAttempted > 1 || attempt.providerAttemptNumber > 1);


totalProvidersAttempted semantic change breaks audit log accuracy for retries

uniqueProvidersAttempted uses startedProviderIds.size, which only counts unique providers. Before this PR, launchedProviderCount counted every individual attempt (including same-provider rectifier retries). Now, if provider A is tried twice (initial + rectifier retry) and then provider B succeeds, totalProvidersAttempted reports 2 (A and B) instead of 3 (3 total launches).

This affects both the success audit log at the bottom of settleSuccess and any downstream tooling that uses totalProvidersAttempted to infer how many requests were made upstream. The field's name implies a count of providers, but its historical meaning was "total attempts". Since the PR adds same-provider rectifier retries to streamingHedge for the first time, this silent semantic change could confuse operators comparing old and new logs.

Consider renaming the field in the log/audit to uniqueProvidersAttempted or adding a separate totalAttemptCount: launchedProviderCount field to keep both pieces of information:

const uniqueProvidersAttempted = startedProviderIds.size; // ... logger.info("...", { // ... totalProvidersAttempted: uniqueProvidersAttempted, // unique providers totalAttemptCount: launchedProviderCount, // total requests fired });

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 3361-3365 Comment: **`totalProvidersAttempted` semantic change breaks audit log accuracy for retries** `uniqueProvidersAttempted` uses `startedProviderIds.size`, which only counts *unique* providers. Before this PR, `launchedProviderCount` counted every individual attempt (including same-provider rectifier retries). Now, if provider A is tried twice (initial + rectifier retry) and then provider B succeeds, `totalProvidersAttempted` reports `2` (A and B) instead of `3` (3 total launches). This affects both the success audit log at the bottom of `settleSuccess` and any downstream tooling that uses `totalProvidersAttempted` to infer how many requests were made upstream. The field's name implies a count of providers, but its historical meaning was "total attempts". Since the PR adds same-provider rectifier retries to `streamingHedge` for the first time, this silent semantic change could confuse operators comparing old and new logs. Consider renaming the field in the log/audit to `uniqueProvidersAttempted` or adding a separate `totalAttemptCount: launchedProviderCount` field to keep both pieces of information:</p> ```typescript const uniqueProvidersAttempted = startedProviderIds.size; // ... logger.info("...", { // ... totalProvidersAttempted: uniqueProvidersAttempted, // unique providers totalAttemptCount: launchedProviderCount, // total requests fired }); ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T17:05:08Z

        return;
      }

+      const isAnthropicProvider =
+        attempt.provider.providerType === "claude" ||
+        attempt.provider.providerType === "claude-auth";
+      if (isAnthropicProvider) {


Signature rectifier fires for ALL failed attempts of the same provider in streamingHedge

The guard !signatureRectifierRetriedProviderIds.has(attempt.provider.id) correctly prevents infinite loops — once rectification is attempted for a provider, subsequent failures from the same provider skip the rectifier. However, there is a window for a race condition in the streaming hedge context:

If the initial provider (A) triggers the rectifier and the system calls launchAlternative(), provider B is launched concurrently. If provider B also fails with a thinking signature error, it will enter this block because signatureRectifierRetriedProviderIds tracks provider IDs separately per provider. That's fine in isolation, but rectifyAnthropicRequestMessage mutates session.request.message in-place. If both provider A's rectifier call and provider B's rectifier call execute concurrently (or in rapid succession), the second rectification runs on an already-rectified message body.

For the !hasRetryBudget path this isn't a concern because return is called after launchAlternative(). But for the hasRetryBudget path (startAttempt + return), two concurrent retries could both attempt to rectify the same message object.

Consider recording the rectification as already applied to the session before launching the retry, or making rectifyAnthropicRequestMessage idempotent (checking if fields were already removed).

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 3118-3124 Comment: **Signature rectifier fires for ALL failed attempts of the same provider in `streamingHedge`** The guard `!signatureRectifierRetriedProviderIds.has(attempt.provider.id)` correctly prevents infinite loops — once rectification is attempted for a provider, subsequent failures from the same provider skip the rectifier. However, there is a window for a race condition in the streaming hedge context: If the initial provider (A) triggers the rectifier and the system calls `launchAlternative()`, provider B is launched concurrently. If provider B *also* fails with a thinking signature error, it will enter this block because `signatureRectifierRetriedProviderIds` tracks provider IDs separately per provider. That's fine in isolation, but `rectifyAnthropicRequestMessage` mutates `session.request.message` in-place. If both provider A's rectifier call and provider B's rectifier call execute concurrently (or in rapid succession), the second rectification runs on an already-rectified message body. For the **`!hasRetryBudget` path** this isn't a concern because `return` is called after `launchAlternative()`. But for the **`hasRetryBudget` path** (`startAttempt` + `return`), two concurrent retries could both attempt to rectify the same message object. Consider recording the rectification as already applied to the session *before* launching the retry, or making `rectifyAnthropicRequestMessage` idempotent (checking if fields were already removed). How can I resolve this? If you propose a fix, please make it concise.

tesgth032 · 2026-03-15T17:06:12Z

本轮跟进主要处理 Greptile/AI review 的两个点并补齐覆盖：

streaming hedge 的顺序故障转移不再写入 hedge_launched / hedge_winner

仅当 first-byte hedge 计时器真正触发时才会进入 hedge_* reason
顺序 failover（例如 rectifier 达到 maxRetryAttempts 后切换供应商）现在会记录为 retry_success，避免审计误导

补齐 streaming hedge + thinking budget rectifier 的 maxRetryAttempts=2 用例

覆盖同供应商重试路径（allowDuplicateProvider=true）
同时增加 maxRetryAttempts=1 且无替代供应商时应抛出原始 400（不掩盖为 503）的 streaming 用例
用例也断言 no-hedge 场景不会出现 hedge_* reason

另外做了两个小整理：

rectifier 分支 specialSettings 持久化改为复用 persistSpecialSettings()（避免 send() 与 streaming 分支漂移）
测试侧改用本地期望常量，移除对生产导出常量的耦合（并取消不必要的 export）

验证：

本地：bun run typecheck / bun run test / bun run build 全通过
Actions：已全绿

- 先挂载 rejects 断言再推进 fake timers，避免 Vitest 记录未处理的 Promise 拒绝 - 移除冗余 doForward 默认 mockImplementation，提升可读性 - 修正 forwarder.ts 中对 retry_failed 语义的误导性注释

- 用 rejects 断言替代手动 catch 变量，避免 promise microtask 时序导致潜在 flake - 将 lastRectifierSwitchError 限定在单次 provider 尝试作用域，并补充为何跳过剩余端点的注释

- retryAttemptNumber 在无同供应商重试预算时缺省，避免误导审计\n\n- streaming hedge 失败路径补齐 thinking signature/budget rectifier，并严格遵守 maxRetryAttempts（可重试则同 provider retry，不可则 failover）\n\n- 补齐单测：stream=true hedge 场景、maxRetryAttempts=2 连续失败边界；并收敛 selectAlternative mock 为 mockResolvedValueOnce

- 在 maxRetryAttempts=1 的 provider failover / 无替代供应商用例中断言 retryAttemptNumber 为 undefined，防止审计字段回归误记

tesgth032 · 2026-03-15T17:54:55Z

本轮补齐了 Greptile 提到的 streaming hedge 并发 rectifier 风险，并用单测保护：

修复：在 sendStreamingWithHedge() 引入 requestVersion + rectifierLock。
- 当 thinking signature / budget rectifier 触发并修改主 session 的 request 后，之前已发出的并发 attempt 会变成“陈旧快照”。
- 现在遇到这类 stale attempt 的 400 时，不再把它当作 NON_RETRYABLE_CLIENT_ERROR 直接终止整个 hedge；而是按剩余预算重试/或进入供应商切换，保证 in-flight 的“整流后重试”不会被错误打断。
- rectifierLock 串行化整流与 specialSettings 持久化，避免并发写入顺序导致的审计丢失/覆盖。
测试：新增 2 个用例覆盖 stale 并发场景（signature / budget 各 1 个），确保不会因并发 attempt 的旧错误而提前失败。

本地验证：bun run typecheck / bun run test / bun run build 以及对点 bunx biome check 全通过。

greptile-apps · 2026-03-15T18:12:03Z

+        if (signatureTrigger) {
+          const settings = await getCachedSystemSettings();
+          const enabled = settings.enableThinkingSignatureRectifier ?? true;
+
+          if (enabled) {
+            // This is a client input / request-shape issue, and must not be counted into circuit breakers.
+            // Also, keep NON_RETRYABLE_CLIENT_ERROR so that if no alternative provider exists,
+            // resolveHedgeTerminalError() returns the original error instead of a generic 503.
+            errorCategory = ErrorCategory.NON_RETRYABLE_CLIENT_ERROR;
+            lastErrorCategory = errorCategory;
+
+            const handled = await withRectifierLock(async () => {
+              if (settled || winnerCommitted) return false;
+
+              // If request has been rectified since this attempt started, treat its error as stale.
+              // Do not terminate early based on an outdated request snapshot.
+              if (attempt.requestVersion < requestVersion) {
+                const hasProviderInFlight = Array.from(attempts).some(
+                  (other) =>
+                    other.provider.id === attempt.provider.id &&
+                    other.requestVersion === requestVersion &&
+                    !other.settled
+                );
+                if (hasProviderInFlight) {


Always-true if (!hasProviderInFlight) dead branch in stale-path

After if (hasProviderInFlight) { return true; }, every subsequent line is only reachable when hasProviderInFlight is false. The if (!hasProviderInFlight) { ... } wrapper is therefore always entered — it is a dead condition. This means the launchAlternative() / finishIfExhausted() calls below are silently only reachable when !hasRetryBudget, which isn't obvious to readers scanning the code.

The same structural issue appears identically in the budget rectifier stale-check path a little further down.

Consider flattening:

// hasProviderInFlight === false here (we returned true above if it was) const currentAttemptNumber = providerAttemptCounts.get(attempt.provider.id) ?? attempt.providerAttemptNumber; const hasRetryBudget = currentAttemptNumber < attempt.maxAttemptsPerProvider; session.addProviderToChain(attempt.provider, { ... }); if (hasRetryBudget) { await startAttempt(attempt.provider, attempt.provider.id === initialProvider.id, { allowDuplicateProvider: true, }); return true; } // !hasRetryBudget → fall through to launchAlternative await launchAlternative(); await finishIfExhausted(); return true;

This makes it explicit that launchAlternative() is reached only when !hasRetryBudget.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 3162-3185 Comment: **Always-true `if (!hasProviderInFlight)` dead branch in stale-path** After `if (hasProviderInFlight) { return true; }`, every subsequent line is only reachable when `hasProviderInFlight` is `false`. The `if (!hasProviderInFlight) { ... }` wrapper is therefore always entered — it is a dead condition. This means the `launchAlternative()` / `finishIfExhausted()` calls below are silently only reachable when `!hasRetryBudget`, which isn't obvious to readers scanning the code. The same structural issue appears identically in the budget rectifier stale-check path a little further down. Consider flattening: ```typescript // hasProviderInFlight === false here (we returned true above if it was) const currentAttemptNumber = providerAttemptCounts.get(attempt.provider.id) ?? attempt.providerAttemptNumber; const hasRetryBudget = currentAttemptNumber < attempt.maxAttemptsPerProvider; session.addProviderToChain(attempt.provider, { ... }); if (hasRetryBudget) { await startAttempt(attempt.provider, attempt.provider.id === initialProvider.id, { allowDuplicateProvider: true, }); return true; } // !hasRetryBudget → fall through to launchAlternative await launchAlternative(); await finishIfExhausted(); return true; ``` This makes it explicit that `launchAlternative()` is reached only when `!hasRetryBudget`. How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T18:12:04Z

@@ -3188,12 +3522,12 @@ export class ProxyForwarder {
            session.sessionId!,


"retry_success" used for cross-provider failover success when hedgeTriggered = false

When a rectifier's !hasRetryBudget path calls launchAlternative() and the alternative provider succeeds without a prior hedge threshold trigger (hedgeTriggered = false), isHedgeWin = false and isRetrySuccess = !isHedgeWin && (uniqueProvidersAttempted > 1) evaluates to true. The chain records reason: "retry_success".

However, isFailoverSuccess is simultaneously true (different provider from initial) and is separately logged via setSuccessAuditValues. This creates an inconsistency between the chain reason field ("retry_success") and the audit value (isFailoverSuccess: true) for what is semantically a provider-switch event, not a same-provider retry.

Operators parsing the chain looking for failover events would use reason, but here the same event would appear as "retry_success" rather than any failover-specific label. The previously flagged hedge_winner issue (prior thread) has been corrected, but a dedicated reason like "rectifier_failover_success" (or reusing "failover_success") would eliminate the remaining ambiguity, since isFailoverSuccess alone is not surfaced in the chain.

reason: isHedgeWin ? "hedge_winner" : isFailoverSuccess ? "failover_success" // distinguish cross-provider failover from same-provider retry : isRetrySuccess ? "retry_success" : "request_success",

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 3498-3522 Comment: **`"retry_success"` used for cross-provider failover success when `hedgeTriggered = false`** When a rectifier's `!hasRetryBudget` path calls `launchAlternative()` and the alternative provider succeeds without a prior hedge threshold trigger (`hedgeTriggered = false`), `isHedgeWin = false` and `isRetrySuccess = !isHedgeWin && (uniqueProvidersAttempted > 1)` evaluates to `true`. The chain records `reason: "retry_success"`. However, `isFailoverSuccess` is simultaneously `true` (different provider from initial) and is separately logged via `setSuccessAuditValues`. This creates an inconsistency between the chain `reason` field (`"retry_success"`) and the audit value (`isFailoverSuccess: true`) for what is semantically a provider-switch event, not a same-provider retry. Operators parsing the chain looking for failover events would use `reason`, but here the same event would appear as `"retry_success"` rather than any failover-specific label. The previously flagged `hedge_winner` issue (prior thread) has been corrected, but a dedicated reason like `"rectifier_failover_success"` (or reusing `"failover_success"`) would eliminate the remaining ambiguity, since `isFailoverSuccess` alone is not surfaced in the chain. ```typescript reason: isHedgeWin ? "hedge_winner" : isFailoverSuccess ? "failover_success" // distinguish cross-provider failover from same-provider retry : isRetrySuccess ? "retry_success" : "request_success", ``` How can I resolve this? If you propose a fix, please make it concise.

tesgth032 · 2026-03-15T18:45:50Z

本轮跟进仅做 1 个等价整理，回应 Greptile inline comment（不改语义，仅提可读性）：

修正 sendStreamingWithHedge() 中 rectifier stale-path 的 always-true 分支：移除 if (hasProviderInFlight) return true; 之后冗余的 if (!hasProviderInFlight) { ... } 包装，使控制流更直观。
- 现在代码明确表达：只有在 不存在同 provider 的 in-flight attempt 且 同 provider 重试预算已耗尽 时才会进入 launchAlternative()；否则继续同 provider 重试或忽略 stale 错误。

验证：

bun run typecheck
bun run test
bun run build
bunx biome check src/app/v1/_lib/proxy/forwarder.ts

备注：bun run lint 当前会命中仓库既有的 Biome 规则告警/错误（与本 PR 无关，且不在本 PR 范围内处理）。

greptile-apps · 2026-03-15T18:51:03Z

        return;
      }

+      const isAnthropicProvider =
+        attempt.provider.providerType === "claude" ||
+        attempt.provider.providerType === "claude-auth";
+      if (isAnthropicProvider) {
+        // Align streaming-hedge behavior with send(): rectify specific Anthropic 400s and then retry/failover,
+        // while strictly respecting provider-level maxRetryAttempts.
+        const signatureTrigger = detectThinkingSignatureRectifierTrigger(errorMessage);
+        if (signatureTrigger) {
+          const settings = await getCachedSystemSettings();
+          const enabled = settings.enableThinkingSignatureRectifier ?? true;
+


Stale attempt with in-flight sibling silently drops from audit chain

When hasProviderInFlight is true, the function returns true immediately without adding a addProviderToChain entry for the stale failure. Every other failure exit from the rectifier block (the !hasProviderInFlight stale path and the fresh path) calls session.addProviderToChain(...) before proceeding. Silently swallowing this failure means concurrent streaming-hedge failures during rectifier handling are invisible to audit logs — operators will see fewer chain entries than actual upstream requests were made.

Consider adding a chain entry before the early return:

if (hasProviderInFlight) { // Record the stale failure so the audit chain reflects every request made. session.addProviderToChain(attempt.provider, { ...attempt.endpointAudit, reason: "retry_failed", attemptNumber: attempt.sequence, statusCode, errorMessage, circuitState: getCircuitState(attempt.provider.id), }); return true; }

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 3152-3165 Comment: **Stale attempt with in-flight sibling silently drops from audit chain** When `hasProviderInFlight` is `true`, the function returns `true` immediately without adding a `addProviderToChain` entry for the stale failure. Every other failure exit from the rectifier block (the `!hasProviderInFlight` stale path and the fresh path) calls `session.addProviderToChain(...)` before proceeding. Silently swallowing this failure means concurrent streaming-hedge failures during rectifier handling are invisible to audit logs — operators will see fewer chain entries than actual upstream requests were made. Consider adding a chain entry before the early return: ```typescript if (hasProviderInFlight) { // Record the stale failure so the audit chain reflects every request made. session.addProviderToChain(attempt.provider, { ...attempt.endpointAudit, reason: "retry_failed", attemptNumber: attempt.sequence, statusCode, errorMessage, circuitState: getCircuitState(attempt.provider.id), }); return true; } ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T18:51:04Z

@@ -3259,6 +3606,7 @@ export class ProxyForwarder {
        return;
      }


providerAttemptCounts incremented before endpoint validation succeeds

providerAttemptCounts.set(provider.id, providerAttemptNumber) is executed synchronously before the async endpoint selection. If endpointSelection is null (no endpoints available), the function exits early — but the count is already bumped. In subsequent stale-path checks for the same provider:

const currentAttemptNumber = providerAttemptCounts.get(attempt.provider.id) ?? attempt.providerAttemptNumber; const hasRetryBudget = currentAttemptNumber < attempt.maxAttemptsPerProvider;

currentAttemptNumber will reflect the phantom increment, making hasRetryBudget appear exhausted one step earlier than it truly is. Worse, if hasRetryBudget is still true, the stale path calls startAttempt again, which increments the count a second time and hits endpoint selection failure once more — producing a redundant launchAlternative() chain.

Move the providerAttemptCounts.set call to after a successful endpoint resolution (alongside startedProviderIds.add):

// After: if (!endpointSelection) { ... return; } startedProviderIds.add(provider.id); providerAttemptCounts.set(provider.id, providerAttemptNumber); launchedProviderCount += 1;

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 3585-3607 Comment: **`providerAttemptCounts` incremented before endpoint validation succeeds** `providerAttemptCounts.set(provider.id, providerAttemptNumber)` is executed synchronously before the async endpoint selection. If `endpointSelection` is `null` (no endpoints available), the function exits early — but the count is already bumped. In subsequent stale-path checks for the same provider: ```typescript const currentAttemptNumber = providerAttemptCounts.get(attempt.provider.id) ?? attempt.providerAttemptNumber; const hasRetryBudget = currentAttemptNumber < attempt.maxAttemptsPerProvider; ``` `currentAttemptNumber` will reflect the phantom increment, making `hasRetryBudget` appear exhausted one step earlier than it truly is. Worse, if `hasRetryBudget` is still `true`, the stale path calls `startAttempt` again, which increments the count a second time and hits endpoint selection failure once more — producing a redundant `launchAlternative()` chain. Move the `providerAttemptCounts.set` call to after a successful endpoint resolution (alongside `startedProviderIds.add`): ```typescript // After: if (!endpointSelection) { ... return; } startedProviderIds.add(provider.id); providerAttemptCounts.set(provider.id, providerAttemptNumber); launchedProviderCount += 1; ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-15T18:51:05Z

+                  circuitState: getCircuitState(attempt.provider.id),
+                });
+
+                if (hasRetryBudget) {
+                  await startAttempt(attempt.provider, attempt.provider.id === initialProvider.id, {
+                    allowDuplicateProvider: true,
+                  });
+                  return true;
+                }
+
+                await launchAlternative();
+                await finishIfExhausted();
+                return true;
+              }
+
+              if (signatureRectifierRetriedProviderIds.has(attempt.provider.id)) {
+                return false;


Stale retry path does not mark provider in signatureRectifierRetriedProviderIds

When the stale path decides hasRetryBudget = true and calls startAttempt, it never adds attempt.provider.id to signatureRectifierRetriedProviderIds. If that stale retry subsequently fails with another signature error, its requestVersion will equal the current requestVersion (fresh path), the guard signatureRectifierRetriedProviderIds.has(attempt.provider.id) returns false, and the code calls rectifyAnthropicRequestMessage a second time. Since the message is already clean, rectified.applied = false and the handler returns false. This causes the failure to fall through to the normal error-handling path, which calls launchAlternative() again.

While not catastrophic (idempotent rectifier + NON_RETRYABLE_CLIENT_ERROR prevents circuit-breaker writes), it produces an extraneous audit entry with hit: false and an unnecessary selectAlternative call.

Add the provider to the set before launching the stale retry:

if (hasRetryBudget) { signatureRectifierRetriedProviderIds.add(attempt.provider.id); await startAttempt(attempt.provider, attempt.provider.id === initialProvider.id, { allowDuplicateProvider: true, }); return true; }

The same pattern applies to the budget rectifier stale path.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/app/v1/_lib/proxy/forwarder.ts Line: 3199-3215 Comment: **Stale retry path does not mark provider in `signatureRectifierRetriedProviderIds`** When the stale path decides `hasRetryBudget = true` and calls `startAttempt`, it never adds `attempt.provider.id` to `signatureRectifierRetriedProviderIds`. If that stale retry subsequently fails with another signature error, its `requestVersion` will equal the current `requestVersion` (fresh path), the guard `signatureRectifierRetriedProviderIds.has(attempt.provider.id)` returns `false`, and the code calls `rectifyAnthropicRequestMessage` a second time. Since the message is already clean, `rectified.applied = false` and the handler returns `false`. This causes the failure to fall through to the normal error-handling path, which calls `launchAlternative()` again. While not catastrophic (idempotent rectifier + `NON_RETRYABLE_CLIENT_ERROR` prevents circuit-breaker writes), it produces an extraneous audit entry with `hit: false` and an unnecessary `selectAlternative` call. Add the provider to the set before launching the stale retry: ```typescript if (hasRetryBudget) { signatureRectifierRetriedProviderIds.add(attempt.provider.id); await startAttempt(attempt.provider, attempt.provider.id === initialProvider.id, { allowDuplicateProvider: true, }); return true; } ``` The same pattern applies to the budget rectifier stale path. How can I resolve this? If you propose a fix, please make it concise.

ding113 · 2026-03-16T14:29:23Z

整流器本来就不应该算在重试次数里，因此此 PR 可能违反了设计预期。

github-project-automation Bot added this to Claude Code Hub Roadmap Mar 15, 2026

github-project-automation Bot moved this to Backlog in Claude Code Hub Roadmap Mar 15, 2026

gemini-code-assist Bot reviewed Mar 15, 2026

View reviewed changes

github-actions Bot added bug Something isn't working area:core area:provider size/M Medium PR (< 500 lines) labels Mar 15, 2026

coderabbitai Bot reviewed Mar 15, 2026

View reviewed changes

coderabbitai Bot approved these changes Mar 15, 2026

View reviewed changes

github-actions Bot reviewed Mar 15, 2026

View reviewed changes

tesgth032 force-pushed the fix/max-retry-attempts-rectifier branch from 13810df to e59ff5a Compare March 15, 2026 12:48

greptile-apps Bot reviewed Mar 15, 2026

View reviewed changes

coderabbitai Bot reviewed Mar 15, 2026

View reviewed changes

coderabbitai Bot requested changes Mar 15, 2026

View reviewed changes

Comment thread tests/unit/proxy/proxy-forwarder-retry-limit.test.ts

greptile-apps Bot reviewed Mar 15, 2026

View reviewed changes

coderabbitai Bot reviewed Mar 15, 2026

View reviewed changes

coderabbitai Bot approved these changes Mar 15, 2026

View reviewed changes

greptile-apps Bot reviewed Mar 15, 2026

View reviewed changes

coderabbitai Bot requested changes Mar 15, 2026

View reviewed changes

Comment thread src/app/v1/_lib/proxy/forwarder.ts

Comment thread src/app/v1/_lib/proxy/forwarder.ts Outdated

greptile-apps Bot reviewed Mar 15, 2026

View reviewed changes

coderabbitai Bot reviewed Mar 15, 2026

View reviewed changes

coderabbitai Bot approved these changes Mar 15, 2026

View reviewed changes

greptile-apps Bot reviewed Mar 15, 2026

View reviewed changes

tesgth032 added 11 commits March 16, 2026 01:23

fix(proxy): rectifier 场景尊重单供应商重试上限

0f00f7a

test(proxy): 复用 thinking budget 常量避免魔数

a3a1ac7

fix(proxy): rectifier 无替代时清理会话绑定并修正审计

4a53b36

test(proxy): 补齐 enableResponseInputRectifier mock

e6174b7

test(proxy): 规避 fake timer 用例的微任务竞态

a6a4707

- 用 rejects 断言替代手动 catch 变量，避免 promise microtask 时序导致潜在 flake - 将 lastRectifierSwitchError 限定在单次 provider 尝试作用域，并补充为何跳过剩余端点的注释

test(proxy): no-budget 切换路径断言 retryAttemptNumber 缺省

e3cfb1d

- 在 maxRetryAttempts=1 的 provider failover / 无替代供应商用例中断言 retryAttemptNumber 为 undefined，防止审计字段回归误记

fix(proxy): streaming hedge 顺序切换不再标记 hedge_*

c69be60

test(proxy): 补齐 streaming budget rectifier 覆盖并去除常量耦合

b50ed1c

fix(proxy): streaming hedge rectifier 避免 stale attempt 误终止

b45a340

tesgth032 force-pushed the fix/max-retry-attempts-rectifier branch from e3862e5 to b45a340 Compare March 15, 2026 17:53

greptile-apps Bot reviewed Mar 15, 2026

View reviewed changes

refactor(proxy): 扁平化 streaming stale-path 控制流

6f6daf6

greptile-apps Bot reviewed Mar 15, 2026

View reviewed changes

tesgth032 changed the title ~~fix(proxy): rectifier 不应绕过 maxRetryAttempts [未完成]~~ fix(proxy): rectifier 不应绕过 maxRetryAttempts Mar 15, 2026

ding113 closed this Mar 16, 2026

github-project-automation Bot moved this from Backlog to Done in Claude Code Hub Roadmap Mar 16, 2026

github-actions Bot mentioned this pull request Mar 18, 2026

fix: run reactive rectifiers during streaming hedge #945

Merged

4 tasks

tesgth032 deleted the fix/max-retry-attempts-rectifier branch June 7, 2026 09:53

	doForward.mockResolvedValueOnce(
	selectAlternative.mockResolvedValueOnce(providerB);

		@@ -3188,12 +3522,12 @@ export class ProxyForwarder {
		session.sessionId!,

Uh oh!

Conversation

tesgth032 commented Mar 15, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Root Cause

Solution

Changes

Core Changes

Supporting Changes

Testing

Automated Tests

Checklist

Greptile Summary

Confidence Score: 2/5

Important Files Changed

Flowchart

Comments Outside Diff (3)

Uh oh!

coderabbitai Bot commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

gemini-code-assist Bot commented Mar 15, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Code Review Summary

PR Size: M

Issues Found

Critical Issues (Must Fix)

High Priority Issues (Should Fix)

Review Coverage

Test Summary

Uh oh!

greptile-apps Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greptile-apps Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 15, 2026

tesgth032 commented Mar 15, 2026 •

edited by greptile-apps Bot

Loading

coderabbitai Bot commented Mar 15, 2026 •

edited

Loading