Skip to content

fix(usage): seal a credential-rotation attempt with the status it answered - #7321

Closed
vadymhimself wants to merge 1 commit into
lidge-jun:devfrom
vadymhimself:fix/attempt-row-credential-recovery-status
Closed

vadymhimself wants to merge 1 commit into
lidge-jun:devfrom
vadymhimself:fix/attempt-row-credential-recovery-status

Conversation

@vadymhimself

@vadymhimself vadymhimself commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • When an account rotation splits the attempt row (noteProviderAttemptSend with a changed account label or account-qualified provider), the sealed attempt that has no recorded status takes one from the recovery kind. That mapping covered only key-401 -> 401 and *-429 -> 429; every other kind fell to 502. An Anthropic/OAuth oauth-account-403 rotation was therefore logged as a 502 server error, and the same for oauth-401 and oauth-account-402, although each kind names the status the account actually returned.
  • The mapping now also gives oauth-401 -> 401, oauth-account-403 -> 403 and oauth-account-402 -> 402. A status already recorded on the attempt still wins, and the failure cause (already derived from the recovery kind) is unchanged.
  • Usage-log only, in src/server/request-log.ts; routing, cooldowns and the client response are unchanged.

Verification

  • New parameterized test an account rotation after %s seals the first row with the status it answered in tests/usage/key-attribution.test.ts, run before the src change: bun test tests/usage/key-attribution.test.ts -> 17 pass, 3 fail (Expected "status": 403 / 401 / 402, Received "status": 502). After the change: 20 pass, 0 fail.
  • bun x tsc --noEmit -> exit 0. bun scripts/file-size-ratchet.ts -> passed.
  • bun run test:changed (Bun 1.4.3 via OCX_TEST_RUNNER_BUN) -> 16947 pass, 34 skip, 1 fail across 692 files. The single failure, tests/cli/cli-connect-readiness.test.ts "a rejected preferred runtime falls back without rewriting the saved selection", fails identically on an unmodified upstream/dev 8640c2f checkout on this machine (local-environment dependent) and is unrelated to the usage log.
  • Full bun run test was not run locally: test:changed already covered 692 files through the changed module's import graph; the remainder is left to CI.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (No docs change needed: logged status only.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (No secrets or auth behaviour touched.)

Source: carried from the vadymhimself fork build (commit ef65d20, status-mapping half only), re-authored against current dev. The fork's other half, splitting the row on a same-account oauth-401 replay, is deliberately not included: it contradicts tests/server/server-xai-oauth-401-replay.test.ts (sendCount 2).

🤖 Generated with Claude Code

Review readiness checklist

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

  • Required local validation passed; commands, results, and any full-suite exception are documented.

  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes
    • Recovery attempts now retain the appropriate 401, 403, or 402 status when an account identity changes. Existing status codes are preserved.

…wered

When an account rotation splits the attempt row, the sealed attempt with no
recorded status fell back to 401 for key-401, 429 for any *-429 kind, and 502
for everything else. An `oauth-account-403` rotation was therefore logged as a
502 server error, and `oauth-401` / `oauth-account-402` likewise, although the
recovery kind names the status the account actually returned.

Map those kinds to 401, 403 and 402 too. A status already recorded on the
attempt still wins.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@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 Oct 18, 2026
@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

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

Review readiness checklist

  • ✅ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ✅ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

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

@github-actions
github-actions Bot marked this pull request as draft October 18, 2026 15:32
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Review in 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: 9b863693-8f49-4c67-9327-edec66a33390
📥 Commits

Reviewing files that changed from the base of the PR and between 8640c2f and c42a453.

📒 Files selected for processing: 2
  • src/server/request-log.ts
  • tests/usage/key-attribution.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

📝 Walkthrough

When account identity changes during recovery, the attempt log now records a recovery-specific status if the attempt has no existing status. A parameterized test checks the 401, 403, and 402 mappings.

Changes: Attempt status recording

Layer / File(s) Summary
Recovery status mapping and test coverage
src/server/request-log.ts, tests/usage/key-attribution.test.ts
When an account rotation seals an attempt without an existing status, oauth-401 maps to 401, oauth-account-403 maps to 403, and oauth-account-402 maps to 402. Existing status takes precedence; other recovery kinds retain the 429-or-502 fallback. The test checks the recorded status for all three mappings.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to c42a4

Rotation logging now records the recovery’s 401/403/402 status when no upstream status exists, while preserving any status already recorded; request behavior is unchanged. No concrete PR-introduced merge risk is established, though the reported CLI readiness failure was not identified or verified at the exact base.

Pre-merge checks: Passed 5
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 1 functions across 2 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: sealing credential-rotation attempts with the status returned by the recovery response.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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 ready for review October 18, 2026 15:39
@Ingwannu

Copy link
Copy Markdown
Owner

Intake/scoped review at c42a4532c8af29a35cb88d6d88334fc919183254.

The sealed attempt now maps oauth-401, oauth-account-402 and oauth-account-403 to the actual account refusal status, while an already-recorded status remains authoritative. Keep credential identity and recovery-kind attribution separate from this status correction. The new parameterized cases cover the three previously misclassified values.

Current-head functional workflows are awaiting fork approval; that is pending execution, not a passing test result. No local suite was run for this branch in this intake. No workflow approval, new scan, live configuration change or merge was performed. Required review/readiness gates remain in force.

@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev as part of the dispatch/usage/failover bundle #7389 (squash bddd61f), with Co-authored-by credit. Adaptations are listed in #7389 (ratchet move for #7323/#7373; the Kiro hop is now fenced to the adapter the failed leg actually resolved). Closing as absorbed. Thank you!

@lidge-jun lidge-jun closed this Oct 19, 2026
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.

3 participants