Skip to content

Feature/Fix: Preserve lineage accounting dimensions#2143

Closed
iam-brain wants to merge 4 commits into
steipete:mainfrom
iam-brain:iam-brain/ledger-accounting-dimensions
Closed

Feature/Fix: Preserve lineage accounting dimensions#2143
iam-brain wants to merge 4 commits into
steipete:mainfrom
iam-brain:iam-brain/ledger-accounting-dimensions

Conversation

@iam-brain

@iam-brain iam-brain commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Stack dependency

This is slice 4 of 15. It follows Feature/Fix: Discover referenced long-lived Codex parents; review and merge that PR first.

Summary

  • retain normalized model attribution alongside complete lineage token observations
  • project deduplicated input, cached input, and output totals into UTC and local daily model rows
  • preserve explicit priced/unpriced state and sum long-context pricing per observation
  • resolve copied equal-time attribution deterministically without changing the token fingerprint

This is the fourth isolated component of the planned Codex scanner rearchitecture. Production aggregation remains unchanged.

Validation

  • swift test --filter CodexLineageLedgerTests (13 tests)
  • make check
  • env SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX26.5.sdk make test (640 selections)
  • repository review gate: no unresolved material findings

Related investigation: steipete/CodexBar#2037.

@clawsweeper clawsweeper Bot added 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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 14, 2026
@clawsweeper

clawsweeper Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 13, 2026, 9:16 PM ET / July 14, 2026, 01:16 UTC.

Summary
Adds lineage-parent discovery and a shadow ledger preserving model attribution, token dimensions, pricing state, and UTC/local daily rows, while adapting the Codex rollout parser and adding focused tests.

Reproducibility: no. live high-confidence reproduction is attached to this PR. The linked investigation and focused synthetic tests make the fork-lineage overcounting path source-reproducible, but they do not show a failing current-main scan against a real rollout family.

Review metrics: 3 noteworthy metrics.

  • Patch surface: 6 files, +978/-9. This is a substantial accounting slice touching a central production parser despite leaving production totals authoritative.
  • Source-to-test additions: 490 source, 487 test lines. The branch has strong synthetic coverage by volume, while runtime proof remains the missing validation layer.
  • Stack position: Slice 4 of 15; 1 open predecessor. Review and merge sequencing can materially change the final diff and interfaces.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #2037
Summary: The investigation issue is the canonical report for forked rollout overcounting; this PR is an intermediate candidate fix, with adjacent predecessor and shadow-consumer slices in the same staged implementation.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Proof guidance:

  • [P1] Needs real behavior proof before merge: Only automated test and check results are provided; before merge, add a redacted terminal/live comparison from a real forked rollout lineage, removing private paths, session identifiers, endpoints, keys, and account details. Updating the PR body should trigger a fresh review; otherwise a maintainer can comment @clawsweeper re-review. 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.

Risk before merge

  • [P1] Legacy cost scans now retain a second per-event representation containing timestamps, models, and two token-total structs; large rollout files could therefore regress memory use or scan latency before shadow accounting is enabled.
  • [P1] The predecessor PR remains open, so its final interfaces or review fixes may require this branch to be rebased or adjusted.
  • [P1] Synthetic tests do not establish that real forked rollout variants produce the expected deduplicated daily and pricing totals.

Maintainer options:

  1. Keep shadow collection opt-in (recommended)
    Split lineage parsing or add an explicit collection mode so existing production snapshot scans do not retain the additional observation array.
  2. Accept measured scan overhead
    Proceed only if a representative large-rollout benchmark demonstrates that the extra retained observations do not materially affect scan memory or latency.

Next step before merge

  • [P1] Wait for the open predecessor, require the contributor to remove legacy-scan observation overhead, and obtain real-setup proof; automation cannot provide proof from the contributor's environment.

Security
Cleared: The diff changes local Swift parsing and tests without introducing dependencies, downloaded code, secret access, workflow permissions, or publishing changes.

Review findings

  • [P2] Avoid collecting lineage observations in legacy scans — Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift:1696
Review details

Best possible solution:

Merge the predecessor first, make lineage-observation collection opt-in for ledger parsing so legacy scans retain their resource profile, then validate the rebased ledger with a redacted real forked-rollout before the shadow consumer in #2144 is considered.

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

No live high-confidence reproduction is attached to this PR. The linked investigation and focused synthetic tests make the fork-lineage overcounting path source-reproducible, but they do not show a failing current-main scan against a real rollout family.

Is this the best way to solve the issue?

No, not yet. A lineage-scoped ledger is a plausible narrow model for forked cumulative counters, but collecting its observations unconditionally inside the legacy scan path should be replaced with opt-in collection before merge.

Full review comments:

  • [P2] Avoid collecting lineage observations in legacy scans — Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift:1696
    parseCodexTokenSnapshots remains on the production cost-scan path, but it now retains a second array of model/timestamp/last/total records even when the legacy caller only consumes snapshots. Large histories have previously required careful allocation control; make observation collection opt-in or split lineage parsing so this shadow-only slice does not increase production scan memory.
    Confidence: 0.87

Overall correctness: patch is incorrect
Overall confidence: 0.82

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 0437067deaa8.

Label changes

Label changes:

  • add P2: The PR addresses a limited-blast-radius but user-visible Codex usage-overcounting defect through a substantial internal accounting change.
  • add merge-risk: 🚨 availability: The legacy scan path now retains additional per-token observation data, which could increase memory use or latency on large histories.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Only automated test and check results are provided; before merge, add a redacted terminal/live comparison from a real forked rollout lineage, removing private paths, session identifiers, endpoints, keys, and account details. Updating the PR body should trigger a fresh review; otherwise a maintainer can comment @clawsweeper re-review. 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.

Label justifications:

  • P2: The PR addresses a limited-blast-radius but user-visible Codex usage-overcounting defect through a substantial internal accounting change.
  • merge-risk: 🚨 availability: The legacy scan path now retains additional per-token observation data, which could increase memory use or latency on large histories.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Only automated test and check results are provided; before merge, add a redacted terminal/live comparison from a real forked rollout lineage, removing private paths, session identifiers, endpoints, keys, and account details. Updating the PR body should trigger a fresh review; otherwise a maintainer can comment @clawsweeper re-review. 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

What I checked:

Likely related people:

  • iam-brain: Beyond authoring this proposal, prior merged release history credits this contributor for Codex long-context pricing, tier preservation, and models.dev pricing work in the same accounting area. (role: prior cost-accounting contributor; confidence: high; commits: 6170f5c8c745, 547567d51686, 3b1c89f9bd98; files: Sources/CodexBarCore/Providers/Codex/CodexLineageLedger.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift, Tests/CodexBarTests/CodexLineageLedgerTests.swift)
  • hhh2210: Recent release history connects this contributor to Codex cost-report generation and parser-cache invalidation fixes adjacent to the modified scanner. (role: recent area contributor; confidence: medium; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift, Sources/CodexBarCore/Generated/CodexParserHash.generated.swift)
  • steipete: The repository owner integrates current default-branch and release changes and is the strongest available routing candidate for sequencing this multi-PR scanner rearchitecture. (role: recent integrator; confidence: medium; commits: 0437067deaa8, 3e05988f5c5f; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift, CHANGELOG.md)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • 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.

@iam-brain

Copy link
Copy Markdown
Contributor Author

Superseded by the consolidated four-PR review stack. This slice is now included in #2156; closing this draft to keep the upstream review surface manageable.

@iam-brain iam-brain closed this Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant