Add Pi token usage provider - #3246
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
![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: c3732c325e
ℹ️ 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".
| let piDaily: CostUsageDailyReport = try await CostUsageScanExecutor.run { checkCancellation in | ||
| try PiSessionCostScanner.loadDailyReportCancellable( | ||
| // Provider-specific by design: this call reads Pi's local aggregate session ledger. | ||
| provider: .pi, |
There was a problem hiding this comment.
When Pi and Claude are both enabled with cost usage, this branch publishes Claude-priced Pi rows under Pi, while the existing loadLocalTokenScanResult path still merges those same rows into Claude at CostUsageFetcher.swift:726-740; SpendDashboardModel then sums provider totals at line 522, so Claude-backed Pi tokens and spend are counted twice. Assign these rows to only one dashboard source or explicitly deduplicate the overlap.
Useful? React with 👍 / 👎.
|
|
||
| // Provider-specific by design: Pi has an independent aggregate token-cost history over its local JSONL logs. | ||
| if provider == .pi { | ||
| var piOptionsOnly = overridePiScannerOptions ?? PiSessionCostScanner.Options() |
There was a problem hiding this comment.
For users who relocate sessions through PI_CODING_AGENT_SESSION_DIR, PI_CODING_AGENT_DIR, Pi's sessionDir setting, or OMP profiles, constructing default options here ignores the supplied environment and scans only ~/.pi/agent/sessions and ~/.omp/agent/sessions. The existing root logic in PiFamilySessionScanner.swift:631-698 already recognizes these supported layouts, but the new token provider will report no sessions for them unless equivalent roots are passed into PiSessionCostScanner.Options.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Maintainer update
I have repaired items 1–3 directly on this PR, preserving @Yuxin-Qiao's contributor credit. The integration is in fa1ab1d9fe9427df40977ab1e6c872773468bd92, with Linux portability and cache-persistence follow-ups through 6f5e173d9020e6f49f73e34de615d5f4468a386a. Regression coverage now exercises disjoint combined totals, native account isolation, selected filesystem scope, cache identity, and incomplete history. Independent review is clean, and both native Linux build/test/smoke jobs pass.
Final verification and the remaining installed-app/widget proof are maintainer-owned. No further contributor changes are requested. The current commands, results, CI links, and proof limitations are recorded in the PR description; this review remains blocking until the remaining verification is complete.
Original source review — historical findings, now repaired
Thanks @Yuxin-Qiao. Reusing the existing scanner and provider descriptors is a sensible starting point, but this needs changes before merge. I checked c3732c3 against main b4694bd and followed the actual shared-spend callers.
-
Prevent overlapping Claude/Pi totals. The new Pi report merges the Anthropic and OpenAI Codex Pi ledgers, while
CostUsageFetcherstill merges Anthropic Pi rows into Claude by default.UsageStore.loadTokenUsageSnapshotleaves that default enabled, and the shared spend model sums the distinct Claude and Pi source inputs. With both enabled and visible, the same Anthropic Pi event contributes twice. Native account-scoped Codex dashboard scans explicitly exclude Pi, so this is not a claim that every Codex view doubles usage. Define whether Pi is a non-additive view of the existing ledger or a disjoint additive source, and prove exactly-once combined totals with Pi plus Claude, including non-Pi Claude usage and source visibility changes. Hiding all Claude usage or subtracting aggregate totals would lose the required attribution. -
Honor the selected filesystem scope. The new fetcher branch does not pass its supplied environment into root resolution; default scanner options read the ambient user's
.pi/.ompdirectories. Agent Sessions already recognizes configured Pi roots and supplied HOME, so relocated history can appear there but be absent from the new provider—or the provider can inspect the wrong default root. Resolve an explicit supported root context and tie cache identity to it. Cover configured versus ambient roots, supplied HOME and OMP profile boundaries using isolated fixtures; do not broaden scanning to every profile as a fallback. -
Do not claim established history without coverage evidence. The branch unconditionally sets
historyCoverageIsEstablished: true, but the reused scanner returns empty discovery for some missing/unreadable roots and skips malformed/truncated records/read failures. Its report currently covers only the two recognized backends. Carry scan-completeness evidence into the snapshot and distinguish confirmed empty from unavailable/partial history. Add negative fixtures, not just an assertion that the flag is true. Keep this at the scanner/report boundary; it should not become another provider-specific publication workaround.
The submitted tests exercise the isolated aggregate and registration, not these combined-source and failure cases. This review is source-based; I have not run this PR's tests or independently verified its reported CI/runtime results. Once the accounting contract is settled and those regressions are covered, the cost-only menu/widget behavior still needs packaged runtime proof. Keeping the provider opt-in does not resolve the overlap or scope issues.
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs real behavior proof before merge. Reviewed September 11, 2026, 6:57 AM ET / 10:57 UTC (Revision 60). ClawSweeper reviewWhat this changesAdds an opt-in Pi local-history provider across the app, CLI, spend dashboard, and widget, with shared session discovery, estimated costs, and cache provenance. Merge readiness⛔ Blocked before merge - 6 items remain This remains a useful, distinct provider addition. Two prior provenance defects persist, and the process-cap repair only covers identical commands. The recorded maintainer-owned runtime verification hold also remains unresolved. Priority: P2 Review scores
Verification
How this fits togetherCodexBar reads local Pi and OMP session logs to estimate token usage and spending. Root discovery and cached accounting feed provider menus, combined spending views, CLI output, and widget snapshots. flowchart TD
A[Live processes and settings] --> B[Resolve session stores]
B --> C[Read Pi and OMP logs]
D[Retained cache provenance] --> B
C --> E[Estimate usage and costs]
E --> D
E --> F[Separate Pi and native totals]
F --> G[Menus dashboard CLI and widget]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve all root selectors and availability metadata through discovery and cache reload, report bounded discovery honestly, and verify fresh and upgraded installations before lifting the recorded runtime hold. Do we have a high-confidence way to reproduce the issue? Yes, the remaining patch defects have concrete source-level triggers involving shared selectors, retained default roots, and capped process contexts; no runtime reproduction was executed. Is this the best way to solve the issue? The existing scanner and provider framework are appropriate, but the current implementation needs lossless provenance merging and explicit handling of incomplete discovery. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 4cfb60692bdd. LabelsLabel justifications:
EvidenceWhat 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
HistoryReview history (59 earlier review cycles; latest 8 shown)
|
Integrate the Pi provider and preserve standalone Claude/Codex reports while counting selected Pi history exactly once in combined spend surfaces. Bind scans, caches and publications to validated roots and coverage, share Pi settings resolution, and retain native Codex account isolation. Add isolated regression and production-view proof, repair template icon and local-source labels, and document the estimator's accounting limits. Refs steipete#3246 Co-authored-by: Yuxin Qiao <104957188+Yuxin-Qiao@users.noreply.github.com>
clawsweeper
Bot
added
P2
6f5e173 to
9723a64
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9723a6497c
ℹ️ 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".
| @@ -0,0 +1 @@ | |||
| <svg width="100" height="100" viewBox="0 0 100 100" fill="none" xmlns="http://www.w3.org/2000/svg"><circle cx="50" cy="50" r="48" fill="#7C3AED"/><text x="50" y="62" text-anchor="middle" font-family="Helvetica, Arial, sans-serif" font-size="42" font-weight="700" fill="white">π</text></svg> No newline at end of file | |||
There was a problem hiding this comment.
ProviderBrandIcon.image(for:) marks every provider SVG as a template image, but this asset composites opaque white text over an opaque purple circle. Template rendering uses the resulting alpha mask, which is only a solid circle, so the π disappears anywhere the provider brand icon is displayed. Encode the glyph and surrounding shape using transparency, preferably with paths rather than font-dependent SVG text.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Use a final-path atomic write and cover repeated cache saves with a portable roundtrip regression. Co-authored-by: Yuxin Qiao <104957188+Yuxin-Qiao@users.noreply.github.com>
365a2a7 to
2df0447
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2df04477d3
ℹ️ 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: 3d04206180
ℹ️ 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 workingDirectory != nil || | ||
| PiFamilySessionScanner.hasAbsoluteSessionDirectorySelector( | ||
| in: process, | ||
| environment: environment) |
There was a problem hiding this comment.
When an OMP process uses --profile work and its CWD cannot be read, this guard drops the process context because only an absolute --session-dir is accepted. OMP profile resolution is CWD-independent when HOME is available, so the later cost scan instead auto-discovers every profile and can include usage from unrelated profiles. Retain --profile contexts without a CWD and resolve them using the existing OMP profile resolver.
Useful? React with 👍 / 👎.
| tokenCost: ProviderTokenCostConfig( | ||
| supportsTokenCost: true, |
There was a problem hiding this comment.
When Pi is enabled in Overview, this token-cost source enters the spend dashboard as a normal native input, and SpendDashboardController.publishCurrentState assigns every such input the .subscription role; SpendDashboardPublication.subscriptionCount therefore includes Pi in the text “%d of %d subscriptions have spend.” A Claude-plus-Pi setup is consequently reported as two subscriptions even though Pi is only a local-history estimator. Add a local-source classification or exclude Pi from subscription coverage counts.
Useful? React with 👍 / 👎.
3d04206 to
c8fb54e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a420f904d5
ℹ️ 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".
| let retentionKey = fields.count >= 5 && !fields[4].isEmpty ? String(fields[4]) : nil | ||
| // A settings file is a replacement point: if it now resolves to another root, the | ||
| // prior root belonged to the superseded selector and must not be carried forward. | ||
| if let retentionKey, currentRetentionKeys.contains(retentionKey) { continue } |
There was a problem hiding this comment.
When the global Pi settings file uses a relative sessionDir, each live process resolves that same file against its own CWD, producing different roots with the identical settings:<file> retention key. After those processes exit, ambient resolution contributes one current key and this check drops every cached live root sharing it without comparing paths, so a complete scan removes valid historical usage from the other projects. Fresh evidence beyond the settings-replacement fix is this many-roots-to-one-key collision; include the resolving CWD in the key for relative settings or otherwise distinguish retained roots from a genuinely superseded value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88fab5e422
ℹ️ 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 safeEnvironment["OMP_PROFILE"] == nil, | ||
| safeEnvironment["PI_PROFILE"] == nil |
There was a problem hiding this comment.
When PI_PROFILE selects a profile stored in the supported ~/.omp/profiles/<profile>/sessions layout, this condition disables profile discovery, while OMPSessionRootResolver.namedProfileRoots checks only <profile>/agent/sessions for non-XDG profiles. The valid store is therefore omitted and the nonexistent legacy root makes the historical scan incomplete; selected profiles should resolve both layouts just as automatic profile discovery does.
Useful? React with 👍 / 👎.
| for parent in profileParents { | ||
| urls.append(contentsOf: Self.profileSessionRoots(in: parent)) | ||
| let resolution = Self.profileSessionRoots(in: parent) | ||
| urls.append(contentsOf: resolution.roots) |
There was a problem hiding this comment.
When PI_CONFIG_DIR points to a custom OMP configuration directory and no profile is explicitly selected, this discovery loop still receives only the hard-coded ~/.omp/profiles and XDG parents. Profiles beneath <HOME>/<PI_CONFIG_DIR>/profiles are never scanned, while unrelated profiles under ~/.omp can be included, so Pi cost history uses the wrong root set; derive this parent from the same configured root used by OMPSessionRootResolver.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26c6696325
ℹ️ 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".
| self.tokenFailureGates[provider.instanceID]?.recordSuccess() | ||
| self.persistWidgetSnapshot(reason: "token-usage") | ||
| cursorCookieHeaderOverride: cursorCookieHeaderOverride, | ||
| includePiSessions: true) |
There was a problem hiding this comment.
When Pi and Claude or ambient Codex are both enabled, this hard-coded true ignores shouldIncludePiSessionsInTokenSnapshot and publishes Pi/OMP rows in the regular Claude/Codex snapshot; provider cards consume that snapshot directly while the Pi card separately publishes the same logs, duplicating and misattributing usage. Fresh evidence after the earlier ownership fix is that the final refresh path explicitly restored unconditional Pi inclusion; pass the ownership policy here or publish the stored native projection to provider-specific surfaces.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
| let role: SpendSourcePublication.Role = switch input?.sourceKind { | ||
| case .openCodex: .enrichment | ||
| case .localHistory: .localHistory | ||
| case .native, nil: .subscription |
There was a problem hiding this comment.
When Pi is configured but its scan is confirmed empty, loading, or unavailable, there is no ProviderInput, so input?.sourceKind is nil and this branch classifies the Pi source as a subscription. Fresh evidence after the local-history classification change is that only input-backed Pi rows receive .localHistory; the empty/unavailable Pi row still inflates subscriptionCount (for example, Claude plus empty Pi is reported as two subscriptions). Derive Pi's role from provider when the input is absent.
Useful? React with 👍 / 👎.
| case .readableDirectory: | ||
| roots.append(xdgLayout) | ||
| continue |
There was a problem hiding this comment.
When an automatically discovered OMP profile contains both profiles/<name>/sessions and the legacy profiles/<name>/agent/sessions—for example during or after a layout migration—the continue after adding the direct layout prevents the legacy layout from being returned. Historical usage remaining there is omitted even though explicitly selected profiles preserve both layouts; inspect and append both existing directories during automatic discovery as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43a1071537
ℹ️ 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".
| output.append(SessionRoot( | ||
| url: url, | ||
| missingIsKnownEmpty: fields[1] == "known-empty", | ||
| resolutionIsComplete: false, |
There was a problem hiding this comment.
When a retained settings file becomes malformed or temporarily unreadable, this changes scopeFingerprint(options:) to an unresolved fingerprint, which refreshPiHistoryScope records as current. The subsequent scan sees the changed, incomplete scope, restores cacheBeforeScan, and returns the old resolved fingerprint; tokenAccountingScopeIsCurrent therefore rejects every retained Pi/Claude/Codex result, so the history this branch intends to preserve disappears until the settings file is repaired. Advertise the cached scope while serving the retained report, or otherwise keep these fingerprints equivalent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15ef441e50
ℹ️ 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".
| let xdgDataHome = Self.environmentURL( | ||
| environment["XDG_DATA_HOME"], | ||
| baseDirectory: baseDirectory, | ||
| fileManager: fileManager) |
There was a problem hiding this comment.
On macOS and Linux, when XDG_DATA_HOME is unset, this new resolver omits the standard ~/.local/share/omp/profiles directory because environmentURL(nil, ...) returns nil. The previous discovery path used the still-present xdgDataHome helper, which falls back to ~/.local/share; consequently, automatic Pi cost scans and source detection now miss OMP sessions stored in default-XDG profiles and can publish an established but undercounted snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13b7c5322e
ℹ️ 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".
| for root in roots { | ||
| let canonical = Self.canonicalURL(root.url) | ||
| guard seenDialectRoots.insert(canonical.path).inserted else { continue } | ||
| guard seen.insert(canonical.path).inserted else { continue } |
There was a problem hiding this comment.
When an OMP process explicitly selects the Pi default path (for example, omp --session-dir /Users/me/.pi/agent/sessions), the Pi dialect adds that path first as an optional known-empty default, and this global deduplication then discards OMP's required/live version of the same root. If the directory later disappears or is temporarily unmounted, the scan is considered complete and cached usage is deleted instead of retained as unavailable. Deduplicate by merging provenance so any explicit occurrence makes the shared root required.
Useful? React with 👍 / 👎.
| let subscriptionProviders = includedProviders.filter { $0.sourceKind != .localHistory } | ||
| let providerCount = max(max(0, providerCount), subscriptionProviders.count) |
There was a problem hiding this comment.
When the spend publication is not initialized or its ownership fingerprint is stale, overviewSpendSubscriptionCount falls back to the raw enabled-provider count, so a Claude-plus-Pi setup supplies 2 here. Filtering the loaded rows cannot reduce that value because this max preserves it, and Overview temporarily reports “1 of 2 subscriptions” even though Pi is a local-history source. The fallback count also needs to exclude Pi rather than relying solely on publication roles.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eca903f281
ℹ️ 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".
| var seenDialectRoots = Set<String>() | ||
| for root in roots { | ||
| let canonical = Self.canonicalURL(root.url) | ||
| guard seenDialectRoots.insert(canonical.path).inserted else { continue } |
There was a problem hiding this comment.
When two same-dialect Pi processes resolve to the same path, this guard discards the later root before appendCostRoot can merge its provenance. Fresh evidence beyond the earlier shared Pi/OMP-root fix is that same-dialect duplicates still bypass that merger: for example, a plain pi process ordered before pi --session-dir ~/.pi/agent/sessions loses the required/live flags, while two project settings pointing to one shared root retain only the first settings key. A later missing root or removal of the first selector can therefore erase valid cached history despite the other explicit selector; pass duplicates through the provenance merge instead of dropping them here.
Useful? React with 👍 / 👎.
| let piRoots = PiFamilySessionRootResolver.costSessionRootURLs( | ||
| environment: piEnvironment, | ||
| baseDirectory: piBaseDirectory) |
There was a problem hiding this comment.
On a Pi-only installation whose sessions are selected solely by a live process's project .pi/settings.json or command-line --session-dir, this resolves roots using only CodexBar's own working directory and supplies no process contexts. Fresh evidence beyond the earlier auto-detection fix is that the actual cost path uses LocalAgentSessionScanner.piSessionProcessContexts, but this startup detector cannot reach those same roots; with empty default stores it returns false, so applyTokenCostDefaultIfNeeded leaves token-cost usage disabled and the new provider shows no history until the user enables it manually.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 911d5d218f
ℹ️ 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".
| let resolution = Self.ompSessionRootResolution( | ||
| process: defaultProcess, | ||
| cwd: cwdURL.path, | ||
| environment: environment) | ||
| roots.append(contentsOf: resolution.roots) |
There was a problem hiding this comment.
When a live OMP process uses --profile work but CodexBar's own environment has no profile selector, the process-context pass resolves work, then this unconditional default-process pass resolves each CWD without that flag; ompSessionRootResolution consequently auto-discovers every stored profile and includes unrelated profile usage in Pi totals. Fresh evidence beyond the prior missing-CWD finding is that this also happens when CWD lookup succeeds. Skip ambient OMP profile discovery when explicit process profiles are present, while still combining profiles selected by multiple live contexts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47a2bdd592
ℹ️ 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".
| let directRoot = canonicalProfileRoot.appendingPathComponent("sessions", isDirectory: true) | ||
| if Self.isDirectory(directRoot, fileManager: fileManager) { | ||
| roots.append(directRoot) |
There was a problem hiding this comment.
When a live OMP process selects a profile stored as profiles/<name>/sessions/*.jsonl, this adds the direct directory as an untyped URL; ompSessionRootResolution later wraps every returned URL with .projectDirectories, so records inspects only child directories instead of the JSONL files directly in this root. The cost scanner still finds these files recursively, but live-session discovery falls back to pid:<pid> and loses the transcript, session ID, title, and activity metadata. Fresh evidence after the earlier direct-profile resolution fix is that the root is now found while its layout is discarded; carry .direct through the resolver.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3239a16afb
ℹ️ 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 Self.isDirectory(xdgSessions, fileManager: fileManager), | ||
| let root = Self.sessionRoot( | ||
| agentRoot: xdgDataHome.appendingPathComponent("omp", isDirectory: true), | ||
| fileManager: fileManager) | ||
| { | ||
| return [root] |
There was a problem hiding this comment.
When ~/.local/share/omp/sessions exists—now detected even when XDG_DATA_HOME is unset—this early return discards the already resolved ~/.omp/agent/sessions root (or the root selected through PI_CONFIG_DIR). Users retaining sessions in both layouts during a migration consequently lose all legacy rows from Pi token and cost totals, and source detection can undercount them; return and deduplicate both roots as the named-profile resolver does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbdfd558ba
ℹ️ 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".
| let processes = Array(AgentSessionCorrelation.newestProcessesFirst( | ||
| // Provider-specific by design: only Pi processes provide project roots for Pi history resolution. | ||
| AgentPSOutputParser.agentProcesses(from: allProcesses) | ||
| .filter { AgentPSOutputParser.provider(for: $0) == .pi }) | ||
| .prefix(max(0, self.config.maxProcessCount))) |
There was a problem hiding this comment.
When more than maxProcessCount Pi/OMP processes are running, this truncates the process list before contexts are deduplicated or their distinct roots are resolved. For example, 64 newer processes using the default store can crowd out an older process with a unique --session-dir; because these contexts are the only way an app or CLI running outside that project discovers the custom store, the cost scan publishes a complete-looking snapshot that omits its history. Deduplicate root-relevant contexts before imposing a limit, or propagate that truncation as an incomplete scan.
Useful? React with 👍 / 👎.
2 participants
Summary
Add Pi as an opt-in local-history provider with its own menu, dashboard source, icon, widget choice, and documentation. Thanks @Yuxin-Qiao for the original implementation; the maintainer follow-up repairs the shared accounting and filesystem boundaries on this same PR.
Pi combines supported Anthropic and OpenAI Codex Pi/OMP session usage. Standalone Claude and unscoped Codex keep their existing inclusive reports. Combined Usage & Spend and Overview views instead select retained native-only values when a visible Pi source owns the same history, so every contribution is counted once. Native account-scoped Codex remains Pi-free. Pi is a local source, not an extra subscription.
The live-session and historical readers now share Pi root selection. Historical caches and publications carry root identity and completeness; explicit missing or invalid roots fail closed, absent default history is unavailable, and malformed/truncated/unsupported records cannot become falsely complete history. Project/global settings, custom agent directories, OMP profiles and eligible XDG roots have isolated regression coverage. A valid custom Pi settings directory does not require a companion OMP installation.
The icon uses a font-free transparent glyph so macOS template rendering retains the Pi mark. Documentation explains the assistant-message list-price estimator, unsupported billing components, standalone CLI behavior, and widget coverage limitations.
Maintainer verification
6f5e173d9020e6f49f73e34de615d5f4468a386a. The integration commitfa1ab1d9fe9427df40977ab1e6c872773468bd92preserves the original contributor commit as a merge parent. Two small follow-ups fix Linux hashing imports and repeated cache persistence.fa1ab1d9fe9with the repository's canonical--no-parallelsetting: 224 tests in 20 suites, plus four portable tests executed on macOS, passed. The production-view render test also passed. Coverage includes accounting/visibility, native accounts, root/cache contracts, backend/day-specific partial evidence, malformed records and boolean counters/timestamps, publication ownership, cached startup, and provider architecture.make check: passed, zero violations across 2,045 Swift files.make teston6f5e173d902: passed all 963 selections across 81 groups in 1,976.1 seconds. Eighty groups passed initially; one cache-test group reached the normal 180-second limit and recovered when all twelve selections passed individually through the existing runner. There were no full-group retries, source/assertion changes, or deadline increases; non-timeout retries stayed disabled. Earlier interrupted candidate runs are not full-pass claims.make check, and a clean independent review of the four-line diff.CryptoKitimport on Linux. The existing Swift Crypto dependency is now selected there, matching the repository's established pattern. Replacement CI run 33123394523 builds on Linux, including a successful musl job, but its x64/ARM test jobs exposed a saved-cache reload failure. The x64 run executed 455 tests in 63 suites with four failed cache-coverage assertions; the corresponding in-memory parser assertions passed. Remaining obsolete jobs were canceled.make check, explicit portable format/lint, and a clean independent review. The new regression reads the actual JSON after both first and replacement saves, checks changed values, and rejects stray temporary files. These macOS results are not a native Linux pass.6f5e173d9020e6f49f73e34de615d5f4468a386a, including native Linux x64/ARM64 builds, tests and CLI smoke checks, musl build, both macOS test shards, lint, and the aggregate gate. The cache repair is now verified on native Linux.Tests use synthetic history, isolated stores, and Keychain/file-access safeguards. The rendered light/dark menus, dashboard, Overview and widget use production views and the same synthetic accounting fixture: standalone Claude $12/120K tokens, Pi $5/50K, combined $15/150K with native Claude $10/100K. Widget publication is also tested through the in-memory save boundary.
One earlier broad focused run used parallel execution and hit the dashboard test's 30-second wait while a main-actor architecture audit occupied roughly 32 seconds. With the repository's existing serial test setting, that same publication test passed in 1.8 seconds and the entire focused set passed. No deadline, assertion, or source change was made to hide the timeout.
These are offscreen render and publication proofs, not installed-app or installed WidgetKit proofs. All automated gates are green, but packaged runtime validation remains a maintainer-owned hold and this PR is not yet merged. No contributor change request is needed. No live account or credential probe, release, or installed-widget claim is included.
Focused verification command
The render test additionally used an owned temporary output directory through
CODEXBAR_PI_PROOF_DIR.Visual proof
All images contain synthetic data rendered through production views on integration commit
fa1ab1d9fe9. Its final rerender is byte-identical to the inspected attachments. The subsequent Linux import and cache-write corrections change no rendering code or synthetic accounting values.Dashboard before and after icon/source-label cleanup
Before: the first integrated render already includes the accounting repair, but incorrectly calls Pi a subscription and loses the template glyph. This is not the original contributor commit or an installed-app screenshot.
After: the same synthetic amounts, with correct source labels and a visible Pi template glyph.
Combined Overview and Pi cost-only widget in dark appearance