feat: configurable maxIterPerTurn per-turn iteration cap (#2037)#2046
Conversation
The main loop had no hard iteration limit — a runaway model could consume unlimited API budget. Added maxIterPerTurn config (default 9) with env override REASONIX_MAX_ITER. Plumbed through all loop construction sites (App, desktop, run, acp). Updated i18n strings.
ae3563a to
99313ef
Compare
|
Thanks — capping the iteration loop is the right call, and the test that drives a cycling tool with unique args is a good way to assert the cap fires before the storm-breaker does. Two concerns on the defaults before I merge:
Once those two are addressed I'll merge. |
…add iterLimitReached type - Change DEFAULT_MAX_ITER_PER_TURN from 9 to 50 (real workloads need 15-30 calls) - Remove redundant parsePositiveIntEnv() inside the loop; use this.maxIterPerTurn directly (loadMaxIterPerTurn() already resolves config > env > default at construction time) - Add iterLimitReached to i18n types (fixes CI build failure) - Move cap check before model call to avoid wasting an API request on the cap-hit iter - Add test for custom maxIterPerTurn option
esengine
left a comment
There was a problem hiding this comment.
Direction is right — closes a real gap (BUG-028 was on my radar too).
A few inconsistencies to clean up before I merge:
-
src/loop.tsfield doc says default 9, code is 50. The class field comment readsConfig > env > default (9)butDEFAULT_MAX_ITER_PER_TURN = 50andloadMaxIterPerTurnfalls back to 50 too. Please reconcile to 50 (or whatever final number we land on — see #3). -
PR description is stale. It says
Changed DEFAULT_MAX_ITER_PER_TURN from 100 to 9andDefault is 9in the usage block, but the code is 50. Please update so the description matches what reviewers will actually merge. -
Default value — is 50 the right number? A defensive cap is good, but 50 tool calls per turn is on the high side; in practice most legitimate turns finish well under 30. I'm fine with 50 as a starting point, but call out in the description why you picked 50 over (say) 30 so it's a deliberate choice we can revisit later.
CI is green and the test coverage looks good. Once 1 + 2 are fixed and 3 has a one-line rationale I'm happy to merge.
Summary
Fixes the "no iteration limit" vulnerability from #2037 (BUG-028). The main loop was an infinite
forloop with no hard cap — a runaway model could consume unlimited API budget.Changes
src/config.ts— AddedmaxIterPerTurn?: numbertoReasonixConfig+loadMaxIterPerTurn()loader (config > env > default 50)src/loop.ts— AddedmaxIterPerTurntoCacheFirstLoopOptionsandCacheFirstLoopinstance.DEFAULT_MAX_ITER_PER_TURN = 50. Loop checksiter >= this.maxIterPerTurnbefore each model call and emits a warning + forces summary.src/cli/ui/App.tsx— PassesloadMaxIterPerTurn()when constructing loopsrc/cli/commands/desktop.ts— Samesrc/cli/commands/run.ts— Samesrc/cli/commands/acp.ts— Samesrc/i18n/EN.ts+zh-CN.ts+JA.ts— Warning message for iteration capsrc/i18n/types.ts— AddediterLimitReachedto loop typetests/loop.test.ts— Added tests verifying default and custom capUsage
Or via env:
REASONIX_MAX_ITER=15Default is 50. 50 = observed ceiling (30) × ~1.7 safety margin. Tighter cap risks truncating legitimate multi-file refactors. Power users can override via config or env.
Verification
npx vitest run tests/loop.test.ts— 70/70 passednpm run verify— all 3805 tests passed