Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/instructions/code-review.instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. During the migration the flows still run on `isomorphic-git`, and patch and diff generation stays hand-rolled in `src/main.js` (stage untracked files, diff working tree against `origin/trunk`) 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; 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 writes and the clone, 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.

**`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.

Expand Down
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. Spawning a `git` found on `PATH`, or calling dugite's `GitProcess` from anywhere else, is a regression. While the #364 migration is in progress the flows still run on `isomorphic-git`; phase by phase they move to the bundled binary, and 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; the writes and the clone, 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).
- **`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.

Expand Down
1 change: 1 addition & 0 deletions TESTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ To run one file: `node --test tests/unit/azure-sign.test.cjs`. To run one journe
tests/
unit/ layers 1-3 — npm test — under three seconds
fixtures/ package.json trees the integration tests copy; not tests themselves
helpers/ the bundled-Git driver the Git suites share; not a test either
e2e/
journeys/ layer 4 — npm run test:e2e
packaged/ layer 5 — npm run test:e2e:packaged, and it needs a build first
Expand Down
Loading
Loading