fix(engine): normalize worktree-pool maxConcurrency so a NaN cap can't disable it - #6041
Conversation
…t disable it availableWorktreeSlots and acquireWorktree read WorktreePoolConfig.maxConcurrency directly in a comparison. A NaN cap (e.g. derived upstream from Number() of an unparsable config value) makes `length >= NaN` always false, so at_capacity never fires and the pool allocates worktrees without bound — contradicting the module's documented "non-positive cap allocates nothing" contract — while availableWorktreeSlots returns NaN. Normalize maxConcurrency through a finiteNonNegativeInt helper at both read sites (non-finite or negative floors to 0, fractional floors down), mirroring governor/rate-limit.ts, governor/budget-cap.ts, and tenant-quota.ts. Adds regression tests for NaN, negative, and fractional maxConcurrency. Closes JSONbored#5828
|
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 #6041 +/- ##
=======================================
Coverage 95.16% 95.17%
=======================================
Files 593 592 -1
Lines 46955 46967 +12
Branches 15006 15007 +1
=======================================
+ Hits 44685 44700 +15
+ Misses 1512 1511 -1
+ Partials 758 756 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-15 07:39:08 UTC
✅ Suggested Action - Approve/Merge
Review summary Nits — 3 non-blocking
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.
|
Summary
availableWorktreeSlotsandacquireWorktreeinpackages/loopover-engine/src/miner/worktree-pool.tsreadWorktreePoolConfig.maxConcurrencydirectly in their comparisons with no normalization. If the cap arrives asNaN(e.g. a caller derives it fromNumber(someUnparsableConfigValue)and doesn't guard the result), every comparison against it isfalse—length >= NaNnever holds — soat_capacitynever fires and the pool allocates worktrees without bound, directly contradicting the type's own documented contract ("A non-positive cap allocates nothing").availableWorktreeSlotsalso returnsNaN, breaking any caller that treats it as a real remaining-slots count.The fix adds a small
finiteNonNegativeInthelper (mirroring the exact pattern already used ingovernor/rate-limit.ts,governor/budget-cap.ts, andtenant-quota.tsin this same package) and normalizesconfig.maxConcurrencythrough it at both read sites: a non-finite or negative cap floors to0(the documented "allocates nothing" behavior), and a fractional cap floors down. No public signature or behavior change for a valid positive cap.Closes #5828
Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #5828).Validation
git diff --check— cleannpm run actionlint— no workflow changesnpm run typecheck— passes (whole project, against the rebuilt@loopover/engine)npm run test:coverage— the two changed engine lines and both arms of the newfiniteNonNegativeInt(finite→floor, non-finite→0) are exercised by the new regression tests plus the existing positive-cap tests, socodecov/patchon the diff is 100%.test/unit/miner-worktree-pool.test.tspasses 10/10; the new NaN and fractional cases fail on the pre-fix code and pass after.packages/loopover-engineown suite: 553/553.npm run test:workers— unaffected (pure in-memory engine scheduling logic, no worker changes)npm run build:mcp/test:mcp-pack— unaffected (no MCP changes)npm run ui:openapi:check/ui:lint/ui:typecheck/ui:build— unaffected (no UI/OpenAPI/route changes)npm audit --audit-level=moderate— no dependency changesmaxConcurrency: NaN,-1, and a fractional value, asserting the pool refuses allocation (or floors correctly) in each caseIf any required check was skipped, explain why:
worktree-pool.ts, +the helper) plus its test. It introduces no workflow, MCP, UI, OpenAPI, or dependency changes. I additionally rannpm run engine-parity:drift-check(ok — this module is engine-only, not asrc/{review,settings,signals}twin) and the backend drift checks (docs,manifest,command-reference,selfhost env-reference), all green.Safety
UI Evidencesection — N/A (no UI/frontend/docs/extension changes).Notes
0cap rather than silently disabling the limit — so no caller relying on the current documented semantics changes behavior; only the previously-undefined invalid-input case is now defined.