Repository navigation
Record which agent produced each history row - #93
Merged
Merged
Conversation
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
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.
history.csvrecorded eight columns with no notion of which agent produced a session, so--historyand--statscould not tell a Claude session's cost from a Codex one. This addsprovideras a persisted dimension.Widening this file is the hazard, not the feature
The old reader was
line.splitn(8, ',')withif fields.len() < 8 { continue }, and the header is written only when the file does not exist — so an upgraded install keeps its 8-column header forever while new rows carry nine. Any header-driven scheme is therefore out, and both naive directions corrupt data:splitn(8)on a nine-field row leaves"0.0100,Codex"in the cost field, whichparse().unwrap_or(0.0)turns into $0.00 — silent cost loss, no error.projectandmodelalready have commas replaced with;at write time — since the commit that created this file — so field counts are unambiguous and per-row detection is safe. The reader splits fully and requires ≥ 8 fields.The ninth field is treated three ways, because "absent" and "unrecognised" are not the same fact:
Claudecodex/CODEX/Codexdo not split a groupThat last row is why
SessionRecord::provideris aStringrather than the enum: a build that predates a third product must still report its rows honestly rather than fold them into a product it happens to know.The upshot is that a single file can hold both widths at once, which is exactly what a user's history looks like after upgrading.
Verified end to end
Against a fixture holding one old-width row and one new-width row:
The pre-column row reads as Claude at its true
$1.25, not$0.00.What it surfaces
--historygains anAgentcolumn.--statsgains a per-agent cost/duration/session breakdown, sorted largest-cost first like the project table, and suppressed when only one agent appears so single-product users see no new noise.Tests
Eleven, each mutation-checked — revert the guard and the matching test fails:
rows_of_either_width_survive_in_the_same_file— the upgrade case. Restoringsplitn(8)fails it on the corrupted cost.an_unknown_provider_label_keeps_the_row— an unrecognised label keeps both the cost and the label. Collapsing it to Claude fails it.a_known_label_is_normalised_for_grouping—codex,CODEX,Codexall group as one.a_header_line_is_never_data_wherever_it_appears— covers both the 8- and 9-column header forms.a_short_row_is_rejected— truncated rows are still refused.a_written_row_round_trips_through_the_parser— for every product inAgentProvider::all().the_header_and_a_row_have_the_same_width— dropping the column from the row fails both this and the round-trip.every_provider_label_round_trips—labelandfrom_labelstay inverse.truncate_does_not_panic_on_a_multibyte_name— restoring the byte slice reproduces the crash.truncate_strips_control_characters— anESCmust not survive into the terminal.format_rowis extracted from the writer purely so the round-trip can be tested without mutatingHOME;HEADERis a constant beside it so the two cannot drift.AgentProvider::from_labelis derived fromall()+label(), so adding a third product cannot leave the inverse behind.cargo clippy --all-targets -- -D warningsclean; 807 tests pass.A second bug, found while auditing row shapes
record_sessioncomputesneeds_headerfrom!path.exists()before opening the file, with no lock, so two processes ending a session at the same moment both write a header — the second lands mid-file. The reader only skipped a header at line 0, so any later one parsed as data:cost_usdfails to parse,unwrap_or(0.0)makes it zero, and a phantom$0.00session inflates every count in--historyand--stats.Pre-existing, but it lives in the function this PR rewrites, so it is fixed here:
parse_recordrejects a header line wherever it appears, which also removes the need for thei == 0special case.Both fixes verified together against a file holding a pre-column row, a Codex row, an unknown-product row and a trailing header —
--statsreports 3 sessions, not 4, and books$0.90toGeminirather than to Claude.A crash found while probing the new column
truncatebounded by bytes —&s[..max - 3]— soagentctl --historypanicked outright on any non-ASCII project directory name:Pre-existing and reachable with a real directory name. It counts characters now, and also strips control characters, because these values come off disk and go to a terminal: a provider label containing
ESC[31mpreviously reached the terminal raw. All three--statsbreakdown loops and the--historyAgent column route their label through it — before this, only per-project did.Verified through the built binary with escapes planted in both the provider and model fields: zero raw ESC bytes in the output, columns intact.
Known limitations, stated rather than hidden
Two display-only issues I deliberately did not fix, both needing a label no writer produces:
{:<25}counts characters while those glyphs take two terminal columns. Fixing it needsunicode-widthas a direct dependency — it is transitive only today — which belongs in its own change, not a migration PR.An older binary reading a file this one has written will still parse
cost,providerinto the cost field and report$0.00for those rows. That is inherent to positional CSV and cannot be fixed from this side. It requires a downgrade after new rows exist; the data is not lost, and re-upgrading reads it correctly. Worth knowing before rolling back.