Skip to content

fix(term): correctly handle control chars - #3459

Merged
jackwener merged 5 commits into
apache:mainfrom
zwpaper:fix/term
Aug 22, 2026
Merged

fix(term): correctly handle control chars#3459
jackwener merged 5 commits into
apache:mainfrom
zwpaper:fix/term

Conversation

@zwpaper

@zwpaper zwpaper commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #

Verification

When output with colors or other control chars in term, it would output some control chars.

this pr fix the issue.

Before:
image

After:
image

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Maka, address and fix the issue while reviewed by human.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review of exact head 349579e23d4fb51118713164b3727282bd6b0551 against main@f1f4e71a9540a4da23159052c924fee72202e989.

One actionable correctness gap: isWindowReportQuery only suppresses a single-parameter CSI t sequence. xterm's own windowOptions implementation dispatches from the first parameter and still generates replies for legal multi-parameter forms such as CSI 14;0 t and CSI 16;0 t (the special non-report case is 14;2). Those variants therefore fall through this handler, xterm emits data into onData, and the same delayed control-sequence leakage this PR is meant to stop remains reachable.

Match xterm's actual response condition: recognize the response-generating operation from the first numeric parameter, preserve 14;2 as non-reporting, and do not treat setters such as 22/23 as queries. Add focused cases for at least 14;0, 14;2, and a second report operation with an extra parameter.

Required conclusions:

  1. Optimal for the actual problem: the parser interception point is correct, but the window-query predicate is incomplete.
  2. Production code to delete: none identified.
  3. Tests to delete/replace: none; extend the predicate coverage above. The registration-count test is structural, but the behavior tests are useful.
  4. Deeper refactor: no; keep the shared xterm parser boundary.
  5. Ready to merge: no until the response variants are covered, required test passes, and the PR adds the CONTRIBUTING-required AI-use declaration (explicitly state “none” if none was used).
  6. Residual risks: this review matched every current xterm response-generating CSI/OSC/DCS handler; future xterm upgrades can add new reply families, so the dependency upgrade should re-audit this list.

This changes user-visible terminal input behavior. Independent human review is required; this automated review is not approval.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting the suppression at xterm's parser seam instead of filtering durable terminal bytes afterward. The direction and cleanup ownership are right, and current-head CI is green. Exact-head reproduction against xterm 6.0.0 found three query grammars where the predicates are narrower than xterm's actual reply behavior; I consolidated them inline.

AI-assisted review disclosure: OpenAI Codex performed an independent exact-head xterm behavior review. I verified the retained sequences against the current implementation, tests, live CI, mergeability, and review state, and I made the final review decision.

const parts = data.split(';');
if (parts.length === 0 || parts.length % 2 !== 0) return false;
for (let index = 0; index < parts.length; index += 2) {
if (!/^\d+$/.test(parts[index] ?? '') || parts[index + 1] !== '?') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] xterm replies to each ? inside a mixed OSC color payload, but this predicate returns false unless every pair/value is a query. Inputs such as OSC 4;0;?;1;#fff ST or OSC 10;?;#fff ST therefore fall through, apply the setter, and still emit a color reply into the Runtime Resource. Could the handler suppress the generated reply while preserving setter effects, with mixed query/set coverage rather than a test that locks in the leak?

const WINDOW_REPORT_OPERATIONS = new Set([11, 13, 14, 15, 16, 18, 19, 20, 21]);

function isSingleParam(params: TerminalParams, expected: number): boolean {
return params.length === 1 && params[0] === expected;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] xterm dispatches DA/DSR from the first parameter and ignores trailing parameters, while this helper requires exactly one. Sequences such as CSI 0;0 c, CSI >0;1 c, CSI 5;0 n, and CSI ?6;1 n still generate replies but bypass suppression. Could these predicates follow xterm's first-parameter semantics and add the multi-parameter cases, while retaining truly non-reporting operations?


export function isWindowReportQuery(params: TerminalParams): boolean {
return (
params.length === 1 &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Window-report queries have the same parameter-shape gap: xterm still replies to forms such as CSI 14;0 t and CSI 16;0 t, but params.length === 1 lets them through. Could this match xterm's reporting operation based on the first parameter and test both a reporting case (14;0) and a non-reporting case (14;2)?

@zwpaper
zwpaper requested review from Astro-Han and jackwener August 22, 2026 10:57
@jackwener
jackwener dismissed their stale review August 22, 2026 11:26

Superseded. The window-query first-parameter gap was fixed on later commits; remaining XTVERSION leak is addressed in e919948.

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head e919948. Parser interception is the right seam: replies must not enter the durable Runtime Resource onData path.

Prior gaps (mixed OSC 4/10/11/12 query+set, first-parameter DA/DSR, CSI 14;0 t / 14;2) are covered on 95eafd2. One remaining xterm reply family was still open: CSI > q / CSI > 0 q (XTVERSION) emits DCS > | xterm.js(...) ST. Pushed that suppression to this branch (e919948) with first-parameter 0 matching xterm, leaving CSI > 1 q unhandled.

Kitty keyboard query is gated on vtExtensions.kittyKeyboard, which this Terminal does not enable.

Not approving: this is a comment-only review after the follow-up fix. Independent human review still required for the user-visible terminal-input behavior.

zwpaper and others added 3 commits August 22, 2026 19:31
CSI > q still emits DCS > | xterm.js(...) ST into the durable Runtime
Resource input path. Intercept the same first-parameter 0 default xterm
uses, and leave CSI > 1 q unhandled.

Generated-by: Grok
CSI 6 n and CSI ? 6 n are live CPR, not delayed capability probes.
Full-screen apps need xterm to answer them; keep suppressing DSR status
and other probe replies.

Generated-by: Grok

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First-principles pass on 07176e3.

The defect is xterm answering capability probes through onData, which this Desktop terminal serializes as durable Runtime Resource input. A reply that outlives the probe is echoed as garbage. Intercepting those probes at the parser is the right seam.

Covered: OSC 4/10/11/12 (including mixed query+set with setter replay), DA, XTVERSION, DSR status (CSI 5 n), DECRQM, window reports, DECRQSS. CPR (CSI 6 n / CSI ? 6 n) is left to xterm so full-screen apps can still locate the cursor.

Kitty keyboard query and color-scheme query are gated off unless vtExtensions is enabled; this Terminal does not enable them.

Approve.

The slash-command type requires midTurn. /transcript is a local TUI
viewer and already opens during a running turn.

Generated-by: Grok
@jackwener
jackwener merged commit b434a49 into apache:main Aug 22, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants