feat(core): expand pi session attribution and reported cost handling - #2575
feat(core): expand pi session attribution and reported cost handling#2575Yuxin-Qiao wants to merge 2 commits into
Conversation
![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: e8b6c1a419
ℹ️ 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".
| if includePiSessions, PiSessionCostScanner.mappedTargetProviders.contains(provider), shouldMergePiUsage { | ||
| let piReport = try PiSessionCostScanner.loadDailyReportCancellable( |
There was a problem hiding this comment.
For every Bedrock request, loadRemoteTokenSnapshot constructs a non-nil snapshot and the caller returns it at lines 238-245, so execution never reaches this new merge branch. Consequently, includePiSessions: true still omits all amazon-bedrock pi/OMP sessions from Bedrock cost output; the remote Bedrock report needs to be combined with the pi report rather than returned early.
Useful? React with 👍 / 👎.
| let total = self.readNonNegativeDouble(cost["total"]) | ||
| if let total { | ||
| return Int64((total * Self.costScale).rounded()) |
There was a problem hiding this comment.
When a malformed pi/OMP row supplies usage.cost.total above roughly $9.22 billion, readNonNegativeDouble accepts it because its upper bound is approximately Int.max, but multiplying by one billion and converting to Int64 traps at runtime. Since this fatal conversion is not catchable by the surrounding JSONL scan, one corrupt cost value crashes the entire cost scan/app instead of being ignored; validate against Double(Int64.max) / costScale before converting.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed August 2, 2026, 11:13 AM ET / 15:13 UTC. ClawSweeper reviewWhat this changesThis PR expands Pi/OMP local-session cost attribution to more existing providers, prefers Pi-reported message costs, adds tool-summary and response-model handling, and updates the cache, tests, CLI help, and documentation. Merge readiness⛔ Blocked until real behavior proof is added - 13 items remain Keep this PR open, but it is not ready to merge: the expanded mapping is blocked by the existing snapshot gate, Bedrock still returns before merging local Pi usage, malformed reported costs can trap the scanner, and OpenAI fallback pricing is missing. It also needs redacted after-fix runtime proof in addition to its unit-test claims. Priority: P2 Review scores
Verification
How this fits togetherCodexBar’s local cost pipeline accepts a selected provider, optionally obtains remote usage, scans native and Pi/OMP session logs, and returns a merged token-cost snapshot to the CLI and app. Pi session records are mapped to CodexBar providers and priced from either Pi’s recorded cost or the models.dev pricing catalog. flowchart TD
A[Selected provider] --> B[Token snapshot entry]
B --> C[Remote usage lookup]
B --> D[Local native log scan]
B --> E[Pi and OMP session scan]
E --> F[Provider mapping and cost pricing]
C --> G[Merge daily cost reports]
D --> G
F --> G
G --> H[CLI and app cost history]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Route all mapped Pi providers through a deliberately supported local-cost snapshot path, merge Pi usage with remote Bedrock output before snapshot creation, bound malformed costs safely, and add focused regressions plus redacted live output proving the repaired provider and cost paths. Do we have a high-confidence way to reproduce the issue? Yes, for the four review defects by source inspection: the token-snapshot gate and Bedrock early return deterministically bypass the new merge block, while the scanner’s conversion and provider-ID switch show the overflow and OpenAI fallback gaps. No live account probe was run because this review is read-only and repository policy prefers non-Keychain parser validation. Is this the best way to solve the issue? No. The proposed mapping table and scanner updates are directionally consistent with the existing Pi pipeline, but the implementation must repair the entry-point, remote-merge, pricing-fallback, and numeric-boundary paths before it can provide the advertised behavior safely. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2bd6a6c37eff. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
- Prefer pi's exact per-message usage.cost over models.dev estimates, falling back only when absent or zero. - Attribute pi providers to their CodexBar counterparts (DeepSeek, Gemini, xAI, OpenRouter, Kimi, MiniMax, Qwen, Zai, OpenCode, Copilot, Mistral, Groq, Bedrock, Azure OpenAI, MiMo, ...). - Use message.responseModel for OpenRouter auto-style routing and price the actual model. - Bucket tool-result, compaction, and branch-summary usage under Tools/summaries. - Merge pi reports into every mapped provider on the local token snapshot pipeline, not just Claude/Codex. - Bump pi session cache schema to v9 and cost formula version to 2. - Add docs/pi.md and Linux scanner coverage.
7bb7910 to
59b8e75
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59b8e75b65
ℹ️ 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".
| // swiftlint:disable:next cyclomatic_complexity | ||
| private static func modelsDevProviderID(for provider: UsageProvider) -> String? { | ||
| switch provider { | ||
| case .codex: "openai" |
There was a problem hiding this comment.
When a pi row uses provider openai and omits usage.cost.total or reports zero, it is attributed to .openai, but this switch only maps .codex to the openai models.dev catalog and lets .openai fall through to nil. Consequently, otherwise priceable OpenAI rows remain unpriced instead of using the documented models.dev fallback; add the .openai case to this mapping.
Useful? React with 👍 / 👎.
clawsweeper
Bot
removed
the
merge-risk: 🚨 security-boundary
|
Maintainer decision on the Usage & Spend program (applies to #2322, #2527, #2548, #2569, #2575, #2577 — closing together): We want the ideas — the token-activity view and the cross-provider cost overview in particular are genuinely good, and we're adopting them. What we can't absorb is the delivery shape: 8k–20k-line PRs bundling scanners, pricing, identity, and UI can't be reviewed honestly, and live testing of the current heads surfaced the kind of defects that big surfaces hide (duplicate token-activity cards in the Models view; the unified Overview rendering no chip for an enabled provider; the account-attribution issue flagged earlier). So we're taking this work as a base in-house and rebuilding it in small, individually landable slices — first the token-activity view rebuilt on the shared scan cache (no separate annual scan), then the cost overview aligned with the #2578 design discussion. Your commits will be cherry-picked where they fit, and every slice that draws on this work will credit you in the changelog, as today's merges already do. @Yuxin-Qiao — you're this repo's most prolific contributor and much of today's release is your work. Focused PRs the size of #2574 land same-day; that's the shape that serves both of us. |
2 participants
Summary
Extends the existing Pi session scanner from a partial two-provider integration into full local usage attribution:
usage.cost.totalover models.dev estimates, falling back only when the session omits cost or reports zero.message.responseModelwhen present (e.g. OpenRouterautorouting) so the actual model is priced and shown in breakdowns.Tools/summaries, matching pi's own session totals.pi-sessions-v9.json;costFormulaVersionbumps to 2 so existing caches rescan.docs/pi.mdand updatesdocs/codex.md,docs/claude.md, and thecostCLI help.Test
PiSessionCostScannerTests: official cost priority + zero-cost fallback,responseModelrouting,Tools/summariesbucketing, DeepSeek/xAI/Gemini attribution, third-party pricing-key invalidation, v8→v9 cache migration.CostUsageFetcherTests+CostUsageFetcherCacheSnapshotTests: merge behavior unchanged and green.TestsLinux/PiSessionCostScannerLinuxTests.swiftcovers provider mapping without network.make check(swiftformat + swiftlint strict + parser hash + package checks) passes.Verification ran on a clean worktree off
main; the working checkout contains unrelated uncommitted WIP, so it was excluded from the PR branch.