fix(windows): contain desktop smoke process trees - #41
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWindows desktop smoke tests now launch through a PowerShell Job Object wrapper with run-specific tokens, platform-specific supervision, bounded teardown, and expanded unit/integration regression coverage. Non-Windows launch behavior remains direct. ChangesWindows smoke lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SmokeTest as smoke-test.mjs
participant Lifecycle as superviseDesktopSmokeProcess
participant Wrapper as smoke-test-windows-job.ps1
participant Electron as Electron process
SmokeTest->>Lifecycle: Start process with windowsJobRunId
Lifecycle->>Wrapper: Launch PowerShell Job wrapper
Wrapper->>Electron: Start target inside Job Object
Wrapper-->>Lifecycle: Emit READY marker and relay output
Lifecycle->>Wrapper: Write shutdown token
Wrapper->>Electron: Terminate Job Object
Lifecycle-->>SmokeTest: Return supervision result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/scripts/smoke-test-windows-job.ps1`:
- Around line 5-9: Rename the unapproved Fail-SmokeJob function to
Stop-SmokeJob, preserving its parameters and behavior, and update all four call
sites to use the new approved function name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eecb787f-8751-4966-9bec-810b6610e6cf
📒 Files selected for processing (6)
.github/workflows/ci.ymlapps/desktop/scripts/smoke-test-lifecycle.mjsapps/desktop/scripts/smoke-test-windows-job.ps1apps/desktop/scripts/smoke-test-windows-job.windows.integration.test.mjsapps/desktop/scripts/smoke-test.mjsapps/desktop/scripts/smoke-test.test.mjs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/scripts/smoke-test-windows-job.ps1`:
- Around line 5-8: Rename Stop-SmokeJob to the non-state-changing
Write-SmokeJobError and update all four call sites to use the new name,
preserving its existing error output and process-exit behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 192ad9c7-07e2-4cc9-af89-ae3099eb0aad
📒 Files selected for processing (5)
apps/desktop/scripts/smoke-test-lifecycle.mjsapps/desktop/scripts/smoke-test-windows-job.ps1apps/desktop/scripts/smoke-test-windows-job.windows.integration.test.mjsapps/desktop/scripts/smoke-test.mjsapps/desktop/scripts/smoke-test.test.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/scripts/smoke-test.mjs
- apps/desktop/scripts/smoke-test-windows-job.windows.integration.test.mjs
- apps/desktop/scripts/smoke-test.test.mjs
- apps/desktop/scripts/smoke-test-lifecycle.mjs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lib/workflow-contracts.ts`:
- Around line 179-184: Update the validation around the desktop smoke check in
workflow-contracts.ts to detect tokenized test:desktop-smoke invocations within
rawCommand, including arguments and shell composition, rather than matching only
the exact command; preserve the existing no-wrapper error behavior. In
scripts/lib/workflow-contracts.test.ts, add an argument-bearing wrapper mutation
within the existing test coverage and assert that validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d3abb641-264a-4ffa-b06f-87054de4a479
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/lib/workflow-contracts.test.tsscripts/lib/workflow-contracts.ts
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
What Changed
KILL_ON_JOB_CLOSEtaskkillcleanupwindows_x64CI gateWhy
The previous Windows smoke cleanup could report success while late Electron descendants were still settling, and PID-tree discovery left a spawn-to-assignment race. Assigning the wrapper to a kill-on-close Job before Electron starts makes containment deterministic and fail-closed.
Verification
Checklist
Summary by cubic
Contain Windows desktop smoke process trees with a checked-in PowerShell 5.1 Job Object wrapper, and run the built desktop smoke directly in CI for deterministic, fail-closed teardown.
Bug Fixes
KILL_ON_JOB_CLOSEbefore launch; start observation only afterSYNARA_SMOKE_JOB_READY <id>; sendSYNARA_SMOKE_JOB_TERMINATE <id>\nthen EOF; enforce 30s startup, 13s teardown, 2s settlement. TreatSYNARA_SMOKE_JOB_ERROR, invalid/missing run id, and unexpected/duplicate READY as fatal; never send shutdown more than once; guard late helper errors; hold output through settlement.SystemRoot\System32\taskkill.exeviaSystemRoot/WINDIR(case-insensitive), pass the Windows env, add a close‑proof margin, abandon fallback if the window expires, and finish with directSIGKILL.SYNARA_SMOKE_JOB_RUN_ID; resolvepowershell.exeviaSystemRoot/WINDIR; keep POSIX direct spawn unchanged.New Features
createDesktopSmokeSpawnSpecto choose the Windows wrapper vs POSIX and pass a clean env/run id/working directory;apps/desktop/scripts/smoke-test.mjsnow uses it with a generated run id.--and quoting, stdin‑EOF shutdown, startup failure, protocol errors, bounded startup/settlement).bun run --cwd apps/desktop smoke-testafter the build; enforce workflow contracts for this exact post‑build command, unconditional fail‑closed behavior, correct ordering, and reject the Turbo wrapper token by exacttest:desktop-smokematching—including quoted or redirected variants;windows_x64also runsscripts/smoke-test.test.mjsandscripts/smoke-test-windows-job.windows.integration.test.mjs.Written for commit 5f0c85e. Summary will update on new commits.
Summary by CodeRabbit