review: enforce the run's spend ceiling in code, in real dollars - #323
review: enforce the run's spend ceiling in code, in real dollars#323jwbron wants to merge 5 commits into
Conversation
…ledger with one rule First slice of the spend ceiling (plan item B), on its own branch stacked on #305 rather than added to it. This commit is the ledger and its arithmetic; the runner and dispatch wiring follow. The rule is one pure function of three numbers, so every interesting property is directly testable: work may start while spend is under the DISPATCH BUDGET, which is the ceiling minus a landing reserve. The reserve is the part that is easy to leave out and expensive to miss: a ceiling with nothing held back turns an over-budget run into a wasted one, because the run cannot validate or post what it already paid to find. Enforcement is per turn, not per wave. Cost arrives one assistant turn at a time and one heavy reviewer can outspend a whole wave of light ones, so a between-waves check notices the overshoot only after paying for it. Crossing aborts in-flight agents through the ledger's AbortSignal and records the shed with the dollar it happened at, because a review that quietly drops a dimension for lack of money is indistinguishable from one that found nothing there. Numbers, derived rather than picked: $12.50 ceiling, because the compiled lock allows maxAiCredits: 2500 for the agent job, the detection pass spends ~$0.27 of that and this ledger does not govern it, and a measured full case's sub-agents ran about $0.50. The ceiling has to sit under the proxy's real-terms equivalent or the shed behaviour is unobservable, since the proxy would kill the run first. $1.50 reserve, sized from the validator dispatch (~$0.30 measured, the expensive tail) plus the reconciler. A test pins both, so changing live spend behaviour is a review conversation and not a diff nobody reads. REVIEW_SPEND_ENFORCEMENT=proxy-only is the rollback, and it measures without enforcing: the report still says crossed, still lists the sheds, and the notice is loud. A rolled-back run is not a blind run. The report also names which enforcement was in force, per the plan's rule that where granularity changes over time the artifact says which one produced it. That report is the substrate telemetry record v0. One thing this ceiling does NOT do, recorded in the module rather than implied: it does not replace the LOCATION property of the proxy cap. awf's cap is un-weakenable because it lives in a container image no Khan repo can edit; this one lives in a repo whose PRs the reviewer reviews. The provider-side workspace limit that would have restored that property is unavailable, so CODEOWNERS plus required review is the whole mitigation.
… and say when it did The runner's seam for the ceiling: `onTurnCost` reports each turn's cost delta as it lands and asks whether to continue. The delta, not the total, because the ledger is run-wide and this runner only ever knows about one agent. Stopping happens at a turn boundary through `shouldStopAfterTurn`, not by throwing. An agent stopped for budget has usually already found something, and killing it mid-turn would discard that along with the turn's cost that was already paid. `AgentResult.stoppedForBudget` carries the fact outward on every success path, including the free-text final. The dispatcher needs it to disclose the dimension as cut short rather than completed: an agent that ran out of money mid-investigation and one that finished and found nothing produce the same empty shape, and only this flag tells them apart.
…tch, and disclose what it cuts Wires the ledger into the run and closes the gap that would have made it decorative. The gap: as first written, the ledger only learned about cost if a caller remembered to hook the runner, and a forgotten hook is a ceiling that silently never fires. It now has exactly one writer, the dispatcher, called once per completed attempt so a retry and a refusal fallback each pay for themselves. The per-turn check became a read-only probe against that accounting, which is what makes double counting impossible without needing per-attempt identity the runner does not have. Two abort paths, because one does not cover the other. The probe stops the agent that is doing the spending, at a turn boundary. The ledger's AbortSignal, now actually consumed by the runner, stops the siblings already in flight when someone else's completed dispatch crosses the line; before this it was an unused field, which would have made "crossing aborts in-flight agents" true only of the agent that crossed. Disclosure reuses the machinery already there: refused and aborted agents become `skippedDimensions` with cause `budget`, get note lines distinct from the tier-budget shed (planned before the run) so a reader can tell "we chose not to" from "we ran out", and appear in `perAgent` with `failed: "budget"` rather than as agents that ran and found nothing. The sheds are read back OUT of the ledger when the result is assembled, so the record and the disclosure cannot disagree. `DispatchResult.spend` rides both staged copies of the run artifact, so the gate and the post-hoc read the same document. Integration tests cover the wiring rather than the arithmetic: one dispatch paid for and the rest refused under a tight ceiling, the whole roster running under the rollback, and the record present in both staged files. CODEOWNERS names @jwbron and @jeresig on the ledger, since it is now the only limit on what a run can spend.
🦋 Changeset detectedLatest commit: 1b2ead2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| recordSpend: (agent, usd) => { | ||
| spentUsd += Math.max(0, usd); | ||
| if (decideSpend(spentUsd, ceilingUsd, landingReserveUsd).crossed) { | ||
| cross(agent, "aborted"); |
There was a problem hiding this comment.
issue (blocking): The agent that completes-and-crosses is mislabeled 'aborted' and disclosed as a shed, so the record contradicts itself. recordSpend is only ever called after a dispatch completes (dispatch-calls.ts:306/334), so the agent passed to cross(agent,'aborted') ran to completion — it was not aborted or cut short. dispatch.ts:672-683 then lists it in skippedDimensions and dispatch.ts:718 emits a 'cut short mid-investigation' note, even though the same agent appears in perAgent/dispatched with its findings kept. spend-ledger.test.ts:97 pins this mislabel. Ironically the actual in-flight siblings killed by the AbortSignal are NOT in ledger.sheds, so they surface as generic 'unavailable', not budget — the disclosure names the wrong agents.
Also flagged by completeness.
| // Cumulative for this agent, because the probe is a | ||
| // question about the run's total and the caller adds this | ||
| // agent's in-flight spend to what it has already settled. | ||
| if (options.onTurnCost?.(usd) === "abort") { |
There was a problem hiding this comment.
todo (blocking): Per-turn budget probe (onTurnCost → budgetExhausted → stoppedForBudget) is untested. grep confirms onTurnCost/budgetExhausted/stoppedForBudget appear only in source, never in any test; dispatch-runner-pi.test.ts never passes onTurnCost, and dispatch-spend.test.ts's fake chargingRunner never invokes it. The runner test harness already emits turn_end events with costs, so this is cheap to pin.
A sketch, not a committable replacement:
it("stops at the turn boundary and flags stoppedForBudget when onTurnCost aborts", async () => {
loop = async ({emit}) => {
emit(turnEnd("first turn", 5));
emit(turnEnd("second turn", 5));
return [];
};
const runner = await createPiRunner({
onTurnCost: (spent) => (spent >= 5 ? "abort" : "continue"),
});
const result = await runner(request());
expect(result.stoppedForBudget).toBe(true);
expect(result.turns).toBe(1); // stopped after the first turn crossed
});
| if (runAbort?.aborted === true) { | ||
| abort.abort(runAbort.reason); | ||
| } else { | ||
| runAbort?.addEventListener( |
There was a problem hiding this comment.
todo (blocking): Run-wide abortSignal linking into the runner's internal AbortController is untested. The ledger unit test asserts signal.aborted flips, but nothing verifies the runner consumes that signal to stop an in-flight request. The fake chargingRunner ignores the signal entirely, so both the pre-aborted and later-abort branches are dead to the suite.
A sketch, not a committable replacement:
it("aborts an in-flight request when the run-wide signal fires", async () => {
const controller = new AbortController();
loop = ({signal}) =>
new Promise((_r, reject) =>
signal?.addEventListener("abort", () => reject(new Error("aborted"))),
);
const runner = await createPiRunner({abortSignal: controller.signal});
const pending = runner(request());
controller.abort(new Error("spend ceiling"));
const result = await pending;
expect(result.stopReason).not.toBe(undefined); // stopped, not run to completion
});
| maxTurns, | ||
| timeoutMs, | ||
| validatorFor, | ||
| mayDispatch: (name) => ledger.mayDispatch(name), |
There was a problem hiding this comment.
issue (blocking): Landing reserve cannot fund the validation it exists for — the validator is gated by the dispatch budget, not the ceiling. mayDispatch is wired uniformly (no exemption for the claim-validator or reconciler) and decideSpend compares against ceiling−reserve. The reserve's stated purpose is 'held back so the run can still validate and post what it already found', but the very dispatch it reserves money for is refused by the same gate the moment the run crosses, leaving the reserve as dead headroom and silently disabling validation exactly on expensive runs. To make the reserve work, landing-phase dispatches (validator/reconciler) must be allowed to spend into it, i.e. checked against the ceiling rather than the dispatch budget.
| return true; | ||
| } | ||
| cross(agent, "refused"); | ||
| return enforcement !== "in-code"; |
There was a problem hiding this comment.
suggestion (non-blocking): Rollback mode emits false 'not assessed' sheds for agents that actually ran. Same root cause as the aborted-mislabel: disclosure is derived unconditionally from ledger.sheds regardless of whether the agent ran. In proxy-only this is worse — every post-cross agent runs and is simultaneously reported as shed, which defeats the stated goal that 'a rolled-back run is not a blind run' by making it a lying run. Sheds should reflect work that was actually refused/aborted, not measured hypotheticals.
| # Pin bumps and policy edits under these paths are behaviour changes: reviewed | ||
| # as such, never batch-merged. | ||
|
|
||
| /workflows/review/lib/spend-ledger.ts @jwbron @jeresig |
There was a problem hiding this comment.
question (non-blocking): Does CODEOWNERS on spend-ledger.ts alone cover the bypass surfaces the mitigation claims to close? The owned file holds the constants, but the enforcement is wired elsewhere: dispatch.ts (unowned) constructs the ledger and accepts an arbitrary ledger override via DispatchOptions, and the rollback flag is read from process.env, settable from the workflow lock/frontmatter — none of which are under the new entries. Grep confirms REVIEW_SPEND_ENFORCEMENT flows through dispatch.ts and the runner, not just the owned module; the mitigation as written protects the number, not the decision to use it. (Also worth confirming branch protection actually requires code-owner review in this repo — a CODEOWNERS file with no such rule enforces nothing.)
A sketch, not a committable replacement:
Extend the entries to the wiring that selects ceiling and enforcement (dispatch.ts's ledger construction and the review workflow files that set env), or record in the CODEOWNERS comment that the caller-override and env-flag surfaces are knowingly outside the mitigation.
| * changes. Each consumer compiles its own lock, so a consumer with a different | ||
| * credit allowance wants a different number here. | ||
| */ | ||
| export const CEILING_USD = 12.5; |
There was a problem hiding this comment.
thought (non-blocking): The pinned CEILING_USD duplicates a cap the repo already discovers at runtime, and its 'sits under the proxy' invariant is unenforced. credit-cap.ts already resolves the live per-run credit cap from the environment (resolveCreditCap, 1 credit = $0.01) and already holds back a landing reserve (LANDING_TARGET_RATIO = 0.75), so the run now carries two independently-sized budget systems — the router's clamped runBudget.maxUsd and the ledger's CEILING_USD/LANDING_RESERVE_USD — that nothing keeps consistent; the PR's own premise that 'a consumer with a different allowance wants a different ceiling' is precisely the problem resolveCreditCap already solves dynamically. The pinned constant makes sense only after the migration deletes the proxy, but until then the load-bearing invariant (ceiling < proxy's real-terms equivalent, which also bakes in an unstated credits-to-real-dollars discount rate) is asserted in a comment rather than checked anywhere.
A sketch, not a committable replacement:
While the proxy exists, derive ceilingUsd from resolveCreditCap (e.g. min(CEILING_USD, discoveredCapUsd × realTermsRatio)) or at least assert at ledger construction that the ceiling sits under the discovered cap, warning loudly when it does not; and say in one place how this ledger's reserve relates to LANDING_TARGET_RATIO so the two reserves cannot silently diverge.
| * flag is read ONCE at construction: an enforcement posture that could | ||
| * change mid-run is not a posture. | ||
| */ | ||
| env?: {[key: string]: string | undefined}; |
There was a problem hiding this comment.
nitpick (non-blocking): Injected env typed with an index-signature literal instead of the repo's Record<string, string | undefined> idiom. Every sibling module that injects the environment types it the same way — credit-cap.ts env: Record<string, string | undefined>, and likewise in router.ts, dispatch-runner-pi.ts, and stage-pr.ts — but spend-ledger.ts line 138 writes env?: {[key: string]: string | undefined};. The two are semantically identical; the literal form just departs from the established spelling.
| env?: {[key: string]: string | undefined}; | |
| env?: Record<string, string | undefined>; |
…nd let the reserve fund landing Review feedback on #323, all four blocking findings plus two non-blocking: - A completed dispatch that crosses the budget is no longer shed as 'aborted': the completer ran and its findings are kept. The agents the crossing actually cut are disclosed via the new ledger.recordAborted, fed by the dispatcher (a budget-stopped completion always; a thrown attempt only when the ledger's signal killed it), and a budget cut recorded upstream as a generic 'unavailable' is upgraded to cause budget in the disclosure. - The landing reserve now funds the landing: ledger.enterLanding() (called at Phase 3) moves the gates to the full ceiling and swaps in a fresh abort signal, so the validator and anything after it can spend the reserve instead of being refused or born aborted by the dispatch-phase crossing. The runner accepts an abort-signal thunk and resolves it per request. - The per-turn budget probe (onTurnCost -> budgetExhausted -> stoppedForBudget) and the run-wide abort linking (pre-aborted and aborted-later) are pinned by tests. - Rollback mode (proxy-only) no longer records refused sheds for dispatches that actually ran; the run instead carries an explicit crossed-with-enforcement-off note. - Injected env typed Record<string, string | undefined> per the sibling-module idiom.
…es/review-pi-harness-seam' into jwies/review-spend-ceiling
|
Addressed all feedback in 70fcbb1 (base sync with #305 in 1b2ead2):
|
|
CI note, unrelated to this PR's diff: the agent job on 1b2ead2 (run 31745225791) died from the firewall api-proxy's cache-miss guard, not from the branch. The artifact log shows |
1 participant
Plan item B, stacked on #305 (base is that branch, so this diff is only the ceiling).
What
Dispatch carries a budget ledger, denominated in real dollars. Work may start only while the run's spend is under
CEILING_USD - LANDING_RESERVE_USD; crossing it refuses further dispatches, aborts the agents already in flight, and discloses every shed as a note line rather than a silently missing dimension.Why in code rather than at the proxy
The only ceiling today is gh-aw's api-proxy credit cap (
maxAiCredits: 2500): denominated in list-price credits (2x what Khan pays), unable to shed gracefully (it fails the run), and gone with the proxy when the migration completes. This supersedes #314's cap-denomination motivation; the proxy cap remains a coarse list-price backstop, and #314's remaining purpose is proxy-side model pricing (which #294 needs).The numbers, derived rather than chosen
$12.50 real) for the agent job; the detection pass ($0.27) is outside this ledgerA test pins both, so changing live spend behavior is a review conversation rather than a diff nobody reads. The ceiling must sit under the proxy's real-terms cap or the shed behavior is unobservable (the proxy would kill the run first). The reserve is the piece that is easy to omit and expensive to miss: a ceiling with nothing held back turns an over-budget run into a wasted one, unable to validate or post what it already paid to find.
Enforcement shape
AbortSignal, consumed by the runner, stops the in-flight siblings when someone else's completed dispatch crosses the line.overshootUsdmeasures it instead of assuming it away.Disclosure, rollback, telemetry
Refused and aborted agents become
skippedDimensionswith causebudget(note lines distinct from planned tier sheds, so "we ran out" reads differently from "we chose not to") andperAgententries withfailed: "budget"; the disclosure is read back from the ledger, so record and notes cannot disagree.REVIEW_SPEND_ENFORCEMENT=proxy-onlymeasures without enforcing (still reportscrossedand the sheds), and the record names which enforcement was in force.DispatchResult.spend(ceiling, reserve, spend, overshoot, sheds, enforcement) stages indispatch-result.jsonand theout/artifact copy.Known trade
Unlike the proxy cap (a container image no Khan repo can edit), this ceiling lives in a repo whose PRs this reviewer reviews. Mitigation: CODEOWNERS on the ledger plus required review; recorded in the module itself.
Tests
1691 pass: 13 on the ledger's arithmetic and abort wiring, 4 integration (one dispatch paid and the rest refused under a tight ceiling; the whole roster under the rollback; the record present in both staged files; the enforcement named).