Skip to content

fix: restore shared Codex spend in account menus - #3540

Merged
steipete merged 1 commit into
steipete:mainfrom
kays0x:fix/codex-multi-account-cost
Sep 10, 2026
Merged

fix: restore shared Codex spend in account menus#3540
steipete merged 1 commit into
steipete:mainfrom
kays0x:fix/codex-multi-account-cost

Conversation

@kays0x

@kays0x kays0x commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Restores the shared ambient Codex ledger beneath multi-account cards. Both stacked and compact account layouts now reach one Codex-owned trailing section, labeled This Mac, without assigning the same spend to individual accounts. Only the existing codex:ambient scope qualifies; managed-account and profile-home history remains isolated.

The maintainer pass also fixes the proposal’s display-mode mismatch: inline-only uses the existing inline dashboard, submenu-only uses the existing compact Cost row, and Both shows each once. The generic compact renderer no longer needs a Codex-specific trailing callback. Account override-card isolation is unchanged.

Validation:

  • The new layout/preference regressions produced 12 failing assertions against the proposal. 53 focused tests then passed, including override isolation and architecture checks.
  • make check: zero violations across 2,173 Swift files. Independent P0–P2 review is clean.
  • Full make test: 1,061 selections across 89 groups passed on the first attempt, with no retries or timeouts (787.6 seconds).
  • Developer-ID-signed native proof exercised two-account stacked and five-account compact menus, all three display modes, and opening the Cost history submenu. The native test passed with no failures. Only synthetic accounts and spend data were used.
  • Native proof source matches commit 9b44b2fe9a6ae85712fbbc7242b700997c11cbdc. Exact-head CI passed all Linux, musl and macOS checks. Merged as cebf9995f672d2c0c9ff3501b24721855b86b16c.

Documentation and the Unreleased changelog explain the ambient scope and credit @kays0x.

Inspected synthetic native before/after proof

Legacy stacked rendering omits spend:

Before: stacked accounts without shared spend

Stacked accounts with shared inline spend and Cost history:

After: shared spend below stacked accounts

Compact accounts with the Cost history submenu open:

After: compact accounts and history

Submenu-only mode keeps the compact Cost row:

After: submenu-only preference

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-10T17:23:40.642391Z 856af00 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@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: 856af00263

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

Comment thread Sources/CodexBar/StatusItemController+CodexStackedMenu.swift Outdated
Comment thread Tests/CodexBarTests/MenuCardCodexAmbientCostTests.swift Outdated
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 10, 2026
@clawsweeper

clawsweeper Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 10, 2026, 4:13 PM ET / 20:13 UTC (Revision 4).

ClawSweeper review

What this changes

Restores one shared local Codex cost section below stacked or compact account menus, preserving display preferences and adding regression coverage and a native proof harness.

Merge readiness

Blocked before merge - 2 items remain

The fix remains useful, and the earlier code findings are addressed. No blocking correctness defect remains, but observable runtime proof is still incomplete.

Priority: P2
Reviewed head: 9b44b2fe9a6ae85712fbbc7242b700997c11cbdc

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The focused implementation resolves the prior findings, while runtime evidence remains the limiting factor.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: Needs real behavior proof before merge: the reported packaged two-account run supports the original symptom, but the current shared renderer has no inspectable after-fix result for compact layout, display modes, or opened history. The native harness source alone does not establish execution. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: Needs real behavior proof before merge: the reported packaged two-account run supports the original symptom, but the current shared renderer has no inspectable after-fix result for compact layout, display modes, or opened history. The native harness source alone does not establish execution. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 8 items Complete introduced patch: Read the complete pinned base-to-head diff across all eight files; the local checkout is the original PR head and the working tree is clean.
Current main still needs the repair: The multi-account branch selects compact or stacked rendering and returns without adding the shared cost section; override-card cost isolation remains intact.
Latest release check: The supplied latest release, v0.58.0, also has no shared cost section after its stacked Codex cards.
Findings None None.
Security None None.

How this fits together

CodexBar combines account usage with cost estimates from local Codex session logs. The menu now presents the ambient ledger outside individual account cards while retaining the existing cost-history submenu.

flowchart TD
 A[Local session logs] --> B[Scoped cost snapshot]
 B --> C{Ambient Codex scope}
 C --> D[Shared cost section]
 E[Stacked or compact accounts] --> D
 F[Cost display preference] --> D
 D --> G[Inline summary and cost history]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: Needs real behavior proof before merge: the reported packaged two-account run supports the original symptom, but the current shared renderer has no inspectable after-fix result for compact layout, display modes, or opened history. The native harness source alone does not establish execution. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Complete next step (P2) - Provide observable after-fix native-menu evidence for the current patch. Screenshots or video are preferred when useful; runtime logs or a diagnostic transcript also count. Redact account details, API keys, IP addresses, and other private information. Updating the PR body should trigger a fresh review; otherwise ask a maintainer to comment @clawsweeper re-review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +55/-8; tests +368/-3 Production growth adds a shared rendering boundary; test growth includes an opt-in native presentation harness.
Display coverage 2 account counts × 3 display styles The parameterized regression checks cover both layout thresholds and every existing cost-display preference.

Technical review

Best possible solution:

Keep one explicitly labeled ambient cost section using existing presentation helpers, with account-scoped histories excluded and stored display preferences preserved.

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

