From 4515b4279507b5fd6e20d779c39783845e2af89e Mon Sep 17 00:00:00 2001 From: JuanMa Date: Tue, 11 Aug 2026 11:32:44 +0200 Subject: [PATCH 1/2] [Add] Apply Gutenberg pull requests to a Gutenberg site (#251) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With a Gutenberg site now cloning, building and serving, the remaining half of "see what a Gutenberg PR does" is reading the PR and applying it. Both were hard-wired to WordPress Core: - Patch paths were always rewritten into wordpress-develop's src/ layout. That rewrite exists because a patch attached to a Trac ticket years ago still names `wp-admin/…`, but a Gutenberg diff is already repo-relative, and a top-level `wp-`-prefixed path in one would be moved under a `src/` directory Gutenberg does not have. parsePatchFiles now takes a `layout`, applyPatchToDir passes it through, and preview, apply and revert all resolve it from the site so they cannot disagree about where a file lives. - Pull requests were always read from WordPress/wordpress-develop. parsePrRef, fetchLinkedPrs and fetchPrDiff now take the site's upstream, so a Gutenberg site lists and fetches WordPress/gutenberg pull requests — and still refuses a PR from the other project, whose diff would not fit its checkout. - "Which PRs belong to this work item" is a different question per provider: a Core PR cites a Trac URL, a Gutenberg PR cites its issue as `#1234`. Added bodyCitesIssue and citesWorkItemFor; the verification stays narrow (`#658` must not match inside `#6580`, and a bare number is not a citation). The linked-PR cache key now includes the repository: Trac ticket #123 and Gutenberg issue #123 are different work items and shared one entry before. Every new parameter defaults to Core's behaviour, so a site with no project type is unchanged and needs no migration. Verified by hand that a `packages/…` diff applies to a Gutenberg-shaped tree under repo-relative, and that the same patch fails under Core's layout — which is the bug this prevents. Co-Authored-By: Claude Opus 4.8 --- src/github-prs.js | 23 +++++++---- src/main.js | 43 ++++++++++++++++----- src/patch-apply.js | 8 +++- src/patch-plan.cjs | 37 ++++++++++++++---- src/patch-sources.cjs | 68 +++++++++++++++++++++++++++----- src/preload.js | 2 +- src/renderer/index.jsx | 13 +++++-- test/github-prs.test.cjs | 50 +++++++++++++++++++++++- test/ipc-wiring.test.cjs | 71 +++++++++++++++++++++++++++++++--- test/patch-plan.test.cjs | 57 +++++++++++++++++++++++++++ test/patch-sources.test.cjs | 77 ++++++++++++++++++++++++++++++++++++- 11 files changed, 401 insertions(+), 48 deletions(-) diff --git a/src/github-prs.js b/src/github-prs.js index 567fc29..2f6f91e 100644 --- a/src/github-prs.js +++ b/src/github-prs.js @@ -19,20 +19,25 @@ * request (#167) needs the same one with a method and a body. */ -const { parseLinkedPrs, classifyHttpFailure } = require('./patch-sources.cjs'); +const { parseLinkedPrs, classifyHttpFailure, citesWorkItemFor, PR_REPO_PATH } = require('./patch-sources.cjs'); const { httpGet } = require('./github-http.cjs'); -const REPO = 'WordPress/wordpress-develop'; +// The default upstream. Both functions take a `repo` so a Gutenberg site reads +// its own pull requests (#251); left unset they behave exactly as before. +const REPO = PR_REPO_PATH; /** * The pull requests that cite a ticket, newest first. * * @param {number|string} ticketId * @param {Object} [deps] + * @param {string} [deps.repo] `owner/repo` to search (defaults to wordpress-develop). + * @param {string} [deps.provider] Work-item provider deciding what "cites" means. * @return {Promise<{status: 'ok'|'rate-limited'|'error'|'offline', items: Array, error?: string}>} */ async function fetchLinkedPrs(ticketId, deps = {}) { const get = deps.httpGet || httpGet; + const repo = deps.repo || REPO; const id = String(ticketId).replace(/[^0-9]/g, ''); if (!id) return { status: 'error', items: [], error: 'No ticket number' }; @@ -40,7 +45,7 @@ async function fetchLinkedPrs(ticketId, deps = {}) { // paginating would multiply requests against the shared unauthenticated quota // this whole feature is careful with, so instead a result that does not fit in // one page is treated as incomplete below. - const query = encodeURIComponent(`repo:${REPO} is:pr ${id}`); + const query = encodeURIComponent(`repo:${repo} is:pr ${id}`); const url = `https://api.github.com/search/issues?q=${query}&per_page=100`; let res; @@ -67,22 +72,26 @@ async function fetchLinkedPrs(ticketId, deps = {}) { return { status: 'error', items: [], error: 'Too many results to list reliably' }; } - return { status: 'ok', items: parseLinkedPrs(json, id) }; + const cites = citesWorkItemFor(deps.provider, repo); + return { status: 'ok', items: parseLinkedPrs(json, id, { cites, repoPath: repo }) }; } /** * The unified diff for one pull request. * - * @param {number} number + * @param {number} number + * @param {Object} [options] + * @param {string} [options.repo] `owner/repo` the pull request belongs to. + * @param {Function} [options.httpGet] Injected for tests, like fetchLinkedPrs. * @return {Promise<{ok: true, text: string}|{ok: false, status: string, error: string}>} */ -async function fetchPrDiff(number) { +async function fetchPrDiff(number, { repo = REPO, httpGet: get = httpGet } = {}) { const n = String(number).replace(/[^0-9]/g, ''); if (!n) return { ok: false, status: 'error', error: 'No pull request number' }; let res; try { - res = await httpGet(`https://api.github.com/repos/${REPO}/pulls/${n}`, { Accept: 'application/vnd.github.v3.diff' }); + res = await get(`https://api.github.com/repos/${repo}/pulls/${n}`, { Accept: 'application/vnd.github.v3.diff' }); } catch (e) { return { ok: false, status: 'offline', error: String(e && e.message ? e.message : e) }; } diff --git a/src/main.js b/src/main.js index 9b3c29d..596cfb3 100644 --- a/src/main.js +++ b/src/main.js @@ -1317,7 +1317,16 @@ ipcMain.handle('git:update-trunk', async (event, sitePath) => { // GitHub; the network code is in src/github-prs.js, these handlers add the // cache and IPC. A last-known-good copy per ticket, in electron-store, is what // lets a rate-limited or offline lookup still show the work that exists. -const patchCacheKey = (ticketId) => `ticketPatches:${ticketId}`; +// Keyed by repository as well as number (#251): a Trac ticket #123 and a +// Gutenberg issue #123 are different work items, and one cache entry for both +// would show a Core site's pull requests under a Gutenberg issue. +const patchCacheKey = (ticketId, repoPath) => `ticketPatches:${repoPath}#${ticketId}`; + +// `owner/repo` for a site's upstream, defaulting to Core's. +const upstreamRepoPath = (meta) => { + const up = projectTypeForSite(meta).upstream; + return `${up.owner}/${up.repo}`; +}; ipcMain.handle('git:list-ticket-patches', async (_e, sitePath) => { try { @@ -1326,16 +1335,22 @@ ipcMain.handle('git:list-ticket-patches', async (_e, sitePath) => { const ticketId = meta.tracTicket; if (!ticketId) return { ok: true, ticket: null, prs: { status: 'no-ticket', items: [] } }; - const result = await fetchLinkedPrs(ticketId); + // Which repository holds the pull requests, and what "cites this work + // item" means, both follow the site's project type (#251). + const type = projectTypeForSite(meta); + const repo = upstreamRepoPath(meta); + const cacheKey = patchCacheKey(ticketId, repo); + + const result = await fetchLinkedPrs(ticketId, { repo, provider: type.workItem.provider }); if (result.status === 'ok') { - s.set(patchCacheKey(ticketId), { checkedAt: new Date().toISOString(), items: result.items }); + s.set(cacheKey, { checkedAt: new Date().toISOString(), items: result.items }); return { ok: true, ticket: ticketId, prs: { status: 'ok', items: result.items } }; } // Could not read GitHub. Fall back to whatever was last seen for this // ticket, labelled with when — a stale-but-shown list beats a short one // presented as complete. - const cached = s.get(patchCacheKey(ticketId)) || null; + const cached = s.get(cacheKey) || null; return { ok: true, ticket: ticketId, @@ -1346,9 +1361,11 @@ ipcMain.handle('git:list-ticket-patches', async (_e, sitePath) => { } }); -ipcMain.handle('git:fetch-pr-diff', async (_e, number) => { +ipcMain.handle('git:fetch-pr-diff', async (_e, sitePath, number) => { try { - return await fetchPrDiff(number); + // The pull request belongs to this site's own upstream (#251) — a + // Gutenberg site reads WordPress/gutenberg, not wordpress-develop. + return await fetchPrDiff(number, { repo: upstreamRepoPath(await readSiteMeta(sitePath)) }); } catch (e) { return { ok: false, status: 'error', error: String(e) }; } @@ -1391,7 +1408,12 @@ const REVERTABLE_PATCH_LIMIT = 512 * 1024; // before deciding. ipcMain.handle('git:preview-patch', async (_e, sitePath, patchText) => { try { - const parsed = parsePatchFiles(patchText); + // The patch layout follows the site's project (#251): Core rewrites + // pre-src/ paths, Gutenberg diffs are already repo-relative. The apply + // below must be given the same layout or the two disagree about where a + // file lives. + const layout = projectTypeForSite(await readSiteMeta(sitePath)).patch.layout; + const parsed = parsePatchFiles(patchText, { layout }); if (!parsed.ok) return { ok: false, error: parsed.error }; let dirtyPaths; try { @@ -1469,7 +1491,10 @@ ipcMain.handle('git:apply-patch', async (event, sitePath, options = {}) => { } sendLog(`\n${reverse ? 'Reverting' : 'Applying'} ${label}…\n`); - const result = await applyPatchToDir({ dir: sitePath, patchText, reverse, onLog: sendLog }); + // Same layout the preview used (#251) — a Gutenberg diff is already + // repo-relative and must not go through Core's src/ rewrite. + const layout = projectTypeForSite(await readSiteMeta(sitePath)).patch.layout; + const result = await applyPatchToDir({ dir: sitePath, patchText, reverse, layout, onLog: sendLog }); if (!result.ok) { // Nothing to revert means the record is describing a patch the // checkout no longer has. Keeping it would leave the site stuck: @@ -1513,7 +1538,7 @@ ipcMain.handle('git:apply-patch', async (event, sitePath, options = {}) => { // 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 undo = await applyPatchToDir({ dir: sitePath, patchText, reverse: true, layout, 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}` }); diff --git a/src/patch-apply.js b/src/patch-apply.js index 332e30c..a3183c4 100644 --- a/src/patch-apply.js +++ b/src/patch-apply.js @@ -272,11 +272,15 @@ function rollback(done) { * @param {string} root0.dir * @param {string} root0.patchText * @param {boolean} [root0.reverse] + * @param {string} [root0.layout] Path layout: 'src-layout' (default) or 'repo-relative'. * @param {Function} [root0.onLog] * @return {Promise} */ -async function applyPatchToDir({ dir, patchText, reverse = false, onLog = () => {} }) { - const parsed = parsePatchFiles(patchText); +async function applyPatchToDir({ dir, patchText, reverse = false, layout, onLog = () => {} }) { + // The layout must match the one the preview used, or the two disagree about + // where a file lives and the patch applies somewhere the contributor was + // never shown (#251). Defaults to Core's src-layout. + const parsed = parsePatchFiles(patchText, { layout }); if (!parsed.ok) return { ok: false, error: parsed.error }; await ensureAutocrlf(dir); diff --git a/src/patch-plan.cjs b/src/patch-plan.cjs index cf7d47e..5de94b9 100644 --- a/src/patch-plan.cjs +++ b/src/patch-plan.cjs @@ -137,14 +137,33 @@ function classify(file, oldPath, newPath) { return 'modify'; } +/** + * Chooses the per-path normalisation for a project's layout (#251). + * + * `src-layout` (WordPress Core, the default) rewrites pre-`src/` paths — a patch + * attached to a ticket years ago still names `wp-admin/…`. `repo-relative` + * (Gutenberg) leaves paths alone: its diffs are already repo-relative + * (`packages/…`), and running them through the Core rewrite would move any + * top-level `wp-`-prefixed path under a `src/` directory that does not exist + * there. + * + * @param {string} [layout] + * @return {(filePath: string) => string} + */ +function pathMapperFor(layout) { + return layout === 'repo-relative' ? (filePath) => filePath : mapToSrcLayout; +} + /** * Parses a patch into the files it touches, with paths normalised to - * repo-relative form for today's layout. + * repo-relative form for the target project's layout. * * @param {string} text + * @param {Object} [options] + * @param {string} [options.layout] 'src-layout' (default) or 'repo-relative'. * @return {{ok: true, files: Array}|{ok: false, error: string}} */ -function parsePatchFiles(text) { +function parsePatchFiles(text, { layout } = {}) { const raw = typeof text === 'string' ? text : ''; if (!raw.trim()) return { ok: false, error: 'The patch is empty.' }; @@ -162,6 +181,7 @@ function parsePatchFiles(text) { } const sections = scanSections(normalizeEol(raw)); + const mapPath = pathMapperFor(layout); const files = []; for (let i = 0; i < parsed.length; i++) { @@ -172,13 +192,13 @@ function parsePatchFiles(text) { // what this was. const section = sections[i]; if (section && section.renameFrom && section.renameTo) { - const oldPath = mapToSrcLayout(section.renameFrom); - const newPath = mapToSrcLayout(section.renameTo); + const oldPath = mapPath(section.renameFrom); + const newPath = mapPath(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); + const binaryPath = mapPath(stripPathPrefix(section.path, section.path).newPath); files.push({ kind: 'binary', oldPath: binaryPath, newPath: binaryPath, path: binaryPath, hunks: [], patch: file }); continue; } @@ -192,9 +212,9 @@ function parsePatchFiles(text) { const target = kind === 'delete' ? oldPath : newPath; files.push({ kind, - oldPath: mapToSrcLayout(oldPath), - newPath: mapToSrcLayout(newPath), - path: mapToSrcLayout(target), + oldPath: mapPath(oldPath), + newPath: mapPath(newPath), + path: mapPath(target), hunks: file.hunks, patch: file }); @@ -242,6 +262,7 @@ module.exports = { SRC_FILES, stripPathPrefix, mapToSrcLayout, + pathMapperFor, parsePatchFiles, planApply }; diff --git a/src/patch-sources.cjs b/src/patch-sources.cjs index 489b5fe..4f6c0f4 100644 --- a/src/patch-sources.cjs +++ b/src/patch-sources.cjs @@ -24,14 +24,20 @@ const PR_REPO_PATH = 'WordPress/wordpress-develop'; /** * Resolves what a contributor pastes into "apply a PR" to a pull request - * number. Accepts a bare number or a wordpress-develop PR URL (with any - * trailing `/files`, `#…`, `?…`). A PR from another repo is rejected by name — - * its diff would not fit this checkout. + * number. Accepts a bare number or a PR URL (with any trailing `/files`, `#…`, + * `?…`) belonging to this site's own upstream. A PR from another repo is + * rejected by name — its diff would not fit this checkout. + * + * `repoPath` is the site's upstream (#251): a Gutenberg site applies + * WordPress/gutenberg PRs, a Core site wordpress-develop ones. It defaults to + * wordpress-develop so existing callers are unchanged. * * @param {string} input + * @param {Object} [options] + * @param {string} [options.repoPath] `owner/repo` this checkout accepts. * @return {{ok: true, number: number}|{ok: false, error: string}} */ -function parsePrRef(input) { +function parsePrRef(input, { repoPath = PR_REPO_PATH } = {}) { const raw = typeof input === 'string' ? input.trim() : ''; if (!raw) return { ok: false, error: 'Enter a pull request URL or number.' }; @@ -48,8 +54,8 @@ function parsePrRef(input) { } const match = /^\/([^/]+\/[^/]+)\/pull\/(\d+)(?:[/?#]|$)/.exec(parsed.pathname + (parsed.pathname.endsWith('/') ? '' : '/')); if (!match) return { ok: false, error: 'That does not look like a pull request URL.' }; - if (match[1].toLowerCase() !== PR_REPO_PATH.toLowerCase()) { - return { ok: false, error: `Only ${PR_REPO_PATH} pull requests can be applied here.` }; + if (match[1].toLowerCase() !== String(repoPath).toLowerCase()) { + return { ok: false, error: `Only ${repoPath} pull requests can be applied here.` }; } return { ok: true, number: Number(match[2]) }; } @@ -71,6 +77,44 @@ function bodyCitesTicket(body, ticketId) { return re.test(body); } +/** + * The same question for a GitHub-issue work item (#251). There is no ticket URL + * to look for: a Gutenberg pull request cites its issue the GitHub way, as + * `#1234` (usually behind a closing keyword) or as the issue's own URL. + * + * The `(?![0-9])` guard is why this is not a bare `includes`: `#658` must not + * match inside `#6580`. A bare number with no `#` is deliberately not accepted — + * that is the prose match the verification exists to reject. + * + * @param {string} body + * @param {number|string} issueId + * @param {string} [repoPath] `owner/repo`, for the URL form. + * @return {boolean} + */ +function bodyCitesIssue(body, issueId, repoPath = '') { + if (typeof body !== 'string') return false; + const id = String(issueId).replace(/[^0-9]/g, ''); + if (!id) return false; + if (new RegExp(`#${id}(?![0-9])`).test(body)) return true; + if (!repoPath) return false; + const path = String(repoPath).replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + return new RegExp(`github\\.com/${path}/issues/${id}(?![0-9])`, 'i').test(body); +} + +/** + * Picks the citation test for a work-item provider, so a caller can ask "does + * this PR belong to this work item?" without knowing which kind it is. + * + * @param {string} provider 'trac' (default) or 'github-issue'. + * @param {string} [repoPath] + * @return {(body: string, id: number|string) => boolean} + */ +function citesWorkItemFor(provider, repoPath) { + return provider === 'github-issue' + ? (body, id) => bodyCitesIssue(body, id, repoPath) + : bodyCitesTicket; +} + /** * What happened to one pull request, from a `search/issues` item: open, merged * or closed-unmerged. @@ -90,9 +134,12 @@ function prState(item) { * * @param {Object} searchJson * @param {number|string} ticketId + * @param {Object} [root0] + * @param {Function} [root0.cites] Citation test (defaults to the Trac one). + * @param {string} [root0.repoPath] `owner/repo`, for the fallback URL. * @return {Array<{number: number, title: string, state: 'open'|'merged'|'closed', updatedAt: string, url: string}>} */ -function parseLinkedPrs(searchJson, ticketId) { +function parseLinkedPrs(searchJson, ticketId, { cites = bodyCitesTicket, repoPath = PR_REPO_PATH } = {}) { const items = searchJson && Array.isArray(searchJson.items) ? searchJson.items : []; const seen = new Set(); const prs = []; @@ -100,7 +147,7 @@ function parseLinkedPrs(searchJson, ticketId) { // `search/issues` returns issues and PRs together; only PRs carry // `pull_request`. if (!item || !item.pull_request) continue; - if (!bodyCitesTicket(item.body, ticketId)) continue; + if (!cites(item.body, ticketId)) continue; if (seen.has(item.number)) continue; seen.add(item.number); prs.push({ @@ -113,7 +160,7 @@ function parseLinkedPrs(searchJson, ticketId) { // unauthenticated quota this file is careful with. state: prState(item), updatedAt: item.updated_at || item.created_at || '', - url: item.html_url || `https://github.com/WordPress/wordpress-develop/pull/${item.number}` + url: item.html_url || `https://github.com/${repoPath}/pull/${item.number}` }); } prs.sort((a, b) => (b.updatedAt || '').localeCompare(a.updatedAt || '')); @@ -143,7 +190,10 @@ function classifyHttpFailure(status, headers = {}) { module.exports = { TICKET_HOST, + PR_REPO_PATH, bodyCitesTicket, + bodyCitesIssue, + citesWorkItemFor, parseLinkedPrs, classifyHttpFailure, parsePrRef diff --git a/src/preload.js b/src/preload.js index 5b0a56d..cac5056 100644 --- a/src/preload.js +++ b/src/preload.js @@ -188,7 +188,7 @@ contextBridge.exposeInMainWorld('api', { , listTicketPatches: (sitePath) => ipcRenderer.invoke('git:list-ticket-patches', sitePath) , - fetchPrDiff: (number) => ipcRenderer.invoke('git:fetch-pr-diff', number) + fetchPrDiff: (sitePath, number) => ipcRenderer.invoke('git:fetch-pr-diff', sitePath, number) , listTracAttachments: (sitePath) => ipcRenderer.invoke('trac:list-attachments', sitePath) , diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index 2f97611..5942531 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -1278,8 +1278,11 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // The build/watch commands and the terminal's allowed scripts for this site's // project type. A plain lookup, not a hook — recomputed each render from the // current type, defaulting to Core. - const projectBuildConfig = getProjectType(projectType).build; + const projectConfig = getProjectType(projectType); + const projectBuildConfig = projectConfig.build; const allowedScripts = projectBuildConfig.allowedScripts; + // `owner/repo` this site's pull requests come from and go to. + const upstreamRepoPath = `${projectConfig.upstream.owner}/${projectConfig.upstream.repo}`; const [statusLoading, setStatusLoading] = useState(true); const [waitingForWatch, setWaitingForWatch] = useState(false); // Trac ticket association (#109) @@ -2885,7 +2888,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit setApplyNotice(''); setFetchingPr(pr.number); try { - const diff = await window.api.fetchPrDiff(pr.number); + const diff = await window.api.fetchPrDiff(sitePath, pr.number); if (!diff || !diff.ok) { setApplyError(diff?.status === 'rate-limited' ? 'GitHub is rate-limiting this connection right now. Open the PR and download its .diff, then use “Choose a patch file”.' @@ -2951,10 +2954,12 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // Apply a PR straight from a pasted URL or number, without needing it to be // linked to the ticket — same fetch → preview flow as the linked-PR list. const previewPrFromInput = () => { - const parsed = parsePrRef(prUrlInput); + // Only this site's own upstream (#251): a Gutenberg diff does not fit a Core + // checkout, and vice versa. + const parsed = parsePrRef(prUrlInput, { repoPath: upstreamRepoPath }); if (!parsed.ok) { setApplyError(parsed.error); setApplyNotice(''); return; } setPrUrlInput(''); - previewPr({ number: parsed.number, url: `https://github.com/WordPress/wordpress-develop/pull/${parsed.number}` }); + previewPr({ number: parsed.number, url: `https://github.com/${upstreamRepoPath}/pull/${parsed.number}` }); }; const runApply = async ({ reverse = false } = {}) => { diff --git a/test/github-prs.test.cjs b/test/github-prs.test.cjs index 440752b..d337ba1 100644 --- a/test/github-prs.test.cjs +++ b/test/github-prs.test.cjs @@ -3,7 +3,7 @@ const test = require('node:test'); const assert = require('node:assert'); const { EventEmitter } = require('node:events'); -const { httpGet, fetchLinkedPrs } = require('../src/github-prs'); +const { httpGet, fetchLinkedPrs, fetchPrDiff } = require('../src/github-prs'); // A stand-in for Electron's `net`: request() hands back an EventEmitter whose // end() lets the test drive the response (or an error) the way the real client @@ -120,3 +120,51 @@ test('fetchLinkedPrs reports offline on a transport failure rather than empty', assert.strictEqual(res.status, 'offline'); assert.deepStrictEqual(res.items, []); }); + +// --- per-project upstream (#251) ------------------------------------------ +// +// Both functions default to wordpress-develop, but a Gutenberg site has to read +// its own repository. Nothing else asserts that `repo`/`provider` actually reach +// the request and the citation test — the IPC-wiring tests stub this module out, +// so a typo in the plumbing would ship green. + +test('fetchLinkedPrs searches the repository it is given', async () => { + const urls = []; + const body = JSON.stringify({ total_count: 0, incomplete_results: false, items: [] }); + const httpGetSpy = async (url) => { urls.push(url); return { status: 200, headers: {}, body }; }; + + await fetchLinkedPrs('123', { httpGet: httpGetSpy }); + assert.match(decodeURIComponent(urls[0]), /repo:WordPress\/wordpress-develop/, 'defaults to Core'); + + await fetchLinkedPrs('123', { httpGet: httpGetSpy, repo: 'WordPress/gutenberg' }); + assert.match(decodeURIComponent(urls[1]), /repo:WordPress\/gutenberg/); +}); + +// The provider decides what "cites this work item" means. A Gutenberg PR says +// `Fixes #123`; filtering it with Core's Trac-URL test would drop every result. +test('fetchLinkedPrs filters by the provider’s citation convention', async () => { + const items = [{ number: 42, pull_request: { url: 'x' }, title: 'Fix', state: 'open', updated_at: '2026-01-01T00:00:00Z', html_url: 'u', body: 'Fixes #123' }]; + const body = JSON.stringify({ total_count: 1, incomplete_results: false, items }); + const httpGetStub = async () => ({ status: 200, headers: {}, body }); + + // Core's test looks for a Trac URL, which this PR does not have. + const asCore = await fetchLinkedPrs('123', { httpGet: httpGetStub, repo: 'WordPress/gutenberg' }); + assert.strictEqual(asCore.items.length, 0); + + const asGutenberg = await fetchLinkedPrs('123', { httpGet: httpGetStub, repo: 'WordPress/gutenberg', provider: 'github-issue' }); + assert.strictEqual(asGutenberg.items.length, 1); + assert.strictEqual(asGutenberg.items[0].number, 42); +}); + +test('fetchPrDiff reads the pull request from the repository it is given', async () => { + const urls = []; + const httpGetSpy = async (url) => { urls.push(url); return { status: 200, headers: {}, body: 'DIFF' }; }; + + const core = await fetchPrDiff(7319, { httpGet: httpGetSpy }); + assert.strictEqual(core.ok, true); + assert.strictEqual(core.text, 'DIFF'); + assert.match(urls[0], /repos\/WordPress\/wordpress-develop\/pulls\/7319$/, 'defaults to Core'); + + await fetchPrDiff(4496, { repo: 'WordPress/gutenberg', httpGet: httpGetSpy }); + assert.match(urls[1], /repos\/WordPress\/gutenberg\/pulls\/4496$/); +}); diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 851c50b..488d9da 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -1798,14 +1798,32 @@ test('sites:set-ticket refuses unregistered site paths before writing metadata', 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 settings = fakeSettingsStore({ sites: ['/sites/wp'], siteMeta: { '/sites/wp': {} } }); + const main = loadMain({ + stubs: { ...silentLogging(), ...settings.stubs, './patch-plan.cjs': { parsePatchFiles, planApply: () => ({}) } } + }); const result = await main.invoke('git:preview-patch', '/sites/wp', 'PATCH TEXT'); - assert.deepEqual(parsePatchFiles.calls, [['PATCH TEXT']]); + // The layout rides along (#251) — a site with no project type is Core's. + assert.deepEqual(parsePatchFiles.calls, [['PATCH TEXT', { layout: 'src-layout' }]]); assert.deepEqual(result, { ok: false, error: 'unreadable' }); }); +// A Gutenberg diff is already repo-relative, so it must NOT go through Core's +// src/ rewrite — the preview and the apply both have to say so (#251). +test('git:preview-patch reads a Gutenberg patch as repo-relative', async () => { + const parsePatchFiles = spy(() => ({ ok: false, error: 'unreadable' })); + const settings = fakeSettingsStore({ sites: ['/sites/gb'], siteMeta: { '/sites/gb': { projectType: 'gutenberg' } } }); + const main = loadMain({ + stubs: { ...silentLogging(), ...settings.stubs, './patch-plan.cjs': { parsePatchFiles, planApply: () => ({}) } } + }); + + await main.invoke('git:preview-patch', '/sites/gb', 'PATCH TEXT'); + + assert.deepEqual(parsePatchFiles.calls, [['PATCH TEXT', { layout: 'repo-relative' }]]); +}); + // 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. @@ -1864,6 +1882,10 @@ test('git:apply-patch delegates a forward apply to patch-apply and records it', assert.equal(args.dir, '/sites/wp'); assert.equal(args.patchText, 'PATCH'); assert.equal(args.reverse, false); + // The layout has to reach the applier, not just the preview (#251): without + // it a Gutenberg diff would silently go through Core's src/ rewrite and land + // on paths the contributor was never shown. + assert.equal(args.layout, 'src-layout'); // 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; @@ -1872,6 +1894,20 @@ test('git:apply-patch delegates a forward apply to patch-apply and records it', assert.deepEqual(stored.files, ['src/a.php']); }); +// The other half of the same invariant: preview and apply must resolve the same +// layout, so the site's own type has to reach the applier too. +test('git:apply-patch applies a Gutenberg patch with the repo-relative layout', async () => { + const applyPatchToDir = spy(async () => ({ ok: true, applied: ['packages/a/index.js'], skipped: [] })); + const settings = fakeSettingsStore({ sites: ['/sites/gb'], siteMeta: { '/sites/gb': { projectType: 'gutenberg' } } }); + const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs, './patch-apply': { applyPatchToDir } } }); + + const event = createIpcEvent(); + const { applyId } = await main.invokeWith('git:apply-patch', event, '/sites/gb', { patchText: 'PATCH', label: 'PR 4496' }); + await applyDone(event, applyId); + + assert.equal(applyPatchToDir.calls[0][0].layout, 'repo-relative'); +}); + 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({ @@ -1905,6 +1941,9 @@ test('git:apply-patch reverts using the stored patch text and clears the record' 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'); + // A revert has to resolve paths the same way the apply did, or it reverses + // hunks against files the patch never touched (#251). + assert.equal(args.layout, 'src-layout'); assert.equal(settings.values.siteMeta['/sites/wp'].appliedPatch, null); }); @@ -2001,14 +2040,32 @@ test('git:apply-patch reports applied-but-untracked when the undo also fails', a test('git:fetch-pr-diff asks github-prs for the diff', async () => { const fetchPrDiff = spy(async () => ({ ok: true, text: 'DIFF' })); - const main = loadMain({ stubs: { ...silentLogging(), './github-prs': { fetchPrDiff, fetchLinkedPrs: async () => ({}) } } }); + const settings = fakeSettingsStore({ sites: ['/sites/wp'], siteMeta: { '/sites/wp': {} } }); + const main = loadMain({ + stubs: { ...silentLogging(), ...settings.stubs, './github-prs': { fetchPrDiff, fetchLinkedPrs: async () => ({}) } } + }); - const result = await main.invoke('git:fetch-pr-diff', 7319); + const result = await main.invoke('git:fetch-pr-diff', '/sites/wp', 7319); - assert.deepEqual(fetchPrDiff.calls, [[7319]]); + // The repository comes from the site, not the caller (#251). + assert.deepEqual(fetchPrDiff.calls, [[7319, { repo: 'WordPress/wordpress-develop' }]]); assert.deepEqual(result, { ok: true, text: 'DIFF' }); }); +// A Gutenberg site reads its own pull requests — fetching #7319 from +// wordpress-develop would hand it a diff from a different project entirely. +test('git:fetch-pr-diff reads a Gutenberg site’s PR from WordPress/gutenberg', async () => { + const fetchPrDiff = spy(async () => ({ ok: true, text: 'DIFF' })); + const settings = fakeSettingsStore({ sites: ['/sites/gb'], siteMeta: { '/sites/gb': { projectType: 'gutenberg' } } }); + const main = loadMain({ + stubs: { ...silentLogging(), ...settings.stubs, './github-prs': { fetchPrDiff, fetchLinkedPrs: async () => ({}) } } + }); + + await main.invoke('git:fetch-pr-diff', '/sites/gb', 71234); + + assert.deepEqual(fetchPrDiff.calls, [[71234, { repo: 'WordPress/gutenberg' }]]); +}); + // git:list-ticket-patches reads the stored ticket, then delegates to github-prs // and caches the result — reachable through the same fakeSettingsStore seam. test('git:list-ticket-patches fetches the linked PRs for the stored ticket', async () => { @@ -2018,7 +2075,9 @@ test('git:list-ticket-patches fetches the linked PRs for the stored ticket', asy const result = await main.invoke('git:list-ticket-patches', '/sites/wp'); - assert.deepEqual(fetchLinkedPrs.calls, [[62281]]); + // Which repo to search, and what "cites this work item" means, follow the + // site's project type (#251); a typeless site is Core's. + assert.deepEqual(fetchLinkedPrs.calls, [[62281, { repo: 'WordPress/wordpress-develop', provider: 'trac' }]]); assert.equal(result.ok, true); assert.equal(result.ticket, 62281); assert.equal(result.prs.status, 'ok'); diff --git a/test/patch-plan.test.cjs b/test/patch-plan.test.cjs index c3c9b97..e33a5bf 100644 --- a/test/patch-plan.test.cjs +++ b/test/patch-plan.test.cjs @@ -335,3 +335,60 @@ test('parsePatchFiles: a deletion in this app\'s own generated shape is a delete assert.strictEqual(res.files[0].kind, 'delete'); assert.strictEqual(res.files[0].path, 'src/old.php'); }); + +// --- per-project path layout (#251) --------------------------------------- +// +// Core patches are normalised into wordpress-develop's src/ layout, because a +// patch attached to a ticket years ago still names `wp-admin/…`. Gutenberg +// diffs are already repo-relative (`packages/…`), and a top-level `wp-` path in +// one would be moved under a `src/` directory that does not exist there. + +test('parsePatchFiles: the default layout still rewrites into src/ (Core)', () => { + const patch = `--- a/wp-login.php ++++ b/wp-login.php +@@ -1,1 +1,2 @@ + one ++two +`; + const res = parsePatchFiles(patch); + assert.strictEqual(res.ok, true, res.error); + assert.strictEqual(res.files[0].path, 'src/wp-login.php'); +}); + +test('parsePatchFiles: repo-relative leaves a wp-prefixed path alone (Gutenberg)', () => { + const patch = `--- a/wp-login.php ++++ b/wp-login.php +@@ -1,1 +1,2 @@ + one ++two +`; + const res = parsePatchFiles(patch, { layout: 'repo-relative' }); + assert.strictEqual(res.ok, true, res.error); + assert.strictEqual(res.files[0].path, 'wp-login.php', 'no src/ rewrite for a repo-relative project'); +}); + +// The paths Gutenberg diffs actually carry pass through both layouts unchanged +// — this is the case that must not regress when the gate is added. +test('parsePatchFiles: a packages/ path is untouched under either layout', () => { + const patch = `--- a/packages/block-editor/src/index.js ++++ b/packages/block-editor/src/index.js +@@ -1,1 +1,2 @@ + one ++two +`; + for (const layout of [undefined, 'src-layout', 'repo-relative']) { + const res = parsePatchFiles(patch, { layout }); + assert.strictEqual(res.ok, true, res.error); + assert.strictEqual(res.files[0].path, 'packages/block-editor/src/index.js', `layout=${layout}`); + } +}); + +test('parsePatchFiles: an unknown layout falls back to the Core rewrite', () => { + const patch = `--- a/wp-login.php ++++ b/wp-login.php +@@ -1,1 +1,2 @@ + one ++two +`; + assert.strictEqual(parsePatchFiles(patch, { layout: 'nonsense' }).files[0].path, 'src/wp-login.php'); +}); diff --git a/test/patch-sources.test.cjs b/test/patch-sources.test.cjs index 47b4675..386bfd9 100644 --- a/test/patch-sources.test.cjs +++ b/test/patch-sources.test.cjs @@ -2,7 +2,7 @@ const test = require('node:test'); const assert = require('node:assert'); -const { bodyCitesTicket, parseLinkedPrs, classifyHttpFailure } = require('../src/patch-sources.cjs'); +const { bodyCitesTicket, bodyCitesIssue, citesWorkItemFor, parseLinkedPrs, classifyHttpFailure } = require('../src/patch-sources.cjs'); // Shaped like a real search/issues item, trimmed to the fields the parser reads. function item(number, overrides = {}) { @@ -133,3 +133,78 @@ test('parsePrRef: non-PR and empty input are rejected with a reason (issue #11)' assert.strictEqual(parsePrRef('https://example.com/pull/1').ok, false); assert.strictEqual(parsePrRef('not a url').ok, false); }); + +// --- GitHub-issue work items (#251) --------------------------------------- +// +// A Gutenberg pull request has no Trac URL to cite: it references its issue the +// GitHub way. The verification still has to be narrow, because the search that +// feeds it matches the bare number anywhere in the text. + +test('bodyCitesIssue: a #-prefixed reference counts', () => { + assert.strictEqual(bodyCitesIssue('Fixes #1234, at last.', 1234), true); + assert.strictEqual(bodyCitesIssue('Closes #1234', 1234), true); + assert.strictEqual(bodyCitesIssue('see #1234.', '1234'), true); +}); + +// The guard that makes the check trustworthy rather than merely plausible. +test('bodyCitesIssue: a longer number is not a match for its prefix', () => { + assert.strictEqual(bodyCitesIssue('Fixes #12345', 1234), false); + assert.strictEqual(bodyCitesIssue('Fixes #1234', 12345), false); +}); + +// A bare number is exactly the prose match the verification exists to reject: +// GitHub's search tokeniser finds "1234" in unrelated sentences. +test('bodyCitesIssue: a bare number without # does not count', () => { + assert.strictEqual(bodyCitesIssue('This fixes 1234 rendering bugs.', 1234), false); +}); + +test('bodyCitesIssue: the issue URL counts when the repo is known', () => { + const body = 'Fixes https://github.com/WordPress/gutenberg/issues/1234'; + assert.strictEqual(bodyCitesIssue(body, 1234, 'WordPress/gutenberg'), true); + // Without a repo the URL form cannot be checked, but the #-form in it can't + // be faked either — a URL alone with no `#1234` is not a match. + assert.strictEqual(bodyCitesIssue(body, 1234), false); +}); + +test('bodyCitesIssue: non-string bodies and empty ids are rejected', () => { + assert.strictEqual(bodyCitesIssue(null, 1234), false); + assert.strictEqual(bodyCitesIssue('#1234', ''), false); +}); + +test('citesWorkItemFor: picks the provider’s test, defaulting to Trac', () => { + const trac = citesWorkItemFor('trac'); + assert.strictEqual(trac, bodyCitesTicket); + assert.strictEqual(citesWorkItemFor(undefined), bodyCitesTicket, 'an unknown provider is Core’s'); + + const gh = citesWorkItemFor('github-issue', 'WordPress/gutenberg'); + assert.strictEqual(gh('Fixes #1234', 1234), true); + // And it does NOT accept the Trac form, which would be a different project. + assert.strictEqual(gh('core.trac.wordpress.org/ticket/1234', 1234), false); +}); + +// The whole point of threading `cites` through: a Gutenberg search result is +// filtered by the GitHub convention, not by a Trac URL that will never be there. +test('parseLinkedPrs: uses the supplied citation test', () => { + const json = { items: [item(11, { body: 'Fixes #1234' }), item(12, { body: 'unrelated' })] }; + const cites = citesWorkItemFor('github-issue', 'WordPress/gutenberg'); + + const prs = parseLinkedPrs(json, 1234, { cites, repoPath: 'WordPress/gutenberg' }); + assert.deepStrictEqual(prs.map((p) => p.number), [11]); +}); + +// The repo guard is what stops a diff from the wrong project being applied to a +// checkout it cannot fit — so it moves with the site, it does not go away. +test('parsePrRef: a Gutenberg site accepts gutenberg PRs and rejects Core ones', () => { + const opts = { repoPath: 'WordPress/gutenberg' }; + + const ok = parsePrRef('https://github.com/WordPress/gutenberg/pull/4496', opts); + assert.strictEqual(ok.ok, true); + assert.strictEqual(ok.number, 4496); + + const wrong = parsePrRef('https://github.com/WordPress/wordpress-develop/pull/7319', opts); + assert.strictEqual(wrong.ok, false); + assert.match(wrong.error, /WordPress\/gutenberg/); + + // A bare number is still just a number — it names no repo to disagree with. + assert.deepStrictEqual(parsePrRef('#4496', opts), { ok: true, number: 4496 }); +}); From 1fa8a6f1e53bdefa90d925cbedf7331ffa257d47 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Wed, 12 Aug 2026 08:33:46 +0200 Subject: [PATCH 2/2] Pin git:list-ticket-patches from both sides (#251) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every other pairing this PR introduces is asserted for Core and for Gutenberg; git:list-ticket-patches had only the Core half. It is also the one call where the provider decides whether any result survives the citation filter, so a handler that resolved the repo per-site but left the provider hardcoded would search the right repository and then discard every genuine match — an empty "Linked pull requests" list with no error to explain it. Co-Authored-By: Claude Opus 5 (1M context) --- test/ipc-wiring.test.cjs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 488d9da..ae4ec3e 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -2084,6 +2084,24 @@ test('git:list-ticket-patches fetches the linked PRs for the stored ticket', asy assert.deepEqual(result.prs.items, [{ number: 7, title: 'x' }]); }); +// The Gutenberg half of the same pair. This is the one call where the provider +// decides whether any result survives at all: a Gutenberg PR cites its work item +// as `#71234`, never as a Trac URL, so a handler that resolved the repo per-site +// but left the provider hardcoded would search the right repository and then +// filter every genuine match away — an empty "Linked pull requests" list with no +// error to explain it. +test('git:list-ticket-patches searches a Gutenberg site’s own repo, with its own provider', async () => { + const fetchLinkedPrs = spy(async () => ({ status: 'ok', items: [{ number: 71300, title: 'x' }] })); + const settings = fakeSettingsStore({ sites: ['/sites/gb'], siteMeta: { '/sites/gb': { projectType: 'gutenberg', tracTicket: 71234 } } }); + const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs, './github-prs': { fetchLinkedPrs } } }); + + const result = await main.invoke('git:list-ticket-patches', '/sites/gb'); + + assert.deepEqual(fetchLinkedPrs.calls, [[71234, { repo: 'WordPress/gutenberg', provider: 'github-issue' }]]); + assert.equal(result.ok, true); + assert.equal(result.ticket, 71234); +}); + test('git:list-ticket-patches falls back to the cached list when GitHub cannot be read', async () => { let call = 0; const fetchLinkedPrs = spy(async () => (++call === 1