fix(engine): normalize classifyPortfolioConvergence's numeric inputs

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.`,
       ],
     };
   }

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");
+});

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");
+    });
+  });
+});