Skip to content

Expose all provider accounts from serve - #2827

Closed
VACInc wants to merge 1 commit into
steipete:mainfrom
VACInc:feat/serve-multi-account-all-providers
Closed

VACInc wants to merge 1 commit into
steipete:mainfrom
VACInc:feat/serve-multi-account-all-providers

Conversation

@VACInc

@VACInc VACInc commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Draft / WIP. Authored against main; the change is a close-read patch that has not been compiled or test-run on macOS yet (see "What was not tested"). Opening as a draft to gather direction before running the full build+test matrix.

Summary

High Level TLDR

codexbar serve exposed multiple Codex and claude-swap accounts, but ordinary token-account providers only returned the
active account. This change makes /usage fetch every configured account and makes dashboard snapshots project those
results into one provider row with accounts[], using the same configured account order and active selection as the app.

Root Cause

The app menu reads all ProviderTokenAccountData.accounts, but the headless resolver returned only the clamped active
account unless an interactive CLI caller explicitly passed --all-accounts. The dashboard also disabled its existing
all-account mode and only attached nested accounts through the claude-swap adapter.

Before behavior was established by source and observed route output: Codex /usage returned multiple rows, while a
generic provider with multiple configured accounts returned only one active row and no dashboard accounts field.

What changed

  • Generalized serve account collection to every token-account provider.
  • Kept Codex on its reconciled live, managed, and profile-home account projection.
  • Grouped account usage into providers[].accounts, with the active account retaining provider-level fields.
  • Kept single-account providers on the existing shape.
  • Preserved claude-swap precedence and separated its cache/in-flight account mode.
  • Added opaque account IDs, identity redaction, per-account errors, and collection-level accountsError handling.

Real behavior proof

Linux static proof completed:

  • git diff --check a0d77c37f9cc85d8703eb0a501c6764112f5a6e8..HEAD passed.
  • The final patch exactly matches commit ee0ae94e312007e63ee42b8226a7ff2e3f0e3ef8.

macOS compilation, focused tests, and live fixture route output are not available from this Linux environment. The
exact macOS commands and route matrix are documented in REPORT-IMPL.md; their copied output must be added before merge.

Verification

Added DashboardMultiAccountSnapshotTests for generic projection, active selection, redaction, opaque IDs, row-local
failure, collection failure, single-account compatibility, and headless all-account resolution. Updated serve cache-mode
coverage and retained the existing claude-swap snapshot suite.

Pending on macOS:

  • swiftformat Sources Tests
  • swiftlint --strict
  • focused dashboard, claude-swap, and serve-router tests
  • swift build
  • make test
  • make check
  • before/after route performance samples

What was not tested

  • Compilation or tests, because this Linux host has no Swift toolchain.
  • AppKit/macOS runtime behavior.
  • Real provider, Keychain, bridge, or network account calls.
  • Before/after latency and provider call volume.

@clawsweeper

clawsweeper Bot commented Aug 10, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

@clawsweeper clawsweeper Bot added merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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. labels Aug 10, 2026
@clawsweeper

clawsweeper Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 2, 2026, 12:28 PM ET / 16:28 UTC.

ClawSweeper review

What this changes

The PR makes serve-mode account collection include every configured token account and groups those results into nested dashboard account rows.

Merge readiness

Blocked until real behavior proof is added - 6 items remain

Keep this draft open: the multi-account dashboard work is useful, but the branch also changes the established generic /usage response from the selected account to every configured account, and it still lacks real after-fix behavior proof.

