From 08669cc9f8272f792b1045f69c2343d7b6476e91 Mon Sep 17 00:00:00 2001 From: luciferlive112116 <291889058+luciferlive112116@users.noreply.github.com> Date: Thu, 16 Jul 2026 07:14:51 +0800 Subject: [PATCH] fix(engine): normalize classifyPortfolioConvergence's numeric inputs classifyPortfolioConvergence feeds the same fail-closed ladder as governor/budget-cap.ts and governor/rate-limit.ts (all three compose into evaluateGovernorChokepoint), but read its counts and thresholds raw while both siblings clamp theirs. A NaN or negative value failed every >= threshold check and the trailing > 0 check, so the classifier quietly returned "converging" -- the allow-equivalent verdict -- where its siblings fail toward deny. It also printed NaN straight into the reasons a maintainer reads. Normalize every count and threshold with the same rule the siblings apply. The fail-closed direction comes from clamping BOTH operands, which is exactly how budget-cap behaves: a non-finite ceiling becomes 0, so used >= limit (0 >= 0) reads exceeded. Here the same clamp makes consecutiveFailures >= maxConsecutiveFailures (0 >= 0) read non_convergent rather than converging. It also closes a quieter hole: NaN <= 0 is false, so an unnormalized non-finite attempts count fell PAST the no-attempts guard and got classified on the streak counters of an item that had never been attempted at all. The helper is module-private, matching this file's siblings rather than coupling to them: budget-cap.ts, rate-limit.ts, portfolio/queue.ts and 15 other pure engine modules each carry their own copy of this one-line clamp, and non-convergence.ts deliberately has no imports. Reusing the rule, not reimplementing a different one. Scoped to malformed input only -- the legitimate-input threshold logic is untouched, with tests pinning that. Tests live in both suites on purpose: the engine package's own node:test file is the issue's named location and runs in test:ci, but it executes against dist/ and so yields no coverage signal for src/. The root vitest spec exercises the same normalization through the source path, which is what Codecov measures -- 100% statements and branches (21/21, 17/17). Verified to fail against the pre-fix classifier. Closes #6173 --- .../src/portfolio/non-convergence.ts | 40 +++++++--- .../test/portfolio-non-convergence.test.ts | 45 +++++++++++ ...olio-non-convergence-normalization.test.ts | 78 +++++++++++++++++++ 3 files changed, 152 insertions(+), 11 deletions(-) create mode 100644 test/unit/portfolio-non-convergence-normalization.test.ts diff --git a/packages/loopover-engine/src/portfolio/non-convergence.ts b/packages/loopover-engine/src/portfolio/non-convergence.ts index 4218a08fd..834f9be67 100644 --- a/packages/loopover-engine/src/portfolio/non-convergence.ts +++ b/packages/loopover-engine/src/portfolio/non-convergence.ts @@ -45,6 +45,15 @@ export type PortfolioConvergenceVerdict = { reasons: string[]; }; +// Normalize any numeric input to a non-negative integer (a non-finite or negative value becomes 0), so no +// malformed count or threshold can make a verdict wrong (#6173). Byte-identical to the rule this classifier's +// fail-closed ladder siblings apply -- governor/rate-limit.ts's and governor/budget-cap.ts's own +// finiteNonNegativeInt -- and kept module-private like theirs (and portfolio/queue.ts's), since every pure +// module here carries its own copy rather than coupling to a shared util for a one-line clamp. +function finiteNonNegativeInt(value: number): number { + return Number.isFinite(value) ? Math.max(0, Math.floor(value)) : 0; +} + /** * Classify one queue item's convergence from its attempt/outcome counts. Pure and deterministic. * @@ -54,12 +63,25 @@ export type PortfolioConvergenceVerdict = { * - A sustained streak — `consecutiveFailures` or `reenqueues` at/above its threshold — reads * `non_convergent`. A single failure or re-enqueue below threshold reads `stalled`, not non-convergent. * - Attempts in progress with no failure streak read `converging`. + * + * Every numeric count and threshold is normalized first (#6173), matching the discipline of the fail-closed + * ladder this signal is composed into (governor/budget-cap.ts, governor/rate-limit.ts). That keeps a malformed + * value from deciding the verdict in the WRONG direction: a non-finite threshold clamps to 0, so the `>=` test + * then reads non_convergent — the deny-equivalent verdict — exactly as a malformed ceiling makes budget-cap's + * `used >= limit` read `exceeded`, rather than a NaN comparison quietly failing every check and reporting + * `converging`. It also keeps `NaN` out of the reason strings a maintainer reads. */ export function classifyPortfolioConvergence( input: PortfolioConvergenceInput, thresholds: PortfolioConvergenceThresholds = DEFAULT_PORTFOLIO_CONVERGENCE_THRESHOLDS, ): PortfolioConvergenceVerdict { - if (input.attempts <= 0) { + const attempts = finiteNonNegativeInt(input.attempts); + const consecutiveFailures = finiteNonNegativeInt(input.consecutiveFailures); + const reenqueues = finiteNonNegativeInt(input.reenqueues); + const maxConsecutiveFailures = finiteNonNegativeInt(thresholds.maxConsecutiveFailures); + const maxReenqueues = finiteNonNegativeInt(thresholds.maxReenqueues); + + if (attempts <= 0) { return { status: "converging", reasons: ["No attempts yet; a first attempt is not evidence of a stuck loop."], @@ -70,25 +92,21 @@ export function classifyPortfolioConvergence( } const reasons: string[] = []; - if (input.consecutiveFailures >= thresholds.maxConsecutiveFailures) { - reasons.push( - `${input.consecutiveFailures} consecutive failures (≥ ${thresholds.maxConsecutiveFailures}).`, - ); + if (consecutiveFailures >= maxConsecutiveFailures) { + reasons.push(`${consecutiveFailures} consecutive failures (≥ ${maxConsecutiveFailures}).`); } - if (input.reenqueues >= thresholds.maxReenqueues) { - reasons.push( - `re-enqueued ${input.reenqueues} times without reaching done (≥ ${thresholds.maxReenqueues}).`, - ); + if (reenqueues >= maxReenqueues) { + reasons.push(`re-enqueued ${reenqueues} times without reaching done (≥ ${maxReenqueues}).`); } if (reasons.length > 0) { return { status: "non_convergent", reasons }; } - if (input.consecutiveFailures > 0 || input.reenqueues > 0) { + if (consecutiveFailures > 0 || reenqueues > 0) { return { status: "stalled", reasons: [ - `${input.consecutiveFailures} consecutive failure(s), ${input.reenqueues} re-enqueue(s) — below the non-convergence threshold.`, + `${consecutiveFailures} consecutive failure(s), ${reenqueues} re-enqueue(s) — below the non-convergence threshold.`, ], }; } diff --git a/packages/loopover-engine/test/portfolio-non-convergence.test.ts b/packages/loopover-engine/test/portfolio-non-convergence.test.ts index 3b7908f69..0330df22a 100644 --- a/packages/loopover-engine/test/portfolio-non-convergence.test.ts +++ b/packages/loopover-engine/test/portfolio-non-convergence.test.ts @@ -68,3 +68,48 @@ test("thresholds are configurable — a stricter cap trips sooner, and the defau assert.equal(strict.status, "non_convergent"); assert.equal(DEFAULT_PORTFOLIO_CONVERGENCE_THRESHOLDS.maxConsecutiveFailures, 3); }); + +// #6173: this classifier feeds the same fail-closed ladder as governor/budget-cap.ts and +// governor/rate-limit.ts, which normalize every numeric input so malformed data can never decide a verdict in +// the wrong direction. Unnormalized, a NaN/negative count failed every `>=` and the trailing `> 0` check and +// quietly reported "converging" — the allow-equivalent verdict — and printed NaN into the reasons a maintainer +// reads. These mirror governor-budget-cap.test.ts's own malformed-input test. + +test("#6173: a malformed threshold fails CLOSED — non_convergent, exactly as a malformed ceiling makes budget-cap read exceeded", () => { + // budget-cap's convention: a non-finite limit clamps to 0, so `used >= limit` (0 >= 0) reads exceeded. + // Here the same clamp makes `consecutiveFailures >= maxConsecutiveFailures` (0 >= 0) read non_convergent. + const v = classifyPortfolioConvergence( + { ...base, attempts: 1, consecutiveFailures: Number.NaN, reenqueues: -3 }, + { maxConsecutiveFailures: Number.NaN, maxReenqueues: Number.POSITIVE_INFINITY }, + ); + assert.equal(v.status, "non_convergent"); + assert.notEqual(v.status, "converging"); +}); + +test("#6173: NaN/negative counts never reach the verdict or the reasons as NaN", () => { + const v = classifyPortfolioConvergence({ ...base, attempts: 4, consecutiveFailures: Number.NaN, reenqueues: -2 }); + // NaN/-2 clamp to 0, so against the real defaults this is an honest "no streak" — not a fabricated one… + assert.equal(v.status, "converging"); + // …and no NaN/negative ever leaks into the operator-facing text. + assert.doesNotMatch(v.reasons.join(" "), /NaN|-\d/); +}); + +test("#6173: a non-finite attempts count reads converging, not a live streak", () => { + // Unnormalized, `NaN <= 0` was false, so a NaN attempts count fell PAST the no-attempts guard and got + // classified on the streak counters of an item that had never actually been attempted. + const v = classifyPortfolioConvergence({ ...base, attempts: Number.NaN, consecutiveFailures: 5, reenqueues: 5 }); + assert.equal(v.status, "converging"); + assert.match(v.reasons.join(" "), /first attempt/i); +}); + +test("#6173: a fractional count is floored, matching rate-limit.ts's integer discipline", () => { + const v = classifyPortfolioConvergence({ ...base, attempts: 3, consecutiveFailures: 3.9 }); + assert.equal(v.status, "non_convergent"); + assert.match(v.reasons.join(" "), /3 consecutive failures/); +}); + +test("#6173: legitimate input is unaffected by normalization", () => { + assert.equal(classifyPortfolioConvergence({ ...base, attempts: 1, consecutiveFailures: 1 }).status, "stalled"); + assert.equal(classifyPortfolioConvergence({ ...base, attempts: 3, consecutiveFailures: 3 }).status, "non_convergent"); + assert.equal(classifyPortfolioConvergence({ ...base, attempts: 5 }).status, "converging"); +}); diff --git a/test/unit/portfolio-non-convergence-normalization.test.ts b/test/unit/portfolio-non-convergence-normalization.test.ts new file mode 100644 index 000000000..2a1e60924 --- /dev/null +++ b/test/unit/portfolio-non-convergence-normalization.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it } from "vitest"; + +import { + classifyPortfolioConvergence, + type PortfolioConvergenceInput, +} from "../../packages/loopover-engine/src/portfolio/non-convergence"; + +// #6173: classifyPortfolioConvergence is composed into the same fail-closed ladder as governor/budget-cap.ts +// and governor/rate-limit.ts, but read its counts raw. A NaN/negative value failed every `>=` threshold check +// and the trailing `> 0` check, so it quietly returned "converging" — the ALLOW-equivalent verdict — where its +// siblings fail toward deny, and printed NaN into the reasons a maintainer reads. +// +// The engine package's own suite (packages/loopover-engine/test/portfolio-non-convergence.test.ts) covers this +// too, but it runs under node:test against dist/, which produces no coverage signal for src/. These exercise +// the same normalization through the SOURCE path, which is what Codecov measures. + +const base: PortfolioConvergenceInput = { attempts: 0, consecutiveFailures: 0, reenqueues: 0, reachedDone: false }; + +describe("classifyPortfolioConvergence input normalization (#6173)", () => { + it("fails CLOSED on a malformed threshold — the direction budget-cap.ts's malformed ceiling produces", () => { + // budget-cap: a non-finite limit clamps to 0, so `used >= limit` (0 >= 0) reads exceeded. Same clamp here + // makes `consecutiveFailures >= maxConsecutiveFailures` (0 >= 0) read non_convergent, not converging. + const verdict = classifyPortfolioConvergence( + { ...base, attempts: 1, consecutiveFailures: Number.NaN, reenqueues: -3 }, + { maxConsecutiveFailures: Number.NaN, maxReenqueues: Number.POSITIVE_INFINITY }, + ); + expect(verdict.status).toBe("non_convergent"); + }); + + it("clamps NaN/negative counts to 0 instead of letting them decide the verdict", () => { + const verdict = classifyPortfolioConvergence({ ...base, attempts: 4, consecutiveFailures: Number.NaN, reenqueues: -2 }); + expect(verdict.status).toBe("converging"); + expect(verdict.reasons.join(" ")).not.toMatch(/NaN|-\d/); + }); + + it("a non-finite attempts count reads converging, not a streak on an item never attempted", () => { + // Unnormalized, `NaN <= 0` was false, so the no-attempts guard was skipped entirely. + const verdict = classifyPortfolioConvergence({ ...base, attempts: Number.NaN, consecutiveFailures: 5, reenqueues: 5 }); + expect(verdict.status).toBe("converging"); + expect(verdict.reasons.join(" ")).toMatch(/first attempt/i); + }); + + it("floors fractional counts, matching rate-limit.ts's integer discipline", () => { + const verdict = classifyPortfolioConvergence({ ...base, attempts: 3, consecutiveFailures: 3.9 }); + expect(verdict.status).toBe("non_convergent"); + expect(verdict.reasons.join(" ")).toContain("3 consecutive failures"); + }); + + it("normalizes the re-enqueue arm independently of the failure arm", () => { + const verdict = classifyPortfolioConvergence({ ...base, attempts: 3, consecutiveFailures: -1, reenqueues: 3.7 }); + expect(verdict.status).toBe("non_convergent"); + expect(verdict.reasons.join(" ")).toContain("re-enqueued 3 times"); + }); + + describe("legitimate input still classifies exactly as before", () => { + it("reached done wins over any streak", () => { + expect(classifyPortfolioConvergence({ attempts: 4, consecutiveFailures: 9, reenqueues: 9, reachedDone: true }).status).toBe("converging"); + }); + + it("a below-threshold streak is stalled (both arms of the stalled OR)", () => { + expect(classifyPortfolioConvergence({ ...base, attempts: 2, consecutiveFailures: 1 }).status).toBe("stalled"); + expect(classifyPortfolioConvergence({ ...base, attempts: 2, reenqueues: 1 }).status).toBe("stalled"); + }); + + it("an at-threshold streak is non_convergent, and both streaks surface both reasons", () => { + expect(classifyPortfolioConvergence({ ...base, attempts: 3, consecutiveFailures: 3 }).status).toBe("non_convergent"); + expect(classifyPortfolioConvergence({ ...base, attempts: 6, consecutiveFailures: 4, reenqueues: 5 }).reasons).toHaveLength(2); + }); + + it("attempts in progress with no streak is converging", () => { + expect(classifyPortfolioConvergence({ ...base, attempts: 5 }).reasons.join(" ")).toMatch(/no failure streak/i); + }); + + it("zero attempts is converging", () => { + expect(classifyPortfolioConvergence({ ...base, attempts: 0 }).status).toBe("converging"); + }); + }); +});