From a48dab596130e77635a6a603713cd285869059eb Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Tue, 13 Oct 2026 11:58:49 +1100 Subject: [PATCH] fix(external-plugins): block approval during intake reruns Remove stale ready-for-review state before evaluating edited submissions or running quality gates. Require a current ready state before fast-tracking or approving, and cover the pending transition with focused tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 821f4796-505e-4fb8-96cf-0b5bbf80a636 --- .../extension.mjs | 7 +++- .../lib/state.mjs | 16 ++++++-- .../external-plugin-command-router.yml | 3 +- .github/workflows/external-plugin-intake.yml | 16 ++++++++ eng/external-plugin-intake-state.mjs | 13 +++++++ eng/external-plugin-intake-state.test.mjs | 34 +++++++++++++++++ ...xternal-plugin-review-board-state.test.mjs | 37 +++++++++++++++++++ 7 files changed, 119 insertions(+), 7 deletions(-) diff --git a/.github/extensions/external-plugin-review-board/extension.mjs b/.github/extensions/external-plugin-review-board/extension.mjs index c65797e17..7e1332c96 100644 --- a/.github/extensions/external-plugin-review-board/extension.mjs +++ b/.github/extensions/external-plugin-review-board/extension.mjs @@ -134,8 +134,11 @@ 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")) { - throw new Error(`#${number} is no longer labelled ready-for-review, so /approve would be ignored.`); + if ( + kind === "approve" && + (!status.labels.includes("ready-for-review") || status.labels.includes("awaiting-review")) + ) { + throw new Error(`#${number} is pending intake or is no longer ready-for-review, so /approve would be ignored.`); } const text = stripCommand(comment, kind); diff --git a/.github/extensions/external-plugin-review-board/lib/state.mjs b/.github/extensions/external-plugin-review-board/lib/state.mjs index 39fe6f5dd..8000e9d51 100644 --- a/.github/extensions/external-plugin-review-board/lib/state.mjs +++ b/.github/extensions/external-plugin-review-board/lib/state.mjs @@ -17,12 +17,14 @@ export const COLUMNS = [ const HISTORY_LIMIT = 200; export const FIRST_PARTY_LABEL = "first party"; -// 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 choices are kept; -// dragging a card back to Unreviewed opts it out so it gets a normal AI review. +// 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 +// choices are kept; dragging a card back to Unreviewed opts it out so it gets a normal AI review. function applyFirstPartyFastTrack(item) { if ( !item.labels?.includes(FIRST_PARTY_LABEL) || + !item.labels?.includes("ready-for-review") || + item.labels?.includes("awaiting-review") || item.fastTrackDismissed || item.decision || item.review || @@ -44,7 +46,13 @@ function applyFirstPartyFastTrack(item) { } function clearFirstPartyFastTrack(item) { - if (item.labels?.includes(FIRST_PARTY_LABEL) || !item.review?.fastTracked || item.reviewStatus === "queued") { + if ( + (item.labels?.includes(FIRST_PARTY_LABEL) && + item.labels?.includes("ready-for-review") && + !item.labels?.includes("awaiting-review")) || + !item.review?.fastTracked || + item.reviewStatus === "queued" + ) { return; } item.review = null; diff --git a/.github/workflows/external-plugin-command-router.yml b/.github/workflows/external-plugin-command-router.yml index 8bce714fe..dd7a4bc38 100644 --- a/.github/workflows/external-plugin-command-router.yml +++ b/.github/workflows/external-plugin-command-router.yml @@ -85,7 +85,8 @@ jobs: }); const fallbackName = evaluation.plugin?.name ?? `issue-${context.issue.number}`; - const canApprove = labelNames.has('ready-for-review') || labelNames.has('approved'); + const canApprove = labelNames.has('approved') || + (labelNames.has('ready-for-review') && !labelNames.has('awaiting-review')); const canReject = !labelNames.has('approved'); if (parsedCommand.command === 'approve' && !canApprove) { diff --git a/.github/workflows/external-plugin-intake.yml b/.github/workflows/external-plugin-intake.yml index a80661537..b5b4da006 100644 --- a/.github/workflows/external-plugin-intake.yml +++ b/.github/workflows/external-plugin-intake.yml @@ -48,6 +48,22 @@ jobs: core.setOutput('issue-labels', JSON.stringify(labels)); core.setOutput('should-sync', (!isApproved && !isClosedWithoutReopen) ? 'true' : 'false'); + - name: Mark submission as awaiting review + if: steps.guard.outputs.should-sync == 'true' + uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 + with: + script: | + const path = require('path'); + const { pathToFileURL } = require('url'); + const intakeState = await import(pathToFileURL(path.join(process.env.GITHUB_WORKSPACE, 'eng', 'external-plugin-intake-state.mjs')).href); + + await intakeState.markExternalPluginIntakeAwaitingReview({ + github, + owner: context.repo.owner, + repo: context.repo.repo, + issueNumber: context.issue.number + }); + - name: Evaluate submission id: evaluation env: diff --git a/eng/external-plugin-intake-state.mjs b/eng/external-plugin-intake-state.mjs index 926399f5c..ef665f93b 100644 --- a/eng/external-plugin-intake-state.mjs +++ b/eng/external-plugin-intake-state.mjs @@ -100,6 +100,19 @@ 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"], + }); +} + export async function upsertExternalPluginIntakeComment({ github, owner, diff --git a/eng/external-plugin-intake-state.test.mjs b/eng/external-plugin-intake-state.test.mjs index c17a0eebf..7db6e38ae 100644 --- a/eng/external-plugin-intake-state.test.mjs +++ b/eng/external-plugin-intake-state.test.mjs @@ -2,13 +2,16 @@ import assert from "node:assert/strict"; import { test } from "node:test"; import { applyExternalPluginIntakeEvaluation, + markExternalPluginIntakeAwaitingReview, syncExternalPluginIntakeLabels, } from "./external-plugin-intake-state.mjs"; function mockGitHub(currentLabels) { const labels = new Set(currentLabels); + const operations = []; return { labels, + operations, async paginate() { return [...labels].map((name) => ({ name })); }, @@ -16,9 +19,11 @@ function mockGitHub(currentLabels) { issues: { listLabelsOnIssue() {}, async addLabels({ labels: addedLabels }) { + operations.push(["add", addedLabels]); for (const name of addedLabels) labels.add(name); }, async removeLabel({ name }) { + operations.push(["remove", name]); labels.delete(name); }, async listComments() { @@ -68,3 +73,32 @@ test("state-only label sync preserves ownership labels for maintainer overrides assert.deepEqual(github.labels, new Set(["external-plugin", "ready-for-review", "first party"])); }); + +test("marking intake pending removes stale state labels but preserves ownership, canvas, and unrelated labels", async () => { + const github = mockGitHub([ + "external-plugin", + "external-plugin-canvas", + "first party", + "ready-for-review", + "requires-submitter-fixes", + "rejected", + "unrelated", + ]); + + await markExternalPluginIntakeAwaitingReview({ + github, + owner: "github", + repo: "awesome-copilot", + issueNumber: 1, + }); + + assert.deepEqual( + github.labels, + new Set(["external-plugin", "external-plugin-canvas", "first party", "awaiting-review", "unrelated"]), + ); + const readyRemoval = github.operations.findIndex(([action, name]) => action === "remove" && name === "ready-for-review"); + const pendingAddition = github.operations.findIndex( + ([action, labels]) => action === "add" && labels.includes("awaiting-review"), + ); + assert.ok(readyRemoval >= 0 && pendingAddition > readyRemoval); +}); diff --git a/eng/external-plugin-review-board-state.test.mjs b/eng/external-plugin-review-board-state.test.mjs index b52ae9537..ece1e2fc8 100644 --- a/eng/external-plugin-review-board-state.test.mjs +++ b/eng/external-plugin-review-board-state.test.mjs @@ -37,6 +37,43 @@ test("first-party submissions are fast-tracked to accept on refresh", async (t) assert.deepEqual(store.pendingReview().map((item) => item.number).sort(), [2, 3]); }); +test("first-party submissions are not fast-tracked while intake is pending", async (t) => { + const store = await createStore(t); + const pendingIssue = issue(1, ["first party", "awaiting-review"]); + 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("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"]); + pendingIssue.labels = pendingIssue.labels.filter((label) => label !== "ready-for-review"); + + await store.syncIssues([pendingIssue]); + + const item = store.getItem(1); + assert.equal(item.review, null); + assert.equal(item.reviewStatus, "unreviewed"); +}); + +test("an existing first-party fast-track is cleared while intake is pending", async (t) => { + const store = await createStore(t); + await store.syncIssues([issue(1, ["first party"])]); + const pendingIssue = issue(1, ["first party", "awaiting-review"]); + pendingIssue.labels = pendingIssue.labels.filter((label) => label !== "ready-for-review"); + + 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("fast-track applies when an existing card gains the first party label", async (t) => { const store = await createStore(t); await store.syncIssues([issue(1)]);