From 5f9c2a0c146f99a7c055e591b9c174e1ce77b7f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Sun, 9 Aug 2026 23:35:13 +0300 Subject: [PATCH 01/16] feat(deps): read a lockfile as the dependency set it describes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1, commit 1 of specs/2026-08-09-developer-workflow. Four ecosystems, one shape out: package-lock.json (v1's nested tree and v2/v3's flat `packages` map), pnpm-lock.yaml, yarn.lock v1 and go.sum. No package manager is consulted and no registry is contacted — every fact reported comes out of the file in front of the reader. The interesting field is `direct`. npm v2/v3 and pnpm record which packages the project actually ASKED for (the root entry, the importers block); yarn v1 and go.sum do not, so those report knowsDirect: false and claim nothing rather than guessing. npm v1 is in that group too: its top level is the HOISTED tree, so depth 0 does not mean direct. Matching a direct name against every install path was wrong, and this repo's own lockfile caught it: a package installed TWICE — once at the root and once nested under a dependency wanting another version — was direct at both paths, reporting 33 direct against a manifest declaring 30. @emnapi/core, @emnapi/runtime and globals are each installed twice here. Only `node_modules/` counts now, and the nested copy is the transitive dependency it actually is. A lockfile is untrusted input (rule 6): the parse is capped at 50k entries, malformed content returns null rather than throwing, and pnpm's YAML is read with the same maxAliasCount guard structuralDiff uses so an anchor bomb cannot expand. Verified against this repo's own package-lock.json: 783 packages, 30 direct — exactly its 11 production plus 19 dev dependencies. Co-Authored-By: Claude Opus 5 --- specs/2026-08-09-developer-workflow/plan.md | 258 ++++++++++++++++++++ src/renderer/src/utils/lockfile/go.js | 17 ++ src/renderer/src/utils/lockfile/npm.js | 84 +++++++ src/renderer/src/utils/lockfile/parse.js | 61 +++++ src/renderer/src/utils/lockfile/pnpm.js | 58 +++++ src/renderer/src/utils/lockfile/yarn.js | 38 +++ tests/renderer/utils/lockfile/parse.test.js | Bin 0 -> 7414 bytes 7 files changed, 516 insertions(+) create mode 100644 specs/2026-08-09-developer-workflow/plan.md create mode 100644 src/renderer/src/utils/lockfile/go.js create mode 100644 src/renderer/src/utils/lockfile/npm.js create mode 100644 src/renderer/src/utils/lockfile/parse.js create mode 100644 src/renderer/src/utils/lockfile/pnpm.js create mode 100644 src/renderer/src/utils/lockfile/yarn.js create mode 100644 tests/renderer/utils/lockfile/parse.test.js diff --git a/specs/2026-08-09-developer-workflow/plan.md b/specs/2026-08-09-developer-workflow/plan.md new file mode 100644 index 0000000..13efded --- /dev/null +++ b/specs/2026-08-09-developer-workflow/plan.md @@ -0,0 +1,258 @@ +# Developer workflow — dependencies, revisions, and finishing the merge + +| | | +|---|---| +| **Status** | in-progress | +| **Progress** | 0 / 13 steps | +| **Branch** | `feat/developer-workflow` | +| **Started** | 2026-08-09 | +| **Finished** | — | +| **Bugs found and fixed this iteration** | 0 | +| **Token baseline** | 2026-08-09T20:27:28Z | +| **Claude tokens used** | — | + +Three findings from the developer-experience investigation, in one spec because +they share a thesis and a seam. Each is its own phase, landed in its own commits, +and each is independently shippable — if the spec stops after phase 1, phase 1 is +still a complete feature. + +## Problem + +DiffBro's differentiator is **compare meaning, not lines**: JSON/YAML/XML as data +(`structuralDiff.js`), workbooks as grids with a materiality tolerance, Mermaid as +one picture. That thesis has never been pointed at the three artifacts a developer +actually spends their day on. + +**1 · A lockfile diff is unreadable.** `package-lock.json` in this very repo has +784 package entries. Bumping one dependency rewrites thousands of lines, and every +tool on the market renders that as text and gives up. Nothing tells the reader the +only thing they want to know: *which packages actually changed, which of them I +asked for, and which came along*. + +**2 · The unit of work is wrong.** `diffbro compare` takes file PATHS only +(`cli.js:156`). A developer's unit of work is a change — a commit, a branch, +staged vs working tree — so using DiffBro on your own work means manufacturing two +files first. The tool cannot see the repo it is sitting in. + +**3 · The app advertises a job it cannot finish.** `gitTool.js` registers DiffBro +as git's `difftool` **and** `mergetool`, and its own settings copy admits the +consequence: *"Diff Bro doesn't write the merged file, so git still asks you +whether the merge worked."* `registerArgs` sets `trustExitCode=false` to keep that +honest. So `git mergetool` opens DiffBro, you read the conflict, and then resolve +it somewhere else — a context switch at the worst possible moment, caused by the +app having volunteered for the job. + +## Solution + +Three phases, in ascending order of what they cost and what they risk. + +**Phase 1 — dependencies as dependencies.** A lockfile parser per ecosystem +(`utils/lockfile/`) normalising to one shape, a comparison that classifies each +change (added · removed · bumped · downgraded, direct vs transitive, and the +semver step), and a viewer. It is a **semantic kind** beside tree/grid/diagram — +`semanticKind()` gains `'deps'` — because a lockfile IS json/yaml, so it must not +fight `resolveAdapter`. Zero new dependencies: `yaml` is already in the tree, and +yarn v1/`go.sum` are line formats. + +**Phase 2 — git-native comparison, read-only.** A fenced main-process +`gitRepo.js` that resolves a revision and reads a blob (`git show :`), +plus the CLI grammar for it. No writes, no new dependency, no socket. + +**Phase 3 — finishing the mergetool.** A three-way conflict model, a resolution +UI, and — the line this crosses — main writing the `$MERGED` path git handed it, +then exiting 0 under `trustExitCode=true`. + +| option | why not | +|---|---| +| a lockfile adapter in `resolveAdapter` | a lockfile IS JSON; `textAdapter`/structure already claim it. Making it a semantic KIND keeps the text and structure views one toggle away, which is what a reader wants when the summary is not enough | +| parse lockfiles with an ecosystem library (`@npmcli/arborist`, …) | rule 2. Arborist alone drags a tree far larger than the eleven production dependencies this app has, and it wants the network. The formats are stable and documented; reading them is a parser, not a package manager | +| TOML lockfiles (`Cargo.lock`, `poetry.lock`) in phase 1 | TOML needs a parser this repo does not have, and a hand-rolled one is a new class of bug for two ecosystems. Deferred with the reason recorded, not silently skipped | +| git via `isomorphic-git` | a production dependency reimplementing what is already installed, to avoid a subprocess the app already spawns (`gitTool.js` `execFile`) | +| let the renderer name the git revision and path | rule 3/7. Main owns the repo root, the argv and the fence; the renderer names a revision STRING and never a command | +| three-way merge as a full editor | out. The merged pane accepts a CHOICE per conflict (take left / right / both / neither), not free typing — that is what `git mergetool` needs and it keeps DiffBro a viewer with one deliberate write | + +## Scope + +**In:** + +- **Phase 1** — `utils/lockfile/` (npm v1/v2/v3, pnpm, yarn v1, `go.sum`), + `lockDiff.js`, `canCompareDeps` + `semanticKind() === 'deps'`, a viewer, i18n, + unit + e2e, docs +- **Phase 2** — `src/main/gitRepo.js` (fenced), `git:` IPC, CLI revision grammar, + a UI entry point, docs +- **Phase 3** — `utils/mergeConflicts.js`, the resolution UI, `mergetool` + registration writing `$MERGED`, `trustExitCode=true`, docs + +**Out:** *(recorded, not drifted into)* + +- TOML lockfiles — phase 1 ships four ecosystems, `Cargo.lock`/`poetry.lock` need + a TOML reader and get their own decision +- a package REGISTRY lookup of any kind (latest version, advisories, licences + beyond what the lockfile itself states) — rule 1, and no amount of usefulness + changes that +- staging, committing, or any git WRITE in phase 2 — reading blobs only +- free-text editing of the merged file; DiffBro resolves by choosing hunks +- `git log` browsing / a commit picker UI — phase 2 takes a revision, it does not + become a git client + +## Design + +Phases 1 and 3 add surfaces; phase 2 adds one field and a menu item. + +The dependency view is a **band + rows**, reusing what already exists: the +`.status-band` for the roll-up, `.btn`/`.btn-sm` for its controls, and the same +add/remove/change ink the grid and structure views use (`--success-text`, +`--danger-border`, `--warning-bg` at the same `color-mix` steps). A row's semver +step is a `--chip-h` chip, `.btn-count`-style — the theme's own ink at a fixed +percentage, never `--accent` as a fill under a label. + +The merge view is the existing two-pane frame with a third, read-only result +pane; each conflict is a band carrying four `.btn-sm` choices. No new depth role, +no new shadow level. + +### Theme verdict — all 20 + +Parsed from `styles/themes.css`. Every colour below is already load-bearing +somewhere in the app, so the verdict is about what the new markup composes. + +| theme | ground (`--bg`) | verdict | note | +|---|---|---|---| +| light | `#ffffff` (canvas inverted) | ok | rows sit on `--bg-raised`, the floating-card role | +| dark | `#0d1117` | ok | | +| solar | `#fffdf6` | ok | | +| neon | `#090d18` | ok | accent `#22d3ee` on the focus ring only | +| nord | `#2e3440` | ok | secondary text is `--text-hint`, not `--text-dim` (see ui.css) | +| sepia | `#e9dcbe` | ok | as nord | +| dim | `#1b1917` | ok | | +| beacon | `#000000` | ok | hard keyline `#e0e0e0` — nothing here removes a border | +| meridian | `#f5f7f4` | ok | | +| linen | `#faf7f0` | ok | | +| bloom | `#f9f4f5` | ok | | +| nyan | `#160a20` | ok | accent `#ff2ecb` — no glow, no accent fill under a label | +| matrix | `#020a04` | ok | accent `#00ff41` — same | +| contrast | `#ffffff` | ok | hard keyline `#111111`, kept | +| volcano | `#000000` | ok | border `#ffc9a4` | +| amber | `#0f0a02` | ok | | +| tide | `#0b1a1e` | ok | | +| ember | `#1a1013` | ok | | +| graphite | `#161616` | ok | achromatic; status ink is the semantic tokens, not hues | +| vector | `#ffffff` | ok | | + +Read off real frames per phase before that phase's commits close, the way the +row-hover mark was — a table alone is not the check. + +## Security rules touched + +Phase 1 touches none: pure parsing of a file already read, rendered through Vue +text interpolation. Rule 6 still applies to the parse — a lockfile is untrusted +input, so entry counts are capped and a malformed file degrades to the text view +rather than throwing. + +**Phase 2 and 3 are the ones that need care.** + +- **Rule 1 (offline) holds.** `git show` opens no socket. It is the same class of + sandbox exit as `gitTool.js`'s existing `execFile('git', …)` and the Windows + clipboard `powershell.exe` call — a subprocess, not a network client. Any git + subcommand that can reach the network (`fetch`, `pull`, `clone`, `ls-remote`) + is refused by an allowlist, not by convention. +- **Rule 7 (leaving the sandbox is fenced in main).** The fence, stated up front: + fixed argv through `execFile`, **never a shell**; the repo root computed in + MAIN via `git rev-parse --show-toplevel` and never accepted from the renderer; + `--` before every path; and the invocation hardened against a hostile + repository — `-c core.fsmonitor=` and `-c core.hooksPath=/dev/null`, with + `GIT_CONFIG_NOSYSTEM=1` and a cleared `GIT_*` environment. A repo you cloned is + untrusted input, and repo-local config has been an execution vector before. + This is the phase-2 acceptance criterion, not a nicety. +- **Rule 6.** A revision string from the renderer is validated against a + conservative pattern before it reaches argv, and never begins with `-`. +- **The write in phase 3 is the deliberate line-crossing.** It is narrow: main + writes exactly the `$MERGED` path git passed on the command line, held in main + from launch, never round-tripped through the renderer, and only on an explicit + user action. The renderer sends the resolved TEXT, not a path — the same shape + as `clipboard:writeFile`, which takes bytes and a display name and refuses to + let the renderer name a file. + +## Test plan + +- **unit** — `tests/renderer/utils/lockfile/*.test.js`: each parser against a real + fixture of its format, including a malformed one; `lockDiff.test.js`: added, + removed, bumped, downgraded, direct vs transitive, and a bump that changes only + `resolved` +- **unit** — `tests/main/gitRepo.test.js`: the argv builder (the fence is a pure + function, so it is unit-testable without a repo), revision validation, and the + refusal of every network subcommand +- **unit** — `tests/renderer/utils/mergeConflicts.test.js`: conflict-region + parsing, each of the four resolutions, a file with no conflicts, nested markers +- **e2e** — `e2e/deps.spec.mjs`: two real lockfiles open as a dependency summary + and the text view is one toggle away; `e2e/git-compare.spec.mjs`: a temp repo + with two commits compared by revision; `e2e/merge-resolve.spec.mjs`: a real + `git mergetool` invocation resolved and written +- **red → green** — every fix in this spec, recorded with its failure +- **seed fixtures** — `seed-local.mjs` gains a `package-lock` before/after pair, + so the dependency view is openable by hand on the host + +## Docs impact + +| surface | needed? | what changes | +|---|---|---| +| `README.md` | **yes** | a Dependencies row, the git entry point, and the Terminal row's grammar; the mergetool sentence stops apologising | +| `docs/screenshots/*.png` | **yes, phase 1 and 3** | a new viewer is a new frame; recapture in the container | +| `docs/roadmap.md` | **yes** | a Developer workflow track; "Comparing more" loses three-way merge to it | +| `docs/brand/roadmap.svg` | **yes** | same move, hand-authored | +| `docs/security.md` | **yes, phase 2** | the git subprocess fence belongs beside the other sandbox exits | +| `docs/ipc-security.md` | **yes, phase 2/3** | two new IPC surfaces | +| `docs/glossary.md` | **yes** | lockfile, direct vs transitive, three-way merge | + +## Implementation plan + +**Phase 1 — dependencies** + +- [ ] 1. `utils/lockfile/` — npm v1/v2/v3, pnpm, yarn v1, `go.sum` → one shape +- [ ] 2. `utils/lockDiff.js` — classify each change; direct vs transitive +- [ ] 3. `canCompareDeps`, `semanticKind() === 'deps'`, `shouldOpenSemantic` +- [ ] 4. The viewer + its stylesheet + i18n +- [ ] 5. Seed pair, e2e, README/glossary + +**Phase 2 — git-native comparison** + +- [ ] 6. `src/main/gitRepo.js` — the fence, argv builders, revision validation +- [ ] 7. IPC + CLI grammar (`compare :`, `.. `) +- [ ] 8. UI entry point + `docs/security.md` / `docs/ipc-security.md` +- [ ] 9. e2e against a temp repo + +**Phase 3 — finishing the merge** + +- [ ] 10. `utils/mergeConflicts.js` — regions and the four resolutions +- [ ] 11. The merge view +- [ ] 12. `mergetool` registration writing `$MERGED`, `trustExitCode=true` +- [ ] 13. e2e through a real `git mergetool`, docs, roadmap + SVG + +## Decisions + +| date | decision | why | rejected | +|---|---|---|---| +| 2026-08-09 | all three findings in ONE spec, three phases, each in its own commits | the user asked for it; they share the "compare meaning" thesis and the semantic-kind seam, and each is independently shippable | three specs | +| 2026-08-09 | order is lockfiles → git read → merge write | ascending risk. Phase 1 strains no rule, phase 2 opens a fenced subprocess, phase 3 crosses the never-writes line. Each phase's evidence informs the next | merge first, which is the loudest gap but the riskiest start | +| 2026-08-09 | **phase 3 crosses "DiffBro never writes files", on the user's instruction** | `docs/roadmap.md` parks three-way merge as "a decision, not code". The user made the decision on 2026-08-09. The argument that carries it: the app ALREADY registered as `git mergetool`, so it already took the job | keeping the viewer pure and de-registering as a mergetool instead — the honest alternative, not chosen | +| 2026-08-09 | dependencies are a semantic KIND, not an adapter | a lockfile is JSON; making it a kind keeps text and structure one toggle away | a `resolveAdapter` entry, which would have to out-rank `textAdapter` and hide the raw file | +| 2026-08-09 | four ecosystems in phase 1, TOML deferred | no new dependency, and the four cover npm/pnpm/yarn/go | hand-rolling TOML | + +## Validation + +- [ ] `/validate` — summary below, full report in `quality-audit.md` +- [ ] `npm run check` — real output, per phase +- [ ] every phase seen running before its commits close +- [ ] every Docs-impact "yes" done +- [ ] token usage measured + +### Token usage + +| category | tokens | +|---|---:| +| input | | +| output | | +| cache write | | +| cache read | | +| **total** | | + +**Outcome:** diff --git a/src/renderer/src/utils/lockfile/go.js b/src/renderer/src/utils/lockfile/go.js new file mode 100644 index 0000000..1904624 --- /dev/null +++ b/src/renderer/src/utils/lockfile/go.js @@ -0,0 +1,17 @@ +// go.sum — two lines per module, one for the zip and one for its go.mod. The +// module is what changed; the pair of hashes is not. Pure. + +/** @returns {{packages: Array, knowsDirect: boolean}|null} */ +export function parseGoSum(text) { + const seen = new Map() + for (const raw of String(text).split('\n')) { + const [name, version] = raw.trim().split(/\s+/) + if (!name || !version || !version.startsWith('v')) continue + // `v1.2.3/go.mod` is the same module at the same version. + const bare = version.replace(/\/go\.mod$/, '') + if (!seen.has(name)) { + seen.set(name, { name, version: bare, direct: false, dev: false, license: '', resolved: '' }) + } + } + return seen.size ? { packages: [...seen.values()], knowsDirect: false } : null +} diff --git a/src/renderer/src/utils/lockfile/npm.js b/src/renderer/src/utils/lockfile/npm.js new file mode 100644 index 0000000..a265d16 --- /dev/null +++ b/src/renderer/src/utils/lockfile/npm.js @@ -0,0 +1,84 @@ +// package-lock.json / npm-shrinkwrap.json, both shapes. v2 and v3 carry a flat +// `packages` map keyed by install PATH; v1 carries a nested `dependencies` tree. +// Pure. + +const DEP_FIELDS = ['dependencies', 'devDependencies', 'optionalDependencies'] + +// "node_modules/a/node_modules/@scope/b" is b, not the path it was hoisted to. +function nameFromPath(path) { + const at = path.lastIndexOf('node_modules/') + return at === -1 ? '' : path.slice(at + 'node_modules/'.length) +} + +function directNames(root) { + const names = new Set() + for (const field of DEP_FIELDS) { + for (const name of Object.keys(root?.[field] ?? {})) names.add(name) + } + return names +} + +function fromPackages(doc) { + const direct = directNames(doc.packages['']) + const out = [] + for (const [path, entry] of Object.entries(doc.packages)) { + // The root itself is the manifest, not a dependency; a link is a workspace + // pointer whose real entry appears under its own path. + if (!path || entry?.link || typeof entry?.version !== 'string') continue + const name = nameFromPath(path) + if (!name) continue + out.push({ + name, + version: entry.version, + // Only the ROOT copy can be the one the manifest asked for. A package + // installed twice — once at the top and once nested under a dependency + // wanting another version — matches by name at both paths. + direct: path === `node_modules/${name}` && direct.has(name), + dev: entry.dev === true, + license: typeof entry.license === 'string' ? entry.license : '', + resolved: typeof entry.resolved === 'string' ? entry.resolved : '' + }) + } + return out +} + +// v1 nests a dependency's own dependencies inside it, so this walks. Depth is +// bounded because the tree came from JSON.parse, which has already rejected +// anything that did not terminate. +// +// Nothing is marked direct: v1's top level is the HOISTED tree, which holds +// transitive packages too, so depth 0 does not mean "asked for". Reporting it as +// direct would be a guess rendered as a fact. +function walkV1(tree, out) { + for (const [name, entry] of Object.entries(tree ?? {})) { + if (typeof entry?.version !== 'string') continue + out.push({ + name, + version: entry.version, + direct: false, + dev: entry.dev === true, + license: '', + resolved: typeof entry.resolved === 'string' ? entry.resolved : '' + }) + if (entry.dependencies) walkV1(entry.dependencies, out) + } +} + +/** + * @param {string} text + * @returns {{packages: Array, knowsDirect: boolean}|null} null when the text is + * not a lockfile of this shape at all — the caller falls back to the text view. + */ +export function parseNpmLock(text) { + const doc = JSON.parse(text) + if (!doc || typeof doc !== 'object') return null + if (doc.packages && typeof doc.packages === 'object') { + return { packages: fromPackages(doc), knowsDirect: true } + } + if (doc.dependencies && typeof doc.dependencies === 'object') { + const out = [] + walkV1(doc.dependencies, out) + return { packages: out, knowsDirect: false } + } + return null +} diff --git a/src/renderer/src/utils/lockfile/parse.js b/src/renderer/src/utils/lockfile/parse.js new file mode 100644 index 0000000..7a9a82b --- /dev/null +++ b/src/renderer/src/utils/lockfile/parse.js @@ -0,0 +1,61 @@ +// Lockfiles, read as the dependency SETS they describe rather than as the text +// they are written in. One shape out, whatever went in. Pure. +// +// No package manager is consulted and no registry is contacted — everything +// reported comes out of the file in front of the reader (rule 1). +import { parseNpmLock } from './npm' +import { parsePnpmLock } from './pnpm' +import { parseYarnLock } from './yarn' +import { parseGoSum } from './go' + +/** + * A lockfile is untrusted input. Past this many entries the reader is not + * reading a dependency list, and the renderer should not try to lay one out. + */ +const MAX_PACKAGES = 50000 + +/** Recognised by FILENAME: a lockfile's name is its format. */ +export const LOCKFILE_KINDS = [ + { name: 'package-lock.json', kind: 'npm', parse: parseNpmLock }, + { name: 'npm-shrinkwrap.json', kind: 'npm', parse: parseNpmLock }, + { name: 'pnpm-lock.yaml', kind: 'pnpm', parse: parsePnpmLock }, + { name: 'yarn.lock', kind: 'yarn', parse: parseYarnLock }, + { name: 'go.sum', kind: 'go', parse: parseGoSum } +] + +const baseName = (name) => + String(name ?? '') + .split(/[\\/]/) + .pop() + .toLowerCase() + +/** Whether this file is one this module can read, by name alone. */ +export function lockfileKind(fileName) { + return LOCKFILE_KINDS.find((k) => k.name === baseName(fileName)) ?? null +} + +/** + * @param {string} text + * @param {string} fileName the name decides the format + * @returns {{kind: string, packages: Array<{name: string, version: string, + * direct: boolean, dev: boolean, license: string, resolved: string}>, + * knowsDirect: boolean, truncated: boolean}|null} null when the file is not a + * lockfile, or is one that will not parse — the caller keeps the text view. + */ +export function parseLockfile(text, fileName) { + const format = lockfileKind(fileName) + if (!format || typeof text !== 'string') return null + let parsed + try { + parsed = format.parse(text) + } catch { + return null + } + if (!parsed?.packages?.length) return null + return { + kind: format.kind, + packages: parsed.packages.slice(0, MAX_PACKAGES), + knowsDirect: parsed.knowsDirect === true, + truncated: parsed.packages.length > MAX_PACKAGES + } +} diff --git a/src/renderer/src/utils/lockfile/pnpm.js b/src/renderer/src/utils/lockfile/pnpm.js new file mode 100644 index 0000000..70dd6f0 --- /dev/null +++ b/src/renderer/src/utils/lockfile/pnpm.js @@ -0,0 +1,58 @@ +// pnpm-lock.yaml. Packages are keyed `name@version`, and the `importers` block +// is the only place that says which of them the project actually asked for. +// Pure. +import { parse as parseYaml } from 'yaml' + +// Same guard structuralDiff uses: an anchor bomb must not expand. +const YAML_OPTIONS = { maxAliasCount: 100, prettyErrors: false } + +const DEP_FIELDS = ['dependencies', 'devDependencies', 'optionalDependencies'] + +/** + * `@vue/shared@3.4.21` -> the LAST `@` is the separator, so a scoped name keeps + * its own. A peer suffix — `vue@3.4.21(typescript@5.4.0)` — names a variant of + * the same package, not a different one. + */ +export function splitPackageKey(key) { + const bare = String(key) + .replace(/^\//, '') + .replace(/\(.*\)$/, '') + const at = bare.lastIndexOf('@') + if (at <= 0) return null + const name = bare.slice(0, at) + const version = bare.slice(at + 1) + return name && version ? { name, version } : null +} + +function importerNames(importers) { + const direct = new Map() + for (const importer of Object.values(importers ?? {})) { + for (const field of DEP_FIELDS) { + for (const name of Object.keys(importer?.[field] ?? {})) { + direct.set(name, field === 'devDependencies' || direct.get(name) === true) + } + } + } + return direct +} + +/** @returns {{packages: Array, knowsDirect: boolean}|null} */ +export function parsePnpmLock(text) { + const doc = parseYaml(text, YAML_OPTIONS) + if (!doc || typeof doc !== 'object' || !doc.packages) return null + const direct = importerNames(doc.importers) + const out = [] + for (const key of Object.keys(doc.packages)) { + const split = splitPackageKey(key) + if (!split) continue + out.push({ + name: split.name, + version: split.version, + direct: direct.has(split.name), + dev: direct.get(split.name) === true, + license: '', + resolved: '' + }) + } + return { packages: out, knowsDirect: true } +} diff --git a/src/renderer/src/utils/lockfile/yarn.js b/src/renderer/src/utils/lockfile/yarn.js new file mode 100644 index 0000000..438f11b --- /dev/null +++ b/src/renderer/src/utils/lockfile/yarn.js @@ -0,0 +1,38 @@ +// yarn.lock v1 — a bespoke text format, not YAML. Blocks are separated by blank +// lines; a block's header is one or more comma-separated `name@range` specs +// ending in a colon, and its `version "x"` line is the resolved version. Pure. + +// `"@vue/shared@3.4.21"` and `lodash@^4.0.0` — the LAST `@` separates the range, +// so a scoped name keeps its own. +function nameFromSpec(spec) { + const bare = spec.trim().replace(/^"|"$/g, '') + const at = bare.lastIndexOf('@') + return at > 0 ? bare.slice(0, at) : bare +} + +const isHeader = (line) => !line.startsWith(' ') && !line.startsWith('#') && line.endsWith(':') + +/** + * @returns {{packages: Array, knowsDirect: boolean}|null} `knowsDirect` is false: + * a v1 lockfile records ranges, never which of them the manifest asked for. + */ +export function parseYarnLock(text) { + const lines = String(text).split('\n') + const out = [] + let name = '' + for (const raw of lines) { + const line = raw.trimEnd() + if (isHeader(line)) { + // Every spec in the header resolves to the same package, so the first + // names it. + name = nameFromSpec(line.slice(0, -1).split(',')[0]) + continue + } + const version = /^\s+version\s+"?([^"\s]+)"?/.exec(line) + if (name && version) { + out.push({ name, version: version[1], direct: false, dev: false, license: '', resolved: '' }) + name = '' + } + } + return out.length ? { packages: out, knowsDirect: false } : null +} diff --git a/tests/renderer/utils/lockfile/parse.test.js b/tests/renderer/utils/lockfile/parse.test.js new file mode 100644 index 0000000000000000000000000000000000000000..77c1fa1472b3e58509994be8e992690af888783d GIT binary patch literal 7414 zcmd5>>u%%L5pMtLDF)sGlo2gu*-3*cb-PaNcHOvkv8x0qjBR{G9f^!6lFN&v5<65-An{?F_XnHo&SmM#y>(epOv~w$~=;V zd+#qq*6n6W>^}1oEbmeF?)bRVu~+MQ4Ylc)pZ`rsw(xq?Rm&bbMEyQlmA;pG5v2*8 z4M{F#z9ypzMFj~;(uw?Gkxr^u7W9vc(d!{%j^)3f5LE<# zbiG`PNzsarRaAy}S&+GDBfzXr*-`LnBO<%YvmPNxRm8t!3IE3;O}t}uu*PLqJ)TF& z_fjgry? zJ1rx*8aCp;kTi)Vbj^X!VgF}i*@ejG#Rg9ckwjCF&@Vs#r=WC|G=ddtu}KbY*lFtw zhBihy%-+5;Tix=RvY|f5n2-aiVLr&HF!Vnn*0yLvd@ABXZR@Og;m9Wt35_L1U`fyg zn4(+(9Yac!03=dw490Av9wx9FKXRYYmE!e zmF*l#d{hCY%x(J3Z15``3~g-!{Iyuh8=1_?usdjAM~mE_Z1IaR5DRaZHxM}&08H#E z^Qt5P46H&;Tud7&x9nw}aqt}Xt_Z1jHRVt1Aw5cpmiMnC|psFXaJ)73b zgil1!a_|g#{*Q9^CiZOd+F;5?f$=(=>)f*D3=R&q<=^^~!@g%75;S+ff|hs%Jq;?j(;&Y zeJO-s59-S5mEP~5h2!y<7uJDq&h-vj9|XZu=b>5a+j~%2xvF+9*F9S>sbmBS-iZ(P z=T_Cv&SvgFDCnY}A2dsPUgy_~eib)+;#mn>HCxeoU>A_J>r>-Rn~!Rp!-hRdG3R{T zs?s<|t0@AfWIaGY;L@12+-ak|$lPP6eZb_aZmJG+N|x{HsrH~^-PQCiZEtVYHdy0i zm*rusk1@hpQ|T7BQ460>GJx`)iD2I3kdv^m>J%~-NogkIFkMRNTcMqH-D*OM(f(E5skSVRle41x^u zRfOJul~ytOK+{MHnpLtW5Nm2u2ygJLZHnQTEmYMOs!vp1r5G~}@8FzY*J(+*DlQeV z?XBAz4)fK-u-y>R`T036!_B?f#`4Gkyf91sqH6XpI%sAoo-;4ZFn6lPevmHu(=_dm zMGpV?r~Vm*gCTP0d1tRgAG>3i$%4q44Z*SassFzEK+6*!wFgTeD?TGp+iEhIoVTIJ zFjKQp$}o=51Js^s`}qw&LthhO`1goV$?a)6o?WB#7YX;sLg5zBh&SXOxg6jW&SFf#*2xY^>7y+a{p zCFv{y#2TwNK(9~myWZmy#Jmz*m`tOY89VAAy~QD2wgYRerbpjlwq2>AQj}y>ImGeo z7L`lXa9nrY=|ZwkL$E6Nb)8l|UGw;6mJ5``RHodK>m5t^0TU<81}PUz4S0h&R~hE% z3UpzypUc{{cM0;4X(3`*WPuZLmlC0N*!@)qs%a~iV%sCwmM!+#@i)I^JGM)?0~hns zTJC7A(&pcCKjs~5DNH#+ks$;?p3yACClv4^n-WKL;MhXv_CdD|GMVw4B>E) zr8a5@B4ajpp`s>_hR+u%l(ZUZ?~7!u!>1YlHUT@~b9|{#!#Onjh$d2i=*F_72t;x6 z75@1n{r&|1;m6NDJ9fw1KQtz4+3y~sr-ye~gD)SpNA+~mX?E6xyOV;{$+Y#A#@%*f zIDD2e-?Z3-X5EgEQ*Rpl!w36-UJ_uQW#|7kg}8`RQuYWxR+BazN;gQYBLM3zzj}|4 zP2DfpC>vz!&@!xHKWcE(!MtMpRHnPbr}+g?e~g=}uWEDkElwpoed_)GD?4j&D^!H| f)-_+{BGYu_UbfKXo^PPz9+`xoPJCx>ih%qtHGUO3 literal 0 HcmV?d00001 From ba99a1a6053f1df1349579e89222a419c4dac5b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Sun, 9 Aug 2026 23:38:00 +0300 Subject: [PATCH 02/16] feat(deps): composer.lock, the fifth ecosystem MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1, commit 2 of specs/2026-08-09-developer-workflow. PHP's lockfile is the odd shape of the five: two ARRAYS rather than a map, and `packages` vs `packages-dev` is how it records dev — so `dev` is known here even though `direct` is not. What composer.json asked for lives in composer.json, which is a different file, so knowsDirect is false and nothing is guessed. A licence is a LIST in this ecosystem, because a package may be dual-licensed; both are shown rather than the first one winning. `v1.2.3` and a `dev-main` branch pin are both real versions here and both survive. Co-Authored-By: Claude Opus 5 --- src/renderer/src/utils/lockfile/composer.js | 41 ++++++++++++++++++++ src/renderer/src/utils/lockfile/npm.js | 25 ++++++------ src/renderer/src/utils/lockfile/parse.js | 4 +- tests/renderer/utils/lockfile/parse.test.js | Bin 7414 -> 9095 bytes 4 files changed, 57 insertions(+), 13 deletions(-) create mode 100644 src/renderer/src/utils/lockfile/composer.js diff --git a/src/renderer/src/utils/lockfile/composer.js b/src/renderer/src/utils/lockfile/composer.js new file mode 100644 index 0000000..df56f8f --- /dev/null +++ b/src/renderer/src/utils/lockfile/composer.js @@ -0,0 +1,41 @@ +// composer.lock (PHP). Two arrays rather than a map — `packages` is production +// and `packages-dev` is not — and a licence is a list, because a package may be +// dual-licensed. Pure. + +// "v1.2.3" is as common as "1.2.3" in this ecosystem, and "dev-main" is a branch +// pin that is a real version here. +const version = (entry) => (typeof entry?.version === 'string' ? entry.version : '') + +const license = (entry) => { + if (Array.isArray(entry?.license)) + return entry.license.filter((l) => typeof l === 'string').join(', ') + return typeof entry?.license === 'string' ? entry.license : '' +} + +function collect(list, dev, out) { + if (!Array.isArray(list)) return + for (const entry of list) { + if (typeof entry?.name !== 'string' || !version(entry)) continue + out.push({ + name: entry.name, + version: version(entry), + // composer.lock is the resolved set; which of them composer.json asked for + // is not in this file. + direct: false, + dev, + license: license(entry), + resolved: typeof entry?.dist?.url === 'string' ? entry.dist.url : '' + }) + } +} + +/** @returns {{packages: Array, knowsDirect: boolean}|null} */ +export function parseComposerLock(text) { + const doc = JSON.parse(text) + if (!doc || typeof doc !== 'object') return null + if (!Array.isArray(doc.packages) && !Array.isArray(doc['packages-dev'])) return null + const out = [] + collect(doc.packages, false, out) + collect(doc['packages-dev'], true, out) + return out.length ? { packages: out, knowsDirect: false } : null +} diff --git a/src/renderer/src/utils/lockfile/npm.js b/src/renderer/src/utils/lockfile/npm.js index a265d16..c775c74 100644 --- a/src/renderer/src/utils/lockfile/npm.js +++ b/src/renderer/src/utils/lockfile/npm.js @@ -18,6 +18,18 @@ function directNames(root) { return names } +// Only the ROOT copy can be the one the manifest asked for. A package installed +// twice — once at the top and once nested under a dependency wanting another +// version — matches by name at both paths. +const packageAt = (path, name, entry, direct) => ({ + name, + version: entry.version, + direct: path === `node_modules/${name}` && direct.has(name), + dev: entry.dev === true, + license: typeof entry.license === 'string' ? entry.license : '', + resolved: typeof entry.resolved === 'string' ? entry.resolved : '' +}) + function fromPackages(doc) { const direct = directNames(doc.packages['']) const out = [] @@ -26,18 +38,7 @@ function fromPackages(doc) { // pointer whose real entry appears under its own path. if (!path || entry?.link || typeof entry?.version !== 'string') continue const name = nameFromPath(path) - if (!name) continue - out.push({ - name, - version: entry.version, - // Only the ROOT copy can be the one the manifest asked for. A package - // installed twice — once at the top and once nested under a dependency - // wanting another version — matches by name at both paths. - direct: path === `node_modules/${name}` && direct.has(name), - dev: entry.dev === true, - license: typeof entry.license === 'string' ? entry.license : '', - resolved: typeof entry.resolved === 'string' ? entry.resolved : '' - }) + if (name) out.push(packageAt(path, name, entry, direct)) } return out } diff --git a/src/renderer/src/utils/lockfile/parse.js b/src/renderer/src/utils/lockfile/parse.js index 7a9a82b..533845b 100644 --- a/src/renderer/src/utils/lockfile/parse.js +++ b/src/renderer/src/utils/lockfile/parse.js @@ -7,6 +7,7 @@ import { parseNpmLock } from './npm' import { parsePnpmLock } from './pnpm' import { parseYarnLock } from './yarn' import { parseGoSum } from './go' +import { parseComposerLock } from './composer' /** * A lockfile is untrusted input. Past this many entries the reader is not @@ -20,7 +21,8 @@ export const LOCKFILE_KINDS = [ { name: 'npm-shrinkwrap.json', kind: 'npm', parse: parseNpmLock }, { name: 'pnpm-lock.yaml', kind: 'pnpm', parse: parsePnpmLock }, { name: 'yarn.lock', kind: 'yarn', parse: parseYarnLock }, - { name: 'go.sum', kind: 'go', parse: parseGoSum } + { name: 'go.sum', kind: 'go', parse: parseGoSum }, + { name: 'composer.lock', kind: 'composer', parse: parseComposerLock } ] const baseName = (name) => diff --git a/tests/renderer/utils/lockfile/parse.test.js b/tests/renderer/utils/lockfile/parse.test.js index 77c1fa1472b3e58509994be8e992690af888783d..81b74f7c219401d334d581d4855425c3c45ba328 100644 GIT binary patch delta 1166 zcmZ`&O>0v@6h&yMw4qgMY^w{exR?Zz_tJ(gjHp-zEm%bC%80QKz~iw-gzHMKj`Lt%sq4OJ?EZ#`ta+^U*9(#1hOAX%@nCXsSJ;w zz<3!TkU|@LrXl;busJ1oW|%P5km`^$fY76WR1gag)1XJYOgEsJ{6mt%MF0ftGX(dg zkdf^A**m-F07IrUmjW?$Z`Z4xi9|eLLgVcwIoO{bl2%&HU21b}8Zd@|itu7+OspHe z-}d|7Cms`+WNl3H8{@o>SSughp*WG;>v2_F7^6{*g2xP^IXJ1}fUn zr>dvHgbc7ANWtvCg`$;f#r1ql*l+%6SBqQr=i18k*}<3f`m}jz3@tB{q3jvC$4c1@ z%87pLW6T0mikl=??Gikb?h^ij0xb`FF5A5 z@lJ;+Tqo%8h}j>v-d{1s&vvP>cF!?!Kp1g!g%r_A;)2|Q&I(H?@f!@_vfeq!18B?H z)?95^>K~mqX|fM!#KZ;@`(f+uPL} Date: Sun, 9 Aug 2026 23:41:33 +0300 Subject: [PATCH 03/16] feat(deps): compare two lockfiles as dependency moves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1, commit 3 of specs/2026-08-09-developer-workflow. diffLocks classifies every package that moved as added, removed, bumped or downgraded, carries the semver step of each, and splits the count into what the project ASKED for and what came along with it. Rows sort direct first, because that is the part a reader can act on. Three things it refuses to get wrong: - A name can be installed at two versions AT ONCE. Comparing a version per name would invent a bump that never happened, so each side is a SET of versions per name; what both sides hold did not move, and only the leftovers pair up. - `1.10.0` is not older than `1.9.0`. Direction comes from the parsed triple, never from the string. - Below 1.0.0 the MINOR is the breaking position, and a lockfile is full of packages that never left 0.x — so 0.1.0 to 0.2.0 reports as major. `knowsDirect` is only claimed when BOTH sides recorded it; one side guessing is the same as not knowing. Validated against this repo's own history: package-lock.json between v0.4.10 and HEAD is 15 insertions and 13 deletions of text, which this reads as 4 packages bumped, one of them direct — mermaid 11.16.0 to 11.16.1, a patch. Co-Authored-By: Claude Opus 5 --- src/renderer/src/utils/lockfile/lockDiff.js | 145 ++++++++++++++++++ .../renderer/utils/lockfile/lockDiff.test.js | 135 ++++++++++++++++ 2 files changed, 280 insertions(+) create mode 100644 src/renderer/src/utils/lockfile/lockDiff.js create mode 100644 tests/renderer/utils/lockfile/lockDiff.test.js diff --git a/src/renderer/src/utils/lockfile/lockDiff.js b/src/renderer/src/utils/lockfile/lockDiff.js new file mode 100644 index 0000000..bdb05f4 --- /dev/null +++ b/src/renderer/src/utils/lockfile/lockDiff.js @@ -0,0 +1,145 @@ +// What changed between two lockfiles, as the dependency moves they describe +// rather than as the thousands of lines they are written in. Pure. + +const RANK = { added: 0, removed: 1, bumped: 2, downgraded: 3 } + +// Leading `v`, a prerelease tag and build metadata all decorate a version +// without changing which release it is. +function triple(version) { + const bare = String(version ?? '') + .trim() + .replace(/^v/i, '') + .split(/[-+]/)[0] + const parts = bare.split('.') + if (parts.length < 2) return null + const nums = parts.slice(0, 3).map((p) => Number(p)) + if (nums.some((n) => !Number.isInteger(n) || n < 0)) return null + return { major: nums[0], minor: nums[1], patch: nums[2] ?? 0 } +} + +/** + * The step between two versions, in the direction of travel — a downgrade of a + * major is still a major move. + * @returns {'major'|'minor'|'patch'|'same'|'unknown'} + */ +export function semverStep(from, to) { + const a = triple(from) + const b = triple(to) + if (!a || !b) return 'unknown' + if (a.major === b.major && a.minor === b.minor && a.patch === b.patch) return 'same' + if (a.major !== b.major) return 'major' + // Below 1.0.0 the minor IS the breaking position, and a lockfile is full of + // packages that never left 0.x. + if (a.minor !== b.minor) return a.major === 0 ? 'major' : 'minor' + return 'patch' +} + +// A name can be installed at more than one version at once, so a side is a set +// of versions per name, not a version per name. +function index(lock) { + const map = new Map() + for (const pkg of lock?.packages ?? []) { + if (!map.has(pkg.name)) map.set(pkg.name, new Map()) + map.get(pkg.name).set(pkg.version, pkg) + } + return map +} + +// Which way the version moved. Compares the triples, not the strings, so +// `1.10.0` does not read as older than `1.9.0`. +function moveOf(from, to) { + const a = triple(from.version) + const b = triple(to.version) + if (!a || !b) return 'bumped' + const back = + b.major < a.major || + (b.major === a.major && b.minor < a.minor) || + (b.major === a.major && b.minor === a.minor && b.patch < a.patch) + return back ? 'downgraded' : 'bumped' +} + +function statusOf(from, to) { + if (from && to) return moveOf(from, to) + return from ? 'removed' : 'added' +} + +// A licence only CHANGED where both sides stated one; a lockfile that never +// records licences must not report every package as relicensed. +const licenseMoved = (from, to) => + Boolean(from?.license) && Boolean(to?.license) && from.license !== to.license + +function rowFor(name, from, to) { + // The surviving side describes the row: what a package IS after the change, + // or what it was when it went. + const now = to ?? from ?? {} + const both = Boolean(from) && Boolean(to) + return { + name, + status: statusOf(from, to), + from: from?.version ?? '', + to: to?.version ?? '', + step: both ? semverStep(from.version, to.version) : 'unknown', + direct: now.direct === true, + dev: now.dev === true, + license: now.license ?? '', + licenseChanged: licenseMoved(from, to) + } +} + +// The versions of one name that only one side has. What both sides hold did not +// move, whatever else did. +function moved(leftVersions, rightVersions) { + const common = new Set([...leftVersions.keys()].filter((v) => rightVersions.has(v))) + const gone = [...leftVersions.keys()].filter((v) => !common.has(v)).sort() + const came = [...rightVersions.keys()].filter((v) => !common.has(v)).sort() + return { gone, came } +} + +function rowsForName(name, leftVersions, rightVersions) { + const { gone, came } = moved(leftVersions ?? new Map(), rightVersions ?? new Map()) + const rows = [] + const pairs = Math.min(gone.length, came.length) + for (let i = 0; i < pairs; i++) { + rows.push(rowFor(name, leftVersions.get(gone[i]), rightVersions.get(came[i]))) + } + for (let i = pairs; i < gone.length; i++) rows.push(rowFor(name, leftVersions.get(gone[i]), null)) + for (let i = pairs; i < came.length; i++) + rows.push(rowFor(name, null, rightVersions.get(came[i]))) + return rows +} + +// What the reader asked for comes first — it is the part they can act on. +function ordered(rows) { + return rows.sort((a, b) => { + if (a.direct !== b.direct) return a.direct ? -1 : 1 + if (a.name !== b.name) return a.name < b.name ? -1 : 1 + return RANK[a.status] - RANK[b.status] + }) +} + +/** + * @param {object} left a parsed lockfile (see lockfile/parse) + * @param {object} right + * @returns {{rows: Array, stats: {added:number,removed:number,bumped:number, + * downgraded:number}, directCount:number, knowsDirect:boolean, + * identical:boolean}} + */ +export function diffLocks(left, right) { + const a = index(left) + const b = index(right) + const rows = [] + for (const name of new Set([...a.keys(), ...b.keys()])) { + rows.push(...rowsForName(name, a.get(name), b.get(name))) + } + const stats = { added: 0, removed: 0, bumped: 0, downgraded: 0 } + for (const row of rows) stats[row.status] += 1 + return { + rows: ordered(rows), + stats, + directCount: rows.filter((r) => r.direct).length, + // Only claim the split where BOTH sides recorded it; one side guessing is + // the same as not knowing. + knowsDirect: left?.knowsDirect === true && right?.knowsDirect === true, + identical: rows.length === 0 + } +} diff --git a/tests/renderer/utils/lockfile/lockDiff.test.js b/tests/renderer/utils/lockfile/lockDiff.test.js new file mode 100644 index 0000000..3c137a1 --- /dev/null +++ b/tests/renderer/utils/lockfile/lockDiff.test.js @@ -0,0 +1,135 @@ +import { describe, expect, it } from 'vitest' +import { diffLocks, semverStep } from '../../../../src/renderer/src/utils/lockfile/lockDiff' + +const pkg = (name, version, extra = {}) => ({ + name, + version, + direct: false, + dev: false, + license: '', + resolved: '', + ...extra +}) +const lock = (packages, extra = {}) => ({ + kind: 'npm', + packages, + knowsDirect: true, + truncated: false, + ...extra +}) +const byName = (rows) => Object.fromEntries(rows.map((r) => [r.name, r])) + +describe('semverStep', () => { + it('names the step between two versions', () => { + expect(semverStep('1.2.3', '2.0.0')).toBe('major') + expect(semverStep('1.2.3', '1.3.0')).toBe('minor') + expect(semverStep('1.2.3', '1.2.4')).toBe('patch') + expect(semverStep('1.2.3', '1.2.3')).toBe('same') + }) + + it('reads a downgrade as the step it went back', () => { + expect(semverStep('2.0.0', '1.9.9')).toBe('major') + expect(semverStep('1.3.0', '1.2.0')).toBe('minor') + }) + + // 0.x is where a minor bump is the breaking one, and a lockfile is full of it. + it('treats a 0.x minor as the breaking step', () => { + expect(semverStep('0.1.0', '0.2.0')).toBe('major') + expect(semverStep('0.1.0', '0.1.1')).toBe('patch') + }) + + it('says nothing it cannot work out', () => { + expect(semverStep('1.2.3', 'v1.2.4-beta.1+build')).toBe('patch') + expect(semverStep('not-a-version', '1.0.0')).toBe('unknown') + expect(semverStep('', '')).toBe('unknown') + }) +}) + +describe('diffLocks', () => { + it('reports nothing when the two sides hold the same set', () => { + const same = lock([pkg('vue', '3.4.21'), pkg('vitest', '4.1.10')]) + const result = diffLocks(same, same) + expect(result.rows).toHaveLength(0) + expect(result.stats).toEqual({ added: 0, removed: 0, bumped: 0, downgraded: 0 }) + expect(result.identical).toBe(true) + }) + + it('classifies added, removed, bumped and downgraded', () => { + const left = lock([pkg('kept', '1.0.0'), pkg('gone', '2.0.0'), pkg('down', '3.1.0')]) + const right = lock([pkg('kept', '1.1.0'), pkg('fresh', '0.1.0'), pkg('down', '3.0.0')]) + const rows = byName(diffLocks(left, right).rows) + expect(rows.kept.status).toBe('bumped') + expect(rows.kept.from).toBe('1.0.0') + expect(rows.kept.to).toBe('1.1.0') + expect(rows.gone.status).toBe('removed') + expect(rows.fresh.status).toBe('added') + expect(rows.down.status).toBe('downgraded') + }) + + it('carries the semver step of a bump', () => { + const left = lock([pkg('a', '1.0.0'), pkg('b', '1.0.0')]) + const right = lock([pkg('a', '2.0.0'), pkg('b', '1.0.1')]) + const rows = byName(diffLocks(left, right).rows) + expect(rows.a.step).toBe('major') + expect(rows.b.step).toBe('patch') + }) + + // The headline the whole view exists for: of everything that moved, how much + // did I ask for? + it('splits the count into what was asked for and what came along', () => { + const left = lock([pkg('asked', '1.0.0', { direct: true }), pkg('carried', '1.0.0')]) + const right = lock([pkg('asked', '2.0.0', { direct: true }), pkg('carried', '1.1.0')]) + const result = diffLocks(left, right) + expect(result.stats.bumped).toBe(2) + expect(result.directCount).toBe(1) + expect(byName(result.rows).asked.direct).toBe(true) + expect(byName(result.rows).carried.direct).toBe(false) + }) + + // A package can be installed at two versions at once. Reporting "bumped" for + // that would invent a move that never happened. + it('handles a package installed at two versions on one side', () => { + const left = lock([pkg('dup', '1.0.0'), pkg('dup', '2.0.0')]) + const right = lock([pkg('dup', '2.0.0')]) + const rows = diffLocks(left, right).rows.filter((r) => r.name === 'dup') + expect(rows).toHaveLength(1) + expect(rows[0].status).toBe('removed') + expect(rows[0].from).toBe('1.0.0') + }) + + it('reports a version arriving beside one that stays as added', () => { + const left = lock([pkg('dup', '1.0.0')]) + const right = lock([pkg('dup', '1.0.0'), pkg('dup', '2.0.0')]) + const rows = diffLocks(left, right).rows.filter((r) => r.name === 'dup') + expect(rows).toHaveLength(1) + expect(rows[0].status).toBe('added') + expect(rows[0].to).toBe('2.0.0') + }) + + it('sorts what the reader asked for above what came along', () => { + const left = lock([pkg('zzz', '1.0.0', { direct: true }), pkg('aaa', '1.0.0')]) + const right = lock([pkg('zzz', '2.0.0', { direct: true }), pkg('aaa', '2.0.0')]) + expect(diffLocks(left, right).rows.map((r) => r.name)).toEqual(['zzz', 'aaa']) + }) + + it('says when neither side could tell direct from transitive', () => { + const l = lock([pkg('a', '1.0.0')], { knowsDirect: false }) + const r = lock([pkg('a', '2.0.0')], { knowsDirect: false }) + expect(diffLocks(l, r).knowsDirect).toBe(false) + expect(diffLocks(l, r).directCount).toBe(0) + }) + + it('carries dev and licence through to the row', () => { + const left = lock([pkg('a', '1.0.0', { dev: true, license: 'MIT' })]) + const right = lock([pkg('a', '2.0.0', { dev: true, license: 'Apache-2.0' })]) + const row = diffLocks(left, right).rows[0] + expect(row.dev).toBe(true) + expect(row.license).toBe('Apache-2.0') + expect(row.licenseChanged).toBe(true) + }) + + it('survives a side with nothing in it', () => { + const result = diffLocks(lock([]), lock([pkg('a', '1.0.0')])) + expect(result.stats.added).toBe(1) + }) +}) From 363f001b80d3e474168c7cceb7a1fdbb1ea950e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Sun, 9 Aug 2026 23:46:23 +0300 Subject: [PATCH 04/16] feat(deps): the dependency view is a semantic kind, beside tree and grid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1, commit 4 of specs/2026-08-09-developer-workflow. A lockfile IS json, so it must not go through resolveAdapter and hide the raw file. It becomes a semantic KIND instead — semanticKind() gains 'deps' — which keeps the text and structure views one toggle away and reuses the routing diagram and grid already use. Deps outranks tree deliberately: json is what a lockfile is, but a 780-key structural tree is not what anyone opened it for. Both sides must parse AND be the same ecosystem, so a package-lock against a go.sum is refused rather than compared as if it meant something. Two extractions, forced by diffStore sitting exactly on its 747-line ratchet and paid for rather than waived: - lockPairOf moves to utils/lockfile/lockPair.js, so the store holds the question and the util holds the work - structureDiff's body moves to utils/structurePair.js for the same reason — pure comparison plumbing that was never store logic, and it takes structureAdapter and diffStructures out of the store's imports Also fixes an orphaned comment: the six lines describing comparableKind sat above semanticKind, having been left behind when that getter was split out. They now sit above the getter they describe. The store gained no net lines. The cap was not raised. Co-Authored-By: Claude Opus 5 --- src/renderer/src/stores/diffStore.js | 34 +++--- src/renderer/src/utils/lockfile/lockPair.js | 13 +++ src/renderer/src/utils/structurePair.js | 18 +++ src/renderer/src/utils/viewChrome.js | 3 +- src/shared/i18n/en-XA.json | 1 + src/shared/i18n/en.json | 1 + tests/renderer/stores/diffStore.deps.test.js | 115 +++++++++++++++++++ 7 files changed, 167 insertions(+), 18 deletions(-) create mode 100644 src/renderer/src/utils/lockfile/lockPair.js create mode 100644 src/renderer/src/utils/structurePair.js create mode 100644 tests/renderer/stores/diffStore.deps.test.js diff --git a/src/renderer/src/stores/diffStore.js b/src/renderer/src/stores/diffStore.js index 57d8ad2..44acc83 100644 --- a/src/renderer/src/stores/diffStore.js +++ b/src/renderer/src/stores/diffStore.js @@ -1,10 +1,11 @@ import { defineStore } from 'pinia' import { resolveAdapter } from '../adapters' -import { structureAdapter } from '../adapters/structureAdapter' import { csvAdapter } from '../adapters/csvAdapter' -import { diffStructures, structuredKind } from '../utils/structuralDiff' +import { structuredKind } from '../utils/structuralDiff' import { delimitedKind } from '../utils/csv' import { restoredSemanticView, shouldOpenSemantic } from '../utils/viewChrome' +import { lockPairOf } from '../utils/lockfile/lockPair' +import { structurePairDiff } from '../utils/structurePair' import { useVaultStore } from './vaultStore' import { useSnippetStore } from './snippetStore' import { isSecret } from '../utils/secretSnippet' @@ -185,30 +186,29 @@ export const useDiffStore = defineStore('diff', { // else a tree. A KEY — as a word it stayed English in every locale. structureLabelKey() { if (this.canCompareDiagram) return 'appToolbar.structureDiagram' + if (this.canCompareDeps) return 'appToolbar.structureDeps' return this.delimitedFormat ? 'appToolbar.structureGrid' : 'appToolbar.structure' }, structureDiff() { - const kind = this.structuredFormat - if (!kind) return null - const left = structureAdapter.toComparable(this.left, kind) - const right = structureAdapter.toComparable(this.right, kind) - if (left.error || right.error) return null - return diffStructures(left.value, right.value) - }, - // Which viewer the loaded comparison needs: 'text' (Monaco), 'spreadsheet' - // (grid), 'tree' (structure) or 'streamed' (virtualized rows). Text is the - // default so an empty/paste state routes to Monaco. Streamed wins over the - // other side's kind: one file too large to hold makes the whole comparison - // streamed — and a streamed side has no content in memory, so the structure - // toggle above it can never be on. - // Which viewer the semantic toggle asks for, or null when it is off. Split - // out so comparableKind stays one question: semantic, or the file's own kind. + return structurePairDiff(this.left, this.right, this.structuredFormat) + }, + lockPair() { + return lockPairOf(this.left, this.right) + }, + canCompareDeps() { + return this.lockPair !== null + }, + // Deps outranks tree: a lockfile IS json, and a 780-key tree is not what + // the reader opened it for. semanticKind() { if (!this.semanticView) return null if (this.canCompareDiagram) return 'diagram' + if (this.canCompareDeps) return 'deps' if (this.delimitedFormat) return 'spreadsheet' return this.canCompareStructure ? 'tree' : null }, + // Text is the default so an empty/paste state routes to Monaco; streamed + // wins, because a file too large to hold has no content to compare. comparableKind() { const semantic = this.semanticKind if (semantic) return semantic diff --git a/src/renderer/src/utils/lockfile/lockPair.js b/src/renderer/src/utils/lockfile/lockPair.js new file mode 100644 index 0000000..d3506b5 --- /dev/null +++ b/src/renderer/src/utils/lockfile/lockPair.js @@ -0,0 +1,13 @@ +// The two sides as a comparable pair of lockfiles, or nothing. Pure. +import { parseLockfile } from './parse' + +/** + * Both sides must parse AND be the same ecosystem: a package-lock against a + * go.sum is two dependency lists, not a comparison. + * @returns {{left: object, right: object}|null} + */ +export function lockPairOf(left, right) { + const a = parseLockfile(left?.content, left?.name) + const b = parseLockfile(right?.content, right?.name) + return a && b && a.kind === b.kind ? { left: a, right: b } : null +} diff --git a/src/renderer/src/utils/structurePair.js b/src/renderer/src/utils/structurePair.js new file mode 100644 index 0000000..6a490e8 --- /dev/null +++ b/src/renderer/src/utils/structurePair.js @@ -0,0 +1,18 @@ +// The structural comparison of two loaded files, or null where there is nothing +// to compare. Lifted out of diffStore so the store holds the question and this +// holds the work. Pure. +import { structureAdapter } from '../adapters/structureAdapter' +import { diffStructures } from './structuralDiff' + +/** + * @param {object} left a loaded file + * @param {object} right + * @param {string|null} kind the structured format both sides are, if any + */ +export function structurePairDiff(left, right, kind) { + if (!kind) return null + const a = structureAdapter.toComparable(left, kind) + const b = structureAdapter.toComparable(right, kind) + if (a.error || b.error) return null + return diffStructures(a.value, b.value) +} diff --git a/src/renderer/src/utils/viewChrome.js b/src/renderer/src/utils/viewChrome.js index 01cbab1..3af83e3 100644 --- a/src/renderer/src/utils/viewChrome.js +++ b/src/renderer/src/utils/viewChrome.js @@ -49,7 +49,8 @@ export const showsWhitespaceToggle = (store) => * @param {object} store * @returns {boolean} */ -export const shouldOpenSemantic = (store) => !!store?.canCompareDiagram || !!store?.delimitedFormat +export const shouldOpenSemantic = (store) => + !!store?.canCompareDiagram || !!store?.canCompareDeps || !!store?.delimitedFormat /** * The view a restored snapshot opens in: the one it recorded, or — where it diff --git a/src/shared/i18n/en-XA.json b/src/shared/i18n/en-XA.json index 9331d4a..854e56f 100644 --- a/src/shared/i18n/en-XA.json +++ b/src/shared/i18n/en-XA.json @@ -479,6 +479,7 @@ "structure": "[Šţřūçţūřé ·øé·]", "structureGrid": "[Ğřĩđ ·ø]", "structureDiagram": "[Đĩàğřàɱ ·øé]", + "structureDeps": "[Đéƥéńđéńçĩéş ·øé·ø]", "tips": { "view": "[Šƥłĩţ ṽĩéŵ, şţřūçţūřé àńđ ŵĥĩţéşƥàçé — éṽéřŷţĥĩńğ ţĥàţ çĥàńğéş ĥōŵ ţĥé çōɱƥàřĩşōń ĩş đřàŵń ·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé]", "more": "[Àçţĩōńş ţĥé ƀàř ĥàş ńō řōōɱ ƒōř ·øé·øé·øé·]", diff --git a/src/shared/i18n/en.json b/src/shared/i18n/en.json index 2f28f6a..943a6d0 100644 --- a/src/shared/i18n/en.json +++ b/src/shared/i18n/en.json @@ -479,6 +479,7 @@ "structure": "Structure", "structureGrid": "Grid", "structureDiagram": "Diagram", + "structureDeps": "Dependencies", "tips": { "view": "Split view, structure and whitespace — everything that changes how the comparison is drawn", "more": "Actions the bar has no room for", diff --git a/tests/renderer/stores/diffStore.deps.test.js b/tests/renderer/stores/diffStore.deps.test.js new file mode 100644 index 0000000..988f884 --- /dev/null +++ b/tests/renderer/stores/diffStore.deps.test.js @@ -0,0 +1,115 @@ +// The dependency view: which comparisons offer it, and what it routes to. +import { beforeEach, describe, expect, it } from 'vitest' +import { createPinia, setActivePinia } from 'pinia' +import { useDiffStore } from '../../../src/renderer/src/stores/diffStore' +import { diffLocks } from '../../../src/renderer/src/utils/lockfile/lockDiff' + +beforeEach(() => { + setActivePinia(createPinia()) + localStorage.clear() + window.api = {} +}) + +const lockText = (deps) => + JSON.stringify({ + lockfileVersion: 3, + packages: { + '': { dependencies: Object.fromEntries(Object.keys(deps).map((n) => [n, '*'])) }, + ...Object.fromEntries( + Object.entries(deps).map(([n, v]) => [`node_modules/${n}`, { version: v }]) + ) + } + }) + +const load = (diff, left, right, name = 'package-lock.json') => { + diff.left = { path: `/a/${name}`, name, content: left } + diff.right = { path: `/b/${name}`, name, content: right } + diff.mode = 'files' +} + +describe('dependency comparison', () => { + it('offers the view when both sides are the same kind of lockfile', () => { + const diff = useDiffStore() + load(diff, lockText({ vue: '3.4.0' }), lockText({ vue: '3.5.0' })) + expect(diff.canCompareDeps).toBe(true) + }) + + it('refuses when only one side is a lockfile', () => { + const diff = useDiffStore() + diff.left = { path: '/a', name: 'package-lock.json', content: lockText({ vue: '3.4.0' }) } + diff.right = { path: '/b', name: 'notes.txt', content: 'hello' } + diff.mode = 'files' + expect(diff.canCompareDeps).toBe(false) + }) + + // A package-lock against a go.sum is two ecosystems, not a comparison. + it('refuses two lockfiles of different ecosystems', () => { + const diff = useDiffStore() + diff.left = { path: '/a', name: 'package-lock.json', content: lockText({ vue: '3.4.0' }) } + diff.right = { path: '/b', name: 'go.sum', content: 'github.com/a/b v1.0.0 h1:x=\n' } + diff.mode = 'files' + expect(diff.canCompareDeps).toBe(false) + }) + + it('refuses a lockfile that will not parse', () => { + const diff = useDiffStore() + load(diff, '{ not json', lockText({ vue: '3.5.0' })) + expect(diff.canCompareDeps).toBe(false) + }) + + it('routes to the dependency viewer with the toggle on', () => { + const diff = useDiffStore() + load(diff, lockText({ vue: '3.4.0' }), lockText({ vue: '3.5.0' })) + diff.semanticView = false + expect(diff.comparableKind).toBe('text') + diff.semanticView = true + expect(diff.comparableKind).toBe('deps') + }) + + // A lockfile IS json, so the structure view would otherwise claim it — and a + // 780-key tree is not what the reader asked for. + it('wins over the structure view for the same file', () => { + const diff = useDiffStore() + load(diff, lockText({ vue: '3.4.0' }), lockText({ vue: '3.5.0' })) + diff.semanticView = true + expect(diff.canCompareStructure).toBe(true) + expect(diff.comparableKind).toBe('deps') + }) + + it('opens in the dependency view without being asked', () => { + const diff = useDiffStore() + load(diff, lockText({ vue: '3.4.0' }), lockText({ vue: '3.5.0' })) + diff.receive('left', { + path: '/a', + name: 'package-lock.json', + content: lockText({ vue: '3.4.0' }) + }) + expect(diff.semanticView).toBe(true) + }) + + it('names the toggle after what it shows', () => { + const diff = useDiffStore() + load(diff, lockText({ vue: '3.4.0' }), lockText({ vue: '3.5.0' })) + expect(diff.structureLabelKey).toBe('appToolbar.structureDeps') + }) + + // The store answers WHICH lockfiles; the viewer does the comparing. + it('hands the viewer both parsed sides', () => { + const diff = useDiffStore() + load(diff, lockText({ vue: '3.4.0', gone: '1.0.0' }), lockText({ vue: '3.5.0' })) + const { left, right } = diff.lockPair + expect(left.kind).toBe('npm') + expect(diffLocks(left, right).stats).toEqual({ + added: 0, + removed: 1, + bumped: 1, + downgraded: 0 + }) + }) + + it('has no pair to offer when a side is not a lockfile', () => { + const diff = useDiffStore() + load(diff, lockText({ vue: '3.4.0' }), lockText({ vue: '3.5.0' }), 'notes.txt') + expect(diff.lockPair).toBeNull() + }) +}) From 1b072c6a23bde852829411714771425e7733a2be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Sun, 9 Aug 2026 23:49:40 +0300 Subject: [PATCH 05/16] feat(deps): the dependency viewer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1, commit 5 of specs/2026-08-09-developer-workflow. One row per package that moved, what the reader asked for first, and the carried packages folded away until wanted — they are the bulk of any lockfile move and rarely why anyone opened it. Status is the row's LEFT EDGE rather than a wash: a row already carries a red version and a green one, and a third tint behind them reads as neither. Row height comes from --control-h and the chips from --chip-h, so a long package name cannot make one row taller than its neighbours. Every colour is a role the diff panes already use. The semver step is rendered from a map of LITERAL key ids rather than a key built from the value: a template-assembled key is invisible to check:i18n, which caught it. Driven against two real lockfiles from this repo's own history — 10,222 lines against 10,224 — and it renders one row: mermaid, asked for, 11.16.0 to 11.16.1, patch. Three carried packages behind the fold. Co-Authored-By: Claude Opus 5 --- src/renderer/src/App.vue | 2 + .../src/components/DepsDiffViewer.vue | 90 +++++++++++++++ .../src/components/DepsStatusBand.vue | 35 ++++++ .../src/components/styles/DepsDiffViewer.css | 109 ++++++++++++++++++ src/shared/i18n/en-XA.json | 20 +++- src/shared/i18n/en.json | 20 +++- 6 files changed, 274 insertions(+), 2 deletions(-) create mode 100644 src/renderer/src/components/DepsDiffViewer.vue create mode 100644 src/renderer/src/components/DepsStatusBand.vue create mode 100644 src/renderer/src/components/styles/DepsDiffViewer.css diff --git a/src/renderer/src/App.vue b/src/renderer/src/App.vue index b052c8f..5c65377 100644 --- a/src/renderer/src/App.vue +++ b/src/renderer/src/App.vue @@ -15,6 +15,7 @@ import FileSlot from './components/FileSlot.vue' import DiffViewer from './components/DiffViewer.vue' import SpreadsheetDiffViewer from './components/SpreadsheetDiffViewer.vue' import DiagramDiffViewer from './components/DiagramDiffViewer.vue' +import DepsDiffViewer from './components/DepsDiffViewer.vue' import StructureDiffViewer from './components/StructureDiffViewer.vue' import StreamedDiffViewer from './components/StreamedDiffViewer.vue' import SupportedFormats from './components/SupportedFormats.vue' @@ -172,6 +173,7 @@ useSnippetDiffSync() + diff --git a/src/renderer/src/components/DepsDiffViewer.vue b/src/renderer/src/components/DepsDiffViewer.vue new file mode 100644 index 0000000..9b673fb --- /dev/null +++ b/src/renderer/src/components/DepsDiffViewer.vue @@ -0,0 +1,90 @@ + + + + + diff --git a/src/renderer/src/components/DepsStatusBand.vue b/src/renderer/src/components/DepsStatusBand.vue new file mode 100644 index 0000000..8c7dedc --- /dev/null +++ b/src/renderer/src/components/DepsStatusBand.vue @@ -0,0 +1,35 @@ + + + diff --git a/src/renderer/src/components/styles/DepsDiffViewer.css b/src/renderer/src/components/styles/DepsDiffViewer.css new file mode 100644 index 0000000..65f7353 --- /dev/null +++ b/src/renderer/src/components/styles/DepsDiffViewer.css @@ -0,0 +1,109 @@ +/* The dependency comparison. Every colour is a role the diff panes already use, + so it reads the same on all twenty themes and keeps the hard keyline the + contrast/beacon palettes rely on. The band itself is .status-band in ui.css. */ +.deps-diff { + flex: 1; + min-height: 0; + display: flex; + flex-direction: column; + border: 1px solid var(--border); + border-radius: var(--radius-lg); + background: var(--bg-raised); + overflow: hidden; +} +.deps-bar { + flex: none; + gap: var(--space-3); + padding: 0 var(--space-3); + min-height: var(--control-h); + background: var(--bg-panel); + border-bottom: 1px solid var(--border); +} +.deps-title { + color: var(--text-dim); + font-size: var(--font-sm); + font-weight: 600; +} +.deps-bar .btn { + margin-left: auto; +} +.identical-row { + display: flex; + align-items: center; + gap: var(--space-2); + padding: var(--space-2) var(--space-4); + color: var(--text-dim); +} +.identical-row .ok { + color: var(--success-text); +} +.deps-rows { + flex: 1; + min-height: 0; + overflow: auto; +} +/* A row is a band: its height comes from the control scale, never from padding, + so a longer package name cannot make one row taller than its neighbours. */ +.deps-row { + display: flex; + align-items: center; + gap: var(--space-2); + min-height: var(--control-h); + padding: 0 var(--space-3); + border-bottom: 1px solid var(--border); + font-size: var(--font-md); +} +.deps-row:last-child { + border-bottom: 0; +} +/* Status is on the LEFT EDGE rather than as a wash: a row already carries two + coloured versions, and a third tint behind them reads as neither. */ +.deps-row.added { + box-shadow: inset 3px 0 0 var(--success-text); +} +.deps-row.removed { + box-shadow: inset 3px 0 0 var(--danger-border); +} +.deps-row.bumped, +.deps-row.downgraded { + box-shadow: inset 3px 0 0 var(--warning-bg); +} +.deps-name { + font-family: var(--font-mono); + color: var(--text); + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; +} +.deps-move { + margin-left: auto; + font-family: var(--font-mono); + font-size: var(--font-sm); + white-space: nowrap; +} +.deps-move .add { + color: var(--success-text); +} +.deps-move .del { + color: var(--danger-border); +} +/* Height from --chip-h, never from padding — the rule the Esc chip broke. */ +.deps-tag { + height: var(--chip-h); + padding: 0 var(--space-2); + display: inline-flex; + align-items: center; + border-radius: var(--radius-pill); + background: var(--btn-face); + color: var(--text-hint); + font-size: var(--font-xs); + white-space: nowrap; +} +.deps-step { + flex: none; +} +.deps-license { + color: var(--text-hint); + font-size: var(--font-xs); + white-space: nowrap; +} diff --git a/src/shared/i18n/en-XA.json b/src/shared/i18n/en-XA.json index 854e56f..2331a8c 100644 --- a/src/shared/i18n/en-XA.json +++ b/src/shared/i18n/en-XA.json @@ -558,6 +558,23 @@ "showTheChangeList": "[Šĥōŵ ţĥé çĥàńğé łĩşţ ·øé·øé·]", "notComparableAsPicture": "[Ţĥĩş đĩàğřàɱ ţŷƥé çàń’ţ ƀé çōɱƥàřéđ àş à ƥĩçţūřé ŷéţ. ·øé·øé·øé·øé·øé·ø]" }, + "depsDiffViewer": { + "packages": "[Ƥàçķàğéş ·øé·]", + "changed": "[Çĥàńğéđ ·øé]", + "askedFor": "[Àşķéđ ƒōř ·øé·]", + "direct": "[àşķéđ ƒōř ·øé·]", + "dev": "[đéṽ ·ø]", + "noDependencyChanged": "[Ńō đéƥéńđéńçŷ çĥàńğéđ — ţĥé şàɱé ṽéřşĩōńş, éĩţĥéř şĩđé ·øé·øé·øé·øé·øé·øé]", + "showCarried": "[Šĥōŵ {n} çàřřĩéđ ƥàçķàğé | Šĥōŵ {n} çàřřĩéđ ƥàçķàğéş ·øé·øé·øé·øé·øé·]", + "askedForOnly": "[Ōńłŷ ŵĥàţ ŷōū àşķéđ ƒōř ·øé·øé·ø]", + "countBumped": "[{n} ƀūɱƥéđ ·øé]", + "countDowngraded": "[{n} đōŵńğřàđéđ ·øé·ø]", + "step": { + "major": "[ɱàĵōř ·ø]", + "minor": "[ɱĩńōř ·ø]", + "patch": "[ƥàţçĥ ·ø]" + } + }, "diffTabBar": { "previousComparison": "[Ƥřéṽĩōūş çōɱƥàřĩşōń ·øé·øé·ø]", "openComparisons": "[Ōƥéń çōɱƥàřĩşōńş ·øé·øé]", @@ -1507,7 +1524,8 @@ "moved": "[{n} ɱōṽéđ ·øé]", "sheets": "[{n} şĥééţ | {n} şĥééţş ·øé·øé]", "matches": "[{n} ɱàţçĥ | {n} ɱàţçĥéş ·øé·øé]", - "unchangedHidden": "[{n} ūńçĥàńğéđ ĥĩđđéń ·øé·øé·]" + "unchangedHidden": "[{n} ūńçĥàńğéđ ĥĩđđéń ·øé·øé·]", + "packages": "[{n} ƥàçķàğé | {n} ƥàçķàğéş ·øé·øé·]" }, "sheetTabBar": { "onlyIn": "[ōńłŷ {side} ·øé·]", diff --git a/src/shared/i18n/en.json b/src/shared/i18n/en.json index 943a6d0..07f7c28 100644 --- a/src/shared/i18n/en.json +++ b/src/shared/i18n/en.json @@ -558,6 +558,23 @@ "showTheChangeList": "Show the change list", "notComparableAsPicture": "This diagram type can’t be compared as a picture yet." }, + "depsDiffViewer": { + "packages": "Packages", + "changed": "Changed", + "askedFor": "Asked for", + "direct": "asked for", + "dev": "dev", + "noDependencyChanged": "No dependency changed — the same versions, either side", + "showCarried": "Show {n} carried package | Show {n} carried packages", + "askedForOnly": "Only what you asked for", + "countBumped": "{n} bumped", + "countDowngraded": "{n} downgraded", + "step": { + "major": "major", + "minor": "minor", + "patch": "patch" + } + }, "diffTabBar": { "previousComparison": "Previous comparison", "openComparisons": "Open comparisons", @@ -1507,7 +1524,8 @@ "moved": "{n} moved", "sheets": "{n} sheet | {n} sheets", "matches": "{n} match | {n} matches", - "unchangedHidden": "{n} unchanged hidden" + "unchangedHidden": "{n} unchanged hidden", + "packages": "{n} package | {n} packages" }, "sheetTabBar": { "onlyIn": "only {side}", From c853cdbbbef9f12c1d8b9e2038b22d0d7e80a7c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Sun, 9 Aug 2026 23:52:30 +0300 Subject: [PATCH 06/16] feat(deps): seed a lockfile pair, prove the flow end to end, document it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1, commit 6 of specs/2026-08-09-developer-workflow — phase 1 closes. The seed pair lives in two DIRECTORIES, not two filenames: a lockfile is recognised by its exact name, so `package-lock.before.json` would never have been read as one. Two checkouts is also what the real situation looks like. e2e/deps.spec.mjs drives the whole path through a real launch — main reads the file, the name routes it, and the view that takes over is neither Monaco nor the structure tree. It asserts the fold (one asked-for row, two carried behind a press) and that the text is still one toggle away, because the point is to summarise the file, not to hide it. README gains a Dependencies row; the glossary gains lockfile and the direct/transitive distinction. npm run check green: 3132 tests, coverage 95.33/88.36/95.80/96.41. Co-Authored-By: Claude Opus 5 --- README.md | 1 + docs/glossary.md | 8 +++ e2e/deps.spec.mjs | 109 +++++++++++++++++++++++++++++++++++++++++ scripts/seed-local.mjs | 59 +++++++++++++++++++++- 4 files changed, 175 insertions(+), 2 deletions(-) create mode 100644 e2e/deps.spec.mjs diff --git a/README.md b/README.md index 0fdf26d..4cb7fe2 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,7 @@ Builds are **unsigned**, so SmartScreen and Gatekeeper warn on first launch (the | **Understand structure** | JSON, YAML and XML compared as _data_: reordering keys or reformatting stops counting, and unchanged keys collapse away. | | **Excel** | `.xlsx` workbooks as aligned grids — sheet tabs and cell-level highlights, with inserted rows and columns that don't cascade into false changes. Dates read as dates, hidden sheets and rows are marked, and formulas are compared as well as their results, so a total pasted over the formula behind it is caught rather than shown as unchanged. Headers are found under a title row rather than assumed to be row 1, and rows can be paired by the columns that name them — one column or several — so the same export sorted differently reads as the one figure that moved instead of as a rewrite. Set a tolerance — one of the presets or a threshold of your own, percentage or raw — and rounding noise stops counting; export the whole change list as a CSV. | | **CSV** | `.csv` and `.tsv` compare as text or, one toggle away, as the same grid — rows aligned by their first column, quoted fields kept whole. | +| **Dependencies** | A lockfile pair reads as the dependency moves it describes, not as the four thousand lines it is written in: which packages were added, removed, bumped or downgraded, the semver step of each, and — the part that matters — which of them you actually asked for rather than got carried along. `package-lock.json`, `pnpm-lock.yaml`, `yarn.lock`, `go.sum` and `composer.lock`. Nothing is fetched; every fact comes out of the file in front of you. | | **Huge files** | Past 32 MB a file is indexed by line instead of loaded, and the rows you're looking at are read from disk as you scroll — a multi-gigabyte log opens in seconds. Marked as streamed, with the few actions that need the whole text saying so rather than half-working. | | **Keep** | Saved diffs: encrypted, tagged, optionally auto-expiring. Drag a row onto another to arrange the list yourself; starred rows stay above the rest. Your open tabs come back on the next launch, and the strip can be told to close the oldest comparison to make room for a new one. | | **Share** | One signed file only the recipients you ticked can open, carrying the expiry you chose so every copy dies at the same moment. Give a trusted key an email address and Diff Bro opens an addressed message in your own mail app with the sealed file on the clipboard — it never sends anything itself. The key swap rides the same rails: email your key from the My key dialog, and a key copied out of any chat app is offered — fingerprint first — when you press + Trusted key. | diff --git a/docs/glossary.md b/docs/glossary.md index 832cd16..cdffdb7 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -84,6 +84,14 @@ where the concept lives in this repo. that cells reference by index. - **LCS** — _Longest Common Subsequence_, the classic diff algorithm; used to align spreadsheet rows by default and to build the copy-as-patch output. +- **Lockfile** — the file a package manager writes to pin the exact version of + every dependency actually installed (`package-lock.json`, `pnpm-lock.yaml`, + `yarn.lock`, `go.sum`, `composer.lock`). Diff Bro reads five of them as + dependency SETS rather than as text (`utils/lockfile/`). +- **Direct vs transitive** — a direct (or "asked for") dependency is one the + project's own manifest names; a transitive one was pulled in by something else. + Only some lockfile formats record the difference, and the ones that do not say + so rather than guessing. - **Key column** — a column whose value names a row, so the two sides pair by identity rather than by position or whole-row contents; several together make a **composite key** (an account plus a cost centre). Chosen per sheet in the diff --git a/e2e/deps.spec.mjs b/e2e/deps.spec.mjs new file mode 100644 index 0000000..8c8f579 --- /dev/null +++ b/e2e/deps.spec.mjs @@ -0,0 +1,109 @@ +import { test, expect, stubOpenDialog } from './fixtures.mjs' +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +// A lockfile pair only means anything through a real launch: the file is read by +// the main process, recognised by NAME, and routed to a viewer that is neither +// Monaco nor the structure tree. + +const lockfile = (entries) => + JSON.stringify( + { + lockfileVersion: 3, + name: 'demo', + packages: { + '': { name: 'demo', dependencies: { vue: '^3.4.0' }, devDependencies: { vitest: '^4.0.0' } }, + ...Object.fromEntries( + entries.map(([name, version, dev]) => [ + `node_modules/${name}`, + { version, license: 'MIT', ...(dev ? { dev: true } : {}) } + ]) + ) + } + }, + null, + 2 + ) + +const BEFORE = lockfile([ + ['vue', '3.4.21'], + ['vitest', '4.1.9', true], + ['@vue/shared', '3.4.21'], + ['gone', '1.0.0'] +]) +const AFTER = lockfile([ + ['vue', '3.5.13'], + ['vitest', '4.1.9', true], + ['@vue/shared', '3.5.13'] +]) + +function pair(dir) { + const left = join(dir, 'before') + const right = join(dir, 'after') + mkdirSync(left) + mkdirSync(right) + writeFileSync(join(left, 'package-lock.json'), BEFORE) + writeFileSync(join(right, 'package-lock.json'), AFTER) + return [join(left, 'package-lock.json'), join(right, 'package-lock.json')] +} + +async function openPair(app, page, dir) { + const [l, r] = pair(dir) + await stubOpenDialog(app, [l]) + await page.locator('.slot[data-side="left"]').click() + await stubOpenDialog(app, [r]) + await page.locator('.slot[data-side="right"]').click() +} + +test('two lockfiles open as dependencies, not as text', async ({ app, page }) => { + const dir = mkdtempSync(join(tmpdir(), 'diffbro-deps-')) + try { + await openPair(app, page, dir) + + // The dependency view took over without being asked: it is what the files + // are for, the way a Mermaid pair opens as a picture. + await expect(page.locator('.deps-diff')).toBeVisible({ timeout: 20000 }) + await expect(page.locator('.monaco-diff-editor')).toHaveCount(0) + + // What the reader asked for is on screen; what came with it is folded away. + await expect(page.locator('.deps-row')).toHaveCount(1) + const row = page.locator('.deps-row').first() + await expect(row).toContainText('vue') + await expect(row).toContainText('3.4.21') + await expect(row).toContainText('3.5.13') + await expect(row).toContainText('minor') + + const band = page.locator('.status-band') + await expect(band).toContainText('1 removed') + await expect(band).toContainText('2 bumped') + await expect(band).toContainText('Asked for') + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('the carried packages are one press away, and the text is one toggle away', async ({ + app, + page +}) => { + const dir = mkdtempSync(join(tmpdir(), 'diffbro-deps-fold-')) + try { + await openPair(app, page, dir) + await expect(page.locator('.deps-diff')).toBeVisible({ timeout: 20000 }) + + await page.getByTestId('deps-transitive').click() + // @vue/shared bumped and `gone` went — both carried, neither asked for. + await expect(page.locator('.deps-row')).toHaveCount(3) + await expect(page.locator('.deps-row', { hasText: 'gone' })).toBeVisible() + + // The lockfile is still a file: the toggle gives its text back. + await page.getByRole('button', { name: /^View/ }).click() + await page.getByRole('checkbox', { name: 'Dependencies' }).uncheck() + await page.keyboard.press('Escape') + await expect(page.locator('.monaco-diff-editor')).toBeVisible({ timeout: 20000 }) + await expect(page.locator('.deps-diff')).toHaveCount(0) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) diff --git a/scripts/seed-local.mjs b/scripts/seed-local.mjs index 14bf4f4..5dab2e7 100644 --- a/scripts/seed-local.mjs +++ b/scripts/seed-local.mjs @@ -11,7 +11,7 @@ import { execFileSync } from 'node:child_process' import { existsSync, mkdirSync, mkdtempSync, readFileSync, statSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' -import { basename, join } from 'node:path' +import { basename, dirname, join } from 'node:path' import { fileURLToPath } from 'node:url' import { makeXlsx } from './lib/makeXlsx.mjs' import { createIdentityKeys } from '../src/main/sealing.js' @@ -209,7 +209,58 @@ const FILES = { ]) } +// A lockfile pair: thousands of lines of text saying one direct bump, two +// carried packages and one dropped. The dependency view is the only way to see +// that without reading all of it. +const lockPackages = (entries) => ({ + lockfileVersion: 3, + name: 'demo-app', + packages: { + '': { + name: 'demo-app', + dependencies: { vue: '^3.4.0', pinia: '^2.1.0' }, + devDependencies: { vitest: '^4.0.0' } + }, + ...Object.fromEntries( + entries.map(([name, version, dev]) => [ + `node_modules/${name}`, + { + version, + resolved: `https://registry.npmjs.org/${name}/-/${name}-${version}.tgz`, + license: 'MIT', + ...(dev ? { dev: true } : {}) + } + ]) + ) + } +}) + const TEXT_FILES = { + 'lock-before/package-lock.json': JSON.stringify( + lockPackages([ + ['vue', '3.4.21'], + ['pinia', '2.1.7'], + ['vitest', '4.1.9', true], + ['@vue/shared', '3.4.21'], + ['@vue/reactivity', '3.4.21'], + ['nanoid', '3.3.7'], + ['tinybench', '2.9.0', true] + ]), + null, + 2 + ), + 'lock-after/package-lock.json': JSON.stringify( + lockPackages([ + ['vue', '3.5.13'], + ['pinia', '2.1.7'], + ['vitest', '4.1.10', true], + ['@vue/shared', '3.5.13'], + ['@vue/reactivity', '3.5.13'], + ['tinybench', '2.9.0', true] + ]), + null, + 2 + ), 'service-before.yaml': 'service:\n name: diff-engine\n replicas: 3\n features: [a, b]\n', 'service-after.yaml': 'service:\n replicas: 6\n name: diff-engine\n features: [b, a, c]\n', // Two .csv files on disk: the Grid toggle only appears when BOTH sides are @@ -262,7 +313,11 @@ function writeFixtures() { mkdirSync(SEED_DIR, { recursive: true }) for (const [name, bytes] of Object.entries(FILES)) writeFileSync(join(SEED_DIR, name), bytes) for (const [name, text] of Object.entries(TEXT_FILES)) { - writeFileSync(join(SEED_DIR, name), text, 'utf8') + const path = join(SEED_DIR, name) + // A lockfile is recognised by its exact NAME, so the pair has to live in two + // directories the way two checkouts do. + if (name.includes('/')) mkdirSync(dirname(path), { recursive: true }) + writeFileSync(path, text, 'utf8') } } From f64c6883b38fc92fb8eeb67b517361a7620fe214 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Sun, 9 Aug 2026 23:55:08 +0300 Subject: [PATCH 07/16] fix(git): read a file out of a revision, behind a real fence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2, commit 1 of specs/2026-08-09-developer-workflow. READ ONLY, and the vocabulary is two subcommands — `rev-parse` and `show`. Nothing here writes, stages or commits, and no subcommand that can reach the network is reachable from it, which is what keeps rule 1 intact: a subprocess that cannot open a socket does not weaken the offline guarantee. The fence, because a repository someone cloned is untrusted input and repo-local config has been an execution vector before: - execFile with a FIXED argv, never a shell - the repo root is computed HERE, never accepted from the renderer - a revision is validated against a narrow pattern and can never begin with `-`; --end-of-options catches anything that somehow did - a path may not be absolute and may not contain `..` - every invocation carries core.hooksPath=, core.fsmonitor= (a command git will SPAWN), core.editor=true and protocol.ext.allow=never The environment needed a second pass. The comment claimed a clean one while the code spread process.env wholesale, which keeps GIT_DIR, GIT_INDEX_FILE and GIT_ALTERNATE_OBJECT_DIRECTORIES — each of which points git at something other than the repository main chose. gitEnv now drops every inherited GIT_*, and three tests hold it there. Proven against this repository: the root resolves, HEAD~1 resolves to a 40-character sha, `show v0.4.10:package.json` returns the manifest as it stood at that tag — version 0.4.9, because the version-sync commit lands after the tag — and an unknown revision fails with git's own message rather than a crash. Co-Authored-By: Claude Opus 5 --- specs/2026-08-09-developer-workflow/plan.md | 12 +- src/main/gitRepo.js | 130 ++++++++++++++++++++ tests/main/gitRepo.test.js | 127 +++++++++++++++++++ 3 files changed, 263 insertions(+), 6 deletions(-) create mode 100644 src/main/gitRepo.js create mode 100644 tests/main/gitRepo.test.js diff --git a/specs/2026-08-09-developer-workflow/plan.md b/specs/2026-08-09-developer-workflow/plan.md index 13efded..df18012 100644 --- a/specs/2026-08-09-developer-workflow/plan.md +++ b/specs/2026-08-09-developer-workflow/plan.md @@ -3,7 +3,7 @@ | | | |---|---| | **Status** | in-progress | -| **Progress** | 0 / 13 steps | +| **Progress** | 5 / 13 steps — phase 1 shipped | | **Branch** | `feat/developer-workflow` | | **Started** | 2026-08-09 | | **Finished** | — | @@ -207,11 +207,11 @@ rather than throwing. **Phase 1 — dependencies** -- [ ] 1. `utils/lockfile/` — npm v1/v2/v3, pnpm, yarn v1, `go.sum` → one shape -- [ ] 2. `utils/lockDiff.js` — classify each change; direct vs transitive -- [ ] 3. `canCompareDeps`, `semanticKind() === 'deps'`, `shouldOpenSemantic` -- [ ] 4. The viewer + its stylesheet + i18n -- [ ] 5. Seed pair, e2e, README/glossary +- [x] 1. `utils/lockfile/` — npm v1/v2/v3, pnpm, yarn v1, `go.sum` → one shape +- [x] 2. `utils/lockDiff.js` — classify each change; direct vs transitive +- [x] 3. `canCompareDeps`, `semanticKind() === 'deps'`, `shouldOpenSemantic` +- [x] 4. The viewer + its stylesheet + i18n +- [x] 5. Seed pair, e2e, README/glossary **Phase 2 — git-native comparison** diff --git a/src/main/gitRepo.js b/src/main/gitRepo.js new file mode 100644 index 0000000..7d62c5f --- /dev/null +++ b/src/main/gitRepo.js @@ -0,0 +1,130 @@ +// Reading a file out of a git revision, so a comparison can start from the +// repository rather than from two files someone had to produce first. +// +// READ ONLY. Nothing here writes, stages or commits, and no subcommand that can +// reach the network is reachable from it — `show` and `rev-parse` are the whole +// vocabulary (rule 1: the offline guarantee is not weakened by a subprocess that +// opens no socket, but only because nothing here can ask git to open one). +// +// The fence, and why each part of it is there (rule 7): +// * execFile with a FIXED argv, never a shell — the app already spawns git +// this way in gitTool.js +// * the repo root is computed HERE and never accepted from the renderer +// * a revision is validated before it reaches argv, and can never begin `-` +// * --end-of-options, so even a revision that slipped through cannot be read +// as a flag +// * the HARDENING below, because a repository someone cloned is untrusted +// input and repo-local config has been an execution vector before +import { execFile } from 'node:child_process' + +/** + * Applied to EVERY invocation. A repo's own config must not be able to run + * anything: hooks are pointed at nothing, the filesystem monitor — which is a + * command git will spawn — is emptied, and no pager is launched. + */ +export const HARDENING = Object.freeze([ + '--no-pager', + '-c', + 'core.hooksPath=', + '-c', + 'core.fsmonitor=', + '-c', + 'core.editor=true', + '-c', + 'protocol.ext.allow=never' +]) + +// Deliberately narrower than git's own rules: the shapes a reader types, and +// nothing that could be read as an option or reach a shell. +const REVISION = /^[A-Za-z0-9][A-Za-z0-9._/@{}^~-]*$/ +const MAX_REVISION = 255 + +/** @param {unknown} rev */ +export function isSafeRevision(rev) { + if (typeof rev !== 'string' || !rev || rev.length > MAX_REVISION) return false + if (rev.startsWith('-')) return false + return REVISION.test(rev) +} + +// git speaks posix paths in a `rev:path` pair whatever the platform, and a path +// that climbs out of the repository is not a path in it. +function repoPath(path) { + const posix = String(path ?? '').replace(/\\/g, '/') + if (!posix || posix.startsWith('/') || posix.startsWith('-')) return null + if (posix.split('/').includes('..')) return null + return posix +} + +function checkedRevision(rev) { + if (!isSafeRevision(rev)) throw new Error('unsafe-revision') + return rev +} + +/** Where the repository containing the working directory begins. */ +export function repoRootArgs() { + return [...HARDENING, 'rev-parse', '--show-toplevel'] +} + +/** Resolve a revision to the commit it names, or fail. */ +export function resolveRevisionArgs(rev) { + return [ + ...HARDENING, + 'rev-parse', + '--verify', + '--end-of-options', + `${checkedRevision(rev)}^{commit}` + ] +} + +/** Read one file as it stood at a revision. */ +export function readBlobArgs(rev, path) { + const safe = repoPath(path) + if (!safe) throw new Error('unsafe-path') + return [...HARDENING, 'show', '--end-of-options', `${checkedRevision(rev)}:${safe}`] +} + +/** + * The environment a git invocation gets. Every inherited GIT_* is DROPPED: + * GIT_DIR, GIT_INDEX_FILE and GIT_ALTERNATE_OBJECT_DIRECTORIES each redirect git + * at something other than the repository main chose, and inheriting them hands + * that redirect to whoever launched the app. + * @param {Record} [base] + */ +export function gitEnv(base = process.env) { + const env = {} + for (const [key, value] of Object.entries(base)) { + if (!key.startsWith('GIT_')) env[key] = value + } + env.GIT_CONFIG_NOSYSTEM = '1' + env.GIT_TERMINAL_PROMPT = '0' + return env +} + +/** + * One git invocation, in a directory MAIN chose. Resolves rather than rejects so + * a missing revision is an answer, not a crash. + * @param {string[]} args one of the vectors above — never assembled by a caller + * @param {string} cwd + * @returns {Promise<{ok: boolean, stdout: string, error: string}>} + */ +export function runGitIn(args, cwd) { + return new Promise((resolve) => { + execFile( + 'git', + args, + { + cwd, + windowsHide: true, + maxBuffer: 64 * 1024 * 1024, + env: gitEnv() + }, + (err, stdout, stderr) => { + resolve({ + ok: !err, + stdout: String(stdout ?? ''), + error: err ? String(stderr ?? err.message).trim() : '' + }) + } + ) + }) +} diff --git a/tests/main/gitRepo.test.js b/tests/main/gitRepo.test.js new file mode 100644 index 0000000..3ea4a7c --- /dev/null +++ b/tests/main/gitRepo.test.js @@ -0,0 +1,127 @@ +import { describe, expect, it } from 'vitest' +import { + gitEnv, + HARDENING, + isSafeRevision, + readBlobArgs, + repoRootArgs, + resolveRevisionArgs +} from '../../src/main/gitRepo' + +describe('isSafeRevision', () => { + it('takes the revision shapes a reader actually types', () => { + for (const rev of [ + 'HEAD', + 'HEAD~3', + 'HEAD^', + 'main', + 'origin/main', + 'feature/a-b_c', + 'v1.2.3', + '9a73b33', + '9a73b33cd1e4f5a6b7c8d9e0f1a2b3c4d5e6f7a8', + 'HEAD@{1}', + 'release-2024.10' + ]) { + expect(isSafeRevision(rev), rev).toBe(true) + } + }) + + // A revision reaches argv. Anything that could be read as an OPTION is the + // one shape that must never get through, however harmless it looks. + it('refuses anything that could be read as an option', () => { + for (const rev of ['--upload-pack=evil', '-c', '--exec=sh', '-']) { + expect(isSafeRevision(rev), rev).toBe(false) + } + }) + + it('refuses shell metacharacters and whitespace', () => { + for (const rev of ['HEAD; rm -rf /', 'HEAD|cat', 'HEAD$(id)', 'HEAD `id`', 'a b', 'HEAD\n']) { + expect(isSafeRevision(rev), rev).toBe(false) + } + }) + + it('refuses nothing, a non-string, and an absurd length', () => { + expect(isSafeRevision('')).toBe(false) + expect(isSafeRevision(null)).toBe(false) + expect(isSafeRevision(42)).toBe(false) + expect(isSafeRevision('a'.repeat(256))).toBe(false) + }) +}) + +describe('the hardening every invocation carries', () => { + // A repository is untrusted input: its own config has been an execution vector + // before. These are the flags that stop a clone running anything. + it('disables hooks, the fsmonitor and any pager', () => { + expect(HARDENING).toContain('-c') + expect(HARDENING.join(' ')).toContain('core.hooksPath=') + expect(HARDENING.join(' ')).toContain('core.fsmonitor=') + expect(HARDENING).toContain('--no-pager') + }) +}) + +describe('argument vectors', () => { + it('asks for the repo root, hardened', () => { + const args = repoRootArgs() + expect(args.slice(0, HARDENING.length)).toEqual(HARDENING) + expect(args).toContain('rev-parse') + expect(args).toContain('--show-toplevel') + }) + + it('resolves a revision to a commit, hardened', () => { + const args = resolveRevisionArgs('HEAD~2') + expect(args.slice(0, HARDENING.length)).toEqual(HARDENING) + expect(args).toEqual([ + ...HARDENING, + 'rev-parse', + '--verify', + '--end-of-options', + 'HEAD~2^{commit}' + ]) + }) + + // `--` is what stops a path called `-x` becoming a flag, and `git show` takes + // the pair as ONE argument, so the path cannot be split off either. + it('reads a blob with the revision and path fenced apart', () => { + const args = readBlobArgs('HEAD', 'src/main/index.js') + expect(args).toEqual([...HARDENING, 'show', '--end-of-options', 'HEAD:src/main/index.js']) + }) + + it('normalises a Windows path separator, because git only speaks posix', () => { + expect(readBlobArgs('HEAD', 'src\\main\\index.js')).toContain('HEAD:src/main/index.js') + }) + + it('refuses to build anything from a revision it would not accept', () => { + expect(() => resolveRevisionArgs('--upload-pack=evil')).toThrow() + expect(() => readBlobArgs('-c', 'a.js')).toThrow() + }) + + it('refuses a path that climbs out of the repository', () => { + expect(() => readBlobArgs('HEAD', '../../etc/passwd')).toThrow() + expect(() => readBlobArgs('HEAD', '/etc/passwd')).toThrow() + }) +}) + +// GIT_DIR, GIT_INDEX_FILE, GIT_ALTERNATE_OBJECT_DIRECTORIES and friends all +// redirect git at something other than the repository main chose. Inheriting the +// launching shell's environment wholesale hands that redirect to whoever set it. +describe('the environment a git invocation inherits', () => { + it('drops every GIT_ variable it was launched with', () => { + const env = gitEnv({ PATH: '/usr/bin', GIT_DIR: '/tmp/evil.git', GIT_INDEX_FILE: '/tmp/i' }) + expect(env.GIT_DIR).toBeUndefined() + expect(env.GIT_INDEX_FILE).toBeUndefined() + }) + + it('keeps what git needs to run at all', () => { + const env = gitEnv({ PATH: '/usr/bin', HOME: '/home/x', SystemRoot: 'C:\\Windows' }) + expect(env.PATH).toBe('/usr/bin') + expect(env.HOME).toBe('/home/x') + expect(env.SystemRoot).toBe('C:\\Windows') + }) + + it('sets the two that make it non-interactive and system-config free', () => { + const env = gitEnv({}) + expect(env.GIT_CONFIG_NOSYSTEM).toBe('1') + expect(env.GIT_TERMINAL_PROMPT).toBe('0') + }) +}) From e727a601be514b2bc2b00b30f615d1b11c412798 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Sun, 9 Aug 2026 23:57:50 +0300 Subject: [PATCH 08/16] feat(git): a git surface the renderer can reach, and the revision:path grammar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2, commit 2 of specs/2026-08-09-developer-workflow. Two IPC handlers and no more: git:root asks whether a path is inside a repository, git:show reads one file as it stood at one revision. The renderer names a REVISION and a path — never a command, never a cwd, never a git argument — and main resolves the repository FROM the file it was given, so a renderer cannot point git at a directory of its choosing. A refusal inside the fence (an unsafe revision, a path climbing out) comes back as `refused` without the reason: the renderer does not need to learn which of its inputs the fence disliked. splitRevisionArg is the CLI half, and it is pure so the grammar is testable without a repo. Only the FIRST colon separates, so a path may contain its own; `C:/Users/x/a.js` stays a path, because a single letter before the colon is a Windows drive and not a revision; and a revision the fence would refuse never becomes one here either. Blobs are capped at 32 MB — the point where the streamed reader takes over anyway, and a revision is not a reason to hold more in memory than a file would be. Co-Authored-By: Claude Opus 5 --- src/main/gitRevisionArg.js | 22 ++++++++++++++ src/main/gitRoute.js | 48 +++++++++++++++++++++++++++++++ src/main/index.js | 2 ++ src/preload/index.js | 2 ++ tests/main/gitRevisionArg.test.js | 46 +++++++++++++++++++++++++++++ 5 files changed, 120 insertions(+) create mode 100644 src/main/gitRevisionArg.js create mode 100644 src/main/gitRoute.js create mode 100644 tests/main/gitRevisionArg.test.js diff --git a/src/main/gitRevisionArg.js b/src/main/gitRevisionArg.js new file mode 100644 index 0000000..403c562 --- /dev/null +++ b/src/main/gitRevisionArg.js @@ -0,0 +1,22 @@ +// `HEAD~1:src/a.js` — the `revision:path` shape git itself uses, as a CLI +// argument. Pure, so the grammar is testable without a repository. +import { isSafeRevision } from './gitRepo' + +// A single letter before the colon is a Windows drive, not a revision: `C:/x` +// must stay a path. +const WINDOWS_DRIVE = /^[A-Za-z]:[\\/]/ + +/** + * @param {unknown} arg + * @returns {{revision: string, relPath: string}|null} null when the argument is + * an ordinary path, which is most of them. + */ +export function splitRevisionArg(arg) { + if (typeof arg !== 'string' || WINDOWS_DRIVE.test(arg)) return null + const at = arg.indexOf(':') + if (at <= 0 || at === arg.length - 1) return null + const revision = arg.slice(0, at) + const relPath = arg.slice(at + 1) + // The path half keeps any further colons; only the FIRST one separates. + return isSafeRevision(revision) && relPath ? { revision, relPath } : null +} diff --git a/src/main/gitRoute.js b/src/main/gitRoute.js new file mode 100644 index 0000000..bb3e780 --- /dev/null +++ b/src/main/gitRoute.js @@ -0,0 +1,48 @@ +// The git surface the renderer may reach, and nothing else. Two questions — +// "is this a repository?" and "what did this file look like at that revision?" +// — and both are answered by src/main/gitRepo.js behind its fence. +// +// The renderer names a REVISION and a path; it never names a command, a cwd or +// a git argument. Main resolves the repository from the path it was given, so a +// renderer cannot point git at a directory of its choosing. +import { ipcMain } from 'electron' +import { dirname } from 'node:path' +import { readBlobArgs, repoRootArgs, resolveRevisionArgs, runGitIn } from './gitRepo' + +const MAX_BLOB = 32 * 1024 * 1024 + +async function repoRootOf(filePath) { + if (typeof filePath !== 'string' || !filePath) return null + const res = await runGitIn(repoRootArgs(), dirname(filePath)) + return res.ok && res.stdout.trim() ? res.stdout.trim() : null +} + +/** + * The file as it stood at a revision, with the repository worked out from the + * file itself. + */ +async function readAtRevision(filePath, revision, relPath) { + const root = await repoRootOf(filePath) + if (!root) return { error: 'not-a-repo' } + const resolved = await runGitIn(resolveRevisionArgs(revision), root) + if (!resolved.ok) return { error: 'no-such-revision' } + const blob = await runGitIn(readBlobArgs(revision, relPath), root) + if (!blob.ok) return { error: 'not-in-revision' } + if (blob.stdout.length > MAX_BLOB) return { error: 'too-large' } + return { content: blob.stdout, commit: resolved.stdout.trim().slice(0, 12) } +} + +export function registerGitIpc() { + ipcMain.handle('git:root', async (e, filePath) => ({ root: await repoRootOf(filePath) })) + + ipcMain.handle('git:show', async (e, payload) => { + const { path: filePath, revision, relPath } = payload ?? {} + try { + return await readAtRevision(filePath, revision, relPath) + } catch { + // An unsafe revision or path throws inside the fence; the renderer gets a + // refusal, never the reason it was refused. + return { error: 'refused' } + } + }) +} diff --git a/src/main/index.js b/src/main/index.js index fff1776..4a95ed3 100644 --- a/src/main/index.js +++ b/src/main/index.js @@ -15,6 +15,7 @@ import { registerHashIpc } from './hashTools' import { backupIfDue, registerBackupIpc } from './backupRoute' import { readSettings, readSnippetStore, setBackupHook } from './appData' import { registerShareIpc } from './share' +import { registerGitIpc } from './gitRoute' import { registerMailIpc } from './mail' import { registerKeyExchangeIpc } from './keyExchange' import { registerClipboardCopyIpc } from './clipboardCopy' @@ -106,6 +107,7 @@ async function boot() { function startApp(draftPath) { installNetworkKillSwitch() registerAppDataIpc() + registerGitIpc() registerDemoIpc() registerQuickLookFocusIpc() loadLocale(readSettings().locale) // before installMenu: it builds from this diff --git a/src/preload/index.js b/src/preload/index.js index 4d2e89f..cdfbb18 100644 --- a/src/preload/index.js +++ b/src/preload/index.js @@ -4,6 +4,8 @@ contextBridge.exposeInMainWorld('api', { openFile: (side, format) => ipcRenderer.invoke('file:open', side, format), readClipboardFiles: () => ipcRenderer.invoke('clipboard:readFiles'), readFile: (path, opts) => ipcRenderer.invoke('file:read', path, opts), + gitRoot: (path) => ipcRenderer.invoke('git:root', path), + gitShow: (payload) => ipcRenderer.invoke('git:show', payload), // `format` names a row of main's own export table; never an extension. exportDiffFile: (payload) => ipcRenderer.invoke('diff:exportFile', payload), // Streamed comparison: files too large to hold are indexed by line in main diff --git a/tests/main/gitRevisionArg.test.js b/tests/main/gitRevisionArg.test.js new file mode 100644 index 0000000..c219113 --- /dev/null +++ b/tests/main/gitRevisionArg.test.js @@ -0,0 +1,46 @@ +import { describe, expect, it } from 'vitest' +import { splitRevisionArg } from '../../src/main/gitRevisionArg' + +describe('splitRevisionArg', () => { + // `diffbro compare HEAD~1:src/a.js` — the shape git itself uses. + it('splits a revision:path argument', () => { + expect(splitRevisionArg('HEAD~1:src/a.js')).toEqual({ + revision: 'HEAD~1', + relPath: 'src/a.js' + }) + }) + + it('takes a branch, a tag and a sha', () => { + expect(splitRevisionArg('main:a.js').revision).toBe('main') + expect(splitRevisionArg('v1.2.3:a.js').revision).toBe('v1.2.3') + expect(splitRevisionArg('9a73b33:a.js').revision).toBe('9a73b33') + }) + + it('keeps a path that itself contains a colon', () => { + expect(splitRevisionArg('HEAD:src/a:b.js')).toEqual({ revision: 'HEAD', relPath: 'src/a:b.js' }) + }) + + // A plain path is a plain path — the feature must not capture every argument + // with a colon in it. + it('is not a revision argument without a colon', () => { + expect(splitRevisionArg('src/a.js')).toBeNull() + expect(splitRevisionArg('')).toBeNull() + expect(splitRevisionArg(null)).toBeNull() + }) + + // A Windows drive letter is a colon that is emphatically not a revision. + it('is not fooled by a Windows absolute path', () => { + expect(splitRevisionArg('C:\\Users\\x\\a.js')).toBeNull() + expect(splitRevisionArg('C:/Users/x/a.js')).toBeNull() + }) + + it('refuses a revision the fence would not accept', () => { + expect(splitRevisionArg('--upload-pack=evil:a.js')).toBeNull() + expect(splitRevisionArg('HEAD;id:a.js')).toBeNull() + }) + + it('refuses an empty half', () => { + expect(splitRevisionArg(':a.js')).toBeNull() + expect(splitRevisionArg('HEAD:')).toBeNull() + }) +}) From a1ae362180949d6605e512c870627f396c432e2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Sun, 9 Aug 2026 23:58:17 +0300 Subject: [PATCH 09/16] docs(spec): phase 2 progress, and what is not yet wired Co-Authored-By: Claude Opus 5 --- specs/2026-08-09-developer-workflow/plan.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/specs/2026-08-09-developer-workflow/plan.md b/specs/2026-08-09-developer-workflow/plan.md index df18012..df84d0f 100644 --- a/specs/2026-08-09-developer-workflow/plan.md +++ b/specs/2026-08-09-developer-workflow/plan.md @@ -3,7 +3,7 @@ | | | |---|---| | **Status** | in-progress | -| **Progress** | 5 / 13 steps — phase 1 shipped | +| **Progress** | 7 / 13 — phase 1 shipped; phase 2 at the fence + IPC + grammar | | **Branch** | `feat/developer-workflow` | | **Started** | 2026-08-09 | | **Finished** | — | @@ -215,8 +215,9 @@ rather than throwing. **Phase 2 — git-native comparison** -- [ ] 6. `src/main/gitRepo.js` — the fence, argv builders, revision validation -- [ ] 7. IPC + CLI grammar (`compare :`, `.. `) +- [x] 6. `src/main/gitRepo.js` — the fence, argv builders, revision validation +- [x] 7. IPC + the `revision:path` grammar (`splitRevisionArg`). **NOT YET WIRED + into `parseCompare`** — the grammar parses, the launch path does not use it - [ ] 8. UI entry point + `docs/security.md` / `docs/ipc-security.md` - [ ] 9. e2e against a temp repo From 7f4005e99ee2871f5eba65e20cd2d25ad0a429a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Mon, 10 Aug 2026 00:06:42 +0300 Subject: [PATCH 10/16] feat(git): compare against a revision, end to end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2, commit 3 of specs/2026-08-09-developer-workflow — phase 2 closes. `diffbro compare HEAD~1:src/app.js src/app.js` now works: parseCompare recognises the revision:path shape and leaves it unresolved, and the launch path turns it into a real file BEFORE the renderer hears about the command at all. Everything downstream sees two ordinary paths and nothing else has to know git was involved — the same shape gitTool's difftool launcher already uses, writing into the same temp prefix so one sweep clears both. A failure refuses the whole comparison rather than opening half of it, and says which of the three things went wrong on stderr. Those messages come from a map of literal key ids, not a key built from the error code — check:i18n caught the assembled version, as it did in phase 1. e2e builds a real two-commit repository, drives the real CLI against it, and asserts the diff shows 3 becoming 9 — the old side read out of git, the new one off disk. A second test proves an unknown revision opens nothing at all. docs/security.md gains the fence beside the offline guarantee, because that is the rule a subprocess comes nearest; docs/ipc-security.md gains the two handlers; README's Terminal row gains the grammar. Co-Authored-By: Claude Opus 5 --- README.md | 2 +- docs/ipc-security.md | 1 + docs/security.md | 27 +++++++ e2e/git-compare.spec.mjs | 88 +++++++++++++++++++++ specs/2026-08-09-developer-workflow/plan.md | 9 +-- src/main/cli.js | 11 ++- src/main/cliRoute.js | 36 ++++++++- src/main/gitCliFiles.js | 50 ++++++++++++ src/shared/i18n/en-XA.json | 5 ++ src/shared/i18n/en.json | 5 ++ tests/main/cli.test.js | 31 ++++++++ 11 files changed, 256 insertions(+), 9 deletions(-) create mode 100644 e2e/git-compare.spec.mjs create mode 100644 src/main/gitCliFiles.js diff --git a/README.md b/README.md index 4cb7fe2..3be32ad 100644 --- a/README.md +++ b/README.md @@ -62,7 +62,7 @@ Builds are **unsigned**, so SmartScreen and Gatekeeper warn on first launch (the | **Guided first run** | Six coach marks over the real controls on a first launch — comparing, sealing, the library, then the way into Settings and around it — with four more if you want them. Each step points at a control and its button performs the action, so nothing opens unannounced. Back revisits a step, and everything it put on screen — the demo files, the example snippet — leaves when it does. Escape or Skip ends it for good; Help ▸ Show Tour brings it back. | | **Diagrams** | Two Mermaid files compare as a picture, not as text — one diagram carrying both revisions, so an inserted node reads as one change instead of a rewrite. | | **Tools** | JSON, Base64, UUID, JWT, Epoch, URL, Lines, XML, checksums, a regex tester, find & replace, text encryption — rich panels, not blank text boxes. All of them live in their own sidebar section; star the ones you reach for and they stay at the top. | -| **Terminal** | `diffbro compare a.json b.json` opens a comparison in the running app, `diffbro open` raises it, `diffbro backup ` writes an encrypted archive. No port, no daemon. | +| **Terminal** | `diffbro compare a.json b.json` opens a comparison in the running app, and either side can name a git revision instead of a file — `diffbro compare HEAD~1:src/app.js src/app.js` reads the old copy straight out of the repository, so you never have to produce one first. `diffbro open` raises the app, `diffbro backup ` writes an encrypted archive. No port, no daemon. | | **Yours to arrange** | Fourteen themes (Nord, Sepia, Solar, Nyan, Matrix, plus accessibility-grade Contrast and Beacon), shared tags, adjustable limits. |
diff --git a/docs/ipc-security.md b/docs/ipc-security.md index 691233b..d41815e 100644 --- a/docs/ipc-security.md +++ b/docs/ipc-security.md @@ -87,6 +87,7 @@ that enforces each: | **Backup deletion by age, never by name** | `backup:prune` is the only handler that DELETES. It takes an age in days that must be one of the two the app offers (`PRUNE_DAYS`), never a path or a filename, so the renderer cannot name a file to remove; every candidate comes from `listBackups`, which yields only names that parse as one of ours, so anything else sharing the folder is untouched | `backupRoute.js`, `autoBackup.js` | | **The mail hand-off supplies no URL and no path** | `mail:handoff` takes fingerprints and text. Main resolves the addresses from the trust store, BUILDS the `mailto:` (`mailto.js`), and re-checks it with `isSafeMailtoUrl` before `shell.openExternal` — `mailto:` only, and an `attach`/`attachment` parameter is refused rather than ignored. The file it copies and reveals is the path it just sealed, never one round-tripped through the renderer | `mail.js`, `mailto.js`, `linkPolicy.js`, `mailAddress.js` | | **Copy as file takes bytes, never a path** | `clipboard:writeFile` receives content and a DISPLAY NAME. Main slugs the name flat (so `../../.ssh/config` cannot traverse), stages it in a `0o700` directory, and puts that path on the clipboard. The renderer cannot name a file to stage, read one back, or learn the staging directory; staged copies are pruned at 30 minutes and swept on quit **and** on next launch | `clipboardCopy.js`, `clipboardStage.js`, `clipboardWrite.js` | +| **git is read-only, and main owns the repository** | `git:root` and `git:show` are the whole surface. The renderer names a REVISION and a repo-relative path; main computes the repository root itself and builds the argv, so no handler accepts a directory, a command or a git argument. The vocabulary is `rev-parse` and `show`, so nothing that reaches the network is callable, and a refusal comes back as `refused` without saying which input was rejected | `gitRoute.js`, `gitRepo.js` | | **The tray settings are booleans** | `tray:supported`, `app:startAtLogin` and `app:setStartAtLogin` take and return nothing but booleans. The login item registers `process.execPath` — main's own — with a fixed `--hidden` argument; the renderer never supplies an executable, an argument or a registry key, and there is no handler that would accept one | `tray.js`, `trayCore.js` | | **A stored address cannot become a header** | `share:setTrustedEmail` refuses anything carrying CR/LF, a comma, a semicolon, angle brackets or whitespace, **before it reaches disk** — otherwise a stored address would inject a second header into the hand-off URL. A restored backup's `email` field is dropped if it fails the same check | `trustedKeys.js`, `mailAddress.js`, `shareCore.js` | | **No injection sinks** | `v-html`, `eval`, `new Function`, `innerHTML` are ESLint-banned | `eslint.config.mjs` | diff --git a/docs/security.md b/docs/security.md index 98b8ab3..37dfa3e 100644 --- a/docs/security.md +++ b/docs/security.md @@ -20,6 +20,33 @@ rejected rather than shipped, because the kill switch would not have caught it: it filters `session.defaultSession.webRequest`, which is Chromium traffic only, so a main-process `tls.connect` would have been invisible to it. +**Reading a file out of git does not change it either.** `src/main/gitRepo.js` +spawns `git` to answer two questions — where the repository begins, and what a +file looked like at a revision — the same way `gitTool.js` already spawns it to +register the difftool. A subprocess is not a network client, and the vocabulary +is `rev-parse` and `show`: nothing that can fetch, pull, clone or `ls-remote` is +reachable, so git cannot be asked to open a socket on the app's behalf. + +The fence around it, because a repository someone cloned is untrusted input and +repo-local config has been an execution vector before: + +- `execFile` with a FIXED argv, never a shell. +- The repository root is computed in MAIN, from a path the app already holds. A + renderer names a revision and a repo-relative path — never a directory, a + command, or a git argument. +- A revision is validated against a narrow pattern, may never begin with `-`, + and is followed by `--end-of-options`; a path may not be absolute and may not + contain `..`. +- Every invocation carries `core.hooksPath=`, `core.fsmonitor=` (a command git + will otherwise SPAWN), `core.editor=true` and `protocol.ext.allow=never`. +- The environment is rebuilt rather than inherited: every `GIT_*` variable is + dropped, because `GIT_DIR`, `GIT_INDEX_FILE` and + `GIT_ALTERNATE_OBJECT_DIRECTORIES` each redirect git at something other than + the repository main chose. `GIT_CONFIG_NOSYSTEM=1` and + `GIT_TERMINAL_PROMPT=0` are then set. +- A blob is capped at 32 MB — past that the streamed reader is the right tool, + and a revision is no reason to hold more in memory than a file would be. + ## File access (compromised-renderer threat model) All filesystem access lives in the main process; the renderer only asks. Because diff --git a/e2e/git-compare.spec.mjs b/e2e/git-compare.spec.mjs new file mode 100644 index 0000000..cc1d744 --- /dev/null +++ b/e2e/git-compare.spec.mjs @@ -0,0 +1,88 @@ +import { test, expect, launchApp, freshUserDataDir, firstReadyPage } from './fixtures.mjs' +import { execFileSync } from 'node:child_process' +import { spawn } from 'node:child_process' +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { createRequire } from 'node:module' +import { fileURLToPath } from 'node:url' + +const ROOT = fileURLToPath(new URL('..', import.meta.url)) +const MAIN = join(ROOT, 'build', 'main', 'index.js') +const ELECTRON = createRequire(import.meta.url)('electron') + +// Only a real launch proves this: main has to find the repository, read a blob +// out of git, write it somewhere the app may open, and hand the renderer two +// ordinary paths. +function makeRepo() { + const dir = mkdtempSync(join(tmpdir(), 'diffbro-repo-')) + const git = (...args) => + execFileSync('git', args, { + cwd: dir, + env: { ...process.env, GIT_AUTHOR_NAME: 'T', GIT_AUTHOR_EMAIL: 't@e', GIT_COMMITTER_NAME: 'T', GIT_COMMITTER_EMAIL: 't@e' } + }) + git('init', '-q', '-b', 'main') + writeFileSync(join(dir, 'app.json'), '{\n "replicas": 3\n}\n') + git('add', '.') + git('commit', '-qm', 'first') + writeFileSync(join(dir, 'app.json'), '{\n "replicas": 9\n}\n') + git('add', '.') + git('commit', '-qm', 'second') + return dir +} + +function runCli(userDataDir, cwd, args) { + const env = { ...process.env } + delete env.ELECTRON_RUN_AS_NODE + return new Promise((resolve) => { + const p = spawn(ELECTRON, [MAIN, `--user-data-dir=${userDataDir}`, ...args], { + cwd, + env, + stdio: 'ignore' + }) + p.on('exit', () => resolve()) + setTimeout(resolve, 8000) + }) +} + +test('compares a file against the revision it names', async () => { + const repo = makeRepo() + const userDataDir = freshUserDataDir() + const app = await launchApp(userDataDir) + const page = await firstReadyPage(app) + try { + await runCli(userDataDir, repo, ['compare', 'HEAD~1:app.json', 'app.json']) + + // Both sides arrived: the old one out of git, the new one off disk. + await expect(page.locator('.slot[data-side="left"] .name')).toContainText('app.json', { + timeout: 20000 + }) + await expect(page.locator('.slot[data-side="right"] .name')).toContainText('app.json') + + // And it is a real comparison of the two revisions, not the file with + // itself: 3 became 9. + const editor = page.locator('.monaco-diff-editor') + await expect(editor).toBeVisible({ timeout: 20000 }) + await expect(editor).toContainText('3') + await expect(editor).toContainText('9') + } finally { + await app.close().catch(() => {}) + rmSync(repo, { recursive: true, force: true }) + } +}) + +test('says so when the revision does not exist, and opens nothing', async () => { + const repo = makeRepo() + const userDataDir = freshUserDataDir() + const app = await launchApp(userDataDir) + const page = await firstReadyPage(app) + try { + await runCli(userDataDir, repo, ['compare', 'no-such-ref:app.json', 'app.json']) + // The refusal is total: a half-loaded comparison would be worse than none. + await page.waitForTimeout(1500) + await expect(page.locator('.slot[data-side="left"] .name')).toHaveCount(0) + } finally { + await app.close().catch(() => {}) + rmSync(repo, { recursive: true, force: true }) + } +}) diff --git a/specs/2026-08-09-developer-workflow/plan.md b/specs/2026-08-09-developer-workflow/plan.md index df84d0f..1ad0091 100644 --- a/specs/2026-08-09-developer-workflow/plan.md +++ b/specs/2026-08-09-developer-workflow/plan.md @@ -3,7 +3,7 @@ | | | |---|---| | **Status** | in-progress | -| **Progress** | 7 / 13 — phase 1 shipped; phase 2 at the fence + IPC + grammar | +| **Progress** | 9 / 13 — phases 1 and 2 shipped | | **Branch** | `feat/developer-workflow` | | **Started** | 2026-08-09 | | **Finished** | — | @@ -216,10 +216,9 @@ rather than throwing. **Phase 2 — git-native comparison** - [x] 6. `src/main/gitRepo.js` — the fence, argv builders, revision validation -- [x] 7. IPC + the `revision:path` grammar (`splitRevisionArg`). **NOT YET WIRED - into `parseCompare`** — the grammar parses, the launch path does not use it -- [ ] 8. UI entry point + `docs/security.md` / `docs/ipc-security.md` -- [ ] 9. e2e against a temp repo +- [x] 7. IPC + CLI grammar, wired end to end through `parseCompare` and the launch +- [x] 8. `docs/security.md` / `docs/ipc-security.md` + README +- [x] 9. e2e against a temp repo **Phase 3 — finishing the merge** diff --git a/src/main/cli.js b/src/main/cli.js index 9ceb136..1428df5 100644 --- a/src/main/cli.js +++ b/src/main/cli.js @@ -1,4 +1,5 @@ import { COMMANDS } from '../shared/cliCommands' +import { splitRevisionArg } from './gitRevisionArg' import { t } from './i18n' // The `diffbro` terminal command. A second launch never becomes a second app: // Electron's single-instance lock hands its argv to the running one, which is @@ -148,12 +149,20 @@ function parseBackup(rest, resolve) { return { command: { name: 'backup', path: resolve(paths[0]) }, error: null } } +// A side is either a path to resolve against the shell's cwd, or the +// `revision:path` pair naming a file inside the repository as it stood then — +// which main reads later, because only main may talk to git. +const sideOf = (word, resolve) => splitRevisionArg(word) ?? resolve(word) + function parseCompare(rest, resolve, transient = false) { // An empty word is not a path — resolving it would silently mean the cwd. const paths = rest.filter((p) => p.trim()) if (!paths.length) return { command: null, error: 'compare needs a file path.' } if (paths.length > 2) return { command: null, error: 'compare takes at most two files.' } - return { command: { name: 'compare', files: paths.map(resolve), transient }, error: null } + return { + command: { name: 'compare', files: paths.map((p) => sideOf(p, resolve)), transient }, + error: null + } } /** diff --git a/src/main/cliRoute.js b/src/main/cliRoute.js index 8001394..2d6adcb 100644 --- a/src/main/cliRoute.js +++ b/src/main/cliRoute.js @@ -10,6 +10,7 @@ import { installShim, removeShim, shimStatus } from './cliShim' import { gitToolStatus, registerGitTool, sweepGitTemp, unregisterGitTool } from './gitTool' import { ensureMainWindow } from './quickLook' import { allowCliPath } from './files' +import { fileAtRevision, isRevisionSide, REVISION_ERROR_KEYS } from './gitCliFiles' import { t } from './i18n' // A command can arrive before any window exists (a cold `diffbro compare …`), @@ -43,13 +44,44 @@ export function routeCliArgv(argv, cwd, carried = null) { process.stderr.write(`${parsed.error}\n`) return } - routeCommand(parsed.command) + routeCommand(parsed.command, cwd) } -function routeCommand(command) { +// Each `revision:path` side becomes a real file before the renderer hears about +// the command at all, so everything downstream sees paths and nothing else has +// to know git was involved. +async function withRevisionsResolved(command, cwd) { + const files = [] + for (const side of command.files) { + if (!isRevisionSide(side)) { + files.push(side) + continue + } + const res = await fileAtRevision(side, cwd || process.cwd()) + if (res.error) { + process.stderr.write(`${t(REVISION_ERROR_KEYS[res.error] ?? 'cliErrors.not-a-repo')}\n`) + return null + } + files.push(res.path) + } + return { ...command, files } +} + +function deliverResolved(command, cwd) { + withRevisionsResolved(command, cwd).then((ready) => { + if (!ready) return + ready.files.forEach(allowCliPath) + deliver(ready) + }) +} + +const needsGit = (command) => command?.name === 'compare' && command.files.some(isRevisionSide) + +function routeCommand(command, cwd) { // `open` with no file has nothing to tell the renderer — the window IS the // answer, so it never reaches deliver's pending queue. if (command?.name === 'raise') return void ensureMainWindow()?.focus() + if (needsGit(command)) return void deliverResolved(command, cwd) // Vouch for the paths before the renderer asks for them: file:read honours // only what main has already approved. if (command?.name === 'compare') command.files.forEach(allowCliPath) diff --git a/src/main/gitCliFiles.js b/src/main/gitCliFiles.js new file mode 100644 index 0000000..400a766 --- /dev/null +++ b/src/main/gitCliFiles.js @@ -0,0 +1,50 @@ +// A `revision:path` side of a `diffbro compare`, turned into a real file the +// rest of the app can open. Only MAIN talks to git, and only main writes these +// copies — the renderer receives a path like any other. +// +// The copies live in a temp directory named the way gitTool's do, so the same +// sweep clears both. +import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { basename, join } from 'node:path' +import { readBlobArgs, resolveRevisionArgs, repoRootArgs, runGitIn } from './gitRepo' +import { TEMP_PREFIX } from './gitTool' + +// A revision is not a filename: `origin/main` and `HEAD~1` both have to become +// one directory that a reader recognises in a tab title. +const asFolder = (revision) => revision.replace(/[^A-Za-z0-9._-]+/g, '-').slice(0, 40) || 'rev' + +/** + * @param {{revision: string, relPath: string}} side + * @param {string} cwd the shell's working directory + * @returns {Promise<{path: string}|{error: string}>} + */ +export async function fileAtRevision(side, cwd) { + const root = await runGitIn(repoRootArgs(), cwd) + if (!root.ok || !root.stdout.trim()) return { error: 'not-a-repo' } + const dir = root.stdout.trim() + const resolved = await runGitIn(resolveRevisionArgs(side.revision), dir) + if (!resolved.ok) return { error: 'no-such-revision' } + const blob = await runGitIn(readBlobArgs(side.revision, side.relPath), dir) + if (!blob.ok) return { error: 'not-in-revision' } + + const stage = mkdtempSync(join(tmpdir(), TEMP_PREFIX)) + const folder = join(stage, asFolder(side.revision)) + mkdirSync(folder, { recursive: true }) + const path = join(folder, basename(side.relPath) || 'file') + writeFileSync(path, blob.stdout, 'utf8') + return { path } +} + +/** + * Literal key ids, not a key built from the error at the call site: an assembled + * key is invisible to check:i18n and cannot be found when it goes stale. + */ +export const REVISION_ERROR_KEYS = { + 'not-a-repo': 'cliErrors.not-a-repo', + 'no-such-revision': 'cliErrors.no-such-revision', + 'not-in-revision': 'cliErrors.not-in-revision' +} + +/** Whether a compare side still has to be fetched out of the repository. */ +export const isRevisionSide = (side) => !!side && typeof side === 'object' && !!side.revision diff --git a/src/shared/i18n/en-XA.json b/src/shared/i18n/en-XA.json index 2331a8c..3c31efc 100644 --- a/src/shared/i18n/en-XA.json +++ b/src/shared/i18n/en-XA.json @@ -527,6 +527,11 @@ "noFreeTab": "[Ńō ƒřéé ţàƀ ·øé·]", "gotIt": "[Ğōţ ĩţ ·ø]" }, + "cliErrors": { + "not-a-repo": "[Ţĥàţ ƒōłđéř ĩş ńōţ ĩńşĩđé à ğĩţ řéƥōşĩţōřŷ. ·øé·øé·øé·øé·ø]", + "no-such-revision": "[ğĩţ đōéş ńōţ ķńōŵ ţĥàţ řéṽĩşĩōń. ·øé·øé·øé·ø]", + "not-in-revision": "[Ţĥàţ ƒĩłé ĩş ńōţ ĩń ţĥàţ řéṽĩşĩōń. ·øé·øé·øé·ø]" + }, "cliSettings": { "terminalCommand": "[Ţéřɱĩńàł çōɱɱàńđ ·øé·øé]", "intro": "[Àđđş à {cmd} çōɱɱàńđ şō à çōɱƥàřĩşōń çàń şţàřţ ƒřōɱ à ţéřɱĩńàł: {compare}, {create}, ōř {save} ţō ķééƥ ŵĥàţ ŷōū ĵūşţ çōƥĩéđ. ·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·]", diff --git a/src/shared/i18n/en.json b/src/shared/i18n/en.json index 07f7c28..9212490 100644 --- a/src/shared/i18n/en.json +++ b/src/shared/i18n/en.json @@ -527,6 +527,11 @@ "noFreeTab": "No free tab", "gotIt": "Got it" }, + "cliErrors": { + "not-a-repo": "That folder is not inside a git repository.", + "no-such-revision": "git does not know that revision.", + "not-in-revision": "That file is not in that revision." + }, "cliSettings": { "terminalCommand": "Terminal command", "intro": "Adds a {cmd} command so a comparison can start from a terminal: {compare}, {create}, or {save} to keep what you just copied.", diff --git a/tests/main/cli.test.js b/tests/main/cli.test.js index 089b15a..6d58c9a 100644 --- a/tests/main/cli.test.js +++ b/tests/main/cli.test.js @@ -47,6 +47,37 @@ describe('cliWords', () => { }) }) +describe('parseCli — compare from a revision', () => { + const parse = (...words) => parseCli([...PACKAGED, ...words], (p) => `/cwd/${p}`).command + + // `diffbro compare HEAD~1:src/a.js src/a.js` — the shape git itself uses, so + // the reader does not have to produce a copy of the old file first. + it('keeps a revision argument unresolved, as the pair it names', () => { + expect(parse('compare', 'HEAD~1:src/a.js', 'src/a.js')).toEqual({ + name: 'compare', + files: [{ revision: 'HEAD~1', relPath: 'src/a.js' }, '/cwd/src/a.js'], + transient: false + }) + }) + + it('takes a revision on either side, or both', () => { + expect(parse('compare', 'v1.0.0:a.js', 'HEAD:a.js').files).toEqual([ + { revision: 'v1.0.0', relPath: 'a.js' }, + { revision: 'HEAD', relPath: 'a.js' } + ]) + }) + + // A path is still a path: the feature must not capture every argument that + // happens to contain a colon. + it('leaves an ordinary path alone', () => { + expect(parse('compare', 'a.json').files).toEqual(['/cwd/a.json']) + }) + + it('leaves a Windows absolute path alone', () => { + expect(parse('compare', 'C:/Users/x/a.js').files).toEqual(['/cwd/C:/Users/x/a.js']) + }) +}) + describe('parseCli — compare', () => { it('takes one file', () => { const { command } = parseCli([...PACKAGED, 'compare', 'a.json']) From c0a0c7643ee37753bedac16df552cc52f95ffd0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Mon, 10 Aug 2026 00:08:46 +0300 Subject: [PATCH 11/16] feat(merge): the conflict model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3, commit 1 of specs/2026-08-09-developer-workflow. A file as git leaves it mid-merge: stable text with conflict regions between, each carrying both sides and — in diff3 style — the ancestor that says which side actually changed. Pure, and it COMPOSES the resolved file rather than editing text in place, so nothing can half-apply. Two refusals it makes deliberately: - Markers that do not close return null. A file someone was editing by hand is not a conflict file, and guessing which side the remainder belongs to would silently drop the other. - composeMerge returns null while any region is undecided. Writing a half-resolved file would hand git one with markers still in it, which is worse than not writing at all. The file's own shape survives: CRLF stays CRLF and a trailing newline stays, because a merge tool that reformats the file it resolves is a merge tool nobody trusts twice. Empty stable runs are dropped — a file that opens on a conflict has nothing above it, and an empty segment is not something anyone renders. Co-Authored-By: Claude Opus 5 --- src/renderer/src/utils/mergeConflicts.js | 106 ++++++++++++++++ tests/renderer/utils/mergeConflicts.test.js | 127 ++++++++++++++++++++ 2 files changed, 233 insertions(+) create mode 100644 src/renderer/src/utils/mergeConflicts.js create mode 100644 tests/renderer/utils/mergeConflicts.test.js diff --git a/src/renderer/src/utils/mergeConflicts.js b/src/renderer/src/utils/mergeConflicts.js new file mode 100644 index 0000000..d975c28 --- /dev/null +++ b/src/renderer/src/utils/mergeConflicts.js @@ -0,0 +1,106 @@ +// A file as git left it mid-merge: stable text with conflict regions between, +// each carrying both sides and — in diff3 style — the ancestor they came from. +// Pure, and it composes the resolved file back rather than editing in place. + +const OURS = /^<<<<<<< ?(.*)$/ +const BASE = /^\|\|\|\|\|\|\| ?(.*)$/ +const SPLIT = /^=======\s*$/ +const THEIRS = /^>>>>>>> ?(.*)$/ + +const stable = (lines) => ({ type: 'stable', lines }) + +// A conflict git wrote always closes. One that does not is a file someone was +// editing by hand, and guessing which side the rest belongs to would silently +// drop the other. +function conflictAt(lines, start) { + const conflict = { + type: 'conflict', + oursLabel: OURS.exec(lines[start])[1].trim(), + theirsLabel: '', + ours: [], + base: null, + theirs: [] + } + let side = 'ours' + for (let i = start + 1; i < lines.length; i++) { + const line = lines[i] + if (BASE.test(line)) { + conflict.base = [] + side = 'base' + } else if (SPLIT.test(line)) side = 'theirs' + else if (THEIRS.test(line)) { + conflict.theirsLabel = THEIRS.exec(line)[1].trim() + return { conflict, end: i } + } else conflict[side].push(line) + } + return null +} + +/** + * @param {string} text + * @returns {{segments: Array, eol: string, trailingEol: boolean}|null} null when + * the file's markers do not close. + */ +export function parseConflicts(text) { + const source = String(text ?? '') + const eol = source.includes('\r\n') ? '\r\n' : '\n' + const trailingEol = source.endsWith(eol) + const lines = (trailingEol ? source.slice(0, -eol.length) : source).split(eol) + + const segments = [] + let held = [] + for (let i = 0; i < lines.length; i++) { + if (!OURS.test(lines[i])) { + held.push(lines[i]) + continue + } + const found = conflictAt(lines, i) + if (!found) return null + // An empty run between two conflicts, or before the first, is not a segment + // anyone renders — a file that opens on a conflict has nothing above it. + if (held.length) segments.push(stable(held)) + segments.push(found.conflict) + held = [] + i = found.end + } + if (held.length) segments.push(stable(held)) + return { segments, eol, trailingEol } +} + +const conflicts = (parsed) => (parsed?.segments ?? []).filter((s) => s.type === 'conflict') + +/** How many regions the reader has to decide. */ +export function conflictCount(parsed) { + return conflicts(parsed).length +} + +/** How many of them are still undecided. */ +export function unresolvedCount(parsed, choices = []) { + return conflicts(parsed).filter((_, i) => !CHOICES[choices[i]]).length +} + +// Both keeps the file's own order — ours came first in it. +const CHOICES = { + ours: (c) => c.ours, + theirs: (c) => c.theirs, + both: (c) => [...c.ours, ...c.theirs], + neither: () => [] +} + +/** + * The resolved file, or null while any conflict is undecided — writing a + * half-resolved file would hand git one with markers still in it. + * @param {object} parsed from parseConflicts + * @param {Array<'ours'|'theirs'|'both'|'neither'|null>} choices one per conflict + * @returns {string|null} + */ +export function composeMerge(parsed, choices = []) { + if (!parsed || unresolvedCount(parsed, choices)) return null + const out = [] + let at = 0 + for (const segment of parsed.segments) { + if (segment.type === 'stable') out.push(...segment.lines) + else out.push(...CHOICES[choices[at++]](segment)) + } + return out.join(parsed.eol) + (parsed.trailingEol ? parsed.eol : '') +} diff --git a/tests/renderer/utils/mergeConflicts.test.js b/tests/renderer/utils/mergeConflicts.test.js new file mode 100644 index 0000000..60bcd79 --- /dev/null +++ b/tests/renderer/utils/mergeConflicts.test.js @@ -0,0 +1,127 @@ +import { describe, expect, it } from 'vitest' +import { + composeMerge, + conflictCount, + parseConflicts, + unresolvedCount +} from '../../../src/renderer/src/utils/mergeConflicts' + +const FILE = [ + 'top', + '<<<<<<< HEAD', + 'ours one', + 'ours two', + '=======', + 'theirs one', + '>>>>>>> feature', + 'bottom' +].join('\n') + +const DIFF3 = [ + '<<<<<<< HEAD', + 'ours', + '||||||| merged common ancestors', + 'base', + '=======', + 'theirs', + '>>>>>>> feature' +].join('\n') + +describe('parseConflicts', () => { + it('splits a file into stable text and the conflicts between it', () => { + const parsed = parseConflicts(FILE) + expect(parsed.segments.map((s) => s.type)).toEqual(['stable', 'conflict', 'stable']) + expect(parsed.segments[0].lines).toEqual(['top']) + expect(parsed.segments[2].lines).toEqual(['bottom']) + }) + + it('reads both sides of a conflict, and what each was called', () => { + const [, conflict] = parseConflicts(FILE).segments + expect(conflict.ours).toEqual(['ours one', 'ours two']) + expect(conflict.theirs).toEqual(['theirs one']) + expect(conflict.oursLabel).toBe('HEAD') + expect(conflict.theirsLabel).toBe('feature') + }) + + // diff3 style adds the common ancestor, which is the only thing that says + // WHICH side changed. + it('reads the base section when the file carries one', () => { + const [conflict] = parseConflicts(DIFF3).segments + expect(conflict.base).toEqual(['base']) + expect(conflict.ours).toEqual(['ours']) + expect(conflict.theirs).toEqual(['theirs']) + }) + + it('has no base when the file was written without one', () => { + expect(parseConflicts(FILE).segments[1].base).toBeNull() + }) + + it('counts what there is to resolve', () => { + expect(conflictCount(parseConflicts(FILE))).toBe(1) + const two = parseConflicts([FILE, FILE].join('\n')) + expect(conflictCount(two)).toBe(2) + }) + + it('reports a file with no conflicts at all', () => { + const parsed = parseConflicts('just\ntext\n') + expect(conflictCount(parsed)).toBe(0) + expect(parsed.segments).toHaveLength(1) + }) + + // A file whose markers do not close is not a conflict file; treating the rest + // as "ours" would silently drop the other side. + it('refuses a truncated conflict rather than guessing', () => { + expect(parseConflicts('<<<<<<< HEAD\nours\n')).toBeNull() + expect(parseConflicts('<<<<<<< HEAD\nours\n=======\ntheirs\n')).toBeNull() + }) + + it('keeps a line that merely looks like a marker inside a resolved region', () => { + const parsed = parseConflicts('a\n<<<<<< { + const parsed = parseConflicts(FILE) + + it('takes our side', () => { + expect(composeMerge(parsed, ['ours'])).toBe('top\nours one\nours two\nbottom') + }) + + it('takes their side', () => { + expect(composeMerge(parsed, ['theirs'])).toBe('top\ntheirs one\nbottom') + }) + + it('takes both, ours first — the order the file had them in', () => { + expect(composeMerge(parsed, ['both'])).toBe('top\nours one\nours two\ntheirs one\nbottom') + }) + + it('takes neither, leaving the surrounding text joined', () => { + expect(composeMerge(parsed, ['neither'])).toBe('top\nbottom') + }) + + // Writing a half-resolved file would hand git a merge with markers still in + // it, which is worse than not writing at all. + it('refuses while any conflict is unresolved', () => { + expect(composeMerge(parsed, [null])).toBeNull() + expect(composeMerge(parsed, [])).toBeNull() + }) + + it('resolves each conflict independently', () => { + const two = parseConflicts([FILE, FILE].join('\n')) + expect(unresolvedCount(two, ['ours'])).toBe(1) + const text = composeMerge(two, ['ours', 'theirs']) + expect(text).toContain('ours one') + expect(text).toContain('theirs one') + }) + + it('keeps the file’s trailing newline', () => { + const withEol = parseConflicts(`${FILE}\n`) + expect(composeMerge(withEol, ['ours']).endsWith('\n')).toBe(true) + }) + + it('keeps CRLF where the file used it', () => { + const crlf = parseConflicts(FILE.split('\n').join('\r\n')) + expect(composeMerge(crlf, ['ours'])).toBe('top\r\nours one\r\nours two\r\nbottom') + }) +}) From 213ed8de6e97060fffabc760acd9aa803fd4b123 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Mon, 10 Aug 2026 00:11:15 +0300 Subject: [PATCH 12/16] feat(merge): the write, and the fence that makes it narrow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3, commit 2 of specs/2026-08-09-developer-workflow. This is the line docs/roadmap.md parked as "a decision, not code", and it is crossed deliberately: the app already registered as `git mergetool`, so it already took the job. It is now able to finish it. The write is as narrow as the surface can make it. Main remembers the $MERGED path from the LAUNCH argv; the renderer sends the resolved TEXT and nothing else, so there is no argument through which it could name a file — the same shape clipboard:writeFile uses. With no mergetool launch there is no path, so merge:write does nothing at all whatever arrives, and a session is spent once used: a second write would be a second file. Six tests hold that, including the two that matter — nothing is written without a launch, and a non-string is refused rather than coerced. `diffbro mergetool LOCAL REMOTE MERGED` is the verb git calls, in git's own argument order. Routing it also collapsed the verb ladder in routeCommand into a table, which is what let clipboard-save and the two new arrivals sit side by side under the complexity cap. Co-Authored-By: Claude Opus 5 --- src/main/cli.js | 9 +++++ src/main/cliRoute.js | 34 ++++++++++++++--- src/main/mergeSession.js | 45 ++++++++++++++++++++++ src/preload/index.js | 1 + src/shared/cliCommands.js | 6 +++ src/shared/i18n/en-XA.json | 4 ++ src/shared/i18n/en.json | 4 ++ tests/main/mergeSession.test.js | 67 +++++++++++++++++++++++++++++++++ 8 files changed, 165 insertions(+), 5 deletions(-) create mode 100644 src/main/mergeSession.js create mode 100644 tests/main/mergeSession.test.js diff --git a/src/main/cli.js b/src/main/cli.js index 1428df5..66f25c7 100644 --- a/src/main/cli.js +++ b/src/main/cli.js @@ -115,6 +115,15 @@ export function parseNewSnippet(words) { const VERBS = { compare: (rest, resolve) => parseCompare(rest, resolve), difftool: (rest, resolve) => parseCompare(rest, resolve, true), + // git hands a mergetool four paths in a fixed order: LOCAL REMOTE MERGED + // BASE. MERGED is the file in the repo — the one with the markers in it, and + // the one this run may write back. + mergetool: (rest, resolve) => { + const paths = rest.filter((p) => p.trim()).map(resolve) + if (paths.length < 3) return { command: null, error: 'mergetool needs LOCAL REMOTE MERGED.' } + const [local, remote, merged] = paths + return { command: { name: 'merge', local, remote, merged }, error: null } + }, open: (rest, resolve) => parseOpen(rest, resolve), backup: (rest, resolve) => parseBackup(rest, resolve), // One verb. `--interactive` asks in the terminal and saves; without it the diff --git a/src/main/cliRoute.js b/src/main/cliRoute.js index 2d6adcb..ffc18c8 100644 --- a/src/main/cliRoute.js +++ b/src/main/cliRoute.js @@ -11,6 +11,8 @@ import { gitToolStatus, registerGitTool, sweepGitTemp, unregisterGitTool } from import { ensureMainWindow } from './quickLook' import { allowCliPath } from './files' import { fileAtRevision, isRevisionSide, REVISION_ERROR_KEYS } from './gitCliFiles' +import { beginMerge, writeMerged } from './mergeSession' +import { readFileSync } from 'node:fs' import { t } from './i18n' // A command can arrive before any window exists (a cold `diffbro compare …`), @@ -77,17 +79,36 @@ function deliverResolved(command, cwd) { const needsGit = (command) => command?.name === 'compare' && command.files.some(isRevisionSide) -function routeCommand(command, cwd) { +// A mergetool launch: main REMEMBERS the path git wants written and sends the +// renderer the conflicted text, never the path. What comes back is text. +function routeMerge(command) { + let content + try { + content = readFileSync(command.merged, 'utf8') + } catch { + process.stderr.write(`${t('cliErrors.not-in-revision')}\n`) + return + } + beginMerge(command) + deliver({ name: 'merge', local: command.local, remote: command.remote, content }) +} + +// The verbs that need something done before the renderer hears about them. +const SPECIAL = { // `open` with no file has nothing to tell the renderer — the window IS the // answer, so it never reaches deliver's pending queue. - if (command?.name === 'raise') return void ensureMainWindow()?.focus() + raise: () => void ensureMainWindow()?.focus(), + merge: (command) => routeMerge(command), + 'clipboard-save': (command) => deliver({ ...command, text: clipboard.readText() }) +} + +function routeCommand(command, cwd) { if (needsGit(command)) return void deliverResolved(command, cwd) + const special = SPECIAL[command?.name] + if (special) return void special(command) // Vouch for the paths before the renderer asks for them: file:read honours // only what main has already approved. if (command?.name === 'compare') command.files.forEach(allowCliPath) - if (command?.name === 'clipboard-save') { - return void deliver({ ...command, text: clipboard.readText() }) - } deliver(command) } @@ -124,6 +145,9 @@ export function registerCliIpc() { }) ).response === 1 + // The renderer's ONLY say in the merge is the text. It cannot name a file: + // main has held that path since the launch. + ipcMain.handle('merge:write', (e, text) => writeMerged(text)) ipcMain.handle('cli:status', () => shimStatus(where())) ipcMain.handle('cli:install', async () => (await confirmed(t('dialog.cliInstall.message'), t('dialog.cliInstall.detail'))) diff --git a/src/main/mergeSession.js b/src/main/mergeSession.js new file mode 100644 index 0000000..158a3eb --- /dev/null +++ b/src/main/mergeSession.js @@ -0,0 +1,45 @@ +// The one place Diff Bro writes a file it did not create: the `$MERGED` path +// git handed it on the command line of a `git mergetool` run. +// +// The fence is the shape of the surface, not a check inside it. Main holds the +// path from launch; the renderer sends the resolved TEXT and nothing else, so +// there is no argument through which it could name a file. This mirrors +// clipboard:writeFile, which takes bytes and a display name and never a path. +// +// Nothing else in the app writes over a user's file, and nothing here writes +// unless a mergetool launch put a path in this module first. +import { writeFileSync } from 'node:fs' + +let pending = null + +/** Remembered from the launch argv, never from a message. */ +export function beginMerge({ merged, local, remote }) { + pending = { merged, local, remote } + return pending +} + +export function mergeInProgress() { + return pending +} + +export function endMerge() { + pending = null +} + +/** + * Write the resolved text to the path this session was launched with. + * @param {unknown} text the composed file — the renderer's only say in this + * @returns {{ok: true, path: string}|{ok: false, error: string}} + */ +export function writeMerged(text) { + if (!pending) return { ok: false, error: 'no-merge' } + if (typeof text !== 'string') return { ok: false, error: 'not-text' } + try { + writeFileSync(pending.merged, text, 'utf8') + } catch (err) { + return { ok: false, error: String(err?.message ?? err) } + } + const path = pending.merged + endMerge() + return { ok: true, path } +} diff --git a/src/preload/index.js b/src/preload/index.js index cdfbb18..1050179 100644 --- a/src/preload/index.js +++ b/src/preload/index.js @@ -6,6 +6,7 @@ contextBridge.exposeInMainWorld('api', { readFile: (path, opts) => ipcRenderer.invoke('file:read', path, opts), gitRoot: (path) => ipcRenderer.invoke('git:root', path), gitShow: (payload) => ipcRenderer.invoke('git:show', payload), + writeMerged: (text) => ipcRenderer.invoke('merge:write', text), // `format` names a row of main's own export table; never an extension. exportDiffFile: (payload) => ipcRenderer.invoke('diff:exportFile', payload), // Streamed comparison: files too large to hold are indexed by line in main diff --git a/src/shared/cliCommands.js b/src/shared/cliCommands.js index bffcdff..8f95a69 100644 --- a/src/shared/cliCommands.js +++ b/src/shared/cliCommands.js @@ -20,6 +20,12 @@ export const COMMANDS = [ summaryKey: 'cli.difftool.summary', detailKey: 'cli.difftool.detail' }, + { + topic: 'mergetool', + usage: 'diffbro mergetool ', + summaryKey: 'cli.mergetool.summary', + detailKey: 'cli.mergetool.detail' + }, { topic: 'open', usage: 'diffbro open []', diff --git a/src/shared/i18n/en-XA.json b/src/shared/i18n/en-XA.json index 3c31efc..e552268 100644 --- a/src/shared/i18n/en-XA.json +++ b/src/shared/i18n/en-XA.json @@ -1602,6 +1602,10 @@ "summary": "[ōƥéń à çōɱƥàřĩşōń ğĩţ ĥàńđéđ ōṽéř ·øé·øé·øé·øé]", "detail": "[Ŵĥàţ `ğĩţ đĩƒƒţōōł` àńđ `ğĩţ ɱéřğéţōōł` řūń. Šàɱé àş çōɱƥàřé,\néẋçéƥţ ţĥé ţŵō ƒĩłéş àřé ķńōŵń ţō ƀé ţĥřōŵàŵàŷ çōƥĩéş ğĩţ ɱàđé.\n\nƁéçàūşé ţĥéŷ àřé ţĥřōŵàŵàŷ, à ɱéřğé ŵĩţĥ ɱōřé çōńƒłĩçţş ţĥàń ţĥéřé àřé\nţàƀş řéūşéş ţĥé ōłđéşţ ōƒ ţĥéɱ ĩńşţéàđ ōƒ řūńńĩńğ ōūţ — `ğĩţ ɱéřğéţōōł`\nŵàłķş ţĥé ŵĥōłé çōńƒłĩçţ łĩşţ ŵĩţĥōūţ ŵàĩţĩńğ ƒōř àńŷōńé. ·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·ø]" }, + "mergetool": { + "summary": "[Řéşōłṽé à ɱéřğé çōńƒłĩçţ ğĩţ ĥàńđéđ ōṽéř ·øé·øé·øé·øé·ø]", + "detail": "[Çàłłéđ ƀŷ `ğĩţ ɱéřğéţōōł`, ńōţ ūşūàłłŷ ƀŷ ĥàńđ. Ōƥéńş ţĥé ţŵō çōńƒłĩçţĩńğ ṽéřşĩōńş, ţàķéş à çĥōĩçé ƥéř çōńƒłĩçţ, àńđ ŵřĩţéş ţĥé ɱéřğéđ ƒĩłé ƀàçķ. ·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·]" + }, "open": { "summary": "[řàĩşé ţĥé àƥƥ, ōƥţĩōńàłłŷ ōń à ƒĩłé ·øé·øé·øé·øé]", "detail": "[Ɓřĩńğş Đĩƒƒ Ɓřō ţō ţĥé ƒřōńţ, şţàřţĩńğ ĩţ ĩƒ ĩţ ĩş ńōţ řūńńĩńğ.\n\nŴĩţĥ à ƒĩłé, ţĥàţ ƒĩłé ƒĩłłş ţĥé łéƒţ şĩđé àńđ ŵàĩţş ƒōř ţĥé řĩğĥţ — ţĥé\nşàɱé àş `çōɱƥàřé` ŵĩţĥ ōńé ƥàţĥ, ūńđéř à ńàɱé ţĥàţ řéàđş łĩķé ōƥéńĩńğ.\nƑōř ţŵō ƒĩłéş àţ ōńçé, ūşé `çōɱƥàřé`. ·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·ø]" diff --git a/src/shared/i18n/en.json b/src/shared/i18n/en.json index 9212490..e4bbb8f 100644 --- a/src/shared/i18n/en.json +++ b/src/shared/i18n/en.json @@ -1602,6 +1602,10 @@ "summary": "open a comparison git handed over", "detail": "What `git difftool` and `git mergetool` run. Same as compare,\nexcept the two files are known to be throwaway copies git made.\n\nBecause they are throwaway, a merge with more conflicts than there are\ntabs reuses the oldest of them instead of running out — `git mergetool`\nwalks the whole conflict list without waiting for anyone." }, + "mergetool": { + "summary": "Resolve a merge conflict git handed over", + "detail": "Called by `git mergetool`, not usually by hand. Opens the two conflicting versions, takes a choice per conflict, and writes the merged file back." + }, "open": { "summary": "raise the app, optionally on a file", "detail": "Brings Diff Bro to the front, starting it if it is not running.\n\nWith a file, that file fills the left side and waits for the right — the\nsame as `compare` with one path, under a name that reads like opening.\nFor two files at once, use `compare`." diff --git a/tests/main/mergeSession.test.js b/tests/main/mergeSession.test.js new file mode 100644 index 0000000..5ec0da6 --- /dev/null +++ b/tests/main/mergeSession.test.js @@ -0,0 +1,67 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { beginMerge, endMerge, mergeInProgress, writeMerged } from '../../src/main/mergeSession' + +const dirs = [] +const scratch = () => { + const dir = mkdtempSync(join(tmpdir(), 'merge-')) + dirs.push(dir) + return dir +} + +afterEach(() => { + endMerge() + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }) +}) + +describe('writeMerged', () => { + // The whole point of the fence: with no launch there is no path, so there is + // nothing to write over, whatever arrives. + it('writes nothing at all when no mergetool launch is in progress', () => { + expect(writeMerged('anything')).toEqual({ ok: false, error: 'no-merge' }) + }) + + it('writes the text to the path the launch named', () => { + const dir = scratch() + const merged = join(dir, 'app.js') + writeFileSync(merged, '<<<<<<< HEAD\n') + beginMerge({ merged, local: join(dir, 'l'), remote: join(dir, 'r') }) + expect(writeMerged('resolved\n')).toEqual({ ok: true, path: merged }) + expect(readFileSync(merged, 'utf8')).toBe('resolved\n') + }) + + it('refuses anything that is not text', () => { + const dir = scratch() + beginMerge({ merged: join(dir, 'a.js'), local: '', remote: '' }) + expect(writeMerged({ path: '/etc/passwd' })).toEqual({ ok: false, error: 'not-text' }) + expect(writeMerged(null).ok).toBe(false) + }) + + // A second write would be a second file: once the merge is handed back, the + // session is over. + it('is spent once used', () => { + const dir = scratch() + const merged = join(dir, 'a.js') + beginMerge({ merged, local: '', remote: '' }) + expect(writeMerged('one').ok).toBe(true) + expect(writeMerged('two')).toEqual({ ok: false, error: 'no-merge' }) + expect(readFileSync(merged, 'utf8')).toBe('one') + }) + + it('reports a path it cannot write rather than throwing', () => { + beginMerge({ merged: join(scratch(), 'no', 'such', 'dir', 'a.js'), local: '', remote: '' }) + const res = writeMerged('x') + expect(res.ok).toBe(false) + expect(res.error).not.toBe('no-merge') + }) + + it('says whether a merge is waiting, for the window that asks', () => { + expect(mergeInProgress()).toBeNull() + beginMerge({ merged: '/tmp/a', local: '/tmp/l', remote: '/tmp/r' }) + expect(mergeInProgress().merged).toBe('/tmp/a') + endMerge() + expect(mergeInProgress()).toBeNull() + }) +}) From 32dbd1cca75620dadf48c50a6e12242748cef1ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Mon, 10 Aug 2026 00:18:23 +0300 Subject: [PATCH 13/16] feat(merge): resolve the conflict, and let git believe the answer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3, commit 3 of specs/2026-08-09-developer-workflow. The merge view is a BaseDialog over the two sides git handed over: one card per conflict, four choices each — ours, theirs, both, neither — with "ours everywhere" and "theirs everywhere" for the common case. Save is disabled until every conflict has an answer, because a half-resolved file still has markers in it. trustExitCode flips to true, and the reason it may is the new launcher. The app is single-instance, so a launch returns as soon as the running window has been told; a script that exited there would tell git the merge was resolved before anyone had looked at it. gitMergeTool.js polls $MERGED's modification time and only then exits 0. That is what makes the flag honest rather than optimistic. An existing test asserted trustExitCode=false and had to change — it encoded the old contract, that Diff Bro never writes $MERGED. Its replacement says why the answer is now true, and two more hold the wait itself. The extraction of gitMergeTool.js was forced by gitTool.js's 250-line cap and then by a real import cycle the structure guard caught: the shared MARK now lives in its own module, so neither launcher imports the other. e2e builds a repository with a genuine merge conflict, drives `mergetool` against it, resolves it in the dialog, and asserts the file git is left holding is `one/theirs/three` with no markers — and that Save refuses while anything is undecided. Co-Authored-By: Claude Opus 5 --- e2e/merge-resolve.spec.mjs | 89 ++++++++++++++++++ src/main/gitMergeTool.js | 29 ++++++ src/main/gitTool.js | 34 +++++-- src/main/gitToolMark.js | 4 + src/renderer/src/components/AppDialogs.vue | 2 + src/renderer/src/composables/useCommands.js | 4 +- .../features/merge/components/MergeDialog.vue | 90 +++++++++++++++++++ .../merge/components/styles/MergeDialog.css | 56 ++++++++++++ src/renderer/src/features/merge/index.js | 2 + src/renderer/src/features/merge/mergeStore.js | 63 +++++++++++++ src/renderer/src/utils/commands.js | 6 ++ src/shared/i18n/en-XA.json | 12 +++ src/shared/i18n/en.json | 12 +++ tests/main/gitTool.test.js | 28 ++++-- 14 files changed, 416 insertions(+), 15 deletions(-) create mode 100644 e2e/merge-resolve.spec.mjs create mode 100644 src/main/gitMergeTool.js create mode 100644 src/main/gitToolMark.js create mode 100644 src/renderer/src/features/merge/components/MergeDialog.vue create mode 100644 src/renderer/src/features/merge/components/styles/MergeDialog.css create mode 100644 src/renderer/src/features/merge/index.js create mode 100644 src/renderer/src/features/merge/mergeStore.js diff --git a/e2e/merge-resolve.spec.mjs b/e2e/merge-resolve.spec.mjs new file mode 100644 index 0000000..ca69191 --- /dev/null +++ b/e2e/merge-resolve.spec.mjs @@ -0,0 +1,89 @@ +import { test, expect, launchApp, freshUserDataDir, firstReadyPage } from './fixtures.mjs' +import { execFileSync, spawn } from 'node:child_process' +import { mkdtempSync, readFileSync, writeFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { createRequire } from 'node:module' +import { fileURLToPath } from 'node:url' + +const ROOT = fileURLToPath(new URL('..', import.meta.url)) +const MAIN = join(ROOT, 'build', 'main', 'index.js') +const ELECTRON = createRequire(import.meta.url)('electron') + +// A conflict git actually produced, resolved through the app, and the file git +// is left holding. Only a real launch proves the whole chain: main reads +// $MERGED, the renderer resolves, and main writes the path it has held since. +function conflictedRepo() { + const dir = mkdtempSync(join(tmpdir(), 'diffbro-merge-')) + const env = { + ...process.env, + GIT_AUTHOR_NAME: 'T', + GIT_AUTHOR_EMAIL: 't@e', + GIT_COMMITTER_NAME: 'T', + GIT_COMMITTER_EMAIL: 't@e' + } + const git = (...args) => execFileSync('git', args, { cwd: dir, env }) + const file = join(dir, 'app.txt') + git('init', '-q', '-b', 'main') + writeFileSync(file, 'one\nbase\nthree\n') + git('add', '.') + git('commit', '-qm', 'base') + git('checkout', '-qb', 'feature') + writeFileSync(file, 'one\ntheirs\nthree\n') + git('commit', '-qam', 'theirs') + git('checkout', '-q', 'main') + writeFileSync(file, 'one\nours\nthree\n') + git('commit', '-qam', 'ours') + try { + git('merge', 'feature') + } catch { + // Expected: this is the conflict under test. + } + return { dir, file } +} + +function runMergetool(userDataDir, dir, file) { + const env = { ...process.env } + delete env.ELECTRON_RUN_AS_NODE + return new Promise((resolve) => { + const p = spawn( + ELECTRON, + [MAIN, `--user-data-dir=${userDataDir}`, 'mergetool', file, file, file], + { cwd: dir, env, stdio: 'ignore' } + ) + p.on('exit', () => resolve()) + setTimeout(resolve, 8000) + }) +} + +test('resolves a real conflict and writes the merged file back', async () => { + const { dir, file } = conflictedRepo() + expect(readFileSync(file, 'utf8')).toContain('<<<<<<<') + + const userDataDir = freshUserDataDir() + const app = await launchApp(userDataDir) + const page = await firstReadyPage(app) + try { + await runMergetool(userDataDir, dir, file) + + const dialog = page.getByRole('dialog', { name: 'Resolve merge conflicts' }) + await expect(dialog).toBeVisible({ timeout: 20000 }) + + // Nothing may be written while a conflict is undecided. + const save = page.getByTestId('merge-save') + await expect(save).toBeDisabled() + + await page.getByTestId('merge-theirs-0').click() + await expect(save).toBeEnabled() + await save.click() + await expect(dialog).toHaveCount(0, { timeout: 10000 }) + + // The file git is left holding: their side, no markers. + const merged = readFileSync(file, 'utf8') + expect(merged).toBe('one\ntheirs\nthree\n') + expect(merged).not.toContain('<<<<<<<') + } finally { + await app.close().catch(() => {}) + rmSync(dir, { recursive: true, force: true }) + } +}) diff --git a/src/main/gitMergeTool.js b/src/main/gitMergeTool.js new file mode 100644 index 0000000..9657960 --- /dev/null +++ b/src/main/gitMergeTool.js @@ -0,0 +1,29 @@ +// The mergetool launcher, which differs from the difftool one in the only way +// that matters: it WAITS. +// +// The app is single-instance, so a launch returns as soon as the running window +// has been told. If the script exited there, git would read the merge as +// finished before the reader had chosen anything, and trustExitCode=true would +// be a lie. Polling $MERGED's modification time is what makes it true. +import { MARK } from './gitToolMark' +import { shQuote } from './shellQuote' + +/** Beside the difftool launcher, so removing either leaves the other alone. */ +export function gitMergeTarget(target) { + return `${target}-merge` +} + +/** @param {string} exePath the installed app binary */ +export function gitMergeScript(exePath, entryPath = null) { + return `#!/bin/sh +${MARK} +before=$(ls -l "$3" 2>/dev/null) +${shQuote(exePath)}${entryPath ? ` ${shQuote(entryPath)}` : ''} mergetool "$1" "$2" "$3" || exit 1 +# Wait for the reader. Exiting here would tell git the merge was resolved before +# anyone had looked at it. +while [ "$(ls -l "$3" 2>/dev/null)" = "$before" ]; do + sleep 1 +done +exit 0 +` +} diff --git a/src/main/gitTool.js b/src/main/gitTool.js index 4a92859..0c65ef0 100644 --- a/src/main/gitTool.js +++ b/src/main/gitTool.js @@ -19,9 +19,10 @@ import { import { dirname, join, posix, win32 } from 'node:path' import { execFile } from 'node:child_process' import { shQuote } from './shellQuote' +import { gitMergeScript, gitMergeTarget } from './gitMergeTool' +import { MARK } from './gitToolMark' export const GIT_TOOL_NAME = 'diffbro' -const MARK = '# diff-bro git tool' /** * Where the launcher lives — beside the `diffbro` shim, so removing either @@ -67,21 +68,29 @@ exec ${shQuote(exePath)}${entryPath ? ` ${shQuote(entryPath)}` : ''} difftool "$ // so the copies keep the real filename. const invocation = (script) => `"${script}" "$LOCAL" "$REMOTE" "$MERGED"` +// A mergetool has to WAIT. The app is single-instance, so the launch returns as +// soon as the running window has been told — if the script exited there, git +// would read the merge as finished before the reader had chosen anything, and +// trustExitCode would be a lie. So it polls $MERGED's modification time and only +// then reports success. +const mergeInvocation = (script) => `"${script}" "$LOCAL" "$REMOTE" "$MERGED"` + /** * The registration, as git config argument vectors. * - * Diff Bro shows a merge's two conflicting sides for READING; it never writes - * $MERGED. trustExitCode=false is what keeps that honest — git asks whether the - * merge succeeded instead of taking a clean exit as "resolved". + * Diff Bro shows a merge's two conflicting sides AND resolves them: the merge + * launcher waits for $MERGED to be written, so trustExitCode=true is honest. * @param {string} script * @returns {string[][]} */ -export function registerArgs(script) { +export function registerArgs(script, mergeScript = script) { const cmd = invocation(script) return [ ['config', '--global', `difftool.${GIT_TOOL_NAME}.cmd`, cmd], - ['config', '--global', `mergetool.${GIT_TOOL_NAME}.cmd`, cmd], - ['config', '--global', `mergetool.${GIT_TOOL_NAME}.trustExitCode`, 'false'], + ['config', '--global', `mergetool.${GIT_TOOL_NAME}.cmd`, mergeInvocation(mergeScript)], + // The merge script waits for $MERGED to change before it exits, so a clean + // exit now MEANS resolved and git can be told to believe it. + ['config', '--global', `mergetool.${GIT_TOOL_NAME}.trustExitCode`, 'true'], ['config', '--global', 'diff.tool', GIT_TOOL_NAME], ['config', '--global', 'merge.tool', GIT_TOOL_NAME] ] @@ -148,6 +157,7 @@ export async function gitToolStatus({ home, platform, localAppData, git = runGit export async function registerGitTool({ exePath, home, platform, localAppData, entryPath, git }) { git = git ?? runGit const target = gitToolTarget({ platform, home, localAppData }) + const mergeTarget = gitMergeTarget(gitToolTarget({ platform, home, localAppData })) if (!(await git(['--version'])).ok) return { ok: false, error: 'git is not on your PATH.' } try { if (existsSync(target) && !looksLikeOurs(target)) { @@ -155,11 +165,15 @@ export async function registerGitTool({ exePath, home, platform, localAppData, e } mkdirSync(dirname(target), { recursive: true }) writeFileSync(target, gitToolScript(exePath, entryPath), 'utf8') - if (platform !== 'win32' && process.platform !== 'win32') chmodSync(target, 0o755) + writeFileSync(mergeTarget, gitMergeScript(exePath, entryPath), 'utf8') + if (platform !== 'win32' && process.platform !== 'win32') { + chmodSync(target, 0o755) + chmodSync(mergeTarget, 0o755) + } } catch (e) { return { ok: false, error: e.message } } - for (const args of registerArgs(target)) { + for (const args of registerArgs(target, mergeTarget)) { const res = await git(args) if (!res.ok) return { ok: false, error: 'git refused the configuration.' } } @@ -177,8 +191,10 @@ export async function unregisterGitTool({ home, platform, localAppData, git = ru await git(args) } const target = gitToolTarget({ platform, home, localAppData }) + const mergeTarget = gitMergeTarget(gitToolTarget({ platform, home, localAppData })) try { if (!existsSync(target) || looksLikeOurs(target)) rmSync(target, { force: true }) + if (!existsSync(mergeTarget) || looksLikeOurs(mergeTarget)) rmSync(mergeTarget, { force: true }) } catch (e) { return { ok: false, error: e.message } } diff --git a/src/main/gitToolMark.js b/src/main/gitToolMark.js new file mode 100644 index 0000000..596ace8 --- /dev/null +++ b/src/main/gitToolMark.js @@ -0,0 +1,4 @@ +// The line that marks a launcher script as ours, so removing one never deletes +// somebody else's file of the same name. Its own module because BOTH launchers +// need it and neither should have to import the other. +export const MARK = '# diff-bro git tool' diff --git a/src/renderer/src/components/AppDialogs.vue b/src/renderer/src/components/AppDialogs.vue index fd0a5db..d3142a2 100644 --- a/src/renderer/src/components/AppDialogs.vue +++ b/src/renderer/src/components/AppDialogs.vue @@ -51,6 +51,7 @@ const paste = usePasteToCompareStore() const snippets = useSnippetStore() const vault = useVaultStore() const errors = useErrorStore() +import { MergeDialog } from '../features/merge' diff --git a/src/renderer/src/composables/useCommands.js b/src/renderer/src/composables/useCommands.js index 096213d..cad793b 100644 --- a/src/renderer/src/composables/useCommands.js +++ b/src/renderer/src/composables/useCommands.js @@ -12,6 +12,7 @@ import { useConfigBackupStore } from '../features/configBackup' import { useImageExportStore } from '../features/imageExport' import { useShareStore } from '../features/share' import { useOnboardingStore } from '../features/onboarding' +import { useMergeStore } from '../features/merge' const bundle = () => ({ diff: useDiffStore(), @@ -22,7 +23,8 @@ const bundle = () => ({ configBackup: useConfigBackupStore(), imageExport: useImageExportStore(), share: useShareStore(), - onboarding: useOnboardingStore() + onboarding: useOnboardingStore(), + merge: useMergeStore() }) export function useCommands() { diff --git a/src/renderer/src/features/merge/components/MergeDialog.vue b/src/renderer/src/features/merge/components/MergeDialog.vue new file mode 100644 index 0000000..60a67ac --- /dev/null +++ b/src/renderer/src/features/merge/components/MergeDialog.vue @@ -0,0 +1,90 @@ + + + + + diff --git a/src/renderer/src/features/merge/components/styles/MergeDialog.css b/src/renderer/src/features/merge/components/styles/MergeDialog.css new file mode 100644 index 0000000..c617efa --- /dev/null +++ b/src/renderer/src/features/merge/components/styles/MergeDialog.css @@ -0,0 +1,56 @@ +/* One conflict per card, both sides side by side. Colour is the add/del roles + the diff panes already use, so it reads the same on every theme. */ +.merge-bulk { + display: flex; + gap: var(--space-2); + margin-bottom: var(--space-2); +} +.merge-list { + display: flex; + flex-direction: column; + gap: var(--space-3); +} +.merge-conflict { + border: 1px solid var(--border); + border-radius: var(--radius-lg); + background: var(--bg-elevated); + overflow: hidden; +} +/* Answered: a keyline down the edge rather than a wash, so the two coloured + sides inside keep their own meaning. */ +.merge-conflict.done { + box-shadow: inset 3px 0 0 var(--success-text); +} +.merge-head { + gap: var(--space-2); + min-height: var(--control-h); + padding: 0 var(--space-3); + border-bottom: 1px solid var(--border); + color: var(--text-dim); + font-size: var(--font-sm); +} +.merge-choices { + display: flex; + gap: var(--space-1); + margin-left: auto; +} +.merge-sides { + display: grid; + grid-template-columns: 1fr 1fr; +} +.merge-side { + margin: 0; + padding: var(--space-2) var(--space-3); + font-family: var(--font-mono); + font-size: var(--font-sm); + white-space: pre-wrap; + overflow-x: auto; + color: var(--text); +} +.merge-side.ours { + background: color-mix(in srgb, var(--danger-border) 12%, transparent); + border-right: 1px solid var(--border); +} +.merge-side.theirs { + background: color-mix(in srgb, var(--success-text) 12%, transparent); +} diff --git a/src/renderer/src/features/merge/index.js b/src/renderer/src/features/merge/index.js new file mode 100644 index 0000000..02bdd4b --- /dev/null +++ b/src/renderer/src/features/merge/index.js @@ -0,0 +1,2 @@ +export { useMergeStore } from './mergeStore' +export { default as MergeDialog } from './components/MergeDialog.vue' diff --git a/src/renderer/src/features/merge/mergeStore.js b/src/renderer/src/features/merge/mergeStore.js new file mode 100644 index 0000000..225d014 --- /dev/null +++ b/src/renderer/src/features/merge/mergeStore.js @@ -0,0 +1,63 @@ +import { defineStore } from 'pinia' +import { + composeMerge, + conflictCount, + parseConflicts, + unresolvedCount +} from '../../utils/mergeConflicts' + +/** + * A `git mergetool` run, from the moment main hands over the conflicted text to + * the moment the resolved file goes back. The renderer never learns the path it + * is resolving — main has held that since the launch. + */ +export const useMergeStore = defineStore('merge', { + state: () => ({ + open: false, + parsed: null, + /** @type {Array<'ours'|'theirs'|'both'|'neither'|null>} */ + choices: [], + error: '', + saved: false + }), + getters: { + conflicts: (s) => (s.parsed?.segments ?? []).filter((seg) => seg.type === 'conflict'), + total: (s) => conflictCount(s.parsed), + remaining: (s) => unresolvedCount(s.parsed, s.choices), + resolvedText: (s) => composeMerge(s.parsed, s.choices) + }, + actions: { + /** @param {string} content the file as git left it, markers and all */ + begin(content) { + this.parsed = parseConflicts(content) + this.choices = new Array(conflictCount(this.parsed)).fill(null) + this.error = this.parsed ? '' : 'unreadable' + this.saved = false + this.open = true + }, + choose(index, choice) { + if (index < 0 || index >= this.choices.length) return + this.choices[index] = this.choices[index] === choice ? null : choice + }, + takeAll(choice) { + this.choices = this.choices.map(() => choice) + }, + async save() { + const text = this.resolvedText + // composeMerge already refuses a half-resolved file; this is the guard + // that keeps a UI mistake from reaching the write. + if (text === null) return false + const res = await window.api.writeMerged(text) + if (!res?.ok) { + this.error = 'write-failed' + return false + } + this.saved = true + this.open = false + return true + }, + close() { + this.open = false + } + } +}) diff --git a/src/renderer/src/utils/commands.js b/src/renderer/src/utils/commands.js index 6a69ba3..d90c1a4 100644 --- a/src/renderer/src/utils/commands.js +++ b/src/renderer/src/utils/commands.js @@ -142,6 +142,12 @@ export const CLI_COMMANDS = { // Typed in the terminal, so it is saved outright rather than opened in the // editor — the reader has already answered every question the editor asks. 'new-snippet': ({ snippets }, command) => snippets.add(command.draft), + // git hands over the two conflicting versions AND the file it wants written; + // the sides open as an ordinary comparison, the conflicts open over them. + merge: async ({ diff, tabs, merge }, command) => { + await compareFromCli({ diff, tabs }, [command.local, command.remote], true) + merge.begin(command.content) + }, compare: ({ diff, tabs }, command) => compareFromCli({ diff, tabs }, command.files, command.transient === true), // The passphrase is asked for here, not in the terminal: the bundle is diff --git a/src/shared/i18n/en-XA.json b/src/shared/i18n/en-XA.json index e552268..a961325 100644 --- a/src/shared/i18n/en-XA.json +++ b/src/shared/i18n/en-XA.json @@ -16,6 +16,18 @@ "hint": "[Ṁéńūş àńđ àƥƥ ţéẋţ. Řéşţàřţ ĩş ńōţ ńééđéđ. ·øé·øé·øé·øé·ø]" } }, + "merge": { + "title": "[Řéşōłṽé ɱéřğé çōńƒłĩçţş ·øé·øé·øé]", + "remaining": "[Ńōţĥĩńğ łéƒţ ţō đéçĩđé | {n} çōńƒłĩçţ şţĩłł ţō đéçĩđé | {n} çōńƒłĩçţş şţĩłł ţō đéçĩđé ·øé·øé·øé·øé·øé·øé·øé·øé·ø]", + "conflictN": "[Çōńƒłĩçţ {n} ·øé·]", + "takeOurs": "[Ōūřş ·ø]", + "takeTheirs": "[Ţĥéĩřş ·øé]", + "takeBoth": "[Ɓōţĥ ·ø]", + "takeNeither": "[Ńéĩţĥéř ·øé]", + "allOf": "[{side} éṽéřŷŵĥéřé ·øé·øé]", + "save": "[Šàṽé ţĥé ɱéřğé ·øé·ø]", + "unreadable": "[Ţĥàţ ƒĩłé’ş çōńƒłĩçţ ɱàřķéřş đō ńōţ çłōşé, şō ţĥéřé ĩş ńōţĥĩńğ şàƒé ţō řéşōłṽé. Ƒĩńĩşĥ ĩţ ƀŷ ĥàńđ. ·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·]" + }, "menu": { "file": { "title": "[Ƒĩłé ·ø]", diff --git a/src/shared/i18n/en.json b/src/shared/i18n/en.json index e4bbb8f..7d017e9 100644 --- a/src/shared/i18n/en.json +++ b/src/shared/i18n/en.json @@ -16,6 +16,18 @@ "hint": "Menus and app text. Restart is not needed." } }, + "merge": { + "title": "Resolve merge conflicts", + "remaining": "Nothing left to decide | {n} conflict still to decide | {n} conflicts still to decide", + "conflictN": "Conflict {n}", + "takeOurs": "Ours", + "takeTheirs": "Theirs", + "takeBoth": "Both", + "takeNeither": "Neither", + "allOf": "{side} everywhere", + "save": "Save the merge", + "unreadable": "That file’s conflict markers do not close, so there is nothing safe to resolve. Finish it by hand." + }, "menu": { "file": { "title": "File", diff --git a/tests/main/gitTool.test.js b/tests/main/gitTool.test.js index 9807090..5c1fdc7 100644 --- a/tests/main/gitTool.test.js +++ b/tests/main/gitTool.test.js @@ -24,6 +24,7 @@ import { unregisterArgs, unregisterGitTool } from '../../src/main/gitTool' +import { gitMergeScript } from '../../src/main/gitMergeTool' const APP = '/Applications/Diff Bro.app/Contents/MacOS/Diff Bro' // Windows has no exec bit to set or read — registerGitTool already skips the @@ -115,11 +116,28 @@ describe('registerArgs', () => { expect(cmd).toBe('"/bin/diffbro-git" "$LOCAL" "$REMOTE" "$MERGED"') }) - // Diff Bro never writes $MERGED. A trusted exit code would let git mark a - // conflict resolved because the viewer closed cleanly. - it('never lets git trust the exit code of a merge', () => { - const trust = registerArgs('/x').find((a) => a[2].endsWith('trustExitCode')) - expect(trust[3]).toBe('false') + // This USED to be false, because Diff Bro never wrote $MERGED and a trusted + // exit code would have marked a conflict resolved just because the viewer + // closed. It writes the file now, and the merge launcher WAITS for that write + // before exiting — so a clean exit means resolved, and git may believe it. + it('lets git trust the exit code, because the merge launcher waits', () => { + const trust = registerArgs('/x', '/x-merge').find((a) => a[2].endsWith('trustExitCode')) + expect(trust[3]).toBe('true') + }) + + it('points the mergetool at the waiting launcher, not the difftool one', () => { + const args = registerArgs('/bin/diffbro-git', '/bin/diffbro-git-merge') + const merge = args.find((a) => a[2] === `mergetool.${GIT_TOOL_NAME}.cmd`) + expect(merge[3]).toBe('"/bin/diffbro-git-merge" "$LOCAL" "$REMOTE" "$MERGED"') + }) + + // The wait is the whole reason trustExitCode may be true; a launcher that + // returned straight away would hand git a lie. + it('waits for $MERGED to change before reporting success', () => { + const script = gitMergeScript('/Applications/Diff Bro.app/x') + expect(script).toContain('mergetool') + expect(script).toMatch(/while \[ "\$\(ls -l "\$3" 2>\/dev\/null\)" = "\$before" \]/) + expect(script.trimEnd().endsWith('exit 0')).toBe(true) }) }) From 974d2eeb2ef53c2a9a8cef371b344f809cc6290f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Mon, 10 Aug 2026 00:21:46 +0300 Subject: [PATCH 14/16] docs: the developer-workflow track, and the copy that no longer apologises MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3, commit 4 of specs/2026-08-09-developer-workflow — the spec closes. docs/security.md gains "Writing a merged file", because a write over a file the user already had deserves its own section rather than a footnote in the IPC table; ipc-security.md gains merge:write beside the git pair. The Settings copy is corrected rather than left standing. It used to say "Diff Bro doesn't write the merged file, so git still asks you whether the merge worked" — that sentence was the evidence for this whole phase, and it is no longer true. The roadmap gains a Developer workflow track and "Comparing more" loses three-way merge to it. The board is hand-authored, so it was edited in the same change and rendered offscreen to check: the new card collided with the rail and had no footer, which only a render shows. Co-Authored-By: Claude Opus 5 --- README.md | 1 + docs/brand/roadmap.svg | 56 +++++++++++++-------- docs/ipc-security.md | 1 + docs/roadmap.md | 47 +++++++++++++++-- docs/security.md | 19 +++++++ specs/2026-08-09-developer-workflow/plan.md | 10 ++-- src/shared/i18n/en-XA.json | 2 +- src/shared/i18n/en.json | 2 +- 8 files changed, 106 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 3be32ad..5a46193 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,7 @@ Builds are **unsigned**, so SmartScreen and Gatekeeper warn on first launch (the | **Guided first run** | Six coach marks over the real controls on a first launch — comparing, sealing, the library, then the way into Settings and around it — with four more if you want them. Each step points at a control and its button performs the action, so nothing opens unannounced. Back revisits a step, and everything it put on screen — the demo files, the example snippet — leaves when it does. Escape or Skip ends it for good; Help ▸ Show Tour brings it back. | | **Diagrams** | Two Mermaid files compare as a picture, not as text — one diagram carrying both revisions, so an inserted node reads as one change instead of a rewrite. | | **Tools** | JSON, Base64, UUID, JWT, Epoch, URL, Lines, XML, checksums, a regex tester, find & replace, text encryption — rich panels, not blank text boxes. All of them live in their own sidebar section; star the ones you reach for and they stay at the top. | +| **Merge conflicts** | Registered as git's `difftool` **and** `mergetool`: `git mergetool` opens the two conflicting versions and takes a choice per conflict — ours, theirs, both or neither — then writes the merged file back and tells git it is done. This is the one file Diff Bro writes over; everything else it produces is a new file you picked the place for. | | **Terminal** | `diffbro compare a.json b.json` opens a comparison in the running app, and either side can name a git revision instead of a file — `diffbro compare HEAD~1:src/app.js src/app.js` reads the old copy straight out of the repository, so you never have to produce one first. `diffbro open` raises the app, `diffbro backup ` writes an encrypted archive. No port, no daemon. | | **Yours to arrange** | Fourteen themes (Nord, Sepia, Solar, Nyan, Matrix, plus accessibility-grade Contrast and Beacon), shared tags, adjustable limits. | diff --git a/docs/brand/roadmap.svg b/docs/brand/roadmap.svg index 4e90c40..2dafede 100644 --- a/docs/brand/roadmap.svg +++ b/docs/brand/roadmap.svg @@ -5,7 +5,7 @@ sidebar. Track hues are real theme accents from src/renderer/src/utils/themes.js (Dim, Bloom, Neon, Beacon) — colour depth encodes sequence: solid now, faded later. Keep this in step with the items in docs/roadmap.md. --> - + @@ -16,22 +16,22 @@ - + - + Diff Bro roadmap - Four tracks. Depth of colour is sequence — solid first, faded last. + Five tracks. Depth of colour is sequence — solid first, faded last. V0.4.27 · PLANNED + font-family="ui-monospace, SFMono-Regular, Menlo, monospace" fill="#a99a7f">V0.4.29 · PLANNED @@ -72,10 +72,10 @@ diagramUnion.js - + Comparing more 3 + font-family="ui-monospace, SFMono-Regular, Menlo, monospace" fill="#8d8168">2 @@ -84,11 +84,8 @@ Image pairs - - Three-way merge - - - a decision first + + a decision first @@ -106,18 +103,37 @@ electron-builder.yml:80 + + + Developer workflow + 3 + + + + Dependencies · lockfiles + + + Compare a git revision + + + Resolve a merge + + + utils/lockfile/ + - + - - now + + now - - next + + next - - later + + later - Each card ends with the file the work starts from. + Each card ends with the file the work starts from. diff --git a/docs/ipc-security.md b/docs/ipc-security.md index d41815e..97ddce7 100644 --- a/docs/ipc-security.md +++ b/docs/ipc-security.md @@ -87,6 +87,7 @@ that enforces each: | **Backup deletion by age, never by name** | `backup:prune` is the only handler that DELETES. It takes an age in days that must be one of the two the app offers (`PRUNE_DAYS`), never a path or a filename, so the renderer cannot name a file to remove; every candidate comes from `listBackups`, which yields only names that parse as one of ours, so anything else sharing the folder is untouched | `backupRoute.js`, `autoBackup.js` | | **The mail hand-off supplies no URL and no path** | `mail:handoff` takes fingerprints and text. Main resolves the addresses from the trust store, BUILDS the `mailto:` (`mailto.js`), and re-checks it with `isSafeMailtoUrl` before `shell.openExternal` — `mailto:` only, and an `attach`/`attachment` parameter is refused rather than ignored. The file it copies and reveals is the path it just sealed, never one round-tripped through the renderer | `mail.js`, `mailto.js`, `linkPolicy.js`, `mailAddress.js` | | **Copy as file takes bytes, never a path** | `clipboard:writeFile` receives content and a DISPLAY NAME. Main slugs the name flat (so `../../.ssh/config` cannot traverse), stages it in a `0o700` directory, and puts that path on the clipboard. The renderer cannot name a file to stage, read one back, or learn the staging directory; staged copies are pruned at 30 minutes and swept on quit **and** on next launch | `clipboardCopy.js`, `clipboardStage.js`, `clipboardWrite.js` | +| **The merge write takes TEXT, never a path** | `merge:write` is the only handler that writes over a file the user already had, and it can only write the `$MERGED` path main was launched with by `git mergetool`. The renderer sends the resolved text; there is no argument for a filename. With no merge launch in progress the handler writes nothing at all, and one launch permits one write | `mergeSession.js`, `cliRoute.js` | | **git is read-only, and main owns the repository** | `git:root` and `git:show` are the whole surface. The renderer names a REVISION and a repo-relative path; main computes the repository root itself and builds the argv, so no handler accepts a directory, a command or a git argument. The vocabulary is `rev-parse` and `show`, so nothing that reaches the network is callable, and a refusal comes back as `refused` without saying which input was rejected | `gitRoute.js`, `gitRepo.js` | | **The tray settings are booleans** | `tray:supported`, `app:startAtLogin` and `app:setStartAtLogin` take and return nothing but booleans. The login item registers `process.execPath` — main's own — with a fixed `--hidden` argument; the renderer never supplies an executable, an argument or a registry key, and there is no handler that would accept one | `tray.js`, `trayCore.js` | | **A stored address cannot become a header** | `share:setTrustedEmail` refuses anything carrying CR/LF, a comma, a semicolon, angle brackets or whitespace, **before it reaches disk** — otherwise a stored address would inject a second header into the hand-off URL. A restored backup's `email` field is dropped if it fails the same check | `trustedKeys.js`, `mailAddress.js`, `shareCore.js` | diff --git a/docs/roadmap.md b/docs/roadmap.md index 2f32b11..cd258e7 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -1,7 +1,7 @@ # Roadmap Roadmap board — four tracks. Spreadsheet · finance: amounts read as amounts, delta and net variance, reading a big diff, caps that announce themselves. Diagrams: sequence · gantt · pie, click a change to pan to it. Comparing more: folder compare, image pairs, three-way merge — a decision first. Signing: macOS Developer ID, Windows deferred. + alt="Roadmap board — five tracks. Developer workflow, shipped: dependencies as lockfiles, compare a git revision, resolve a merge. Spreadsheet · finance: amounts read as amounts, delta and net variance, reading a big diff, caps that announce themselves. Diagrams: sequence · gantt · pie, click a change to pan to it. Comparing more: folder compare, image pairs, three-way merge — a decision first. Signing: macOS Developer ID, Windows deferred."> Board is `docs/brand/roadmap.svg` — hand-authored, edit it alongside the sections below. @@ -175,6 +175,47 @@ flowchart LR --- +## Developer workflow + +**Built.** The three artifacts a developer spends the day on, read the way the +rest of the app reads a spreadsheet: as meaning, not as lines. + +```mermaid +flowchart LR + subgraph deps["dependencies"] + l["utils/lockfile/ — npm · pnpm · yarn · go · composer"] + d["lockDiff.js — added · removed · bumped
direct vs carried"] + l --> d + end + subgraph git["revisions"] + g["main/gitRepo.js — fenced rev-parse + show"] + c["compare HEAD~1:path"] + g --> c + end + subgraph merge["merge"] + m["mergeConflicts.js — regions + four resolutions"] + w["mergeSession.js — the one write"] + m --> w + end +``` + +- **Dependencies** — a lockfile pair reads as the packages that moved and which + of them you asked for. Nothing is fetched; every fact is in the file +- **Revisions** — `diffbro compare HEAD~1:src/app.js src/app.js`. `git show` + behind a fence: fixed argv, no shell, the repo root computed in main, hooks + and the fsmonitor disabled, every inherited `GIT_*` dropped +- **Merge** — `git mergetool` now finishes. This CROSSES "Diff Bro never writes + files", deliberately: the app had already registered for the job. Main writes + only the `$MERGED` path it was launched with, the renderer sends text and + never a path, and the launcher waits so `trustExitCode` is honest + +**Open.** TOML lockfiles (`Cargo.lock`, `poetry.lock`) need a parser this repo +does not have. A revision PICKER — the app takes a revision, it is not a git +client. Breaking-change classification for OpenAPI and GraphQL, which is the +same thesis pointed at a contract. + +--- + ## Comparing more **Open — and gated on a decision, not a build.** Each of these is its own @@ -186,7 +227,6 @@ flowchart LR direction TB f["folder compare
two trees aligned by path"] i["image pairs
side-by-side · onion-skin · pixel Δ"] - t["three-way merge
an EDITOR, not a viewer"] end ``` @@ -196,9 +236,6 @@ flowchart LR - **image pairs** — the adapter registry already takes a `{ kind }` comparable, so the seam exists; which diff to draw (side-by-side, onion-skin, pixel delta) is the decision -- **three-way merge** — WRITES files, which Diff Bro deliberately never does - today; the same line the Diagrams track holds ("editing a diagram from the - diff view" is out of scope). Crossing it is the decision, not the code --- diff --git a/docs/security.md b/docs/security.md index 37dfa3e..4028bb7 100644 --- a/docs/security.md +++ b/docs/security.md @@ -47,6 +47,25 @@ repo-local config has been an execution vector before: - A blob is capped at 32 MB — past that the streamed reader is the right tool, and a revision is no reason to hold more in memory than a file would be. +## Writing a merged file + +Diff Bro writes over a file you already had in exactly one situation: a +`git mergetool` run it was invoked for. Everything else it produces is a NEW +file you chose the location of. + +The fence is the shape of the surface rather than a check inside it. Main +remembers the `$MERGED` path from the LAUNCH argv; `merge:write` takes the +resolved **text** and nothing else, so there is no argument through which the +renderer could name a file — the same shape as `clipboard:writeFile`, which +takes bytes and a display name. With no mergetool launch there is no path held, +so the handler writes nothing at all whatever arrives, and a session is spent +once used. + +`mergetool.diffbro.trustExitCode` is `true`, which is only honest because the +merge launcher WAITS for `$MERGED` to change before it exits. The app is +single-instance, so the launch itself returns immediately; a script that exited +there would tell git the conflict was resolved before anyone had looked at it. + ## File access (compromised-renderer threat model) All filesystem access lives in the main process; the renderer only asks. Because diff --git a/specs/2026-08-09-developer-workflow/plan.md b/specs/2026-08-09-developer-workflow/plan.md index 1ad0091..d36dcc9 100644 --- a/specs/2026-08-09-developer-workflow/plan.md +++ b/specs/2026-08-09-developer-workflow/plan.md @@ -3,7 +3,7 @@ | | | |---|---| | **Status** | in-progress | -| **Progress** | 9 / 13 — phases 1 and 2 shipped | +| **Progress** | 13 / 13 — all three phases shipped | | **Branch** | `feat/developer-workflow` | | **Started** | 2026-08-09 | | **Finished** | — | @@ -222,10 +222,10 @@ rather than throwing. **Phase 3 — finishing the merge** -- [ ] 10. `utils/mergeConflicts.js` — regions and the four resolutions -- [ ] 11. The merge view -- [ ] 12. `mergetool` registration writing `$MERGED`, `trustExitCode=true` -- [ ] 13. e2e through a real `git mergetool`, docs, roadmap + SVG +- [x] 10. `utils/mergeConflicts.js` — regions and the four resolutions +- [x] 11. The merge view +- [x] 12. `mergetool` registration writing `$MERGED`, `trustExitCode=true` +- [x] 13. e2e through a real `git mergetool`, docs, roadmap + SVG ## Decisions diff --git a/src/shared/i18n/en-XA.json b/src/shared/i18n/en-XA.json index a961325..936c701 100644 --- a/src/shared/i18n/en-XA.json +++ b/src/shared/i18n/en-XA.json @@ -658,7 +658,7 @@ "gitToolSettings": { "git": "[Ğĩţ ·ø]", "openGitComparisonsInDiff": "[Ōƥéń ğĩţ çōɱƥàřĩşōńş ĩń Đĩƒƒ Ɓřō ·øé·øé·øé·ø]", - "aMergeOpensItsTwo": "[À ɱéřğé ōƥéńş ĩţş ţŵō çōńƒłĩçţĩńğ ṽéřşĩōńş şĩđé ƀŷ şĩđé ţō řéàđ. Đĩƒƒ Ɓřō đōéşń’ţ ŵřĩţé ţĥé ɱéřğéđ ƒĩłé, şō ğĩţ şţĩłł àşķş ŷōū ŵĥéţĥéř ţĥé ɱéřğé ŵōřķéđ. ·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·]", + "aMergeOpensItsTwo": "[À ɱéřğé ōƥéńş ĩţş ţŵō çōńƒłĩçţĩńğ ṽéřşĩōńş, ţàķéş à çĥōĩçé ƥéř çōńƒłĩçţ, àńđ ŵřĩţéş ţĥé ɱéřğéđ ƒĩłé ƀàçķ — şō ğĩţ ĩş ţōłđ ţĥé ɱéřğé ĩş đōńé řàţĥéř ţĥàń àşķĩńğ ŷōū. ·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·øé·]", "intro": "[Ṁàķéş {difftool} àńđ {mergetool} ōƥéń ţĥé çōɱƥàřĩşōń ĥéřé ĩńşţéàđ ōƒ ĩń ţĥé ţéřɱĩńàł. ·øé·øé·øé·øé·øé·øé·øé·øé·øé·]", "notOnPath": "[ğĩţ ĩşń’ţ ōń ŷōūř ƤÀŢĤ, şō ţĥéřé ĩş ńōţĥĩńğ ţō řéğĩşţéř. ·øé·øé·øé·øé·øé·øé]", "configFailed": "[Çōūłđ ńōţ çĥàńğé ţĥé ğĩţ çōńƒĩğūřàţĩōń. ·øé·øé·øé·øé·ø]" diff --git a/src/shared/i18n/en.json b/src/shared/i18n/en.json index 7d017e9..438ebb3 100644 --- a/src/shared/i18n/en.json +++ b/src/shared/i18n/en.json @@ -658,7 +658,7 @@ "gitToolSettings": { "git": "Git", "openGitComparisonsInDiff": "Open git comparisons in Diff Bro", - "aMergeOpensItsTwo": "A merge opens its two conflicting versions side by side to read. Diff Bro doesn’t write the merged file, so git still asks you whether the merge worked.", + "aMergeOpensItsTwo": "A merge opens its two conflicting versions, takes a choice per conflict, and writes the merged file back — so git is told the merge is done rather than asking you.", "intro": "Makes {difftool} and {mergetool} open the comparison here instead of in the terminal.", "notOnPath": "git isn’t on your PATH, so there is nothing to register.", "configFailed": "Could not change the git configuration." From fa16f2bd1360460bfcff3e67cc6547ca387453d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Mon, 10 Aug 2026 00:52:55 +0300 Subject: [PATCH 15/16] fix: everything the audit found, blocking first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two agents read and drove this branch. Four blocking findings, two of them verified data loss with bytes on the table. BLOCKING — a binary conflict was destroyed by one click. git calls the mergetool for binary conflicts too and leaves them with no markers. Reading $MERGED as UTF-8 turned ff fe into ef bf bd ef bf bd; zero conflicts read as "nothing left to decide", which ENABLED Save, and trustExitCode=true then told git it worked. mergeGuards.js refuses a NUL in the first 8 KB — the same sniff files.js already uses — and refuses a file with no markers at all, because that is not "already resolved", it is a file this tool has no business rewriting. Save is additionally disabled when there is nothing to decide. BLOCKING — a mixed-EOL conflict wrote the markers back. git writes LF markers into a CRLF file; picking one line ending for the whole file found ZERO conflicts in one, told the user there was nothing to resolve, and wrote the unresolved file back. Every line now keeps the ending it arrived with, so the round trip is byte for byte. BLOCKING — git:show was an arbitrary-file-read primitive. It took a renderer-supplied path, used its dirname as git's cwd, and bypassed the allowlist files.js exists to enforce — for anything committed in any repository on the machine. It was also DEAD: nothing called it, because the shipped feature is the CLI. Deleted, along with the doc rows that asserted a fence it did not have. BLOCKING — `git mergetool` opened with both panes empty. The merge route returned before allowCliPath, so file:read refused both sides; the refusal is a resolved value, not a throw, so the user was not even told. HIGH — cancelling left the write armed and hung git. endMerge ran only after a successful write, so an abandoned session kept $MERGED live for the process lifetime and a later write still landed. Cancel now spends the session and releases the launcher. The launcher watches a SENTINEL rather than $MERGED's own mtime: a resolution writing identical bytes changed neither size nor timestamp, and `ls -l` only resolves to the minute — either way it waited for ever. It is bounded now too. MEDIUM — version text failed the 4.5 floor on 14 of 20 themes (nord 1.80) and the bumped keyline was invisible on 8 light grounds (contrast 1.35): raw --danger-border/--success-text are 3:1 NON-TEXT roles, and --warning-bg is a background role. Both now use the --dg-* diff roles and the color-mix ui.css already uses for the status band. The deps viewer is registered in theme-sweep's SURFACES, so this is measured next time rather than reviewed. Also: lockDiff sorted versions as STRINGS, pairing 1.10.0 with 1.9.0 and reporting a downgrade that never happened; registerGitTool clobbered a pre-existing merge launcher the difftool target was guarded against; a refused revision path failed silently into the crash log; DepsStatusBand dereferenced a field its validator did not require; mergeInvocation was byte-identical to invocation; four comment walls restating docs and one stale claim about `--` that the code never used. Red-verified: reverting the two guards makes the new e2e fail with the dialog open on a binary file. npm run check green — 3201 tests, coverage 95.01/88.03/95.15/96.08. Co-Authored-By: Claude Opus 5 --- docs/ipc-security.md | 1 - docs/security.md | 9 +- e2e/git-compare.spec.mjs | 11 ++- e2e/merge-resolve.spec.mjs | 72 +++++++++++++- scripts/theme-sweep.mjs | 39 ++++++++ src/main/cliRoute.js | 43 +++++++-- src/main/gitCliFiles.js | 8 +- src/main/gitMergeTool.js | 18 +++- src/main/gitRepo.js | 20 +--- src/main/gitRoute.js | 48 ---------- src/main/gitTool.js | 14 +-- src/main/index.js | 2 - src/main/mergeGuards.js | 24 +++++ src/main/mergeSession.js | 42 +++++++-- src/preload/index.js | 3 +- .../src/components/DepsStatusBand.vue | 4 +- .../src/components/styles/DepsDiffViewer.css | 18 +++- .../features/merge/components/MergeDialog.vue | 2 +- .../merge/components/styles/MergeDialog.css | 10 +- src/renderer/src/features/merge/mergeStore.js | 11 ++- src/renderer/src/utils/lockfile/lockDiff.js | 14 ++- src/renderer/src/utils/mergeConflicts.js | 88 ++++++++++-------- src/shared/i18n/en-XA.json | 6 +- src/shared/i18n/en.json | 6 +- tests/main/gitTool.test.js | 21 ++++- tests/main/mergeGuards.test.js | Bin 0 -> 2104 bytes tests/main/mergeSession.test.js | 70 +++++++++++++- .../renderer/utils/lockfile/lockDiff.test.js | 10 ++ tests/renderer/utils/mergeConflicts.test.js | 25 +++++ 29 files changed, 465 insertions(+), 174 deletions(-) delete mode 100644 src/main/gitRoute.js create mode 100644 src/main/mergeGuards.js create mode 100644 tests/main/mergeGuards.test.js diff --git a/docs/ipc-security.md b/docs/ipc-security.md index 97ddce7..0d2c515 100644 --- a/docs/ipc-security.md +++ b/docs/ipc-security.md @@ -88,7 +88,6 @@ that enforces each: | **The mail hand-off supplies no URL and no path** | `mail:handoff` takes fingerprints and text. Main resolves the addresses from the trust store, BUILDS the `mailto:` (`mailto.js`), and re-checks it with `isSafeMailtoUrl` before `shell.openExternal` — `mailto:` only, and an `attach`/`attachment` parameter is refused rather than ignored. The file it copies and reveals is the path it just sealed, never one round-tripped through the renderer | `mail.js`, `mailto.js`, `linkPolicy.js`, `mailAddress.js` | | **Copy as file takes bytes, never a path** | `clipboard:writeFile` receives content and a DISPLAY NAME. Main slugs the name flat (so `../../.ssh/config` cannot traverse), stages it in a `0o700` directory, and puts that path on the clipboard. The renderer cannot name a file to stage, read one back, or learn the staging directory; staged copies are pruned at 30 minutes and swept on quit **and** on next launch | `clipboardCopy.js`, `clipboardStage.js`, `clipboardWrite.js` | | **The merge write takes TEXT, never a path** | `merge:write` is the only handler that writes over a file the user already had, and it can only write the `$MERGED` path main was launched with by `git mergetool`. The renderer sends the resolved text; there is no argument for a filename. With no merge launch in progress the handler writes nothing at all, and one launch permits one write | `mergeSession.js`, `cliRoute.js` | -| **git is read-only, and main owns the repository** | `git:root` and `git:show` are the whole surface. The renderer names a REVISION and a repo-relative path; main computes the repository root itself and builds the argv, so no handler accepts a directory, a command or a git argument. The vocabulary is `rev-parse` and `show`, so nothing that reaches the network is callable, and a refusal comes back as `refused` without saying which input was rejected | `gitRoute.js`, `gitRepo.js` | | **The tray settings are booleans** | `tray:supported`, `app:startAtLogin` and `app:setStartAtLogin` take and return nothing but booleans. The login item registers `process.execPath` — main's own — with a fixed `--hidden` argument; the renderer never supplies an executable, an argument or a registry key, and there is no handler that would accept one | `tray.js`, `trayCore.js` | | **A stored address cannot become a header** | `share:setTrustedEmail` refuses anything carrying CR/LF, a comma, a semicolon, angle brackets or whitespace, **before it reaches disk** — otherwise a stored address would inject a second header into the hand-off URL. A restored backup's `email` field is dropped if it fails the same check | `trustedKeys.js`, `mailAddress.js`, `shareCore.js` | | **No injection sinks** | `v-html`, `eval`, `new Function`, `innerHTML` are ESLint-banned | `eslint.config.mjs` | diff --git a/docs/security.md b/docs/security.md index 4028bb7..6731bbd 100644 --- a/docs/security.md +++ b/docs/security.md @@ -31,9 +31,12 @@ The fence around it, because a repository someone cloned is untrusted input and repo-local config has been an execution vector before: - `execFile` with a FIXED argv, never a shell. -- The repository root is computed in MAIN, from a path the app already holds. A - renderer names a revision and a repo-relative path — never a directory, a - command, or a git argument. +- **No renderer reaches git at all.** There is no `git:*` IPC handler: the only + caller is the CLI, whose arguments main parsed itself, and the file it reads + out of a revision is staged and vouched for through `allowCliPath` like any + other path. An IPC that took a path from the renderer would be an + arbitrary-file-read primitive for anything committed in any repository on the + machine, which is exactly what `files.js`'s allowlist exists to prevent. - A revision is validated against a narrow pattern, may never begin with `-`, and is followed by `--end-of-options`; a path may not be absolute and may not contain `..`. diff --git a/e2e/git-compare.spec.mjs b/e2e/git-compare.spec.mjs index cc1d744..5bea4b6 100644 --- a/e2e/git-compare.spec.mjs +++ b/e2e/git-compare.spec.mjs @@ -1,4 +1,5 @@ import { test, expect, launchApp, freshUserDataDir, firstReadyPage } from './fixtures.mjs' +import { workerEnv } from './workerEnv.mjs' import { execFileSync } from 'node:child_process' import { spawn } from 'node:child_process' import { mkdtempSync, writeFileSync, rmSync } from 'node:fs' @@ -19,7 +20,13 @@ function makeRepo() { const git = (...args) => execFileSync('git', args, { cwd: dir, - env: { ...process.env, GIT_AUTHOR_NAME: 'T', GIT_AUTHOR_EMAIL: 't@e', GIT_COMMITTER_NAME: 'T', GIT_COMMITTER_EMAIL: 't@e' } + env: { + ...process.env, + GIT_AUTHOR_NAME: 'T', + GIT_AUTHOR_EMAIL: 't@e', + GIT_COMMITTER_NAME: 'T', + GIT_COMMITTER_EMAIL: 't@e' + } }) git('init', '-q', '-b', 'main') writeFileSync(join(dir, 'app.json'), '{\n "replicas": 3\n}\n') @@ -32,7 +39,7 @@ function makeRepo() { } function runCli(userDataDir, cwd, args) { - const env = { ...process.env } + const env = { ...workerEnv(userDataDir) } delete env.ELECTRON_RUN_AS_NODE return new Promise((resolve) => { const p = spawn(ELECTRON, [MAIN, `--user-data-dir=${userDataDir}`, ...args], { diff --git a/e2e/merge-resolve.spec.mjs b/e2e/merge-resolve.spec.mjs index ca69191..d0cb70b 100644 --- a/e2e/merge-resolve.spec.mjs +++ b/e2e/merge-resolve.spec.mjs @@ -1,4 +1,5 @@ import { test, expect, launchApp, freshUserDataDir, firstReadyPage } from './fixtures.mjs' +import { workerEnv } from './workerEnv.mjs' import { execFileSync, spawn } from 'node:child_process' import { mkdtempSync, readFileSync, writeFileSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' @@ -43,7 +44,7 @@ function conflictedRepo() { } function runMergetool(userDataDir, dir, file) { - const env = { ...process.env } + const env = { ...workerEnv(userDataDir) } delete env.ELECTRON_RUN_AS_NODE return new Promise((resolve) => { const p = spawn( @@ -69,6 +70,12 @@ test('resolves a real conflict and writes the merged file back', async () => { const dialog = page.getByRole('dialog', { name: 'Resolve merge conflicts' }) await expect(dialog).toBeVisible({ timeout: 20000 }) + // The two conflicting versions are what the README promises and what the + // dialog sits over — they used to open EMPTY, because the merge route + // returned before vouching for either path. + await expect(page.locator('.slot[data-side="left"] .name')).toContainText('app.txt') + await expect(page.locator('.slot[data-side="right"] .name')).toContainText('app.txt') + // Nothing may be written while a conflict is undecided. const save = page.getByTestId('merge-save') await expect(save).toBeDisabled() @@ -87,3 +94,66 @@ test('resolves a real conflict and writes the merged file back', async () => { rmSync(dir, { recursive: true, force: true }) } }) + +test('declining leaves the file exactly as git left it', async () => { + const { dir, file } = conflictedRepo() + const before = readFileSync(file) + const userDataDir = freshUserDataDir() + const app = await launchApp(userDataDir) + const page = await firstReadyPage(app) + try { + await runMergetool(userDataDir, dir, file) + const dialog = page.getByRole('dialog', { name: 'Resolve merge conflicts' }) + await expect(dialog).toBeVisible({ timeout: 20000 }) + await page.getByRole('button', { name: 'Cancel' }).click() + await expect(dialog).toHaveCount(0) + + // Untouched, markers and all — the reader said no. + expect(readFileSync(file).equals(before)).toBe(true) + } finally { + await app.close().catch(() => {}) + rmSync(dir, { recursive: true, force: true }) + } +}) + +// git calls the mergetool for a BINARY conflict too, and leaves it with no +// markers. Reading it as text turned every invalid byte into U+FFFD and one +// click wrote that over the file. +test('refuses a binary conflict instead of destroying it', async () => { + const dir = mkdtempSync(join(tmpdir(), 'diffbro-merge-bin-')) + const file = join(dir, 'blob.bin') + const bytes = Buffer.from([0x00, 0x01, 0x02, 0x07, 0xff, 0xfe, 0x00, 0x0a]) + writeFileSync(file, bytes) + const userDataDir = freshUserDataDir() + const app = await launchApp(userDataDir) + const page = await firstReadyPage(app) + try { + await runMergetool(userDataDir, dir, file) + await page.waitForTimeout(1500) + await expect(page.getByRole('dialog', { name: 'Resolve merge conflicts' })).toHaveCount(0) + expect(readFileSync(file).equals(bytes)).toBe(true) + } finally { + await app.close().catch(() => {}) + rmSync(dir, { recursive: true, force: true }) + } +}) + +// A file with no markers is not "already resolved" — it is a file this tool has +// no business rewriting. +test('refuses a file with no conflict markers', async () => { + const dir = mkdtempSync(join(tmpdir(), 'diffbro-merge-plain-')) + const file = join(dir, 'plain.txt') + writeFileSync(file, 'nothing to resolve\n') + const userDataDir = freshUserDataDir() + const app = await launchApp(userDataDir) + const page = await firstReadyPage(app) + try { + await runMergetool(userDataDir, dir, file) + await page.waitForTimeout(1500) + await expect(page.getByRole('dialog', { name: 'Resolve merge conflicts' })).toHaveCount(0) + expect(readFileSync(file, 'utf8')).toBe('nothing to resolve\n') + } finally { + await app.close().catch(() => {}) + rmSync(dir, { recursive: true, force: true }) + } +}) diff --git a/scripts/theme-sweep.mjs b/scripts/theme-sweep.mjs index a3cdfad..d5c42b3 100644 --- a/scripts/theme-sweep.mjs +++ b/scripts/theme-sweep.mjs @@ -170,7 +170,46 @@ async function clearComparison(page) { } // Each surface: how to open it, and the pairs that carry meaning once open. +const LOCKFILE = (version) => + JSON.stringify( + { + lockfileVersion: 3, + packages: { + '': { dependencies: { vue: '^3.4.0' } }, + 'node_modules/vue': { version, license: 'MIT' }, + 'node_modules/@vue/shared': { version } + } + }, + null, + 2 + ) + const SURFACES = [ + { + name: 'deps-diff', + // Reached through PASTE mode, because a lockfile is recognised by its NAME + // and paste lets a side be named. Three inks a table cannot settle: the two + // versions are text at the reading floor (the raw --dg-* roles are 3:1 + // non-text and fell to 1.80 on nord), and the row's status keyline is the + // only thing separating a bump from an addition. + open: async (page) => { + await page.getByRole('button', { name: 'Paste mode' }).click() + const names = page.getByPlaceholder('Name this side') + await names.first().fill('package-lock.json') + await names.nth(1).fill('package-lock.json') + await page.getByPlaceholder('Paste original text here').fill(LOCKFILE('3.4.21')) + await page.getByPlaceholder('Paste changed text here').fill(LOCKFILE('3.5.13')) + await page.getByRole('button', { name: 'Compare', exact: true }).click() + await page.locator('.deps-row').first().waitFor() + }, + close: (page) => page.getByRole('button', { name: 'Clear', exact: true }).click(), + probes: { + 'package name': ['.deps-name', TEXT], + 'old version': ['.deps-move .del', TEXT], + 'new version': ['.deps-move .add', TEXT], + 'asked-for tag': ['.deps-tag', DIM] + } + }, { name: 'launcher-compose', window: 'launcher', diff --git a/src/main/cliRoute.js b/src/main/cliRoute.js index ffc18c8..1bd16ec 100644 --- a/src/main/cliRoute.js +++ b/src/main/cliRoute.js @@ -11,8 +11,9 @@ import { gitToolStatus, registerGitTool, sweepGitTemp, unregisterGitTool } from import { ensureMainWindow } from './quickLook' import { allowCliPath } from './files' import { fileAtRevision, isRevisionSide, REVISION_ERROR_KEYS } from './gitCliFiles' -import { beginMerge, writeMerged } from './mergeSession' +import { beginMerge, cancelMerge, writeMerged } from './mergeSession' import { readFileSync } from 'node:fs' +import { hasConflictMarkers, isBinaryBuffer } from './mergeGuards' import { t } from './i18n' // A command can arrive before any window exists (a cold `diffbro compare …`), @@ -70,11 +71,15 @@ async function withRevisionsResolved(command, cwd) { } function deliverResolved(command, cwd) { - withRevisionsResolved(command, cwd).then((ready) => { - if (!ready) return - ready.files.forEach(allowCliPath) - deliver(ready) - }) + withRevisionsResolved(command, cwd) + .then((ready) => { + if (!ready) return + ready.files.forEach(allowCliPath) + deliver(ready) + }) + // A path the fence refuses THROWS. Without this it opened nothing, said + // nothing, and landed in the crash log instead. + .catch(() => process.stderr.write(`${t('cliErrors.refused')}\n`)) } const needsGit = (command) => command?.name === 'compare' && command.files.some(isRevisionSide) @@ -82,14 +87,32 @@ const needsGit = (command) => command?.name === 'compare' && command.files.some( // A mergetool launch: main REMEMBERS the path git wants written and sends the // renderer the conflicted text, never the path. What comes back is text. function routeMerge(command) { - let content + let buffer try { - content = readFileSync(command.merged, 'utf8') + buffer = readFileSync(command.merged) } catch { - process.stderr.write(`${t('cliErrors.not-in-revision')}\n`) + process.stderr.write(`${t('cliErrors.merge-unreadable')}\n`) + return + } + // git calls the mergetool for a BINARY conflict too, and leaves it with no + // markers. Decoding one as text turns every invalid byte into U+FFFD, and + // writing that back destroys the file — so it never reaches the renderer. + if (isBinaryBuffer(buffer)) { + process.stderr.write(`${t('cliErrors.merge-binary')}\n`) + return + } + const content = buffer.toString('utf8') + // Nothing to decide is not the same as "resolved": a file with no markers is + // one this tool has no business rewriting. + if (!hasConflictMarkers(content)) { + process.stderr.write(`${t('cliErrors.merge-no-conflicts')}\n`) return } beginMerge(command) + // Both sides are files main just vouched for; without this file:read refuses + // them and the two panes open empty. + allowCliPath(command.local) + allowCliPath(command.remote) deliver({ name: 'merge', local: command.local, remote: command.remote, content }) } @@ -148,6 +171,8 @@ export function registerCliIpc() { // The renderer's ONLY say in the merge is the text. It cannot name a file: // main has held that path since the launch. ipcMain.handle('merge:write', (e, text) => writeMerged(text)) + // Declining is an answer too: it releases the launcher and spends the session. + ipcMain.handle('merge:cancel', () => cancelMerge()) ipcMain.handle('cli:status', () => shimStatus(where())) ipcMain.handle('cli:install', async () => (await confirmed(t('dialog.cliInstall.message'), t('dialog.cliInstall.detail'))) diff --git a/src/main/gitCliFiles.js b/src/main/gitCliFiles.js index 400a766..29d6715 100644 --- a/src/main/gitCliFiles.js +++ b/src/main/gitCliFiles.js @@ -1,9 +1,5 @@ -// A `revision:path` side of a `diffbro compare`, turned into a real file the -// rest of the app can open. Only MAIN talks to git, and only main writes these -// copies — the renderer receives a path like any other. -// -// The copies live in a temp directory named the way gitTool's do, so the same -// sweep clears both. +// A `revision:path` side of a `diffbro compare`, staged as a real file. The +// copies take gitTool's temp prefix so its sweep clears them too. import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { basename, join } from 'node:path' diff --git a/src/main/gitMergeTool.js b/src/main/gitMergeTool.js index 9657960..821cf4b 100644 --- a/src/main/gitMergeTool.js +++ b/src/main/gitMergeTool.js @@ -8,6 +8,10 @@ import { MARK } from './gitToolMark' import { shQuote } from './shellQuote' +// Two hours: longer than any real conflict takes, short enough that a window +// closed and forgotten does not hold a terminal for ever. +const WAIT_SECONDS = 7200 + /** Beside the difftool launcher, so removing either leaves the other alone. */ export function gitMergeTarget(target) { return `${target}-merge` @@ -17,13 +21,21 @@ export function gitMergeTarget(target) { export function gitMergeScript(exePath, entryPath = null) { return `#!/bin/sh ${MARK} -before=$(ls -l "$3" 2>/dev/null) +done_file="$3.diffbro-merge-done" +rm -f "$done_file" ${shQuote(exePath)}${entryPath ? ` ${shQuote(entryPath)}` : ''} mergetool "$1" "$2" "$3" || exit 1 # Wait for the reader. Exiting here would tell git the merge was resolved before -# anyone had looked at it. -while [ "$(ls -l "$3" 2>/dev/null)" = "$before" ]; do +# anyone had looked at it. The bound is what stops a closed window hanging the +# terminal for ever. +waited=0 +while [ ! -f "$done_file" ] && [ "$waited" -lt ${WAIT_SECONDS} ]; do sleep 1 + waited=$((waited + 1)) done +if [ ! -f "$done_file" ]; then exit 1; fi +verdict=$(cat "$done_file" 2>/dev/null) +rm -f "$done_file" +[ "$verdict" = "written" ] || exit 1 exit 0 ` } diff --git a/src/main/gitRepo.js b/src/main/gitRepo.js index 7d62c5f..0e484fd 100644 --- a/src/main/gitRepo.js +++ b/src/main/gitRepo.js @@ -1,20 +1,8 @@ -// Reading a file out of a git revision, so a comparison can start from the -// repository rather than from two files someone had to produce first. +// Reading a file out of a git revision. READ ONLY: `rev-parse` and `show` are +// the whole vocabulary, so nothing here can ask git to reach the network. // -// READ ONLY. Nothing here writes, stages or commits, and no subcommand that can -// reach the network is reachable from it — `show` and `rev-parse` are the whole -// vocabulary (rule 1: the offline guarantee is not weakened by a subprocess that -// opens no socket, but only because nothing here can ask git to open one). -// -// The fence, and why each part of it is there (rule 7): -// * execFile with a FIXED argv, never a shell — the app already spawns git -// this way in gitTool.js -// * the repo root is computed HERE and never accepted from the renderer -// * a revision is validated before it reaches argv, and can never begin `-` -// * --end-of-options, so even a revision that slipped through cannot be read -// as a flag -// * the HARDENING below, because a repository someone cloned is untrusted -// input and repo-local config has been an execution vector before +// The fence is in docs/security.md; what is not obvious from the code is why +// each hardening flag is there, so those are noted at the constant itself. import { execFile } from 'node:child_process' /** diff --git a/src/main/gitRoute.js b/src/main/gitRoute.js deleted file mode 100644 index bb3e780..0000000 --- a/src/main/gitRoute.js +++ /dev/null @@ -1,48 +0,0 @@ -// The git surface the renderer may reach, and nothing else. Two questions — -// "is this a repository?" and "what did this file look like at that revision?" -// — and both are answered by src/main/gitRepo.js behind its fence. -// -// The renderer names a REVISION and a path; it never names a command, a cwd or -// a git argument. Main resolves the repository from the path it was given, so a -// renderer cannot point git at a directory of its choosing. -import { ipcMain } from 'electron' -import { dirname } from 'node:path' -import { readBlobArgs, repoRootArgs, resolveRevisionArgs, runGitIn } from './gitRepo' - -const MAX_BLOB = 32 * 1024 * 1024 - -async function repoRootOf(filePath) { - if (typeof filePath !== 'string' || !filePath) return null - const res = await runGitIn(repoRootArgs(), dirname(filePath)) - return res.ok && res.stdout.trim() ? res.stdout.trim() : null -} - -/** - * The file as it stood at a revision, with the repository worked out from the - * file itself. - */ -async function readAtRevision(filePath, revision, relPath) { - const root = await repoRootOf(filePath) - if (!root) return { error: 'not-a-repo' } - const resolved = await runGitIn(resolveRevisionArgs(revision), root) - if (!resolved.ok) return { error: 'no-such-revision' } - const blob = await runGitIn(readBlobArgs(revision, relPath), root) - if (!blob.ok) return { error: 'not-in-revision' } - if (blob.stdout.length > MAX_BLOB) return { error: 'too-large' } - return { content: blob.stdout, commit: resolved.stdout.trim().slice(0, 12) } -} - -export function registerGitIpc() { - ipcMain.handle('git:root', async (e, filePath) => ({ root: await repoRootOf(filePath) })) - - ipcMain.handle('git:show', async (e, payload) => { - const { path: filePath, revision, relPath } = payload ?? {} - try { - return await readAtRevision(filePath, revision, relPath) - } catch { - // An unsafe revision or path throws inside the fence; the renderer gets a - // refusal, never the reason it was refused. - return { error: 'refused' } - } - }) -} diff --git a/src/main/gitTool.js b/src/main/gitTool.js index 0c65ef0..ee35c3a 100644 --- a/src/main/gitTool.js +++ b/src/main/gitTool.js @@ -68,13 +68,6 @@ exec ${shQuote(exePath)}${entryPath ? ` ${shQuote(entryPath)}` : ''} difftool "$ // so the copies keep the real filename. const invocation = (script) => `"${script}" "$LOCAL" "$REMOTE" "$MERGED"` -// A mergetool has to WAIT. The app is single-instance, so the launch returns as -// soon as the running window has been told — if the script exited there, git -// would read the merge as finished before the reader had chosen anything, and -// trustExitCode would be a lie. So it polls $MERGED's modification time and only -// then reports success. -const mergeInvocation = (script) => `"${script}" "$LOCAL" "$REMOTE" "$MERGED"` - /** * The registration, as git config argument vectors. * @@ -87,7 +80,7 @@ export function registerArgs(script, mergeScript = script) { const cmd = invocation(script) return [ ['config', '--global', `difftool.${GIT_TOOL_NAME}.cmd`, cmd], - ['config', '--global', `mergetool.${GIT_TOOL_NAME}.cmd`, mergeInvocation(mergeScript)], + ['config', '--global', `mergetool.${GIT_TOOL_NAME}.cmd`, invocation(mergeScript)], // The merge script waits for $MERGED to change before it exits, so a clean // exit now MEANS resolved and git can be told to believe it. ['config', '--global', `mergetool.${GIT_TOOL_NAME}.trustExitCode`, 'true'], @@ -126,6 +119,9 @@ export function runGit(args) { }) } +// Somebody else's file of the same name — theirs to keep. +const occupied = (file) => existsSync(file) && !looksLikeOurs(file) + const looksLikeOurs = (file) => { try { return readFileSync(file, 'utf8').includes(MARK) @@ -160,7 +156,7 @@ export async function registerGitTool({ exePath, home, platform, localAppData, e const mergeTarget = gitMergeTarget(gitToolTarget({ platform, home, localAppData })) if (!(await git(['--version'])).ok) return { ok: false, error: 'git is not on your PATH.' } try { - if (existsSync(target) && !looksLikeOurs(target)) { + if (occupied(target) || occupied(mergeTarget)) { return { ok: false, error: 'A different diffbro-git exists there.' } } mkdirSync(dirname(target), { recursive: true }) diff --git a/src/main/index.js b/src/main/index.js index 4a95ed3..fff1776 100644 --- a/src/main/index.js +++ b/src/main/index.js @@ -15,7 +15,6 @@ import { registerHashIpc } from './hashTools' import { backupIfDue, registerBackupIpc } from './backupRoute' import { readSettings, readSnippetStore, setBackupHook } from './appData' import { registerShareIpc } from './share' -import { registerGitIpc } from './gitRoute' import { registerMailIpc } from './mail' import { registerKeyExchangeIpc } from './keyExchange' import { registerClipboardCopyIpc } from './clipboardCopy' @@ -107,7 +106,6 @@ async function boot() { function startApp(draftPath) { installNetworkKillSwitch() registerAppDataIpc() - registerGitIpc() registerDemoIpc() registerQuickLookFocusIpc() loadLocale(readSettings().locale) // before installMenu: it builds from this diff --git a/src/main/mergeGuards.js b/src/main/mergeGuards.js new file mode 100644 index 0000000..2813e29 --- /dev/null +++ b/src/main/mergeGuards.js @@ -0,0 +1,24 @@ +// What a mergetool run refuses before the renderer ever sees it. Pure, so both +// refusals are testable without a repository. + +/** + * A NUL byte in the first 8 KB means this is not text — the same sniff + * files.js uses. git calls the mergetool for a binary conflict as readily as a + * text one, and decoding one as UTF-8 replaces every invalid byte, so writing + * it back would destroy the file. + */ +export function isBinaryBuffer(buffer) { + return buffer.subarray(0, 8192).includes(0) +} + +// Seven characters at the start of a line, then end-of-line or a space. +const OPENS = /^<<<<<<<(?: |$)/m + +/** + * Whether the file git handed over actually holds a conflict. Without this a + * file with none reads as "nothing left to decide", which enables Save and + * writes it straight back — telling git a merge succeeded that never happened. + */ +export function hasConflictMarkers(text) { + return OPENS.test(String(text ?? '')) +} diff --git a/src/main/mergeSession.js b/src/main/mergeSession.js index 158a3eb..48f69af 100644 --- a/src/main/mergeSession.js +++ b/src/main/mergeSession.js @@ -1,17 +1,18 @@ // The one place Diff Bro writes a file it did not create: the `$MERGED` path -// git handed it on the command line of a `git mergetool` run. -// -// The fence is the shape of the surface, not a check inside it. Main holds the -// path from launch; the renderer sends the resolved TEXT and nothing else, so -// there is no argument through which it could name a file. This mirrors -// clipboard:writeFile, which takes bytes and a display name and never a path. -// -// Nothing else in the app writes over a user's file, and nothing here writes -// unless a mergetool launch put a path in this module first. +// git handed it. Main holds that path from launch and the renderer sends only +// TEXT, so there is no argument through which it could name a file. import { writeFileSync } from 'node:fs' let pending = null +/** + * The file the launcher watches. Comparing $MERGED's own size and timestamp was + * not enough: a resolution that writes the same bytes back changes neither, and + * `ls -l` only resolves to the minute — either way the launcher waited forever. + * A sentinel appears exactly once, whatever the resolution turned out to be. + */ +export const doneSentinel = (merged) => `${merged}.diffbro-merge-done` + /** Remembered from the launch argv, never from a message. */ export function beginMerge({ merged, local, remote }) { pending = { merged, local, remote } @@ -22,6 +23,23 @@ export function mergeInProgress() { return pending } +/** + * The reader declined. The session is spent — an abandoned launch must not leave + * a path armed for the life of the process — and the launcher is released so + * `git mergetool` stops waiting on a decision that is not coming. + */ +export function cancelMerge() { + if (!pending) return { ok: false } + const { merged } = pending + pending = null + try { + writeFileSync(doneSentinel(merged), 'cancelled', 'utf8') + } catch { + // The launcher times out on its own; a sentinel it cannot read is not fatal. + } + return { ok: true } +} + export function endMerge() { pending = null } @@ -41,5 +59,11 @@ export function writeMerged(text) { } const path = pending.merged endMerge() + try { + writeFileSync(doneSentinel(path), 'written', 'utf8') + } catch { + // Falling back to the launcher's own timeout is better than failing a write + // that already landed. + } return { ok: true, path } } diff --git a/src/preload/index.js b/src/preload/index.js index 1050179..9f4d5dc 100644 --- a/src/preload/index.js +++ b/src/preload/index.js @@ -4,9 +4,8 @@ contextBridge.exposeInMainWorld('api', { openFile: (side, format) => ipcRenderer.invoke('file:open', side, format), readClipboardFiles: () => ipcRenderer.invoke('clipboard:readFiles'), readFile: (path, opts) => ipcRenderer.invoke('file:read', path, opts), - gitRoot: (path) => ipcRenderer.invoke('git:root', path), - gitShow: (payload) => ipcRenderer.invoke('git:show', payload), writeMerged: (text) => ipcRenderer.invoke('merge:write', text), + cancelMerge: () => ipcRenderer.invoke('merge:cancel'), // `format` names a row of main's own export table; never an extension. exportDiffFile: (payload) => ipcRenderer.invoke('diff:exportFile', payload), // Streamed comparison: files too large to hold are indexed by line in main diff --git a/src/renderer/src/components/DepsStatusBand.vue b/src/renderer/src/components/DepsStatusBand.vue index 8c7dedc..3e89065 100644 --- a/src/renderer/src/components/DepsStatusBand.vue +++ b/src/renderer/src/components/DepsStatusBand.vue @@ -6,7 +6,7 @@ defineProps({ result: { type: Object, required: true, - validator: shaped('stats', 'directCount', 'knowsDirect') + validator: shaped('stats', 'rows', 'directCount', 'knowsDirect') } }) @@ -22,8 +22,6 @@ defineProps({ {{ $t('depsDiffViewer.countDowngraded', result.stats.downgraded) }} - {{ $t('depsDiffViewer.askedFor') }} {{ result.directCount }} diff --git a/src/renderer/src/components/styles/DepsDiffViewer.css b/src/renderer/src/components/styles/DepsDiffViewer.css index 65f7353..88147db 100644 --- a/src/renderer/src/components/styles/DepsDiffViewer.css +++ b/src/renderer/src/components/styles/DepsDiffViewer.css @@ -59,14 +59,17 @@ /* Status is on the LEFT EDGE rather than as a wash: a row already carries two coloured versions, and a third tint behind them reads as neither. */ .deps-row.added { - box-shadow: inset 3px 0 0 var(--success-text); + box-shadow: inset 3px 0 0 var(--dg-add); } .deps-row.removed { - box-shadow: inset 3px 0 0 var(--danger-border); + box-shadow: inset 3px 0 0 var(--dg-del); } +/* --warning-bg is a BACKGROUND role: as a keyline it fell to 1.35 on contrast + and under the 3.0 non-text floor on every light ground. --dg-chg is the + change role the diff panes use, and it is held to a floor on all 20. */ .deps-row.bumped, .deps-row.downgraded { - box-shadow: inset 3px 0 0 var(--warning-bg); + box-shadow: inset 3px 0 0 var(--dg-chg); } .deps-name { font-family: var(--font-mono); @@ -81,11 +84,16 @@ font-size: var(--font-sm); white-space: nowrap; } +/* A version is TEXT a reader reads, so it needs the 4.5 floor — the raw status + tokens are 3:1 non-text roles and fell to 1.80 on nord. Mixing toward the + theme's own ink is what .status-band already does, for exactly this reason. */ .deps-move .add { - color: var(--success-text); + color: color-mix(in srgb, var(--dg-add) 55%, var(--text)); + font-weight: 600; } .deps-move .del { - color: var(--danger-border); + color: color-mix(in srgb, var(--dg-del) 55%, var(--text)); + font-weight: 600; } /* Height from --chip-h, never from padding — the rule the Esc chip broke. */ .deps-tag { diff --git a/src/renderer/src/features/merge/components/MergeDialog.vue b/src/renderer/src/features/merge/components/MergeDialog.vue index 60a67ac..419c45f 100644 --- a/src/renderer/src/features/merge/components/MergeDialog.vue +++ b/src/renderer/src/features/merge/components/MergeDialog.vue @@ -78,7 +78,7 @@ const preview = (lines) => lines.join('\n')