Skip to content

fix(server): keep terminal history after cancelled control sequences - #23417

Closed
yashranaway wants to merge 1 commit into
pingdotgg:mainfrom
yashranaway:fix/terminal-cancelled-sequence-history
Closed

yashranaway wants to merge 1 commit into
pingdotgg:mainfrom
yashranaway:fix/terminal-cancelled-sequence-history

Conversation

@yashranaway

Copy link
Copy Markdown
Contributor

A cancelled terminal escape sequence can swallow later output from saved history. After reconnecting, an unfinished title or device-control sequence leaves the history blank, while a cancelled cursor sequence can eat the next character. This small parser fix recognizes both standard cancellation characters and resumes collecting output.

Six regressions failed before the fix and pass now, covering both escape encodings and history restored after reopening. All 48 terminal manager tests pass on Linux. Targeted lint, formatting and server typecheck pass.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 22, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at bb5450b

Macroscope's review found this PR approvable — This is a focused terminal-history bug fix that teaches the existing sanitizer to stop at standard cancellation characters, with regression coverage for CSI, OSC, DCS, and C1 forms. Runtime impact is limited to preserving output after cancelled sequences; no defaults, schemas, infrastructure, security, billing, or static-analysis configuration are changed.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between 3b95e78 and bb5450b.

📒 Files selected for processing: 2
  • apps/server/src/terminal/Manager.test.ts
  • apps/server/src/terminal/Manager.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Terminal history sanitization now recognizes CAN and SUB as control-sequence cancellation characters. Tests cover canceled OSC, DCS, and CSI sequences in ESC and C1 forms.

Changes: Terminal history cancellation handling

Layer / File(s) Summary
Handle canceled control sequences
apps/server/src/terminal/Manager.ts, apps/server/src/terminal/Manager.test.ts
The sanitizer recognizes CAN and SUB in string control sequences and ESC- and C1-prefixed CSI sequences. Tests check that lines following canceled sequences remain in reopened history.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to bb545

Cancelled control sequences no longer appear to swallow later output in reopened terminal history. No issue requiring a fix before merge was identified.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: preserving terminal history after cancelled control sequences.
Description check Passed The description explains the problem, the parser change, and focused verification results. It does not use the required section headings and does not provide a scope-and-approval link or explain why t…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, the parser change, and focused verification results. It does not use the required section headings and does not provide a scope-and-approval link or explain why this focused fix qualifies without one.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

GigaZoro pushed a commit to GigaZoro/CodeShip that referenced this pull request Oct 30, 2026
…(upstream pingdotgg#23417)

Cherry-picked from pingdotgg#23417

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copy link
Copy Markdown
Member

Closing: current main already covers this as of c839797b6d. If you still see it on a build that includes that commit, reply and we'll reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants