Skip to content

test(e2e): harden terminal restart regressions on Windows - #9064

Merged
brennanb2025 merged 4 commits into
stablyai:mainfrom
slashdevcorpse:agent/windows-terminal-restart-regression
Jul 18, 2026
Merged

brennanb2025 merged 4 commits into
stablyai:mainfrom
slashdevcorpse:agent/windows-terminal-restart-regression

Conversation

@slashdevcorpse

@slashdevcorpse slashdevcorpse commented Jul 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Make terminal restart probes native-PowerShell safe: fresh-shell discovery sends no startup control bytes, settled Windows probes separate Ctrl+C from the command, PowerShell installs a prompt function, and cold daemon loss uses bounded taskkill.exe termination.
  • Gate keyboard assertions on a full renderer-input round trip that survives replay suppression, delayed ConPTY echo, and pane replacement without accepting replayed markers from a stale pane.
  • Enable the cold daemon-loss restart regression on Windows and add a focused Windows 2022 workflow whose three selected cases cannot silently skip required setup.
  • Keep this test-only companion independently mergeable from fix(terminal): replay cold restores at recovered grid #9043 while recommending that both PRs land together as regression coverage for [Bug] bug on windows upon returning session #8751.

Screenshots

No visual change.

Testing

  • pnpm lint
  • pnpm typecheck
  • pnpm test — the full Windows suite was attempted: 30,274 passed, 342 skipped, and 100 failed with 2 suite errors. The failures are existing broad Windows incompatibilities such as /bin/sh ENOENT, POSIX paths/signals, and line-ending assumptions; none touch this patch.
  • pnpm build — the full production/native build was not run; pnpm exec electron-vite build --mode e2e passed on the rebased commit.
  • Added or updated high-quality tests that would catch regressions, or explained why tests were not needed

Focused validation on commit 2ef995ea34a0af8682f9315533d21a87e85f1325:

  • pnpm exec vitest run tests/e2e/restored-terminal-input-readiness.unit.test.ts tests/e2e/terminal-probe-input-sequence.unit.test.ts — 2 files / 9 tests passed.
  • Required Windows workflow command — 3/3 passed in 2.0 minutes with normal cleanup:
    • clean restart with the live daemon session restored;
    • cold restore after the daemon was force-killed between launches;
    • daemon snapshot relaunch preserving the cursor on the PowerShell prompt.
  • The cold daemon-loss assertion also passed 5/5 in an earlier isolated repetition.
  • Workflow YAML parsing and git diff --check passed.

The original driver was red on Windows because a fresh PowerShell probe was prefixed with POSIX Ctrl+U/startup control bytes. During adversarial review, a real clean-restart run exposed a second red case: readiness kept only the latest 100 ms marker while valid ConPTY echoes arrived later. The terminal tail contained the markers, but the helper never observed the immediately previous one. The final helper retains all outstanding markers for the same concrete pane instance and has dedicated delayed-echo and pane-replacement regressions.

AI Review Report

Three parallel audits checked Windows runtime behavior, replay/readiness semantics, CI non-skippability and security, cross-platform compatibility, and code minimality.

Findings corrected during review:

  • Bound readiness evidence to the concrete mounted pane so replaying an old marker into a replacement pane cannot false-pass.
  • Retained delayed same-pane markers instead of chasing only the newest marker every 100 ms.
  • Changed stale diagnostics from “renderer transport unbound” to the accurate renderer-input-path possibilities: replay, focus, or binding.
  • Made missing seeded repo state and missing PaneManager hard failures under the dedicated workflow flag.
  • Removed renderer-crash coverage from this workflow and reverted its spec diff to keep the companion focused.
  • Extracted restored-input readiness into a concrete module rather than adding a forbidden max-lines waiver.

Cross-platform review covered macOS, Linux, Windows/ConPTY, WSL, and SSH. POSIX shells retain Ctrl+C + Ctrl+U behavior; Windows receives Ctrl+C as a separate payload and never receives Ctrl+U. No shortcut, provider, path-separator, or local-only product behavior changed.

Final verdict from all three audits: ship, with no blockers.

Security Audit

This PR changes tests and CI only; it adds no product dependency, auth, secret, network, filesystem API, or IPC surface. Windows termination targets only the daemon PID parsed from the test's isolated user-data directory, passes arguments through execFileSync without a shell, is bounded by a 10-second timeout, and verifies the PID is gone.

The workflow uses pull_request rather than pull_request_target, grants read-only contents permission, does not persist checkout credentials, runs serially with a 30-minute job timeout, and uploads only failure traces.

Notes

@coderabbitai

coderabbitai Bot commented Jul 16, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4d55f4aa-447d-4f85-8fff-c83350bf8353

📥 Commits

Reviewing files that changed from the base of the PR and between fa37926ee53bcf1637f0bc9b211835dd8844cc66 and f098294.

