Work a second ticket without rebuilding the world: the git layer (#108) - #168
Open
juanmaguitar wants to merge 8 commits into
Open
Work a second ticket without rebuilding the world: the git layer (#108)#168juanmaguitar wants to merge 8 commits into
juanmaguitar wants to merge 8 commits into
Conversation
Part of #108. This is the main-process half — branches, parking, and the diff base. The "Working on: #59234" switcher is a follow-up; nothing in `src/renderer/index.jsx` calls the new bridge yet, so the observable change today is that linking a ticket creates a branch and switching back to one restores its files. A site was one working tree and one implicit patch. Starting a second ticket meant either a second clone (~54 MB, another `npm install`, another first build) or piling both tickets into one tree and getting a patch that mixes them. The site is the expensive substrate; a ticket is a cheap branch on top of it. ## Two invariants **`trunk` is never committed to.** It stays the pristine snapshot every branch is diffed against. Work started on trunk is *carried* into a ticket branch — `git.branch({checkout: true})` only moves HEAD, so uncommitted edits come along and nothing is thrown away. **A ticket branch holds exactly one WIP commit.** Parking passes `parent: [baseOid]` explicitly rather than committing onto the previous WIP commit, so re-parking rewrites that commit instead of stacking a pile of saves nobody asked for. ## The diff base is the branch point, not `origin/trunk` The issue asks for `origin/trunk`. That is wrong, and `src/main.js` already said so: diffing against a trunk that has moved embeds reversed upstream changes and the patch applies nowhere. It cannot be the live `refs/heads/trunk` either, because `updateToLatestTrunk` moves that ref while existing branches stay where they were born. So each branch records the trunk oid it forked from, and `statusMatrix({ref: baseOid})` diffs against it — which also sidesteps `findMergeBase` on a `depth: 1` clone's truncated graph. The `origin/trunk` fetch that was performed and never used is deleted rather than put to work, saving a network round trip per "view patch". ## What this drags in - `readTrunkInfo` read `HEAD` to date the snapshot. On a ticket branch that is a WIP commit made minutes ago, so the staleness dot (#94) would never light up. It reads `refs/heads/trunk` now. - `discardChanges` checked out `trunk` by name, which would silently move the contributor to another ticket. It checks out whatever is current; parked work survives, since destroying a ticket is what deleting its branch is for. - `appliedPatch` and `updateIncomplete` describe the work, not the site, so they moved under the branch. Left at site level, ticket A's "patch applied · Revert" banner showed while on ticket B, offering to reverse A's hunks against B's tree. - The update runs from trunk, so it parks the ticket first — and returns to it afterwards. When it fails it clears the stored ticket and says in the log where the work is parked, rather than leaving the panel naming a ticket the worktree is no longer on while every patch comes out empty. ## Two things that cannot be allowed to lose work Leaving a **dirty trunk** cannot be parked (invariant 1) and `checkout({force})` would eat it, so switching refuses with `code: 'dirty-trunk'` for the caller to offer the honest options. `git.checkout` writes HEAD only after every file operation succeeds, so a failure part-way — an `EPERM` on Windows from an editor or an antivirus holding a file — leaves HEAD on the source branch over a half-swapped worktree. Parking then would commit the mixture over the good WIP commit, and parking rewrites. The error is tagged `stage: 'checkout'` (the contract `updateToLatestTrunk` already uses) and the site is marked mid-switch until it is reconciled. ## Patch generation no longer touches the index The `git.add` loop that staged every untracked file to get it into the diff, and never unstaged it (#85), is redundant: `statusMatrix` reports untracked files as `[path, 0, 2, 0]` on its own and the `head !== workdir` filter keeps them. Verified through the handler, with an assertion that the index is byte-identical afterwards. `staleStagedPaths` stays — parking stages the worktree, and indexes dirtied by earlier versions are still out there. File deletions are still dropped from generated patches. That is #85's other half, unchanged here and left to its own change: rewriting the patch generator in the commit that introduces branches would make a failure impossible to attribute. ## Migration `siteMeta[sitePath]` gains `branches` and `currentBranch`. A site with a linked ticket gets that branch created at the current trunk tip and its work carried onto it; a site without one gains an empty map. It runs only from handlers already about to move HEAD, never from a read — a list call can land while an install or the Playground server is running against that directory. Nothing is persisted when the git half fails, so a site on an unmounted volume retries instead of being stranded on the old shape forever. ## Tests 405 pass on `.nvmrc`'s Node and on Electron's. The new integration suite drives real repositories in `tmpdir`; the wiring tests cover the three new channels, the registry gate in front of them, and the mid-switch refusal. The behaviour changes to `readTrunkInfo`, `discardChanges`, per-branch applied state, and returning to the branch after an update were each confirmed to fail with the previous code. Reviewed with `/self-review` before this commit: 9 `[fix here]` findings, all fixed. Two `[follow-up]` remain — the renderer does not call the new bridge yet, and `mergeBranchMeta` inherits a read-modify-write race from the existing metadata pattern. One partial: a ticket switch still resolves an `invoke()` with no progress channel, so the window is quiet during a large checkout; streaming that needs the UI change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
juanmaguitar
force-pushed
the
juanmaguitar/working-on-a-second-ticket-means-rebuilding-the
branch
from
August 8, 2026 08:39
56fc7df to
4760128
Compare
…-on-a-second-ticket-means-rebuilding-the # Conflicts: # src/main.js
juanmaguitar
added a commit
that referenced
this pull request
Aug 8, 2026
… five minutes (#170) ## Why Two things keep going wrong on pull requests here, and neither is anybody's fault — nothing in the repo says otherwise. **Nobody says how to test the change by hand.** This app fails in the places `node --test` cannot reach: a real clone of `wordpress-develop`, a `node_modules` that takes minutes to install, an OS file dialog, a Windows path with a space in it. A green suite is not evidence the feature works, and a reviewer who has to guess how to drive the change usually does not drive it at all. **The descriptions are hard to read.** Detail arrives in the order it was discovered rather than the order a reviewer needs it, so the first screen is rarely the part that explains the change. The cost is not that the detail exists — it is that it sits in front of the summary. ## What changes A pull request template, plus the two rules it encodes, written down where agents and humans both read them. `.github/pull_request_template.md` — GitHub loads it into every new PR, including ones opened with `gh pr create` (as long as no `--body` replaces it). The organising rule is **a reviewer understands the change in five minutes**: Why, What changes, How to test this, Risks, Related stay visible, and everything deeper goes into a `<details>` block collapsed by default. Depth is not the enemy of a readable PR; depth *in the way* is — so detail moves rather than disappears. `AGENTS.md` gains two subsections under "Before opening a pull request": the shape of the description, and the manual-testing requirement spelled out (starting state, numbered steps in the words on screen, expected results stated so they can come out false, **what must not have happened**, platforms, and what could not be tested by hand). `CONTRIBUTING.md` gets the human-facing pointer and a fourth step in the pre-PR checklist. **Deliberately not in this PR:** any enforcement. No workflow fails a PR with a missing section — the same reasoning that keeps AI review off CI here (a public repo, no credentials) applies, and a bot that checks for a heading measures headings, not testability. ## How to test this **Platforms:** any — nothing here executes. **Starting state:** a clone of this branch, and a browser signed in to GitHub. 1. Open <trunk...juanmaguitar/pr-description-template> and click **Create pull request**. → The description box is pre-filled with the template: Why, What changes, How to test this, Risks and limitations, Related, then four collapsed blocks. **Close the tab without opening the PR.** 2. Expand one of the `<details>` blocks in the preview of [`.github/pull_request_template.md`](.github/pull_request_template.md). → The guidance inside is an HTML comment, so it never appears in the rendered description a reviewer reads. 3. Read this PR's own description against the template. → It follows it — this is the first PR written to the new shape, and the "How to test this" you are reading now is the section the rule requires. 4. From a checkout of this branch, run `gh pr create` on a throwaway branch. → The body opens pre-filled with the same template, confirming it is not a web-UI-only feature. **What must not have happened:** - No second template file. GitHub shows **no picker** when a PR is opened — several templates in `.github/PULL_REQUEST_TEMPLATE/` are only reachable by appending `?template=name.md` to the URL, so the default loads anyway and the rest are never seen. `ls .github/` should show one template and no `PULL_REQUEST_TEMPLATE/` directory. - The review standard did not move or gain a copy. `.github/instructions/code-review.instructions.md` is untouched; if it had been restated in the template it would drift, which is the failure mode this repo has already designed against. - No behaviour change. `git diff --stat trunk...HEAD` touches three Markdown files and nothing under `src/`. ## Risks and limitations Nothing enforces any of this, so it holds only as long as people follow it — the honest ceiling of a convention in a repo that deliberately runs no credentialed bots. A template is a guess about what reviewers need. If a section turns out to be dead weight in practice, deleting it is a one-line PR and preferable to leaving a heading everybody skips. The "five minutes" is a target, not a measurement. A 1,500-line PR will not hit it whatever the description says — which is why the template ends by asking whether a diff over ~800 lines should have been two. ## Related Follow-up to #168, where these commits were originally written. They affect every PR rather than that one, so they are split out to review and merge independently — which is also the advice the template itself gives. --- <details> <summary>Design decisions and alternatives considered</summary> **One template, not one per kind of change.** The first draft was written for bugfixes — `## Problem` / `## Solution` — and a feature PR does not have a problem in that sense. The obvious fix is separate `bug.md` and `feature.md` templates under `.github/PULL_REQUEST_TEMPLATE/`. GitHub's documentation rules this out: there is no picker UI at PR-creation time, and selecting a non-default template means appending `?template=name.md` to the URL by hand. In practice the default would load every time and the second file would be dead. So the headings became `## Why` and `## What changes`, which fit a fix, a feature and a process change, and the template calls out the three places where those genuinely want different things: a fix names its root cause and the test that fails without it, a feature names what it deliberately leaves out and shows its surface, and the testing steps follow the path a contributor actually takes. **Guidance inside the sections, not a separate style guide.** The hints live in HTML comments in the template itself, at the moment they are needed. A style document nobody opens while writing a PR description does not change any PR description. **The rule lives in `AGENTS.md`, not only in the template.** An agent drafting a PR body may never open the template file; it does read `AGENTS.md`. The template is the shape, `AGENTS.md` is the requirement, `CONTRIBUTING.md` points at both without restating either. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> No `/self-review` pass. The standard's five dimensions — architecture, security, performance, cross-platform, tests — have no purchase on three Markdown files with no executable content; running it here would produce a paragraph saying so. What was checked instead: that GitHub actually auto-loads a root `pull_request_template.md` (it does, including via `gh pr create`), that multiple templates require an explicit `?template=` parameter (they do — the reason there is one file), and that nothing here duplicates the review standard. </details> <details> <summary>Implementation notes</summary> Three commits, in the order the thinking went: 1. **Require a "How to test this" section on every pull request** — the rule, in `AGENTS.md` and `CONTRIBUTING.md`, before any template existed. 2. **Add a pull request template built for a five-minute review** — the template, and the visible-versus-collapsed split. 3. **Make the template fit features, not just fixes** — the `Problem`/`Solution` → `Why`/`What changes` rename, after the second commit's own template proved bug-shaped when used on a feature PR (#168). The collapsed blocks are Design decisions and alternatives considered, Review outcome, Implementation notes, and Screenshots or recording. Review outcome is collapsed rather than dropped because `AGENTS.md` requires it while a reviewer rarely needs it in the first minute — the headline count surfaces in **Risks and limitations** when it changes how the PR should be read. The **Screenshots or recording** block is deleted from this description rather than left empty: nothing in the app changed, and the only visible surface is GitHub's own PR form, which step 1 above walks you to directly. </details> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-on-a-second-ticket-means-rebuilding-the # Conflicts: # src/main.js
… turns The Windows leg failed on "git:apply-patch never reported done" while macOS passed. Nothing about the handler is platform-specific: the helper polled a fixed 50 event-loop turns, and per-branch state (#108) made these handlers read the worktree to decide where that state lives. How many turns that takes is a property of the filesystem underneath, and the same failing path lookup is slower on Windows. Waiting on a budget of time instead still returns the instant the message lands, so nothing gets slower in the normal case — and it cannot start failing again because a handler grew one more await. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-on-a-second-ticket-means-rebuilding-the # Conflicts: # src/main.js
…roke first Windows failed again, in three tests that arrived with #189 and #184 and carry their own inline "poll 50 event-loop turns" loop — the same shape already fixed for git:apply-patch, in code that had not been written yet when that fix landed. The cause is unchanged: per-branch state (#108) makes these handlers read the worktree to decide where that state lives, so how many turns a result takes belongs to the filesystem rather than to the code, and the same failing path lookup is slower on Windows. There is now one helper and no inline loops, with a line saying that a new one is this bug waiting to happen again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ns-rebuilding-the
…ns-rebuilding-the
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A site is one working tree and one implicit patch. There is no branching, stashing or committing anywhere in the app.
So a contributor who finishes ticket #59234 and wants to start #61002 has two options, and both are bad: clone a second site — another ~54 MB fetch, another
npm install, another first build, which is exactly the setup cost this app exists to remove — or keep working in the same tree, where the second ticket's changes pile onto the first and the generated patch mixes both with no way to separate them afterwards.Finishing a first ticket and starting a second is the moment a first-time contributor becomes a repeat one. It is currently the worst-supported transition in the app.
What changes
The site is the expensive substrate (clone +
node_modules+ build output). A ticket becomes a cheap branch on top of it, so switching costs seconds instead of a rebuild.Two invariants hold it up:
trunkis never committed to. It stays the pristine snapshot every branch is diffed against. Work started on trunk is carried into a ticket branch —git.branch({checkout: true})only moves HEAD, so uncommitted edits come along and nothing is thrown away.parent: [baseOid]explicitly instead of committing onto the previous WIP commit, so re-parking rewrites that commit rather than stacking a pile of saves nobody asked for.The diff base becomes the branch point each branch records, which is what makes a patch mean "only my work on this ticket" once WIP commits exist.
Deliberately not in this PR: the "Working on:" switcher and splitting today's single Unlink into switch and delete this ticket's work — this is the main-process half, and nothing in
src/renderer/index.jsxcalls the new bridge yet, so a ticket is linked and resumed through the existing Trac ticket panel. Also left out: replaying an old branch onto an updated trunk, and the half of #85 that drops file deletions from patches. Each is named under Risks with its reason.How to test this
Any platform. Windows is the more interesting one — this touches paths, line endings and checkout behaviour. Buildkite builds signed artifacts for this branch; check the build matches the current head commit.
Starting state: a site that has finished its first install and build, on no ticket.
59234. → The panel shows#59234.wp-login.php— add a comment line you will recognise.wp-login.phpand that line, and nothing else.61002in the same panel. → Completes in seconds, with no install or build output.wp-login.php. → Your step-2 edit is gone — this is #61002's tree. Now edit a different file, saywp-comments-post.php.59234again. → Your step-2 edit is back exactly as you left it.wp-login.php.Then the case the invariants exist for: with
59234linked and an uncommitted edit in the tree, click Update to latest trunk. The log says it is parking your work, then that it is returning to your branch. Afterwards the panel still names the ticket and Submit patch still produces your change — not an empty patch.What must not have happened:
node_moduleswas never reinstalled and the site was never rebuilt. Steps 4 and 6 are checkouts; if either took minutes or streamed install output, that is the regression this whole change exists to prevent.Risks and limitations
Review outcome: 9
[fix here]· 2[follow-up]— all 9 fixed. Breakdown in the collapsed section below. The serious one was user-visible: "Update to latest trunk" parked the ticket and never came back, so the site sat on trunk while the panel still named the ticket, every patch came out empty, and the only route back to a morning's work was to unlink and re-link.Known limitations, all deliberate:
branches:deleteis unreachable in the shipped app until the switcher lands.invoke(), so the window is quiet during a large checkout. The duplicated worktree scan is gone, but streaming the progress needs the UI change.rebase. A branch born three weeks ago keeps producing a patch that is correct against its own base but no longer applies on current trunk. The route without rebase — generate patch → branch off new trunk →applyPatchToDir— is the flow already designed in Sites have no update path: patches age against a frozen trunk #94's comments, and is its own issue.mergeBranchMetainherits a read-modify-write race from the existing metadata pattern. Narrow and pre-existing, but the value at stake isbaseOid. Filed as Overlapping writes to a site's metadata can drop a ticket branch's patch base #172.What could not be tested by hand: the mid-switch recovery. It needs
git.checkoutto fail part-way — realistically a WindowsEPERMfrom an editor or antivirus holding a file — which I could not stage reliably. Its guard is covered by a wiring test instead.Related
Part of #108. Part of the contribution-flow tracker #110. Touches ground shared with #94 (trunk update) and #85 (patch generation), neither of which this closes.
The PR-description template this description follows started life on this branch and now lives in #170, which reviews and merges independently — it affects every PR rather than this one.
This branch has #169 merged into it. That PR added the mentor-handoff header to
git:save-patch, which reads the base the patch was diffed against — so the conflict was a real one, not a textual overlap. See the resolution note in the collapsed design section.Design decisions and alternatives considered
The diff base is the branch point, not
origin/trunk— against what #108 asks for#108 states the diff base "must become
origin/trunk". I did not do that, and the codebase already argued why:src/main.jsdocumented that diffing local edits against a trunk that has moved embeds reversed upstream changes and foreign context lines, producing a patch that applies nowhere.It cannot be the live
refs/heads/trunkeither, becauseupdateToLatestTrunkmoves that ref while existing ticket branches stay where they were born — reading it would reintroduce the same drift for every branch created before an update.So each branch records the trunk oid it forked from, and
statusMatrix({ref: baseOid})diffs against it. This also sidestepsfindMergeBase, which operates on a truncated graph in adepth: 1clone.Consequence: the
origin/trunkfetch that was performed and never used — the "dangling diff base" flagged in both #94 and #108 — is deleted rather than put to work, saving a network round trip on every "view patch".Merging #169: a handoff header must name the base the patch was diffed against
#169 landed on trunk while this branch was open, and gave
git:save-patcha provenance header carryingtrunkOidandtrunkDateread from the site record. Taking either side of the conflict verbatim would have been wrong: keeping this branch's version drops the header entirely, and keeping trunk's version diffs against the implicitHEADagain — a ticket branch's WIP commit — producing an empty patch under a header that looks authoritative.The resolution diffs against the branch's recorded
baseOidand puts that commit in the header. The date comes off the commit itself rather than the site record, because on a ticket branch the two describe different commits as soon as anyone updates trunk: the oid would say "born at the July snapshot" and the date would say "August 8th". A mentor applying the patch would go to the wrong tree. When there is no branch base — a site on trunk, which is every site today — the site record is used exactly as #169 wrote it, so nothing on that path changes.Covered by a new wiring test that builds a repository whose trunk has moved past the branch point; it fails on the naive resolution.
Parking amends rather than accumulates
Passing
parent: [baseOid]on every park keeps exactly one WIP commit per branch. Accumulating would leave a trail of saves the user never asked for, which every later operation — patch, delete, switch — would then have to stay correct in the presence of. That is the risk #108 itself flags at the end.WIP commits use a synthetic author
isomorphic-git requires
author.name/emailand there is no hostgit configto read — a contributor having no git installed is the premise of the project. These commits never leave the disk; the deliverable is still the patch, and its real author is the name on the Trac ticket.Rejected:
git.stashIt would avoid inventing commits, but it is a single stack per repository rather than per branch, and reading a stash back requires the same worktree state it was taken from. Branches are what the model actually wants.
Review outcome (9 [fix here] · 2 [follow-up] — all 9 fixed)
Run per AGENTS.md before opening, with the judgement pass in a fresh context so the session that wrote the code was not the one grading it.
stage: 'checkout'and the site is marked mid-switch until reconciled.appliedPatch/updateIncompletewere declared per-branch but no handler was moved, so ticket A's "patch applied · Revert" banner showed while on ticket B. One reader and one writer now.branches:deleteresetcurrentBranch/tracTicketunconditionally, silently unlinking the ticket you were working on when deleting a different one.branchesmap on failure — permanently, via its own "already migrated" guard.withRegisteredSiteswallowed failures withoutlogError.git.addloop was safe was only asserted by re-implementing the filter inline, never through the real handler with an untracked file present.git:update-trunktest's own comment said it exercised the park path; it did not — the block could be deleted and the suite stayed green.[follow-up], both in Risks above: the renderer not calling the new bridge, and themergeBranchMetarace.One thing worth flagging rather than burying: the test written for finding 3 passed against the broken code on the first attempt, because it seeded no site-level value that could leak. It was hardened until it failed for the right reason. That is the "green while proving nothing" pattern the review standard names, produced while fixing a finding about it.
Second pass, after merging trunk (#169): 0 [fix here] · 1 [follow-up]. A fresh-context review of the merge resolution confirmed
baseProvenanceon both paths, the completeness of the merge from both parents, and that nothing renderer-controlled reaches the handoff filename or header. The one finding: the handoff header's base now follows the worktree while its ticket and filename still follow the registry, so an out-of-band checkout can produce a correctly-diffed file attributed to the wrong ticket. Filed as #175 rather than fixed here — three other surfaces read the registry the same way, and one authority applied to all four is a separate change.Implementation notes
New:
src/ticket-branches.js— park / start / switch / delete / list, with no Electron dependency sonode --testdrives it against real repositories, reusingensureAutocrlffromtrunk-update.js.Changed behaviour this dragged in:
readTrunkInforesolvedHEADto date the trunk snapshot. On a ticket branch that is a WIP commit made minutes ago, so the staleness dot (Sites have no update path: patches age against a frozen trunk #94) would never light up. It readsrefs/heads/trunk, falling back toHEADfor an adopted repository with no trunk branch.discardChangeschecked outtrunkby name, which under this model would silently move the contributor to another ticket. It checks out whatever is current; parked work survives, since destroying a ticket is what deleting its branch is for.Patch generation no longer touches the index. The
git.addloop that staged every untracked file and never unstaged it (#85) is redundant —statusMatrixreports untracked files as[path, 0, 2, 0]unaided, and thehead !== workdirfilter keeps them. Verified empirically before removal, and now through the handler with an assertion that the index is byte-identical afterwards.staleStagedPathsstays: parking stages the worktree, and indexes dirtied by earlier app versions are still out there.Migration.
siteMeta[sitePath]gainsbranchesandcurrentBranch;tracTicket,appliedPatchandupdateIncompletemove under the branch. A site with a linked ticket gets that branch created at the current trunk tip with its work carried onto it; a site without one gains an empty map. It runs only from handlers already about to move HEAD — a list call can land while an install or the Playground server is running against that directory.Tests: 405 pass on
.nvmrc's Node and on Electron's bundled Node;npm run lintclean. The behaviour changes toreadTrunkInfo,discardChanges, per-branch applied state and returning to the branch after an update were each confirmed to fail with the previous code by reverting the source and re-running.Screenshots or recording
Nothing on screen changed. This PR is the main-process layer; the visible surface — the "Working on:" switcher, and splitting today's single Unlink into switch and delete this ticket's work — is the follow-up.