Repository navigation
fix(cli): CLI tests and a refused preview stop killing orphaned Chrome - #6022
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
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
|
Agreed on both points, fixed in cdcc766.
preview + orphanCleanup tests: 55/55, 3 runs in a row on Linux. CLI tsc, oxfmt, oxlint and the comment checks exit 0. |
jrusso1020
left a comment
There was a problem hiding this comment.
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
Edit accuracy: accurate 271 (base branch 271), smooth 179 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
What changes
Three changes:
packages/cli/src/utils/orphanCleanup.test.tsare deleted:killOrphanedProcesses > returns 0 when no orphans existand> does not kill non-orphaned Chrome processes.previewnow 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.tsmockskillOrphanedProcessesfor 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:Test (runtime)exactly so: "expected 0 kills, got 2". Main passed the same suite on the same commit.A third test reached the same cleanup without calling it directly. The
previewtest "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 otherpreviewtest 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.tsfeeds a fixedpstable 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.ppid === 1check inkillOrphansMatchingmakesorphanCleanup.scan.test.tsfail (1 failed). The rule stays guarded after the deletion.tsc --noEmit, oxfmt, lint, fallow, the process-ownership check and the comment checks: exit 0.What I did NOT exercise