fix(parsers): a UTF-8 BOM silently drops the first entry of a log - #195
Merged
Merged
Conversation
The shared reader in `_io.py` opened every log with `encoding="utf-8"`.
Python's plain `utf-8` codec does not strip a byte-order mark: it decodes
`EF BB BF` to the character U+FEFF and hands it to the caller. The `utf-16`
codec used one line above does strip its BOM, so UTF-16 logs were fine while
the far more common UTF-8-with-BOM log was not.
The consequence is that the first line reached `json.loads` as
`'\ufeff{"message": ...'`, which raises `JSONDecodeError: Unexpected UTF-8
BOM`. Every parser swallows that exception with `continue`, so the first
entry of the session was skipped with no warning and no diagnostic. A log
holding a single entry therefore reported zero usages while the read was
marked faithful -- indistinguishable in every command's output from "no
activity today".
This is the #185 failure mode reached through a third door. The lossy-read
fallback cannot help here, because a BOM file *is* valid UTF-8: the utf-8
attempt succeeds, so nothing is ever flagged as unfaithful. Unlike the two
blind spots fixed by #194, the bytes are intact and there is nothing to
warn about -- the reader simply mis-decoded them.
The fix is to read with `utf-8-sig`, which strips the BOM when present and
is byte-for-byte identical to `utf-8` when it is not, so BOM-less logs are
unaffected. No parser change was needed: the defect lived entirely in the
shared decode step.
A UTF-8 BOM is ordinary output rather than corruption -- it is what
PowerShell's Out-File, many editors, and several exporters emit -- so this
was reachable with a completely valid log file.
Verified: the new test reports 19 failed / 1 passed against origin/main and
20 passed with this fix, on Python 3.10, 3.11, 3.12 and 3.13 (the full CI
matrix). The one test that passes before the fix is the BOM-less control,
which pins the fix against over-stripping. The full suite goes from 276
passed to 295 passed on every version, with no previously passing test
regressed.
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.
The defect
src/agentcost/_io.pyopened every log withencoding="utf-8". Python's plainutf-8codec does not strip a byte-order mark -- it decodesEF BB BFto thecharacter U+FEFF and hands it to the caller. The
utf-16codec used a few linesabove does strip its BOM, so UTF-16 logs were handled correctly while the far
more common UTF-8-with-BOM log was not.
The first line therefore reached
json.loadsas:which raises:
Every parser swallows that with
except json.JSONDecodeError: continue. So thefirst entry of the session was skipped with no warning and no diagnostic.
Why it is worth fixing
A log holding a single entry reports zero usages while the read is marked
faithful -- indistinguishable in every command's output from "no activity
today". The cost silently under-reports, which is the worst direction for a cost
tool.
A UTF-8 BOM is ordinary output, not corruption: it is what PowerShell's
Out-File, many editors, and several exporters emit. The log file is valid andintact.
This is the #185 failure mode via a third door
The lossy-read fallback cannot help here, precisely because a BOM file is
valid UTF-8 -- the utf-8 attempt succeeds, so nothing is ever flagged as
unfaithful. Unlike the cases fixed by #194, there is nothing to warn about: the
reader simply mis-decoded intact bytes.
The fix
utf-8-sigstrips the BOM when present and is byte-for-byte identical toutf-8when it is not. No parser change was needed -- the defect lived entirelyin the shared decode step.
Evidence: before / after
Measured with non-editable installs of each tree, asserting the imported
agentcost._iopath per run (an earlier attempt was invalidated because thevenv's editable
.pthresolved to the working copy rather than the tree undertest):
origin/main+ new testFull suite: 276 passed -> 295 passed on all four versions, no previously
passing test regressed.
The 1 test that passes before the fix is
test_bomless_utf8_log_is_unchanged-- a deliberate control that fails the patchif it starts stripping bytes from logs that have no BOM. The rest fail because
the first entry is genuinely lost.
Reproduce:
What the new tests cover
All five parsers (claude, codex, hermes, opencode, cursor) via parametrization:
warned would pass the first two tests while spamming every Windows-written log
its neighbours, and not aborting a scan
CursorParser(strict=True)must not raise: a BOM is valid UTF-8Why this does not close #185
Deliberately not closing it. #185's two reported blind spots (the unreachable
latin-1 warning and BOM-less UTF-16 mojibake) were addressed by #186 and #194, as
recorded in that issue's own comments. This is a different defect in the same
subsystem, found by probing the decoder after #194 -- the decode succeeds here,
so neither of #185's mechanisms is involved. Closing #185 from this PR would
misattribute work already merged and would hide that a distinct path to silent
data loss existed after the fix. Suggesting #185 be closed on the strength of
#194 alone, with this tracked separately.
Refs #185