Let a site's folder be opened while it is still being cloned

src/editor-launch.js

@@ -29,7 +29,7 @@
 
 const path = require('path');
 const { describeRefused } = require('./safe-log');
-const { isRegisteredSite } = require('./site-registry');
+const { isActionableSite } = require('./site-registry');
 
 // Path semantics follow the platform being asked about, not the platform the
 // test happens to run on: `path.isAbsolute('C:\\x')` is false under POSIX, and a
@@ -288,7 +288,8 @@ const REFUSAL_REASONS = {
 // The `editor:open` handler's body.
 //
 // Two gates, both of which have to pass before anything is spawned. The folder
-// must be one the app has on record — `isRegisteredSite` from site-registry.js,
+// must be one the app has on record, or one it is creating right now —
+// `isActionableSite` from site-registry.js,
 // the same boundary `sites:delete` uses — so "open this site" cannot become
 // "open this arbitrary directory". And the application must be absolute and of
 // the platform's shape, so an editor path that has been tampered with, or an
@@ -307,12 +308,13 @@ const REFUSAL_REASONS = {
 // the app runs to collect their output; here the window is the point.
 async function openSiteInEditor(sitePath, editorPath, {
 	sites,
+	pending,
 	platform,
 	statPath,
 	spawn,
 	onRefused
 } = {}) {
-	if (!isRegisteredSite(sitePath, sites)) {
+	if (!isActionableSite(sitePath, { sites, pending })) {
 		if (typeof onRefused === 'function') {
 			onRefused(REFUSAL_REASONS.UNREGISTERED_SITE, describeRefused(sitePath));
 		}

src/main.js

@@ -38,6 +38,7 @@ const { buildPullRequestEntries } = require('./pr-files.cjs');
 const { openAndScrape, fetchAttachment } = require('./trac-view');
 const { openExternalUrl, ALLOWED_URL_SCHEMES } = require('./external-url');
 const { deleteRegisteredSite, revealRegisteredSite, clearRegisteredSiteLog } = require('./site-registry');
+const { createSetupTracker } = require('./setup-tracker');
 const { planInitialRead, planTailRead } = require('./log-tail');
 const { getStore } = require('./settings-store');
 const { parseTicketRef } = require('./renderer/trac-ticket.cjs');
@@ -158,6 +159,11 @@ const cancelledChildren = new WeakSet();
 const runIdByDirectory = {};
 /** @type {Record<string, { child: import('child_process').ChildProcess, url?: string }>} */
 const playgroundServers = {};
+// The sites being created right now — liveness, not truth, which is why it is
+// here beside the other per-site maps and not in the store. See
+// setup-tracker.js: a directory exists minutes before its clone finishes, and
+// the guards need to know that without anything half-finished being persisted.
+const setupTracker = createSetupTracker();
 /** @type {Record<string, { filePath: string, fileWatcher?: import('fs').FSWatcher, dirWatcher?: import('fs').FSWatcher, lastSize: number }>} */
 const wpDebugWatchers = {};
 /** @type {Record<string, { server: import('smtp-server').SMTPServer, port: number }>} */
@@ -1088,8 +1094,15 @@ ipcMain.handle('wordpress:setup', async (event, destDir, options = {}) => {
 	const uniqueName = findAvailableDirName(destDir, sanitizedName);
 	const siteDir = path.join(destDir, uniqueName);
 	await fse.ensureDir(siteDir);
-	event.sender.send('download:status', { phase: 'cloning', target: siteDir });
-	try {
+
+	// Tracked from here, where the directory starts existing, to the `done`
+	// below, where the store takes over. In between, `siteDir` is a real folder
+	// the app made and the registry has never heard of — so without this the
+	// guards refuse to open it for the whole clone (#180), and `sites:delete`
+	// would happily remove it if they did not. setup-tracker.js has the why;
+	// `track` releases the entry however this ends.
+	return setupTracker.track(siteDir, async () => {
+		event.sender.send('download:status', { phase: 'cloning', target: siteDir });
 		await git.clone({
 			http,
 			fs,
@@ -1104,37 +1117,34 @@ ipcMain.handle('wordpress:setup', async (event, destDir, options = {}) => {
 				event.sender.send('download:progress', { target: siteDir, message: msg });
 			}
 		});
-	} catch (e) {
-		// Fallback/error
-		throw e;
-	}
-	await ensureAutocrlf(siteDir);
+		await ensureAutocrlf(siteDir);
 
-	const s = await getStore();
-	const sites = s.get('sites');
-	if (!sites.includes(siteDir)) {
-		sites.push(siteDir);
-		s.set('sites', sites);
-		const meta = s.get('siteMeta');
-		const siteLabel = typeof options.siteLabel === 'string' && options.siteLabel.trim().length
-			? options.siteLabel.trim()
-			: uniqueName;
-		const existingMeta = meta[siteDir] || {};
-		meta[siteDir] = {
-			...existingMeta,
-			initialized: false,
-			createdAt: existingMeta.createdAt || new Date().toISOString(),
-			label: existingMeta.label || siteLabel
-		};
-		try {
-			const { trunkOid, trunkDate } = await readTrunkInfo(siteDir);
-			meta[siteDir].trunkOid = trunkOid;
-			meta[siteDir].trunkDate = trunkDate;
-		} catch {}
-		s.set('siteMeta', meta);
-	}
-	event.sender.send('download:status', { phase: 'done', target: siteDir, sitePath: siteDir });
-	return siteDir;
+		const s = await getStore();
+		const sites = s.get('sites');
+		if (!sites.includes(siteDir)) {
+			sites.push(siteDir);
+			s.set('sites', sites);
+			const meta = s.get('siteMeta');
+			const siteLabel = typeof options.siteLabel === 'string' && options.siteLabel.trim().length
+				? options.siteLabel.trim()
+				: uniqueName;
+			const existingMeta = meta[siteDir] || {};
+			meta[siteDir] = {
+				...existingMeta,
+				initialized: false,
+				createdAt: existingMeta.createdAt || new Date().toISOString(),
+				label: existingMeta.label || siteLabel
+			};
+			try {
+				const { trunkOid, trunkDate } = await readTrunkInfo(siteDir);
+				meta[siteDir].trunkOid = trunkOid;
+				meta[siteDir].trunkDate = trunkDate;
+			} catch {}
+			s.set('siteMeta', meta);
+		}
+		event.sender.send('download:status', { phase: 'done', target: siteDir, sitePath: siteDir });
+		return siteDir;
+	});
 });
 
 ipcMain.handle('sites:mark-initialized', async (_e, sitePath) => {
@@ -1162,6 +1172,9 @@ ipcMain.handle('sites:delete', async (_e, sitePath) => {
 	const s = await getStore();
 	return deleteRegisteredSite(sitePath, {
 		sites: s.get('sites'),
+		// A site whose clone is still running is refused outright, registered or
+		// not: `remove` would be deleting a tree isomorphic-git is writing into.
+		pending: setupTracker.paths(),
 		forget: () => {
 			s.set('sites', s.get('sites').filter((p) => p !== sitePath));
 			const meta = s.get('siteMeta');
@@ -1171,7 +1184,7 @@ ipcMain.handle('sites:delete', async (_e, sitePath) => {
 		// Best-effort, as before: a site whose registry entry is gone should not be
 		// stuck undeletable because its directory is missing or locked.
 		remove: async (p) => { try { await fse.remove(p); } catch {} },
-		onRefused: (description) => logEvent('sites', `refused to delete ${description} — not a registered site`)
+		onRefused: (description) => logEvent('sites', `refused to delete ${description} — not a registered site, or still being created`)
 	});
 });
 
@@ -1312,6 +1325,7 @@ ipcMain.handle('editor:open', async (_e, sitePath, editorPath) => {
 	return openSiteInEditor(sitePath, target, {
 		...editorLaunchDeps(),
 		sites: s.get('sites'),
+		pending: setupTracker.paths(),
 		spawn,
 		onRefused: (reason, description) => logEvent('editor', `refused to open ${description} — ${reason}`)
 	});
@@ -1377,6 +1391,7 @@ ipcMain.handle('dir:show', async (_e, sitePath) => {
 	const s = await getStore();
 	return revealRegisteredSite(sitePath, {
 		sites: s.get('sites'),
+		pending: setupTracker.paths(),
 		reveal: (target) => shell.openPath(target),
 		onRefused: (description) => logEvent('sites', `refused to reveal ${description} — not a registered site`)
 	});

src/renderer/index.jsx

@@ -3125,7 +3125,14 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit
               // "Already up to date." in the terminal.
               { title: 'Update to latest trunk', onClick: startTrunkUpdate },
               { title:'Forget this site', onClick:()=>confirmAnd('Remove this site from the list?', ()=>onForget(sitePath)) },
-              { title:'Delete this site', onClick:()=>confirmAnd('Delete this site from disk? This cannot be undone.', ()=>onDelete(sitePath)) }
+              // Not while the clone is running: deleting the site would be
+              // removing a directory the app is still writing into. The main
+              // process refuses it either way (see site-registry.js) — that is
+              // the backstop, and not offering a control that cannot work is
+              // the actual answer.
+              ...(isPending ? [] : [
+                { title:'Delete this site', onClick:()=>confirmAnd('Delete this site from disk? This cannot be undone.', ()=>onDelete(sitePath)) }
+              ])
             ]}
           />
         </div>

src/setup-tracker.js

@@ -0,0 +1,97 @@
+// Which sites this process is creating right now.
+//
+// Creating a site clones `wordpress-develop`, which takes minutes. The window
+// shows the site immediately, and it should: the directory exists from the
+// first moment, and a contributor watching a clone has every reason to open the
+// folder and look. But `dir:show` and `editor:open` are gated on the `sites`
+// registry, and the registry does not learn about the site until the clone
+// finishes — so for the whole clone the app refused to open a folder it had
+// created itself (#180).
+//
+// The fix is not to register it early. `sites` is persisted, and a half-cloned
+// directory written into it survives the crash or the quit that the
+// unregister-on-failure path cannot catch — a phantom site for a directory that
+// was never finished, which is the thing AGENTS.md's architecture rules single
+// out. It would also widen the allow-list for the recursive delete in
+// `sites:delete` to include a tree isomorphic-git is writing into.
+//
+// So this is the other half of the boundary, and the distinction it draws is
+// **liveness against truth**. The store answers "which sites exist"; this
+// answers "what is this process doing right now". They differ in lifetime, and
+// that is the point: an entry here cannot outlive the process, so a restart
+// mid-clone lands back on exactly today's behaviour instead of a new broken
+// state that has to be reconciled.
+//
+// It sits beside six existing per-site maps in main.js that hold liveness the
+// same way — `playgroundServers`, `runningInstalls`, `runningScripts`,
+// `runIdByDirectory`, `wpDebugWatchers`, `smtpServers` — and its entries are
+// the shortest-lived of them all: one handler call, released in a `finally`.
+//
+// Pure, so both halves are testable without an Electron process.
+//
+// On what the keys are, precisely, because the next widening of
+// `isActionableSite` will be argued from it: a key is
+// `path.join(destDir, uniqueName)`. The leaf is main's — `findAvailableDirName`
+// picks a name that does not exist yet, from a string with path separators
+// already stripped — but `destDir` is the renderer's, straight off the
+// `wordpress:setup` invoke. So this is not "a path main computed from nothing";
+// it is "a directory main is about to create and clone into, under a parent the
+// contributor chose in a file dialog".
+//
+// That is enough for what it is used for. Being here says the app is writing
+// into that directory right now, which is a fact about this process regardless
+// of who named it, and the same call would register the same path in the store
+// minutes later. It would not be enough to justify widening anything
+// destructive, which is why `deleteRegisteredSite` refuses these outright
+// rather than treating them as a second registry.
+
+'use strict';
+
+function createSetupTracker() {
+	const inFlight = new Set();
+
+	// True when this call is the one that claimed the path. False for a path
+	// already being set up — two windows can resolve the same directory name
+	// before either creates it, and two clones interleaving in one tree is worse
+	// than the second one refusing.
+	function begin(sitePath) {
+		if (typeof sitePath !== 'string' || sitePath === '') return false;
+		if (inFlight.has(sitePath)) return false;
+		inFlight.add(sitePath);
+		return true;
+	}
+
+	function end(sitePath) {
+		return inFlight.delete(sitePath);
+	}
+
+	function has(sitePath) {
+		return typeof sitePath === 'string' && sitePath !== '' && inFlight.has(sitePath);
+	}
+
+	// A copy. The array is handed to the guards as their `pending` list, and a
+	// guard that could be widened by whoever it is guarding is not a guard.
+	function paths() {
+		return [...inFlight];
+	}
+
+	// Runs `work` with the path tracked, and releases it however that ends. The
+	// release is the whole reason this is a function rather than two calls: a
+	// clone that throws is exactly when a forgotten `end` would leave the site
+	// permanently undeletable, and exactly when the caller is thinking about
+	// something else.
+	async function track(sitePath, work) {
+		if (!begin(sitePath)) {
+			throw new Error(`A setup is already running for ${sitePath}`);
+		}
+		try {
+			return await work();
+		} finally {
+			end(sitePath);
+		}
+	}
+
+	return { begin, end, has, paths, track };
+}
+
+module.exports = { createSetupTracker };

src/site-registry.js

@@ -13,6 +13,13 @@
 // one the app should carry out. This is the same shape as external-url.js — a
 // pure check, a safe log formatter, and a wrapper whose effects are injected so
 // both branches can be tested without an Electron process.
+//
+// There is a second, shorter-lived record: the sites this process is creating
+// right now (setup-tracker.js). A site's directory exists from the moment the
+// app makes it, minutes before the clone finishes and the store hears about it,
+// and the two verbs here want opposite answers about it — open it, yes; delete
+// it, absolutely not. So `pending` widens `revealRegisteredSite` and is an
+// outright refusal in `deleteRegisteredSite`. The asymmetry is the point.
 
 const { describeRefused } = require('./safe-log');
 
@@ -36,6 +43,21 @@ function isRegisteredSite(sitePath, sites) {
 	return sites.includes(sitePath);
 }
 
+// True for a path this app is responsible for right now: one it has on record,
+// or one it is creating this very moment (see setup-tracker.js for why the
+// second kind is deliberately not in the store).
+//
+// `pending` goes through the same exact-match predicate as `sites` rather than
+// any looser comparison. It is a list of directories the app is writing into,
+// so a prefix match would turn "this site is being cloned" into a lever on
+// everything beneath it.
+//
+// This widens what may be *opened*. It must never be used to widen what may be
+// removed — see `deleteRegisteredSite`, which refuses a pending path outright.
+function isActionableSite(sitePath, { sites, pending } = {}) {
+	return isRegisteredSite(sitePath, sites) || isRegisteredSite(sitePath, pending);
+}
+
 // A refused path is attacker-influenced by hypothesis, and it is about to be
 // written into the file contributors attach to bug reports, so it has to stay on
 // one line and it has to be bounded. safe-log.js is where both live, and why.
@@ -48,7 +70,18 @@ function describeRefusedSite(sitePath) {
 // the store, `remove` is the real `fse.remove` in the app, and both are recording
 // stubs in the tests. A path that is not registered performs neither: no store
 // mutation and no removal, just a logged refusal.
-async function deleteRegisteredSite(sitePath, { sites, forget, remove, onRefused } = {}) {
+async function deleteRegisteredSite(sitePath, { sites, pending, forget, remove, onRefused } = {}) {
+	// Checked before the registry, and separately from it. A site whose clone is
+	// still running is the one case where `remove` would delete a tree another
+	// part of this process is actively writing into, so it is refused whether or
+	// not it is registered. Until this existed the refusal came for free from the
+	// path not being in `sites` yet; making the folder openable mid-clone is what
+	// took that accident away.
+	if (isRegisteredSite(sitePath, pending)) {
+		if (typeof onRefused === 'function') onRefused(describeRefusedSite(sitePath));
+		return false;
+	}
+
 	if (!isRegisteredSite(sitePath, sites)) {
 		if (typeof onRefused === 'function') onRefused(describeRefusedSite(sitePath));
 		return false;
@@ -60,15 +93,17 @@ async function deleteRegisteredSite(sitePath, { sites, forget, remove, onRefused
 }
 
 // The `dir:show` handler's body. `shell.openPath` hands a local path to whatever
-// the OS has registered for it, so "show this site in the file manager" gets the
-// same boundary as "delete this site": only a path the app has on record. The
-// reveal itself is injected, like `remove` above.
+// the OS has registered for it, so "show this site in the file manager" is
+// bounded the same way "delete this site" is — except that a site still being
+// created counts here and does not there, since opening a folder mid-clone is
+// what a contributor watching one wants and removing it is not. The reveal
+// itself is injected, like `remove` above.
 //
 // `reveal` resolves to electron's own convention — the empty string on success,
 // an error message otherwise — and that is passed through rather than reduced to
 // a boolean, so the renderer can say what went wrong.
-async function revealRegisteredSite(sitePath, { sites, reveal, onRefused } = {}) {
-	if (!isRegisteredSite(sitePath, sites)) {
+async function revealRegisteredSite(sitePath, { sites, pending, reveal, onRefused } = {}) {
+	if (!isActionableSite(sitePath, { sites, pending })) {
 		if (typeof onRefused === 'function') onRefused(describeRefusedSite(sitePath));
 		return { ok: false, reason: REVEAL_REASONS.UNREGISTERED_SITE };
 	}
@@ -98,6 +133,7 @@ async function clearRegisteredSiteLog(sitePath, { sites, truncate, onRefused } =
 module.exports = {
 	REVEAL_REASONS,
 	isRegisteredSite,
+	isActionableSite,
 	describeRefusedSite,
 	revealRegisteredSite,
 	deleteRegisteredSite,