From 61478a060c0269cd631986b72bbee818e2b5723c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 12:24:27 -0700 Subject: [PATCH 1/3] feat(miner): wire attempt-metering.ts into the iterate loop for a real mid-attempt budget abort Closes #5395 - iterate-loop.ts now accumulates real per-iteration usage (turns, costUsd, and wall-clock time measured around each driver invocation via an injected clock) into attempt-metering.ts's AttemptMeterTotals, and evaluates it every iteration against an optional AttemptBudget -- a breach on any axis now abandons mid-attempt via the same costCeilingReached signal iterate-policy.ts already exposed for the narrower, never-wired maxTotalTurns field it replaces. - attempt-input-builder.js's buildAttemptLoopInput sets that budget from AmsPolicySpec.capLimits -- the same cap ceilings the Governor's cross-cycle GovernorCapUsage already enforces after the fact (loop-cli.js's saveCapUsage between loop cycles), so one runaway attempt can no longer burn through the whole cross-cycle budget before anything reacts. - tokens stays an honest 0: no CodingAgentDriver reports a real per-iteration token count today. - Split runIterateLoop into a thin wrapper over an internal core so the new finalMeterTotals/budgetBreaches fields attach once at a single always-reached return point, rather than threading them onto the core's own genuinely-unreachable v8-ignored fallback branch (which vitest's ignore-comment doesn't suppress from Codecov's raw patch view). --- .../src/miner/iterate-loop.ts | 96 ++++++++++++++++--- .../test/iterate-loop.test.ts | 61 +++++++++++- .../docs/coding-agent-driver.md | 21 ++-- .../lib/attempt-input-builder.js | 10 ++ test/unit/miner-attempt-input-builder.test.ts | 20 ++++ 5 files changed, 181 insertions(+), 27 deletions(-) diff --git a/packages/gittensory-engine/src/miner/iterate-loop.ts b/packages/gittensory-engine/src/miner/iterate-loop.ts index 6b9ec75d7..1ecb75758 100644 --- a/packages/gittensory-engine/src/miner/iterate-loop.ts +++ b/packages/gittensory-engine/src/miner/iterate-loop.ts @@ -15,10 +15,12 @@ // precedence then abandons rather than optimistically continuing or handing off. The loop never fabricates a // "pass" from anything other than a genuinely successful `runSelfReview` call. // -// BOUNDED INSIDE THE LOOP: both the iteration ceiling (`input.maxIterations`) and the optional cumulative-cost -// ceiling (`input.maxTotalTurns`, summing every iteration's `turnsUsed`) are enforced here every iteration -- -// not left to an external caller to remember. A `maxIterations <= 0` input abandons immediately, before ever -// invoking the driver. +// BOUNDED INSIDE THE LOOP: both the iteration ceiling (`input.maxIterations`) and the optional cumulative +// budget (`input.budget`, evaluated every iteration via attempt-metering.ts's `accumulateAttemptUsage`/ +// `evaluateAttemptBudget` against real per-iteration turns/costUsd/wallClockMs -- tokens stays an honest 0, +// no driver reports a real token count today, #5395) are enforced here every iteration -- not left to an +// external caller to remember, and not just capped after the fact between loop cycles (loop-cli.js's own +// governor cap usage). A `maxIterations <= 0` input abandons immediately, before ever invoking the driver. // // AUDITABLE: every iteration's decision (continue / handoff / abandon) is recorded via the injected // `appendAttemptLogEvent` dependency (attempt-log.ts's normalized event shape) before this function returns @@ -32,6 +34,7 @@ import { invokeCodingAgentDriver } from "./coding-agent-invoke.js"; import type { AttemptLogEvent, AttemptLogEventType } from "./attempt-log.js"; import { runSelfReview, type AttemptDiffState, type SelfReviewAdapterDeps, type SelfReviewContext, type SelfReviewVerdict } from "./self-review-adapter.js"; import { decideNextActionWithReason, deriveSelfReviewOutcome, type IterateLoopDecision, type HandoffPacket, type IterationState, type SelfReviewOutcome } from "./iterate-policy.js"; +import { accumulateAttemptUsage, evaluateAttemptBudget, type AttemptBudget, type AttemptBudgetAxis, type AttemptMeterTotals } from "./attempt-metering.js"; /** Everything one call to {@link runIterateLoop} needs, aside from the injected {@link IterateLoopDeps}. * Identity/context fields mirror self-review-adapter.ts's `AttemptDiffState`/`SelfReviewContext` exactly -- @@ -51,10 +54,12 @@ export type IterateLoopInput = { maxIterations: number; /** Per-iteration turn budget, passed through to each `CodingAgentDriverTask`. */ maxTurnsPerIteration: number; - /** Optional hard ceiling on CUMULATIVE turns spent across every iteration of this attempt so far (summed - * from each iteration's `CodingAgentDriverResult.turnsUsed`). Omitted means no additional cost ceiling - * beyond what `maxIterations * maxTurnsPerIteration` already implies. */ - maxTotalTurns?: number | undefined; + /** Optional cumulative budget ceiling(s), evaluated every iteration against the real running totals + * (attempt-metering.ts's `AttemptMeterTotals`, #5395). Omitted means no additional ceiling beyond what + * `maxIterations * maxTurnsPerIteration` already implies. Any breached axis (turns/costUsd/wallClockMs -- + * tokens is never real-tracked today) abandons via the SAME `costCeilingReached` policy signal + * iterate-policy.ts already exposes; that policy has no notion of which axis, only "cost ceiling reached". */ + budget?: AttemptBudget | undefined; // Self-review identity fields -- mirror `AttemptDiffState`'s own identity fields (self-review-adapter.ts). repoFullName: string; @@ -82,6 +87,10 @@ export type IterateLoopDeps = { driver: CodingAgentDriver; runSlopAssessment: SelfReviewAdapterDeps["runSlopAssessment"]; appendAttemptLogEvent: (event: AttemptLogEvent) => void; + /** Injected clock for real per-iteration wall-clock measurement (#5395), mirroring this package's own + * injected-dependency discipline elsewhere (never a hardcoded `Date.now()` a test can't control). Defaults + * to the real `Date.now` when omitted. */ + nowMs?: (() => number) | undefined; }; /** The terminal outcomes a full loop run can end in -- never `"continue"`, which is only ever a per-iteration, @@ -106,6 +115,14 @@ export type IterateLoopResult = { * `CodingAgentDriverResult.costUsd`. Only the `agent-sdk` provider reports this today (the CLI-subprocess * providers report no cost signal) -- always `0` for a provider that never reports one, never fabricated. */ totalCostUsd: number; + /** The real accumulated {@link AttemptMeterTotals} across every iteration that ran (attempt-metering.ts, + * #5395) -- a superset of `totalTurnsUsed`/`totalCostUsd` above that also carries `wallClockMs` (real, + * measured around each driver invocation) and `tokens` (always 0 today -- no driver reports a real token + * count, an honest absence rather than a fabricated number). */ + finalMeterTotals: AttemptMeterTotals; + /** The budget axes breached at the point this attempt abandoned, when `input.budget` was set and at least + * one axis was at/over its ceiling -- empty when no budget was configured or none breached. */ + budgetBreaches: AttemptBudgetAxis[]; iterations: readonly IterateLoopIterationRecord[]; /** Populated only when `outcome === "handoff"`. */ handoffPacket?: HandoffPacket | undefined; @@ -180,7 +197,13 @@ function safeAppendAttemptLogEvent(deps: IterateLoopDeps, event: AttemptLogEvent } } -function logDecision(input: IterateLoopInput, deps: IterateLoopDeps, iterationNumber: number, decision: IterateLoopDecision): void { +function logDecision( + input: IterateLoopInput, + deps: IterateLoopDeps, + iterationNumber: number, + decision: IterateLoopDecision, + budgetBreaches: readonly AttemptBudgetAxis[], +): void { safeAppendAttemptLogEvent(deps, { eventType: attemptLogEventTypeForDecision(decision), attemptId: input.attemptId, @@ -191,6 +214,10 @@ function logDecision(input: IterateLoopInput, deps: IterateLoopDeps, iterationNu iterationNumber, action: decision.action, ...(decision.abandonReason !== undefined ? { abandonReason: decision.abandonReason } : {}), + // Which real axis (or axes) breached, when this decision was a budget-driven abandon (#5395) -- the + // policy's own `costCeilingReached` is a single boolean with no notion of which axis, so this is the + // only place that detail survives for an operator reading the attempt log back. + ...(budgetBreaches.length > 0 ? { budgetBreaches } : {}), }, }); } @@ -218,7 +245,23 @@ function buildHandoffPacket(input: IterateLoopInput, verdict: SelfReviewVerdict, }; } -function immediateAbandonNoIterationsPermitted(input: IterateLoopInput, deps: IterateLoopDeps): IterateLoopResult { +const ZERO_METER_TOTALS: AttemptMeterTotals = { tokens: 0, turns: 0, wallClockMs: 0, costUsd: 0 }; + +/** The result shape {@link runIterateLoopCore} itself returns -- everything BUT the meter fields, which the + * thin {@link runIterateLoop} wrapper attaches once, from `tracker`, at its own single always-reached return + * point (#5395). Keeps the core's own internal return statements -- including the `/* v8 ignore *\/`-guarded + * unreachable fallback -- byte-identical to their pre-#5395 shape, so that genuinely unreachable branch never + * needs new fields threaded onto it (v8's ignore-comment suppresses vitest's OWN text-reporter percentage, + * but NOT the raw lcov Codecov reads -- a new field on that branch would show as a real uncovered patch line + * with no way to actually exercise it). */ +type IterateLoopCoreResult = Omit; + +/** Mutable accumulator threaded into {@link runIterateLoopCore} so the wrapper can read the real running + * totals/breaches after the core returns, without the core itself needing to carry them on every return + * statement. */ +type MeterTracker = { totals: AttemptMeterTotals; breaches: AttemptBudgetAxis[] }; + +function immediateAbandonNoIterationsPermitted(input: IterateLoopInput, deps: IterateLoopDeps): IterateLoopCoreResult { const decision: IterateLoopDecision = { action: "abandon", abandonReason: "max_iterations_reached", @@ -242,9 +285,10 @@ function immediateAbandonNoIterationsPermitted(input: IterateLoopInput, deps: It * Every iteration: invoke the driver, self-review the resulting diff (never fabricating a pass from a failed * or errored driver/self-review run), consult the policy with the running iteration/cost/no-progress state, * and record the decision via the attempt-log. `"continue"` decisions loop again; `"handoff"`/`"abandon"` - * return immediately. + * return immediately. See {@link IterateLoopCoreResult}'s own doc comment for why this doesn't carry + * `finalMeterTotals`/`budgetBreaches` itself -- {@link runIterateLoop} attaches those. */ -export async function runIterateLoop(input: IterateLoopInput, deps: IterateLoopDeps): Promise { +async function runIterateLoopCore(input: IterateLoopInput, deps: IterateLoopDeps, tracker: MeterTracker): Promise { // Truncated toward zero rather than used as-is: a fractional maxIterations (a caller bug -- "how many times // to run a coding agent" has no fractional meaning) would otherwise let this loop's own `for` bound and // iterate-policy.ts's `iterationNumber >= maxIterations` ceiling check disagree by less than one iteration @@ -263,12 +307,14 @@ export async function runIterateLoop(input: IterateLoopInput, deps: IterateLoopD payload: { maxIterations, maxTurnsPerIteration: input.maxTurnsPerIteration }, }); + const nowMs = deps.nowMs ?? Date.now; const iterations: IterateLoopIterationRecord[] = []; let previousBlockerCodes: readonly string[] | null = null; let totalTurnsUsed = 0; let totalCostUsd = 0; for (let iterationNumber = 1; iterationNumber <= maxIterations; iterationNumber += 1) { + const iterationStartMs = nowMs(); const driverResult = await runDriverSafely(input, deps, { attemptId: input.attemptId, workingDirectory: input.workingDirectory, @@ -276,21 +322,32 @@ export async function runIterateLoop(input: IterateLoopInput, deps: IterateLoopD instructions: input.instructions, maxTurns: input.maxTurnsPerIteration, }); + const iterationElapsedMs = Math.max(0, nowMs() - iterationStartMs); totalTurnsUsed += driverResult.turnsUsed ?? 0; totalCostUsd += driverResult.costUsd ?? 0; + // tokens stays an honest 0: no CodingAgentDriver reports a real per-iteration token count today (#5395) -- + // an absence, never a fabricated number, matching this package's costUsd discipline elsewhere. + tracker.totals = accumulateAttemptUsage(tracker.totals, { + tokens: 0, + turns: driverResult.turnsUsed ?? 0, + wallClockMs: iterationElapsedMs, + costUsd: driverResult.costUsd ?? 0, + }); + const budgetVerdict = input.budget !== undefined ? evaluateAttemptBudget(tracker.totals, input.budget) : undefined; + tracker.breaches = budgetVerdict?.breaches ?? []; const { outcome: selfReview, verdict } = evaluateSelfReviewOutcome(input, driverResult, deps); const state: IterationState = { iterationNumber, maxIterations, - costCeilingReached: input.maxTotalTurns !== undefined && totalTurnsUsed >= input.maxTotalTurns, + costCeilingReached: budgetVerdict !== undefined && !budgetVerdict.withinBudget, selfReview, previousBlockerCodes, rejectionSignaled: input.rejectionSignaled, }; const decision = decideNextActionWithReason(state); - logDecision(input, deps, iterationNumber, decision); + logDecision(input, deps, iterationNumber, decision, decision.abandonReason === "cost_ceiling_reached" ? tracker.breaches : []); iterations.push({ iterationNumber, driverResult, decision }); if (decision.action === "handoff") { @@ -322,3 +379,14 @@ export async function runIterateLoop(input: IterateLoopInput, deps: IterateLoopD const fallbackDecision: IterateLoopDecision = { action: "abandon", abandonReason: "max_iterations_reached", reason: "Iterate loop exhausted its iteration budget." }; return { outcome: "abandon", finalDecision: fallbackDecision, iterationsUsed: maxIterations, totalTurnsUsed, totalCostUsd, iterations }; } + +/** + * Thin wrapper over {@link runIterateLoopCore} that attaches the real accumulated + * {@link AttemptMeterTotals}/breached axes (#5395) once, at this single always-reached return point -- see + * {@link IterateLoopCoreResult}'s own doc comment for why the core itself doesn't carry these fields. + */ +export async function runIterateLoop(input: IterateLoopInput, deps: IterateLoopDeps): Promise { + const tracker: MeterTracker = { totals: ZERO_METER_TOTALS, breaches: [] }; + const core = await runIterateLoopCore(input, deps, tracker); + return { ...core, finalMeterTotals: tracker.totals, budgetBreaches: tracker.breaches }; +} diff --git a/packages/gittensory-engine/test/iterate-loop.test.ts b/packages/gittensory-engine/test/iterate-loop.test.ts index 13d1189b5..63e2e1a48 100644 --- a/packages/gittensory-engine/test/iterate-loop.test.ts +++ b/packages/gittensory-engine/test/iterate-loop.test.ts @@ -239,20 +239,71 @@ test("a fractional maxIterations truncates toward the lower integer, not silentl assert.equal(result.iterationsUsed, 1); }); -test("abandon (cost_ceiling_reached): the cumulative-turns ceiling stops the loop even with iterations still available", async () => { +test("abandon (cost_ceiling_reached): a maxTurns budget breach stops the loop even with iterations still available", async () => { const pullRequests: PullRequestRecord[] = [openPr(42, "Retry uploads on 5xx responses", [7])]; const { deps } = collectingDeps({ driver: driverReturning(okResult(["src/upload.ts"], 50)) }); - const input = passingInput({ maxIterations: 10, maxTotalTurns: 20, reviewContext: baseReviewContext({ pullRequests }) }); + const input = passingInput({ maxIterations: 10, budget: { maxTurns: 20 }, reviewContext: baseReviewContext({ pullRequests }) }); const result = await runIterateLoop(input, deps); assert.equal(result.outcome, "abandon"); assert.equal(result.finalDecision.abandonReason, "cost_ceiling_reached"); assert.equal(result.iterationsUsed, 1); + assert.deepEqual(result.budgetBreaches, ["turns"]); + assert.equal(result.finalMeterTotals.turns, 50); }); -test("continue: maxTotalTurns omitted never trips the cost ceiling, regardless of turns spent", async () => { - const { deps } = collectingDeps({ driver: driverReturning(okResult(["src/upload.ts"], 1_000_000)) }); - const result = await runIterateLoop(passingInput({ maxTotalTurns: undefined }), deps); +test("abandon (cost_ceiling_reached): a maxCostUsd budget breach reports costUsd as the breached axis", async () => { + const pullRequests: PullRequestRecord[] = [openPr(42, "Retry uploads on 5xx responses", [7])]; + const { deps } = collectingDeps({ driver: driverReturning({ ok: true, changedFiles: ["src/upload.ts"], summary: "x", turnsUsed: 1, costUsd: 6 }) }); + const input = passingInput({ maxIterations: 10, budget: { maxCostUsd: 5 }, reviewContext: baseReviewContext({ pullRequests }) }); + const result = await runIterateLoop(input, deps); + + assert.equal(result.outcome, "abandon"); + assert.equal(result.finalDecision.abandonReason, "cost_ceiling_reached"); + assert.deepEqual(result.budgetBreaches, ["costUsd"]); + assert.equal(result.finalMeterTotals.costUsd, 6); +}); + +test("abandon (cost_ceiling_reached): a maxWallClockMs budget breach uses the real injected clock, not a fabricated duration", async () => { + const pullRequests: PullRequestRecord[] = [openPr(42, "Retry uploads on 5xx responses", [7])]; + let call = 0; + const timestamps = [1_000, 1_000 + 90_000]; // 90s elapsed on the one iteration + const { deps } = collectingDeps({ driver: driverReturning(okResult(["src/upload.ts"], 1)) }); + const input = passingInput({ maxIterations: 10, budget: { maxWallClockMs: 60_000 }, reviewContext: baseReviewContext({ pullRequests }) }); + const result = await runIterateLoop(input, { + ...deps, + nowMs: () => timestamps[call++] ?? timestamps[timestamps.length - 1]!, + }); + + assert.equal(result.outcome, "abandon"); + assert.equal(result.finalDecision.abandonReason, "cost_ceiling_reached"); + assert.deepEqual(result.budgetBreaches, ["wallClockMs"]); + assert.equal(result.finalMeterTotals.wallClockMs, 90_000); +}); + +test("continue: budget omitted never trips the cost ceiling, regardless of turns/cost spent", async () => { + const { deps } = collectingDeps({ driver: driverReturning({ ok: true, changedFiles: ["src/upload.ts"], summary: "x", turnsUsed: 1_000_000, costUsd: 999 }) }); + const result = await runIterateLoop(passingInput({ budget: undefined }), deps); + assert.equal(result.outcome, "handoff"); + assert.deepEqual(result.budgetBreaches, []); +}); + +test("finalMeterTotals.tokens is always an honest 0 -- no driver reports a real token count", async () => { + const { deps } = collectingDeps({ driver: driverReturning(okResult()) }); + const result = await runIterateLoop(passingInput(), deps); + assert.equal(result.finalMeterTotals.tokens, 0); +}); + +test("immediate abandon: finalMeterTotals/budgetBreaches are the honest zero/empty shape when the driver never ran", async () => { + const { deps } = collectingDeps(); + const result = await runIterateLoop(baseInput({ maxIterations: 0 }), deps); + assert.deepEqual(result.finalMeterTotals, { tokens: 0, turns: 0, wallClockMs: 0, costUsd: 0 }); + assert.deepEqual(result.budgetBreaches, []); +}); + +test("REGRESSION: a passing self-review on the same iteration that breaches budget still hands off (pass wins the precedence ladder)", async () => { + const { deps } = collectingDeps({ driver: driverReturning(okResult(["src/upload.ts"], 999)) }); + const result = await runIterateLoop(passingInput({ maxIterations: 1, budget: { maxTurns: 1 } }), deps); assert.equal(result.outcome, "handoff"); }); diff --git a/packages/gittensory-miner/docs/coding-agent-driver.md b/packages/gittensory-miner/docs/coding-agent-driver.md index 67486cfa5..702f42c52 100644 --- a/packages/gittensory-miner/docs/coding-agent-driver.md +++ b/packages/gittensory-miner/docs/coding-agent-driver.md @@ -117,14 +117,19 @@ path with house-rule enforcement built in by default -- no such caller exists in real house-rule enforcement for the live `agent-sdk` provider happens via `buildHouseRulesAgentSdkHooks`, attached directly in `constructProductionCodingAgentDriver`. -**Metering:** `attempt-metering.ts`'s `accumulateAttemptUsage`/`meterAttemptUsage`/`evaluateAttemptBudget` -(tokens/turns/wallClockMs/costUsd, with mid-attempt budget-abort) have **zero production callers** as of this -writing -- see [#5395](https://github.com/JSONbored/gittensory/issues/5395) for the tracked decision on -whether to wire them in for real or remove them. What IS real today: `iterate-loop.ts` sums each iteration's -real `driverResult.turnsUsed`/`costUsd` into `IterateLoopResult.totalTurnsUsed`/`totalCostUsd`, which -`packages/gittensory-miner/lib/loop-cli.js` uses to increment the governor's persisted `GovernorCapUsage` -between loop cycles -- an after-the-fact, cross-cycle total, not the per-iteration mid-attempt abort -`attempt-metering.ts` was designed to provide. +**Metering:** `attempt-metering.ts`'s `accumulateAttemptUsage`/`evaluateAttemptBudget` are wired into +`iterate-loop.ts` for real ([#5395](https://github.com/JSONbored/gittensory/issues/5395)) -- every iteration +accumulates real `turns`/`costUsd`/`wallClockMs` (`tokens` stays an honest 0; no driver reports a real +per-iteration token count today) into a running `AttemptMeterTotals`, and `runIterateLoop`'s optional +`input.budget: AttemptBudget` is evaluated against it each iteration via the SAME `costCeilingReached` signal +`iterate-policy.ts` already exposed for the (now-removed) turns-only `maxTotalTurns` ceiling -- a genuine +mid-attempt abort, not just a between-cycle cap. `packages/gittensory-miner/lib/attempt-input-builder.js`'s +`buildAttemptLoopInput` sets `budget` from the SAME `AmsPolicySpec.capLimits` the Governor's cross-cycle +`GovernorCapUsage` already uses (`packages/gittensory-miner/lib/loop-cli.js`'s `governorState.saveCapUsage` +between loop cycles) -- one attempt can no longer burn through the entire cross-cycle budget before anything +reacts. `wallClockMs` uses a real injected clock (`IterateLoopDeps.nowMs`, defaulting to `Date.now`) measured +around each iteration's driver invocation. The result's `finalMeterTotals`/`budgetBreaches` fields surface +which axis (if any) triggered an abandon, for an operator reading the attempt log back. ## Related docs diff --git a/packages/gittensory-miner/lib/attempt-input-builder.js b/packages/gittensory-miner/lib/attempt-input-builder.js index 58fd1d222..233fef8b4 100644 --- a/packages/gittensory-miner/lib/attempt-input-builder.js +++ b/packages/gittensory-miner/lib/attempt-input-builder.js @@ -68,6 +68,16 @@ export function buildAttemptLoopInput(input) { mode: input.mode, maxIterations: input.amsPolicySpec.maxIterations, maxTurnsPerIteration: input.amsPolicySpec.maxTurnsPerIteration, + // Real mid-attempt budget (#5395): the SAME Governor cap ceilings that already bound cross-cycle spend + // (loop-cli.js's after-the-fact governorState.saveCapUsage) now also bound this ONE attempt in progress, + // via the engine's real accumulateAttemptUsage/evaluateAttemptBudget -- a runaway attempt can no longer + // burn through the entire cross-cycle budget before anything reacts. No maxTokens: no driver reports a + // real per-iteration token count today, so that axis has no real ceiling to set (never fabricated). + budget: { + maxTurns: input.amsPolicySpec.capLimits.turns, + maxWallClockMs: input.amsPolicySpec.capLimits.elapsedMs, + maxCostUsd: input.amsPolicySpec.capLimits.budget, + }, repoFullName: input.repoFullName, contributorLogin: input.minerLogin, title: input.codingTaskSpec.title, diff --git a/test/unit/miner-attempt-input-builder.test.ts b/test/unit/miner-attempt-input-builder.test.ts index aadc19845..7c9f5b1fc 100644 --- a/test/unit/miner-attempt-input-builder.test.ts +++ b/test/unit/miner-attempt-input-builder.test.ts @@ -93,6 +93,11 @@ describe("buildAttemptLoopInput (#5132)", () => { mode: "dry_run", maxIterations: DEFAULT_AMS_POLICY_SPEC.maxIterations, maxTurnsPerIteration: DEFAULT_AMS_POLICY_SPEC.maxTurnsPerIteration, + budget: { + maxTurns: DEFAULT_AMS_POLICY_SPEC.capLimits.turns, + maxWallClockMs: DEFAULT_AMS_POLICY_SPEC.capLimits.elapsedMs, + maxCostUsd: DEFAULT_AMS_POLICY_SPEC.capLimits.budget, + }, repoFullName: "acme/widgets", contributorLogin: "alice", title: "Uploads should retry on 5xx", @@ -105,6 +110,21 @@ describe("buildAttemptLoopInput (#5132)", () => { }); }); + it("REGRESSION (#5395): budget mirrors AmsPolicySpec's real capLimits, not hardcoded literals", () => { + const loopInput = buildAttemptLoopInput({ + codingTaskSpec: codingTaskSpec(), + reviewContext: reviewContext(), + worktreePath: "/fake", + attemptId: "a1", + mode: "dry_run", + repoFullName: "acme/widgets", + minerLogin: "alice", + rejectionSignaled: false, + amsPolicySpec: { ...DEFAULT_AMS_POLICY_SPEC, capLimits: { budget: 9, turns: 8, elapsedMs: 7 } }, + }); + expect(loopInput.budget).toEqual({ maxTurns: 8, maxWallClockMs: 7, maxCostUsd: 9 }); + }); + it("threads a real rejectionSignaled:true through unchanged", () => { const loopInput = buildAttemptLoopInput({ codingTaskSpec: codingTaskSpec(), From 5e8ac5d3d6757ccdfc4e458ff33175a3f7d0041a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 13:10:03 -0700 Subject: [PATCH 2/3] fix(miner): a reached budget ceiling is a hard stop, not bypassable by a same-iteration pass Flagged by gittensory's own review on PR #5437: the previous design fed a budget breach into iterate-policy.ts's costCeilingReached signal, whose precedence checks selfReview.kind === "pass" first -- so an iteration that both breaches the ceiling and produces a clean self-review would hand off instead of abandoning, silently defeating the point of wiring a mid-attempt abort in at all. A reached budget ceiling is now checked and, if breached, acted on directly in iterate-loop.ts BEFORE self-review even runs -- self-review is skipped entirely once the ceiling is breached, since its verdict can no longer change an already-decided outcome. The spend/turns/time already happened either way; this only stops it from silently buying a pass on the SAME iteration that broke the ceiling. --- .../src/miner/iterate-loop.ts | 34 ++++++++++++++----- .../test/iterate-loop.test.ts | 31 +++++++++++------ test/unit/miner-attempt-runner.test.ts | 16 +++++++++ 3 files changed, 62 insertions(+), 19 deletions(-) diff --git a/packages/gittensory-engine/src/miner/iterate-loop.ts b/packages/gittensory-engine/src/miner/iterate-loop.ts index 1ecb75758..7961a45cb 100644 --- a/packages/gittensory-engine/src/miner/iterate-loop.ts +++ b/packages/gittensory-engine/src/miner/iterate-loop.ts @@ -56,9 +56,12 @@ export type IterateLoopInput = { maxTurnsPerIteration: number; /** Optional cumulative budget ceiling(s), evaluated every iteration against the real running totals * (attempt-metering.ts's `AttemptMeterTotals`, #5395). Omitted means no additional ceiling beyond what - * `maxIterations * maxTurnsPerIteration` already implies. Any breached axis (turns/costUsd/wallClockMs -- - * tokens is never real-tracked today) abandons via the SAME `costCeilingReached` policy signal - * iterate-policy.ts already exposes; that policy has no notion of which axis, only "cost ceiling reached". */ + * `maxIterations * maxTurnsPerIteration` already implies. A breach on any axis (turns/costUsd/wallClockMs + * -- tokens is never real-tracked today) is a HARD, unconditional stop: checked and abandoned on BEFORE + * self-review even runs, so a same-iteration pass can never bypass the ceiling (this is deliberately NOT + * routed through iterate-policy.ts's `costCeilingReached` field, whose own precedence checks a self-review + * pass first -- see the loop body's own comment at the check site for why that ordering doesn't fit a hard + * budget ceiling). */ budget?: AttemptBudget | undefined; // Self-review identity fields -- mirror `AttemptDiffState`'s own identity fields (self-review-adapter.ts). @@ -214,9 +217,9 @@ function logDecision( iterationNumber, action: decision.action, ...(decision.abandonReason !== undefined ? { abandonReason: decision.abandonReason } : {}), - // Which real axis (or axes) breached, when this decision was a budget-driven abandon (#5395) -- the - // policy's own `costCeilingReached` is a single boolean with no notion of which axis, so this is the - // only place that detail survives for an operator reading the attempt log back. + // Which real axis (or axes) breached, on the hard-budget-ceiling abandon path (#5395, checked directly + // in the loop body before self-review even runs -- see that check site's own comment) -- an operator + // reading the attempt log back otherwise has no way to see which axis actually tripped. ...(budgetBreaches.length > 0 ? { budgetBreaches } : {}), }, }); @@ -336,18 +339,33 @@ async function runIterateLoopCore(input: IterateLoopInput, deps: IterateLoopDeps const budgetVerdict = input.budget !== undefined ? evaluateAttemptBudget(tracker.totals, input.budget) : undefined; tracker.breaches = budgetVerdict?.breaches ?? []; + // A reached budget ceiling is a HARD, unconditional stop -- checked and, if breached, acted on BEFORE + // self-review even runs, so a same-iteration pass can never bypass it. The spend/turns/time on this + // iteration are sunk either way (the driver already ran), but handing off anyway would let a single + // over-budget iteration silently defeat the entire point of wiring a ceiling in (#5395's own mid-attempt + // abort goal) -- so this abandons regardless of what this iteration's own result looks like. + if (budgetVerdict !== undefined && !budgetVerdict.withinBudget) { + const decision: IterateLoopDecision = { + action: "abandon", + abandonReason: "cost_ceiling_reached", + reason: `Reached the attempt's budget ceiling (${tracker.breaches.join(", ")}) on iteration ${iterationNumber}; abandoning regardless of this iteration's own result.`, + }; + logDecision(input, deps, iterationNumber, decision, tracker.breaches); + iterations.push({ iterationNumber, driverResult, decision }); + return { outcome: "abandon", finalDecision: decision, iterationsUsed: iterationNumber, totalTurnsUsed, totalCostUsd, iterations }; + } + const { outcome: selfReview, verdict } = evaluateSelfReviewOutcome(input, driverResult, deps); const state: IterationState = { iterationNumber, maxIterations, - costCeilingReached: budgetVerdict !== undefined && !budgetVerdict.withinBudget, selfReview, previousBlockerCodes, rejectionSignaled: input.rejectionSignaled, }; const decision = decideNextActionWithReason(state); - logDecision(input, deps, iterationNumber, decision, decision.abandonReason === "cost_ceiling_reached" ? tracker.breaches : []); + logDecision(input, deps, iterationNumber, decision, []); iterations.push({ iterationNumber, driverResult, decision }); if (decision.action === "handoff") { diff --git a/packages/gittensory-engine/test/iterate-loop.test.ts b/packages/gittensory-engine/test/iterate-loop.test.ts index 63e2e1a48..26a933f0b 100644 --- a/packages/gittensory-engine/test/iterate-loop.test.ts +++ b/packages/gittensory-engine/test/iterate-loop.test.ts @@ -239,10 +239,9 @@ test("a fractional maxIterations truncates toward the lower integer, not silentl assert.equal(result.iterationsUsed, 1); }); -test("abandon (cost_ceiling_reached): a maxTurns budget breach stops the loop even with iterations still available", async () => { - const pullRequests: PullRequestRecord[] = [openPr(42, "Retry uploads on 5xx responses", [7])]; +test("abandon (cost_ceiling_reached): a maxTurns budget breach stops the loop even with iterations still available AND a driver result that would otherwise pass self-review", async () => { const { deps } = collectingDeps({ driver: driverReturning(okResult(["src/upload.ts"], 50)) }); - const input = passingInput({ maxIterations: 10, budget: { maxTurns: 20 }, reviewContext: baseReviewContext({ pullRequests }) }); + const input = passingInput({ maxIterations: 10, budget: { maxTurns: 20 } }); const result = await runIterateLoop(input, deps); assert.equal(result.outcome, "abandon"); @@ -253,9 +252,8 @@ test("abandon (cost_ceiling_reached): a maxTurns budget breach stops the loop ev }); test("abandon (cost_ceiling_reached): a maxCostUsd budget breach reports costUsd as the breached axis", async () => { - const pullRequests: PullRequestRecord[] = [openPr(42, "Retry uploads on 5xx responses", [7])]; const { deps } = collectingDeps({ driver: driverReturning({ ok: true, changedFiles: ["src/upload.ts"], summary: "x", turnsUsed: 1, costUsd: 6 }) }); - const input = passingInput({ maxIterations: 10, budget: { maxCostUsd: 5 }, reviewContext: baseReviewContext({ pullRequests }) }); + const input = passingInput({ maxIterations: 10, budget: { maxCostUsd: 5 } }); const result = await runIterateLoop(input, deps); assert.equal(result.outcome, "abandon"); @@ -265,11 +263,10 @@ test("abandon (cost_ceiling_reached): a maxCostUsd budget breach reports costUsd }); test("abandon (cost_ceiling_reached): a maxWallClockMs budget breach uses the real injected clock, not a fabricated duration", async () => { - const pullRequests: PullRequestRecord[] = [openPr(42, "Retry uploads on 5xx responses", [7])]; let call = 0; const timestamps = [1_000, 1_000 + 90_000]; // 90s elapsed on the one iteration const { deps } = collectingDeps({ driver: driverReturning(okResult(["src/upload.ts"], 1)) }); - const input = passingInput({ maxIterations: 10, budget: { maxWallClockMs: 60_000 }, reviewContext: baseReviewContext({ pullRequests }) }); + const input = passingInput({ maxIterations: 10, budget: { maxWallClockMs: 60_000 } }); const result = await runIterateLoop(input, { ...deps, nowMs: () => timestamps[call++] ?? timestamps[timestamps.length - 1]!, @@ -301,10 +298,22 @@ test("immediate abandon: finalMeterTotals/budgetBreaches are the honest zero/emp assert.deepEqual(result.budgetBreaches, []); }); -test("REGRESSION: a passing self-review on the same iteration that breaches budget still hands off (pass wins the precedence ladder)", async () => { - const { deps } = collectingDeps({ driver: driverReturning(okResult(["src/upload.ts"], 999)) }); - const result = await runIterateLoop(passingInput({ maxIterations: 1, budget: { maxTurns: 1 } }), deps); - assert.equal(result.outcome, "handoff"); +test("REGRESSION: a hard budget ceiling abandons even on the same iteration a passing self-review would have produced (ceiling wins, not pass) -- gittensory review #5437", async () => { + let selfReviewRan = false; + const { deps } = collectingDeps({ + driver: driverReturning(okResult(["src/upload.ts"], 999)), + runSlopAssessment: () => { + selfReviewRan = true; + return noopSlop; + }, + }); + const result = await runIterateLoop(passingInput({ maxIterations: 5, budget: { maxTurns: 1 } }), deps); + assert.equal(result.outcome, "abandon"); + assert.equal(result.finalDecision.abandonReason, "cost_ceiling_reached"); + assert.deepEqual(result.budgetBreaches, ["turns"]); + // Self-review is skipped entirely once the ceiling is breached -- its verdict can never change an + // already-decided outcome, so there's no reason to spend the (cheap, local) computation. + assert.equal(selfReviewRan, false); }); test("abandon (rejection_signaled): wins even over a self-review that would otherwise cleanly pass", async () => { diff --git a/test/unit/miner-attempt-runner.test.ts b/test/unit/miner-attempt-runner.test.ts index 141d75d2a..29ea92651 100644 --- a/test/unit/miner-attempt-runner.test.ts +++ b/test/unit/miner-attempt-runner.test.ts @@ -182,6 +182,22 @@ describe("runMinerAttempt (#2337) — the real create->review->gate->submit pipe expect(claimLedgerListClaims).not.toHaveBeenCalled(); }); + it("REGRESSION (#5395, gittensory review #5437): a real hard budget-ceiling breach abandons through the full real runMinerAttempt pipeline, even though the driver's own result would otherwise pass self-review", async () => { + const claimLedgerListClaims = vi.fn(); + const deps = baseDeps({ claimLedger: { listClaims: claimLedgerListClaims } }); + // okDriverResult()'s default turnsUsed (5) immediately breaches this maxTurns:1 ceiling on iteration 1, + // even though the SAME driver result is exactly what the happy-path test above uses to reach a real + // "submitted" outcome -- proving the ceiling wins over a same-iteration pass, not just in isolation. + const result = await runMinerAttempt(baseAttemptInput({ loopInput: passingLoopInput({ budget: { maxTurns: 1 } }) }), deps); + + expect(result.outcome).toBe("abandon"); + expect(result.loopResult.finalDecision.abandonReason).toBe("cost_ceiling_reached"); + expect(result.loopResult.budgetBreaches).toEqual(["turns"]); + expect(result.loopResult.iterationsUsed).toBe(1); + // No downstream gate consulted -- the same real short-circuit as the maxIterations:0 case above. + expect(claimLedgerListClaims).not.toHaveBeenCalled(); + }); + it("abandon mid-loop: a real iteration ran (and was billed) before the ceiling forced abandon", async () => { // maxIterations: 1 with a driver that never produces a passing self-review runs ONE real iteration, then // decideNextActionWithReason's own iterationNumber>=maxIterations check abandons -- a genuinely different From a760181fa46f4ef2122419f4da153ff3af39a525 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 13:24:00 -0700 Subject: [PATCH 3/3] test(miner): cover the within-budget branch of the hard budget-ceiling check Codecov flagged 2 partial branches on the ceiling check added in the prior commit: every existing test either omitted budget entirely or set one that immediately breached, so the "budget configured but comfortably within limits" branch never ran. Adds that case at both the engine's own iterate-loop level and through the real runMinerAttempt pipeline. --- packages/gittensory-engine/test/iterate-loop.test.ts | 8 ++++++++ test/unit/miner-attempt-runner.test.ts | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/packages/gittensory-engine/test/iterate-loop.test.ts b/packages/gittensory-engine/test/iterate-loop.test.ts index 26a933f0b..28076271f 100644 --- a/packages/gittensory-engine/test/iterate-loop.test.ts +++ b/packages/gittensory-engine/test/iterate-loop.test.ts @@ -285,6 +285,14 @@ test("continue: budget omitted never trips the cost ceiling, regardless of turns assert.deepEqual(result.budgetBreaches, []); }); +test("handoff: a budget that IS configured but comfortably within limits never trips the ceiling", async () => { + const { deps } = collectingDeps({ driver: driverReturning(okResult(["src/upload.ts"], 5)) }); + const result = await runIterateLoop(passingInput({ budget: { maxTurns: 20, maxCostUsd: 5, maxWallClockMs: 60_000 } }), deps); + assert.equal(result.outcome, "handoff"); + assert.deepEqual(result.budgetBreaches, []); + assert.equal(result.finalMeterTotals.turns, 5); +}); + test("finalMeterTotals.tokens is always an honest 0 -- no driver reports a real token count", async () => { const { deps } = collectingDeps({ driver: driverReturning(okResult()) }); const result = await runIterateLoop(passingInput(), deps); diff --git a/test/unit/miner-attempt-runner.test.ts b/test/unit/miner-attempt-runner.test.ts index 29ea92651..ab3b1314c 100644 --- a/test/unit/miner-attempt-runner.test.ts +++ b/test/unit/miner-attempt-runner.test.ts @@ -198,6 +198,16 @@ describe("runMinerAttempt (#2337) — the real create->review->gate->submit pipe expect(claimLedgerListClaims).not.toHaveBeenCalled(); }); + it("a budget that IS configured but comfortably within limits never trips the ceiling -- real pipeline reaches submitted", async () => { + const deps = baseDeps(); + const result = await runMinerAttempt(baseAttemptInput({ loopInput: passingLoopInput({ budget: { maxTurns: 20, maxCostUsd: 5, maxWallClockMs: 60_000 } }) }), deps); + + expect(result.outcome).toBe("submitted"); + if (result.outcome !== "submitted") throw new Error("expected submitted"); + expect(result.loopResult.outcome).toBe("handoff"); + expect(result.loopResult.budgetBreaches).toEqual([]); + }); + it("abandon mid-loop: a real iteration ran (and was billed) before the ceiling forced abandon", async () => { // maxIterations: 1 with a driver that never produces a passing self-review runs ONE real iteration, then // decideNextActionWithReason's own iterationNumber>=maxIterations check abandons -- a genuinely different