Test that the IPC handlers use the modules they delegate to

[juanmaguitar]

Closes #129.

The house pattern is to lift a handler's decision-making into a small module and test that module directly. Every test in test/ talks to the module; none talks to the handler. So the connection between the two is untested: delete the openExternalUrl call from the url:open handler, call shell.openExternal in its place, and the whole suite stays green while the app loses the guard #121 added. For a module that formats a string that is a small gap. For one that exists to refuse something, the connection is the entire point.

What changed

test/ipc-wiring.test.cjs (new) — loads src/main.js in an ordinary node --test process with require('electron') replaced by a recording stub, captures the callbacks main.js registers with ipcMain.handle, and invokes them with the delegate module stubbed. It runs the real handler body, so it fails on a deleted call, a renamed export, or a bypass added beside it — not just on missing text. Nothing in the startup path runs: the stub's app.whenReady() returns a promise that never settles, so requiring the file registers handlers and does nothing else. No Electron process, and the suite stays a plain node --test.

The harness lives in the test file rather than a helper, because node --test discovers every .cjs under test/ and would report a helper as a file of its own — and the suite is deliberately run without a path (see the comment in scripts/run-tests-electron.cjs).

src/main.js — the electron-store import moves from module load into getStore(). It has to: an ESM import() is invisible to the Module._load hook, so at require time it pulled in the real electron behind the stub's back and rejected. Deferring it also removes a startup unhandled rejection — a promise nobody was awaiting yet, at the one moment the app has no way to report it. Concurrent callers still share a single import, and a failure still surfaces to the handler that asked. No behaviour change: no handler can run before a window exists.

The coverage guard

The last test requires every channel main.js registers to be classified, each with a reason:

• wired — asserted by a test above.
• no delegation — electron-store reads and writes, a dialog, a path check. There is no module call to delete.
• unwired invariants — npm:kill, playground:start/stop, playground-web:start/stop. These call no module; they hold the spawn and kill invariants inline instead (the child env built by hand rather than by buildChildEnv, shell: false, detached on POSIX, child.kill() where the npm paths use killChildTree). Deleting detached: from playground:start still leaves this suite green. Recorded rather than folded into "no delegation", because fixing it is a decision about main.js — see follow-ups.
• not reachable — site:status and wordpress:setup do delegate, but behind a store read and a network clone respectively.

ipcMain.on channels are recorded too, so a future one-way handler cannot arrive invisible to the guard. A new handler fails the suite until someone classifies it, and a renamed one fails it from the other side. That is what makes this a fix to the pattern rather than seven one-off tests.

Testing

14 tests: url:open → external-url (both with a spy and end-to-end through the real module — file: refused and logged, https: opened as the normalized href), git:worktree-dirty / git:discard-changes / git:update-trunk / sites:add → trunk-update, the three patch channels → ensureAutocrlf and git-update.normalizeEol against a real throwaway repository, npm:install / npm:run-script → npm-runner (the environment buildChildEnv returned is the one that reaches spawn; a failing close consults shouldRetryWithRelaxedEngines, and a true actually starts a second attempt), and the before-quit sweep → kill-tree.

Each assertion was mutation-checked — the acceptance criterion in the issue is "a handler that stops using its module fails the suite", so each mutation was applied alone and reverted:

mutation	result
url:open calls shell.openExternal directly	2 tests fail
ensureAutocrlf deleted from sites:add	1 fails
ensureAutocrlf deleted from the patch path	2 fail
child.kill() instead of killChildTree on quit	1 fails
child env hand-rolled instead of buildChildEnv	2 fail
normalizeEol dropped from both sides of the diff	1 fails
a new unclassified ipcMain.handle	guard fails
a new ipcMain.on channel	guard fails

Suite: 172 passing, 0 failing, on node --test and again on Electron's bundled Node via npm run test:electron. npm run lint clean across the repo.

The second commit

The first commit passed locally and broke test/electron-node-version.test.cjs on both platforms in CI. Worth writing down, because the trap is invisible on any machine that has already run the app.

node_modules/electron/index.js resolves the binary path at module scope and spawns Electron's installer when it is missing. Requiring the package is therefore not inert on a cold checkout: it starts a download into the same dist directory that electron-node-version.test.cjs spawns the binary out of, and node --test runs files concurrently. That test got a half-written framework — segment '__TEXT' load command content extends beyond end of file on macOS, a second "Downloading Electron binary..." on Windows. Locally the binary is already there, the require is a file read, and nothing happens.

Two paths reached it, both from the new file:

• Stubs are built by merging over the real module, so stubbing src/logging.js means requiring it, and it requires electron. That require ran before the Module._load hook was installed. It now happens inside it, and the hook also covers electron/* subpaths and anything resolving into the package.
• sites:add was allowed to run past its delegation into getStore(), whose import('electron-store') does import {app} from 'electron' through the ESM loader — where Module._load does not apply and no hook can help. The test now stops at the delegation, which is all it was ever asserting, the same way the patch-channel test does.

The 14th test fails if the real package is loaded at all, since neither path is visible on a machine where the binary is present. Verified in both directions by removing node_modules/electron/path.txt: before the fix the suite prints "Downloading Electron binary..." and that test fails; after it, neither happens.

No manual pass needed: nothing here ships in the app except the lazy store import, which no handler can observe.

Review

Ran the review in .github/instructions/code-review.instructions.md, judgement pass dispatched to a subagent with the diff and the standard. 1 [fix here] · 3 [follow-up].

Fixed here:

• 🔵 tests — the npm tests reach the real runNpmWithEngineRetry, which calls ensureNodeShimDir() and leaves $TMPDIR/electron-node-shims-<pid>/ behind on every run, on both runtimes. It is module-local and cannot be stubbed; an after() hook now sweeps it.

Also taken, though reported as follow-ups, because they were two lines each:

• 🔵 tests — the guard only saw ipcMain.handle; on was a silent no-op in the stub. Now recorded and asserted.
• 🟡 tests — "no delegation" was documented as "deleting a line from it could not disarm a guard", which was not true of the five spawn/kill handlers. Split into the UNWIRED_INVARIANTS list described above, with what each one holds inline.

Left as follow-ups:

• 🟡 tests — actually wiring those five handlers, either by giving them the modules the npm handlers use or by asserting their spawn options directly. It is a change to main.js, not to this test file, so it does not belong in this PR.
• 🔵 security — sites:delete (src/main.js:658) does fse.remove() on a renderer-supplied path with no check that it is one of the registered sites. Pre-existing and unrelated to this change; filed as Deleting a site trusts the path it is given #133.

Style notes, neither wrong today: assert.equal(spawned.options.env, env) asserts object identity, which is deliberate (a deepEqual would pass for a handler that rebuilt an identical object by hand) but would fail a future env: {...buildChildEnv(), X: 1}. And the 'child_process' stub key now also registers under 'node:child_process', so switching to the prefixed spelling in main.js cannot silently unhook it.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds IPC wiring tests to ensure handlers continue using their delegated modules.

Changes:

• Adds a stubbed Electron harness and wiring coverage guard.
• Lazily initializes electron-store to support Node-based tests.
• Review found one issue: one-way IPC channels cannot be classified as intended.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
test/ipc-wiring.test.cjs	Tests handler-to-module wiring and channel coverage.
src/main.js	Defers store initialization until first use.