From 118d0bc7961e929377ae70f0a98c85d9234ab5e3 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Thu, 6 Aug 2026 12:38:27 +0200 Subject: [PATCH 1/5] Apply a patch from a file to the checkout, then rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/main.js | 135 ++++++++++- src/patch-apply.js | 281 +++++++++++++++++++++++ src/patch-plan.cjs | 247 ++++++++++++++++++++ src/preload.js | 21 ++ src/renderer/index.jsx | 200 +++++++++++++++- src/renderer/update-plan.cjs | 34 ++- test/ipc-wiring.test.cjs | 19 +- test/patch-apply.integration.test.cjs | 319 ++++++++++++++++++++++++++ test/patch-plan.test.cjs | 258 +++++++++++++++++++++ 9 files changed, 1505 insertions(+), 9 deletions(-) create mode 100644 src/patch-apply.js create mode 100644 src/patch-plan.cjs create mode 100644 test/patch-apply.integration.test.cjs create mode 100644 test/patch-plan.test.cjs diff --git a/src/main.js b/src/main.js index 0bdfde8..7bf14c1 100644 --- a/src/main.js +++ b/src/main.js @@ -29,6 +29,8 @@ const { buildMenuTemplate } = require('./menu'); const { killChildTree } = require('./kill-tree'); const { normalizeEol } = require('./git-update.cjs'); const { ensureAutocrlf, readTrunkInfo, collectDirtyFiles, discardChanges, updateToLatestTrunk } = require('./trunk-update'); +const { applyPatchToDir } = require('./patch-apply'); +const { parsePatchFiles, planApply } = require('./patch-plan.cjs'); const { openExternalUrl, ALLOWED_URL_SCHEMES } = require('./external-url'); const { deleteRegisteredSite } = require('./site-registry'); const { getStore } = require('./settings-store'); @@ -440,14 +442,18 @@ ipcMain.handle('git:update-trunk', async (event, sitePath) => { (async () => { try { const result = await updateToLatestTrunk({ dir: sitePath, url: WORDPRESS_GIT_URL, onLog: sendLog }); + // An update resets the worktree, so any applied patch is gone with + // it either way — clear the record so the "applied" banner does not + // outlive the patch. (This is also where a discard's cleanup lands: + // the dirty-tree modal always discards and then updates.) if (result.upToDate) { - await mergeSiteMeta(sitePath, { trunkOid: result.oldOid, trunkDate: result.trunkDate }); + await mergeSiteMeta(sitePath, { trunkOid: result.oldOid, trunkDate: result.trunkDate, appliedPatch: null }); } else { // HEAD has moved but install/build have not run yet: persist // the incomplete flag now so the state survives a crash or // quit mid-chain; the renderer clears it after a successful // build. - await mergeSiteMeta(sitePath, { trunkOid: result.newOid, trunkDate: result.trunkDate, updateIncomplete: true }); + await mergeSiteMeta(sitePath, { trunkOid: result.newOid, trunkDate: result.trunkDate, updateIncomplete: true, appliedPatch: null }); } sendDone({ ok: true, ...result }); } catch (e) { @@ -467,6 +473,116 @@ ipcMain.handle('git:update-trunk', async (event, sitePath) => { return { updateId }; }); +// --- Applying someone else's patch (#11) --- the diff mechanics live in +// src/patch-apply.js; these handlers add IPC plumbing and electron-store writes. + +// A patch big enough to bloat the settings file is not worth keeping around for +// an undo button. Above this the patch still applies, only "Revert" is not +// offered — said out loud rather than silently dropped. +const REVERTABLE_PATCH_LIMIT = 512 * 1024; + +// Reading a patch without touching the checkout, so the contributor sees which +// files it would change — and which of their own edits it collides with — +// before deciding. +ipcMain.handle('git:preview-patch', async (_e, sitePath, patchText) => { + try { + const parsed = parsePatchFiles(patchText); + if (!parsed.ok) return { ok: false, error: parsed.error }; + let dirtyPaths = []; + try { dirtyPaths = await collectDirtyFiles(sitePath); } catch {} + const plan = planApply({ files: parsed.files, dirtyPaths }); + return { ok: true, ...plan, files: parsed.files.map((f) => ({ kind: f.kind, path: f.path })) }; + } catch (e) { + return { ok: false, error: String(e) }; + } +}); + +ipcMain.handle('dialog:choose-patch-file', async () => { + const result = await dialog.showOpenDialog({ + title: 'Choose a patch file', + properties: ['openFile'], + filters: [ + { name: 'Patch Files', extensions: ['patch', 'diff'] }, + { name: 'All Files', extensions: ['*'] } + ] + }); + if (result.canceled || result.filePaths.length === 0) return null; + const filePath = result.filePaths[0]; + try { + const text = await fs.promises.readFile(filePath, 'utf8'); + return { filePath, name: path.basename(filePath), text }; + } catch (e) { + return { filePath, error: String(e) }; + } +}); + +ipcMain.handle('git:apply-patch', async (event, sitePath, options = {}) => { + const applyId = `${Date.now()}-${Math.random().toString(16).slice(2)}`; + const sender = event.sender; + const sendLog = (data) => { + try { sender.send('git:apply-patch:log', { applyId, data }); } catch {} + }; + const sendDone = (payload) => { + try { sender.send('git:apply-patch:done', { applyId, ...payload }); } catch {} + }; + + (async () => { + try { + const reverse = Boolean(options.reverse); + // Reverting reads the patch the app stored when it applied it, so + // the renderer never has to hold a copy of the text. + let patchText = String(options.patchText || ''); + let label = String(options.label || 'patch'); + const s = await getStore(); + const stored = ((s.get('siteMeta') || {})[sitePath] || {}).appliedPatch; + if (reverse) { + if (!stored || !stored.text) { + sendDone({ ok: false, error: 'There is no stored patch to revert.' }); + return; + } + patchText = stored.text; + label = stored.label || label; + } else if (stored) { + // Only one patch is tracked at a time, so a second apply would + // make the first one silently unrevertable and invisible. + sendDone({ ok: false, error: `${stored.label} is already applied. Revert it before applying another patch.` }); + return; + } + sendLog(`\n${reverse ? 'Reverting' : 'Applying'} ${label}…\n`); + + const result = await applyPatchToDir({ dir: sitePath, patchText, reverse, onLog: sendLog }); + if (!result.ok) { + sendDone({ ok: false, ...result }); + return; + } + + if (reverse) { + await mergeSiteMeta(sitePath, { appliedPatch: null }); + } else { + const revertable = patchText.length <= REVERTABLE_PATCH_LIMIT; + if (!revertable) { + sendLog('This patch is too large to keep for an undo, so Revert will not be offered.\n'); + } + await mergeSiteMeta(sitePath, { + appliedPatch: { + label, + appliedAt: new Date().toISOString(), + files: result.applied, + text: revertable ? patchText : null + } + }); + } + sendDone({ ok: true, ...result, reverse }); + } catch (e) { + logError('git:apply-patch', String(e && e.stack ? e.stack : e)); + sendLog(`\nApplying the patch failed: ${String(e && e.message ? e.message : e)}\n`); + sendDone({ ok: false, error: String(e) }); + } + })(); + + return { applyId }; +}); + ipcMain.handle('sites:mark-update-complete', async (_e, sitePath) => { await mergeSiteMeta(sitePath, { updateIncomplete: false }); return true; @@ -546,9 +662,20 @@ ipcMain.handle('site:status', async (_e, sitePath) => { } } catch {} - return { hasNodeModules, hasBuilt, skipInitWizard: Boolean(m.skipInitWizard), initialized: Boolean(m.initialized), installFailed: Boolean(m.installFailed), trunkOid, trunkDate, updateIncomplete: Boolean(m.updateIncomplete), tracTicket: m.tracTicket || null }; + // Summarised rather than passed through: the stored patch text is only + // needed by the main process to reverse it, and this is polled. + const appliedPatch = m.appliedPatch + ? { + label: m.appliedPatch.label, + appliedAt: m.appliedPatch.appliedAt, + files: m.appliedPatch.files || [], + revertable: Boolean(m.appliedPatch.text) + } + : null; + + return { hasNodeModules, hasBuilt, skipInitWizard: Boolean(m.skipInitWizard), initialized: Boolean(m.initialized), installFailed: Boolean(m.installFailed), trunkOid, trunkDate, updateIncomplete: Boolean(m.updateIncomplete), tracTicket: m.tracTicket || null, appliedPatch }; } catch { - return { hasNodeModules: false, hasBuilt: false, skipInitWizard: false, initialized: false, installFailed: false, trunkOid: null, trunkDate: null, updateIncomplete: false, tracTicket: null }; + return { hasNodeModules: false, hasBuilt: false, skipInitWizard: false, initialized: false, installFailed: false, trunkOid: null, trunkDate: null, updateIncomplete: false, tracTicket: null, appliedPatch: null }; } }); diff --git a/src/patch-apply.js b/src/patch-apply.js new file mode 100644 index 0000000..2c62f97 --- /dev/null +++ b/src/patch-apply.js @@ -0,0 +1,281 @@ +'use strict'; + +/** + * Applying someone else's patch to a checkout (issue #11). + * + * There is no `git apply` available: the app never shells out to a git binary, + * and isomorphic-git has no apply primitive. So hunks are matched and written + * here, using the `diff` package the app already bundles for the generating + * side. + * + * The rule that shapes everything below is **all or nothing**. A patch that + * half-applies is worse than one that does not apply at all: the contributor + * would build it, test it, and draw conclusions from a tree that matches + * neither trunk nor the patch. Every file is resolved in memory first — and + * because a write can still fail on the way out (a directory that is really a + * file, a read-only attribute, Windows holding a file open, a full disk), the + * previous contents are captured during resolution and restored if any write + * throws. + */ + +const fs = require('fs'); +const path = require('path'); +const JsDiff = require('diff'); +const { ensureAutocrlf } = require('./trunk-update'); +const { normalizeEol } = require('./git-update.cjs'); +const { parsePatchFiles } = require('./patch-plan.cjs'); + +/** + * Resolves a patch's repo-relative path inside the site directory, refusing + * anything that climbs out of it. A patch is untrusted input downloaded from a + * ticket, and `../../` in a header would otherwise write anywhere on disk. + * + * `path.resolve` normalises `..` but not symlinks, and a checkout contains + * plenty of those, so the deepest ancestor that exists is realpath-ed before + * the comparison. Otherwise a path leading through a symlinked directory + * passes a purely lexical check and lands outside the site folder. + * + * @param {string} dir + * @param {string} relPath + * @return {string|null} Absolute path, or null if it escapes the directory. + */ +function resolveInside(dir, relPath) { + let root; + try { root = fs.realpathSync(path.resolve(dir)); } catch { root = path.resolve(dir); } + + const lexical = path.resolve(root, relPath); + // Walk up to the nearest existing ancestor, resolve that for real, then put + // the not-yet-existing remainder back on. + let existing = lexical; + const trailing = []; + 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; } + + if (abs !== root && !abs.startsWith(root + path.sep)) return null; + return abs; +} + +/** + * The line ending a file already uses, so applying a patch does not silently + * rewrite a genuinely-CRLF file to LF. wordpress-develop carries fixtures whose + * line endings are the thing under test. + * + * @param {string} text + * @return {string} + */ +function dominantEol(text) { + const crlf = (text.match(/\r\n/g) || []).length; + if (!crlf) return '\n'; + const lf = (text.match(/\n/g) || []).length; + return crlf * 2 >= lf ? '\r\n' : '\n'; +} + +/** + * Inverts one parsed file so the patch can be undone. + * + * Reversing through `formatPatch` would be simpler but wrong: it emits the + * headers swapped (`--- b/…`, `+++ a/…`), which no longer look like a git + * patch to the prefix-stripping in patch-plan.cjs, and the paths come back + * carrying a literal `b/`. Reversing the hunks while keeping the paths already + * resolved on the way in avoids the round trip entirely. + * + * @param {Object} file + * @return {Object} + */ +function reverseFile(file) { + const reversed = JsDiff.reversePatch(file.patch); + const INVERSE = { add: 'delete', delete: 'add' }; + const kind = INVERSE[file.kind] || file.kind; + const oldPath = file.newPath; + const newPath = file.oldPath; + return { + ...file, + kind, + oldPath, + newPath, + path: kind === 'delete' ? oldPath : newPath, + hunks: reversed.hunks || [], + patch: reversed + }; +} + +/** + * Works out what one file's new content should be, without touching disk. + * Also captures what is there now, so a failed write can be rolled back. + * + * @param {string} dir + * @param {Object} file + * @return {Object} + */ +function resolveFile(dir, file) { + const target = resolveInside(dir, file.path); + if (!target) return { error: `${file.path} points outside the site folder` }; + + const readIfPresent = (abs) => (fs.existsSync(abs) ? fs.readFileSync(abs) : null); + + 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) }; + } + + if (file.kind === 'add') { + if (fs.existsSync(target)) return { error: `${file.path} already exists, so the patch cannot add it` }; + const content = JsDiff.applyPatch('', file.patch); + if (content === false) return { error: `${file.path} could not be created from the patch` }; + return { op: 'write', abs: target, path: file.path, content, previous: null }; + } + + if (file.kind === 'rename') { + const source = resolveInside(dir, file.oldPath); + if (!source) return { error: `${file.oldPath} points outside the site folder` }; + if (!fs.existsSync(source)) return { error: `${file.oldPath} is not in this checkout, so the patch cannot move it` }; + if (fs.existsSync(target)) return { error: `${file.newPath} already exists, so the patch cannot move ${file.oldPath} onto it` }; + 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'); + } + return { + op: 'rename', abs: target, from: source, path: file.path, content, + previous: null, previousFrom: readIfPresent(source) + }; + } + + if (!fs.existsSync(target)) return { error: `${file.path} is not in this checkout, so the patch does not fit it` }; + + // Matching happens on LF, the way the generating side normalises, so a CRLF + // checkout does not make every context line miss — but the file is written + // back with the endings it already had. + const raw = fs.readFileSync(target, 'utf8'); + const applied = JsDiff.applyPatch(normalizeEol(raw), file.patch); + if (applied === false) { + return { error: `${file.path} has moved on since the patch was written, so it no longer applies` }; + } + const content = dominantEol(raw) === '\r\n' ? applied.replace(/\n/g, '\r\n') : applied; + return { op: 'write', abs: target, path: file.path, content, previous: Buffer.from(raw, 'utf8') }; +} + +/** + * Puts back everything a failed run had already written. + * + * @param {Array} done + */ +function rollback(done) { + for (const action of done.reverse()) { + try { + if (action.op === 'rename' && action.previousFrom !== null) { + fs.mkdirSync(path.dirname(action.from), { recursive: true }); + fs.writeFileSync(action.from, action.previousFrom); + fs.rmSync(action.abs, { force: true }); + continue; + } + if (action.previous === null) { + fs.rmSync(action.abs, { force: true }); + continue; + } + fs.mkdirSync(path.dirname(action.abs), { recursive: true }); + fs.writeFileSync(action.abs, action.previous); + } catch { + // Best effort: the caller is already reporting a failure, and a + // rollback that cannot run must not mask the original cause. + } + } +} + +/** + * Applies (or reverses) a patch across a checkout. + * + * Binary files are skipped and named rather than failing the whole patch: a + * text diff cannot carry their content, and refusing an otherwise-good pull + * request over an image would help nobody. Everything else is all or nothing. + * + * @param {Object} root0 + * @param {string} root0.dir + * @param {string} root0.patchText + * @param {boolean} [root0.reverse] + * @param {Function} [root0.onLog] + * @return {Promise} + */ +async function applyPatchToDir({ dir, patchText, reverse = false, onLog = () => {} }) { + const parsed = parsePatchFiles(patchText); + if (!parsed.ok) return { ok: false, error: parsed.error }; + + await ensureAutocrlf(dir); + + const files = reverse ? parsed.files.map(reverseFile) : parsed.files; + const actions = []; + const skipped = []; + const failures = []; + + for (const file of files) { + if (file.kind === 'binary') { + skipped.push(file.path); + continue; + } + const resolved = resolveFile(dir, file); + if (resolved.error) { + failures.push(resolved.error); + continue; + } + actions.push(resolved); + } + + if (failures.length) { + onLog(`\nThe patch was not applied — the checkout is unchanged.\n${failures.map((f) => ` • ${f}\n`).join('')}`); + return { ok: false, error: failures[0], failures, applied: [], skipped }; + } + + if (!actions.length && !skipped.length) { + return { ok: false, error: 'The patch does not change any files.', applied: [], skipped }; + } + + // Every file resolved cleanly, so the writes below are the first thing to + // touch the working tree — and the only place a partial result could still + // appear, which is what the rollback is for. + const done = []; + try { + 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); + } + } catch (e) { + rollback(done); + const message = `writing ${String(e && e.message ? e.message : e)}`; + onLog(`\nThe patch could not be written, so the checkout was put back as it was: ${message}\n`); + return { ok: false, error: message, applied: [], skipped, rolledBack: true }; + } + + // New files are deliberately left unstaged. Staging them is what leaves the + // residue that updateToLatestTrunk has to clear before a force checkout + // (see staleStagedPaths in git-update.cjs), and an unstaged new file still + // shows up in the patch the contributor generates afterwards. + const applied = actions.map((a) => a.path); + onLog(`\n${reverse ? 'Reverted' : 'Applied'} ${applied.length} file${applied.length === 1 ? '' : 's'}.\n`); + if (skipped.length) { + onLog(`Skipped ${skipped.length} binary file${skipped.length === 1 ? '' : 's'} the app cannot apply: ${skipped.join(', ')}\n`); + } + + return { ok: true, applied, skipped }; +} + +module.exports = { applyPatchToDir, resolveInside, reverseFile, dominantEol }; diff --git a/src/patch-plan.cjs b/src/patch-plan.cjs new file mode 100644 index 0000000..5578c7b --- /dev/null +++ b/src/patch-plan.cjs @@ -0,0 +1,247 @@ +'use strict'; + +/** + * Reading a patch before applying it (issue #11): what files it touches, where + * those files live in today's checkout, and what applying it would disturb. + * + * Patches reach the app from three places that do not agree on a format: + * attachments on a Trac ticket (Subversion style, no `a/` `b/` prefixes, paths + * sometimes against the pre-`src/` layout), `.diff` files from a + * wordpress-develop pull request (git style, `a/` `b/` prefixes), and the app's + * own generated patches (`createTwoFilesPatch` output, `a/` `b/` prefixes but + * no `diff --git` line). Everything downstream works in repo-relative paths, so + * normalising happens here, once. + * + * Lives at `src/` rather than `src/renderer/` because both the main process and + * the applier consume it — same placement as git-update.cjs — and it keeps the + * `diff` package out of the renderer bundle. The renderer-facing half of this + * feature (the step chain) is in renderer/update-plan.cjs instead. + */ + +const JsDiff = require('diff'); +const { normalizeEol } = require('./git-update.cjs'); + +// Files that stayed at the repo root when core moved everything else under +// src/. A patch naming one of these is already correct for today's layout. +const ROOT_FILES = [ + '.editorconfig', + '.gitignore', + '.jshintrc', + '.travis.yml', + 'Gruntfile.js', + 'package.json', + 'phpunit.xml.dist', + 'wp-cli.yml', + 'wp-config-sample.php', + 'wp-tests-config-sample.php' +]; + +// Files that did move under src/ despite not being wp-* prefixed, so the +// wp-* rule below would miss them. +const SRC_FILES = ['index.php', 'license.txt', 'readme.html', 'xmlrpc.php']; + +// Directories that only ever existed in the modern layout. +const MODERN_DIRS = ['src/', 'tests/', 'tools/']; + +/** + * Strips the leading `a/` and `b/` that git puts on diff headers. + * + * Deliberately conditional: Subversion-style patches from Trac carry no prefix + * at all, so stripping unconditionally would turn `wp-admin/admin.php` into + * `admin.php` and write to the wrong place. Both sides have to look prefixed + * before either is trusted — `/dev/null` counts as agreement, since an added or + * deleted file only has one real side. + * + * @param {string} oldName + * @param {string} newName + * @return {{oldPath: string, newPath: string}} + */ +function stripPathPrefix(oldName, newName) { + const isNull = (name) => name === '/dev/null'; + const looksPrefixed = (name, letter) => isNull(name) || new RegExp(`^${letter}/`).test(name); + const bothPrefixed = looksPrefixed(oldName, 'a') && looksPrefixed(newName, 'b'); + const drop = (name) => (isNull(name) || !bothPrefixed ? name : name.slice(2)); + // `Index: trunk/wp-…` is what an older Subversion checkout produced; the + // branch name is not part of the repo-relative path either way. + const dropTrunk = (name) => (isNull(name) ? name : name.replace(/^trunk\//, '')); + return { oldPath: dropTrunk(drop(oldName)), newPath: dropTrunk(drop(newName)) }; +} + +/** + * Rewrites a path written against the pre-src/ layout to where that file lives + * today. A patch attached to a ticket years ago still names `wp-admin/…`. + * + * @param {string} filePath + * @return {string} + */ +function mapToSrcLayout(filePath) { + if (!filePath || filePath === '/dev/null') return filePath; + if (MODERN_DIRS.some((dir) => filePath.startsWith(dir))) return filePath; + if (ROOT_FILES.includes(filePath)) return filePath; + if (SRC_FILES.includes(filePath)) return `src/${filePath}`; + if (filePath.startsWith('wp-')) return `src/${filePath}`; + // Unrecognised: leave it alone rather than guess a move that would write + // outside the tree the contributor expects. + return filePath; +} + +/** + * Walks the raw patch for its per-file section headers. + * + * Needed because jsdiff returns the byte-identical shape `[{hunks: []}]` for a + * binary file, a 100%-similarity rename, and text that is not a patch at all — + * it keeps neither the filenames nor the marker that tells them apart. Without + * this, prose pasted in would be reported as a binary file and a pure rename + * would be rejected as garbage. + * + * @param {string} raw + * @return {Array<{path: string, isBinary: boolean, renameFrom: string, renameTo: string}>} + */ +function scanSections(raw) { + const sections = []; + const last = () => sections[sections.length - 1]; + for (const line of raw.split('\n')) { + const git = /^diff --git (?:"?a\/)?(.+?)"? (?:"?b\/)?(.+?)"?$/.exec(line); + if (git) { + sections.push({ path: git[2] || git[1], isBinary: false, renameFrom: '', renameTo: '' }); + continue; + } + const svn = /^Index: (.+)$/.exec(line); + if (svn) { + sections.push({ path: svn[1].trim(), isBinary: false, renameFrom: '', renameTo: '' }); + continue; + } + if (!sections.length) continue; + if (/^Binary files .* differ$/.test(line) || /^GIT binary patch$/.test(line)) { + last().isBinary = true; + continue; + } + const from = /^rename from (.+)$/.exec(line); + if (from) { last().renameFrom = from[1].trim(); continue; } + const to = /^rename to (.+)$/.exec(line); + if (to) last().renameTo = to[1].trim(); + } + return sections; +} + +/** + * @param {Object} file A jsdiff parsePatch entry. + * @param {string} oldPath + * @param {string} newPath + * @return {string} + */ +function classify(file, oldPath, newPath) { + if (oldPath === '/dev/null') return 'add'; + if (newPath === '/dev/null') return 'delete'; + if (oldPath !== newPath) return 'rename'; + return 'modify'; +} + +/** + * Parses a patch into the files it touches, with paths normalised to + * repo-relative form for today's layout. + * + * @param {string} text + * @return {{ok: true, files: Array}|{ok: false, error: string}} + */ +function parsePatchFiles(text) { + const raw = typeof text === 'string' ? text : ''; + if (!raw.trim()) return { ok: false, error: 'The patch is empty.' }; + + let parsed; + try { + // Normalise line endings on the way in so hunk context matches what the + // applier reads off disk, which is normalised the same way. + parsed = JsDiff.parsePatch(normalizeEol(raw)); + } catch (e) { + return { ok: false, error: `Could not read the patch: ${String(e && e.message ? e.message : e)}` }; + } + + if (!parsed || parsed.length === 0) { + return { ok: false, error: 'No file changes found in the patch.' }; + } + + const sections = scanSections(normalizeEol(raw)); + const files = []; + + for (let i = 0; i < parsed.length; i++) { + const file = parsed[i]; + + if (!file.hunks || file.hunks.length === 0) { + // jsdiff kept nothing, so the raw section is the only evidence of + // what this was. + const section = sections[i]; + if (section && section.renameFrom && section.renameTo) { + const oldPath = mapToSrcLayout(section.renameFrom); + const newPath = mapToSrcLayout(section.renameTo); + files.push({ kind: 'rename', oldPath, newPath, path: newPath, hunks: [], patch: file }); + continue; + } + if (section && section.isBinary) { + const binaryPath = mapToSrcLayout(stripPathPrefix(section.path, section.path).newPath); + files.push({ kind: 'binary', oldPath: binaryPath, newPath: binaryPath, path: binaryPath, hunks: [], patch: file }); + continue; + } + return { ok: false, error: 'That does not look like a patch — no file changes found.' }; + } + + const oldName = file.oldFileName || file.index || ''; + const newName = file.newFileName || file.index || ''; + const { oldPath, newPath } = stripPathPrefix(oldName, newName); + const kind = classify(file, oldPath, newPath); + const target = kind === 'delete' ? oldPath : newPath; + files.push({ + kind, + oldPath: mapToSrcLayout(oldPath), + newPath: mapToSrcLayout(newPath), + path: mapToSrcLayout(target), + hunks: file.hunks, + patch: file + }); + } + + return { ok: true, files }; +} + +/** + * What applying these files would mean for a given checkout. + * + * `conflicts` is the honest version of the dirty-tree question: applying a + * patch is not destructive the way a hard reset is, so the only changes worth + * mentioning are the ones on files the contributor has already edited. + * + * @param {Object} root0 + * @param {Array} root0.files + * @param {string[]} [root0.dirtyPaths] + * @return {{paths: string[], conflicts: string[], unsupported: string[], needsInstall: boolean}} + */ +function planApply({ files, dirtyPaths = [] } = {}) { + const list = Array.isArray(files) ? files : []; + const paths = list.map((f) => f.path).filter(Boolean); + // A rename disturbs the file it moves away from as well as the one it + // creates, so both sides count when looking for collisions. + const touched = new Set(paths); + for (const f of list) { + if (f.kind === 'rename' && f.oldPath) touched.add(f.oldPath); + } + const dirty = new Set(dirtyPaths); + return { + paths, + conflicts: [...touched].filter((p) => dirty.has(p)), + // Binary hunks cannot be applied from a text diff. Naming them is the + // difference between "this patch is partly unapplied" and a silent gap. + unsupported: list.filter((f) => f.kind === 'binary').map((f) => f.path || '(unnamed binary file)'), + // 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') + }; +} + +module.exports = { + ROOT_FILES, + SRC_FILES, + stripPathPrefix, + mapToSrcLayout, + parsePatchFiles, + planApply +}; diff --git a/src/preload.js b/src/preload.js index 261b7d9..2c23dc7 100644 --- a/src/preload.js +++ b/src/preload.js @@ -77,6 +77,27 @@ contextBridge.exposeInMainWorld('api', { discardChanges: (sitePath) => ipcRenderer.invoke('git:discard-changes', sitePath) , markUpdateComplete: (sitePath) => ipcRenderer.invoke('sites:mark-update-complete', sitePath) +, + choosePatchFile: () => ipcRenderer.invoke('dialog:choose-patch-file') +, + previewPatch: (sitePath, patchText) => ipcRenderer.invoke('git:preview-patch', sitePath, patchText) +, + applyPatch: async (sitePath, options, onLog, onDone) => { + const { applyId } = await ipcRenderer.invoke('git:apply-patch', sitePath, options); + const logHandler = (_e, payload) => { + if (payload.applyId === applyId && onLog) onLog(payload); + }; + const doneHandler = (_e, payload) => { + if (payload.applyId === applyId) { + ipcRenderer.removeListener('git:apply-patch:log', logHandler); + ipcRenderer.removeListener('git:apply-patch:done', doneHandler); + if (onDone) onDone(payload); + } + }; + ipcRenderer.on('git:apply-patch:log', logHandler); + ipcRenderer.on('git:apply-patch:done', doneHandler); + return { applyId }; + } , updateTrunk: async (sitePath, onLog, onDone) => { const { updateId } = await ipcRenderer.invoke('git:update-trunk', sitePath); diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index 5c4d766..f701219 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -19,7 +19,7 @@ import 'xterm/css/xterm.css'; import { computeSetupStepState } from './setup-steps.cjs'; import { planDevServerStart, formatElapsed } from './dev-server-command.cjs'; import { pathBasename } from './path-basename.cjs'; -import { trunkAgeInfo, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE } from './update-plan.cjs'; +import { trunkAgeInfo, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, APPLY_STATE_TO_STEP } from './update-plan.cjs'; import { parseTicketRef, ticketUrl } from './trac-ticket.cjs'; const TERMINAL_ALLOWED_SCRIPTS = ['build', 'build:dev', 'dev', 'test', 'watch', 'grunt']; @@ -811,6 +811,14 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit const [trunkDate, setTrunkDate] = useState(null); const [updateIncomplete, setUpdateIncomplete] = useState(false); const [updateState, setUpdateState] = useState('idle'); // idle | fetching | installing | building + // Applying someone else's patch (#11) + const [applyState, setApplyState] = useState('idle'); // idle | applying | installing | building + const [applyPreview, setApplyPreview] = useState(null); + // Held separately from applyPreview: the preview is cleared the moment the + // chain starts, and the step list still has to know whether install runs. + const [applyNeedsInstall, setApplyNeedsInstall] = useState(false); + const [applyError, setApplyError] = useState(''); + const [appliedPatch, setAppliedPatch] = useState(null); const [dirtyModalOpen, setDirtyModalOpen] = useState(false); const [dirtySaving, setDirtySaving] = useState(false); const [dirtyFiles, setDirtyFiles] = useState([]); @@ -938,6 +946,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit setTrunkDate(s?.trunkDate || null); setUpdateIncomplete(Boolean(s?.updateIncomplete)); setTracTicket(s?.tracTicket || null); + setAppliedPatch(s?.appliedPatch || null); if (metaPatchRef.current) { // A null trunkDate here means the git read failed (e.g. clone still // running) — keep whatever the sidebar already shows in that case. @@ -1539,6 +1548,113 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit } }; + // --- Applying someone else's patch (#11) --- + // Same three-stage shape as the update chain, and the same npm wrappers, so + // exit codes and terminal streaming behave identically. + const isApplying = applyState !== 'idle'; + const applySteps = planApplySteps({ needsInstall: applyNeedsInstall }); + const applyStepStates = updateStepStatuses(applySteps, applyState, APPLY_STATE_TO_STEP); + + const finishApply = (message) => { + terminalStateRef.current.running = false; + terminalKillRef.current = null; + setApplyState('idle'); + if (message) writeToTerminal(message); + loadStatus().catch(() => {}); + }; + + const runApplyInstallAndBuild = (needsInstall, verb) => { + const runBuildStep = () => { + setApplyState('building'); + writeToTerminal('\nRunning npm run build…\n'); + runScript('build', { + onLog: (chunk) => writeToTerminal(chunk), + onDone: ({ code }) => { + finishApply(code === 0 + ? `\n${verb} — open the site to try it out.\n` + : `\nThe patch is ${verb.toLowerCase()} but the build failed, so the site still runs the old assets.\n`); + } + }); + }; + if (needsInstall) { + setApplyState('installing'); + writeToTerminal('\nThe patch changes package-lock.json — running npm install…\n'); + runInstall({ + onLog: (chunk) => writeToTerminal(chunk), + onDone: ({ code }) => { + if (code !== 0) { + finishApply('\nnpm install failed, so the build was skipped. The patch is applied but dependencies are stale.\n'); + return; + } + runBuildStep(); + } + }); + } else { + writeToTerminal(`\n${SKIP_INSTALL_MESSAGE}\n`); + runBuildStep(); + } + }; + + // Reads a patch file and works out what it would do, without touching the + // checkout — the contributor decides after seeing the file list. + const choosePatchFile = async () => { + setApplyError(''); + try { + const chosen = await window.api.choosePatchFile(); + if (!chosen) return; + if (chosen.error) { + setApplyError(`Could not read that file: ${chosen.error}`); + return; + } + const preview = await window.api.previewPatch(sitePath, chosen.text); + if (!preview || !preview.ok) { + setApplyError(preview?.error || 'Could not read that patch.'); + return; + } + setApplyPreview({ ...preview, label: chosen.name, text: chosen.text }); + } catch (e) { + setApplyError(String(e)); + } + }; + + const runApply = ({ reverse = false } = {}) => { + const state = terminalStateRef.current; + if (state.running) { + writeToTerminal('A command is already running. Press Ctrl+C to stop it.\n'); + return; + } + const preview = applyPreview; + const needsInstall = reverse + ? Boolean(appliedPatch?.files?.includes('package-lock.json')) + : Boolean(preview.needsInstall); + setApplyError(''); + setApplyNeedsInstall(needsInstall); + setApplyState('applying'); + state.running = true; + // 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(() => {}); }; + window.api.applyPatch( + sitePath, + reverse ? { reverse: true } : { patchText: preview.text, label: preview.label }, + ({ data }) => writeToTerminal(data), + (res) => { + if (!res || !res.ok) { + setApplyError(res?.error || 'The patch could not be applied.'); + finishApply(); + return; + } + setApplyPreview(null); + runApplyInstallAndBuild(needsInstall, reverse ? 'Reverted' : 'Applied'); + } + ).catch((e) => { + // A rejected invoke never reaches onDone, so without this the terminal + // stays wedged with `running` set and no way back short of a reload. + setApplyError(String(e)); + finishApply(); + }); + }; + // Step 1: fetch + reset in the main process, then hand over to the npm // steps. Assumes the tree is clean (startTrunkUpdate handles dirty trees). const beginTrunkUpdate = () => { @@ -2123,6 +2239,88 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit )} + {skipInit ? ( +
+
Try someone else's patch
+
+ Apply a .diff or .patch file to this checkout and rebuild, so you can test the work before adding your own. Your own changes are left alone. +
+ + {appliedPatch && !isApplying ? ( +
+
+ {appliedPatch.label} is applied — {appliedPatch.files.length} file{appliedPatch.files.length === 1 ? '' : 's'} + {appliedPatch.appliedAt ? `, ${new Date(appliedPatch.appliedAt).toLocaleString()}` : ''}. +
+
+ {appliedPatch.revertable ? ( + + ) : ( + Too large to undo automatically — use Update to latest trunk to reset. + )} +
+
+ ) : null} + + {applyPreview && !isApplying ? ( +
+
+ {applyPreview.label} changes {applyPreview.paths.length} file{applyPreview.paths.length === 1 ? '' : 's'}: +
+
+ {applyPreview.paths.map((p) =>
{p}
)} +
+ {applyPreview.conflicts.length ? ( +
+ You have your own edits to {applyPreview.conflicts.join(', ')}. The patch is applied on top of them: it succeeds if the changes do not overlap, and fails without touching anything if they do. Save a patch of your work first if you want a copy. +
+ ) : null} + {applyPreview.unsupported.length ? ( +
+ {applyPreview.unsupported.join(', ')} {applyPreview.unsupported.length === 1 ? 'is a binary file and will be skipped' : 'are binary files and will be skipped'}. +
+ ) : null} + {applyPreview.needsInstall ? ( +
It changes package-lock.json, so dependencies will be installed before the rebuild.
+ ) : null} +
+ + +
+
+ ) : null} + + {isApplying ? ( +
+ {applyStepStates.map((state, i) => { + const step = applySteps[i]; + const mark = UPDATE_STEP_MARKS[state.status]; + const stepLabel = state.status === 'skipped' ? step.skipMessage : step.label; + return ( +
+ + {stepLabel} +
+ ); + })} +
+ ) : null} + + {applyError ? ( +
+ {applyError} The checkout was not changed. +
+ ) : null} + + {!applyPreview && !isApplying ? ( +
+ +
+ ) : null} +
+ ) : null}
Terminal
diff --git a/src/renderer/update-plan.cjs b/src/renderer/update-plan.cjs index 186a8ac..e2e4567 100644 --- a/src/renderer/update-plan.cjs +++ b/src/renderer/update-plan.cjs @@ -66,16 +66,43 @@ function planUpdateSteps({ lockfileChanged } = {}) { // Which chain step each renderer updateState is executing. const STATE_TO_STEP = { fetching: 'fetch', installing: 'install', building: 'build' }; +// Applying someone else's patch (#11) is the same three-stage chain with a +// different first step, so it shares updateStepStatuses below. It lives here +// rather than beside the patch parsing because this module is the renderer's +// half and carries no dependencies — importing the parser into the bundle +// would drag the `diff` package in for two constants. +const APPLY_STATE_TO_STEP = { applying: 'apply', installing: 'install', building: 'build' }; + +/** + * The chain applying a patch runs. Like the update chain, the install step is + * named even when skipped so "apply" always means the same thing. + * + * @param {Object} root0 + * @param {boolean} [root0.needsInstall] + * @return {Array} + */ +function planApplySteps({ needsInstall } = {}) { + return [ + { key: 'apply', label: 'Apply the patch', skipped: false }, + { key: 'install', label: 'Install dependencies', skipped: !needsInstall, skipMessage: SKIP_INSTALL_MESSAGE }, + { key: 'build', label: 'Rebuild', skipped: false } + ]; +} + /** * Maps the chain steps to checklist visual states for a given renderer * updateState. Steps before the current one are complete, the current one is * current, later ones pending; skipped steps stay 'skipped' once passed. * + * The state→step map is a parameter so a different chain can reuse this: the + * applying chain (#11) has the same three-stage shape with its own state names. + * * @param {Array} steps * @param {string} updateState + * @param {Object} [stateToStep] */ -function updateStepStatuses(steps, updateState) { - const activeKey = STATE_TO_STEP[updateState] || null; +function updateStepStatuses(steps, updateState, stateToStep = STATE_TO_STEP) { + const activeKey = stateToStep[updateState] || null; const order = steps.map((s) => s.key); let activeIndex = -1; if (activeKey) { @@ -116,6 +143,9 @@ function updateOutcome({ fetchOk, upToDate, moved, installNeeded, installCode, b module.exports = { STALE_THRESHOLD_DAYS, SKIP_INSTALL_MESSAGE, + STATE_TO_STEP, + APPLY_STATE_TO_STEP, + planApplySteps, trunkAgeInfo, planUpdateSteps, updateStepStatuses, diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index bbc0e2c..a44d088 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -952,6 +952,18 @@ test('sites:set-ticket refuses unregistered site paths before writing metadata', assert.deepEqual(settings.values.siteMeta, {}); }); +// --- apply handlers (#11) ------------------------------------------------ + +test('git:preview-patch reads the patch through patch-plan', async () => { + const parsePatchFiles = spy(() => ({ ok: false, error: 'unreadable' })); + const main = loadMain({ stubs: { ...silentLogging(), './patch-plan.cjs': { parsePatchFiles, planApply: () => ({}) } } }); + + const result = await main.invoke('git:preview-patch', '/sites/wp', 'PATCH TEXT'); + + assert.deepEqual(parsePatchFiles.calls, [['PATCH TEXT']]); + assert.deepEqual(result, { ok: false, error: 'unreadable' }); +}); + // --- the harness's own guard --------------------------------------------- // Requiring the real `electron` package is not a harmless fallback: its @@ -1000,7 +1012,8 @@ const WIRED = new Set([ 'playground:stop', 'playground-web:start', 'playground-web:stop', - 'sites:set-ticket' + 'sites:set-ticket', + 'git:preview-patch' ]); // Channels with no module to reach: they read or write electron-store, drive a @@ -1016,6 +1029,7 @@ const NO_DELEGATION = new Map([ ['sites:forget', 'electron-store write'], ['sites:set-label', 'electron-store write'], ['dialog:choose-dir', 'opens the directory dialog'], + ['dialog:choose-patch-file', 'opens the file-open dialog and reads the chosen file'], ['playground-web:available', 'checks a path on disk'], ['smtp:get', 'electron-store read'], ['smtp:clear', 'electron-store write'], @@ -1037,7 +1051,8 @@ const NO_DELEGATION = new Map([ // Channels that do delegate, but whose call sits behind something this harness // cannot stand in for yet. Each one is a known hole, not an oversight. 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'] ]); const CLASSIFIED = [...WIRED, ...NO_DELEGATION.keys(), ...NOT_REACHABLE.keys()]; diff --git a/test/patch-apply.integration.test.cjs b/test/patch-apply.integration.test.cjs new file mode 100644 index 0000000..333b0c1 --- /dev/null +++ b/test/patch-apply.integration.test.cjs @@ -0,0 +1,319 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const git = require('isomorphic-git'); +const { applyPatchToDir, resolveInside, dominantEol } = require('../src/patch-apply'); + +// A real on-disk repo, like trunk-update.integration.test.cjs: applyPatchToDir +// calls ensureAutocrlf, which reads and writes git config, so a bare temp +// directory would not exercise the same path. +async function makeRepo(t, files) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'patch-apply-test-')); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + await git.init({ fs, dir, defaultBranch: 'trunk' }); + for (const [relPath, content] of Object.entries(files)) { + const abs = path.join(dir, relPath); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + await git.add({ fs, dir, filepath: relPath }); + } + await git.commit({ + fs, dir, message: 'base', + author: { name: 'Test', email: 'test@example.com' } + }); + return dir; +} + +// Snapshot every file so "nothing was written" can be asserted byte for byte +// rather than just on the one file we happen to think about. +function snapshot(dir) { + const out = {}; + const walk = (rel) => { + for (const entry of fs.readdirSync(path.join(dir, rel), { withFileTypes: true })) { + if (entry.name === '.git') continue; + const next = path.join(rel, entry.name); + if (entry.isDirectory()) walk(next); + else out[next] = fs.readFileSync(path.join(dir, next), 'utf8'); + } + }; + walk('.'); + return out; +} + +const FOO = 'src/wp-includes/foo.php'; +const BAR = 'src/wp-includes/bar.php'; +const FOO_BODY = 'one\ntwo\nthree\n'; +const BAR_BODY = 'alpha\nbeta\ngamma\n'; + +const FOO_PATCH = `diff --git a/${FOO} b/${FOO} +--- a/${FOO} ++++ b/${FOO} +@@ -1,3 +1,3 @@ + one +-two ++TWO + three +`; + +// Second file's context does not match what is on disk, so this hunk fails. +const BAR_PATCH_THAT_FAILS = `diff --git a/${BAR} b/${BAR} +--- a/${BAR} ++++ b/${BAR} +@@ -1,3 +1,3 @@ + nothing +-like ++LIKE + reality +`; + +test('applyPatchToDir: a single-file patch applies (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + const res = await applyPatchToDir({ dir, patchText: FOO_PATCH }); + assert.strictEqual(res.ok, true); + assert.deepStrictEqual(res.applied, [FOO]); + assert.strictEqual(fs.readFileSync(path.join(dir, FOO), 'utf8'), 'one\nTWO\nthree\n'); +}); + +// The rule the whole module is built around. A patch where the second file +// fails must not leave the first one rewritten. +test('applyPatchToDir: one failing file leaves the whole tree untouched (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY, [BAR]: BAR_BODY }); + const before = snapshot(dir); + + const res = await applyPatchToDir({ dir, patchText: FOO_PATCH + BAR_PATCH_THAT_FAILS }); + + assert.strictEqual(res.ok, false); + assert.match(res.error, /bar\.php/); + assert.deepStrictEqual(res.applied, []); + assert.deepStrictEqual(snapshot(dir), before, 'no file may change when any file fails'); +}); + +test('applyPatchToDir: reverting restores the original content (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + await applyPatchToDir({ dir, patchText: FOO_PATCH }); + const res = await applyPatchToDir({ dir, patchText: FOO_PATCH, reverse: true }); + assert.strictEqual(res.ok, true); + assert.strictEqual(fs.readFileSync(path.join(dir, FOO), 'utf8'), FOO_BODY); +}); + +// Reverting must undo the patch, not reset the checkout: work the contributor +// did on other files has to survive. +test('applyPatchToDir: reverting keeps unrelated local work (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY, [BAR]: BAR_BODY }); + await applyPatchToDir({ dir, patchText: FOO_PATCH }); + fs.writeFileSync(path.join(dir, BAR), 'my own work\n'); + + await applyPatchToDir({ dir, patchText: FOO_PATCH, reverse: true }); + + assert.strictEqual(fs.readFileSync(path.join(dir, FOO), 'utf8'), FOO_BODY); + assert.strictEqual(fs.readFileSync(path.join(dir, BAR), 'utf8'), 'my own work\n'); +}); + +test('applyPatchToDir: a patch creates and removes files (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + const addPatch = `diff --git a/src/new.php b/src/new.php +new file mode 100644 +--- /dev/null ++++ b/src/new.php +@@ -0,0 +1,2 @@ ++hello ++world +`; + const addRes = await applyPatchToDir({ dir, patchText: addPatch }); + assert.strictEqual(addRes.ok, true); + assert.strictEqual(fs.readFileSync(path.join(dir, 'src/new.php'), 'utf8'), 'hello\nworld\n'); + + // Reversing an addition is a deletion. + await applyPatchToDir({ dir, patchText: addPatch, reverse: true }); + assert.strictEqual(fs.existsSync(path.join(dir, 'src/new.php')), false); +}); + +// A patch is untrusted input downloaded from a ticket. +test('applyPatchToDir: a path escaping the site folder is refused (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + const outside = path.join(dir, '..', 'escaped.txt'); + const evil = `diff --git a/../escaped.txt b/../escaped.txt +new file mode 100644 +--- /dev/null ++++ b/../escaped.txt +@@ -0,0 +1 @@ ++pwned +`; + const res = await applyPatchToDir({ dir, patchText: evil }); + assert.strictEqual(res.ok, false); + assert.match(res.error, /outside the site folder/); + assert.strictEqual(fs.existsSync(outside), false); +}); + +test('applyPatchToDir: a missing target file fails without writing (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + const before = snapshot(dir); + const res = await applyPatchToDir({ dir, patchText: BAR_PATCH_THAT_FAILS }); + assert.strictEqual(res.ok, false); + assert.match(res.error, /not in this checkout/); + assert.deepStrictEqual(snapshot(dir), before); +}); + +test('applyPatchToDir: binary files are skipped and named, not silently dropped (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + const withBinary = FOO_PATCH + `diff --git a/src/x.png b/src/x.png +index 111..222 100644 +Binary files a/src/x.png and b/src/x.png differ +`; + const res = await applyPatchToDir({ dir, patchText: withBinary }); + assert.strictEqual(res.ok, true); + assert.deepStrictEqual(res.applied, [FOO]); + assert.deepStrictEqual(res.skipped, ['src/x.png']); +}); + +test('applyPatchToDir: an unreadable patch reports why and changes nothing (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + const before = snapshot(dir); + const res = await applyPatchToDir({ dir, patchText: 'this is not a patch\n' }); + assert.strictEqual(res.ok, false); + assert.deepStrictEqual(snapshot(dir), before); +}); + +// Finding from self-review: the pre-validation rejection above is the path that +// cannot write by construction. This is the one that can — a write that throws +// partway through, after earlier files are already on disk. +test('applyPatchToDir: a write failing partway through is rolled back (issue #11)', async (t) => { + // src/blocker is a regular file, so creating src/blocker/new.php fails with + // ENOTDIR — deterministically, on every platform — after foo.php has + // already been written. + const dir = await makeRepo(t, { [FOO]: FOO_BODY, 'src/blocker': 'not a directory\n' }); + const before = snapshot(dir); + + const blockedAdd = `diff --git a/src/blocker/new.php b/src/blocker/new.php +new file mode 100644 +--- /dev/null ++++ b/src/blocker/new.php +@@ -0,0 +1 @@ ++hello +`; + + const res = await applyPatchToDir({ dir, patchText: FOO_PATCH + blockedAdd }); + + assert.strictEqual(res.ok, false); + assert.strictEqual(res.rolledBack, true); + assert.deepStrictEqual(snapshot(dir), before, 'a failed write must leave nothing behind'); +}); + +test('applyPatchToDir: a rename moves the file and its content (issue #11)', async (t) => { + const dir = await makeRepo(t, { 'src/old.php': 'one\ntwo\n' }); + const renamePatch = `diff --git a/src/old.php b/src/new.php +similarity index 90% +rename from src/old.php +rename to src/new.php +--- a/src/old.php ++++ b/src/new.php +@@ -1,2 +1,2 @@ + one +-two ++TWO +`; + const res = await applyPatchToDir({ dir, patchText: renamePatch }); + assert.strictEqual(res.ok, true, res.error); + assert.strictEqual(fs.existsSync(path.join(dir, 'src/old.php')), false); + assert.strictEqual(fs.readFileSync(path.join(dir, 'src/new.php'), 'utf8'), 'one\nTWO\n'); + + await applyPatchToDir({ dir, patchText: renamePatch, reverse: true }); + assert.strictEqual(fs.readFileSync(path.join(dir, 'src/old.php'), 'utf8'), 'one\ntwo\n'); + assert.strictEqual(fs.existsSync(path.join(dir, 'src/new.php')), false); +}); + +// A 100%-similarity rename has no hunks at all, which used to be rejected as +// "not a patch" — killing the whole apply for any PR that moved a file. +test('applyPatchToDir: a pure rename with no hunks applies (issue #11)', async (t) => { + const dir = await makeRepo(t, { 'src/old.php': 'unchanged\n' }); + const purePatch = `diff --git a/src/old.php b/src/new.php +similarity index 100% +rename from src/old.php +rename to src/new.php +`; + const res = await applyPatchToDir({ dir, patchText: purePatch }); + assert.strictEqual(res.ok, true, res.error); + assert.strictEqual(fs.readFileSync(path.join(dir, 'src/new.php'), 'utf8'), 'unchanged\n'); + assert.strictEqual(fs.existsSync(path.join(dir, 'src/old.php')), false); +}); + +test('applyPatchToDir: reverting a deletion puts the file back (issue #11)', async (t) => { + const dir = await makeRepo(t, { 'src/old.php': 'one\ntwo\n' }); + const deletePatch = `diff --git a/src/old.php b/src/old.php +deleted file mode 100644 +--- a/src/old.php ++++ /dev/null +@@ -1,2 +0,0 @@ +-one +-two +`; + assert.strictEqual((await applyPatchToDir({ dir, patchText: deletePatch })).ok, true); + assert.strictEqual(fs.existsSync(path.join(dir, 'src/old.php')), false); + + const back = await applyPatchToDir({ dir, patchText: deletePatch, reverse: true }); + assert.strictEqual(back.ok, true, back.error); + assert.strictEqual(fs.readFileSync(path.join(dir, 'src/old.php'), 'utf8'), 'one\ntwo\n'); +}); + +// wordpress-develop carries fixtures whose line endings are the thing under +// test; rewriting them to LF because a patch touched the file would corrupt +// exactly those. +test('applyPatchToDir: a CRLF file keeps CRLF after patching (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY.replace(/\n/g, '\r\n') }); + const res = await applyPatchToDir({ dir, patchText: FOO_PATCH }); + assert.strictEqual(res.ok, true, res.error); + assert.strictEqual(fs.readFileSync(path.join(dir, FOO), 'utf8'), 'one\r\nTWO\r\nthree\r\n'); +}); + +test('dominantEol: reports the ending a file actually uses (issue #11)', () => { + assert.strictEqual(dominantEol('a\nb\n'), '\n'); + assert.strictEqual(dominantEol('a\r\nb\r\n'), '\r\n'); + assert.strictEqual(dominantEol(''), '\n'); + // A mostly-LF file with one stray CRLF stays LF. + assert.strictEqual(dominantEol('a\nb\nc\r\nd\ne\n'), '\n'); +}); + +// resolveInside is exported so both the lexical and the symlink case can be +// exercised from one machine, the way win-spawn-patch.test.cjs does. +test('resolveInside: refuses paths that climb out, allows ones that stay in (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + assert.notStrictEqual(resolveInside(dir, 'src/wp-includes/foo.php'), null); + assert.notStrictEqual(resolveInside(dir, 'src/does/not/exist/yet.php'), null); + assert.strictEqual(resolveInside(dir, '../escaped.txt'), null); + assert.strictEqual(resolveInside(dir, 'src/../../escaped.txt'), null); + assert.strictEqual(resolveInside(dir, path.resolve(os.tmpdir(), 'absolute.txt')), null); +}); + +// path.resolve normalises ".." but not symlinks, so a lexical-only check lets a +// patch write through a symlinked directory to anywhere on disk. +test('resolveInside: refuses a path leading through a symlink out of the tree (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'patch-apply-outside-')); + t.after(() => fs.rmSync(outside, { recursive: true, force: true })); + fs.symlinkSync(outside, path.join(dir, 'escape-hatch'), 'dir'); + + assert.strictEqual(resolveInside(dir, 'escape-hatch/evil.txt'), null); +}); + +test('applyPatchToDir: a patch through a symlinked directory is refused (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'patch-apply-outside-')); + t.after(() => fs.rmSync(outside, { recursive: true, force: true })); + fs.symlinkSync(outside, path.join(dir, 'escape-hatch'), 'dir'); + + const evil = `diff --git a/escape-hatch/evil.txt b/escape-hatch/evil.txt +new file mode 100644 +--- /dev/null ++++ b/escape-hatch/evil.txt +@@ -0,0 +1 @@ ++pwned +`; + const res = await applyPatchToDir({ dir, patchText: evil }); + assert.strictEqual(res.ok, false); + assert.match(res.error, /outside the site folder/); + assert.strictEqual(fs.existsSync(path.join(outside, 'evil.txt')), false); +}); diff --git a/test/patch-plan.test.cjs b/test/patch-plan.test.cjs new file mode 100644 index 0000000..d425110 --- /dev/null +++ b/test/patch-plan.test.cjs @@ -0,0 +1,258 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert'); +const { + stripPathPrefix, + mapToSrcLayout, + parsePatchFiles, + planApply +} = require('../src/patch-plan.cjs'); +const { updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, APPLY_STATE_TO_STEP } = require('../src/renderer/update-plan.cjs'); + +// The four header shapes that actually reach the app. Kept verbatim rather than +// generated: the whole point of these tests is that real-world formatting — +// tabs before "(revision N)", missing a/ b/ prefixes, /dev/null sides — is +// handled, and a generator would only produce the shape we already expect. +const GITHUB_DIFF = `diff --git a/src/wp-includes/foo.php b/src/wp-includes/foo.php +index 384bf9e..dbfa038 100644 +--- a/src/wp-includes/foo.php ++++ b/src/wp-includes/foo.php +@@ -1,3 +1,3 @@ + a +-b ++B + c +`; + +const TRAC_SVN_DIFF = `Index: src/wp-includes/foo.php +=================================================================== +--- src/wp-includes/foo.php\t(revision 59234) ++++ src/wp-includes/foo.php\t(working copy) +@@ -1,3 +1,3 @@ + a +-b ++B + c +`; + +const OLD_LAYOUT_DIFF = `Index: wp-admin/admin.php +=================================================================== +--- wp-admin/admin.php\t(revision 1) ++++ wp-admin/admin.php\t(working copy) +@@ -1,2 +1,2 @@ + x +-y ++Y +`; + +const ADD_DIFF = `diff --git a/src/new.php b/src/new.php +new file mode 100644 +index 0000000..e69de29 +--- /dev/null ++++ b/src/new.php +@@ -0,0 +1,2 @@ ++one ++two +`; + +const DELETE_DIFF = `diff --git a/src/old.php b/src/old.php +deleted file mode 100644 +--- a/src/old.php ++++ /dev/null +@@ -1,2 +0,0 @@ +-one +-two +`; + +const BINARY_DIFF = `diff --git a/src/x.png b/src/x.png +index 111..222 100644 +Binary files a/src/x.png and b/src/x.png differ +`; + +test('parsePatchFiles: a GitHub pull request diff resolves to a repo-relative path (issue #11)', () => { + const res = parsePatchFiles(GITHUB_DIFF); + assert.strictEqual(res.ok, true); + assert.strictEqual(res.files.length, 1); + assert.strictEqual(res.files[0].path, 'src/wp-includes/foo.php'); + assert.strictEqual(res.files[0].kind, 'modify'); +}); + +// The bug this guards: Subversion patches carry no a/ b/ prefix, so stripping +// two characters unconditionally would write to "c/wp-includes/foo.php". +test('parsePatchFiles: a Subversion-style Trac attachment keeps its full path (issue #11)', () => { + const res = parsePatchFiles(TRAC_SVN_DIFF); + assert.strictEqual(res.ok, true); + assert.strictEqual(res.files[0].path, 'src/wp-includes/foo.php'); +}); + +test('parsePatchFiles: both formats agree on the same target path (issue #11)', () => { + assert.strictEqual( + parsePatchFiles(GITHUB_DIFF).files[0].path, + parsePatchFiles(TRAC_SVN_DIFF).files[0].path + ); +}); + +test('parsePatchFiles: a patch against the pre-src layout is remapped (issue #11)', () => { + const res = parsePatchFiles(OLD_LAYOUT_DIFF); + assert.strictEqual(res.files[0].path, 'src/wp-admin/admin.php'); +}); + +test('parsePatchFiles: added and deleted files are classified, not treated as renames (issue #11)', () => { + const added = parsePatchFiles(ADD_DIFF).files[0]; + assert.strictEqual(added.kind, 'add'); + assert.strictEqual(added.path, 'src/new.php'); + + const deleted = parsePatchFiles(DELETE_DIFF).files[0]; + assert.strictEqual(deleted.kind, 'delete'); + // A deletion's target is the file that exists today, not /dev/null. + assert.strictEqual(deleted.path, 'src/old.php'); +}); + +// jsdiff represents a binary file as an entry with no hunks. Left unchecked +// that reads as "a file with no changes", so applying would report success +// while silently skipping it. +test('parsePatchFiles: a binary file is reported as binary rather than an empty change (issue #11)', () => { + const res = parsePatchFiles(BINARY_DIFF); + assert.strictEqual(res.ok, true); + assert.strictEqual(res.files[0].kind, 'binary'); +}); + +test('parsePatchFiles: empty and unreadable input is rejected with a reason (issue #11)', () => { + for (const empty of ['', ' ', null, undefined]) { + const res = parsePatchFiles(empty); + assert.strictEqual(res.ok, false); + assert.strictEqual(res.error, 'The patch is empty.'); + } + const notAPatch = parsePatchFiles('this is just prose, not a diff at all\n'); + assert.strictEqual(notAPatch.ok, false); +}); + +test('parsePatchFiles: CRLF line endings parse the same as LF (issue #11)', () => { + const crlf = GITHUB_DIFF.replace(/\n/g, '\r\n'); + const res = parsePatchFiles(crlf); + assert.strictEqual(res.ok, true); + assert.strictEqual(res.files[0].path, 'src/wp-includes/foo.php'); +}); + +test('parsePatchFiles: a multi-file patch yields one entry per file (issue #11)', () => { + const res = parsePatchFiles(GITHUB_DIFF + ADD_DIFF + DELETE_DIFF); + assert.strictEqual(res.ok, true); + assert.deepStrictEqual(res.files.map((f) => f.kind), ['modify', 'add', 'delete']); +}); + +test('stripPathPrefix: only strips when both sides are prefixed (issue #11)', () => { + assert.deepStrictEqual( + stripPathPrefix('a/src/foo.php', 'b/src/foo.php'), + { oldPath: 'src/foo.php', newPath: 'src/foo.php' } + ); + assert.deepStrictEqual( + stripPathPrefix('src/foo.php', 'src/foo.php'), + { oldPath: 'src/foo.php', newPath: 'src/foo.php' } + ); + // /dev/null counts as agreement — an added file has only one real side. + assert.deepStrictEqual( + stripPathPrefix('/dev/null', 'b/src/new.php'), + { oldPath: '/dev/null', newPath: 'src/new.php' } + ); +}); + +test('stripPathPrefix: an old Subversion trunk/ prefix is dropped (issue #11)', () => { + assert.deepStrictEqual( + stripPathPrefix('trunk/wp-admin/admin.php', 'trunk/wp-admin/admin.php'), + { oldPath: 'wp-admin/admin.php', newPath: 'wp-admin/admin.php' } + ); +}); + +test('mapToSrcLayout: modern paths are left alone (issue #11)', () => { + for (const p of ['src/wp-includes/foo.php', 'tests/phpunit/bar.php', 'tools/baz.js']) { + assert.strictEqual(mapToSrcLayout(p), p); + } +}); + +test('mapToSrcLayout: root-level build files stay at the root (issue #11)', () => { + for (const p of ['package.json', 'Gruntfile.js', '.editorconfig', 'wp-cli.yml', 'wp-tests-config-sample.php']) { + assert.strictEqual(mapToSrcLayout(p), p); + } +}); + +// wp-cli.yml and wp-config-sample.php both start with "wp-" but did not move. +// This is the case the bare wp-* rule gets wrong without the exception list. +test('mapToSrcLayout: wp-prefixed files move to src, except the ones that did not (issue #11)', () => { + assert.strictEqual(mapToSrcLayout('wp-admin/admin.php'), 'src/wp-admin/admin.php'); + assert.strictEqual(mapToSrcLayout('wp-includes/post.php'), 'src/wp-includes/post.php'); + assert.strictEqual(mapToSrcLayout('wp-cli.yml'), 'wp-cli.yml'); + assert.strictEqual(mapToSrcLayout('wp-config-sample.php'), 'wp-config-sample.php'); +}); + +test('mapToSrcLayout: the loose root files that did move are remapped (issue #11)', () => { + assert.strictEqual(mapToSrcLayout('index.php'), 'src/index.php'); + assert.strictEqual(mapToSrcLayout('xmlrpc.php'), 'src/xmlrpc.php'); + assert.strictEqual(mapToSrcLayout('license.txt'), 'src/license.txt'); +}); + +test('mapToSrcLayout: an unrecognised path is left alone rather than guessed (issue #11)', () => { + assert.strictEqual(mapToSrcLayout('some/other/thing.php'), 'some/other/thing.php'); +}); + +test('planApply: only files the contributor already edited count as conflicts (issue #11)', () => { + const { files } = parsePatchFiles(GITHUB_DIFF + ADD_DIFF); + const plan = planApply({ files, dirtyPaths: ['src/wp-includes/foo.php', 'src/unrelated.php'] }); + assert.deepStrictEqual(plan.paths, ['src/wp-includes/foo.php', 'src/new.php']); + assert.deepStrictEqual(plan.conflicts, ['src/wp-includes/foo.php']); +}); + +test('planApply: a clean tree has no conflicts (issue #11)', () => { + const { files } = parsePatchFiles(GITHUB_DIFF); + assert.deepStrictEqual(planApply({ files, dirtyPaths: [] }).conflicts, []); +}); + +test('planApply: binary files are listed as unsupported (issue #11)', () => { + const { files } = parsePatchFiles(GITHUB_DIFF + BINARY_DIFF); + assert.deepStrictEqual(planApply({ files }).unsupported, ['src/x.png']); +}); + +test('planApply: an install is needed only when the lockfile is touched (issue #11)', () => { + assert.strictEqual(planApply({ files: parsePatchFiles(GITHUB_DIFF).files }).needsInstall, false); + + const lockDiff = `diff --git a/package-lock.json b/package-lock.json +--- a/package-lock.json ++++ b/package-lock.json +@@ -1,3 +1,3 @@ + { +- "x": 1 ++ "x": 2 + } +`; + assert.strictEqual(planApply({ files: parsePatchFiles(lockDiff).files }).needsInstall, true); +}); + +test('planApplySteps: the install step is named even when skipped (issue #11)', () => { + const steps = planApplySteps({ needsInstall: false }); + assert.deepStrictEqual(steps.map((s) => s.key), ['apply', 'install', 'build']); + assert.strictEqual(steps[1].skipped, true); + assert.strictEqual(steps[1].skipMessage, SKIP_INSTALL_MESSAGE); + assert.strictEqual(planApplySteps({ needsInstall: true })[1].skipped, false); +}); + +// The apply chain reuses the update chain's renderer helper by passing its own +// state map; this is what proves the third parameter actually drives it. +test('planApplySteps: updateStepStatuses drives the apply chain too (issue #11)', () => { + const steps = planApplySteps({ needsInstall: false }); + const at = (state) => updateStepStatuses(steps, state, APPLY_STATE_TO_STEP).map((s) => s.status); + + assert.deepStrictEqual(at('applying'), ['current', 'pending', 'pending']); + assert.deepStrictEqual(at('building'), ['complete', 'skipped', 'current']); + assert.deepStrictEqual(at('done'), ['complete', 'skipped', 'complete']); + // An unknown state must not mark anything complete. + assert.deepStrictEqual(at('idle'), ['pending', 'pending', 'pending']); +}); + +test('planApplySteps: the update chain is unaffected by the new state map (issue #11)', () => { + const steps = planApplySteps({ needsInstall: true }); + // 'fetching' belongs to the update chain, not this one. + assert.deepStrictEqual( + updateStepStatuses(steps, 'fetching', APPLY_STATE_TO_STEP).map((s) => s.status), + ['pending', 'pending', 'pending'] + ); +}); From eb64100d902f741cbb2646b4ff89b0376e342864 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 09:40:08 +0200 Subject: [PATCH 2/5] Make patch apply/rollback fully all-or-nothing (Copilot #135 review) 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 --- src/patch-apply.js | 85 +++++++++++++----- test/patch-apply.integration.test.cjs | 122 +++++++++++++++++++++++++- 2 files changed, 184 insertions(+), 23 deletions(-) diff --git a/src/patch-apply.js b/src/patch-apply.js index 2c62f97..814dddc 100644 --- a/src/patch-apply.js +++ b/src/patch-apply.js @@ -44,18 +44,26 @@ function resolveInside(dir, relPath) { try { root = fs.realpathSync(path.resolve(dir)); } catch { root = path.resolve(dir); } const lexical = path.resolve(root, relPath); - // Walk up to the nearest existing ancestor, resolve that for real, then put - // the not-yet-existing remainder back on. + // Walk up to the nearest existing entry, resolve that for real, then put the + // not-yet-existing remainder back on. lstat, not existsSync: existsSync + // follows links and reports a dangling symlink as absent, so the walk would + // step past a symlink pointing outside the checkout and hand back its lexical + // path — which the writer would then follow out of the tree. let existing = lexical; const trailing = []; - while (!fs.existsSync(existing)) { + const entryExists = (p) => { try { fs.lstatSync(p); return true; } catch { return false; } }; + while (!entryExists(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; } + // The deepest existing entry is realpath-ed for real. A symlink here that + // dangles (realpath throws) or resolves outside root is an escape, not a + // path to write through — fail closed rather than falling back to lexical. + let realExisting; + try { realExisting = fs.realpathSync(existing); } catch { return null; } + const abs = path.join(realExisting, ...trailing); if (abs !== root && !abs.startsWith(root + path.sep)) return null; return abs; @@ -117,11 +125,17 @@ function resolveFile(dir, file) { const target = resolveInside(dir, file.path); if (!target) return { error: `${file.path} points outside the site folder` }; - const readIfPresent = (abs) => (fs.existsSync(abs) ? fs.readFileSync(abs) : null); - 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) }; + const previous = fs.readFileSync(target); + // Validate the file still matches what the patch expects to remove, so an + // edit made after the preview fails all-or-nothing rather than being + // silently deleted with the contributor's changes in it. + if (file.hunks && file.hunks.length + && JsDiff.applyPatch(normalizeEol(previous.toString('utf8')), file.patch) === false) { + return { error: `${file.path} has moved on since the patch was written, so it no longer applies` }; + } + return { op: 'delete', abs: target, path: file.path, previous }; } if (file.kind === 'add') { @@ -136,10 +150,14 @@ function resolveFile(dir, file) { if (!source) return { error: `${file.oldPath} points outside the site folder` }; if (!fs.existsSync(source)) return { error: `${file.oldPath} is not in this checkout, so the patch cannot move it` }; if (fs.existsSync(target)) return { error: `${file.newPath} already exists, so the patch cannot move ${file.oldPath} onto it` }; - const original = fs.readFileSync(source, 'utf8'); - // A 100%-similarity rename has no hunks: the content moves unchanged. - let content = original; + const originalBuf = fs.readFileSync(source); + // A 100%-similarity rename has no hunks: the bytes move unchanged, so they + // are carried as a Buffer. Git emits binary renames with no binary marker, + // so this path is reachable for them — decoding through utf8 would corrupt + // the file. Decode to text only when hunks actually need applying. + let content = originalBuf; if (file.hunks.length) { + const original = originalBuf.toString('utf8'); 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` }; @@ -148,7 +166,7 @@ function resolveFile(dir, file) { } return { op: 'rename', abs: target, from: source, path: file.path, content, - previous: null, previousFrom: readIfPresent(source) + previous: null, previousFrom: originalBuf }; } @@ -169,28 +187,43 @@ function resolveFile(dir, file) { /** * Puts back everything a failed run had already written. * + * Returns the paths it could not restore. The same full-disk, lock, or + * permission condition that broke a write can also break its undo, and the + * caller must not claim a clean restore when the tree is actually unknown. + * * @param {Array} done + * @return {Array} paths whose rollback failed (empty when fully restored) */ function rollback(done) { + const errors = []; + // Removing something that was never created — already gone, or its parent is + // not even a directory — is the desired end state, not a failure. Actions are + // registered before they mutate (so a half-done one is still undoable), which + // means rollback can see ones that never ran; only a content restoration that + // cannot be written back is a real, unrecoverable loss. + const removeQuietly = (target) => { + try { fs.rmSync(target, { force: true }); } + catch (e) { if (!e || (e.code !== 'ENOTDIR' && e.code !== 'ENOENT')) throw e; } + }; for (const action of done.reverse()) { try { if (action.op === 'rename' && action.previousFrom !== null) { fs.mkdirSync(path.dirname(action.from), { recursive: true }); fs.writeFileSync(action.from, action.previousFrom); - fs.rmSync(action.abs, { force: true }); + removeQuietly(action.abs); continue; } if (action.previous === null) { - fs.rmSync(action.abs, { force: true }); + removeQuietly(action.abs); continue; } fs.mkdirSync(path.dirname(action.abs), { recursive: true }); fs.writeFileSync(action.abs, action.previous); - } catch { - // Best effort: the caller is already reporting a failure, and a - // rollback that cannot run must not mask the original cause. + } catch (e) { + errors.push(`${action.path}: ${String(e && e.message ? e.message : e)}`); } } + return errors; } /** @@ -246,21 +279,29 @@ async function applyPatchToDir({ dir, patchText, reverse = false, onLog = () => const done = []; try { for (const action of actions) { + // Registered before its mutations, not after: a rename that writes its + // destination and then throws removing the source would otherwise be + // invisible to rollback and leave a partial patch behind. Undoing an + // action whose mutations had not started yet is harmless. + done.push(action); 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.writeFileSync(action.abs, action.content); fs.rmSync(action.from, { force: true }); } else { fs.mkdirSync(path.dirname(action.abs), { recursive: true }); - fs.writeFileSync(action.abs, action.content, 'utf8'); + fs.writeFileSync(action.abs, action.content); } - done.push(action); } } catch (e) { - rollback(done); + const recovery = rollback(done); const message = `writing ${String(e && e.message ? e.message : e)}`; + if (recovery.length) { + onLog(`\nThe patch could not be written, and the checkout could not be fully put back — it is in an unknown state. Could not undo: ${recovery.join('; ')}\n`); + return { ok: false, error: message, applied: [], skipped, rolledBack: false, recovery }; + } onLog(`\nThe patch could not be written, so the checkout was put back as it was: ${message}\n`); return { ok: false, error: message, applied: [], skipped, rolledBack: true }; } @@ -278,4 +319,4 @@ async function applyPatchToDir({ dir, patchText, reverse = false, onLog = () => return { ok: true, applied, skipped }; } -module.exports = { applyPatchToDir, resolveInside, reverseFile, dominantEol }; +module.exports = { applyPatchToDir, resolveInside, reverseFile, dominantEol, rollback }; diff --git a/test/patch-apply.integration.test.cjs b/test/patch-apply.integration.test.cjs index 333b0c1..6ec5799 100644 --- a/test/patch-apply.integration.test.cjs +++ b/test/patch-apply.integration.test.cjs @@ -6,7 +6,7 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); const git = require('isomorphic-git'); -const { applyPatchToDir, resolveInside, dominantEol } = require('../src/patch-apply'); +const { applyPatchToDir, resolveInside, dominantEol, rollback } = require('../src/patch-apply'); // A real on-disk repo, like trunk-update.integration.test.cjs: applyPatchToDir // calls ensureAutocrlf, which reads and writes git config, so a bare temp @@ -317,3 +317,123 @@ new file mode 100644 assert.match(res.error, /outside the site folder/); assert.strictEqual(fs.existsSync(path.join(outside, 'evil.txt')), false); }); + +// existsSync follows a symlink and is false for a dangling one, so a naive +// walk-up steps past a link pointing outside the checkout and hands back its +// lexical path — which writeFileSync would then follow out of the tree. lstat +// closes that hole. (Copilot #1.) +test('applyPatchToDir: an add through a dangling symlink out of the tree is refused (issue #11)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'patch-apply-dangling-')); + t.after(() => fs.rmSync(outsideDir, { recursive: true, force: true })); + const outside = path.join(outsideDir, 'target.txt'); // never created → dangling + fs.symlinkSync(outside, path.join(dir, 'sneaky.txt')); + + const evil = `diff --git a/sneaky.txt b/sneaky.txt +new file mode 100644 +--- /dev/null ++++ b/sneaky.txt +@@ -0,0 +1 @@ ++pwned +`; + const res = await applyPatchToDir({ dir, patchText: evil }); + assert.strictEqual(res.ok, false); + assert.match(res.error, /outside the site folder/); + assert.strictEqual(fs.existsSync(outside), false, 'nothing may be written through the link'); +}); + +// A pure (100%-similarity) rename carries the bytes unchanged. Git emits binary +// renames with no binary marker, so reading the source as utf8 and writing the +// string back would corrupt it — the bytes must survive intact. (Copilot #5.) +test('applyPatchToDir: a pure rename preserves non-utf8 (binary) bytes (issue #11)', async (t) => { + const bytes = Buffer.from([0xff, 0xfe, 0x00, 0x01, 0x80, 0x0a]); + const dir = await makeRepo(t, { 'src/logo.bin': bytes }); + const purePatch = `diff --git a/src/logo.bin b/src/moved.bin +similarity index 100% +rename from src/logo.bin +rename to src/moved.bin +`; + const res = await applyPatchToDir({ dir, patchText: purePatch }); + assert.strictEqual(res.ok, true, res.error); + assert.ok(fs.readFileSync(path.join(dir, 'src/moved.bin')).equals(bytes), 'bytes must survive the rename'); + assert.strictEqual(fs.existsSync(path.join(dir, 'src/logo.bin')), false); +}); + +// A deletion with a pre-image must match what is on disk. If the contributor +// edited the file after previewing, deleting it anyway silently discards their +// work; all-or-nothing means failing instead. (Copilot #2.) +test('applyPatchToDir: deleting a file edited since the patch fails all-or-nothing (issue #11)', async (t) => { + const dir = await makeRepo(t, { 'src/old.php': 'one\ntwo\n' }); + const deletePatch = `diff --git a/src/old.php b/src/old.php +deleted file mode 100644 +--- a/src/old.php ++++ /dev/null +@@ -1,2 +0,0 @@ +-one +-two +`; + fs.writeFileSync(path.join(dir, 'src/old.php'), 'my own work\n'); + const before = snapshot(dir); + + const res = await applyPatchToDir({ dir, patchText: deletePatch }); + + assert.strictEqual(res.ok, false); + assert.match(res.error, /moved on since the patch was written/); + assert.deepStrictEqual(snapshot(dir), before, 'the edited file must not be deleted'); +}); + +// A rename that completes and is then undone by a later failure must restore the +// source and remove the destination — registering each action before its +// mutations is what lets rollback see a half-done one. (Copilot #3.) +test('applyPatchToDir: a later failure rolls a completed rename fully back (issue #11)', async (t) => { + const dir = await makeRepo(t, { 'src/old.php': 'one\ntwo\n', 'src/blocker': 'not a directory\n' }); + const before = snapshot(dir); + const renameThenBlocked = `diff --git a/src/old.php b/src/new.php +similarity index 100% +rename from src/old.php +rename to src/new.php +diff --git a/src/blocker/child.php b/src/blocker/child.php +new file mode 100644 +--- /dev/null ++++ b/src/blocker/child.php +@@ -0,0 +1 @@ ++hello +`; + const res = await applyPatchToDir({ dir, patchText: renameThenBlocked }); + + assert.strictEqual(res.ok, false); + assert.strictEqual(res.rolledBack, true); + assert.deepStrictEqual(snapshot(dir), before, 'the rename must be fully undone'); +}); + +// A rollback can hit the same fault that broke the write. rollback must report +// what it could not restore so the caller stops claiming a clean tree. Driven +// directly with an un-restorable action (its parent is a file → ENOTDIR), which +// fails the same way whether or not the tests run as root. (Copilot #4.) +test('rollback: reports the paths it could not restore instead of swallowing them (issue #11)', async (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'patch-apply-rollback-')); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + fs.writeFileSync(path.join(dir, 'afile'), 'i am a file\n'); + + // Restoring this action means writing under `afile`, which is a file, not a + // directory — mkdirSync/writeFileSync throw ENOTDIR. + const recovery = rollback([ + { op: 'write', abs: path.join(dir, 'afile', 'child'), path: 'afile/child', previous: Buffer.from('x') } + ]); + + assert.ok(Array.isArray(recovery) && recovery.length === 1); + assert.match(recovery[0], /afile\/child/); +}); + +// The clean path still returns no recovery errors, so the caller reports a real +// rollback as one. +test('rollback: returns an empty list when it restores everything (issue #11)', async (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'patch-apply-rollback-ok-')); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + fs.writeFileSync(path.join(dir, 'added'), 'new\n'); + + const recovery = rollback([{ op: 'write', abs: path.join(dir, 'added'), path: 'added', previous: null }]); + + assert.deepStrictEqual(recovery, []); + assert.strictEqual(fs.existsSync(path.join(dir, 'added')), false, 'an added file is removed on rollback'); +}); From d0b22d4a362118e15b2ae9ee530daa76d49f76a3 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 09:46:55 +0200 Subject: [PATCH 3/5] Guard and test the patch IPC handlers (Copilot #135 review) - 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 --- src/main.js | 53 ++++++++++++++++++---- test/ipc-wiring.test.cjs | 97 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 136 insertions(+), 14 deletions(-) diff --git a/src/main.js b/src/main.js index 7bf14c1..c7e0455 100644 --- a/src/main.js +++ b/src/main.js @@ -423,6 +423,11 @@ ipcMain.handle('git:worktree-dirty', async (_e, sitePath) => { ipcMain.handle('git:discard-changes', async (_e, sitePath) => { try { await discardChanges(sitePath); + // Clearing the applied-patch record belongs with the reset that removed + // the patch from the tree — not with the trunk update that may follow and + // fail on the network, which would leave a revert banner for a patch that + // is already gone. + await mergeSiteMeta(sitePath, { appliedPatch: null }); return { ok: true }; } catch (e) { return { ok: false, error: String(e) }; @@ -488,8 +493,15 @@ ipcMain.handle('git:preview-patch', async (_e, sitePath, patchText) => { try { const parsed = parsePatchFiles(patchText); if (!parsed.ok) return { ok: false, error: parsed.error }; - let dirtyPaths = []; - try { dirtyPaths = await collectDirtyFiles(sitePath); } catch {} + let dirtyPaths; + try { + dirtyPaths = await collectDirtyFiles(sitePath); + } catch (e) { + // Failing open here would promise "no collisions" precisely when the + // app could not look — surface the failure instead. + logError('git:preview-patch', String(e && e.stack ? e.stack : e)); + return { ok: false, error: 'Could not check your working tree for conflicts, so the preview was not shown.' }; + } const plan = planApply({ files: parsed.files, dirtyPaths }); return { ok: true, ...plan, files: parsed.files.map((f) => ({ kind: f.kind, path: f.path })) }; } catch (e) { @@ -534,6 +546,13 @@ ipcMain.handle('git:apply-patch', async (event, sitePath, options = {}) => { let patchText = String(options.patchText || ''); let label = String(options.label || 'patch'); const s = await getStore(); + // sitePath crosses IPC untrusted and becomes the root for patch + // writes, so it has to be a site the app actually manages — the same + // gate sites:set-ticket applies before it touches metadata. + if (!(s.get('sites') || []).includes(sitePath)) { + sendDone({ ok: false, error: 'Site is not registered' }); + return; + } const stored = ((s.get('siteMeta') || {})[sitePath] || {}).appliedPatch; if (reverse) { if (!stored || !stored.text) { @@ -563,14 +582,30 @@ ipcMain.handle('git:apply-patch', async (event, sitePath, options = {}) => { if (!revertable) { sendLog('This patch is too large to keep for an undo, so Revert will not be offered.\n'); } - await mergeSiteMeta(sitePath, { - appliedPatch: { - label, - appliedAt: new Date().toISOString(), - files: result.applied, - text: revertable ? patchText : null + try { + await mergeSiteMeta(sitePath, { + appliedPatch: { + label, + appliedAt: new Date().toISOString(), + files: result.applied, + text: revertable ? patchText : null + } + }); + } catch (persistErr) { + // Persistence is part of the transaction: the patch is on disk + // but its revert record could not be saved, so undo the apply + // rather than leave a patch the app cannot revert. If the undo + // also fails, say so plainly instead of reporting a clean fail. + logError('git:apply-patch', `persist failed, undoing apply: ${String(persistErr && persistErr.stack ? persistErr.stack : persistErr)}`); + const undo = await applyPatchToDir({ dir: sitePath, patchText, reverse: true, onLog: sendLog }); + const why = String(persistErr && persistErr.message ? persistErr.message : persistErr); + if (undo.ok) { + sendDone({ ok: false, error: `The patch applied but its revert record could not be saved, so it was undone. ${why}` }); + } else { + sendDone({ ok: false, appliedButUntracked: true, files: result.applied, error: `The patch applied but its revert record could not be saved and it could not be undone — the checkout has the patch and the app cannot revert it. ${why}` }); } - }); + return; + } } sendDone({ ok: true, ...result, reverse }); } catch (e) { diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index a44d088..a62804e 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -444,12 +444,16 @@ test('git:worktree-dirty reports what trunk-update found, not its own guess', as }); }); -test('git:discard-changes goes through trunk-update', async () => { +test('git:discard-changes resets through trunk-update and clears the applied-patch record', async () => { const discardChanges = spy(async () => {}); - const main = loadMain({ stubs: { ...silentLogging(), './trunk-update': { discardChanges } } }); + const settings = fakeSettingsStore({ sites: ['/sites/wp'], siteMeta: { '/sites/wp': { appliedPatch: { label: 'x' } } } }); + const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs, './trunk-update': { discardChanges } } }); assert.deepEqual(await main.invoke('git:discard-changes', '/sites/wp'), { ok: true }); assert.deepEqual(discardChanges.calls, [['/sites/wp']]); + // The record is cleared with the reset, not left for a later trunk update to + // clear — otherwise a failed update leaves a revert banner for a gone patch. + assert.equal(settings.values.siteMeta['/sites/wp'].appliedPatch, null); }); test('git:update-trunk hands the update to trunk-update and streams its log back', async () => { @@ -964,6 +968,89 @@ test('git:preview-patch reads the patch through patch-plan', async () => { assert.deepEqual(result, { ok: false, error: 'unreadable' }); }); +// git:apply-patch reads the store for its guard before delegating, which is the +// seam fakeSettingsStore stands in for — so it is a wired handler, not a hole. +// It streams, so its result comes back on the :done channel, not the return. +async function applyDone(event, applyId, cap = 50) { + for (let i = 0; i < cap; i++) { + const hit = event.sent.find((m) => m.channel === 'git:apply-patch:done' && m.payload.applyId === applyId); + if (hit) return hit.payload; + await new Promise((r) => setImmediate(r)); + } + throw new Error('git:apply-patch never reported done'); +} + +test('git:apply-patch refuses an unregistered site path before touching patch-apply', async () => { + const applyPatchToDir = spy(async () => ({ ok: true, applied: [], skipped: [] })); + const settings = fakeSettingsStore({ sites: ['/sites/wp'] }); + const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs, './patch-apply': { applyPatchToDir } } }); + + const event = createIpcEvent(); + const { applyId } = await main.invokeWith('git:apply-patch', event, '/sites/unknown', { patchText: 'P' }); + + assert.deepEqual(await applyDone(event, applyId), { applyId, ok: false, error: 'Site is not registered' }); + assert.deepEqual(applyPatchToDir.calls, []); +}); + +test('git:apply-patch delegates a forward apply to patch-apply and records it', async () => { + const applyPatchToDir = spy(async () => ({ ok: true, applied: ['src/a.php'], skipped: [] })); + const settings = fakeSettingsStore({ sites: ['/sites/wp'] }); + const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs, './patch-apply': { applyPatchToDir } } }); + + const event = createIpcEvent(); + const { applyId } = await main.invokeWith('git:apply-patch', event, '/sites/wp', { patchText: 'PATCH', label: 'PR 11705' }); + const done = await applyDone(event, applyId); + + assert.equal(done.ok, true); + assert.equal(applyPatchToDir.calls.length, 1); + const [args] = applyPatchToDir.calls[0]; + assert.equal(args.dir, '/sites/wp'); + assert.equal(args.patchText, 'PATCH'); + assert.equal(args.reverse, false); + // The revert record is what makes Revert possible; without it the patch is + // applied but silently unrevertable. + const stored = settings.values.siteMeta['/sites/wp'].appliedPatch; + assert.equal(stored.label, 'PR 11705'); + assert.equal(stored.text, 'PATCH'); + assert.deepEqual(stored.files, ['src/a.php']); +}); + +test('git:apply-patch refuses a second patch while one is already applied', async () => { + const applyPatchToDir = spy(async () => ({ ok: true, applied: [], skipped: [] })); + const settings = fakeSettingsStore({ + sites: ['/sites/wp'], + siteMeta: { '/sites/wp': { appliedPatch: { label: 'first', text: 'X' } } } + }); + const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs, './patch-apply': { applyPatchToDir } } }); + + const event = createIpcEvent(); + const { applyId } = await main.invokeWith('git:apply-patch', event, '/sites/wp', { patchText: 'SECOND' }); + const done = await applyDone(event, applyId); + + assert.equal(done.ok, false); + assert.match(done.error, /already applied/); + assert.deepEqual(applyPatchToDir.calls, []); +}); + +test('git:apply-patch reverts using the stored patch text and clears the record', async () => { + const applyPatchToDir = spy(async () => ({ ok: true, applied: ['src/a.php'], skipped: [] })); + const settings = fakeSettingsStore({ + sites: ['/sites/wp'], + siteMeta: { '/sites/wp': { appliedPatch: { label: 'L', text: 'STORED' } } } + }); + const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs, './patch-apply': { applyPatchToDir } } }); + + const event = createIpcEvent(); + const { applyId } = await main.invokeWith('git:apply-patch', event, '/sites/wp', { reverse: true }); + const done = await applyDone(event, applyId); + + assert.equal(done.ok, true); + const [args] = applyPatchToDir.calls[0]; + assert.equal(args.reverse, true); + assert.equal(args.patchText, 'STORED', 'a revert applies the patch the app stored, not the renderer'); + assert.equal(settings.values.siteMeta['/sites/wp'].appliedPatch, null); +}); + // --- the harness's own guard --------------------------------------------- // Requiring the real `electron` package is not a harmless fallback: its @@ -1013,7 +1100,8 @@ const WIRED = new Set([ 'playground-web:start', 'playground-web:stop', 'sites:set-ticket', - 'git:preview-patch' + 'git:preview-patch', + 'git:apply-patch' ]); // Channels with no module to reach: they read or write electron-store, drive a @@ -1051,8 +1139,7 @@ const NO_DELEGATION = new Map([ // Channels that do delegate, but whose call sits behind something this harness // cannot stand in for yet. Each one is a known hole, not an oversight. const NOT_REACHABLE = new Map([ - ['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'] + ['wordpress:setup', 'calls ensureAutocrlf and readTrunkInfo only after cloning wordpress-develop over the network'] ]); const CLASSIFIED = [...WIRED, ...NO_DELEGATION.keys(), ...NOT_REACHABLE.keys()]; From 823720fb3395fb0b8e4ac04c08ae7fa9d4034af8 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 09:48:04 +0200 Subject: [PATCH 4/5] Rebuild deps when a patch renames the lockfile away (Copilot #135 review) 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 --- src/patch-plan.cjs | 2 +- test/patch-plan.test.cjs | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/patch-plan.cjs b/src/patch-plan.cjs index 5578c7b..cf7d47e 100644 --- a/src/patch-plan.cjs +++ b/src/patch-plan.cjs @@ -233,7 +233,7 @@ function planApply({ files, dirtyPaths = [] } = {}) { unsupported: list.filter((f) => f.kind === 'binary').map((f) => f.path || '(unnamed binary file)'), // 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') + needsInstall: touched.has('package-lock.json') }; } diff --git a/test/patch-plan.test.cjs b/test/patch-plan.test.cjs index d425110..2cc01fe 100644 --- a/test/patch-plan.test.cjs +++ b/test/patch-plan.test.cjs @@ -227,6 +227,18 @@ test('planApply: an install is needed only when the lockfile is touched (issue # assert.strictEqual(planApply({ files: parsePatchFiles(lockDiff).files }).needsInstall, true); }); +// Renaming the lockfile away removes it just as much as editing it does, so a +// rebuild is still needed — the rename's source side has to count, not only its +// destination. (Copilot #12.) +test('planApply: renaming the lockfile away still needs an install (issue #11)', () => { + const renameAway = `diff --git a/package-lock.json b/package-lock.json.bak +similarity index 100% +rename from package-lock.json +rename to package-lock.json.bak +`; + assert.strictEqual(planApply({ files: parsePatchFiles(renameAway).files }).needsInstall, true); +}); + test('planApplySteps: the install step is named even when skipped (issue #11)', () => { const steps = planApplySteps({ needsInstall: false }); assert.deepStrictEqual(steps.map((s) => s.key), ['apply', 'install', 'build']); From 5ea49635755ef80f9e2bd608d60d688e522bd750 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 09:53:19 +0200 Subject: [PATCH 5/5] Test the undo-on-persist-failure path in git:apply-patch (self-review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- test/ipc-wiring.test.cjs | 51 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index a62804e..79edea9 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -1051,6 +1051,57 @@ test('git:apply-patch reverts using the stored patch text and clears the record' assert.equal(settings.values.siteMeta['/sites/wp'].appliedPatch, null); }); +// A store whose siteMeta write throws: the patch lands on disk but its revert +// record cannot be saved. Persistence is part of the transaction. +function storeThatFailsToPersist() { + return { + get: (key) => (key === 'sites' ? ['/sites/wp'] : {}), + set: (key) => { if (key === 'siteMeta') throw new Error('disk full'); } + }; +} + +test('git:apply-patch undoes the apply when its revert record cannot be saved', async () => { + const applyPatchToDir = spy(async ({ reverse }) => ({ ok: true, applied: reverse ? [] : ['src/a.php'], skipped: [] })); + const main = loadMain({ + stubs: { + ...silentLogging(), + './settings-store': { getStore: async () => storeThatFailsToPersist() }, + './patch-apply': { applyPatchToDir } + } + }); + + const event = createIpcEvent(); + const { applyId } = await main.invokeWith('git:apply-patch', event, '/sites/wp', { patchText: 'PATCH', label: 'L' }); + const done = await applyDone(event, applyId); + + assert.equal(done.ok, false); + assert.match(done.error, /could not be saved/); + // The forward apply, then the undo — the tree is put back to match what the + // renderer is told rather than left with an unrevertable patch. + assert.deepEqual(applyPatchToDir.calls.map((c) => c[0].reverse), [false, true]); +}); + +test('git:apply-patch reports applied-but-untracked when the undo also fails', async () => { + const applyPatchToDir = spy(async ({ reverse }) => + reverse ? { ok: false, error: 'cannot undo' } : { ok: true, applied: ['src/a.php'], skipped: [] }); + const main = loadMain({ + stubs: { + ...silentLogging(), + './settings-store': { getStore: async () => storeThatFailsToPersist() }, + './patch-apply': { applyPatchToDir } + } + }); + + const event = createIpcEvent(); + const { applyId } = await main.invokeWith('git:apply-patch', event, '/sites/wp', { patchText: 'PATCH', label: 'L' }); + const done = await applyDone(event, applyId); + + assert.equal(done.ok, false); + assert.equal(done.appliedButUntracked, true); + assert.deepEqual(done.files, ['src/a.php']); + assert.match(done.error, /could not be undone/); +}); + // --- the harness's own guard --------------------------------------------- // Requiring the real `electron` package is not a harmless fallback: its