Repository navigation
test: keep cold first-exec and PowerShell start out of two timed cases - #7598
Conversation
The PATH-fallback activation cases copied a fresh Bun binary per case and executed it inside the 5s case budget; the OS first-run scan of that copy dominated under host load (#7578). One shared PATH Bun is now copied and warmed once in a budgeted beforeAll on every platform. The real CurrentUser Root inspection paid PowerShell's cold start inside the 10s status deadline and read as unknown on a loaded Windows runner (#7569). The same inspection is replayed once through warmColdSpawn, and the deadline breach itself is proven on every platform through a runner seam. Refs #7578 Refs #7569
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "test: keep cold first-exec and PowerShell start out of two timed cases" matches the main change. The PR moves cold process-start costs out of two timed tests. It does this by warming one sh… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "test: keep cold first-exec and PowerShell start out of two timed cases" matches the main change. The PR moves cold process-start costs out of two timed tests. It does this by warming one shared PATH Bun copy in beforeAll for the launcher fallback tests, and by warming the real certificate-store inspection through warmColdSpawn for the CurrentUser Root test. The title names both timing changes and the test-only scope. It is a 70-character single sentence, and it uses the "test:" prefix correctly. The title is slightly dense with jargon, but it is specific and accurate.
-
✨ Finishing Touches: 💡 1
-
📝 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
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.
Comment @coderabbitai help to get the list of available commands.
github-actions
Bot
added
the
chore
|
✅ Deterministic PR hygiene checks passed. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed b42cbfd95c185adc56cdfbc6ac0a50bd83bd7d9b. Independent capped Linux/Bun 1.4.3 run of tests/cli/ocx-launcher-runtime.test.ts and tests/clients/desktop-compatibility-trust.test.ts: 84 passed, 4 skipped, 0 failed; 193 assertions.
The first local run was 80/4/4: this host's inherited umask 0002 made fixture child directories group-writable, so the existing strict PATH candidate validator correctly refused them. Setting UMask=0077 on the isolated systemd test unit produced the passing run above; applying it only to the launching Python process did not propagate through the user manager. No repository trust check, product deadline, shared directory mode or test assertion was relaxed. The private unit's homes and descendants were newly created as ubuntu.
The shared PATH Bun warmup runs in bounded setup, and the deterministic killed-child inspection case retains the 10-second product deadline and returns unknown despite partial not-trusted stdout. The Windows real-store cases were skipped locally; this is not Windows/native-store certification, and no OS trust mutation or live client was run. The current-head dispatch was still in progress at my CI snapshot; its final conclusion, project typecheck/full-suite and the separate trust-boundary readiness requirements remain independent of this focused pass.
Resolve the runtime fixture overlap by retaining dev's complete #7552 isolation and assertions. Its version-equivalence coverage supersedes the lane's redundant fallback cache seed. Preserve the lane's config ownership changes and isolated Desktop catalog fixture. Includes dev commits 43f872f (#7552) and bd5c2ea (#7598).
Summary
Two cases failed on timing, not on the behaviour they assert. Each paid a cold process start inside a timed window. This PR moves that cost into budgeted setup and leaves both product bounds unchanged.
#7578:
Node launcher PATH fallback activation › missing bundle reaches validated PATH Bun. Every PATH-fallback case copied an 88 MB Bun binary to a fresh path and ran it inside the case's 5s budget. The OS first-run scan of that new path costs about 1s on an idle macOS host, while a later run of the same path costs about 3ms. On Windows that copy was warmed inside the case; on macOS it was not warmed at all. Under host load the scan pushed the case past 5s. It can also eat into the launcher's own 5s PATH probe. A hardlink or a rename does not reliably keep the scan cache, which is keyed by path. The cases now share one PATH Bun, copied and run once in abeforeAllthat useswarmColdSpawnandCOLD_SPAWN_WARMUP_HOOK_BUDGET_MS, on every platform. The launcher only reads and executes that file, so no assertion changes. Locally each PATH case dropped from about 1.2s to about 0.22s, and the scan now shows up once in the warm-up log line.#7569:
real CurrentUser Root inspection remains read-only for an unregistered synthetic CA. The first PowerShell call in a shard pays process start, .NET load and the X509 store open. That cost crossed the 10s status deadline, and the inspection returnedunknown(run 39848076205,windows 7/9). The timing-dependent half is now split out:createPowerShellCertificateTrustRunnerturns the real runner into a factory with the same defaults. Its options are the inspect deadline, platform,execand PowerShell resolver. Production still calls it with no arguments, so runtime behaviour is unchanged.execFilereports a killed child with partialnot-trustedoutput. The test asserts the result isunknown, with exactly oneinspectrequest at the 10s deadline. When the runner was changed to ignore the error, the test went red.beforeAllreplays the same inspection once throughwarmColdSpawn, following the precedent indesktop-compatibility-launch.test.ts, and fails closed if the warm-up gets no answer. The measured call keeps the unchanged 10s product deadline and still assertsnot-trusted.The
bundledandrepairlauncher cases still run a fresh Bun copy inside their window (about 1.2s and 0.9s locally). Copying or installing that binary is the behaviour those cases test, and neither case was reported as flaky, so they are left as is.Refs #7578
Refs #7569
Verification
bun test tests/cli/ocx-launcher-runtime.test.ts tests/clients/desktop-compatibility-trust.test.ts: 84 pass, 4 skip (Windows-only), 0 fail.execFileerror turned the new deadline test red (1 fail), and restoring it turned it green (7 pass).OCX_TEST_RUNNER_BUN=<bun 1.4.3> bun run test:changed: 8523 pass, 35 skip, 0 fail (381 files).bun test tests/ci-workflows/cold-spawn-warmup.test.ts tests/ci-workflows/warmup-registration.test.ts: 43 pass, 0 fail.bun run typecheck,bun run structure:checkandbun run privacy:scanall pass.bun run testwas not run locally because this host was shared with other worktrees' suites. Coverage is left to the exact-headlane=alldispatch below, which runs the whole suite on Linux, macOS and all nine Windows shards. The real-store case runs only on Windows.lane=allrun 39910034535 onb42cbfd95: all 30 latest jobs succeeded and 5 were skipped. That covers all nine Windows shards, Linuxtest 1-4/4,macos 1-3/3,macos controland theciaggregate. Both target files ran inwindows 6/9:elapsedMs=492, the real CurrentUser Root inspection passed in 402ms against the unchanged 10s deadline, and the new deadline test passed. It also passed onmacos 3/3.elapsedMs=521and the PATH cases ran in 3.4-3.5s each. On the same shard at the merge base26415a90e(run 39909006323) they took 6.5s, 6.5s and 16.9s, withbundled/repairat 9.6s and 12.8s. On Linuxtest 2/4the case took 144ms, and onmacos 1/3342ms.nonecase, which copies no Bun, takes 3.27s). That comes from building each case's package fixture. CI applies--timeout 60000per test, so the 5s default that [Bug]: ocx-launcher-runtime "missing bundle reaches validated PATH Bun" times out at 5 s under host load #7578 hit binds only local runs.windows 5/9on an unrelated case:tests/codex-integration/codex-transition-state.test.ts› "the row validator refuses every whitespace-only txId" timed out at 15s (25.4s). That file does not import any module this PR changes, and no existing issue tracks it. Re-running only the failed jobs passed that shard. Theciaggregate then read the re-run shard asnullfrom the jobs API and failed. A re-run of that job alone succeeded, reporting all nine Windows legs assuccess.Checklist
unknownon any failure.)Summary by CodeRabbit