diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 13a8715..c653c50 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -18,6 +18,30 @@ protocol. **Newest entries at the top.** Tag each entry with one or more of: --- +## 2026-07-23 — hand-built cost-ledger JSON silently under-reported API spend #mistake #fix #gotcha +**Context:** issue #165 — `_ledger_append` in `src/trinity/llm/openai_compatible_pool.py` writes one +token-usage record per call to the JSONL cost ledger (`TRINITY_COST_LEDGER`). +**Expected:** every recorded call is counted by `costing.ledger_cost_report`. +**Actual:** the record was hand-formatted with an f-string interpolating `provider` and `model` +straight into a JSON literal: +```python +f'{{"provider":"{provider}","m":"{model}","p":{int(prompt_tokens)},"c":{int(completion_tokens)}}}\n' +``` +so any id containing a `"`, a backslash, or a control character emitted a **structurally invalid** +line. The reader skips unparseable lines rather than raising, so the failure was silent: affected +calls simply vanished from the spend total. +**Root cause:** string interpolation into JSON with no escaping — the classic injection shape, here +producing corruption rather than a security issue. Provider/model ids come from config and remote +API responses, so they are not guaranteed to be JSON-safe. +**Fix / decision:** build a `dict` and serialize with `json.dumps(..., ensure_ascii=False)`, which +escapes quotes, backslashes, and control characters correctly by construction. `ensure_ascii=False` +keeps non-ASCII model names readable rather than `\uXXXX`-escaped; the output is still valid JSON +since the file is read back as UTF-8. The `except Exception: pass` guard is retained — ledger +writes must never take down a run. Added `tests/test_cost_ledger_json.py` (14 cases, offline): +adversarial ids (embedded quote, backslash, newline, unicode) round-trip through `json.loads`, and +the totals `ledger_cost_report` computes are unchanged for ordinary ids. +**Follow-up:** none. Worth a grep for other hand-built JSON writers if any remain. + ## 2026-07-12 — code grader: add resource limits on top of the HOME/secrets fix #security #decision **Context:** issue #71 — the code grader (`run_pass_at_1`) runs untrusted miner/LLM candidate code. The core secret-leak fix (isolated throwaway HOME/cwd, scrubbed env, `python -I`) already landed on main. **Finding:** main's sandbox closes the HOME/secrets exfiltration vector but has **no resource limits** — an untrusted candidate can still exhaust host memory or fork-bomb the eval box within its wall-clock timeout (verified: a 4 GiB `bytearray` allocation runs to completion and "passes" on main). diff --git a/src/trinity/llm/openai_compatible_pool.py b/src/trinity/llm/openai_compatible_pool.py index ea33d32..5c33eb4 100644 --- a/src/trinity/llm/openai_compatible_pool.py +++ b/src/trinity/llm/openai_compatible_pool.py @@ -3,6 +3,7 @@ import argparse import asyncio +import json import os import sys import time @@ -69,16 +70,25 @@ def _ledger_append( prompt_tokens: int, completion_tokens: int, ) -> None: - """Append one token-usage record to the cost ledger, if TRINITY_COST_LEDGER is set.""" + """Append one token-usage record to the cost ledger, if TRINITY_COST_LEDGER is set. + + The record is serialized with :func:`json.dumps` rather than hand-built, so a + provider/model id containing a quote, a backslash, or a control character + still produces valid JSON. Hand-formatting silently emitted unparseable lines + that ``costing.ledger_cost_report`` then skipped, under-reporting API spend. + """ path = os.environ.get("TRINITY_COST_LEDGER") if not path: return try: + record = { + "provider": provider, + "m": model, + "p": int(prompt_tokens), + "c": int(completion_tokens), + } with open(path, "a") as f: - f.write( - f'{{"provider":"{provider}","m":"{model}","p":{int(prompt_tokens)},' - f'"c":{int(completion_tokens)}}}\n' - ) + f.write(json.dumps(record, ensure_ascii=False) + "\n") except Exception: pass diff --git a/tests/test_cost_ledger_json.py b/tests/test_cost_ledger_json.py new file mode 100644 index 0000000..0b977f7 --- /dev/null +++ b/tests/test_cost_ledger_json.py @@ -0,0 +1,97 @@ +"""Offline tests for cost-ledger JSON escaping (#165). + +`_ledger_append` hand-built each ledger line with an f-string, so a provider or +model id containing a quote, a backslash, or a control character emitted a line +that is not valid JSON. `costing.ledger_cost_report` skips unparseable lines +silently, so those calls vanished from `cost_usd` / `cost_calls` / the per-model +breakdown — silent under-reporting of API spend, with no error and no warning. + +These tests pin that every such id round-trips as valid JSON and is counted. +Pure stdlib: no network / GPU / torch. +""" +from __future__ import annotations + +import json + +import pytest + +from trinity.costing import ledger_cost_report +from trinity.llm.openai_compatible_pool import _ledger_append + +# Model ids that broke the hand-built writer, plus benign controls. +_TRICKY_IDS = [ + 'weird"model', # double quote -> unescaped, truncated the JSON string + "back\\slash", # backslash -> invalid JSON escape + "ctrl\tchar", # control char -> raw tab is invalid inside a JSON string + "new\nline", # newline -> would also split the JSONL record + "uniécode/model", + "normal-model", +] + + +@pytest.fixture() +def ledger(tmp_path, monkeypatch): + path = tmp_path / "ledger.jsonl" + monkeypatch.setenv("TRINITY_COST_LEDGER", str(path)) + return path + + +@pytest.mark.parametrize("model", _TRICKY_IDS) +def test_each_record_is_valid_json(ledger, model): + _ledger_append("openrouter", model, 11, 22) + lines = [ln for ln in ledger.read_text().splitlines() if ln.strip()] + assert len(lines) == 1 + row = json.loads(lines[0]) # would raise before the fix + assert row["m"] == model + assert row["provider"] == "openrouter" + assert row["p"] == 11 + assert row["c"] == 22 + + +@pytest.mark.parametrize("provider", ['prov"ider', "prov\\ider", "openrouter"]) +def test_provider_is_escaped_too(ledger, provider): + _ledger_append(provider, "some-model", 1, 2) + row = json.loads(ledger.read_text().strip()) + assert row["provider"] == provider + + +def test_no_call_is_dropped_from_the_cost_report(ledger): + for i, model in enumerate(_TRICKY_IDS): + _ledger_append("openrouter", model, 100 + i, 200 + i) + + report = ledger_cost_report(ledger) + + assert report["cost_calls"] == len(_TRICKY_IDS) + assert report["cost_prompt_tokens"] == sum(100 + i for i in range(len(_TRICKY_IDS))) + assert report["cost_completion_tokens"] == sum(200 + i for i in range(len(_TRICKY_IDS))) + + +def test_tricky_model_ids_survive_in_the_per_model_breakdown(ledger): + for model in _TRICKY_IDS: + _ledger_append("openrouter", model, 1, 1) + + per_model = ledger_cost_report(ledger)["cost_per_model"] + for model in _TRICKY_IDS: + assert any(key.endswith(model) for key in per_model), model + + +def test_newline_in_id_does_not_split_the_record(ledger): + # A raw newline would otherwise end the JSONL record mid-string and turn one + # call into two unparseable lines. + _ledger_append("openrouter", "new\nline", 5, 6) + lines = [ln for ln in ledger.read_text().splitlines() if ln.strip()] + assert len(lines) == 1 + assert json.loads(lines[0])["m"] == "new\nline" + + +def test_appends_accumulate(ledger): + _ledger_append("openrouter", "a", 1, 2) + _ledger_append("openrouter", "b", 3, 4) + rows = [json.loads(ln) for ln in ledger.read_text().splitlines() if ln.strip()] + assert [r["m"] for r in rows] == ["a", "b"] + + +def test_no_ledger_env_is_a_no_op(tmp_path, monkeypatch): + monkeypatch.delenv("TRINITY_COST_LEDGER", raising=False) + _ledger_append("openrouter", "model", 1, 2) # must not raise + assert not list(tmp_path.iterdir())