[Move] Ticket branch writes onto the bundled Git (#385)

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

@@ -61,7 +61,7 @@ Invariants. Breaking one is a `[fix here]` finding even when the code works on t
 
 **Child processes run on Electron's bundled Node, never the host's.** Spawns go through `process.execPath` with `ELECTRON_RUN_AS_NODE=1` in the environment (see `runNpmWithEngineRetry` and the `playground:start` handler in `src/main.js`, and `buildChildEnv`). A bare `spawn('node')` or `spawn('npm')` assumes a host toolchain that is not there. On Windows child `npm` processes find a `node` at all only because of the `PATH` shim built by `ensureNodeShimDir` — new spawns must inherit that environment rather than build their own. The one exception is the bundled Git, which is not a Node process and gets its own environment from `src/git-binary.cjs` (next invariant).
 
-**Git is the binary the app ships, never the host's.** Since #364 the app bundles Git through `dugite`, unpacked from `app.asar`. `require('dugite')` appears in exactly one file, `src/git-binary.cjs`; every Git spawn resolves the binary with its `resolveGitBinary`, takes its environment from `buildGitEnv`, its options from `SPAWN_OPTIONS` (which already sets `detached` the way section 4 asks), and starts its arguments with `BASE_ARGS`. That env drops every `GIT_*` variable the host had (dugite would otherwise honour `LOCAL_GIT_DIRECTORY` and `GIT_EXEC_PATH` and run a different Git), turns the host's system and global config off, and turns prompting off, so the host's shell or `~/.gitconfig` cannot change what the app does. A `spawn('git')` that relies on `PATH`, a hand-joined path into the dugite tree, a `GitProcess.exec` outside that file, a Git call given `buildChildEnv`'s environment, or one spawned without an explicit `cwd` is a regression. Parse only porcelain-stable output, with the flag that pins it (`--porcelain=v2`, `-z`, an explicit `--format`); parsing human-facing output is a finding however convenient. `src/git-run.cjs` is the only module that spawns the binary and `src/git-read.cjs` the only one that parses its output; a new read belongs there, with a parser test on fixture bytes, not inline at a call site. Since #384 every read outside the write flows runs on the bundled Git and returns the same shapes the `isomorphic-git` calls returned (status rows included), so a facade signature that changes with the engine is a finding; the new-site clone runs on it too (`src/git-clone.cjs`, the one place that reads Git's human-facing progress lines, because there is no porcelain for progress); the remaining writes, with the reads inside them, still run on `isomorphic-git` until #385, and patch and diff generation stays hand-rolled in `src/main.js` until the phase that moves it.
+**Git is the binary the app ships, never the host's.** Since #364 the app bundles Git through `dugite`, unpacked from `app.asar`. `require('dugite')` appears in exactly one file, `src/git-binary.cjs`; every Git spawn resolves the binary with its `resolveGitBinary`, takes its environment from `buildGitEnv`, its options from `SPAWN_OPTIONS` (which already sets `detached` the way section 4 asks), and starts its arguments with `BASE_ARGS`. That env drops every `GIT_*` variable the host had (dugite would otherwise honour `LOCAL_GIT_DIRECTORY` and `GIT_EXEC_PATH` and run a different Git), turns the host's system and global config off, and turns prompting off, so the host's shell or `~/.gitconfig` cannot change what the app does. A `spawn('git')` that relies on `PATH`, a hand-joined path into the dugite tree, a `GitProcess.exec` outside that file, a Git call given `buildChildEnv`'s environment, or one spawned without an explicit `cwd` is a regression. Parse only porcelain-stable output, with the flag that pins it (`--porcelain=v2`, `-z`, an explicit `--format`); parsing human-facing output is a finding however convenient. `src/git-run.cjs` is the only module that spawns the binary and `src/git-read.cjs` the only one that parses its output (`src/git-write.cjs` reading back the single object id `write-tree` and `commit-tree` print is not a parse); a new read belongs there, with a parser test on fixture bytes, not inline at a call site. Since #384 every read outside the write flows runs on the bundled Git and returns the same shapes the `isomorphic-git` calls returned (status rows included), so a facade signature that changes with the engine is a finding; the new-site clone (`src/git-clone.cjs`) and the ticket-branch writes (`src/ticket-branches.js` over the primitives in `src/git-write.cjs`: stage, commit-tree, update-ref, branch, checkout) run on it too, with `src/git-progress.cjs` as the one place that reads Git's human-facing progress lines, because there is no porcelain for progress; a second parser of those lines anywhere else is a finding. The remaining writes, the trunk update and patch apply, with the reads inside them, still run on `isomorphic-git` until #385 moves them, and patch and diff generation stays hand-rolled in `src/main.js` until the phase that moves it.
 
 **`electron-store` is the only persistence layer.** No database, no sidecar JSON. It holds the site registry and per-site metadata and is the single source of truth for "known sites". A second store, a cache file, or state parked in a module-level variable that outlives a handler is architectural drift — flag it.
 

AGENTS.md

@@ -87,7 +87,7 @@ When asked to add an existing pull request to an existing stack, preserve its co
 ## Architecture notes (non-obvious)
 
 - **Child processes run on Electron's own Node, not the system Node.** `npm install`, `npm run <script>`, and the Playground server are spawned via `process.execPath` + `ELECTRON_RUN_AS_NODE=1` — this is the mechanism behind "zero prerequisites." On Windows this requires shimming `node`/`npm`/`npx` into `PATH` so child `npm` processes can find a `node` binary at all.
-- **Git has no host dependency.** The app ships its own Git binary via `dugite` (#364), unpacked from `app.asar` and resolved only through `src/git-binary.cjs`, which also builds the environment it runs with (host system and global config off, prompting off, no `GIT_*` variable inherited from the host and nothing from a Node child). `require('dugite')` lives in that one file; `src/git-run.cjs` is the only place the binary is spawned, and `src/git-read.cjs` holds every read and its parser. Spawning a `git` found on `PATH`, or calling dugite's `GitProcess` from anywhere else, is a regression. As of #384 every read outside the write flows (status, branches, history, blobs, the patch walk) runs on the bundled binary, and since #385's first flow so does the new-site clone (`src/git-clone.cjs`: partial, `--filter=blob:none`, repo config written at clone time); the remaining writes, including the reads inside them, still run on `isomorphic-git` until #385 moves them flow by flow. Only porcelain-stable output (`--porcelain=v2`, `-z`, explicit `--format`) is ever parsed. Patch/diff generation is done by hand in `main.js`, not `git diff`: one status scan against the branch point each ticket recorded (#108), nothing staged, and `/dev/null` naming whichever side of an addition or a deletion does not exist — the app reads its own patches back when a mentor applies one, and that filename is the only thing its parser reads an add or a delete from (#85).
+- **Git has no host dependency.** The app ships its own Git binary via `dugite` (#364), unpacked from `app.asar` and resolved only through `src/git-binary.cjs`, which also builds the environment it runs with (host system and global config off, prompting off, no `GIT_*` variable inherited from the host and nothing from a Node child). `require('dugite')` lives in that one file; `src/git-run.cjs` is the only place the binary is spawned, and `src/git-read.cjs` holds every read and its parser. Spawning a `git` found on `PATH`, or calling dugite's `GitProcess` from anywhere else, is a regression. As of #384 every read outside the write flows (status, branches, history, blobs, the patch walk) runs on the bundled binary, and since #385's first flow so does the new-site clone (`src/git-clone.cjs`: partial, `--filter=blob:none`, repo config written at clone time); Ticket branches (park, start, switch, delete) write through `src/git-write.cjs`, one Git command per primitive, with the checkout's progress read by `src/git-progress.cjs`, the single parser of Git's human-facing lines that the clone shares; the remaining writes, the trunk update and patch apply, still run on `isomorphic-git` until #385 moves them. Only porcelain-stable output (`--porcelain=v2`, `-z`, explicit `--format`) is ever parsed. Patch/diff generation is done by hand in `main.js`, not `git diff`: one status scan against the branch point each ticket recorded (#108), nothing staged, and `/dev/null` naming whichever side of an addition or a deletion does not exist — the app reads its own patches back when a mentor applies one, and that filename is the only thing its parser reads an add or a delete from (#85).
 - **`electron-store` is the only persistence layer** — no separate DB. It holds the site registry and per-site metadata; treat it as the single source of truth for "known sites."
 - Long-running child-process output (installs, scripts, server) is streamed to the renderer via correlated IDs (`installId`/`runId`), not returned synchronously — expect async event handlers, not return values, when tracing that flow.
 

TESTING.md

@@ -55,7 +55,7 @@ Layers 1–3 are `npm test`. Layers 4 and 5 are the two `test:e2e` commands. Tha
 ## Where to put a new test
 
 - **A pure function, a derived string, a decision with branches** → layer 1. If it lives inside `src/renderer/index.jsx` today, move it to a `src/renderer/*.cjs` module first: that component mounts at module scope and nothing in the suite can load it, so a decision made there is untestable by construction.
-- **Anything that asks Git a question** → layer 2, against a real repository. There is a local Git server fixture in `tests/unit/trunk-update-fetch.integration.test.cjs` for cases that need a remote, because `isomorphic-git` has no `file://` transport.
+- **Anything that asks Git a question** → layer 2, against a real repository. A remote is a bare repository on disk reached over `file://`, as in `tests/unit/git-clone.integration.test.cjs`; the loopback Git server in `tests/unit/trunk-update-fetch.integration.test.cjs` remains for the flow still on `isomorphic-git`, which has no `file://` transport.
 - **A new IPC handler** → layer 3, plus the `contextBridge` entry in `src/preload.js`, which layer 5 checks is actually exposed.
 - **A flow that spans the interface, the main process, Git and the store at once** → layer 4.
 - **Something that can only break during packaging** → layer 5.

docs/guide/ticket-branches.md

@@ -42,7 +42,7 @@ A switch is a scan of the working tree followed by a checkout. On a real `wordpr
 - **Swapping files for #61002… 63%**
 - **Ready to work on #61002**
 
-If the file swap itself fails part-way — an editor or an antivirus holding a file open is the usual cause — the app marks the site and refuses every further ticket action with *A previous switch from … to … did not finish. Retry it before making other changes*. The refusal is on the switch too, so the way out is **Unlink**, which is the one action it allows: that puts you back on trunk, and you can then link the ticket you wanted. Your work on the ticket you were leaving is not at risk; it was committed to its branch before any file moved.
+If the file swap itself fails part-way — an editor or an antivirus holding a file open is the usual cause — the app marks the site and refuses every further ticket action with *A previous switch from … to … did not finish. Retry it before making other changes*. Two actions get through: retrying the same switch (**Continue working on** the ticket it was heading for, or linking it again), and **Unlink**, which puts you back on trunk. Both finish the file swap and nothing else; neither saves the half-swapped files as work. Your work on the ticket you were leaving is not at risk; it was committed to its branch before any file moved.
 
 Do not force-quit during a switch. A killed process writes no such marker, so the half-swapped tree is left behind with nothing saying so.
 

src/git-clone.cjs

@@ -21,14 +21,13 @@
  * assumes; `core.symlinks=false` matches what the old engine wrote; and on
  * Windows `core.longpaths=true` because the tree has paths past MAX_PATH.
  *
- * Progress arrives on stderr as the lines Git prints for a human, which is
- * the one place the app reads non-porcelain output: Git has no machine
- * format for progress, the lines have had the same shape for fifteen years,
- * and a line that does not parse is dropped rather than shown.
+ * Progress arrives on stderr as the lines Git prints for a human, parsed by
+ * git-progress.cjs, the one place the app reads non-porcelain output.
  */
 
 const path = require('path');
 const { spawnGit, GitError } = require('./git-run.cjs');
+const { createProgressReader, failureReason } = require('./git-progress.cjs');
 
 /**
  * The branch a new site checks out. `trunk` is the pristine snapshot every
@@ -60,61 +59,6 @@ function cloneArgs({ url, dir, branch = DEFAULT_BRANCH, platform = process.platf
 	];
 }
 
-// `Receiving objects:  42% (1234/5678), 12.00 MiB | 3.00 MiB/s`, with or
-// without the `remote: ` prefix the server-side phases carry.
-const PROGRESS_LINE = /^(?:remote: )?([A-Za-z][A-Za-z ]*?):\s+(\d+)% \((\d+)\/(\d+)\)/;
-
-/**
- * One progress event per phase line, from a chunk that may hold several
- * lines and may end mid-line. Git separates updates to the same phase with
- * `\r` and phases with `\n`; both end a segment here.
- *
- * @param {string} text
- * @return {Array<{phase: string, percent: number, loaded: number, total: number}>}
- */
-function parseProgressLines(text) {
-	const events = [];
-	for (const segment of text.split(/[\r\n]/)) {
-		const match = PROGRESS_LINE.exec(segment);
-		if (!match) continue;
-		events.push({
-			phase: match[1].toLowerCase(),
-			percent: Number(match[2]),
-			loaded: Number(match[3]),
-			total: Number(match[4])
-		});
-	}
-	return events;
-}
-
-/**
- * Feeds chunks in, emits complete progress lines out, and holds back the
- * tail that has not ended yet so a percentage split across two chunks is not
- * reported twice, or wrongly.
- *
- * @param {Function} onEvent
- * @return {{push: Function, flush: Function}}
- */
-function createProgressReader(onEvent) {
-	let pending = '';
-	const emit = (text) => {
-		for (const event of parseProgressLines(text)) onEvent(event);
-	};
-	return {
-		push(chunk) {
-			pending += chunk;
-			const cut = Math.max(pending.lastIndexOf('\r'), pending.lastIndexOf('\n'));
-			if (cut === -1) return;
-			emit(pending.slice(0, cut + 1));
-			pending = pending.slice(cut + 1);
-		},
-		flush() {
-			if (pending) emit(pending);
-			pending = '';
-		}
-	};
-}
-
 /**
  * Clones `url` into `dir`. Resolves when the checkout is complete; rejects
  * with a GitError carrying Git's stderr when it is not, in which case `dir`
@@ -169,15 +113,10 @@ function cloneSite({ url, dir, branch = DEFAULT_BRANCH, onProgress = null, onChi
 				return;
 			}
 			const text = stderr.join('');
-			// Git's reason is its `fatal:` (or `error:`) line, which is not always
-			// the last one: "Please make sure you have the correct access rights
-			// and the repository exists." follows it. Fall back to the last line
-			// that is not a progress update.
-			const lines = text.split(/[\r\n]/).filter((line) => line.trim() && !PROGRESS_LINE.test(line));
-			const reason = lines.filter((line) => /^(fatal|error):/.test(line)).pop() || lines.pop() || (signal ? `killed by ${signal}` : 'no output');
+			const reason = failureReason(text, signal);
 			reject(new GitError(`git clone failed (${status === null ? signal : status}): ${reason}`, { code: status, signal, stderr: text, args, cwd }));
 		});
 	});
 }
 
-module.exports = { DEFAULT_BRANCH, cloneArgs, parseProgressLines, createProgressReader, cloneSite };
+module.exports = { DEFAULT_BRANCH, cloneArgs, cloneSite };

src/git-progress.cjs

@@ -0,0 +1,83 @@
+'use strict';
+
+/**
+ * Git's progress lines, and its reason for failing, read off stderr. The one
+ * place the app parses non-porcelain output: Git has no machine format for
+ * progress, the lines have had the same shape for fifteen years, and a line
+ * that does not parse is dropped rather than shown. Shared by the clone
+ * (git-clone.cjs) and the checkout (git-write.cjs), which print the same
+ * `Phase:  42% (1234/5678)` shape; the environment pins `LC_ALL=C`
+ * (git-binary.cjs) so the words are the ones these patterns expect.
+ */
+
+// `Receiving objects:  42% (1234/5678), 12.00 MiB | 3.00 MiB/s`, with or
+// without the `remote: ` prefix the server-side phases carry.
+const PROGRESS_LINE = /^(?:remote: )?([A-Za-z][A-Za-z ]*?):\s+(\d+)% \((\d+)\/(\d+)\)/;
+
+/**
+ * One progress event per phase line, from a chunk that may hold several
+ * lines and may end mid-line. Git separates updates to the same phase with
+ * `\r` and phases with `\n`; both end a segment here.
+ *
+ * @param {string} text
+ * @return {Array<{phase: string, percent: number, loaded: number, total: number}>}
+ */
+function parseProgressLines(text) {
+	const events = [];
+	for (const segment of text.split(/[\r\n]/)) {
+		const match = PROGRESS_LINE.exec(segment);
+		if (!match) continue;
+		events.push({
+			phase: match[1].toLowerCase(),
+			percent: Number(match[2]),
+			loaded: Number(match[3]),
+			total: Number(match[4])
+		});
+	}
+	return events;
+}
+
+/**
+ * Feeds chunks in, emits complete progress lines out, and holds back the
+ * tail that has not ended yet so a percentage split across two chunks is not
+ * reported twice, or wrongly.
+ *
+ * @param {Function} onEvent
+ * @return {{push: Function, flush: Function}}
+ */
+function createProgressReader(onEvent) {
+	let pending = '';
+	const emit = (text) => {
+		for (const event of parseProgressLines(text)) onEvent(event);
+	};
+	return {
+		push(chunk) {
+			pending += chunk;
+			const cut = Math.max(pending.lastIndexOf('\r'), pending.lastIndexOf('\n'));
+			if (cut === -1) return;
+			emit(pending.slice(0, cut + 1));
+			pending = pending.slice(cut + 1);
+		},
+		flush() {
+			if (pending) emit(pending);
+			pending = '';
+		}
+	};
+}
+
+/**
+ * Git's reason for a failed command, for the error message. Its `fatal:` (or
+ * `error:`) line is not always the last one: "Please make sure you have the
+ * correct access rights and the repository exists." follows it. Falls back to
+ * the last line that is not a progress update, then to the signal.
+ *
+ * @param {string}  stderr
+ * @param {?string} [signal]
+ * @return {string}
+ */
+function failureReason(stderr, signal = null) {
+	const lines = String(stderr || '').split(/[\r\n]/).filter((line) => line.trim() && !PROGRESS_LINE.test(line));
+	return lines.filter((line) => /^(fatal|error):/.test(line)).pop() || lines.pop() || (signal ? `killed by ${signal}` : 'no output');
+}
+
+module.exports = { PROGRESS_LINE, parseProgressLines, createProgressReader, failureReason };