feat: include OMP session costs#2269
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 18, 2026, 12:26 AM ET / 04:26 UTC. Summary Reproducibility: not applicable. this is an additive feature rather than a report of broken behavior. The contributor nevertheless supplied redacted terminal proof from a real OMP session and a post-change Codex cost scan. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: If maintainers want OMP to be a supported local-cost source, retain the shared pi-compatible parser, entry-ID deduplication, cache invalidation, focused scanner coverage, and explicit provider documentation; otherwise keep the existing pi-only source boundary. Do we have a high-confidence way to reproduce the issue? Not applicable: this is an additive feature rather than a report of broken behavior. The contributor nevertheless supplied redacted terminal proof from a real OMP session and a post-change Codex cost scan. Is this the best way to solve the issue? Unclear: reusing the existing pi-compatible scanner is the narrowest implementation path, but only after maintainers decide that OMP is an intended supported source rather than an unsupported compatible format. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5d1d22630513. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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. How this review workflow works
Review history (3 earlier review cycles) |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab1f762696
ℹ️ 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.applyContributions( | ||
| daysByProvider: &cache.daysByProvider, | ||
| contributions: usage.unkeyedContributions, | ||
| sign: 1) |
There was a problem hiding this comment.
Deduplicate unkeyed mirrored session entries
When the same session is present under both scanned roots but an assistant row has no top-level id (a shape this parser accepts elsewhere and stores in unkeyedContributions), this branch applies those unkeyed totals once per file before the per-session seenEntries filter runs. That means mirrored pi/OMP copies with matching sessionID but id-less assistant usage are still double-counted in the daily totals instead of being counted once.
Useful? React with 👍 / 👎.
|
Merged as Verification:
Thanks @kevcube for bringing OMP session-cost coverage to CodexBar, and for the useful redacted live proof. |
Summary
~/.omp/agent/sessions/**/*.jsonlalongside compatible pi session logsRelated to #384.
Redacted runtime proof
I ran the branch against an actual OMP session. The first command reads only metadata and redacts the session filename and token counts:
A fresh local Codex cost scan on the same branch reports that OMP model. Token and cost values are redacted:
Verification
swift test --build-system native --filter 'scanner merges omp sessions with pi sessions'make checkCodexBarCLIand ran the redacted runtime proof above against local OMP sessionsThe native SwiftPM builder and
DEVELOPER_DIR=/Applications/Xcode-beta.app/Contents/Developerwere used locally because the current macOS 27 command-line toolchain's default builder could not initialize its property-list parser.