Skip to content

feat(proxy): opt-in same-target 429 wait-and-retry before key failover (#487) - #865

Open
harryzhou2000 wants to merge 29 commits into
lidge-jun:devfrom
harryzhou2000:feat/429-same-target-retry
Open

feat(proxy): opt-in same-target 429 wait-and-retry before key failover (#487)#865
harryzhou2000 wants to merge 29 commits into
lidge-jun:devfrom
harryzhou2000:feat/429-same-target-retry

Conversation

@harryzhou2000

@harryzhou2000 harryzhou2000 commented Aug 1, 2026

Copy link
Copy Markdown

Summary

Adds an opt-in, provider-level retryOn429 policy: on HTTP 429 the proxy waits (upstream Retry-After or a fixed interval, capped) and replays the identical pre-stream request on the same key before any multi-key failover.

Why

Behavior

  • Config: providers.<name>.retryOn429 = { enabled?, attempts?, intervalMs?, maxIntervalMs?, respectRetryAfter? } (defaults: enabled=true, attempts=3, intervalMs=5000, maxIntervalMs=60000, respectRetryAfter=true). Default off when absent → zero behavior change.
  • API-key providers only (authMode: "key", or the documented omitted default for custom API-key providers). Fail closed: OAuth/forward credentials are never replayed on the same token; local runtimes (Ollama etc.) have no remote key to preserve; unknown/custom auth modes are rejected rather than guessed at. providerConfigSeed now preserves the registry auth kind (including "local") so the gate survives the seed round-trip.
  • Pre-stream only (429 arrives before any bytes are relayed → replay is lossless). Runs before key failover; failover still works after attempts exhaust; final 429 keeps Retry-After.
  • Retry budget is scoped per request and lives outside the recovery loop, so a 413/401 replay that comes back 429 cannot re-arm a fresh budget (bounded to attempts).
  • Covers /v1/responses, /v1/chat/completions, and routed /v1/messages (all enter handleResponses), plus the other key-auth surfaces that bypass that loop: the Responses passthrough wire (openai-responses key-auth gateways, e.g. the built-in DeepSeek preset), the image/video bridge and web-search sidecar loops (before their on429 key rotation), and Anthropic terminal-guard continuations (before key/account failover).
  • Abort during the wait: the sleep is abort-aware. Once the server observes the client disconnect (Bun propagates it asynchronously, observed 1–10s), the unread 429 body is released, the upstream fetch is aborted, and the request is cancelled with 499 before any replay. Because propagation is async, a replay may precede the cancel if the interval elapses first — bounded by the same attempts budget.
  • Any single wait — Retry-After or the fixed fallback — is capped at maxIntervalMs; the schema caps maxIntervalMs at 600000 (the effective per-wait cooldown ceiling). An already-expired HTTP-date Retry-After retries immediately (like Retry-After: 0).
  • Every surface releases the unread 429 body BEFORE the backoff and records the rate-limit-429 recovery kind on replay sends; the image/video and web-search bridge loops restart their response-header deadline after each deliberate wait, so backoffs never consume the connect budget or surface as a 504.
  • Config load degrades invalid optional retryOn429 fields with a warning instead of tripping the whole schema (which would hide every provider/key behind a default config); the management write boundary still rejects invalid policies.

Files

  • src/types.tsRateLimitRetryPolicy + OcxProviderConfig.retryOn429
  • src/config.ts — zod validation (outer provider schema stays passthrough; a typo inside retryOn429 degrades, never rejects the whole config), maxIntervalMs ≤ 600000
  • src/providers/key-failover.ts — policy normalization gated to key-auth + delay computation (Retry-After seconds/HTTP-date/0, capped)
  • src/providers/derive.tsproviderConfigSeed preserves the registry auth kind (incl. "local")
  • src/server/responses/core.ts — recovery-loop replay before the multi-key failover while; passthrough-wire replay before the forward-pool logic; terminal-guard continuation replay before key/account failover; abort path cancels the unread 429 body first
  • src/images/loop.ts, src/web-search/loop.ts — same-target replay before their on429 key rotation (new retryOn429Policy dep)
  • src/usage/log.tsAttemptRecoveryKind member rate-limit-429 and its persisted-usage whitelist entry
  • gui/src/pages/Logs.tsx — recovery-kind union includes rate-limit-429 (and pre-existing anthropic-oauth-429)
  • docs-site configuration reference (all five locales), structure/04 transport note, devlog/_plan/260802_429_same_target_retry/

Test plan

  • bun test tests/rate-limit-retry.test.ts tests/server-rate-limit-retry-e2e.test.ts tests/usage-log.test.ts tests/key-failover.test.ts tests/retry-after-429.test.ts — 79 pass (policy/delay units incl. fail-closed auth gating, expired HTTP-date, Retry-After: 0, fallback cap; persisted rate-limit-429 recovery kind; deterministic direct-handler abort test; e2e: replay-to-success with byte-identical bodies, opt-in passthrough, exhaustion, retry-before-failover ordering, key-auth openai-responses passthrough replay with identical body/auth, per-request budget across a 2-key pool)
  • New surface tests: terminal-guard continuation 429 replay with byte-identical requests + per-request budget across a 2-key pool (tests/terminal-guard-server.test.ts); image bridge + web-search loop same-key replay before rotation with rate-limit-429 telemetry (tests/images/loop.test.ts, tests/web-search.test.ts); config load degradation (tests/config-user-edits.test.ts); registry auth-kind preservation (tests/provider-registry-parity.test.ts)
  • Full suite: 6805 pass / 6 skip / 8 fail (baseline environmental failures; stalled-400 test flaked once under load in one run and passed in the other two)
  • bun run typecheck
  • bun run privacy:scan
  • cd gui && bun run lint
  • Full suite: 6711 pass; remaining failures reproduce identically on pristine dev in this environment (WS/auth/connection-refused) plus GUI tests that pass once gui/ deps are installed

Commits

  • 667ad08f — feature implementation
  • 2efb887b — audit round-1 fixes (usage-log whitelist, key-auth gating, budget scope, abort body cancel, schema cap, GUI union, docs)
  • a06a4160 — audit round-3 xhigh fixes: coverage extended to passthrough wire / image+web-search bridges / terminal continuations, fixed-fallback cap, local-mode gating, docs + tests
  • 58c36c9e — review-bot round: fail-closed auth (registry preserves local), expired Retry-After → immediate, release 429 bodies before backoff, rate-limit-429 recovery on every surface, bridge header-deadline restart, continuation budget hoisted + upstream-signal sleep, config load degradation, identical-replay + budget regression tests, provider/adapter docs (5 locales)
  • 568e565c — review-bot round 2: awaited body-cancel before backoff, stale-deadline cleared pre-sleep + 499 re-check post-sleep, misnamed retryOn429 keys warn at load, opt-in + ordering wording in provider/adapter guides (5 locales)
  • 3c19337e — docstring coverage pass on the diff (JSDoc for seeded provider config, responses core helpers, image/web-search iteration prep, terminal-guard continuation, and test helpers)
  • 1af83c39 — attach JSDoc directly to the remaining diff-touched declarations (terminal-guard continuation, image/web-search fetchOnce, cooldown check, provider-config interface, persisted-usage helper)
  • d4e19788 — review round 3: retry budget hoisted per request across bridge iterations, single dispatch-time attempt telemetry (recovery kind passed through), invalid enabled master switch discards the whole policy, secret-safe config warnings (path + type only)
  • 58bf694f — local audit round (gpt-5.6-terra + DeepSeek-V4-Flash): one request-wide 429 budget shared between the main recovery loop and the terminal-guard continuation; awaited 429-body cancellation before every backoff (all surfaces); post-sleep client-abort re-check before dispatching every replay; xAI x-grok-req-id pinned per logical request so same-key replays are byte-identical; new regression tests (shared continuation budget, continuation abort-during-wait, bridge budget not re-armed after rotation, far-future HTTP-date cap, stable xAI request id)
  • 4fee87b5 — review round 4: unrecognized retryOn429 field NAMES are redacted before logging (secret-shaped property names become [REDACTED], ordinary typos stay readable)
  • 5945711b — review round 4 cleanup: rename the xAI request-id param to pinnedRequestId (fallback only at transport resolution); anchor the secret-warning test assertion to the exact field+type diagnostic
  • e65106f0 — review round 4 follow-up: JSON-escape the redacted field name in load warnings so control-character property names (newline/ANSI) cannot forge log lines
  • 89535fb7 — review round 5: redact and JSON-escape the PROVIDER name in all retryOn429 load warnings (sanitizer runs pre-validation, so the name is untrusted; secret-shaped names log as [REDACTED], control characters escaped)
  • eb9890e5 — maintainer architecture review round: (1) deliberate 429 backoffs now yield adapter heartbeats every min(10s, stall/2) in the terminal continuation and image/web-search loops, so a wait that outlives the bridge stall budget can never trip upstream_stall_timeout; (2) one immutable cached outbound request per same-target sequence — the main recovery loop, terminal continuation, and both sidecar loops reuse the exact URL/serialized body/headers, rebuilding only after key/account/oauth/tier changes (builder runs once per target, asserted); (3) regression tests: wait > stall budget still succeeds (all three surfaces), wait > connectTimeoutMs restarts the header deadline (no 504), full-header equality on e2e replays
  • 4e172054 — review round 6: clamp sleepWithHeartbeats step to ≥1ms (non-positive interval can no longer spin unobserved); pin the contract that a policy degraded to {} still resolves as enabled (object presence = opt-in) with sanitizer + resolved-policy assertions
  • 02601815 — review round 6 follow-up: normalize NaN heartbeat intervals to the 1ms step (NaN no longer aborts the wait after one beat); regression test asserts the full duration elapses
  • e8613e36 — merge upstream/dev (docs overhaul restructure). Conflicts were doc-only (5 locale configuration.md indexes): dev's restructured pages are kept, and the retryOn429 reference row moved into the new per-locale configuration/providers.md pages after responsesItemIdRepair. 302 targeted tests green on the merged tree.

Draft — opened for review, not for merge.

Summary by CodeRabbit

  • New Features

    • Added optional same-key retries for HTTP 429 responses across supported request types.
    • Retries honor Retry-After, configurable delays and limits, cancellation, heartbeats, and failover.
    • Added provider-level retryOn429 configuration for API-key authentication.
  • Bug Fixes

    • Improved request identifier stability, configuration validation, secret redaction, response cleanup, and cancellation handling.
    • Preserved provider authentication modes during configuration loading.
    • Added localized recovery labels for rate-limit and other retry events.
  • Documentation

    • Documented retry behavior, limits, supported authentication modes, exclusions, and provider routing across translations.

lidge-jun#487)

Codex never retries HTTP 429 (openai/codex#30471 keeps retry_429=false and the misleading 'exceeded retry limit' error), and single-key pools have no failover, so provider-level retryOn429 waits (Retry-After or fixed interval) and replays the identical pre-stream request on the same key before any key rotation.

- types.ts: RateLimitRetryPolicy + OcxProviderConfig.retryOn429
- config.ts: lenient zod validation (strip unknown; typo degrades, never rejects config)
- key-failover.ts: rateLimitRetryPolicyFor + rateLimitRetryDelayMs (Retry-After capped at maxIntervalMs)
- responses/core.ts: recovery-loop replay before multi-key failover; abort-aware sleep; covers Responses, chat completions, and routed Claude messages
- usage/log.ts: AttemptRecoveryKind 'rate-limit-429'
- docs-site configuration reference + structure/04 transport note + devlog plan unit
- tests: policy unit tests + e2e (single-key replay, passthrough without knob, exhaustion, retry-before-failover ordering)

Verified: typecheck, privacy scan, 43/43 retry-related tests; full suite failures are pre-existing on pristine dev in this environment (WS/auth/connection-refused) plus missing-gui-deps artifacts that pass once installed.
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds opt-in, same-key HTTP 429 retries for API-key providers. Retries use bounded delays, Retry-After, abort-aware heartbeats, request replay, recovery telemetry, and existing failover paths across response, passthrough, image, web-search, and Anthropic continuation flows.

Changes

Rate-limit retry policy and implementation

Layer / File(s) Summary
Retry policy contract and configuration
src/types.ts, src/providers/key-failover.ts, src/usage/log.ts, src/providers/derive.ts, structure/04_transports-and-sidecars.md, devlog/_plan/260802_429_same_target_retry/*
Defines RateLimitRetryPolicy interface with enablement, attempt count, fixed interval, maximum interval, and Retry-After handling. Specifies API-key eligibility, default retry settings, Retry-After parsing including zero-second and past-date handling, and recovery classification. Preserves local-provider authentication metadata during seeding.
Configuration loading and validation
src/config.ts, src/server/auth-cors.ts, tests/config-user-edits.test.ts, tests/management-provider-validation.test.ts
Loads configuration and sanitizes malformed retry policies before schema validation. Drops invalid fields and unknown keys. Preserves valid settings. Validates through management boundaries with secret-shaped name and field redaction. Tests cover invalid nesting, non-object values, disabled states, empty objects, diagnostics, type-specific warnings, and control-character escaping.
Upstream retry helper functions
src/lib/upstream-retry.ts, tests/upstream-retry.test.ts, tests/rate-limit-retry.test.ts
Provides releaseResponseBodyBestEffort to clean up response bodies with bounded timeouts and abort awareness. Provides sleepWithHeartbeats for abort-aware chunked sleep with heartbeat emissions after each chunk. Tests cover clamped intervals, NaN fallback behavior, cancellation timeouts, already-aborted signals, and null-body handling. Includes rate-limit delay and abort-integration tests.
Core response loop same-key retries
src/server/responses/core.ts, src/providers/xai-transport.ts, tests/server-rate-limit-retry-e2e.test.ts, tests/terminal-guard-server.test.ts, tests/abort-race.test.ts
Implements pre-stream 429 replay in the main response loop, passthrough key-auth requests, and Anthropic terminal continuations. Caches built requests per adapter for replay. Releases response bodies before backoff. Handles client abort during backoff as 499 error. Shares per-request retry budgets across key rotation. Supports abort-aware waits with heartbeats derived from stall timeout. Preserves xAI request ID per resolved transport for replay stability. Maps request-build failures to 400 or 499 responses. Tests cover single-key retry, disabled/exhausted retries, failover ordering, passthrough preservation, shared budgets, terminal-continuation budgets, and build-request error handling.
Image and web-search bridge same-adapter retries
src/images/loop.ts, src/web-search/loop.ts, tests/images/loop.test.ts, tests/web-search.test.ts
Adds same-adapter 429 replay with request caching per adapter. Honors Retry-After or fixed intervals with capping. Releases response bodies and refreshes header deadlines. Sleeps with abort-aware heartbeats. Converts cancellation to 499 errors. Retries before key rotation. Tracks cumulative per-request retry counts. Tests cover successful replay, timeout exceeding stall/connection budgets, shared retry budgets across iterations, exhausted-budget failover, and heartbeat behavior preventing upstream stall errors.
Recovery observability and localization
gui/src/pages/Logs.tsx, gui/src/i18n/*.ts, tests/usage-log.test.ts
Persists "rate-limit-429" recovery classification. Renders localized attempt detail labels through recovery-kind mapping with fallback for unknown cached values. Provides English and multilingual labels for eight recovery categories: transient 5xx, connection reset, OAuth reauthentication, key rate limit, general rate limit, Anthropic OAuth rate limit, oversized image payload, and unknown reason.
Request identity and configuration validation
src/providers/xai-transport.ts, tests/xai-transport.test.ts, tests/provider-registry-parity.test.ts, tests/abort-race.test.ts
Pins xAI request ID per resolved transport instead of generating per fetch, enabling stable identity across retries and replay. Validates provider metadata seeds authMode: "local" and authMode: "key" correctly. Maps synchronous build-request exceptions to 400 invalid_request_error. Maps client abort during build-request to 499 client_cancelled instead of 400 error.
Reference documentation
docs-site/src/content/docs/**
Documents retryOn429 scope limited to authMode: "key" API-key providers, excluding OAuth, forward, and local credentials. Explains opt-in behavior, default values, Retry-After handling, and supported request paths. Defines retry limits, shared budgets, and failover ordering. Adds configuration field documentation in English, Japanese, Korean, Russian, and Chinese locales.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ResponsesCore as Responses<br/>Core
  participant Transport as Provider<br/>Transport
  participant Upstream as Upstream<br/>Provider
  Client->>ResponsesCore: Submit request
  ResponsesCore->>Transport: Send with current API key
  Transport->>Upstream: Forward request
  Upstream-->>ResponsesCore: HTTP 429
  ResponsesCore->>ResponsesCore: Release body, wait
  ResponsesCore->>Transport: Replay identical request
  Transport->>Upstream: Forward on same key
  Upstream-->>ResponsesCore: Success or final 429
  alt Retry budget exhausted
    ResponsesCore->>Transport: Rotate to next key
    Transport->>Upstream: Forward new request
  else Replay succeeds
    Upstream-->>ResponsesCore: Response data
  end
  ResponsesCore->>Client: Stream or error
Loading

Possibly related PRs

Suggested reviewers: wibias, ingwannu, lidge-jun

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 64.10% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: opt-in same-target HTTP 429 retries before key failover.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the enhancement New feature or request label Aug 1, 2026
- usage/log: whitelist the rate-limit-429 recovery kind so persisted usage rows and
  post-restart /api/logs keep the reason
- key-failover: gate retryOn429 to key-auth providers (no same-token OAuth replays,
  no silent no-op on forward passthrough); accept Retry-After 0 as immediate
- config: cap maxIntervalMs at the effective 10-minute cooldown ceiling
- core: hoist the retry budget outside the recovery loop so 413/401 replays cannot
  re-arm it; cancel the unread 429 body before aborting on client cancel
- gui: add rate-limit-429 (and pre-existing anthropic-oauth-429) to the Logs union
- tests: OAuth/forward gating, HTTP-date Retry-After, Retry-After 0, persisted
  recovery-kind, and a deterministic direct-handler abort test (real-socket
  disconnect propagation is async in Bun, so the e2e version was timing-flaky)
- docs: key-auth-only note, maxIntervalMs cap, abort-propagation nuance, locale rows
… xhigh)

- core: the Responses passthrough wire (openai-responses key-auth gateways, e.g.
  the built-in DeepSeek preset) now replays 429 on the same key pre-relay, before
  the forward-pool logic; Anthropic terminal-guard continuations replay before
  key/account failover
- images/loop + web-search/loop: same-target replay before on429 key rotation via
  a new retryOn429Policy dep (abort-aware, heartbeat seams preserved)
- key-failover: the fixed fallback is now capped at maxIntervalMs (a single wait
  never exceeds the cap); authMode local runtimes are gated out alongside
  oauth/forward, so the knob matches the documented API-key scope exactly
- tests: key-auth openai-responses passthrough e2e, terminal-guard continuation
  replay, image/web-search same-key replay (rotation stays zero), fallback cap,
  local-mode gating
- docs: coverage and cap wording in devlog 010, structure/04, and all five
  configuration locale rows
@harryzhou2000
harryzhou2000 marked this pull request as ready for review August 1, 2026 18:54

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@devlog/_plan/260802_429_same_target_retry/010_design.md`:
- Around line 57-67: Update the latency and concurrency sections of the retry
design to distinguish retry-wait time from total request latency, noting that
connection, response, and configured timeout durations also contribute. Revise
the request-volume bound to account for multi-key failover, including the
possibility of exhausting the retry budget on one key before attempting another,
and document the combined bound alongside the existing failover behavior.
- Around line 7-9: Update same-key retry handling around the
continuation-request rebuild in core response processing so every retry replays
a cached, body-safe representation of the identical upstream request, including
serialized body and authentication headers, rather than rebuilding it each time.
Apply this to passthrough Responses and Anthropic terminal continuations; only
rebuild after the target or adapter changes, defining deterministic equivalence
where adapters must rebuild. Extend the rate-limit retry and terminal-guard
tests to assert body and authentication/header equality, not just send counts.
- Around line 32-34: Update rate-limit retry authentication handling to fail
closed: normalize omitted key-provider auth modes to "key", preserve "local" in
providerConfigSeed (including the mapping in derive), and make
rateLimitRetryPolicyFor allow retries only when authMode === "key". Add
regression coverage for omitted, local, and unknown authentication modes while
preserving existing key-provider behavior.

In `@docs-site/src/content/docs/reference/configuration.md`:
- Line 353: Update the retryOn429 documentation across the English, Japanese,
Korean, Russian, and Chinese provider and adapter reference pages. State that it
applies only to authMode: "key" and excludes oauth, forward, and local
providers; document same-key replay, raw openai-responses passthrough,
translated openai-chat/Anthropic requests, and that custom runTurn transports
are excluded. Keep the existing defaults and behavior description consistent
across all pages.

In `@src/images/loop.ts`:
- Around line 467-490: The 429 retry waits must not consume the cumulative
header deadline in either loop. In src/images/loop.ts lines 467-490, update the
flow around the headerDeadline and rateLimitRetryPolicy retry loop so each
deliberate wait is excluded, while preserving the configured retry outcome and
preventing the 504 header-timeout path from firing due to that wait; apply the
identical change in src/web-search/loop.ts lines 361-384 around its
headerDeadline and retry loop.

In `@src/providers/key-failover.ts`:
- Around line 76-96: Ensure local providers cannot enter the key-failover retry
path when authMode is undefined: preserve authMode: "local" through the provider
derivation/routing flow or pass the registry auth kind into
rateLimitRetryPolicyFor, while retaining undefined as the default for custom
API-key providers. Add a regression test covering a local provider configured
with retryOn429: {}.

In `@src/server/responses/core.ts`:
- Around line 1631-1671: Update the retry closure in the passthrough 429 loop to
pass `"rate-limit-429"` to `noteAttemptSend` whenever `fetchWithTransientRetry`
provides no recovery kind, while preserving any recovery kind it does provide.
Keep the existing `rateLimitPolicy` retry flow unchanged.
- Around line 2629-2665: Hoist the rateLimitPolicy and rateLimitRetries
declarations out of the terminal continuation while loop, placing them beside
imageTierBias so the retry budget persists across key rotation, account
rotation, and image-tier 413 continues. Resolve rateLimitPolicy once from the
initial route.provider and preserve the existing retry loop behavior while
preventing the budget from resetting per iteration.

In `@tests/rate-limit-retry.test.ts`:
- Around line 99-159: The retry tests lack coverage proving the retry budget is
scoped per request rather than reset during failover. Add focused regression
coverage near the existing retry-loop tests using a two-key provider pool,
retryOn429 with attempts set to 1, and upstream responses that always return
429; assert the total upstream send count remains within the request-wide bound,
covering both the main recovery loop and terminal-continuation path.

In `@tests/usage-log.test.ts`:
- Around line 39-62: Replace the as never cast in the
persists-the-rate-limit-429-recovery-kind-on-attempts test with a
PersistedUsageEntry-typed object. Keep the recoveryKinds literal type-checked
against the recovery-kind union, and cast only individual fields that genuinely
require it while preserving the existing round-trip assertion.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a0cd7539-4937-4e9d-bf2a-e78d059e114a

📥 Commits

Reviewing files that changed from the base of the PR and between aae9426 and a06a416.

📒 Files selected for processing (22)
  • devlog/_plan/260802_429_same_target_retry/000_research.md
  • devlog/_plan/260802_429_same_target_retry/010_design.md
  • docs-site/src/content/docs/ja/reference/configuration.md
  • docs-site/src/content/docs/ko/reference/configuration.md
  • docs-site/src/content/docs/reference/configuration.md
  • docs-site/src/content/docs/ru/reference/configuration.md
  • docs-site/src/content/docs/zh-cn/reference/configuration.md
  • gui/src/pages/Logs.tsx
  • src/config.ts
  • src/images/loop.ts
  • src/providers/key-failover.ts
  • src/server/responses/core.ts
  • src/types.ts
  • src/usage/log.ts
  • src/web-search/loop.ts
  • structure/04_transports-and-sidecars.md
  • tests/images/loop.test.ts
  • tests/rate-limit-retry.test.ts
  • tests/server-rate-limit-retry-e2e.test.ts
  • tests/terminal-guard-server.test.ts
  • tests/usage-log.test.ts
  • tests/web-search.test.ts

Comment thread devlog/_plan/260802_429_same_target_retry/010_design.md
Comment thread devlog/_plan/260802_429_same_target_retry/010_design.md Outdated
Comment thread devlog/_plan/260802_429_same_target_retry/010_design.md Outdated
Comment thread docs-site/src/content/docs/reference/configuration.md Outdated
Comment thread src/images/loop.ts
Comment thread src/providers/key-failover.ts
Comment thread src/server/responses/core.ts
Comment thread src/server/responses/core.ts
Comment thread tests/rate-limit-retry.test.ts
Comment thread tests/usage-log.test.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a06a416024

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/server/responses/core.ts Outdated
Comment thread src/server/responses/core.ts Outdated
Comment thread src/images/loop.ts Outdated
Comment thread src/providers/key-failover.ts
Comment thread src/config.ts Outdated
Comment thread src/server/responses/core.ts
Comment thread src/server/responses/core.ts Outdated
… deadlines, recovery labels)

- key-failover: fail closed - only authMode key (or the documented omitted
  default) may replay; unknown modes rejected; an already-expired HTTP-date
  Retry-After retries immediately (like Retry-After: 0)
- derive: providerConfigSeed preserves the registry auth kind (incl. local) so
  the gate survives the seed round-trip and routing
- core: release the unread 429 body BEFORE every backoff (main loop, passthrough,
  continuations); passthrough and continuation replay sends record the
  rate-limit-429 recovery kind; continuation budget hoisted outside the failover
  loop so rotation/413 cannot re-arm it; continuation waits sleep on the upstream
  signal so an SSE body-cancel aborts them too
- images/web-search loops: release the 429 body before the wait, restart the
  response-header deadline after each deliberate wait (backoffs never consume the
  connect budget or surface as 504), and record rate-limit-429 on replay sends
- config: load-time degradation for invalid optional retryOn429 fields (warn +
  drop the field) instead of tripping the whole schema and hiding all providers
  behind a default config; the management write boundary still rejects
- tests: fail-closed auth modes (incl. unknown), expired HTTP-date, per-request
  budget across 2-key pools (e2e + terminal continuation), byte-identical replay
  bodies/auth (passthrough + continuation), recovery telemetry in both loops,
  config load degradation, registry auth-kind preservation, typed usage-log entry
- docs: retry-wait vs total-latency and attempts+poolKeys volume bounds,
  identical-replay equivalence, deadline/backoff behavior in devlog 010 and
  structure/04; retryOn429 boundary notes in the provider guide and adapter
  reference (English + ja/ko/ru/zh-cn)
@harryzhou2000

Copy link
Copy Markdown
Author

All 17 inline review comments are addressed in 58c36c9 (plus docstrings in 0feede1 for the coverage check). Highlights: fail-closed auth (registry now preserves authMode "local"), expired Retry-After dates retry immediately, unread 429 bodies are released before every backoff, rate-limit-429 is recorded on every retry surface, bridge loops restart their header deadline after each wait, the continuation budget is per-request, invalid optional retryOn429 fields degrade at load instead of discarding the config, and replay identity is asserted byte-for-byte in tests. Replies were posted on each thread; the 7 open threads are resolved. The PR remains a draft.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/server/responses/core.ts (1)

2647-2685: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift

Emit upstream heartbeats during 429 backoff

The bridge checks upstream activity every 2,000 ms. It increments stallTicks when no adapter event arrives and aborts at ceil(stallTimeoutSec * 1000 / 2000) ticks. Wire heartbeats do not reset this counter.

The retry waits can be silent for up to 600,000 ms. This exceeds the default 300-second core budget and the default 330-second web-search budget.

Apply the fix to these live generators:

  • src/server/responses/core.ts:2647-2685
  • src/web-search/loop.ts:363-393
  • src/images/loop.ts:469-498

Split each sleepWithAbort call into chunks shorter than 2,000 ms, such as 1,000 ms. Yield { type: "heartbeat" } after every chunk, including the final chunk. Add regression coverage for a backoff longer than stallTimeoutSec. The pre-stream retry loops in src/server/responses/core.ts do not require this change.

Update the web-search timeout documentation and tests to state that retry backoff remains live through these upstream heartbeats.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/responses/core.ts` around lines 2647 - 2685, Update the live
retry-backoff loops in src/server/responses/core.ts:2647-2685,
src/web-search/loop.ts:363-393, and src/images/loop.ts:469-498 to split
sleepWithAbort waits into sub-2,000 ms chunks, yielding a heartbeat after every
chunk including the final one, while preserving abort handling; leave pre-stream
retry loops unchanged. Add regression coverage for backoffs exceeding
stallTimeoutSec, and update web-search timeout documentation and tests to
describe continued liveness through upstream heartbeats.
src/images/loop.ts (1)

469-476: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

Keep the retry budget scoped consistently.

src/images/loop.ts initializes rateLimitRetries inside prepareIterationEvents, while one runWithImageBridge request can execute multiple image-loop iterations. This can re-arm the same-key budget and exceed the documented per-request send bound.

  • src/images/loop.ts#L469-L476: move the policy and counter to request scope, or explicitly define the budget per iteration.
  • structure/04_transports-and-sidecars.md#L235-L235: update the attempts + poolKeys bound if per-iteration scope is intentional.
  • devlog/_plan/260802_429_same_target_retry/010_design.md#L95-L110: add a multi-iteration image-bridge regression test.
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 'prepareIterationEvents|rateLimitRetries|rateLimitRetryPolicy|HARD_CAP|retryOn429Policy' \
  src/images/loop.ts tests/images/loop.test.ts

As per path instructions, add a focused regression in the flat Bun tests for this src/** behavior change.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/images/loop.ts` around lines 469 - 476, Keep the 429 retry policy and
counter request-scoped in prepareIterationEvents/runWithImageBridge so multiple
image-loop iterations cannot reset the same-key retry budget; preserve the
documented per-request send bound. Update
structure/04_transports-and-sidecars.md:235 to reflect the chosen request-scoped
bound, and extend devlog/_plan/260802_429_same_target_retry/010_design.md:95-110
with a multi-iteration image-bridge regression scenario. Add a focused
regression to the flat Bun tests covering this behavior.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@devlog/_plan/260802_429_same_target_retry/010_design.md`:
- Around line 88-89: Align the design and associated tests with the current
implementation: update the expired HTTP-date and numeric Retry-After: 0 cases to
expect the configured fallback interval when the parser returns undefined,
unless the parser and rateLimitRetryDelayMs are intentionally changed to
propagate a zero delay. Keep the documented behavior, parser expectations, and
tests consistent.

In `@docs-site/src/content/docs/ja/reference/adapters.md`:
- Around line 41-43: Update the retry descriptions in
docs-site/src/content/docs/ja/reference/adapters.md:41-43,
docs-site/src/content/docs/ko/reference/adapters.md:48-50,
docs-site/src/content/docs/ru/reference/adapters.md:51-53, and
docs-site/src/content/docs/zh-cn/reference/adapters.md:46-48 to state that
same-key 429 replay occurs before other handling or failover, while preserving
the existing translated behavior and custom runTurn exception.

In `@docs-site/src/content/docs/zh-cn/guides/providers.md`:
- Around line 37-40: Update the Chinese provider guide passage describing
retryOn429 to explicitly state that it is opt-in and disabled by default unless
configured, while preserving the existing API-key-only restriction and OAuth,
forward, and local exclusions. Keep the wording consistent with the
corresponding English provider guide.

In `@src/config.ts`:
- Around line 1135-1149: Update the retryOn429 sanitization around the fields
definition and loop to iterate over all keys in policy, warning and ignoring any
key not in the recognized field set. Preserve the existing validators and
warning behavior for known fields with invalid values, and continue rebuilding
p.retryOn429 from only accepted entries.

In `@src/images/loop.ts`:
- Around line 479-486: Await the unread 429 response body cancellation in the
retry flow around sleepWithAbort, while preserving handling for already-closed
bodies and cancellation failures. Update
devlog/_plan/260802_429_same_target_retry/010_design.md lines 53-55 and
structure/04_transports-and-sidecars.md lines 236-237 only as needed to keep
their “release before backoff” claims accurate after the implementation change.
- Around line 482-498: The retry flow around sleepWithAbort in
src/images/loop.ts lines 482-498 must clear headerDeadline before sleeping,
check signal.aborted immediately afterward and throw the existing 499 LoopError
before telemetry or replay, then create the replacement deadline before
onRateLimitRetrySend and fetchOnce. Update
structure/04_transports-and-sidecars.md lines 238-242 to preserve and document
the 499-before-replay guarantee; no other behavior needs changing.

---

Outside diff comments:
In `@src/images/loop.ts`:
- Around line 469-476: Keep the 429 retry policy and counter request-scoped in
prepareIterationEvents/runWithImageBridge so multiple image-loop iterations
cannot reset the same-key retry budget; preserve the documented per-request send
bound. Update structure/04_transports-and-sidecars.md:235 to reflect the chosen
request-scoped bound, and extend
devlog/_plan/260802_429_same_target_retry/010_design.md:95-110 with a
multi-iteration image-bridge regression scenario. Add a focused regression to
the flat Bun tests covering this behavior.

In `@src/server/responses/core.ts`:
- Around line 2647-2685: Update the live retry-backoff loops in
src/server/responses/core.ts:2647-2685, src/web-search/loop.ts:363-393, and
src/images/loop.ts:469-498 to split sleepWithAbort waits into sub-2,000 ms
chunks, yielding a heartbeat after every chunk including the final one, while
preserving abort handling; leave pre-stream retry loops unchanged. Add
regression coverage for backoffs exceeding stallTimeoutSec, and update
web-search timeout documentation and tests to describe continued liveness
through upstream heartbeats.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9881d8f6-9808-4382-a953-a3a067cfc38b

📥 Commits

Reviewing files that changed from the base of the PR and between a06a416 and 0feede1.

📒 Files selected for processing (26)
  • devlog/_plan/260802_429_same_target_retry/010_design.md
  • docs-site/src/content/docs/guides/providers.md
  • docs-site/src/content/docs/ja/guides/providers.md
  • docs-site/src/content/docs/ja/reference/adapters.md
  • docs-site/src/content/docs/ko/guides/providers.md
  • docs-site/src/content/docs/ko/reference/adapters.md
  • docs-site/src/content/docs/reference/adapters.md
  • docs-site/src/content/docs/ru/guides/providers.md
  • docs-site/src/content/docs/ru/reference/adapters.md
  • docs-site/src/content/docs/zh-cn/guides/providers.md
  • docs-site/src/content/docs/zh-cn/reference/adapters.md
  • src/config.ts
  • src/images/loop.ts
  • src/providers/derive.ts
  • src/providers/key-failover.ts
  • src/server/responses/core.ts
  • src/web-search/loop.ts
  • structure/04_transports-and-sidecars.md
  • tests/config-user-edits.test.ts
  • tests/images/loop.test.ts
  • tests/provider-registry-parity.test.ts
  • tests/rate-limit-retry.test.ts
  • tests/server-rate-limit-retry-e2e.test.ts
  • tests/terminal-guard-server.test.ts
  • tests/usage-log.test.ts
  • tests/web-search.test.ts

Comment thread devlog/_plan/260802_429_same_target_retry/010_design.md
Comment thread docs-site/src/content/docs/ja/reference/adapters.md Outdated
Comment thread docs-site/src/content/docs/zh-cn/guides/providers.md
Comment thread src/config.ts Outdated
Comment thread src/images/loop.ts
Comment thread src/images/loop.ts Outdated
…ne race, config warnings, locale docs)

- images/web-search loops: AWAIT the unread 429 body cancellation before the
  backoff; clear the old header deadline BEFORE the sleep; re-check client
  cancellation after the wait so 499 wins over stale-deadline edges; start the
  fresh deadline before telemetry and replay
- config: sanitizeRetryOn429ForLoad warns about misnamed keys (e.g. attempt)
  instead of silently dropping them
- docs: locale adapter pages state same-key replay runs before other
  handling/failover; provider guide (English + ja/ko/ru/zh-cn) states retryOn429
  is opt-in (absent = off); devlog and structure/04 note the awaited
  cancellation and the 499-before-replay guarantee
- test: config-user-edits covers the misnamed-key drop
@harryzhou2000
harryzhou2000 marked this pull request as draft August 1, 2026 19:50
@harryzhou2000
harryzhou2000 marked this pull request as ready for review August 1, 2026 19:57
@harryzhou2000
harryzhou2000 marked this pull request as draft August 1, 2026 19:59

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
devlog/_plan/260802_429_same_target_retry/010_design.md (1)

47-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Document the runTurn exception for bridge retries.

src/images/loop.ts enters the adapter.runTurn branch at Lines 355-433 and returns before the HTTP 429 retry loop at Lines 477-513. A key-auth custom runTurn adapter therefore does not receive this HTTP retry policy. State that bridge retries apply to HTTP adapters only and exclude custom runTurn transports.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@devlog/_plan/260802_429_same_target_retry/010_design.md` around lines 47 -
53, Update the retry-policy documentation near the bridge retry references to
state that image/video bridge retries apply only to HTTP adapters. Explicitly
exclude custom adapters using the adapter.runTurn path in src/images/loop.ts,
which returns before the HTTP 429 retry loop; do not imply that runTurn
transports receive the same wait-and-replay behavior.
src/server/responses/core.ts (1)

2668-2707: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Await terminal-continuation body cancellation before the backoff.

At Line 2679, response.body.cancel() is detached with void. sleepWithAbort() then starts at Line 2681 while cancellation can still be pending. Under repeated 429 responses, unread bodies can remain active through the retry wait and replay.

Await cancellation before sleeping.

Proposed fix
-        try { void response.body?.cancel().catch(() => {}); } catch { /* already closed */ }
+        try { await response.body?.cancel().catch(() => {}); } catch { /* already closed */ }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/responses/core.ts` around lines 2668 - 2707, In the
terminal-continuation retry loop, update the response body cleanup before
sleepWithAbort to await response.body cancellation rather than detaching the
promise. Preserve the existing safe handling for absent or already-closed
bodies, and ensure the backoff starts only after cancellation has settled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@devlog/_plan/260802_429_same_target_retry/010_design.md`:
- Around line 47-53: Update the retry-policy documentation near the bridge retry
references to state that image/video bridge retries apply only to HTTP adapters.
Explicitly exclude custom adapters using the adapter.runTurn path in
src/images/loop.ts, which returns before the HTTP 429 retry loop; do not imply
that runTurn transports receive the same wait-and-replay behavior.

In `@src/server/responses/core.ts`:
- Around line 2668-2707: In the terminal-continuation retry loop, update the
response body cleanup before sleepWithAbort to await response.body cancellation
rather than detaching the promise. Preserve the existing safe handling for
absent or already-closed bodies, and ensure the backoff starts only after
cancellation has settled.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 72915daa-eace-482c-83cd-3c1d098cc169

📥 Commits

Reviewing files that changed from the base of the PR and between 0feede1 and 1af83c3.

📒 Files selected for processing (24)
  • devlog/_plan/260802_429_same_target_retry/010_design.md
  • docs-site/src/content/docs/guides/providers.md
  • docs-site/src/content/docs/ja/guides/providers.md
  • docs-site/src/content/docs/ja/reference/adapters.md
  • docs-site/src/content/docs/ko/guides/providers.md
  • docs-site/src/content/docs/ko/reference/adapters.md
  • docs-site/src/content/docs/ru/guides/providers.md
  • docs-site/src/content/docs/ru/reference/adapters.md
  • docs-site/src/content/docs/zh-cn/guides/providers.md
  • docs-site/src/content/docs/zh-cn/reference/adapters.md
  • src/config.ts
  • src/images/loop.ts
  • src/providers/derive.ts
  • src/providers/key-failover.ts
  • src/server/responses/core.ts
  • src/types.ts
  • src/web-search/loop.ts
  • structure/04_transports-and-sidecars.md
  • tests/config-user-edits.test.ts
  • tests/images/loop.test.ts
  • tests/server-rate-limit-retry-e2e.test.ts
  • tests/terminal-guard-server.test.ts
  • tests/usage-log.test.ts
  • tests/web-search.test.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1af83c39cb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/images/loop.ts Outdated
Comment thread src/config.ts
Comment thread src/images/loop.ts Outdated
Comment thread src/config.ts Outdated
… send telemetry, invalid master switch, secret-safe warnings)
@harryzhou2000
harryzhou2000 marked this pull request as ready for review August 1, 2026 20:17

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4e1978821

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/config.ts Outdated
Comment thread src/server/responses/core.ts Outdated
…ncel, post-sleep abort re-checks, pinned xAI req-id)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/server/responses/core.ts (1)

