Skip to content

fix(cost): write ledger records with json.dumps so quoted model ids aren't dropped#166

Open
RealDiligent wants to merge 2 commits into
mini-router:mainfrom
RealDiligent:fix/critical-issue-cost-ledger-json-escaping
Open

fix(cost): write ledger records with json.dumps so quoted model ids aren't dropped#166
RealDiligent wants to merge 2 commits into
mini-router:mainfrom
RealDiligent:fix/critical-issue-cost-ledger-json-escaping

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Problem

_ledger_append (src/trinity/llm/openai_compatible_pool.py) builds each cost-ledger line by hand:

f.write(
    f'{{"provider":"{provider}","m":"{model}","p":{int(prompt_tokens)},'
    f'"c":{int(completion_tokens)}}}\n'
)

model is route.model_id, read verbatim from configs/models*.yaml. A model id containing a double quote ("), backslash (\), or control character makes the line invalid JSON. The reader, ledger_cost_report (src/trinity/costing.py), wraps json.loads in try/except json.JSONDecodeError: continue, so those lines are silently skipped.

Impact

Silent under-reporting of API spend: every call for such a model disappears from cost_usd, cost_calls, and the per-model breakdown, with no error — and the JSONL file is corrupted for any other consumer. The cost ledger exists to prevent exactly this kind of drift.

Fix

Serialize the record with json.dumps so the writer and reader agree on escaping:

-        with open(path, "a") as f:
-            f.write(
-                f'{{"provider":"{provider}","m":"{model}","p":{int(prompt_tokens)},'
-                f'"c":{int(completion_tokens)}}}\n'
-            )
+        record = json.dumps(
+            {"provider": provider, "m": model, "p": int(prompt_tokens), "c": int(completion_tokens)}
+        )
+        with open(path, "a") as f:
+            f.write(record + "\n")

The line format (keys provider/m/p/c, one JSON object per line) is unchanged for well-behaved ids, so existing ledgers keep parsing.

Tests

New offline tests/test_cost_ledger.py round-trips _ledger_appendledger_cost_report, including a model id containing a quote and a backslash. Verified the metacharacter test fails on the old f-string (cost_calls == 0, call dropped) and passes with the fix. Full router suite green locally (only the pre-existing test_run_remote / test_setup_remote python3-on-PATH failures remain, unrelated).

Risk / tradeoffs

Minimal. Same on-disk format for normal ids; only adds correct escaping. No API or config changes.

Closes #165

@github-actions github-actions Bot added the train Training or routing changes label Jul 13, 2026
@RealDiligent

Copy link
Copy Markdown
Contributor Author

Note on CI: the test-router failure here is pre-existing on main, not from this change. It's tests/test_validate_submission.py::test_wrong_theta_length_fails, which went stale after #150 made the submission size check warning-only — main's own CI is currently red on the same test (tracked in #160, fix open in #161). This PR touches only the cost ledger (openai_compatible_pool.py / costing.py); test-validator and web are green, and the new test_cost_ledger.py passes. Once #161 lands, a re-run here goes green.

@RealDiligent RealDiligent force-pushed the fix/critical-issue-cost-ledger-json-escaping branch from 21e5dd4 to d5e3373 Compare July 13, 2026 05:07
_ledger_append hand-built each ledger line with an f-string, so a model
id containing a double quote, backslash, or control character produced an
invalid JSON line. ledger_cost_report catches json.JSONDecodeError and
skips such lines, so those calls were silently dropped from the cost total
(under-reported API spend) and the JSONL file was corrupted for any other
reader.

Serialize the record with json.dumps so the writer and reader agree on
escaping. Added an offline round-trip test covering a model id with a
quote and a backslash.

Closes mini-router#165

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@RealDiligent

Copy link
Copy Markdown
Contributor Author

Updated: rebased onto current main and CI is now green (test-router, test-validator, web all pass).

The earlier red here was not from the cost-ledger change — it was the pre-existing test_wrong_theta_length_fails, which went dead after the submission size check became warning-only (main's own CI is red on it for every branch; see #160). This PR now also un-stales that test (rename → test_wrong_theta_length_warns, asserting the current warning-only behavior) so CI passes. Happy to drop that commit if #160/#161 lands first — the cost-ledger fix (json.dumps in _ledger_append) is the substantive change and is independent.

After the submission size check became warning-only, a wrong theta length
no longer produces a hard error, so test_wrong_theta_length_fails could
never pass: it asserted result.ok is False and relied on make_spec().n_total
differing from a hardcoded 128 (that n_total is 128 in the CI environment).
main's CI is currently red on this test for every PR branched off it (mini-router#160).

Rename to test_wrong_theta_length_warns and assert the current behavior: a
theta length that disagrees with the summary's declared n_total is surfaced
as a non-fatal warning (ok stays True), independent of the canonical n_total.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

train Training or routing changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cost ledger: model ids with a quote or backslash write malformed JSON and are dropped from the cost total

1 participant