Repository navigation
fix(orchestration): return a subagent's screen as plain text - #191
Conversation
wait_for_agent, get_agent_result and observe_agent returned the raw PTY
tail, which for a full-screen TUI such as OpenCode is cursor moves,
colours and spinner redraws, often starting mid-sequence. They now render
the masked scrollback window (the last 65,536 characters) through a
headless xterm at the session's size and return the text it shows, masked
again and capped as before (observe_agent within its maxChars). A cut
window starts at its first escape or line break, so the rest of a cut
sequence ("18;9H") is never printed as text. Should rendering fail, the
masked raw tail is returned as before.
exitLines were the regex-stripped last 16 KiB, so a TUI that redraws only
the cells that changed read as its redraws joined together, and a line
drawn early and never redrawn (the model's status line) was lost. They are
now rendered from the same window, masked before and after.
observe() and result() become async; their callers await them.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf0760b2c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const starts = [window.indexOf("\u001b"), window.indexOf("\n")].filter((index) => index >= 0); | ||
| return starts.length > 0 ? window.slice(Math.min(...starts)) : window; |
There was a problem hiding this comment.
For a newline-terminated, non-ANSI line longer than 65,536 characters (e.g. minified JSON), the window starts with ordinary text but its first newline is at the very end. Slicing here reduces the entire source to \n, so the headless terminal renders nothing and all three agent tools return empty output, despite the last 8 KiB being available. Only discard a partial control sequence when one actually crosses the window boundary; ordinary text must not be discarded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 291de91. A cut window no longer starts at its first escape or line break. It is masked with 4 KiB of what precedes it, and starts earlier only when an escape sequence begun before the cut is still open there (CSI, OSC/DCS/APC strings, or a two-character escape): then at that sequence's escape, so xterm reads it whole. Otherwise it starts exactly at the cut and ordinary text is kept. Test: "a cut window of ordinary text keeps its text: only a control sequence crossing the cut is dropped" in tests/orchestration-providers-wait.test.mjs (one newline-terminated JSON line over 70,000 characters; wait_for_agent, observe_agent and get_agent_result must return its last 8,192 characters). It failed before the fix with empty output. The two existing cut-sequence tests (18;9H, a cut SGR) still pass.
| // Every row, scrollback included (sessionScreen's terminalScreenText reads only the visible screen of a live one). | ||
| const shown = rowsText(); | ||
| if (terminal.buffer.active.type === "alternate" || !leftScreen) return shown; | ||
| return shown ? `${leftScreen}\n${shown}` : leftScreen; |
There was a problem hiding this comment.
When more than 8,192 characters of normal-buffer history precede entry into the alternate screen, leaving that screen restores the old history into shown. This concatenation places the final TUI frame before those old logs, so screenText's character cap and the tail/line limits used for exitLines discard the final frame and its error message, returning startup history instead. The previous raw tail retained the recent error. Preserve chronological order by keeping old normal-buffer history before the saved frame and appending only output produced after leaving the alternate screen.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 291de91. A marker now records the normal-buffer row where the history ended as the TUI entered its alternate screen. After it leaves, the text is the history above the marker, then the saved frame, then the rows from the marker on (what was printed after leaving). Test: "leaving the full screen after long history keeps the order: history, the last frame, then what followed" (200 lines of history, over 8 KiB, then a frame with an error, then "Session ended."). output and exitLines must contain the error and end with "Session ended.". It failed before the fix: both returned only the old history and "Session ended.".
…order A window cut from the end of the scrollback started at its first escape or line break. For one newline-terminated line longer than the window (minified JSON, no escape) that left only the final "\n", so all three tools returned empty output. The window is now masked with 4 KiB of what precedes it, and starts earlier only when an escape sequence begun before the cut is still open there: at that sequence's escape, so the terminal reads it whole. Ordinary text at the cut is kept. When more than the output cap of normal-buffer history preceded the alternate screen, leaving it restored that history, and the saved last frame was placed before it, so the caps on output and exitLines dropped the frame and its error. A marker now records where the history ended as the TUI entered its alternate screen; the result is the history, then the saved frame, then what was printed after leaving.
Split out of #160 as a small, independent PR on 1.8.5 (it touches only what 1.8.5 does not already cover). It does not depend on the other parts.
Problem
wait_for_agent,get_agent_resultandobserve_agentreturnoutputas the masked raw PTY tail. For a full-screen TUI (OpenCode, and Codex/Claude in their full-screen modes) that is cursor moves, colours, erase sequences and spinner redraws, often starting in the middle of a sequence. The orchestrating model has to guess the screen from escape codes, and most of the 8 KiB budget is spent on redraws.exitLines(why an agent stopped) are the regex-stripped last 16 KiB. A TUI that redraws only the cells that changed reads as its redraws joined together ("memoy addressry address"), and a line drawn once early and never again (the model's status line) is outside the window and lost.Before (OpenCode subagent,
get_agent_result.output, abridged):After:
Change
renderedTerminalText(data, geometry): writes the data into a headless xterm (the@xterm/headlessthe material and handoff code already uses, loaded lazily) at the session's size and returns its rows as text, wrapped rows joined. If the TUI left its alternate screen as it exited, its last frame is kept in order: the history from before it entered that screen, then the frame, then what it printed afterwards.AgentControlService.screenText/screenExitLines: the last 64 KiB are masked (with 4 KiB more before them); when an escape sequence begun before the cut is still open at it, the window starts at that sequence's escape (so18;9Hof a cut sequence is never printed), otherwise exactly at the cut, so ordinary text is kept, the window is rendered, and the text is masked again before it is capped to the same sizes as before. If rendering throws, the masked raw tail is returned as before.sessionScreen.ts:terminalScreenTextis split so the row reader (terminalRowsText) can read the scrollback too; behaviour of the existing function is unchanged.observe()andresult()becomeasync; the orchestration handler awaits them. Tool descriptions (and the regenerated native catalog) anddocs/agent-orchestration.mddescribe the output as plain screen text instead of a raw tail.Why this and not less
Stripping escape sequences with a regex is what
exitLinesdid, and it is exactly what breaks for TUIs: removing a cursor move does not put the text where the cursor moved it, so redraws concatenate. Only a terminal emulator gives the screen a person sees, and CanvasTTY already ships one for the main process. Rendering needs xterm's asynchronous write callback, soobserve/resultmust become async; there is no synchronous form of this. The raw tail stays as a fallback, and masking happens both before rendering (a value written whole) and after (a value the screen shows across rows).What changes for existing workflows
outputof the three tools is now what the terminal shows, not escape codes; its length limits (maxChars, 8 192) are unchanged. Line-based CLIs read the same, without colour codes.exitLinescomes from the same rendered window, so it may now include a status line drawn early that the 16 KiB window missed.Validation
tests/orchestration-providers-wait.test.mjs(recorded OpenCode-style redraws, a long line-based transcript, a window cut inside an escape sequence, exitLines of a TUI that redraws in place,observe_agentwithinmaxChars, a TUI that left its alternate screen on exit, a cut window of one long line without escapes, a TUI that left its alternate screen after more than 8 KiB of history). On f3eee29 the first six fail; with the change all pass. The last two were added for review findings and failed on the first commit of this PR (empty output; the frame and its error dropped by the caps).observe/resultnow await them (agent-control,launch-contributors,secret-redaction).npm run build(typecheck),npm run audit:secrets, the eager renderer bundle check and the full test suite pass. Full suite: 2659 tests, 2653 pass, 0 fail, 6 skipped. The renderer is untouched (eager bundle unchanged against the other branches;renderer-bundle-splittingpasses).