Skip to content

fix(cli): CLI tests and a refused preview stop killing orphaned Chrome - #6022

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/orphan-cleanup-tests-no-live-table
Oct 21, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
fix/orphan-cleanup-tests-no-live-table

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

What changes

Three changes:

  • Two tests in packages/cli/src/utils/orphanCleanup.test.ts are deleted: killOrphanedProcesses > returns 0 when no orphans exist and > does not kill non-orphaned Chrome processes.
  • preview now runs its orphan cleanup after the host-app refusal, not before it. A preview refused inside a host app's agent run no longer kills orphaned Chrome on the way out. That is what the cleanup's own comment already required: it must not run for an invocation that never starts anything.
  • preview.test.ts mocks killOrphanedProcesses for the whole file. The host-app refusal test now also checks that the cleanup is never called.

Why

Both tests called the real killOrphanedProcesses(). That reads the live process table (ps -U <uid>) and kills every Chrome headless shell whose parent is init. So they asserted the state of the whole machine, not the function:

  • On CI they flake. The CLI suite runs process tests in parallel, and some briefly leave a Chrome without its parent (the "kill the CLI mid busy-loop" tests do that on purpose). The release PR for v0.8.183 failed Test (runtime) exactly so: "expected 0 kills, got 2". Main passed the same suite on the same commit.
  • Off CI they are destructive. Running the CLI tests on a developer's machine ran the real cleanup over that developer's whole process table. It killed any orphaned headless Chrome they owned, for example one left by another HyperFrames process or a separate Puppeteer script, and then failed the test for it.

A third test reached the same cleanup without calling it directly. The preview test "refuses before spawning anything" ran the real command, which called the cleanup before refusing. Moving the cleanup fixes the command; the file-wide mock keeps every other preview test away from the real process table.

The rule these tests meant to check, "kill orphaned Chrome, leave Chrome that still has a parent", is already covered without touching real processes. orphanCleanup.scan.test.ts feeds a fixed ps table with orphaned and parented Chrome and checks that only the orphans (101 and 103) are terminated.

Evidence

Linux, each file run alone:

  • orphanCleanup.test.ts + orphanCleanup.scan.test.ts: exit 0, 9 passed, 3 runs in a row.
  • Mutation, restored after: removing the ppid === 1 check in killOrphansMatching makes orphanCleanup.scan.test.ts fail (1 failed). The rule stays guarded after the deletion.
  • CLI tsc --noEmit, oxfmt, lint, fallow, the process-ownership check and the comment checks: exit 0.

What I did NOT exercise

  • macOS and Windows runs. The deleted tests and the scan test are all skipped on Windows, so on Windows this changes nothing.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed exact 42bdf74702fec2e74b8ccc3e2fb11a8e28a309b6, with an independent indirect-caller audit. The two unsafe direct tests are removed, and production orphanCleanup.ts has the same Git blob as the base. The retained scan test mocks both process discovery and termination and checks orphaned Chrome against parented Chrome and unrelated Node processes.

[P2 blocker] An indirect unit test still scans and kills from the real process table. packages/cli/src/commands/preview.test.ts:619–637 calls the actual preview command with a valid temporary project, --background and --json in a Desktop run. That test file mocks lintProject and cancellableProcess, but not orphanCleanup or child_process, and the CLI Vitest config supplies no global replacement. packages/cli/src/commands/preview.ts:482 calls killOrphanedProcesses before the host-app refusal at lines 499–503. Thus this ordinary CLI test still reaches the same user-wide orphan-Chrome cleanup, despite expecting to refuse before spawning anything. Stub/inject the cleanup for this test and pin the isolation; keep the real implementation in production. This is a remaining instance of the exact destructive-test behavior this PR aims to remove.

Validation: read the deleted tests, full cleanup/scan modules, preview call path, test mocks and CLI Vitest configuration. Parent reran an independent actual preview.run extraction with the valid test arguments and a sentinel cleanup stub: cleanup executes once before the host-app guard, without running real ps or termination. Actual cleanup functions against a fake process table select only 101/103; removing the PPID guard selects 101/102/103/104, so matching coverage remains meaningful. These are isolated actual-source probes, not a full workspace Vitest or macOS/Windows run. The retained scan test is skipped on Windows, contrary to the body wording that it behaves the same on every OS.

Verdict: REQUEST CHANGES
Reasoning: Production behavior and deterministic matching coverage are preserved, but deleting the two direct calls leaves an indirect test capable of killing unrelated developer Chrome processes.

— hf-review

@miguel-heygen miguel-heygen changed the title test(cli): drop the orphan cleanup tests that read the live process table fix(cli): CLI tests and a refused preview stop killing orphaned Chrome Oct 21, 2026
@miguel-heygen

miguel-heygen commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator Author

Agreed on both points, fixed in cdcc766.

  • preview ran its orphan cleanup before the host-app refusal, so a refused preview killed orphaned Chrome on the way out. That broke the cleanup's own comment. The cleanup now runs after the refusal. preview.test.ts mocks killOrphanedProcesses for the whole file, and the refusal test checks that it is never called. With the old order restored, that check fails (called 1 time).
  • The body no longer says the scan test behaves the same on every OS. It is skipped on Windows, like the deleted tests.

preview + orphanCleanup tests: 55/55, 3 runs in a row on Linux. CLI tsc, oxfmt, oxlint and the comment checks exit 0.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed exact cdcc7669b8cd3889506ef4679a6f579690038370. The prior blocker is resolved: packages/cli/src/commands/preview.ts:480–503 refuses a host-owned background preview before orphan cleanup, and the filewide hoisted mock in preview.test.ts protects every test invocation. The new refusal assertion pins zero cleanup calls. The body now accurately states Windows matching tests are skipped.

Validation: read the full updated preview command/test and the unchanged cleanup/matching coverage. Parent reran a fresh independent exact-source probe: refused preview calls cleanup zero times; permitted preview still calls the unchanged cleanup; restoring the old order breaks the refusal check. Module resets and nested child_process/fs mocks do not unregister the orphanCleanup mock. No real ps/kill, full workspace Vitest, macOS or Windows run was performed.

Verdict: APPROVE
Reasoning: The residual indirect destructive test is isolated, the refusal path no longer cleans up processes, and permitted production previews retain their existing cleanup.

— hf-review

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 179 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • seqheldtwice-tlhold-px-r0-root-z100: tracking 0.41, pressJump 60.41, drop 0.35, reload 0.35, render 0.02, renderKey -, undo true, teleport true / tracking 0.08, pressJump 0.11, drop 0.35, reload 0.35, render 0.02, renderKey -, undo true, teleport true / tracking 0.08, pressJump 0.11, drop 0.35, reload 0.35, render 0.02, renderKey -, undo true, teleport true

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 21, 2026
Merged via the queue into main with commit 5641a91 Oct 21, 2026
131 of 134 checks passed
@miguel-heygen
miguel-heygen deleted the fix/orphan-cleanup-tests-no-live-table branch October 21, 2026 12:52
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.

2 participants