2617-2666: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reduce the duplicated recovery-label expression in fetchContinuation.

Lines 2638 and 2647 both derive the same label from replay. The second one additionally merges the transient-retry kind. The logic is correct, but the label rule now lives in two places, so a future change to the label (for example a distinct kind for continuation replays) can easily update only one branch.

♻️ Optional consolidation
       if (continuationEstimate !== undefined) logCtx.usageLogInputTokens = continuationEstimate;
+      const replayKind: AttemptRecoveryKind | undefined = replay ? "rate-limit-429" : undefined;
       try {
         if (activeAdapter.fetchResponse) {
-          noteAttemptSend(logCtx.activeAttempt, continuationEstimate, replay ? "rate-limit-429" : undefined);
+          noteAttemptSend(logCtx.activeAttempt, continuationEstimate, replayKind);
           return await activeAdapter.fetchResponse(continuationRequest, {
@@
         return await fetchWithResetRetry(
           recovery => {
-            noteAttemptSend(logCtx.activeAttempt, continuationEstimate, recovery ?? (replay ? "rate-limit-429" : undefined));
+            noteAttemptSend(logCtx.activeAttempt, continuationEstimate, recovery ?? replayKind);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/responses/core.ts` around lines 2617 - 2666, Consolidate the
replay-derived recovery label in fetchContinuation by computing it once before
the fetch branches, then reuse that value in both noteAttemptSend calls while
preserving the fetchWithResetRetry recovery override behavior.
src/config.ts (1)

1144-1158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Include the accepted range in the warning for out-of-range numbers.

The validators at Lines 1146-1148 reject both wrong types and out-of-range numbers, but the warning at Line 1157 reports only typeof value. A user who writes "attempts": 999 sees providers.x.retryOn429.attempts (number) is invalid, which gives no hint about the accepted bound. Numbers and booleans cannot carry secret material, so the range can be stated safely while the value itself stays unlogged.

♻️ Proposed diagnostic improvement
-    const fields: Array<[string, (value: unknown) => boolean]> = [
-      ["enabled", value => typeof value === "boolean"],
-      ["attempts", value => typeof value === "number" && Number.isInteger(value) && value >= 1 && value <= 20],
-      ["intervalMs", value => typeof value === "number" && Number.isInteger(value) && value >= 100 && value <= 600_000],
-      ["maxIntervalMs", value => typeof value === "number" && Number.isInteger(value) && value >= 100 && value <= 600_000],
-      ["respectRetryAfter", value => typeof value === "boolean"],
+    const fields: Array<[string, (value: unknown) => boolean, string]> = [
+      ["enabled", value => typeof value === "boolean", "expected boolean"],
+      ["attempts", value => typeof value === "number" && Number.isInteger(value) && value >= 1 && value <= 20, "expected integer 1-20"],
+      ["intervalMs", value => typeof value === "number" && Number.isInteger(value) && value >= 100 && value <= 600_000, "expected integer 100-600000"],
+      ["maxIntervalMs", value => typeof value === "number" && Number.isInteger(value) && value >= 100 && value <= 600_000, "expected integer 100-600000"],
+      ["respectRetryAfter", value => typeof value === "boolean", "expected boolean"],
     ];
     const cleaned: Record<string, unknown> = {};
-    for (const [key, isValid] of fields) {
+    for (const [key, isValid, expectation] of fields) {
       const value = policyRecord[key];
       if (value === undefined) continue;
       if (isValid(value)) cleaned[key] = value;
       // Log only the received type, never the value (provider config can hold secrets).
-      else console.warn(`⚠️  config.json providers.${name}.retryOn429.${key} (${typeof value}) is invalid — ignoring the field`);
+      else console.warn(`⚠️  config.json providers.${name}.retryOn429.${key} (${typeof value}) is invalid — ${expectation}; ignoring the field`);
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/config.ts` around lines 1144 - 1158, Update the validation warning in the
retry policy field loop around the `fields` validators to include the accepted
ranges for numeric keys such as `attempts`, `intervalMs`, and `maxIntervalMs`,
while retaining type-only reporting and never logging the received value. Keep
boolean warnings unchanged and preserve the existing validation and
field-cleaning behavior.
src/images/loop.ts (1)

483-515: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Bound pre-header 429 retries before creating SSE

For non-runTurn image requests and all web-search requests, prepareIterationDrained consumes the retry heartbeat before bridgeToResponsesSSE is created. The client receives no response headers while same-target 429 retries wait. The default policy can wait 3 minutes; valid configuration can wait up to 200 minutes (20 × 600_000ms). Bound the aggregate eager-phase wait to the request header budget, or move these retries into the live produce() phase. Apply the fix to both loops. runTurn image requests already skip the eager drain.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/images/loop.ts` around lines 483 - 515, Bound same-target 429 retry
waiting during the eager preparation phase so it cannot exceed the request
header-timeout budget before SSE creation, or defer the retries into the live
produce phase. Apply the corresponding fix to the retry loop in
src/images/loop.ts lines 483-515 and src/web-search/loop.ts lines 380-412;
preserve the existing runTurn image behavior, which already skips eager
draining.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/providers/xai-transport.ts`:
- Around line 135-138: Rename the second parameter of withGeneratedRequestId to
reflect that it receives the already-resolved request ID, whether configured or
generated. Update all references within the helper while preserving the single
fallback in the request setup where configuredRequestId ?? randomUUID() is
computed, ensuring retries continue using the same ID.

In `@tests/config-user-edits.test.ts`:
- Around line 183-185: In the test assertion for the retryOn429 diagnostic,
replace the loose "string" substring check with an assertion anchored to the
retryOn429 field and its parenthesized received type. Keep the secret-redaction
assertion unchanged and ensure the expectation specifically verifies the
type-only warning from loadConfig().

---

Outside diff comments:
In `@src/config.ts`:
- Around line 1144-1158: Update the validation warning in the retry policy field
loop around the `fields` validators to include the accepted ranges for numeric
keys such as `attempts`, `intervalMs`, and `maxIntervalMs`, while retaining
type-only reporting and never logging the received value. Keep boolean warnings
unchanged and preserve the existing validation and field-cleaning behavior.

In `@src/images/loop.ts`:
- Around line 483-515: Bound same-target 429 retry waiting during the eager
preparation phase so it cannot exceed the request header-timeout budget before
SSE creation, or defer the retries into the live produce phase. Apply the
corresponding fix to the retry loop in src/images/loop.ts lines 483-515 and
src/web-search/loop.ts lines 380-412; preserve the existing runTurn image
behavior, which already skips eager draining.

In `@src/server/responses/core.ts`:
- Around line 2617-2666: Consolidate the replay-derived recovery label in
fetchContinuation by computing it once before the fetch branches, then reuse
that value in both noteAttemptSend calls while preserving the
fetchWithResetRetry recovery override behavior.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3f71039b-4261-460f-853e-cf5dc91ff470

📥 Commits

Reviewing files that changed from the base of the PR and between 1af83c3 and 58bf694.

📒 Files selected for processing (11)
  • src/config.ts
  • src/images/loop.ts
  • src/providers/xai-transport.ts
  • src/server/responses/core.ts
  • src/web-search/loop.ts
  • tests/config-user-edits.test.ts
  • tests/images/loop.test.ts
  • tests/rate-limit-retry.test.ts
  • tests/terminal-guard-server.test.ts
  • tests/web-search.test.ts
  • tests/xai-transport.test.ts

Comment thread src/providers/xai-transport.ts
Comment thread tests/config-user-edits.test.ts Outdated
…n sanitizer/error paths

CodeRabbit round on the merge head:
- Capture initialRequest and continuationRequest in consts after their build
  blocks so fetch callbacks read narrowed values (let unions kept undefined).
- Replace the seven bare transportToken bumps with invalidateSameTargetRequest()
  so cache invalidation is structurally coupled to credential/adapter mutations.
- Drop a retryOn429 policy whose every supplied field was invalid instead of
  persisting {} (which would opt IN to retries with defaults); keep an
  intentionally empty policy.
- Redact + JSON-escape the provider name in the retryOn429 management error
  path; regression tests for both behaviors.
@harryzhou2000

Copy link
Copy Markdown
Author

@coderabbitai resume

Addressed the remaining five findings from the 12:00-12:10 round in 22ac868:

  • core.ts byte-identical replay cache (trivial): all seven bare transportToken += 1 bumps now go through invalidateSameTargetRequest(), coupling invalidation to the credential/adapter mutation sites.
  • core.ts let-union captures (trivial, both sites): initialRequest -> const builtInitialRequest after the build try/catch; continuationRequest -> const builtContinuationRequest after the if/else build block. Callback and finally reads use the narrowed consts.
  • config.ts sanitizer (major): a retryOn429 policy whose every supplied field was invalid is now deleted (with a warning) instead of persisting {}; an intentionally empty {} is still preserved as opt-in. Load test updated + a dedicated intentional-{} test added.
  • auth-cors.ts redaction (major): the retryOn429 error path emits provider ${JSON.stringify(redactSecretString(name))} ...; regression test asserts a sk-super-secret-9876 provider name never reaches the response.

Validated: typecheck, privacy scan, 132 targeted tests (retry/abort/continuation/config/management) and 255 core.ts-consumer tests all pass.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Commit and current head ==='
git rev-parse --short HEAD
git show --no-patch --format='commit=%H%nparents=%P%nsubject=%s' 22ac8685

printf '%s\n' '=== Changed files ==='
git diff-tree --no-commit-id --name-only -r 22ac8685

printf '%s\n' '=== Focused structural outline ==='
ast-grep outline src/server/responses/core.ts --items all --type function --match 'invalidateSameTargetRequest|handleResponses' --view expanded || true
ast-grep outline src/config.ts --items all --type function,variable --match 'sanitize|retryOn429|loadConfig' --view expanded || true
ast-grep outline src/server/auth-cors.ts --items all --type function --match 'providerManagementConfigError' --view expanded || true

printf '%s\n' '=== Token invalidation and narrowed request bindings ==='
rg -n -C 8 'invalidateSameTargetRequest|transportToken\s*\+=|builtInitialRequest|builtContinuationRequest|initialRequest|continuationRequest' \
  src/server/responses/core.ts

printf '%s\n' '=== Sanitizer and management redaction ==='
rg -n -C 12 'retryOn429|redactSecretString|providerManagementConfigError' \
  src/config.ts src/server/auth-cors.ts

printf '%s\n' '=== Relevant regression tests ==='
rg -n -C 10 'invalid.*retryOn429|intentionally empty|intentional.*empty|sk-super-secret-9876|redact|transportToken|buildRequest' \
  tests/config-user-edits.test.ts tests/management-provider-validation.test.ts tests/abort-race.test.ts \
  2>/dev/null || true

printf '%s\n' '=== Exact patch ==='
git show --format= --unified=45 22ac8685 -- \
  src/server/responses/core.ts \
  src/config.ts \
  src/server/auth-cors.ts \
  tests/config-user-edits.test.ts \
  tests/management-provider-validation.test.ts

Length of output: 117154


@harryzhou2000 Verified on 22ac8685.

The five findings are addressed.

  • src/server/responses/core.ts Line 2456 defines invalidateSameTargetRequest(). All seven previous transportToken += 1 sites now call it. These sites cover OAuth refresh, key rotation, Anthropic account rotation, and image-tier changes in both the main recovery loop and terminal continuation loop.
  • src/server/responses/core.ts Lines 2448-2486 use builtInitialRequest after the guarded initial build. Lines 2802-2835 use builtContinuationRequest after the continuation build. The fetch callbacks and finally blocks now use narrowed immutable bindings.
  • src/config.ts Lines 1352-1379 preserve an explicit retryOn429: {}. If supplied fields produce no valid fields, the sanitizer deletes the policy and emits a warning. This prevents malformed input such as { attempts: 0 } from enabling default retries.
  • tests/config-user-edits.test.ts covers both cases. The invalid-only policy remains disabled. The explicit empty policy enables the documented defaults.
  • src/server/auth-cors.ts Line 429 applies redactSecretString() and JSON.stringify() to the provider name in the retryOn429 error path.
  • tests/management-provider-validation.test.ts verifies that sk-super-secret-9876 does not appear in that error response.

I found no regression in the inspected changes. Normal review is resumed.

✅ Action performed

Reviews resumed.

@harryzhou2000

Copy link
Copy Markdown
Author

@coderabbitai resume

Verification on 22ac868:

  • typecheck, privacy scan, GUI lint: clean.
  • Full suite shard 1/2: 3839 pass / 0 fail.
  • Shard 2/2 residual failures (server-auth WS upgrade/auth statuses, active-registry websocket 129, claude-messages 502-vs-503, openai compact transport) reproduce identically on plain upstream/dev (6a7351b) and on the pre-fix merge head 528b455 on this loaded machine (A/B worktree runs) — they are pre-existing environment flakes, not introduced by this PR.
  • All 46 review threads resolved; the five findings from the 12:00-12:10 round are addressed and verified.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

@harryzhou2000 Acknowledged.

The reported baseline comparison supports the classification of the shard-2 failures as pre-existing environment flakes. The affected areas are WebSocket auth and upgrade status tests, active-registry WebSocket test 129, Claude Messages status handling, and OpenAI compact transport.

The clean typecheck, privacy scan, GUI lint, and shard-1 result provide additional validation for 22ac8685. Normal review is resumed.


✏️ Learnings added
Learnt from: harryzhou2000
URL: https://github.com/lidge-jun/opencodex/pull/865

Timestamp: 2026-08-03T12:43:51.618Z
Learning: For PR `#865`, residual full-suite shard-2 failures in server-auth WebSocket upgrade/auth statuses, active-registry WebSocket test 129, Claude Messages 502-versus-503 handling, and OpenAI compact transport reproduced on both upstream/dev commit 6a7351b4 and pre-fix merge head 528b4550 under a loaded machine. Treat these failures as pre-existing environment flakes unless a later change provides contrary evidence.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Reviews resumed.

Restore CodeRabbit docstring coverage above the 80% threshold for the
invalidateSameTargetRequest helper, the RECOVERY_KIND_KEYS map, and
recoveryKindKey.
@harryzhou2000

Copy link
Copy Markdown
Author

@coderabbitai resume

7a07a2f adds JSDoc to the new diff-touched declarations (invalidateSameTargetRequest, RECOVERY_KIND_KEYS, recoveryKindKey) to restore the docstring coverage check above the 80% threshold. Typecheck and GUI lint pass.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Commit metadata ==='
git show --no-patch --format='commit=%H%nparents=%P%nsubject=%s' 7a07a2fe

printf '%s\n' '=== Changed files ==='
git diff-tree --no-commit-id --name-status -r 7a07a2fe

printf '%s\n' '=== Exact patch ==='
git show --format= --unified=25 7a07a2fe -- \
  src/server/responses/core.ts \
  gui/src/pages/Logs.tsx

printf '%s\n' '=== Affected declaration contexts ==='
rg -n -C 12 \
  'invalidateSameTargetRequest|RECOVERY_KIND_KEYS|recoveryKindKey' \
  src/server/responses/core.ts \
  gui/src/pages/Logs.tsx

Length of output: 28778


@harryzhou2000 Verified in 7a07a2fe.

The commit changes only JSDoc comments.

  • src/server/responses/core.ts Lines 2453-2457 document invalidateSameTargetRequest. The comment explains the stale-credential replay risk and requires cache invalidation after credential, adapter, or parsed-request mutation.
  • gui/src/pages/Logs.tsx Lines 291-294 document RECOVERY_KIND_KEYS. The comment states that the mapping converts each AttemptRecoveryKind wire value into a localized logs-detail label.
  • gui/src/pages/Logs.tsx Lines 313-315 document recoveryKindKey. The comment states that the function maps one recovery kind to its i18n key.

The implementation and call sites are unchanged. The docstring coverage fix does not introduce a functional regression. Normal review is resumed.

✅ Action performed

Reviews resumed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/server/responses/core.ts (1)

2495-2503: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The image-tier bias discovered by the main recovery loop is lost for the terminal-guard continuation.

imageTierBias at Line 2503 is scoped to the if (!upstreamResponse.ok) block and is bumped to 1 at Line 2716 when the main flow needs a smaller image tier to clear an Anthropic 413. fetchTerminalGuardContinuation declares its own, independent imageTierBias at Line 2770, starting at 0.

Because fetchContinuation (Line 2777) keys its cache on sameTargetParsed === nextParsed, and nextParsed is always a different object from parsed, the very first continuation call always rebuilds the request (Line 2784) using the continuation's own imageTierBias, not the value the main flow already proved necessary. If the same oversized images are still present, the continuation's first attempt gets a redundant 413, and only its own shouldAttemptImageTierRetry check (Lines 2952-2962) rediscovers the tier reduction on a second attempt.

This self-recovers (matches the shared rateLimitPolicy/rateLimitRetries pattern already applied at Line 2495-2496 for the same producer/consumer boundary), so it costs one avoidable round trip and one avoidable 413 rather than a hard failure. Hoist the discovered bias the same way the retry budget was hoisted.

🐛 Proposed fix to share the discovered image tier with the continuation
   const rateLimitPolicy = rateLimitRetryPolicyFor(route.provider);
   let rateLimitRetries = 0;
+  // Shared with fetchTerminalGuardContinuation below: an image-tier reduction that let the
+  // main request clear a 413 must not be forgotten on the very next continuation build.
+  let sharedImageTierBias = 0;
   if (!upstreamResponse.ok) {
         imageRetryAttempted = true;
         imageTierBias = 1;
+        sharedImageTierBias = 1;
         invalidateSameTargetRequest();
   const fetchTerminalGuardContinuation = async function* (nextParsed: OcxParsedRequest): AsyncGenerator<AdapterEvent> {
-    let imageTierBias = 0;
+    let imageTierBias = sharedImageTierBias;
     let response: Response | undefined;

Also applies to: 2714-2718, 2769-2789

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/responses/core.ts` around lines 2495 - 2503, Hoist imageTierBias
to the same shared scope as rateLimitPolicy and rateLimitRetries, initialize it
once before the main recovery loop, and remove the independent declaration
inside fetchTerminalGuardContinuation. Keep the existing updates in the main
recovery flow and ensure fetchContinuation reuses the discovered bias when
rebuilding continuation requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/server/responses/core.ts`:
- Around line 2495-2503: Hoist imageTierBias to the same shared scope as
rateLimitPolicy and rateLimitRetries, initialize it once before the main
recovery loop, and remove the independent declaration inside
fetchTerminalGuardContinuation. Keep the existing updates in the main recovery
flow and ensure fetchContinuation reuses the discovered bias when rebuilding
continuation requests.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d59d1154-6426-452f-ade9-23763e003ccc

📥 Commits

Reviewing files that changed from the base of the PR and between e502173 and 7a07a2f.

📒 Files selected for processing (12)
  • gui/src/i18n/de.ts
  • gui/src/i18n/en.ts
  • gui/src/i18n/ja.ts
  • gui/src/i18n/ko.ts
  • gui/src/i18n/ru.ts
  • gui/src/i18n/zh.ts
  • gui/src/pages/Logs.tsx
  • src/config.ts
  • src/server/auth-cors.ts
  • src/server/responses/core.ts
  • tests/config-user-edits.test.ts
  • tests/management-provider-validation.test.ts

@harryzhou2000

Copy link
Copy Markdown
Author

@coderabbitai The Docstring Coverage pre-merge check currently reports 64.86% (below the 80% threshold) but does not list the missing declarations. Please list the exact functions/declarations that need docstrings so we can fix them.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Current commit and docstring-related changed files ==='
git rev-parse --short HEAD
git diff --name-only "$(git merge-base HEAD upstream/dev 2>/dev/null || git merge-base HEAD origin/dev 2>/dev/null || true)" HEAD \
  | rg '\.(ts|tsx|js|jsx|mjs|cjs)$' || true

printf '%s\n' '=== Locate coverage configuration and references ==='
fd -HI -t f . \
  | rg -i 'docstring|coverage|workflow|action|biome|eslint|package\.json|readme' \
  | head -300

rg -n -i -C 4 'docstring.{0,30}coverage|coverage.{0,30}docstring|jsdoc|tsdoc' \
  .github package.json pnpm-workspace.yaml turbo.json . 2>/dev/null \
  -g '!node_modules/**' -g '!dist/**' -g '!build/**' \
  | head -800 || true

printf '%s\n' '=== Declarations in diff-touched TypeScript and TSX files ==='
base="$(git merge-base HEAD upstream/dev 2>/dev/null || git merge-base HEAD origin/dev 2>/dev/null)"
git diff --name-only "$base" HEAD | rg '\.(ts|tsx)$' | while IFS= read -r file; do
  printf '\n--- %s ---\n' "$file"
  ast-grep outline "$file" --items all --type function,class,interface,type,variable,method --view expanded 2>/dev/null || true
done

Length of output: 9617


🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="lidge-jun/opencodex"
pr="865"

printf '%s\n' '=== PR revisions and changed files ==='
gh pr view "$pr" --repo "$repo" --json baseRefOid,headRefOid,baseRefName,headRefName,commits,files \
  --jq '{baseRefName, headRefName, baseRefOid, headRefOid, commits: [.commits[] | {oid: .oid, messageHeadline: .messageHeadline}], files: [.files[].path]}'

printf '%s\n' '=== PR checks ==='
gh pr checks "$pr" --repo "$repo" --json name,state,link,workflow,bucket 2>/dev/null || true

printf '%s\n' '=== Relevant package scripts ==='
node -e '
const p = require("./package.json");
for (const [name, cmd] of Object.entries(p.scripts ?? {})) {
  if (/doc|string|cover|check|lint/i.test(name + " " + cmd)) console.log(`${name}: ${cmd}`);
}
'

printf '%s\n' '=== Workflow command references ==='
rg -n -i -C 5 'docstring|jsdoc|tsdoc|coverage' .github/workflows package.json gui/package.json \
  -g '*.yml' -g '*.yaml' -g 'package.json' || true

printf '%s\n' '=== Candidate checker/configuration files ==='
fd -HI -t f . \
  -E node_modules -E dist -E build \
  | rg -i 'doc|string|cover|quality|check|audit' \
  | head -500

Length of output: 34718


⚙️ CodeRabbit configuration inspected

Length of output: 7440


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
head="7a07a2fe8e1a53c9767968cae20470ffea574624"

printf '%s\n' '=== Base availability and TypeScript files changed by this PR ==='
git cat-file -e "${base}^{commit}"
git diff --name-only "$base" "$head" -- \
  'src/**/*.ts' 'gui/src/**/*.ts' 'gui/src/**/*.tsx' \
  | sort

printf '%s\n' '=== Added and modified declaration hunks ==='
git diff --unified=0 "$base" "$head" -- \
  'src/**/*.ts' 'gui/src/**/*.ts' 'gui/src/**/*.tsx' \
  | rg -n -C 2 '^\+.*\b(export\s+)?(async\s+)?function\b|^\+.*\b(class|interface|type)\b|^\+.*\b(const|let)\s+[A-Za-z_$][A-Za-z0-9_$]*\s*=|^\+.*\b[A-Za-z_$][A-Za-z0-9_$]*\s*\([^;{}]*\)\s*\{' \
  || true

printf '%s\n' '=== Current outlines for changed production and GUI files ==='
git diff --name-only "$base" "$head" -- \
  'src/**/*.ts' 'gui/src/**/*.ts' 'gui/src/**/*.tsx' \
  | while IFS= read -r file; do
      printf '\n--- %s ---\n' "$file"
      ast-grep outline "$file" --items all --view expanded 2>/dev/null || true
    done

printf '%s\n' '=== Diff sections with immediate source context ==='
git diff --unified=8 "$base" "$head" -- \
  src/config.ts src/images/loop.ts src/lib/upstream-retry.ts \
  src/providers/derive.ts src/providers/key-failover.ts src/providers/xai-transport.ts \
  src/server/auth-cors.ts src/server/responses/core.ts src/types.ts src/usage/log.ts \
  src/web-search/loop.ts gui/src/pages/Logs.tsx \
  | sed -n '1,2600p'

Length of output: 165012


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
head="7a07a2fe8e1a53c9767968cae20470ffea574624"

python3 - <<'PY'
import re
import subprocess
from pathlib import Path

base = "6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
head = "7a07a2fe8e1a53c9767968cae20470ffea574624"

files = subprocess.check_output(
    ["git", "diff", "--name-only", base, head],
    text=True,
).splitlines()
files = [f for f in files if re.search(r"\.(?:ts|tsx)$", f)]

# Obtain exact added-line ranges from zero-context hunks.
ranges = {}
for f in files:
    diff = subprocess.check_output(
        ["git", "diff", "--unified=0", base, head, "--", f],
        text=True,
    )
    for m in re.finditer(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@", diff, re.M):
        start = int(m.group(1))
        count = int(m.group(2) or "1")
        if count:
            ranges.setdefault(f, []).append((start, start + count - 1))

# Match declaration lines that are added directly in this PR.
patterns = [
    re.compile(r"^\s*(?:export\s+)?(?:declare\s+)?(?:abstract\s+)?(?:async\s+)?function\s+([A-Za-z_$][\w$]*)\b"),
    re.compile(r"^\s*(?:export\s+)?(?:default\s+)?(?:abstract\s+)?(class|interface|type|enum|namespace)\s+([A-Za-z_$][\w$]*)\b"),
    re.compile(r"^\s*(?:export\s+)?(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*(?::[^=]+)?="),
    re.compile(r"^\s*(?:export\s+)?(?:async\s+)?function\*\s+([A-Za-z_$][\w$]*)\b"),
]

def is_added(f, line):
    return any(a <= line <= b for a, b in ranges.get(f, []))

def attached_doc(lines, n):
    # Skip blank lines directly before the declaration. A JSDoc block must then end on the
    # prior non-blank line and use /** rather than ordinary // comments.
    i = n - 2
    while i >= 0 and not lines[i].strip():
        i -= 1
    if i < 0 or not lines[i].rstrip().endswith("*/"):
        return False, ""
    end = i
    while i >= 0 and "/**" not in lines[i]:
        i -= 1
    if i < 0:
        return False, ""
    return True, " ".join(x.strip(" \t/*") for x in lines[i:end + 1]).strip()

print("=== Added declaration candidates and attached JSDoc status ===")
count = 0
for f in files:
    path = Path(f)
    if not path.exists():
        continue
    lines = path.read_text(encoding="utf-8").splitlines()
    for idx, line in enumerate(lines, 1):
        if not is_added(f, idx):
            continue
        name = None
        for pat in patterns:
            m = pat.match(line)
            if m:
                name = m.group(m.lastindex)
                break
        if not name:
            continue
        has_doc, doc = attached_doc(lines, idx)
        count += 1
        print(f"{f}:Line {idx}: {name}: {'JSDOC' if has_doc else 'MISSING'}")
        if doc:
            print(f"  {doc[:300]}")
print(f"=== Added declaration candidates: {count} ===")

print("\n=== Existing exported declarations whose bodies or members changed ===")
# For each changed file, show current exported symbols with line ranges. This captures
# declarations changed internally, not only headers added by the PR.
for f in files:
    print(f"\n--- {f} ---")
    subprocess.run(
        ["ast-grep", "outline", f, "--items", "all", "--view", "expanded"],
        check=False,
    )
PY

printf '%s\n' '=== Direct source context for likely undocumented diff declarations ==='
rg -n -C 5 \
  '^(export )?(const (en|de|ja|ko|ru|zh|DEFAULT_RATE_LIMIT_RETRY)|type AttemptRecoveryKind|interface (ImageBridgeDeps|WebSearchLoopDeps)|function providerManagementConfigError)' \
  gui/src/i18n/en.ts gui/src/i18n/de.ts gui/src/i18n/ja.ts gui/src/i18n/ko.ts gui/src/i18n/ru.ts gui/src/i18n/zh.ts \
  gui/src/pages/Logs.tsx src/images/loop.ts src/web-search/loop.ts src/providers/key-failover.ts src/server/auth-cors.ts src/usage/log.ts

Length of output: 111601


@harryzhou2000 I reconstructed the 64.86% result as 24 documented declarations out of 37 assessed declarations.

These 13 declarations do not have an attached /** ... */ JSDoc block on head 7a07a2fe:

File Declaration Line
gui/src/i18n/en.ts en Line 3
gui/src/i18n/de.ts de Line 4
gui/src/i18n/ja.ts ja Line 3
gui/src/i18n/ko.ts ko Line 3
gui/src/i18n/ru.ts ru Line 3
gui/src/i18n/zh.ts zh Line 3
gui/src/pages/Logs.tsx AttemptRecoveryKind Line 79
src/config.ts providerConfigSchema Line 571
src/images/loop.ts ImageBridgeDeps Line 207
src/providers/key-failover.ts DEFAULT_RATE_LIMIT_RETRY Line 25
src/server/auth-cors.ts providerManagementConfigError Line 394
src/usage/log.ts AttemptRecoveryKind Line 10
src/web-search/loop.ts WebSearchLoopDeps Line 211

The locale files already have // comments. The coverage check requires a JSDoc block. Convert or add a block directly above each exported locale object.

For example:

/** English i18n catalog. This object defines the compile-checked `TKey` set. */
export const en = {

Add concise JSDoc to the remaining declarations. This should document all 37 assessed declarations and raise the reconstructed coverage to 100%.

🐇 (\_/ )

You are interacting with an AI system.

CodeRabbit docstring-coverage round: add JSDoc to the six locale catalogs,
both AttemptRecoveryKind types, providerConfigSchema, ImageBridgeDeps,
WebSearchLoopDeps, DEFAULT_RATE_LIMIT_RETRY, and providerManagementConfigError,
bringing assessed coverage to 37/37.
@harryzhou2000

Copy link
Copy Markdown
Author

@coderabbitai resume

2bd814d adds the missing JSDoc blocks for all 13 declarations you listed (six locale catalogs, both AttemptRecoveryKind types, providerConfigSchema, ImageBridgeDeps, WebSearchLoopDeps, DEFAULT_RATE_LIMIT_RETRY, providerManagementConfigError) — assessed coverage should now be 37/37. Typecheck and GUI lint pass.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
gui/src/i18n/ko.ts (1)

577-579: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use Korean wording for request-rate limits.

요금 제한 describes a fee or charge restriction. These labels describe HTTP request throttling. Use wording consistent with the existing pws.healthLabel.rateLimited translation.

Proposed wording
-  "logs.detail.attempt.recovery.key429": "키 요금 제한 (429)",
-  "logs.detail.attempt.recovery.rateLimit429": "요금 제한 (429)",
-  "logs.detail.attempt.recovery.anthropicOauth429": "Anthropic OAuth 요금 제한 (429)",
+  "logs.detail.attempt.recovery.key429": "키 요청 한도 초과 (429)",
+  "logs.detail.attempt.recovery.rateLimit429": "요청 한도 초과 (429)",
+  "logs.detail.attempt.recovery.anthropicOauth429": "Anthropic OAuth 요청 한도 초과 (429)",
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@gui/src/i18n/ko.ts` around lines 577 - 579, Update the three recovery labels
under logs.detail.attempt.recovery.key429, rateLimit429, and anthropicOauth429
to use the Korean request-throttling wording established by
pws.healthLabel.rateLimited instead of 요금 제한, while preserving the existing 429
context and Anthropic OAuth naming.
gui/src/pages/Logs.tsx (1)

317-323: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Add a localized fallback for unknown recovery kinds.

validCachedLogs checks only top-level fields. A stale or malformed cached row can contain a recovery kind that is not in RECOVERY_KIND_KEYS. The lookup then returns undefined, and t(recoveryKindKey(kind)) can break the log detail dialog or render an invalid label.

Add a localized fallback or validate nested cached recovery kinds before this lookup. Add the fallback key to every locale catalog.

As per path instructions, GUI user-visible strings must go through i18n locale files.

Proposed fix
 function recoveryKindKey(kind: AttemptRecoveryKind) {
-  return RECOVERY_KIND_KEYS[kind];
+  return RECOVERY_KIND_KEYS[kind] ?? "logs.detail.attempt.recovery.unknown";
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@gui/src/pages/Logs.tsx` around lines 317 - 323, Update recoveryKindKey to
return a valid localized fallback key when kind is absent from
RECOVERY_KIND_KEYS, and add that fallback translation key to every locale
catalog. Ensure t(recoveryKindKey(kind)) always receives a defined key without
changing valid recovery-kind mappings.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@gui/src/i18n/ko.ts`:
- Around line 577-579: Update the three recovery labels under
logs.detail.attempt.recovery.key429, rateLimit429, and anthropicOauth429 to use
the Korean request-throttling wording established by pws.healthLabel.rateLimited
instead of 요금 제한, while preserving the existing 429 context and Anthropic OAuth
naming.

In `@gui/src/pages/Logs.tsx`:
- Around line 317-323: Update recoveryKindKey to return a valid localized
fallback key when kind is absent from RECOVERY_KIND_KEYS, and add that fallback
translation key to every locale catalog. Ensure t(recoveryKindKey(kind)) always
receives a defined key without changing valid recovery-kind mappings.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c1b83b64-2d6a-4b5f-8de9-c25fcf014b51

📥 Commits

Reviewing files that changed from the base of the PR and between 7a07a2f and 2bd814d.

📒 Files selected for processing (13)
  • gui/src/i18n/de.ts
  • gui/src/i18n/en.ts
  • gui/src/i18n/ja.ts
  • gui/src/i18n/ko.ts
  • gui/src/i18n/ru.ts
  • gui/src/i18n/zh.ts
  • gui/src/pages/Logs.tsx
  • src/config.ts
  • src/images/loop.ts
  • src/providers/key-failover.ts
  • src/server/auth-cors.ts
  • src/usage/log.ts
  • src/web-search/loop.ts

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Commit and current head ==='
git rev-parse --short HEAD
git show --no-patch --format='commit=%H%nsubject=%s' 2bd814db
git diff-tree --no-commit-id --name-status -r 2bd814db

printf '%s\n' '=== Exact patch ==='
git show --format= --unified=12 2bd814db -- \
  gui/src/i18n/en.ts gui/src/i18n/de.ts gui/src/i18n/ja.ts \
  gui/src/i18n/ko.ts gui/src/i18n/ru.ts gui/src/i18n/zh.ts \
  gui/src/pages/Logs.tsx src/config.ts src/images/loop.ts \
  src/providers/key-failover.ts src/server/auth-cors.ts src/usage/log.ts \
  src/web-search/loop.ts

printf '%s\n' '=== Required declaration contexts ==='
rg -n -C 6 \
  'export const (en|de|ja|ko|ru|zh)\b|type AttemptRecoveryKind\b|const providerConfigSchema\b|interface ImageBridgeDeps\b|const DEFAULT_RATE_LIMIT_RETRY\b|function providerManagementConfigError\b|interface WebSearchLoopDeps\b' \
  gui/src/i18n/en.ts gui/src/i18n/de.ts gui/src/i18n/ja.ts \
  gui/src/i18n/ko.ts gui/src/i18n/ru.ts gui/src/i18n/zh.ts \
  gui/src/pages/Logs.tsx src/config.ts src/images/loop.ts \
  src/providers/key-failover.ts src/server/auth-cors.ts src/usage/log.ts \
  src/web-search/loop.ts

Length of output: 24780


@harryzhou2000 Verified on 2bd814db.

All 13 previously missing declarations now have directly attached JSDoc blocks:

  • The six locale catalogs in gui/src/i18n/en.ts, gui/src/i18n/de.ts, gui/src/i18n/ja.ts, gui/src/i18n/ko.ts, gui/src/i18n/ru.ts, and gui/src/i18n/zh.ts.
  • AttemptRecoveryKind in gui/src/pages/Logs.tsx and src/usage/log.ts.
  • providerConfigSchema in src/config.ts.
  • ImageBridgeDeps in src/images/loop.ts.
  • WebSearchLoopDeps in src/web-search/loop.ts.
  • DEFAULT_RATE_LIMIT_RETRY in src/providers/key-failover.ts.
  • providerManagementConfigError in src/server/auth-cors.ts.

The JSDoc blocks are adjacent to their declarations. I found no functional changes in this commit. The docstring coverage finding is addressed. Normal review is resumed.

✅ Action performed

Reviews resumed.

…and docs

- core.ts: hoist imageTierBias to shared scope so a 413-driven tier reduction
  survives into the terminal-guard continuation (avoid redundant 413 round trip);
  consolidate the replay-derived recovery label into one replayKind value.
- gui Logs.tsx + locales: localized fallback for unknown recovery kinds
  (logs.detail.attempt.recovery.unknown) in every locale; use the established
  Korean request-throttling wording for the 429 recovery labels.
- config.ts: sanitize retryOn429 in configDiagnosticsFromRaw before schema
  validation so an invalid hand-edit cannot push the diagnostics path to a
  default fallback that the config command would persist over user providers;
  regression test added.
- docs: restore leading pipes on all malformed ja providers.md table separator
  rows (MD055); sync the Copilot mixed-wire paragraph into ja/ko/ru/zh-cn
  guides before the Cursor sections; document the runTurn exception for bridge
  retries in the 429 design devlog.
@harryzhou2000

Copy link
Copy Markdown
Author

@coderabbitai resume

Addressed the formal-review findings (including the outside-diff ones that could not be posted inline) in d2db429 and earlier commits:

  1. imageTierBias lost for terminal-guard continuation (review on 7a07a2f, core.ts 2495-2503): hoisted imageTierBias to the shared per-request scope next to rateLimitPolicy/rateLimitRetries; the continuation now inherits a tier reduction the main loop already proved necessary, avoiding the redundant 413 round trip.
  2. ko.ts wording (review on 2bd814d): the three 429 recovery labels now use the established Korean request-throttling wording 요청 한도 초과 matching pws.healthLabel.rateLimited.
  3. Unknown recovery-kind fallback (review on 2bd814d): recoveryKindKey falls back to a new logs.detail.attempt.recovery.unknown key added in every locale.
  4. ja providers.md MD055 separators (reviews on 528b455): restored the leading pipe on all five malformed delimiter rows (2-col and 3-col), including the unchanged ones.
  5. Copilot mixed-wire paragraph sync (review on 528b455): translated and inserted the paragraph before the Cursor sections in ja/ko/ru/zh-cn guides.
  6. configDiagnosticsFromRaw sanitize (review on 4fee87b, Major): sanitizeRetryOn429ForLoad now runs before schema validation in the diagnostics path too, with a regression test asserting the fallback cannot swallow user providers.
  7. runTurn exception documentation (review on 1af83c3): the 429 design devlog now states bridge retries apply to HTTP adapters only and exclude adapter.runTurn transports.
  8. replayKind consolidation (review on 58bf694, trivial): the replay-derived label is computed once and shared by both send paths in fetchContinuation.

(The earlier Critical heartbeat finding was already addressed via sleepWithHeartbeats in all three live generators, and the sanitizer delete-on-empty finding via 22ac868.)

Validated: typecheck, privacy scan, GUI lint, 102 targeted server tests, and GUI i18n tests all pass.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs-site/src/content/docs/ja/guides/providers.md`:
- Around line 272-275: Update the modelAdapters reference link in
docs-site/src/content/docs/ja/guides/providers.md lines 272-275 to
/ja/reference/configuration/providers/,
docs-site/src/content/docs/ko/guides/providers.md lines 273-276 to
/ko/reference/configuration/providers/,
docs-site/src/content/docs/ru/guides/providers.md lines 285-288 to
/ru/reference/configuration/providers/, and
docs-site/src/content/docs/zh-cn/guides/providers.md lines 252-255 to
/zh-cn/reference/configuration/providers/, keeping each translated guide within
its locale tree and synchronized with the English source and locale navigation.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6a8e7726-567e-45d1-9c42-a73ec62b1617

📥 Commits

Reviewing files that changed from the base of the PR and between 2bd814d and d2db429.

📒 Files selected for processing (15)
  • devlog/_plan/260802_429_same_target_retry/010_design.md
  • docs-site/src/content/docs/ja/guides/providers.md
  • docs-site/src/content/docs/ko/guides/providers.md
  • docs-site/src/content/docs/ru/guides/providers.md
  • docs-site/src/content/docs/zh-cn/guides/providers.md
  • gui/src/i18n/de.ts
  • gui/src/i18n/en.ts
  • gui/src/i18n/ja.ts
  • gui/src/i18n/ko.ts
  • gui/src/i18n/ru.ts
  • gui/src/i18n/zh.ts
  • gui/src/pages/Logs.tsx
  • src/config.ts
  • src/server/responses/core.ts
  • tests/config-user-edits.test.ts

Comment thread docs-site/src/content/docs/ja/guides/providers.md
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Commit and changed files ==='
git rev-parse --short HEAD
git show --no-patch --format='commit=%H%nparents=%P%nsubject=%s' d2db4295
git diff-tree --no-commit-id --name-status -r d2db4295

printf '%s\n' '=== Focused changed-file outlines ==='
for file in \
  src/server/responses/core.ts \
  src/config.ts \
  gui/src/pages/Logs.tsx \
  gui/src/i18n/ko.ts \
  docs-site/src/content/docs/ja/reference/configuration/providers.md \
  docs-site/src/content/docs/ja/guides/providers.md \
  docs-site/src/content/docs/ko/guides/providers.md \
  docs-site/src/content/docs/ru/guides/providers.md \
  docs-site/src/content/docs/zh-cn/guides/providers.md \
  devlog/_plan/260802_429_same_target_retry/010_design.md
do
  if [ -f "$file" ]; then
    printf '\n--- %s ---\n' "$file"
    ast-grep outline "$file" --items all --view expanded 2>/dev/null || true
  fi
done

printf '%s\n' '=== Exact relevant patch ==='
git show --format= --unified=25 d2db4295 -- \
  src/server/responses/core.ts src/config.ts gui/src/pages/Logs.tsx gui/src/i18n/ko.ts \
  docs-site/src/content/docs/ja/reference/configuration/providers.md \
  docs-site/src/content/docs/ja/guides/providers.md \
  docs-site/src/content/docs/ko/guides/providers.md \
  docs-site/src/content/docs/ru/guides/providers.md \
  docs-site/src/content/docs/zh-cn/guides/providers.md \
  devlog/_plan/260802_429_same_target_retry/010_design.md \
  tests

printf '%s\n' '=== Implementation and test references ==='
rg -n -C 8 \
  'imageTierBias|fetchContinuation|replayKind|recoveryKindKey|recovery\.unknown|sanitizeRetryOn429ForLoad|configDiagnosticsFromRaw|runTurn|Cursor|^\|?[\s:-]+\|' \
  src/server/responses/core.ts src/config.ts gui/src/pages/Logs.tsx gui/src/i18n \
  tests devlog/_plan/260802_429_same_target_retry/010_design.md \
  docs-site/src/content/docs/ja docs-site/src/content/docs/ko \
  docs-site/src/content/docs/ru docs-site/src/content/docs/zh-cn \
  2>/dev/null || true

Length of output: 200583


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Commit metadata and exact changed paths ==='
git show --no-patch --format='commit=%H%nsubject=%s' d2db4295
git diff-tree --no-commit-id --name-only -r d2db4295

printf '%s\n' '=== d2db4295 patch only ==='
git show --format= --unified=18 d2db4295

printf '%s\n' '=== Terminal continuation state and replay label ==='
rg -n -C 12 \
  '\bimageTierBias\b|\brateLimitPolicy\b|\brateLimitRetries\b|\breplayKind\b|function fetchContinuation|const fetchContinuation' \
  src/server/responses/core.ts

printf '%s\n' '=== Retry-policy diagnostic sanitization ==='
rg -n -C 12 \
  'function sanitizeRetryOn429ForLoad|const sanitizeRetryOn429ForLoad|function configDiagnosticsFromRaw|const configDiagnosticsFromRaw|safeParse|providerConfigSchema' \
  src/config.ts

printf '%s\n' '=== Recovery-kind fallback and locale keys ==='
rg -n -C 6 \
  'RECOVERY_KIND_KEYS|function recoveryKindKey|const recoveryKindKey|recovery\.unknown|rate-limit-429|요청 한도 초과' \
  gui/src/pages/Logs.tsx gui/src/i18n/en.ts gui/src/i18n/de.ts gui/src/i18n/ja.ts \
  gui/src/i18n/ko.ts gui/src/i18n/ru.ts gui/src/i18n/zh.ts

printf '%s\n' '=== Documentation changes ==='
rg -n -C 5 \
  'runTurn|HTTP adapters|Cursor|Copilot|mixed-wire|retry' \
  devlog/_plan/260802_429_same_target_retry/010_design.md \
  docs-site/src/content/docs/ja/guides/providers.md \
  docs-site/src/content/docs/ko/guides/providers.md \
  docs-site/src/content/docs/ru/guides/providers.md \
  docs-site/src/content/docs/zh-cn/guides/providers.md \
  2>/dev/null || true

printf '%s\n' '=== Markdown table delimiter rows ==='
rg -n '^\|?[-:| ]+\|[-:| ]+\|?$' \
  docs-site/src/content/docs/ja/reference/configuration/providers.md \
  docs-site/src/content/docs/ja/guides/providers.md \
  2>/dev/null || true

printf '%s\n' '=== Relevant regression tests in d2db4295 ==='
git show --format= --name-only d2db4295 -- tests | sed '/^$/d'
git show --format= --unified=20 d2db4295 -- tests

Length of output: 159886


@harryzhou2000 Verified in d2db4295.

The formal-review findings are addressed.

  1. src/server/responses/core.ts shares imageTierBias across the request and terminal continuation. The shared state is at Line 2499. The continuation request builder applies the bias at Line 2788. A 413 tier reduction from the main request now persists into the continuation.

  2. gui/src/i18n/ko.ts uses 요청 한도 초과 for key429, rateLimit429, and anthropicOauth429 at Lines 577-579. This matches pws.healthLabel.rateLimited at Line 1486.

  3. gui/src/pages/Logs.tsx handles malformed or stale recovery kinds. recoveryKindKey() returns logs.detail.attempt.recovery.unknown at Line 323. All six locale catalogs define this key.

  4. docs-site/src/content/docs/ja/guides/providers.md restores the leading pipe for table delimiter rows. The inspected delimiter rows at Lines 12, 33, 86, 146, and 246 are valid Markdown table separators.

  5. The Japanese, Korean, Russian, and Simplified Chinese provider guides include the Copilot mixed-wire paragraph before the Cursor section. The paragraph documents the GPT-5 Responses routing and modelAdapters precedence.

  6. src/config.ts calls sanitizeRetryOn429ForLoad(parsed) at Line 1792 before configSchema.safeParse(parsed) at Line 1793 in configDiagnosticsFromRaw. The regression test in tests/config-user-edits.test.ts confirms that an invalid retryOn429 field does not cause a default-config fallback or discard the user provider.

  7. devlog/_plan/260802_429_same_target_retry/010_design.md now states that custom adapter.runTurn transports return before the HTTP 429 retry loop. The retry policy is therefore limited to HTTP adapters.

  8. src/server/responses/core.ts computes replayKind once at Line 2811. Both continuation send paths use this value at Lines 2814 and 2823.

The inspected changes match the reported fixes. Normal review is resumed.

✅ Action performed

Reviews resumed.

… tree

The translated Copilot paragraphs referenced the English /reference/... path;
use the locale-prefixed /ja|ko|ru|zh-cn/reference/configuration/providers/
links matching the rest of each guide.
@harryzhou2000

Copy link
Copy Markdown
Author

@coderabbitai resume

6c7ea9f fixes the locale-link finding from the d2db429 round: the translated Copilot paragraphs now use locale-prefixed /ja|ko|ru|zh-cn/reference/configuration/providers/ links.

Also: the Docstring Coverage pre-merge check still reports below 80% (it moved 64.86% -> 65.79% -> 64.10% across the last three rounds even after documenting all 13 declarations you listed). Please list the declarations it currently counts as missing on head 6c7ea9f so we can close it out.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="lidge-jun/opencodex"
pr="865"

printf '%s\n' '=== PR revision ==='
gh pr view "$pr" --repo "$repo" \
  --json baseRefName,baseRefOid,headRefName,headRefOid \
  --jq '{baseRefName,baseRefOid,headRefName,headRefOid}'

printf '%s\n' '=== Current checkout and reported commit ==='
git rev-parse HEAD
git show --no-patch --format='commit=%H%nparents=%P%nsubject=%s' 6c7ea9f7

printf '%s\n' '=== PR check metadata ==='
gh pr checks "$pr" --repo "$repo" --json name,state,link,workflow,bucket \
  | jq '.[] | select((.name + " " + (.workflow // "")) | test("docstring"; "i"))' || true

printf '%s\n' '=== Candidate checker and workflow files ==='
fd -HI -t f . -E node_modules -E dist -E build \
  | rg -i '(^|/)(docstring|jsdoc|tsdoc|.*coverage.*|.*quality.*|.*check.*)(\.[^/]+)?$|\.github/workflows/' \
  | head -400

printf '%s\n' '=== Checker references ==='
rg -n -i -C 6 'docstring.{0,40}coverage|coverage.{0,40}docstring|jsdoc|tsdoc' \
  .github package.json pnpm-workspace.yaml turbo.json scripts src gui 2>/dev/null \
  -g '!node_modules/**' -g '!dist/**' -g '!build/**' \
  | head -1200 || true

printf '%s\n' '=== Files changed from PR base ==='
base="$(gh pr view "$pr" --repo "$repo" --json baseRefOid --jq '.baseRefOid')"
printf 'base=%s\n' "$base"
git diff --name-only "$base" HEAD -- \
  '*.ts' '*.tsx' '*.js' '*.jsx' '*.mjs' '*.cjs' | sort

printf '%s\n' '=== Current TypeScript/TSX declaration outline in changed source files ==='
git diff --name-only "$base" HEAD -- '*.ts' '*.tsx' \
  | while IFS= read -r file; do
      printf '\n--- %s ---\n' "$file"
      ast-grep outline "$file" --items all --view expanded 2>/dev/null || true
    done

Length of output: 87238


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== PR-quality script outline ==='
ast-grep outline .github/scripts/pr-quality.cjs --items all --view expanded
ast-grep outline .github/scripts/pr-quality.test.cjs --items all --view expanded

printf '%s\n' '=== Docstring logic and threshold references ==='
rg -n -i -C 12 'docstring|jsdoc|tsdoc|coverage|declaration|threshold|80' \
  .github/scripts/pr-quality.cjs \
  .github/scripts/pr-quality.test.cjs \
  .github/workflows/pr-hygiene.yml \
  docs-site/src/content/docs/contributing/pr-quality.md \
  docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md \
  docs/superpowers/plans/2026-07-28-pr-quality-gates.md \
  2>/dev/null

printf '%s\n' '=== PR-quality script source ==='
cat -n .github/scripts/pr-quality.cjs | sed -n '1,900p'

printf '%s\n' '=== Focused docstring tests ==='
rg -n -i -C 20 'docstring|jsdoc|tsdoc|coverage' .github/scripts/pr-quality.test.cjs \
  | head -1000

printf '%s\n' '=== Workflow invocation ==='
rg -n -C 12 'pr-quality|quality' .github/workflows/pr-hygiene.yml package.json 2>/dev/null

Length of output: 46856


🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="lidge-jun/opencodex"
pr="865"
head="6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"
base="6a7351b4d9f729fcc33241cbb385a01a70b21e0e"

printf '%s\n' '=== Check runs on head ==='
gh api --paginate "repos/$repo/commits/$head/check-runs?per_page=100" \
  --jq '.check_runs[] | {name,status,conclusion,details_url,app:.app.slug,output:{title:.output.title,summary:.output.summary,text:.output.text}}' \
  || true

printf '%s\n' '=== Legacy statuses on head ==='
gh api "repos/$repo/commits/$head/status" \
  --jq '.statuses[] | {context,state,target_url,description,creator:.creator.login}' \
  || true

printf '%s\n' '=== PR comments that mention coverage or docstrings ==='
gh api --paginate "repos/$repo/issues/$pr/comments?per_page=100" \
  --jq '.[] | select((.body // "") | test("docstring|coverage"; "i")) | {user:.user.login,created_at,html_url,body}' \
  || true

printf '%s\n' '=== Review comments that mention coverage or docstrings ==='
gh api --paginate "repos/$repo/pulls/$pr/comments?per_page=100" \
  --jq '.[] | select((.body // "") | test("docstring|coverage"; "i")) | {user:.user.login,path,line,original_line,commit_id,html_url,body}' \
  || true

printf '%s\n' '=== Added or modified declaration headers in production and GUI source ==='
git diff --unified=0 "$base" "$head" -- \
  'src/**/*.ts' 'gui/src/**/*.ts' 'gui/src/**/*.tsx' \
  | rg -n -C 3 \
    '^\+.*\b(export\s+)?(async\s+)?function(\s*\*)?\s+[A-Za-z_$][A-Za-z0-9_$]*|^\+.*\b(export\s+)?(class|interface|type|enum)\s+[A-Za-z_$][A-Za-z0-9_$]*|^\+.*\b(export\s+)?(const|let|var)\s+[A-Za-z_$][A-Za-z0-9_$]*\s*[:=]|^\+.*^\s*[A-Za-z_$][A-Za-z0-9_$]*\s*\([^;]*\)\s*\{' \
  || true

printf '%s\n' '=== Public declarations whose source range intersects a changed line ==='
python3 - <<'PY'
import json
import re
import subprocess
from pathlib import Path

base = "6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
head = "6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"
files = subprocess.check_output(
    ["git", "diff", "--name-only", base, head, "--", "src/**/*.ts", "gui/src/**/*.ts", "gui/src/**/*.tsx"],
    text=True,
).splitlines()

def changed_ranges(file):
    diff = subprocess.check_output(["git", "diff", "--unified=0", base, head, "--", file], text=True)
    result = []
    for m in re.finditer(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@", diff, re.M):
        start, count = int(m.group(1)), int(m.group(2) or "1")
        if count:
            result.append((start, start + count - 1))
    return result

def intersects(start, end, ranges):
    return any(start <= b and a <= end for a, b in ranges)

# ast-grep JSON outline supplies locations without executing repository code.
for file in files:
    try:
        data = subprocess.check_output(
            ["ast-grep", "outline", file, "--items", "all", "--json=stream"],
            text=True,
            stderr=subprocess.DEVNULL,
        )
    except subprocess.CalledProcessError:
        continue
    ranges = changed_ranges(file)
    print(f"\n--- {file} ---")
    for raw in data.splitlines():
        try:
            item = json.loads(raw)
        except json.JSONDecodeError:
            continue
        # Print raw shape once if the expected location format is absent.
        text = json.dumps(item, ensure_ascii=False)
        if "range" not in item:
            continue
        r = item["range"]
        start = r["start"]["line"] + 1
        end = r["end"]["line"] + 1
        if intersects(start, end, ranges):
            name = item.get("name") or item.get("text") or item.get("kind") or "<unnamed>"
            kind = item.get("kind") or item.get("type") or ""
            print(f"{start}-{end}\t{kind}\t{name}")
PY

Length of output: 79956


⚙️ CodeRabbit configuration inspected

Length of output: 7440


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
head="6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"

printf '%s\n' '=== CodeRabbit configuration file ==='
if [ -f .coderabbit.yaml ]; then
  cat -n .coderabbit.yaml
else
  printf '%s\n' 'No .coderabbit.yaml in the checkout.'
fi

printf '%s\n' '=== Added TypeScript/TSX lines that declare named symbols ==='
python3 - <<'PY'
import re
import subprocess
from pathlib import Path

base = "6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
head = "6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"
files = subprocess.check_output(
    ["git", "diff", "--name-only", base, head, "--", "src/**/*.ts", "gui/src/**/*.ts", "gui/src/**/*.tsx"],
    text=True,
).splitlines()

patterns = [
    ("function", re.compile(r"^\s*(?:export\s+)?(?:async\s+)?function\s*\*?\s+([A-Za-z_$][\w$]*)\b")),
    ("class/interface/type/enum", re.compile(r"^\s*(?:export\s+)?(?:declare\s+)?(?:abstract\s+)?(?:class|interface|type|enum)\s+([A-Za-z_$][\w$]*)\b")),
    ("variable", re.compile(r"^\s*(?:export\s+)?(?:const|let|var)\s+([A-Za-z_$][\w$]*)\b")),
    ("method", re.compile(r"^\s*(?:async\s+)?([A-Za-z_$][\w$]*)\s*\([^;{}]*\)\s*(?::\s*[^={]+)?\s*\{")),
]

def added_lines(file):
    diff = subprocess.check_output(["git", "diff", "--unified=0", base, head, "--", file], text=True)
    current = None
    result = set()
    for line in diff.splitlines():
        m = re.match(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@", line)
        if m:
            current = int(m.group(1))
            continue
        if current is None:
            continue
        if line.startswith("+") and not line.startswith("+++"):
            result.add(current)
            current += 1
        elif line.startswith("-") and not line.startswith("---"):
            continue
        else:
            current += 1
    return result

def jsdoc_before(lines, lineno):
    i = lineno - 2
    while i >= 0 and not lines[i].strip():
        i -= 1
    if i < 0 or not lines[i].rstrip().endswith("*/"):
        return False
    while i >= 0:
        if "/**" in lines[i]:
            return True
        i -= 1
    return False

candidates = []
for file in files:
    lines = Path(file).read_text(encoding="utf-8").splitlines()
    added = added_lines(file)
    for n in sorted(added):
        line = lines[n - 1]
        for kind, pattern in patterns:
            match = pattern.match(line)
            if match:
                name = match.group(1)
                # Exclude control-flow constructs misread as methods.
                if kind == "method" and name in {"if", "for", "while", "switch", "catch"}:
                    continue
                candidates.append((file, n, kind, name, jsdoc_before(lines, n), line.strip()))
                break

for file, line, kind, name, documented, source in candidates:
    print(f"{file}:Line {line}: {kind}: {name}: {'JSDOC' if documented else 'MISSING'}")
    print(f"  {source}")
print(f"TOTAL={len(candidates)} DOCUMENTED={sum(x[4] for x in candidates)} MISSING={sum(not x[4] for x in candidates)}")
PY

printf '%s\n' '=== Current contexts for candidates without directly adjacent JSDoc ==='
python3 - <<'PY'
import re
import subprocess
from pathlib import Path

base = "6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
head = "6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"
files = subprocess.check_output(["git", "diff", "--name-only", base, head, "--", "src/**/*.ts", "gui/src/**/*.ts", "gui/src/**/*.tsx"], text=True).splitlines()

for file in files:
    diff = subprocess.check_output(["git", "diff", "--unified=0", base, head, "--", file], text=True)
    added = set()
    cur = None
    for row in diff.splitlines():
        m = re.match(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@", row)
        if m: cur = int(m.group(1)); continue
        if cur is None: continue
        if row.startswith("+") and not row.startswith("+++"): added.add(cur); cur += 1
        elif row.startswith("-") and not row.startswith("---"): pass
        else: cur += 1
    lines = Path(file).read_text(encoding="utf-8").splitlines()
    for n in sorted(added):
        line = lines[n - 1]
        if re.match(r"^\s*(?:export\s+)?(?:async\s+)?function\s*\*?\s+\w+|^\s*(?:export\s+)?(?:const|let|var)\s+\w+|^\s*(?:export\s+)?(?:class|interface|type|enum)\s+\w+", line):
            i = n - 2
            while i >= 0 and not lines[i].strip(): i -= 1
            if i < 0 or not lines[i].rstrip().endswith("*/"):
                lo, hi = max(1, n - 4), min(len(lines), n + 5)
                print(f"\n--- {file}:Line {n} ---")
                for j in range(lo, hi + 1):
                    print(f"{j}: {lines[j-1]}")
PY

Length of output: 49370


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
head="6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"

python3 - <<'PY'
import re
import subprocess
from pathlib import Path

BASE = "6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
HEAD = "6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"
files = subprocess.check_output(
    ["git", "diff", "--name-only", BASE, HEAD, "--", "src/**/*.ts", "gui/src/**/*.ts", "gui/src/**/*.tsx"],
    text=True,
).splitlines()

def added_lines(file):
    diff = subprocess.check_output(["git", "diff", "--unified=0", BASE, HEAD, "--", file], text=True)
    result, current = set(), None
    for row in diff.splitlines():
        m = re.match(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@", row)
        if m:
            current = int(m.group(1))
        elif current is not None and row.startswith("+") and not row.startswith("+++"):
            result.add(current); current += 1
        elif current is not None and row.startswith("-") and not row.startswith("---"):
            pass
        elif current is not None:
            current += 1
    return result

def jsdoc_before(lines, line):
    i = line - 2
    while i >= 0 and not lines[i].strip():
        i -= 1
    if i < 0 or not lines[i].rstrip().endswith("*/"):
        return False
    while i >= 0:
        if "/**" in lines[i]:
            return True
        i -= 1
    return False

# Candidate categories: type/interface/class/enum declarations, named function declarations,
# top-level/exported values, and named callable arrow functions. Ordinary function-local state
# variables are deliberately excluded.
decl_patterns = [
    ("type", re.compile(r"^\s*(?:export\s+)?type\s+([A-Za-z_$][\w$]*)\b")),
    ("interface", re.compile(r"^\s*(?:export\s+)?interface\s+([A-Za-z_$][\w$]*)\b")),
    ("class", re.compile(r"^\s*(?:export\s+)?(?:abstract\s+)?class\s+([A-Za-z_$][\w$]*)\b")),
    ("enum", re.compile(r"^\s*(?:export\s+)?(?:const\s+)?enum\s+([A-Za-z_$][\w$]*)\b")),
    ("function", re.compile(r"^\s*(?:export\s+)?(?:async\s+)?function\s*\*?\s+([A-Za-z_$][\w$]*)\b")),
    ("arrow function", re.compile(r"^\s*(?:export\s+)?(?:const|let|var)\s+([A-Za-z_$][\w$]*)\b.*=\s*(?:async\s*)?(?:\([^)]*\)|[A-Za-z_$][\w$]*)\s*=>")),
]

# Brace-depth estimate is sufficient to identify module scope here. Remove strings/comments
# conservatively so punctuation within them does not affect the depth.
def brace_delta(line):
    clean = re.sub(r"//.*$", "", line)
    clean = re.sub(r"/\*.*?\*/", "", clean)
    clean = re.sub(r'"(?:\\.|[^"\\])*"', '""', clean)
    clean = re.sub(r"'(?:\\.|[^'\\])*'", "''", clean)
    return clean.count("{") - clean.count("}")

all_candidates = []
for file in files:
    lines = Path(file).read_text(encoding="utf-8").splitlines()
    added = added_lines(file)
    depth = 0
    for idx, source in enumerate(lines, 1):
        module_scope = depth == 0
        if idx in added:
            found = None
            for category, pattern in decl_patterns:
                m = pattern.match(source)
                if m:
                    found = (category, m.group(1))
                    break
            if found is None and module_scope:
                # Module-level values are part of the public/module declaration surface.
                m = re.match(r"^\s*(?:export\s+)?(?:const|let|var)\s+([A-Za-z_$][\w$]*)\b", source)
                if m:
                    found = ("module value", m.group(1))
            if found:
                all_candidates.append((file, idx, found[0], found[1], jsdoc_before(lines, idx), source.strip()))
        depth += brace_delta(source)

print("=== Documentation-oriented added declarations ===")
for item in all_candidates:
    file, line, category, name, documented, source = item
    print(f"{file}:Line {line}: {category}: {name}: {'JSDOC' if documented else 'MISSING'}")
    print(f"  {source}")

docs = sum(item[4] for item in all_candidates)
print(f"\nTOTAL={len(all_candidates)} DOCUMENTED={docs} MISSING={len(all_candidates)-docs}")

print("\n=== Missing candidates with source context ===")
for file, line, category, name, documented, source in all_candidates:
    if documented:
        continue
    lines = Path(file).read_text(encoding="utf-8").splitlines()
    print(f"\n--- {file}:Line {line}: {category} {name} ---")
    for n in range(max(1, line - 3), min(len(lines), line + 4) + 1):
        print(f"{n}: {lines[n - 1]}")
PY

printf '%s\n' '=== PR declaration changes, grouped by commit ==='
git log --format='%H %s' "$base..$head" | while read -r commit subject; do
  printf '\n--- %s %s ---\n' "${commit:0:8}" "$subject"
  git show --format= --unified=0 "$commit" -- 'src/**/*.ts' 'gui/src/**/*.ts' 'gui/src/**/*.tsx' \
    | rg '^\+.*\b(export\s+)?(async\s+)?function\s*\*?\s+[A-Za-z_$][A-Za-z0-9_$]*|^\+.*\b(export\s+)?(type|interface|class|enum)\s+[A-Za-z_$][A-Za-z0-9_$]*|^\+.*\b(export\s+)?(const|let|var)\s+[A-Za-z_$][A-Za-z0-9_$]*\s*=' \
    || true
done

Length of output: 13606


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
head="6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"

python3 - <<'PY'
import re
import subprocess
from pathlib import Path

BASE = "6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
HEAD = "6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"
PATHS = ["src/*.ts", "src/**/*.ts", "gui/src/*.ts", "gui/src/*.tsx", "gui/src/**/*.ts", "gui/src/**/*.tsx"]
files = subprocess.check_output(
    ["git", "diff", "--name-only", BASE, HEAD, "--", *PATHS],
    text=True,
).splitlines()

def added_lines(file):
    diff = subprocess.check_output(["git", "diff", "--unified=0", BASE, HEAD, "--", file], text=True)
    result, current = set(), None
    for row in diff.splitlines():
        m = re.match(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@", row)
        if m:
            current = int(m.group(1))
        elif current is not None and row.startswith("+") and not row.startswith("+++"):
            result.add(current)
            current += 1
        elif current is not None and row.startswith("-") and not row.startswith("---"):
            pass
        elif current is not None:
            current += 1
    return result

def jsdoc_before(lines, line):
    i = line - 2
    while i >= 0 and not lines[i].strip():
        i -= 1
    if i < 0 or not lines[i].rstrip().endswith("*/"):
        return False
    while i >= 0:
        if "/**" in lines[i]:
            return True
        i -= 1
    return False

patterns = [
    ("type", re.compile(r"^\s*(?:export\s+)?type\s+([A-Za-z_$][\w$]*)\b")),
    ("interface", re.compile(r"^\s*(?:export\s+)?interface\s+([A-Za-z_$][\w$]*)\b")),
    ("class", re.compile(r"^\s*(?:export\s+)?(?:abstract\s+)?class\s+([A-Za-z_$][\w$]*)\b")),
    ("enum", re.compile(r"^\s*(?:export\s+)?(?:const\s+)?enum\s+([A-Za-z_$][\w$]*)\b")),
    ("function", re.compile(r"^\s*(?:export\s+)?(?:async\s+)?function\s*\*?\s+([A-Za-z_$][\w$]*)\b")),
    ("arrow function", re.compile(r"^\s*(?:export\s+)?(?:const|let|var)\s+([A-Za-z_$][\w$]*)\b.*=\s*(?:async\s*)?(?:\([^)]*\)|[A-Za-z_$][\w$]*)\s*=>")),
]

def brace_delta(line):
    clean = re.sub(r"//.*$", "", line)
    clean = re.sub(r"/\*.*?\*/", "", clean)
    clean = re.sub(r'"(?:\\.|[^"\\])*"', '""', clean)
    clean = re.sub(r"'(?:\\.|[^'\\])*'", "''", clean)
    return clean.count("{") - clean.count("}")

candidates = []
for file in files:
    lines = Path(file).read_text(encoding="utf-8").splitlines()
    added = added_lines(file)
    depth = 0
    for idx, source in enumerate(lines, 1):
        module_scope = depth == 0
        if idx in added:
            found = None
            for category, pattern in patterns:
                m = pattern.match(source)
                if m:
                    found = (category, m.group(1))
                    break
            if found is None and module_scope:
                m = re.match(r"^\s*(?:export\s+)?(?:const|let|var)\s+([A-Za-z_$][\w$]*)\b", source)
                if m:
                    found = ("module value", m.group(1))
            if found:
                candidates.append((file, idx, found[0], found[1], jsdoc_before(lines, idx), source.strip()))
        depth += brace_delta(source)

print("=== Assessed-declaration candidate inventory ===")
for file, line, category, name, documented, source in candidates:
    print(f"{file}:Line {line}: {category}: {name}: {'JSDOC' if documented else 'MISSING'}")

documented = sum(item[4] for item in candidates)
print(f"\nTOTAL={len(candidates)} DOCUMENTED={documented} MISSING={len(candidates)-documented}")

print("\n=== Missing declaration contexts ===")
for file, line, category, name, documented, source in candidates:
    if documented:
        continue
    lines = Path(file).read_text(encoding="utf-8").splitlines()
    print(f"\n--- {file}:Line {line}: {category} {name} ---")
    for n in range(max(1, line - 3), min(len(lines), line + 4) + 1):
        print(f"{n}: {lines[n - 1]}")
PY

printf '%s\n' '=== Complete changed source-file list ==='
git diff --name-only "$base" "$head" -- \
  'src/*.ts' 'src/**/*.ts' 'gui/src/*.ts' 'gui/src/*.tsx' 'gui/src/**/*.ts' 'gui/src/**/*.tsx' \
  | sort

Length of output: 8944


🤖 web_search executed:

CodeRabbit "Docstring Coverage" pre-merge checks which declarations are counted as missing documentation TypeScript

Length of output: 9183


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
head="6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"

printf '%s\n' '=== Directly added exported declarations ==='
git diff --unified=0 "$base" "$head" -- \
  'src/*.ts' 'src/**/*.ts' 'gui/src/*.ts' 'gui/src/*.tsx' 'gui/src/**/*.ts' 'gui/src/**/*.tsx' \
  | rg -n -C 2 \
    '^\+\s*export\s+(?:declare\s+)?(?:async\s+)?function\s*\*?\s+[A-Za-z_$][A-Za-z0-9_$]*|^\+\s*export\s+(?:const|let|var)\s+[A-Za-z_$][A-Za-z0-9_$]*|^\+\s*export\s+(?:abstract\s+)?(?:class|interface|type|enum)\s+[A-Za-z_$][A-Za-z0-9_$]*' \
  || true

python3 - <<'PY'
import re
import subprocess
from pathlib import Path

BASE = "6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
HEAD = "6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"
PATHS = ["src/*.ts", "src/**/*.ts", "gui/src/*.ts", "gui/src/*.tsx", "gui/src/**/*.ts", "gui/src/**/*.tsx"]
files = subprocess.check_output(["git", "diff", "--name-only", BASE, HEAD, "--", *PATHS], text=True).splitlines()

direct_patterns = [
    ("function", re.compile(r"^\s*export\s+(?:async\s+)?function\s*\*?\s+([A-Za-z_$][\w$]*)\b")),
    ("value", re.compile(r"^\s*export\s+(?:const|let|var)\s+([A-Za-z_$][\w$]*)\b")),
    ("type", re.compile(r"^\s*export\s+type\s+([A-Za-z_$][\w$]*)\b")),
    ("interface", re.compile(r"^\s*export\s+interface\s+([A-Za-z_$][\w$]*)\b")),
    ("class", re.compile(r"^\s*export\s+(?:abstract\s+)?class\s+([A-Za-z_$][\w$]*)\b")),
    ("enum", re.compile(r"^\s*export\s+(?:const\s+)?enum\s+([A-Za-z_$][\w$]*)\b")),
]

def added_lines(file):
    text = subprocess.check_output(["git", "diff", "--unified=0", BASE, HEAD, "--", file], text=True)
    current, output = None, set()
    for row in text.splitlines():
        hunk = re.match(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@", row)
        if hunk:
            current = int(hunk.group(1))
        elif current is not None and row.startswith("+") and not row.startswith("+++"):
            output.add(current); current += 1
        elif current is not None and row.startswith("-") and not row.startswith("---"):
            pass
        elif current is not None:
            current += 1
    return output

def jsdoc(lines, n):
    i = n - 2
    while i >= 0 and not lines[i].strip():
        i -= 1
    if i < 0 or not lines[i].strip().endswith("*/"):
        return False
    while i >= 0:
        if "/**" in lines[i]:
            return True
        i -= 1
    return False

items = []
for file in files:
    lines = Path(file).read_text(encoding="utf-8").splitlines()
    for n in sorted(added_lines(file)):
        for category, pattern in direct_patterns:
            m = pattern.match(lines[n - 1])
            if m:
                items.append((file, n, category, m.group(1), jsdoc(lines, n)))
                break

print("=== Direct added exported declarations and JSDoc ===")
for item in items:
    print(f"{item[0]}:Line {item[1]}: {item[2]} {item[3]}: {'JSDOC' if item[4] else 'MISSING'}")
print(f"TOTAL={len(items)} DOCUMENTED={sum(x[4] for x in items)} MISSING={sum(not x[4] for x in items)}")

# Get exported declarations from the current files by extracting outline text and identify
# declarations that are not direct additions but have a changed line in their textual span.
# The source context is printed for human verification.
print("\n=== Existing exported declaration headers near changed hunks ===")
for file in files:
    lines = Path(file).read_text(encoding="utf-8").splitlines()
    changed = added_lines(file)
    exported_headers = []
    for n, line in enumerate(lines, 1):
        for category, pattern in direct_patterns:
            m = pattern.match(line)
            if m:
                exported_headers.append((n, category, m.group(1)))
                break
    for i, (start, category, name) in enumerate(exported_headers):
        end = (exported_headers[i+1][0] - 1) if i + 1 < len(exported_headers) else len(lines)
        # Only report an existing header when a later changed line lies before next export.
        if start not in changed and any(start <= c <= end for c in changed):
            print(f"{file}:Line {start}: {category} {name}: {'JSDOC' if jsdoc(lines, start) else 'MISSING'}")
PY

Length of output: 8579


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== AST JSON shape samples ==='
ast-grep run --lang ts --pattern 'export function $NAME($$$) { $$$ }' --json=stream src/config.ts | head -5 || true
ast-grep run --lang ts --pattern 'export interface $NAME { $$$ }' --json=stream src/types.ts | head -5 || true
ast-grep run --lang ts --pattern 'export type $NAME = $VALUE' --json=stream src/usage/log.ts | head -5 || true
ast-grep run --lang tsx --pattern 'export const $NAME = $VALUE' --json=stream gui/src/i18n/en.ts | head -5 || true

printf '%s\n' '=== Exact exported declaration ranges that intersect PR additions ==='
python3 - <<'PY'
import json
import re
import subprocess
from pathlib import Path

BASE = "6a7351b4d9f729fcc33241cbb385a01a70b21e0e"
HEAD = "6c7ea9f77e5fd66c24af0f1ddfccb8096b7d67df"
PATHS = ["src/*.ts", "src/**/*.ts", "gui/src/*.ts", "gui/src/*.tsx", "gui/src/**/*.ts", "gui/src/**/*.tsx"]
files = subprocess.check_output(["git", "diff", "--name-only", BASE, HEAD, "--", *PATHS], text=True).splitlines()

patterns = [
    "export function $NAME($$$) { $$$ }",
    "export async function $NAME($$$) { $$$ }",
    "export function* $NAME($$$) { $$$ }",
    "export async function* $NAME($$$) { $$$ }",
    "export const $NAME = $VALUE",
    "export let $NAME = $VALUE",
    "export var $NAME = $VALUE",
    "export interface $NAME { $$$ }",
    "export type $NAME = $VALUE",
    "export class $NAME { $$$ }",
    "export enum $NAME { $$$ }",
]

def added_lines(file):
    text = subprocess.check_output(["git", "diff", "--unified=0", BASE, HEAD, "--", file], text=True)
    result, current = set(), None
    for row in text.splitlines():
        m = re.match(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@", row)
        if m:
            current = int(m.group(1))
        elif current is not None and row.startswith("+") and not row.startswith("+++"):
            result.add(current); current += 1
        elif current is not None and row.startswith("-") and not row.startswith("---"):
            pass
        elif current is not None:
            current += 1
    return result

def jsdoc(lines, line):
    i = line - 2
    while i >= 0 and not lines[i].strip():
        i -= 1
    if i < 0 or not lines[i].rstrip().endswith("*/"):
        return False
    while i >= 0:
        if "/**" in lines[i]:
            return True
        i -= 1
    return False

def name_from_text(text):
    m = re.search(r"export\s+(?:async\s+)?(?:function\*?\s+|(?:const|let|var|interface|type|class|enum)\s+)([A-Za-z_$][\w$]*)", text)
    return m.group(1) if m else "<unknown>"

found = {}
for file in files:
    lang = "tsx" if file.endswith(".tsx") else "ts"
    changed = added_lines(file)
    for pattern in patterns:
        cmd = ["ast-grep", "run", "--lang", lang, "--pattern", pattern, "--json=stream", file]
        result = subprocess.run(cmd, text=True, capture_output=True)
        for row in result.stdout.splitlines():
            try:
                item = json.loads(row)
            except json.JSONDecodeError:
                continue
            # ast-grep's match JSON reports range.start/end in zero-based locations.
            r = item.get("range", {})
            start = r.get("start", {}).get("line")
            end = r.get("end", {}).get("line")
            if start is None or end is None:
                continue
            start += 1
            end += 1
            if any(start <= line <= end for line in changed):
                text = item.get("text", "")
                key = (file, start, end, name_from_text(text))
                found[key] = text

items = []
for (file, start, end, name), text in sorted(found.items()):
    lines = Path(file).read_text(encoding="utf-8").splitlines()
    items.append((file, start, end, name, jsdoc(lines, start)))

for file, start, end, name, documented in items:
    print(f"{file}:Line {start}-{end}: {name}: {'JSDOC' if documented else 'MISSING'}")
print(f"TOTAL={len(items)} DOCUMENTED={sum(x[4] for x in items)} MISSING={sum(not x[4] for x in items)}")

print("\n=== Missing contexts ===")
for file, start, end, name, documented in items:
    if documented:
        continue
    lines = Path(file).read_text(encoding="utf-8").splitlines()
    print(f"\n--- {file}:Line {start}: {name} ---")
    for line in range(max(1, start - 3), min(len(lines), start + 4) + 1):
        print(f"{line}: {lines[line-1]}")
PY

Length of output: 17773


@harryzhou2000 I cannot provide an exact current missing-declaration list from the available check output.

The CodeRabbit pre-merge report exposes only the aggregate value, 64.10%. The active configuration exposes only mode: warning and threshold: 80. It does not expose the declaration set that the checker assessed.

I verified the documented TypeScript eligibility rule: exported functions, values, classes, interfaces, and types require an adjacent /** ... */ block. On 6c7ea9f7, all directly added exported declarations have JSDoc:

  • src/config.ts Line 1395: retryOn429PolicyConfigError
  • src/lib/upstream-retry.ts Line 84: releaseResponseBodyBestEffort
  • src/lib/upstream-retry.ts Line 125: sleepWithHeartbeats
  • src/providers/key-failover.ts Line 98: rateLimitRetryPolicyFor
  • src/providers/key-failover.ts Line 123: rateLimitRetryDelayMs
  • src/types.ts Line 919: RateLimitRetryPolicy

The current branch also has JSDoc on the changed exported interfaces and values, including ImageBridgeDeps, WebSearchLoopDeps, OcxProviderConfig, AttemptRecoveryKind, and all six locale catalogs.

My earlier list of 13 declarations was a heuristic reconstruction, not the checker’s actual candidate list. The later percentages prove that reconstruction was not reliable. I withdraw that earlier conclusion.

A fresh CodeRabbit evaluation with per-declaration diagnostics is required to identify the remaining denominator precisely. The current review is rate-limited, so the existing report cannot be refreshed yet.

✅ Action performed

Reviews resumed.

Merge upstream/dev (14b20de, incl. lidge-jun#671 exact account routing) into
feat/429-same-target-retry. Resolve src/server/responses/core.ts: keep the
same-target 429 wait-and-retry loop on the passthrough wire and take
upstream's !authCtx.fixedAccount guard on the forward-pool retry branch.
Typecheck, privacy scan, 240 retry/passthrough tests, and 184
account-routing tests pass.
@harryzhou2000

Copy link
Copy Markdown
Author

@coderabbitai resume

Merged upstream/dev (14b20de, including #671 exact account routing) into feat/429-same-target-retry as c3b0ec8 and resolved the single conflict in src/server/responses/core.ts: the same-target 429 wait-and-retry loop on the passthrough wire is preserved, and the forward-pool retry branch now carries upstream's !authCtx.fixedAccount guard. Verified: typecheck, privacy scan, 240 retry/passthrough tests and 184 account-routing tests pass. This should also refresh the Docstring Coverage evaluation on the new head.

The shared-request cache refactor inserted the cachedRequest/cachedAdapter
declarations between the fetchOnce JSDoc and the function itself, orphaning the
doc block; reattach an adjacent JSDoc to both fetchOnce declarations and add
one to the bounded-body onAbort hook so the declarations stay documented.
@harryzhou2000

Copy link
Copy Markdown
Author

@coderabbitai resume

Docstring root-cause found: the shared-request cache refactor had orphaned the JSDoc blocks — let cachedRequest/cachedAdapter were inserted between each fetchOnce doc and its declaration (same for web-search), so the declarations counted as undocumented despite having a doc block. 4650bae reattaches adjacent JSDoc to both fetchOnce declarations and documents the bounded-body onAbort hook. A local diff-touched declaration scan now reads 12/12 (100%) on the PR delta. Please re-run the Docstring Coverage evaluation on head 4650bae and, if it still reports below threshold, emit the per-declaration diagnostics you mentioned.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
src/server/responses/core.ts (2)

2881-2886: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Redact terminal-continuation errors before yielding them.

The catch blocks at Lines 2881-2886 and 2929-2934 copy raw error.message text into AdapterEvent messages. The Responses bridge can serialize these messages to the client. A request-building or transport error may contain credential-shaped text or upstream request details.

The surrounding error paths already use redactSecretString at Lines 2475-2476, 2781-2783, and 3034. Apply the same sanitizer to both continuation messages.

Proposed fix
-          yield { type: "error", message: `Provider continuation failed: ${error instanceof Error ? error.message : String(error)}` };
+          yield { type: "error", message: `Provider continuation failed: ${redactSecretString(error instanceof Error ? error.message : String(error))}` };

Apply the same change in both catch blocks.

As per path instructions, src/** code must never log or serialize tokens or OAuth material into responses.

Also applies to: 2929-2934

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/responses/core.ts` around lines 2881 - 2886, Apply
redactSecretString to the error detail used in both terminal-continuation catch
blocks, including the paths yielding “Provider continuation failed” messages
near the continuation handlers. Preserve the existing client-aborted response
and fallback String(error) behavior, but sanitize the resulting error text
before placing it in either AdapterEvent message.

Source: Path instructions


2845-2846: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve recovery kinds across terminal-continuation failover.

fetchContinuation only maps the boolean replay state to rate-limit-429 at Lines 2845-2846. After key rotation, Anthropic account rotation, or image-tier retry, the loop calls fetchContinuation() with the default state. The next send therefore has no key-429, anthropic-oauth-429, or image-413 recovery classification.

The main recovery loop records these classifications. The continuation path must use the same contract so persisted attempt data and GUI recovery details remain accurate.

Pass an AttemptRecoveryKind through fetchContinuation, or store a pending recovery kind before each continue.

Based on the PR objectives, recovery classifications must be persisted and rendered in the GUI.

Also applies to: 2949-2954, 2976-2984, 2997-2999

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/responses/core.ts` around lines 2845 - 2846, Preserve the active
AttemptRecoveryKind when continuation retries follow key rotation, Anthropic
account rotation, or image-tier recovery. Update fetchContinuation and its
callers to accept and forward the pending recovery classification instead of
deriving only rate-limit-429 from the boolean replay state, ensuring subsequent
sends persist and render key-429, anthropic-oauth-429, and image-413
consistently with the main recovery loop.
docs-site/src/content/docs/ja/reference/configuration/providers.md (1)

36-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Prefix the localized routing links.

English uses the root path, while translated pages use locale prefixes. Update:

  • docs-site/src/content/docs/ja/reference/configuration/providers.md:36 to /ja/reference/configuration/routing/
  • docs-site/src/content/docs/ko/reference/configuration/providers.md:36 to /ko/reference/configuration/routing/

The current links open the English routing page instead of the localized page.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs-site/src/content/docs/ja/reference/configuration/providers.md` at line
36, Update the routing link in
docs-site/src/content/docs/ja/reference/configuration/providers.md:36 to use the
/ja/reference/configuration/routing/ locale-prefixed path, and make the
corresponding change in
docs-site/src/content/docs/ko/reference/configuration/providers.md:36 to use
/ko/reference/configuration/routing/.

Source: Path instructions

docs-site/src/content/docs/ru/reference/configuration/providers.md (1)

37-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep translated routing links inside their locale trees.

Both changed links use the English absolute route from translated configuration pages. Use the locale-prefixed route at each site.

  • docs-site/src/content/docs/ru/reference/configuration/providers.md#L37-L37: use /ru/reference/configuration/routing/.
  • docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L35-L35: use /zh-cn/reference/configuration/routing/.

As per path instructions, translated docs-site/** pages must preserve locale navigation.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs-site/src/content/docs/ru/reference/configuration/providers.md` at line
37, Update the routing link in
docs-site/src/content/docs/ru/reference/configuration/providers.md:37-37 to use
/ru/reference/configuration/routing/. Update the corresponding link in
docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:35-35 to
use /zh-cn/reference/configuration/routing/, preserving locale-specific
navigation in both translated pages.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@docs-site/src/content/docs/ja/reference/configuration/providers.md`:
- Line 36: Update the routing link in
docs-site/src/content/docs/ja/reference/configuration/providers.md:36 to use the
/ja/reference/configuration/routing/ locale-prefixed path, and make the
corresponding change in
docs-site/src/content/docs/ko/reference/configuration/providers.md:36 to use
/ko/reference/configuration/routing/.

In `@docs-site/src/content/docs/ru/reference/configuration/providers.md`:
- Line 37: Update the routing link in
docs-site/src/content/docs/ru/reference/configuration/providers.md:37-37 to use
/ru/reference/configuration/routing/. Update the corresponding link in
docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:35-35 to
use /zh-cn/reference/configuration/routing/, preserving locale-specific
navigation in both translated pages.

In `@src/server/responses/core.ts`:
- Around line 2881-2886: Apply redactSecretString to the error detail used in
both terminal-continuation catch blocks, including the paths yielding “Provider
continuation failed” messages near the continuation handlers. Preserve the
existing client-aborted response and fallback String(error) behavior, but
sanitize the resulting error text before placing it in either AdapterEvent
message.
- Around line 2845-2846: Preserve the active AttemptRecoveryKind when
continuation retries follow key rotation, Anthropic account rotation, or
image-tier recovery. Update fetchContinuation and its callers to accept and
forward the pending recovery classification instead of deriving only
rate-limit-429 from the boolean replay state, ensuring subsequent sends persist
and render key-429, anthropic-oauth-429, and image-413 consistently with the
main recovery loop.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9176eec6-2272-4a3f-8827-7e21f9957d95

📥 Commits

Reviewing files that changed from the base of the PR and between d2db429 and c3b0ec8.

📒 Files selected for processing (18)
  • docs-site/src/content/docs/ja/guides/providers.md
  • docs-site/src/content/docs/ja/reference/configuration/providers.md
  • docs-site/src/content/docs/ko/guides/providers.md
  • docs-site/src/content/docs/ko/reference/configuration/providers.md
  • docs-site/src/content/docs/reference/configuration/providers.md
  • docs-site/src/content/docs/ru/guides/providers.md
  • docs-site/src/content/docs/ru/reference/configuration/providers.md
  • docs-site/src/content/docs/zh-cn/guides/providers.md
  • docs-site/src/content/docs/zh-cn/reference/configuration/providers.md
  • gui/src/i18n/de.ts
  • gui/src/i18n/en.ts
  • gui/src/i18n/ja.ts
  • gui/src/i18n/ko.ts
  • gui/src/i18n/ru.ts
  • gui/src/i18n/zh.ts
  • src/server/responses/core.ts
  • structure/04_transports-and-sidecars.md
  • tests/web-search.test.ts

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Current head and merge commit ==='
git rev-parse HEAD
git show --no-patch --format='commit=%H%nparents=%P%nsubject=%s' c3b0ec8b

printf '%s\n' '=== Changed paths in the merge resolution ==='
git diff-tree --no-commit-id --name-status -m -r c3b0ec8b -- src/server/responses/core.ts

printf '%s\n' '=== Merge-resolution diff for core.ts ==='
git show --cc --format= --unified=35 c3b0ec8b -- src/server/responses/core.ts

printf '%s\n' '=== Focused passthrough 429 retry context ==='
rg -n -C 18 \
  'passthrough|rateLimitRetry|rateLimitRetries|rateLimitRetryDelayMs|status\s*===\s*429|response\.status\s*===\s*429' \
  src/server/responses/core.ts

printf '%s\n' '=== Fixed-account forward-pool retry context ==='
rg -n -C 18 \
  'fixedAccount|forward.*pool|pool.*forward|authCtx' \
  src/server/responses/core.ts

printf '%s\n' '=== Relevant function structure ==='
ast-grep outline src/server/responses/core.ts --items all --type function --match 'handleResponses|fetch' --view expanded

Length of output: 127992


@harryzhou2000 Verified on c3b0ec8b.

c3b0ec8b merges feature head 6c7ea9f7 with upstream/dev commit 14b20def.

The passthrough same-target 429 retry loop remains in src/server/responses/core.ts at Lines 1736-1787. It still:

  • Applies only when rateLimitRetryPolicyFor(route.provider) returns a policy.
  • Releases the 429 response body before the backoff.
  • Uses Retry-After through rateLimitRetryDelayMs(...).
  • Replays the existing request before forward-pool handling.
  • Stops and returns a client-cancelled response when the abort signal fires.

The forward-pool alternate-account branch now checks !authCtx.fixedAccount at Line 1789. A request with an exact account selector cannot enter alternate-account resolution.

retryCodexPoolOnAlternateAccount(...) also retains its defense-in-depth guard at Line 351. It returns "no-alternate" when firstAuthCtx.fixedAccount is true.

The inspected merge resolution preserves both required behaviors. Normal review is resumed.

✅ Action performed

Reviews resumed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)
docs-site/src/content/docs/ru/reference/configuration/providers.md (1)

98-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Do not describe retry-budget exhaustion as disabling key failover.

retryOn429.attempts limits additional same-key replays for one request. The documented contract performs those replays before the normal key-failover path. The current Russian text says that an exhausted budget returns 429 without key failover.

State that the budget stops further same-key replays. Then document that normal failover or final-error handling applies according to the available targets.

As per path instructions, user-facing documentation must match actual CLI and API behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs-site/src/content/docs/ru/reference/configuration/providers.md` at line
98, Update the retryOn429 documentation row to clarify that exhausting attempts
only stops additional same-key replays. State that normal key failover or
final-error handling then proceeds according to the available targets, removing
the claim that exhaustion directly returns 429 without key failover.

Source: Path instructions

src/images/loop.ts (2)

444-494: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Bridge transient-retry telemetry is incomplete.

Both bridge loops report telemetry before fetchWithResetRetry, so additional helper-driven sends are not recorded with their recovery kind.

  • src/images/loop.ts#L444-L494: move onAttemptSend into the fetchWithResetRetry callback and use retryRecovery ?? recovery.
  • src/web-search/loop.ts#L343-L397: apply the same callback-based telemetry and preserve the initial recovery kind.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/images/loop.ts` around lines 444 - 494, Update the fetch flows in
src/images/loop.ts lines 444-494 and src/web-search/loop.ts lines 343-397: move
onAttemptSend into each fetchWithResetRetry callback so every helper-driven send
is recorded, passing retryRecovery ?? recovery to preserve the initial recovery
kind when no retry recovery is available. Apply the same telemetry change in
both sites.

502-536: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Bridge key-failover telemetry omits key-429.

The rotated adapter sends are not classified as key failovers, unlike the core recovery loop.

  • src/images/loop.ts#L502-L536: call fetchOnce(adapter, "key-429") after deps.on429 returns the rotated adapter.
  • src/web-search/loop.ts#L405-L439: make the same fetchOnce(adapter, "key-429") change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/images/loop.ts` around lines 502 - 536, Update the rotated-adapter retry
paths in src/images/loop.ts lines 502-536 and src/web-search/loop.ts lines
405-439 so the fetchOnce call made after deps.on429 returns the rotated adapter
uses the "key-429" classification, matching core recovery telemetry; no other
retry behavior should change.
structure/04_transports-and-sidecars.md (1)

256-280: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Exclude runTurn transports from the retry scope. src/images/loop.ts:364-426 handles runTurn adapters through an event queue and receives no HTTP status, so the policy passed at src/server/responses/core.ts:2236 cannot replay a 429. Limit lines 256-279 to HTTP-capable API-key adapters and explicitly exclude custom runTurn transports.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@structure/04_transports-and-sidecars.md` around lines 256 - 280, Update the
retryOn429 documentation to scope the policy only to HTTP-capable API-key
adapters, explicitly excluding custom runTurn transports handled through the
event queue in the image loop. Remove any wording that implies runTurn adapters
can receive HTTP 429 statuses or replay requests via the policy passed from the
Responses core.
docs-site/src/content/docs/ko/reference/configuration/providers.md (1)

36-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep translated routing links inside their locale trees.

  • docs-site/src/content/docs/ko/reference/configuration/providers.md#L36-L36: use /ko/reference/configuration/routing/.
  • docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L35-L35: use /zh-cn/reference/configuration/routing/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs-site/src/content/docs/ko/reference/configuration/providers.md` at line
36, Update the routing links in
docs-site/src/content/docs/ko/reference/configuration/providers.md:36-36 and
docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:35-35 to
remain within their locale trees, using the /ko/reference/configuration/routing/
and /zh-cn/reference/configuration/routing/ paths respectively.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@docs-site/src/content/docs/ko/reference/configuration/providers.md`:
- Line 36: Update the routing links in
docs-site/src/content/docs/ko/reference/configuration/providers.md:36-36 and
docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:35-35 to
remain within their locale trees, using the /ko/reference/configuration/routing/
and /zh-cn/reference/configuration/routing/ paths respectively.

In `@docs-site/src/content/docs/ru/reference/configuration/providers.md`:
- Line 98: Update the retryOn429 documentation row to clarify that exhausting
attempts only stops additional same-key replays. State that normal key failover
or final-error handling then proceeds according to the available targets,
removing the claim that exhaustion directly returns 429 without key failover.

In `@src/images/loop.ts`:
- Around line 444-494: Update the fetch flows in src/images/loop.ts lines
444-494 and src/web-search/loop.ts lines 343-397: move onAttemptSend into each
fetchWithResetRetry callback so every helper-driven send is recorded, passing
retryRecovery ?? recovery to preserve the initial recovery kind when no retry
recovery is available. Apply the same telemetry change in both sites.
- Around line 502-536: Update the rotated-adapter retry paths in
src/images/loop.ts lines 502-536 and src/web-search/loop.ts lines 405-439 so the
fetchOnce call made after deps.on429 returns the rotated adapter uses the
"key-429" classification, matching core recovery telemetry; no other retry
behavior should change.

In `@structure/04_transports-and-sidecars.md`:
- Around line 256-280: Update the retryOn429 documentation to scope the policy
only to HTTP-capable API-key adapters, explicitly excluding custom runTurn
transports handled through the event queue in the image loop. Remove any wording
that implies runTurn adapters can receive HTTP 429 statuses or replay requests
via the policy passed from the Responses core.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0eb360b8-4f4d-4694-8457-2ec054d0b575

📥 Commits

Reviewing files that changed from the base of the PR and between d2db429 and 4650bae.

📒 Files selected for processing (21)
  • docs-site/src/content/docs/ja/guides/providers.md
  • docs-site/src/content/docs/ja/reference/configuration/providers.md
  • docs-site/src/content/docs/ko/guides/providers.md
  • docs-site/src/content/docs/ko/reference/configuration/providers.md
  • docs-site/src/content/docs/reference/configuration/providers.md
  • docs-site/src/content/docs/ru/guides/providers.md
  • docs-site/src/content/docs/ru/reference/configuration/providers.md
  • docs-site/src/content/docs/zh-cn/guides/providers.md
  • docs-site/src/content/docs/zh-cn/reference/configuration/providers.md
  • gui/src/i18n/de.ts
  • gui/src/i18n/en.ts
  • gui/src/i18n/ja.ts
  • gui/src/i18n/ko.ts
  • gui/src/i18n/ru.ts
  • gui/src/i18n/zh.ts
  • src/images/loop.ts
  • src/lib/upstream-retry.ts
  • src/server/responses/core.ts
  • src/web-search/loop.ts
  • structure/04_transports-and-sidecars.md
  • tests/web-search.test.ts

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="lidge-jun/opencodex"
pr="865"
commit="4650baef"

printf '%s\n' '=== Commit and PR head ==='
git show --no-patch --format='commit=%H%nsubject=%s' "$commit"
gh pr view "$pr" --repo "$repo" \
  --json headRefOid,baseRefOid,headRefName \
  --jq '{headRefName,headRefOid,baseRefOid}'

head="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"

printf '%s\n' '=== Changed files in 4650baef ==='
git diff-tree --no-commit-id --name-status -r "$commit"

printf '%s\n' '=== JSDoc and declaration contexts ==='
rg -n -C 10 \
  'fetchOnce|cachedRequest|cachedAdapter|onAbort|releaseResponseBodyBestEffort' \
  src/images/loop.ts src/web-search/loop.ts src/lib/upstream-retry.ts

printf '%s\n' '=== Exact production patch ==='
git show --format= --unified=20 "$commit" -- \
  src/images/loop.ts src/web-search/loop.ts src/lib/upstream-retry.ts

printf '%s\n' '=== Docstring Coverage check runs on the PR head ==='
gh api --paginate "repos/${repo}/commits/${head}/check-runs?per_page=100" \
  --jq '.check_runs[]
    | select((.name + " " + (.workflow_name // "") + " " + (.output.title // ""))
      | test("docstring|coverage"; "i"))
    | {
        name, status, conclusion, details_url,
        title: .output.title, summary: .output.summary, text: .output.text
      }' || true

printf '%s\n' '=== Legacy coverage statuses on the PR head ==='
gh api "repos/${repo}/commits/${head}/status" \
  --jq '.statuses[]
    | select((.context + " " + (.description // "")) | test("docstring|coverage"; "i"))
    | {context,state,description,target_url}' || true

Length of output: 35311


@harryzhou2000 Verified on 4650baef6c323770622ee2d6055c0b8cbb3c6e0d, which is the current PR head.

The JSDoc blocks are now adjacent to the declarations that the cache variables previously separated:

  • src/images/loop.ts Lines 454-458: the fetchOnce JSDoc block is directly before fetchOnce.
  • src/web-search/loop.ts Lines 353-357: the fetchOnce JSDoc block is directly before fetchOnce.
  • src/lib/upstream-retry.ts Lines 101-105: the onAbort JSDoc block is directly before onAbort.

This fixes the documented attachment failure mode.

I could not verify a refreshed Docstring Coverage result. GitHub currently returns no matching check run and no legacy status for Docstring Coverage on 4650baef. The result is therefore inconclusive. No per-declaration diagnostics are available until that check publishes output.

I will resume normal review. If the refreshed check still reports below 80%, provide its run URL or output and I will identify the remaining declarations from the actual evaluator data.

✅ Action performed

Reviews resumed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants