Bound deployment spend with three gates - #43
Open
artyomsv wants to merge 24 commits into
Open
Conversation
There is no rate limiting of any kind in the codebase today. The only bounds that exist are a per-call token cap, the auto-retry budget, the conversation turn cap and the diff clip -- none of which bound how much a repository can spend or how often it can be reviewed. Places each check where its inputs already are: rate at admission, size on DiffFetched, spend before the LLM call. Records why folding the size check into the pre-spend gate was rejected -- the diff stats exist on DiffFetched and nowhere later, so checking them at the spend gate would need either new columns or a change to a Kafka wire type, and would run the context fan-out before discarding it. Keys the rate counter by provider as well as repo, which the ledger debt entry explicitly predicted this feature would otherwise inherit, and gives every cap a call-count axis beside the money one, since a money-only cap is inert by design on an unmetered deployment. Draft, pending review.
Two findings reshaped it, both verified against the code. The conversation path was outside every gate. planFollowUp emits a paid call guarded only by isSpendable, threads are free to open, the turn cap is per-thread and an @-mention removes it entirely -- the codebase says so in as many words. A spend cap that skips this path leaves the abuse case it exists for half-open, gating /review while comment-driven spend runs free. The first draft repeated an assumption the code comment above that guard already records as falsified once. A refused review had no lifecycle. The pre-spend refusal it models on writes a note and nothing else, so the review sits in reviewing until the stuck-review row fires blaming a webhook or a worker -- and since archiving refuses a running review, it cannot even be cleared. That is tolerable for a one-time configuration error and fatal for a cap that refuses by design. Refusals now reach a terminal refused status, distinct from failed so a policy decision is not filed as an outage. Also: names the two axes, since a review's calls and a repo's admissions are different units; records that the spend read must not filter archived charges, or archiving becomes a budget reset; states the cap as soft with bounded overshoot rather than implying exactness; corrects the claim that a counter is the only pre-hoc source; and settles the window question, since a rolling window's capacity-return instant is computable and so strictly more precise. Splits the admission rate limit into its own spec at the state seam -- everything here needs no new storage.
Ten tasks from the approved design, each ending in an independently testable deliverable. Front-loads the verified signatures, since the previous plan told an implementer to paste code calling five methods that did not exist. Orders the refusal vocabulary and the terminal status before any gate, because all three gates depend on both and a gate without a terminal state leaves the review stuck and unarchivable. Records the two tests most likely to be dropped: that an unset cap is a no-op, which is what stops an upgrade silently changing a running deployment, and that an at-mention does not bypass the spend cap, which is the unbounded case the conversation gate exists for.
The pre-spend refusal this copies writes a note and nothing else, so the review sits in reviewing until the stuck-review row fires blaming a webhook or a worker -- and since archiving refuses a running review, it cannot be cleared at all. Tolerable for a one-time configuration error, unacceptable for a cap that refuses by design. Uses refused rather than failed because the archive guard, the attention queries and the reviews list all key on status, so filing a policy decision as an infrastructure failure puts it in the same bucket as a genuine outage. The aggregate has one terminal-failure event, so refused is a read-model refinement of ReviewFailedTerminally. Projecting that event unconditionally relabelled the refusal as failed one broker round trip after the saga wrote it, and invisibly: the note stayed right while the badge, the list and the attention row all read the status. The projection no longer coarsens a status the saga made more specific.
Every cap (changed files, diff bytes, spend, calls) has no default, so a deployment that configures nothing behaves exactly as it does today -- a non-null default would silently change every running deployment's behaviour on upgrade. An unparseable stored value is also treated as unset rather than zero, so a typo in Settings cannot refuse every review. The rolling window is the one exception and always has an effective value (a day), since a window with no length is not a lesser cap but a meaningless one. CapSettingsResource writes an empty string to clear a limit back to unset, because AppSettingRepository has no delete -- CapPolicy already parses blank the same as absent, so the two meet without a new method.
Deliberately does not filter archived charges. Ten ledger reads beside this one do, and copying them would make archiving a review refund its budget -- an operator tidying the list would silently buy more spend. The discriminating guard is a charge with archived_at stamped, not an archived review: archiving stamps review_status alone and never the ledger, so an archived review's charges stay unmarked and a copied filter leaves that test green. Both cases are pinned; the stamped one is what fails when the filter comes back. Counts calls as well as money, because a money-only cap is inert on an unmetered deployment where every charge is an asserted zero, and an unpriceable call carries a null cost that SUM skips while the call axis counts it anyway. A read failure reports no usage and logs at ERROR. A cap that refuses every review because its own query failed is an outage wearing the face of policy.
Adds a Limits section beside Code review and Conversation with the five fleet-wide caps: max changed files, max diff bytes, spend cap, call cap, and the rolling window in minutes. Every field is optional and a blank input sends null, never 0 -- exactly the coercion that caused ADR-023's "unknown became zero" bug, and here 0 would mean "cap of zero", refusing every review, instead of "no cap". Kept as raw input strings rather than numbers so a cleared field stays blank instead of becoming 0, mirroring the model rate fields' existing pattern. A typed 0 (or any non-positive or fractional value) is rejected before any of the three settings groups are saved, since there is one Save button for the whole page.
Adds the pre-spend gate beside ADR-023's priceability check at ContextAssembled: deployment-wide spend and call count over the policy's rolling window, refused through the same terminal path as an unpriceable model. Both axes are checked because a money-only cap is inert by design on an UNMETERED deployment -- every charge there is a legitimate zero, so summed cost never approaches a positive limit; the call count is what actually bounds an unpriced fleet. Skips the ledger read entirely when neither limit is configured, so an unset cap stays a true no-op rather than a query that always answers allow. Also fixes a pre-existing hand-built ResultSaga fixture that never set a cap policy, which the new gate turned into a null pointer.
A reply in a live thread emitted a paid AnswerFollowUp guarded only by whether the model could be priced. Threads cost nothing to open, the turn cap is per-thread, and an @-mention removes that cap entirely, so the only genuinely unbounded spend path in the system was the one the review gates did not cover. The gate sits after the mention override rather than beside it: the override must keep bypassing the turn cap and must not also bypass the spend cap. The free turn-cap notice stays outside the gate because it buys nothing, and silencing it would restore the failure it exists to fix. A refusal here does not move the review. The review may have completed, and refusing one reply is not a retraction of that outcome, so this records the decision on the timeline and leaves status and note alone. Both paid-call sites now share one SpendGate rather than each holding a copy of the comparison. Two copies of a money gate are free to drift, and drift in a money gate is invisible until it fails to fire.
CAP_REACHED fires from the exact question SpendGate already asks before every paid call, so this row and the two enforcement sites cannot drift on what "over the cap" means. Unlike the two ledger-wide cost rows, it carries no acknowledgement: it describes usage right now rather than a past event, so it clears on its own once the charges that tripped it age out of the rolling window or the operator raises the limit. Names the instant capacity returns as the oldest in-window charge plus the window length, the concrete advantage a rolling window has over a fixed bucket -- backed by a new SpendWindow.oldestChargeAt, deliberately carrying no archived_at filter for the same reason since has none.
The orchestrator now sets a review's status to 'refused' when a spend or diff-size cap declines to run it, but nothing carried that value into the dashboard. ReviewStatus is a compile-time union and the status arrives as runtime JSON, so the type checker had nothing to check and every test stayed green while the screen was wrong. STATUS_LABEL is a Record keyed by that union, so it answered undefined and the badge rendered blank. Worse, miniPipeline tested each known status in turn and fell through to its terminal case, drawing a review the deployment declined to spend on as five green segments under "done". A refusal shown as a success is worse than one shown as nothing, because silence at least looks like nothing happened. matchesChip matched no chip, so the row was reachable only under All. Adds the union member, a Refused label, its own miniPipeline branch and a warn-coloured pill -- not crit, which would read as an outage, and not muted, which would read as nothing to do. Files the row under Needs attention rather than Closed. Closed holds cancelled and superseded, which mean nothing to do, while a refusal always leaves a decision: raise the cap, wait for the window, or split the pull request. What settles it is the diff-size gate, whose refusals raise no attention row at all, since CAP_REACHED is derived from the spend and call caps only -- under Closed those reviews would have had no surface anywhere. The three places that say "needs attention" now share one predicate so they cannot disagree. The test renders an actual refused row rather than probing the helpers, because the badge alone would still let the green "done" through.
ADR-025 records the three gates and why each sits where it does, the new terminal refused status, the dual money-and-call axis, the single SpendGate shared by both paid-call sites, and that unset means unlimited. Several entries are corrections of earlier reasoning rather than new decisions, and those matter more. The conversation path was the unbounded one and the codebase already said so, so a cap gating only the review path would have closed the front door and left the window up. A refused review needed a terminal state because the refusal it copied left one stuck in reviewing until the stuck-review row fired blaming a webhook, and since archiving refuses a running review it could not be cleared at all. And refused is a read-model refinement of the aggregate's one terminal-failure event, which the projection had been coarsening back to failed one round trip later. Also records that the spend read must not filter archived charges while the ten reads beside it must, that the cap is soft with overshoot bounded by in-flight reviews, that the window rolls so the instant capacity returns can be named, why oldestChargeAt lives on SpendWindow rather than in the attention queries, and why the cap row is blocking rather than a warning -- severity describes impact, not fault. Records the seam that let a new backend status reach the dashboard as a success, since that is a fact about the boundary rather than about this feature, and files the class under techdebt/spire-ui. Corrects the roadmap and the security doc, which both still told an operator there was no ceiling on spend, and marks the per-repo admission rate limit as the one part still deferred. Adds a runbook mode covering a tripped call cap, a refused review being archivable, the attention row appearing and clearing itself, the diff-size gate, and an at-mention failing to buy a way around the cap. Widens the god-class debt entry to the three classes now past the guideline, with measured counts and a separate framing for each. The result saga went 517 to 625 and back to 598 when the spend comparison was extracted, so its gates want their own collaborator and the direction is set. The attention queries went 377 to 432 under a different cause: one private method per condition is that file's own convention, followed correctly, and it simply has no term that stops growing.
artyomsv
force-pushed
the
feat/fleet-cost-caps
branch
from
August 9, 2026 23:51
8614739 to
42d8ef1
Compare
A cap-refused review has no findings and no reconciliation, so the
findings card fell past the failed/cancelled branch into the empty
state and told the operator "clean - no issues found in this diff".
Nothing had reviewed that diff.
`r.note` is rendered in exactly one place in the UI, inside that same
branch, so the actionable refusal text - raise the limit in Settings,
or split the pull request - was written, stored, sent over the wire
and displayed nowhere. That contradicts SMOKE-TEST Mode M-1, which
asserts the note is visible.
The status check becomes a lookup keyed by status, so `refused` gets
its own heading ("Why it was refused" - nothing stalled, nothing was
stopped) and a status missing from the map is the absence of an
explanation card rather than a fall-through into the findings
branches. A blank note now renders a placeholder: an empty card is
the same false reassurance.
findCell joins failed/cancelled in showing a placeholder, since a `0`
was a findings count for a diff no model ever saw.
The existing refused tests covered only the list row; these render
the detail page and assert the "clean" claim is absent, not merely
that the new heading appears.
The ReviewGenerated handler charged inside ifCurrentRun, so a commit pushed (or the PR closed) while the LLM call was in flight dropped the handler whole and the charge with it. The worker's PR-head re-check runs before the call, so nothing about the stale run un-spends the money. That was an under-reported cost card until the ledger became an enforcement input. Now it is a cap-evasion primitive: push-then-push in a loop buys uncounted paid calls without bound, and one GenerateReview stands for up to two of them. Charge first and unconditionally -- the shape FollowUpGenerated has always had -- and keep the staleness guard on everything downstream, extracted to onReviewGenerated. Redelivery is still a no-op without a new mechanism: recordCharges is ON CONFLICT (call_ref, token_type) DO NOTHING and reviewSlot keys on the commit, so a superseded run's ref cannot collide with the new run's.
The status guard added with the refused lifecycle lives in projectTerminalFailure and is reached only from DomainEventSink. ResultSaga.onReviewFailed is the other writer of a terminal status and went straight through updateStatus with no precondition -- and ReviewFailed is the one result the saga does not wrap in its stale-run guard, so the aggregate already being terminal did not stop it either. A replayed ReviewFailed (cs.results is at-least-once, and DLQ replay is deliberate) therefore relabelled the review failed, overwrote the note that is the operator's only explanation, and populated error_detail -- the field ADR-025 leaves unset for a policy decision so the detail page does not show an infrastructure fault. Status, stage, note and error now land in one guarded statement, so the guard cannot hold for the badge while the rest is overwritten anyway. scheduleRetry gets the same predicate, for the worse half of the same defect: a replayed retryable failure dragged a refused review back to reviewing with a fresh due time, and the sweeper would then re-run the pipeline and spend again on a review that was refused on purpose.
`rerunnable` listed completed and failed, so the button did not render for a refused review - while `ReviewRerunService.rerun()` gates on nothing but `archived`, so the operation was permitted all along. That worked directly against the operator. The recovery path for a cap refusal is raise the limit, then re-run, and the refusal note on the same page says exactly that. With no button, the only way to act on the advice was to push a commit to a pull request that may be perfectly finished. The boolean becomes a total map over `ReviewStatus`, each answer carrying its reason. It cannot catch a status the backend has and `api.ts` does not - an unknown one hides the button, which is the conservative direction when pressing it spends money - but it fails the build the moment the union grows, so the next status is a decision rather than whichever way a `||` chain happened to fall. Only `refused` changes; the other six are pinned as they already behaved. The confirmation gains a line for a refusal: no model call was made, so the wording must not imply a previous result is being replaced, and what the operator needs is that pressing it before raising the limit changes nothing. Added beside the existing note rather than replacing it - an earlier round may have posted comments before a later one was refused.
CapRefusal put the configured cap into detail() and note(), which reach three surfaces a spire-viewer reads: the review timeline, the review's note and the CAP_REACHED attention row. ADR-022's third rule makes configuration admin-only including its reads, and the precedent is in this repository -- ADR-023's review round dropped per-token rates from ReviewDetail for exactly this. The measured figure is the review's own context and stays. The limit moves to logDetail(), a suffix on detail() rather than a second wording so the two can never describe one refusal differently. Money now formats with Locale.ROOT: a de-DE default rendered "$0,01" into operator-facing text.
Three ways a refusal was quieter or louder than the truth. SpendWindow.since answered Usage(0, 0) on a failed ledger read, which is indistinguishable from "nothing has been spent" -- so the gate saw "allowed" and the attention panel, whose whole contract is that every row is a condition true right now, reported health while no paid call was being refused. The plausible trigger is not a total outage but connection-pool exhaustion under load, i.e. the exact burst the cap exists to bound. The read answers Optional.empty(), SpendGate.Decision carries ledgerUnreadable beside the verdict so all three sites still share one comparison, and CAP_UNENFORCEABLE states the condition. Failing open is unchanged and deliberate. "Capacity returns at X" was a lower bound presented as an exact instant: ageing the oldest charge out restores enough capacity only when the overshoot is no larger than that charge. With a call cap of 100 and 300 calls in the window it names an instant that changes nothing. Reworded to "No capacity returns before X"; ADR-025 stated the exact claim as fact and is corrected. A refused follow-up wrote only to the timeline -- an in-memory list capped at 500 entries deployment-wide and lost on restart -- so the only durable trace of an unanswered reply was the deployment-wide CAP_REACHED row, which never says which thread. It now appends FollowUpRefused to the review's event log under the conversation root. Still no setNote: "Not reviewed" is false on a reviewed PR. SpendGate's javadoc now states why the spend gates refuse on >= while the diff gate refuses on >, and SpendGateTest pins both boundaries -- the money axis was reachable by no test at any level.
The "unset never becomes zero" invariant was enforced only at the REST boundary. CapPolicy reads app_setting directly and returned OptionalLong.of(0) for a stored "0", after which spent >= 0 is true for every review, forever, on every gate -- a deployment-wide review outage from one row. Not reachable through the application today, which is the argument this repository has already lost once: ADR-023 held the conversation path safe by construction because the registry forbids an unpriceable model, and V30 then created rateless models directly in SQL. A non-positive stored value now reads as unset, alongside the unparseable one it already treated that way; the REST rejection stays, so an operator still gets a 400 rather than a silent no-op. An out-of-range windowMinutes was accepted with a 200 and then threw on every use. Instant.now().minus(window) throws beyond the instant range, and it throws inside AttentionQueries.collect (outside its SQLException catch, so the whole panel goes dark) and inside both spend gates, which dead-letter. GET /api/settings/caps builds the same Duration, so it 500s too and the value cannot be cleared through the product. Bounded at a year at the REST boundary, and read as unset above it. Only the window is bounded above: the four caps are compared, never added to an instant, so a huge one is an unreachable limit.
/api/settings/caps was missing from OperatorAuthTest's CONFIGURATION_READS, whose javadoc says a new screen absent from the list is meant to be a failing test rather than an oversight. The role was already right; only the assertion was missing. diffSizeDecision's maxDiffBytes branch was exercised by nothing -- both fake policies returned empty for it and every case drove maxChangedFiles -- so the copy-paste that compares the file count against the byte limit would have shipped green. Verified by making exactly that mutation: two tests fail. Neither comparison's boundary was pinned either. The diff gate refuses on > and the spend gate on >=; both readings are defensible and the difference was indistinguishable from the drift SpendGate exists to prevent. Now stated in both javadocs and asserted on both axes. SpendCapGateTest's refusal case asserted the status and the note but never that GenerateReview is not emitted, which is the assertion that matters on the most expensive of the three gates -- a regression that refuses and spends anyway passed it. The pattern was already in the two sibling suites.
computeStages had no refused case, so it fell to default and drew all six stepper nodes grey. A review refused at the pre-spend gate had fetched its diff and assembled its context; the detail page said neither happened. cancelled and superseded already mark their completed steps done, so the omission was inconsistent within the one switch. Grouped with cancelled/superseded rather than failed: the work that ran really ran, and no step is marked failed because nothing failed -- the same split ADR-025 makes for the status itself.
Three gaps the QA lens found, all of the shape where the control is present, looks tested, and the path that matters has never run. The money axis was never driven true by a real charge anywhere. Every integration test that trips a cap uses UNMETERED lines whose cost is an asserted zero, so SUM(cost_millicents) was never compared against a limit it could cross -- the primary axis of the feature had no end-to-end coverage. SpendCapGateTest now records a real metered charge that crosses a configured spend cap, with the call cap left unset so the refusal can only be the money one, and asserts the note names spend and not calls. It also asserts the fixture really recorded money first, so it cannot pass on ambient spend from a sibling suite. Which axis is reported when both are over was whichever if was written first. Money wins, and now says why: it is the axis the operator configured in the unit they care about, while the call count is the proxy that bounds an UNMETERED fleet -- reporting the proxy sends them to raise the wrong limit. Pinned from both sides, so it is ordering rather than precedence-by-accident. oldestChargeAt had no direct test; the attention test only asserted its message contained the wording, never that the instant was right. Now anchored: the window starts exactly on one seeded charge, which makes it the minimum whatever else is in the shared database. The first draft of that test was vacuous -- one distinct timestamp in scope makes MIN and MAX the same row, and it survived that mutation when it ran first in the class. A second, newer charge is now seeded so "earliest" means something; both tests fail on MIN -> MAX.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bounds what a deployment can spend, refuses pathologically large diffs, and gives a
refused review a terminal state it can be cleared from.
This is Spec A of two. The per-repo admission rate limit is deliberately not here —
it is the only part needing new storage, and the design's final section records what it
must carry.
Why
The ROADMAP recorded this as a deferred, known operator-facing gap. Precisely stated, the
codebase had no admission or spend limiting at all. The bounds that existed were
narrower than they sound:
maxTokensper LLM call — bounds one call's output, nothing aggregateSPIRE_REVIEW_MAX_ATTEMPTS— the auto-retry budget, not a spend budgetThe two findings that reshaped the design
The conversation path was unbounded, and the code already said so
ConversationSaga.planFollowUpemitted a paid call guarded only byisSpendable. Threadscost nothing to open, the turn cap is per-thread, and
CallRefsstates outright that an@-mention removes it. The comment directly above that guard already recorded that this path
had been assumed safe once and was not.
The first draft of this design repeated the assumption. A cap gating
/reviewwhile leavingthis open would have closed the front door and left the window up.
A refused review was stuck, and after ADR-024 also unarchivable
The pre-spend refusal this design modelled on wrote a note and nothing else — no status
change.
AttentionQueries' own javadoc records the consequence: the review "sits inREVIEWING until REVIEW_STUCK eventually fires", and that row blames "a webhook delivery
path or a worker" — false when the truth is a deliberate policy decision. ADR-024's archive
guard refuses a
reviewingrow, so it could not even be cleared.Tolerable for a one-time configuration error. Unacceptable for a cap that refuses by design.
What changed
Three gates, each where its inputs already are:
ResultSaga, onDiffFetchedResultSaga, beforeGenerateReviewConversationSaga.planFollowUpThe diff check must be on
DiffFetched:changedFilesandsizeBytesexist there andnowhere later, and refusing at that point skips the context fan-out — per-issue API calls,
a 20-second bounded wait and an encrypted blob write — that would otherwise be discarded.
One decision, one implementation.
SpendGateis shared by both spend gates and by theattention row. Two copies of a money check drift, and drift in a money gate is invisible
until it fails to fire.
A terminal
refusedstatus, distinct fromfailedbecause the archive guard, theattention queries and the reviews-list filters all key on status — filing a policy decision
as an infrastructure failure would put it in the same bucket as an outage. Same reasoning
that split
pr_stateout ofstatus.Both axes. Every cap is money or calls, whichever trips first, because a
money-denominated cap is inert by design on an
UNMETEREDdeployment where every charge isan asserted zero. An
UNKNOWN-priced row's NULL cost is skipped bySUMand caught by thecall count — exactly the accounting hole ADR-023 identified.
Unset means unlimited, and unset is the default. A deployment that configures nothing
behaves exactly as it does today, and an unset cap does not even read the ledger.
Three things worth a reviewer's attention
The spend read deliberately does not filter
archived_at. Ten ledger reads beside it do,and are right to — they answer "what does this review's page show", and a purged review has
no page. This one answers "what has already been spent", and archiving must not refund a
budget.
The terminal status was being silently overwritten.
refuseroutes throughRecordFailure→ReviewFailedTerminally, which was projected asstatus = 'failed'— sothe refusal was relabelled one Kafka round trip later while its note stayed correct. The
aggregate keeps one terminal-failure event, so
refusedis a read-model refinement theprojection now declines to coarsen.
The UI had no
refusedstatus, and rendered one as a success.ReviewStatusis acompile-time union over runtime JSON, so
tschad nothing to check and no test constructed arefused row —
progress()fell through to its default branch and produced fivedonesegments with a green "done" label. Fixed in
e336ca1, with a test that builds a refused rowand asserts badge, progress chain and chip membership. The class of gap is recorded as debt:
any new backend status degrades silently into whatever the default branch does — and the review
below found three more instances of it on the detail page.
Not built
The admission rate limit (Spec B), per-actor limits, per-repo spend caps — which need
provider_typeonllm_charge, the existing debt entry's own recommendation — queuing, andcost estimation before a call. That last one would need a token estimate the system cannot
validate, which is the fabricated-number problem ADR-023 exists to prevent; these caps refuse
on measured history and measured input only.
Verification
Final counts are below, after the review fixes. Ten tasks, each with a test written and confirmed failing before its implementation. Two
tests were rewritten after proving order-dependent — the module shares one Postgres across
~76 suites and several write real charges into the same rolling window a cap reads, so the
cap tests measure a live baseline and set limits as
baseline + headroomrather than usingliterals.
Details in
docs/DECISIONS.md(ADR-025),docs/SMOKE-TEST.md, anddocs/superpowers/specs/2026-08-09-fleet-cost-caps-design.md.Reviewed on four lenses; every finding fixed
Security, code-quality, rules and QA. Rules came back clean. The other three found 13 issues,
all fixed in this PR.
The one that mattered most was not in the diff.
chargeGeneratedCallssat insideifCurrentRun, so when an author pushed a new commit while the LLM call was in flight, the wholehandler was dropped and the charge was never written. That line was correct when written —
dropping a stale run's outcome is right, and the charge merely shared the block. This PR promotes
the ledger from a report into an enforcement input, which turned an under-reported cost card into
a cap-evasion primitive: push-then-push in a loop buys uncounted paid calls, unbounded, rather than
the "in-flight × per-review cost" overshoot the design claims. Fixed in
467a8ae, with the chargerecorded before the staleness guard and the guard applied to everything downstream.
A refusal could still trigger spending.
onReviewFailedbypassed the new terminal-status guard.The review found it could relabel a refusal
failed; the fix found the retryable branch was worse —it wrote
status='reviewing'and a freshretry_at, so the sweeper would re-run the pipeline andspend again on a review refused for exceeding a spend cap. Both closed in
8bfb2f2.A refused review told the operator its diff was clean.
refusedreached the list surfaces butnot the detail page, which rendered
✓ clean — No issues found in this diff.for a review nothinghad looked at — and the actionable note was displayed nowhere in the UI. This branch's own runbook
asserts that note is visible, which is useful evidence Mode M had never had a live pass. Fixed in
ff53185,fcf89eeandcf33792, the last because Re-run was hidden on a refused review while thebackend permitted it — the refusal advises raising the cap and then offered no way to act on it.
Also fixed: the configured cap no longer appears on viewer-readable surfaces (ADR-022's third rule);
a failed ledger read is now visibly
CAP_UNENFORCEABLErather than silently indistinguishable fromzero spend; the attention row says "No capacity returns before X" instead of naming an instant that
is only a lower bound, and ADR-025 is corrected where it stated the exact claim as fact; a stored
0reads as unset rather than refusing every review forever;windowMinutesis bounded on writeand read; a refused follow-up now leaves a durable record; and
/api/settings/capsjoins the authlist whose purpose is catching exactly that omission.
The money axis had no end-to-end test. Every cap test used
UNMETEREDcharges costing zero, soall of them tripped the call axis — the feature's headline capability, refusing on money, was never
exercised.
SpendGateTestcloses it.Verification
1283Java tests across167suites;338spire-uivitest tests across46files;tsc --noEmitsilent. Forced reruns, not cached passes.Two items deliberately left, both recorded:
refuseForTest's rationale is stale now that both gatesexist, and a refused follow-up is still silent to the person who asked — a
NotifyTurnCap-shapednotice is a design decision rather than a defect fix, and belongs with the rate-limit spec.