Skip to content

fix(server): preserve terminal exits when clearing scrollback - #25192

Open
maria-rcks wants to merge 2 commits into
mainfrom
maria/takeover-pr-22137
Open

maria-rcks wants to merge 2 commits into
mainfrom
maria/takeover-pr-22137

Conversation

@maria-rcks

Copy link
Copy Markdown
Collaborator

Problem

Clearing scrollback while an output subscriber is busy drops a queued process exit and releases the active drain. The terminal can remain marked running after its shell exits.

Change

Keep pending exit events, discard pending output, and leave drain ownership with the active consumer. One regression test in the existing terminal manager suite checks exit delivery, discarded output, the observer snapshot, and empty persisted scrollback.

Verification

  • Regression test fails on current main with the queued exit missing.
  • Blacksmith vp run check:changed passes: formatting, lint, knip, GraphQL, affected typechecks, and all 44 terminal manager tests.
  • Local real node-pty / bash reproduction: hold an output subscriber, queue output and exit 7, clear, then release. Observed started → output → cleared → exited; observer snapshot: exited, code 7, empty history; persisted log: empty.
  • No client code changes. Browser, desktop, and mobile interactions are unverified; the fix is in their shared server terminal manager.

Takeover

Replaces #22137, with thanks to @saphid for the original implementation.

Built with GPT-6.1-Sol in the Codex harness on T3 Code.

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

macroscopeapp Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 4a6f460

Macroscope's review found this PR approvable — Clearing terminal scrollback now preserves queued process exits and the active event drain, allowing normal exit cleanup and notifications to complete. The change is narrowly scoped and introduces no material new exposure.

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

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 26, 2026
@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ The exact PR base did not have a successful artifact. Baseline uses the latest successful main measurement shown below.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 21.5 KiB 21.5 KiB 0 B (0.0%) 24.4 KiB ✅
Codex Live turn messages 1 1 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.9 KiB 21.9 KiB 0 B (0.0%) 24.4 KiB ✅
Claude Live turn messages 1 1 0 (0.0%) 8 ✅

Baseline: 3d78e91 · PR result: 4a6f460 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

TerminalManager.clear now discards pending output events but preserves queued process exit events. A regression test checks that the exit is delivered and that the observer sees an exited snapshot with empty history.

Changes: Terminal clear and event delivery

Layer / File(s) Summary
Clear history while retaining pending exits
apps/server/src/terminal/Manager.ts, apps/server/src/terminal/Manager.test.ts
clear retains pending exit events, discards pending output, and resets the event index. The regression test checks delivery of the queued exit and the resulting terminal state.

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

Change: Bug fix

Suggested reviewers: saphid, t3dotgg

Merge Risk: Minimal · up to 4a6f4

No merge-blocking issue is identified; merge after normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly describes the main fix.
Description check Passed The description is mostly complete and includes detailed verification. It lacks the required Scope and approval section; add an approval link or explain why the fix qualifies without one.
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.
Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 64b413f and 4a6f460.

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; 2 remain after this review.

@maria-rcks
maria-rcks force-pushed the maria/takeover-pr-22137 branch from 64b413f to 4a6f460 Compare November 6, 2026 04:07
@macroscopeapp
macroscopeapp Bot dismissed their stale review November 6, 2026 04:08

Dismissing prior approval to re-evaluate 4a6f460

This branch has not been deployed

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

Labels

size:XS 0-9 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.

1 participant