From 5e580d8b337604581b333c7b007d84f42c508488 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 15 Jul 2026 18:09:33 +1000 Subject: [PATCH] fix(engine): reject non-finite agent-sdk usage so it can't crash the iterate loop (#5827) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit agent-sdk-driver.ts extracted num_turns/total_cost_usd (and the usage token fields) with a bare `typeof === "number"` check, so a malformed Agent SDK result message (num_turns: -1, NaN, Infinity) passed an out-of-contract value straight through. That value fed accumulateAttemptUsage in iterate-loop.ts's runIterateLoopCore, which deliberately throws a RangeError on negative/non-finite input — and that call sits outside the loop's driver/self-review try/catch, so the throw rejected runIterateLoopCore before any decision was logged, violating the loop's "every iteration's decision is recorded before returning" guarantee. Harden the driver's usage extraction with a finiteNonNegativeNumber helper mirroring cli-subprocess-driver.ts's existing one — an invalid value degrades to undefined (the driver's "field absent" contract). Add a matching call-site clamp in runIterateLoopCore so no current or future driver can crash the loop instead of being governed. attempt-metering.ts's throw-on-invalid contract is unchanged; this only stops feeding it invalid input. Regression tests cover negative/NaN/Infinity num_turns, total_cost_usd, and token fields, and the loop completing (decision logged, poisoned axis clamped) rather than rejecting uncaught. Because Codecov measures the engine src via the vitest suite (not the engine's node --test suite), the driver + loop guards are covered from both test surfaces. --- .../src/miner/agent-sdk-driver.ts | 26 ++++-- .../loopover-engine/src/miner/iterate-loop.ts | 16 +++- .../test/agent-sdk-driver.test.ts | 41 +++++++++ .../loopover-engine/test/iterate-loop.test.ts | 31 +++++++ test/unit/agent-sdk-driver.test.ts | 37 ++++++++ test/unit/iterate-loop-usage-guard.test.ts | 86 +++++++++++++++++++ 6 files changed, 226 insertions(+), 11 deletions(-) create mode 100644 test/unit/iterate-loop-usage-guard.test.ts diff --git a/packages/loopover-engine/src/miner/agent-sdk-driver.ts b/packages/loopover-engine/src/miner/agent-sdk-driver.ts index 24efcac60c..7b2729c084 100644 --- a/packages/loopover-engine/src/miner/agent-sdk-driver.ts +++ b/packages/loopover-engine/src/miner/agent-sdk-driver.ts @@ -76,16 +76,25 @@ function asRecord(value: unknown): Record | null { return typeof value === "object" && value !== null ? (value as Record) : null; } +/** A finite, non-negative number, else undefined — mirrors `cli-subprocess-driver.ts`'s helper of the same name + * so both drivers reject the same out-of-contract usage values (`NaN`, `Infinity`, negatives) from untrusted + * driver output. A malformed value degrades to the driver's existing "field absent" contract (undefined) rather + * than propagating downstream, where `attempt-metering.ts`'s accumulateAttemptUsage would throw a RangeError and + * reject the whole iterate loop before its decision is ever logged (#5827). */ +function finiteNonNegativeNumber(value: unknown): number | undefined { + return typeof value === "number" && Number.isFinite(value) && value >= 0 ? value : undefined; +} + /** Real token count from the SDK's own result message (#5653). Both `SDKResultSuccess` and `SDKResultError` * declare `usage: NonNullableUsage` unconditionally -- present whenever a result message arrived at all, same * as `total_cost_usd`. `NonNullableUsage`'s `input_tokens`/`output_tokens` are themselves non-nullable numbers * once `usage` exists, but this driver reads `resultMessage` as a loosely-typed record (like every other field - * read here), so both are re-validated defensively rather than trusted from an untyped source. Returns - * undefined (never a fabricated 0) when `usage` is absent or malformed. */ + * read here), so both are re-validated defensively (finite + non-negative, #5827) rather than trusted from an + * untyped source. Returns undefined (never a fabricated 0) when `usage` is absent or malformed. */ function tokensFromResultMessage(resultMessage: Record | null): number | undefined { const usage = asRecord(resultMessage?.usage); - const inputTokens = typeof usage?.input_tokens === "number" ? usage.input_tokens : undefined; - const outputTokens = typeof usage?.output_tokens === "number" ? usage.output_tokens : undefined; + const inputTokens = finiteNonNegativeNumber(usage?.input_tokens); + const outputTokens = finiteNonNegativeNumber(usage?.output_tokens); if (inputTokens === undefined && outputTokens === undefined) return undefined; return (inputTokens ?? 0) + (outputTokens ?? 0); } @@ -175,13 +184,14 @@ export function createAgentSdkCodingAgentDriver( }; } - const turnsUsed = - typeof resultMessage?.num_turns === "number" ? resultMessage.num_turns : undefined; + // finiteNonNegativeNumber (not a bare typeof check): a malformed num_turns/total_cost_usd (NaN, Infinity, + // negative) must degrade to undefined here, or it reaches accumulateAttemptUsage unguarded and throws a + // RangeError that rejects runIterateLoopCore before any decision is logged (#5827). + const turnsUsed = finiteNonNegativeNumber(resultMessage?.num_turns); // Real dollar cost: the SDK's own SDKResultSuccess/SDKResultError message types both declare // `total_cost_usd: number` unconditionally -- present whenever a result message arrived at all, success // or not (the session was billed either way), absent only when the stream produced no result message. - const costUsd = - typeof resultMessage?.total_cost_usd === "number" ? resultMessage.total_cost_usd : undefined; + const costUsd = finiteNonNegativeNumber(resultMessage?.total_cost_usd); const tokensUsed = tokensFromResultMessage(resultMessage); const resultText = typeof resultMessage?.result === "string" ? redactSecrets(resultMessage.result) : ""; diff --git a/packages/loopover-engine/src/miner/iterate-loop.ts b/packages/loopover-engine/src/miner/iterate-loop.ts index 4df1f7cdcd..460d06a951 100644 --- a/packages/loopover-engine/src/miner/iterate-loop.ts +++ b/packages/loopover-engine/src/miner/iterate-loop.ts @@ -276,6 +276,16 @@ function logDecision( * respectively) before ever reaching its `"continue"` fallthrough, so `outcome.kind === "fail"` is guaranteed * whenever this is reached from the real call site below, not just the common case. */ +/** A finite, non-negative usage value, else 0. accumulateAttemptUsage (attempt-metering.ts) deliberately THROWS + * a RangeError on a negative/non-finite input to protect its own direct callers; this call site sits outside the + * loop's driver/self-review try/catch blocks, so an uncaught throw here would reject runIterateLoopCore before + * its decision is logged, violating the loop's "every iteration's decision is recorded before returning" contract + * (#5827). The Agent SDK driver now degrades bad usage fields to undefined at the source, but this call takes any + * driver's result — clamp here too so no current or future driver can crash the loop instead of being governed. */ +function finiteNonNegativeUsage(value: number | undefined): number { + return typeof value === "number" && Number.isFinite(value) && value >= 0 ? value : 0; +} + function blockerCodesFromContinuingOutcome(outcome: SelfReviewOutcome): readonly string[] { if (outcome.kind === "fail") return outcome.blockerCodes; /* v8 ignore next -- unreachable: see this function's own doc comment above. */ @@ -410,10 +420,10 @@ async function runIterateLoopCore(input: IterateLoopInput, deps: IterateLoopDeps // that reports one (Agent SDK's own result-message usage, or CLI JSON/JSONL stdout) -- 0 only when the // driver genuinely reports no token signal for this iteration, same honest-absence discipline as costUsd. tracker.totals = accumulateAttemptUsage(tracker.totals, { - tokens: driverResult.tokensUsed ?? 0, - turns: driverResult.turnsUsed ?? 0, + tokens: finiteNonNegativeUsage(driverResult.tokensUsed), + turns: finiteNonNegativeUsage(driverResult.turnsUsed), wallClockMs: iterationElapsedMs, - costUsd: driverResult.costUsd ?? 0, + costUsd: finiteNonNegativeUsage(driverResult.costUsd), }); const budgetVerdict = input.budget !== undefined ? evaluateAttemptBudget(tracker.totals, input.budget) : undefined; tracker.breaches = budgetVerdict?.breaches ?? []; diff --git a/packages/loopover-engine/test/agent-sdk-driver.test.ts b/packages/loopover-engine/test/agent-sdk-driver.test.ts index 16b61a7297..a1bdb61805 100644 --- a/packages/loopover-engine/test/agent-sdk-driver.test.ts +++ b/packages/loopover-engine/test/agent-sdk-driver.test.ts @@ -196,6 +196,47 @@ test("tokensUsed is undefined when usage exists but is malformed (not an object, assert.equal(nonNumericResult.tokensUsed, undefined); }); +// #5827: a malformed num_turns/total_cost_usd (negative, NaN, Infinity) must degrade to undefined — the same +// finite/non-negative discipline cli-subprocess-driver.ts applies — so it never reaches accumulateAttemptUsage +// (attempt-metering.ts), which throws a RangeError on such input and would reject the whole iterate loop. +test("turnsUsed degrades to undefined for a negative, NaN, or Infinity num_turns (#5827)", async () => { + for (const badTurns of [-1, Number.NaN, Number.POSITIVE_INFINITY]) { + const driver = driverWith({ + query: queryYielding([{ type: "result", subtype: "success", is_error: false, num_turns: badTurns, result: "done" }]), + }); + const result = await driver.run(task); + assert.equal(result.turnsUsed, undefined, `num_turns=${String(badTurns)} should degrade to undefined`); + } +}); + +test("costUsd degrades to undefined for a negative, NaN, or Infinity total_cost_usd (#5827)", async () => { + for (const badCost of [-0.5, Number.NaN, Number.POSITIVE_INFINITY]) { + const driver = driverWith({ + query: queryYielding([{ type: "result", subtype: "success", is_error: false, num_turns: 2, result: "done", total_cost_usd: badCost }]), + }); + const result = await driver.run(task); + assert.equal(result.costUsd, undefined, `total_cost_usd=${String(badCost)} should degrade to undefined`); + } +}); + +test("tokensUsed ignores a negative/NaN/Infinity usage field instead of poisoning the sum (#5827)", async () => { + // output_tokens is out-of-contract (negative); input_tokens is valid — the sum uses only the valid field. + const partiallyBad = driverWith({ + query: queryYielding([ + { type: "result", subtype: "success", is_error: false, num_turns: 2, result: "done", usage: { input_tokens: 100, output_tokens: -5 } }, + ]), + }); + assert.equal((await partiallyBad.run(task)).tokensUsed, 100); + + // Both fields out-of-contract → undefined (never a fabricated 0 or a NaN/Infinity sum). + const allBad = driverWith({ + query: queryYielding([ + { type: "result", subtype: "success", is_error: false, num_turns: 2, result: "done", usage: { input_tokens: Number.NaN, output_tokens: Number.POSITIVE_INFINITY } }, + ]), + }); + assert.equal((await allBad.run(task)).tokensUsed, undefined); +}); + test("tokensUsed sums whichever of input/output tokens IS a real number, when only input is present", async () => { const driver = driverWith({ query: queryYielding([ diff --git a/packages/loopover-engine/test/iterate-loop.test.ts b/packages/loopover-engine/test/iterate-loop.test.ts index 2007e32089..211db8f919 100644 --- a/packages/loopover-engine/test/iterate-loop.test.ts +++ b/packages/loopover-engine/test/iterate-loop.test.ts @@ -262,6 +262,37 @@ test("abandon (cost_ceiling_reached): a maxCostUsd budget breach reports costUsd assert.equal(result.finalMeterTotals.costUsd, 6); }); +// #5827: an out-of-contract usage value from a driver (negative/NaN/Infinity) must NOT let accumulateAttemptUsage +// throw a RangeError out of runIterateLoopCore before the iteration's decision is logged. The loop must still +// complete, record its decision, and clamp the poisoned axis to 0 rather than rejecting uncaught. +test("does not reject uncaught when a driver reports an out-of-contract usage value; still logs a decision (#5827)", async () => { + for (const badTurns of [-1, Number.NaN, Number.POSITIVE_INFINITY]) { + const { deps, events } = collectingDeps({ + driver: driverReturning({ ok: true, changedFiles: ["src/upload.ts"], summary: "x", turnsUsed: badTurns as number }), + }); + const result = await runIterateLoop(passingInput({ maxIterations: 3 }), deps); + + assert.equal(result.outcome, "handoff", `turnsUsed=${String(badTurns)} must not crash the loop`); + assert.ok( + events.some((event) => event.actionClass === "iterate_loop"), + "an iterate_loop decision must still be logged despite the out-of-contract usage value", + ); + // The poisoned axis is clamped to 0, not propagated as a negative/NaN/Infinity total. + assert.equal(result.finalMeterTotals.turns, 0); + } +}); + +test("clamps only the out-of-contract axis, preserving a valid axis on the same iteration (#5827)", async () => { + const { deps } = collectingDeps({ + driver: driverReturning({ ok: true, changedFiles: ["src/upload.ts"], summary: "x", turnsUsed: Number.NaN, costUsd: 0.02 }), + }); + const result = await runIterateLoop(passingInput({ maxIterations: 3 }), deps); + + assert.equal(result.outcome, "handoff"); + assert.equal(result.finalMeterTotals.turns, 0); // NaN clamped + assert.equal(result.finalMeterTotals.costUsd, 0.02); // valid axis preserved +}); + test("abandon (cost_ceiling_reached): a maxWallClockMs budget breach uses the real injected clock, not a fabricated duration", async () => { let call = 0; const timestamps = [1_000, 1_000 + 90_000]; // 90s elapsed on the one iteration diff --git a/test/unit/agent-sdk-driver.test.ts b/test/unit/agent-sdk-driver.test.ts index a4a8fa3583..763d4ce26c 100644 --- a/test/unit/agent-sdk-driver.test.ts +++ b/test/unit/agent-sdk-driver.test.ts @@ -236,6 +236,43 @@ describe("createAgentSdkCodingAgentDriver", () => { expect((await nonNumericFields.run(task)).tokensUsed).toBeUndefined(); }); + // #5827: a malformed num_turns/total_cost_usd (negative, NaN, Infinity) must degrade to undefined — the same + // finite/non-negative discipline cli-subprocess-driver.ts applies — so it never reaches accumulateAttemptUsage, + // which throws a RangeError on such input and would reject the whole iterate loop before its decision is logged. + it("turnsUsed degrades to undefined for a negative, NaN, or Infinity num_turns (#5827)", async () => { + for (const badTurns of [-1, Number.NaN, Number.POSITIVE_INFINITY]) { + const driver = driverWith({ + query: queryYielding([{ type: "result", subtype: "success", is_error: false, num_turns: badTurns, result: "done" }]), + }); + expect((await driver.run(task)).turnsUsed).toBeUndefined(); + } + }); + + it("costUsd degrades to undefined for a negative, NaN, or Infinity total_cost_usd (#5827)", async () => { + for (const badCost of [-0.5, Number.NaN, Number.POSITIVE_INFINITY]) { + const driver = driverWith({ + query: queryYielding([{ type: "result", subtype: "success", is_error: false, num_turns: 2, result: "done", total_cost_usd: badCost }]), + }); + expect((await driver.run(task)).costUsd).toBeUndefined(); + } + }); + + it("tokensUsed ignores a negative/NaN/Infinity usage field instead of poisoning the sum (#5827)", async () => { + const partiallyBad = driverWith({ + query: queryYielding([ + { type: "result", subtype: "success", is_error: false, num_turns: 2, result: "done", usage: { input_tokens: 100, output_tokens: -5 } }, + ]), + }); + expect((await partiallyBad.run(task)).tokensUsed).toBe(100); + + const allBad = driverWith({ + query: queryYielding([ + { type: "result", subtype: "success", is_error: false, num_turns: 2, result: "done", usage: { input_tokens: Number.NaN, output_tokens: Number.POSITIVE_INFINITY } }, + ]), + }); + expect((await allBad.run(task)).tokensUsed).toBeUndefined(); + }); + it("tokensUsed sums whichever of input/output tokens IS a real number, when only input is present", async () => { const driver = driverWith({ query: queryYielding([ diff --git a/test/unit/iterate-loop-usage-guard.test.ts b/test/unit/iterate-loop-usage-guard.test.ts new file mode 100644 index 0000000000..e4d361f6a3 --- /dev/null +++ b/test/unit/iterate-loop-usage-guard.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from "vitest"; + +import { + parseFocusManifest, + runIterateLoop, + type AttemptLogEvent, + type CodingAgentDriver, + type CodingAgentDriverResult, + type IterateLoopDeps, + type IterateLoopInput, + type SelfReviewContext, + type SelfReviewSlopAssessment, +} from "../../packages/loopover-engine/src/index"; + +// Codecov measures packages/loopover-engine/src/** via the vitest suite (NOT the engine's own node --test +// suite), so runIterateLoop's #5827 finiteNonNegativeUsage guard needs coverage here, importing from src. + +const REPO = { fullName: "acme/widgets", owner: "acme", name: "widgets", isInstalled: true, isRegistered: true, isPrivate: false }; +const noopSlop: SelfReviewSlopAssessment = { slopRisk: 0, band: "clean", findings: [] }; + +function baseReviewContext(): SelfReviewContext { + return { + manifest: parseFocusManifest({ gate: { duplicates: "block", linkedIssue: "advisory" } }), + repo: REPO, + issues: [{ repoFullName: "acme/widgets", number: 7, title: "Uploads should retry on 5xx", state: "open", labels: [], linkedPrs: [] }], + pullRequests: [], + }; +} + +function passingInput(overrides: Partial = {}): IterateLoopInput { + return { + attemptId: "attempt-1", + workingDirectory: "/tmp/attempt-1", + acceptanceCriteriaPath: "/tmp/attempt-1/acceptance-criteria.json", + instructions: "Add retry to the upload client", + mode: "live", + maxIterations: 3, + maxTurnsPerIteration: 20, + repoFullName: "acme/widgets", + contributorLogin: "miner1", + title: "Add retry to the upload client", + reviewContext: baseReviewContext(), + rejectionSignaled: false, + body: "Closes #7", + linkedIssues: [7], + ...overrides, + }; +} + +function driverReturning(result: CodingAgentDriverResult): CodingAgentDriver { + return { async run() { return result; } }; +} + +function collectingDeps(driver: CodingAgentDriver): { deps: IterateLoopDeps; events: AttemptLogEvent[] } { + const events: AttemptLogEvent[] = []; + const deps: IterateLoopDeps = { + driver, + runSlopAssessment: () => noopSlop, + appendAttemptLogEvent: (event) => { + events.push(event); + }, + }; + return { deps, events }; +} + +describe("runIterateLoop usage guard (#5827)", () => { + it("does not reject uncaught when a driver reports an out-of-contract usage value; still logs a decision", async () => { + for (const badTurns of [-1, Number.NaN, Number.POSITIVE_INFINITY]) { + const { deps, events } = collectingDeps(driverReturning({ ok: true, changedFiles: ["src/upload.ts"], summary: "x", turnsUsed: badTurns as number })); + const result = await runIterateLoop(passingInput(), deps); + + expect(result.outcome).toBe("handoff"); + expect(events.some((event) => event.actionClass === "iterate_loop")).toBe(true); + expect(result.finalMeterTotals.turns).toBe(0); // poisoned axis clamped, not propagated + } + }); + + it("clamps only the out-of-contract axis, preserving a valid axis on the same iteration", async () => { + const { deps } = collectingDeps(driverReturning({ ok: true, changedFiles: ["src/upload.ts"], summary: "x", turnsUsed: Number.NaN, costUsd: 0.02 })); + const result = await runIterateLoop(passingInput(), deps); + + expect(result.outcome).toBe("handoff"); + expect(result.finalMeterTotals.turns).toBe(0); + expect(result.finalMeterTotals.costUsd).toBe(0.02); + }); +});