From 666a521cd50a19725786f1fa367b4f5027ec5fee Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 13 Oct 2026 13:01:19 +1100 Subject: [PATCH] fix(external-plugins): preserve cards during intake Include submissions awaiting review or submitter fixes in the review board feed so refreshes preserve their existing state. Keep fast-track and approval gated on a ready intake state, and add regression coverage for card persistence across pending transitions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 821f4796-505e-4fb8-96cf-0b5bbf80a636 --- .../external-plugin-review-board/README.md | 10 +-- .../extension.mjs | 11 ++- .../lib/github.mjs | 29 ++++---- .../lib/github.test.mjs | 21 ++++++ .../lib/state.mjs | 9 ++- eng/board-state-storage.test.mjs | 2 +- eng/external-plugin-intake-state.mjs | 8 +-- eng/external-plugin-intake-state.test.mjs | 2 +- ...xternal-plugin-review-board-state.test.mjs | 71 ++++++++++++++++++- 9 files changed, 131 insertions(+), 32 deletions(-) create mode 100644 .github/extensions/external-plugin-review-board/lib/github.test.mjs diff --git a/.github/extensions/external-plugin-review-board/README.md b/.github/extensions/external-plugin-review-board/README.md index 08917fb56..3ecb51cde 100644 --- a/.github/extensions/external-plugin-review-board/README.md +++ b/.github/extensions/external-plugin-review-board/README.md @@ -1,7 +1,7 @@ # External Plugin Review Board -A maintainer-only Copilot canvas extension for triaging `external-plugin` submissions that are -`ready-for-review` in github/awesome-copilot. +A maintainer-only Copilot canvas extension for triaging active `external-plugin` submissions in +github/awesome-copilot. Open it by asking Copilot to "open the plugin review board" (canvas id `external-plugin-review-board`). @@ -16,8 +16,10 @@ Open it by asking Copilot to "open the plugin review board" (canvas id `external - **Re-review** (in the details panel) takes optional guidance on what to focus on, then has the agent spin up a separate sub-session to re-assess that one submission. The sub-session reports its result back and it's recorded on the card along with your guidance. -- **Refresh** fetches open `external-plugin` + `ready-for-review` issues, adds new ones, and removes any - that are closed or no longer ready for review. +- **Refresh** fetches open `external-plugin` issues in `ready-for-review`, `awaiting-review`, or + `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. - **First-party fast-track**: cards labelled `first party` (repository owned by a Microsoft/GitHub organization) skip AI review and go straight to *Accept* with a suggested `/approve`. Only submissions that passed automated intake reach the board, so hard validation failures are still held back by intake. diff --git a/.github/extensions/external-plugin-review-board/extension.mjs b/.github/extensions/external-plugin-review-board/extension.mjs index 7e1332c96..e9f292542 100644 --- a/.github/extensions/external-plugin-review-board/extension.mjs +++ b/.github/extensions/external-plugin-review-board/extension.mjs @@ -6,7 +6,7 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { joinSession, createCanvas, CanvasError } from "@github/copilot-sdk/extension"; -import { BoardStore, RECOMMENDATIONS } from "./lib/state.mjs"; +import { BoardStore, isReadyForApproval, RECOMMENDATIONS } from "./lib/state.mjs"; import * as gh from "./lib/github.mjs"; import { buildReviewPrompt, buildRereviewPrompt, REVIEW_FIELDS_SCHEMA } from "./lib/review-prompt.mjs"; import { startBoardServer, expectedError } from "./lib/server.mjs"; @@ -49,7 +49,7 @@ async function refresh() { refreshInFlight ??= (async () => { try { await resolvePendingDecisions(); - const issues = await gh.listReadyIssues(store.state.repo); + const issues = await gh.listBoardIssues(store.state.repo); detailCache.clear(); return await store.syncIssues(issues); } finally { @@ -134,10 +134,7 @@ async function postDecision({ number, kind, comment }) { const status = await gh.getIssueStatus(store.state.repo, number); if (status.state !== "OPEN") throw new Error(`#${number} is ${status.state.toLowerCase()}. Refresh the board.`); - if ( - kind === "approve" && - (!status.labels.includes("ready-for-review") || status.labels.includes("awaiting-review")) - ) { + if (kind === "approve" && !isReadyForApproval(status.labels)) { throw new Error(`#${number} is pending intake or is no longer ready-for-review, so /approve would be ignored.`); } @@ -241,7 +238,7 @@ session = await joinSession({ }, { name: "refresh", - description: "Fetch open ready-for-review external plugin issues; adds new ones and removes closed ones.", + description: "Fetch open external plugin submissions in active intake states; adds new ones and removes inactive ones.", handler: wrap(() => refresh()), }, { diff --git a/.github/extensions/external-plugin-review-board/lib/github.mjs b/.github/extensions/external-plugin-review-board/lib/github.mjs index 128a2c847..bcca92e95 100644 --- a/.github/extensions/external-plugin-review-board/lib/github.mjs +++ b/.github/extensions/external-plugin-review-board/lib/github.mjs @@ -29,7 +29,22 @@ function runGh(args, { input } = {}) { }); } -export async function listReadyIssues(repo) { +export function mapBoardIssues(issues) { + const boardStates = new Set(["ready-for-review", "awaiting-review", "requires-submitter-fixes"]); + return issues + .filter((issue) => (issue.labels ?? []).some((label) => boardStates.has(label.name))) + .map((issue) => ({ + number: issue.number, + title: issue.title, + author: issue.author?.login ?? "unknown", + createdAt: issue.createdAt, + updatedAt: issue.updatedAt, + url: issue.url, + labels: (issue.labels ?? []).map((label) => label.name), + })); +} + +export async function listBoardIssues(repo) { const out = await runGh([ "issue", "list", @@ -37,8 +52,6 @@ export async function listReadyIssues(repo) { repo, "--label", "external-plugin", - "--label", - "ready-for-review", "--state", "open", "--limit", @@ -46,15 +59,7 @@ export async function listReadyIssues(repo) { "--json", "number,title,author,createdAt,updatedAt,labels,url", ]); - return JSON.parse(out).map((issue) => ({ - number: issue.number, - title: issue.title, - author: issue.author?.login ?? "unknown", - createdAt: issue.createdAt, - updatedAt: issue.updatedAt, - url: issue.url, - labels: (issue.labels ?? []).map((label) => label.name), - })); + return mapBoardIssues(JSON.parse(out)); } const FULL_JSON = ["-H", "Accept: application/vnd.github.full+json"]; diff --git a/.github/extensions/external-plugin-review-board/lib/github.test.mjs b/.github/extensions/external-plugin-review-board/lib/github.test.mjs new file mode 100644 index 000000000..5ac6dd2c2 --- /dev/null +++ b/.github/extensions/external-plugin-review-board/lib/github.test.mjs @@ -0,0 +1,21 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { mapBoardIssues } from "./github.mjs"; + +test("board feed includes submissions in each active intake state", () => { + const issues = ["ready-for-review", "awaiting-review", "requires-submitter-fixes", "rejected"].map( + (state, index) => ({ + number: index + 1, + title: `Submission ${index + 1}`, + author: { login: "contributor" }, + labels: [{ name: "external-plugin" }, { name: state }], + url: `https://github.com/github/awesome-copilot/issues/${index + 1}`, + }), + ); + + assert.deepEqual( + mapBoardIssues(issues).map((issue) => issue.number), + [1, 2, 3], + ); +}); diff --git a/.github/extensions/external-plugin-review-board/lib/state.mjs b/.github/extensions/external-plugin-review-board/lib/state.mjs index 8000e9d51..ca5ce7253 100644 --- a/.github/extensions/external-plugin-review-board/lib/state.mjs +++ b/.github/extensions/external-plugin-review-board/lib/state.mjs @@ -16,6 +16,11 @@ export const COLUMNS = [ const HISTORY_LIMIT = 200; export const FIRST_PARTY_LABEL = "first party"; +const PENDING_INTAKE_LABELS = ["awaiting-review", "requires-submitter-fixes"]; + +export function isReadyForApproval(labels) { + return labels.includes("ready-for-review") && !labels.some((label) => PENDING_INTAKE_LABELS.includes(label)); +} // Validated submissions from first-party organizations skip AI triage and land in Accept. Only // cards without an AI review, queued review, or manual bucket are fast-tracked so maintainer @@ -24,7 +29,7 @@ function applyFirstPartyFastTrack(item) { if ( !item.labels?.includes(FIRST_PARTY_LABEL) || !item.labels?.includes("ready-for-review") || - item.labels?.includes("awaiting-review") || + item.labels?.some((label) => PENDING_INTAKE_LABELS.includes(label)) || item.fastTrackDismissed || item.decision || item.review || @@ -49,7 +54,7 @@ function clearFirstPartyFastTrack(item) { if ( (item.labels?.includes(FIRST_PARTY_LABEL) && item.labels?.includes("ready-for-review") && - !item.labels?.includes("awaiting-review")) || + !item.labels?.some((label) => PENDING_INTAKE_LABELS.includes(label))) || !item.review?.fastTracked || item.reviewStatus === "queued" ) { diff --git a/eng/board-state-storage.test.mjs b/eng/board-state-storage.test.mjs index 3db0f568b..8edef8c43 100644 --- a/eng/board-state-storage.test.mjs +++ b/eng/board-state-storage.test.mjs @@ -80,7 +80,7 @@ async function loadCanvas({ id, extensionDir, workspacePath, triggerDuringJoin = } else if (specifier === "./lib/github.mjs") { exports = { listOpenPullRequests: async () => [item], - listReadyIssues: async () => [item], + listBoardIssues: async () => [item], }; } else { exports = await import(specifier.startsWith(".") diff --git a/eng/external-plugin-intake-state.mjs b/eng/external-plugin-intake-state.mjs index ef665f93b..7feac27bb 100644 --- a/eng/external-plugin-intake-state.mjs +++ b/eng/external-plugin-intake-state.mjs @@ -101,16 +101,16 @@ export async function syncExternalPluginIntakeLabels({ } export async function markExternalPluginIntakeAwaitingReview({ github, owner, repo, issueNumber }) { - for (const name of ["ready-for-review", "requires-submitter-fixes", "rejected"]) { - await removeLabel({ github, owner, repo, issueNumber, name }); - } - await github.rest.issues.addLabels({ owner, repo, issue_number: issueNumber, labels: ["external-plugin", "awaiting-review"], }); + + for (const name of ["ready-for-review", "requires-submitter-fixes", "rejected"]) { + await removeLabel({ github, owner, repo, issueNumber, name }); + } } export async function upsertExternalPluginIntakeComment({ diff --git a/eng/external-plugin-intake-state.test.mjs b/eng/external-plugin-intake-state.test.mjs index 7db6e38ae..6abbe3eaa 100644 --- a/eng/external-plugin-intake-state.test.mjs +++ b/eng/external-plugin-intake-state.test.mjs @@ -100,5 +100,5 @@ test("marking intake pending removes stale state labels but preserves ownership, const pendingAddition = github.operations.findIndex( ([action, labels]) => action === "add" && labels.includes("awaiting-review"), ); - assert.ok(readyRemoval >= 0 && pendingAddition > readyRemoval); + assert.ok(pendingAddition >= 0 && readyRemoval > pendingAddition); }); diff --git a/eng/external-plugin-review-board-state.test.mjs b/eng/external-plugin-review-board-state.test.mjs index ece1e2fc8..015f533dd 100644 --- a/eng/external-plugin-review-board-state.test.mjs +++ b/eng/external-plugin-review-board-state.test.mjs @@ -4,7 +4,11 @@ import os from "node:os"; import path from "node:path"; import { test } from "node:test"; -import { BoardStore, effectiveColumn } from "../.github/extensions/external-plugin-review-board/lib/state.mjs"; +import { + BoardStore, + effectiveColumn, + isReadyForApproval, +} from "../.github/extensions/external-plugin-review-board/lib/state.mjs"; function issue(number, labels = []) { return { @@ -48,6 +52,27 @@ test("first-party submissions are not fast-tracked while intake is pending", asy assert.equal(effectiveColumn(item), "unreviewed"); }); +test("first-party submissions are not fast-tracked in other pending intake states", async (t) => { + const store = await createStore(t); + const pendingIssue = issue(1, ["first party", "requires-submitter-fixes"]); + await store.syncIssues([pendingIssue]); + + const item = store.getItem(1); + assert.equal(item.review, null); + assert.equal(item.reviewStatus, "unreviewed"); + assert.equal(effectiveColumn(item), "unreviewed"); +}); + +test("approval requires ready status and no pending intake label", () => { + assert.equal(isReadyForApproval(["external-plugin", "ready-for-review"]), true); + assert.equal(isReadyForApproval(["external-plugin", "ready-for-review", "awaiting-review"]), false); + assert.equal( + isReadyForApproval(["external-plugin", "ready-for-review", "requires-submitter-fixes"]), + false, + ); + assert.equal(isReadyForApproval(["external-plugin", "rejected"]), false); +}); + test("first-party submissions are not fast-tracked without a ready-for-review label", async (t) => { const store = await createStore(t); const pendingIssue = issue(1, ["first party"]); @@ -124,6 +149,50 @@ test("removing the first-party label preserves decisions, manual buckets, and qu assert.deepEqual(store.getItem(3).review, queuedReview); }); +test("pending intake refreshes preserve review, manual, queued, and fast-track dismissal state", async (t) => { + const store = await createStore(t); + await store.syncIssues([ + issue(1), + issue(2), + issue(3), + issue(4, ["first party"]), + ]); + + const aiReview = { recommendation: "needs-review", rationale: "Keep this review." }; + store.getItem(1).review = aiReview; + store.getItem(1).reviewStatus = "reviewed"; + await store.moveItem(2, "probably-reject"); + await store.markQueued([3], { rereviewGuidance: "Re-check this submission." }); + await store.moveItem(4, "unreviewed"); + + await store.syncIssues([ + issue(1, ["awaiting-review"]), + issue(2, ["requires-submitter-fixes"]), + issue(3, ["awaiting-review"]), + issue(4, ["first party", "requires-submitter-fixes"]), + ]); + + assert.deepEqual(store.getItem(1).review, aiReview); + assert.equal(store.getItem(2).manualColumn, "probably-reject"); + assert.equal(store.getItem(3).reviewStatus, "queued"); + assert.equal(store.getItem(3).rereviewGuidance, "Re-check this submission."); + assert.equal(store.getItem(4).fastTrackDismissed, true); + + const result = await store.syncIssues([ + issue(1), + issue(2), + issue(3), + issue(4, ["first party"]), + ]); + + assert.deepEqual(result.fastTracked, []); + assert.deepEqual(store.getItem(1).review, aiReview); + assert.equal(store.getItem(2).manualColumn, "probably-reject"); + assert.equal(store.getItem(3).reviewStatus, "queued"); + assert.equal(store.getItem(4).fastTrackDismissed, true); + assert.equal(store.getItem(4).review, null); +}); + test("fast-track does not override an existing review or manual bucket", async (t) => { const store = await createStore(t); await store.syncIssues([issue(1), issue(2)]);