fix(cli): defer unknown-model pricing warnings until after report - #722
Conversation
Collect pricing fallback warnings during inline ingest in `tj quickstart` and `tj backfill claude-code`, then render them after the report body so they no longer appear ahead of output. Fixes Metabuilder-Labs#585.
|
| Filename | Overview |
|---|---|
| tokenjam/core/cost.py | Adds context-local warning deferral, explicit draining/rendering, markup escaping, and fallback logging for unconsumed warnings. |
| tokenjam/cli/cmd_backfill.py | Defers pricing warnings through ingestion and prints them after both successful and zero-session summaries, resolving the prior warning-loss finding. |
| tokenjam/cli/cmd_quickstart.py | Defers warnings until after human-readable reports and routes JSON-mode warnings to stderr. |
| tests/unit/test_backfill.py | Covers warning ordering and the parse-before-filter zero-session regression path. |
| tests/unit/test_cost.py | Covers deferred collection, consumption, fallback logging, deduplication, and Rich markup escaping. |
| tests/unit/test_quickstart.py | Verifies report ordering and stdout/stderr separation for JSON output. |
Reviews (2): Last reviewed commit: "fix(cli): make deferred pricing warnings..." | Re-trigger Greptile
anilmurty
left a comment
There was a problem hiding this comment.
Thanks @syf2211 — the design here is right, and one decision in particular was the non-obvious correct call. Three things to fix before this goes in, all small.
What's right, and worth saying: using contextvars.ContextVar rather than a module global or a threading.local is the choice that actually holds up. core/cost.py is on the ingest hot path — CostEngine.process_span() runs on the ingest thread, [alerts] async_hooks = true adds a worker thread, and the daemon serves concurrent requests through a threadpool. A global would have looked identical in every test and in CI and been a real bug the moment the daemon touched this path. I verified the isolation empirically: a child thread starts with a fresh context and logs normally, so warnings can't leak into or be swallowed by a CLI defer context. The set/reset in try/finally is exception-safe too — I raised inside the block and confirmed the var resets, so the process can't get stuck in defer mode. Also a good catch that --json must stay byte-clean, with a test, which the issue never asked for.
1. The dedup set is marked at collection time, not emission time (blocking)
_UNKNOWN_MODEL_WARNED is marked when a message is collected, so any escape from the with block that doesn't reach print_deferred_pricing_warnings() loses that warning permanently for the process — the dedup budget is already spent. Three live escapes:
- backfill's
sessions_seen == 0early return (this is Greptile's P1 — see below); - any exception inside the block — a DB failure mid-backfill, or a Ctrl-C during a long ingest;
print_deferred_pricing_warnings(messages=...)doesn't clear the list it printed (only thedrain_...()path does), so the two call modes have different semantics and calling it twice double-prints.
A swallowed pricing warning is worse than a mistimed one: that warning is the signal that a model fell back to default rates, so every dollar figure downstream is wrong and the user has no idea.
Rather than patching each call site, I'd make it fail-safe in the context manager: in defer_pricing_warnings's finally, after reset(token), logger.warning anything the caller didn't consume, and have print_deferred_pricing_warnings clear the list it printed. That closes all three at once and makes every future with defer_pricing_warnings() correct by construction.
2. Greptile's P1 is valid and live
Confirmed rather than assumed — the branch has one commit at 00:17:18Z and the P1 is dated 00:21:28Z, four minutes later, with no follow-up pushes. The mechanism: in iter_claude_code_sessions, parse_claude_code_session() (which calls calculate_cost) runs before the ended_at < since filter, so a file whose mtime passes the cheap prefilter but whose last message predates the cutoff gets priced, collects the warning, and is discarded without incrementing sessions_seen. Reproduced end to end with a 2020-dated transcript and an unknown model: exit 0, "No sessions found", and the pair already in the warned set. Fix 1 above resolves this case too.
3. Rich markup injection in the warning line
console.print(f"[yellow]{message}[/yellow]") interpolates a model name — arbitrary text from a transcript or any backfill adapter — into Rich markup unescaped:
ll[/]ama-3→MarkupError: closing tag '[/yellow]' doesn't match any open tag, i.e. the CLI crashesmy[model]v2→ renders asmyv2
The second is the worse one in practice, since the entire job of that message is to hand the user the exact model id to add to pricing.toml. from rich.markup import escape and wrap it. CLAUDE.md already flags this pattern for cmd_policy.py.
Nit, non-blocking: raw [yellow] should be the named warn role from tokenjam/utils/theme.py (Critical Rule 35 — identical rendering, auditable in one place). There's precedent for raw [yellow] elsewhere in cmd_backfill.py, so this is genuinely a nit, but this is new code in core/.
Also: this is based on the 0.6.10 merge and main is now at 0.6.11, so it'll need gh pr update-branch before merge.
Tests are load-bearing — I reverted just the two CLI files and all three new tests failed, and they pin ordering rather than presence. Ping me when it's updated.
- Log unconsumed warnings in defer_pricing_warnings finally block - Clear messages list when print_deferred_pricing_warnings renders them - Escape Rich markup and use warn theme role for warning output - Surface deferred warnings on backfill zero-session early return - Add regression tests for escape, fail-safe, and zero-session path
|
Thanks for the detailed review @anilmurty — addressed all three blocking items in 21b78be:
Added regression tests for fail-safe finally, consumed-path no double-log, Rich escape, and the zero-session backfill path. Merged upstream main (0.6.11). All targeted tests pass. |
anilmurty
left a comment
There was a problem hiding this comment.
Approving — all three asks addressed, and I verified each by running it rather than reading the diff.
- Zero-session early return (the P1): the real CLI repro now prints
No sessions found.and then the pricing note. Reverting the fix hunks brings back the silent swallow, so the behaviour is genuinely pinned. - Exception / Ctrl-C: the warning reaches stderr via the
finally, and the nextcalculate_coston a different unknown model still warns — so the dedup budget isn't corrupted, which was the part I actually cared about. - Double-print: calling the print twice emits once, and the mixed drain-then-messages case behaves the same. The two call modes now have identical semantics.
- Markup:
ll[/]ama-3renders literally with noMarkupError, andmy[model]v2keeps its brackets — so the string a user pastes intopricing.tomlis byte-exact.warnrole taken too.
No regressions: contextvars isolation still holds under a child thread, no double-report on the success path, the fallback goes through the logger so --json stdout stays byte-clean, and the merge of 0.6.11 is a clean auto-merge with nothing reverted. All four new tests fail when the fix is reverted.
The fix is better than the one I asked for. I suggested the finally fallback or patching the call site; you did both, and the split is the right way round. The explicit print_deferred_pricing_warnings at backfill's zero-session return means the user gets the note on the console, in the report's own voice and position, while the finally is the safety net for the paths nobody enumerated — an exception, a Ctrl-C, the next early return someone adds six months from now. If you'd only done the finally, the P1 would be technically fixed while the user saw a raw stderr log line ahead of the report, which is the exact defect #585 was opened about.
Two optional nits, neither worth another round:
print_deferred_pricing_warningsis a rendering function living incore/, and itsrich.markupimport is the only rich import in that package. Not a rule violation — the ban is core→cli/api — butdefer_/drain_are the domain half and the print is presentation, so the latter would sit more naturally incli/orutils/formatting.py.messages.clear()runs before the print loop, so aBrokenPipeErrormid-loop (tj backfill | head) would drop the remainder with thefinallyseeing an empty list. Clearing after the loop closes it.
Summary
Defer unknown-model pricing fallback warnings during inline ingest in
tj quickstartandtj backfill claude-code, then render them after the report/summary body.Motivation
When ingest runs before rendering,
calculate_cost()could emit pricing warnings to stderr while the CLI was still silent or before the report printed. That made the first visible output look like an error ahead of the actual report (#585).Changes
defer_pricing_warnings()/print_deferred_pricing_warnings()intokenjam/core/cost.py.--json).tj backfill claude-codesummary output similarly.Tests
All passed.
Also ran:
ruff check tokenjam/ ...— passedmypy tokenjam/core/cost.py tokenjam/cli/cmd_quickstart.py tokenjam/cli/cmd_backfill.py— passedNotes
logger.warning.Fixes #585