Skip to content

fix(claude): bound account switch reconciliation - #3812

Closed
steipete wants to merge 3 commits into
mainfrom
triage/20260921-small-prs-a
Closed

steipete wants to merge 3 commits into
mainfrom
triage/20260921-small-prs-a

Conversation

@steipete

Copy link
Copy Markdown
Owner

A stalled ambient Claude refresh could leave claude-swap account chips disabled after activation. This bounds that wait to five seconds while preserving adapter-list reconciliation, serialized credential transactions, and late ambient results. A subsequent switch can reconcile its adapter even when its ambient refresh is queued behind a stalled predecessor. The task-group timeout proposed in #3750 was rewritten to use the existing bounded join, because a task group still waits for a suspended child.

Muse now names Disable Keychain access when it blocks a Keychain-only login. Active subscriptions with omitted or null subs_usage retain plan/identity and an unavailable-quota note; malformed quota payloads still fail. Credential precedence and no-prompt behavior are preserved. Thanks @keepitmello and @oldcai!

TRY support from @aguvener is already on main in ec12a0243d2a (#3624). The currency follow-up only shares the existing conversion path and removes redundant private rate/cache wrappers, with isolated offline fallback/cache tests. Production code is net −5 lines (6 files changed, 75 insertions(+), 80 deletions(-)); tests are 5 files changed, 168 insertions(+), 2 deletions(-).

Verification

Final rebased branch:

CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test -j 2 --no-parallel --filter 'ClaudeSwap|ClaudeProviderRuntimeTests|BoundedTaskJoinTests|MuseCredentialsTests|MusePluginTests|CodexBarPluginTests|UsageFormatterTests|SettingsStoreCoverageTests|ProviderArchitectureGatekeeperTests'
make check

Results: 352 tests in 27 suites, 99 portable plugin tests in 10 suites, and 14 CLI swap tests passed. XCTest reported 10 passing tests and 3 skipped opt-in native UI proofs. make check passed with 0 violations in 2,517 files. Independent reviews of Claude, Muse, and the complete rebased branch found no actionable P0–P2 issues.

Regression checks, run before their production fixes:

CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test -j 2 --filter ClaudeSwapSwitchReconciliationTests
CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test -j 2 --no-parallel --filter 'MuseCredentialsTests|MusePluginTests'

Claude: 3 existing tests passed; the 2 new tests failed with 3 issues while the ambient probe stayed suspended. Muse: 16 tests passed; the 2 new behavior tests failed with 6 issues. Both regressions pass after the fixes. An initial broad concurrent Claude run hit 23 timing issues in 3 existing subprocess tests; the repository-standard serial rerun and final run passed.

All account proof uses synthetic adapters, credentials, responses, or in-memory defaults. No real account probes, Keychain prompts, cookie imports, or app relaunches were used. Bundled Muse JavaScript was regenerated with Scripts/regenerate-plugin-js.sh --write.

Closes #3750
Closes #3772
Refs #3736
Refs #3624

steipete and others added 3 commits September 20, 2026 23:56
Bound the ambient refresh wait without cancelling the provider request, and
refresh the adapter independently when a stalled predecessor delays that read.
Keep credential transactions serialized through the current adapter list.

Adopts #3750 with a bounded join instead of a task group that still waits for
its suspended child. Refs #3736.

Co-authored-by: keepitmello <keepitmello@users.noreply.github.com>
Name the disabled Keychain setting without changing credential precedence or
prompt policy. Keep active subscription identity and an unavailable-quota
note when subs_usage is omitted or null, while rejecting malformed payloads.
Share snapshot fields and remove redundant credential checks.

Adopts #3772. Verified with synthetic credentials and both plugin engines.

Co-authored-by: oldcai <hi@oldcai.com>
Route USD conversion through the existing pivot and remove one-use cache and
rate wrappers. Preserve public initialization, normalized currency codes,
cached-rate precedence, and locking. Use in-memory defaults for offline
fallback and cache coverage.

Follow-up cleanup for the converter reviewed in #3624, whose TRY support is
already on main. No currency behavior changes are intended.

Co-authored-by: aguvener <90110302+aguvener@users.noreply.github.com>
@clawsweeper

clawsweeper Bot commented Sep 21, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 21, 2026
@clawsweeper

clawsweeper Bot commented Sep 21, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 21, 2026, 3:25 AM ET / 07:25 UTC.

ClawSweeper review

What this changes

The branch bounds Claude account-switch refresh waits, improves Muse login diagnostics and missing-quota handling, and consolidates currency conversion and cache access.

Merge readiness

Ready for maintainer review

The fixes remain useful: current main still waits indefinitely for ambient Claude refresh and rejects windowless Muse subscriptions. No blocking defect was found in the introduced changes. This owner-authored PR is not eligible for cleanup closure.

Priority: P2
Reviewed head: f8052f911e481e86584853e361df7ce4c8b4a791

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, maintainable repair with relevant regression coverage and no blocking source finding.
Proof confidence 🌊 off-meta tidepool Not applicable: OWNER-authored PR: the external-contributor proof gate does not apply. Reported synthetic checks exercise switch reconciliation and Muse parsing; inspected TRY screenshots establish earlier currency behavior only.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: OWNER-authored PR: the external-contributor proof gate does not apply. Reported synthetic checks exercise switch reconciliation and Muse parsing; inspected TRY screenshots establish earlier currency behavior only.
Evidence reviewed 8 items Policy and patch identity: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-notes directory was found. Reviewed the pinned main-to-head introduced diff. The working tree remained clean; no builds, tests, or account probes were executed.
Claude timeout preserves reconciliation: The five-second join cancels a wrapper waiter rather than the ambient provider request. After timeout, an unchanged adapter-task identity triggers an independent read; the existing loop still waits for adapter replacements within the original configuration. Provider refresh generation checks reject superseded ambient results.
Main and release still need the Claude fix: Pinned main and latest release v0.63.0 retain the unconditional ambient-refresh await before adapter reconciliation. The merged TRY work at #3624 does not implement the Claude or Muse repairs.
Findings None None.
Security None None.

How this fits together

CodexBar turns provider credentials, usage responses, and exchange rates into menu-bar account status and spending displays. Claude switching reconciles an external account adapter with ambient usage, while Muse parses subscription responses through its bundled provider plugin.

flowchart TD
 A[Account switch] --> B[Serialized adapter activation]
 B --> C[Bounded ambient refresh wait]
 C --> D[Adapter reconciliation]
 D --> G[Account and usage display]
 E[Muse login and subscription response] --> G
 F[Cached or live currency rates] --> H[Converted spending display]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +75/−80 (net −5); tests +168/−2 (net +166) The fixes reuse existing mechanisms and add focused regression coverage without net production growth.

Technical review

Best possible solution:

Keep account activation serialized through adapter reconciliation while allowing slow ambient usage to finish independently, and display unavailable Muse quota honestly without changing credential selection.

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

Yes, from source: suspend ambient Claude refresh after activation, or supply an active Muse response with omitted/null quota windows. Current main exposes both failure paths; this read-only review did not execute them.

Is this the best way to solve the issue?

Yes. Reusing the bounded join avoids structured-task timeout draining, preserves adapter serialization, and keeps Muse recovery within existing credential and parser boundaries.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against ec12a0243d2a.

Labels

Label changes:

  • add P2: This repairs bounded Claude account-switch responsiveness and Muse subscription display problems without evidence of a product-wide outage.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: OWNER-authored PR: the external-contributor proof gate does not apply. Reported synthetic checks exercise switch reconciliation and Muse parsing; inspected TRY screenshots establish earlier currency behavior only.

Label justifications:

  • P2: This repairs bounded Claude account-switch responsiveness and Muse subscription display problems without evidence of a product-wide outage.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: OWNER-authored PR: the external-contributor proof gate does not apply. Reported synthetic checks exercise switch reconciliation and Muse parsing; inspected TRY screenshots establish earlier currency behavior only.

Evidence

What I checked:

  • Policy and patch identity: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-notes directory was found. Reviewed the pinned main-to-head introduced diff. The working tree remained clean; no builds, tests, or account probes were executed. (AGENTS.md:1, f8052f911e48)
  • Claude timeout preserves reconciliation: The five-second join cancels a wrapper waiter rather than the ambient provider request. After timeout, an unchanged adapter-task identity triggers an independent read; the existing loop still waits for adapter replacements within the original configuration. Provider refresh generation checks reject superseded ambient results. (Sources/CodexBar/Providers/Claude/UsageStore+ClaudeSwapRefresh.swift:182, f8052f911e48)
  • Main and release still need the Claude fix: Pinned main and latest release v0.63.0 retain the unconditional ambient-refresh await before adapter reconciliation. The merged TRY work at feat: add Turkish Lira (TRY) to supported currencies #3624 does not implement the Claude or Muse repairs. (Sources/CodexBar/Providers/Claude/UsageStore+ClaudeSwapRefresh.swift:182, f3e718c897d5)
  • Muse repair preserves existing security boundaries: Inline credentials retain precedence, invalid inline tokens still fail, and Keychain reads retain the access gate and no-UI query. The plugin only accepts absent/null quota windows after confirming an active subscription; malformed quota objects still fail. Endpoint, bearer credential, permissions, and plugin API contracts are unchanged. (Sources/CodexBarCore/Resources/Plugins/muse.ts:63, f8052f911e48)
  • Currency compatibility: The public initializer still uses standard defaults, existing cache keys and values are preserved, and conversion delegates to the existing USD pivot. Added in-memory tests cover empty/default caches, normalized codes, unavailable currencies, and cached-rate precedence without changing the persisted format. (Sources/CodexBarCore/CurrencyExchange.swift:48, f8052f911e48)
  • Validation and proof scope: The captured PR body reports passing focused Swift, portable-plugin, CLI-swap, and lint checks, including before-fix regression failures. Its account validation explicitly uses synthetic adapters and responses. The three locally inspected screenshots belong to the earlier TRY PR and show currency selection and USD/TRY totals, not this head's Claude or Muse behavior. OWNER authorship exempts this PR from the external-contributor live-proof gate. (f8052f911e48)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • aguvener: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

steipete added a commit that referenced this pull request Sep 21, 2026
Bound the ambient refresh wait without cancelling the provider request, and
refresh the adapter independently when a stalled predecessor delays that read.
Keep credential transactions serialized through the current adapter list.

Adopts #3750 with a bounded join instead of a task group that still waits for
its suspended child. Refs #3736.

Co-authored-by: keepitmello <keepitmello@users.noreply.github.com>
(cherry picked from commit b73c145)
steipete added a commit that referenced this pull request Sep 21, 2026
Name the disabled Keychain setting without changing credential precedence or
prompt policy. Keep active subscription identity and an unavailable-quota
note when subs_usage is omitted or null, while rejecting malformed payloads.
Share snapshot fields and remove redundant credential checks.

Adopts #3772. Verified with synthetic credentials and both plugin engines.

Co-authored-by: oldcai <hi@oldcai.com>
(cherry picked from commit 4d8a9ab)
steipete added a commit that referenced this pull request Sep 21, 2026
Route USD conversion through the existing pivot and remove one-use cache and
rate wrappers. Preserve public initialization, normalized currency codes,
cached-rate precedence, and locking. Use in-memory defaults for offline
fallback and cache coverage.

Follow-up cleanup for the converter reviewed in #3624, whose TRY support is
already on main. No currency behavior changes are intended.

Co-authored-by: aguvener <90110302+aguvener@users.noreply.github.com>
(cherry picked from commit f8052f9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant