fix(cli): bound SubAgent display by visual height to prevent flicker#3721
Merged
Conversation
added 2 commits
April 29, 2026 10:58
The SubAgent runtime display used hard-coded MAX_TASK_PROMPT_LINES=5 and MAX_TOOL_CALLS=5 plus character-length truncation (`length > 80`). On narrow terminals the soft-wrapped content overflowed the available height as the tool-call list grew, forcing Ink to clear and redraw on every update. Pull AgentExecutionDisplay onto the same visual-height/visual-width slicing pattern that ToolMessage and ConversationMessages already use: - Add `sliceTextByVisualHeight` to textUtils — counts soft wraps as visual rows, supports top/bottom overflow direction. - AgentExecutionDisplay now derives maxTaskPromptLines / maxToolCalls from the assigned `availableHeight` and uses `truncateToVisualWidth` (CJK + emoji safe) instead of substring(0, 80). Compact mode is unchanged. - Drop the 300 ms debounced `refreshStatic` AppContainer fired on every terminalWidth change — that was a flicker source on resize and the static area no longer needs the refresh. Tests: - textUtils.test.ts covers undefined maxHeight, top/bottom overflow, and soft-wrap counting. - AgentExecutionDisplay.test.tsx asserts the height-bounded render keeps the prompt + tool list inside the assigned rows. - AppContainer.test.tsx asserts width-only changes no longer clear the terminal.
Two reusable tools for measuring TUI flicker:
- `scripts/measure-flicker.mjs` — standalone Node script that counts the
ANSI escape sequences which betray flicker (clearTerminalPair, clearScreen,
eraseLine, cursorUp) inside any recorded raw stream (`script` log,
`tmux pipe-pane` output, custom PTY capture). Supports baseline diff mode.
- `integration-tests/terminal-capture/subagent-flicker-regression.ts` —
end-to-end ratchet that boots a mock OpenAI server, drives a real qwen
process through an `agent` tool dispatch + 5 `read_file` SubAgent rounds,
then reads PTY bytes and asserts ANSI-redraw counts stay below configured
ceilings. Mirrors PR #43f128b20's resize-clear-regression pattern.
Reference numbers (60-col / 18-row terminal, fixed build):
clearTerminalPair=5, clearScreen=10, eraseLine=440, cursorUp=132
The ratchet defaults to 10/20 ceilings — roughly 2× steady state — so
regressions like reverting sliceTextByVisualHeight or restoring the
width-driven refreshStatic trip the build.
Implementation notes captured in the script's docstring:
- Strips HTTP_PROXY family env vars (NO_PROXY isn't honored by undici,
so corp proxy would otherwise hijack the loopback request).
- Drops `--bare` (bare mode hard-codes the registered tool set and
rejects the `agent` tool); HOME is sandboxed to a temp dir instead.
- Mock server speaks SSE because the CLI requests stream:true.
Contributor
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. |
wenshao
commented
Apr 29, 2026
Three issues from inline review on PR #3721: 1. **availableHeight as total budget (Critical).** The previous formula only constrained prompt + tool-call height, not the surrounding header / section labels / gaps / footer. Default and verbose mode could still overrun the parent-provided budget. Subtract a fixed-row overhead (10 rows running, 18 completed) before computing `maxTaskPromptLines` / `maxToolCalls`. Add unit tests that assert the rendered frame line-count stays within `availableHeight` for both running and completed states. 2. **Ratchet that actually distinguishes fix from no-fix.** The previous `clearTerminalPair` / `clearScreen` ceilings passed for both fixed and unfixed builds. Add an `eraseLine` upper bound (default 460) — that's the metric whose drop reflects the in-place-update efficiency the visual-height fix delivers (no-fix observed 469, with-fix 434). Refresh docstring with the current numbers and a coverage map that honestly states what this ratchet does and does not exercise. 3. **Keypress scope.** `useKeypress` was active on every mounted `AgentExecutionDisplay`, including completed/historical instances in chat history — Ctrl+E / Ctrl+F would toggle them all in lock-step and cause large scrollback reflows. Gate `isActive` on `data.status === 'running'`. Test mock now also honors `{ isActive }` so the new "completed displays ignore Ctrl+E" regression is enforceable.
wenshao
commented
Apr 29, 2026
Three follow-up issues from inline review on PR #3721: 1. **sliceTextByVisualHeight reservedRows early-return (Critical).** The early return compared `visualLineCount <= targetMaxHeight` and ignored `reservedRows`, so a caller asking us to keep one row free for a footer could still receive the full input back with `hiddenLinesCount: 0` even though only `targetMaxHeight - reservedRows` content rows were actually available. Compare against `visibleContentHeight` instead and add a regression test for the `'a\nb\nc' / 3 / reservedRows: 1` case the reviewer flagged. 2. **Footer hint and rendered prompt now share one slicing result (Suggestion).** Previously `hasMoreLines` looked at `data.taskPrompt.split('\n').length` (hard newlines only), but the prompt body was already truncated by `sliceTextByVisualHeight` (which counts soft wraps). A long single-line prompt could be visually truncated without the footer ever surfacing the "ctrl+f to show more" hint. Lift the slice into the parent component and feed both the rendered `TaskPromptSection` and the footer's `hasMoreLines` from the same `hiddenLinesCount`. 3. **Running → completed transition test (Critical).** The previous "completed displays ignore Ctrl+E" test rendered already-completed data, so `useKeypress` was inactive from the start and Ctrl+E was a no-op trivially. It missed the real path: a running subagent gets expanded, then completes while preserving the expanded `displayMode` — which is exactly when the completed-state budget has to hold the layout. Replace the test with a `rerender`-based one that runs the full transition, asserts the completed expanded frame stays within `availableHeight`, and asserts the post-transition Ctrl+E is a no-op. Bumped `COMPLETED_FIXED_OVERHEAD` from 18 to 22 to accommodate the ExecutionSummary + ToolUsage block accounting that the new transition test exposed.
yiliang114
reviewed
Apr 29, 2026
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Solid self-review loop — the overhead-aware budget, the isActive scoping, and the sliceTextByVisualHeight extraction all look good after the two rounds of fixes.
One remaining scope issue flagged inline on the keypress listener.
Per @yiliang114's review on PR #3721 — `data.status === 'running'` alone fixes the historical/scrollback case but two SubAgents running in parallel both stay `running`, so a single Ctrl+E / Ctrl+F still toggles them in lock-step and the dual reflow brings back the flicker the gating was meant to prevent. The component already receives `isFocused` from ToolMessage (via SubagentExecutionRenderer) for the inline confirmation prompt — reuse it on the keypress hook: isActive: data.status === 'running' && isFocused Adds a regression test that renders a running SubAgent with `isFocused={false}` and asserts Ctrl+E is a no-op (frame unchanged).
yiliang114
approved these changes
Apr 29, 2026
This was referenced Apr 30, 2026
mabry1985
added a commit
to protoLabsAI/protoCLI
that referenced
this pull request
May 2, 2026
…eight, model-switch event (QwenLM#3178, QwenLM#3721, QwenLM#3667) (#199) * feat(core): detect tool validation retry loops and inject stop directive (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). * fix(cli): bound SubAgent display by visual height to prevent flicker (QwenLM#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> * fix(cli): refresh static header on model switch (QwenLM#3667) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: euxaristia <25621994+euxaristia@users.noreply.github.com> Co-authored-by: Automaker <automaker@localhost> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: pomelo <czynwu@outlook.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…wenLM#3721) * fix(cli): bound SubAgent display by visual height to prevent flicker The SubAgent runtime display used hard-coded MAX_TASK_PROMPT_LINES=5 and MAX_TOOL_CALLS=5 plus character-length truncation (`length > 80`). On narrow terminals the soft-wrapped content overflowed the available height as the tool-call list grew, forcing Ink to clear and redraw on every update. Pull AgentExecutionDisplay onto the same visual-height/visual-width slicing pattern that ToolMessage and ConversationMessages already use: - Add `sliceTextByVisualHeight` to textUtils — counts soft wraps as visual rows, supports top/bottom overflow direction. - AgentExecutionDisplay now derives maxTaskPromptLines / maxToolCalls from the assigned `availableHeight` and uses `truncateToVisualWidth` (CJK + emoji safe) instead of substring(0, 80). Compact mode is unchanged. - Drop the 300 ms debounced `refreshStatic` AppContainer fired on every terminalWidth change — that was a flicker source on resize and the static area no longer needs the refresh. Tests: - textUtils.test.ts covers undefined maxHeight, top/bottom overflow, and soft-wrap counting. - AgentExecutionDisplay.test.tsx asserts the height-bounded render keeps the prompt + tool list inside the assigned rows. - AppContainer.test.tsx asserts width-only changes no longer clear the terminal. * test(tui): add SubAgent flicker regression script and ANSI counter Two reusable tools for measuring TUI flicker: - `scripts/measure-flicker.mjs` — standalone Node script that counts the ANSI escape sequences which betray flicker (clearTerminalPair, clearScreen, eraseLine, cursorUp) inside any recorded raw stream (`script` log, `tmux pipe-pane` output, custom PTY capture). Supports baseline diff mode. - `integration-tests/terminal-capture/subagent-flicker-regression.ts` — end-to-end ratchet that boots a mock OpenAI server, drives a real qwen process through an `agent` tool dispatch + 5 `read_file` SubAgent rounds, then reads PTY bytes and asserts ANSI-redraw counts stay below configured ceilings. Mirrors PR #43f128b20's resize-clear-regression pattern. Reference numbers (60-col / 18-row terminal, fixed build): clearTerminalPair=5, clearScreen=10, eraseLine=440, cursorUp=132 The ratchet defaults to 10/20 ceilings — roughly 2× steady state — so regressions like reverting sliceTextByVisualHeight or restoring the width-driven refreshStatic trip the build. Implementation notes captured in the script's docstring: - Strips HTTP_PROXY family env vars (NO_PROXY isn't honored by undici, so corp proxy would otherwise hijack the loopback request). - Drops `--bare` (bare mode hard-codes the registered tool set and rejects the `agent` tool); HOME is sandboxed to a temp dir instead. - Mock server speaks SSE because the CLI requests stream:true. * fix(cli): address inline review on SubAgent flicker fix Three issues from inline review on PR QwenLM#3721: 1. **availableHeight as total budget (Critical).** The previous formula only constrained prompt + tool-call height, not the surrounding header / section labels / gaps / footer. Default and verbose mode could still overrun the parent-provided budget. Subtract a fixed-row overhead (10 rows running, 18 completed) before computing `maxTaskPromptLines` / `maxToolCalls`. Add unit tests that assert the rendered frame line-count stays within `availableHeight` for both running and completed states. 2. **Ratchet that actually distinguishes fix from no-fix.** The previous `clearTerminalPair` / `clearScreen` ceilings passed for both fixed and unfixed builds. Add an `eraseLine` upper bound (default 460) — that's the metric whose drop reflects the in-place-update efficiency the visual-height fix delivers (no-fix observed 469, with-fix 434). Refresh docstring with the current numbers and a coverage map that honestly states what this ratchet does and does not exercise. 3. **Keypress scope.** `useKeypress` was active on every mounted `AgentExecutionDisplay`, including completed/historical instances in chat history — Ctrl+E / Ctrl+F would toggle them all in lock-step and cause large scrollback reflows. Gate `isActive` on `data.status === 'running'`. Test mock now also honors `{ isActive }` so the new "completed displays ignore Ctrl+E" regression is enforceable. * fix(cli): address round-2 inline review on SubAgent flicker Three follow-up issues from inline review on PR QwenLM#3721: 1. **sliceTextByVisualHeight reservedRows early-return (Critical).** The early return compared `visualLineCount <= targetMaxHeight` and ignored `reservedRows`, so a caller asking us to keep one row free for a footer could still receive the full input back with `hiddenLinesCount: 0` even though only `targetMaxHeight - reservedRows` content rows were actually available. Compare against `visibleContentHeight` instead and add a regression test for the `'a\nb\nc' / 3 / reservedRows: 1` case the reviewer flagged. 2. **Footer hint and rendered prompt now share one slicing result (Suggestion).** Previously `hasMoreLines` looked at `data.taskPrompt.split('\n').length` (hard newlines only), but the prompt body was already truncated by `sliceTextByVisualHeight` (which counts soft wraps). A long single-line prompt could be visually truncated without the footer ever surfacing the "ctrl+f to show more" hint. Lift the slice into the parent component and feed both the rendered `TaskPromptSection` and the footer's `hasMoreLines` from the same `hiddenLinesCount`. 3. **Running → completed transition test (Critical).** The previous "completed displays ignore Ctrl+E" test rendered already-completed data, so `useKeypress` was inactive from the start and Ctrl+E was a no-op trivially. It missed the real path: a running subagent gets expanded, then completes while preserving the expanded `displayMode` — which is exactly when the completed-state budget has to hold the layout. Replace the test with a `rerender`-based one that runs the full transition, asserts the completed expanded frame stays within `availableHeight`, and asserts the post-transition Ctrl+E is a no-op. Bumped `COMPLETED_FIXED_OVERHEAD` from 18 to 22 to accommodate the ExecutionSummary + ToolUsage block accounting that the new transition test exposed. * fix(cli): gate SubAgent useKeypress on isFocused for parallel runs Per @yiliang114's review on PR QwenLM#3721 — `data.status === 'running'` alone fixes the historical/scrollback case but two SubAgents running in parallel both stay `running`, so a single Ctrl+E / Ctrl+F still toggles them in lock-step and the dual reflow brings back the flicker the gating was meant to prevent. The component already receives `isFocused` from ToolMessage (via SubagentExecutionRenderer) for the inline confirmation prompt — reuse it on the keypress hook: isActive: data.status === 'running' && isFocused Adds a regression test that renders a running SubAgent with `isFocused={false}` and asserts Ctrl+E is a no-op (frame unchanged). --------- Co-authored-by: wenshao <wenshao@U-K7F6PQY3-2157.local>
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
The SubAgent runtime display (
AgentExecutionDisplay) used hard-codedMAX_TASK_PROMPT_LINES = 5/MAX_TOOL_CALLS = 5plus character-length truncation (firstLine.length > 80). On narrow terminals, soft-wrapped content overflowed the available height as the tool-call list grew, forcing Ink to clear and redraw on every update — visible flicker on every tool addition / mode switch / resize.This brings
AgentExecutionDisplayonto the same visual-height / visual-width slicing pattern thatToolMessageandConversationMessagesalready use after PRs #3591 and #3713, and removes a 300 ms width-drivenrefreshStaticinAppContainerthat was firing a full-screen clear on every resize event.What changed
packages/cli/src/ui/utils/textUtils.ts— new sharedsliceTextByVisualHeight(text, maxHeight, maxWidth, opts). Counts soft wraps as visual rows, supports `top` / `bottom` overflow direction. Same shape used by ToolMessage's slicer; this just lifts it out so SubAgent can share it.packages/cli/src/ui/components/subagents/runtime/AgentExecutionDisplay.tsx—Tests
Plus, in the second commit, two reusable validation tools:
Test plan