feat(core): backport enhanced loop detection (#3236)#196
Merged
Conversation
WalkthroughThe PR extends loop detection to classify loops by type—adding thought-repetition, read-file-operation, and action-stagnation detection—and propagates the detected loop type through the event stream so the CLI can display a human-readable halt message to users. ChangesEnhanced Loop Detection with Type Classification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 3/5 reviews remaining, refill in 13 minutes and 57 seconds. Comment |
This was referenced May 2, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/nonInteractiveCli.ts`:
- Around line 380-382: The cronStream path does not handle
GeminiEventType.LoopDetected like the responseStream does; update the cronStream
event loop (the code handling events from cronStream) to call
emitLoopDetectedMessage(config, event.value?.loopType) when event.type ===
GeminiEventType.LoopDetected instead of simply forwarding the event to the
adapter so TEXT-mode cron runs also print the loop explanation; locate the
cronStream event handler in nonInteractiveCli.ts and mirror the same conditional
and call used in the responseStream branch to ensure consistent behavior.
In `@packages/core/src/services/loopDetectionService.ts`:
- Around line 145-150: The checks for loop detection should short-circuit so
only the first detector that returns true sets loopDetected and triggers
logLoopDetected/lastLoopType; keep trackToolCall(event.value) called as before
but run checkToolCallLoop(event.value) first and if it returns true set
this.loopDetected and skip calling checkReadFileLoop() and
checkActionStagnation(), otherwise proceed to the next check(s); ensure you
update the control flow around checkToolCallLoop, checkReadFileLoop, and
checkActionStagnation so they do not all run once one has already signaled a
loop (prevent multiple logLoopDetected calls and overwriting lastLoopType).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0f5bf17-387c-4136-9793-7fcc2f4ddc08
📒 Files selected for processing (7)
packages/cli/src/nonInteractiveCli.tspackages/core/src/core/client.tspackages/core/src/core/turn.tspackages/core/src/index.tspackages/core/src/services/loopDetectionService.test.tspackages/core/src/services/loopDetectionService.tspackages/core/src/telemetry/types.ts
Comment on lines
+380
to
+382
| if (event.type === GeminiEventType.LoopDetected) { | ||
| emitLoopDetectedMessage(config, event.value?.loopType); | ||
| } |
There was a problem hiding this comment.
Handle loop-detected events in the cron stream path too.
This only covers the primary responseStream loop. The cronStream loop further down still just forwards GeminiEventType.LoopDetected to the adapter, so TEXT-mode cron runs can halt without the new stderr explanation.
Suggested fix
for await (const event of cronStream) {
if (abortController.signal.aborted) {
const summary = scheduler.getExitSummary();
scheduler.stop();
if (summary) {
process.stderr.write(summary + '\n');
}
resolve();
return;
}
adapter.processEvent(event);
if (event.type === GeminiEventType.ToolCallRequest) {
cronToolCallRequests.push(event.value);
}
+ if (event.type === GeminiEventType.LoopDetected) {
+ emitLoopDetectedMessage(config, event.value?.loopType);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/nonInteractiveCli.ts` around lines 380 - 382, The cronStream
path does not handle GeminiEventType.LoopDetected like the responseStream does;
update the cronStream event loop (the code handling events from cronStream) to
call emitLoopDetectedMessage(config, event.value?.loopType) when event.type ===
GeminiEventType.LoopDetected instead of simply forwarding the event to the
adapter so TEXT-mode cron runs also print the loop explanation; locate the
cronStream event handler in nonInteractiveCli.ts and mirror the same conditional
and call used in the responseStream branch to ensure consistent behavior.
Comment on lines
+145
to
+150
| const toolCallLoop = this.checkToolCallLoop(event.value); | ||
| this.trackToolCall(event.value); | ||
| const readFileLoop = this.checkReadFileLoop(); | ||
| const actionStagnation = this.checkActionStagnation(); | ||
|
|
||
| this.loopDetected = toolCallLoop || readFileLoop || actionStagnation; |
There was a problem hiding this comment.
Short-circuit once one loop detector fires.
These checks currently keep running after the first true, so one tool call can emit multiple logLoopDetected() events and overwrite lastLoopType. A concrete case is the 8th read_file after a non-read primer: checkReadFileLoop() fires, then checkActionStagnation() fires and replaces the surfaced reason with ACTION_STAGNATION.
Suggested fix
- const toolCallLoop = this.checkToolCallLoop(event.value);
- this.trackToolCall(event.value);
- const readFileLoop = this.checkReadFileLoop();
- const actionStagnation = this.checkActionStagnation();
-
- this.loopDetected = toolCallLoop || readFileLoop || actionStagnation;
+ if (this.checkToolCallLoop(event.value)) {
+ this.loopDetected = true;
+ break;
+ }
+
+ this.trackToolCall(event.value);
+
+ if (this.checkReadFileLoop()) {
+ this.loopDetected = true;
+ break;
+ }
+
+ if (this.checkActionStagnation()) {
+ this.loopDetected = true;
+ }
break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/services/loopDetectionService.ts` around lines 145 - 150,
The checks for loop detection should short-circuit so only the first detector
that returns true sets loopDetected and triggers logLoopDetected/lastLoopType;
keep trackToolCall(event.value) called as before but run
checkToolCallLoop(event.value) first and if it returns true set
this.loopDetected and skip calling checkReadFileLoop() and
checkActionStagnation(), otherwise proceed to the next check(s); ensure you
update the control flow around checkToolCallLoop, checkReadFileLoop, and
checkActionStagnation so they do not all run once one has already signaled a
loop (prevent multiple logLoopDetected calls and overwriting lastLoopType).
Comment on lines
+510
to
+520
| private checkActionStagnation(): boolean { | ||
| if (this.sameNameStreak >= STAGNATION_THRESHOLD) { | ||
| this.lastLoopType = LoopType.ACTION_STAGNATION; | ||
| logLoopDetected( | ||
| this.config, | ||
| new LoopDetectedEvent(LoopType.ACTION_STAGNATION, this.promptId), | ||
| ); | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Honor the cold-start read exemption in stagnation detection too.
The read-loop cold-start guard only applies to checkReadFileLoop(). An initial burst of read_file calls still trips ACTION_STAGNATION here, so a legitimate “summarize this project” exploration can halt before any non-read action ever happens.
Suggested fix
private checkActionStagnation(): boolean {
+ if (
+ !this.hasSeenNonReadTool &&
+ this.lastSeenToolName !== null &&
+ this.isReadLikeTool(this.lastSeenToolName)
+ ) {
+ return false;
+ }
+
if (this.sameNameStreak >= STAGNATION_THRESHOLD) {
this.lastLoopType = LoopType.ACTION_STAGNATION;
logLoopDetected(
this.config,
new LoopDetectedEvent(LoopType.ACTION_STAGNATION, this.promptId),
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Backport from upstream QwenLM/qwen-code: enhanced loop detection with new stagnation + validation-retry checks, plus
lastLoopTypefield on the LoopDetectedEvent payload for observability.feat(core): enhanced loop detection with stagnation + validation-retry checksWhat changes
LoopTypeenum values for the various loop categories (consecutive identical tool calls, chanting, repetitive thoughts, read-file loop, action stagnation).LoopDetectionService.lastLoopTypefield +getLastLoopType()getter so consumers can surface why a loop was flagged.Adaptations from upstream
lastLoopType = LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLSassignment to bridge.coreToolScheduler.ts) — it referencesvalidationRetryCountsfield that depends on the un-ported feat(core): detect tool validation retry loops and inject stop directive QwenLM/qwen-code#3178 (retry-loop directive injection). The lastLoopType labelling is the meaningful observability win; the retry pruning is incremental.coreToolScheduler.test.tsIDE interaction + retry directive tests) — they assert behavior of the feat(core): detect tool validation retry loops and inject stop directive QwenLM/qwen-code#3178 retry machinery we didn't port.nonInteractiveClidrain logic — referencesLocalQueueItemandSendMessageType.Notificationfrom the un-ported background-agents subsystem.What was tried but skipped
Originally targeted QwenLM#3297 (lazy tool registry + inflight dedup) and QwenLM#3313 (truncated tool call recovery) for this branch. Both aborted:
ESCALATED_MAX_TOKENSconstant added by a prior un-ported upstream PR; plus a 158-line conflict ingeminiChat.ts. Filed under issue upstream port: tool flow correctness fixes (#3313, #3297) #189.Test plan
npm run typecheckclean across all workspacesvitest runfor affected files: 120/120 pass (loopDetectionService, turn, coreToolScheduler)🤖 Generated with Claude Code
Summary by CodeRabbit