Skip to content

Commit 6b0a8a2

Browse files
juanmaguitarclaude
andauthored
[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>
1 parent 904441b commit 6b0a8a2

6 files changed

Lines changed: 133 additions & 15 deletions

File tree

‎.github/instructions/code-review.instructions.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ macOS and Windows are the primary targets; Linux artifacts are published too. CI
148148
- Mocking the very thing the test claims to verify.
149149
- Asserting implementation details — exact log strings, call order of internals — instead of observable behaviour. These break on harmless refactors and survive real bugs.
150150
- 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".
151152
- 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.
152153
- 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.
153154

‎src/hide-child-windows.js‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,17 @@
2222
// the patch ran, which is exactly the kind of thing nobody should have to
2323
// reason about. "Hide the console flashes everywhere, once at startup" is the
2424
// plausible-looking change that reinstates that bug; the ipc-wiring suite
25-
// asserts main.js does not make it. Only the four runners — install, script,
26-
// server, playground-web — take this patch, and only because they load npm's or
27-
// Playground's CLI in-process.
25+
// asserts main.js does not make it. The four runners — install, script,
26+
// server, playground-web — take this patch because they load npm's or
27+
// Playground's CLI in-process; and since #497 so does every Node process below
28+
// them, through the win-spawn-patch.js preload, which requires the copy of this
29+
// file that ensureNodeShimDir() puts beside it. Those descendants are Electron
30+
// running as Node, consoleless like the runners, and Gutenberg's build scripts
31+
// spawn cmd.exe from one of them (cross-spawn around the tsc and wp-build .cmd
32+
// stubs) — without this, each of those was a visible black window.
33+
//
34+
// Self-contained (child_process only): the copy in the shim dir has no
35+
// neighbours from src/ but the patch itself.
2836

2937
const PATCHED = Symbol.for('wp-dev-env.windowsHidePatched');
3038

‎src/main.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,16 @@ function ensureNodeShimDir() {
238238
fs.copyFileSync(path.join(__dirname, 'win-spawn-patch.js'), dest);
239239
spawnPatchPath = dest;
240240
} catch {}
241+
// The patch also hides console windows in every descendant Node
242+
// (#497), through hide-child-windows.js required from beside it.
243+
// Separate try: a missing copy costs the hiding, not the patch.
244+
try {
245+
fs.copyFileSync(path.join(__dirname, 'hide-child-windows.js'), path.join(nodeShimDir, 'hide-child-windows.js'));
246+
} catch (e) {
247+
// 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+
}
241251
// Intentionally do NOT create node.exe here, as Electron's exe depends on adjacent DLLs.
242252
// Using node.exe from a temp dir causes STATUS_DLL_NOT_FOUND (0xC0000135) when spawned by npm.
243253
} else {

‎src/win-spawn-patch.js‎

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,11 @@
1919
// It is the one tool the build spawns bare that a host install shadows with an
2020
// incompatible one.
2121
//
22-
// Deliberately self-contained (Node built-ins only): this file is copied into the
23-
// temp shim dir and required from there, because --require into a path inside
24-
// app.asar is not reliable under ELECTRON_RUN_AS_NODE.
22+
// Deliberately self-contained: Node built-ins, plus the one sibling
23+
// ensureNodeShimDir() copies beside it (hide-child-windows.js, #497) and
24+
// nothing else. This file is copied into the temp shim dir and required from
25+
// there, because --require into a path inside app.asar is not reliable under
26+
// ELECTRON_RUN_AS_NODE, and the shim dir has no other neighbour from src/.
2527

2628
const path = require('path');
2729
const fs = require('fs');
@@ -192,14 +194,35 @@ function applyPatch(childProcess = require('child_process'), config = {}) {
192194
return childProcess;
193195
}
194196

195-
// Only self-applies when the app explicitly asked for it, so requiring this
196-
// module from a test never mutates the test process.
197-
if (process.env.WPTK_SPAWN_PATCH === '1') {
198-
applyPatch(require('child_process'), {
199-
npmCliPath: process.env.WPTK_NPM_CLI || null,
200-
npxCliPath: process.env.WPTK_NPX_CLI || null,
201-
nodeCompatPath: process.env.WPTK_NODE_COMPAT_PATH || null
197+
// What the preload does to the process it was loaded into. Only when the app
198+
// explicitly asked for it, so requiring this module from a test never mutates
199+
// the test process, and only on Windows, where both problems live.
200+
//
201+
// The second half is #497. Every descendant Node here is Electron running as
202+
// Node, a GUI-subsystem binary with no console of its own. When one of them
203+
// spawns cmd.exe without `windowsHide` — cross-spawn wrapping a .cmd stub, as
204+
// Gutenberg's tools/build-scripts do for `tsc` and `wp-build` — Windows finds
205+
// no console to inherit and allocates a brand-new visible one: the black
206+
// windows of #497, and closing one breaks the build. patchChildProcess() from
207+
// hide-child-windows.js (the copy ensureNodeShimDir() puts beside this file)
208+
// forces the flag on every child_process entry point of the process, the way
209+
// the four runners already do for themselves. The runners then get it twice,
210+
// which the patch's own marker makes a no-op. A missing copy costs the hiding,
211+
// never the spawn patch, and says nothing: this process's stdout and stderr
212+
// are the build's output, so a warning here would land in the middle of npm's
213+
// lines; main.js logs the copy failure where the log is.
214+
function selfApply({ env = process.env, platform = process.platform, childProcess = require('child_process'), requireHide = () => require(path.join(__dirname, 'hide-child-windows.js')) } = {}) {
215+
if (platform !== 'win32' || env.WPTK_SPAWN_PATCH !== '1') return;
216+
applyPatch(childProcess, {
217+
npmCliPath: env.WPTK_NPM_CLI || null,
218+
npxCliPath: env.WPTK_NPX_CLI || null,
219+
nodeCompatPath: env.WPTK_NODE_COMPAT_PATH || null
202220
});
221+
try {
222+
requireHide().patchChildProcess(childProcess, platform);
223+
} catch {}
203224
}
204225

205-
module.exports = { resolveSpawnTarget, applyPatch, defaultLookup, PATCH_MARKER };
226+
selfApply();
227+
228+
module.exports = { resolveSpawnTarget, applyPatch, selfApply, defaultLookup, PATCH_MARKER };

‎tests/unit/ipc-wiring.test.cjs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1835,6 +1835,11 @@ test('main.js does not apply the windowsHide patch when it loads', () => {
18351835
[],
18361836
'main.js patched its own child_process with windowsHide — every spawn in the process now carries the flag, including the editor launch (#181)'
18371837
);
1838+
// Since #497 the win-spawn-patch preload reaches hideChildWindows on its
1839+
// own, by absolute path, which the stub map above cannot see. main.js only
1840+
// copies that file; requiring it would be the second way in.
1841+
const preloadRequired = Object.keys(require.cache).filter((file) => file.endsWith(`${path.sep}win-spawn-patch.js`));
1842+
assert.deepEqual(preloadRequired, [], 'main.js required the spawn-patch preload, which applies windowsHide by itself on Windows');
18381843
});
18391844

18401845
// --- npm:* -> src/npm-runner.js + src/kill-tree.js -----------------------
@@ -1908,6 +1913,15 @@ function assertShimsPreloadCompat(label) {
19081913
shim.includes(`--require "${compat}"`),
19091914
`${label}: the node shim starts a child without the preload, so any yargs-based tool it runs misreads its arguments`
19101915
);
1916+
// On Windows the spawn patch is preloaded into every descendant Node and
1917+
// requires the hide-child-windows copy beside it (#497); both have to be
1918+
// there, since a path inside app.asar is not reliable under
1919+
// ELECTRON_RUN_AS_NODE.
1920+
if (process.platform === 'win32') {
1921+
for (const name of ['win-spawn-patch.js', 'hide-child-windows.js']) {
1922+
assert.ok(fs.existsSync(path.join(shimDir, name)), `${label}: ${name} was not copied next to the shims`);
1923+
}
1924+
}
19111925
}
19121926

19131927
// The three cross-platform decisions every spawn in main.js makes. No module

‎tests/unit/win-spawn-patch.test.cjs‎

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const fs = require('node:fs');
44
const os = require('node:os');
55
const path = require('node:path');
66

7-
const { resolveSpawnTarget, applyPatch, defaultLookup, PATCH_MARKER } = require('../../src/win-spawn-patch.js');
7+
const { resolveSpawnTarget, applyPatch, selfApply, defaultLookup, PATCH_MARKER } = require('../../src/win-spawn-patch.js');
88

99
// The Windows shim layout ensureNodeShimDir() writes, as the patch sees it.
1010
const WIN = {
@@ -242,3 +242,65 @@ test('applyPatch is idempotent so a duplicated --require cannot double-wrap', ()
242242
fake.spawn('node');
243243
assert.equal(depth, 1);
244244
});
245+
246+
// #497: the preload reaches every descendant Node on Windows, and each of those
247+
// is Electron running as Node, a GUI-subsystem binary with no console. A
248+
// cmd.exe it spawns without windowsHide (cross-spawn wrapping a .cmd stub, as
249+
// Gutenberg's build scripts do for tsc and wp-build) gets a brand-new visible
250+
// console. So the same preload that fixes `spawn('node')` also hides consoles,
251+
// through the hide-child-windows copy beside it.
252+
function fakeChildProcess() {
253+
return { spawn: () => 'spawned', spawnSync: () => {}, execFile: () => {}, execFileSync: () => {} };
254+
}
255+
256+
test('selfApply applies the spawn patch and hides consoles on Windows when the app asked for it', () => {
257+
const cp = fakeChildProcess();
258+
const hidden = [];
259+
selfApply({ env: { WPTK_SPAWN_PATCH: '1' }, platform: 'win32', childProcess: cp, requireHide: () => ({ patchChildProcess: (target, platform) => { hidden.push([target, platform]); return target; } }) });
260+
261+
assert.equal(cp[PATCH_MARKER], true, 'the spawn patch was not applied');
262+
assert.deepEqual(hidden, [[cp, 'win32']], 'patchChildProcess was not applied to the same child_process');
263+
});
264+
265+
// The default requireHide is the one production runs: the copy in the shim dir
266+
// resolving its sibling by name. Reproduced with the two files copied into a
267+
// temp dir, the way ensureNodeShimDir() lays them out, and the real
268+
// hide-child-windows applied to a fake child_process so the test process is
269+
// never touched.
270+
test('selfApply from a shim-dir copy finds hide-child-windows.js beside it', () => {
271+
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'wptk-shims-'));
272+
try {
273+
for (const name of ['win-spawn-patch.js', 'hide-child-windows.js']) {
274+
fs.copyFileSync(path.join(__dirname, '../../src', name), path.join(dir, name));
275+
}
276+
const copy = require(path.join(dir, 'win-spawn-patch.js'));
277+
const cp = fakeChildProcess();
278+
copy.selfApply({ env: { WPTK_SPAWN_PATCH: '1' }, platform: 'win32', childProcess: cp });
279+
280+
assert.equal(cp[PATCH_MARKER], true);
281+
assert.equal(cp[Symbol.for('wp-dev-env.windowsHidePatched')], true, 'the sibling copy was not found, so consoles would show (#497)');
282+
} finally {
283+
fs.rmSync(dir, { recursive: true, force: true });
284+
}
285+
});
286+
287+
test('selfApply does nothing without the flag, and nothing off Windows', () => {
288+
for (const input of [
289+
{ env: {}, platform: 'win32' },
290+
{ env: { WPTK_SPAWN_PATCH: '0' }, platform: 'win32' },
291+
{ env: { WPTK_SPAWN_PATCH: '1' }, platform: 'darwin' }
292+
]) {
293+
const cp = fakeChildProcess();
294+
let hideAsked = false;
295+
selfApply({ ...input, childProcess: cp, requireHide: () => { hideAsked = true; return { patchChildProcess: (target) => target }; } });
296+
assert.equal(cp[PATCH_MARKER], undefined, JSON.stringify(input));
297+
assert.equal(hideAsked, false, JSON.stringify(input));
298+
}
299+
});
300+
301+
test('selfApply keeps the spawn patch when the hide copy is missing beside it', () => {
302+
const cp = fakeChildProcess();
303+
selfApply({ env: { WPTK_SPAWN_PATCH: '1' }, platform: 'win32', childProcess: cp, requireHide: () => { throw new Error("Cannot find module './hide-child-windows.js'"); } });
304+
305+
assert.equal(cp[PATCH_MARKER], true, 'a missing hide copy must not cost the spawn patch');
306+
});

0 commit comments

Comments
 (0)