Apply a patch from a file to the checkout, then rebuild - #135
Merged
juanmaguitar merged 5 commits intoAug 7, 2026
Conversation
This was referenced Aug 6, 2026
juanmaguitar
force-pushed
the
juanmaguitar/issue-11-apply-patch
branch
from
August 7, 2026 05:19
a33f3a4 to
37d5ebb
Compare
juanmaguitar
force-pushed
the
juanmaguitar/issue-11-apply-patch
branch
from
August 7, 2026 06:27
37d5ebb to
4324c0f
Compare
Associating a site with a Trac ticket (#109) only pays off if a contributor can act on what is on that ticket. This is the first half of that: take a .diff or .patch file, apply it to the wordpress-develop checkout, and rebuild so the change can be tested. Discovering patches from the ticket itself builds on top of this and lands separately. There is no git binary to shell out to and isomorphic-git has no apply primitive, so hunks are matched and written by hand with the diff package the app already bundles for generating patches. Three properties shape it: - All or nothing. Every file is resolved in memory first; if any hunk fails to match, nothing is written. And because a write can still fail on the way out (a path that is really a file, a read-only attribute, Windows holding a file open, a full disk), the prior contents are captured during resolution and restored if a write throws midway. A half-applied tree is worse than an unapplied one: the contributor would build and test something that matches neither trunk nor the patch. - Patches come in three dialects that disagree on format — Subversion-style Trac attachments with no a/ b/ prefixes and paths against the pre-src/ layout, git-style PR .diff files, and the app's own createTwoFilesPatch output. Parsing normalises all of them to repo-relative paths for today's layout, in a pure module (src/patch-plan.cjs) that both the main process and the applier share, kept out of the renderer bundle. - The association is local: applying never touches the network. Line endings are matched on LF but written back as the file had them, so a genuinely-CRLF fixture is not silently rewritten. Paths are resolved through symlinks and refused if they escape the site folder, since a patch is untrusted input. Applied patches are recorded in siteMeta so Revert can undo them after a restart, and the record is cleared when a trunk update or a discard resets the tree out from under it. The apply -> install? -> rebuild chain reuses the trunk update's step helper and npm wrappers. Refs #11, part of #110. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar
force-pushed
the
juanmaguitar/issue-11-apply-patch
branch
from
August 7, 2026 06:47
4324c0f to
118d0bc
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Review outcome: 13 [fix here] findings.
Adds the patch preview, application, rebuild, persistence, and revert workflow for WordPress checkouts.
Changes:
- Parses and normalizes supported patch formats.
- Applies or reverses patches with conflict previews and rebuild steps.
- Adds IPC/UI integration and automated tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
src/main.js |
Adds patch IPC handlers and persistence. |
src/patch-apply.js |
Implements patch application and rollback. |
src/patch-plan.cjs |
Parses patches and creates apply plans. |
src/preload.js |
Exposes patch IPC APIs. |
src/renderer/index.jsx |
Adds preview, apply, and revert UI. |
src/renderer/update-plan.cjs |
Defines patch workflow steps. |
test/ipc-wiring.test.cjs |
Extends IPC coverage classification. |
test/patch-apply.integration.test.cjs |
Tests on-disk patch application. |
test/patch-plan.test.cjs |
Tests parsing and planning behavior. |
Comment on lines
+51
to
+58
| while (!fs.existsSync(existing)) { | ||
| const parent = path.dirname(existing); | ||
| if (parent === existing) break; | ||
| trailing.unshift(path.basename(existing)); | ||
| existing = parent; | ||
| } | ||
| let abs; | ||
| try { abs = path.join(fs.realpathSync(existing), ...trailing); } catch { abs = lexical; } |
Comment on lines
+122
to
+125
| if (file.kind === 'delete') { | ||
| if (!fs.existsSync(target)) return { error: `${file.path} is already gone, so the patch cannot remove it` }; | ||
| return { op: 'delete', abs: target, path: file.path, previous: readIfPresent(target) }; | ||
| } |
Comment on lines
+248
to
+260
| for (const action of actions) { | ||
| if (action.op === 'delete') { | ||
| fs.rmSync(action.abs, { force: true }); | ||
| } else if (action.op === 'rename') { | ||
| fs.mkdirSync(path.dirname(action.abs), { recursive: true }); | ||
| fs.writeFileSync(action.abs, action.content, 'utf8'); | ||
| fs.rmSync(action.from, { force: true }); | ||
| } else { | ||
| fs.mkdirSync(path.dirname(action.abs), { recursive: true }); | ||
| fs.writeFileSync(action.abs, action.content, 'utf8'); | ||
| } | ||
| done.push(action); | ||
| } |
Comment on lines
+189
to
+192
| } catch { | ||
| // Best effort: the caller is already reporting a failure, and a | ||
| // rollback that cannot run must not mask the original cause. | ||
| } |
Comment on lines
+139
to
+148
| const original = fs.readFileSync(source, 'utf8'); | ||
| // A 100%-similarity rename has no hunks: the content moves unchanged. | ||
| let content = original; | ||
| if (file.hunks.length) { | ||
| const applied = JsDiff.applyPatch(normalizeEol(original), file.patch); | ||
| if (applied === false) { | ||
| return { error: `${file.oldPath} has moved on since the patch was written, so it no longer applies` }; | ||
| } | ||
| content = applied.replace(/\n/g, dominantEol(original) === '\r\n' ? '\r\n' : '\n'); | ||
| } |
Comment on lines
+491
to
+493
| let dirtyPaths = []; | ||
| try { dirtyPaths = await collectDirtyFiles(sitePath); } catch {} | ||
| const plan = planApply({ files: parsed.files, dirtyPaths }); |
Comment on lines
+1634
to
+1636
| // Same contract as the other chains: while `running` is set, Ctrl+C in the | ||
| // terminal has to reach the child process the chain is about to spawn. | ||
| terminalKillRef.current = () => { killCurrent().catch(() => {}); }; |
| } | ||
| sendLog(`\n${reverse ? 'Reverting' : 'Applying'} ${label}…\n`); | ||
|
|
||
| const result = await applyPatchToDir({ dir: sitePath, patchText, reverse, onLog: sendLog }); |
Comment on lines
+234
to
+236
| // Same rule the trunk update uses (#94): the lockfile moving is what | ||
| // makes an install necessary rather than merely possible. | ||
| needsInstall: paths.includes('package-lock.json') |
| const NOT_REACHABLE = new Map([ | ||
| ['wordpress:setup', 'calls ensureAutocrlf and readTrunkInfo only after cloning wordpress-develop over the network'] | ||
| ['wordpress:setup', 'calls ensureAutocrlf and readTrunkInfo only after cloning wordpress-develop over the network'], | ||
| ['git:apply-patch', 'reads electron-store for the applied-patch guard before it delegates to patch-apply'] |
Closes the transactional gaps a reviewer found in the applier: - Register each action before its mutations, so a rename that writes its destination and then fails to remove its source is still undone (#3). - rollback reports the paths it could not restore; a failed rollback no longer reports a clean restore, and removals of never-created files are tolerated rather than counted as losses (#4). - Carry a pure (100%-similarity) rename as a Buffer; git emits binary renames with no marker, and the old utf8 round-trip corrupted them (#5). - Validate a deletion's pre-image, so a file edited after preview fails all-or-nothing instead of being silently deleted (#2). - resolveInside uses lstat, so a dangling symlink pointing outside the checkout can no longer be written through (#1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- git:apply-patch rejects a sitePath that is not a registered site before writing anything, the same gate sites:set-ticket uses (#8). - Persisting the revert record is now part of the apply transaction: if the store write fails the apply is undone, and if that also fails the result says the patch is applied-but-untracked rather than reporting a clean failure (#7). - git:preview-patch surfaces a worktree-inspection failure instead of reporting "no collisions" when it could not look (#9). - git:discard-changes clears the applied-patch record with the reset, so a later trunk-update network failure cannot leave a revert banner for a patch that is already gone (#6). - git:apply-patch is now a WIRED handler with real tests (guard, reverse lookup, delegation, done event, metadata) via fakeSettingsStore, instead of a NOT_REACHABLE hole (#13). Not gating git:preview-patch: it is read-only, and the registered-site check would add a store dependency to a handler that has none. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…iew) planApply keyed needsInstall off `paths`, which holds only a rename's destination, so moving package-lock.json away read as "no install needed". Use the `touched` set, which already includes both sides of a rename. (#12) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The #7 transactional branch — undo the apply when the revert record cannot be saved, and report applied-but-untracked when that undo also fails — had no coverage. Drive store.set to throw and assert both outcomes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 7, 2026
Part of #110. Refs #109, #11. **Stacked on #135** (base is that branch, not `trunk`), which is itself stacked on #123. Review order: #123 → #135 → this. The full ticket-to-tested-patch flow merges once the stack is complete. ## Why #123 links a site to a Trac ticket; #135 applies a patch file. This joins them: it shows the work already on the ticket so a contributor can see and test it before adding their own — the Contributor-Day failure the whole flow exists to prevent (several people writing overlapping patches because none could see anybody else's). ## What this does When a site has a ticket linked, the panel lists the ticket's **linked pull requests**, newest first, each with its state and last-updated date. "Apply…" fetches that PR's diff and drops it into the existing preview → apply → rebuild flow from #135 — so a downloaded `.diff` and a linked PR are one path from the preview onward. A Refresh button re-checks on demand. Ships the PR half of "patches on a ticket" first, deliberately: on the busiest tickets the real work is a wordpress-develop PR, and the Trac attachment list is emptiest exactly where activity is highest. Trac attachments (behind the proof-of-work interstitial) are not listed yet — the panel points at the ticket for those. ## How, and the constraints that shaped it - **PRs are found by GitHub's convention and verified locally.** A PR cites its ticket in the body, so a broad search for the number is verified narrowly against `core.trac.wordpress.org/ticket/<id>` — GitHub's tokeniser matches the bare number in unrelated text, so the local verification is what makes the list trustworthy rather than merely plausible. - **The web `.diff` routes return 422 unauthenticated** (verified 2026-08-06, both `github.com/.../pull/N.diff` and `patch-diff.githubusercontent.com`), so the diff is fetched through the REST API with the diff media type. Requests go through **Electron `net`**, not a new HTTP dependency. - **Unauthenticated GitHub is 60/hour and a Contributor-Day room shares one NAT IP.** So lookups are manual, not polled; each ticket's result is cached in `electron-store` as last-known-good with a timestamp; and a rate-limited or offline answer shows the cached list *labelled with when*, never a short list presented as complete. `classifyHttpFailure` tells a spent limit — primary and secondary — apart from an empty ticket. ## Testing - `test/patch-sources.test.cjs` — the parse, verify (including the `65820` vs `658200` precision case) and rate-limit-classification logic, unit tested without a network. - `npm test` / `npm run test:electron`: **220/220** on both runtimes. `npm run lint`: clean. ## Self-review (per AGENTS.md) Ran the review with the judgement pass on fresh context. It returned **1 `[fix here]` · 1 `[follow-up]`, both fixed:** - 🔴 **Temporal-dead-zone crash** — a `useEffect` referenced the `loadTicketPatches` callback in its dependency array ~660 lines before the `const` was defined, throwing `ReferenceError` on every `SiteRow` render. The `node --test` suite has no DOM, so 220 green tests did not catch it; the review did. Fixed by placing the effect after the definition. **This is exactly why the manual pass below still matters.** - 🔵 GitHub's *secondary* (abuse) rate limit is a 403 with `Retry-After` while the primary quota is unspent — now classified as rate-limited rather than a generic error, with a test. Verified clean by the review: `httpGet` settle-once/timeout/abort, the cache cannot be poisoned by a failed lookup (only `ok` writes), no new trust-boundary hole (PR number stripped to digits; diff flows through #135's existing engine unchanged), additive `electron-store` keys so no migration. ## Not done The Electron `net` client and the two IPC handlers are not unit-tested (no repo precedent for mocking `net`; the logic lives in the pure module, which is covered). And **nothing in this stack has been exercised in the running app yet** — the TDZ crash above shows why that pass is owed before the final merge, and it is the main thing left. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 7, 2026
Part of #110. Refs #109, #11. **Stacked on #136** (base is that branch), which stacks on #135 → #123. Review order: #123 → #135 → #136 → this. The whole flow merges once the stack is complete. ## Why PRs are only half of "the work already on a ticket". On many tickets — good-first-bugs especially — the patch a contributor wants to try is a `.diff` **attached to the ticket**, not a PR (verified live: ticket #37578 has three `.diff` attachments and one PR; before this, the app showed only the PR). This adds the attachment half, under the PR list, loaded on demand. ## What this does A **"Show Trac attachments"** button under the linked-PR list. Because Trac serves the attachment list only to a real browser (everything else hits the proof-of-work interstitial), clicking it opens the real ticket in an embedded window where the contributor clears the challenge **once**, the app scrapes the `#attachments` block, and the window closes — it is a means, not the UI. The attachments appear as a native in-app list (filename · author · date · size), each `.diff`/`.patch` with **Apply…**; non-patches (e.g. a `.txt`) are shown but marked "not a patch". Applying downloads the file through that same challenge-passing session and hands it to the existing preview → apply engine, so an attachment, a PR and a chosen file are one path from the preview onward. On demand, not on link: opening a Trac window can surface the challenge, so it happens when the contributor asks, not for every ticket. The persistent session means the challenge is passed once, not per open. ## Verified end to end against live Trac (#37578) A deterministic Electron harness exercised the real `trac-view.js`: - the window passes the challenge unattended; - the parser reads the **real** markup — 4 attachments with authors and **absolute timestamps**, the `.txt` correctly marked not-a-patch; - **the raw-attachment download is authorised by the session cookie** — a real `wp-admin/includes/dashboard.php` diff (1701 bytes) comes back. This was the one load-bearing runtime assumption, now confirmed. Also confirmed in the running app: the panel renders, the attachment list populates from the live scrape, and the PR apply→preview wiring works. ## Security (the app's first remote, untrusted content — AGENTS.md) - The window: `contextIsolation`, no `nodeIntegration`, `sandbox`, a dedicated `persist:trac` partition, and **no preload** — the page cannot reach the app or Node; only the `#attachments` HTML crosses back, read from the main process via `executeJavaScript`. - Navigation pinned to `core.trac.wordpress.org` against **both `will-navigate` and `will-redirect`** (the latter catches 3xx / `<meta refresh>`). - The parser **never emits an off-host URL**, and `fetchAttachment` re-checks the host before sending the session cookie — so a poisoned ticket page cannot get an attacker link in front of the user or leak the cookie off-host. - The downloaded diff is untrusted → flows through #135's apply engine, which defends against path traversal. ## Testing - `test/trac-attachments.test.cjs` — the pure parser: dedup, encoded names, off-host and cross-ticket rejection, absolute-date extraction, missing-metadata rows, empty input. The fixture matches the live markup (confirmed by the harness above). - `npm test` / `npm run test:electron`: **229/229** both runtimes. `npm run lint`: clean. - `src/trac-view.js`'s window/net glue is untested, consistent with the other `net` client (`github-prs.js`) — no repo precedent for mocking BrowserWindow; the logic lives in the covered pure parser. ## Self-review (per AGENTS.md) Ran the review with the judgement pass on fresh context. It returned **2 `[fix here]` (both 🔵), both fixed before this PR:** - Navigation lock covered only `will-navigate` → added `will-redirect` (3xx / meta-refresh could otherwise move the pinned window off-origin). - The link regex accepted an absolute off-host href (Apply was safe, but the filename rendered as an `openExternal` link) → the parser now rejects any non-Trac-host URL, with a test. Reviewer verified clean: window config, `setWindowOpenHandler`, host re-check before the cookie fetch, poll-loop lifecycle with `isDestroyed` guards + `destroy()` in `finally`, additive `electron-store` (no migration), no new dependency. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 7, 2026
Part of #110. Refs #109, #11. **Stacked on #139** (base is that branch), which stacks on #136 → #135 → #123. Review order: #123 → #135 → #136 → #139 → this. ## Why A contributor arriving at a ticket wants to try the latest fix. The panel listed PRs and Trac attachments as two separate sections, but nothing said which one — across both — was the most recent. Usually it's a PR (visible immediately); but sometimes the newest fix is a `.diff` uploaded to Trac, and that was buried under the attachments button with no signal. ## What this does - The newest known patch carries a **"Latest" pill**, whether it's a PR row or a Trac-attachment row. - When the newest is an attachment, a note says so explicitly: *"The most recent patch on this ticket is a file attachment, not a pull request."* - Because attachments load on demand, "latest" is judged across what's loaded: **PRs alone until the contributor opens attachments** (the normal case), then both. Opening attachments isn't forced on every ticket — the verdict simply completes once they're looked at. ## How The comparison is a pure module (`src/latest-patch.cjs`), unit tested. A PR is dated by `updatedAt`, an attachment by its scraped upload time. A `.txt` never competes; a relative-only date ("15 months ago") can't win. The attachment timestamp is **anchored to UTC by hand** rather than parsed in the machine's local zone, so two contributors in different timezones see the same patch marked latest — a consistent answer is the whole point. ## Testing - `test/latest-patch.test.cjs` — PR-wins (normal), attachment-wins-once-loaded (the case the feature exists for), `.txt` excluded, relative-date can't win, attachments-only, empty→null, and the UTC-determinism of the date parse (PM/AM and the 12-o'clock edge). - `npm test` / `npm run test:electron`: **238/238** both runtimes. `npm run lint`: clean. ## Self-review (per AGENTS.md) Ran the review with the judgement pass on fresh context. It returned **0 `[fix here]` · 1 `[follow-up]`** — the follow-up was the timezone skew (PR date is UTC ISO, attachment date was parsed in local time), which two contributors could see differently. **Fixed before this PR** by anchoring the attachment parse to UTC, with a determinism test. The reviewer verified the key/equality logic (exactly one row marked, right row), NaN/`.txt` exclusion, null-safety, and stale-state reset on ticket switch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 7, 2026
Part of #110. Refs #109, #11. **Stacked on #140** (base is that branch), atop #139 → #136 → #135 → #123. From hands-on testing of the stack. ## Three fixes reported from testing 1. **The apply error persisted after Cancel.** Applying a PR that no longer fits ("…has moved on since the patch was written") showed an error; clicking Cancel dismissed the preview but left the error on screen. Cancel now clears it too. 2. **The "Try someone else's patch" sub-copy contradicted itself during a PR preview** — it said "Apply a `.diff`/`.patch` file…" while showing "PR #4496 changes 1 file". The sub-copy is now hidden whenever a preview or the apply chain is showing (the preview card speaks for itself), and the idle copy names pull requests as well as files. 3. **The failure message ran two sentences together** ("…no longer applies The checkout was not changed."). A period is inserted when the reason doesn't already end in punctuation. ## One requested addition **Apply a PR straight from a pasted URL or number**, without it having to be linked to the ticket. A "Paste a pull request URL or number" input + "Apply PR" button in the panel's idle state. `parsePrRef` (pure, `src/patch-sources.cjs`) accepts a wordpress-develop PR URL or a bare number and rejects other repos, other hosts, issue URLs and junk — including crafted `..` paths (URL normalisation collapses them, then the repo/anchor check rejects). The number rides the **existing** `previewPr → fetchPrDiff → preview → apply` flow, so a pasted PR shares the same trust boundary as the linked-PR list: `fetchPrDiff` hardcodes the repo and strips the number to digits, and the diff flows through the apply engine's path-traversal defence. ## Testing - `test/patch-sources.test.cjs` — `parsePrRef`: bare/`#`number, URL with trailing `/files` and `#…`, scheme-less, wrong-repo rejection, and issue/foreign-host/junk rejects. - `npm test` / `npm run test:electron`: **242/242** both runtimes. `npm run lint`: clean. ## Self-review (per AGENTS.md) Judgement pass on fresh context: **0 findings** across the five dimensions. It independently confirmed the `..`-traversal and foreign-repo/host rejections, that the pasted-number path introduces no new trust boundary (same `fetchPrDiff` + apply engine), that the punctuation ternary can't throw (error is truthy-guarded), and that hiding the sub-copy never strips the idle call-to-action. Pending your visual confirm in the running app for the three UI fixes and the paste-a-PR flow — these are your reported issues, verified here by tests + review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <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.
Part of #110. Closes #11 (the stack applies a patch from a file, a linked PR, and a Trac attachment — the whole of #11). Stacked on #123 (base is that branch, not
trunk) — review #123 first; this diff is only the apply engine on top of it. The whole ticket-to-tested-patch flow lands as one merge once the stack is complete.Why
Associating a site with a Trac ticket (#123) only pays off if a contributor can act on what is on that ticket. This is the first half of acting: take a
.diff/.patchfile, apply it to thewordpress-developcheckout, and rebuild so the change can be tested. Discovering the patches from the ticket itself is the next branch in the stack.What this does
package-lock.jsonmoves (so an install runs before the rebuild).How, and why it is built the way it is
There is no
gitbinary to shell out to and isomorphic-git has no apply primitive, so hunks are matched and written by hand with thediffpackage the app already bundles for generating patches.a/b/prefixes, sometimes the pre-src/layout), git-style PR.diffs, and the app's owncreateTwoFilesPatchoutput all normalise to repo-relative paths for today's layout, in a pure module (src/patch-plan.cjs) shared by the main process and the applier and kept out of the renderer bundle.Testing
test/patch-plan.test.cjsandtest/patch-apply.integration.test.cjs— the applier tests run against real on-disk isomorphic-git repos. The parser is also validated against the real diff of wordpress-develop PR #7990.npm test/npm run test:electron: 213/213 on both runtimes.npm run lint: clean.Self-review (per AGENTS.md)
Ran the review in
.github/instructions/code-review.instructions.mdwith the judgement pass dispatched to a fresh-context subagent. It returned 5[fix here]· 6[follow-up]; all 11 were fixed before this PR:appliedPatchdesynced from the tree — cleared on discard and on a trunk update's force checkout; a second apply is refused by name rather than silently orphaning the first.+86 KB → +11 KBbundle reduction by moving the parser out of the renderer tree, a floating-promise reset, and an install-step label that showed "skipped" while installing.Not done
The
ipcMainhandler layer has no unit tests (the repo has no precedent for mocking handlers; the logic beneath them is covered), and nothing has been exercised in the running app yet — same manual-pass gap as #123. Both are called out rather than implied.🤖 Generated with Claude Code