Speed up pnpm dev-server startup

[Maximo-Guk]

pnpm dev-server is now 2.2x faster

Changes:

#	Change	Scope
1	build:app becomes a cached Vite+ task with an explicit input set	production build + deploy
2	build:app:dev builds the same bundle unminified for the dev pre-flight	dev, shares machinery with prod
3	Worker build.command spawns its binary directly, not via pnpm exec	dev only, falls back if unresolved
4	The 2 app watchers are deferred until Wrangler is listening	dev only, process lifecycle

Stacked on #173 ( Typescript v7 bump )

[ask-bonk]

All imports are used. The review is complete.

I reviewed all 15 changed files thoroughly, focusing on the two areas the author flagged (the build:app cache correctness and the spawn-based shutdown), plus the process-lifecycle signal handling, exit-code propagation, the pnpm-indirection rewriter, and the cache input/output globs.

Findings:

• build:app cache correctness (the flagged concern): The input excludes (dist-app/**, src/generated/**, .wrangler/** at base: "workspace") and outputs are internally consistent, and build:app vs build:app:dev have distinct cache keys, so the dev pre-flight's unminified app.txt can't leak into a shipped build/deploy artifact. The VITE_FRONTEND_ERROR_REPORTING env is correctly declared in the fingerprint. Sound.
• Shutdown/signal handling: wranglerChild is null before spawn and handled via ?.; the exitCode !== null/=== null checks correctly distinguish "wrangler running" (let it drive teardown, Ctrl-C reaches the group) from "nothing to wait for" (exit directly). SIGTERM forwarding, the startDeferredWatchers stoppingDevWatchers guard, and the 128 + constants.signals[signal] exit-code math are all correct.
• withoutPnpmIndirection / resolveBinEntry: Handles both pnpm run <script> (backend's build:worker, with && chaining preserved) and pnpm exec <bin> (gatekeepers' capnweb-validate), and falls back to the original command on any resolution failure, so an unresolved binary is safe. Dependencies aren't installed in this CI checkout so I couldn't runtime-verify resolution, but the fallback makes a miss harmless.

Two non-blocking nits (not posted inline): packages/gatekeeper-scheduler/build-app.mjs has garbled comments at line 8 ("pre-flight. see") and lines 17–18 ("Always set explicitly an inherited …" — missing punctuation); the equivalent gatekeeper-context comments read cleanly.

No actionable defects found.

LGTM!

github run

[ask-bonk]

The distinction is clear: -r selects all packages recursively; without -r (as in the package deploy/build scripts) vp scopes to the cwd package. This is the author's design consistent with AGENTS.md guidance. I can't runtime-verify without installed deps, but the pattern is deliberate and consistent.

I've completed a thorough independent review. Let me consolidate my findings.

Review Summary

I reviewed the process-lifecycle rewrite in run-dev-server.js, the new kill-process-tree.js and clean-error-reporting-artifacts.mjs scripts, the build:app/build:app:dev Vite+ task split, the shared configurator vite.config extraction, cache-key isolation, and the pnpm exec de-indirection.

Correctness — sound.

• killProcessTree collects the whole tree breadth-first before signalling (load-bearing, since signalling the wrapper reparents and hides its children from pgrep -P), guards code !== 0 on the child-lister no-match, and rejects non-positive/non-integer pids before process.kill (where 0/negatives address process groups). Ran node --test scripts/kill-process-tree.test.js → 4/4 pass.
• Signal/shutdown handling distinguishes pre-spawn / running / exited via wranglerChild?.exitCode; two-signal escalation, the FORCE_KILL_GRACE_MS backstop, and 128 + constants.signals[signal] are correct. The synchronous exit-handler stopDevWatchers() is only reachable as a backstop or on the wrangler-spawn-error path, where the deferred app watchers (the only ones with grandchildren) haven't started and the configurator watcher is in-process — so the shallow .kill() is sufficient. Normal paths go through stopDevWatchersDeep() first.
• Cache-key isolation between build:app (minified, output: dist-app/** + app.txt) and build:app:dev (unminified, output: app.txt) is sound; GATEKEEPER_APP_UNMINIFIED is always set explicitly, and vp restores cached outputs on a hit, so no unminified bytes can leak into a shipped build/deploy artifact. build/deploy use build:app.
• clean-error-reporting-artifacts.mjs deletes only .js/.js.map in src/generated+dist-app, matching the reporting artifacts; the direct build/deploy configurator path self-cleans (builder line 1033), and the uncached clean:error-reporting-artifacts task covers the cached vp path where a hit skips the builder. Consistent.
• withoutPnpmIndirection expands the backend's pnpm run build:worker (&&-chain preserved, inner pnpm exec rewritten) and gatekeepers' pnpm exec capnweb-validate, with a safe fallback on any resolution miss; shellSafe guards the quoting.
• Docs (AGENTS.md, SKILL/SKELETON), tsconfig.vite.json, and the vite-config test are updated to vite.app.config.ts consistently; no dangling pnpm run build:configurator/build:app references. build:app:watch remains orphaned but pre-dates this PR.

Non-blocking findings (comment/prose only — not posted inline, since prior reviewers already raised them):

1. scripts/build-gatekeeper-configurator.test.js:177 — the comment inverts its own logic: "If these two ever agreed, the pre-flight's output would differ from the watcher's." The --dev flag exists to make the pre-flight and watchers agree on env mode; divergence is the failure. Should read "diverged"/"disagreed". Code is correct.
2. Both build-app.mjs files (gatekeeper-context:9-10/24-25, gatekeeper-scheduler:7-8/17-18) have garbled comment prose — "…pre-flight see the unminified note…" runs two sentences together, and "Always set explicitly an inherited GATEKEEPER_APP_UNMINIFIED would…" is missing punctuation after "explicitly".

I could not runtime-verify bin resolution, vp package scoping/trailing-arg forwarding of --dev, or the full build, since dependencies aren't installed in this read-only checkout — but the fallback path makes any resolution miss harmless and the author reports a passing build.

The only issues I found are comment-only defects, already noted by prior reviewers. No new actionable defects.

LGTM!

github run

[ask-bonk]

That's just the read-only checkout lacking installed dependencies (typescript6 alias), not a defect in the PR. The kill-process-tree test (which has no external deps) passes 4/4.

I've now completed a thorough independent review of all substantive files. My analysis confirms the mechanisms are sound. Let me consolidate.

Review Summary

I reviewed the process-lifecycle rewrite in run-dev-server.js, the new kill-process-tree.js and clean-error-reporting-artifacts.mjs scripts, the shared gatekeeper-configurator-vite-config.js, the build:app/build:app:dev Vite+ task split, cache-key isolation, and the pnpm exec de-indirection.

Correctness — sound.

• killProcessTree collects the whole tree breadth-first before signalling (load-bearing — signalling the wrapper first reparents its children and hides them from pgrep -P), guards code !== 0 on the child-lister no-match, and rejects 0/negative/non-integer/partially-numeric pids before process.kill (where 0/negatives address process groups). Ran node --test scripts/kill-process-tree.test.js → 4/4 pass.
• Signal/shutdown handling distinguishes pre-spawn (wranglerChild null → ?.exitCode undefined !== null), running (exitCode === null → let Wrangler drive teardown), and exited via wranglerChild?.exitCode; the two-signal escalation, FORCE_KILL_GRACE_MS backstop, and 128 + constants.signals[signal] math (130/143) are correct. Every path that reaches process.exit with live app watchers (the only ones with grandchildren) first awaits stopDevWatchersDeep(); the sole exception is the narrow dev-only wrangler error handler.
• Cache-key isolation: build:app (minified, output: dist-app/** + app.txt) and build:app:dev (unminified via node build-app.mjs --dev → GATEKEEPER_APP_UNMINIFIED: "true", output: app.txt only) are distinct tasks with identical env fingerprints; the watcher's isWatch and the pre-flight's --dev both resolve unminified, so their app.txt bytes match. vp restores cached outputs on a hit, so no unminified bytes leak into a shipped build/deploy artifact.
• clean-error-reporting-artifacts.mjs deletes only .js/.js.map in src/generated+dist-app; the direct build/deploy configurator path self-cleans (build-gatekeeper-configurator.mjs:1033), and the uncached clean:error-reporting-artifacts dependsOn covers the cached vp path where a hit skips the builder. Consistent.
• withoutPnpmIndirection expands the backend's pnpm run build:worker (&&-chain preserved, inner pnpm exec rewritten) and gatekeepers' pnpm exec capnweb-validate, with shellSafe-guarded quoting and a safe fallback to the committed command on any resolution miss.
• Docs (AGENTS.md, SKILL/SKELETON), tsconfig.vite.json, and the vite-config test are updated to vite.app.config.ts consistently; no dangling pnpm run build:configurator/build:app references.

Findings (non-blocking, comment/prose only — not posting inline; already raised by prior reviewers):

1. scripts/build-gatekeeper-configurator.test.js:176-178 — the comment inverts its own logic: "If these two ever agreed, the pre-flight's output would differ from the watcher's." The --dev flag exists to make the pre-flight and watchers agree on env mode; divergence is the failure. Should read "diverged"/"disagreed". The code and assertions are correct.
2. Both build-app.mjs files have garbled comment prose: gatekeeper-context:9-10 / gatekeeper-scheduler:7-8 run two sentences together ("…pre-flight see the unminified note…"), and the GATEKEEPER_APP_UNMINIFIED comment (context:24-25, scheduler:17-18) is missing punctuation after "explicitly".
3. build:app:watch in both package.jsons is now orphaned (dev-server spawns build-app.mjs --watch directly). Pre-existing, harmless.

I could not runtime-verify bin resolution, vp package scoping, or the full build/configurator test (typescript6 alias and other deps aren't installed in this read-only checkout — the one configurator test failure is ERR_MODULE_NOT_FOUND, an environment limitation, not a defect), but the fallback path makes any resolution miss harmless.

The three findings are comment-only, and all were already raised by prior reviewers. No new actionable defects.

LGTM!

github run

[ask-bonk]

No dangling references. The build:app:watch orphaned script (mentioned by prior reviewers) is pre-existing. Let me verify the build:app:watch script still exists but is unused — this is harmless and pre-dates this PR's intent (the dev-server spawns build-app.mjs --watch directly).

Let me do a final check on the build-gatekeeper-configurator.mjs — the pre-flight passes --dev and the watcher passes --watch. Both map to development mode. Verified at line 15: watchMode || devMode ? "development" : "production". Consistent.

I've completed a thorough review. My analysis confirms the mechanisms are sound and the tests pass (4/4 for kill-process-tree). Let me consolidate.

Review Summary

I independently reviewed the substantive changes: the process-lifecycle rewrite in run-dev-server.js, the new kill-process-tree.js and clean-error-reporting-artifacts.mjs scripts, the build:app/build:app:dev Vite+ task split, the shared gatekeeper-configurator-vite-config.js, and the pnpm exec de-indirection.

Correctness — sound.

• killProcessTree (scripts/kill-process-tree.js): collects the whole tree breadth-first before signalling (load-bearing — signalling the wrapper first reparents its children and hides them from pgrep -P), guards code !== 0 on the child-lister no-match, and rejects non-positive/non-integer pids (including "123", 12.5) before process.kill, where 0/negatives address process groups. Ran node --test scripts/kill-process-tree.test.js → 4/4 pass.
• Signal/shutdown handling (run-dev-server.js): the pre-spawn / running / exited states are distinguished correctly via wranglerChild?.exitCode; the two-signal escalation, FORCE_KILL_GRACE_MS backstop, and 128 + constants.signals[signal] math are correct. The synchronous exit-handler stopDevWatchers() is only a backstop or reachable on the wrangler spawn-error path — where the only spawned watchers are the configurator ones, which run entirely in-process (node:fs.watch, no grandchildren). The deferred app watchers (the only ones with pnpm exec vite grandchildren) start after Wrangler is listening, past that path, and every normal path deep-kills via stopDevWatchersDeep() first.
• Cache-key isolation: build:app (minified, output: dist-app/** + app.txt) and build:app:dev (unminified, output: app.txt) are distinct tasks with identical env fingerprints; GATEKEEPER_APP_UNMINIFIED is always set explicitly in both build-app.mjs files, and vp restores cached outputs on a hit, so no unminified bytes can leak into a shipped build/deploy artifact.
• clean-error-reporting-artifacts.mjs: deletes only .js/.js.map in src/generated + dist-app (matching the reporting artifacts; .txt/bundle untouched); wired as an uncached dependsOn so a cache hit — which only restores, never deletes — can't leave a stale enabled-reporting sourcemap alongside a disabled bundle.
• withoutPnpmIndirection: correctly expands the backend's pnpm run build:worker (&&-chain preserved, inner pnpm exec rewritten) and gatekeepers' pnpm exec capnweb-validate, with shellSafe-guarded quoting and a safe fallback to the committed command on any resolution miss.
• Configurator extraction / script rewrites: build/deploy now call the configurator builder directly (production mode) and vp run --cache build:app for the app; only the dev-server pre-flight uses the build:configurator task (with --dev, mapping to development env mode, agreeing with the --watch watcher). No dangling pnpm run build:configurator/build:app references; docs/tsconfig/vite-config test updated to vite.app.config.ts.

Findings (non-blocking, comment/prose only — not posted inline, since prior reviewers already raised these):

1. scripts/build-gatekeeper-configurator.test.js:177 — the comment inverts its own logic: "If these two ever agreed, the pre-flight's output would differ from the watcher's." The --dev flag exists to make the pre-flight and watcher agree on env mode; divergence is the failure. Should read "diverged"/"disagreed". Code and assertions are correct.
2. Both build-app.mjs files (gatekeeper-context:9-10/24-25, gatekeeper-scheduler:7-8/17-18) have garbled comment prose — "…for the pnpm dev-server pre-flight see the unminified note…" runs two sentences together (missing terminator after "pre-flight"), and "Always set explicitly an inherited GATEKEEPER_APP_UNMINIFIED would…" is missing punctuation after "explicitly".

I could not runtime-verify bin resolution, vp package scoping/trailing-arg forwarding of --dev, or the full build, since dependencies aren't installed in this read-only checkout — but the fallback path makes any resolution miss harmless and the author reports a passing build.

The only issues are comment-only defects, all already raised by prior reviewers. No new actionable defects.

LGTM!

github run

[ask-bonk]

Confirmed. withoutPnpmIndirection("pnpm run build:worker") matches ^pnpm run ([\w:.@-]+)$ (: allowed), expands the body node build-browser-runtime.mjs && pnpm exec capnweb-validate build --out .wrangler/validate, recurses at depth 1 (doesn't match ^pnpm run$), then the replaceAll rewrites pnpm exec capnweb-validate while preserving the && prefix and trailing args. The [\w@/.-]+ captures capnweb-validate. Sound.

My review is complete. Let me consolidate.

Review Summary

I independently reviewed the substantive changes: the process-lifecycle rewrite in run-dev-server.js, the new kill-process-tree.js and clean-error-reporting-artifacts.mjs scripts, the shared gatekeeper-configurator-vite-config.js, the build:app/build:app:dev Vite+ task split, and the pnpm exec de-indirection.

Correctness — sound.

• killProcessTree (scripts/kill-process-tree.js:65-83): collects the whole tree breadth-first before signalling (load-bearing — signalling the wrapper first reparents its children and hides them from pgrep -P, correctly noted at :79-82), guards code !== 0 on the child-lister no-match (:30), and rejects non-positive/non-integer pids (:69) before process.kill, where 0/negatives address process groups. Ran node --test scripts/kill-process-tree.test.js → 3/3 pass.
• Signal/shutdown handling (run-dev-server.js:174-204, 565-576): pre-spawn (wranglerChild null → ?.exitCode undefined !== null), running (exitCode === null → let Wrangler drive teardown), and exited are distinguished correctly. The two-signal escalation, FORCE_KILL_GRACE_MS backstop, and 128 + constants.signals[signal] math are correct. The wrangler-error path's bare process.exit(1) (:567) is safe: only the in-process configurator watchers (node:fs.watch, no grandchildren) have spawned by then, and the deferred app watchers start only after waitForPort.
• Cache-key isolation: build:app (minified, output: dist-app/** + app.txt) and build:app:dev (unminified, output: app.txt) are distinct tasks with identical env fingerprints; GATEKEEPER_APP_UNMINIFIED is set explicitly in both build-app.mjs files, and vp restores cached outputs on a hit, so no unminified bytes can leak into a shipped build/deploy artifact.
• clean-error-reporting-artifacts.mjs: deletes only .js/.js.map in src/generated+dist-app, wired as an uncached dependsOn so a cache hit (restore-only) can't leave a stale enabled-reporting sourcemap alongside a disabled bundle.
• withoutPnpmIndirection: verified it expands the backend's pnpm run build:worker (&&-chain preserved, inner pnpm exec capnweb-validate rewritten) and gatekeepers' pnpm exec, with shellSafe-guarded quoting and a safe fallback on any resolution miss.
• Package/config consistency: audited all 16 gatekeepers — every configurator- or app-bearing package gained a vite.config.ts; cloudflare (neither) correctly has none. No dangling pnpm run build:configurator/build:app script references; tsconfig.vite.json, the vite-config test, AGENTS.md, and SKILL/SKELETON docs updated consistently.
• CI timeout-minutes: 20 is a reasonable backstop against a wedged test process, matching the AGENTS.md note about OOM-killed workerd children.

Findings (non-blocking, comment/prose only — not posted inline, since prior reviewers already raised them):

1. scripts/build-gatekeeper-configurator.test.js:177 — the comment inverts its own logic: "If these two ever agreed, the pre-flight's output would differ from the watcher's." The --dev flag exists to make the pre-flight and watchers agree on env mode; divergence is the failure. Should read "diverged"/"disagreed". The code and assertions are correct.
2. Both build-app.mjs files (gatekeeper-context:9-10/24-25, gatekeeper-scheduler:7-8/17-18) have garbled comment prose — "…pre-flight see the unminified note…" runs two sentences together (missing terminator after "pre-flight"), and "Always set explicitly an inherited GATEKEEPER_APP_UNMINIFIED would…" is missing punctuation after "explicitly".

Both findings are comment-only and were already raised by prior reviewers, so I have not posted them inline. I could not runtime-verify bin resolution, vp package scoping, or the full build (dependencies aren't installed in this read-only checkout), but the fallback path makes any resolution miss harmless and the author reports a passing build.

No new actionable defects found.

LGTM!

github run