📒 Files selected for processing (1)
  • .github/workflows/windows-terminal-restart-e2e.yml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/windows-terminal-restart-e2e.yml

📝 Walkthrough

Walkthrough

Adds Windows CI coverage for terminal restart regressions, including Electron build setup, targeted Playwright execution, and failure trace uploads. Introduces platform-specific settled shell input probes and marker-based restored-pane readiness polling. Updates restart E2E tests with daemon termination, stricter Windows gating, seeded repository handling, restored-input checks, and PowerShell prompt setup. Adds unit tests for probe sequences and readiness behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: hardening Windows terminal restart regression tests.
Description check ✅ Passed The description includes all required sections and is specific enough for this repository's template.
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.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (1)
tests/e2e/restored-terminal-input-readiness.unit.test.ts (1)

42-145: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Cover the page.evaluate rejection recovery path.

Add a test where the first evaluation rejects during document replacement, then a replacement pane accepts a new marker. Assert stale attempts are discarded and the retry succeeds.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 022b9c0a-524e-439d-9109-07231e342083

📥 Commits

Reviewing files that changed from the base of the PR and between b892565 and 47209a0852cc7d2dd768e5d141684f4522d9a194.

📒 Files selected for processing (9)
  • .github/workflows/windows-terminal-restart-e2e.yml
  • tests/e2e/helpers/restored-terminal-input-readiness.ts
  • tests/e2e/helpers/terminal-input-probes.ts
  • tests/e2e/helpers/terminal.ts
  • tests/e2e/restart-restore-terminal-input.spec.ts
  • tests/e2e/restored-terminal-input-readiness.unit.test.ts
  • tests/e2e/terminal-probe-input-sequence.ts
  • tests/e2e/terminal-probe-input-sequence.unit.test.ts
  • tests/e2e/terminal-restart-persistence.spec.ts

@slashdevcorpse

Copy link
Copy Markdown
Contributor Author

Addressed CodeRabbit's document-replacement recovery review in 2ef995ea3.

The new regression proves the catch path clears real stale state:

  • call 1 drops marker A and records A pending;
  • call 2 writes marker B, swaps to the replacement pane, then page.evaluate rejects;
  • call 3 starts with zero pending attempts and must send fresh marker C through the replacement pane;
  • call 4 observes C and succeeds.

The assertions require pending counts [0, 1, 0, 1], two original-pane inputs, one replacement-pane input, and C !== B. Removing the catch-path reset makes the test fail.

Validation after the fix:

  • pnpm lint
  • pnpm typecheck
  • focused readiness/probe units: 2 files, 9/9 tests
  • independent adversarial audit: ship, no blockers

The walkthrough's docstring-percentage item is a generic warning rather than a repository gate. I did not add restatement-only docstrings to internal E2E helpers because the project requires concise comments for non-obvious reasons; the full repository lint, max-lines, and localization gates pass. There were no inline review threads to resolve.

@brennanb2025

Copy link
Copy Markdown
Contributor

Thanks for the thorough companion coverage here. Reviewed against latest main and verified:

  • Platform-aware probe sequences correctly separate the PowerShell/ConPTY path (no POSIX Ctrl+U; Ctrl+C sent as its own write) from the POSIX path, and fresh-shell discovery drops the startup control bytes that ConPTY was echoing as literal ^C.
  • Restored-input readiness gate binds evidence to the concrete mounted pane instance, so a marker replayed into a replacement pane can't false-pass, and it retains every same-pane marker so delayed ConPTY echo doesn't flake.
  • Windows-2022 workflow runs exactly the 3 intended cases (grep patterns each match one real title), and ORCA_REQUIRE_WINDOWS_TERMINAL_RESTART_E2E=1 turns every silent-skip path (missing seeded repo, missing PaneManager) into a hard failure. Fork-safe: pull_request, contents: read, persist-credentials: false, no secrets, failure-only artifacts.

One small hardening pushed on top (fa37926): the POSIX cold-kill path now tolerates ESRCH if the daemon self-exits between the liveness check and the SIGKILL, matching the tolerance the Windows taskkill branch already has — otherwise a benign race could throw a spurious test failure.

Local checks green: the two new unit suites (9/9), pnpm lint, and pnpm typecheck. Leaving the merge to you so the ordering with #9043 can be coordinated.

@brennanb2025
brennanb2025 force-pushed the agent/windows-terminal-restart-regression branch from fa37926 to f098294 Compare July 18, 2026 00:01
@brennanb2025
brennanb2025 merged commit 803a442 into stablyai:main Jul 18, 2026
4 checks passed
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
)

* test(e2e): harden Windows terminal restart regressions

* test(e2e): cover renderer replacement rejection

* test(e2e): tolerate ESRCH when force-killing the daemon on POSIX

* fix(ci): preserve Windows restart test selection

---------

Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
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