Skip to content

Feature/Fix: Add a reversible lineage accounting selector#2150

Closed
iam-brain wants to merge 11 commits into
steipete:mainfrom
iam-brain:iam-brain/lineage-accounting-selector
Closed

Feature/Fix: Add a reversible lineage accounting selector#2150
iam-brain wants to merge 11 commits into
steipete:mainfrom
iam-brain:iam-brain/lineage-accounting-selector

Conversation

@iam-brain

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

Copy link
Copy Markdown
Contributor

Stack dependency

This is slice 11 of 15. It follows Feature/Fix: Define lineage promotion gates; review and merge that PR first.

Summary

  • add internal legacy, shadow, and lineage accounting modes with legacy as the default
  • isolate caches by accounting mode and schema for safe rollback/rebuild
  • run shadow and lineage through the two-pass scanner with debug-only diagnostics
  • combine primary ledger rows with permanent family-scoped containment without double counting
  • fail lineage mode closed before cache publication when evidence is incomplete or cancellation occurs

Validation

  • swift test --jobs 4 --filter CodexLineageAccountingSelectorTests (7 passed)
  • make check
  • env SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX26.5.sdk make test (54 groups, no failures or retries)

Related investigation: steipete/CodexBar#2037.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. 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:20 PM ET / July 14, 2026, 01:20 UTC.

Summary
Adds eight lineage-accounting modules, integrates reversible legacy/shadow/lineage selection and mode-scoped caching into the Codex cost scanner, and adds focused lineage tests.

Reproducibility: yes. at source level: fork-heavy logs with retained metadata identities exercise the new knownIDs and containment-key paths, and the linked successor's frozen-history run demonstrates unresolved incorrect totals. This review did not run private local histories.

Review metrics: 4 noteworthy metrics.

  • Diff size: 18 files, +4,079/-19. The change is a broad scanner rearchitecture rather than a narrow selector patch, increasing review and regression surface.
  • New lineage surface: 8 production modules added. Discovery, ledger, reconciliation, shadowing, residual classification, promotion, and selection must be reviewed as one accounting system.
  • Test surface: 8 test files, +1,653 lines. Coverage is substantial, but it remains synthetic and does not replace proof against real fork-heavy histories.
  • Stack position: slice 11 of 15. The next slice already changes correctness-critical identity behavior, so this head is not the final behavior maintainers would ship.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #2037
Summary: The issue is the canonical fork-lineage overcount report; this PR is a candidate architectural fix, with one prerequisite and one corrective validation successor in the same stack.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
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:

  • Incorporate the successor's physical-owner and scope-key corrections into the actual merge head.
  • Attach redacted current-head frozen-history output showing daily accuracy, cancellation behavior, cache rollback, and cold/warm runtime.
  • Resolve the prerequisite stack order and identify one final reviewed head whose behavior matches what will ship.

Proof guidance:

  • [P1] Needs real behavior proof before merge: This PR lists focused and full tests but no after-fix run on a real fork-heavy history; the linked successor's corpus result includes additional corrections and therefore cannot prove this head. Add a redacted live replay transcript or logs, then update the PR body to trigger review again; if needed, ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] If lineage mode is selected before the successor corrections land, inherited metadata IDs can suppress real parent discovery and containment identities can combine or discard unrelated scoped usage, producing incorrect token and cost totals.
  • [P1] The frozen-history successor reports materially slower lineage scanning and unresolved daily errors; activating shadow or lineage on large histories may increase refresh latency or resource use without yet providing trustworthy totals.
  • [P1] This is cumulative slice 11 of 15, depends on Feature/Fix: Define lineage promotion gates #2149, and is corrected by Feature/Fix: Validate lineage accounting on frozen local histories #2151; merging or reviewing the intermediate head as a complete production implementation would obscure the actual landing behavior.

Maintainer options:

  1. Fold corrections into the landing head (recommended)
    Carry the successor's ancestry and scoped-containment fixes into the reviewed merge head and attach redacted frozen-corpus accuracy and performance proof before landing.
  2. Accept experimental-only risk
    Merge only if maintainers explicitly guarantee legacy remains the sole production authority and immediately own the known incorrect lineage and performance behavior.
  3. Pause the lineage stack
    Do not land the cumulative stack if the unresolved accuracy, performance, and review-size costs outweigh the reversible migration value.

Next step before merge

  • [P1] Review this as part of the ordered lineage stack and require the successor fixes plus real-history proof on the chosen landing head; an automated repair job should not compete with the already-open corrective successor or decide the architecture.

Maintainer decision needed

  • Question: Should this intermediate selector slice remain separately mergeable, or should the lineage stack land only after the successor identity fixes and frozen-history proof are incorporated into the reviewed head?
  • Rationale: The current head is deliberately reversible but contains defects acknowledged by the next stack slice, so only maintainers can decide whether preserving the intermediate commit boundary is worth the accounting and review risk.
  • Likely owner: steipete — The repository owner is the best available decision owner for accepting a large scanner rearchitecture and its staged rollout contract.
  • Options:
    • Land a corrected stack head (recommended): Review the prerequisite first, then require the physical-owner and scope fixes plus corpus proof on the actual head selected for merge while preserving legacy as default.
    • Merge the intermediate slice: Accept this head only as inaccessible experimental scaffolding, explicitly owning that its lineage result is known incorrect until a guaranteed immediate follow-up lands.
    • Pause the rearchitecture: Keep legacy accounting and pursue a narrower fork-overcount guardrail if the full lineage pipeline is too broad or slow to adopt.

Security
Cleared: The diff adds local parsing, hashing, and cache logic but no new dependency, workflow, secret access, permission expansion, or external code-execution surface.

Review findings

  • [P1] Do not treat metadata IDs as discovered documents — Sources/CodexBarCore/Providers/Codex/CodexLineageDiscovery.swift:27-30
  • [P1] Scope containment identity by physical owner and root — Sources/CodexBarCore/Providers/Codex/CodexLineageAccountingSelector.swift:26
Review details

Best possible solution:

Keep legacy accounting as the production default, incorporate the successor's physical-owner and scope-identity fixes into the eventual landing head, and require repeat frozen-corpus accuracy plus cold/warm performance evidence before exposing or promoting lineage authority.

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

Yes at source level: fork-heavy logs with retained metadata identities exercise the new knownIDs and containment-key paths, and the linked successor's frozen-history run demonstrates unresolved incorrect totals. This review did not run private local histories.

Is this the best way to solve the issue?

No, not in this intermediate form: a reversible legacy default is a sound safety boundary, but the landing head should include the successor's identity corrections and real corpus performance and accuracy evidence rather than merge a known-wrong lineage implementation.

Full review comments:

  • [P1] Do not treat metadata IDs as discovered documents — Sources/CodexBarCore/Providers/Codex/CodexLineageDiscovery.swift:27-30
    metadataSessionID may be retained by fork copies, so inserting it into knownIDs can make an actual referenced parent look already present and stop parent discovery. The successor PR explicitly removes this physical-ancestry assumption; key presence by the rollout's physical owner and use metadata only as supporting evidence.
    Confidence: 0.98
  • [P1] Scope containment identity by physical owner and root — Sources/CodexBarCore/Providers/Codex/CodexLineageAccountingSelector.swift:26
    A single string identity cannot distinguish retained IDs or repeated owners across account or session roots, so the component envelope can collapse siblings or unrelated scoped documents and lose legitimate usage. Carry physical owner plus scope through ContainedDocument and deduplicate on that composite key, as the successor PR describes.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a normal-priority correction and rearchitecture for inaccurate local cost accounting, with legacy still the default and no demonstrated emergency runtime impact.
  • add merge-risk: 🚨 compatibility: Selecting this head's lineage mode can change established cost totals using ancestry and containment identities that the next PR explicitly corrects.
  • add merge-risk: 🚨 availability: The linked frozen-history run reports materially slower lineage processing on 819,779 observations, which CI unit tests do not settle.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab 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: This PR lists focused and full tests but no after-fix run on a real fork-heavy history; the linked successor's corpus result includes additional corrections and therefore cannot prove this head. Add a redacted live replay transcript or logs, then update the PR body to trigger review again; if needed, ask a maintainer to comment @clawsweeper re-review.

