From 3b2c4935170e765f1341df5200020283d9a1f6f1 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 27 Oct 2026 11:45:59 +1100 Subject: [PATCH 1/9] feat(boards): refresh review canvases every 15 minutes Reuse the existing refresh routes while canvases are open, preserve local review state, and surface polling errors without clearing board data. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../external-plugin-review-board/README.md | 7 + .../lib/server.mjs | 18 ++ .../public/app.js | 1 + .github/extensions/open-pr-board/README.md | 8 + .../extensions/open-pr-board/lib/server.mjs | 18 ++ .../extensions/open-pr-board/public/app.js | 1 + eng/board-periodic-refresh.test.mjs | 206 ++++++++++++++++++ eng/board-preview.test.mjs | 37 +++- 8 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 eng/board-periodic-refresh.test.mjs diff --git a/.github/extensions/external-plugin-review-board/README.md b/.github/extensions/external-plugin-review-board/README.md index 728c1fd287..ea21ac4855 100644 --- a/.github/extensions/external-plugin-review-board/README.md +++ b/.github/extensions/external-plugin-review-board/README.md @@ -22,6 +22,11 @@ Open it by asking Copilot to "open the plugin review board" (canvas id `external `requires-submitter-fixes` states. Pending intake cards stay on the board so their AI reviews, manual buckets, queued re-reviews, and fast-track dismissal state are preserved. Approval and first-party fast-track are unavailable until intake is ready. + The same read-only refresh runs automatically every 15 minutes while the canvas is open; + concurrent fetches are shared and closing the canvas stops its timer. Selection, staged feedback, + reviews, queue tokens, and local decisions remain intact for submissions still on the board. + Polling never starts AI reviews or writes to GitHub. Failures show an error without clearing + the board; the next scheduled or manual refresh can retry. - **Blocked intake status** keeps submissions visible in their existing buckets with a *Blocked by intake* badge and a reason (submitter fixes required, automated intake pending, or missing `ready-for-review`). The toolbar shows how many submissions can actually be queued and @@ -78,6 +83,8 @@ not imported; boards start fresh. ## Regression tests +Run `node --experimental-vm-modules --test eng/board-periodic-refresh.test.mjs` to check +automatic refresh cadence, concurrent/manual refreshes, state preservation, errors, and disposal. Run `node --test eng/board-column-order.test.mjs` to check oldest-first column ordering. Run `node --experimental-vm-modules --test eng/board-state-storage.test.mjs` from the repository root to check both canvases' session-scoped persistence without accessing GitHub. diff --git a/.github/extensions/external-plugin-review-board/lib/server.mjs b/.github/extensions/external-plugin-review-board/lib/server.mjs index bfe3675548..4e3f56e1f8 100644 --- a/.github/extensions/external-plugin-review-board/lib/server.mjs +++ b/.github/extensions/external-plugin-review-board/lib/server.mjs @@ -24,6 +24,7 @@ const CSP = [ ].join("; "); const MAX_BODY = 64 * 1024; +const REFRESH_INTERVAL_MS = 15 * 60 * 1000; export function expectedError(message, status) { return Object.assign(new Error(message), { publicMessage: message, status }); @@ -168,9 +169,26 @@ export async function startBoardServer({ publicDir, api }) { await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); port = server.address().port; + let refreshing = false; + const refreshTimer = setInterval(async () => { + if (refreshing) return; + refreshing = true; + try { + await api.routes["POST /api/refresh"]({}); + } catch (error) { + if (!Number.isInteger(error?.status) || error.status >= 500) reportError(api, error); + const frame = `event: refresh-error\ndata: ${JSON.stringify({ error: clientErrorMessage(error) })}\n\n`; + for (const client of sseClients) client.write(frame); + } finally { + refreshing = false; + } + }, REFRESH_INTERVAL_MS); + refreshTimer.unref(); + return { url: `http://127.0.0.1:${port}/`, async close() { + clearInterval(refreshTimer); unsubscribe(); for (const client of sseClients) client.end(); sseClients.clear(); diff --git a/.github/extensions/external-plugin-review-board/public/app.js b/.github/extensions/external-plugin-review-board/public/app.js index e8d4e9c670..0a8a7f6f06 100644 --- a/.github/extensions/external-plugin-review-board/public/app.js +++ b/.github/extensions/external-plugin-review-board/public/app.js @@ -887,6 +887,7 @@ function connect() { renderBoard(); if (selected != null) renderDrawer(); }); + events.addEventListener("refresh-error", (event) => toast(JSON.parse(event.data).error, { error: true })); events.addEventListener("error", () => { ui.meta.textContent = "Reconnecting…"; }); diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index 8fc740b6a6..89d7fee51c 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -8,6 +8,12 @@ authenticated `gh` CLI. ## What it does +- **Automatic refresh** — fetches open PRs every 15 minutes while the canvas is open, + using the same read-only path as manual Refresh. Concurrent fetches are shared; + closing the canvas stops its timer. Existing reviews, queue tokens, manual buckets, + decisions, selection, and staged feedback are retained for PRs still on the board. + Refresh does not start AI reviews or write to GitHub. Failures show an error without + clearing the board, and the next scheduled or manual refresh can retry. - **Buckets** — `Unreviewed`, `Reviewing`, `Close`, `Request changes`, `Needs insight`, `Approve`, `Actioned`. The AI recommendation picks the bucket; dragging a card overrides it. Cards in each bucket are sorted oldest to newest @@ -94,6 +100,8 @@ Results arriving after a manual move, reset, or replacement review are rejected. ## Regression tests +Run `node --experimental-vm-modules --test eng/board-periodic-refresh.test.mjs` to check +automatic refresh cadence, concurrent/manual refreshes, state preservation, errors, and disposal. Run `node --test eng/board-column-order.test.mjs` to check oldest-first column ordering. Run `node --test eng/open-pr-board.test.mjs` from the repository root. GitHub writes are tested with injected in-process fakes, never the authenticated CLI. diff --git a/.github/extensions/open-pr-board/lib/server.mjs b/.github/extensions/open-pr-board/lib/server.mjs index b90b697fa0..4399f87fcd 100644 --- a/.github/extensions/open-pr-board/lib/server.mjs +++ b/.github/extensions/open-pr-board/lib/server.mjs @@ -24,6 +24,7 @@ const CSP = [ ].join("; "); const MAX_BODY = 64 * 1024; +const REFRESH_INTERVAL_MS = 15 * 60 * 1000; export function expectedError(message, status) { return Object.assign(new Error(message), { publicMessage: message, status }); @@ -168,9 +169,26 @@ export async function startBoardServer({ publicDir, api }) { await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); port = server.address().port; + let refreshing = false; + const refreshTimer = setInterval(async () => { + if (refreshing) return; + refreshing = true; + try { + await api.routes["POST /api/refresh"]({}); + } catch (error) { + if (!Number.isInteger(error?.status) || error.status >= 500) reportError(api, error); + const frame = `event: refresh-error\ndata: ${JSON.stringify({ error: clientErrorMessage(error) })}\n\n`; + for (const client of sseClients) client.write(frame); + } finally { + refreshing = false; + } + }, REFRESH_INTERVAL_MS); + refreshTimer.unref(); + return { url: `http://127.0.0.1:${port}/`, async close() { + clearInterval(refreshTimer); unsubscribe(); for (const client of sseClients) client.end(); sseClients.clear(); diff --git a/.github/extensions/open-pr-board/public/app.js b/.github/extensions/open-pr-board/public/app.js index 1ae8cd60ca..ebb516ae87 100644 --- a/.github/extensions/open-pr-board/public/app.js +++ b/.github/extensions/open-pr-board/public/app.js @@ -897,6 +897,7 @@ function applySnapshot(snapshot) { const events = new EventSource(`/events?token=${encodeURIComponent(TOKEN)}`); events.addEventListener("board", (event) => applySnapshot(JSON.parse(event.data))); +events.addEventListener("refresh-error", (event) => toast(JSON.parse(event.data).error, { error: true })); events.addEventListener("error", () => { el.summary.textContent = "Connection lost — reopen the canvas to reconnect."; }); diff --git a/eng/board-periodic-refresh.test.mjs b/eng/board-periodic-refresh.test.mjs new file mode 100644 index 0000000000..fa2ff4a651 --- /dev/null +++ b/eng/board-periodic-refresh.test.mjs @@ -0,0 +1,206 @@ +import assert from "node:assert/strict"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { setImmediate } from "node:timers/promises"; +import vm from "node:vm"; +import { test } from "node:test"; + +const INTERVAL = 15 * 60 * 1000; +const repo = "github/awesome-copilot"; + +function item(number) { + return { + number, title: `Example ${number}`, author: "contributor", + url: `https://github.com/${repo}/issues/${number}`, + labels: ["external-plugin", "ready-for-review"], + headRefOid: "first", updatedAt: "2026-10-01T00:00:00Z", + }; +} + +async function loadBoard(t, id) { + const workspacePath = await mkdtemp(path.join(os.tmpdir(), "board-polling-")); + t.after(() => rm(workspacePath, { recursive: true, force: true })); + const isPR = id === "open-pr-board"; + const column = isPR ? "needs-insight" : "needs-review"; + const review = { recommendation: isPR ? "approve" : "accept", rationale: "Ready." }; + const items = { + 1: { ...item(1), review, reviewStatus: "reviewed" }, + 2: { ...item(2), manualColumn: column }, + 3: { ...item(3), reviewStatus: "queued", queueId: "current-token", rereviewGuidance: "Check docs" }, + 4: { ...item(4), decision: { kind: isPR ? "close" : "reject", status: "confirmed" } }, + }; + const stateFile = path.join(workspacePath, "extensions", id, "state", + isPR ? "github__awesome-copilot.json" : "board.json"); + const initial = { + repo, items, updatedAt: "2026-10-01", lastRefreshedAt: "2026-10-01", + history: [{ number: 4, comment: "Previous action" }], + }; + await mkdir(path.dirname(stateFile), { recursive: true }); + await writeFile(stateFile, JSON.stringify(initial)); + + let canvas; + let calls = 0; + let sends = 0; + let fetchItems = async () => [1, 2, 3, 4, 5].map(item); + const logs = []; + const context = vm.createContext({ setTimeout }); + const sdk = { + createCanvas: (definition) => definition, + joinSession: async ({ canvases }) => { + [canvas] = canvases; + return { workspacePath, log: (message) => logs.push(message), send: () => { sends++; } }; + }, + CanvasError: class CanvasError extends Error { + constructor(code, message) { super(message); this.code = code; } + }, + }; + const url = new URL(`../.github/extensions/${id}/extension.mjs`, import.meta.url); + const module = new vm.SourceTextModule(await readFile(url, "utf8"), { + context, initializeImportMeta: (meta) => { meta.url = url.href; }, + }); + await module.link(async (specifier) => { + let exports; + if (specifier === "@github/copilot-sdk/extension") exports = sdk; + else if (specifier === "./lib/github.mjs") { + const list = () => { calls++; return fetchItems(); }; + // Any unexpected GitHub operation (especially a write) fails the test. + exports = { listOpenPullRequests: list, listBoardIssues: list }; + } else { + exports = await import(specifier.startsWith(".") ? new URL(specifier, url).href : specifier); + } + return new vm.SyntheticModule(Object.keys(exports), function () { + for (const [name, value] of Object.entries(exports)) this.setExport(name, value); + }, { context }); + }); + await module.evaluate(); + const ctx = { instanceId: "polling-board", input: {} }; + t.after(() => canvas.onClose(ctx)); + const action = (name, input = {}) => canvas.actions.find((entry) => entry.name === name) + .handler({ ...ctx, input }); + return { + action, open: () => canvas.open(ctx), close: () => canvas.onClose(ctx), + calls: () => calls, sends: () => sends, logs, initial, stateFile, + setFetch: (fn) => { fetchItems = fn; }, + }; +} + +async function readFrame(reader, eventName) { + let text = ""; + while (!text.includes(`event: ${eventName}\n`)) { + const { value, done } = await reader.read(); + assert.equal(done, false, `Expected ${eventName} event`); + text += new TextDecoder().decode(value); + } + return text; +} + +for (const id of ["open-pr-board", "external-plugin-review-board"]) { + test(`${id}: scheduled refresh preserves state, shares manual fetches, recovers, and stops on close`, + { timeout: 15000 }, async (t) => { + t.mock.timers.enable({ apis: ["setInterval"] }); + const board = await loadBoard(t, id); + const opened = await board.open(); + await board.open(); // Focusing an existing canvas must not create another timer. + const html = await (await fetch(opened.url)).text(); + const token = html.match(/ controller.abort()); + await readFrame(reader, "board"); + + t.mock.timers.tick(INTERVAL - 1); + assert.equal(board.calls(), 0, "No fetch before fifteen minutes"); + t.mock.timers.tick(1); + // Joining via the existing action waits for the scheduled fetch, not another fetch. + await board.action("refresh"); + assert.equal(board.calls(), 1); + const saved = JSON.parse(await readFile(board.stateFile, "utf8")); + assert.ok(saved.items["5"], "New matching item is discovered"); + assert.deepEqual(saved.history, board.initial.history); + for (const [number, before] of Object.entries(board.initial.items)) { + for (const field of ["review", "reviewStatus", "queueId", "manualColumn", "decision", "rereviewGuidance"]) { + assert.deepEqual(saved.items[number][field], before[field], `${number}: ${field}`); + } + } + assert.match(await readFrame(reader, "board"), /Example 5/); + await setImmediate(); + + let finish; + board.setFetch(() => new Promise((resolve) => { finish = resolve; })); + const manual = board.action("refresh"); + t.mock.timers.tick(INTERVAL); + await setImmediate(); + t.mock.timers.tick(INTERVAL); + assert.equal(board.calls(), 2, "Manual and scheduled refreshes share one fetch"); + await board.action("move_item", { number: 2, column: id === "open-pr-board" ? "close" : "probably-reject" }); + await board.action("record_review", { reviews: [{ + number: 3, queueId: "current-token", + recommendation: id === "open-pr-board" ? "approve" : "accept", + rationale: "Finished while fetching.", + }] }); + finish([1, 2, 3, 4, 5, 6].map(item)); + await manual; + await setImmediate(); + const after = JSON.parse(await readFile(board.stateFile, "utf8")); + assert.ok(after.items["6"]); + assert.equal(after.items["2"].manualColumn, id === "open-pr-board" ? "close" : "probably-reject", + "A user action during the fetch is not overwritten"); + assert.equal(after.items["3"].review.rationale, "Finished while fetching.", + "An in-flight review can finish during refresh without losing its result"); + assert.equal(after.items["3"].reviewStatus, "reviewed"); + + board.setFetch(async () => { throw new Error("GitHub unavailable"); }); + t.mock.timers.tick(INTERVAL); + assert.match(await readFrame(reader, "refresh-error"), /GitHub unavailable/); + assert.deepEqual(JSON.parse(await readFile(board.stateFile, "utf8")), after, + "A failed refresh leaves good data intact"); + assert.ok(board.logs.some((message) => message.includes("GitHub unavailable"))); + await setImmediate(); + + board.setFetch(async () => [1, 2, 3, 4, 5, 6, 7].map(item)); + t.mock.timers.tick(INTERVAL); + await board.action("refresh"); + assert.ok(JSON.parse(await readFile(board.stateFile, "utf8")).items["7"]); + assert.equal(board.calls(), 4, "Polling continues after an error"); + assert.equal(board.sends(), 0, "Polling never starts AI reviews"); + + controller.abort(); + await board.close(); + t.mock.timers.tick(INTERVAL * 2); + assert.equal(board.calls(), 4, "Closing the canvas cancels its timer"); + await board.open(); + t.mock.timers.tick(INTERVAL); + await board.action("refresh"); + assert.equal(board.calls(), 5, "Reopening starts exactly one new timer"); + await setImmediate(); + board.setFetch(() => new Promise((resolve) => { finish = resolve; })); + t.mock.timers.tick(INTERVAL); + const closingRefresh = board.action("refresh"); + await setImmediate(); + await board.close(); + t.mock.timers.tick(INTERVAL * 2); + assert.equal(board.calls(), 6, "Closing during a fetch prevents later scheduled fetches"); + finish([1, 2, 3, 4, 5, 6, 7].map(item)); + await closingRefresh; + }); + + test(`${id}: polling errors use the existing toast without changing the displayed board`, async () => { + const source = await readFile(new URL(`../.github/extensions/${id}/public/app.js`, import.meta.url), "utf8"); + const handler = source.split("\n").find((line) => line.includes('events.addEventListener("refresh-error"')); + const handlers = new Map(); + const messages = []; + const context = vm.createContext({ + events: { addEventListener: (name, fn) => handlers.set(name, fn) }, + toast: (message, options) => messages.push({ message, error: options.error }), + board: { items: [1, 2] }, selected: 1, selectedNumber: 1, + }); + vm.runInContext(handler, context); + handlers.get("refresh-error")({ data: JSON.stringify({ error: "GitHub unavailable" }) }); + assert.deepEqual(messages, [{ message: "GitHub unavailable", error: true }]); + assert.deepEqual(context.board.items, [1, 2]); + assert.equal(context.selected, 1); + assert.equal(context.selectedNumber, 1); + }); +} diff --git a/eng/board-preview.test.mjs b/eng/board-preview.test.mjs index 77b92a8103..c874922e17 100644 --- a/eng/board-preview.test.mjs +++ b/eng/board-preview.test.mjs @@ -77,12 +77,47 @@ async function createUI(boardName, request = async () => ({ return context.decide(kind); } return { - context, nodes, drawer, listeners, messages, select, decide, + context, nodes, drawer, listeners, messages, select, decide, source, selected: () => vm.runInContext(isPR ? "selectedNumber" : "selected", context), }; } for (const boardName of ["open-pr-board", "external-plugin-review-board"]) { + test(`${boardName}: live refresh retains selection, drafts, tabs, and in-flight action guards`, async () => { + const ui = await createUI(boardName); + const isPR = boardName === "open-pr-board"; + vm.runInContext(` + stagedComments.set(1, "Draft feedback"); + activeTab = "comments"; + bulkBusy = true; + ${isPR ? "decisionsInFlight.add(1)" : "decisionBusy = true"}; + `, ui.context); + ui.nodes.get("decision-comment").value = "Draft feedback"; + const snapshot = { + items: [1, 2, 3].map((number) => ({ number, reviewEligible: true })), + columns: [], + }; + if (isPR) ui.context.applySnapshot(snapshot); + else { + const handlers = new Map(); + ui.context.EventSource = class { + addEventListener(name, handler) { handlers.set(name, handler); } + }; + const start = ui.source.indexOf("function connect()"); + vm.runInContext(ui.source.slice(start, ui.source.indexOf("\nconnect();", start)), ui.context); + ui.context.connect(); + handlers.get("board")({ data: JSON.stringify(snapshot) }); + } + assert.equal(ui.selected(), 1); + assert.equal(ui.drawer.classList.contains("open"), true); + assert.equal(ui.nodes.get("decision-comment").value, "Draft feedback"); + assert.equal(vm.runInContext("stagedComments.get(1)", ui.context), "Draft feedback"); + assert.equal(vm.runInContext("activeTab", ui.context), "comments"); + assert.equal(vm.runInContext("bulkBusy", ui.context), true); + assert.equal(vm.runInContext(isPR ? "decisionsInFlight.has(1)" : "decisionBusy", ui.context), true); + assert.equal(vm.runInContext("board.items.length", ui.context), 3); + }); + test(`${boardName}: clicking inside retains the preview; clicking outside closes it`, async () => { const ui = await createUI(boardName); const { handler, capture } = ui.listeners.get("pointerdown"); From cedca119df1f4648a78a05ee43f123698eb59496 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 27 Oct 2026 12:07:41 +1100 Subject: [PATCH 2/9] fix(boards): keep refreshed previews and approvals commit-safe Pin approvals to the displayed SHA and reject stale AI bulk reviews. Reload changed PR previews and invalidate plugin issue details after refresh without losing drafts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../external-plugin-review-board/README.md | 2 + .../public/app.js | 14 +++- .github/extensions/open-pr-board/README.md | 6 +- .../extensions/open-pr-board/extension.mjs | 5 +- .../open-pr-board/lib/decisions.mjs | 7 +- .../extensions/open-pr-board/lib/github.mjs | 1 + .../extensions/open-pr-board/public/app.js | 30 ++++++++- eng/board-bulk-actions.test.mjs | 12 ++++ eng/board-preview.test.mjs | 44 +++++++++++- eng/open-pr-board.test.mjs | 67 ++++++++++++++++++- 10 files changed, 175 insertions(+), 13 deletions(-) diff --git a/.github/extensions/external-plugin-review-board/README.md b/.github/extensions/external-plugin-review-board/README.md index ea21ac4855..21f342a962 100644 --- a/.github/extensions/external-plugin-review-board/README.md +++ b/.github/extensions/external-plugin-review-board/README.md @@ -42,6 +42,8 @@ Open it by asking Copilot to "open the plugin review board" (canvas id `external Fast-track never overrides an existing review or a bucket you chose; drag a card to *Unreviewed* to opt it out and have it reviewed normally. Cards show *First party* / *Third party* pills from the intake labels. - **Details panel** shows the AI review, the rendered issue body, and all comments. + Successful refreshes invalidate cached issue details and reload the selected body/comments + tab without changing staged feedback. Clicking outside the panel or successfully submitting an item action closes it; failed actions leave it open so you can correct or retry them. - **Quick actions** post `/approve` (with an optional note) or `/reject ` straight to the issue diff --git a/.github/extensions/external-plugin-review-board/public/app.js b/.github/extensions/external-plugin-review-board/public/app.js index 0a8a7f6f06..81d54053da 100644 --- a/.github/extensions/external-plugin-review-board/public/app.js +++ b/.github/extensions/external-plugin-review-board/public/app.js @@ -44,6 +44,7 @@ let filter = ""; let selected = null; let activeTab = "review"; const details = new Map(); +let detailGeneration = 0; const pendingConfirm = { kind: null, timer: null }; const stagedComments = new Map(); let bulkBusy = false; @@ -571,12 +572,15 @@ function renderComment(comment) { async function loadDetail(number, { force = false } = {}) { if (!force && details.has(number) && !details.get(number).error) return; + const generation = detailGeneration; details.delete(number); if (selected === number) renderDrawer(); try { const data = await api(force ? "POST" : "GET", `/api/issue/${number}`); + if (generation !== detailGeneration) return; details.set(number, data); } catch (error) { + if (generation !== detailGeneration) return; details.set(number, { error: error.message }); } if (selected === number) renderDrawer(); @@ -773,6 +777,7 @@ ui.refresh.addEventListener("click", () => withBusy(ui.refresh, "Refreshing…", async () => { try { const result = await api("POST", "/api/refresh"); + ++detailGeneration; details.clear(); const parts = [`${result.total} open`]; if (result.added.length) parts.push(`${result.added.length} new`); @@ -883,9 +888,16 @@ document.addEventListener("keydown", (event) => { function connect() { const events = new EventSource(`/events?token=${encodeURIComponent(token)}`); events.addEventListener("board", (event) => { - board = JSON.parse(event.data); + const snapshot = JSON.parse(event.data); + const refreshed = board?.lastRefreshedAt !== snapshot.lastRefreshedAt; + board = snapshot; + if (refreshed) { + ++detailGeneration; + details.clear(); + } renderBoard(); if (selected != null) renderDrawer(); + if (refreshed && selected != null && activeTab !== "review") loadDetail(selected, { force: true }); }); events.addEventListener("refresh-error", (event) => toast(JSON.parse(event.data).error, { error: true })); events.addEventListener("error", () => { diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index 89d7fee51c..b758df9e42 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -46,8 +46,10 @@ authenticated `gh` CLI. - **GitHub actions** — submit feedback, request changes, close a PR, or approve it and enable auto-merge with a merge commit. Each action needs two clicks to confirm and is also recorded in local board history. Approval is pinned to the board's commit - SHA, and auto-merge requires that same head commit. If the head changes, refresh - and review the new commit before approving again. + SHA displayed by the client, and auto-merge requires that same head commit. + A changed head automatically reloads an open preview; approval is unavailable until + that preview has loaded. Bulk approval skips stale AI reviews. If the head changes, + review the new commit before approving again. Only one GitHub action can run per PR at a time. - **Reset board** — after a second click to confirm, clear every local AI review, queued review, manual move, recorded action, and history entry, then reload open PRs. diff --git a/.github/extensions/open-pr-board/extension.mjs b/.github/extensions/open-pr-board/extension.mjs index fd717c7e3a..4cef91f0d0 100644 --- a/.github/extensions/open-pr-board/extension.mjs +++ b/.github/extensions/open-pr-board/extension.mjs @@ -215,7 +215,10 @@ function routesFor(instanceId, store) { "GET /api/pr/:number": ({ number }) => getPullRequest(store, number), "POST /api/pr/:number": ({ number }) => getPullRequest(store, number, { force: true }), "POST /api/decision": ({ body }) => - applyDecisionOnGitHub({ store, number: Number(body.number), kind: body.kind, comment: body.comment, refresh }), + applyDecisionOnGitHub({ + store, number: Number(body.number), kind: body.kind, comment: body.comment, + headRefOid: body.headRefOid, refresh, + }), }; // Board errors are user-facing messages (gh failures, "already actioned", ...); surface only the // message text, never the stack, and log the full error for debugging. diff --git a/.github/extensions/open-pr-board/lib/decisions.mjs b/.github/extensions/open-pr-board/lib/decisions.mjs index 6bbae16fc5..c80bb57bed 100644 --- a/.github/extensions/open-pr-board/lib/decisions.mjs +++ b/.github/extensions/open-pr-board/lib/decisions.mjs @@ -4,7 +4,7 @@ const DECISION_KINDS = ["approve", "request-changes", "comment", "close"]; const MAX_COMMENT_LENGTH = 60000; const decisionsInFlight = new Set(); -export async function applyDecisionOnGitHub({ store, number, kind, comment, refresh, github = gh }) { +export async function applyDecisionOnGitHub({ store, number, kind, comment, headRefOid, refresh, github = gh }) { if (!DECISION_KINDS.includes(kind)) throw new Error(`Unknown decision "${kind}".`); const key = `${store.repo}#${number}`; if (decisionsInFlight.has(key)) throw new Error(`#${number} already has a GitHub action in progress.`); @@ -21,7 +21,10 @@ export async function applyDecisionOnGitHub({ store, number, kind, comment, refr let result; if (kind === "approve") { - result = await github.approveAndEnableAutoMerge(store.repo, number, { body, headRefOid: item.headRefOid }); + if (!headRefOid || headRefOid !== item.headRefOid) { + throw new Error("PR changed since it was displayed; refresh and review it again before approving."); + } + result = await github.approveAndEnableAutoMerge(store.repo, number, { body, headRefOid }); } else if (kind === "close") { result = await github.closePullRequest(store.repo, number, { body }); } else { diff --git a/.github/extensions/open-pr-board/lib/github.mjs b/.github/extensions/open-pr-board/lib/github.mjs index 6f1f9d5644..241b0493d7 100644 --- a/.github/extensions/open-pr-board/lib/github.mjs +++ b/.github/extensions/open-pr-board/lib/github.mjs @@ -242,6 +242,7 @@ export async function getPullRequestDetail(repo, number) { number: pr.number, title: pr.title, state: pr.state, + headRefOid: pr.head?.sha, merged: Boolean(pr.merged), mergeableState: pr.mergeable_state ?? null, url: pr.html_url, diff --git a/.github/extensions/open-pr-board/public/app.js b/.github/extensions/open-pr-board/public/app.js index ebb516ae87..9935b9c4bc 100644 --- a/.github/extensions/open-pr-board/public/app.js +++ b/.github/extensions/open-pr-board/public/app.js @@ -36,6 +36,7 @@ let board = null; let selectedNumber = null; let detail = null; let detailError = null; +let previewHeadRefOid = null; let activeTab = "review"; let filterText = ""; let pendingConfirm = null; @@ -399,9 +400,15 @@ async function bulkAction(columnId, kind) { if (!current || current.headRefOid !== item.headRefOid) { throw new Error("PR moved, changed, or was already actioned; review it again."); } + if (kind === "approve" && current.reviewStale) { + throw new Error("PR changed since its AI review; review the current commit before approving."); + } decisionsInFlight.add(item.number); const result = await api("/api/decision", { - method: "POST", body: { number: item.number, kind, comment: item.comment }, + method: "POST", body: { + number: item.number, kind, comment: item.comment, + ...(kind === "approve" ? { headRefOid: item.headRefOid } : {}), + }, }); completed++; if (selectedNumber === item.number) closeDrawer(); @@ -638,13 +645,19 @@ function renderDrawerHeader() { async function loadDetail(number, { force = false } = {}) { if (selectedNumber !== number) return; const generation = ++detailGeneration; + const headRefOid = currentItem()?.headRefOid; + previewHeadRefOid = null; detail = null; detailError = null; renderDrawerBody(); try { const response = await api(`/api/pr/${number}`, force ? { method: "POST" } : {}); if (generation !== detailGeneration || selectedNumber !== number) return; + if (response.headRefOid && response.headRefOid !== headRefOid) { + throw new Error("PR changed while loading its preview; refresh and review the current commit."); + } detail = response; + previewHeadRefOid = response.headRefOid ?? null; } catch (error) { if (generation !== detailGeneration || selectedNumber !== number) return; detailError = error.message; @@ -669,6 +682,7 @@ function openDrawer(number) { el.comment.value = stagedComment(item); detail = null; detailError = null; + previewHeadRefOid = null; } renderDrawerHeader(); renderDrawerBody(); @@ -682,6 +696,7 @@ function closeDrawer() { selectedNumber = null; detail = null; detailError = null; + previewHeadRefOid = null; clearTimeout(pendingConfirm?.timer); pendingConfirm = null; el.drawer.classList.remove("open"); @@ -721,6 +736,10 @@ function armConfirm(button, kind) { async function submitDecision(button, kind) { const item = currentItem(); if (!item || bulkBusy || decisionsInFlight.has(item.number)) return; + if (kind === "approve" && (!previewHeadRefOid || previewHeadRefOid !== item.headRefOid)) { + toast("PR preview is loading or changed; review the current commit before approving.", { error: true }); + return; + } if (!armConfirm(button, kind)) return; clearTimeout(pendingConfirm.timer); pendingConfirm = null; @@ -730,7 +749,10 @@ async function submitDecision(button, kind) { decisionsInFlight.add(item.number); renderDrawerHeader(); const result = await run(button, () => - api("/api/decision", { method: "POST", body: { number: item.number, kind, comment } }), + api("/api/decision", { method: "POST", body: { + number: item.number, kind, comment, + ...(kind === "approve" ? { headRefOid: previewHeadRefOid } : {}), + } }), ); decisionsInFlight.delete(item.number); if (selectedNumber === item.number) renderDrawerHeader(); @@ -884,13 +906,15 @@ el.comment.addEventListener("input", () => { }); function applySnapshot(snapshot) { + const previousHead = currentItem()?.headRefOid; board = snapshot; renderBoard(); if (selectedNumber !== null) { if (!currentItem()) closeDrawer(); else { renderDrawerHeader(); - renderDrawerBody(); + if (previousHead !== currentItem().headRefOid) loadDetail(selectedNumber, { force: true }); + else renderDrawerBody(); } } } diff --git a/eng/board-bulk-actions.test.mjs b/eng/board-bulk-actions.test.mjs index 195b911da5..cf8e60f4bd 100644 --- a/eng/board-bulk-actions.test.mjs +++ b/eng/board-bulk-actions.test.mjs @@ -171,7 +171,19 @@ test("PR changed during a batch is skipped, not approved at a new commit", async const ui = await createUI("open-pr-board", items, { request: async () => { items[1].headRefOid = "second"; return {}; }, }); + await ui.context.bulkAction("approve", "approve"); assert.equal(ui.calls.length, 1); assert.match(ui.messages.at(-1).text, /#2: PR moved, changed/); }); + +test("bulk approvals send the displayed commit and refuse stale AI reviews", async () => { + const ui = await createUI("open-pr-board", [ + { number: 1, column: "approve", headRefOid: "displayed" }, + { number: 2, column: "approve", headRefOid: "new", reviewStale: true }, + ]); + await ui.context.bulkAction("approve", "approve"); + assert.equal(ui.calls.length, 1); + assert.equal(ui.calls[0][1].body.headRefOid, "displayed"); + assert.match(ui.messages.at(-1).text, /changed since its AI review/); +}); diff --git a/eng/board-preview.test.mjs b/eng/board-preview.test.mjs index c874922e17..ae83ec88f2 100644 --- a/eng/board-preview.test.mjs +++ b/eng/board-preview.test.mjs @@ -62,7 +62,7 @@ async function createUI(boardName, request = async () => ({ nodes.get("board").inert = isPR; if (isPR) { vm.runInContext(`drawerOriginNumber = ${number}`, context); - vm.runInContext("el.toolbar.inert = true", context); + vm.runInContext('el.toolbar.inert = true; previewHeadRefOid = "first"', context); } nodes.get("decision-comment").value = "Feedback"; } @@ -150,6 +150,48 @@ for (const boardName of ["open-pr-board", "external-plugin-review-board"]) { }); } + test("plugin polling invalidates cached details, reloads the selected tab, and rejects pre-refresh responses", async () => { + const requests = []; + const pending = new Map(); + const ui = await createUI("external-plugin-review-board", (...args) => { + requests.push(args); + return new Promise((resolve) => pending.set(args[1], resolve)); + }); + vm.runInContext(` + board.lastRefreshedAt = "before"; + activeTab = "comments"; + details.set(1, { bodyHtml: "old", comments: [] }); + details.set(3, { bodyHtml: "old unselected", comments: [] }); + stagedComments.set(1, "Keep my feedback"); + `, ui.context); + ui.nodes.get("decision-comment").value = "Keep my feedback"; + const stale = ui.context.loadDetail(2, { force: true }); + const handlers = new Map(); + ui.context.EventSource = class { + addEventListener(name, handler) { handlers.set(name, handler); } + }; + const start = ui.source.indexOf("function connect()"); + vm.runInContext(ui.source.slice(start, ui.source.indexOf("\nconnect();", start)), ui.context); + ui.context.connect(); + const snapshot = { lastRefreshedAt: "after", items: [{ number: 1 }], columns: [] }; + handlers.get("board")({ data: JSON.stringify(snapshot) }); + assert.equal(vm.runInContext("details.size", ui.context), 0); + assert.deepEqual(requests.map((args) => args.slice(0, 2)), [ + ["POST", "/api/issue/2"], ["POST", "/api/issue/1"], + ]); + pending.get("/api/issue/2")({ bodyHtml: "stale response", comments: [] }); + pending.get("/api/issue/1")({ bodyHtml: "new body", comments: [{ bodyHtml: "intake completed" }] }); + await stale; + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(vm.runInContext("details.has(2)", ui.context), false); + assert.equal(vm.runInContext("details.get(1).bodyHtml", ui.context), "new body"); + assert.equal(ui.selected(), 1); + assert.equal(ui.nodes.get("decision-comment").value, "Keep my feedback"); + assert.equal(vm.runInContext("stagedComments.get(1)", ui.context), "Keep my feedback"); + handlers.get("board")({ data: JSON.stringify(snapshot) }); + assert.equal(requests.length, 2, "Unchanged refresh timestamp does not reload details on local edits"); + }); + test(`${boardName}: a failed action leaves the preview open`, async () => { const ui = await createUI(boardName, async () => { throw new Error("Offline"); }); await ui.decide(); diff --git a/eng/open-pr-board.test.mjs b/eng/open-pr-board.test.mjs index bf914b4e09..3c4c0b18e5 100644 --- a/eng/open-pr-board.test.mjs +++ b/eng/open-pr-board.test.mjs @@ -32,7 +32,7 @@ test("approval passes the board's commit SHA to the GitHub helper", async (t) => const store = await createStore(t); let options; await applyDecisionOnGitHub({ - store, number: 1, kind: "approve", comment: "Reviewed.", refresh: async () => {}, + store, number: 1, kind: "approve", comment: "Reviewed.", headRefOid: "first", refresh: async () => {}, github: { approveAndEnableAutoMerge: async (repo, number, input) => { options = input; @@ -43,6 +43,20 @@ test("approval passes the board's commit SHA to the GitHub helper", async (t) => assert.deepEqual(options, { body: "Reviewed.", headRefOid: "first" }); }); +test("approval rejects a missing or previously displayed SHA after a scheduled sync without writing", async (t) => { + const store = await createStore(t); + await store.syncPullRequests([pr("second")]); + let writes = 0; + for (const headRefOid of [undefined, "", "first"]) { + await assert.rejects(applyDecisionOnGitHub({ + store, number: 1, kind: "approve", headRefOid, refresh: async () => {}, + github: { approveAndEnableAutoMerge: async () => { writes++; } }, + }), /changed since it was displayed/); + } + assert.equal(writes, 0); + assert.equal(store.getItem(1).decision, null); +}); + test("approval and auto-merge are pinned to the reviewed commit", async () => { const headRefOid = "a".repeat(40); const calls = []; @@ -322,16 +336,63 @@ async function createUI() { select(number) { selectedNumber = number; board = { columns: [], items: [1, 2].map(number => ({ - number, title: "Example", labels: [], createdAt: "2026-10-01", column: "unreviewed", + number, title: "Example", labels: [], createdAt: "2026-10-01", column: "unreviewed", headRefOid: "first", })) }; }, - state: () => ({ detail, detailError, activeTab }), + state: () => ({ detail, detailError, activeTab, previewHeadRefOid }), render: renderDrawerBody, }; `, context); return { context, ui: context.ui, nodes, tabs, document }; } +test("a refreshed PR head reloads the preview and blocks approval until the new detail arrives", async () => { + const { context, ui, nodes } = await createUI(); + const requests = []; + const messages = []; + context.notify = (message) => messages.push(message); + vm.runInContext("toast = notify", context); + ui.select(1); + context.nextRequest = deferred(); + const initial = ui.loadDetail(1); + context.nextRequest.resolve({ headRefOid: "first", files: [{ filename: "old.js" }] }); + await initial; + assert.equal(ui.state().previewHeadRefOid, "first"); + nodes.get("decision-comment").value = "Keep this feedback"; + context.nextRequest = deferred(); + const pending = context.nextRequest; + context.applySnapshot({ + columns: [], items: [{ number: 1, title: "New head", labels: [], headRefOid: "second" }], + }); + assert.equal(ui.state().detail, null); + assert.equal(ui.state().previewHeadRefOid, null); + await ui.submitDecision(nodes.get("decision-approve"), "approve"); + assert.match(messages[0], /preview is loading or changed/); + assert.equal(nodes.get("decision-comment").value, "Keep this feedback"); + pending.resolve({ headRefOid: "second", files: [{ filename: "new.js" }] }); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(ui.state().previewHeadRefOid, "second"); + assert.equal(ui.state().detail.files[0].filename, "new.js"); + context.request = async (...args) => { requests.push(args); throw new Error("stop after request"); }; + vm.runInContext("api = request", context); + await ui.submitDecision(nodes.get("decision-approve"), "approve"); + await ui.submitDecision(nodes.get("decision-approve"), "approve"); + assert.equal(requests.length, 1); + assert.equal(requests[0][1].body.headRefOid, "second"); +}); + +test("a cached preview from another commit cannot authorize approval", async () => { + const { context, ui } = await createUI(); + ui.select(1); + context.nextRequest = deferred(); + const pending = ui.loadDetail(1); + context.nextRequest.resolve({ headRefOid: "older", files: [] }); + await pending; + assert.equal(ui.state().previewHeadRefOid, null); + assert.equal(ui.state().detail, null); + assert.match(ui.state().detailError, /changed while loading/); +}); + test("older PR detail responses and errors cannot overwrite a newer selected PR", async () => { const { context, ui } = await createUI(); for (const fail of [false, true]) { From 4e8cb448520f70e64ccfe874ad061c42d59cd7b5 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 27 Oct 2026 12:29:24 +1100 Subject: [PATCH 3/9] fix(boards): retain approval commit provenance across refreshes Capture queued-review and manual-approval heads so bulk actions cannot carry old choices onto new commits. Skip shared-server polling when no refresh handler exists. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/extensions/open-pr-board/README.md | 3 ++ .../extensions/open-pr-board/lib/server.mjs | 2 +- .../extensions/open-pr-board/lib/state.mjs | 12 +++++- .../extensions/open-pr-board/public/app.js | 4 +- eng/board-bulk-actions.test.mjs | 1 + eng/board-periodic-refresh.test.mjs | 19 +++++++++ eng/open-pr-board.test.mjs | 39 +++++++++++++++++++ 7 files changed, 76 insertions(+), 4 deletions(-) diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index b758df9e42..6444528e90 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -50,6 +50,9 @@ authenticated `gh` CLI. A changed head automatically reloads an open preview; approval is unavailable until that preview has loaded. Bulk approval skips stale AI reviews. If the head changes, review the new commit before approving again. + Queued reviews and manual approval moves retain their original commit SHA, so a + refresh cannot carry those approval choices onto newer commits. Legacy reviews or + approval moves without a recorded SHA must be reviewed or explicitly moved again. Only one GitHub action can run per PR at a time. - **Reset board** — after a second click to confirm, clear every local AI review, queued review, manual move, recorded action, and history entry, then reload open PRs. diff --git a/.github/extensions/open-pr-board/lib/server.mjs b/.github/extensions/open-pr-board/lib/server.mjs index 4399f87fcd..c471d61395 100644 --- a/.github/extensions/open-pr-board/lib/server.mjs +++ b/.github/extensions/open-pr-board/lib/server.mjs @@ -171,7 +171,7 @@ export async function startBoardServer({ publicDir, api }) { let refreshing = false; const refreshTimer = setInterval(async () => { - if (refreshing) return; + if (refreshing || typeof api.routes["POST /api/refresh"] !== "function") return; refreshing = true; try { await api.routes["POST /api/refresh"]({}); diff --git a/.github/extensions/open-pr-board/lib/state.mjs b/.github/extensions/open-pr-board/lib/state.mjs index 7a0f3f4983..d010f09546 100644 --- a/.github/extensions/open-pr-board/lib/state.mjs +++ b/.github/extensions/open-pr-board/lib/state.mjs @@ -29,6 +29,7 @@ export function effectiveColumn(item) { /** A review is stale when the pull request changed after the review was recorded. */ function reviewIsStale(item) { + if (item.review && (!item.review.headRefOid || item.review.headRefOid !== item.headRefOid)) return true; if (!item.review?.reviewedAt || !item.updatedAt) return false; return new Date(item.updatedAt).getTime() > new Date(item.review.reviewedAt).getTime() + 1000; } @@ -118,6 +119,8 @@ export class BoardStore { ...item, column: effectiveColumn(item), reviewStale: reviewIsStale(item), + approvalStale: item.manualColumn === "approve" && + (!item.manualHeadRefOid || item.manualHeadRefOid !== item.headRefOid), })) .sort((a, b) => b.number - a.number); const counts = {}; @@ -189,9 +192,13 @@ export class BoardStore { for (const number of numbers) { const item = this.getItem(number); if (!item) continue; - previous.push({ number: item.number, reviewStatus: item.reviewStatus, queueId: item.queueId }); + previous.push({ + number: item.number, reviewStatus: item.reviewStatus, queueId: item.queueId, + queueHeadRefOid: item.queueHeadRefOid, + }); item.reviewStatus = "queued"; item.queueId = randomBytes(8).toString("hex"); + item.queueHeadRefOid = item.headRefOid ?? null; queued.push({ number: item.number, queueId: item.queueId }); } await this.commit(); @@ -204,6 +211,7 @@ export class BoardStore { if (!item) continue; item.reviewStatus = entry.reviewStatus; item.queueId = entry.queueId; + item.queueHeadRefOid = entry.queueHeadRefOid; } return this.commit(); } @@ -221,6 +229,7 @@ export class BoardStore { item.review = { recommendation, ...fields, + headRefOid: item.queueHeadRefOid ?? null, reviewedAt: new Date().toISOString(), }; item.reviewStatus = "reviewed"; @@ -249,6 +258,7 @@ export class BoardStore { item.manualColumn = null; } else { item.manualColumn = column; + item.manualHeadRefOid = item.headRefOid ?? null; if (item.reviewStatus === "queued") { item.reviewStatus = item.review ? "reviewed" : "none"; item.queueId = null; diff --git a/.github/extensions/open-pr-board/public/app.js b/.github/extensions/open-pr-board/public/app.js index 9935b9c4bc..0d537f3c91 100644 --- a/.github/extensions/open-pr-board/public/app.js +++ b/.github/extensions/open-pr-board/public/app.js @@ -400,8 +400,8 @@ async function bulkAction(columnId, kind) { if (!current || current.headRefOid !== item.headRefOid) { throw new Error("PR moved, changed, or was already actioned; review it again."); } - if (kind === "approve" && current.reviewStale) { - throw new Error("PR changed since its AI review; review the current commit before approving."); + if (kind === "approve" && (current.manualColumn === "approve" ? current.approvalStale : current.reviewStale)) { + throw new Error("PR changed since its AI review or manual approval choice; review the current commit before approving."); } decisionsInFlight.add(item.number); const result = await api("/api/decision", { diff --git a/eng/board-bulk-actions.test.mjs b/eng/board-bulk-actions.test.mjs index cf8e60f4bd..94373e9b64 100644 --- a/eng/board-bulk-actions.test.mjs +++ b/eng/board-bulk-actions.test.mjs @@ -181,6 +181,7 @@ test("bulk approvals send the displayed commit and refuse stale AI reviews", asy const ui = await createUI("open-pr-board", [ { number: 1, column: "approve", headRefOid: "displayed" }, { number: 2, column: "approve", headRefOid: "new", reviewStale: true }, + { number: 3, column: "approve", headRefOid: "new", manualColumn: "approve", approvalStale: true }, ]); await ui.context.bulkAction("approve", "approve"); assert.equal(ui.calls.length, 1); diff --git a/eng/board-periodic-refresh.test.mjs b/eng/board-periodic-refresh.test.mjs index fa2ff4a651..976f18beeb 100644 --- a/eng/board-periodic-refresh.test.mjs +++ b/eng/board-periodic-refresh.test.mjs @@ -5,10 +5,29 @@ import path from "node:path"; import { setImmediate } from "node:timers/promises"; import vm from "node:vm"; import { test } from "node:test"; +import { startBoardServer } from "../.github/extensions/open-pr-board/lib/server.mjs"; const INTERVAL = 15 * 60 * 1000; const repo = "github/awesome-copilot"; +test("shared server consumers without a refresh route never poll or report refresh errors", async (t) => { + t.mock.timers.enable({ apis: ["setInterval"] }); + const errors = []; + let unsubscribed = false; + const server = await startBoardServer({ + api: { + snapshot: () => ({}), routes: {}, + subscribe: () => () => { unsubscribed = true; }, + logger: { error: (...args) => errors.push(args) }, + }, + }); + t.after(() => server.close()); + t.mock.timers.tick(INTERVAL * 3); + await setImmediate(); + assert.deepEqual(errors, []); + assert.equal(unsubscribed, false); +}); + function item(number) { return { number, title: `Example ${number}`, author: "contributor", diff --git a/eng/open-pr-board.test.mjs b/eng/open-pr-board.test.mjs index 3c4c0b18e5..592e86a1ef 100644 --- a/eng/open-pr-board.test.mjs +++ b/eng/open-pr-board.test.mjs @@ -57,6 +57,45 @@ test("approval rejects a missing or previously displayed SHA after a scheduled s assert.equal(store.getItem(1).decision, null); }); +test("a review finishing after a head sync retains its queued commit and remains stale", async (t) => { + const store = await createStore(t); + const { queued } = await store.markQueued([1]); + await store.syncPullRequests([pr("second")]); + await store.recordReviews([{ + number: 1, queueId: queued[0].queueId, recommendation: "approve", + rationale: "Reviewed the first commit.", headRefOid: "second", + }]); + assert.equal(store.getItem(1).review.headRefOid, "first", "Payload cannot overwrite queue provenance"); + assert.equal(store.snapshot().items[0].reviewStale, true); + const next = await store.markQueued([1]); + await store.recordReviews([{ number: 1, queueId: next.queued[0].queueId, recommendation: "approve" }]); + assert.equal(store.getItem(1).review.headRefOid, "second"); + assert.equal(store.snapshot().items[0].reviewStale, false); +}); + +test("manual approval provenance survives refresh and is renewed only by an explicit move", async (t) => { + const store = await createStore(t); + await store.moveItem(1, "approve"); + assert.equal(store.snapshot().items[0].approvalStale, false); + await store.syncPullRequests([pr("second")]); + assert.equal(store.getItem(1).manualColumn, "approve"); + assert.equal(store.getItem(1).manualHeadRefOid, "first"); + assert.equal(store.snapshot().items[0].approvalStale, true); + await store.moveItem(1, "approve"); + assert.equal(store.snapshot().items[0].approvalStale, false); + const { previous } = await store.markQueued([1]); + await store.restoreQueueState(previous); + assert.equal(store.getItem(1).queueHeadRefOid, previous[0].queueHeadRefOid); +}); + +test("legacy approval choices without commit provenance cannot silently authorize bulk approval", async (t) => { + const store = await createStore(t); + store.getItem(1).review = { recommendation: "approve", reviewedAt: "2099-01-01" }; + assert.equal(store.snapshot().items[0].reviewStale, true); + store.getItem(1).manualColumn = "approve"; + assert.equal(store.snapshot().items[0].approvalStale, true); +}); + test("approval and auto-merge are pinned to the reviewed commit", async () => { const headRefOid = "a".repeat(40); const calls = []; From f9c05ffca7b0716fd1a0efc7fc738b34c6e7f932 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 27 Oct 2026 12:50:10 +1100 Subject: [PATCH 4/9] fix(pr-board): preserve heads across delayed actions and moves Carry displayed commit provenance in move requests, retain the original action head across GitHub waits, and retry failed previews on successful refresh. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/extensions/open-pr-board/README.md | 3 ++ .../extensions/open-pr-board/extension.mjs | 5 +- .../open-pr-board/lib/decisions.mjs | 3 +- .../extensions/open-pr-board/lib/state.mjs | 6 +-- .../extensions/open-pr-board/public/app.js | 12 +++-- eng/open-pr-board-drag.test.mjs | 3 +- eng/open-pr-board.test.mjs | 53 ++++++++++++++++++- 7 files changed, 73 insertions(+), 12 deletions(-) diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index 6444528e90..e5e9d31b08 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -53,6 +53,9 @@ authenticated `gh` CLI. Queued reviews and manual approval moves retain their original commit SHA, so a refresh cannot carry those approval choices onto newer commits. Legacy reviews or approval moves without a recorded SHA must be reviewed or explicitly moved again. + Moves carry the client's displayed SHA even if polling updates the board before the + request arrives. Delayed feedback actions retain their original SHA so new commits + still return to Unreviewed. A successful refresh retries a failed open preview. Only one GitHub action can run per PR at a time. - **Reset board** — after a second click to confirm, clear every local AI review, queued review, manual move, recorded action, and history entry, then reload open PRs. diff --git a/.github/extensions/open-pr-board/extension.mjs b/.github/extensions/open-pr-board/extension.mjs index 4cef91f0d0..b3ed05170e 100644 --- a/.github/extensions/open-pr-board/extension.mjs +++ b/.github/extensions/open-pr-board/extension.mjs @@ -211,7 +211,7 @@ function routesFor(instanceId, store) { "POST /api/review": ({ body }) => startReview({ store, numbers: body.numbers, instanceId, send: true }), "POST /api/rereview": ({ body }) => startRereview({ store, number: body.number, guidance: body.guidance, instanceId, send: true }), - "POST /api/move": ({ body }) => store.moveItem(Number(body.number), String(body.column)), + "POST /api/move": ({ body }) => store.moveItem(Number(body.number), String(body.column), body.headRefOid), "GET /api/pr/:number": ({ number }) => getPullRequest(store, number), "POST /api/pr/:number": ({ number }) => getPullRequest(store, number, { force: true }), "POST /api/decision": ({ body }) => @@ -346,10 +346,11 @@ session = await joinSession({ repo: repoProperty, number: { type: "integer" }, column: { type: "string", enum: ["unreviewed", ...RECOMMENDATIONS] }, + headRefOid: { type: "string", description: "Commit SHA displayed when choosing the bucket; required for a current manual approval choice." }, }, }, handler: wrap(async (ctx) => - (await storeForContext(ctx)).moveItem(ctx.input.number, ctx.input.column), + (await storeForContext(ctx)).moveItem(ctx.input.number, ctx.input.column, ctx.input.headRefOid), ), }, { diff --git a/.github/extensions/open-pr-board/lib/decisions.mjs b/.github/extensions/open-pr-board/lib/decisions.mjs index c80bb57bed..6266d07108 100644 --- a/.github/extensions/open-pr-board/lib/decisions.mjs +++ b/.github/extensions/open-pr-board/lib/decisions.mjs @@ -13,6 +13,7 @@ export async function applyDecisionOnGitHub({ store, number, kind, comment, head const item = store.getItem(number); if (!item) throw new Error(`Pull request #${number} is not on the board. Refresh first.`); if (item.decision) throw new Error(`#${number} was already actioned (${item.decision.kind}).`); + const actionHeadRefOid = item.headRefOid ?? null; const body = String(comment ?? "").trim().slice(0, MAX_COMMENT_LENGTH); if ((kind === "request-changes" || kind === "comment") && !body) { @@ -33,7 +34,7 @@ export async function applyDecisionOnGitHub({ store, number, kind, comment, head const warnings = result.warning ? [result.warning] : []; try { - await store.recordDecision(number, { kind, comment: body, status: result.status }); + await store.recordDecision(number, { kind, comment: body, status: result.status, headRefOid: actionHeadRefOid }); } catch (error) { warnings.push(`GitHub was updated, but the board could not record the action: ${error.message}`); } diff --git a/.github/extensions/open-pr-board/lib/state.mjs b/.github/extensions/open-pr-board/lib/state.mjs index d010f09546..31985f4c20 100644 --- a/.github/extensions/open-pr-board/lib/state.mjs +++ b/.github/extensions/open-pr-board/lib/state.mjs @@ -245,7 +245,7 @@ export class BoardStore { return results; } - async moveItem(number, column) { + async moveItem(number, column, headRefOid = null) { const item = this.getItem(number); if (!item) throw new Error(`Pull request #${number} is not on the board.`); if (!DROPPABLE.has(column)) throw new Error(`Column "${column}" cannot be set directly.`); @@ -258,7 +258,7 @@ export class BoardStore { item.manualColumn = null; } else { item.manualColumn = column; - item.manualHeadRefOid = item.headRefOid ?? null; + item.manualHeadRefOid = headRefOid; if (item.reviewStatus === "queued") { item.reviewStatus = item.review ? "reviewed" : "none"; item.queueId = null; @@ -275,7 +275,7 @@ export class BoardStore { comment: decision.comment ?? "", at: new Date().toISOString(), status: decision.status ?? null, - headRefOid: item.headRefOid ?? null, + headRefOid: decision.headRefOid === undefined ? item.headRefOid ?? null : decision.headRefOid, }; this.state.history.unshift({ number: item.number, diff --git a/.github/extensions/open-pr-board/public/app.js b/.github/extensions/open-pr-board/public/app.js index 0d537f3c91..45dea58814 100644 --- a/.github/extensions/open-pr-board/public/app.js +++ b/.github/extensions/open-pr-board/public/app.js @@ -256,6 +256,7 @@ function renderCard(item) { }, ondragstart: (event) => { event.dataTransfer.setData("text/plain", String(item.number)); + event.dataTransfer.setData("application/x-pr-head", item.headRefOid ?? ""); event.dataTransfer.effectAllowed = "move"; card.classList.add("dragging"); }, @@ -303,7 +304,9 @@ function renderBoard() { const number = Number(event.dataTransfer.getData("text/plain")); if (!Number.isInteger(number)) return; try { - await api("/api/move", { method: "POST", body: { number, column: column.id } }); + await api("/api/move", { method: "POST", body: { + number, column: column.id, headRefOid: event.dataTransfer.getData("application/x-pr-head") || null, + } }); } catch (error) { toast(error.message, { error: true }); } @@ -836,7 +839,7 @@ el.drawerColumn.addEventListener("change", async (event) => { const column = event.target.value; if (column === item.column) return; try { - await api("/api/move", { method: "POST", body: { number: item.number, column } }); + await api("/api/move", { method: "POST", body: { number: item.number, column, headRefOid: item.headRefOid } }); if (selectedNumber === item.number) closeDrawer(); } catch (error) { toast(error.message, { error: true }); @@ -907,13 +910,16 @@ el.comment.addEventListener("input", () => { function applySnapshot(snapshot) { const previousHead = currentItem()?.headRefOid; + const refreshed = board?.updatedAt !== snapshot.updatedAt; board = snapshot; renderBoard(); if (selectedNumber !== null) { if (!currentItem()) closeDrawer(); else { renderDrawerHeader(); - if (previousHead !== currentItem().headRefOid) loadDetail(selectedNumber, { force: true }); + if (previousHead !== currentItem().headRefOid || (refreshed && detailError)) { + loadDetail(selectedNumber, { force: true }); + } else renderDrawerBody(); } } diff --git a/eng/open-pr-board-drag.test.mjs b/eng/open-pr-board-drag.test.mjs index 769b8b0085..029868a4a0 100644 --- a/eng/open-pr-board-drag.test.mjs +++ b/eng/open-pr-board-drag.test.mjs @@ -55,11 +55,12 @@ test("PR cards explicitly enable native dragging; actioned cards explicitly disa test("dragging transfers the PR number for moving and clears its visual state on completion", () => { const context = createContext(); - const card = context.renderCard({ number: 42, title: "Example", author: "contributor" }); + const card = context.renderCard({ number: 42, title: "Example", author: "contributor", headRefOid: "displayed" }); const values = new Map(); const dataTransfer = { setData: (type, value) => values.set(type, value) }; card.listeners.dragstart({ dataTransfer }); assert.equal(values.get("text/plain"), "42"); + assert.equal(values.get("application/x-pr-head"), "displayed"); assert.equal(dataTransfer.effectAllowed, "move"); assert.equal(card.classList.contains("dragging"), true); card.listeners.dragend(); diff --git a/eng/open-pr-board.test.mjs b/eng/open-pr-board.test.mjs index 592e86a1ef..22fd528d75 100644 --- a/eng/open-pr-board.test.mjs +++ b/eng/open-pr-board.test.mjs @@ -75,13 +75,13 @@ test("a review finishing after a head sync retains its queued commit and remains test("manual approval provenance survives refresh and is renewed only by an explicit move", async (t) => { const store = await createStore(t); - await store.moveItem(1, "approve"); + await store.moveItem(1, "approve", "first"); assert.equal(store.snapshot().items[0].approvalStale, false); await store.syncPullRequests([pr("second")]); assert.equal(store.getItem(1).manualColumn, "approve"); assert.equal(store.getItem(1).manualHeadRefOid, "first"); assert.equal(store.snapshot().items[0].approvalStale, true); - await store.moveItem(1, "approve"); + await store.moveItem(1, "approve", "second"); assert.equal(store.snapshot().items[0].approvalStale, false); const { previous } = await store.markQueued([1]); await store.restoreQueueState(previous); @@ -96,6 +96,34 @@ test("legacy approval choices without commit provenance cannot silently authoriz assert.equal(store.snapshot().items[0].approvalStale, true); }); +test("sync before a delayed manual move retains the client's original commit", async (t) => { + const store = await createStore(t); + await store.syncPullRequests([pr("second")]); + await store.moveItem(1, "approve", "first"); + assert.equal(store.getItem(1).manualHeadRefOid, "first"); + assert.equal(store.snapshot().items[0].approvalStale, true); + await store.moveItem(1, "approve"); + assert.equal(store.snapshot().items[0].approvalStale, true, "Missing client provenance fails closed"); +}); + +for (const kind of ["comment", "request-changes"]) { + test(`a delayed ${kind} response retains the action head when polling discovers new commits`, async (t) => { + const store = await createStore(t); + const pending = deferred(); + const action = applyDecisionOnGitHub({ + store, number: 1, kind, comment: "Feedback on first", + github: { submitPullRequestReview: async () => { await pending.promise; return { status: "submitted" }; } }, + refresh: () => store.syncPullRequests([pr("second")]), + }); + await store.syncPullRequests([pr("second")]); + pending.resolve(); + await action; + assert.equal(store.getItem(1).decision, null); + assert.equal(store.snapshot().items[0].column, "unreviewed"); + assert.equal(store.state.history[0].kind, kind); + }); +} + test("approval and auto-merge are pinned to the reviewed commit", async () => { const headRefOid = "a".repeat(40); const calls = []; @@ -432,6 +460,27 @@ test("a cached preview from another commit cannot authorize approval", async () assert.match(ui.state().detailError, /changed while loading/); }); +test("a successful same-head refresh retries a failed preview without disturbing the draft", async () => { + const { context, ui, nodes } = await createUI(); + ui.select(1); + context.nextRequest = deferred(); + const failed = ui.loadDetail(1); + context.nextRequest.reject(new Error("Transient GitHub error")); + await failed; + assert.match(ui.state().detailError, /Transient/); + nodes.get("decision-comment").value = "Keep my feedback"; + context.nextRequest = deferred(); + context.applySnapshot({ + updatedAt: "new-refresh", columns: [], + items: [{ number: 1, title: "Example", labels: [], headRefOid: "first" }], + }); + assert.equal(ui.state().detailError, null); + context.nextRequest.resolve({ headRefOid: "first", files: [] }); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(ui.state().previewHeadRefOid, "first"); + assert.equal(nodes.get("decision-comment").value, "Keep my feedback"); +}); + test("older PR detail responses and errors cannot overwrite a newer selected PR", async () => { const { context, ui } = await createUI(); for (const fail of [false, true]) { From 93d1d11d097d372a21b207a2d74708d3f6bbc902 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 27 Oct 2026 13:16:17 +1100 Subject: [PATCH 5/9] fix(boards): reject stale feedback and out-of-order details Track the latest detail request per issue and require the client-displayed head before dispatching PR feedback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../external-plugin-review-board/README.md | 2 ++ .../public/app.js | 9 ++++-- .github/extensions/open-pr-board/README.md | 2 ++ .../open-pr-board/lib/decisions.mjs | 4 +++ .../extensions/open-pr-board/public/app.js | 4 +-- eng/board-bulk-actions.test.mjs | 4 +-- eng/board-preview.test.mjs | 31 +++++++++++++++++++ eng/open-pr-board.test.mjs | 21 +++++++++++-- 8 files changed, 68 insertions(+), 9 deletions(-) diff --git a/.github/extensions/external-plugin-review-board/README.md b/.github/extensions/external-plugin-review-board/README.md index 21f342a962..bf4aab5503 100644 --- a/.github/extensions/external-plugin-review-board/README.md +++ b/.github/extensions/external-plugin-review-board/README.md @@ -44,6 +44,8 @@ Open it by asking Copilot to "open the plugin review board" (canvas id `external - **Details panel** shows the AI review, the rendered issue body, and all comments. Successful refreshes invalidate cached issue details and reload the selected body/comments tab without changing staged feedback. + When detail requests overlap, only the latest request for each issue can update its + cache, so an older poll cannot overwrite comments loaded after posting feedback. Clicking outside the panel or successfully submitting an item action closes it; failed actions leave it open so you can correct or retry them. - **Quick actions** post `/approve` (with an optional note) or `/reject ` straight to the issue diff --git a/.github/extensions/external-plugin-review-board/public/app.js b/.github/extensions/external-plugin-review-board/public/app.js index 81d54053da..6205596e37 100644 --- a/.github/extensions/external-plugin-review-board/public/app.js +++ b/.github/extensions/external-plugin-review-board/public/app.js @@ -45,6 +45,7 @@ let selected = null; let activeTab = "review"; const details = new Map(); let detailGeneration = 0; +const detailRequests = new Map(); const pendingConfirm = { kind: null, timer: null }; const stagedComments = new Map(); let bulkBusy = false; @@ -573,15 +574,19 @@ function renderComment(comment) { async function loadDetail(number, { force = false } = {}) { if (!force && details.has(number) && !details.get(number).error) return; const generation = detailGeneration; + const request = {}; + detailRequests.set(number, request); details.delete(number); if (selected === number) renderDrawer(); try { const data = await api(force ? "POST" : "GET", `/api/issue/${number}`); - if (generation !== detailGeneration) return; + if (generation !== detailGeneration || detailRequests.get(number) !== request) return; details.set(number, data); } catch (error) { - if (generation !== detailGeneration) return; + if (generation !== detailGeneration || detailRequests.get(number) !== request) return; details.set(number, { error: error.message }); + } finally { + if (detailRequests.get(number) === request) detailRequests.delete(number); } if (selected === number) renderDrawer(); } diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index e5e9d31b08..a70a362de7 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -56,6 +56,8 @@ authenticated `gh` CLI. Moves carry the client's displayed SHA even if polling updates the board before the request arrives. Delayed feedback actions retain their original SHA so new commits still return to Unreviewed. A successful refresh retries a failed open preview. + Feedback requests also carry the displayed SHA and are rejected if polling has + already discovered a newer head before dispatch. Only one GitHub action can run per PR at a time. - **Reset board** — after a second click to confirm, clear every local AI review, queued review, manual move, recorded action, and history entry, then reload open PRs. diff --git a/.github/extensions/open-pr-board/lib/decisions.mjs b/.github/extensions/open-pr-board/lib/decisions.mjs index 6266d07108..2d610a62a3 100644 --- a/.github/extensions/open-pr-board/lib/decisions.mjs +++ b/.github/extensions/open-pr-board/lib/decisions.mjs @@ -14,6 +14,10 @@ export async function applyDecisionOnGitHub({ store, number, kind, comment, head if (!item) throw new Error(`Pull request #${number} is not on the board. Refresh first.`); if (item.decision) throw new Error(`#${number} was already actioned (${item.decision.kind}).`); const actionHeadRefOid = item.headRefOid ?? null; + if ((kind === "comment" || kind === "request-changes") && + (!headRefOid || headRefOid !== actionHeadRefOid)) { + throw new Error("PR changed since it was displayed; review the current commit before submitting feedback."); + } const body = String(comment ?? "").trim().slice(0, MAX_COMMENT_LENGTH); if ((kind === "request-changes" || kind === "comment") && !body) { diff --git a/.github/extensions/open-pr-board/public/app.js b/.github/extensions/open-pr-board/public/app.js index 45dea58814..bf068d031b 100644 --- a/.github/extensions/open-pr-board/public/app.js +++ b/.github/extensions/open-pr-board/public/app.js @@ -410,7 +410,7 @@ async function bulkAction(columnId, kind) { const result = await api("/api/decision", { method: "POST", body: { number: item.number, kind, comment: item.comment, - ...(kind === "approve" ? { headRefOid: item.headRefOid } : {}), + ...(kind !== "close" ? { headRefOid: item.headRefOid } : {}), }, }); completed++; @@ -754,7 +754,7 @@ async function submitDecision(button, kind) { const result = await run(button, () => api("/api/decision", { method: "POST", body: { number: item.number, kind, comment, - ...(kind === "approve" ? { headRefOid: previewHeadRefOid } : {}), + ...(kind !== "close" ? { headRefOid: kind === "approve" ? previewHeadRefOid : item.headRefOid } : {}), } }), ); decisionsInFlight.delete(item.number); diff --git a/eng/board-bulk-actions.test.mjs b/eng/board-bulk-actions.test.mjs index 94373e9b64..5afff10d0e 100644 --- a/eng/board-bulk-actions.test.mjs +++ b/eng/board-bulk-actions.test.mjs @@ -60,8 +60,8 @@ for (const boardName of ["open-pr-board", "external-plugin-review-board"]) { vm.runInContext('stagedComments.set(1, "Edited feedback"); filterText = filter = "no matches";', ui.context); await ui.context.bulkAction("test", reject); assert.deepEqual(ui.calls.map(args => ({ ...payload(args) })), [ - { number: 1, kind: reject, comment: "Edited feedback" }, - { number: 2, kind: reject, comment: "Feedback 2" }, + { number: 1, kind: reject, comment: "Edited feedback", ...(isPR ? { headRefOid: "first" } : {}) }, + { number: 2, kind: reject, comment: "Feedback 2", ...(isPR ? { headRefOid: "first" } : {}) }, ]); assert.match(ui.confirmations[0], /Edited feedback/); assert.match(ui.confirmations[0], /hidden search results/); diff --git a/eng/board-preview.test.mjs b/eng/board-preview.test.mjs index ae83ec88f2..9ceb89c42c 100644 --- a/eng/board-preview.test.mjs +++ b/eng/board-preview.test.mjs @@ -82,6 +82,37 @@ async function createUI(boardName, request = async () => ({ }; } +for (const outcome of ["success", "error"]) { + test(`plugin detail reload ignores an older polling ${outcome} after the post-comment reload`, async () => { + const requests = []; + const ui = await createUI("external-plugin-review-board", () => new Promise((resolve, reject) => { + requests.push({ resolve, reject }); + })); + const polling = ui.context.loadDetail(1, { force: true }); + const posted = ui.context.loadDetail(1, { force: true }); + requests[1].resolve({ bodyHtml: "new", comments: [{ bodyHtml: "posted comment" }] }); + await posted; + if (outcome === "success") requests[0].resolve({ bodyHtml: "old", comments: [] }); + else requests[0].reject(new Error("Old failure")); + await polling; + assert.equal(vm.runInContext("details.get(1).bodyHtml", ui.context), "new"); + assert.equal(vm.runInContext("details.get(1).comments.length", ui.context), 1); + assert.equal(vm.runInContext("detailRequests.size", ui.context), 0); + }); +} + +for (const kind of ["comment", "request-changes"]) { + test(`PR drawer ${kind} includes the displayed SHA`, async () => { + const calls = []; + const ui = await createUI("open-pr-board", async (...args) => { + calls.push(args); + return { status: "submitted" }; + }); + await ui.decide(kind); + assert.equal(calls[0][1].body.headRefOid, "first"); + }); +} + for (const boardName of ["open-pr-board", "external-plugin-review-board"]) { test(`${boardName}: live refresh retains selection, drafts, tabs, and in-flight action guards`, async () => { const ui = await createUI(boardName); diff --git a/eng/open-pr-board.test.mjs b/eng/open-pr-board.test.mjs index 22fd528d75..194a192691 100644 --- a/eng/open-pr-board.test.mjs +++ b/eng/open-pr-board.test.mjs @@ -107,11 +107,26 @@ test("sync before a delayed manual move retains the client's original commit", a }); for (const kind of ["comment", "request-changes"]) { + test(`${kind} rejects feedback dispatched after the displayed head has changed`, async (t) => { + const store = await createStore(t); + await store.syncPullRequests([pr("second")]); + let writes = 0; + for (const headRefOid of ["first", undefined]) { + await assert.rejects(applyDecisionOnGitHub({ + store, number: 1, kind, comment: "Feedback on first", headRefOid, + github: { submitPullRequestReview: async () => { writes++; } }, + refresh: async () => {}, + }), /changed since it was displayed/); + } + assert.equal(writes, 0); + assert.equal(store.getItem(1).decision, null); + }); + test(`a delayed ${kind} response retains the action head when polling discovers new commits`, async (t) => { const store = await createStore(t); const pending = deferred(); const action = applyDecisionOnGitHub({ - store, number: 1, kind, comment: "Feedback on first", + store, number: 1, kind, comment: "Feedback on first", headRefOid: "first", github: { submitPullRequestReview: async () => { await pending.promise; return { status: "submitted" }; } }, refresh: () => store.syncPullRequests([pr("second")]), }); @@ -312,7 +327,7 @@ test("concurrent GitHub decisions for the same repo/PR cannot both write", async submitPullRequestReview: async () => { calls++; await waiting.promise; return { status: "comment-submitted" }; }, closePullRequest: async () => { calls++; throw new Error("must not be reached"); }, }; - const args = { store, number: 1, comment: "Feedback", github, refresh: async () => {} }; + const args = { store, number: 1, comment: "Feedback", headRefOid: "first", github, refresh: async () => {} }; const first = applyDecisionOnGitHub({ ...args, kind: "comment" }); await assert.rejects(applyDecisionOnGitHub({ ...args, kind: "close" }), /in progress/); assert.equal(calls, 1); @@ -323,7 +338,7 @@ test("concurrent GitHub decisions for the same repo/PR cannot both write", async test("decision lock is released on failure and independent repositories can proceed", async (t) => { const store = await createStore(t); - const args = { store, number: 1, kind: "comment", comment: "Feedback", refresh: async () => {} }; + const args = { store, number: 1, kind: "comment", comment: "Feedback", headRefOid: "first", refresh: async () => {} }; await assert.rejects(applyDecisionOnGitHub({ ...args, github: { submitPullRequestReview: async () => { throw new Error("offline"); } }, }), /offline/); From 03f1e38909169be700dec5d0e15c247b578826c4 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 27 Oct 2026 13:37:39 +1100 Subject: [PATCH 6/9] fix(plugin-board): discard invalidated cache writes and render errors Use cache request identity to prevent late reads from restoring invalidated issue data, and tolerate missing comments on failed detail loads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../extension.mjs | 14 +++++++---- .../public/app.js | 2 +- eng/board-periodic-refresh.test.mjs | 24 ++++++++++++++++++- eng/external-plugin-review-board-ui.test.mjs | 16 +++++++++++++ 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/.github/extensions/external-plugin-review-board/extension.mjs b/.github/extensions/external-plugin-review-board/extension.mjs index 2e3d092472..b276ae678a 100644 --- a/.github/extensions/external-plugin-review-board/extension.mjs +++ b/.github/extensions/external-plugin-review-board/extension.mjs @@ -81,10 +81,16 @@ async function refresh() { async function getIssue(number, { force = false } = {}) { const cached = detailCache.get(number); - if (!force && cached && Date.now() - cached.at < DETAIL_TTL_MS) return cached.data; - const data = await gh.getIssueDetail(store.state.repo, number); - detailCache.set(number, { at: Date.now(), data }); - return data; + if (!force && cached?.data && Date.now() - cached.at < DETAIL_TTL_MS) return cached.data; + const request = {}; + detailCache.set(number, request); + try { + const data = await gh.getIssueDetail(store.state.repo, number); + if (detailCache.get(number) === request) detailCache.set(number, { at: Date.now(), data }); + return data; + } finally { + if (detailCache.get(number) === request) detailCache.delete(number); + } } async function startReview({ numbers, instanceId, send }) { diff --git a/.github/extensions/external-plugin-review-board/public/app.js b/.github/extensions/external-plugin-review-board/public/app.js index 6205596e37..6339e0edfb 100644 --- a/.github/extensions/external-plugin-review-board/public/app.js +++ b/.github/extensions/external-plugin-review-board/public/app.js @@ -437,7 +437,7 @@ function renderDrawer() { for (const tab of ui.tabs) { tab.setAttribute("aria-selected", String(tab.dataset.tab === activeTab)); if (tab.dataset.tab === "comments") { - const count = details.get(item.number)?.comments.length; + const count = details.get(item.number)?.comments?.length; tab.textContent = count != null ? `Comments (${count})` : "Comments"; } } diff --git a/eng/board-periodic-refresh.test.mjs b/eng/board-periodic-refresh.test.mjs index 976f18beeb..dadd0c2c62 100644 --- a/eng/board-periodic-refresh.test.mjs +++ b/eng/board-periodic-refresh.test.mjs @@ -37,7 +37,7 @@ function item(number) { }; } -async function loadBoard(t, id) { +async function loadBoard(t, id, { getIssueDetail } = {}) { const workspacePath = await mkdtemp(path.join(os.tmpdir(), "board-polling-")); t.after(() => rm(workspacePath, { recursive: true, force: true })); const isPR = id === "open-pr-board"; @@ -85,6 +85,7 @@ async function loadBoard(t, id) { const list = () => { calls++; return fetchItems(); }; // Any unexpected GitHub operation (especially a write) fails the test. exports = { listOpenPullRequests: list, listBoardIssues: list }; + if (getIssueDetail) exports.getIssueDetail = getIssueDetail; } else { exports = await import(specifier.startsWith(".") ? new URL(specifier, url).href : specifier); } @@ -114,6 +115,27 @@ async function readFrame(reader, eventName) { return text; } +test("plugin server cache cannot be repopulated by a detail request invalidated by refresh", async (t) => { + let finish; + let calls = 0; + const board = await loadBoard(t, "external-plugin-review-board", { + getIssueDetail: () => { + calls++; + if (calls === 1) return new Promise((resolve) => { finish = resolve; }); + return { bodyHtml: "new", comments: [] }; + }, + }); + const old = board.action("get_issue", { number: 1 }); + await board.action("refresh"); + finish({ bodyHtml: "old", comments: [] }); + await old; + const latest = await board.action("get_issue", { number: 1 }); + assert.equal(latest.issue.bodyHtml, "new"); + assert.equal(calls, 2, "Reopening an unselected issue fetches fresh details"); + await board.action("get_issue", { number: 1 }); + assert.equal(calls, 2, "The fresh result remains cached"); +}); + for (const id of ["open-pr-board", "external-plugin-review-board"]) { test(`${id}: scheduled refresh preserves state, shares manual fetches, recovers, and stops on close`, { timeout: 15000 }, async (t) => { diff --git a/eng/external-plugin-review-board-ui.test.mjs b/eng/external-plugin-review-board-ui.test.mjs index 2faa0be048..4344e2b62b 100644 --- a/eng/external-plugin-review-board-ui.test.mjs +++ b/eng/external-plugin-review-board-ui.test.mjs @@ -93,6 +93,22 @@ test("Approve is disabled for submissions that are not review-eligible", () => { assert.equal(ui.reject.disabled, false); }); +test("an automatic detail failure renders its error and allows the drawer to be rendered again", () => { + const item = { number: 42, author: "contributor", labels: [], reviewEligible: true }; + const { context, ui } = createContext(item); + ui.tabs = [{ dataset: { tab: "comments" }, setAttribute() {} }]; + ui.drawerBody = { replaceChildren(...children) { this.children = children; } }; + context.activeTab = "comments"; + context.h = element; + context.details.set(42, { error: "GitHub unavailable" }); + evaluateFunction(context, appSource, "function renderDrawer()", "function renderTab("); + evaluateFunction(context, appSource, "function renderTab(", "function renderReview("); + context.renderDrawer(); + assert.equal(ui.tabs[0].textContent, "Comments"); + assert.match(text(ui.drawerBody.children[0]), /GitHub unavailable/); + assert.doesNotThrow(() => context.renderDrawer()); +}); + test("Approve retains its existing eligible, pending-decision, and bulk-busy states", () => { const eligible = { number: 42, author: "contributor", labels: [], reviewEligible: true }; From 1291ae1020702bc2ea2fb62e1ede04088992759c Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 27 Oct 2026 14:04:40 +1100 Subject: [PATCH 7/9] fix(pr-board): expose provenance and guard detail cache writes Include head SHA and approval staleness in agent summaries, and prevent invalidated PR detail reads from overwriting current cached previews. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/extensions/open-pr-board/README.md | 2 +- .../extensions/open-pr-board/extension.mjs | 16 ++++++-- eng/board-periodic-refresh.test.mjs | 38 +++++++++++++++---- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index a70a362de7..61a5b9b175 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -88,7 +88,7 @@ touching GitHub. | Action | Purpose | | ---------------- | ----------------------------------------------------------------------- | -| `get_board` | Current buckets, counts, and per-PR state. | +| `get_board` | Current buckets, counts, per-PR commit SHA, and approval staleness. | | `refresh` | Re-fetch open pull requests from GitHub. | | `start_review` | Queue a review pass and return the review instructions. | | `start_rereview` | Queue a re-review of one PR with optional guidance. | diff --git a/.github/extensions/open-pr-board/extension.mjs b/.github/extensions/open-pr-board/extension.mjs index b3ed05170e..5a939fe659 100644 --- a/.github/extensions/open-pr-board/extension.mjs +++ b/.github/extensions/open-pr-board/extension.mjs @@ -97,10 +97,16 @@ async function resetBoard(store) { async function getPullRequest(store, number, { force = false } = {}) { const key = cacheKey(store.repo, number); const cached = detailCache.get(key); - if (!force && cached && Date.now() - cached.at < DETAIL_TTL_MS) return cached.data; - const data = await gh.getPullRequestDetail(store.repo, number); - detailCache.set(key, { at: Date.now(), data }); - return data; + if (!force && cached?.data && Date.now() - cached.at < DETAIL_TTL_MS) return cached.data; + const request = {}; + detailCache.set(key, request); + try { + const data = await gh.getPullRequestDetail(store.repo, number); + if (detailCache.get(key) === request) detailCache.set(key, { at: Date.now(), data }); + return data; + } finally { + if (detailCache.get(key) === request) detailCache.delete(key); + } } function requireItem(store, number) { @@ -182,6 +188,7 @@ function boardSummary(store) { counts: snapshot.counts, items: snapshot.items.map((item) => ({ number: item.number, + headRefOid: item.headRefOid, title: item.title, author: item.author, url: item.url, @@ -193,6 +200,7 @@ function boardSummary(store) { aiRecommendation: item.review?.recommendation ?? null, aiRationale: item.review?.rationale ?? null, reviewStale: item.reviewStale, + approvalStale: item.approvalStale, reviewStatus: item.reviewStatus, manualOverride: item.manualColumn ?? null, suggestedComment: item.review?.suggestedComment ?? null, diff --git a/eng/board-periodic-refresh.test.mjs b/eng/board-periodic-refresh.test.mjs index dadd0c2c62..fd4d662704 100644 --- a/eng/board-periodic-refresh.test.mjs +++ b/eng/board-periodic-refresh.test.mjs @@ -37,7 +37,7 @@ function item(number) { }; } -async function loadBoard(t, id, { getIssueDetail } = {}) { +async function loadBoard(t, id, { getIssueDetail, getPullRequestDetail } = {}) { const workspacePath = await mkdtemp(path.join(os.tmpdir(), "board-polling-")); t.after(() => rm(workspacePath, { recursive: true, force: true })); const isPR = id === "open-pr-board"; @@ -86,6 +86,7 @@ async function loadBoard(t, id, { getIssueDetail } = {}) { // Any unexpected GitHub operation (especially a write) fails the test. exports = { listOpenPullRequests: list, listBoardIssues: list }; if (getIssueDetail) exports.getIssueDetail = getIssueDetail; + if (getPullRequestDetail) exports.getPullRequestDetail = getPullRequestDetail; } else { exports = await import(specifier.startsWith(".") ? new URL(specifier, url).href : specifier); } @@ -115,26 +116,47 @@ async function readFrame(reader, eventName) { return text; } -test("plugin server cache cannot be repopulated by a detail request invalidated by refresh", async (t) => { +test("PR agent board summaries provide provenance for manual approval choices", async (t) => { + const board = await loadBoard(t, "open-pr-board"); + const before = (await board.action("get_board")).items.find((entry) => entry.number === 2); + assert.equal(before.headRefOid, "first"); + await board.action("move_item", { number: 2, column: "approve", headRefOid: before.headRefOid }); + const current = (await board.action("get_board")).items.find((entry) => entry.number === 2); + assert.equal(current.approvalStale, false); + board.setFetch(async () => [1, 2, 3, 4].map((number) => ({ ...item(number), headRefOid: "second" }))); + await board.action("refresh"); + const updated = (await board.action("get_board")).items.find((entry) => entry.number === 2); + assert.equal(updated.headRefOid, "second"); + assert.equal(updated.approvalStale, true); +}); + +for (const id of ["open-pr-board", "external-plugin-review-board"]) { +test(`${id}: server cache cannot be repopulated by a detail request invalidated by refresh`, async (t) => { let finish; let calls = 0; - const board = await loadBoard(t, "external-plugin-review-board", { - getIssueDetail: () => { + const isPR = id === "open-pr-board"; + const board = await loadBoard(t, id, { + [isPR ? "getPullRequestDetail" : "getIssueDetail"]: () => { calls++; if (calls === 1) return new Promise((resolve) => { finish = resolve; }); return { bodyHtml: "new", comments: [] }; }, }); - const old = board.action("get_issue", { number: 1 }); + const action = isPR ? "get_pr" : "get_issue"; + const field = isPR ? "pullRequest" : "issue"; + const old = board.action(action, { number: 1 }); await board.action("refresh"); + const latest = await board.action(action, { number: 1 }); + assert.equal(latest[field].bodyHtml, "new"); finish({ bodyHtml: "old", comments: [] }); await old; - const latest = await board.action("get_issue", { number: 1 }); - assert.equal(latest.issue.bodyHtml, "new"); + const reopened = await board.action(action, { number: 1 }); + assert.equal(reopened[field].bodyHtml, "new"); assert.equal(calls, 2, "Reopening an unselected issue fetches fresh details"); - await board.action("get_issue", { number: 1 }); + await board.action(action, { number: 1 }); assert.equal(calls, 2, "The fresh result remains cached"); }); +} for (const id of ["open-pr-board", "external-plugin-review-board"]) { test(`${id}: scheduled refresh preserves state, shares manual fetches, recovers, and stops on close`, From 6b3889a13450e3a1931b2c529153a3de4f22b745 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 27 Oct 2026 14:29:15 +1100 Subject: [PATCH 8/9] fix(pr-board): preserve current state when old feedback completes Record obsolete-head feedback in history without installing a decision that would clear newer queues, reviews, or manual moves on refresh. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/extensions/open-pr-board/README.md | 2 ++ .../extensions/open-pr-board/lib/state.mjs | 7 +++-- eng/open-pr-board.test.mjs | 30 +++++++++++++++++++ 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index 61a5b9b175..b72aa4fd02 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -56,6 +56,8 @@ authenticated `gh` CLI. Moves carry the client's displayed SHA even if polling updates the board before the request arrives. Delayed feedback actions retain their original SHA so new commits still return to Unreviewed. A successful refresh retries a failed open preview. + Feedback completed for an older head is recorded only in history, preserving any + review queue, completed review, or manual bucket already created for the newer head. Feedback requests also carry the displayed SHA and are rejected if polling has already discovered a newer head before dispatch. Only one GitHub action can run per PR at a time. diff --git a/.github/extensions/open-pr-board/lib/state.mjs b/.github/extensions/open-pr-board/lib/state.mjs index 31985f4c20..2a8654512b 100644 --- a/.github/extensions/open-pr-board/lib/state.mjs +++ b/.github/extensions/open-pr-board/lib/state.mjs @@ -270,7 +270,7 @@ export class BoardStore { async recordDecision(number, decision) { const item = this.getItem(number); if (!item) throw new Error(`Pull request #${number} is not on the board.`); - item.decision = { + const recorded = { kind: decision.kind, comment: decision.comment ?? "", at: new Date().toISOString(), @@ -283,9 +283,12 @@ export class BoardStore { url: item.url, kind: decision.kind, comment: decision.comment ?? "", - at: item.decision.at, + at: recorded.at, status: decision.status ?? null, }); + const obsoleteFeedback = (recorded.kind === "comment" || recorded.kind === "request-changes") && + recorded.headRefOid && item.headRefOid && recorded.headRefOid !== item.headRefOid; + if (!obsoleteFeedback) item.decision = recorded; this.state.history = this.state.history.slice(0, HISTORY_LIMIT); return this.commit(); } diff --git a/eng/open-pr-board.test.mjs b/eng/open-pr-board.test.mjs index 194a192691..b1454de21b 100644 --- a/eng/open-pr-board.test.mjs +++ b/eng/open-pr-board.test.mjs @@ -137,6 +137,36 @@ for (const kind of ["comment", "request-changes"]) { assert.equal(store.snapshot().items[0].column, "unreviewed"); assert.equal(store.state.history[0].kind, kind); }); + + for (const currentState of ["queued", "reviewed", "manual"]) { + test(`obsolete ${kind} feedback preserves the newer head's ${currentState} state`, async (t) => { + const store = await createStore(t); + const pending = deferred(); + const action = applyDecisionOnGitHub({ + store, number: 1, kind, comment: "Feedback on first", headRefOid: "first", + github: { submitPullRequestReview: async () => { await pending.promise; return { status: "submitted" }; } }, + refresh: () => store.syncPullRequests([pr("second")]), + }); + await store.syncPullRequests([pr("second")]); + const { queued } = await store.markQueued([1]); + if (currentState === "reviewed") { + await store.recordReviews([{ number: 1, queueId: queued[0].queueId, recommendation: "approve" }]); + } else if (currentState === "manual") { + await store.moveItem(1, "needs-insight", "second"); + } + const before = structuredClone(store.getItem(1)); + pending.resolve(); + await action; + assert.deepEqual(store.getItem(1), before, "Obsolete feedback must not replace current review or move state"); + assert.equal(store.state.history[0].kind, kind); + if (currentState === "queued") { + const [result] = await store.recordReviews([{ + number: 1, queueId: queued[0].queueId, recommendation: "approve", + }]); + assert.equal(result.status, "recorded", "The newer head's queue token remains valid"); + } + }); + } } test("approval and auto-merge are pinned to the reviewed commit", async () => { From c601e5dd38b8c36dd37f362f3b71fc701699d3d7 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 27 Oct 2026 14:51:00 +1100 Subject: [PATCH 9/9] fix(pr-board): clear obsolete choices when delayed feedback finishes Use commit provenance to clear old-head reviews, queues, and manual buckets while preserving state created for the current head. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/extensions/open-pr-board/README.md | 1 + .../extensions/open-pr-board/lib/state.mjs | 19 +++++++++- eng/open-pr-board.test.mjs | 38 +++++++++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index b72aa4fd02..3d7a4fc0be 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -58,6 +58,7 @@ authenticated `gh` CLI. still return to Unreviewed. A successful refresh retries a failed open preview. Feedback completed for an older head is recorded only in history, preserving any review queue, completed review, or manual bucket already created for the newer head. + Any retained review, queue, or manual bucket belonging to the older head is cleared. Feedback requests also carry the displayed SHA and are rejected if polling has already discovered a newer head before dispatch. Only one GitHub action can run per PR at a time. diff --git a/.github/extensions/open-pr-board/lib/state.mjs b/.github/extensions/open-pr-board/lib/state.mjs index 2a8654512b..a79f3c5260 100644 --- a/.github/extensions/open-pr-board/lib/state.mjs +++ b/.github/extensions/open-pr-board/lib/state.mjs @@ -288,7 +288,24 @@ export class BoardStore { }); const obsoleteFeedback = (recorded.kind === "comment" || recorded.kind === "request-changes") && recorded.headRefOid && item.headRefOid && recorded.headRefOid !== item.headRefOid; - if (!obsoleteFeedback) item.decision = recorded; + if (!obsoleteFeedback) { + item.decision = recorded; + } else { + if (item.review && item.review.headRefOid !== item.headRefOid) item.review = null; + if (item.manualColumn && item.manualHeadRefOid !== item.headRefOid) { + item.manualColumn = null; + item.manualHeadRefOid = null; + } + if (item.queueId && item.queueHeadRefOid !== item.headRefOid) { + item.queueId = null; + item.queueHeadRefOid = null; + } + if (item.reviewStatus === "queued" && !item.queueId) { + item.reviewStatus = item.review ? "reviewed" : "none"; + } else if (item.reviewStatus === "reviewed" && !item.review) { + item.reviewStatus = "none"; + } + } this.state.history = this.state.history.slice(0, HISTORY_LIMIT); return this.commit(); } diff --git a/eng/open-pr-board.test.mjs b/eng/open-pr-board.test.mjs index b1454de21b..f9a4678b50 100644 --- a/eng/open-pr-board.test.mjs +++ b/eng/open-pr-board.test.mjs @@ -167,6 +167,44 @@ for (const kind of ["comment", "request-changes"]) { } }); } + + for (const oldState of ["queued", "reviewed", "manual"]) { + test(`obsolete ${kind} feedback clears the old head's ${oldState} state`, async (t) => { + const store = await createStore(t); + const { queued } = await store.markQueued([1]); + if (oldState === "reviewed") { + await store.recordReviews([{ number: 1, queueId: queued[0].queueId, recommendation: "request-changes" }]); + } else if (oldState === "manual") { + await store.moveItem(1, "close", "first"); + } + const pending = deferred(); + const action = applyDecisionOnGitHub({ + store, number: 1, kind, comment: "Feedback on first", headRefOid: "first", + github: { submitPullRequestReview: async () => { await pending.promise; return { status: "submitted" }; } }, + refresh: () => store.syncPullRequests([pr("second")]), + }); + await store.syncPullRequests([pr("second")]); + pending.resolve(); + await action; + assert.equal(store.snapshot().items[0].column, "unreviewed"); + assert.equal(store.getItem(1).review, null); + assert.equal(store.getItem(1).manualColumn, null); + assert.equal(store.getItem(1).queueId, null); + assert.equal(store.state.history[0].kind, kind); + }); + } + + test(`obsolete ${kind} feedback clears an old review while preserving the new head's queue`, async (t) => { + const store = await createStore(t); + const old = await store.markQueued([1]); + await store.recordReviews([{ number: 1, queueId: old.queued[0].queueId, recommendation: "request-changes" }]); + await store.syncPullRequests([pr("second")]); + const current = await store.markQueued([1]); + await store.recordDecision(1, { kind, headRefOid: "first" }); + assert.equal(store.getItem(1).review, null); + assert.equal(store.getItem(1).queueId, current.queued[0].queueId); + assert.equal(store.snapshot().items[0].column, "reviewing"); + }); } test("approval and auto-merge are pinned to the reviewed commit", async () => {