diff --git a/actions/setup/js/add_reviewer.cjs b/actions/setup/js/add_reviewer.cjs index 3d83903f80c..7d27ab3d37e 100644 --- a/actions/setup/js/add_reviewer.cjs +++ b/actions/setup/js/add_reviewer.cjs @@ -9,7 +9,7 @@ */ /** - * @typedef {{ reviewers?: Array, pull_request_number?: number|string }} AddReviewerMessage + * @typedef {{ reviewers?: Array, team_reviewers?: Array, pull_request_number?: number|string }} AddReviewerMessage */ /** @type {string} Safe output type handled by this module */ @@ -30,6 +30,7 @@ const { COPILOT_REVIEWER_BOT } = require("./constants.cjs"); */ async function main(config = {}) { const allowedReviewers = config.allowed ?? []; + const allowedTeamReviewers = config.allowed_team_reviewers ?? []; const maxCount = config.max ?? 10; const githubClient = await createAuthenticatedGitHubClient(config); const isStaged = isStagedMode(config); @@ -38,6 +39,9 @@ async function main(config = {}) { if (allowedReviewers.length > 0) { core.info(`Allowed reviewers: ${allowedReviewers.join(", ")}`); } + if (allowedTeamReviewers.length > 0) { + core.info(`Allowed team reviewers: ${allowedTeamReviewers.join(", ")}`); + } let processedCount = 0; @@ -68,22 +72,27 @@ async function main(config = {}) { } const requestedReviewers = message.reviewers ?? []; + const requestedTeamReviewers = message.team_reviewers ?? []; core.info(`Requested reviewers: ${JSON.stringify(requestedReviewers)}`); + core.info(`Requested team reviewers: ${JSON.stringify(requestedTeamReviewers)}`); - // Use shared helper to filter, sanitize, dedupe, and limit + // Use shared helper to filter, sanitize, dedupe, and limit across both reviewer types const uniqueReviewers = processItems(requestedReviewers, allowedReviewers, maxCount); + const remainingReviewerSlots = Math.max(0, maxCount - uniqueReviewers.length); + const uniqueTeamReviewers = processItems(requestedTeamReviewers, allowedTeamReviewers, remainingReviewerSlots); - if (uniqueReviewers.length === 0) { + if (uniqueReviewers.length === 0 && uniqueTeamReviewers.length === 0) { core.info("No reviewers to add"); return { success: true, prNumber, reviewersAdded: [], + teamReviewersAdded: [], message: "No valid reviewers found", }; } - core.info(`Adding ${uniqueReviewers.length} reviewers to PR #${prNumber}: ${JSON.stringify(uniqueReviewers)}`); + core.info(`Adding reviewers to PR #${prNumber}: reviewers=${JSON.stringify(uniqueReviewers)}, team_reviewers=${JSON.stringify(uniqueTeamReviewers)}`); // If in staged mode, preview without executing if (isStaged) { @@ -94,6 +103,7 @@ async function main(config = {}) { previewInfo: { number: prNumber, reviewers: uniqueReviewers, + team_reviewers: uniqueTeamReviewers, }, }; } @@ -104,14 +114,19 @@ async function main(config = {}) { const otherReviewers = uniqueReviewers.filter(r => r !== "copilot"); // Add non-copilot reviewers first - if (otherReviewers.length > 0) { - await githubClient.rest.pulls.requestReviewers({ + if (otherReviewers.length > 0 || uniqueTeamReviewers.length > 0) { + /** @type {{ owner: string, repo: string, pull_number: number, reviewers: string[], team_reviewers?: string[] }} */ + const reviewerRequest = { owner: context.repo.owner, repo: context.repo.repo, pull_number: prNumber, reviewers: otherReviewers, - }); - core.info(`Successfully added ${otherReviewers.length} reviewer(s) to PR #${prNumber}`); + }; + if (uniqueTeamReviewers.length > 0) { + reviewerRequest.team_reviewers = uniqueTeamReviewers; + } + await githubClient.rest.pulls.requestReviewers(reviewerRequest); + core.info(`Successfully added reviewers to PR #${prNumber}: reviewers=${JSON.stringify(otherReviewers)}, team_reviewers=${JSON.stringify(uniqueTeamReviewers)}`); } // Add copilot reviewer separately if requested @@ -134,6 +149,7 @@ async function main(config = {}) { success: true, prNumber, reviewersAdded: uniqueReviewers, + teamReviewersAdded: uniqueTeamReviewers, }; } catch (error) { const errorMessage = getErrorMessage(error); diff --git a/actions/setup/js/add_reviewer.test.cjs b/actions/setup/js/add_reviewer.test.cjs index b4a06580bd8..59e64103688 100644 --- a/actions/setup/js/add_reviewer.test.cjs +++ b/actions/setup/js/add_reviewer.test.cjs @@ -84,6 +84,25 @@ describe("add_reviewer (Handler Factory Architecture)", () => { }); }); + it("should add team reviewers to PR", async () => { + const message = { + type: "add_reviewer", + team_reviewers: ["platform-team", "security-team"], + }; + + const result = await handler(message, {}); + + expect(result.success).toBe(true); + expect(result.teamReviewersAdded).toEqual(["platform-team", "security-team"]); + expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + pull_number: 123, + reviewers: [], + team_reviewers: ["platform-team", "security-team"], + }); + }); + it("should add copilot reviewer separately", async () => { const message = { type: "add_reviewer", @@ -110,6 +129,33 @@ describe("add_reviewer (Handler Factory Architecture)", () => { }); }); + it("should keep team reviewers with non-copilot reviewers when copilot is requested", async () => { + const message = { + type: "add_reviewer", + reviewers: ["user1", "copilot"], + team_reviewers: ["platform-team"], + }; + + const result = await handler(message, {}); + + expect(result.success).toBe(true); + expect(result.reviewersAdded).toEqual(["user1", "copilot"]); + expect(result.teamReviewersAdded).toEqual(["platform-team"]); + expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenNthCalledWith(1, { + owner: "testowner", + repo: "testrepo", + pull_number: 123, + reviewers: ["user1"], + team_reviewers: ["platform-team"], + }); + expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenNthCalledWith(2, { + owner: "testowner", + repo: "testrepo", + pull_number: 123, + reviewers: ["copilot-pull-request-reviewer[bot]"], + }); + }); + it("should filter by allowed reviewers", async () => { const message = { type: "add_reviewer", @@ -128,6 +174,28 @@ describe("add_reviewer (Handler Factory Architecture)", () => { }); }); + it("should filter by allowed team reviewers", async () => { + const { main } = require("./add_reviewer.cjs"); + const restrictedHandler = await main({ max: 10, allowed_team_reviewers: ["platform-team"] }); + + const message = { + type: "add_reviewer", + team_reviewers: ["platform-team", "security-team"], + }; + + const result = await restrictedHandler(message, {}); + + expect(result.success).toBe(true); + expect(result.teamReviewersAdded).toEqual(["platform-team"]); + expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + pull_number: 123, + reviewers: [], + team_reviewers: ["platform-team"], + }); + }); + it("should enforce max count limit", async () => { const { main } = require("./add_reviewer.cjs"); const limitedHandler = await main({ max: 1, allowed: ["user1", "user2"] }); @@ -431,6 +499,29 @@ describe("add_reviewer (Handler Factory Architecture)", () => { }); }); + it("should enforce maxCount across reviewers and team_reviewers combined", async () => { + const { main } = require("./add_reviewer.cjs"); + const tightHandler = await main({ max: 3, allowed: [], allowed_team_reviewers: [] }); + + const message = { + type: "add_reviewer", + reviewers: ["user1", "user2", "user3"], + team_reviewers: ["platform-team", "security-team"], + }; + + const result = await tightHandler(message, {}); + + expect(result.success).toBe(true); + expect(result.reviewersAdded).toEqual(["user1", "user2", "user3"]); + expect(result.teamReviewersAdded).toEqual([]); + expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + pull_number: 123, + reviewers: ["user1", "user2", "user3"], + }); + }); + it("should count staged calls toward max processedCount", async () => { const originalEnv = process.env.GH_AW_SAFE_OUTPUTS_STAGED; process.env.GH_AW_SAFE_OUTPUTS_STAGED = "true"; diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 46b50eba205..4a1e1a35686 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -238,6 +238,7 @@ async function main(config = {}) { const titlePrefix = config.title_prefix || ""; const envLabels = config.labels ? (Array.isArray(config.labels) ? config.labels : config.labels.split(",")).map(label => String(label).trim()).filter(label => label) : []; const configReviewers = config.reviewers ? (Array.isArray(config.reviewers) ? config.reviewers : config.reviewers.split(",")).map(r => String(r).trim()).filter(r => r) : []; + const configTeamReviewers = config.team_reviewers ? (Array.isArray(config.team_reviewers) ? config.team_reviewers : config.team_reviewers.split(",")).map(r => String(r).trim()).filter(r => r) : []; const rawAssignees = config.assignees ? (Array.isArray(config.assignees) ? config.assignees : config.assignees.split(",")).map(a => String(a).trim()).filter(a => a) : []; const hasCopilotInAssignees = rawAssignees.some(a => a.toLowerCase() === "copilot"); const configAssignees = sanitizeFallbackAssignees(rawAssignees); @@ -356,6 +357,9 @@ async function main(config = {}) { if (configReviewers.length > 0) { core.info(`Configured reviewers: ${configReviewers.join(", ")}`); } + if (configTeamReviewers.length > 0) { + core.info(`Configured team reviewers: ${configTeamReviewers.join(", ")}`); + } if (configAssignees && configAssignees.length > 0) { core.info(`Configured assignees (for fallback issues): ${configAssignees.join(", ")}`); } @@ -1448,20 +1452,25 @@ ${patchPreview}`; } // Add configured reviewers if specified - if (configReviewers.length > 0) { + if (configReviewers.length > 0 || configTeamReviewers.length > 0) { const hasCopilot = configReviewers.includes("copilot"); const otherReviewers = configReviewers.filter(r => r !== "copilot"); - if (otherReviewers.length > 0) { - core.info(`Requesting ${otherReviewers.length} reviewer(s) for pull request #${pullRequest.number}: ${JSON.stringify(otherReviewers)}`); + if (otherReviewers.length > 0 || configTeamReviewers.length > 0) { + core.info(`Requesting reviewers for pull request #${pullRequest.number}: reviewers=${JSON.stringify(otherReviewers)}, team_reviewers=${JSON.stringify(configTeamReviewers)}`); try { - await githubClient.rest.pulls.requestReviewers({ + /** @type {{ owner: string, repo: string, pull_number: number, reviewers: string[], team_reviewers?: string[] }} */ + const reviewerRequest = { owner: repoParts.owner, repo: repoParts.repo, pull_number: pullRequest.number, reviewers: otherReviewers, - }); - core.info(`Requested reviewers for pull request #${pullRequest.number}: ${JSON.stringify(otherReviewers)}`); + }; + if (configTeamReviewers.length > 0) { + reviewerRequest.team_reviewers = configTeamReviewers; + } + await githubClient.rest.pulls.requestReviewers(reviewerRequest); + core.info(`Requested reviewers for pull request #${pullRequest.number}: reviewers=${JSON.stringify(otherReviewers)}, team_reviewers=${JSON.stringify(configTeamReviewers)}`); } catch (reviewerError) { core.warning(`Failed to request reviewers for PR #${pullRequest.number}: ${reviewerError instanceof Error ? reviewerError.message : String(reviewerError)}`); } diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index e6840627545..9f1ab5f3351 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -999,6 +999,24 @@ describe("create_pull_request - configured reviewers", () => { ); }); + it("should request configured team reviewers after creating the PR", async () => { + const { main } = require("./create_pull_request.cjs"); + const handler = await main({ team_reviewers: ["platform-team"], allow_empty: true }); + + const result = await handler({ title: "Test PR", body: "Test body" }, {}); + + expect(result.success).toBe(true); + expect(global.github.rest.pulls.requestReviewers).toHaveBeenCalledWith( + expect.objectContaining({ + owner: "test-owner", + repo: "test-repo", + pull_number: 42, + reviewers: [], + team_reviewers: ["platform-team"], + }) + ); + }); + it("should handle copilot reviewer separately from regular reviewers", async () => { const { main } = require("./create_pull_request.cjs"); const handler = await main({ reviewers: ["user1", "copilot"], allow_empty: true }); @@ -1012,6 +1030,18 @@ describe("create_pull_request - configured reviewers", () => { expect(global.github.rest.pulls.requestReviewers).toHaveBeenCalledWith(expect.objectContaining({ reviewers: ["copilot-pull-request-reviewer[bot]"] })); }); + it("should keep configured team reviewers with non-copilot reviewers when copilot is configured", async () => { + const { main } = require("./create_pull_request.cjs"); + const handler = await main({ reviewers: ["user1", "copilot"], team_reviewers: ["platform-team"], allow_empty: true }); + + const result = await handler({ title: "Test PR", body: "Test body" }, {}); + + expect(result.success).toBe(true); + expect(global.github.rest.pulls.requestReviewers).toHaveBeenCalledTimes(2); + expect(global.github.rest.pulls.requestReviewers).toHaveBeenNthCalledWith(1, expect.objectContaining({ reviewers: ["user1"], team_reviewers: ["platform-team"] })); + expect(global.github.rest.pulls.requestReviewers).toHaveBeenNthCalledWith(2, expect.objectContaining({ reviewers: ["copilot-pull-request-reviewer[bot]"] })); + }); + it("should not call requestReviewers when no reviewers are configured", async () => { const { main } = require("./create_pull_request.cjs"); const handler = await main({ allow_empty: true }); diff --git a/actions/setup/js/types/safe-outputs-config.d.ts b/actions/setup/js/types/safe-outputs-config.d.ts index fe05260545d..a48f34c97bc 100644 --- a/actions/setup/js/types/safe-outputs-config.d.ts +++ b/actions/setup/js/types/safe-outputs-config.d.ts @@ -82,6 +82,7 @@ interface CreatePullRequestConfig extends SafeOutputConfig { "title-prefix"?: string; labels?: string[]; reviewers?: string | string[]; + "team-reviewers"?: string | string[]; assignees?: string | string[]; draft?: boolean; "if-no-changes"?: string; @@ -161,6 +162,7 @@ interface AddLabelsConfig extends SafeOutputConfig { */ interface AddReviewerConfig extends SafeOutputConfig { reviewers?: string[]; + "team-reviewers"?: string[]; target?: string; } diff --git a/docs/adr/26915-add-team-reviewer-support-to-safe-outputs.md b/docs/adr/26915-add-team-reviewer-support-to-safe-outputs.md new file mode 100644 index 00000000000..69050855d81 --- /dev/null +++ b/docs/adr/26915-add-team-reviewer-support-to-safe-outputs.md @@ -0,0 +1,87 @@ +# ADR-26915: Add Team Reviewer Support to `create-pull-request` and `add-reviewer` Safe Outputs + +**Date**: 2026-04-17 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The `create-pull-request` and `add-reviewer` safe output handlers previously only supported individual user reviewers through a `reviewers` field. GitHub's pull request API exposes a distinct `team_reviewers` parameter for requesting reviews from GitHub teams (identified by team slugs). Workflows that needed to assign team reviewers had no supported mechanism — team slug values passed through the `reviewers` field would be silently dropped before reaching the GitHub API. This gap blocked users who want to enforce team-level code review policies through agentic workflows. + +### Decision + +We will add a dedicated `team-reviewers` field to both the `create-pull-request` and `add-reviewer` safe output configurations. This field accepts either a single team slug string or an array of team slugs, mirroring the normalization behavior of the existing `reviewers` field. The compiler will pass configured team reviewers to the corresponding handler config keys (`team_reviewers` for `create_pull_request`, `allowed_team_reviewers` for `add_reviewer`), and the JavaScript runtime handlers will forward them to GitHub's `pulls.requestReviewers` API. The `add-reviewer` safe output's validation will be updated to require at least one of `reviewers` or `team_reviewers` rather than requiring `reviewers` unconditionally. + +### Alternatives Considered + +#### Alternative 1: Reuse the existing `reviewers` field with a prefix convention + +Team slugs could be distinguished from user handles by a naming convention (e.g., `team:platform-reviewers`). This avoids schema changes but introduces an implicit, fragile parsing convention with no schema-level validation support. It also deviates from GitHub's own API model, which treats user and team reviewers as distinct parameters — mixing them into one field risks future ambiguity and makes allowlist configuration more complex. + +#### Alternative 2: Add team reviewer support only to `add-reviewer`, not `create-pull-request` + +Since `add-reviewer` is a discrete action, it could be modified first as a lower-risk change. However, `create-pull-request` also configures reviewers at PR creation time through the same handler infrastructure. Splitting the implementation would result in inconsistent behavior between the two safe outputs, requiring users to work around the gap with an extra `add-reviewer` step after every PR creation. + +#### Alternative 3: Expose a generic `extra_params` pass-through in the safe output config + +A free-form escape hatch would let callers forward arbitrary GitHub API parameters including `team_reviewers`. This was not chosen because it bypasses the allowlist security model that safe outputs are built on — the design requires that all outputs be validated against operator-configured constraints before reaching the GitHub API. + +### Consequences + +#### Positive +- Workflows can now request team reviews at PR creation time and via the `add-reviewer` handler, removing a gap in the safe output API surface. +- The implementation is consistent with the existing `reviewers` field: single-string-to-array normalization, allowlist filtering, and schema validation all work identically for team reviewers. +- Existing `copilot` reviewer handling is unaffected and continues to be requested through its own code path. + +#### Negative +- Operators must now configure `allowed_team_reviewers` in their handler config to restrict which teams can be requested as reviewers, adding a new allowlist to maintain. +- The `add-reviewer` safe output now uses a `requiresOneOf` validation rule instead of a hard `required: true` on `reviewers`, which is a slight loosening of the input schema that must be communicated to existing users. + +#### Neutral +- The JSON schemas (`main_workflow_schema.json`, `safe_outputs_tools.json`) and TypeScript config typings are updated to reflect the new field. +- Go and JavaScript tests are extended to cover handler config emission, pass-through, copilot coexistence, and allowlist filtering for team reviewers. +- Team slug length is validated against a new `MaxGitHubTeamSlugLength = 100` constant, consistent with how usernames are validated via `MaxGitHubUsernameLength`. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Safe Output Configuration + +1. Implementations **MUST** accept a `team-reviewers` field in both the `create-pull-request` and `add-reviewer` safe output configuration blocks. +2. Implementations **MUST** normalize a single team-slug string value for `team-reviewers` to a single-element array before YAML unmarshaling. +3. Implementations **MUST NOT** silently drop `team-reviewers` values during config parsing or handler config generation. +4. Implementations **MUST** validate each team slug against `MaxGitHubTeamSlugLength` (100 characters). + +### Handler Config Generation + +1. Implementations **MUST** emit `team_reviewers` in the `create_pull_request` handler config when `team-reviewers` is configured. +2. Implementations **MUST** emit `allowed_team_reviewers` in the `add_reviewer` handler config when `team-reviewers` is configured. +3. Implementations **MUST NOT** merge team reviewer slugs into the user `reviewers` / `allowed` handler config fields. + +### Runtime Behavior + +1. The `create_pull_request` handler **MUST** forward configured team reviewers to the `team_reviewers` parameter of the GitHub `pulls.requestReviewers` API call. +2. The `add_reviewer` handler **MUST** filter team reviewers from tool output against the `allowed_team_reviewers` allowlist before forwarding to the GitHub API. +3. The `add_reviewer` handler **MUST** accept tool outputs that contain `team_reviewers` but no `reviewers`, and vice versa; at least one **MUST** be present. +4. The Copilot reviewer **SHOULD** continue to be requested via its dedicated code path and **MUST NOT** be affected by team reviewer changes. + +### Schema and Validation + +1. The workflow schema (`main_workflow_schema.json`) **MUST** declare `team-reviewers` as an optional field accepting a string or array of strings for `create-pull-request`, and as an optional array of strings for `add-reviewer`. +2. The safe output tool schema (`safe_outputs_tools.json`) **MUST** reflect that `add_reviewer` requires at least one of `reviewers` or `team_reviewers` via an `anyOf` constraint. +3. Implementations **MUST** apply `ItemSanitize: true` to `team_reviewers` field validation, consistent with `reviewers`. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24580129798) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/docs/src/content/docs/reference/safe-outputs-pull-requests.md b/docs/src/content/docs/reference/safe-outputs-pull-requests.md index 6c214c027e1..c574a39333d 100644 --- a/docs/src/content/docs/reference/safe-outputs-pull-requests.md +++ b/docs/src/content/docs/reference/safe-outputs-pull-requests.md @@ -32,6 +32,7 @@ safe-outputs: title-prefix: "[ai] " # prefix for titles labels: [automation] # labels to attach reviewers: [user1, copilot] # reviewers (use 'copilot' for bot) + team-reviewers: [platform-reviewers] # team slugs to request as reviewers assignees: [user1] # assignees for fallback issues (including protected-files and PR creation failure fallbacks) draft: true # create as draft — enforced as policy (default: true) max: 3 # max PRs per run (default: 1) @@ -228,12 +229,13 @@ Like `create-pull-request`, pushes with GitHub Agentic Workflows do not trigger ## Add Reviewer (`add-reviewer:`) -Adds reviewers to pull requests. Specify `reviewers` to restrict to specific GitHub usernames. +Adds reviewers to pull requests. Specify `reviewers` to restrict to specific GitHub usernames and `team-reviewers` to restrict to specific team slugs. ```yaml wrap safe-outputs: add-reviewer: - reviewers: [user1, copilot] # restrict to specific reviewers + reviewers: [user1, copilot] # restrict to specific user/bot reviewers + team-reviewers: [platform-reviewers] # restrict to specific team reviewers max: 3 # max reviewers (default: 3) target: "*" # "triggering" (default), "*", or number target-repo: "owner/repo" # cross-repository diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 42abef8ec10..ccc3a9cfcad 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -5765,6 +5765,22 @@ ], "description": "Optional reviewer(s) to assign to the pull request. Accepts either a single string or an array of usernames. Use 'copilot' to request a code review from GitHub Copilot." }, + "team-reviewers": { + "oneOf": [ + { + "type": "string", + "description": "Single team slug to assign as a reviewer to the pull request." + }, + { + "type": "array", + "description": "List of team slugs to assign as reviewers to the pull request.", + "items": { + "type": "string" + } + } + ], + "description": "Optional team reviewer(s) to assign to the pull request. Accepts either a single string or an array of team slugs." + }, "assignees": { "oneOf": [ { @@ -6444,6 +6460,22 @@ "minItems": 1, "maxItems": 50 }, + "team-reviewers": { + "description": "Optional allowed team reviewer or list of allowed team reviewers. If omitted, any team reviewers are allowed.", + "oneOf": [ + { + "type": "string" + }, + { + "type": "array", + "items": { + "type": "string" + }, + "minItems": 1, + "maxItems": 50 + } + ] + }, "max": { "description": "Optional maximum number of reviewers to add (default: 3) Supports integer or GitHub Actions expression (e.g. '${{ inputs.max }}').", "oneOf": [ diff --git a/pkg/workflow/add_reviewer.go b/pkg/workflow/add_reviewer.go index 8d27898c153..b61249df6dd 100644 --- a/pkg/workflow/add_reviewer.go +++ b/pkg/workflow/add_reviewer.go @@ -10,7 +10,8 @@ var addReviewerLog = logger.New("workflow:add_reviewer") type AddReviewerConfig struct { BaseSafeOutputConfig `yaml:",inline"` SafeOutputTargetConfig `yaml:",inline"` - Reviewers []string `yaml:"reviewers,omitempty"` // Optional list of allowed reviewers. If omitted, any reviewers are allowed. + Reviewers []string `yaml:"reviewers,omitempty"` // Optional list of allowed reviewers. If omitted, any reviewers are allowed. + TeamReviewers []string `yaml:"team-reviewers,omitempty"` // Optional list of allowed team reviewers. If omitted, any team reviewers are allowed. } // parseAddReviewerConfig handles add-reviewer configuration @@ -25,6 +26,20 @@ func (c *Compiler) parseAddReviewerConfig(outputMap map[string]any) *AddReviewer // Get config data for pre-processing before YAML unmarshaling configData, _ := outputMap["add-reviewer"].(map[string]any) + // Pre-process reviewers fields to convert single string to array BEFORE unmarshaling + if configData != nil { + if reviewers, exists := configData["reviewers"]; exists { + if reviewerStr, ok := reviewers.(string); ok { + configData["reviewers"] = []string{reviewerStr} + } + } + if teamReviewers, exists := configData["team-reviewers"]; exists { + if teamReviewerStr, ok := teamReviewers.(string); ok { + configData["team-reviewers"] = []string{teamReviewerStr} + } + } + } + // Pre-process templatable int fields if err := preprocessIntFieldAsString(configData, "max", addReviewerLog); err != nil { addReviewerLog.Printf("Invalid max value: %v", err) diff --git a/pkg/workflow/compiler_safe_outputs_config_test.go b/pkg/workflow/compiler_safe_outputs_config_test.go index 6c59b462b6f..8d1bfdb30ac 100644 --- a/pkg/workflow/compiler_safe_outputs_config_test.go +++ b/pkg/workflow/compiler_safe_outputs_config_test.go @@ -900,7 +900,8 @@ func TestHandlerConfigReviewers(t *testing.T) { Name: "Test Workflow", SafeOutputs: &SafeOutputsConfig{ CreatePullRequests: &CreatePullRequestsConfig{ - Reviewers: []string{"user1", "user2", "copilot"}, + Reviewers: []string{"user1", "user2", "copilot"}, + TeamReviewers: []string{"team-a", "team-b"}, }, }, } @@ -933,6 +934,58 @@ func TestHandlerConfigReviewers(t *testing.T) { assert.Equal(t, "user1", reviewerSlice[0]) assert.Equal(t, "user2", reviewerSlice[1]) assert.Equal(t, "copilot", reviewerSlice[2]) + + teamReviewers, ok := prConfig["team_reviewers"] + require.True(t, ok, "Should have team_reviewers field") + + teamReviewerSlice, ok := teamReviewers.([]any) + require.True(t, ok, "team_reviewers should be an array") + assert.Len(t, teamReviewerSlice, 2, "Should have 2 team reviewers") + assert.Equal(t, "team-a", teamReviewerSlice[0]) + assert.Equal(t, "team-b", teamReviewerSlice[1]) + } + } + } +} + +func TestHandlerConfigAddReviewerTeamReviewers(t *testing.T) { + compiler := NewCompiler() + + workflowData := &WorkflowData{ + Name: "Test Workflow", + SafeOutputs: &SafeOutputsConfig{ + AddReviewer: &AddReviewerConfig{ + Reviewers: []string{"user1"}, + TeamReviewers: []string{"team-a"}, + }, + }, + } + + var steps []string + compiler.addHandlerManagerConfigEnvVar(&steps, workflowData) + + for _, step := range steps { + if strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") { + parts := strings.Split(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ") + if len(parts) == 2 { + jsonStr := strings.TrimSpace(parts[1]) + jsonStr = strings.Trim(jsonStr, "\"") + jsonStr = strings.ReplaceAll(jsonStr, "\\\"", "\"") + + var config map[string]map[string]any + err := json.Unmarshal([]byte(jsonStr), &config) + require.NoError(t, err, "Handler config JSON should be valid") + + reviewerConfig, ok := config["add_reviewer"] + require.True(t, ok, "Should have add_reviewer handler") + + teamReviewers, ok := reviewerConfig["allowed_team_reviewers"] + require.True(t, ok, "Should have allowed_team_reviewers field") + + teamReviewerSlice, ok := teamReviewers.([]any) + require.True(t, ok, "allowed_team_reviewers should be an array") + assert.Len(t, teamReviewerSlice, 1, "Should have 1 allowed team reviewer") + assert.Equal(t, "team-a", teamReviewerSlice[0]) } } } diff --git a/pkg/workflow/compiler_safe_outputs_handlers.go b/pkg/workflow/compiler_safe_outputs_handlers.go index a6ec04e4cb0..4a8c2ec8a24 100644 --- a/pkg/workflow/compiler_safe_outputs_handlers.go +++ b/pkg/workflow/compiler_safe_outputs_handlers.go @@ -145,6 +145,7 @@ var handlerRegistry = map[string]handlerBuilder{ return newHandlerConfigBuilder(). AddTemplatableInt("max", c.Max). AddStringSlice("allowed", c.Reviewers). + AddStringSlice("allowed_team_reviewers", c.TeamReviewers). AddIfNotEmpty("target", c.Target). AddIfNotEmpty("target-repo", c.TargetRepoSlug). AddStringSlice("allowed_repos", c.AllowedRepos). @@ -368,6 +369,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddIfNotEmpty("title_prefix", c.TitlePrefix). AddStringSlice("labels", c.Labels). AddStringSlice("reviewers", c.Reviewers). + AddStringSlice("team_reviewers", c.TeamReviewers). AddStringSlice("assignees", c.Assignees). AddTemplatableBool("draft", c.Draft). AddIfNotEmpty("if_no_changes", c.IfNoChanges). diff --git a/pkg/workflow/create_pull_request.go b/pkg/workflow/create_pull_request.go index 270da2522b4..c5dff8909fb 100644 --- a/pkg/workflow/create_pull_request.go +++ b/pkg/workflow/create_pull_request.go @@ -21,6 +21,7 @@ type CreatePullRequestsConfig struct { Labels []string `yaml:"labels,omitempty"` AllowedLabels []string `yaml:"allowed-labels,omitempty"` // Optional list of allowed labels. If omitted, any labels are allowed (including creating new ones). Reviewers []string `yaml:"reviewers,omitempty"` // List of users/bots to assign as reviewers to the pull request + TeamReviewers []string `yaml:"team-reviewers,omitempty"` // List of team slugs to assign as team reviewers to the pull request Assignees []string `yaml:"assignees,omitempty"` // List of users to assign to any fallback issue created by create-pull-request Draft *string `yaml:"draft,omitempty"` // Pointer to distinguish between unset (nil), literal bool, and expression values IfNoChanges string `yaml:"if-no-changes,omitempty"` // Behavior when no changes to push: "warn" (default), "error", or "ignore" @@ -66,6 +67,13 @@ func (c *Compiler) parsePullRequestsConfig(outputMap map[string]any) *CreatePull createPRLog.Printf("Converted single reviewer string to array before unmarshaling") } } + if teamReviewers, exists := configData["team-reviewers"]; exists { + if teamReviewerStr, ok := teamReviewers.(string); ok { + // Convert single string to array + configData["team-reviewers"] = []string{teamReviewerStr} + createPRLog.Printf("Converted single team-reviewer string to array before unmarshaling") + } + } // Pre-process the assignees field to convert single string to array BEFORE unmarshaling if assignees, exists := configData["assignees"]; exists { if assigneeStr, ok := assignees.(string); ok { diff --git a/pkg/workflow/js/safe_outputs_tools.json b/pkg/workflow/js/safe_outputs_tools.json index 02bb7c0b5a9..4a469e4c2c5 100644 --- a/pkg/workflow/js/safe_outputs_tools.json +++ b/pkg/workflow/js/safe_outputs_tools.json @@ -632,8 +632,17 @@ "description": "Add reviewers to a GitHub pull request. Reviewers receive notifications and can approve or request changes. Use 'copilot' as a reviewer name to request the Copilot PR review bot.", "inputSchema": { "type": "object", - "required": [ - "reviewers" + "anyOf": [ + { + "required": [ + "reviewers" + ] + }, + { + "required": [ + "team_reviewers" + ] + } ], "properties": { "reviewers": { @@ -643,6 +652,13 @@ }, "description": "GitHub usernames to add as reviewers (e.g., ['octocat', 'copilot']). Users must have access to the repository." }, + "team_reviewers": { + "type": "array", + "items": { + "type": "string" + }, + "description": "GitHub team slugs to add as team reviewers (e.g., ['platform-team', 'security-reviewers']). Teams must have access to the repository." + }, "pull_request_number": { "type": [ "number", diff --git a/pkg/workflow/safe_output_validation_config_test.go b/pkg/workflow/safe_output_validation_config_test.go index b0a0ca16aa2..9c02441c118 100644 --- a/pkg/workflow/safe_output_validation_config_test.go +++ b/pkg/workflow/safe_output_validation_config_test.go @@ -195,6 +195,7 @@ func TestValidationConfigConsistency(t *testing.T) { "requiresOneOf:title,body": true, "requiresOneOf:title,body,labels": true, "requiresOneOf:issue_number,pull_number": true, + "requiresOneOf:reviewers,team_reviewers": true, "startLineLessOrEqualLine": true, "parentAndSubDifferent": true, } diff --git a/pkg/workflow/safe_outputs_config_generation_test.go b/pkg/workflow/safe_outputs_config_generation_test.go index c6f36a6697f..26ffde8a690 100644 --- a/pkg/workflow/safe_outputs_config_generation_test.go +++ b/pkg/workflow/safe_outputs_config_generation_test.go @@ -448,6 +448,7 @@ func TestGenerateSafeOutputsConfigCreatePullRequestTargetRepo(t *testing.T) { BaseBranch: "dev", Draft: strPtr("true"), Reviewers: []string{"corb3nik"}, + TeamReviewers: []string{"platform-reviewers"}, TitlePrefix: "[refactor] ", FallbackAsIssue: &falseVal, }, @@ -479,6 +480,11 @@ func TestGenerateSafeOutputsConfigCreatePullRequestTargetRepo(t *testing.T) { assert.Len(t, reviewers, 1, "Should have 1 reviewer") assert.Equal(t, "corb3nik", reviewers[0], "reviewer should match") + teamReviewers, ok := prConfig["team_reviewers"].([]any) + require.True(t, ok, "team_reviewers should be an array") + assert.Len(t, teamReviewers, 1, "Should have 1 team reviewer") + assert.Equal(t, "platform-reviewers", teamReviewers[0], "team reviewer should match") + assert.Equal(t, "[refactor] ", prConfig["title_prefix"], "title_prefix should be set") assert.Equal(t, false, prConfig["fallback_as_issue"], "fallback_as_issue should be false") } diff --git a/pkg/workflow/safe_outputs_validation_config.go b/pkg/workflow/safe_outputs_validation_config.go index e13a3871feb..09aeb601a7a 100644 --- a/pkg/workflow/safe_outputs_validation_config.go +++ b/pkg/workflow/safe_outputs_validation_config.go @@ -38,6 +38,7 @@ type TypeValidationConfig struct { const ( MaxBodyLength = 65000 MaxGitHubUsernameLength = 39 + MaxGitHubTeamSlugLength = 100 ) // ValidationConfig contains all safe output type validation rules @@ -90,9 +91,11 @@ var ValidationConfig = map[string]TypeValidationConfig{ }, }, "add_reviewer": { - DefaultMax: 3, + DefaultMax: 3, + CustomValidation: "requiresOneOf:reviewers,team_reviewers", Fields: map[string]FieldValidation{ - "reviewers": {Required: true, Type: "array", ItemType: "string", ItemSanitize: true, ItemMaxLength: MaxGitHubUsernameLength}, + "reviewers": {Type: "array", ItemType: "string", ItemSanitize: true, ItemMaxLength: MaxGitHubUsernameLength}, + "team_reviewers": {Type: "array", ItemType: "string", ItemSanitize: true, ItemMaxLength: MaxGitHubTeamSlugLength}, "pull_request_number": {IssueOrPRNumber: true}, "repo": {Type: "string", MaxLength: 256}, // Optional: target repository in format "owner/repo" },