fix(terminal): filter terminal query sequences from captured output#2334
fix(terminal): filter terminal query sequences from captured output#2334jpshackelford wants to merge 4 commits intomainfrom
Conversation
Filter terminal query sequences (DSR, OSC, DA, etc.) from captured PTY output before returning from terminal tool. These queries cause the terminal to respond when displayed, producing visible escape code garbage. Root cause: CLI tools like `gh` send terminal queries as part of their progress/spinner UI. When captured and displayed, the terminal processes them and responds, causing visible garbage like `^[[38;1R`. Solution: Add filter_terminal_queries() to strip query sequences while preserving legitimate formatting codes (colors, bold, etc.). Fixes: #2244 Co-authored-by: openhands <openhands@all-hands.dev>
API breakage checks (Griffe)Result: Passed |
Agent server REST API breakage checks (OpenAPI)Result: Failed Log excerpt (first 1000 characters) |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Requires Eval Verification
Taste Rating: Code is clean and solves a real problem. However, this touches terminal output handling and needs eval verification before approval per repo guidelines.
Assessment:
- ✅ Solves real problem (visible escape code garbage)
- ✅ Simple, targeted solution (filter at source)
- ✅ Comprehensive tests with good coverage
⚠️ Touches terminal/stdout handling → flag for lightweight evals
The implementation is solid. Regex patterns are compiled, tests verify both removal and preservation, and the fix is applied at the right layer. Once evals confirm no regressions, this is ready to merge.
Coverage Report •
|
||||||||||||||||||||
Change the OSC filter pattern from matching specific codes (10, 11, 4) to matching any OSC query (sequences ending with ;? before terminator). This is more future-proof and catches additional query types like: - OSC 12 (cursor color) - OSC 17 (highlight background) - Any other OSC queries that follow the standard format The pattern now matches: ESC ] Ps [;param] ;? TERMINATOR Where ;? indicates it's a query, not a set operation. Importantly, SET operations are preserved: - OSC 0 (window title) - OSC 8 (hyperlinks) - OSC 7 (working directory) Co-authored-by: openhands <openhands@all-hands.dev>
|
Good catch! I've updated the OSC pattern to be more general. Before: Matched only OSC codes 10, 11, 4 The new pattern: This catches all OSC queries:
While preserving SET operations:
Added 5 new tests to verify the behavior. See commit a499579. |
Adds .pr/test_real_world.py that runs an agent with the gh command to verify terminal query sequences are properly filtered. Usage: LLM_BASE_URL="https://llm-proxy.eval.all-hands.dev" LLM_API_KEY="$LLM_API_KEY" \ uv run python .pr/test_real_world.py Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a
|
Manual Testing InstructionsA real-world test script is available in How to Run# Clone and checkout the branch
git fetch origin fix/terminal-escape-filter-minimal
git checkout fix/terminal-escape-filter-minimal
# Run the test (uses All-Hands LLM proxy)
LLM_BASE_URL="https://llm-proxy.eval.all-hands.dev" LLM_API_KEY="$LLM_API_KEY" \
uv run python .pr/test_real_world.pyWhat the Test Does
Success Criteria✅ Pass if:
❌ Fail if:
Without the Fix (for comparison)To see the problem on git checkout main
LLM_BASE_URL="https://llm-proxy.eval.all-hands.dev" LLM_API_KEY="$LLM_API_KEY" \
uv run python .pr/test_real_world.pyYou should see escape code garbage like |
|
I have confirmed that this solution works. Note that there is a change in what the agent displays. Since we filter out OSC queries, the cli does not render the gh spinner. I think this an acceptable limitation. (Note that we aren't preventing the spinner from displaying, but when Why the
|
|
I think this is ready except that should probably test the CLI built against this version of the SDK to ensure that our approach here doesn't interfere with the TUI. Perhaps the best course is to open a PR that will build the CI against this branch of the SDK and recruit some users to use it for a day or two. |
This CLI build uses the software-agent-sdk branch from PR #2334 which includes the terminal escape filter fix for tools like gh, npm that use spinner/progress UI. SDK PR: OpenHands/software-agent-sdk#2334 Co-authored-by: openhands <openhands@all-hands.dev>
|
It looks like testing this in the CLI is blocked until breaking change in #2133 is dealt with in the CLI unless we rebase this fix branch on v1.11.5. |
|
This PR was merged and should address the blocker on testing with OpenHands-CLI OpenHands/OpenHands-CLI#587 |
|
@OpenHands Do a /codereview-roasted on this PR. Publish your review feedback as review on the PR, using your appropriate event in gh api. (not a comment, a review, you are allowed to review) |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
There was a problem hiding this comment.
🔴 Needs improvement
This is aimed at a real bug and the implementation stays pleasantly small, but there are two correctness holes here: the fix only applies to output coming back through TerminalSession, and the filter is stateless so split escape sequences can still leak through incremental updates. Since this also touches terminal/stdout handling, I’d want lightweight eval coverage after those are addressed.
Verdict: not ready as the claimed fix for #2244 yet.
Key insight: sanitizing after the stream has already been split into per-observation chunks is too late; either the sanitization needs carry-over state, or part of the fix has to live at the actual SDK terminal boundary rather than only in the terminal tool output path.
|
|
||
| # Filter terminal query sequences that would cause the terminal to | ||
| # respond when displayed, producing visible garbage | ||
| command_output = filter_terminal_queries(command_output) |
There was a problem hiding this comment.
This only sanitizes bytes that flow back through the terminal tool. Issue #2244 also reproduces when the SDK process itself emits terminal queries (the minimal repro in the issue does that directly, and Rich capability detection is another example). Those paths never touch TerminalSession, so this patch doesn’t actually close the full bug it claims to fix. Either narrow the scope to PTY-emitted queries only, or handle the SDK-side leak at the conversation/visualizer boundary too.
| # Convert to bytes for regex matching (escape sequences are byte-level) | ||
| output_bytes = output.encode("utf-8", errors="surrogateescape") | ||
|
|
||
| # Remove each type of query sequence |
There was a problem hiding this comment.
This filter is stateless, which means a query split across observations survives unchanged. For example, if one update ends with \u001b]11; and the next starts with ?\u0007, neither call matches, but the client still receives the full OSC query once those chunks are rendered in sequence. That’s not a theoretical edge case here because long-running commands are surfaced incrementally. The fix needs carry-over state for incomplete escape sequences, or it needs to run before the output is sliced into deltas.
|
Final summary:
Checklist:
Because I did not change repository files, there was nothing to push to the remote branch. |
Summary
Summary
Fixes #2244 - Filter terminal query sequences from captured PTY output to prevent visible escape code garbage when displayed.
Problem
When CLI tools like
gh,npm, or other progress-indicator tools run inside the SDK's PTY, they send terminal query sequences as part of their spinner/progress UI:\x1b[6n- DSR (Device Status Report) - cursor position query\x1b]11;?- OSC 11 - background color query\x1b[c- DA (Device Attributes) queryThese queries get captured as part of the command output. When the output is later displayed to the user's terminal, the terminal processes these queries and responds, causing visible garbage like:
How to Reproduce
gh pr list --repo OpenHands/openhandsVisual Example
Before fix:
After fix:
Root Cause Analysis
The escape codes are IN the captured PTY output stream, not generated by terminal responses to the SDK's own queries. When
gh(or similar tools) runs:ghsends\x1b[6nto query cursor position (for spinner positioning)Solution
Add
filter_terminal_queries()to strip terminal query sequences from captured output before returning from the terminal tool. This removes the queries at the source, so the user's terminal never sees them.Filtered sequences:
\x1b[6n) - cursor position query\x1b]10;?,\x1b]11;?,\x1b]4;?) - color queries\x1b[c,\x1b[>c) - device attributes\x1bP$q...\x1b\\) - terminal state queriesPreserved sequences:
\x1b[31m,\x1b[0m, etc.)\x1b[H,\x1b[5A, etc.)Testing
Files Changed
openhands-tools/.../utils/escape_filter.pyopenhands-tools/.../utils/__init__.pyopenhands-tools/.../terminal_session.py_get_command_output()tests/tools/terminal/test_escape_filter.pyDesign Decisions
Filter at source: Apply filter where output is captured (terminal tool), not where it's displayed. This is simpler and more reliable.
Byte-level regex: Use compiled regex patterns on bytes for accurate escape sequence matching.
Preserve formatting: Only remove query sequences that trigger responses; keep colors and cursor movement intact.
Minimal scope: This fix targets only the terminal tool output processing - no SDK-level changes needed.
Fixes: #2244
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:39dc01c-pythonRun
All tags pushed for this build
About Multi-Architecture Support
39dc01c-python) is a multi-arch manifest supporting both amd64 and arm6439dc01c-python-amd64) are also available if needed