Repository navigation
feat(usage): add the model usage and cost ledger - #344
Conversation
Implements #266 against the ADR-012 contract (mctlhq/mctl-agents, docs/adr/012-model-usage-cost-attribution-contract.md). The ledger is a financial read model at the grain (run, model), so that "what did this DevLoop cost" stays answerable after trace data has aged out. Store, pricing catalog, ingestion and the query surface, plus a nil-store 503 that must never degrade into an empty 200 — an operator reading zero spend from an unconfigured ledger would conclude the opposite of the truth. Three decisions worth stating, because each is easy to undo later: Idempotency is the primary key. The row id is sha256(session_id | result_uuid | model_key), so a Temporal retry, a workflow replay, a restart mid-write and an export reprocessing all land on the same row and `ON CONFLICT DO NOTHING` makes the repeat a no-op. retry_attempt is deliberately NOT in the key: a retry that re-runs the model is a genuinely different invocation carrying a new session_id and a real new charge, and deduping it would lose money that was actually spent. Absent is not zero. Every token column is nullable. A provider that does not report cache tokens and a run that read no cache are different facts, and SUM() ignores the first while counting the second. NOT NULL DEFAULT 0 would have silently converted "not measured" into "nothing spent", so the Go types are *int64 and the summary buckets stay nullable too. Rates live in a file, not in the binary. A published price is a fact about the world with an effective date; a wrong constant produces plausible-looking money and a price change would need a release. Cost is derived once at ingest and stored with the rate-card version that produced it, which is what makes a later card unable to reach back and rewrite history. The record type has no prompt, completion or message field. That is enforced by construction rather than by a redaction pass, so a future producer written by someone who never read the ADR still cannot leak conversation text into the ledger; a reflection test asserts the absence rather than testing a scrubber. Tests cover the ADR's nine invariants at the layer each actually lives in. The idempotency, absent-versus-zero round trip, price-change immutability and the CHECK constraint are exercised against real Postgres, because a mocked pool can only show that the code handles a conflict, never that the database produces one. The route-registration test is there because a handler with no route stayed invisible once already in the lifecycle work. Note on the issue's first acceptance criterion: "ingested idempotently from the DevLoop pilot" cannot be satisfied inside this issue, because the pilot instrumentation is .github#50 and the canonical graph has #50 depending on this item, not the reverse. Read here as "the ingestion path is proven idempotent against pilot-shaped records"; the live producer arrives with #50. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 4m 22s —— View job Code review complete
Verdict submitted against Blocking (P2)
Non-blocking (P3)
What holds up wellThe Caveat
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Finding 1: Nil pointer dereference when ingesting batch containing null record
Finding 2: Pricing catalog ignores
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if c == nil { | ||
| return Pricing{}, false | ||
| } | ||
| list := c.byModel[pricingKey(model)] |
There was a problem hiding this comment.
P2 — Pricing.Provider is accepted, documented in the rate-card example (docs/model-usage-ledger.md:38, catalog_file.go:37) and then never consulted. NewCatalog keys byModel on pricingKey(e.CanonicalModel) alone (line 68) and At resolves on model + instant only, so a Bedrock card and a firstParty card for the same canonical model land in one list and whichever has the later effective_from prices both. Those rates genuinely differ per provider, so this silently produces plausible-looking wrong money — the exact failure the package doc says it exists to prevent.
Two ways it bites:
[{provider: "bedrock", effective_from: 2026-09-01, input_per_mtok: 20}, {provider: "firstParty", effective_from: 2026-01-01, input_per_mtok: 10}]prices every firstParty invocation after September at the Bedrock rate.- Two cards for the same model with the same
effective_fromand different providers:sort.Slice(line 73) is not stable, so which one wins is unspecified and can differ between process restarts. Identical records priced differently across restarts is directly against invariant 7.
Either key the catalog on (canonical_model, provider) with a provider-agnostic fallback, or reject a catalog that carries a non-empty provider until it is honoured. Also worth making the sort deterministic (sort.SliceStable, or tie-break on Version) and rejecting duplicate (model, provider, effective_from) triples in NewCatalog. No test covers a multi-provider catalog today.
There was a problem hiding this comment.
Confirmed and fixed in 5d56091. The catalog now keys on (canonical_model, provider); a card with no provider is an explicit wildcard used only when the provider has no card of its own. sort.SliceStable, and two cards sharing (model, provider, effective_from) are now rejected at load rather than ordered — as you say, there is no correct answer there, and picking one silently breaks invariant 7 from a second direction. Covered by TestPricingIsResolvedPerProvider, TestProviderlessCardIsAWildcard and TestCatalogRejectsTwoCardsEffectiveAtTheSameInstant. agy raised the same defect independently (its finding 2).
| if strings.TrimSpace(r.ModelKey) == "" { | ||
| return fmt.Errorf("%w: model_key is required", ErrInvalidRecord) | ||
| } | ||
| if r.ID == "" { |
There was a problem hiding this comment.
P2 — the deterministic id is only used when the producer did not supply one, and Record.ID is decoded straight off the wire (json:"id", line 88) by ingestUsageRequest. So the guarantee the PR description calls "idempotency is the primary key" is opt-out: a producer that sets "id": "<uuid4>" on each delivery gets a new row per retry and double-counts a paid invocation, and a producer with an id-generation bug can pre-occupy the sha256 of a future real invocation, which ON CONFLICT DO NOTHING then silently drops.
Every test in this PR omits id, so nothing catches a producer that sends one. Suggest recomputing unconditionally here:
r.ID = DeterministicID(r.SessionID, r.ResultUUID, r.ModelKey, r.NumTurns)or, if a supplied id must be accepted, comparing it against the derived value and returning ErrInvalidRecord on mismatch. The field can still be serialised on read — it is only the ingest direction that should not trust it.
There was a problem hiding this comment.
Correct, and the sharpest finding in the round — the guarantee the PR description leads with was opt-out. Fixed in 5d56091: EnsureID now derives the id unconditionally, and a supplied id that disagrees returns ErrInvalidRecord rather than being silently overwritten, so a producer computing the key wrongly finds out instead of quietly double-counting. TestSuppliedIDCannotBypassTheDedupeKey covers both directions, and TestUsageHandlers_ClientSuppliedIDIsRejected covers it over HTTP, which is the path that actually mattered.
| SUM(provider_reported_cost), SUM(calculated_cost), SUM(invoice_reconciled_cost) | ||
| FROM model_usage_records%s | ||
| GROUP BY %s | ||
| ORDER BY COUNT(*) DESC, key ASC`, col, where, col) |
There was a problem hiding this comment.
P2 — Summary emits no LIMIT and ignores Filter.Limit, and a zero Filter matches everything. GET /api/v1/usage/summary?group_by=temporal_workflow_id (or work_item_id) with no since/until is a legitimate operator query that returns one bucket per distinct workflow id in the whole ledger — unbounded rows, all accumulated into out and then JSON-encoded in one buffer. List is capped at 200/1000 rows; the aggregate path, whose cardinality is the one that actually grows without bound over months, has no cap at all.
Suggest applying Filter.Limit (with the same default/ceiling as List) plus an explicit LIMIT $n here, and signalling truncation in the response so an operator cannot read a clipped bucket list as the complete breakdown.
There was a problem hiding this comment.
Confirmed and fixed in 5d56091. Summary now takes the same page bounds as List and the response carries truncated and limit. Your point about which side needed it more is the right one: List caps rows that are bounded per query anyway, while the group-by cardinality on temporal_workflow_id/work_item_id grows for as long as the ledger exists. TestSummaryIsBoundedAndSaysSo asserts both the cap and the flag.
| where, args := f.where() | ||
| limit := f.Limit | ||
| if limit <= 0 || limit > 1000 { | ||
| limit = 200 |
There was a problem hiding this comment.
P2 — asking for more rows silently returns fewer. usageFilterFromQuery only rejects limit <= 0, so ?limit=5000 reaches here and falls into the limit > 1000 branch, which resets to 200 rather than clamping to 1000. A caller who asks for 5000 gets a smaller page than a caller who asks for 1000, and nothing in the response says so — ListUsageRecords returns "count": len(records), which reads as "this is how many records match".
That matters more here than on a status endpoint: this file is a financial read model, and anyone summing records[].calculated_cost client-side over a silently clipped page understates real spend. Two things worth fixing together:
- clamp instead of shrink (
if limit > 1000 { limit = 1000 }), or rejectlimit > 1000as a 400 in the handler — the same "an unparseable filter is an error, never a dropped predicate" rulehandlers_usage.go:129states; - make truncation visible (a
truncatedflag or a cursor), since with no pagination at all any result at the cap is indistinguishable from a complete one.
There was a problem hiding this comment.
Confirmed and fixed in 5d56091, both halves. The store clamps to the ceiling instead of resetting to the default, and the handler rejects limit>1000 as a 400 — consistent with the "an unparseable filter is an error, never a dropped predicate" rule you quoted from the same file. Truncation is now visible on the summary side; on List the clamp plus the 400 means a caller can no longer receive silently fewer rows than it asked for. TestOverLargeLimitClampsRatherThanShrinks and TestUsageHandlers_OverLargeLimitIsRejected.
| } | ||
| parts = []string{sessionID, modelKey, turns} | ||
| } | ||
| sum := sha256.Sum256([]byte(strings.Join(parts, "|"))) |
There was a problem hiding this comment.
P3 — the parts are joined with an unescaped |, so distinct field tuples can produce the same digest, and a collision here is a silently dropped paid invocation (ON CONFLICT DO NOTHING). Two shapes:
- delimiter injection:
("a", "b|c", "d")and("a", "b", "c|d")both hasha|b|c|d; - the fallback produces a 3-part string of the same shape as the uuid path, so
(session, resultUUID: "X", model: "3")collides with(session, resultUUID: "", model: "X", numTurns: 3).
Unlikely with uuid-shaped session ids, but free to rule out: hash length-prefixed parts, or give the fallback its own discriminator (e.g. "v1:noresult" as a leading element). TestDeterministicIDFallsBackWithoutResultUUID covers the non-colliding case only.
There was a problem hiding this comment.
Taken, despite the low probability — the consequence is what decided it: a collision here is not a bad read but a paid invocation that ON CONFLICT DO NOTHING discards silently, and it costs nothing to rule out by construction. 5d56091 length-prefixes every part and gives the two shapes distinct discriminators (v1.uuid / v1.noresult), so neither of your cases can collide. TestDeterministicIDResistsDelimiterInjection covers both.
| web_search_requests BIGINT, | ||
|
|
||
| provider_reported_cost DOUBLE PRECISION, | ||
| calculated_cost DOUBLE PRECISION, |
There was a problem hiding this comment.
P3 — DOUBLE PRECISION for money in a ledger whose stated purpose is reproducible cost. SUM(double precision) in Postgres is order-dependent, and under a parallel aggregate plan the partial-sum order is not fixed, so the same GET /usage/summary can return slightly different totals on successive calls over a large table. That undercuts the reproducibility the calculated_cost_requires_version CHECK just above is there to protect.
NUMERIC would make both the stored value and SUM() exact and deterministic. Cheap to change now while the table is empty; a type change later is a migration on a table nobody wants to rewrite.
There was a problem hiding this comment.
Taken. NUMERIC in 5d56091. Your framing decided it: the table is empty today and a type change later is a migration nobody wants, so the cost of doing it now is approximately zero and the cost of deferring is not. I re-ran the Postgres-backed tests against a freshly dropped table rather than the existing one, since CREATE TABLE IF NOT EXISTS would have left the old DOUBLE PRECISION columns in place and the change would have gone untested.
| } | ||
| r.Body = http.MaxBytesReader(w, r.Body, usageMaxBodyBytes) | ||
| var req ingestUsageRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { |
There was a problem hiding this comment.
P3 — a batch that trips the 1 MiB MaxBytesReader surfaces as 400 "invalid JSON body". The producer has no way to tell "your JSON is malformed" (do not retry) from "your batch is too big" (split and retry), and it is inconsistent with the record-count cap eleven lines below, which correctly answers 413. docs/model-usage-ledger.md:77 presents both caps as one rule.
errors.As(err, new(*http.MaxBytesError)) → 413 with a "batch exceeds 1 MiB" message would make both caps self-describing.
There was a problem hiding this comment.
Fixed in 5d56091 — errors.As(err, new(*http.MaxBytesError)) → 413 with a byte-count message. You are right that it was inconsistent with the record-count cap eleven lines below and that the doc presents both as one rule; the doc now states both as 413. TestUsageHandlers_OversizedBodyIs413NotMalformedJSON.
| } | ||
| } | ||
| if err := r.Validate(); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
P3 — one bad record rejects the whole batch. Validation runs over every record before the transaction opens, so a single malformed entry in a 500-record batch means none of the other 499 paid invocations land, and the handler turns it into a 400 that gives the producer no way to know which record was at fault. If the defect is in the producer rather than transient, that batch never lands at all — which is the "the money is spent whether or not the row lands" failure the router comment at router.go:472 says the ledger cannot absorb.
The all-or-nothing write is right; it is the all-or-nothing validation that costs here. Consider partitioning: insert the valid records, and return the rejected ids with their reasons ({"accepted": n, "deduped": n, "rejected": [{"index": i, "reason": "..."}]}), which also removes the ambiguity in the current 400.
There was a problem hiding this comment.
Partially taken, and I want to be explicit about the part I did not take. The ambiguity is fixed: a rejection now names the offending index (record 7: ...), so a producer knows what to correct rather than re-sending 500 records blind. I did not implement partial acceptance. It changes the ingest contract rather than fixing a defect, and the argument for it depends on a producer that emits malformed records persistently — which does not exist yet, since the first producer arrives with .github#50. Deferring it also keeps the success response one shape rather than two. If the pilot shows records being rejected in practice, that is the evidence for doing it, and it should be its own PR so it can roll back independently. Flagging the disposition rather than silently declining.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 4 P2, 3 P3.
Well-structured ledger and the invariant-per-layer test design is genuinely good. Four things block:
- Pricing is provider-blind (
pricing.go).Pricing.Provideris parsed, documented in the rate-card example, and never consulted — a Bedrock card can price a firstParty invocation. Equaleffective_fromentries are also resolved by a non-stablesort.Slice, so pricing can differ between restarts. - Client-supplied
idbypasses the dedupe key (types.go:181).EnsureIDonly derives the sha256 whenid" is empty, andid` is decoded straight off the wire — so the central idempotency guarantee is opt-out, and no test sends one. Summaryis unbounded (store.go:429). NoLIMIT, ignoresFilter.Limit, and a zero filter matches everything:group_by=temporal_workflow_idreturns a bucket per workflow in the whole ledger.limit>1000silently returns 200 rows (store.go:315), fewer than asking for 1000, with no truncation signal — on a surface whose own doc says a filter must never be silently dropped.
P3s: hash delimiter ambiguity, DOUBLE PRECISION for money, 1 MiB overflow reported as "invalid JSON body", all-or-nothing batch validation. Note I could not execute go build/go test in this environment, so this is a static review; the Postgres-backed tests were not re-run here.
Claude raised 4 P2 / 3 P3; agy raised 3 P2 (one of which claude did not find)
and 2 P3. All verified against the code before fixing; none dismissed.
P2 — a null record panicked the process (agy 1). {"records":[null]} decodes to
a nil element and the ingest loop dereferenced it. Reproduced as a panic first,
then fixed: nil is rejected as ErrInvalidRecord naming the index, and the guard
also sits on EnsureID/Validate so a non-HTTP caller cannot reach it either.
P2 — the dedupe key was opt-out (claude 2). EnsureID only derived the sha256
when `id` was empty, and `id` decodes straight off the wire, so a producer
emitting a uuid per delivery would get a row per retry and double-count a paid
invocation. The id is now always derived; a supplied id that disagrees is a 400
rather than a silent overwrite, so a producer computing it wrongly finds out.
P2 — pricing was provider-blind (claude 1, agy 2). Pricing.Provider was parsed,
documented in the rate-card example and never consulted, so a Bedrock card
priced a firstParty invocation at Bedrock rates. The catalog now resolves on
(canonical_model, provider) with a provider-less card as an explicit wildcard.
sort.Slice is also not stable, so two cards sharing an effective_from could
resolve differently between restarts — those are now rejected at load, since
two cards in force at one instant have no correct answer.
P2 — Summary was unbounded (claude 3). No LIMIT and a zero filter matches
everything, so group_by=temporal_workflow_id returned a bucket per workflow
that ever ran. Now capped like List, and the response carries truncated/limit:
a clipped breakdown is otherwise indistinguishable from a complete one.
P2 — a model filter missed its own records (agy 3). A producer reporting only
model_key leaves canonical_model empty, so filtering on canonical_model omitted
those rows and a repo's spend read lower than it was. The filter now matches the
model_key fallback rather than writing model_key into canonical_model, which
would have made the record claim something the provider never reported.
P2 — an over-large limit shrank the page (claude 4). limit>1000 reset to the
200 default, so asking for 5000 returned fewer rows than asking for 1000, with
nothing saying so. The handler now rejects it as a 400 like every other filter,
and the store clamps for direct callers.
P3s taken: the id parts are length-prefixed with per-shape discriminators, so no
field tuple can collide with another (a collision is a paid invocation that ON
CONFLICT DO NOTHING discards); money is NUMERIC, since SUM(double precision) is
order-dependent under a parallel plan and the table is empty now; a body over
the byte cap answers 413 instead of "invalid JSON body", so a producer can tell
"do not retry" from "split and retry"; a rejected batch names the offending
index. Catalog loading and the list success path gained the tests agy noted
were missing.
Not taken: partial batch acceptance (claude P3, store.go:195). Returning
per-record rejects changes the ingest contract rather than fixing a defect, and
the ambiguity it was aimed at is gone now that the error names the index. Worth
its own PR if the producer in .github#50 turns out to need it.
Lint: gosec G304 on the catalog read is annotated at the call site rather than
excluded repo-wide — the path is an operator-set env var, and a repo-wide
exclude would hide the next variable-path read. Dropped an unused test helper.
Every fix has a regression test, and the Postgres-backed ones were re-run
against a freshly dropped table so the NUMERIC change is actually exercised
rather than inheriting the old column type.
Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 7m 35s —— View job Code review complete
Verdict submitted against Round 1 is genuinely clearedChecked against the code, not the commit message. The catalog keys on Blocking (P2)
Non-blocking (P3)
What holds up wellThe Caveat
|
|
Round 1 addressed in Finding 1 (P2) — nil pointer on a null record: correct, and the most valuable finding of the round. claude did not catch this one. Any authenticated admin could have crashed the process with a three-word body. Fixed with a nil check that names the index, plus guards on Finding 2 (P2) — provider-blind pricing: correct, same defect claude raised independently. Catalog now keys on Finding 3 (P2) — I fixed it in the filter rather than by writing Findings 4 and 5 (P3) — missing tests: correct, both added. Nothing was dismissed this round — all 5 P2 and 4 of 5 P3 are fixed with regression tests. The one I did not take is partial batch acceptance (claude P3), dispositioned on its own thread: it changes the ingest contract rather than fixing a defect, and the ambiguity it targeted is gone now that errors name the index. One note on verification, since claude flagged that it could not execute anything: the Postgres-backed tests were re-run against a freshly dropped table. |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| } | ||
| writeJSON(w, http.StatusOK, map[string]interface{}{ | ||
| "records": records, | ||
| "count": len(records), |
There was a problem hiding this comment.
P2 — List is the one surface left where truncation is silent. Round 2 gave Summary a truncated/limit pair on exactly this argument ("a clipped bucket list is indistinguishable from a complete one", store.go:453-457) and gave the handler a 400 for limit > 1000, but the record list still answers {"records": [...], "count": len(records)} with no cap signal at all.
Concretely: GET /api/v1/usage/records?repository=mctlhq/mctl-api&since=... over a busy week matches 4,000 rows. The store returns the newest 200 — the default, whose rule docs/model-usage-ledger.md documents but whose value it never states — count reads 200, and a caller summing records[].calculated_cost reports about 5% of real spend as the whole. Asking for the 1000 ceiling has the same shape one order of magnitude up, and since there is no offset and no cursor, the remaining rows are not reachable by any request at all.
That is the failure mode this endpoint's own 400-on-over-large-limit was added to prevent ("a caller summing a clipped page understates real spend", handlers_usage.go:198-202); clamping the page fixed the smaller half of it.
Cheapest fix is the one already applied one function down: select limit+1 in Store.List, drop the extra row, and return truncated plus the effective limit next to count. Keyset paging would also make the rest reachable rather than merely visible as missing — ORDER BY recorded_at DESC, id ASC is already a unique sort key, so a cursor is nearly free.
There was a problem hiding this comment.
Taken in 56cac52. You are right, and the inconsistency was mine: I made exactly this argument for Summary in round 1 and then left List returning count: len(records) against an undocumented default. List now returns a ListResult with truncated and limit, selected as limit+1 so truncation is detected rather than inferred. TestUsageHandlers_ListReportsTruncation asserts a clipped page says so, and the docs now state that count is the size of this page, not the number of matching records.
| SUM(provider_reported_cost), SUM(calculated_cost), SUM(invoice_reconciled_cost) | ||
| FROM model_usage_records%s | ||
| GROUP BY %s | ||
| ORDER BY COUNT(*) DESC, key ASC |
There was a problem hiding this comment.
P3 — the summary clips by volume on a ledger that is read for money. ORDER BY COUNT(*) DESC with LIMIT limit+1 means a bucket holding three very expensive invocations loses its place to 200 cheap ones, so group_by=target_repo truncated at the default 200 can drop precisely the repo an operator is hunting for. truncated tells them something was cut, but not that the cut was by row count rather than by spend — and on this table the bucket that matters most is the one most likely to be missing.
ORDER BY SUM(calculated_cost) DESC NULLS LAST, COUNT(*) DESC, key ASC would make the retained set the one the question is about while keeping the tie-break deterministic.
Worth documenting the 200-bucket default in the summary section of docs/model-usage-ledger.md too: the records section states its limit rule, the summary section states none, so an operator has no way to know a breakdown is capped until they happen to read truncated: true.
There was a problem hiding this comment.
Fair, and taken in 56cac52 — as a disclosure rather than a reordering. The response now carries truncated_by: "record_count".
I did not reorder buckets by cost. calculated_cost is nullable by design (a model with no rate card, or a provider-reported cost under a future provider), so ordering by it would sort unpriced records to one end and bury exactly the records whose absence of cost an operator most needs to notice. Ordering by volume and naming the axis keeps the failure visible instead of moving it somewhere harder to see. If a cost-ranked breakdown turns out to be what operators actually ask for, that is a query parameter with its own null-handling decision, not a change to the default.
| // the token counts are still the truth, and a cost can be derived | ||
| // later once a card exists. Dropping the record would lose the | ||
| // measurement permanently. | ||
| if err := s.catalog.Calculate(r); err != nil && !errors.Is(err, ErrNoPricing) { |
There was a problem hiding this comment.
P3 — ErrNoPricing is swallowed with no trace anywhere. Not rejecting the record is right, and the comment above makes the case well, but nothing records that a ledger configured with a catalog just stored an unpriced row.
Two realistic paths land here every time rather than occasionally:
- A card naming
provider: "firstParty"— the shape bothdocs/model-usage-ledger.md:38andcatalog_file.go:37show — while the producer omitsprovider. The field isomitempty, andAtfalls back only to a provider-less wildcard card, never to another provider's, so every record from that producer is costless. - A catalog that fails to load at boot:
cmd/api/main.gologs that once, the store runs withcatalog == nilfrom then on, and this branch is never even reached.
In both the ledger answers calculated_cost: null for every row. The absent-is-not-zero design means a reader can at least tell "not measured" from "zero", which is why this is P3 — but nothing points at why, and the discovery moment is the first reconciliation rather than the first ingest.
A slog.Warn with model, provider and recorded_at (no content — the type has no field for it), or an Unpriced []string beside Accepted/Deduped in IngestResult, would surface it where it happens.
There was a problem hiding this comment.
Correct and taken in 56cac52. The two cases you name are the ones that worried me too: a catalog that fails to load at boot and one whose cards all name a provider the producer omits both end with every row costless, and "spend is zero" then reads identically to "spend was never priced". Now a slog.Warn naming canonical model, model key and provider. Still non-fatal — the token counts remain true and a cost can be derived later — which is why it needed to be loud rather than an error.
| // One row beyond the limit, so truncation is detected rather than guessed. | ||
| args = append(args, limit+1) | ||
| q := fmt.Sprintf(` | ||
| SELECT %s AS key, |
There was a problem hiding this comment.
P3 — the model_key fallback landed on the filter but not on the aggregation. Filter.where now matches model_key when canonical_model is empty (store.go:303-314), so ?model=claude-opus-5 finds a record whose producer reported only model_key. group_by=canonical_model still groups that same record under ''.
So the two surfaces disagree about one row: the filter returns it as that model's spend, the per-model breakdown files it under an empty key alongside every other model's unattributed spend. Same class of answer the filter fix was for — visible rather than silent, hence P3, but still not that model's cost.
COALESCE(NULLIF(canonical_model, ''), model_key) in both the projection and the GROUP BY for GroupByModel keeps them consistent. TestModelFilterMatchesRecordsWithoutCanonicalModel covers the filter side; the summary side has no equivalent.
There was a problem hiding this comment.
Correct and taken in 56cac52 — the filter learned the fallback in round 1 and the group-by did not, which is precisely the half-fix worth catching. groupColumns[GroupByModel] is now COALESCE(NULLIF(canonical_model, ), model_key). The map values remain fixed SQL fragments rather than request data, so the closed GroupBy set still keeps a query parameter out of the query. TestSummaryByModelUsesTheModelKeyFallback.
| if err != nil { | ||
| t.Fatalf("List: %v", err) | ||
| } | ||
| if len(huge) != 3 { |
There was a problem hiding this comment.
P3 — this test cannot fail on the regression it names. It seeds 3 rows and asserts 3 come back, which the pre-fix code satisfies just as well: the old branch reset limit > maxQueryLimit to defaultQueryLimit (200), and clamping to 1000 versus resetting to 200 are indistinguishable at any row count below 200.
To actually lock the fix in, either seed more than defaultQueryLimit records and assert more than 200 come back, or factor the clamp shared by List and Summary into a helper (clampLimit(int) int) and table-test it directly — the latter also covers it on a laptop with no TEST_DATABASE_URL, where this test silently skips.
There was a problem hiding this comment.
Correct, and this is the finding I am most glad you caught — the test claimed to lock in a fix it could not detect. Three rows cannot distinguish a clamp to 1000 from a reset to 200, so it passed against the unfixed code.
Rewritten in 56cac52 with DefaultQueryLimit+1 rows, and this time I re-introduced the original bug and watched it fail before keeping the fix:
rows = 200, want 201; an over-large limit did not clamp to the ceiling
It now also asserts that the default page reports truncated. I mutation-checked the other regressions in this PR and simply did not do it here; that was the gap, not the row count.
|
|
||
| // usageMaxLimit mirrors the store's page ceiling so the handler can reject an | ||
| // over-large request instead of letting the store clamp it invisibly. | ||
| const usageMaxLimit = 1000 |
There was a problem hiding this comment.
P3 — usageMaxLimit restates usage.maxQueryLimit (store.go:147) across a package boundary, and the comment above says so. The two can drift: raise the store's ceiling and the handler keeps rejecting at 1000, so every limit between the two values becomes a 400 for a page the store would have served, with an error message quoting a number that is no longer the ceiling.
Exporting the store's constant (usage.MaxQueryLimit) and referring to it here removes the possibility rather than documenting it.
There was a problem hiding this comment.
Taken in 56cac52. DefaultQueryLimit and MaxQueryLimit are exported from the store and the handler reads the ceiling from there, so the two cannot drift. Your failure mode is the right one to name: a store ceiling raised without the handler constant would have valid limits answering 400, which looks like a client bug and is not.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 5 P3.
Round 1 is genuinely cleared. I re-checked each of the four P2s against the code rather than against the commit message: the catalog now keys on (canonical_model, provider) with a provider-less wildcard and rejects two cards effective at the same instant instead of resolving them by an unstable sort; the id is derived unconditionally and a disagreeing supplied id is a 400, covered over HTTP as well as in the unit tests; Summary is capped and reports truncated/limit; and the over-large limit is a 400 in the handler with a clamp behind it. The P3s landed too — length-prefixed id parts with per-shape discriminators, NUMERIC money, 413 for an oversized body, the offending index named in the rejection. Declining partial-batch acceptance with a reason is a fair call.
One thing blocks:
Listtruncation is still silent (handlers_usage.go:232).Summarygottruncated/limiton the argument that a clipped list reads as a complete one; the record list still returnscount: len(records)against an undocumented default of 200 and a 1000 ceiling, with no offset or cursor, so a caller summingrecords[].calculated_costover a busy week understates spend and cannot tell. Same fix as the function below it: selectlimit+1, reporttruncated.
P3s: summary truncates by COUNT(*) rather than by spend on a cost ledger; ErrNoPricing swallowed with no log or counter, which a provider-less record or a catalog that failed to load turns into null cost on every row; group_by=canonical_model still buckets model_key-only records under '' though the filter learned that fallback; TestOverLargeLimitClampsRatherThanShrinks passes against the pre-fix code; usageMaxLimit duplicates the store ceiling across a package boundary.
Caveat unchanged from round 1: go build, go vet and go test were not executable in this environment (the sandbox declined them), so this is a static read and the Postgres-backed tests were not re-run here.
claude: 0 P1, 1 P2, 5 P3 on 5d56091, and it confirmed round 1 cleared by reading the code rather than the commit message. agy: PASS. All six taken. P2 — List truncation was still silent. Round 1 gave Summary truncated/limit on the argument that a clipped result reads as a complete one, and then left the record list returning count: len(records) against an undocumented 200 default. The inconsistency was mine and the argument applies identically: a caller summing records[].calculated_cost over a busy window understates spend with nothing to warn it. List now returns a ListResult carrying truncated and limit, selected as limit+1 so truncation is detected rather than guessed. P3 — the clamp test was worthless. This is the one worth calling out: round 1's TestOverLargeLimitClampsRatherThanShrinks used three records, and three records cannot tell a clamp to 1000 from a reset to 200. It passed against the code it claimed to lock in. Rewritten with DefaultQueryLimit+1 rows, and this time I re-introduced the original bug to watch it fail first: rows = 200, want 201; an over-large limit did not clamp to the ceiling Same discipline as the other regressions in this PR; I simply skipped it here. P3 — ErrNoPricing was swallowed. A catalog that fails to load at boot, or one whose cards all name a provider the producer omits, made every row costless with nothing pointing at why, and "spend is zero" is indistinguishable from "spend was never priced". Now a warning naming model and provider. Still not fatal: the token counts are true and a cost can be derived later. P3 — group_by=canonical_model ignored the fallback the filter learned in round 1, bucketing model_key-only records under the empty string, so a per-model breakdown showed their spend under "" instead of the model that incurred it. The mapped value is a fixed SQL fragment, so the closed GroupBy set still keeps request data out of the query. P3 — summary truncation drops the tail by volume. Buckets order by COUNT(*), so a few expensive invocations can be clipped by many cheap ones. Not reordered by cost — cost is nullable and ordering by it would bury unpriced records — but the response now says truncated_by: "record_count", because an operator needs to know the axis before concluding where the money went. P3 — the handler's limit ceiling duplicated the store's across a package boundary. Exported DefaultQueryLimit/MaxQueryLimit and took the ceiling from the store; drift would have turned valid limits into 400s. Docs updated for all of it, including that count is the size of this page and not the number of matching records. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 3m 56s —— View job Code review complete
Verdict submitted against Round 2 is clearedChecked against the code, not the commit message. Blocking (P2)
Non-blocking (P3)
What holds up wellThe Caveat
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Finding 1 (P2)
Finding 2 (P3)
|
| writeJSON(w, http.StatusOK, map[string]interface{}{ | ||
| "group_by": string(by), | ||
| "buckets": res.Buckets, | ||
| "truncated": res.Truncated, |
There was a problem hiding this comment.
P2 — truncated_by never leaves the process. Round 2 added it to SummaryResult with a deliberate justification (store.go:507-511: "an operator seeing truncation needs to know the tail was dropped by volume, not by spend"), and docs/model-usage-ledger.md:129-133 tells operators the response carries truncated, truncated_by and limit. This handler builds the response map by hand and lists only group_by, buckets, truncated, limit — so res.TruncatedBy, set at store.go:567, is discarded before it reaches the wire.
Concretely: GET /api/v1/usage/summary?group_by=target_repo over a ledger with more than 200 repos answers 200 buckets with truncated: true and limit: 200, and no truncated_by. An operator who read the doc looks for it, does not find it, and is back in exactly the position round 2 identified — knowing something was cut but not that the cut was by row count. The repo with three very expensive invocations that lost its place to 200 cheap ones stays invisible, with no signal that spend was the wrong axis to clip on. omitempty also makes its absence here indistinguishable from the field being present and empty.
Nothing catches it: the summary handler test decodes only group_by, buckets and truncated; TestSummaryIsBoundedAndSaysSo asserts Truncated and Limit at the store layer; and no test in the PR references TruncatedBy or truncated_by at all. Adding "truncated_by": res.TruncatedBy here plus one assertion in TestSummaryIsBoundedAndSaysSo closes both halves.
The general hazard is worth noting too: both read handlers hand-build an untyped map, so a field added to the store result type is silently dropped unless someone remembers to update the literal. Writing res straight out — it already carries the right JSON tags — would make the struct the contract rather than a copy of it.
| // cards all name a provider the producer omits, makes every | ||
| // row costless, and "spend is zero" is indistinguishable from | ||
| // "spend was never priced" without this line. | ||
| slog.Warn("usage record stored without calculated cost: no pricing card matched", |
There was a problem hiding this comment.
P3 — the warning sits inside the per-record loop, so the two failure modes it was added for are exactly the ones that make it unbounded. A producer whose cards all name a provider it omits, or a catalog that failed to load at boot, does not produce an occasional unpriced row — it produces one for every record, so a 500-record batch emits 500 identical warn lines and steady ingestion emits one per invocation indefinitely. The signal that a rate card is missing then arrives as log volume, which is the shape operators mute.
The content is right — model, provider, instant, and no session text. It is the cardinality that wants bounding: one warn per batch carrying the distinct model/provider pairs and a count, or the alternative from the round-2 thread (Unpriced []string beside Accepted/Deduped) so the producer sees it in its own response rather than in someone else’s logs. A counter would also make "how many rows are we storing costless" answerable without grepping.
| } | ||
| r.ID = derived | ||
| if r.RecordedAt.IsZero() { | ||
| r.RecordedAt = time.Now().UTC() |
There was a problem hiding this comment.
P3 — recorded_at is silently stamped with ingest time when the producer omits it, and that field is not just a timestamp on this table: Catalog.Calculate resolves the rate card at r.RecordedAt (pricing.go:154) and every since/until query filters on it. So a record that sat in a producer-side buffer, or that is replayed from an export weeks later, is priced with the card in force at ingest rather than at invocation and lands in the wrong week of every spend report — with ingested_at already recording ingest time separately, so the column that was meant to be the invocation instant now silently holds a copy of it.
The doc does not mention recorded_at at all and its ingest example (docs/model-usage-ledger.md:80) shows a record without one, so the shape a producer is most likely to copy is exactly the one that takes this path. Validate rejects a negative token count and an unknown outcome but not a missing instant.
Given how carefully the rest of this package refuses to invent values it was not given — absent is not zero, a client id is a 400 rather than an overwrite — requiring recorded_at would be the consistent call: ErrInvalidRecord when it is zero, and update the doc example. If defaulting has to stay for the pilot, it is worth saying so in the doc and in this comment, since the consequence is a mispriced row rather than a missing field.
| CREATE INDEX IF NOT EXISTS model_usage_repo_issue_idx ON model_usage_records (target_repo, issue_number); | ||
| CREATE INDEX IF NOT EXISTS model_usage_repo_pr_idx ON model_usage_records (target_repo, pr_number); | ||
| CREATE INDEX IF NOT EXISTS model_usage_agent_idx ON model_usage_records (agent, devloop_stage); | ||
| CREATE INDEX IF NOT EXISTS model_usage_model_idx ON model_usage_records (canonical_model, provider); |
There was a problem hiding this comment.
P3 — model_usage_model_idx ON (canonical_model, provider) no longer covers either query that reads the model dimension, because both learned the model_key fallback in later rounds. The filter is canonical_model = $1 OR (canonical_model = '' AND model_key = $1) (store.go:328-329), and the summary groups on COALESCE(NULLIF(canonical_model, ''), model_key) (store.go:461) — an OR across two columns and an expression, neither of which this index can serve, so both fall back to a sequential scan that grows with the ledger.
Not urgent on an empty table, and the recorded_at index still bounds the windowed queries. But an expression index matching the group-by exactly — ON (COALESCE(NULLIF(canonical_model, ''), model_key), provider) — would cost nothing to add now while the schema is being created for the first time, and is the same "cheap now, migration later" argument that moved money to NUMERIC in round 2.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 3 P3.
Round 2 is cleared, checked against the code rather than the commit message. List now selects limit+1 and returns truncated plus the effective limit, with a handler test that seeds 201 records and asserts the clip is visible; clampLimit is a shared helper and TestOverLargeLimitClampsRatherThanShrinks now seeds more than the default page, so it can actually fail on the regression it names; group_by=canonical_model uses the same model_key fallback the filter does, with a test; usage.MaxQueryLimit is exported and the duplicated handler constant is gone; ErrNoPricing is logged rather than swallowed.
One thing blocks:
truncated_bynever reaches the response (handlers_usage.go:273). Round 2 added it toSummaryResultanddocs/model-usage-ledger.md:129-133tells operators the summary carries it, butGetUsageSummaryhand-builds its response map with onlygroup_by,buckets,truncated,limit, so the field set atstore.go:567is dropped before serialisation. No test referencesTruncatedByortruncated_by, which is why it survived. The remediation for "buckets are clipped by volume, not by spend" therefore reaches no consumer.
P3s: the unpriced-record warning is inside the per-record loop, so the two failure modes it exists for (a provider-less producer, a catalog that failed to load) emit one line per record forever rather than occasionally; recorded_at is silently stamped with ingest time when omitted, and that field also selects the rate card and drives every since/until window, while the doc example omits it; model_usage_model_idx no longer covers either model-dimension query now that both use the model_key fallback.
Caveat unchanged from the previous rounds: go build, go vet and go test were declined by the sandbox here, so this is a static read and the Postgres-backed tests were not re-run.
… hand agy: FAIL:P2 on 56cac52 (it passed 5d56091). Both findings are correct and verified against the code. P2 — `truncated_by` never reached a client. Round 2 added it to SummaryResult and documented it, but GetUsageSummary builds an explicit response map, so the field was set on the struct, promised in docs/model-usage-ledger.md, and dropped on the way to the wire. An operator seeing a truncated summary could not tell the tail was cut by volume rather than by spend, which is the whole reason the field exists. Fixed at the mechanism rather than the field. Both handlers now return types that embed the store result (listUsageResponse, summaryUsageResponse) instead of copying its fields into a map, so the wire shape follows the type and a field added to a result cannot silently fail to reach a client again. The hand-built map was the defect; `truncated_by` was only the first field to fall through it. P3 — agy also named why nothing caught it: List truncation had an HTTP test and Summary did not, so the gap sat exactly where there was no coverage. Added TestUsageHandlers_SummaryReportsTruncationAndItsAxis, asserting over the wire rather than on the struct, since the struct was already right. Verified by restoring the hand-built map and watching it fail first: truncated_by = "", want "record_count" Round 2's P3 about ordering by COUNT(*) stands as dispositioned — the axis is now actually disclosed, which was the condition that disposition relied on. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Round 3 (agy Finding 1 (P2) — writeJSON(w, http.StatusOK, map[string]interface{}{
"group_by": string(by), "buckets": res.Buckets,
"truncated": res.Truncated, "limit": res.Limit, // truncated_by absent
})This one stings a little, because the field exists precisely so an operator can tell a summary was cut by volume rather than by spend — and I argued that point on the round-2 thread while shipping a handler that never sent it. Fixed at the mechanism, not the field. Both handlers now return types that embed the store result ( Finding 2 (P3) — correct, and it explains the miss. Verified by restoring the hand-built map and watching the new test fail first: This is now the third round where a reviewer caught a test that could not detect the bug it named (round 2: the three-row clamp test). Both are the same mistake on my side — asserting at the layer that was already correct instead of the one that was not. Round 2’s P3 about ordering buckets by CI green on |
|
Claude finished @mashkovd's task in 4m 1s —— View job Code review complete
Verdict submitted against Round 3 is clearedChecked against the code, not the commit message. Non-blocking (P3)
Still open from round 3, deferred rather than re-arguedThe unpriced-record What holds up wellThe Caveat
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| *usage.SummaryResult | ||
| } | ||
|
|
||
| type ingestUsageResponse struct { |
There was a problem hiding this comment.
P3 — the one response left hand-built is ingestUsageResponse, and it is the one with an open proposal to grow a field.
This commit's argument is right and worth applying consistently: listUsageResponse and summaryUsageResponse now embed the store result so a field added to the type reaches the wire, but IngestResult (store.go:181-184) still gets projected by hand here — and it already loses information in the projection. Accepted and Deduped arrive as two distinct []string, and ids concatenates them, so a producer that receives {"accepted": 3, "deduped": 2, "ids": [...5 ids...]} cannot tell which two collided. The counts are the same, but the per-id answer is gone.
The drift half is concrete rather than hypothetical: the still-open unpriced-record thread proposes Unpriced []string beside Accepted/Deduped on IngestResult as the bounded alternative to the per-record slog.Warn. Adding it there would set the field, and this literal would silently drop it — exactly the failure truncated_by just had, in the one place the fix was not applied.
IngestResult already carries json:"accepted" / json:"deduped" tags, so embedding it and keeping the counts as the extra fields would preserve both shapes:
type ingestUsageResponse struct {
*usage.IngestResult
AcceptedCount int `json:"accepted_count"`
DedupedCount int `json:"deduped_count"`
}If the counts-only shape is deliberate for payload size, that is a fair call — worth saying so in the comment above the type, since the file now reads as though embedding is the rule.
There was a problem hiding this comment.
Taken in 3b238a3 — you caught the fix being applied everywhere except the one place it mattered most, in the same commit that argued for it. ingestUsageResponse now embeds *usage.IngestResult with explicit accepted_count/deduped_count.
The information loss is the part worth fixing rather than the inconsistency: a producer told {"accepted": 3, "deduped": 2} could not tell which two collided, and on a ledger whose whole claim is "no paid invocation is counted twice", "two of these five were already recorded" is exactly the answer a producer wants to be able to check. Nothing consumes this shape yet — the first producer arrives with .github#50 — so changing it is free today and would not have been later.
| } | ||
|
|
||
| req := adminCtx(httptest.NewRequest("GET", | ||
| "/api/v1/usage/summary?group_by=temporal_workflow_id&repository=mctlhq/mctl-api&limit=2", nil)) |
There was a problem hiding this comment.
P3 — this is the only test in the file whose query is not scoped to its own fixtures. Every other one filters on workflow_id=<prefix>-wf; this one filters on repository=mctlhq/mctl-api, which usageRecordBody hardcodes for every record in both this file and store_test.go.
It happens to be safe today — each test deletes its own rows in t.Cleanup and the job runs go test -p 1 ./..., so no foreign rows are in the table when this runs. But the assertion it protects is a lower-bound one (truncated must be true because 3 buckets exceed limit=2), and foreign rows can only push bucket count up. So if this test's own ingest ever stopped producing three distinct workflow ids, leftover rows from a crashed run — t.Cleanup ignores its DELETE error — would keep truncated: true and the test would still pass while measuring nothing it claims to.
The prefix is available and unused here; scoping through a field the group-by does not consume keeps it honest:
rec["work_item_id"] = prefix
...
"/api/v1/usage/summary?group_by=temporal_workflow_id&work_item_id="+prefix+"&limit=2"Same cost, and it reads consistently with the rest of the file.
There was a problem hiding this comment.
Fixed in 3b238a3. You are right that it is safe today and right that it is the wrong shape anyway: the assertion is a lower bound (truncated true because 3 buckets > limit=2), and foreign rows can only push the count up — so the failure mode is this test quietly passing after its own ingest stopped producing three distinct workflow ids. Now scoped to a per-test work_item_id like every other test in the file.
| for i := 0; i < rt.NumField(); i++ { | ||
| name := strings.ToLower(rt.Field(i).Name) | ||
| for _, bad := range forbidden { | ||
| if name == bad { |
There was a problem hiding this comment.
P3 — the privacy invariant is the strongest idea in this PR and the assertion implementing it is the weakest form of itself. name == bad is exact equality on the lowercased field name, so the forbidden list catches a field called Prompt and misses PromptText, UserMessage, MessageText, ResponseBody, RawContent — the names a producer is actually more likely to reach for than the bare noun.
That matters because of what the test is for. The comment above it makes the right argument: this proves there is no field to scrub, and it is the guarantee that survives "a future producer written by someone who never read the ADR". But the person who adds PromptSummary to Record is precisely that person, and the test greets them green.
strings.Contains(name, bad) is strictly stronger and passes against the current type unchanged — I checked every field: none of prompt, completion, message, content, text, body, response appears as a substring of any of the 35 field names (recordedat, terminalreason, stopreason and the rest are all clear).
if strings.Contains(name, bad) {Worth applying the same widening to the JSON-shape guard in handlers_usage_test.go:277, which checks for "text" and "content" with the quotes attached and so would miss "output_text" — the one name the forbidden list here already anticipates.
There was a problem hiding this comment.
Taken in 3b238a3, and thank you for pushing on this one — it is the finding I would least have wanted to ship.
You are right that the assertion was the weakest form of the idea it protects. name == bad catches Prompt and waves through PromptText, UserMessage, MessageText, ResponseBody — and the whole point of the test is surviving someone who never read ADR-012, who is exactly the person reaching for those names rather than the bare noun. Now strings.Contains.
Verified rather than assumed, in both directions: it passes against all current field names unchanged, and adding the exact field you named fails it —
Record has field "PromptText", whose name contains "prompt": it can carry model content,
and ADR-012 invariant 8 forbids the field existing at all
The three P3s you listed as deferred from round 3 I have left deferred and named in the commit message: the per-record unpriced warning, recorded_at defaulting to ingest time while also selecting the rate card, and model_usage_model_idx no longer covering either model-dimension query after the model_key fallback. The middle one is the one I would look at first if this ledger starts ingesting late — a record that arrives an hour after the fact gets priced at the ingest instant, which is almost always the same rate card but not guaranteed to be.
There was a problem hiding this comment.
No P1/P2 findings (3 P3). Good to merge.
Round 3 is cleared, checked against the code rather than the commit message. truncated_by now reaches the wire: GetUsageSummary and ListUsageRecords serialise *usage.SummaryResult / *usage.ListResult by embedding instead of hand-building a map, so the wire shape follows the type and the whole class of drift goes away rather than the one instance. TestUsageHandlers_SummaryReportsTruncationAndItsAxis asserts truncated_by: "record_count" over HTTP, which is the layer where the bug lived — the struct was already correct. group_by and count stay at the outer level and collide with nothing in the embedded types, so the documented response shapes are unchanged apart from the field that was missing.
Everything from rounds 1 and 2 still holds: unconditional id derivation with a disagreeing supplied id as a 400, provider-aware pricing with an explicit wildcard and same-instant cards rejected at load, nil-record guard, clampLimit shared by List and Summary, truncated/limit on both read paths, model_key fallback in both the filter and the group-by, NUMERIC money, the calculated_cost_requires_version CHECK, nil store to 503, and TestRecordHasNoFreeTextContentField asserting the absence of a field rather than testing a scrubber. Every predicate is parameterised and the group-by column comes from a closed map — no injection path. No goroutines, no shared mutable state: the catalog is read-only after build and records are per-request.
P3s, none blocking:
handlers_usage.go:97—ingestUsageResponseis the one response still projected by hand, andIngestResultis the type with an open proposal (Unpriced []string) to grow a field. It also flattensAccepted/Dedupedinto oneidslist, so a producer cannot tell which ids collided.handlers_usage_test.go:481— the new summary test filters onrepository=mctlhq/mctl-api, shared by every fixture in both test files, rather than on its own prefix. Safe today given per-test cleanup and-p 1, but its assertion is a lower bound, so foreign rows could only mask a regression.usage_test.go:39— the privacy assertion uses exact field-name equality, soPromptTextorUserMessagewould pass.strings.Containsis strictly stronger and passes against the current type unchanged.
Still open and consciously deferred from round 3, listed for the record rather than re-argued: the unpriced-record slog.Warn is per record rather than per batch; recorded_at is defaulted to ingest time when omitted, and that field also selects the rate card; model_usage_model_idx no longer covers either model-dimension query now that both use the model_key fallback.
Caveat unchanged across all four rounds: go build, go vet and go test were declined by the sandbox here, so this is a static read and the Postgres-backed tests were not re-run.
… fix claude approved e681181 (0 P1, 0 P2, 3 P3) and agy passed, so the gate was already met. Taking all three anyway: two are one-liners and the third is about the invariant this whole ledger rests on. The privacy guard matched field names by exact equality, so the forbidden list caught `Prompt` and waved through `PromptText`, `UserMessage`, `MessageText`, `ResponseBody` — the names someone is actually more likely to reach for. The test exists precisely to survive a producer written by someone who never read ADR-012, and it was the weakest form of itself. Now substring matching, which passes against all 35 current field names unchanged. Verified by adding the exact field claude named and watching it fail: Record has field "PromptText", whose name contains "prompt" `ingestUsageResponse` was the one response still projected by hand, in the same commit that argued hand-built maps are the defect — and it lost information doing it: Accepted and Deduped arrive as two lists and `ids` concatenated them, so a producer told "3 accepted, 2 deduped" could not tell WHICH two collided. Now embeds IngestResult with explicit counts. Nothing consumes this shape yet — the first producer arrives with .github#50 — so changing it is free today and would not be later. The summary truncation test filtered on the repository every fixture hardcodes rather than its own prefix. Its assertion is a lower bound, so foreign rows could only mask a regression in its own ingest. Scoped to a per-test work_item_id like every other test in the file. Deferred, as claude listed them and I agree they are not this PR's scope: the unpriced warning is per record rather than per batch; recorded_at defaults to ingest time while also selecting the rate card; model_usage_model_idx no longer covers either model-dimension query now that both use the model_key fallback. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 4m 2s —— View job Code review complete
Verdict submitted against Round 3 is clearedChecked against the code, not the commit message.
Non-blocking (P3)
Still open from earlier rounds, deferred rather than re-arguedThe unpriced-record What holds up wellThe Caveat
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| t.Fatalf("Ingest: %v", err) | ||
| } | ||
|
|
||
| res, err := s.Summary(ctx, Filter{TargetRepo: "mctlhq/mctl-api", Limit: 2}, GroupByWorkflow) |
There was a problem hiding this comment.
P3 — this commit fixed the handler-side sibling of this test and left this one on the old pattern.
TestUsageHandlers_SummaryReportsTruncationAndItsAxis moved off repository=mctlhq/mctl-api to a per-test work_item_id this round, with a comment explaining exactly why: the assertion is a lower bound, so foreign rows can only mask a regression in the test's own ingest. This test makes the same assertion (res.Truncated must be true because 5 workflow buckets exceed Limit: 2) through the same shared filter — TargetRepo: "mctlhq/mctl-api" is hardcoded by testRecord here and by usageRecordBody in handlers_usage_test.go, so it matches every fixture in both files.
Concretely: if this test's own ingest ever stopped producing five distinct temporal_workflow_id values, rows left behind by a crashed run — t.Cleanup ignores its DELETE error — would keep the bucket count above 2 and both assertions would still pass while measuring nothing they claim to. Safe today under per-test cleanup and go test -p 1, but that is the property the round-4 fix declined to rely on next door.
prefix is already in scope and each record already gets %s-wf-%d, so scoping through a dimension the group-by does not consume is the same one-line change that was applied to the handler test:
r.WorkItemID = prefix + "-bounded"
...
s.Summary(ctx, Filter{WorkItemID: prefix + "-bounded", Limit: 2}, GroupByWorkflow)
| // UserMessage or ResponseBody long before the bare noun — exact equality | ||
| // would wave every one of them through. Checked against all current field | ||
| // names: none contains any of these, so the stronger form costs nothing. | ||
| forbidden := []string{"prompt", "completion", "message", "content", "text", "body", "response"} |
There was a problem hiding this comment.
P3 — the widening is the right call, and "text" has one foreseeable false positive worth pre-empting: "context" contains it.
ContextWindow, ContextWindowTokens or MaxContextTokens are plausible additions to a model usage record — context window size is a usage dimension, not content — and each would fail this test with a message asserting the opposite of the truth: Record has field "ContextWindowTokens", whose name contains "text": it can carry model content. Whoever hits that has to read the guard to work out it is a substring artefact, and the tempting fix is to delete "text" from the list, which loses input_text/output_text — the two names the round-3 thread named specifically.
Failing safe is the right default, so this is a nit rather than a defect. Anchoring the two that are actually compound would keep both properties:
forbidden := []string{"prompt", "completion", "message", "content", "body", "response", "_text", "text_"}with name taken from the JSON tag rather than the Go field name, so Aux string \x60json:"prompt"\x60 is caught too — the wire shape is what leaks.
Same widening is worth applying to the companion guard at handlers_usage_test.go:277, which was left on "\"text\"" / "\"content\"" this round: quoted-key matching misses "output_text", the one name this list already anticipates.
There was a problem hiding this comment.
No P1/P2 findings (2 P3). Good to merge.
Round 3 is cleared, checked against the code rather than the commit message.
ingestUsageResponsenow embeds*usage.IngestResultand keeps the counts asaccepted_count/deduped_count, soacceptedanddedupedarrive as the two distinct id lists the store actually computed — a producer can now tell which ids collided, not just how many. The doc example matches the new shape, and no other consumer of the oldidsfield exists in the tree. With that, all three response types follow their struct rather than a hand-built literal, which closes thetruncated_byclass of drift everywhere instead of at one site.- The summary truncation test is scoped through a per-test
work_item_idrather than the repository every fixture hardcodes. - The privacy guard is
strings.Contains, which is the form that survivesPromptText/UserMessage/ResponseBody. I checked it against all 35 current field names — none containsprompt,completion,message,content,text,bodyorresponse, so the stronger assertion passes unchanged.
P3s: TestSummaryIsBoundedAndSaysSo (store_test.go:378) is still on the hardcoded target_repo filter — the exact pattern its handler-side sibling moved off in this commit, and its assertion is lower-bound in the same way; "text" as a bare substring will false-positive on a future ContextWindow* field with a message asserting the opposite of the truth, and the companion JSON-shape guard at handlers_usage_test.go:277 was left on quoted-key matching, which misses "output_text".
Still open from earlier rounds, deferred rather than re-argued: the unpriced-record slog.Warn is per record rather than per batch; recorded_at is defaulted to ingest time when omitted, and that field also selects the rate card and drives every since/until window; model_usage_model_idx covers neither model-dimension query now that both use the model_key fallback; the summary section of the doc still does not state the 200-bucket default. All P3, none blocking.
What holds up: the (run, model) grain; absent-vs-zero carried from *int64 through nullable columns to nullable summary buckets; cost derived once at ingest and stored with its rate-card version; the calculated_cost_requires_version CHECK; provider-aware pricing with an explicit wildcard and same-instant cards rejected at load; unconditional id derivation with a disagreeing supplied id as a 400; clampLimit shared by both read paths; nil store to 503. Every predicate is parameterised and the group-by column comes from a closed map — no injection path. No goroutines and no shared mutable state: the catalog is read-only after build, records are per-request.
Caveat unchanged across all four rounds: go build, go vet and go test were declined by the sandbox here, so this is a static read and the Postgres-backed tests were not re-run.
Implements #266 against the ADR-012 contract (
mctlhq/mctl-agents,docs/adr/012-model-usage-cost-attribution-contract.md).A financial read model at the grain
(run, model), so "what did this DevLoop cost" stays answerable after trace data ages out. Store + versioned pricing catalog + idempotent ingestion + query surface.Three decisions worth reviewing
Idempotency is the primary key.
id = sha256(session_id | result_uuid | model_key),ON CONFLICT DO NOTHING. A Temporal retry, a workflow replay, a restart mid-write and an export reprocessing all land on the same row.retry_attemptis deliberately not in the key. A retry that re-runs the model is a different invocation with a newsession_idand a real new charge — deduping it would lose money that was actually spent. Covered byTestRetryThatRerunsTheModelIsNotDeduped.Absent is not zero. Every token column is nullable and every Go counter is
*int64. A provider that does not report cache tokens and a run that read no cache are different facts;SUM()ignores the first and counts the second.NOT NULL DEFAULT 0would have silently turned "not measured" into "nothing spent". The summary buckets stay nullable for the same reason.Rates live in a file, not the binary. A published price is a fact about the world with an effective date; a wrong constant produces plausible-looking money. Cost is derived once at ingest and stored with the rate-card version, so a later card cannot reach back and rewrite history. With no catalog configured the ledger still records every token count and derives no cost — the honest behaviour when rates are unknown.
Privacy
The record type has no prompt, completion or message field. Enforced by construction, not by a redaction pass —
TestRecordHasNoFreeTextContentFieldasserts the absence of the field rather than testing a scrubber, so a future producer that never read the ADR still cannot leak conversation text.A nil store is 503, never an empty 200
An operator reading zero spend from an unconfigured ledger would conclude the opposite of the truth. All three endpoints are admin-only: aggregate spend per repo and per agent is commercially sensitive in a way a workflow status is not.
Testing
ADR-012's nine invariants, each at the layer it actually lives in. Idempotency, the absent-vs-zero round trip, price-change immutability and the
CHECKconstraint run against real Postgres — a mocked pool can only show the code handles a conflict, never that the database produces one. (These skip locally withoutTEST_DATABASE_URL; they ran green here againstpostgres:16.)TestUsageRoutesAreRegisteredexists because a handler with no route stayed invisible once already in the lifecycle work.Scope note on acceptance criterion 1
"Ingested idempotently from the DevLoop pilot" cannot be satisfied inside this issue: the pilot instrumentation is
mctlhq/.github#50, and the canonical roadmap has #50 depending on this item, not the reverse (the issue prose states it backwards — noted on #266). Read here as the ingestion path is proven idempotent against pilot-shaped records; the live producer arrives with #50.Closes #266
🤖 Generated with Claude Code