Yes, from source: with multiple visible Codex accounts and ambient cost data, main takes account-only renderers that omit the shared cost section. No runtime reproduction was executed during this review.

Is this the best way to solve the issue?

Yes. The common caller and existing cost views provide a narrow repair while preserving account isolation and all three display modes.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: This repairs missing cost information in a bounded multi-account menu workflow.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Needs real behavior proof before merge: the reported packaged two-account run supports the original symptom, but the current shared renderer has no inspectable after-fix result for compact layout, display modes, or opened history. The native harness source alone does not establish execution. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Complete introduced patch: Read the complete pinned base-to-head diff across all eight files; the local checkout is the original PR head and the working tree is clean. (9b44b2fe9a6a)
  • Current main still needs the repair: The multi-account branch selects compact or stacked rendering and returns without adding the shared cost section; override-card cost isolation remains intact. (Sources/CodexBar/StatusItemController+Menu.swift:693, e9da07961168)
  • Latest release check: The supplied latest release, v0.58.0, also has no shared cost section after its stacked Codex cards. (Sources/CodexBar/StatusItemController+CodexStackedMenu.swift:62, 88fa2f45fa1e)
  • Prior display and layout findings resolved: The common caller appends the section after either layout. Its separate inline-dashboard and token-usage branches reuse the existing model's preference filtering and the established Cost-row renderer. (Sources/CodexBar/StatusItemController+CodexStackedMenu.swift:68, 9b44b2fe9a6a)
  • Scope and test isolation: The new predicate accepts only the existing codex:ambient scope. Managed and profile homes resolve to distinct signatures; account override models remain unchanged. The fixture uses testSettingsStore, which injects in-memory or no-op secret stores, and ordinary settings tests skip app-group migration. (Sources/CodexBar/UsageStore+TokenCost.swift:595, 9b44b2fe9a6a)
  • Runtime evidence remains incomplete: The captured body and contributor discussion report successful packaged two-account rendering. The current branch adds an opt-in native UI harness, but no execution receipt, screenshot, recording, or runtime transcript accompanies it. The proof scratch directory is empty, and the latest discussion contains only the review-start placeholder. This preserves the distinction between reported live success and unobserved current-head behavior. (Tests/CodexBarTests/CodexSharedCostNativeProofTests.swift:8, 9b44b2fe9a6a)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • sf-jin-ku: 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.

  • Attach redacted screenshots, a recording, or a runtime transcript showing the current build's stacked and compact cost presentation, display modes, and opened history submenu.

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-09-10T17:28:52.968Z sha 856af00 :: needs real behavior proof before merge. :: [P1] Inject an in-memory token-account store into the fixture
  • reviewed 2026-09-10T18:15:51.301Z sha 9bceb85 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-10T18:41:45.787Z sha 8d7af20 :: needs real behavior proof before merge. :: [P2] Preserve the selected cost-display mode in the shared section

@kays0x
kays0x force-pushed the fix/codex-multi-account-cost branch from 856af00 to 9bceb85 Compare September 10, 2026 18:10
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. label Sep 10, 2026
@kays0x
kays0x force-pushed the fix/codex-multi-account-cost branch from 9bceb85 to 8d7af20 Compare September 10, 2026 18:37
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 10, 2026
@kays0x

kays0x commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

The implementation changed substantially since the last review, so the previous findings no longer describe the code.

  • The override-card fallback is gone. StatusItemController+MenuCardModel.swift is now unchanged from main, so the isolation rule is untouched. Testing against two live accounts showed that giving each override card the ambient snapshot rendered the same total four times over in a two-account menu, because tokenUsage drives both the inline usage dashboard and the cost block.
  • The machine-wide ledger is now rendered once below the account cards, gated on UsageStore.tokenCostIsAccountAgnostic(for:).
  • Both earlier findings are addressed: the tests build their stores via testSettingsStore/withStatusItemControllerForTesting so no Keychain access is reachable, and the compact layout shares the section through a trailingSection hook on addCompactAccountMenuRows.

Worth scrutinising: whether the shared section belongs in addCompactAccountMenuRows versus the common caller, and whether gating on tokenCostScope returning codex:ambient is the right signal for "this spend belongs to no single account".

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Render ambient spend once after either account layout, preserving inline/submenu preferences and account-scoped isolation. Reuse existing cost views and keep Codex-specific trailing content out of the generic compact renderer.

Co-authored-by: kays0x <48529172+kays0x@users.noreply.github.com>
@steipete
steipete force-pushed the fix/codex-multi-account-cost branch from 8d7af20 to 9b44b2f Compare September 10, 2026 20:09
@steipete steipete changed the title fix: show Codex cost in the multi-account menu fix: restore shared Codex spend in account menus Sep 10, 2026
@steipete
steipete merged commit cebf999 into steipete:main Sep 10, 2026
9 checks passed
@steipete

Copy link
Copy Markdown
Owner

Merged as cebf999. The shared ambient ledger now appears once beneath stacked or compact Codex accounts, with inline-only, submenu-only and Both preferences respected. Account override cards and managed-profile history remain isolated.

The proposal failed twelve layout/preference assertions before the maintainer fix. All 53 focused tests, the full 1,061-selection local suite, lint, independent P0–P2 review and all Linux/musl/macOS CI checks passed. Signed synthetic native proof covers both account layouts, all display modes and opening Cost history; inspected images are in the PR body.

Provider docs and the Unreleased changelog are updated. Thanks @kays0x!

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: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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