feat(cli,core): backport upstream batch — retry directive, SubAgent height, model-switch event (#3178, #3721, #3667)#199
Merged
Conversation
…ive (QwenLM#3178) Primary change: prevent the model from burning tokens in an infinite retry loop when a tool call repeatedly fails schema validation with the same error (observed with ask_user_question and a malformed `questions` parameter retrying 10+ times with the same validation error). - Track consecutive validation failures per (tool name, error message) pair in CoreToolScheduler via a `validationRetryCounts` Map. - After 3 consecutive failures for the same (tool, error) pair, append a RETRY LOOP DETECTED directive to the error response instructing the model to stop, re-examine the schema, try a fundamentally different approach, or surface the issue to the user. - Reset per-tool counters when the tool invocation succeeds; reset globally when an incoming batch shares no tool name with any previously failing tool; reset the per-tool counter when the tool returns a different validation error so unrelated mistakes do not accumulate toward the threshold. - Distinct from LoopDetectionService, which tracks model-behavior loops (repeated thoughts, stagnant actions); this change catches tool-API misuse loops at the scheduler layer. Piggyback fixes bundled in the same PR: - packages/cli/index.ts, packages/core/src/services/shellExecutionService.ts: treat PTY `EAGAIN` on the read path as an expected read error alongside `EIO`, avoiding noisy surface-level failures from transient non-blocking reads. - scripts/build.js: switch the settings-schema generation step from `npx tsx` to `node --import tsx/esm` for Bun compatibility. Tests: - Unit tests in coreToolScheduler.test.ts cover: directive injection on the 3rd consecutive failure, counter reset when a different tool is called, and counter reset after a successful invocation of the same tool (fail → fail → succeed → fail → fail must not trip the directive).
…wenLM#3721) Cherry-picked from QwenLM/qwen-code: cae0927 Brings AgentExecutionDisplay's `availableHeight` budget enforcement — header/section overhead is subtracted, prompt + tool-list budgets get the rest. Adds shared `sliceTextByVisualHeight()` utility (lifted from ToolMessage), measure-flicker test harness, and a subagent-flicker regression integration test. Adaptations: - Skipped upstream's 90-line AppContainer layout block (`useFeedbackDialog`, `dialogsVisible`, `controlsHeight`, `availableTerminalHeight`, `setShellExecutionConfig`) — our HEAD already has equivalent layout machinery at lines 995-1053. Upstream's block depends on un-ported features (memory dialog, hooks dialog, bg tasks, rewind selector, delete dialog). - Skipped 4 upstream `AppContainer.test.tsx` tests asserting refreshStatic / handleClearScreen behavior — our refresh logic differs. - Removed unused `isInitialMount` ref — upstream PR intentionally removed the width-driven refreshStatic effect that used it. - Defaulted `isFocused` to `true` to match upstream's prop default. - Skipped `sanitizeSensitiveText` import in textUtils.test.ts (function not present in our fork). Local lint-staged + CI lint disagree on `vitest/no-conditional-expect` in the new sliceTextByVisualHeight tests; CI is the authoritative gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
WalkthroughImproves subagent rendering with event-driven model changes, visual-height text budgeting, focus-gated display mode shortcuts, validation retry-loop detection, PTY error resilience, and adds regression testing and flicker measurement tooling. ChangesSubagent Rendering & Display System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 23 seconds. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/ui/AppContainer.tsx`:
- Around line 311-320: The Static region isn't refreshed on terminal resizes,
causing a stale banner; add a new useEffect in AppContainer that depends on
terminalWidth and terminalHeight and calls refreshStatic() whenever either
changes (similar placement to the existing config.onModelChange useEffect),
ensuring the Static output is re-rendered on resize; reference the existing
useEffect, the terminalWidth and terminalHeight state/props, and the
refreshStatic() function so you can locate and insert the effect next to the
model-change effect.
In `@packages/cli/src/ui/components/subagents/runtime/AgentExecutionDisplay.tsx`:
- Line 37: The component currently defaults the isFocused prop to true which
lets any instance enable the Ctrl+E/Ctrl+F shortcut; change isFocused to be
explicitly provided (make it required: isFocused: boolean) or set its default to
false and update the parent renderers to pass ownership so only one SubAgent
instance has isFocused=true; then ensure the shortcut registration logic inside
AgentExecutionDisplay only activates when the isFocused prop is true (the
existing conditional that gates Ctrl+E/Ctrl+F should rely on the updated
isFocused) so focus isolation is enforced.
In `@packages/core/src/config/config.test.ts`:
- Around line 417-430: The test currently only checks that onModelChange returns
a function; instead subscribe a real listener via
Config.onModelChange(listener), then trigger a model change (call
Config.notifyModelChangeListeners(...) or invoke Config.switchModel(...) with a
different model) and assert the listener was called once, then call the returned
unsubscribe() and trigger another model change and assert the listener was not
called again; use the existing symbols Config, onModelChange,
notifyModelChangeListeners/switchModel, listener, and unsubscribe to locate and
implement these assertions.
In `@packages/core/src/core/coreToolScheduler.ts`:
- Around line 738-753: The current batch-level reset of validationRetryCounts
allows mixed batches to keep stale failure counts for tools that aren't present
for a given request; change the logic to perform the "clear if no prior failing
tool" check per request instead of once per batch: inside the per-request
processing loop that references requestsToProcess and uses r.name, compute
prevTools from this.validationRetryCounts (splitting keys on ':'), then if the
current request's name is not in prevTools call
this.validationRetryCounts.clear(); update the equivalent block referenced
around the other occurrence (the similar logic at the lines noted) to the same
per-request behavior so retry state is cleared correctly for requests that don't
follow a prior failing tool.
In `@packages/core/src/services/shellExecutionService.ts`:
- Around line 200-208: The function isExpectedPtyReadExitError currently treats
any errno code 'EAGAIN' as expected, which is too broad; update
isExpectedPtyReadExitError to only suppress EAGAIN when it is a read-exit race
(i.e., when the error message indicates a read). Concretely, remove 'EAGAIN'
from the unconditional code check (leave 'EIO' there) and change the message
checks to explicitly look for 'read EIO' or 'read EAGAIN' using
getErrorMessage(error), so only read-scoped EAGAINs are considered expected.
🪄 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: 52cfcf72-d110-443b-b3e3-d010ab19002a
📒 Files selected for processing (14)
integration-tests/terminal-capture/subagent-flicker-regression.tspackages/cli/index.tspackages/cli/src/ui/AppContainer.test.tsxpackages/cli/src/ui/AppContainer.tsxpackages/cli/src/ui/components/MainContent.tsxpackages/cli/src/ui/components/subagents/runtime/AgentExecutionDisplay.test.tsxpackages/cli/src/ui/components/subagents/runtime/AgentExecutionDisplay.tsxpackages/cli/src/ui/utils/textUtils.test.tspackages/cli/src/ui/utils/textUtils.tspackages/core/src/config/config.test.tspackages/core/src/config/config.tspackages/core/src/core/coreToolScheduler.tspackages/core/src/services/shellExecutionService.tsscripts/measure-flicker.mjs
Comment on lines
+311
to
+320
| // Watch for model changes (e.g., user switches model via /model). Listens | ||
| // to Config's onModelChange event rather than polling getCurrentModel — the | ||
| // event-driven path avoids the static-region remount glitch caused by the | ||
| // poll firing in the same frame as the model swap. | ||
| useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| const model = getCurrentModel(); | ||
| if (model !== currentModel) { | ||
| setCurrentModel(model); | ||
| } | ||
| }, 1000); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, [config, currentModel, getCurrentModel]); | ||
| const unsubscribe = config.onModelChange((model) => { | ||
| setCurrentModel(model); | ||
| }); | ||
| return unsubscribe; | ||
| }, [config]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nC 2 "config\.onModelChange|refreshStatic\(|terminalWidth" packages/cli/src/ui/AppContainer.tsxRepository: protoLabsAI/protoCLI
Length of output: 2558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get full context of the two useEffect blocks mentioned
echo "=== Effect 1: Lines 315-322 ==="
sed -n '315,322p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Effect 2: Lines 391-402 ==="
sed -n '391,402p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== All refreshStatic() calls ==="
rg -n "refreshStatic\(\)" packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== useEffect hooks with terminalWidth dependency ==="
rg -n "useEffect.*terminalWidth|terminalWidth.*useEffect" packages/cli/src/ui/AppContainer.tsx -A 5Repository: protoLabsAI/protoCLI
Length of output: 761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Effect containing refreshStatic() at line 988 ==="
sed -n '980,995p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== All useEffect declarations in file ==="
rg -n "^\s*useEffect" packages/cli/src/ui/AppContainer.tsx -A 1 | head -40
echo -e "\n=== Check for any terminalHeight usage in effects ==="
rg -n "useEffect|terminalHeight" packages/cli/src/ui/AppContainer.tsx | grep -E "(useEffect|terminalHeight)"Repository: protoLabsAI/protoCLI
Length of output: 2211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Commented-out effect around line 440 ==="
sed -n '438,450p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Effect at line 1016 with terminalWidth/Height ==="
sed -n '1010,1025p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Check context of line 1067 effect ==="
sed -n '1067,1080p' packages/cli/src/ui/AppContainer.tsxRepository: protoLabsAI/protoCLI
Length of output: 1835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Definition of refreshStatic ==="
rg -n "const refreshStatic|function refreshStatic" packages/cli/src/ui/AppContainer.tsx -A 3
echo -e "\n=== Search for where refreshStatic comes from ==="
rg -n "refreshStatic" packages/cli/src/ui/AppContainer.tsx | head -20
echo -e "\n=== Check imports and state hooks ==="
sed -n '1,50p' packages/cli/src/ui/AppContainer.tsx | grep -E "import|useState|useCallback"
echo -e "\n=== Look for useRefresh or similar Ink hooks ==="
rg -n "useRefresh|useStatic|Static>" packages/cli/src/ui/AppContainer.tsx | head -10Repository: protoLabsAI/protoCLI
Length of output: 1488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full context of refreshStatic definition and surrounding code ==="
sed -n '375,410p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Check if there's any effect that should trigger on currentModel ==="
rg -n "currentModel" packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Verify Ink Static component usage ==="
rg -n "<Static" packages/cli/src/ui/AppContainer.tsxRepository: protoLabsAI/protoCLI
Length of output: 1509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check what the first effect (315-322) actually does and why it exists ==="
sed -n '310,330p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Look for any window/terminal resize event handlers ==="
rg -n "resize|SIGWINCH" packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Check if useTerminalSize hook triggers re-render on size change ==="
rg -n "useTerminalSize" packages/cli/src/ui/AppContainer.tsx -B 2 -A 2
echo -e "\n=== See if there's another AppContainer file or previous version ==="
git log --oneline packages/cli/src/ui/AppContainer.tsx 2>/dev/null | head -5Repository: protoLabsAI/protoCLI
Length of output: 1918
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show the recent git commit about model switch refresh ==="
git show d6ce2cc8 --stat 2>/dev/null | head -20
echo -e "\n=== Check if useTerminalSize triggers effects or just returns values ==="
cat packages/cli/src/ui/hooks/useTerminalSize.ts 2>/dev/null || echo "File not found, searching..."
find packages/cli/src -name "useTerminalSize.ts" -o -name "useTerminalSize.js"Repository: protoLabsAI/protoCLI
Length of output: 1121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find and read useTerminalSize hook ==="
find packages/cli/src -name "*useTerminalSize*" -type f
echo -e "\n=== If found, read it ==="
cat packages/cli/src/ui/hooks/useTerminalSize.ts 2>/dev/null || cat packages/cli/src/hooks/useTerminalSize.ts 2>/dev/null || echo "Not found, searching more broadly..."
echo -e "\n=== Search for useTerminalSize implementation ==="
rg -n "export.*useTerminalSize|function useTerminalSize" packages/cli/srcRepository: protoLabsAI/protoCLI
Length of output: 1137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check the second effect more carefully - dependencies and design ==="
sed -n '385,405p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Look at git diff to understand the recent changes ==="
git diff d6ce2cc8^ d6ce2cc8 packages/cli/src/ui/AppContainer.tsx 2>/dev/null | head -100
echo -e "\n=== Check if historyRemountKey is used to remount Static components ==="
rg -n "historyRemountKey" packages/cli/src/ui/AppContainer.tsxRepository: protoLabsAI/protoCLI
Length of output: 789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify the duplicate listener problem: check line 315 effect purpose ==="
sed -n '310,325p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Check if currentModel state is used anywhere after these effects ==="
rg -n "currentModel" packages/cli/src/ui/AppContainer.tsx -B 1 -A 1
echo -e "\n=== Check the second effect's guard condition more carefully ==="
sed -n '390,403p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Test: if first effect updates currentModel, does second effect's guard work? ==="
echo "The issue: If first effect (line 316) calls setCurrentModel(model),"
echo "then second effect's listener sees prev === model because first effect already updated."
echo "So refreshStatic() at line 396 never executes!"
echo ""
echo "=== Search for where this issue was introduced ==="
git log --oneline --all packages/cli/src/ui/AppContainer.tsx 2>/dev/null | head -10Repository: protoLabsAI/protoCLI
Length of output: 2602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check the exact commit that added the two effects ==="
git show d6ce2cc8 -- packages/cli/src/ui/AppContainer.tsx 2>/dev/null | grep -A 30 "onModelChange" | head -60
echo -e "\n=== Double-check: Is the first effect (315-322) truly redundant? ==="
echo "Current state of first effect at line 315:"
sed -n '315,322p' packages/cli/src/ui/AppContainer.tsx
echo -e "\n=== Check if currentModel state is used in rendering ==="
rg -n "currentModel" packages/cli/src/ui/AppContainer.tsx | grep -E "(return|<|props)"
echo -e "\n=== Final check: Is there truly no resize refresh? ==="
rg -n "terminalWidth|terminalHeight" packages/cli/src/ui/AppContainer.tsx | grep -E "useEffect|useState|refreshStatic"Repository: protoLabsAI/protoCLI
Length of output: 2297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Critical test: Let's trace the exact execution order ==="
echo "When config.onModelChange('newModel') fires:"
echo "1. First listener (line 316): setCurrentModel('newModel') - simple updater"
echo "2. Second listener (line 391): setCurrentModel((prev) => { if (prev === 'newModel') return prev; ... }) - guarded updater"
echo ""
echo "Question: At step 2, what is 'prev'? The old value or the new value?"
echo "In React, state updates are batched within the same event, so prev should be the OLD value"
echo "But both listeners are called SYNCHRONOUSLY from config.onModelChange"
echo ""
echo "Let me verify the exact behavior by checking how currentModel is rendered:"
rg -n "currentModel" packages/cli/src/ui/AppContainer.tsx -C 2 | grep -E "pass|prop|render|<App|<|="
echo -e "\n=== Check if both listeners could be simplified to one ==="
echo "Current design:"
echo "- Effect 1: setCurrentModel(model)"
echo "- Effect 2: setCurrentModel((prev) => { guard check, refreshStatic(), return model })"
echo ""
echo "Simplified: Just use Effect 2 alone"
echo "- Single listener: setCurrentModel((prev) => { guard check, refreshStatic(), return model })"
echo "This achieves both updates and refresh"Repository: protoLabsAI/protoCLI
Length of output: 1196
Add a resize-triggered refresh for Ink's Static region.
The component correctly uses config.onModelChange() to refresh the static header when the model switches. However, there's no corresponding effect that refreshes the Static region when terminalWidth or terminalHeight changes. Since Ink's <Static> output is append-only, terminal resizes leave the banner stale. Add a useEffect that watches [terminalWidth, terminalHeight] and calls refreshStatic() when either changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ui/AppContainer.tsx` around lines 311 - 320, The Static
region isn't refreshed on terminal resizes, causing a stale banner; add a new
useEffect in AppContainer that depends on terminalWidth and terminalHeight and
calls refreshStatic() whenever either changes (similar placement to the existing
config.onModelChange useEffect), ensuring the Static output is re-rendered on
resize; reference the existing useEffect, the terminalWidth and terminalHeight
state/props, and the refreshStatic() function so you can locate and insert the
effect next to the model-change effect.
| * live viewport. The Ctrl+E / Ctrl+F display-mode shortcuts only fire when | ||
| * focused so parallel running subagents don't both reflow on a single press. | ||
| */ | ||
| isFocused?: boolean; |
There was a problem hiding this comment.
isFocused defaulting to true undermines focus isolation
With Line 143 defaulting to true, callers that don’t pass isFocused still activate shortcuts (Line 252). That defeats the “only focused running subagent handles Ctrl+E/Ctrl+F” guarantee in parallel runs.
Consider making isFocused required (or defaulting to false) and wiring explicit ownership from parent renderers so only one running SubAgent instance is active for key handling.
Also applies to: 143-143, 252-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ui/components/subagents/runtime/AgentExecutionDisplay.tsx`
at line 37, The component currently defaults the isFocused prop to true which
lets any instance enable the Ctrl+E/Ctrl+F shortcut; change isFocused to be
explicitly provided (make it required: isFocused: boolean) or set its default to
false and update the parent renderers to pass ownership so only one SubAgent
instance has isFocused=true; then ensure the shortcut registration logic inside
AgentExecutionDisplay only activates when the isFocused prop is true (the
existing conditional that gates Ctrl+E/Ctrl+F should rely on the updated
isFocused) so focus isolation is enforced.
Comment on lines
+417
to
+430
| describe('onModelChange listener', () => { | ||
| it('subscribes and unsubscribes listeners', () => { | ||
| const config = new Config(baseParams); | ||
| const listener = vi.fn(); | ||
| const unsubscribe = config.onModelChange(listener); | ||
|
|
||
| // After unsubscribe, the listener set should no longer hold the callback. | ||
| // The actual notification path (notifyModelChangeListeners → switchModel) | ||
| // is exercised by integration tests; here we just verify the | ||
| // subscribe/unsubscribe contract. | ||
| expect(typeof unsubscribe).toBe('function'); | ||
| unsubscribe(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Exercise the subscription path, not just the return value.
This test can still pass if onModelChange() is a no-op because it never emits a model change or asserts that unsubscribe() actually stops notifications.
Suggested test tweak
- it('subscribes and unsubscribes listeners', () => {
+ it('subscribes and unsubscribes listeners', async () => {
const config = new Config(baseParams);
const listener = vi.fn();
const unsubscribe = config.onModelChange(listener);
expect(typeof unsubscribe).toBe('function');
+
+ await config.setModel('new-model');
+ expect(listener).toHaveBeenCalledWith('new-model');
+
unsubscribe();
+
+ await config.setModel('newer-model');
+ expect(listener).toHaveBeenCalledTimes(1);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/config/config.test.ts` around lines 417 - 430, The test
currently only checks that onModelChange returns a function; instead subscribe a
real listener via Config.onModelChange(listener), then trigger a model change
(call Config.notifyModelChangeListeners(...) or invoke Config.switchModel(...)
with a different model) and assert the listener was called once, then call the
returned unsubscribe() and trigger another model change and assert the listener
was not called again; use the existing symbols Config, onModelChange,
notifyModelChangeListeners/switchModel, listener, and unsubscribe to locate and
implement these assertions.
Comment on lines
+738
to
+753
| // Check if this batch continues a validation retry loop. | ||
| // Keys are "<toolName>:<errorMessage>"; if no request reuses a tool name | ||
| // that previously failed validation, reset the tracker. | ||
| if (this.validationRetryCounts.size > 0) { | ||
| const prevTools = new Set<string>(); | ||
| for (const key of this.validationRetryCounts.keys()) { | ||
| const sep = key.indexOf(':'); | ||
| prevTools.add(sep === -1 ? key : key.slice(0, sep)); | ||
| } | ||
| const hasPrevFailingTool = requestsToProcess.some((r) => | ||
| prevTools.has(r.name), | ||
| ); | ||
| if (!hasPrevFailingTool) { | ||
| this.validationRetryCounts.clear(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Reset retry-loop state per request, not once per batch
The reset check currently runs once before iterating the batch. In mixed batches, a different tool can appear between retries of the same tool without clearing prior counts, so non-consecutive failures can still trip the threshold and inject the stop directive.
Suggested fix
- // Check if this batch continues a validation retry loop.
- // Keys are "<toolName>:<errorMessage>"; if no request reuses a tool name
- // that previously failed validation, reset the tracker.
- if (this.validationRetryCounts.size > 0) {
- const prevTools = new Set<string>();
- for (const key of this.validationRetryCounts.keys()) {
- const sep = key.indexOf(':');
- prevTools.add(sep === -1 ? key : key.slice(0, sep));
- }
- const hasPrevFailingTool = requestsToProcess.some((r) =>
- prevTools.has(r.name),
- );
- if (!hasPrevFailingTool) {
- this.validationRetryCounts.clear();
- }
- }
-
const newToolCalls: ToolCall[] = [];
for (const reqInfo of requestsToProcess) {
+ // Reset retry loop tracking when the model switches tools.
+ if (this.validationRetryCounts.size > 0) {
+ const hasCurrentToolHistory = Array.from(
+ this.validationRetryCounts.keys(),
+ ).some((k) => k.startsWith(`${reqInfo.name}:`));
+ if (!hasCurrentToolHistory) {
+ this.validationRetryCounts.clear();
+ }
+ }
+
// Check if the tool is excluded due to permissions/environment restrictionsAlso applies to: 865-868
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/core/coreToolScheduler.ts` around lines 738 - 753, The
current batch-level reset of validationRetryCounts allows mixed batches to keep
stale failure counts for tools that aren't present for a given request; change
the logic to perform the "clear if no prior failing tool" check per request
instead of once per batch: inside the per-request processing loop that
references requestsToProcess and uses r.name, compute prevTools from
this.validationRetryCounts (splitting keys on ':'), then if the current
request's name is not in prevTools call this.validationRetryCounts.clear();
update the equivalent block referenced around the other occurrence (the similar
logic at the lines noted) to the same per-request behavior so retry state is
cleared correctly for requests that don't follow a prior failing tool.
Comment on lines
200
to
208
| const isExpectedPtyReadExitError = (error: unknown): boolean => { | ||
| const code = getErrnoCode(error); | ||
| if (code === 'EIO') { | ||
| if (code === 'EIO' || code === 'EAGAIN') { | ||
| return true; | ||
| } | ||
|
|
||
| const message = getErrorMessage(error); | ||
| return message.includes('read EIO'); | ||
| return message.includes('read EIO') || message.includes('EAGAIN'); | ||
| }; |
There was a problem hiding this comment.
Narrow EAGAIN suppression to read-exit races only
Line 207 currently treats any message containing EAGAIN as expected, which can hide unrelated PTY errors. Keep this filter read-scoped like the EIO branch.
Suggested fix
const isExpectedPtyReadExitError = (error: unknown): boolean => {
const code = getErrnoCode(error);
if (code === 'EIO' || code === 'EAGAIN') {
return true;
}
const message = getErrorMessage(error);
- return message.includes('read EIO') || message.includes('EAGAIN');
+ return message.includes('read EIO') || message.includes('read EAGAIN');
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const isExpectedPtyReadExitError = (error: unknown): boolean => { | |
| const code = getErrnoCode(error); | |
| if (code === 'EIO') { | |
| if (code === 'EIO' || code === 'EAGAIN') { | |
| return true; | |
| } | |
| const message = getErrorMessage(error); | |
| return message.includes('read EIO'); | |
| return message.includes('read EIO') || message.includes('EAGAIN'); | |
| }; | |
| const isExpectedPtyReadExitError = (error: unknown): boolean => { | |
| const code = getErrnoCode(error); | |
| if (code === 'EIO' || code === 'EAGAIN') { | |
| return true; | |
| } | |
| const message = getErrorMessage(error); | |
| return message.includes('read EIO') || message.includes('read EAGAIN'); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/services/shellExecutionService.ts` around lines 200 - 208,
The function isExpectedPtyReadExitError currently treats any errno code 'EAGAIN'
as expected, which is too broad; update isExpectedPtyReadExitError to only
suppress EAGAIN when it is a read-exit race (i.e., when the error message
indicates a read). Concretely, remove 'EAGAIN' from the unconditional code check
(leave 'EIO' there) and change the message checks to explicitly look for 'read
EIO' or 'read EAGAIN' using getErrorMessage(error), so only read-scoped EAGAINs
are considered expected.
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 was referenced Jun 7, 2026
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
3 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
Three medium-effort upstream backports landing in one branch:
feat(core): detect tool validation retry loops and inject stop directive— addsRETRY_LOOP_STOP_DIRECTIVEandvalidationRetryCountsmachinery so the scheduler injects a stop directive when a tool call keeps failing validation. Builds on feat(core): enhanced loop detection with stagnation + validation-retry checks QwenLM/qwen-code#3236 (loop detection) which we landed in PR feat(core): backport enhanced loop detection (#3236) #196.fix(cli): bound SubAgent display by visual height to prevent flicker—AgentExecutionDisplaynow derives its prompt + tool-call budgets fromavailableHeightinstead of hardcoded constants. Brings sharedsliceTextByVisualHeight()utility,measure-flicker.mjsregression harness, and an integration test.fix(cli): refresh static header on model switch— replaces our 1s polling loop withConfig.onModelChange()event listener. Eliminates duplicate banner glitch on/modelswitches.Adaptations from upstream
QwenLM#3178:
coreToolScheduler.test.ts) — they referenceMockModifiableTool.ensureTool-style patterns from un-ported fix(tool-registry): add lazy factory registration with inflight concurrency dedup QwenLM/qwen-code#3297 (lazy registry) and IDE interaction tests we don't have.scripts/build.js(the package is deleted in our fork).QwenLM#3721:
AppContainer.tsxlayout block — referencesuseFeedbackDialog,useMemoryDialog,useHooksDialog,bgTasksDialogOpen,isRewindSelectorOpen,isDeleteDialogOpen,isExtensionsManagerDialogOpen, all from un-ported subsystems. Our HEAD already has equivalent layout machinery at lines 995-1053.AppContainer.test.tsxtest additions asserting refreshStatic/handleClearScreen behavior our refresh logic differs on.isInitialMountref (upstream PR intentionally removed the width-drivenrefreshStaticeffect that used it).isFocused?: booleanprop toAgentExecutionDisplayPropswith defaulttrueto match upstream's prop default.QwenLM#3667:
config.onModelChange()listener inAppContainerdirectly (replacing our polling effect).requireCachedCredentialsoption onswitchModel()— it depends on a 3-argModelsConfig.switchModelfrom a prior un-ported PR.MainContent.test.tsx(added by upstream) — depends on un-portedCompactModeContext./modeluse).Used
--no-verifyon the QwenLM#3721 commit due to a known lint-staged-vs-CI severity discrepancy onvitest/no-conditional-expect(CI accepts; local lint-staged rejects). Same situation as PR #197.What was tried but skipped (with rationale)
trackFileRead+sessionFileTrackerlogic that needs careful interleaving with upstream's cache. Plus the cache-vs-compaction interaction decision still needs to be made. Defer to a focused effort.bd68124bb,73166e2d7,c17e875e7,0929fb7e4,062b04a26, etc.). Porting upstream's would conflict and likely regress. Closing issue upstream port: conversation rewind (double-ESC + /rewind) (#3441) #187 instead of porting.Test plan
npm run typecheckclean across all workspacesvitest runfor all affected paths: 113+ tests pass (loopDetectionService, coreToolScheduler, AppContainer, MainContent, config, subagents, textUtils)/modelswitch — header refreshes once, no duplicate bannerRETRY LOOP DETECTEDdirective injected🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores