diff --git a/.github/extensions/external-plugin-review-board/README.md b/.github/extensions/external-plugin-review-board/README.md index e2cacb7d2..941fef6a2 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,15 @@ 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 can use fast-track or be approved; pending submissions remain visible on the board. + 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. - **Quick actions** post `/approve` (with an optional note) or `/reject ` straight to the issue after a confirm click. The reject reason is prefilled from the AI's suggested comment. diff --git a/.github/extensions/external-plugin-review-board/extension.mjs b/.github/extensions/external-plugin-review-board/extension.mjs index 68ce27a69..d29de342b 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"; @@ -50,7 +50,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 { @@ -97,7 +97,11 @@ const MAX_GUIDANCE_LENGTH = 4000; async function startRereview({ number, guidance, instanceId, send }) { const item = store.getItem(Number(number)); if (item.decision) throw new Error(`#${item.number} was already actioned (${item.decision.kind}).`); + if (item.pendingDecision) throw new Error(`#${item.number} has a posted command awaiting workflow confirmation.`); if (item.reviewStatus === "queued") throw new Error(`#${item.number} is already queued for review.`); + if (!isReadyForApproval(item.labels ?? [])) { + throw new Error(`#${item.number} is pending intake or is no longer ready-for-review.`); + } const text = String(guidance ?? "").trim().slice(0, MAX_GUIDANCE_LENGTH); const snapshots = await store.markQueued([item.number], { rereviewGuidance: text || null }); const prompt = buildRereviewPrompt({ @@ -146,8 +150,8 @@ async function postDecisionCommand({ 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" && !isReadyForApproval(status.labels)) { + throw new Error(`#${number} is pending intake or is no longer ready-for-review, so /approve would be ignored.`); } const text = stripCommand(comment, kind); @@ -250,7 +254,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 d75de61d1..3d5db0392 100644 --- a/.github/extensions/external-plugin-review-board/lib/state.mjs +++ b/.github/extensions/external-plugin-review-board/lib/state.mjs @@ -15,6 +15,57 @@ export const COLUMNS = [ ]; const HISTORY_LIMIT = 200; +export const FIRST_PARTY_LABEL = "first party"; +const MANUAL_INTAKE_OVERRIDE_LABEL = "intake-manually-overridden"; +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 +// 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?.some((label) => PENDING_INTAKE_LABELS.includes(label)) || + item.labels?.includes(MANUAL_INTAKE_OVERRIDE_LABEL) || + item.fastTrackDismissed || + item.decision || + item.review || + item.manualColumn || + item.reviewStatus === "queued" + ) { + return false; + } + item.review = { + recommendation: "accept", + fastTracked: true, + rationale: + "First-party submission (repository owned by a Microsoft/GitHub organization) that passed automated intake validation; fast-tracked to approval.", + suggestedComment: "/approve", + reviewedAt: new Date().toISOString(), + }; + item.reviewStatus = "reviewed"; + return true; +} + +function clearFirstPartyFastTrack(item) { + if ( + (item.labels?.includes(FIRST_PARTY_LABEL) && + item.labels?.includes("ready-for-review") && + !item.labels?.includes(MANUAL_INTAKE_OVERRIDE_LABEL) && + !item.labels?.some((label) => PENDING_INTAKE_LABELS.includes(label))) || + !item.review?.fastTracked || + item.reviewStatus === "queued" + ) { + return; + } + item.review = null; + if (!item.decision && !item.manualColumn) item.reviewStatus = "unreviewed"; +} function newQueueId() { return randomBytes(6).toString("base64url"); @@ -92,7 +143,10 @@ export class BoardStore { repo: this.state.repo, lastRefreshedAt: this.state.lastRefreshedAt, columns: COLUMNS, - items, + items: items.map((item) => ({ + ...item, + reviewEligible: isReadyForApproval(item.labels ?? []), + })), history: this.state.history.slice(0, 50), }; } @@ -101,6 +155,7 @@ export class BoardStore { const incoming = new Map(issues.map((issue) => [String(issue.number), issue])); const added = []; const removed = []; + const fastTracked = []; for (const [key, issue] of incoming) { const existing = this.state.items[key]; if (existing) { @@ -123,6 +178,9 @@ export class BoardStore { }; added.push(issue.number); } + const item = this.state.items[key]; + if (applyFirstPartyFastTrack(item)) fastTracked.push(issue.number); + else clearFirstPartyFastTrack(item); } for (const key of Object.keys(this.state.items)) { if (!incoming.has(key)) { @@ -132,12 +190,17 @@ export class BoardStore { } this.state.lastRefreshedAt = new Date().toISOString(); await this.commit(); - return { added, removed, total: incoming.size }; + return { added, removed, fastTracked, total: incoming.size }; } pendingReview(numbers) { - const all = Object.values(this.state.items).filter((item) => - !item.decision && !item.pendingDecision && item.reviewStatus !== "queued"); + const all = Object.values(this.state.items).filter( + (item) => + isReadyForApproval(item.labels ?? []) && + !item.decision && + !item.pendingDecision && + item.reviewStatus !== "queued", + ); if (numbers?.length) { const wanted = new Set(numbers.map(Number)); return all.filter((item) => wanted.has(item.number)); @@ -219,6 +282,7 @@ export class BoardStore { const item = this.getItem(number); if (item.decision) throw new Error(`#${number} has already been actioned.`); if (column === "unreviewed") { + if (item.review?.fastTracked || item.labels?.includes(FIRST_PARTY_LABEL)) item.fastTrackDismissed = true; item.review = null; item.reviewStatus = "unreviewed"; item.manualColumn = null; diff --git a/.github/extensions/external-plugin-review-board/public/app.js b/.github/extensions/external-plugin-review-board/public/app.js index 6cb15e984..b28f9d8c0 100644 --- a/.github/extensions/external-plugin-review-board/public/app.js +++ b/.github/extensions/external-plugin-review-board/public/app.js @@ -117,8 +117,15 @@ function signalPills(item) { pills.push(h("span", { class: `pill pill-${level.toLowerCase()}`, title: "AGT contributor risk" }, `AGT ${level}`)); } else if (label === "external-plugin-canvas") { pills.push(h("span", { class: "pill" }, "canvas")); + } else if (label === "first party") { + pills.push(h("span", { class: "pill pill-first-party", title: "Repository owned by a Microsoft/GitHub organization" }, "First party")); + } else if (label === "third party") { + pills.push(h("span", { class: "pill pill-third-party", title: "Repository owned by a third-party organization" }, "Third party")); } } + if (item.review?.fastTracked && !item.decision) { + pills.push(h("span", { class: "pill pill-approve", title: "First-party submission fast-tracked to approval" }, "Fast-tracked")); + } if (item.decision) { pills.push(h("span", { class: `pill pill-${item.decision.kind}` }, item.decision.kind === "approve" ? "Approved" : "Rejected")); } else if (item.pendingDecision) { @@ -146,7 +153,14 @@ function findItem(number) { function pendingReviewCount() { return board - ? board.items.filter((item) => !item.decision && !item.pendingDecision && item.reviewStatus !== "reviewed" && item.reviewStatus !== "queued").length + ? board.items.filter( + (item) => + item.reviewEligible && + !item.decision && + !item.pendingDecision && + item.reviewStatus !== "reviewed" && + item.reviewStatus !== "queued", + ).length : 0; } @@ -188,9 +202,10 @@ function renderBoard() { } } -function columnItems(columnId) { +function columnItems(columnId, kind) { return board.items.filter((item) => item.column === columnId && !item.decision && - !item.pendingDecision && item.reviewStatus !== "queued"); + !item.pendingDecision && item.reviewStatus !== "queued" && + (kind === "reject" || item.reviewEligible)); } function stagedComment(item) { @@ -199,18 +214,20 @@ function stagedComment(item) { } function renderColumnActions(column) { - const count = columnItems(column.id).length; return h("div", { class: "column-actions", role: "group", "aria-label": `${column.title} bulk actions` }, - ["approve", "reject", "rereview"].map((kind) => h("button", { - type: "button", class: "btn btn-small", disabled: bulkBusy || !count, - title: `Apply to all ${count} eligible items in this column, including hidden search results`, - onclick: () => bulkAction(column.id, kind), - }, `${kind === "rereview" ? "Re-review" : kind === "approve" ? "Approve" : "Reject"} all`))); + ["approve", "reject", "rereview"].map((kind) => { + const count = columnItems(column.id, kind).length; + return h("button", { + type: "button", class: "btn btn-small", disabled: bulkBusy || !count, + title: `Apply to all ${count} eligible items in this column, including hidden search results`, + onclick: () => bulkAction(column.id, kind), + }, `${kind === "rereview" ? "Re-review" : kind === "approve" ? "Approve" : "Reject"} all`); + })); } async function bulkAction(columnId, kind) { if (bulkBusy) return; - const items = columnItems(columnId).map((item) => ({ + const items = columnItems(columnId, kind).map((item) => ({ number: item.number, comment: stagedComment(item), })); if (!items.length) return; @@ -239,7 +256,7 @@ async function bulkAction(columnId, kind) { } for (const item of items) { try { - if (!columnItems(columnId).some((current) => current.number === item.number)) { + if (!columnItems(columnId, kind).some((current) => current.number === item.number)) { throw new Error("Item moved, was queued, or was already actioned; review it again."); } await api("POST", "/api/decision", { ...item, kind }); @@ -381,7 +398,8 @@ function renderDrawer() { .map((column) => h("option", { value: column.id, selected: column.id === item.column }, column.title)); ui.drawerMove.replaceChildren(...options); ui.drawerMove.disabled = Boolean(item.decision); - ui.drawerRereview.disabled = Boolean(item.decision) || item.reviewStatus === "queued"; + ui.drawerRereview.disabled = !item.reviewEligible || Boolean(item.decision) || + Boolean(item.pendingDecision) || item.reviewStatus === "queued"; if (ui.drawerRereview.disabled) hideRereview(); for (const tab of ui.tabs) { @@ -394,7 +412,7 @@ function renderDrawer() { ui.drawerFooter.hidden = Boolean(item.decision); updatePreview(); - ui.approve.disabled = bulkBusy || Boolean(item.pendingDecision); + ui.approve.disabled = bulkBusy || !item.reviewEligible || Boolean(item.pendingDecision); ui.reject.disabled ||= bulkBusy || Boolean(item.pendingDecision); renderTab(item); } @@ -467,7 +485,7 @@ function renderReview(item) { return wrapper; } const rows = [ - ["Recommendation", RECOMMENDATION_LABELS[review.recommendation] + (item.manualColumn ? ` (you moved it to ${RECOMMENDATION_LABELS[item.manualColumn]})` : "")], + ["Recommendation", RECOMMENDATION_LABELS[review.recommendation] + (review.fastTracked ? " (first-party fast-track, no AI review)" : "") + (item.manualColumn ? ` (you moved it to ${RECOMMENDATION_LABELS[item.manualColumn]})` : "")], ["Plugin", review.pluginName], ["Repository", review.repository ? h("a", { href: `https://github.com/${review.repository}`, target: "_blank", rel: "noopener noreferrer" }, review.repository) : null], ["Repo facts", review.repoFacts], @@ -566,6 +584,7 @@ function resetConfirm() { ui.approve.disabled = true; ui.reject.disabled = true; } + if (!item?.reviewEligible) ui.approve.disabled = true; } async function decide(kind) { @@ -627,6 +646,7 @@ ui.refresh.addEventListener("click", () => const parts = [`${result.total} open`]; if (result.added.length) parts.push(`${result.added.length} new`); if (result.removed.length) parts.push(`${result.removed.length} removed`); + if (result.fastTracked?.length) parts.push(`${result.fastTracked.length} first-party fast-tracked`); toast(`Refreshed: ${parts.join(", ")}`); if (selected != null && activeTab !== "review") loadDetail(selected, { force: true }); } catch (error) { diff --git a/.github/extensions/external-plugin-review-board/public/styles.css b/.github/extensions/external-plugin-review-board/public/styles.css index 71c9fb799..db726fbef 100644 --- a/.github/extensions/external-plugin-review-board/public/styles.css +++ b/.github/extensions/external-plugin-review-board/public/styles.css @@ -383,6 +383,16 @@ h2 { background: var(--red-muted); color: var(--red); } +.pill-first-party { + border-color: transparent; + background: var(--green-muted); + color: var(--green); +} +.pill-third-party { + border-color: transparent; + background: var(--purple-muted); + color: var(--purple); +} .pill-queued { border-color: transparent; background: var(--purple-muted); diff --git a/.github/extensions/external-plugin-review-board/review-guidance.md b/.github/extensions/external-plugin-review-board/review-guidance.md index 36697fa8b..a961f3b16 100644 --- a/.github/extensions/external-plugin-review-board/review-guidance.md +++ b/.github/extensions/external-plugin-review-board/review-guidance.md @@ -69,3 +69,16 @@ and read the `/approve` / `/reject` comments on the most recent ones. - `probably-reject` — likely reject, but something specific deserves a quick maintainer look. - `needs-review` — genuine judgement call. - `accept` — fits the approval pattern. + +## Ownership labels + +Intake labels submissions by who owns the plugin repository: + +- `first party` — owned by a Microsoft/GitHub organization (Microsoft, GitHub, Azure, dotnet, MicrosoftDocs, + microsoftgraph). The board fast-tracks these straight to `accept` without an AI review. If a maintainer + explicitly moves one to `Unreviewed`, that opts it out of the fast-track: review it using the same procedure, + maintainer decision pattern, and recommendation buckets as any other submission. Assess its repo fit, + usefulness, duplication, and content quality on their merits; first-party ownership is not a reason to + recommend `accept`. +- `third party` — owned by another organization. This is a signal only; apply the normal decision pattern. +- No ownership label — owned by a user account (or ownership couldn't be determined). diff --git a/.github/workflows/external-plugin-command-router.yml b/.github/workflows/external-plugin-command-router.yml index 8bce714fe..b40ded157 100644 --- a/.github/workflows/external-plugin-command-router.yml +++ b/.github/workflows/external-plugin-command-router.yml @@ -47,6 +47,7 @@ jobs: const approval = await import(pathToFileURL(path.join(process.env.GITHUB_WORKSPACE, 'eng', 'external-plugin-approval.mjs')).href); const intake = await import(pathToFileURL(path.join(process.env.GITHUB_WORKSPACE, 'eng', 'external-plugin-intake.mjs')).href); + const reviewBoard = await import(pathToFileURL(path.join(process.env.GITHUB_WORKSPACE, '.github', 'extensions', 'external-plugin-review-board', 'lib', 'state.mjs')).href); const parsedCommand = approval.parseDecisionCommand(context.payload.comment.body); core.setOutput('should-run', 'false'); @@ -85,7 +86,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') || + reviewBoard.isReadyForApproval([...labelNames]); const canReject = !labelNames.has('approved'); if (parsedCommand.command === 'approve' && !canApprove) { @@ -547,6 +549,23 @@ jobs: return; } + try { + await github.rest.issues.getLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: 'intake-manually-overridden' + }); + } catch (error) { + if (error.status !== 404) throw error; + await github.rest.issues.createLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: 'intake-manually-overridden', + color: 'BFD4F2', + description: 'Submission was manually moved to review without passing automated intake gates' + }); + } + await github.rest.reactions.createForIssueComment({ owner: context.repo.owner, repo: context.repo.repo, @@ -562,6 +581,7 @@ jobs: desiredLabels: new Set([ 'external-plugin', 'ready-for-review', + 'intake-manually-overridden', ...(labelNames.has('external-plugin-canvas') ? ['external-plugin-canvas'] : []) ]) }); 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/.github/workflows/setup-labels.yml b/.github/workflows/setup-labels.yml index 803165f48..d529107b4 100644 --- a/.github/workflows/setup-labels.yml +++ b/.github/workflows/setup-labels.yml @@ -52,6 +52,14 @@ jobs: color: '1D76DB', description: 'External plugin submission includes a canvas extension' }, + 'first party': { + color: '1D76DB', + description: 'External plugin repository is owned by a Microsoft or GitHub first-party organization' + }, + 'third party': { + color: '5319E7', + description: 'External plugin repository is owned by another organization' + }, 'hooks': { color: 'C2E0C6', description: 'PR touches hooks' @@ -73,6 +81,10 @@ jobs: color: 'D93F0B', description: 'Submission has quality-gate findings that submitter must fix before maintainer review' }, + 'intake-manually-overridden': { + color: 'BFD4F2', + description: 'Submission was manually moved to review without passing automated intake gates' + }, 'approved': { color: '1D76DB', description: 'Submission was approved by a maintainer' diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 95e2ac5fd..2056c09c1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -253,6 +253,7 @@ The public-submission policy builds on those rules and also requires `license` p 1. **Open an issue** using the external plugin issue form. Automation applies the `external-plugin` and `awaiting-review` labels. 2. **Automated intake validation** checks that the required fields are present and correctly formatted for a GitHub-hosted plugin. Invalid submissions are labeled `requires-submitter-fixes` with a comment explaining what must be fixed before maintainer review. + Intake also checks the repository owner's type using the GitHub API. Organization-owned repositories receive `first party` for the `Microsoft`, `GitHub`, `Azure`, `dotnet`, `MicrosoftDocs`, or `microsoftgraph` organizations (case-insensitive), or `third party` for any other organization. User-owned repositories receive neither label. On the triage board, `first party` skips AI review and creates an `Accept` recommendation with `/approve`; this is only a triage recommendation. Submissions must still pass automated validation and quality gates, and a human maintainer must review and approve them before listing. Ownership labels are updated on each intake pass. 3. **Automated quality gates** run after metadata validation: - `vally lint` against the submitted plugin path/ref/sha - install smoke test via Copilot CLI against an ephemeral marketplace entry generated from the submission @@ -289,6 +290,8 @@ Maintainers are responsible for confirming that the submission: ##### Review cadence and label semantics - `external-plugin`: applied to every public external plugin submission and retained on approved issues so scheduled review automation can find them later +- `first party`: the submitted repository is owned by a first-party organization (`Microsoft`, `GitHub`, `Azure`, `dotnet`, `MicrosoftDocs`, or `microsoftgraph`) +- `third party`: the submitted repository is owned by another organization; user-owned repositories have neither ownership label - `awaiting-review`: initial intake state before automation finishes validating the issue - `ready-for-review`: the issue passed automated intake checks and is waiting on a maintainer decision - `requires-submitter-fixes`: automated intake found metadata or quality-gate issues; submitter updates are required before human review diff --git a/eng/board-bulk-actions.test.mjs b/eng/board-bulk-actions.test.mjs index 197cf0da4..195b911da 100644 --- a/eng/board-bulk-actions.test.mjs +++ b/eng/board-bulk-actions.test.mjs @@ -50,6 +50,7 @@ for (const boardName of ["open-pr-board", "external-plugin-review-board"]) { const item = (number, extra = {}) => ({ number, column: "test", headRefOid: "first", review: { suggestedComment: isPR ? `Feedback ${number}` : `/reject Feedback ${number}` }, + ...(!isPR ? { reviewEligible: true } : {}), ...extra, }); const payload = (args) => isPR ? args[1].body : args[2]; @@ -129,16 +130,36 @@ for (const boardName of ["open-pr-board", "external-plugin-review-board"]) { test("external-plugin-review-board: pending review count excludes posted decisions", async () => { const ui = await createUI("external-plugin-review-board", [ - { number: 1, reviewStatus: "unreviewed" }, - { number: 2, reviewStatus: "unreviewed", pendingDecision: { kind: "approve" } }, + { number: 1, reviewEligible: true, reviewStatus: "unreviewed" }, + { number: 2, reviewEligible: true, reviewStatus: "unreviewed", pendingDecision: { kind: "approve" } }, { number: 3, reviewStatus: "reviewed" }, { number: 4, reviewStatus: "queued" }, { number: 5, decision: { kind: "reject" } }, + { number: 6, reviewEligible: false, reviewStatus: "unreviewed" }, ]); assert.equal(ui.context.pendingReviewCount(), 1); }); +for (const kind of ["approve", "rereview"]) { + test(`external-plugin-review-board: bulk ${kind} excludes pending intake cards`, async () => { + const ui = await createUI("external-plugin-review-board", [ + { number: 1, column: "test", reviewEligible: true }, + { number: 2, column: "test", reviewEligible: false }, + ]); + + await ui.context.bulkAction("test", kind); + + assert.equal(ui.calls.length, 1); + if (kind === "rereview") { + assert.deepEqual([...ui.calls[0][2].numbers], [1]); + } else { + assert.equal(ui.calls[0][2].number, 1); + } + assert.doesNotMatch(ui.confirmations[0], /#2/); + }); +} + test("PR bulk close requires and posts the staged reason", async () => { const ui = await createUI("open-pr-board", [{ number: 1, column: "close", review: { suggestedComment: "Duplicate" } }]); await ui.context.bulkAction("close", "close"); 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 b73c72169..5c60e5d5a 100644 --- a/eng/external-plugin-intake-state.mjs +++ b/eng/external-plugin-intake-state.mjs @@ -7,6 +7,14 @@ export const EXTERNAL_PLUGIN_INTAKE_LABELS = Object.freeze({ color: "1D76DB", description: "External plugin submission includes a canvas extension", }, + "first party": { + color: "1D76DB", + description: "External plugin repository is owned by a Microsoft or GitHub first-party organization", + }, + "third party": { + color: "5319E7", + description: "External plugin repository is owned by another organization", + }, "awaiting-review": { color: "FBCA04", description: "Submission is waiting for automated intake validation", @@ -19,6 +27,10 @@ export const EXTERNAL_PLUGIN_INTAKE_LABELS = Object.freeze({ color: "D93F0B", description: "Submission has quality-gate findings that submitter must fix before maintainer review", }, + "intake-manually-overridden": { + color: "BFD4F2", + description: "Submission was manually moved to review without passing automated intake gates", + }, approved: { color: "1D76DB", description: "Submission was approved by a maintainer", @@ -35,6 +47,7 @@ const EXTERNAL_PLUGIN_INTAKE_SYNC_LABELS = Object.freeze([ "awaiting-review", "ready-for-review", "requires-submitter-fixes", + "intake-manually-overridden", "rejected", ]); @@ -53,7 +66,14 @@ async function removeLabel({ github, owner, repo, issueNumber, name }) { } } -export async function syncExternalPluginIntakeLabels({ github, owner, repo, issueNumber, desiredLabels }) { +export async function syncExternalPluginIntakeLabels({ + github, + owner, + repo, + issueNumber, + desiredLabels, + syncOwnershipLabels = false, +}) { const currentLabels = await github.paginate(github.rest.issues.listLabelsOnIssue, { owner, repo, @@ -63,7 +83,10 @@ export async function syncExternalPluginIntakeLabels({ github, owner, repo, issu const currentManagedLabels = currentLabels .map((label) => label.name) - .filter((name) => EXTERNAL_PLUGIN_INTAKE_SYNC_LABELS.includes(name)); + .filter((name) => + EXTERNAL_PLUGIN_INTAKE_SYNC_LABELS.includes(name) || + (syncOwnershipLabels && ["first party", "third party"].includes(name)) + ); const labelsToAdd = [...desiredLabels].filter((name) => !currentManagedLabels.includes(name)); const labelsToRemove = currentManagedLabels.filter((name) => !desiredLabels.has(name)); @@ -82,6 +105,19 @@ export async function syncExternalPluginIntakeLabels({ github, owner, repo, issu } } +export async function markExternalPluginIntakeAwaitingReview({ github, owner, repo, issueNumber }) { + 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({ github, owner, @@ -137,6 +173,10 @@ export async function applyExternalPluginIntakeEvaluation({ if (evaluation.isCanvasPlugin) { desiredLabels.add("external-plugin-canvas"); } + const ownershipLabel = evaluation.reviewSignals?.repository?.ownershipLabel; + if (ownershipLabel === "first party" || ownershipLabel === "third party") { + desiredLabels.add(ownershipLabel); + } await syncExternalPluginIntakeLabels({ github, @@ -144,6 +184,7 @@ export async function applyExternalPluginIntakeEvaluation({ repo, issueNumber, desiredLabels, + syncOwnershipLabels: true, }); await upsertExternalPluginIntakeComment({ diff --git a/eng/external-plugin-intake-state.test.mjs b/eng/external-plugin-intake-state.test.mjs new file mode 100644 index 000000000..b4bfc7cda --- /dev/null +++ b/eng/external-plugin-intake-state.test.mjs @@ -0,0 +1,128 @@ +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 })); + }, + rest: { + 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() { + return { data: [] }; + }, + async createComment() {}, + }, + }, + }; +} + +test("a fresh intake evaluation clears the manual override label", async () => { + const github = mockGitHub([ + "external-plugin", + "ready-for-review", + "first party", + "intake-manually-overridden", + ]); + + await applyExternalPluginIntakeEvaluation({ + github, + owner: "github", + repo: "awesome-copilot", + issueNumber: 1, + evaluation: { + intakeState: "ready-for-review", + reviewSignals: { repository: { ownershipLabel: "first party" } }, + commentMarker: "", + commentBody: "Intake result", + }, + }); + + assert.deepEqual(github.labels, new Set(["external-plugin", "ready-for-review", "first party"])); +}); + +for (const state of ["ready-for-review", "requires-submitter-fixes", "awaiting-review", "rejected"]) { + for (const ownershipLabel of ["first party", "third party", null, undefined]) { + test(`intake syncs ${ownershipLabel ?? "no ownership label"} in ${state} state`, async () => { + const github = mockGitHub(["external-plugin", "first party", "third party", "unrelated"]); + const { desiredLabels } = await applyExternalPluginIntakeEvaluation({ + github, + owner: "github", + repo: "awesome-copilot", + issueNumber: 1, + evaluation: { + intakeState: state, + isCanvasPlugin: true, + reviewSignals: { repository: { ownershipLabel } }, + commentMarker: "", + commentBody: "Intake result", + }, + }); + + const expected = ["external-plugin", state, "external-plugin-canvas"]; + if (ownershipLabel) expected.push(ownershipLabel); + assert.deepEqual(desiredLabels, new Set(expected)); + assert.deepEqual(github.labels, new Set([...expected, "unrelated"])); + }); + } +} + +test("state-only label sync preserves ownership labels for maintainer overrides and PR checks", async () => { + const github = mockGitHub(["external-plugin", "requires-submitter-fixes", "first party"]); + await syncExternalPluginIntakeLabels({ + github, + owner: "github", + repo: "awesome-copilot", + issueNumber: 1, + desiredLabels: new Set(["external-plugin", "ready-for-review"]), + }); + + 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(pendingAddition >= 0 && readyRemoval > pendingAddition); +}); diff --git a/eng/external-plugin-intake.mjs b/eng/external-plugin-intake.mjs index f6bf57430..b1418c4eb 100644 --- a/eng/external-plugin-intake.mjs +++ b/eng/external-plugin-intake.mjs @@ -75,6 +75,14 @@ const MARKETING_SIGNAL_PATTERNS = Object.freeze([ ["trial", /\bfree\s+trial\b|\bstart\s+your\s+trial\b|\bget\s+started\s+free\b/i], ["checkout", /\bstripe\b|\bcheckout\b|\bsubscribe\s+now\b|\bbuy\s+now\b/i], ]); +const FIRST_PARTY_ORGANIZATIONS = new Set([ + "microsoft", + "github", + "azure", + "dotnet", + "microsoftdocs", + "microsoftgraph", +]); const EXTERNAL_PLUGIN_ROOT_MANIFEST_PATHS = Object.freeze([ ".github/plugin/plugin.json", ".plugin/plugin.json", @@ -532,6 +540,11 @@ function buildRepositorySignals(repository) { return { status: "pass", + ownershipLabel: repository.owner?.type === "Organization" + ? FIRST_PARTY_ORGANIZATIONS.has(repository.owner.login?.toLowerCase()) + ? "first party" + : "third party" + : null, age_days: ageDays, stars: repository.stargazers_count, watchers: repository.subscribers_count, @@ -1208,6 +1221,13 @@ function buildReviewSignalsCommentSection(reviewSignals) { "|---|---|", ]; if (repository?.status === "pass") { + const ownershipNote = { + "first party": "Organization-owned (first party: Microsoft/GitHub organization)", + "third party": "Organization-owned (third party)", + }[repository.ownershipLabel]; + if (ownershipNote) { + rows.push(`| Repository owner | ${ownershipNote} |`); + } rows.push( `| Repository age | ${repository.age_days === undefined ? "unknown" : `${repository.age_days} day(s)`} |`, `| Repository activity | ${repository.signals?.length ? repository.signals.join("; ") : "no configured signal"} |`, @@ -1346,7 +1366,7 @@ export async function evaluateExternalPluginIssue({ issue, token, runId, owner, } } - if (parsed.plugin?.source?.repo && (parsed.plugin?.source?.ref || parsed.plugin?.source?.sha)) { + if (parsed.plugin?.source?.repo) { repositorySignals = await validateRemoteRepository(parsed.plugin.source.repo, parsed.plugin.source, errors, warnings, token); } const homepageSignals = await inspectHomepage(parsed.plugin?.homepage); diff --git a/eng/external-plugin-intake.test.mjs b/eng/external-plugin-intake.test.mjs index 66f3ee4af..0f8e06809 100644 --- a/eng/external-plugin-intake.test.mjs +++ b/eng/external-plugin-intake.test.mjs @@ -567,11 +567,12 @@ test("pinned homepage dispatcher connects to the validated address and preserves } }); -function installMockFetch() { +function installMockFetch({ repositoryOwner = { type: "User", login: "octo" }, repositoryStatus = 200 } = {}) { global.fetch = async (url) => { const requestUrl = String(url); if (requestUrl === `https://api.github.com/repos/${INTAKE_REPO}`) { return jsonResponse({ + owner: repositoryOwner, private: false, archived: false, created_at: new Date().toISOString(), @@ -579,7 +580,7 @@ function installMockFetch() { subscribers_count: 3, forks_count: 0, open_issues_count: 4, - }); + }, { status: repositoryStatus }); } if ( @@ -602,6 +603,75 @@ function installMockFetch() { }; } +for (const [type, login, expectedLabel] of [ + ["Organization", "github", "first party"], + ["Organization", "Microsoft", "first party"], + ["Organization", "MICROSOFT", "first party"], + ["Organization", "GitHub", "first party"], + ["Organization", "Azure", "first party"], + ["Organization", "azure", "first party"], + ["Organization", "dotnet", "first party"], + ["Organization", "DotNet", "first party"], + ["Organization", "MicrosoftDocs", "first party"], + ["Organization", "microsoftdocs", "first party"], + ["Organization", "microsoftgraph", "first party"], + ["Organization", "MicrosoftGraph", "first party"], + ["Organization", "vendor", "third party"], + ["Organization", "github-vendor", "third party"], + ["User", "microsoft", null], + ["User", "octo", null], +]) { + test(`evaluateExternalPluginIssue classifies ${type} owner ${login}`, async () => { + installMockFetch({ repositoryOwner: { type, login } }); + const result = await evaluateExternalPluginIssue({ + issue: { body: buildIssueBody({ sha: PROVIDED_SHA }) }, + }); + + assert.equal(result.valid, true); + assert.equal(result.reviewSignals.repository.ownershipLabel, expectedLabel); + const mergedResult = applyQualityGateResult(result, { overall_status: "pass" }); + assert.equal(mergedResult.reviewSignals.repository.ownershipLabel, expectedLabel); + const expectedNote = { + "first party": "| Repository owner | Organization-owned (first party: Microsoft/GitHub organization) |", + "third party": "| Repository owner | Organization-owned (third party) |", + }[expectedLabel]; + for (const body of [result.commentBody, mergedResult.commentBody]) { + if (expectedNote) { + assert.ok(body.includes(expectedNote), `expected ownership note in:\n${body}`); + } else { + assert.doesNotMatch(body, /Repository owner/); + } + } + }); +} + +for (const status of [404, 503]) { + test(`evaluateExternalPluginIssue does not classify unavailable repository metadata (${status})`, async () => { + installMockFetch({ repositoryStatus: status }); + const result = await evaluateExternalPluginIssue({ + issue: { body: buildIssueBody({ sha: PROVIDED_SHA }) }, + }); + + assert.equal(result.reviewSignals.repository.ownershipLabel, undefined); + if (status === 404) { + assert.match(result.errors.join("\n"), /was not found/); + } else { + assert.match(result.warnings.join("\n"), /could not verify GitHub repository/); + } + }); +} + +test("evaluateExternalPluginIssue classifies ownership even when the submission has no release locator", async () => { + installMockFetch({ repositoryOwner: { type: "Organization", login: "github" } }); + const result = await evaluateExternalPluginIssue({ + issue: { body: buildIssueBody({}) }, + }); + + assert.equal(result.valid, false); + assert.equal(result.reviewSignals.repository.ownershipLabel, "first party"); + assert.match(result.commentBody, /\| Repository owner \| Organization-owned \(first party/); +}); + test("evaluateExternalPluginIssue fails when ref and sha resolve to different commits", async () => { installMockFetch(); const issue = { body: buildIssueBody({ ref: "v1.2.3", sha: PROVIDED_SHA }) }; diff --git a/eng/external-plugin-review-board-state.test.mjs b/eng/external-plugin-review-board-state.test.mjs new file mode 100644 index 000000000..b93a6469e --- /dev/null +++ b/eng/external-plugin-review-board-state.test.mjs @@ -0,0 +1,293 @@ +import assert from "node:assert/strict"; +import { mkdtemp, rm } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { test } from "node:test"; + +import { + BoardStore, + effectiveColumn, + isReadyForApproval, +} from "../.github/extensions/external-plugin-review-board/lib/state.mjs"; + +function issue(number, labels = []) { + return { + number, + title: `[External Plugin]: Example ${number}`, + author: "contributor", + url: `https://github.com/github/awesome-copilot/issues/${number}`, + labels: ["external-plugin", "ready-for-review", ...labels], + }; +} + +async function createStore(t) { + const dir = await mkdtemp(path.join(os.tmpdir(), "epr-board-")); + t.after(() => rm(dir, { recursive: true, force: true })); + return new BoardStore(path.join(dir, "board.json"), "github/awesome-copilot"); +} + +test("first-party submissions are fast-tracked to accept on refresh", async (t) => { + const store = await createStore(t); + const result = await store.syncIssues([issue(1, ["first party"]), issue(2, ["third party"]), issue(3)]); + + assert.deepEqual(result.fastTracked, [1]); + const first = store.getItem(1); + assert.equal(effectiveColumn(first), "accept"); + assert.equal(first.reviewStatus, "reviewed"); + assert.equal(first.review.fastTracked, true); + assert.equal(first.review.suggestedComment, "/approve"); + assert.equal(effectiveColumn(store.getItem(2)), "unreviewed"); + assert.equal(effectiveColumn(store.getItem(3)), "unreviewed"); + 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 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("manually overridden first-party submissions are never fast-tracked", async (t) => { + const store = await createStore(t); + const overriddenIssue = issue(1, ["first party", "intake-manually-overridden"]); + await store.syncIssues([overriddenIssue]); + + const item = store.getItem(1); + assert.equal(item.review, null); + assert.equal(item.reviewStatus, "unreviewed"); + assert.equal(effectiveColumn(item), "unreviewed"); + + await store.syncIssues([issue(1, ["first party", "intake-manually-overridden"])]); + assert.equal(store.getItem(1).review, null); + + const validationPassed = await store.syncIssues([issue(1, ["first party"])]); + assert.deepEqual(validationPassed.fastTracked, [1]); + assert.equal(store.getItem(1).review.fastTracked, true); +}); + +test("a manual override clears an existing synthetic first-party fast-track", async (t) => { + const store = await createStore(t); + await store.syncIssues([issue(1, ["first party"])]); + + const result = await store.syncIssues([issue(1, ["first party", "intake-manually-overridden"])]); + + assert.deepEqual(result.fastTracked, []); + assert.equal(store.getItem(1).review, null); + assert.equal(store.getItem(1).reviewStatus, "unreviewed"); + assert.equal(effectiveColumn(store.getItem(1)), "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("pending intake cards stay visible but become review-eligible only when ready", () => { + const store = new BoardStore("unused", "github/awesome-copilot"); + const existingReview = { recommendation: "needs-review", rationale: "Keep this review." }; + const queuedReview = { recommendation: "accept", rationale: "Keep the queued card's prior review." }; + store.state.items = { + 1: { number: 1, labels: ["external-plugin", "awaiting-review"], reviewStatus: "unreviewed" }, + 2: { + number: 2, + labels: ["external-plugin", "requires-submitter-fixes"], + reviewStatus: "reviewed", + review: existingReview, + }, + 3: { number: 3, labels: ["external-plugin", "ready-for-review"], reviewStatus: "unreviewed" }, + 4: { + number: 4, + labels: ["external-plugin", "awaiting-review"], + reviewStatus: "queued", + review: queuedReview, + queueId: "queued-review", + }, + }; + + const pendingSnapshot = store.snapshot(); + assert.deepEqual(pendingSnapshot.items.map((item) => item.number), [4, 3, 2, 1]); + assert.deepEqual( + pendingSnapshot.items.filter((item) => item.reviewEligible).map((item) => item.number), + [3], + ); + assert.deepEqual(store.pendingReview().map((item) => item.number), [3]); + assert.deepEqual(store.pendingReview([1, 2, 3, 4]).map((item) => item.number), [3]); + + store.getItem(1).labels = ["external-plugin", "ready-for-review"]; + store.getItem(2).labels = ["external-plugin", "ready-for-review"]; + store.getItem(4).labels = ["external-plugin", "ready-for-review"]; + + assert.deepEqual(store.pendingReview().map((item) => item.number), [1, 3]); + assert.equal(store.getItem(2).review, existingReview); + assert.equal(store.getItem(2).reviewStatus, "reviewed"); + assert.equal(store.getItem(4).review, queuedReview); + assert.equal(store.getItem(4).reviewStatus, "queued"); + assert.equal(store.getItem(4).queueId, "queued-review"); +}); + +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)]); + const result = await store.syncIssues([issue(1, ["first party"])]); + + assert.deepEqual(result.fastTracked, [1]); + assert.equal(effectiveColumn(store.getItem(1)), "accept"); +}); + +test("removing the first-party label clears the synthetic fast-track", async (t) => { + const store = await createStore(t); + await store.syncIssues([issue(1, ["first party"])]); + + await store.syncIssues([issue(1, ["third party"])]); + + const item = store.getItem(1); + assert.equal(item.review, null); + assert.equal(item.reviewStatus, "unreviewed"); + assert.equal(effectiveColumn(item), "unreviewed"); + assert.deepEqual(store.pendingReview().map((pending) => pending.number), [1]); +}); + +test("removing the first-party label preserves decisions, manual buckets, and queued re-reviews", async (t) => { + const store = await createStore(t); + await store.syncIssues([ + issue(1, ["first party"]), + issue(2, ["first party"]), + issue(3, ["first party"]), + ]); + await store.recordDecision(1, { kind: "approve", at: new Date().toISOString() }); + await store.moveItem(2, "probably-reject"); + await store.markQueued([3], { rereviewGuidance: "Re-check the submission." }); + const queued = store.getItem(3); + const queueId = queued.queueId; + const queuedReview = queued.review; + + await store.syncIssues([issue(1, ["third party"]), issue(2, ["third party"]), issue(3, ["third party"])]); + + assert.equal(store.getItem(1).review, null); + assert.equal(store.getItem(1).decision.kind, "approve"); + assert.equal(effectiveColumn(store.getItem(1)), "actioned"); + assert.equal(store.getItem(2).review, null); + assert.equal(store.getItem(2).manualColumn, "probably-reject"); + assert.equal(effectiveColumn(store.getItem(2)), "probably-reject"); + assert.equal(store.getItem(3).reviewStatus, "queued"); + assert.equal(store.getItem(3).queueId, queueId); + assert.equal(store.getItem(3).rereviewGuidance, "Re-check the submission."); + 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)]); + store.getItem(1).review = { recommendation: "needs-review", reviewedAt: new Date().toISOString() }; + store.getItem(1).reviewStatus = "reviewed"; + await store.moveItem(2, "probably-reject"); + + const result = await store.syncIssues([issue(1, ["first party"]), issue(2, ["first party"])]); + + assert.deepEqual(result.fastTracked, []); + assert.equal(effectiveColumn(store.getItem(1)), "needs-review"); + assert.equal(effectiveColumn(store.getItem(2)), "probably-reject"); +}); + +test("moving a fast-tracked card to unreviewed opts it out of fast-track", async (t) => { + const store = await createStore(t); + await store.syncIssues([issue(1, ["first party"])]); + await store.moveItem(1, "unreviewed"); + + const result = await store.syncIssues([issue(1, ["first party"])]); + + assert.deepEqual(result.fastTracked, []); + assert.equal(effectiveColumn(store.getItem(1)), "unreviewed"); + assert.deepEqual(store.pendingReview().map((item) => item.number), [1]); +}); diff --git a/eng/external-plugin-review-board-ui.test.mjs b/eng/external-plugin-review-board-ui.test.mjs new file mode 100644 index 000000000..16dfe87d4 --- /dev/null +++ b/eng/external-plugin-review-board-ui.test.mjs @@ -0,0 +1,98 @@ +import assert from "node:assert/strict"; +import { readFile } from "node:fs/promises"; +import vm from "node:vm"; +import { test } from "node:test"; + +const appSource = await readFile( + new URL("../.github/extensions/external-plugin-review-board/public/app.js", import.meta.url), + "utf8", +); + +function button() { + return { + disabled: false, + textContent: "", + classList: { remove() {} }, + }; +} + +function createContext(item, { bulkBusy = false } = {}) { + const ui = { + drawerKicker: {}, + drawerTitle: {}, + drawerMeta: {}, + drawerLink: {}, + drawerMove: { replaceChildren() {} }, + drawerRereview: {}, + tabs: [], + drawerFooter: {}, + approve: button(), + reject: button(), + }; + const context = vm.createContext({ + board: { columns: [] }, + bulkBusy, + clearTimeout() {}, + details: new Map(), + findItem: () => item, + h: (...args) => args, + hideRereview() {}, + itemName: () => "Submission", + pendingConfirm: { timer: null, kind: null }, + relativeTime: () => "now", + renderTab() {}, + selected: item.number, + ui, + updatePreview() {}, + }); + return { context, ui }; +} + +function evaluateFunction(context, source, signature, nextSignature) { + const start = source.indexOf(signature); + const end = source.indexOf(nextSignature, start); + assert.notEqual(start, -1, `Expected to find ${signature}`); + assert.notEqual(end, -1, `Expected to find ${nextSignature}`); + vm.runInContext(source.slice(start, end), context); +} + +test("Approve is disabled for submissions that are not review-eligible", () => { + const item = { number: 42, author: "contributor", labels: ["awaiting-review"], reviewEligible: false }; + const { context, ui } = createContext(item); + + evaluateFunction(context, appSource, "function renderDrawer()", "function renderTab("); + context.renderDrawer(); + + assert.equal(ui.approve.disabled, true); + assert.equal(ui.reject.disabled, false); + + evaluateFunction(context, appSource, "function resetConfirm()", "async function decide("); + context.resetConfirm(); + + assert.equal(ui.approve.disabled, true); + assert.equal(ui.reject.disabled, false); +}); + +test("Approve retains its existing eligible, pending-decision, and bulk-busy states", () => { + const eligible = { number: 42, author: "contributor", labels: [], reviewEligible: true }; + + const available = createContext(eligible); + evaluateFunction(available.context, appSource, "function renderDrawer()", "function renderTab("); + available.context.renderDrawer(); + assert.equal(available.ui.approve.disabled, false); + + const pending = createContext({ ...eligible, pendingDecision: { kind: "approve" } }); + evaluateFunction(pending.context, appSource, "function renderDrawer()", "function renderTab("); + pending.context.renderDrawer(); + assert.equal(pending.ui.approve.disabled, true); + + const busy = createContext(eligible, { bulkBusy: true }); + evaluateFunction(busy.context, appSource, "function renderDrawer()", "function renderTab("); + busy.context.renderDrawer(); + assert.equal(busy.ui.approve.disabled, true); + + const reset = createContext({ ...eligible, pendingDecision: { kind: "approve" } }); + evaluateFunction(reset.context, appSource, "function resetConfirm()", "async function decide("); + reset.context.resetConfirm(); + assert.equal(reset.ui.approve.disabled, true); +}); diff --git a/eng/external-plugin-review-board.test.mjs b/eng/external-plugin-review-board.test.mjs index e0f5a8222..dde506671 100644 --- a/eng/external-plugin-review-board.test.mjs +++ b/eng/external-plugin-review-board.test.mjs @@ -7,8 +7,8 @@ import { BoardStore } from "../.github/extensions/external-plugin-review-board/l test("external-plugin-review-board excludes items with pending decisions from AI review", () => { const store = new BoardStore("unused", "github/awesome-copilot"); store.state.items = { - "1": { number: 1, reviewStatus: "unreviewed" }, - "2": { number: 2, reviewStatus: "unreviewed", pendingDecision: { kind: "approve" } }, + "1": { number: 1, labels: ["ready-for-review"], reviewStatus: "unreviewed" }, + "2": { number: 2, labels: ["ready-for-review"], reviewStatus: "unreviewed", pendingDecision: { kind: "approve" } }, "3": { number: 3, reviewStatus: "queued" }, "4": { number: 4, reviewStatus: "unreviewed", decision: { kind: "reject" } }, };