Give every pull request a testable description a reviewer can read in five minutes - #170
Merged
Merged
Conversation
A green suite is not a tested feature here. This app's failures live where `node --test` cannot go: a real `wordpress-develop` clone, a `node_modules` that takes minutes, an OS file dialog, a Windows path with a space in it. The suite proves the logic; a person driving the app proves the feature. Reviewers were being handed diffs with no way in — working out which button, in which state, to reach the change is the author's job, and doing it for them costs one paragraph instead of a round trip. AGENTS.md carries the shape (starting state, numbered steps in the words on screen, falsifiable expected results, what must *not* have happened, which platforms, and what could not be tested by hand and why). CONTRIBUTING.md gains it as step 4 of the pre-PR checklist and links there rather than restating it — the same split the rest of these two files already use. The "what must NOT have happened" line is the one worth keeping: most of this project's regressions are silent. Work quietly discarded, a `node_modules` quietly rebuilt, a patch quietly missing a file. None of those announce themselves to someone who does not know to look. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Descriptions here have been long and undifferentiated — everything at the same level, so a reviewer had to read all of it to find the part that mattered. Length was never the problem. Ordering was. `.github/pull_request_template.md` is loaded by GitHub into every new pull request, so the shape arrives without anyone having to remember it. Above the fold: Problem, Solution, How to test this, Risks, Related. Everything else — design decisions and what was rejected, the review outcome, implementation notes, screenshots — goes in a `<details>` block collapsed by default. The rule that makes it work is that depth moves rather than disappears. Detail is not what makes a PR unreadable; detail in the way is. A reviewer who wants the reasoning behind a decision can open one block, and one who wants to approve a small change never has to. Two things kept visible on purpose: - **What must not have happened**, inside the testing section. Most regressions in this project are silent, and a reviewer who does not know to look for a quietly rebuilt `node_modules` will not find one. - **The review-outcome count**, surfaced in Risks when it changes how the PR should be read, with the breakdown collapsed underneath. Burying "one finding was user-visible data loss" three blocks down defeats the purpose of running the review at all. AGENTS.md and CONTRIBUTING.md point at the template rather than restating it, the same split those two files already use everywhere else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first section led with error messages and reproduction steps, and handled features in a clause at the end. That is backwards for a repo whose open issues are mostly things the app cannot do yet — and the PR that introduced the template is itself a feature that had to be bent to fit its own "Problem" heading. `Problem` becomes `Why` and `Solution` becomes `What changes`, each carrying explicit guidance for the three kinds of change that actually land here: fixing something, building something, changing how we work. Three places where they genuinely differ are now called out instead of left to the author: - A fix names its root cause, and the test that fails without it — this repo's standard is that a bugfix's test must fail on the old code, so the template asks which test and whether that was checked. - A feature names what it deliberately leaves out. A reviewer who cannot tell a missing piece from a rejected one asks about every one of them. - Testing steps follow the path a contributor takes, including what happens when they get it wrong — not the shortest path to the new code. ## One template, not one per kind GitHub shows no picker when a pull request is opened. Several templates in `.github/PULL_REQUEST_TEMPLATE/` are selectable only by appending `?template=name.md` to the URL, so in practice the default loads and the others are never seen. A split would have produced two files, one of which nobody reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
… (#168) ## 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: 1. **`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. 2. **A ticket branch holds exactly one WIP commit.** Parking passes `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.jsx` calls 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. 1. In the site's **Trac ticket** panel, link `59234`. → The panel shows `#59234`. 2. Edit `wp-login.php` — add a comment line you will recognise. 3. Click **Submit patch**. → The patch contains `wp-login.php` and that line, and nothing else. 4. Link `61002` in the same panel. → Completes in **seconds**, with no install or build output. 5. Open `wp-login.php`. → Your step-2 edit is **gone** — this is #61002's tree. Now edit a *different* file, say `wp-comments-post.php`. 6. Link `59234` again. → Your step-2 edit is back exactly as you left it. 7. Click **Submit patch**. → It contains **only** `wp-login.php`. Then the case the invariants exist for: with `59234` linked 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_modules` was 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. - **No work was silently discarded.** Both edits survive the round trip. Deletions too — delete a file in step 2 instead of editing one, and it must still be deleted after step 6. - **The patch never contains the other ticket's work**, and never contains reversed upstream changes — lines you did not write, appearing as removals. ## 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: - **The renderer does not call the new bridge yet**, so `branches:delete` is unreachable in the shipped app until the switcher lands. - **A ticket switch has no progress channel.** It resolves an `invoke()`, so the window is quiet during a large checkout. The duplicated worktree scan is gone, but streaming the progress needs the UI change. - **Replaying an old branch onto an updated trunk is not here.** isomorphic-git has no `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 #94's comments, and is its own issue. - **File deletions are still dropped from generated patches** (#85's other half). Unchanged here: rewriting the patch generator in the same change that introduces branches would make a failure impossible to attribute. - **`mergeBranchMeta` inherits a read-modify-write race** from the existing metadata pattern. Narrow and pre-existing, but the value at stake is `baseOid`. Filed as #172. - **A contributor's own git client can move HEAD behind the registry's back**, and while the patch base now follows the worktree, the ticket a handoff file is named after — and three other surfaces — still answer from the registry. Found by the post-merge self-review pass; filed as #175 because the fix is one authority applied to four surfaces, not a patch to this handler. **What could not be tested by hand:** the mid-switch recovery. It needs `git.checkout` to fail part-way — realistically a Windows `EPERM` from 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. --- <details> <summary>Design decisions and alternatives considered</summary> ### 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.js` documented 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/trunk` either, because `updateToLatestTrunk` moves 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 sidesteps `findMergeBase`, which operates on a truncated graph in a `depth: 1` clone. Consequence: the `origin/trunk` fetch 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-patch` a provenance header carrying `trunkOid` and `trunkDate` read 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 implicit `HEAD` again — a ticket branch's WIP commit — producing an empty patch under a header that looks authoritative. The resolution diffs against the branch's recorded `baseOid` and 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`/`email` and there is no host `git config` to 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.stash` It 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. </details> <details> <summary>Review outcome (9 [fix here] · 2 [follow-up] — all 9 fixed)</summary> 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. | # | Dimension | What was wrong | |---|---|---| | 1 🔴 | Architecture | The update parked the ticket and never returned, stranding the site on trunk while the panel still named the ticket. Now it returns; on failure it clears the stored ticket and says in the log where the work is parked. | | 2 🟡 | Architecture | A checkout that failed part-way left HEAD on the source branch over a half-swapped tree; parking again would commit the mixture over the good WIP commit. Errors are now tagged `stage: 'checkout'` and the site is marked mid-switch until reconciled. | | 3 🟡 | Architecture | `appliedPatch` / `updateIncomplete` were 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. | | 4 🟡 | Architecture | `branches:delete` reset `currentBranch`/`tracTicket` unconditionally, silently unlinking the ticket you were working on when deleting a different one. | | 5 🟡 | Architecture | The migration performed git writes from read-shaped entry points, and persisted an empty `branches` map on failure — permanently, via its own "already migrated" guard. | | 6 🔵 | Architecture | `withRegisteredSite` swallowed failures without `logError`. | | 7 🟡 | Performance | Every switch scanned the whole worktree twice. One scan now, shared. (The missing progress channel remains — see Risks.) | | 8 🟡 | Tests | The claim that removing the `git.add` loop was safe was only asserted by re-implementing the filter inline, never through the real handler with an untracked file present. | | 9 🟡 | Tests | The `git:update-trunk` test'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 the `mergeBranchMeta` race. 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 `baseProvenance` on 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. </details> <details> <summary>Implementation notes</summary> **New:** `src/ticket-branches.js` — park / start / switch / delete / list, with no Electron dependency so `node --test` drives it against real repositories, reusing `ensureAutocrlf` from `trunk-update.js`. **Changed behaviour this dragged in:** - `readTrunkInfo` resolved `HEAD` to date the trunk 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`, falling back to `HEAD` for an adopted repository with no trunk branch. - `discardChanges` checked out `trunk` by 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.add` loop that staged every untracked file and never unstaged it (#85) is redundant — `statusMatrix` reports untracked files as `[path, 0, 2, 0]` unaided, and the `head !== workdir` filter keeps them. Verified empirically before removal, and now through the handler with an assertion that the index is byte-identical afterwards. `staleStagedPaths` stays: parking stages the worktree, and indexes dirtied by earlier app versions are still out there. **Migration.** `siteMeta[sitePath]` gains `branches` and `currentBranch`; `tracTicket`, `appliedPatch` and `updateIncomplete` move 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 lint` clean. 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 by reverting the source and re-running. </details> <details> <summary>Screenshots or recording</summary> 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. </details> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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 --testcannotreach: a real clone of
wordpress-develop, anode_modulesthat takes minutes to install, an OSfile 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 withgh pr create(as long as no--bodyreplaces it). The organising rule is a reviewer understandsthe 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 areadable PR; depth in the way is — so detail moves rather than disappears.
AGENTS.mdgains two subsections under "Before opening a pull request": the shape of thedescription, 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.mdgets the human-facing pointer anda 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.
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.
<details>blocks in the preview of.github/pull_request_template.md. → The guidance inside isan HTML comment, so it never appears in the rendered description a reviewer reads.
written to the new shape, and the "How to test this" you are reading now is the section the rule
requires.
gh pr createon a throwaway branch. → The body openspre-filled with the same template, confirming it is not a web-UI-only feature.
What must not have happened:
.github/PULL_REQUEST_TEMPLATE/are only reachable by appending?template=name.mdto the URL, sothe default loads anyway and the rest are never seen.
ls .github/should show one template and noPULL_REQUEST_TEMPLATE/directory..github/instructions/code-review.instructions.mdis untouched; if it had been restated in thetemplate it would drift, which is the failure mode this repo has already designed against.
git diff --stat trunk...HEADtouches three Markdown files and nothing undersrc/.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.
Design decisions and alternatives considered
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 separatebug.mdandfeature.mdtemplates under.github/PULL_REQUEST_TEMPLATE/. GitHub's documentationrules this out: there is no picker UI at PR-creation time, and selecting a non-default template means
appending
?template=name.mdto the URL by hand. In practice the default would load every time andthe second file would be dead. So the headings became
## Whyand## 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 neveropen the template file; it does read
AGENTS.md. The template is the shape,AGENTS.mdis therequirement,
CONTRIBUTING.mdpoints at both without restating either.Review outcome (required — see AGENTS.md)
No
/self-reviewpass. 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(itdoes, 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.
Implementation notes
Three commits, in the order the thinking went:
AGENTS.mdandCONTRIBUTING.md, before any template existed.visible-versus-collapsed split.
Problem/Solution→Why/What changesrename, after the second commit's own template proved bug-shaped when used on a featurePR (Work a second ticket without rebuilding the world: the git layer (#108) #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.mdrequires it while a reviewer rarely needs it in the first minute — the headline countsurfaces 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.