fix(engine): normalize classifyPortfolioConvergence's numeric inputs#6225
Conversation
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 JSONbored#6173
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6225 +/- ##
=======================================
Coverage 95.35% 95.35%
=======================================
Files 598 598
Lines 47102 47108 +6
Branches 14988 14989 +1
=======================================
+ Hits 44913 44921 +8
+ Misses 1464 1463 -1
+ Partials 725 724 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loopover-orb
Bot
added
the
gittensor:bug
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-15 23:39:05 UTC
Review summary Nits — 4 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://gittensory.aethereal.dev/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
1 participant
Summary
Closes #6173
classifyPortfolioConvergenceis composed into the same fail-closed ladder asgovernor/budget-cap.tsandgovernor/rate-limit.ts(all three feedevaluateGovernorChokepoint), but read its counts and thresholds raw while both siblings clamp theirs. ANaNor negative value failed every>=threshold check and the trailing> 0check, so the classifier quietly returned"converging"— the allow-equivalent verdict — where its siblings fail toward deny. It also printedNaNstraight into the reasons a maintainer reads.Fix: 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 — its own test spells this out: "budget: used 0 (NaN→0), limit 0 (Infinity→0) ⇒ 0 >= 0 ⇒ exceeded." Here the same clamp makes
consecutiveFailures >= maxConsecutiveFailures(0 >= 0) readnon_convergentrather thanconverging.It also closes a quieter hole:
NaN <= 0is false, so an unnormalized non-finiteattemptscount fell past the no-attempts guard and got classified on the streak counters of an item that had never been attempted at all.Scoped to malformed input only — the legitimate-input threshold logic is untouched, with tests pinning that.
On "reuse the helper, don't reimplement it"
There is no shared export to import:
finiteNonNegativeIntis module-private in every one of the 18 pure engine modules that use it, including both named siblings and this file's own directory neighbourportfolio/queue.ts.non-convergence.tsalso deliberately has no imports at all. So this reuses the rule — byte-identical torate-limit.ts's — as a private copy matching that established convention, rather than couplingportfolio/togovernor/for a one-line clamp or refactoring 18 modules (well outside this issue's scope).Scope
fix(engine): …).CONTRIBUTING.md; nosite//CNAME/lovable; no changelog edit.Validation
git diff --checkclean.npm run build:miner— exit 0.npm run typecheck— exit 0.npm run test --workspace @loopover/engine(atest:cistep) — 563 pass, 0 fail.packages/loopover-engine/src/portfolio/non-convergence.ts: 100% statements (21/21), 100% branch (17/17), 100% functions (2/2), 100% lines (21/21).coverage/lcov.info: every changed executable line is hit.a malformed threshold fails CLOSED,a non-finite attempts count reads converging,a fractional count is floored), then pass again with it restored.main.Why the tests live in two suites
packages/loopover-engine/test/portfolio-non-convergence.test.tsis the location the issue names and runs intest:ci— but it executes undernode:testagainstdist/, so it produces no coverage signal forsrc/. The root vitest spec exercises the same normalization through the source path, which is what Codecov measures. Both are needed: the first satisfies the issue's named file and the package's own gate; the second satisfies the 99% patch bar.If any required check was skipped, explain why:
test:cinot run end-to-end locally (Linux-only steps on Windows).test/unit/miner-portfolio-queue.test.tshas 2 unrelated failures in this environment (file-permission/XDG-path assertions) — confirmed pre-existing on a clean checkout ofmain, and untouched by this diff.Safety