feat: fall back to alternate auditor model on provider limit - #81
Merged
Conversation
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change adds auditor fallback-chain handling, persisted index resets, provider-limit recovery under loop locks, fallback status display, updated documentation, and expanded tests. ChangesAuditor fallback recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Watchdog
participant LoopRuntime
participant LoopService
participant LoopsRepo
participant AuditorSession
Watchdog->>LoopRuntime: provider-limit during auditor phase
LoopRuntime->>LoopService: handle provider limit under state lock
LoopService->>LoopsRepo: advance fallback index
LoopRuntime->>AuditorSession: redispatch fallback audit
AuditorSession-->>LoopRuntime: successful audit
LoopRuntime->>LoopService: reset fallback index
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
test/loop/runtime-service-seam.test.ts (1)
14-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep
makeFakeLoopServicealigned withLoopService.The fixture omits eight required members, including
advanceAuditorFallbackIndex. Add no-op stubs for all missing members or type the fixture as a narrower service contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/loop/runtime-service-seam.test.ts` around lines 14 - 69, Update makeFakeLoopService to match the current LoopService contract by adding no-op vi.fn stubs for all eight missing required members, including advanceAuditorFallbackIndex. Preserve the existing fixture behavior and return defaults, or explicitly type it against a narrower contract if that is the established testing pattern.test/loops-repo.test.ts (1)
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider building this schema from the shared migration helper.
This file maintains the
loopsDDL by hand, so every storage column addition requires a manual edit here, as this change shows.setupLoopsTestDbintest/helpers/loops-test-db.tsapplies the registered migrations and keeps the test schema aligned with production. Migrating this suite to that helper removes the drift risk. Treat this as optional cleanup for a follow-up, because the column set and partial indexes in this file need care during the switch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/loops-repo.test.ts` at line 56, As optional follow-up cleanup, migrate the loops test database setup from hand-maintained DDL to the shared setupLoopsTestDb helper so registered migrations define the schema and remain aligned with production. Preserve the existing column set and partial indexes when switching the setup path.src/services/execution.ts (1)
2299-2313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the success response path for the absorbed fallback.
The absorbed branch builds its own
ok({ operation: 'loop.restart', ... })payload that duplicates Lines 2334-2345. The two copies have already diverged: the success path publishespublishWorkspaceDetachedToastwhenbindFailedis true (Lines 2324-2332), and the absorbed branch does not. A restart that binds no workspace and then absorbs a provider limit therefore drops the detach warning. Extract one helper that emits the toast and the response, and call it from both sites.♻️ Sketch of the shared response helper
+ const restartSucceeded = (sessionId: string): ForgeExecutionResponse<LoopRestartedResult> => {+ if (bindFailed) {+ publishWorkspaceDetachedToast({+ client: deps.client,+ directory: stoppedState.projectDir ?? stoppedState.worktreeDir,+ loopName: stoppedState.loopName,+ logger: deps.logger,+ context: 'on restart',+ })+ }+ return ok({+ operation: 'loop.restart',+ loopName: stoppedState.loopName,+ sessionId,+ previousSessionId,+ worktreeDir: stoppedState.worktreeDir,+ worktreeBranch: stoppedState.worktreeBranch,+ worktree: !!stoppedState.worktree,+ sandbox: restartSandbox,+ bindFailed,+ iteration: stoppedState.iteration,+ })+ }Then the absorbed branch becomes:
if (absorbed) { deps.loopHandler!.startWatchdog(stoppedState.loopName) - return ok({- operation: 'loop.restart',- loopName: stoppedState.loopName,- sessionId: restartedSessionId!,- previousSessionId,- worktreeDir: stoppedState.worktreeDir,- worktreeBranch: stoppedState.worktreeBranch,- worktree: !!stoppedState.worktree,- sandbox: restartSandbox,- bindFailed,- iteration: stoppedState.iteration,- })+ return restartSucceeded(restartedSessionId!) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/execution.ts` around lines 2299 - 2313, Extract a shared helper around the loop restart success response that publishes publishWorkspaceDetachedToast when bindFailed is true and returns the complete ok({ operation: 'loop.restart', ... }) payload. Replace both the absorbed fallback branch and the normal success path with calls to this helper, preserving their existing restart values and ensuring absorbed restarts also emit the detach warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/dashboard/app/components.ts`:
- Around line 771-775: Update the auditor model display in LoopDetailStat to
resolve and show the selected fallback model when auditorFallbackIndex is set,
rather than appending only the fallback number to the base auditorModel.
Preserve the base model display when no fallback is active, using the existing
dashboard payload/model lookup symbols.
In `@src/loop/runtime.ts`:
- Around line 1582-1589: Guard retryChoice.variant in both recovery calls to
promptAuditSession so it is forwarded only when retryChoice.model is present,
matching the primary auditor dispatch behavior. Update the retry send around
promptAuditSession and the corresponding workspace-recovery resend; leave
auditorModel unchanged.
- Around line 2434-2457: Update the busy cleanup logic around isAwaitingBusy and
coalescedLimitSessions so clearPromptInFlightBySession executes synchronously
before acquiring withStateLock, including in the coalesced branch. Keep
withStateLock scoped only to clearing the coalescedLimitSessions marker,
preserving the documented immediate-clear behavior.
In `@src/tools/loop.ts`:
- Around line 6-7: Update auditorModelStatusLabel to use the shared
buildAuditorModelChain and resolveLoopAuditorChoice resolution path, preserving
the fallback order from resolveUsageFallbackModelLabel when choice.model is
undefined. Keep auditorModelStatusLabel limited to formatting, and remove its
independent auditor-choice resolution logic.
- Around line 19-24: Update auditorModelStatusLabel to compute and reuse the
normalized fallback index returned or established by auditorModelChoiceAt when
selecting the model, then use that same clamped index for the fallback label and
comparison. Ensure persisted indices beyond the current chain produce a valid
label consistent with the selected choice.
---
Nitpick comments:
In `@src/services/execution.ts`:
- Around line 2299-2313: Extract a shared helper around the loop restart success
response that publishes publishWorkspaceDetachedToast when bindFailed is true
and returns the complete ok({ operation: 'loop.restart', ... }) payload. Replace
both the absorbed fallback branch and the normal success path with calls to this
helper, preserving their existing restart values and ensuring absorbed restarts
also emit the detach warning.
In `@test/loop/runtime-service-seam.test.ts`:
- Around line 14-69: Update makeFakeLoopService to match the current LoopService
contract by adding no-op vi.fn stubs for all eight missing required members,
including advanceAuditorFallbackIndex. Preserve the existing fixture behavior
and return defaults, or explicitly type it against a narrower contract if that
is the established testing pattern.
In `@test/loops-repo.test.ts`:
- Line 56: As optional follow-up cleanup, migrate the loops test database setup
from hand-maintained DDL to the shared setupLoopsTestDb helper so registered
migrations define the schema and remain aligned with production. Preserve the
existing column set and partial indexes when switching the setup path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b734cd3-d6bf-4e86-8335-8001d254d5bf
📒 Files selected for processing (45)
AGENTS.mddocs/api/_media/configuration.mddocs/api/_media/loop-system.mddocs/api/functions/createForgePlugin.mddocs/api/functions/createParentSessionLookup.mddocs/api/functions/createSessionDirectoryLookup.mddocs/api/interfaces/CompactionConfig.mddocs/api/interfaces/CreateParentSessionLookupOptions.mddocs/api/interfaces/CreateSessionDirectoryLookupOptions.mddocs/api/interfaces/DashboardConfig.mddocs/api/interfaces/PluginConfig.mddocs/api/variables/VERSION.mddocs/api/variables/default.mddocs/configuration.mddocs/loop-system.mdforge-config.jsoncsrc/dashboard/app-bundle.tssrc/dashboard/app/components.tssrc/hooks/watchdog.tssrc/loop/runtime-usage.tssrc/loop/runtime.tssrc/loop/service.tssrc/loop/state.tssrc/services/execution.tssrc/storage/migrations/index.tssrc/storage/repos/loops-repo.tssrc/tools/loop.tssrc/types.tssrc/utils/loop-helpers.tstest/hooks/loop-section-advancement.test.tstest/loop-helpers.test.tstest/loop/runtime-service-seam.test.tstest/loop/runtime.test.tstest/loop/state-mapper.test.tstest/loops-repo.test.tstest/services/attach-loop.test.tstest/services/execution-attach-cleanup.test.tstest/services/execution-in-flight-guard.test.tstest/services/execution-restart.test.tstest/services/execution.start-loop.test.tstest/storage-migrations.test.tstest/tools/plan-adjust.test.tstest/tools/review-section-scope.test.tstest/tools/section-read.test.tstest/watchdog.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Adds an auditor model fallback chain: when an auditor session hits a provider limit, the loop advances through auditorFallbackModels instead of terminating, re-dispatches the audit on the next configured model, and resets the chain index to 0 on a successful audit (both auditor phases) and on loop restart. Every detection path routes through handleAuditorProviderLimit, including the watchdog's retry status poll, which calls it via the per-loop state lock. Adds resetAuditorFallbackIndex to loops-repo/service, usage role/phase helpers to loop-helpers, and dashboard auditor-fallback position display.
chriswritescode-devforce-pushed
the
feat/auditor-model-fallback
branch
from
August 1, 2026 23:27
086bea8 to
7f6b96eCompareContributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/loop/runtime.ts (2)
2853-2884: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPersist the fallback-index reset during restart.
restartAuditorState.auditorFallbackIndex = 0only affects model selection. Thedeps.loopsRepo.restartpayload omits this field, so the persisted index is not reset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/loop/runtime.ts` around lines 2853 - 2884, Update the restart flow around restartAuditorState and deps.loopsRepo.restart so the restart payload includes auditorFallbackIndex set to 0, ensuring the persisted fallback index is reset along with model-selection state.Source: Coding guidelines
1004-1068: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix stale-session handling so it does not terminate the active loop.
At Line 1011,
handleAuditorProviderLimitreturnsfalsewhenopts.eventSessionIddoes not match the loop's current session. Both callers that passeventSessionId(thesession.errorhandler at Line 2360 and thesession.status: retryhandler at Line 2472) treatfalseas "not absorbed" and terminate the loop.The
session.errorhandler already has a parallel staleness check for non-limit errors (Lines 2366-2370) that ignores the event and returns without terminating. A limit-classified error from a session the loop already rotated away from should get the same treatment: ignore it, do not terminate the still-active loop.As written, a delayed provider-limit event from an old, already-rotated auditor session can wrongly terminate a currently healthy loop.
🐛 Proposed fix to ignore stale-session limit signals
let state = loopService.getActiveState(loopName) if (!state?.active || !isAuditorPhase(state.phase)) return false - if (opts?.eventSessionId && opts.eventSessionId !== state.sessionId) return false+ if (opts?.eventSessionId && opts.eventSessionId !== state.sessionId) {+ // Stale/delayed event from a session the loop has already rotated+ // away from. Ignore it — do not terminate the still-active loop.+ logger.log(`Loop: ignoring stale provider-limit event for session ${opts.eventSessionId} (current=${state.sessionId})`)+ return true+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/loop/runtime.ts` around lines 1004 - 1068, Update handleAuditorProviderLimit so an eventSessionId that differs from the active state.sessionId is treated as an absorbed stale signal rather than returning false; return true while leaving the active loop unchanged. Preserve the existing behavior for inactive/non-auditor loops and matching-session limit handling so callers such as the session.error and session.status retry handlers do not terminate a healthy loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/loop/runtime.ts`:
- Around line 980-984: Replace the duplicated chain construction, fallback-index
lookup, and choice logging in the shown audit flow with a call to
resolveLoopAuditorChoice(getConfig(), loopService, loopName, logger). Remove the
local buildAuditorModelChain, index, and auditorModelChoiceAt usage, while
preserving the resulting choice for subsequent processing and keeping
handleAuditorProviderLimit as the fallback-or-terminate decision point.
---
Outside diff comments:
In `@src/loop/runtime.ts`:
- Around line 2853-2884: Update the restart flow around restartAuditorState and
deps.loopsRepo.restart so the restart payload includes auditorFallbackIndex set
to 0, ensuring the persisted fallback index is reset along with model-selection
state.
- Around line 1004-1068: Update handleAuditorProviderLimit so an eventSessionId
that differs from the active state.sessionId is treated as an absorbed stale
signal rather than returning false; return true while leaving the active loop
unchanged. Preserve the existing behavior for inactive/non-auditor loops and
matching-session limit handling so callers such as the session.error and
session.status retry handlers do not terminate a healthy loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f48534d8-eb4b-443e-9c52-3f2c67b3757d
📒 Files selected for processing (36)
AGENTS.mddocs/api/_media/configuration.mddocs/api/functions/createForgePlugin.mddocs/api/functions/createParentSessionLookup.mddocs/api/functions/createSessionDirectoryLookup.mddocs/api/interfaces/CompactionConfig.mddocs/api/interfaces/CreateParentSessionLookupOptions.mddocs/api/interfaces/CreateSessionDirectoryLookupOptions.mddocs/api/interfaces/DashboardConfig.mddocs/api/interfaces/PluginConfig.mddocs/api/variables/VERSION.mddocs/api/variables/default.mddocs/configuration.mdsrc/dashboard/app-bundle.tssrc/dashboard/app/components.tssrc/hooks/watchdog.tssrc/loop/runtime.tssrc/loop/service.tssrc/services/execution.tssrc/storage/repos/loops-repo.tssrc/tools/loop.tssrc/utils/loop-helpers.tstest/hooks/loop-section-advancement.test.tstest/loop-helpers.test.tstest/loop/runtime-service-seam.test.tstest/loop/runtime.test.tstest/loops-repo.test.tstest/services/attach-loop.test.tstest/services/execution-attach-cleanup.test.tstest/services/execution-in-flight-guard.test.tstest/services/execution-restart.test.tstest/services/execution.start-loop.test.tstest/tools/plan-adjust.test.tstest/tools/review-section-scope.test.tstest/tools/section-read.test.tstest/watchdog.test.ts
🚧 Files skipped from review as they are similar to previous changes (32)
- docs/api/functions/createParentSessionLookup.md
- test/tools/review-section-scope.test.ts
- test/loop/runtime-service-seam.test.ts
- docs/api/variables/VERSION.md
- docs/api/_media/configuration.md
- test/services/execution-attach-cleanup.test.ts
- AGENTS.md
- docs/api/functions/createSessionDirectoryLookup.md
- test/hooks/loop-section-advancement.test.ts
- test/services/execution-in-flight-guard.test.ts
- docs/api/interfaces/DashboardConfig.md
- test/tools/section-read.test.ts
- docs/api/functions/createForgePlugin.md
- docs/api/interfaces/CreateSessionDirectoryLookupOptions.md
- docs/api/interfaces/CompactionConfig.md
- test/watchdog.test.ts
- test/services/attach-loop.test.ts
- docs/configuration.md
- test/tools/plan-adjust.test.ts
- docs/api/variables/default.md
- test/loop/runtime.test.ts
- src/dashboard/app-bundle.ts
- src/dashboard/app/components.ts
- src/hooks/watchdog.ts
- test/services/execution.start-loop.test.ts
- src/services/execution.ts
- src/loop/service.ts
- test/loop-helpers.test.ts
- src/storage/repos/loops-repo.ts
- src/tools/loop.ts
- src/utils/loop-helpers.ts
- test/services/execution-restart.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an auditor model fallback chain. When an auditor session hits a provider limit, the loop advances through
auditorFallbackModelsinstead of terminating, re-dispatches the audit on the next configured model, and resets the chain index to 0 on a successful audit (both auditor phases) and on loop restart, so a transient limit never permanently drops the auditor variant.Key changes
isAuditorPhase,usageRoleForPhasehelpers; usage attribution label routes through the chain choice.handleAuditorProviderLimitwalks the fallback chain, re-dispatches the audit prompt on the next model, coalesces duplicate limit signals per failed prompt, and resets the index on successful audits. Removes the localisAuditorPhaseduplicate in favor of the shared helper.retrystatus poll routes auditor provider limits throughhandleAuditorProviderLimitvia the per-loop state lock.resetAuditorFallbackIndex(compare-and-set reset to 0).auditorModeland resets the fallback index.auditor modelstatus line shows the effective chain position.app-bundle.ts).Validation
pnpm typecheck && pnpm lint && pnpm test— all green (166 test files, 2874 tests).Deferred follow-ups (pr-review ledger)
AUDITOR_FALLBACK_SETTLE_MS = 250settle sleep insrc/loop/runtime.ts; replacing it with the tracked abort terminal is a design change affecting the fallback timing suite.advanceAuditorFallbackIndexconflates three outcomes into an ambiguousnull; benign for the current two callers.config.auditorModeltier lives in two ad-hoc patches outsidebuildAuditorModelChain; adding it to the chain is a compatibility change deferred deliberately.Summary by CodeRabbit
New Features
Bug Fixes
Documentation