Priority: P2
Reviewed head: ee0ae94e312007e63ee42b8226a7ff2e3f0e3ef8
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The patch has focused unit coverage and a clear goal, but an unresolved P1 compatibility regression and absent real behavior proof make it unready to merge.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The changed production owner is serve’s account-selection path into provider usage fetches, but the PR body reports no macOS build, focused run, or redacted after-fix /usage and dashboard snapshot result from a configured multi-account setup. Add those real outputs after the compatibility repair, redact account names/tokens/private endpoints, and update the PR body for re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The changed production owner is serve’s account-selection path into provider usage fetches, but the PR body reports no macOS build, focused run, or redacted after-fix /usage and dashboard snapshot result from a configured multi-account setup. Add those real outputs after the compatibility repair, redact account names/tokens/private endpoints, and update the PR body for re-review.
Evidence reviewed 5 items Introduced compatibility regression: The introduced generic resolver call receives includeAllAccounts; the normal /usage route constructs its context without an override, whose default is true, so configured non-Codex providers now fetch and emit every account instead of their selected account.
Current-main contract: Current main documents --all-accounts as the explicit generic multi-account action and limits serve’s all-account guarantee to visible Codex accounts; its generic resolver continues to return the clamped active account absent that selection.
Feature history: The existing dashboard account projection and serve behavior were recently established by commits from Peter Steinberger, including the claude-swap dashboard feature and serve dashboard work; the current branch has not changed since the prior review cycle.
Findings 1 actionable finding [P1] Preserve the selected-account /usage default
Security None None.

How this fits together

CodexBar’s local serve command reads configured provider accounts, fetches usage, and exposes it through /usage and dashboard snapshots. Those API responses feed local polling clients and the built-in dashboard.

flowchart LR
    A[Provider account configuration] --> B[Serve route]
    B --> C[Account selection]
    C --> D[Provider usage fetch]
    D --> E[/usage response]
    D --> F[Dashboard snapshot]
    F --> G[Nested account rows]
Loading

Decision needed

Question Recommendation
Should generic GET /usage remain selected-account compatible while dashboard snapshots gain all configured accounts, or should its response cardinality intentionally expand? Keep /usage selected-only: Use all-account collection only for dashboard snapshots and retain the existing generic /usage cardinality.

Why: The branch’s central behavior changes a documented, existing API selection contract and can add credential refreshes for existing polling clients; that compatibility tradeoff requires owner intent.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The changed production owner is serve’s account-selection path into provider usage fetches, but the PR body reports no macOS build, focused run, or redacted after-fix /usage and dashboard snapshot result from a configured multi-account setup. Add those real outputs after the compatibility repair, redact account names/tokens/private endpoints, and update the PR body for re-review.
  • Preserve the selected-account `/usage` default (P1) - This newly passes the route’s default all-account mode into the generic token resolver. Consequently, a normal /usage request for a provider with two configured accounts emits and refreshes both accounts, whereas current main reserves generic multi-account selection for --all-accounts and documents serve’s all-account behavior only for Codex. The prior review raised this same unresolved blocker at this head; keep /usage selected-only and enable aggregation only for dashboard collection unless the API contract is deliberately revised.
  • Resolve merge risk (P1) - Existing generic /usage clients can receive multiple rows and trigger refreshes for every configured credential, changing both response cardinality and request latency within the existing route.
  • Resolve merge risk (P1) - The branch is draft, has a dirty merge state against current main, and its required macOS validation and real route evidence remain unavailable.
  • Complete next step (P2) - Rebase, resolve the /usage selected-account compatibility decision, run the required macOS checks, and add redacted after-fix /usage plus dashboard snapshot evidence.

Findings

  • [P1] Preserve the selected-account /usage default — Sources/CodexBarCLI/CLIUsageCommand.swift:288-290
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 15 files; production +193/-44, tests +234/-20, docs +22/-16 The account-selection behavior spans serving, payload projection, documentation, and focused tests, so the API compatibility boundary needs a refreshed review after rebase.

Merge-risk options

Maintainer options:

  1. Split the two route collection modes (recommended)
    Restore selected-account collection for generic /usage and retain all-account collection only for dashboard snapshots, then prove both routes on macOS.
  2. Approve and version the API change
    If all-account /usage is intended, document its upgrade impact and establish explicit compatibility behavior for existing polling clients.

Technical review

Best possible solution:

Keep generic /usage selected-account compatible and use all-account collection for dashboard snapshots; only expand /usage after an explicit, documented compatibility decision with upgrade proof.

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

Yes, source-reproducible: configure two generic provider token accounts and request /usage; the introduced includeAllAccounts path resolves both rather than the active account.

Is this the best way to solve the issue?

No: the dashboard aggregation goal can be implemented without silently changing generic /usage; preserve its selected-account default unless the maintainer explicitly adopts a new API contract.

Full review comments:

  • [P1] Preserve the selected-account /usage default — Sources/CodexBarCLI/CLIUsageCommand.swift:288-290
    This newly passes the route’s default all-account mode into the generic token resolver. Consequently, a normal /usage request for a provider with two configured accounts emits and refreshes both accounts, whereas current main reserves generic multi-account selection for --all-accounts and documents serve’s all-account behavior only for Codex. The prior review raised this same unresolved blocker at this head; keep /usage selected-only and enable aggregation only for dashboard collection unless the API contract is deliberately revised.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 620f3b8e4eef.

Labels

Label justifications:

  • P2: This is a bounded local API/dashboard improvement with a concrete compatibility blocker.
  • merge-risk: 🚨 compatibility: Generic /usage changes from selected-account output to multiple configured-account rows for existing users.
  • merge-risk: 🚨 auth-provider: The changed route selects and refreshes additional configured provider credentials.
  • merge-risk: 🚨 availability: Fetching every configured account can increase route work and expose more per-account failures or deadline pressure.
  • 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 changed production owner is serve’s account-selection path into provider usage fetches, but the PR body reports no macOS build, focused run, or redacted after-fix /usage and dashboard snapshot result from a configured multi-account setup. Add those real outputs after the compatibility repair, redact account names/tokens/private endpoints, and update the PR body for re-review.

Evidence

What I checked:

  • Introduced compatibility regression: The introduced generic resolver call receives includeAllAccounts; the normal /usage route constructs its context without an override, whose default is true, so configured non-Codex providers now fetch and emit every account instead of their selected account. (Sources/CodexBarCLI/CLIUsageCommand.swift:288, ee0ae94e3120)
  • Current-main contract: Current main documents --all-accounts as the explicit generic multi-account action and limits serve’s all-account guarantee to visible Codex accounts; its generic resolver continues to return the clamped active account absent that selection. (docs/cli.md:195, 620f3b8e4eef)
  • Feature history: The existing dashboard account projection and serve behavior were recently established by commits from Peter Steinberger, including the claude-swap dashboard feature and serve dashboard work; the current branch has not changed since the prior review cycle. (Sources/CodexBarCLI/CLIServeCommand.swift:176, b1b80a9b3bc1)
  • Proof remains absent: The PR body explicitly says macOS compilation, focused tests, live fixture route output, provider calls, and latency measurements were not run; static diff checking is not after-fix behavior proof for account collection. (ee0ae94e3120)
  • Review continuity: The prior completed ClawSweeper review raised the same selected-account /usage compatibility finding at this exact head SHA. No new commit resolves it. (Sources/CodexBarCLI/CLIUsageCommand.swift:288, ee0ae94e3120)

Likely related people:

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

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Resolve the /usage cardinality decision and preserve selected-account compatibility unless an API migration is approved.
  • Rebase onto current main and run the required macOS focused tests, make test, and make check.
  • Add redacted real /usage and dashboard snapshot output; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review.

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.

History

Review history (3 earlier review cycles)
  • reviewed 2026-08-10T01:11:37.792Z sha ee0ae94 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-11T20:17:53.244Z sha ee0ae94 :: needs real behavior proof before merge. :: [P1] Preserve the selected-account /usage default
  • reviewed 2026-08-12T15:59:07.228Z sha ee0ae94 :: needs real behavior proof before merge. :: [P1] Preserve the selected-account /usage default

@steipete

Copy link
Copy Markdown
Owner

Thanks @VACInc. Dashboard account grouping, opaque IDs, and row-local errors are useful, but separate from cross-device cost aggregation. This patch also changes ordinary generic /usage from the selected account to every configured account through the default includeAllAccounts path. Keeping this open pending the dashboard/API decision and consolidation with #3791. The recommended contract preserves generic /usage cardinality and enables all-account collection specifically for dashboard snapshots. #3791 overlaps this work but is not a landed replacement on the reviewed baseline.

@VACInc VACInc closed this Sep 23, 2026
@steipete

Copy link
Copy Markdown
Owner

Thanks @VACInc. At ee0ae94e3120, all-account collection defaults on, changing generic /usage from selected-account output to multiple rows. That conflicts with the required compatibility boundary. Public IDs also hash internal cache keys rather than durable account-source metadata. #3890 addresses those design points, pending verification. The recommendation is to close this implementation without adopting it.

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: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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