From 3ad1aad4598e86a514f6a849b5cbb47e18586854 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Tue, 21 Jul 2026 17:31:58 -0700 Subject: [PATCH 1/4] Assign per-language architects when a review is ready When triage marks a board-review issue ready-for-review, resolve one architect per selected language from the shared api-review-approvers.yml roster (round-robin by issue number) and assign them. Assignment fires GitHub's native notifications so architects are reached through their own preferences. The resolved handles are shown in the existing "materials verified" comment (single comment, no extra ping). Idempotent: re-edits do not re-assign. - Add rust data-plane approver (heaths) to api-review-approvers.yml. - Surface selected languages with no configured architect as "unassigned" in the triage comment instead of silently omitting them. - Document the assignment step and C++/Rust language labels in ARCH-BOARD-REVIEW-PROCESS.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f922dad1-6173-4c65-b4bb-03f056572963 --- .github/api-review-approvers.yml | 2 + .../ARCH-BOARD-REVIEW-PROCESS.md | 39 +++-- .../src/arch-board-review/assign-reviewers.js | 137 ++++++++++++++++ .../workflows/src/arch-board-review/triage.js | 34 +++- .../assign-reviewers.test.js | 152 ++++++++++++++++++ .../test/arch-board-review/triage.test.js | 9 ++ 6 files changed, 358 insertions(+), 15 deletions(-) create mode 100644 .github/workflows/src/arch-board-review/assign-reviewers.js create mode 100644 .github/workflows/test/arch-board-review/assign-reviewers.test.js diff --git a/.github/api-review-approvers.yml b/.github/api-review-approvers.yml index 955876bd269b..4371373499df 100644 --- a/.github/api-review-approvers.yml +++ b/.github/api-review-approvers.yml @@ -19,6 +19,8 @@ data-plane: - maorleger go: - jhendrixMSFT + rust: + - heaths management-plane: all: diff --git a/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md b/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md index d07c2a8ecd4a..b64158189e9b 100644 --- a/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md +++ b/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md @@ -21,10 +21,12 @@ Service team fills out issue form │ • Sync language labels │ └─────────────────────────────┘ ↓ - ┌──────────────┐ ┌──────────────┐ - │ All valid? │───→│ ready-for- │ - │ │ Y │ review label │ - └──────────────┘ └──────────────┘ + ┌──────────────┐ ┌───────────────────────────┐ + │ All valid? │───→│ ready-for-review label │ + │ │ Y │ + assign one architect │ + │ │ │ per language (notifies │ + │ │ │ them via GitHub) │ + └──────────────┘ └───────────────────────────┘ │ N ↓ ┌──────────────┐ @@ -49,13 +51,13 @@ Architect applies -api-approved label ### Applied by automation -| Label | When applied | Meaning | -| -------------------------------------------- | --------------------------------- | ------------------------------- | -| `architecture` | On issue creation (from template) | Issue is a board review request | -| `board-review` | On issue creation (from template) | Issue is a board review request | -| `.NET`, `Java`, `Python`, `TypeScript`, `Go` | Triage detects selected language | Language needs review | -| `ready-for-review` | All required artifacts validated | Ready for architects | -| `needs-info` | Missing or invalid artifacts | Service team needs to fix | +| Label | When applied | Meaning | +| ----------------------------------------------------------- | --------------------------------- | ------------------------------- | +| `architecture` | On issue creation (from template) | Issue is a board review request | +| `board-review` | On issue creation (from template) | Issue is a board review request | +| `.NET`, `Java`, `Python`, `TypeScript`, `Go`, `C++`, `Rust` | Triage detects selected language | Language needs review | +| `ready-for-review` | All required artifacts validated | Ready for architects | +| `needs-info` | Missing or invalid artifacts | Service team needs to fix | ### Applied by architects (manually) @@ -78,6 +80,21 @@ When all selected languages have `-api-approved`, the issue is auto-closed Unauthorized label additions are reverted with a comment. +## Reviewer Assignment + +When an issue reaches `ready-for-review`, `assign-reviewers.js` assigns one +architect per selected language, chosen from the same +`api-review-approvers.yml` roster (round-robin, keyed off the issue number). +Assigning the issue triggers GitHub's native notifications, so architects are +reached through their own notification preferences. The assigned handles are +listed in the triage "materials verified" comment for visibility. + +For **management plane** issues, the management-plane approvers are included in +the candidate pool alongside the language-specific architects. + +Assignment is idempotent: an architect already assigned is not re-assigned, so +subsequent edits to a ready issue do not re-notify. + ## Approver Configuration Approvers are defined in `.github/api-review-approvers.yml`: diff --git a/.github/workflows/src/arch-board-review/assign-reviewers.js b/.github/workflows/src/arch-board-review/assign-reviewers.js new file mode 100644 index 000000000000..582f2a139e8f --- /dev/null +++ b/.github/workflows/src/arch-board-review/assign-reviewers.js @@ -0,0 +1,137 @@ +/** + * Assign Reviewers — routes a ready review request to per-language architects. + * + * Called by triage once an issue reaches `ready-for-review`. For each selected + * language it picks one architect (round-robin, keyed off the issue number) from + * the shared `api-review-approvers.yml` roster and assigns them to the issue. + * Assigning triggers GitHub's native notifications, so architects are reached + * through their own notification preferences. + * + * The roster is the same source of truth used by approval validation, so there is + * no separate reviewer list to keep in sync. Management-plane issues additionally + * consider the management-plane approvers. + * + * The assignment itself is the notification mechanism; the resolved handles are + * surfaced in triage's "materials verified" comment for visibility. Idempotent: + * only newly resolved assignees are added, so edits to an already-assigned issue + * do not re-assign or re-notify. + */ +import { readFile } from "node:fs/promises"; + +import { getSelectedLanguages } from "./issue-parsing.js"; +import { + getApproversForLanguage, + getManagementApprovers, + isManagementPlaneIssue, + loadApproversConfig, +} from "./approval-check.js"; + +const DEFAULT_APPROVERS_PATH = new URL("../../../api-review-approvers.yml", import.meta.url); + +/** + * Deterministically pick one candidate, distributing across issues by number. + * + * @param {string[]} candidates + * @param {number} issueNumber + * @returns {string | null} + */ +function pickReviewer(candidates, issueNumber) { + if (!candidates || candidates.length === 0) { + return null; + } + return candidates[issueNumber % candidates.length]; +} + +/** + * Resolve one architect per selected language from the roster. + * + * @param {object} params + * @param {string} params.issueBody + * @param {number} params.issueNumber + * @param {object} params.approversConfig + * @returns {{ byLanguage: { language: string, reviewer: string }[], assignees: string[], unassigned: string[] }} + * `unassigned` lists selected languages that have no configured architect. + */ +function resolveAssignments({ issueBody, issueNumber, approversConfig }) { + const selectedLanguages = getSelectedLanguages(issueBody); + const managementReviewers = isManagementPlaneIssue(issueBody) + ? getManagementApprovers(approversConfig) + : []; + + const byLanguage = []; + const assigneeSet = new Set(); + const unassigned = []; + + for (const language of selectedLanguages) { + const candidates = [ + ...new Set([ + ...getApproversForLanguage(approversConfig, language.id), + ...managementReviewers, + ]), + ]; + const reviewer = pickReviewer(candidates, issueNumber); + if (reviewer) { + byLanguage.push({ language: language.label, reviewer }); + assigneeSet.add(reviewer); + } else { + unassigned.push(language.label); + } + } + + return { byLanguage, assignees: [...assigneeSet], unassigned }; +} + +export { pickReviewer, resolveAssignments }; + +/** + * Resolve reviewers for the issue and assign any that are not already assigned. + * Does not post a comment - the resolved handles are surfaced by the caller's + * validation comment. Returns the per-language mapping for that rendering. + * + * @returns {Promise<{ byLanguage: { language: string, reviewer: string }[], assigned: string[], unassigned: string[], skipped: boolean }>} + */ +export default async function assignReviewers({ + github, + context, + core, + approversConfig, + approversPath = DEFAULT_APPROVERS_PATH, + readFileImpl = readFile, +}) { + const issue = context.payload.issue; + const issueBody = issue.body ?? ""; + const issueNumber = issue.number; + const currentAssignees = (issue.assignees ?? []).map((assignee) => assignee.login); + const { owner, repo } = context.repo; + + const resolvedConfig = + approversConfig ?? (await loadApproversConfig(approversPath, readFileImpl)); + + const { byLanguage, assignees, unassigned } = resolveAssignments({ + issueBody, + issueNumber, + approversConfig: resolvedConfig, + }); + + if (unassigned.length > 0) { + core?.warning?.(`No architect configured for: ${unassigned.join(", ")}`); + } + + const currentLower = new Set(currentAssignees.map((login) => login.toLowerCase())); + const newAssignees = assignees.filter((login) => !currentLower.has(login.toLowerCase())); + + if (newAssignees.length === 0) { + core?.info?.("Reviewers already assigned; skipping assignment."); + return { byLanguage, assigned: [], unassigned, skipped: true }; + } + + await github.rest.issues.addAssignees({ + owner, + repo, + issue_number: issueNumber, + assignees: newAssignees, + }); + + core?.info?.(`Assigned reviewers: ${newAssignees.join(", ")}`); + return { byLanguage, assigned: newAssignees, unassigned, skipped: false }; +} diff --git a/.github/workflows/src/arch-board-review/triage.js b/.github/workflows/src/arch-board-review/triage.js index 5f7e995c8e1b..24398a730b63 100644 --- a/.github/workflows/src/arch-board-review/triage.js +++ b/.github/workflows/src/arch-board-review/triage.js @@ -18,6 +18,7 @@ import { LANGUAGE_DEFINITIONS, } from "./issue-parsing.js"; import { validateUrl as defaultValidateUrl } from "./url-validation.js"; +import defaultAssignReviewers from "./assign-reviewers.js"; import { commentOrUpdate } from "../comment.js"; import { addLabels, ensureLabel, ensureLabelRemoved, removeLabel } from "../labels.js"; @@ -156,12 +157,23 @@ async function analyzeTriage(issueBody, currentLabels, { validateUrl = defaultVa }; } -function buildSuccessComment({ selectedLanguages, validated, warnings }) { +function buildSuccessComment({ selectedLanguages, validated, warnings, assignments, unassigned }) { let body = `✅ **All materials verified for ${selectedLanguages.map((language) => language.label).join(", ")}.**\n\nThis review request is ready for architects. The \`ready-for-review\` label has been applied.\n\n`; if (warnings.length > 0) { body += `**Note:** ${warnings.join(", ")}\n\n`; } + if (assignments && assignments.length > 0) { + const assignedLine = assignments + .map((entry) => `${entry.language} → @${entry.reviewer}`) + .join(" · "); + body += `**Assigned for review:** ${assignedLine}\n\nAssigned architects are notified via GitHub. Apply your \`-api-approved\` label when the review is complete.\n\n`; + } + + if (unassigned && unassigned.length > 0) { + body += `⚠️ **No architect is configured for: ${unassigned.join(", ")}.** These languages need manual assignment - please assign a reviewer.\n\n`; + } + body += "
Validation details\n\n"; for (const entry of validated) { body += `**${entry.language}:**\n`; @@ -202,7 +214,13 @@ function buildFailureComment({ missing, validated }) { export { COMMENT_IDENTIFIER, analyzeTriage, buildFailureComment, buildSuccessComment }; -export default async function triage({ github, context, core, validateUrl = defaultValidateUrl }) { +export default async function triage({ + github, + context, + core, + validateUrl = defaultValidateUrl, + assignReviewers = defaultAssignReviewers, +}) { const issue = context.payload.issue; const issueBody = issue.body ?? ""; const issueNumber = issue.number; @@ -234,10 +252,18 @@ export default async function triage({ github, context, core, validateUrl = defa await ensureLabel(github, owner, repo, issueNumber, "ready-for-review", currentLabels); await ensureLabelRemoved(github, owner, repo, issueNumber, "needs-info", currentLabels); - await postComment(buildSuccessComment(result)); + const assignment = await assignReviewers({ github, context, core }); + + await postComment( + buildSuccessComment({ + ...result, + assignments: assignment?.byLanguage ?? [], + unassigned: assignment?.unassigned ?? [], + }), + ); core?.info?.("Review request is ready for review."); - return { ...result, status: "ready-for-review" }; + return { ...result, status: "ready-for-review", assignment }; } await ensureLabel(github, owner, repo, issueNumber, "needs-info", currentLabels); diff --git a/.github/workflows/test/arch-board-review/assign-reviewers.test.js b/.github/workflows/test/arch-board-review/assign-reviewers.test.js new file mode 100644 index 000000000000..85e553247ea5 --- /dev/null +++ b/.github/workflows/test/arch-board-review/assign-reviewers.test.js @@ -0,0 +1,152 @@ +import { describe, expect, it, vi } from "vitest"; + +import assignReviewers, { + pickReviewer, + resolveAssignments, +} from "../../src/arch-board-review/assign-reviewers.js"; + +const approversConfig = { + "data-plane": { + java: ["java-user-1", "java-user-2"], + python: ["python-user-1", "python-user-2"], + }, + "management-plane": { + all: ["mgmt-user-1", "mgmt-user-2"], + }, +}; + +function createGithubMock() { + return { + rest: { + issues: { + addAssignees: vi.fn().mockResolvedValue({}), + }, + }, + }; +} + +function createContext({ issueBody, number = 456, assignees = [] }) { + return { + repo: { owner: "Azure", repo: "azure-sdk" }, + payload: { + issue: { + number, + body: issueBody, + assignees: assignees.map((login) => ({ login })), + }, + }, + }; +} + +describe("pickReviewer", () => { + it("returns null for an empty candidate list", () => { + expect(pickReviewer([], 5)).toBeNull(); + expect(pickReviewer(undefined, 5)).toBeNull(); + }); + + it("distributes deterministically by issue number", () => { + const candidates = ["a", "b"]; + expect(pickReviewer(candidates, 4)).toBe("a"); + expect(pickReviewer(candidates, 5)).toBe("b"); + }); +}); + +describe("resolveAssignments", () => { + it("resolves one reviewer per selected language", () => { + const issueBody = "- [x] Java\n- [x] Python"; + const { byLanguage, assignees } = resolveAssignments({ + issueBody, + issueNumber: 456, + approversConfig, + }); + expect(byLanguage).toEqual([ + { language: "Java", reviewer: "java-user-1" }, + { language: "Python", reviewer: "python-user-1" }, + ]); + expect(assignees).toEqual(["java-user-1", "python-user-1"]); + }); + + it("includes management-plane approvers in the pool for management-plane issues", () => { + // Java pool is [java-user-1, java-user-2, mgmt-user-1, mgmt-user-2]; index 2 lands on a + // management approver, proving plane-aware selection (would be java-user-1 without it). + const issueBody = "This is a management plane library.\n- [x] Java"; + const { byLanguage } = resolveAssignments({ + issueBody, + issueNumber: 2, + approversConfig, + }); + expect(byLanguage).toEqual([{ language: "Java", reviewer: "mgmt-user-1" }]); + }); + + it("reports selected languages with no configured architect as unassigned", () => { + const { byLanguage, assignees, unassigned } = resolveAssignments({ + issueBody: "- [x] C++", + issueNumber: 1, + approversConfig, + }); + expect(byLanguage).toEqual([]); + expect(assignees).toEqual([]); + expect(unassigned).toEqual(["C++"]); + }); + + it("returns nothing when no languages are selected", () => { + const { byLanguage, assignees, unassigned } = resolveAssignments({ + issueBody: "no languages here", + issueNumber: 1, + approversConfig, + }); + expect(byLanguage).toEqual([]); + expect(assignees).toEqual([]); + expect(unassigned).toEqual([]); + }); +}); + +describe("assignReviewers", () => { + const core = { info: vi.fn(), warning: vi.fn() }; + + it("assigns resolved reviewers and returns the per-language mapping", async () => { + const github = createGithubMock(); + const context = createContext({ issueBody: "- [x] Java" }); + + const result = await assignReviewers({ github, context, core, approversConfig }); + + expect(result.assigned).toEqual(["java-user-1"]); + expect(result.byLanguage).toEqual([{ language: "Java", reviewer: "java-user-1" }]); + expect(github.rest.issues.addAssignees).toHaveBeenCalledWith( + expect.objectContaining({ assignees: ["java-user-1"] }), + ); + }); + + it("does not post a comment (comment is owned by triage)", async () => { + const github = createGithubMock(); + const context = createContext({ issueBody: "- [x] Java" }); + + await assignReviewers({ github, context, core, approversConfig }); + + expect(github.rest.issues.createComment).toBeUndefined(); + }); + + it("is idempotent when the reviewer is already assigned (case-insensitive)", async () => { + const github = createGithubMock(); + const context = createContext({ + issueBody: "- [x] Java", + assignees: ["JAVA-USER-1"], + }); + + const result = await assignReviewers({ github, context, core, approversConfig }); + + expect(result.skipped).toBe(true); + expect(github.rest.issues.addAssignees).not.toHaveBeenCalled(); + }); + + it("surfaces unassigned languages without assigning", async () => { + const github = createGithubMock(); + const context = createContext({ issueBody: "- [x] C++" }); + + const result = await assignReviewers({ github, context, core, approversConfig }); + + expect(result.unassigned).toEqual(["C++"]); + expect(result.skipped).toBe(true); + expect(github.rest.issues.addAssignees).not.toHaveBeenCalled(); + }); +}); diff --git a/.github/workflows/test/arch-board-review/triage.test.js b/.github/workflows/test/arch-board-review/triage.test.js index 48609693255a..bb93b6334855 100644 --- a/.github/workflows/test/arch-board-review/triage.test.js +++ b/.github/workflows/test/arch-board-review/triage.test.js @@ -36,6 +36,7 @@ function createGithubMock() { paginate: vi.fn().mockResolvedValue([]), rest: { issues: { + addAssignees: vi.fn().mockResolvedValue({}), addLabels: vi.fn().mockResolvedValue({}), createComment: vi.fn().mockResolvedValue({ data: { id: 1 } }), listComments: {}, @@ -118,6 +119,14 @@ describe("triage", () => { body: expect.stringContaining("All materials verified"), }), ); + expect(github.rest.issues.addAssignees).toHaveBeenCalledWith( + expect.objectContaining({ assignees: ["JonathanGiles"] }), + ); + expect(github.rest.issues.createComment).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.stringContaining("Assigned for review:"), + }), + ); }); it("syncs language labels when the selection changes", async () => { From e64583f60780a4ba9b59cc392be7de3bc77bf1f4 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Wed, 22 Jul 2026 10:52:33 -0700 Subject: [PATCH 2/4] Reconcile reviewer assignments on edits; show handles as code Address remaining PR review comments: - r3626637027: render assigned handles as inline code instead of @login in the success comment so it does not fire a redundant mention ping (the addAssignees call is the notification). - r3626637091: reconcile stale assignments. When an edit changes the selected languages, architects the automation previously assigned but no longer resolved are removed via removeAssignees. Only roster members are removed, so manually added (non-roster) assignees are always preserved. Add getAllApprovers helper to approval-check.js and cover reconciliation and manual-assignee preservation in tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f922dad1-6173-4c65-b4bb-03f056572963 --- .../ARCH-BOARD-REVIEW-PROCESS.md | 5 +- .../src/arch-board-review/approval-check.js | 20 ++++++ .../src/arch-board-review/assign-reviewers.js | 63 +++++++++++++++---- .../workflows/src/arch-board-review/triage.js | 2 +- .../assign-reviewers.test.js | 36 +++++++++++ 5 files changed, 111 insertions(+), 15 deletions(-) diff --git a/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md b/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md index b64158189e9b..6f47c3c5f546 100644 --- a/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md +++ b/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md @@ -93,7 +93,10 @@ For **management plane** issues, the management-plane approvers are included in the candidate pool alongside the language-specific architects. Assignment is idempotent: an architect already assigned is not re-assigned, so -subsequent edits to a ready issue do not re-notify. +subsequent edits to a ready issue do not re-notify. If an edit changes the +selected languages, architects the automation previously assigned but that no +longer apply are removed; assignees added manually (anyone outside the roster) +are always preserved. ## Approver Configuration diff --git a/.github/workflows/src/arch-board-review/approval-check.js b/.github/workflows/src/arch-board-review/approval-check.js index 85a67969671d..f0bbe50ee1bb 100644 --- a/.github/workflows/src/arch-board-review/approval-check.js +++ b/.github/workflows/src/arch-board-review/approval-check.js @@ -29,6 +29,25 @@ function getManagementApprovers(approversConfig) { return approversConfig["management-plane"]?.all ?? []; } +/** + * Every login that appears anywhere in the roster (all data-plane languages plus + * management-plane). Used to distinguish automation-managed assignees from manual + * ones during reviewer reconciliation. + */ +function getAllApprovers(approversConfig) { + const all = new Set(); + const dataPlane = approversConfig["data-plane"] ?? {}; + for (const approvers of Object.values(dataPlane)) { + for (const login of approvers ?? []) { + all.add(login); + } + } + for (const login of getManagementApprovers(approversConfig)) { + all.add(login); + } + return [...all]; +} + function isManagementPlaneIssue(issueBody) { return /management\s*plane/i.test(issueBody); } @@ -64,6 +83,7 @@ export { APPROVAL_LABEL_PATTERN, buildApprovalCompleteComment, buildUnauthorizedComment, + getAllApprovers, getAllowedApprovers, getApproversForLanguage, getManagementApprovers, diff --git a/.github/workflows/src/arch-board-review/assign-reviewers.js b/.github/workflows/src/arch-board-review/assign-reviewers.js index 582f2a139e8f..5bcbb1cd3cbb 100644 --- a/.github/workflows/src/arch-board-review/assign-reviewers.js +++ b/.github/workflows/src/arch-board-review/assign-reviewers.js @@ -15,11 +15,17 @@ * surfaced in triage's "materials verified" comment for visibility. Idempotent: * only newly resolved assignees are added, so edits to an already-assigned issue * do not re-assign or re-notify. + * + * Edits are reconciled: if a language selection changes (e.g. Java swapped for + * Python), architects the automation previously assigned but that no longer apply + * are removed. Only roster members are removed, so assignees added manually (or by + * anyone outside the roster) are always preserved. */ import { readFile } from "node:fs/promises"; import { getSelectedLanguages } from "./issue-parsing.js"; import { + getAllApprovers, getApproversForLanguage, getManagementApprovers, isManagementPlaneIssue, @@ -84,11 +90,12 @@ function resolveAssignments({ issueBody, issueNumber, approversConfig }) { export { pickReviewer, resolveAssignments }; /** - * Resolve reviewers for the issue and assign any that are not already assigned. + * Resolve reviewers for the issue, assign any that are not already assigned, and + * remove stale automation-assigned architects that no longer apply after an edit. * Does not post a comment - the resolved handles are surfaced by the caller's * validation comment. Returns the per-language mapping for that rendering. * - * @returns {Promise<{ byLanguage: { language: string, reviewer: string }[], assigned: string[], unassigned: string[], skipped: boolean }>} + * @returns {Promise<{ byLanguage: { language: string, reviewer: string }[], assigned: string[], removed: string[], unassigned: string[], skipped: boolean }>} */ export default async function assignReviewers({ github, @@ -117,21 +124,51 @@ export default async function assignReviewers({ core?.warning?.(`No architect configured for: ${unassigned.join(", ")}`); } + const resolvedLower = new Set(assignees.map((login) => login.toLowerCase())); const currentLower = new Set(currentAssignees.map((login) => login.toLowerCase())); + const rosterManaged = new Set( + getAllApprovers(resolvedConfig).map((login) => login.toLowerCase()), + ); + const newAssignees = assignees.filter((login) => !currentLower.has(login.toLowerCase())); - if (newAssignees.length === 0) { - core?.info?.("Reviewers already assigned; skipping assignment."); - return { byLanguage, assigned: [], unassigned, skipped: true }; + // Reconcile edits: remove architects the automation manages (roster members) + // that are no longer resolved for the current languages. Manual assignees that + // are not in the roster are left untouched. + const staleAssignees = currentAssignees.filter( + (login) => rosterManaged.has(login.toLowerCase()) && !resolvedLower.has(login.toLowerCase()), + ); + + if (newAssignees.length > 0) { + await github.rest.issues.addAssignees({ + owner, + repo, + issue_number: issueNumber, + assignees: newAssignees, + }); + core?.info?.(`Assigned reviewers: ${newAssignees.join(", ")}`); } - await github.rest.issues.addAssignees({ - owner, - repo, - issue_number: issueNumber, - assignees: newAssignees, - }); + if (staleAssignees.length > 0) { + await github.rest.issues.removeAssignees({ + owner, + repo, + issue_number: issueNumber, + assignees: staleAssignees, + }); + core?.info?.(`Removed stale reviewers: ${staleAssignees.join(", ")}`); + } + + if (newAssignees.length === 0 && staleAssignees.length === 0) { + core?.info?.("Reviewers already up to date; no assignment changes."); + return { byLanguage, assigned: [], removed: [], unassigned, skipped: true }; + } - core?.info?.(`Assigned reviewers: ${newAssignees.join(", ")}`); - return { byLanguage, assigned: newAssignees, unassigned, skipped: false }; + return { + byLanguage, + assigned: newAssignees, + removed: staleAssignees, + unassigned, + skipped: false, + }; } diff --git a/.github/workflows/src/arch-board-review/triage.js b/.github/workflows/src/arch-board-review/triage.js index 24398a730b63..f059df18d4e0 100644 --- a/.github/workflows/src/arch-board-review/triage.js +++ b/.github/workflows/src/arch-board-review/triage.js @@ -165,7 +165,7 @@ function buildSuccessComment({ selectedLanguages, validated, warnings, assignmen if (assignments && assignments.length > 0) { const assignedLine = assignments - .map((entry) => `${entry.language} → @${entry.reviewer}`) + .map((entry) => `${entry.language} → \`${entry.reviewer}\``) .join(" · "); body += `**Assigned for review:** ${assignedLine}\n\nAssigned architects are notified via GitHub. Apply your \`-api-approved\` label when the review is complete.\n\n`; } diff --git a/.github/workflows/test/arch-board-review/assign-reviewers.test.js b/.github/workflows/test/arch-board-review/assign-reviewers.test.js index 85e553247ea5..f977be998499 100644 --- a/.github/workflows/test/arch-board-review/assign-reviewers.test.js +++ b/.github/workflows/test/arch-board-review/assign-reviewers.test.js @@ -20,6 +20,7 @@ function createGithubMock() { rest: { issues: { addAssignees: vi.fn().mockResolvedValue({}), + removeAssignees: vi.fn().mockResolvedValue({}), }, }, }; @@ -149,4 +150,39 @@ describe("assignReviewers", () => { expect(result.skipped).toBe(true); expect(github.rest.issues.addAssignees).not.toHaveBeenCalled(); }); + + it("removes a stale automation-assigned architect when the language selection changes", async () => { + // Previously assigned java-user-1 (roster member); issue was edited to Python only. + const github = createGithubMock(); + const context = createContext({ + issueBody: "- [x] Python", + assignees: ["java-user-1"], + }); + + const result = await assignReviewers({ github, context, core, approversConfig }); + + expect(result.assigned).toEqual(["python-user-1"]); + expect(result.removed).toEqual(["java-user-1"]); + expect(github.rest.issues.addAssignees).toHaveBeenCalledWith( + expect.objectContaining({ assignees: ["python-user-1"] }), + ); + expect(github.rest.issues.removeAssignees).toHaveBeenCalledWith( + expect.objectContaining({ assignees: ["java-user-1"] }), + ); + }); + + it("preserves manual (non-roster) assignees during reconciliation", async () => { + const github = createGithubMock(); + const context = createContext({ + issueBody: "- [x] Python", + assignees: ["some-manual-user", "java-user-1"], + }); + + const result = await assignReviewers({ github, context, core, approversConfig }); + + expect(result.removed).toEqual(["java-user-1"]); + expect(github.rest.issues.removeAssignees).toHaveBeenCalledWith( + expect.objectContaining({ assignees: ["java-user-1"] }), + ); + }); }); From 1c2cbff9b816aeb8393fc864b1c03b2184f3cf93 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Wed, 22 Jul 2026 10:55:10 -0700 Subject: [PATCH 3/4] Harden reviewer assignment: resilient triage, clearer skip reasons - Wrap assignReviewers in try/catch in triage so a failed assignment (invalid handle, permissions, API hiccup) still posts the success comment instead of leaving a ready-for-review issue with no feedback. - Add a reason field to the assignReviewers skipped result to distinguish 'already-assigned' from 'no-reviewers-resolved'. - Strengthen the no-comment test to assert createComment is not called (mock now includes createComment) rather than checking mock shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f922dad1-6173-4c65-b4bb-03f056572963 --- .../src/arch-board-review/assign-reviewers.js | 15 +++++++++--- .../workflows/src/arch-board-review/triage.js | 7 +++++- .../assign-reviewers.test.js | 5 +++- .../test/arch-board-review/triage.test.js | 23 +++++++++++++++++++ 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/.github/workflows/src/arch-board-review/assign-reviewers.js b/.github/workflows/src/arch-board-review/assign-reviewers.js index 5bcbb1cd3cbb..3122d0ca37b4 100644 --- a/.github/workflows/src/arch-board-review/assign-reviewers.js +++ b/.github/workflows/src/arch-board-review/assign-reviewers.js @@ -95,7 +95,10 @@ export { pickReviewer, resolveAssignments }; * Does not post a comment - the resolved handles are surfaced by the caller's * validation comment. Returns the per-language mapping for that rendering. * - * @returns {Promise<{ byLanguage: { language: string, reviewer: string }[], assigned: string[], removed: string[], unassigned: string[], skipped: boolean }>} + * @returns {Promise<{ byLanguage: { language: string, reviewer: string }[], assigned: string[], removed: string[], unassigned: string[], skipped: boolean, reason: string | null }>} + * When `skipped` is true, `reason` is `"already-assigned"` (reviewers exist and + * nothing changed) or `"no-reviewers-resolved"` (no architect could be resolved + * for the selected languages). When `skipped` is false, `reason` is null. */ export default async function assignReviewers({ github, @@ -160,8 +163,13 @@ export default async function assignReviewers({ } if (newAssignees.length === 0 && staleAssignees.length === 0) { - core?.info?.("Reviewers already up to date; no assignment changes."); - return { byLanguage, assigned: [], removed: [], unassigned, skipped: true }; + const reason = assignees.length === 0 ? "no-reviewers-resolved" : "already-assigned"; + core?.info?.( + reason === "already-assigned" + ? "Reviewers already up to date; no assignment changes." + : "No architect could be resolved for the selected languages.", + ); + return { byLanguage, assigned: [], removed: [], unassigned, skipped: true, reason }; } return { @@ -170,5 +178,6 @@ export default async function assignReviewers({ removed: staleAssignees, unassigned, skipped: false, + reason: null, }; } diff --git a/.github/workflows/src/arch-board-review/triage.js b/.github/workflows/src/arch-board-review/triage.js index f059df18d4e0..bd059e252282 100644 --- a/.github/workflows/src/arch-board-review/triage.js +++ b/.github/workflows/src/arch-board-review/triage.js @@ -252,7 +252,12 @@ export default async function triage({ await ensureLabel(github, owner, repo, issueNumber, "ready-for-review", currentLabels); await ensureLabelRemoved(github, owner, repo, issueNumber, "needs-info", currentLabels); - const assignment = await assignReviewers({ github, context, core }); + let assignment = null; + try { + assignment = await assignReviewers({ github, context, core }); + } catch (error) { + core?.warning?.(`Reviewer assignment failed: ${error.message}`); + } await postComment( buildSuccessComment({ diff --git a/.github/workflows/test/arch-board-review/assign-reviewers.test.js b/.github/workflows/test/arch-board-review/assign-reviewers.test.js index f977be998499..9c5684df7f3a 100644 --- a/.github/workflows/test/arch-board-review/assign-reviewers.test.js +++ b/.github/workflows/test/arch-board-review/assign-reviewers.test.js @@ -21,6 +21,7 @@ function createGithubMock() { issues: { addAssignees: vi.fn().mockResolvedValue({}), removeAssignees: vi.fn().mockResolvedValue({}), + createComment: vi.fn().mockResolvedValue({}), }, }, }; @@ -124,7 +125,7 @@ describe("assignReviewers", () => { await assignReviewers({ github, context, core, approversConfig }); - expect(github.rest.issues.createComment).toBeUndefined(); + expect(github.rest.issues.createComment).not.toHaveBeenCalled(); }); it("is idempotent when the reviewer is already assigned (case-insensitive)", async () => { @@ -137,6 +138,7 @@ describe("assignReviewers", () => { const result = await assignReviewers({ github, context, core, approversConfig }); expect(result.skipped).toBe(true); + expect(result.reason).toBe("already-assigned"); expect(github.rest.issues.addAssignees).not.toHaveBeenCalled(); }); @@ -148,6 +150,7 @@ describe("assignReviewers", () => { expect(result.unassigned).toEqual(["C++"]); expect(result.skipped).toBe(true); + expect(result.reason).toBe("no-reviewers-resolved"); expect(github.rest.issues.addAssignees).not.toHaveBeenCalled(); }); diff --git a/.github/workflows/test/arch-board-review/triage.test.js b/.github/workflows/test/arch-board-review/triage.test.js index bb93b6334855..c12773fd442b 100644 --- a/.github/workflows/test/arch-board-review/triage.test.js +++ b/.github/workflows/test/arch-board-review/triage.test.js @@ -129,6 +129,29 @@ describe("triage", () => { ); }); + it("still posts the success comment when reviewer assignment fails", async () => { + const github = createGithubMock(); + const context = createContext({ + issueBody: createIssueBody(), + labels: ["board-review", "needs-info"], + }); + const validateUrl = vi.fn().mockResolvedValue({ valid: true, reason: null }); + const assignReviewers = vi.fn().mockRejectedValue(new Error("addAssignees failed")); + const core = { info: vi.fn(), warning: vi.fn() }; + + const result = await triage({ github, context, core, validateUrl, assignReviewers }); + + expect(result.status).toBe("ready-for-review"); + expect(core.warning).toHaveBeenCalledWith( + expect.stringContaining("Reviewer assignment failed"), + ); + expect(github.rest.issues.createComment).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.stringContaining("All materials verified"), + }), + ); + }); + it("syncs language labels when the selection changes", async () => { const analysis = await analyzeTriage(createIssueBody(), ["board-review", "Java", "Python"], { validateUrl: vi.fn().mockResolvedValue({ valid: true, reason: null }), From 80faf549fc0d5c598fad27e589bfdd4d475c4baf Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Thu, 23 Jul 2026 11:38:39 -0700 Subject: [PATCH 4/4] Assign all architects per language; surface assignment failures Per reviewer feedback: - Replace the round-robin picker with assigning every configured architect for each selected language. Simpler and no state; if it gets noisy for languages with multiple architects we can add distribution later. - When the assignment API call fails, the 'materials verified' comment now includes a visible note asking for manual assignment (with a link to the approvers file) instead of only logging a warning, so the issue is never left silently unassigned. - Add a link to api-review-approvers.yml in the unassigned-language note too. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f922dad1-6173-4c65-b4bb-03f056572963 --- .../ARCH-BOARD-REVIEW-PROCESS.md | 22 ++--- .../src/arch-board-review/approval-check.js | 2 +- .../src/arch-board-review/assign-reviewers.js | 69 +++++++--------- .../workflows/src/arch-board-review/triage.js | 25 +++++- .../assign-reviewers.test.js | 82 ++++++++++--------- .../test/arch-board-review/triage.test.js | 7 +- 6 files changed, 112 insertions(+), 95 deletions(-) diff --git a/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md b/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md index 6f47c3c5f546..ec910d222f14 100644 --- a/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md +++ b/.github/workflows/src/arch-board-review/ARCH-BOARD-REVIEW-PROCESS.md @@ -82,21 +82,23 @@ Unauthorized label additions are reverted with a comment. ## Reviewer Assignment -When an issue reaches `ready-for-review`, `assign-reviewers.js` assigns one -architect per selected language, chosen from the same -`api-review-approvers.yml` roster (round-robin, keyed off the issue number). -Assigning the issue triggers GitHub's native notifications, so architects are -reached through their own notification preferences. The assigned handles are -listed in the triage "materials verified" comment for visibility. +When an issue reaches `ready-for-review`, `assign-reviewers.js` assigns every +architect configured for each selected language, drawn from the same +`api-review-approvers.yml` approvers list. Assigning the issue triggers GitHub's +native notifications, so architects are reached through their own notification +preferences. The assigned handles are listed in the triage "materials verified" +comment for visibility. -For **management plane** issues, the management-plane approvers are included in -the candidate pool alongside the language-specific architects. +For **management plane** issues, the management-plane approvers are assigned +alongside the language-specific architects. Assignment is idempotent: an architect already assigned is not re-assigned, so subsequent edits to a ready issue do not re-notify. If an edit changes the selected languages, architects the automation previously assigned but that no -longer apply are removed; assignees added manually (anyone outside the roster) -are always preserved. +longer apply are removed; assignees added manually (anyone outside the approvers +list) are always preserved. If the assignment API call fails, the "materials verified" +comment still posts with a note asking for manual assignment, so the issue is +never left silently unassigned. ## Approver Configuration diff --git a/.github/workflows/src/arch-board-review/approval-check.js b/.github/workflows/src/arch-board-review/approval-check.js index f0bbe50ee1bb..d9d42553c3e3 100644 --- a/.github/workflows/src/arch-board-review/approval-check.js +++ b/.github/workflows/src/arch-board-review/approval-check.js @@ -30,7 +30,7 @@ function getManagementApprovers(approversConfig) { } /** - * Every login that appears anywhere in the roster (all data-plane languages plus + * Every login that appears anywhere in the approvers list (all data-plane languages plus * management-plane). Used to distinguish automation-managed assignees from manual * ones during reviewer reconciliation. */ diff --git a/.github/workflows/src/arch-board-review/assign-reviewers.js b/.github/workflows/src/arch-board-review/assign-reviewers.js index 3122d0ca37b4..63fd1e0e1da0 100644 --- a/.github/workflows/src/arch-board-review/assign-reviewers.js +++ b/.github/workflows/src/arch-board-review/assign-reviewers.js @@ -2,14 +2,14 @@ * Assign Reviewers — routes a ready review request to per-language architects. * * Called by triage once an issue reaches `ready-for-review`. For each selected - * language it picks one architect (round-robin, keyed off the issue number) from - * the shared `api-review-approvers.yml` roster and assigns them to the issue. - * Assigning triggers GitHub's native notifications, so architects are reached - * through their own notification preferences. + * language it assigns every configured architect from the shared + * `api-review-approvers.yml` approvers list to the issue. Assigning triggers + * GitHub's native notifications, so architects are reached through their own + * notification preferences. * - * The roster is the same source of truth used by approval validation, so there is - * no separate reviewer list to keep in sync. Management-plane issues additionally - * consider the management-plane approvers. + * The approvers list is the same source of truth used by approval validation, so + * there is no separate reviewer list to keep in sync. Management-plane issues + * additionally consider the management-plane approvers. * * The assignment itself is the notification mechanism; the resolved handles are * surfaced in triage's "materials verified" comment for visibility. Idempotent: @@ -18,8 +18,8 @@ * * Edits are reconciled: if a language selection changes (e.g. Java swapped for * Python), architects the automation previously assigned but that no longer apply - * are removed. Only roster members are removed, so assignees added manually (or by - * anyone outside the roster) are always preserved. + * are removed. Only configured approvers are removed, so assignees added manually + * (or by anyone outside the approvers list) are always preserved. */ import { readFile } from "node:fs/promises"; @@ -35,30 +35,20 @@ import { const DEFAULT_APPROVERS_PATH = new URL("../../../api-review-approvers.yml", import.meta.url); /** - * Deterministically pick one candidate, distributing across issues by number. + * Resolve the architects for each selected language from the approvers list. * - * @param {string[]} candidates - * @param {number} issueNumber - * @returns {string | null} - */ -function pickReviewer(candidates, issueNumber) { - if (!candidates || candidates.length === 0) { - return null; - } - return candidates[issueNumber % candidates.length]; -} - -/** - * Resolve one architect per selected language from the roster. + * Every configured architect for a language is assigned (management-plane issues + * additionally include the management-plane approvers). There is intentionally no + * load balancing - if a language has more than one architect they are all tagged. + * If that ever gets noisy we can add distribution later. * * @param {object} params * @param {string} params.issueBody - * @param {number} params.issueNumber * @param {object} params.approversConfig - * @returns {{ byLanguage: { language: string, reviewer: string }[], assignees: string[], unassigned: string[] }} + * @returns {{ byLanguage: { language: string, reviewers: string[] }[], assignees: string[], unassigned: string[] }} * `unassigned` lists selected languages that have no configured architect. */ -function resolveAssignments({ issueBody, issueNumber, approversConfig }) { +function resolveAssignments({ issueBody, approversConfig }) { const selectedLanguages = getSelectedLanguages(issueBody); const managementReviewers = isManagementPlaneIssue(issueBody) ? getManagementApprovers(approversConfig) @@ -69,16 +59,17 @@ function resolveAssignments({ issueBody, issueNumber, approversConfig }) { const unassigned = []; for (const language of selectedLanguages) { - const candidates = [ + const reviewers = [ ...new Set([ ...getApproversForLanguage(approversConfig, language.id), ...managementReviewers, ]), ]; - const reviewer = pickReviewer(candidates, issueNumber); - if (reviewer) { - byLanguage.push({ language: language.label, reviewer }); - assigneeSet.add(reviewer); + if (reviewers.length > 0) { + byLanguage.push({ language: language.label, reviewers }); + for (const reviewer of reviewers) { + assigneeSet.add(reviewer); + } } else { unassigned.push(language.label); } @@ -87,7 +78,7 @@ function resolveAssignments({ issueBody, issueNumber, approversConfig }) { return { byLanguage, assignees: [...assigneeSet], unassigned }; } -export { pickReviewer, resolveAssignments }; +export { resolveAssignments }; /** * Resolve reviewers for the issue, assign any that are not already assigned, and @@ -95,7 +86,7 @@ export { pickReviewer, resolveAssignments }; * Does not post a comment - the resolved handles are surfaced by the caller's * validation comment. Returns the per-language mapping for that rendering. * - * @returns {Promise<{ byLanguage: { language: string, reviewer: string }[], assigned: string[], removed: string[], unassigned: string[], skipped: boolean, reason: string | null }>} + * @returns {Promise<{ byLanguage: { language: string, reviewers: string[] }[], assigned: string[], removed: string[], unassigned: string[], skipped: boolean, reason: string | null }>} * When `skipped` is true, `reason` is `"already-assigned"` (reviewers exist and * nothing changed) or `"no-reviewers-resolved"` (no architect could be resolved * for the selected languages). When `skipped` is false, `reason` is null. @@ -119,7 +110,6 @@ export default async function assignReviewers({ const { byLanguage, assignees, unassigned } = resolveAssignments({ issueBody, - issueNumber, approversConfig: resolvedConfig, }); @@ -129,17 +119,18 @@ export default async function assignReviewers({ const resolvedLower = new Set(assignees.map((login) => login.toLowerCase())); const currentLower = new Set(currentAssignees.map((login) => login.toLowerCase())); - const rosterManaged = new Set( + const configuredApprovers = new Set( getAllApprovers(resolvedConfig).map((login) => login.toLowerCase()), ); const newAssignees = assignees.filter((login) => !currentLower.has(login.toLowerCase())); - // Reconcile edits: remove architects the automation manages (roster members) - // that are no longer resolved for the current languages. Manual assignees that - // are not in the roster are left untouched. + // Reconcile edits: remove architects the automation manages (configured + // approvers) that are no longer resolved for the current languages. Manual + // assignees that are not in the approvers list are left untouched. const staleAssignees = currentAssignees.filter( - (login) => rosterManaged.has(login.toLowerCase()) && !resolvedLower.has(login.toLowerCase()), + (login) => + configuredApprovers.has(login.toLowerCase()) && !resolvedLower.has(login.toLowerCase()), ); if (newAssignees.length > 0) { diff --git a/.github/workflows/src/arch-board-review/triage.js b/.github/workflows/src/arch-board-review/triage.js index bd059e252282..cb08d24de8b9 100644 --- a/.github/workflows/src/arch-board-review/triage.js +++ b/.github/workflows/src/arch-board-review/triage.js @@ -23,6 +23,8 @@ import { commentOrUpdate } from "../comment.js"; import { addLabels, ensureLabel, ensureLabelRemoved, removeLabel } from "../labels.js"; const COMMENT_IDENTIFIER = "arch-board-triage-bot"; +const APPROVERS_FILE_URL = + "https://github.com/Azure/azure-sdk/blob/main/.github/api-review-approvers.yml"; function createValidationItem(kind, value) { return `${kind}: ${value}`; @@ -157,21 +159,33 @@ async function analyzeTriage(issueBody, currentLabels, { validateUrl = defaultVa }; } -function buildSuccessComment({ selectedLanguages, validated, warnings, assignments, unassigned }) { +function buildSuccessComment({ + selectedLanguages, + validated, + warnings, + assignments, + unassigned, + assignmentFailed, +}) { let body = `✅ **All materials verified for ${selectedLanguages.map((language) => language.label).join(", ")}.**\n\nThis review request is ready for architects. The \`ready-for-review\` label has been applied.\n\n`; if (warnings.length > 0) { body += `**Note:** ${warnings.join(", ")}\n\n`; } - if (assignments && assignments.length > 0) { + if (assignmentFailed) { + body += `⚠️ **Automatic reviewer assignment failed.** Please assign an architect manually - see the [architect approvers list](${APPROVERS_FILE_URL}).\n\n`; + } else if (assignments && assignments.length > 0) { const assignedLine = assignments - .map((entry) => `${entry.language} → \`${entry.reviewer}\``) + .map( + (entry) => + `${entry.language} → ${entry.reviewers.map((reviewer) => `\`${reviewer}\``).join(", ")}`, + ) .join(" · "); body += `**Assigned for review:** ${assignedLine}\n\nAssigned architects are notified via GitHub. Apply your \`-api-approved\` label when the review is complete.\n\n`; } if (unassigned && unassigned.length > 0) { - body += `⚠️ **No architect is configured for: ${unassigned.join(", ")}.** These languages need manual assignment - please assign a reviewer.\n\n`; + body += `⚠️ **No architect is configured for: ${unassigned.join(", ")}.** These languages need manual assignment - please assign a reviewer or add one to the [architect approvers list](${APPROVERS_FILE_URL}).\n\n`; } body += "
Validation details\n\n"; @@ -253,9 +267,11 @@ export default async function triage({ await ensureLabelRemoved(github, owner, repo, issueNumber, "needs-info", currentLabels); let assignment = null; + let assignmentFailed = false; try { assignment = await assignReviewers({ github, context, core }); } catch (error) { + assignmentFailed = true; core?.warning?.(`Reviewer assignment failed: ${error.message}`); } @@ -264,6 +280,7 @@ export default async function triage({ ...result, assignments: assignment?.byLanguage ?? [], unassigned: assignment?.unassigned ?? [], + assignmentFailed, }), ); diff --git a/.github/workflows/test/arch-board-review/assign-reviewers.test.js b/.github/workflows/test/arch-board-review/assign-reviewers.test.js index 9c5684df7f3a..0af600eae366 100644 --- a/.github/workflows/test/arch-board-review/assign-reviewers.test.js +++ b/.github/workflows/test/arch-board-review/assign-reviewers.test.js @@ -1,7 +1,6 @@ import { describe, expect, it, vi } from "vitest"; import assignReviewers, { - pickReviewer, resolveAssignments, } from "../../src/arch-board-review/assign-reviewers.js"; @@ -40,50 +39,37 @@ function createContext({ issueBody, number = 456, assignees = [] }) { }; } -describe("pickReviewer", () => { - it("returns null for an empty candidate list", () => { - expect(pickReviewer([], 5)).toBeNull(); - expect(pickReviewer(undefined, 5)).toBeNull(); - }); - - it("distributes deterministically by issue number", () => { - const candidates = ["a", "b"]; - expect(pickReviewer(candidates, 4)).toBe("a"); - expect(pickReviewer(candidates, 5)).toBe("b"); - }); -}); - describe("resolveAssignments", () => { - it("resolves one reviewer per selected language", () => { + it("assigns every architect for each selected language", () => { const issueBody = "- [x] Java\n- [x] Python"; const { byLanguage, assignees } = resolveAssignments({ issueBody, - issueNumber: 456, approversConfig, }); expect(byLanguage).toEqual([ - { language: "Java", reviewer: "java-user-1" }, - { language: "Python", reviewer: "python-user-1" }, + { language: "Java", reviewers: ["java-user-1", "java-user-2"] }, + { language: "Python", reviewers: ["python-user-1", "python-user-2"] }, ]); - expect(assignees).toEqual(["java-user-1", "python-user-1"]); + expect(assignees).toEqual(["java-user-1", "java-user-2", "python-user-1", "python-user-2"]); }); - it("includes management-plane approvers in the pool for management-plane issues", () => { - // Java pool is [java-user-1, java-user-2, mgmt-user-1, mgmt-user-2]; index 2 lands on a - // management approver, proving plane-aware selection (would be java-user-1 without it). + it("includes management-plane approvers for management-plane issues", () => { const issueBody = "This is a management plane library.\n- [x] Java"; const { byLanguage } = resolveAssignments({ issueBody, - issueNumber: 2, approversConfig, }); - expect(byLanguage).toEqual([{ language: "Java", reviewer: "mgmt-user-1" }]); + expect(byLanguage).toEqual([ + { + language: "Java", + reviewers: ["java-user-1", "java-user-2", "mgmt-user-1", "mgmt-user-2"], + }, + ]); }); it("reports selected languages with no configured architect as unassigned", () => { const { byLanguage, assignees, unassigned } = resolveAssignments({ issueBody: "- [x] C++", - issueNumber: 1, approversConfig, }); expect(byLanguage).toEqual([]); @@ -94,7 +80,6 @@ describe("resolveAssignments", () => { it("returns nothing when no languages are selected", () => { const { byLanguage, assignees, unassigned } = resolveAssignments({ issueBody: "no languages here", - issueNumber: 1, approversConfig, }); expect(byLanguage).toEqual([]); @@ -106,16 +91,18 @@ describe("resolveAssignments", () => { describe("assignReviewers", () => { const core = { info: vi.fn(), warning: vi.fn() }; - it("assigns resolved reviewers and returns the per-language mapping", async () => { + it("assigns all resolved reviewers and returns the per-language mapping", async () => { const github = createGithubMock(); const context = createContext({ issueBody: "- [x] Java" }); const result = await assignReviewers({ github, context, core, approversConfig }); - expect(result.assigned).toEqual(["java-user-1"]); - expect(result.byLanguage).toEqual([{ language: "Java", reviewer: "java-user-1" }]); + expect(result.assigned).toEqual(["java-user-1", "java-user-2"]); + expect(result.byLanguage).toEqual([ + { language: "Java", reviewers: ["java-user-1", "java-user-2"] }, + ]); expect(github.rest.issues.addAssignees).toHaveBeenCalledWith( - expect.objectContaining({ assignees: ["java-user-1"] }), + expect.objectContaining({ assignees: ["java-user-1", "java-user-2"] }), ); }); @@ -128,11 +115,11 @@ describe("assignReviewers", () => { expect(github.rest.issues.createComment).not.toHaveBeenCalled(); }); - it("is idempotent when the reviewer is already assigned (case-insensitive)", async () => { + it("is idempotent when all reviewers are already assigned (case-insensitive)", async () => { const github = createGithubMock(); const context = createContext({ issueBody: "- [x] Java", - assignees: ["JAVA-USER-1"], + assignees: ["JAVA-USER-1", "JAVA-USER-2"], }); const result = await assignReviewers({ github, context, core, approversConfig }); @@ -142,6 +129,21 @@ describe("assignReviewers", () => { expect(github.rest.issues.addAssignees).not.toHaveBeenCalled(); }); + it("only adds the reviewers that are not already assigned", async () => { + const github = createGithubMock(); + const context = createContext({ + issueBody: "- [x] Java", + assignees: ["java-user-1"], + }); + + const result = await assignReviewers({ github, context, core, approversConfig }); + + expect(result.assigned).toEqual(["java-user-2"]); + expect(github.rest.issues.addAssignees).toHaveBeenCalledWith( + expect.objectContaining({ assignees: ["java-user-2"] }), + ); + }); + it("surfaces unassigned languages without assigning", async () => { const github = createGithubMock(); const context = createContext({ issueBody: "- [x] C++" }); @@ -154,27 +156,27 @@ describe("assignReviewers", () => { expect(github.rest.issues.addAssignees).not.toHaveBeenCalled(); }); - it("removes a stale automation-assigned architect when the language selection changes", async () => { - // Previously assigned java-user-1 (roster member); issue was edited to Python only. + it("removes stale automation-assigned architects when the language selection changes", async () => { + // Previously assigned the Java architects; issue was edited to Python only. const github = createGithubMock(); const context = createContext({ issueBody: "- [x] Python", - assignees: ["java-user-1"], + assignees: ["java-user-1", "java-user-2"], }); const result = await assignReviewers({ github, context, core, approversConfig }); - expect(result.assigned).toEqual(["python-user-1"]); - expect(result.removed).toEqual(["java-user-1"]); + expect(result.assigned).toEqual(["python-user-1", "python-user-2"]); + expect(result.removed).toEqual(["java-user-1", "java-user-2"]); expect(github.rest.issues.addAssignees).toHaveBeenCalledWith( - expect.objectContaining({ assignees: ["python-user-1"] }), + expect.objectContaining({ assignees: ["python-user-1", "python-user-2"] }), ); expect(github.rest.issues.removeAssignees).toHaveBeenCalledWith( - expect.objectContaining({ assignees: ["java-user-1"] }), + expect.objectContaining({ assignees: ["java-user-1", "java-user-2"] }), ); }); - it("preserves manual (non-roster) assignees during reconciliation", async () => { + it("preserves manual (unconfigured) assignees during reconciliation", async () => { const github = createGithubMock(); const context = createContext({ issueBody: "- [x] Python", diff --git a/.github/workflows/test/arch-board-review/triage.test.js b/.github/workflows/test/arch-board-review/triage.test.js index c12773fd442b..efcc9fc2296a 100644 --- a/.github/workflows/test/arch-board-review/triage.test.js +++ b/.github/workflows/test/arch-board-review/triage.test.js @@ -120,7 +120,7 @@ describe("triage", () => { }), ); expect(github.rest.issues.addAssignees).toHaveBeenCalledWith( - expect.objectContaining({ assignees: ["JonathanGiles"] }), + expect.objectContaining({ assignees: ["JonathanGiles", "alzimmermsft"] }), ); expect(github.rest.issues.createComment).toHaveBeenCalledWith( expect.objectContaining({ @@ -150,6 +150,11 @@ describe("triage", () => { body: expect.stringContaining("All materials verified"), }), ); + expect(github.rest.issues.createComment).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.stringContaining("Automatic reviewer assignment failed"), + }), + ); }); it("syncs language labels when the selection changes", async () => {