Codex provider: decode each rollout once (token-totals seam + fork-prefix fan-out) - #302
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCodex cumulative ChangesCodex token accounting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WholesaleRenderer
participant CodexProvider
participant CacheManager
participant ProjectIndex
WholesaleRenderer->>CodexProvider: load session entries and cumulative totals
CodexProvider-->>WholesaleRenderer: return LoadedSession
WholesaleRenderer->>CacheManager: store session and project aggregates
WholesaleRenderer->>ProjectIndex: render project token totals
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@claude_code_log/converter.py`:
- Around line 3073-3091: Guard the
project_aggregates.update(project_token_totals) call so it runs only when
cumulative provider project totals are available, preserving
compute_project_aggregates(combined_messages) otherwise. Apply the same
availability guard to the project-card data spread, using the existing
provider-total/session-total condition rather than treating all-zero
project_token_totals as valid.
In `@claude_code_log/providers/codex.py`:
- Around line 277-298: Update the usage-record handling around total_raw and
prev_total so missing or non-integer total_tokens records are skipped for
monotonicity and last-record tracking instead of coerced to zero. Only perform
the decrease check and assign prev_total/last_usage when total_tokens is a valid
integer, while preserving the existing reset warning and None return for genuine
decreases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 09a1d0ea-ff84-40a1-b701-c2f6a0245666
📒 Files selected for processing (6)
claude_code_log/converter.pyclaude_code_log/providers/base.pyclaude_code_log/providers/codex.pydev-docs/tools-coverage.mdtest/test_codex_token_accounting.pywork/codex-backlog.md
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 9 minutes. |
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
claude_code_log/converter.py (1)
3053-3080: 📐 Maintainability & Code Quality | 🔵 TrivialReminder: run
just ci(and Ruff/pyright/ty) before pushing this change.As per path instructions,
**/*changes should be validated withjust cibefore pushing, and Python changes (**/*.{py,pyi}) should additionally pass Ruff formatting/linting plus pyright/ty type checking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@claude_code_log/converter.py` around lines 3053 - 3080, Before finalizing the changes around session_totals and has_provider_token_totals in converter.py, run just ci and ensure the Python checks also pass Ruff formatting/linting, pyright, and ty; resolve any reported failures before pushing.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@claude_code_log/providers/codex.py`:
- Around line 507-535: Memoize decoded rollout records by path to avoid repeated
full-file reads and JSON decoding across load_session_under and
session_token_totals. Add a _cached_decoded_records helper, use it in _load_in,
_with_inherited_prefix, and session_token_totals instead of
list(_decode_records(...)), and briefly document the intentional memory-for-CPU
tradeoff.
---
Nitpick comments:
In `@claude_code_log/converter.py`:
- Around line 3053-3080: Before finalizing the changes around session_totals and
has_provider_token_totals in converter.py, run just ci and ensure the Python
checks also pass Ruff formatting/linting, pyright, and ty; resolve any reported
failures before pushing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7dc6577a-a9cd-4960-b93f-f07f8969ac88
📒 Files selected for processing (6)
claude_code_log/converter.pyclaude_code_log/providers/base.pyclaude_code_log/providers/codex.pydev-docs/tools-coverage.mdtest/test_codex_token_accounting.pywork/codex-backlog.md
Codex records token usage as cumulative `token_count` events rather than the per-assistant-message `usage` the Claude path sums, so provider project cards and the index have shown blank token totals. Extract the session total from the last cumulative `token_count` record and thread it into the wholesale walker's project summaries and session cache, so provider index totals now populate like the Claude path. - providers/base: ProviderTokenTotals + a session_token_totals seam (default None; Claude-style per-message providers are unaffected). - providers/codex: read the last cumulative total_token_usage, computed over post-inherited-prefix records so a fork never inherits its parent's counter. Map input = input_tokens - cached, cache_read = cached_input_tokens, output (already includes reasoning); total_tokens is authoritative and never recomputed. cache_creation is omitted, not zeroed. Pre-accounting rollouts return None so their totals are omitted rather than shown as zero. - converter: sum each session's cumulative total into the project card totals and store per-session totals on the session cache, bypassing the per-message usage accumulators a cumulative figure must never flow through. Scope is session/project totals only. Per-message and per-turn attribution is a documented, evidenced limit: a `token_count` follows nearly every agent-loop step (measured ~75.6% tool-execution, ~22.5% assistant/agent over 4138 events), so a per-step delta cannot be attributed to a single rendered message. Account- level fields in the payload (rate_limits, model_context_window) are never read. Pins cover the subtraction (the double-count site), last-record-wins across compaction, the degenerate-record rule, None-for-pre-accounting, the project card totals, session-cache storage, and the no-account-leak guarantee. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ndex rows) The token-accounting section overstated the surfaces: it claimed per-session token rows on the wholesale index, which the code deliberately does not render (the Claude index shows none either, and the drift pin locks the two session- dict shapes together). State what ships — project-card totals on the index and per-session totals persisted to the session cache (Claude-schema parity) — and make the per-session-row deferral visible in the doc, not only the backlog. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…follow-ups Review follow-up (non-blocking notes N2/N3 + a related backlog record). - N3: enforce the cumulative-monotonicity assumption instead of trusting it. _token_totals_from_records now tracks total_tokens across records and, if it ever decreases (a hypothetical future counter reset mid-session), omits the session's totals and warns — the same fail-closed rule used for pre-accounting rollouts. "last" would understate across a reset and max() would report a pre-reset peak; both are confidently wrong, so no number beats a wrong one. Pinned with a synthetic non-monotonic sequence (mutation-checked: disabling the guard returns the post-reset tail). The corpus has zero violations, so the guard fires only on a spec change. - N2: document that ProviderTokenTotals.total_tokens is currently unconsumed by the render/cache paths — it is the reconstruction anchor the tests validate and the reserve a future per-turn layer would use, not a hunt-for-consumer. - Backlog: record the two pre-existing index token-display gaps together — per-session token rows on the HTML index (a Claude-path UX change for its own PR) and the absence of any token display on the Markdown index (a feature that never existed; matters for the vault/Obsidian workflow, where --expand-paths defaults --combined=no). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both raised in review on the open PR. Neither is reachable from the real
corpus, so both are pinned synthetically.
1. A malformed `total_tokens` no longer trips the monotonicity guard.
`total = total_raw if isinstance(total_raw, int) else 0` conflated "field
absent or wrong type" with "value is genuinely 0". A single mid-session
record with a non-int total coerced to 0, compared less than the running
total, and the guard omitted the WHOLE session's totals — for a
data-quality problem, not the counter reset the guard exists for.
Such a record is now skipped instead: out of the ordering comparison and
out of last-record selection, since `_map_cumulative_usage` treats the
stored total as authoritative and would otherwise carry a fabricated 0
through. One warning per session names the shapes actually seen
("absent", "str", ...) so the next surprise is diagnosable from the log
line. `bool` is excluded deliberately — it is an `int` subclass, so a
JSON `true` would otherwise be accepted as the total 1.
The guard itself is unchanged: a genuine decrease between two well-formed
records still omits the session, pinned by its own test.
2. The project-level token override is guarded symmetrically.
`project_aggregates.update(project_token_totals)` was unconditional,
unlike the session-level override twelve lines above it, which is gated
on `session_total is not None`. `_sum_provider_token_totals` returns an
all-zero dict when every session's seam returns None — the default — so
for a provider whose usage lives on its messages this REPLACED real
aggregates with zeros.
Sweeping the neighbourhood found the same asymmetry on the project card
(`**project_token_totals`), which had it for the same reason. Both are
now gated on `has_provider_token_totals`, and the card falls back to the
per-message aggregate rather than to zeros. The fallback is computed
lazily, so the Codex path is unaffected.
The fixture needed care: the only other real provider has no token
accounting at all, so its per-message totals are already zero and
zeroing zeros is a no-op — a test built on it would pass whether or not
the guard existed. The pin therefore uses a provider double that reports
genuine per-assistant-message usage and inherits the default (None)
cumulative seam.
Mutation-verified, each reddening only its own test: restoring the
coercion; allowing `bool` through; unguarding the card override;
unguarding the cache-side override. The last two are independent, which
is what makes them two findings rather than one.
…arning Follow-up to the malformed-total fix. The warning is still one line per session — a pathological rollout must not emit thousands — but a bare count left the reader nothing to search on. It now carries the first offending record's timestamp alongside the count and the shapes seen, so the rollout can be opened at that record instead of re-scanned. The record timestamp was already to hand on the decoded record, so this stays O(1) per session.
`render_provider_wholesale` asked each provider for a session's entries and
then, separately, for its cumulative token totals. For a provider that reads
both from the same file those two calls repeat everything — index lookup,
identity resolution, rollout decode, inherited-prefix strip — and only the
tail differs: normalize for entries, last cumulative `token_count` for totals.
The second decode is not a recomputation worth caching. It is work the first
call already did and discarded.
Measured on a real 34-rollout archive (152 MB), page cache pre-warmed,
`use_cache=False`, both arms in one process:
decodes, totals seam live 354 -> 236
decodes, totals seam absent 236 236
cost of asking for totals +118 -> +0
byte amplification 12.0x -> 8.0x (1908.7 MB -> 1272.5 MB)
hottest single file 42x -> 28x
peak RSS 786.5 MB 785.9 MB (unchanged)
The 202 remaining redundant decodes are pre-existing fork-prefix
amplification — `_with_inherited_prefix` re-decodes a parent once per child —
and are deliberately untouched here.
A decoded-record cache was the other candidate and was rejected on measured
grounds: unbounded it holds 264 MB resident for 152 MB of source, and an
entry-count LRU cannot bound it either, since one rollout decodes to 124 MB.
Any cache here needs a byte budget, which is a design decision rather than a
patch. Restructuring the seam removes the same work at no memory cost.
Shape: `load_session_with_totals` returns entries and totals together, and its
BASE implementation is exactly the pair of calls the walker used to make — so a
provider that does not override it cannot behave differently. `session_token_totals`
remains the seam for a totals-only lookup; only its use by the walker is gone.
Totals are computed BEFORE normalizing, so correctness does not depend on
whether the normalize passes transform their input.
**Note the wall clock barely moves, and that is expected.** Profiling puts
~70s of a ~93s instrumented run in HTML/markdown generation; decoding is not
the bottleneck, so removing 118 decodes is real I/O and CPU saved but not a
visible speedup. This is a resource-use fix, not a performance fix. Interleaved
runs on an otherwise-idle machine: 54.6s vs 56.4s median.
Two pins, both of which were wrong before they were right:
* The decode pin compares the two arms rather than asserting "once per distinct
path" — that is the end state of the wider decode work, not what this change
delivers. It first passed under its own mutation, because stubbing only the
combined seam let a reverted walker take the same path in both arms; it now
stubs totals at the provider level, and reverting the walker fails it with
"totals cost 6 extra decodes".
* The date-filter pin guards what no decode count can see: a session emptied by
`--from-date` must contribute no tokens, not just no messages. Collecting
totals before the survival test would remove decodes AND silently change
project totals. Its obvious mutation does not discriminate — the gate is at
consumption, not collection — so the mutation that reddens it hoists
collection *and* sums unfiltered.
Incidental corroboration for that pin: stubbing the totals seam changes exactly
435 bytes of 116.5 MB of rendered output, and those bytes are the index's
`Input: … | Output: …` line — the string the pin asserts on. The seam
demonstrably feeds the thing the test measures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cboos
force-pushed
the
dev/codex-token-accounting
branch
from
July 28, 2026 23:28
e5079b0 to
50fcdd9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
test/test_codex_token_accounting.py (3)
465-465: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: hoist the repeated
import loggingto module level.Three tests import
logginglocally with no cycle or cost to avoid.Also applies to: 515-515, 544-544
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_codex_token_accounting.py` at line 465, Move the repeated local logging imports in the affected tests to the module-level imports in test_codex_token_accounting.py. Remove the now-redundant imports from the three test bodies while preserving their existing logging behavior.
306-312: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: use keyword args for
ProviderTokenTotalshere, as elsewhere in this file.All four fields are
int, so a future field reorder would silently construct wrong values rather than fail. Line 187 already uses keywords.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_codex_token_accounting.py` around lines 306 - 312, Update the ProviderTokenTotals instances in the _sum_provider_token_totals test input to use keyword arguments for all four fields, matching the existing convention elsewhere in the file and preventing positional values from depending on field order.
129-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: narrow
_decoded's return type to drop the# type: ignore[arg-type]at every call site.Returning the element type
_decode_recordsactually yields (rather thanobject) removes the four suppressions on lines 210, 233, 248, 269 and restores type checking on those calls.As per coding guidelines: "Run Ruff formatting and linting, plus
pyrightand/ortytype checking, when validating Python changes."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_codex_token_accounting.py` around lines 129 - 130, Update the `_decoded` helper’s return annotation to the concrete element type yielded by `CodexProvider._decode_records`, so callers no longer require `# type: ignore[arg-type]` suppressions. Remove the four corresponding suppressions at the call sites and validate the changes with Ruff formatting/linting and pyright or ty.Source: Coding guidelines
work/codex-backlog.md (1)
77-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnchor the template references by block/macro name
The cited
index.html:45-48/:90-91token-summary block exists, but hard-coded template line numbers in a backlog entry can drift on unrelated edits. Use a nearby semantic anchor such as the{% if summary.token_summary %} ... {% endif %}and{% if project.token_summary %} ... {% endif %}blocks instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@work/codex-backlog.md` around lines 77 - 92, Update the backlog entry to reference the semantic template blocks `{% if summary.token_summary %} ... {% endif %}` and `{% if project.token_summary %} ... {% endif %}` instead of hard-coded index.html line numbers, preserving the existing distinction between session and project token-summary behavior.test/test_codex_decode_once.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 TrivialReminder: run
just cibefore pushing these changes, as required by the repository's coding guidelines.As per coding guidelines, "Before pushing changes, remind the user to run
just ci."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_codex_decode_once.py` at line 1, Before completing the changes, run the repository’s full validation command `just ci` and resolve any failures it reports.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@claude_code_log/providers/codex.py`:
- Around line 356-362: Update the _as_int helper used for input_tokens,
cached_input_tokens, output_tokens, and total_tokens to reject booleans
explicitly before accepting integers, returning 0 for boolean or other
non-integer values while preserving normal integer mapping.
In `@test/test_codex_token_accounting.py`:
- Line 451: Complete the type annotations in
test/test_codex_token_accounting.py: at lines 451, 507, and 541 annotate caplog
as pytest.LogCaptureFixture and add the module-level pytest import; at lines
587-594 annotate get_data_dir, discover_sessions, load_session, and
load_session_under consistently with BaseProvider, and annotate _render_permsg’s
monkeypatch parameter at line 641. At lines 129-130 narrow _decoded’s return
type to the element type produced by _decode_records, then remove the related
type: ignore[arg-type] suppressions at lines 210, 233, 248, and 269. Run Ruff
formatting/linting and pyright and/or ty afterward.
- Around line 496-499: Update the warning collection around the comprehension
filtering records containing “total_tokens was” to avoid accessing r.message;
collect the matching LogRecord objects or use r.getMessage() for the collected
values, while preserving the existing len(warnings) assertion.
- Around line 113-121: Update the timestamp construction in the token-total
record loop to format the seconds component as a two-digit, zero-padded value,
preserving valid ISO-8601 output for all record counts and the literal timestamp
expected by test_malformed_total_does_not_omit_the_session.
---
Nitpick comments:
In `@test/test_codex_decode_once.py`:
- Line 1: Before completing the changes, run the repository’s full validation
command `just ci` and resolve any failures it reports.
In `@test/test_codex_token_accounting.py`:
- Line 465: Move the repeated local logging imports in the affected tests to the
module-level imports in test_codex_token_accounting.py. Remove the now-redundant
imports from the three test bodies while preserving their existing logging
behavior.
- Around line 306-312: Update the ProviderTokenTotals instances in the
_sum_provider_token_totals test input to use keyword arguments for all four
fields, matching the existing convention elsewhere in the file and preventing
positional values from depending on field order.
- Around line 129-130: Update the `_decoded` helper’s return annotation to the
concrete element type yielded by `CodexProvider._decode_records`, so callers no
longer require `# type: ignore[arg-type]` suppressions. Remove the four
corresponding suppressions at the call sites and validate the changes with Ruff
formatting/linting and pyright or ty.
In `@work/codex-backlog.md`:
- Around line 77-92: Update the backlog entry to reference the semantic template
blocks `{% if summary.token_summary %} ... {% endif %}` and `{% if
project.token_summary %} ... {% endif %}` instead of hard-coded index.html line
numbers, preserving the existing distinction between session and project
token-summary behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b681d8bf-9fa2-47ef-ad04-25499eff77b2
📒 Files selected for processing (7)
claude_code_log/converter.pyclaude_code_log/providers/base.pyclaude_code_log/providers/codex.pydev-docs/tools-coverage.mdtest/test_codex_decode_once.pytest/test_codex_token_accounting.pywork/codex-backlog.md
🚧 Files skipped from review as they are similar to previous changes (3)
- dev-docs/tools-coverage.md
- claude_code_log/providers/base.py
- claude_code_log/converter.py
Four review findings. The first is the one that mattered.
**`bool` is an `int` subclass, and the exclusion covered only one field.**
`total_tokens` had an explicit boolean rejection at the record-selection guard;
the three *component* fields — `input_tokens`, `cached_input_tokens`,
`output_tokens` — reached `_map_cumulative_usage` unchecked. A record whose
total is well-formed but whose components carry a JSON `true` passes selection
and contributes a phantom 1 to a token column.
Fixed at the shared predicate rather than at the reported field. All four
values route through one `_as_int` helper, so stating the check there covers
every present component and any future one by construction — patching the
named field would have left the same defect in its siblings, which is how it
arrived in the first place.
The pin derives its field list from the usage dict instead of naming fields
inline, for the same reason: an inline list of four silently stops covering the
fifth. Mutation-checked — reverting `_as_int` fails it on
`cached_input_tokens`, a field the report did not name, which is the evidence
that the loop covers more than the reported case.
Three test-helper corrections:
* Zero-pad generated timestamp seconds. `f"…:0{3 + i}Z"` emits `00:00:010Z`
from the 7th record on, and the fixture is load-bearing for a test asserting
a literal timestamp. Fixed in both fixtures that build timestamps this way,
not only the one flagged.
* Filter and collect log records through the same accessor. The filter used
`getMessage()` while the comprehension collected `.message`, which is only
populated once a formatter has run — so the collected value could differ from
what was matched. Only the count is used, so it collects the records.
* Close the annotation gaps that stopped the type checkers verifying these
contracts: `caplog` as `pytest.LogCaptureFixture`, the provider double's
overrides against `BaseProvider`, and `_decoded` narrowed from
`list[object]` to `list[_DecodedRecord]` — which removed all four
`arg-type` suppressions rather than leaving them to hide future drift.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
(Claude) All four findings from the latest review are addressed in Booleans in the mapped components — real, and fixed at the shared predicate rather than the reported field. You're right that All four values route through one The pin derives its field list from the usage dict rather than naming fields inline, so a fifth component is covered automatically instead of silently escaping. Mutation-checked: reverting Zero-padding — real.
Annotation gaps — fixed as specified, including the part that makes it worth doing: narrowing
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/test_codex_token_accounting.py`:
- Around line 165-181: Update the boolean-input regression setup around _usage
and _map_cumulative_usage so the True case is observable: use a zero-cache
baseline or a dedicated input_tokens=True case, ensuring incorrect coercion
yields a detectable mapped value of 1. Preserve coverage for every baseline
field and the existing False-input assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6af014a-9a2f-49a7-9040-d25c3a8a5e8b
⛔ Files ignored due to path filters (6)
test/test_data/real_projects/-Users-dain-workspace-claude-code-log-sample/images/image_0001.pngis excluded by!**/*.pngtest/test_data/real_projects/-Users-dain-workspace-claude-code-log-sample/images/image_0002.pngis excluded by!**/*.pngtest/test_data/real_projects/-Users-dain-workspace-claude-code-log-sample/images/image_0003.pngis excluded by!**/*.pngtest/test_data/real_projects/-Users-dain-workspace-coderabbit-review-helper/images/image_0001.pngis excluded by!**/*.pngtest/test_data/real_projects/-Users-dain-workspace-danieldemmel-me-next/images/image_0001.pngis excluded by!**/*.pngtest/test_data/real_projects/-Users-dain-workspace-danieldemmel-me-next/images/image_0002.pngis excluded by!**/*.png
📒 Files selected for processing (3)
claude_code_log/providers/codex.pytest/test_codex_decode_once.pytest/test_codex_token_accounting.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/test_codex_decode_once.py
- claude_code_log/providers/codex.py
Review caught that the boolean test I had just added was **vacuous for the very field the original report named.** `input = max(input_tokens - cached, 0)`, so with the fixture's `cached_input_tokens=20` an incorrectly accepted `True` (== 1) clamps to 0 — indistinguishable from a correctly rejected boolean. The test passed with the bug present. Two things went wrong and both are worth naming, because the second is the one that let the first through: * The assertion searched the output for a literal `1`. That only works where the wrong value survives arithmetic intact. * The mutation check reported "1 failed" and I read that as the pin working. It had failed on `cached_input_tokens` — a different field. Running the mutation and watching *which* assertion fires, rather than counting failures, would have shown `input_tokens` sailing through. Fixed by making the field observable rather than by strengthening the wording: the baseline now has a zero cache, so an accepted boolean cannot clamp away, and each field is compared against the same usage dict with that field *absent* — the definition of "treated as malformed" — instead of hunting for the wrong value. Comparing `True` against an explicit `0` was also insufficient: both clamp identically under a non-zero cache, which is how the second version stayed vacuous too. Mutation now names `input_tokens` first, and per-field checks confirm all four mapped fields are observable. `reasoning_output_tokens` is not, by design — the mapping never reads it — and the docstring says so, so the asymmetry does not read as a hole later. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Six PNG files (3.6 MB) were committed by accident: they were untracked render output sitting in the worktree, and a `git add -A test` swept them into a commit about a boolean guard. They are absent from main, nothing reads them, and the only `images/image_0001.png` reference in the suite is `tmp_path / "images" / ...` — a temp directory, not these files. The reason they could be staged at all is a stale enumeration. `.gitignore` already had a section excluding generated artefacts under this corpus — `*/cache/` and `*/*.html` — written before referenced-image export existed. That mode writes an `images/` directory next to the HTML it references, so a render pointed at the test corpus drops binaries into a location the list did not cover. Adding `*/images/` completes it, with a note that `git add -f` remains available if an image ever needs to be a genuine fixture. Files are untracked, not deleted: they are legitimate local render output. `test/test_image_export.py` passes (6/6), which is the check that matters — it exercises the export against `tmp_path` and never depended on them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review flagged, as non-blocking, that `load_session_with_totals(max_messages<=0)` returned `token_totals=None` even for a session that has totals. It is worth more than a note: base/override interchangeability is the entire argument for this seam's shape, and that argument had a hole. Measured rather than reasoned, and the first repair was also wrong: * returning `None` disagreed with the base, which loads no entries (`_load_in` returns early) but still calls `session_token_totals` — so the base reports the totals and the override did not; * resolving eagerly to compute them fixed that and introduced the opposite divergence: `FileNotFoundError` on an unknown id where the base returns empty, because on this path the base never resolves at all. So the branch now **delegates to the base**. There is nothing to share when no entries are requested — no decode for the totals to piggyback on — so the override has no advantage to offer, and delegating makes equivalence hold by construction instead of by argument, at the same single decode the base pays. Pinned by comparing the two implementations directly across `max_messages in (None, 0, -1, 5)` plus an unknown id, because the walker never passes `max_messages<=0` and no caller-driven test can reach the difference. Both wrong versions fail it, on different assertions: the `None` variant at `max_messages=0`, the eager-resolve variant on the unknown id. Incidental finding worth knowing: `_SESSION_ID_RE` is not a UUID validator — `"abc"` fullmatches — so an unknown id reaches the index lookup and surfaces as `FileNotFoundError`, not `ValueError`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Requested in review, and worth more than a note because the claim is falsifiable: the pin compares the two implementations against each other, so the subject under test is the *pair* rather than either side reached through a caller. Without that stated, the obvious "simplification" is a render-and-assert, and the walker never passes `max_messages<=0`, so the simplified version would cover nothing. Verified rather than asserted. With the `token_totals=None` defect restored, a full wholesale render still shows the totals and a caller-driven check **passes**, while this test fails. That is the evidence the warning is worth having, and it is now in the docstring — an unverified `DO NOT SIMPLIFY` note is just a plausible one. Also records that the two wrong versions fail on *different* assertions, which is what distinguishes "this test discriminates between them" from "this test goes red when something is off". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Discovery computes each fork's inherited-prefix length, publishes it on
CodexSessionInfo, and throws the identity away. The load path then
recomputed the same value from scratch -- decoding the child and its
parent again -- before decoding the child a third time for its records.
The data was already available and correctly grouped; the load path
simply did not have it.
The index build is where this is cheapest to fix, because it already
reads every rollout's header to map thread ids to paths and discards
everything but the id. Keeping the identity it produced turns three
header reads per rollout (index build, discovery, load) into one, and
gives discovery somewhere to record the resolved prefix for the loads
that follow.
So _index_cache widens from a paths dict to a record holding paths,
headers and resolved identities, rather than gaining a second map beside
it: same key, same lifetime, same staleness assumption, one thing to
reason about. The key stays the RESOLVED root, which is load-bearing
rather than tidy -- an inherited prefix is only meaningful against the
sibling set it was computed within, so a prefix found under one root must
never answer a lookup under another.
Retaining this for the whole run is safe because entry size is constant:
a CodexSessionIdentity is a handful of scalars and two Paths, so bounding
the entry COUNT bounds the memory. That is the property that matters,
not how long it lives -- and it is exactly what a decoded-record cache
lacks, where one rollout is 124 MB and no entry-count bound is a memory
bound. Reasoning from lifetime instead would license the cache that was
already rejected on measurement.
Two invariants are easy to break here and both fail silently:
- A duplicated thread id is RETAINED by discovery (first path wins,
with a warning) but ILLEGAL to load. The index therefore holds a
usable identity for an id whose load must raise, so the ambiguity
check stays ahead of the lookup; behind it, the loader would quietly
return the first rollout.
- inherited_prefix_records == 0 is indistinguishable from "not
computed" if membership is inferred from the value, and 0 is the
COMMON case. Membership in the resolved map is therefore the
computed signal, with entries admitted only after resolution. A
plain non-fork session cannot detect that mistake -- with no parent
the recomputation decodes nothing -- so the test uses a fork child
with a resolvable parent and a genuine zero prefix.
A standalone load that never ran discovery computes the prefix exactly as
before: the fast path must not be the only correct path.
Measured over a frozen 34-rollout corpus (152 MB, page cache pre-warmed,
use_cache=False), decodes fall 236 -> 118 and bytes actually parsed fall
798.4 -> 478.2 MB. This is resource use, not latency -- rendering
dominates the wall clock and is untouched.
Tests count the primitive rather than the output, because correct output
is compatible with arbitrarily redundant decoding -- which is why this
survived a green suite. Each pin was checked against the mutation it
exists for: disabling the reuse reddens the decode counts, inferring
membership from the value reddens only the zero-prefix case, and moving
the ambiguity check behind the lookup reddens only the duplicate-id case.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_with_inherited_prefix decoded the parent inside the per-child call, so a parent shared by k forks was fully materialised k times in a single discovery pass. In the measured corpus one 11.6 MB rollout is the parent of 12 forks, and a 90.8 MB rollout is a parent too: 25 fork children resolve against only 11 distinct parents. Discovery now groups children by parent, decodes each distinct parent once, resolves every one of its children against those records, and releases them before the next group. Peak residency is one parent's candidate list plus one child's -- the same pair the per-child path already held transiently -- so this buys the reduction without a byte budget, and is not the decoded-record cache rejected earlier: nothing survives its group. _prefix_against carries the comparison against already-decoded parent records; _with_inherited_prefix keeps the parent lookup and decode and delegates to it, so a standalone load that never ran discovery behaves exactly as before. Discovery order is preserved independently of the grouping, so SessionInfo emission order does not change. The risk this shape adds is correctness, not cost: sharing one decoded parent between siblings invites resolving the prefix once per GROUP rather than once per child. The test children therefore inherit DIFFERENT amounts, since a fixture whose siblings agree cannot tell the two apart, and the per-path decode budget is asserted as an exact equality -- copying one sibling's answer to the others is caught by the count falling to 2 as readily as by the wrong prefix. Measured over the same frozen 34-rollout corpus: decodes 118 -> 104 (236 -> 104 for the two changes together, over a 34-path floor), bytes actually parsed 478.2 -> 327.8 MB (798.4 -> 327.8 MB together), and the hottest single file 28x -> 3x. Discovery's own byte cost falls 319.6 -> 169.2 MB, which is where this half of the work lands: it removes 14 full parent materialisations rather than many cheap calls. The residue is structural rather than redundant. A rollout is decoded once as a fork child and once as a shared parent when it is both, plus one header read and one decode for its own load, so the per-file ceiling is 4 -- of which one is an early-exit header read. Reaching a lower figure would mean either holding candidate lists across groups (the retention already rejected) or a bounded tail read, which is measurable future work and not assumed here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Its docstrings said a forked session "still re-decodes its parent" and called those 202 decodes pre-existing and out of scope. That described the tree accurately when it was written and does not describe this one: the preceding two commits removed them. The test's own scope is unchanged and still right -- it pins the delta the totals seam adds, not a whole-pipeline figure -- so only the claims about the provider's current state are corrected, with a pointer to where the wider property is now pinned. A scope note that reads as a statement of fact is the kind that gets cited later as evidence the work was never done. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cboos
changed the title
Three in-repo copies said the totals seam cost "+636 MB re-decoded".
That number is Sum(file size x decode calls), which bills EVERY call as a
full file read. It is not one: the decoder streams lazily and
_read_identity returns on the first session_meta record, so 102 of those
354 calls read a single line and were charged a whole file.
Bytes actually handed to the parser, measured with a counting proxy around
the file object rather than a re-implemented decoder:
charged parsed
two-call walker 1908.7 MB 1276.6 MB (354 decodes)
totals seam fixed 1272.5 MB 798.4 MB (236 decodes)
seam cost +636 MB +478 MB (+118 decodes)
So the seam's honest cost is +478 MB, and the charged figure overstates
this corpus by 1.59x. The gap is not an estimate: 102 early-exit calls
against 158.6 MB of source predicts 475.8 MB of fake charge, and the
observed charged-minus-parsed gap is 474.1 MB, leaving 1.7 MB as what
those calls genuinely read.
50fcdd9's message carries the same table and is left alone -- rewriting a
reviewed commit to fix its prose costs more than it buys, and the
correction belongs where people will look next: the code comments, the
test that documents the measurement, and the PR body.
Worth stating in the plainest way, because the figure was arithmetically
correct and reproduced by three independent instruments: a cost metric
that multiplies a per-call constant by a call count is partly a call
count in disguise, and it flatters exactly those changes that remove
cheap calls. The preceding commits remove ~68 early-exit calls; scored
charged that is ~317 MB "saved" and ~1 MB real. Both figures are now
reported side by side wherever they appear, with the parsed one leading.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.gitignore (1)
193-201: 🧹 Nitpick | 🔵 TrivialReminder: run
just cibefore pushing.Run
just cibefore pushing these changes, covering all three reviewed files in this layer.As per coding guidelines, "Before pushing changes, remind the user to run
just ci."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore around lines 193 - 201, Before pushing the changes, run the repository’s full CI command, just ci, to validate all reviewed files.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/test_codex_token_accounting.py`:
- Around line 125-128: Update the record-generation logic in
_rollout_with_tokens to construct each timestamp from a valid datetime base plus
timedelta seconds, rather than formatting 3 + i directly into the seconds field.
Preserve the existing timestamp sequence and literal formatting expectations
while allowing values that cross minute boundaries to roll over correctly.
---
Nitpick comments:
In @.gitignore:
- Around line 193-201: Before pushing the changes, run the repository’s full CI
command, just ci, to validate all reviewed files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 925fb30f-9946-4400-ba3f-9acdcfeaf404
📒 Files selected for processing (6)
.gitignoreclaude_code_log/converter.pyclaude_code_log/providers/codex.pytest/test_codex_decode_once.pytest/test_codex_fork_prefix_decodes.pytest/test_codex_token_accounting.py
🚧 Files skipped from review as they are similar to previous changes (2)
- claude_code_log/converter.py
- claude_code_log/providers/codex.py
Comment on lines
+125
to
+128
| # Zero-padded: an unpadded f"…:0{3 + i}" emits "00:00:010Z" from the | ||
| # 7th record on, and this fixture is load-bearing for tests that | ||
| # assert on the literal timestamp. | ||
| records.append(_token_count_record(tu, f"2026-01-02T00:00:{3 + i:02d}Z")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle timestamp rollover in _rollout_with_tokens.
Line 128 emits 2026-01-02T00:00:60Z when i == 57. This is not a valid ISO-8601 timestamp. Generate the timestamp with datetime and timedelta so larger fixtures remain valid.
Proposed fix
+from datetime import datetime, timedelta, timezone
+
...
- records.append(_token_count_record(tu, f"2026-01-02T00:00:{3 + i:02d}Z"))
+ timestamp = (
+ datetime(2026, 1, 2, tzinfo=timezone.utc) + timedelta(seconds=3 + i)
+ ).strftime("%Y-%m-%dT%H:%M:%SZ")
+ records.append(_token_count_record(tu, timestamp))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Zero-padded: an unpadded f"…:0{3 + i}" emits "00:00:010Z" from the | |
| # 7th record on, and this fixture is load-bearing for tests that | |
| # assert on the literal timestamp. | |
| records.append(_token_count_record(tu, f"2026-01-02T00:00:{3 + i:02d}Z")) | |
| # Zero-padded: an unpadded f"…:0{3 + i}" emits "00:00:010Z" from the | |
| # 7th record on, and this fixture is load-bearing for tests that | |
| # assert on the literal timestamp. | |
| timestamp = ( | |
| datetime(2026, 1, 2, tzinfo=timezone.utc) + timedelta(seconds=3 + i) | |
| ).strftime("%Y-%m-%dT%H:%M:%SZ") | |
| records.append(_token_count_record(tu, timestamp)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/test_codex_token_accounting.py` around lines 125 - 128, Update the
record-generation logic in _rollout_with_tokens to construct each timestamp from
a valid datetime base plus timedelta seconds, rather than formatting 3 + i
directly into the seconds field. Preserve the existing timestamp sequence and
literal formatting expectations while allowing values that cross minute
boundaries to roll over correctly.
Three copies of this file had drifted apart by topic rather than by version, and the parts worth keeping were spread across them. Folded together here: The session-id item. _SESSION_ID_RE is a character-set filter, not a UUID validator, so "abc" passes validation and fails later as a not-found error -- a caller cannot distinguish a malformed id from a missing session. Held out of the decode work deliberately: it changes an exception type at a boundary. Both providers share the pattern AND its use, so a one-sided fix would leave the defect live and make the two disagree about what a bad id does; the Codex side now has two validation call sites rather than one, and citations are against this tree. Item 2, rewritten as-built. It described the fork-prefix half as open and unowned, which the two preceding commits falsified -- true when written, false now, and it reads perfectly either way. It now records what landed, with the figures labelled: 236 -> 104 calls, 6.94x -> 3.06x per path, parsed bytes 798.4 -> 327.8 MB (1276.6 -> 327.8 across the whole arc), peak 727.3 -> 641.8 MB, and a retained identity map costing 20 KB. The hottest-file rows carry their subject, because the earlier "28x -> 3x" tracked one file while the superlative moved to a different one at 4x. What remains is stated as unmeasured: the per-file ceiling of 4 is structural, and going below it needs either retention already rejected on measurement or a bounded parent-tail read that nobody has measured. No figure is offered for either. The byte metric caveat travels with the numbers rather than after them, since the published figures were charged rather than read and the error flatters this exact fix -- collapsing the redundant header reads scores ~317 MB charged and ~1.1 MB real. Over-charge is not a constant (1.495 / 1.594 / 1.482), so charged cannot be converted to parsed. The measurement METHOD is recorded because the corpus is not being kept: freeze and hash the input by content rather than by md5sum output, attach at the single decode primitive and tag phases, run both arms in one process interleaved on an idle box, count the lines actually parsed rather than inferring bytes from calls, and anchor memory on the reproducible max rather than a median. Figures are labelled a historical snapshot of one archive, not a target -- a different fork density will not reproduce them. The standing "avoid long-lived cache state" rule is left in force and reconciled rather than quietly contradicted: what landed retains identities of fixed size, so bounding the count bounds the memory, and the rule still forbids retaining decoded records. The unpushed measurement branch's analysis is marked superseded, so it cannot be merged later and republish the pre-correction figures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Makes the Codex provider decode each rollout once per phase instead of up to
28 times, and adds the session/project token totals that need those same records.
The two halves are one change: Codex records usage as cumulative
token_countevents, so producing totals means having the decoded records — and the reason
totals looked expensive was that the provider was already decoding everything
several times over.
The architecture: three decode sites where one would do
A forked session was fully decoded three times per load, and its parent once per
child:
Two independent fixes:
Reuse what discovery already computed. The index build already reads every
rollout's header and keeps only the thread id.
_index_cachewidens from a pathsdict to a record carrying
paths/headers/resolved, so the load path takesthe prefix length discovery computed instead of re-deriving it. A load with no
prior discovery still computes it, so a caller that never discovers behaves
identically.
Decode a shared parent once per discovery.
_with_inherited_prefixdecodedthe parent inside the per-child call, so a parent shared by k forks was
materialised k times. Discovery now groups children by parent, decodes each
distinct parent once, and releases it before the next group. Memory is bounded by
one parent's candidate list rather than by the corpus — which is why this needs no
cache byte-budget.
Measured
Frozen 34-rollout corpus (158.6 MB), wholesale render,
use_cache=False, pagecache pre-warmed, both arms in one process:
_decode_recordscallsEvery count above includes all
_decode_recordscalls, header reads amongthem. Note that the hottest file changes identity: the 11.6 MB parent of 12
forks — the worst case before, at 28x — now takes 3, while the new maximum of 4
falls on files that are both a fork child and a shared parent (one header
read, one as child, one as parent, one own load). That 4 is the structural ceiling
explained below.
Peak memory falls — fewer parents are materialised concurrently, while the
retained identity map costs 30 KB measured deeply (725 bytes per entry across
34); a shallow
sys.getsizeofreports less by construction, since it does notfollow what the dicts point at. Immaterial either way against an 85.5 MB drop. An
entry-count bound is a memory bound here because entry size is constant: a
CodexSessionIdentityis a fixed handful of scalars. That is the inverse of thedecoded-record cache this PR deliberately does not add, where a single entry
reaches 124 MB and no entry-count bound could constrain it.
A correction to a figure this PR previously claimed
An earlier revision reported "byte amplification 12.0x → 8.0x (1908.7 → 1272.5
MB)". Those are charged bytes —
Σ(file size × decode calls)— which billsevery call as a full file read. But
_read_identityreturns on the firstsession_metaand_decode_recordsstreams lazily, so a one-line read wascharged a whole file. 102 of the old 236 calls were such early exits; charged
1272.5 MB against 798.4 MB actually consumed, a 1.59x over-statement that
closes to 1.7 MB against a predicted 475.8 MB of phantom charge.
This matters here specifically: part of this change removes ~68 of those early
exits, which under the charged metric would score ~317 MB "saved" that is ~1.1
MB real. Every byte figure above is consumed.
What is left, and why it is structural
A rollout that is both a fork child and a shared parent is decoded once in each
role, plus a header read and its own load — a per-file ceiling of 4, one of which
is an early exit. Going below that needs either candidate lists retained across
groups (the corpus-sized retention rejected above) or a bounded tail read. Both
are measurable future work rather than assumed wins.
This is resource use, not latency. Rendering dominates the wall clock and is
untouched — roughly 70s of a ~93s instrumented run is HTML/markdown generation.
Read the numbers above as amplification removed, not as a speed-up.
Token accounting: the field mapping (the reviewable core)
A
token_countrecord'stotal_token_usagemaps onto the index's token columnsas:
input = input_tokens - cached_input_tokens— billable, non-cached inputcache_read = cached_input_tokens— the cached tokens, counted once and only hereoutput = output_tokens— already includes reasoning, so reasoning is not added againcache_creation— structurally absent in the Codex format, so omitted, notzeroed. The renderer shows three of the four token columns and never a "Cache
Creation" one; a blank there is the honest representation.
Keeping
inputandcache_readdisjoint is load-bearing: folding the cachedtokens back into
inputwould double-count them and inflate every total.Totals come from the last cumulative
token_countrecord, bypassing theper-message summation a cumulative figure must never flow through.
Monotonicity is enforced, not assumed
total_token_usageis cumulative, sototal_tokensmust never decrease across asession (compaction lowers the live context window, not the cumulative counter).
If a record's total is lower than its predecessor's, the assumption has broken and
no single record is the honest total — so the session's totals are omitted and a
warning logged, the same fail-closed treatment as a pre-accounting session with
no
token_countevents. A wrong number is worse than an absent one.Per-session totals are stored on the session cache (parity with the Claude
schema); the project card displays the rolled-up total. Per-session token rows on
the index are out of scope — the Claude index does not render them either.
No snapshot churn: nothing about rendered output changes.
Refs #226
Summary by CodeRabbit