From 91cc37d8dfa1f991712329595b060f324c71a84c Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 07:07:23 +0200 Subject: [PATCH] Prove the delete handler still goes through the registry gate (#145) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a site is the least recoverable action in the app. #138 put a gate in front of it — `deleteRegisteredSite` refuses any path the app does not have on record — and the module is well tested, but nothing proved the handler still calls it. `test/ipc-wiring.test.cjs` classified `sites:delete` under NO_DELEGATION as "electron-store write plus a directory removal", which was false: replacing the handler body with a bare `fse.remove` kept the whole suite green, the exact failure that suite exists to catch. The reason it was classified around the hole rather than through it is that the handler reads the settings store first, and the store arrives via a dynamic `import()` that the harness's `Module._load` hook cannot intercept. So the store moves into `src/settings-store.js`, a seam the harness can stand in for, the same shape the neighbouring registry calls already have. `getStore` is unchanged — same singleton, same deferred import, same reasons for deferring it. With the seam in place, `sites:delete` moves into WIRED and gains two tests: one that it asks site-registry at all, and one end-of-wire test on real directories with the real `fse.remove`, proving an unregistered path reaches neither the removal nor the store. Both fail if the gate is cut. `site:status` was in NOT_REACHABLE for the same store limitation, so its recorded reason would have become untrue; it is wired too rather than left carrying a stale claim. Co-Authored-By: Claude Opus 5 (1M context) --- src/main.js | 26 +-------- src/settings-store.js | 36 ++++++++++++ test/ipc-wiring.test.cjs | 122 +++++++++++++++++++++++++++++++++++---- 3 files changed, 149 insertions(+), 35 deletions(-) create mode 100644 src/settings-store.js diff --git a/src/main.js b/src/main.js index ca383a4..3fe3096 100644 --- a/src/main.js +++ b/src/main.js @@ -31,6 +31,7 @@ const { normalizeEol } = require('./git-update.cjs'); const { ensureAutocrlf, readTrunkInfo, collectDirtyFiles, discardChanges, updateToLatestTrunk } = require('./trunk-update'); const { openExternalUrl, ALLOWED_URL_SCHEMES } = require('./external-url'); const { deleteRegisteredSite } = require('./site-registry'); +const { getStore } = require('./settings-store'); const WORDPRESS_GIT_URL = 'https://github.com/WordPress/wordpress-develop.git'; @@ -124,31 +125,6 @@ function spawnRunner(runnerPath, args, { cwd, extraEnv = {} }) { }); } -let store; // initialized asynchronously due to ESM-only module -let storeReady = null; - -// The import starts on first use rather than at require time. Deferring it costs -// nothing — no handler can run before a window exists — and it keeps two things -// out of module load: a rejected promise nobody is awaiting yet (an unhandled -// rejection at startup, where the app has no way to report it), and the ESM -// loader pulling in `electron` behind `Module._load`'s back, which is what lets -// test/ipc-wiring.test.cjs require this file outside an Electron process. -async function getStore() { - if (!store) { - if (!storeReady) { - storeReady = import('electron-store').then((m) => { - const Store = m.default || m; - store = new Store({ - name: 'settings', - defaults: { sites: [], siteMeta: {} } - }); - }); - } - await storeReady; - } - return store; -} - function findAvailableDirName(rootDir, baseName) { const sanitizedBase = baseName || 'wordpress-develop-trunk'; let candidate = sanitizedBase; diff --git a/src/settings-store.js b/src/settings-store.js new file mode 100644 index 0000000..6ee0585 --- /dev/null +++ b/src/settings-store.js @@ -0,0 +1,36 @@ +// The one `electron-store` instance the app has, behind a seam tests can reach. +// +// `electron-store` is ESM-only, so main.js loaded it with a dynamic `import()`. +// That import is invisible to `Module._load`, which is what test/ipc-wiring.test.cjs +// uses to stand in for a module — so every handler that read the store was +// unreachable from that suite, and the delete handler's registry gate could be cut +// without failing anything (#145). Keeping the import in a module of its own means +// the harness replaces this file instead of trying to intercept the ESM loader. +// +// The import starts on first use rather than at require time. Deferring it costs +// nothing — no handler can run before a window exists — and it keeps a rejected +// promise nobody is awaiting yet out of module load, where the app has no way to +// report it. It also keeps the ESM loader from pulling in `electron` behind +// `Module._load`'s back, which is what lets that suite require main.js outside an +// Electron process at all. + +let store; +let storeReady = null; + +async function getStore() { + if (!store) { + if (!storeReady) { + storeReady = import('electron-store').then((m) => { + const Store = m.default || m; + store = new Store({ + name: 'settings', + defaults: { sites: [], siteMeta: {} } + }); + }); + } + await storeReady; + } + return store; +} + +module.exports = { getStore }; diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 6052e3f..bd3590c 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -326,6 +326,107 @@ test('url:open refuses a file: address and opens an http one', async () => { assert.deepEqual(main.calls.openExternal, ['https://wordpress.org/']); }); +// The settings store is an ESM-only dependency loaded through a dynamic import, +// which `Module._load` cannot intercept — so it lives behind src/settings-store.js +// and this stands in for it. Values are held in a plain object the test can read +// back, because "the store was not written" is half of what the delete gate +// promises (#145). +function fakeSettingsStore(initial = {}) { + const values = { sites: [], siteMeta: {}, ...initial }; + const store = { + // A copy, like the real one: `conf` re-reads and re-deserializes the file on + // every `get`, so a handler that mutates what it read and never sets it back + // loses the write in the app. Returning the live object here would let that + // bug pass. + get: (key) => structuredClone(values[key]), + set: (key, value) => { values[key] = value; } + }; + return { values, stubs: { './settings-store': { getStore: async () => store } } }; +} + +// --- sites:delete -> src/site-registry.js -------------------------------- + +test('sites:delete asks site-registry whether the path may be removed', async () => { + const deleteRegisteredSite = spy(async () => true); + const settings = fakeSettingsStore({ sites: ['/sites/wp'] }); + const main = loadMain({ + stubs: { ...silentLogging(), ...settings.stubs, './site-registry': { deleteRegisteredSite } } + }); + + await main.invoke('sites:delete', '/sites/wp'); + + assert.equal(deleteRegisteredSite.calls.length, 1); + const [sitePath, options] = deleteRegisteredSite.calls[0]; + assert.equal(sitePath, '/sites/wp'); + // The module decides on the registry it is handed and acts through the + // callbacks: without the store's own `sites` array it would be deciding + // against nothing, and without the callbacks it could not act on its answer. + assert.deepEqual(options.sites, ['/sites/wp']); + assert.equal(typeof options.forget, 'function'); + assert.equal(typeof options.remove, 'function'); + assert.equal(typeof options.onRefused, 'function'); +}); + +// The end of the wire, with the real module and the real `fse.remove` in place, +// on real directories: this is the assertion #145 is about. It fails if the +// handler stops consulting site-registry, however it stops — deleted call, +// renamed export, or a bare `fse.remove(sitePath)` added beside it. +test('sites:delete removes a registered directory and refuses an unregistered one', async (t) => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ipc-wiring-delete-')); + t.after(() => fs.rmSync(root, { recursive: true, force: true })); + const registered = path.join(root, 'registered'); + const unregistered = path.join(root, 'unregistered'); + for (const dir of [registered, unregistered]) { + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync(path.join(dir, 'src', 'index.php'), ' src/trunk-update.js ---------------------------------- + +test('site:status reports the trunk snapshot trunk-update read, not its own guess', async () => { + const readTrunkInfo = spy(async () => ({ trunkOid: 'abc123', trunkDate: '2026-01-01T00:00:00Z' })); + const settings = fakeSettingsStore({ sites: ['/sites/wp'], siteMeta: { '/sites/wp': { initialized: true } } }); + const main = loadMain({ + stubs: { ...silentLogging(), ...settings.stubs, './trunk-update': { readTrunkInfo } } + }); + + const status = await main.invoke('site:status', '/sites/wp'); + + assert.deepEqual(readTrunkInfo.calls, [['/sites/wp']]); + assert.equal(status.trunkOid, 'abc123'); + assert.equal(status.trunkDate, '2026-01-01T00:00:00Z'); + // Written through to siteMeta so the sidebar can render staleness dots from + // siteMeta alone, without per-site git I/O (#94). + assert.equal(settings.values.siteMeta['/sites/wp'].trunkOid, 'abc123'); +}); + // --- git:* -> src/trunk-update.js ---------------------------------------- test('git:worktree-dirty reports what trunk-update found, not its own guess', async () => { @@ -377,10 +478,10 @@ test('git:update-trunk hands the update to trunk-update and streams its log back test('sites:add normalizes line endings before adopting a directory', async () => { // Throwing ends the handler at its first delegation, which is the only thing // under test — and it has to end there. The next line is a store write, and - // reaching it would start `import('electron-store')`, whose own - // `import {app} from 'electron'` loads the real electron package through the - // ESM loader, out of reach of the hook. See the guard test below for why that - // must not happen. + // this test hands the harness no settings store, so reaching it would start + // the real `import('electron-store')`, whose own `import {app} from 'electron'` + // loads the real electron package through the ESM loader, out of reach of the + // hook. See the guard test below for why that must not happen. const ensureAutocrlf = spy(async () => { throw new Error('not a repository'); }); const main = loadMain({ stubs: { ...silentLogging(), './trunk-update': { ensureAutocrlf } } }); @@ -788,10 +889,11 @@ test('playground-web:stop ends the web server tree rather than signalling the ch // Two holes this closes, both of which reached the package on a cold checkout: // stubs are built by merging over the real module, so stubbing src/logging.js // means requiring it and it requires `electron` — that require has to happen -// with the hook already installed. And any handler allowed to reach `getStore()` -// starts `import('electron-store')`, which imports `electron` through the ESM -// loader, where `Module._load` does not apply and no hook can help. Tests stop -// short of the store instead. +// with the hook already installed. And a handler that reaches the real +// `getStore()` starts `import('electron-store')`, which imports `electron` +// through the ESM loader, where `Module._load` does not apply and no hook can +// help. That is what src/settings-store.js is a seam for: a test whose handler +// touches the store stubs it (fakeSettingsStore above), and the rest stop short. test('the harness never loads the real electron package', () => { loadMain({ stubs: silentLogging() }); @@ -813,6 +915,8 @@ const WIRED = new Set([ 'git:create-patch', 'git:save-patch', 'sites:add', + 'sites:delete', + 'site:status', 'npm:install', 'npm:run-script', 'npm:kill', @@ -833,7 +937,6 @@ const NO_DELEGATION = new Map([ ['sites:set-skip-init', 'electron-store write'], ['sites:mark-initialized', 'electron-store write'], ['sites:forget', 'electron-store write'], - ['sites:delete', 'electron-store write plus a directory removal'], ['sites:set-label', 'electron-store write'], ['dialog:choose-dir', 'opens the directory dialog'], ['playground-web:available', 'checks a path on disk'], @@ -857,7 +960,6 @@ const NO_DELEGATION = new Map([ // Channels that do delegate, but whose call sits behind something this harness // cannot stand in for yet. Each one is a known hole, not an oversight. const NOT_REACHABLE = new Map([ - ['site:status', 'reads electron-store before calling readTrunkInfo, and the store is a dynamic ESM import that Module._load cannot replace'], ['wordpress:setup', 'calls ensureAutocrlf and readTrunkInfo only after cloning wordpress-develop over the network'] ]);