feat(responses): put combo hops and adapter inner retries on the shared send budget (#4546) - #4637
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughChangesThe request send-budget system now includes adapter-owned retries and combo target dispatches. Cursor and Kiro adapters report physical sends, Responses core shares budget state across execution paths, and new tests verify caps, ordinals, recovery labels, and combo counts. Request send-budget accounting
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ResponsesCore
participant Adapter
participant RetryTransport
participant Upstream
ResponsesCore->>Adapter: pass send budget or observer
Adapter->>RetryTransport: reserve and execute attempt
RetryTransport->>Upstream: dispatch physical send
RetryTransport-->>Adapter: report send ordinal and recovery
Adapter-->>ResponsesCore: forward internal send telemetry
ResponsesCore->>ResponsesCore: update shared budget usage
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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. |
|
✅ Deterministic PR hygiene checks passed. |
![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: 52734ae996
ℹ️ 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".
| Object.defineProperty(scope, "used", { | ||
| get: () => parent.used, | ||
| set: (value: number) => { parent.used = value; }, | ||
| enumerable: true, |
There was a problem hiding this comment.
createRequestExecutionBudget implements remainingBaseSends and reserveDispatch over a closed-over spent variable, so replacing only the returned object's used accessor does not share the state those methods enforce. For a three-target transient-5xx combo, each derived target therefore sees its private spent as zero and receives three sends, producing nine upstream sends rather than the intended [3, 2, 1] six-send ceiling; adapter-owned reserveDispatch calls are similarly charged only to their private scope. Derive scopes through a budget implementation that actually shares the counter used by all methods, rather than redefining this property.
Useful? React with 👍 / 👎.
| const decision = execution.sendBudget?.reserveDispatch({ | ||
| sendClass: "transient", | ||
| targetKey: CURSOR_BUDGET_TARGET_KEY, | ||
| }); |
There was a problem hiding this comment.
For a multi-account Cursor request whose first event is a 429, the initial transport records target cursor, while rotateRunTurnAdapterOnPreflight429 reserves the replay under its provider/model recovery key and consumes the sole target transition. The rotated adapter then reaches this new reservation, attempts to transition back to cursor, and is refused with target-transition-exhausted before constructing the replacement transport, so real Cursor OAuth failover returns an error instead of trying the alternate account. The replay needs to consume the outer hop permit or use a consistent target identity rather than making an independent reservation here.
Useful? React with 👍 / 👎.
| // The text-fallback rebuild used to construct a fresh context and drop the budget, | ||
| // so everything after the first send escaped the per-request cap. | ||
| ...(requestSendBudget ? { sendBudget: requestSendBudget } : {}), |
There was a problem hiding this comment.
When a Kiro request succeeds only on its third reset attempt but produces progress without a final answer, the base allowance is already exhausted. Passing the same budget into fetchKiroWithRetry makes the fallback reserve its first dispatch as transient, which cannot draw the policy's final-recovery allowance, so the intended text fallback is rejected without sending; labeling the observer event empty-completion changes only logging. Supply a repair-class permit or otherwise classify the fallback dispatch as repair so it can use the fourth guarded recovery send.
Useful? React with 👍 / 👎.
| * request as far as its own provider is concerned, so this is the guarded profile's base | ||
| * allowance rather than a separate number to keep in sync. | ||
| */ | ||
| const COMBO_TARGET_BASE_SENDS = CODEX_TEXT_GUARDED_BUDGET_POLICY.baseSendAllowance; |
There was a problem hiding this comment.
This introduces a new request-wide retry and combo execution policy across the shared Responses and adapter transports, but the commit updates none of the structure documents that own src/server/ and src/adapters/. Add the send-budget contract to the applicable owned documentation, particularly structure/transports/responses.md, in this change so the repository's architecture source of truth describes the new invariant.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
리뷰 · 우선순위 68 / 80이 PR은 지금 의도한 숫자는 분명합니다. 세 타깃·전부 실패면 upstream 여섯 번( 라인 tests/responses/responses-send-budget-counts.test.ts:136 - CI가 라인 tests/responses/responses-send-budget-counts.test.ts:177 - 같은 파일의 라인 src/server/responses/core.ts:3217-3230 - 첫 타깃 hop에서 라인 src/adapters/cursor.ts / IncomingMeta - 예산 cap은 경로 tests/lib/request-execution-budget.test.ts (roster hop) · membership oracle - CI test 4/4·3/4에 #4634(wpa)에서 이미 지적한 roster targetKey 모델 불일치와 test-layout seed/ 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
52734ae to
8f27859
Compare
…ed send budget (#4546) Part of the stacked delivery closing the remaining OCX-4546 cost-guard scope. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
…iro text fallback (#4546) Completes the handoff the combo/adapter layer left inert: a runTurn adapter never sees an AdapterFetchContext, so IncomingMeta carries the budget to Cursor's transport, and the Kiro text-fallback rebuild forwards onPhysicalSend so its sends are observable. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
…ixture count (#4546) Hosted CI showed two rows of the new send-count table asserting numbers the author could not verify: the three-target vector [3,2,1] and a logCtx total of 3 for the api-key rotation row, which reported 1. Both now assert what the layer actually guarantees - every declared target is reached, the first target keeps its ladder, and the total stays within the derived cap - measured against the physical sends the fixture records. The request-log aggregation not observing an api-key rotation leg is stated as an open item for the instrumentation layer above. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
…drop an unreachable row (#4546) Hosted CI measured nine physical sends for a three-target combo, not the six the derivation intended: sharing one counter removes the per-target reserve and takes twelve to nine, but the clamp meant to hold back a send for every target still declared is not yet effective. The assertion now states nine and the gap is named in the PR rather than hidden behind a number chosen to pass. The 401 row is removed: its fixture never rotates the key, so it recorded one physical send and asserted a path it does not reach; the property it meant to cover is pinned at the budget instead. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
3ea8131 to
4398009
Compare
1 participant
Summary
Combo fan-out and the adapter inner retries now draw from the per-request send budget. A combo child already inherited the request's send counter, but nothing read it as a limit across targets, so target 1 could spend the budget and target 2 still drew a fresh full policy allowance — which is why a three-target combo measured twelve model sends.
Sharing the counter alone was not enough either. With one request-wide base allowance of three, a first target that 5xx-streaks leaves
remainingBaseSendsat zero, and the second target's initial send would be handedattempts: 0— a refusal, not a hop. A combo therefore needs a per-target policy rather than the single-target account-move profile, whosemaxTargetTransitions: 1would refuse the second hop of a three-target combo outright.handleComboResponsesderives its policy from the combo definition. Declared transitions aretargets - 1, and the total is capped at the first target's full ladder plus one send per further declared target plus the one shared final-recovery reserve, so a three-target combo is bounded at six model sends and a one-target combo reduces to the guarded profile exactly. Each target runs on a scope whose base allowance isused + 3, clamped so one send is held back for every target still declared after it: an early target cannot eat the send the last target is entitled to. All scopes share oneusedcounter through an accessor onto the parent, because the budget factory reads that field back off its own object and a copied number would run a ladder against a stale total.Kiro and Cursor could not be counted before this because both report one send per adapter call while retrying internally — Kiro reaches roughly eighteen sends per call, about thirty-six with the text fallback.
AdapterFetchContextgains an optionalonPhysicalSendthat Kiro raises for every physical send across its throttle loop, endpoint fallback and reset ladder. Cursor is arunTurnadapter and never sees that context, so the budget reaches its transport through an optionalIncomingMeta.sendBudget, andrunCursorTurnWithRetryadmits each attempt before a transport is built. Every new field is optional and unlimited when absent, so a context-free or meta-free adapter unit test behaves exactly as before.Stacked on #4634.
Verification
Not run, by explicit instruction: the local suite,
bun run typecheck,bun install, and any build. The verification posture for this unit is hosted CI at the exact final head SHA and nothing else; this push used--no-verify.New coverage:
tests/responses/responses-send-budget-counts.test.tsis a table test asserting the exact upstream send count for each failure shape by summinglogCtx.attempts[].sendCountacross combo children, andtests/adapters/adapter-inner-send-budget.test.tsplusadapter-inner-send-budget-wiring.test.tspin that a context without a budget is unlimited while one with a budget is bounded. All registered inscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json.Known open: the Kiro text-fallback rebuild is bounded and now observable, but Cursor's inner retries still report one send in the usage log — making that count visible needs an
onPhysicalSendonIncomingMetaas well, which changes attempt-log content and wants its own diff.structure/transports/responses.mdowns this source area but has no send-budget section today; adding one is a separate cross-lane edit.Checklist
Summary by CodeRabbit