You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit 6b0a8a2
Browse filesBrowse the repository at this point in the historyBrowse files
[Fix] Hide console windows in every Node process below the runners, not only in the runners (#512)
## Why
On Windows, a Gutenberg build opens black console windows, and closing
one breaks the build: npm reports `Build failed: Command failed:
wp-build` while `wp-build` runs on as an orphan (#497, #498). A Core
build never shows one.
**Where it comes from, pinned from the code.** The four runners call
`hideChildWindows()` before loading npm in-process, so npm's own
`cmd.exe` children are spawned with `windowsHide`. Gutenberg's
`tools/build-scripts/build.mjs` is a separate Node process below npm:
`node` on PATH is the app's `node.cmd` shim, which runs Electron as
Node, and Electron is a GUI-subsystem binary with no console of its own.
`build.mjs` runs `tsc` and `wp-build` through `cross-spawn`, which
resolves their `.cmd` stubs and calls `spawn('cmd.exe', ['/d','/s','/c',
…])` with no `windowsHide`. A console application spawned by a
consoleless parent without `CREATE_NO_WINDOW` gets a brand-new visible
console. Core's grunt spawns no `.cmd` below npm, which is why it never
showed one.
## What changes
The `win-spawn-patch.js` preload already reaches every descendant Node
on Windows through `NODE_OPTIONS=--require` (that is how a bare
`spawn('node')` keeps working). Its self-apply is now `selfApply()`,
exported and injectable, and on Windows it also applies
`hideChildWindows()` from a copy of `hide-child-windows.js` that
`ensureNodeShimDir()` places beside it, for the same reason the patch
itself is copied: a `--require` into `app.asar` is not reliable under
`ELECTRON_RUN_AS_NODE`. The runners get the patch twice, which its
marker makes a no-op. A missing copy costs the hiding, never the spawn
patch. Nothing changes off Windows, and main.js still never takes the
patch (the editor launch needs its window, #181).
## How to test this
Platform: **Windows** (the change is a no-op elsewhere). Build: the
Buildkite artifact for the current head. Use a Gutenberg site past the
install step; a copy, not one you care about.
1. Click **Run full build**. Expected: no console window at any point
during the transpile and bundle phases; the build ends with the same
output as before.
2. Optional: **Start build watch**. Expected: no window either; the tab
reaches *(watching)*.
**What must not have happened:** a window during either; a build that
fails to start (`spawn node` EINVAL would mean the spawn patch was lost
with the hiding); Core's build changing in any way.
Done on Windows 11 (UTM VM, arm64 host), by hand, on the Buildkite
artifact for `f49c77f` (#513's head, which includes this branch at
`5084f31`), Gutenberg site past install: **Run full build** ran to the
end with no console window at any point, and **Start build watch** the
same; Task Manager showed the `conhost.exe` of the hidden consoles where
the windows used to be. Not tested by hand on macOS: nothing runs
differently there. The unit tests inject the platform.
Tests: `tests/unit/win-spawn-patch.test.cjs`, three `selfApply` cases,
fail on trunk (no `selfApply`) and pass here;
`tests/unit/ipc-wiring.test.cjs` asserts the copy lands beside the patch
on Windows. A third case lays the two files out in a temp dir the way
`ensureNodeShimDir()` does and applies the real hide patch to a fake
`child_process`, so the sibling lookup production runs is what the suite
pins. Lint clean, 1614 unit tests pass.
## Risks and limitations
- The hiding applies to every `child_process` entry point of every
descendant Node, overriding an explicit `windowsHide: false`. A build
tool that wanted a visible window would lose it; none does.
- The one link this cannot reach is a `cmd.exe` spawned by another
`cmd.exe` (a `.cmd` stub calling a `.cmd` stub); those inherit the
hidden console of the Node that started the chain, so they are covered
by inheritance, not by the patch.
- #498 (the orphan the closed window leaves) is the next PR in this
stack.
## Related
Fixes#497. #498's PR is stacked on this one. Seen on the #508 pass.
Part of #251, v1.2.0-beta.2.
---
<details>
<summary>Review outcome (required — see AGENTS.md)</summary>
2 [fix here] · 2 [follow-up] on `82f0d2f`, all four taken in `5084f31`.
- Fixed: the header of `win-spawn-patch.js` still said built-ins only.
- Fixed: the default `requireHide`, the only one production runs, had no
test; `selfApply` now applies `patchChildProcess` to the same
`child_process` it patched, which made it testable from a shim-dir
layout in a temp dir.
- Taken: a failed copy of `hide-child-windows.js` is logged from main,
the one process that can say so (the preload's stdout is the build's).
- Taken: the #181 guard also asserts main never requires the preload,
the second way into `hideChildWindows` the stub map cannot see.
- The review named a rule the instructions file did not cover: a
stub-map test that an absolute-path `require` walks around. Added to
`.github/instructions/code-review.instructions.md` §5.
</details>
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Copy file name to clipboardExpand all lines: .github/instructions/code-review.instructions.md
+1Lines changed: 1 addition & 0 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -148,6 +148,7 @@ macOS and Windows are the primary targets; Linux artifacts are published too. CI
148
148
- Mocking the very thing the test claims to verify.
149
149
- Asserting implementation details — exact log strings, call order of internals — instead of observable behaviour. These break on harmless refactors and survive real bugs.
150
150
- Cleanup that is green while doing nothing. `t.after` hooks run in the order they were registered, so a fixture that reaches outside its own temporary directory (a worktree beside it, a patch file next to it) cannot clean up through a repository an earlier hook already removed; remove it as a directory with `removeRepo`, and use the throwing helper (`gitOk`) for any cleanup that goes through Git, or the only symptom is disk filling up.
151
+
- A stub-map test that an absolute-path `require` walks around. The `ipc-wiring` loader stubs modules by their relative request (`'./hide-child-windows'`); a module reached by `require(path.join(__dirname, …))`, or by a file that main.js copies rather than requires, never hits the map, so a test asserting "this stub was not called" stays green while the real module runs. Assert on `require.cache` as well when the rule is "never loaded", not "never called".
151
152
- A journey locator that changed meaning. `tests/e2e/journeys/*.spec.js` pin user-facing strings with `getByText(…, { exact: true })`; when a PR turns a fixed string into a conditional one (a headline that follows a state), grep the journeys for the old string and check that each assertion still asserts what it meant, rather than passing only because the fixture never reaches the new branch.
152
153
- Passing on only one of the two Node runtimes. CI runs the suite on `.nvmrc`'s Node *and* on Electron's bundled Node because the two are set independently and have drifted; a test (or the code under it) that assumes the newer of the two is broken on the other.
// The preload cannot say so itself (its stdout is the build's), so
248
+
// this is the one line that explains black windows coming back.
249
+
logError('shims',`could not copy hide-child-windows.js next to the shims; console windows will show below the runners: ${String(e&&e.message ? e.message : e)}`);
250
+
}
241
251
// Intentionally do NOT create node.exe here, as Electron's exe depends on adjacent DLLs.
242
252
// Using node.exe from a temp dir causes STATUS_DLL_NOT_FOUND (0xC0000135) when spawned by npm.
0 commit comments