Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true📝 WalkthroughWalkthroughThe change bounds API-key 429 failover for generic Responses requests. It adds rotation controls, scoped combo send budgets, validation coverage, and documentation for shared limits and cooldown handling. ChangesResponses key failover
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponsesAdapter
participant ProviderTransport
participant KeyFailover
Client->>ResponsesAdapter: submit Responses request
ResponsesAdapter->>ProviderTransport: send request with current API key
ProviderTransport-->>ResponsesAdapter: return 429
ResponsesAdapter->>KeyFailover: request bounded rotation
KeyFailover-->>ResponsesAdapter: record cooldown and return replacement or no replacement
ResponsesAdapter->>ProviderTransport: send recovery or continuation request
ProviderTransport-->>ResponsesAdapter: return response or final error
ResponsesAdapter-->>Client: return Responses result
Merge Risk: 🟡 Moderate · up to Multi-target Responses requests can exceed their bounded upstream-send allowance, so the shared accounting should be corrected before merge. The provider documentation should also clarify which authentication and retry modes the limit covers. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 5 files. (7 skipped: 7 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
2/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
리뷰 · 우선순위 64 / 80설명 이 PR(작성자 luvs01, draft)은 API 키 풀에서 429가 났을 때 키가 무한히 돌 수 있는 구멍을 막는다. 짧은 쿨다운이 끝나면 이미 시도한 키를 다시 고를 수 있고, reset-only 전송은 공유 transient retry budget을 안 켜서 요청이 키만 바꾸며 계속 도는 장면이 나온다. 고치는 방법은 단순하다. 현재 테스트는 핵심을 잘 잡는다. 문서 쪽은 영어 providers.md와 8개 로케일, 그리고 우선순위 64는 '실제 비용/루프 버그를 막는 코드'라서 문서 PR보다 높고, 동시에 draft·임팩트 스위트 미완료·structure 산포·tip 뒤처짐 때문에 70대로 올리지 않은 점수다. 콤보 전체 예산이 아니고 '키를 한 번씩만' 보장도 아니라고 본문이 솔직히 말한다. 그 범위를 메인테이너가 받아들일지가 머지 판단의 핵심이다. src/server/responses/core.ts keyPool429RetryAllowed - continuation=false일 때만 auth-recovery reserve를 엿보는데, countedExternally:true 로 reserveDispatch를 호출한다. '검사만 하고 소비하지 않는다'는 주석과 실제 reserve API 의미가 같은지 tip 기준으로 한 번 더 확인이 필요하다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78957f72f0
ℹ️ 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".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7dc2d7343
ℹ️ 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".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Updated author evidence for the current head 9336e38. Both automated reviews are complete and their two findings are resolved, including the initially unpooled key and sidecar sites. CI completed with the explicitly documented Windows exceptions; this is author-ready, not a green-CI claim. The reserve question was rechecked: reserveDispatch returns a permit, and shared state is charged only by permit.use(); the allowance query does not call it. Pools with fewer than two entries already decline key rotation in rotateKeyAfterFailure, while existing same-target retry and OAuth fallback policies remain separate. The referenced dev changes from 62f0222 to 4f788f9 are a version bump and release evidence, with no diff in core.ts, key-failover.ts or request-execution-budget.ts. Rebasing solely to move the base label would restart validation without changing those semantics. The ownership manifest was not expanded: the short links satisfy its existing source-owner mapping while the contract stays in one canonical section. Maintainer acceptance of the invocation-level design remains a merge decision. The four author-checklist boxes and review-ready label are complete. GitHub denied the author account permission to mark this PR ready, and the repository automation also reported a failed draft conversion. It therefore remains technically draft; a maintainer must perform that state transition. I have not retried the denied mutation. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="90f6c2b39bda6883637b3598a67cc9048424e1d8"
echo '=== HEAD / COMMIT ==='
git rev-parse HEAD
git show --no-ext-diff --format=fuller --stat --summary "$commit"
echo '=== CHANGED FILES ==='
git diff-tree --no-commit-id --name-status -r "$commit"
echo '=== RELEVANT DIFF ==='
git diff --no-ext-diff --unified=100 "${commit}^" "$commit" -- \
src/server/responses/combo-send-budget.ts \
src/server/responses/core.ts \
src/lib/request-execution-budget.ts \
tests/responses/responses-send-budget-counts.test.ts \
tests/lib/execution-budget-permits.test.ts \
tests/server/server-key-failover-e2e.test.ts
echo '=== OUTLINES ==='
ast-grep outline src/server/responses/combo-send-budget.ts --items all
ast-grep outline src/lib/request-execution-budget.ts --items all
ast-grep outline tests/responses/responses-send-budget-counts.test.ts --items all
echo '=== IMPLEMENTATIONS ==='
cat -n src/server/responses/combo-send-budget.ts
sed -n '1,320p' src/lib/request-execution-budget.ts
echo '=== FACTORY CALL SITES / RESERVATIONS ==='
rg -n -C 12 '\b(comboExecutionBudgetPolicy|deriveSendBudgetScope|comboTargetSendBudget|reserveDispatch|finalRecoveryAllowance|reserveSpent|pendingExternalSends)\b' \
src/server/responses/core.ts \
src/server/responses/combo-send-budget.ts \
src/lib/request-execution-budget.ts \
tests -g '*.ts'
echo '=== PHYSICAL-SEND REGRESSION ==='
rg -n -C 30 '\[4,\s*1,\s*1\]|recovery-heavy|physical send|physical-send|combo' \
tests/responses/responses-send-budget-counts.test.ts \
tests/lib/execution-budget-permits.test.ts
echo '=== COMBO EXECUTION ==='
rg -n -C 20 '\b(comboExecutionBudgetPolicy|comboTargetSendBudget|deriveSendBudgetScope)\s*\(' src/server/responses/core.tsLength of output: 196127 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== BUDGET ACCOUNTING CALL SITES ==='
rg -n -C 10 'onSendsConsumed|sendBudget\.used|\.used \+=|\.used =|remainingTransientSendBudget|recoverySendAllowance' \
src tests -g '*.ts'
echo '=== RESPONSE CORE SEND BUDGET HELPERS ==='
sed -n '5260,5405p' src/server/responses/core.ts
echo '=== COMBO CHILD EXECUTION AND FAILURE BRANCHES ==='
sed -n '3230,3555p' src/server/responses/core.ts
echo '=== KIRO COMBO TEST AND RETRY ACCOUNTING ==='
sed -n '45,95p' tests/adapters/adapter-inner-send-budget.test.ts
fd -a '^kiro-retry\.ts$|^transport-retry\.ts$|^upstream-retry\.ts$' src
rg -n -C 12 'onSendsConsumed|sendBudget|remainingBaseSends' src/adapters/kiro-retry.ts src/lib/upstream-retry.ts src/adapters/cursor/transport-retry.ts 2>/dev/null || true
echo '=== CHILD EARLY RETURNS / THROWS BEFORE ADAPTER SEND ==='
rg -n -C 8 'return |throw |handleResponses\(' src/server/responses/core.ts -g '*.ts' | sed -n '1,520p'Length of output: 141117
The prior later-target starvation issue is addressed.
I inspected the source and tests. I did not run the test suite in this environment. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90f6c2b39b
ℹ️ 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".
|
The latest inline findings are corrected in @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63807e330f
ℹ️ 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".
|
Updated to @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c23a71c11b
ℹ️ 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".
…unds Port the PR final dispatch deltas to current owners. Retry helpers settle the prepaid hop, while adapter sends retain the current dev dispatch-boundary confirmation and unused-hop refund. Product tests and CI not run here. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
c23a71c to
6da3d84
Compare
|
Cross-platform CI is red at The failing case is The source in the same branch reads: const result = await rebuildAndRefetch("oauth-account-429",
transportState.activeAdapter.fetchResponse ? () => { hop.permit?.use(); } : undefined);So the callback became conditional on Two ways out, and the second is better. Updating the expected literal to the conditional form restores green immediately. Asserting on the structure rather than on an exact source substring would stop this recurring: an exact-substring oracle over a multi-line expression breaks on any reformatting, including one that a later PR in this area will almost certainly cause. Context for the timing: this PR is the bottom of the responses-budget lane in |
… exact source text
The source-oracle assertions in tests/lib/execution-budget-permits.test.ts were
written against an earlier form of the adapter-recovery-oauth-429 block and did
not survive this branch's own refactor of it, so Cross-platform CI was red on
test 3/4 and macos 1/2 with an identical failure on both.
Two things moved. The confirm callback became conditional on the adapter owning
its own fetchResponse, so the exact substring
rebuildAndRefetch("oauth-account-429", () => { hop.permit?.use(); }) no longer
appears anywhere. And the refund moved out of the failed arm into a finally, so
one idempotent release now covers the failed return, the success path and a
throw, which means the per-branch release the old assertion looked for is gone
by design.
Both assertions are rewritten to bind the behaviour instead of the formatting:
that rebuildAndRefetch is called with the oauth-account-429 tag and reaches
hop.permit?.use() within a bounded window, that the reservation is parked in
pendingHopPermit when the retry helper rather than the adapter will settle it,
that the failed arm returns, and that the finally releases. The neighbouring
assertions in this file already use indexOf and toMatch for exactly this reason;
this line was the only exact multi-line substring left, and it is why an
otherwise sound change went red.
No runtime code changed. 14 pass / 0 fail on
bun test tests/lib/execution-budget-permits.test.ts.
Co-authored-by: luvs01 <luvs01@users.noreply.github.com>
|
The corrective follow-up is published at @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c749cc4c8
ℹ️ 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".
Declare when an HTTP adapter accounts for its own physical sends. Kiro receives the exact prepaid OAuth hop in a derived scope instead of charging a second reservation. Other fetchResponse implementations retain caller-owned confirmation after pacing. Carry the same ownership through queued continuations and refund unused hops. Actual Kiro server regressions preserve all three credential attempts, including third-account success and a preceding reset. Before the change they stopped after the first account with a budget-exhaustion 502. The Kiro, permit and inner-budget run passed 30 tests/160 assertions. Two existing retry-helper OAuth cases passed with an explicit 20-second timeout after one default-5-second timeout. Typecheck, structure, privacy, file-size and diff checks pass; hosted CI remains separate.
Use a reasoning-only Kiro completion with the explicit emptyCompletionRetry opt-in. The public server takes one initial send, retries the first account, then follows two quota-driven OAuth hops and accepts the third account within four physical sends. With only adapter-continuation restored to its pre-fix version, the fixture stops after the two first-account sends and fails. Restoring the prepaid continuation transfer passes this one test with four assertions. The previously passing suite was not repeated.
|
The adapter-owned OAuth accounting follow-up and direct continuation regression are published at @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77134192fe
ℹ️ 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".
|
@codex review Please review current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ce3aee226
ℹ️ 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".
|
@codex review Current head: 8885847. The additional change applies the same first-send-only prepaid permit consumption to the passthrough retry helper. Its real OAuth regression covers account A returning 429, account B's prepaid send resetting, then B succeeding using the remaining base attempt (three physical sends, no recovery reserve spent). The corresponding original-code control fails; 47 related tests / 491 assertions and type/structure/privacy/ratchet checks pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8885847bf5
ℹ️ 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".
| sendBudget: replaySendBudget, | ||
| onPhysicalSend: send => noteAdapterPhysicalSend(continuationEstimate, send), | ||
| stream: nextParsed.stream, | ||
| executor: storedPoolReplayDispatchNotifier(providerFetch(route.provider, options.codexWsRuntimeIdentity, { |
There was a problem hiding this comment.
When an empty-completion continuation on Command Code receives a 429 and rotates OAuth accounts, this notifier never confirms the reserved hop: that adapter captures its own executor and fetchCommandCode never invokes ctx.executor (and MiMo similarly calls fetch directly). The continuation therefore performs the replay, then lines 296-297 release its permit as if no dispatch occurred, allowing subsequent account rotations to send outside the shared request ceiling. Confirm caller-owned permits after the outer pacing wait before invoking adapters that do not declare fetchResponseUsesSendBudget, or require those adapters to dispatch through the supplied executor, with a focused non-Kiro OAuth continuation regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bdf3dd9. Command Code inference (including its retry) and MiMo inference/401 replay now use ctx.executor when supplied. Command Code catalog discovery and MiMo credential bootstrap retain their separate existing transports. This lets the caller confirm its reserved hop at actual inference dispatch while retaining pre-dispatch refunds.
The real Command Code empty-completion -> 429 -> OAuth rotation regression fails on the original code with five sends and unnecessary further account rotations; the fix makes three sends and settles the existing caller-owned hop exactly once. This is not a claim of a new global cap on all internal adapter sends. Executor-selection regressions cover both adapters. All four fetchResponse implementations were checked; Google already uses the supplied executor and Kiro settles its own budget.
78 adapter/regression tests and the two recalled-native-combo tests passed (80 tests / 309 assertions), with the existing fixture/timeouts unchanged. Type, structure, privacy, ratchet and diff checks passed. Latest-head cross-platform CI is still pending.
2 participants
Summary
Bound API-key rotations and physical sends across a logical Responses request. Preserve an exact prepaid compact recovery through intermediate combo scopes, settle it once, and retain its inherited total/recovery ceiling. A prepaid first send remains available even when the remainder cannot fund every later declared target. OAuth/forward account pools are separate from the documented API-key rotation count.
Current author verification
Exact head:
8885847bf52010c1b3f8a90036f21fdb14c51f61.35045571767is pending. CI completion remains pending; no exact-head all-green claim is made. Earlier-head results are not validation of this new head.Review readiness checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation