Skip to content

fix: Claude OAuth 429時に前回表示を保持 / Preserve Claude OAuth display on 429#1956

Open
ruushu wants to merge 1 commit into
steipete:mainfrom
ruushu:fix/claude-oauth-rate-limit-stale-cache
Open

fix: Claude OAuth 429時に前回表示を保持 / Preserve Claude OAuth display on 429#1956
ruushu wants to merge 1 commit into
steipete:mainfrom
ruushu:fix/claude-oauth-rate-limit-stale-cache

Conversation

@ruushu

@ruushu ruushu commented Jul 6, 2026

Copy link
Copy Markdown

Problem

Claude OAuth usage refreshes can receive HTTP 429 from Anthropic's OAuth usage endpoint.
Before this change, background refreshes could continue retrying the same account during the cooldown and replace a valid last-good display with a provider-level error.

Validation

  • git diff --check: passed.
  • swift build --target CodexBarCore: passed.
  • swift build --target CodexBarCLI: passed.
  • swift test --filter ClaudeOAuthTests: not completed in this run.
  • make check: portable checks and SwiftFormat passed; full lint did not complete in this run.

Goal

  • Avoid hammering Claude OAuth usage after a 429.
  • Keep the previous Claude usage display when a 429 happens and a last-good snapshot exists.
  • Still show the actionable 429 guidance when there is no previous snapshot to display.

Constraints

  • Do not persist raw Claude access tokens.
  • Keep user-initiated refresh able to bypass the background cooldown.
  • Preserve existing Auto source fallback behavior.
  • Preserve explicit OAuth source semantics.
  • Keep stacked multi-account rows and the selected card on their last-good values when possible.

Approach

  • Scope ClaudeOAuthUsageRateLimitGate by access token fingerprint using the existing SHA-256 prefix helper.
  • Treat a 429 without Retry-After as a 30 minute background cooldown.
  • Add helpers for detecting Claude OAuth usage 429 failures through ClaudeOAuthFetchError and ClaudeUsageError.
  • Preserve the provider-level Claude snapshot and clear provider errors when OAuth 429 happens with prior data.
  • Preserve stacked token account snapshots and selected-card fallback data when OAuth 429 happens with prior account data.
  • Add focused Swift tests for token-scoped cooldowns, default cooldown, provider snapshot preservation, token-account preservation, and no-prior-data error behavior.

Copilot AI review requested due to automatic review settings July 6, 2026 22:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 6, 2026, 7:05 PM ET / 23:05 UTC.

Summary
The PR adds Claude OAuth 429 cooldown scoping, last-good display preservation, and focused OAuth 429 resilience tests.

Reproducibility: yes. from source inspection: current main uses a provider-wide OAuth 429 gate and will surface OAuth 429 failures through normal provider error handling when prior data exists. I did not run live OAuth probes because AGENTS.md warns against validation that can display Keychain prompts without explicit request.

Review metrics: 1 noteworthy metric.

  • PR diff size: 7 files changed; 406 added, 26 removed. The functional diff is focused on Claude OAuth/store handling plus tests, so the remaining blockers are narrow rather than broad rewrite risk.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real behavior proof for the Claude OAuth 429 path and update the PR body.
  • [P2] Fix the Linux token fingerprint fallback and the stale-history recording path.
  • Run the focused Claude OAuth/resilience tests and make check after the code fixes.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists build/check output only; it needs redacted after-fix real behavior proof such as live output, terminal logs, or a reproducible transcript showing Claude OAuth 429 preservation. After adding proof, update the PR body so ClawSweeper re-reviews automatically; if it does not, ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] The PR changes Claude OAuth cooldown and display semantics for multi-account OAuth users, so Linux token-keying and stale-history handling need fixes before merge.
  • [P1] The PR body lists builds/checks but does not include redacted live output, logs, terminal transcript, screenshot, recording, or artifact showing after-fix OAuth 429 behavior.

Maintainer options:

  1. Fix OAuth 429 edge cases before merge (recommended)
    Repair the Linux token fingerprint fallback and avoid recording cached 429 rows as fresh history, then refresh proof against the repaired head.
  2. Hold for manual adoption
    A maintainer can take over the branch if they want the behavior but prefer to rewrite the cooldown/history handling around existing ownership helpers.

Next step before merge

  • [P1] Contributor-side real behavior proof is missing, so ClawSweeper should not open a repair lane until the proof gate is satisfied or a maintainer explicitly takes over.

Security
Cleared: The diff does not add dependencies, broaden secret access, or persist raw Claude tokens; the remaining concerns are functional auth-provider correctness issues.

Review findings

  • [P2] Use a cross-platform token fingerprint — Sources/CodexBarCore/Providers/Claude/ClaudeOAuth/ClaudeOAuthUsageRateLimitGate.swift:48
  • [P2] Avoid recording cached 429 rows as fresh history — Sources/CodexBar/UsageStore+TokenAccounts.swift:1126
Review details

Best possible solution:

Land a narrow repair that keeps OAuth 429 snapshots without recording stale samples, uses a cross-platform token fingerprint, and includes contributor-side real behavior proof for the 429 path.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection: current main uses a provider-wide OAuth 429 gate and will surface OAuth 429 failures through normal provider error handling when prior data exists. I did not run live OAuth probes because AGENTS.md warns against validation that can display Keychain prompts without explicit request.

Is this the best way to solve the issue?

No, not yet: preserving the last-good display is the right direction, but the current patch still collapses token-scoped cooldowns on Linux and records cached 429 rows as fresh history samples.

Full review comments:

  • [P2] Use a cross-platform token fingerprint — Sources/CodexBarCore/Providers/Claude/ClaudeOAuth/ClaudeOAuthUsageRateLimitGate.swift:48
    On Linux CLI builds, sha256Prefix returns nil because it only imports CryptoKit, so this fallback makes every access token use the same claudeOAuthUsageRateLimitBlockedUntilV2.unavailable key. A 429 from one Claude account can therefore block background refreshes for all other accounts; use the existing Crypto fallback pattern instead.
    Confidence: 0.88
  • [P2] Avoid recording cached 429 rows as fresh history — Sources/CodexBar/UsageStore+TokenAccounts.swift:1126
    When a stacked Claude token-account refresh hits an OAuth 429, this returns the prior cached snapshot as usage; refreshTokenAccounts treats any non-nil usage as a freshly fetched sample and later records it with the current time. During a cooldown, repeated retries can write stale usage points into plan-utilization history as if they were real observations; preserve the row via snapshot without marking it as fetched usage.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 8021ed86d5cd.

Label changes

Label justifications:

  • P2: This is a normal-priority provider reliability fix for Claude OAuth 429 handling with limited blast radius but real user-facing display impact.
  • merge-risk: 🚨 compatibility: The PR changes persisted OAuth cooldown keying and token-account history behavior, so upgrades and Linux builds need compatibility-safe handling.
  • merge-risk: 🚨 auth-provider: The changed behavior is specific to Claude OAuth token routing, rate-limit gating, and provider display state.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists build/check output only; it needs redacted after-fix real behavior proof such as live output, terminal logs, or a reproducible transcript showing Claude OAuth 429 preservation. After adding proof, update the PR body so ClawSweeper re-reviews automatically; if it does not, ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

What I checked:

Likely related people:

  • Peter Steinberger: Blame and follow-history on the Claude OAuth rate-limit gate, fetcher, and token-account refresh path point to commit a345464, which added these current-main files around the v0.40.0 release boundary. (role: introduced behavior; confidence: high; commits: a3454640cf53; files: Sources/CodexBarCore/Providers/Claude/ClaudeOAuth/ClaudeOAuthUsageRateLimitGate.swift, Sources/CodexBarCore/Providers/Claude/ClaudeOAuth/ClaudeOAuthUsageFetcher.swift, Sources/CodexBar/UsageStore+TokenAccounts.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (2 earlier review cycles)
  • reviewed 2026-07-06T22:35:30.760Z sha c5a95f2 :: needs real behavior proof before merge. :: [P2] Avoid recording cached 429 rows as fresh history
  • reviewed 2026-07-06T22:58:10.604Z sha c5a95f2 :: needs real behavior proof before merge. :: [P2] Use a cross-platform token fingerprint | [P2] Avoid recording cached 429 rows as fresh history

@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: c5a95f2ab2

ℹ️ 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".


private static func blockedUntilKey(accessToken: String) -> String {
let data = Data(accessToken.utf8)
let fingerprint = ClaudeOAuthCredentialsStore.sha256Prefix(data) ?? "unavailable"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Fix token hash fallback for Linux OAuth cooldowns

On Linux CLI builds, ClaudeOAuthCredentialsStore.sha256Prefix returns nil because that helper only uses CryptoKit while the package's Linux dependency is the Crypto module, so this fallback makes every access token use the same claudeOAuthUsageRateLimitBlockedUntilV2.unavailable key. In multi-account OAuth usage, a 429 from one Claude account will therefore block background refreshes for all other accounts for the cooldown window, defeating the token-scoped gate and leaving valid accounts stale. Use the existing Crypto fallback path here instead of collapsing to a shared key.

Useful? React with 👍 / 👎.

Comment on lines +1124 to +1127
return ResolvedAccountOutcome(
snapshot: snapshot,
usage: priorUsage,
sourceLabel: priorSnapshot.sourceLabel)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid recording cached 429 rows as fresh history

When a stacked Claude token-account refresh hits an OAuth 429, this returns the prior cached snapshot as usage; refreshTokenAccounts treats every non-nil resolved.usage as a newly fetched sample and later records it with the current Date(). During a cooldown/outage, each retry can write stale usage points into plan-utilization history and reset detection as if they were real observations. Preserve the row via snapshot, but don't mark cached 429 data as a fetched usage sample.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants