Skip to content

fix: decode BOM-less UTF-16 logs in the shared encoding fallback - #186

Merged
yunaremaia merged 1 commit into
yunaremaia:mainfrom
CodeByPeace:fix/bomless-utf16-fallback
Oct 3, 2026
Merged

yunaremaia merged 1 commit into
yunaremaia:mainfrom
CodeByPeace:fix/bomless-utf16-fallback

Conversation

@CodeByPeace

Copy link
Copy Markdown
Contributor

Addresses #185 (the BOM-less UTF-16 part only).

Description

A UTF-16 log without a BOM was being read as UTF-8, because the NUL bytes are valid UTF-8. Every line then failed json.loads, so the file gave zero usages and no warning.

_read_lines_with_fallback now checks the first two bytes before anything else. If it looks like UTF-16 little-endian or big-endian, it uses that encoding first. Everything else goes through the same loop as before, so UTF-16 files with a BOM and normal UTF-8 files behave the same.

Type of change

  • Bug fix

Testing

  • Tests added in tests/test_utf16_fallback.py for UTF-16 little-endian and big-endian without a BOM. They fail on main with 0 usages instead of 1 and pass with this change.
  • Manual testing performed. I wrote the same log line in four encodings and checked the usage count for each. The two without a BOM gave 0 before the change.
  • Full suite: 216 passed.

Notes

I left the latin-1 and unreachable None part of #185 alone. I asked on #182 which way you prefer, and I'll do it once you say.

The check only catches this when the first character is ASCII, which is true for JSON logs.

AI use: written with Claude Sonnet 5.5. I ran and checked every command and test myself.

The utf-16 codec needs a BOM and the utf-8 attempt succeeds on NUL bytes, so a BOM-less utf-16-le/be log was decoded as mojibake and yielded zero usages. Sniff the first two bytes and try the matching codec first. Addresses yunaremaia#185.
@yunaremaia
yunaremaia merged commit 7d36938 into yunaremaia:main Oct 3, 2026
4 checks passed
@yunaremaia

Copy link
Copy Markdown
Owner

Merged — thanks. This was the right scope for a first PR: 21 lines, CI green, and both new tests fail on main for the reason in the issue (0 usages instead of 1) and pass with the change.

On the latin-1 half, my preference, with the reasoning so you can push back if you read the code differently:

Do not keep latin-1 as a decoding fallback. The measurement in the issue is the argument: latin-1 maps all 256 byte values, so it never raises UnicodeDecodeError, the loop always returns on the third attempt, and return None is unreachable. That is not a stylistic wart — it is why the contract in the docstring cannot be honoured. The useful outcome is a binary check, not a wider net of codecs:

  • decode with utf-8, or the sniffed UTF-16 variant
  • if that fails, do not silently mojibake the bytes. Return a result carrying the lines you could recover plus a warning, so the parsers can surface "this file was not read faithfully" instead of contributing zero usages with no diagnostic.

That second half is the part I would want settled before touching this again, and it is what #185 describes as consequence one: a genuinely binary log currently decodes as latin-1, every line fails json.loads, and the file contributes nothing silently.

One thing worth keeping in mind while you do it: utf-8 with errors="replace" is not equivalent to latin-1 here. Replace-substitution keeps the line structure, so json.loads fails at the same place, but the log tells you it was lossy. If you go that way, please make the warning fire — the current shape makes it easy to add a codec and forget the diagnostic.

And thank you for stating the tooling in the PR description. That is the right way to do it and it makes the change much easier to review and trust.

@yunaremaia

Copy link
Copy Markdown
Owner

Verified on the merged result, on top of what I noted on the latin-1 direction. The scope you picked was correct and the sniff holds up under the cases I could think of. Recording what I actually checked so the next person does not have to re-derive it.

The two-byte sniff. Ran _sniff_bomless_utf16() against every encoding a log realistically arrives in:

input first 2 bytes sniff
utf-8 7b22 None
utf-8 with BOM efbb None
utf-8 with non-ASCII values 7b22 None
latin-1 7b22 None
utf-16 (BOM) fffe None
utf-16-le, no BOM 7b00 utf-16-le
utf-16-be, no BOM 007b utf-16-be

The interesting question was whether a legitimate UTF-8 log can mis-trigger it. It cannot: the trigger requires head[1] == 0 or head[0] == 0, and a NUL byte cannot occur inside a UTF-8 multi-byte sequence, so the only UTF-8 bytes that trigger are files with a literal NUL in the first two positions — which are not valid UTF-8 JSON anyway. I also brute-forced every two-character prefix of an ASCII JSONL start: zero mis-triggers, LE=0 BE=0. A UTF-8 BOM also does not trip it, since efbb has neither byte zero.

Operator precedence. The line is correct, and I confirmed it by AST rather than by eye — the RHS node is an IfExp whose body is the BinOp:

encodings = (sniffed,) + _ENCODINGS if sniffed else _ENCODINGS

if/else binds looser than +, so this is ((sniffed,) + _ENCODINGS) if sniffed else _ENCODINGS, which is the intent: sniffed first, then the unchanged fallback tuple. Behaviourally, sniffed="utf-16-le" yields ('utf-16-le', 'utf-8', 'utf-16', 'latin-1') and sniffed=None yields ('utf-8', 'utf-16', 'latin-1'). A one-line docstring note or a redundant paren pair would save the next reader the AST check, but it is not wrong.

Tests: red/green proof. I checked out the pre-PR parent and put only the PR's test file on top of it. Both new cases fail for exactly the reason in the issue, then pass with the change:

  • pre-PR src/agentcost/_io.py (2d95f13): assert 0 == 1 — 0 usages, no warning. Confirms the "silently yielding zero usages" claim in the CHANGELOG rather than taking it on faith.
  • with the PR: 8 passed.

The parametrization genuinely covers both variants — utf-16-le and utf-16-be each fail red and pass green independently, so neither is a no-op assertion.

Suite. 215 passed locally, 1 failed: test_init_project, which also fails on the pre-PR parent 2d95f13 with the same assertion, so it is my terminal's line-width wrapping .agentcost.toml across a newline, not a regression. That accounts for the 216 in your description.

One edge, deliberately not a blocker. A UTF-32 file (LE or BE) starts 7b00 and is sniffed as utf-16-le. I checked that this is pre-existing and not made worse: UTF-32 yields 0 usages both before and after your change, because UTF-32 is outside the _ENCODINGS contract on both sides. Not a regression, and not something this PR owes — flagging it only so it is not rediscovered later as if it were new.

The CHANGELOG entry is accurate on every checkable claim: correct codecs named, correct issue number, right section, and the silent-zero-usage consequence is exactly what the red test demonstrates.

On the second half of #185, the point that mattered to me was not adding a fourth codec but removing the escape hatch that makes the loop unconditionally succeed. Worth deciding deliberately when you get there — thanks again for keeping this one tight and for flagging the remaining scope rather than folding it in.

@CodeByPeace

Copy link
Copy Markdown
Contributor Author

I'd like to take the latin-1 half of #185.

While checking it I found a crash. A clean latin-1 log with an accent, like "café", makes _parse_file raise UnicodeError: UTF-16 stream does not start with BOM. The utf-16 step raises UnicodeError when there is no BOM, and the loop only catches UnicodeDecodeError, so it never reaches latin-1 for this file. A directory scan with that file between two valid ones aborts the whole run. I ran the same file on the commit before #184 and it crashed there too, with a UnicodeDecodeError.

My plan is to try utf-16 only when the file starts with a BOM, then utf-8. If both fail, the file is read as utf-8 with errors="replace", and on my test file the JSON still parsed. The reader then says the read was lossy, so the parsers can log the warning.

One question. Should --strict make a lossy file fail the run? Only the Cursor parser has --strict today.

@CodeByPeace

Copy link
Copy Markdown
Contributor Author

Opened a PR for the latin-1 half: #193.
It drops latin-1 and warns when some bytes had to be replaced. I did not change --strict yet, so tell me which way you prefer and I'll do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants