Share spend sources between Overview and dashboard - #3067
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ee893768c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de95d85544
ℹ️ 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".
| guard let separator = identity.firstIndex(of: "|") else { return nil } | ||
| return "codex:\(identity[..<separator])" |
There was a problem hiding this comment.
For a profile-home Codex account whose path contains | (a valid macOS filename character), the visible account ID is profile:<path> and the configuration stores it as <id>|<cache-hash>. Splitting at the first delimiter truncates that ID, so the roster contains a phantom truncated source alongside the actual loaded codex:<id> input, inflating subscription counts and reporting incorrect availability. Parse from the final delimiter, as codexOwnershipByID already does, or represent the identity structurally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 546bd90. All three identity-parsing sites (orderedSourceIDs, codexDisplayNamesByID, and codexOwnershipByID) now consistently use lastIndex(of: "|"). Added a regression test (profile-home path containing pipe preserves full account identity) that configures a pipe-containing profile-home path and asserts exactly one source with the full identity — no phantom truncated source.
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs real behavior proof before merge. Reviewed August 19, 2026, 5:30 AM ET / 09:30 UTC. ClawSweeper reviewWhat this changesThis PR publishes one app-scoped spend-source catalog so the Overview menu and Usage & Spend dashboard derive their accounting from the same cached provider and account data. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 6 items remain Keep open: the exact head still has a P2 account-identity parsing defect, and this broad shared-controller refactor needs maintainer sign-off plus real behavior proof. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherProvider token snapshots, Codex account caches, and spend settings feed a shared spend controller. It publishes immutable source states that the synchronous Overview menu and detailed dashboard both transform into their displays. flowchart LR
A[Provider and account snapshots] --> B[Shared spend controller]
C[Spend settings and ownership] --> B
B --> D[Immutable spend publication]
D --> E[Overview menu]
D --> F[Usage and Spend dashboard]
B --> G[Background refresh and reconciliation]
Decision needed
Why: VISION.md requires sign-off for broad refactors and meaningful maintenance complexity; this architecture choice cannot be resolved from correctness evidence alone. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Use a structured account identity, or consistently split its serialized form at the final delimiter, add the profile-path regression, then obtain a redacted fresh-app proof before maintainers decide whether to adopt the app-scoped controller. Do we have a high-confidence way to reproduce the issue? Yes, from source: configure a Codex profile-home path containing Is this the best way to solve the issue? No. A shared publication can reduce drift, but this implementation must preserve full account identities and receive the VISION-required architecture sign-off. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 453174fe13ee. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
codexDisplayNamesByID and orderedSourceIDs split serialized account identities at the first pipe, but profile-home account IDs can themselves contain pipe characters. This truncated the ID and created phantom unavailable sources, inflating the coverage denominator. Use lastIndex(of:) consistently with codexOwnershipByID and add a regression for a pipe-containing profile-home path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
de95d85 to
546bd90
Compare
|
@clawsweeper re-review Rebased onto current main (includes merged #3063 and #3064). Fixed the P2 account-identity delimiter bug in 546bd90 — all three identity-parsing sites now use |
|
🦞👀 Re-review progress:
|
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: Share spend sources between Overview and dashboard This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
The regression test added in 546bd90 shifted four SpendDashboardController.swift gatekeeper anchors by one line. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
Coverage and math
Stack
maincomparison temporarily includes its two commits and will narrow when Align Overview spend calendar with dashboard #3064 landsVerification
swift build --target CodexBarpassed after a temporary local expansion of the unavailable SwiftUI@Entrymacro; tracked macro source was restored with no diff0/13 files require formattingswiftc -parsegit diff --checkpassesTestingmodule / SourceKit framework unavailable); hosted Xcode CI is the authorityResearch
Architecture and primary Apple/Swift sources are recorded in
docs/research/shared-spend-source-publications.md.