Label justifications:

  • P2: This is a normal-priority correction and rearchitecture for inaccurate local cost accounting, with legacy still the default and no demonstrated emergency runtime impact.
  • merge-risk: 🚨 compatibility: Selecting this head's lineage mode can change established cost totals using ancestry and containment identities that the next PR explicitly corrects.
  • merge-risk: 🚨 availability: The linked frozen-history run reports materially slower lineage processing on 819,779 observations, which CI unit tests do not settle.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab 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: This PR lists focused and full tests but no after-fix run on a real fork-heavy history; the linked successor's corpus result includes additional corrections and therefore cannot prove this head. Add a redacted live replay transcript or logs, then update the PR body to trigger review again; if needed, ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Repository policy applied: The full repository AGENTS.md was read and applied; its scanner guidance favors focused parser validation, cache safety, bounded runtime behavior, and real runtime proof where appropriate. (AGENTS.md:1, 0437067deaa8)
  • Current supported behavior: Current main documents local Codex JSONL scanning with a codex-v2 cache; lineage accounting is not an existing supported mode. (docs/codex.md:357, 0437067deaa8)
  • Inherited metadata is used as presence identity: The new discovery path inserts metadataSessionID into knownIDs, although fork copies may retain an ancestor metadata ID; that can falsely satisfy a parent lookup and corrupt the discovered family. (Sources/CodexBarCore/Providers/Codex/CodexLineageDiscovery.swift:27, 734f0f6abf09)
  • Containment key lacks scope: ContainedDocument exposes only one string identity, so the selector cannot distinguish the same physical owner or retained identity across Codex roots or account scopes. (Sources/CodexBarCore/Providers/Codex/CodexLineageAccountingSelector.swift:26, 734f0f6abf09)
  • Successor PR identifies required corrections: The open successor Feature/Fix: Validate lineage accounting on frozen local histories #2151 says it stops treating retained metadata IDs as physical ancestry edges and keys containment by physical owner and scope, directly identifying defects in this stacked head. (f95188c17644)
  • Successor corpus result remains promotion-negative: The successor's frozen-history replay reports about 65% lower aggregate error but still has a 10.4% low day, a 4.4% high day, unclassified earlier-day divergence, materially slower performance, and supportsLegacyRemoval false. (f95188c17644)

Likely related people:

  • iam-brain: Beyond authoring this proposal, release history credits this contributor with several previously merged Codex cost-history, pricing-boundary, cache, and model-breakdown changes in the same subsystem. (role: feature owner and recent cost-accounting contributor; confidence: high; commits: 6170f5c8c745, 734f0f6abf09; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift, Sources/CodexBarCore/Providers/Codex/CodexLineageLedger.swift, Sources/CodexBarCore/Providers/Codex/CodexLineageAccountingSelector.swift)
  • BarryYangi: Release history attributes the recent repeated-local-snapshot accounting correction to Codex cost scanner may overcount repeated token_count rows when total_token_usage is unchanged #1062, directly adjacent to lineage deduplication behavior. (role: recent adjacent accounting contributor; confidence: medium; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift)
  • steipete: The repository and current Codex scanner documentation are owned and recently integrated under steipete, making this the strongest available owner for the architectural and rollout decision. (role: repository owner and recent integration owner; confidence: medium; commits: 0437067deaa8, 3e05988f5c5f; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift, docs/codex.md, 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 #2157; closing this draft to keep the upstream review surface manageable.

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. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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