[boazdavid]

This PR stacks on the draft re-land #923. It must not merge unless the repo owner says yes to the re-land. Savings figures change.

Summary

• This PR combines three fix branches on top of Feat: Re-land the accountant pricing of proxy savings (#768) with the #893 fixes #923. Each branch fixes issues found in the review of feat(accountant): Price proxy savings with the accountant library #768 and Fix: Wrong savings in the accountant integration (review of #768) #893.
• fix(accountant): Fix the filter, keep-alive and TTL gates (#911, #912, #919) #924: filter, keep-alive and TTL gates (Accountant integration (#893 B2): the double-count fix also drops measured tool-filter requests and tokens, and items no longer sum to the total #911, Accountant integration (#893 B4): the keep-alive rescue gate needs a component row, so a keep-alive-only setup still loses the rescue #912, Accountant integration: keepalive_saved_usd is credited by the library and re-gated by a separate SQL model; use one #919).
• fix(accountant): Stop three fallbacks that lose the saving (#906, #907, #908) #925: three fallbacks that lose the saving (Accountant integration: the default tail breakpoint makes every request fall back (check_sum) and book no saving #906, Accountant integration: the plausibility gate drops the whole result when removed tool declarations exceed message tokens #907, Accountant integration: tool defer fails check_sum and conservation on every request #908).
• fix(accountant): Fix rejected-request token booking and no-history estimates (#914, #915) #926: rejected requests and first request on a warm cache (Accountant integration (#893 Major 4): saved tokens are booked on a request the provider rejected #914, Accountant integration (#893 Major 5): the first request on an unseen warm cache books a baseline about 2x too high #915).
• The combined branch is the one to review. It has two conflict resolutions that no single PR has.

What the merge changed

• dash/accountant_switch_test.go: both branches added a test at the same place. Both tests stay.
• dash/schema.go, store.go, query.go, event.go, accountant_price.go: gates adds acc_priced, recording adds estimated_saved_usd. Both columns stay, in this order, everywhere.
• Git merged the INSERT placeholder row without a conflict. It had 91 placeholders for 92 columns, so every insert failed at run time and 48 proxy tests failed. Fixed in the last merge commit. A reviewer should check the column, placeholder and argument counts again.

How it was tested

• go test ./dash/... passes.
• go test ./proxy/... passes except TestALateFirstByteOnTheRealTransportNeverReachesTheClient. That test also fails on the re-land base 189c169d. It expects an answer within 5 ms and this machine is loaded.
• apply, keepalive, components, tooldefer pass.

Known limits

• Accountant integration: tool defer fails check_sum and conservation on every request #908 stops the fallbacks but does not credit the deferral. The Overview shows about -$0.0006 against a true +$0.0185.
• Accountant integration: the default tail breakpoint makes every request fall back (check_sum) and book no saving #906 reports $0.0817 against a true $0.0736 (+11%) with the default tail breakpoint. The baseline is 0.13959 against a control of 0.13155. Cause not found.
• Accountant integration (#893 Major 5): the first request on an unseen warm cache books a baseline about 2x too high #915 holds the estimated saving out of every total. The first request on an unseen warm cache counts as 0 where the true saving is about 0.0216.

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

[boazdavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 1 (glm, user of @boazdavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.241	offload-review skill, @boazdavid's session	fresh	1 of 2	1 of 5	850893277db4	blocking

Verdict: blocking

Findings

1. dash/toolsuggest.go:199 — the filter saving of an estimated request appears in no dollar total

   • What fails: a request the accountant priced as an estimate (the first request of every thread) books no saving anywhere (#915: BaselineCostUSD == CostUSD, every component SavedUSD 0), but its toolfilter row still carries priced_by = 'accountant' (dash/accountant_price.go:331 sets it before the estimated branch zeroes the dollars). declFilterInBaseline then matches that row, so accumulateDeclFilterRow returns at dash/toolsuggest.go:253 and the measured credit adds no dollars. The comment and docs/dashboard.md:125 justify this with "the net saving has them" — for an estimated row the net saving has nothing.
   • Proof: trace an estimated request with filtered_decl_tokens > 0. Accountant side: dash/accountant_price.go:335-349 holds fig.DeltaUSD in EstimatedSavedUSD, outside every total. Measured side: dash/toolsuggest.go:199-200 selects the row as in-baseline, dash/toolsuggest.go:253 skips the USD. Result: the Realized block and /api/adapt/tools count the request and its reads but show $0 for it. The same drop applies to the carriedCost path (dash/adapttools.go:473-479). Every thread's first request is estimated, so this is systematic, not an edge. The new test TestFilterCreditKeepsFactsAndSkipsDollarsForAccountantPricedRows covers only non-estimated rows.
   • Fix: make declFilterInBaseline false for a row whose request is estimated (join r.price_estimated), so the measured credit keeps the measured dollars for exactly those rows the books hold out. Or hold the filter part out of EstimatedSavedUSD and say so in the docs. Pick one owner for the figure; today it has none.

2. dash/ui/app.js:1828 — the Overview says estimated requests are "not counted above", but rows written before this change are counted

   • What fails: price_estimated changed meaning in this PR. Before, an estimated row booked its figure (BaselineCostUSD = CostUSD + DeltaUSD, base commit 189c169, dash/accountant_price.go). After, an estimated row books nothing and the UI text reads "N of M requests are not counted above ... Estimated saving, not in any total: $X". No migration unbooks the old rows, so a database that has both shows a false claim: the old rows are in NetSavedUSD and TotalSavedUSD, and the card says they are not. The request detail page (dash/ui/app.js:3710-3714) shows the same contradiction: "Not counted ... no saving is booked" beside a non-zero net saving that is in the totals, and "Estimated saving (not counted): ≈ $0.00" for a row whose estimate was booked.
   • Proof: git show 189c169:dash/accountant_price.go — the base accOverride ends with e.BaselineCostUSD = e.CostUSD + fig.DeltaUSD; e.PriceEstimated = estimated. Overview.EstimatedRequests is SUM(r.price_estimated) (dash/overview.go:681), so those rows are counted, and NetSavedUSD sums their booked baseline_cost_usd. The docs answer this with "A row written before the column existed reads as exact" (docs/dashboard.md:161) — false: price_estimated is a pre-existing column and those rows read as estimated. Exposure is bounded: the accountant line is not in any tag (newest tag v0.3.4, 2026-10-01, predates #768), so only databases written by the unreleased branch are affected. That is still the class of row this repo elsewhere handles on purpose (acc_priced defaults 0, priced_by empty, "rows written before the switch keep their legacy figures").
   • Fix: distinguish the two meanings. Count only rows where the hold-out applied (for example price_estimated = 1 AND estimated_saved_usd > 0, or a new marker), or migrate the old rows. Then fix the docs sentence at docs/dashboard.md:161.

3. dash/event.go:809 — a rejected request that still reported usage books dollars beside zeroed tokens

   • What fails: DropUnbilledSavings zeroes the token saving of every non-2xx row, on the ground that "the provider billed none of it, so tokens and dollars agree" (docs/dashboard.md:166-170). But finish runs the accountant and PriceMode whenever usageOK is true, with no status test (proxy/dashcapture.go:733-745), and the response path parses usage from the body whatever the status (proxy/proxy.go:2308). A gateway that returns a 4xx/429 body with a usage object then produces a row with tokens_after == tokens_before, zero SavedUnique, but a non-zero BaselineCostUSD - CostUSD and component SavedUSD from the accountant figure. Tokens and dollars disagree — the exact defect #914 set out to remove.
   • Proof: DropUnbilledSavings (dash/event.go:809-818) tests only the status; c.accReq != nil && usageOK && !c.accDone (proxy/dashcapture.go:733) tests none; responseUsageWhy at proxy/proxy.go:2308 runs on respBody of any status. The new test TestFinishBooksNoSavedTokensOnARejectedRequest covers only rejected rows with usageOK false, so the gap is untested.
   • Fix: skip the accountant compute (or force the fallback) when e.Status is not 0 and not 2xx, or extend DropUnbilledSavings to zero the dollar figures too.

Non-blocking

• dash/adapttools.go:473-479 — the inBaseline branch never increments out.pricedRequests, so st.Priced = scan.pricedRequests > 0 (dash/toolapi.go:675) reports "not priced" when every in-scope filtered row is accountant-priced. A deliberate $0 then reads as "no measurement". Consider counting the row as priced with a known rate.
• dash/accountant_price.go:335-350 — the estimated hold-out also zeroes the headttl row (premium and rescue). The comment names only the keep-alive rescue and the split as staying. The first request of a thread is where a 1h head is written, so the premium cost shows as $0 exactly there. If this is intended, say so in the comment and docs; if not, keep the ttl legs booked as their own column.
• dash/event.go:866 — Price resets AccountantPriced but not EstimatedSavedUSD. No production caller mixes PriceMode and Price today (only costOnlyCopy in tests), so this is latent. Reset both for symmetry.
• proxy/accountant_envelope.go:291 — messageOps returns nil for any change it cannot explain, and the sum check then sends the event to the no-saving fallback. Fail-open, documented. Acceptable.

What I checked

• The full diff 189c169..8508932 (33 files) and the code around every hunk.
• The tail-breakpoint operation: markTail runs in a defer in apply.BodyOpts (apply/apply.go:409-422) after wireSizes chained the pipeline ops, so tailOp's SizeBefore meets the chain's end on the same message. The OpsDropped == 0 guard, the stable ChangeID, and the append-after-pipeline ordering are consistent with the library's sum check.
• messageOps and the tooldefer wiring: the full-slice append in proxy/proxy.go:1596 avoids aliasing; the insertion and append forms match the wireSizes splice conventions; ops for messages, tools and system cover the client-to-forwarded diff in order.
• PriceMode/accOverride: the estimated hold-out, AccountantPriced, the plausibility bound with toolSectionOperators (operator strings match envelopeOwner's output), and the storage round-trip (insertBatch columns, scanRequest, additiveColumns migration, golden files).
• The keep-alive gate: acc_priced = 1 sits after the col > 0 guard and before the probes, so the credited-row short-circuit order holds; every kaSaved/kaSavedSince caller aliases requests; manual pings book manual_saved_usd, not keepalive_saved_usd, so the gate does not touch them.
• accBilledTTL: the CacheWrite1h < CacheWrite test matches Usage's documented semantics (usage.go:31) and keepalive.ResolveTier's rule; the breakpoint copy protects the slice the sibling store keeps; it runs before computeSafe on the only compute path (rounds <= 1).
• DropUnbilledSavings ordering in finish (status is set in the event literal before it runs), and the status-0 and 2xx exclusions.
• DeclFilterSavings, DeclFilterSavingsByTenant, carriedCost: the inBaseline SELECT column, the skipUSD return after the tier-token facts, and byID initialization for every key.
• Release containment: the accountant line (#768 and its fixes) is in no tag; newest tag v0.3.4 predates it. This bounds finding 2 to databases written by the unreleased branch.
• Commit list (9 fix commits, 3 merges) read in full.

[boazdavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 2 (glm, user of @boazdavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.241	offload-review skill, @boazdavid's session	fresh	2 of 2	1 of 5	850893277db4	blocking

Verdict: blocking

Findings

1. dash/event.go:809 — a rejected request that still parsed usage books dollar savings, so tokens and dollars disagree.
   • What fails: DropUnbilledSavings zeroes the token side only (TokensAfter, SavedUnique, per-row SavedGross). It does not stop the accountant path. In finish (proxy/dashcapture.go:733-750) the order is: DropUnbilledSavings, then computeSafe (it runs, because it is gated on usageOK, not on status), then PriceMode. If a provider or gateway returns a non-2xx status whose body still parses usage (responseUsageWhy at proxy/proxy.go:2307 parses the body with no status check, and usageOK is sticky), then: tokens book 0, but accOverride applies, sets each component SavedUSD from the library figure and BaselineCostUSD = CostUSD + DeltaUSD. The row then shows $saved_usd > 0 beside saved_gross = 0. This is the exact disagreement the PR says cannot happen: docs/dashboard.md says "The provider billed none of it, so tokens and dollars agree", and the code comment at dash/event.go:804 says "cost and baseline are 0", which is false when usage parsed. The ping path already guards this (proxy/accountant_ping.go:106 refuses a non-2xx ping); the request path does not.
   • Proof: trace above; no line between DropUnbilledSavings (proxy/dashcapture.go:529) and PriceMode (proxy/dashcapture.go:750) reads c.status. accOverride (dash/accountant_price.go:250) reads no status either, and DropUnbilledSavings setting Acted = false removes the FallbackActedWithoutFigure gate, which makes the override easier, not harder.
   • Fix: gate the accountant compute (or the override) on a 2xx status, as PingSavings does. Or zero the dollar side in DropUnbilledSavings too. Reachability is low (an error body that carries usage), so the team may accept it — but then the docs and the code comment must say so.

Non-blocking

• dash/accountant_price.go:335 — the estimate hold is all-or-nothing. A first request whose net figure is a cost (the tooldefer names list, an expand or adjudicate injection) now books 0 instead of that cost, so net saved is overstated by it. The docs and the issue speak only of savings that are too high. The held figure can be negative and renders as "Estimated saving (not counted): ≈ $-x". The cost side is measured, not estimate-dependent. Small figures; deliberate and test-covered (proxy/accountant_inject_test.go). Name it in the docs, or hold out only the removal half.
• proxy/accountant_envelope.go:291 — the comment on messageOps says "Any other change returns nil". The equal-length case returns one op per changed message, so a two-message change is explained, not refused. That is safe (it explains more), but the comment overstates the refusal. Fix the comment.
• dash/event.go:866 — Price resets PriceEstimated and AccountantPriced but not EstimatedSavedUSD. No production path calls Price after PriceMode today (pings and the import path build fresh events), but a future caller could store a held figure on a row with price_estimated = 0. Reset it there too, for symmetry with PriceMode.

What I checked

• The full diff 189c169d..HEAD (9 fix commits, 3 merges), every changed file, and the code around each change: apply/apply.go, apply/ops.go, proxy/proxy.go, proxy/accountant_wire.go, proxy/accountant_envelope.go, proxy/dashcapture.go, dash/accountant_price.go, dash/event.go, dash/keepalive.go, dash/adapttools.go, dash/toolsuggest.go, dash/overview.go, dash/overview_lean.go, dash/query.go, dash/schema.go, dash/store.go, dash/ui/app.js, both docs files, all new and changed tests.
• The accountant library at the pinned version (v0.1.1-0.20261104203351-5fa19864db4c, read from the module cache): how MessageSplice positions are undone (compute.go undo/apply), when penalty_unknown is set (PhaseUnknown, whole result), what isChange keeps, and the ended-change rules. The tail op and the tooldefer op chain correctly with pipeline ops, and the ChangeID schemes (hash of before-bytes; position for an insert) match the library's replace-an-active-change semantics.
• The insert SQL: 92 columns, 92 placeholders, 92 arguments; the new columns sit at the same place in all three lists.
• tooldefer/rewrite.go placeList: it produces exactly the two shapes messageOps explains (append to a system message at index 1, or insert one at index 1, including the one-message append). Anything else makes messageOps return nil, the sum check fails, and the event falls back — the safe direction.
• accBilledTTL against entryFacts and the library's TTL use: it only lowers 1h to 5m, only with an exact lifetime, only when a write was billed below the 1h tier, and it copies the breakpoints slice before mutating. SharedWarm is documented as judged with the asked tier.
• The keep-alive gate change: every kaSaved/kaSavedSince caller aliases requests as r., the acc_priced column is in both the CREATE TABLE and additiveColumns, and the credited-row guard stays first. When the override applies, keepalive_saved_usd is always the library's figure (accOverride sets it), so trusting acc_priced = 1 is sound. Ping events use Price, never PriceMode, so they never carry the marker.
• The plausibility-bound exclusion (toolSectionOperators): context-guru.expand and context-guru.adjudicate operators are emitted only by the envelope reporter for tools-section declarations; pipeline message components use their own names, so no message operation escapes the bound.
• #911 read-side changes: DeclFilterSavings and the carriedCost filter scan skip dollars for accountant-priced rows and keep requests, tokens, dates and Since (set before the skip). The filter scan's pricedRequests is not consumed by any reader that could misread a $0.
• Fail-open: no change touches the forwarded body; all changes are reporting and pricing. A wrong or missing operation falls back to no saving, never to a wrong forward.
• Test coverage: each of the six issues (#906, #907, #908, #911, #912/#919, #914, #915) has a unit test and, where it needs the wire, an end-to-end test. I did not run them (live tests not approved); I read them and their helpers.

[boazdavid]

Round 1 fixes are pushed to integ/batch1: dcd1b31, 359a770, 0a397f9.

Review 1, finding 1 (estimated filter saving in no total): fixed in 0a397f9. Owner: the measured filter credit. declFilterInBaseline is now false when estimate_held = 1, so the credit keeps its dollars for exactly the rows the books hold out, and EstimatedSavedUSD stays out of every total. Reason: the credit is measured from the rate card and the tier and does not depend on the unknown cache state. Test: TestFilterCreditKeepsDollarsForAHeldEstimatedRequest (Realized, /api/adapt/tools, Overview baseline delta 0). The non-blocking pricedRequests point is fixed: a baseline-priced filtered row now counts as priced. docs/dashboard.md updated.

Review 1, finding 2 (price_estimated changed meaning): fixed in 0a397f9. New column requests.estimate_held (default 0), set only when the hold-out applied. The Overview count (full and lean), the Overview text and the request detail read it. A row written before #915 has estimate_held = 0, keeps its booked figure and is not "not counted". The docs sentence is replaced. Test: TestOverviewCountsEstimatedRequests now holds an old-meaning row and new-meaning rows.

Review 1 finding 3 and Review 2 finding 1 (rejected request with usage books dollars): fixed in dcd1b31. The accountant compute is skipped for a status that is not 0 and not 2xx (acc_fallback reason rejected_status). Test TestFinishBooksNoDollarsOnARejectedRequestThatReportedUsage sends a 429 and a 400 with a usage object: tokens and dollars are both 0. A 200 control books a saving, and the test fails without the fix.

Non-blocking: the hold-out covers the head-TTL legs; the comment and the docs now say so, and say that the keep-alive rescue and split stay booked. A negative held figure renders as "a cost of $x" and the docs name it. Price now resets EstimatedSavedUSD and EstimateHeld. The messageOps comment is corrected in 359a770.

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

[boazdavid]

Round 1 replies. All findings are fixed in 0a397f9d and later. Details are in the earlier summary comment. Head is now 0206b444: it also merges #928 (#916, #917, #918, #920).

Reviewer 1, finding 1 (filter saving of an estimated request in no total). Fixed in 0a397f9d. The measured filter credit owns the figure. declFilterInBaseline is now (r.estimate_held = 0 AND EXISTS ...). A held request adds measured dollars to Realized and /api/adapt/tools. The totals still hold it out, so nothing is counted twice. Test: TestFilterCreditKeepsDollarsForAHeldEstimatedRequest. The non-blocking pricedRequests item is fixed in the same commit.

Reviewer 1, finding 2 (price_estimated changed meaning). Fixed in 0a397f9d. New column requests.estimate_held, default 0, set only when the hold-out applied. The Overview and the request detail read it. Old rows keep their legacy meaning. The docs sentence "reads as exact" is replaced. Test: TestOverviewCountsEstimatedRequests has an old-meaning row beside new-meaning rows. The INSERT has 93 columns and 93 placeholders, counted by script after the merge with #928.

Reviewers 1 and 2, rejected request with usage. Fixed in dcd1b31f. The accountant compute is skipped when the status is not 0 and not 2xx. The row falls back with reason rejected_status and books tokens 0, baseline equal to cost, and every component saving 0. Test: TestFinishBooksNoDollarsOnARejectedRequestThatReportedUsage (a 429 and a 400 with a usage object; a 200 control books a saving). It fails when the fix is off.

Non-blocking, all done.

• The TTL legs are held out with the rest of the estimate. The code comment and the docs say so. The keep-alive rescue and the split saving stay booked.
• A negative held figure is a cost. The UI says "a cost of $x" and the docs say so.
• Price and PriceMode reset EstimatedSavedUSD and EstimateHeld.
• The messageOps comment is fixed (359a770d).

Known limits, stated plainly.

• The first request of a thread books 0 where the true saving is about 0.0216 (context-guru#915). The held figure is not corrected.
• With the default tail breakpoint the Overview reports 0.0817 against a true 0.0736 (+11%). The cause is not found.
• Tool defer (Accountant integration: tool defer fails check_sum and conservation on every request #908) stops the fallbacks but does not credit the deferral.
• TestALateFirstByteOnTheRealTransportNeverReachesTheClient is a 5 ms timing test. It fails on the untouched base 189c169d on a loaded machine.

[boazdavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 2 (glm, user of @boazdavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.241	offload-review skill, @boazdavid's session	resumed	2 of 2	2 of 5	0206b444ccbc	approve

Verdict: approve

Findings

None. Every round-1 finding is fixed and verified in the code. The new commits (#916, #917, #918, #920 and the round-1 fixes) contain no defect I could confirm.

Earlier findings, re-checked

• My finding 1 (rejected request with usage books dollars) — fixed in dcd1b31f. capture.rejected() (proxy/dashcapture.go:880) returns c.status != 0 && (c.status < 200 || c.status > 299). This matches DropUnbilledSavings exactly. finish now skips computeSafe and PriceMode for such a row and sets accNoResult = "rejected_status" (proxy/dashcapture.go:733-745). Tokens and dollars are both 0. The test TestFinishBooksNoDollarsOnARejectedRequestThatReportedUsage covers a 429 and a 400 with a usage object, with a 200 control that still books.
• Reviewer 1, finding 1 (estimated filter saving in no total) — fixed in 0a397f9d. declFilterInBaseline now starts with r.estimate_held = 0 AND (dash/toolsuggest.go:199). The measured filter credit keeps its dollars for exactly the rows the books hold out. EstimatedSavedUSD stays outside every total. The test TestFilterCreditKeepsDollarsForAHeldEstimatedRequest covers Realized, /api/adapt/tools and the Overview.
• Reviewer 1, finding 2 (price_estimated changed meaning) — fixed in 0a397f9d. New column requests.estimate_held, default 0, set only when the hold-out applied. The INSERT has 93 columns, 93 placeholders and 93 arguments; I counted them by script. Price, PriceMode, scanRequest and requestCols all carry the field. The Overview (full and lean) and the request detail read estimate_held. TestOverviewCountsEstimatedRequests holds an old-meaning row that must not count.

New commits, reviewed as a first review

• 2da3cb04 (#916, stats cache key). cacheKey now uses the exact query (dash/api.go). I traced the UI side: rangeMs maps to === 'now' to 0, writeRange omits until when it is 0, and resolveTime rounds now-X ranges down to 30 s. So the common view still hits the cache, and two windows that differ by one second no longer share a key. Sound.
• 6138171b (#917, state file). accStateVersion = 2 with a {Version, Snapshot} wrap; restore refuses other versions and bare snapshots. save syncs the temp file before the rename, skips the write when the bytes are unchanged (with an os.Stat re-check so a removed file is written again), and cleans stale temps. withoutCacheMarks now finds cache marks nested in objects and arrays, up to 64 deletions, with gjson path escaping for . * ? | # @. A failed delete breaks the loop and the mark stays in the hash, so the hash only becomes more conservative. Fail-safe. TestSavingsStateSkipsAnUnchangedWrite is consistent: the first save from the fresh tracker writes (its snapshot differs), the second skips, a removed file is written again.
• a1b89cf1 (#918, panic recovery). KeepAlivePing, PingSavings and accBegin each recover a panic and log it. I checked the accBegin drop path: accEnd guards c.accReq == nil (proxy/accountant_wire.go:45) and savingsTracker.drop guards req != nil. A hostile body can no longer reach the pre-forward recover and discard the pipeline rewrite. Sound.
• ab085e8e (#920). Pins that a first request books a cost when the price map is loaded. The old $0 row was an unloaded price map, not a pricing defect. No production change.
• 359a770d. Comment fix only, on messageOps. Correct now.
• 0206b444. Merge of the four above into integ/batch1. No new content beyond the commits read above.

Non-blocking

• dash/ui/app.js:3711-3714 — the comment says the net saving of a price_estimated row is "Marked '≈'", but no code marks it now. The base marked it: (e.price_estimated ? '≈ ' : '') plus the caveat "the net may be too high". The fix for reviewer 1's finding 2 switched the detail page to estimate_held and dropped the old marking. A row written before #915 (price_estimated = 1, estimate_held = 0) keeps its booked figure and now shows an unmarked net. At head the two flags are always set together, so such rows exist only in databases written by the unreleased branch. The docs make no false claim about them. Suggest: mark the net with "≈" when price_estimated && !estimate_held, or fix the comment.

What I checked

• Last review: round 1, 850893277db4cfd5736e06a1bfc55de881693ab9, submitted 2026-11-05T05:55:34Z.
• New commits read: 8 — 2da3cb04 (#916 cache key), 6138171b (#917 state file), a1b89cf1 (#918 panic recovery), ab085e8e (#920 first-request test), dcd1b31f (rejected-status fix), 359a770d (messageOps comment), 0a397f9d (estimate_held), 0206b444 (merge).
• Read in full: 2 reviews, 2 comments, 0 inline comments.
• New items since my last review, and what I did about each:
  • The three round-1 fix commits and the merge: verified each fix in the code, as listed under "Earlier findings".
  • Reviewer 1's two findings and both reviewers' non-blocking notes: checked the fixes (r.estimate_held = 0 AND, the estimate_held column and its readers, pricedRequests++, the head-TTL legs named in the comment and docs, estUsd rendering a negative figure as a cost, Price resetting EstimatedSavedUSD and EstimateHeld, the messageOps comment).
  • The author's comment with the fix details: read in full; each claim matched the code.
  • The author's "Known limits" list: read. The +11 % Overview gap with the default tail breakpoint is stated plainly and the cause is not found. I did not re-raise it; it is an accuracy question of the accountant line, not a defect in a changed line, and it is disclosed.
  • The merge of #928: reviewed as new commits above.
• The full diff 8508932..0206b444 (1240 lines), every changed file, and the code around each change: proxy/dashcapture.go, proxy/accountant_state.go, proxy/accountant_savings_body.go, proxy/accountant_ping.go, proxy/accountant_wire.go, proxy/accountant_state_test.go, proxy/rejected_savings_test.go, cmd/context-guru-proxy/main.go, dash/event.go, dash/schema.go, dash/store.go, dash/query.go, dash/accountant_price.go, dash/toolsuggest.go, dash/adapttools.go, dash/overview.go, dash/overview_lean.go, dash/api.go, dash/ui/app.js, all new and changed tests, both docs files.
• The insert SQL after the merge: 93 columns, 93 placeholders, 93 arguments, counted by script.
• One design point I judged correct rather than a defect: finish does not drop the ping facts for a rejected request (pings.drop stays in the non-rejected branch). The facts were never consumed by a compute that booked nothing, so the next request may still credit a rescue from them without double counting.
• I did not run the tests (live tests not approved). I read them and their helpers.

[boazdavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 1 (glm, user of @boazdavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.241	offload-review skill, @boazdavid's session	resumed	1 of 2	2 of 5	0206b444ccbc	approve

Verdict: approve

Earlier findings

1. My finding 1 (filter dollars of a held estimated request in no total) — fixed in 0a397f9d.

   • declFilterInBaseline is now (r.estimate_held = 0 AND EXISTS (...)) (dash/toolsuggest.go:204). Both users of the const share it: declFilterSavingsQuery (dash/toolsuggest.go:214), DeclFilterSavingsByTenant (dash/toolsuggest.go:274) and carriedCost through inBaselineCol (dash/adapttools.go:341). So every credit path keeps the dollars of a held row.
   • The author's reason is sound: the measured credit is computed from the rate card and the billed tier. It does not depend on the unknown cache state, so it is the right owner.
   • accOverride sets EstimateHeld = true only in the estimated branch (dash/accountant_price.go:354), and PriceMode resets it (dash/accountant_price.go:218). The stored figure stays out of every total. No path counts a held row twice.
   • TestFilterCreditKeepsDollarsForAHeldEstimatedRequest proves the fix: 10 rows, 4 held, all carrying a toolfilter row with priced_by = accountant. It fails without the estimate_held term, because the EXISTS clause alone matches held rows too.

2. My finding 2 (price_estimated changed meaning, UI claim false for old rows) — fixed in 0a397f9d.

   • New column requests.estimate_held (dash/schema.go:118, additiveColumns dash/schema.go:781, default 0). The INSERT has 93 columns, 93 placeholders and 93 arguments, with boolInt(e.EstimateHeld) at the same position as the column (dash/store.go:596, dash/store.go:676). scanRequest reads it back (dash/query.go:196, dash/query.go:226).
   • Both Overview paths count it: full (dash/overview.go:682) and lean (dash/overview_lean.go:119). The UI text and the request detail key off estimate_held (dash/ui/app.js:1834, dash/ui/app.js:3715-3718). A pre-#915 row has estimate_held = 0, keeps its booked figure, and is no longer called "not counted". The docs sentence is replaced (docs/dashboard.md:162-166).
   • TestOverviewCountsEstimatedRequests now holds one old-meaning row beside new-meaning rows and wants 2, not 3.

3. My finding 3 and reviewer 2's finding 1 (a rejected request that reported usage booked dollars beside zeroed tokens) — fixed in dcd1b31f.

   • I checked this independently, not only from the author's comment. finish now tests c.rejected() before the compute (proxy/dashcapture.go:741): a status that is not 0 and not 2xx sets accNoResult = "rejected_status", skips computeSafe, and PriceMode falls back with no_result. noResultWhy reports accNoResult before the rounds reason (proxy/dashcapture.go:884-887). Status 0 stays excluded, as before.
   • The ping facts are not dropped on a rejected request. That is correct: the request booked nothing, so the next request can consume them once. No double credit.
   • TestFinishBooksNoDollarsOnARejectedRequestThatReportedUsage sends a 429 and a 400 with a usage object, and a 200 control that books a saving. It pins the fix.
   • I agree with reviewer 2's finding 1 after this check. The fixes match.

4. Non-blocking items — all fixed. priceCarriedSession counts an in-baseline row as priced (dash/adapttools.go:480). The TTL legs are named as held out in the comment (dash/accountant_price.go:345-352) and the docs. Price resets EstimatedSavedUSD and EstimateHeld (dash/event.go:873). A negative held figure renders as "a cost of $x" (estUsd, dash/ui/app.js:105). The messageOps comment matches the code (359a770d).

Findings

None in the new commits.

Non-blocking

• Rows written by the round-1 head read as not held. A database written by 8508932 holds rows with price_estimated = 1, estimated_saved_usd > 0 and baseline_cost_usd = cost_usd, but no estimate_held value. The new column defaults to 0, so such a row reads as "counted", and declFilterInBaseline matches it: its filter dollars appear in no total, the defect of my round-1 finding 1. The author's comment covers only pre-#915 rows, which did book their figure; rows from the round-1 branch did not. Exposure is small: the branch is unreleased and the window is hours. A one-time UPDATE requests SET estimate_held = 1 WHERE price_estimated = 1 AND estimated_saved_usd != 0 in the migration would catch them, minus rows whose figure was exactly 0.
• The +11% Overview figure, cause not found. The author states: with the default tail breakpoint the Overview reports 0.0817 against a true 0.0736. The direction is flattering, not conservative, and it is the headline number of the product. I could not prove the mechanism by reading the code, so I do not make it a finding. Track it before the release: find the cause, or change the default.
• docs/dashboard.md:169 says acc_fallback reason rejected_status. The stored reason is no_result (dash/accountant_price.go:131). rejected_status appears only in the acc_fallback log line, as the why attribute (proxy/accountant_shadow.go:20-23). Say "log reason" in the docs, so a user inspecting the database finds what the docs promise.

What I checked

• Last review: round 1, 850893277db4cfd5736e06a1bfc55de881693ab9, posted 2026-11-05T05:53:59Z.
• New commits read: 8 (2da3cb04 #916, 6138171b #917, a1b89cf1 #918, ab085e8e #920, dcd1b31f, 359a770d, 0a397f9d, merge 0206b444). Full diff 8508932..0206b444 read, and each changed file around the changes.
• Read in full: 2 reviews (mine and reviewer 2's, round 1), 2 issue comments, 0 inline comments. New items since my last review: the 8 commits above and the author's two comments.
• The estimate_held wiring end to end: PriceMode/accOverride (set and reset), Price, the column DDL and additiveColumns, insertBatch (93/93/93, counted by script), scanRequest, Overview full and lean, the UI text and the request detail, docs/dashboard.md, docs/data-dependence.md.
• declFilterInBaseline and its three users, accumulateDeclFilterRow's skipUSD, priceCarriedSession's in-baseline branch, and TestFilterCreditKeepsDollarsForAHeldEstimatedRequest.
• dcd1b31f: the rejected() gate, its place before computeSafe, the ping-facts drop skip, noResultWhy's new order, and the test's 200 control.
• 2da3cb04 (#916): cacheKey keeps since/until exact. The UI quantizes now to 30 s in resolveTime (dash/ui/app.js:281) and omits until while the window is live (dash/ui/app.js:300-302), so a repaint burst still shares a key. dashCacheTTL is 30 s, so the key changes no faster than the body expires.
• 6138171b (#917): the version wrap (accStateVersion = 2, an unwrapped or other-version file is refused and the proxy starts empty), Sync before the rename, the skip-when-unchanged write (a stale mtime is then discarded by the one-hour age rule, which is consistent: an unchanged state describes entries no cache keeps), removeStaleTemps scoped to path + ".tmp*", and findCacheMark/withoutCacheMarks: the walk enters objects and arrays only, strings stay data, keys are escaped for the gjson path, and the 64-mark cap is far above the wire's four. The hash change is why the version bumped.
• a1b89cf1 (#918): the three recovers — KeepAlivePing (cmd/context-guru-proxy/main.go:976), PingSavings with named returns and injectable library calls, and accBegin, whose recover calls accDrop before the panic can reach the pre-forward recover and cost the rewrite. savings.drop is nil-safe (proxy/accountant_savings.go:34).
• ab085e8e (#920): test only; pins first-request pricing and the unloaded-price-map cause of the $0 row.
• I did not run any build or test. Live tests are not approved; I read the code and the tests.