Skip to content

fix(sidecars): rebind the sidecar retry request after OAuth rotation - #5238

Merged
lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:fix/kiro-oauth-rebind
Sep 20, 2026
Merged

lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:fix/kiro-oauth-rebind

Conversation

@luvs01

@luvs01 luvs01 commented Sep 20, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Fix a mixed-identity bug in the sidecar 429 rotation path. runWithWebSearch and runWithImageBridge build each attempt from an iteration-local shallow copy (iterParsed) of the outer parsed request, but rotateSidecarProviderOn429 rebound only the outer parsed. On a Kiro OAuth rotation the retried request kept the failed account's _kiroAuthContext (region/profile) and continuation/replay scope — pairing the new account's bearer with the previous account's routing metadata.
  • The on429 hook now receives the exact iterParsed the retry is built from. The shared hook hands it to applyFailoverSnapshot (which already syncs _kiroAuthContext for a supplied retry request) and rebinds the route reasoning-replay/continuation scope on it — the same pattern the continuation loop already uses for nextParsed.

Verification

  • bun test tests/images/loop.test.ts tests/web-search/web-search.test.ts tests/oauth/generic-oauth-failover.test.ts — 176 pass, 0 fail
  • bun test tests/web-search/web-search-sidecar-429.test.ts tests/adapters/anthropic/anthropic-sidecar-account-failover.test.ts tests/web-search/web-search-timeout-contract.test.ts — 11 pass, 0 fail
  • bun x tsc --noEmit — clean
  • bun run test:changed — ran the changed-file suite; the only failures are pre-existing environmental ones on this Windows host (SpendLedgerOwnerError in tests/codex-integration/bearer-admission-routed-provider.test.ts, plus temp-file EBUSY/hook timeouts). The same failures reproduce on a clean dev checkout with these changes stashed.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes

    • Improved retry handling after rate-limit responses for image, video, and web-search requests.
    • Ensured retried requests use refreshed authentication details and correctly clear outdated provider continuation data.
    • Preserved request-specific routing information when switching providers during failover.
  • Tests

    • Added coverage confirming refreshed authentication and request metadata are applied successfully during retries.

The web-search and image-bridge loops build each attempt from an iteration-local shallow copy (iterParsed) of the outer parsed request, but rotateSidecarProviderOn429 rebound only the outer parsed. On a Kiro OAuth rotation the retried request kept the failed account's _kiroAuthContext and continuation/replay scope, pairing the new account's bearer with the previous account's routing metadata.

The on429 hook now receives the exact iterParsed the retry is built from. The shared hook hands it to applyFailoverSnapshot, which already syncs _kiroAuthContext for a supplied retry request, and rebinds the route reasoning-replay/continuation scope on it — the same pattern the continuation loop uses for nextParsed.
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 20, 2026
@github-actions

github-actions Bot commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

@coderabbitai

coderabbitai Bot commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7258170f-18b6-45fe-a52f-3845a2b03eee

📥 Commits

Reviewing files that changed from the base of the PR and between 2a2c01a and 44deec0.

📒 Files selected for processing (6)
  • src/images/loop.ts
  • src/server/responses/sidecar-execution.ts
  • src/web-search/loop.ts
  • tests/images/loop.test.ts
  • tests/oauth/generic-oauth-failover.test.ts
  • tests/web-search/web-search.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

Changes

429 retry request propagation

Layer / File(s) Summary
Retry contracts and loop wiring
src/images/loop.ts, src/web-search/loop.ts, src/server/responses/sidecar-execution.ts
Image and web-search 429 callbacks now accept the iteration-local OcxParsedRequest. The loops pass that request to the rotation callbacks.
Sidecar retry rebinding
src/server/responses/sidecar-execution.ts
Sidecar failover applies the snapshot and reasoning-replay routing metadata to the retry request before the retry is built.
Retry behavior validation
tests/images/loop.test.ts, tests/web-search/web-search.test.ts, tests/oauth/generic-oauth-failover.test.ts
Tests verify that rotated authentication reaches the rebuilt request, provider continuation is removed, and shared sidecar wiring uses retryParsed.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant SidecarLoop
  participant rotateSidecarProviderOn429
  participant applyFailoverSnapshot
  participant RotatedAdapter
  SidecarLoop->>rotateSidecarProviderOn429: pass iteration-local retryParsed
  rotateSidecarProviderOn429->>applyFailoverSnapshot: apply snapshot to retryParsed
  rotateSidecarProviderOn429->>RotatedAdapter: rebind routing metadata
  RotatedAdapter-->>SidecarLoop: build request from retryParsed
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 6 files.
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: rebinding the sidecar retry request after OAuth rotation. It matches the changes in the image and web-search retry paths.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

❤️ Share

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

@github-actions
github-actions Bot marked this pull request as draft September 20, 2026 02:03
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 64 / 80

웹 검색이랑 이미지(영상 포함) 사이드카가 429를 맞고 계정을 바꿀 때, 새 계정 토큰과 옛 계정 길이 서로 안 맞던 구멍을 막습니다. 루프는 매 시도마다 parsed를 얕게 복사한 iterParsed로 요청을 만듭니다. 그런데 예전 rotateSidecarProviderOn429는 바깥 parsed만 고쳤습니다. 그래서 다시 보낼 때는 새 토큰인데, Kiro 지역·프로필(_kiroAuthContext)과 이어가기 정보는 실패한 계정 것이 남았습니다.

이번 고침은 on429에 그 iterParsed를 넘겨서, applyFailoverSnapshot이 그 객체에도 Kiro 문맥을 넣고, bindRouteReasoningReplayScope도 그 객체에 다시 묶습니다. 이어가기 루프가 nextParsed에 하던 방식과 같습니다. 베이스는 dev입니다. 이 글을 쓰는 지금 dev 끝은 2a2c01a7102e입니다. #5224입니다. 패키지 버전은 2.60.0입니다. 이 브랜치는 tip보다 커밋 5개가 뒤입니다. 겹치는 파일은 없어 보입니다. types.ts와 config.ts는 안 바꿉니다. 상태는 드래프트이고, 준비 체크 네 칸이 비어 있습니다. hygiene와 대상 브랜치 검사는 통과했습니다. 같은 수정을 하는 다른 열린 PR은 없습니다. 이 조회에서 테스트는 다시 돌리지 않았습니다.

src/server/responses/sidecar-execution.ts rotateSidecarProviderOn429 - 이어가기 쪽은 bindRouteReasoningReplayScope에 oauthCredentialSnapshot: transportState.replayOAuthCredentialSnapshot을 넣습니다. 사이드카 훅은 바깥 parsed든 retryParsed든 그 칸을 안 넣습니다. applyFailoverSnapshot이 스냅샷은 갱신합니다. 그런데 사이드카 Pick 타입에 replayOAuthCredentialSnapshot이 없어서, 이 PR도 그걸 넘기지 않습니다. Kiro 지역·프로필은 고쳐집니다. OAuth 계정 지문이 빠지면 이어가기·추론 재생 주인 판정이 이어가기 경로와 달라질 수 있습니다.

tests/images/loop.test.ts, tests/web-search/web-search.test.ts - on429를 테스트가 직접 만듭니다. 테스트 안에서 _kiroAuthContext를 바꾸고 _providerContinuation을 지운 뒤, buildRequest가 그 값을 받는지 봅니다. 공유 훅 rotateSidecarProviderOn429 → applyFailoverSnapshot → bindRouteReasoningReplayScope가 한 줄로 이어지는지는 이 두 파일이 안 봅니다. tests/oauth/generic-oauth-failover.test.ts는 소스 문자열에 applyFailoverSnapshot(snapshot, retryParsed)와 parsed: retryParsed가 있는지만 봅니다.

메인테이너의 판단이 필요한 지점

사이드카 429 회전에도 이어가기와 같이 oauthCredentialSnapshot을 넣을지 정해야 합니다. 이 PR 범위 밖이어도, 계정 바뀐 뒤 주인 판정을 이어가기와 같게 둘지 말지는 메인테이너 몫입니다.

작성자는 test:changed 실패가 지금 dev에도 난다고 적었습니다. 그 말이 맞다면 이 PR만의 문제는 아닙니다. Ready 전에 CI로 한 번 더 보면 됩니다.

너의 추천

구멍의 원인과 고치는 방향은 맞습니다. iterParsed를 넘기고 스냅샷·스코프를 그 객체에 다시 묶는 쪽을 유지하세요. Ready로 올리기 전에 사이드카 bind에도 oauthCredentialSnapshot을 넣을지 짧게 정하고, 넣기로 하면 공유 훅을 한 번 타는 테스트를 추가하세요. 지금은 드래프트로 두세요. 머지하지 마세요. tip의 커밋 5개는 이 파일과 안 겹칩니다. types.ts/config.ts 분할과 무관해서 이 PR을 닫지 마세요.

이 댓글은 grok-bot이 작성했습니다

@luvs01
luvs01 marked this pull request as ready for review September 20, 2026 02:12
@lidge-jun
lidge-jun merged commit 148b09b into lidge-jun:dev Sep 20, 2026
14 of 15 checks passed
@luvs01
luvs01 deleted the fix/kiro-oauth-rebind branch September 20, 2026 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants