Prove the delete handler still goes through the registry gate - #154
Merged
Conversation
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) <noreply@anthropic.com>
juanmaguitar
force-pushed
the
juanmaguitar/issue-145-delete-gate
branch
from
August 7, 2026 05:37
69c8313 to
91cc37d
Compare
juanmaguitar
changed the base branch from
juanmaguitar/issue-146-child-process-wiring
to
trunk
August 7, 2026 05:37
juanmaguitar
added a commit
that referenced
this pull request
Aug 7, 2026
Closes #150. Each site gains **Open in editor** and **Show in Finder** / **Show in Explorer** / **Show in file manager**, next to the path and its copy button — which stays exactly as it was, since it is the floor under everything else. First use opens a picker of the editors found on the machine, with **Choose application…** always beside them rather than only as a fallback. After that the button names the editor ("Open in Sublime Text") and launches straight through, with **Change editor** next to it. A launch that fails — nothing chosen yet, or an editor that has since moved — reopens the picker and says why. Nothing here can end at a disabled button or a bare "unavailable". ## The constraint this is built around The earlier editor dropdown (#24) was removed in #26 because its detection ran `which`/`where`, and **a packaged Electron app does not inherit the shell's `PATH`** — a correctly installed VS Code read as missing in the shipped build while working fine in `npm start`. So `src/editor-launch.js` never consults `PATH`, at either end: - **Detection** stats absolute, per-platform install locations (`/Applications/*.app`, `%LOCALAPPDATA%\Programs\…`, JetBrains Toolbox, the common Linux packages). No `which`, no `where`, no spawning anything to find out what is installed. - **Launching** refuses a relative command, because `spawn('code', …)` without a shell would resolve it through that same missing environment. Both are pinned by tests, including one that runs detection with an empty `PATH` and gets the same answer. The table is a convenience, not the contract: `editor:choose` opens a file dialog for anything it misses and validates the result to the same standard, so an editor this app has never heard of works exactly as well. ## Guards `editor:open` and `dir:show` both refuse a folder the app has no record of — the same `isRegisteredSite` boundary `sites:delete` uses — so "open this site" cannot become "open this arbitrary directory". `dir:show` goes through a new `revealRegisteredSite` in `site-registry.js` rather than calling `shell.openPath` itself. Spawns are `shell: false`, `detached`, `stdio: 'ignore'`, `windowsHide: true`. Refusals are logged, never dropped. Nothing added here goes near `shell.openExternal`. ## Two moves that are not the feature - **`src/safe-log.js`** — `external-url.js` and `site-registry.js` each carried a copy of the escape-and-truncate step for logging a refused value, and site-registry's copy said in a comment that the third caller should move it somewhere shared. This is that third caller. Behaviour identical; both existing suites pass untouched, which is the proof. - **`src/settings-store.js`** — this branch originally extracted `getStore()` out of `main.js` for the same reason #154 did, and the two collided on rebase. Trunk's version won: the file here is trunk's, plus `preferences` in its defaults. The wiring tests likewise use trunk's `fakeSettingsStore` rather than the near-identical helper this branch had added. The one thing that survived from this side is the consequence — the four new channels read the store before reaching their guard module, and behind that seam they are wiring tests rather than entries on the known-holes list. Adding `preferences` to the store defaults is additive: `sites` and `siteMeta` keep their keys and shapes, so existing registries need no migration. ## Review Ran per AGENTS.md, with the judgement pass in a fresh context. `npm run lint` clean, 213 tests pass. **4 findings · 4 fixed · 0 deferred** - 🟡 **Architecture** — a launch was reported successful the moment `spawn` returned. `spawn` returns a handle before the OS has been asked to execute anything, so the failure that actually happens (EACCES on a non-executable file, a Windows policy's EPERM, a path deleted since the check) arrived afterwards on the `'error'` event with nothing listening: "opening your editor", then a button that did nothing, with the uncaught emit going only to the log file. The answer now comes from the child — `'error'` either way, plus `open`'s exit code on macOS where the child is `/usr/bin/open` and exits in milliseconds; `'spawn'` elsewhere, since waiting for exit would mean waiting for the contributor to close their editor. - 🟡 **Tests** — the test for that path injected a spawn that *throws*, the one failure real `spawn` does not produce for an unexecutable target. The fake now returns a handle and emits on a later turn; the two new cases fail on the previous code. - 🟡 **Performance** — detection ran per site row, and every site is mounted at once, so with N sites that was N × ~14 synchronous stats on the main process at load. The choice is now held once for the window; load asks `editor:get` (a store read, no filesystem) and detection waits for the picker. - 🔵 **Architecture** — the same per-row state meant choosing an editor in one site left every other row's button reading "Open in editor". Fixed by the same hoist. `chosenMissing` went with it: nothing consumed it, and an editor that has moved is reported by trying to open it. Clean on security and cross-platform. The Windows and Linux branches are exercised from macOS by injecting `platform`, `env` and the filesystem, per the house pattern in `test/win-spawn-patch.test.cjs`. ## Testing Driven on macOS against a real site: detection found the three editors installed, the picker remembered the choice, the second click went straight through without the picker, the other site's row agreed on the label, and both refusals (a folder the registry does not hold, a file that is not an application) came back as refusals. **Not verified locally: the packaged build** — which is the only place the #24 failure ever showed up. The Buildkite artifact for this branch is the check that matters, on Windows and macOS both. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #145. Stacked on #153 — review that one first; this PR targets its branch and its diff will shrink once #153 merges.
#138 added the gate that refuses to delete a path that is not a registered site, and the gate module is well tested. Nothing tested that the delete handler uses it: replacing the handler body with a bare recursive removal kept the whole suite green. The channel was filed under
NO_DELEGATIONwith a reason that was not true, because the handler reached the settings store through a dynamic ESM import the wiring harness cannot intercept — the same hole recorded forsite:status.That import now lives in
src/settings-store.js, a module the harness can stand in for.getStore()is unchanged for every caller.sites:deletemoves intoWIREDwith two tests, andsite:statusleavesNOT_REACHABLE, since its recorded reason would otherwise have become false.Verified by temporarily replacing the handler body with a bare
fse.remove: both new tests fail, and the suite was green before the change. The second test runs against real temporary directories with the realfse.remove, so it proves an unregistered path reaches neither the removal nor the store.Review
.github/instructions/code-review.instructions.mdrun against the branch: 2 findings, both low — 1 fixed here (the fake store now returns a structured clone onget, so a test could not mutate shared state through it), 1 deferred: the handler swallows the error from its best-effort removal, which predates this change and is not what this PR is about.Test written before the fix.
npm testandnpm run test:electronboth 188/188 on the stack, lint clean.