From 8dc30d6dbfa687719ac123dc830101c95fe79823 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Thu, 9 Jul 2026 19:02:24 -0400 Subject: [PATCH 1/3] Notify external area owners on PRs via Policy Service GitHub doesn't allow non-collaborators in CODEOWNERS or as formal reviewers, so external area owners (e.g. graphql) are now @-mentioned in a comment via a generated Policy Service policy instead of the broken requestReview approach. --- .../prs.external-reviewers.generated.yml | 31 +++++++++ .github/policies/prs.external-reviewers.yml | 21 ------- CONTRIBUTING.md | 8 +++ eng/common/config/area.ts | 20 ++++++ eng/common/config/labels.ts | 16 ++++- eng/common/scripts/labels/automation.ts | 63 ++++++++++++++++++- eng/common/scripts/labels/policy.ts | 20 ++++++ eng/common/scripts/labels/types.ts | 1 + 8 files changed, 156 insertions(+), 24 deletions(-) create mode 100644 .github/policies/prs.external-reviewers.generated.yml delete mode 100644 .github/policies/prs.external-reviewers.yml diff --git a/.github/policies/prs.external-reviewers.generated.yml b/.github/policies/prs.external-reviewers.generated.yml new file mode 100644 index 00000000000..d5303af4f9d --- /dev/null +++ b/.github/policies/prs.external-reviewers.generated.yml @@ -0,0 +1,31 @@ +# This file is generated by the sync-labels script DO NOT EDIT manually +# cspell:ignore steverice swatkatz +id: prs.external-reviewers +name: Notify external area owners on PRs +description: GitHub doesn't allow CODEOWNERS entries or formal review requests + for users that are not repository collaborators, but some areas are + contributed by external teams. Mention those owners in a comment instead. + https://github.com/orgs/community/discussions/23042 +resource: repository +disabled: false +configuration: + resourceManagementConfiguration: + eventResponderTasks: + - if: + - payloadType: Pull_Request + then: + - if: + - includesModifiedFiles: + files: + - packages/graphql/**/* + - not: + hasLabel: + label: external-review:emitter:graphql + then: + - addReply: + reply: "@steverice @swatkatz — this PR modifies files in the `emitter:graphql` + area, which your team owns. Please take a look. (You can't + be added as a formal reviewer because you're not a + repository collaborator, so this is a heads-up instead.)" + - addLabel: + label: external-review:emitter:graphql diff --git a/.github/policies/prs.external-reviewers.yml b/.github/policies/prs.external-reviewers.yml deleted file mode 100644 index 697150e62a5..00000000000 --- a/.github/policies/prs.external-reviewers.yml +++ /dev/null @@ -1,21 +0,0 @@ -# cspell:ignore swatkatz -id: prs.external-reviewers -name: Assign External Reviewers to PRs. -description: Github doesn't allow CODEOWNERS to have external users but some emitters are contributed externally https://github.com/orgs/community/discussions/23042 -resource: repository -disabled: false -configuration: - resourceManagementConfiguration: - eventResponderTasks: - - if: - - payloadType: Pull_Request - then: - - if: - - includesModifiedFiles: - files: - - packages/graphql/**/* - then: - - requestReview: - reviewer: steverice - - requestReview: - reviewer: swatkatz diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e3e55323483..03dee5fb0e5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -462,6 +462,14 @@ Process labels | `stale` | #ffffff | Mark a PR that hasn't been recently updated and will be closed. | | `triaged:core` | #5319e7 | | +#### external-review + +Markers indicating that the external owners of an area have been notified on a PR + +| Name | Color | Description | +| --------------------------------- | ------- | ------------------------------------------------------------------------------- | +| `external-review:emitter:graphql` | #ededed | The external owners of the 'emitter:graphql' area have been notified on this PR | + #### misc Misc labels diff --git a/eng/common/config/area.ts b/eng/common/config/area.ts index d66a60b59de..dfe381c2038 100644 --- a/eng/common/config/area.ts +++ b/eng/common/config/area.ts @@ -34,6 +34,26 @@ export const AreaPaths: Record = { spector: ["packages/spector/", "packages/http-specs"], }; +/** + * External area owners: teams/users outside the org that should be notified on PRs + * touching their area. GitHub does not allow non-collaborators in CODEOWNERS, nor can + * they be added as formal reviewers, so they are @-mentioned in a comment instead. + * + * Keyed by the same area names as {@link AreaPaths}; the paths are reused from there. + */ +export const ExternalOwners: Partial> = { + // cspell:ignore swatkatz + "emitter:graphql": ["steverice", "swatkatz"], +}; + +/** + * Marker label applied once external owners of an area have been notified on a PR, + * used to avoid re-pinging them on every subsequent push. + */ +export function externalReviewMarkerLabel(area: string): string { + return `external-review:${area}`; +} + /** * Path that should trigger every CI build. */ diff --git a/eng/common/config/labels.ts b/eng/common/config/labels.ts index 286c297f5f5..be05d1c2340 100644 --- a/eng/common/config/labels.ts +++ b/eng/common/config/labels.ts @@ -1,7 +1,7 @@ // cspell:ignore bfff import { defineConfig, defineLabels } from "../scripts/labels/config.js"; import { repo } from "../scripts/utils/common.js"; -import { AreaPaths } from "./area.js"; +import { AreaPaths, ExternalOwners, externalReviewMarkerLabel } from "./area.js"; /** * Labels that are used to categorize issue for which area they belong to. @@ -197,6 +197,19 @@ export const CommonLabels = { }, }, }, + "external-review": { + description: + "Markers indicating that the external owners of an area have been notified on a PR", + labels: Object.fromEntries( + Object.keys(ExternalOwners).map((area) => [ + externalReviewMarkerLabel(area), + { + color: "ededed", + description: `The external owners of the '${area}' area have been notified on this PR`, + }, + ]), + ), + }, }; export default defineConfig({ @@ -230,4 +243,5 @@ export default defineConfig({ }, }, areaPaths: AreaPaths, + externalOwners: ExternalOwners, }); diff --git a/eng/common/scripts/labels/automation.ts b/eng/common/scripts/labels/automation.ts index f3b110cc2ee..6b2274a2476 100644 --- a/eng/common/scripts/labels/automation.ts +++ b/eng/common/scripts/labels/automation.ts @@ -1,9 +1,12 @@ import { resolve } from "path"; import { stringify } from "yaml"; +import { externalReviewMarkerLabel } from "../../config/area.js"; import { CheckOptions, syncFile } from "../utils/common.js"; import { expandFolder } from "../utils/find-area-changed.js"; import { PolicyServiceConfig, + addLabel, + addReply, and, eventResponderTask, hasLabel, @@ -24,15 +27,35 @@ export interface SyncLabelAutomationOptions extends CheckOptions {} export async function syncLabelAutomation(config: RepoConfig, options: SyncLabelAutomationOptions) { await syncPolicyFile(createIssueTriageConfig(config), options); await syncPolicyFile(createPrTriageConfig(config), options); + await syncPolicyFile( + createExternalReviewersConfig(config), + options, + collectExternalOwners(config), + ); } -async function syncPolicyFile(policy: PolicyServiceConfig, options: CheckOptions) { +async function syncPolicyFile( + policy: PolicyServiceConfig, + options: CheckOptions, + ignoreWords: string[] = [], +) { const yaml = stringify(policy); - const content = `# This file is generated by the sync-labels script DO NOT EDIT manually\n${yaml}`; + const cspell = ignoreWords.length > 0 ? `# cspell:ignore ${ignoreWords.join(" ")}\n` : ""; + const content = `# This file is generated by the sync-labels script DO NOT EDIT manually\n${cspell}${yaml}`; const filename = resolve(policyFolder, `${policy.id}.generated.yml`); await syncFile(filename, content, options); } +function collectExternalOwners(config: RepoConfig): string[] { + const all = new Set(); + for (const owners of Object.values(config.externalOwners)) { + for (const owner of owners) { + all.add(owner); + } + } + return [...all].sort(); +} + function createIssueTriageConfig(config: RepoConfig): PolicyServiceConfig { const areaLabels = config.labels.area.labels; return { @@ -129,3 +152,39 @@ function createPrTriageConfig(config: RepoConfig): PolicyServiceConfig { }, }; } +function createExternalReviewersConfig(config: RepoConfig): PolicyServiceConfig { + const entries = Object.entries(config.externalOwners).filter(([, owners]) => owners.length > 0); + return { + id: "prs.external-reviewers", + name: "Notify external area owners on PRs", + description: + "GitHub doesn't allow CODEOWNERS entries or formal review requests for users that are not repository collaborators, but some areas are contributed by external teams. Mention those owners in a comment instead. https://github.com/orgs/community/discussions/23042", + resource: "repository", + disabled: false, + configuration: { + resourceManagementConfiguration: { + eventResponderTasks: [ + eventResponderTask({ + if: [payloadType("Pull_Request")], + then: entries.map(([area, owners]) => { + const globs = (config.areaPaths[area] ?? []).map(expandFolder); + const marker = externalReviewMarkerLabel(area); + const mentions = owners.map((owner) => `@${owner}`).join(" "); + // Only ping the first time the paths match (guarded by the marker label) so + // owners are not re-notified on every subsequent push. + return { + if: [includesModifiedFiles(globs), not(hasLabel(marker))], + then: [ + addReply( + `${mentions} — this PR modifies files in the \`${area}\` area, which your team owns. Please take a look. (You can't be added as a formal reviewer because you're not a repository collaborator, so this is a heads-up instead.)`, + ), + addLabel(marker), + ], + }; + }), + }), + ], + }, + }, + }; +} diff --git a/eng/common/scripts/labels/policy.ts b/eng/common/scripts/labels/policy.ts index fb04c9b6348..df2307d368f 100644 --- a/eng/common/scripts/labels/policy.ts +++ b/eng/common/scripts/labels/policy.ts @@ -166,3 +166,23 @@ export type EventResponderTask = { export function eventResponderTask(options: EventResponderTask): EventResponderTask { return options; } + +export type AddReply = { + addReply: { reply: string }; +}; + +/** + * Post a comment on the issue/PR. @-mentions in the body notify the mentioned users, + * including users that are not repository collaborators. + */ +export function addReply(reply: string): AddReply { + return { addReply: { reply } }; +} + +export type AddLabel = { + addLabel: { label: string }; +}; + +export function addLabel(label: string): AddLabel { + return { addLabel: { label } }; +} diff --git a/eng/common/scripts/labels/types.ts b/eng/common/scripts/labels/types.ts index a461d553d12..4c1809f6d71 100644 --- a/eng/common/scripts/labels/types.ts +++ b/eng/common/scripts/labels/types.ts @@ -5,6 +5,7 @@ export interface RepoConfig { }; labels: LabelsConfig; areaPaths: Record; + externalOwners: Record; } export interface LabelsConfig { From dc00201868f6c77bf66874e1fd11e240355c6d7e Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Fri, 10 Jul 2026 08:37:38 -0400 Subject: [PATCH 2/3] Reuse area label as notification guard instead of a new marker label --- .../policies/prs.external-reviewers.generated.yml | 4 +--- CONTRIBUTING.md | 8 -------- eng/common/config/area.ts | 8 -------- eng/common/config/labels.ts | 15 +-------------- eng/common/scripts/labels/automation.ts | 12 +++++------- 5 files changed, 7 insertions(+), 40 deletions(-) diff --git a/.github/policies/prs.external-reviewers.generated.yml b/.github/policies/prs.external-reviewers.generated.yml index d5303af4f9d..cd1120d67dc 100644 --- a/.github/policies/prs.external-reviewers.generated.yml +++ b/.github/policies/prs.external-reviewers.generated.yml @@ -20,12 +20,10 @@ configuration: - packages/graphql/**/* - not: hasLabel: - label: external-review:emitter:graphql + label: emitter:graphql then: - addReply: reply: "@steverice @swatkatz — this PR modifies files in the `emitter:graphql` area, which your team owns. Please take a look. (You can't be added as a formal reviewer because you're not a repository collaborator, so this is a heads-up instead.)" - - addLabel: - label: external-review:emitter:graphql diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 03dee5fb0e5..e3e55323483 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -462,14 +462,6 @@ Process labels | `stale` | #ffffff | Mark a PR that hasn't been recently updated and will be closed. | | `triaged:core` | #5319e7 | | -#### external-review - -Markers indicating that the external owners of an area have been notified on a PR - -| Name | Color | Description | -| --------------------------------- | ------- | ------------------------------------------------------------------------------- | -| `external-review:emitter:graphql` | #ededed | The external owners of the 'emitter:graphql' area have been notified on this PR | - #### misc Misc labels diff --git a/eng/common/config/area.ts b/eng/common/config/area.ts index dfe381c2038..08801be9c38 100644 --- a/eng/common/config/area.ts +++ b/eng/common/config/area.ts @@ -46,14 +46,6 @@ export const ExternalOwners: Partial> "emitter:graphql": ["steverice", "swatkatz"], }; -/** - * Marker label applied once external owners of an area have been notified on a PR, - * used to avoid re-pinging them on every subsequent push. - */ -export function externalReviewMarkerLabel(area: string): string { - return `external-review:${area}`; -} - /** * Path that should trigger every CI build. */ diff --git a/eng/common/config/labels.ts b/eng/common/config/labels.ts index be05d1c2340..24a8f197ae4 100644 --- a/eng/common/config/labels.ts +++ b/eng/common/config/labels.ts @@ -1,7 +1,7 @@ // cspell:ignore bfff import { defineConfig, defineLabels } from "../scripts/labels/config.js"; import { repo } from "../scripts/utils/common.js"; -import { AreaPaths, ExternalOwners, externalReviewMarkerLabel } from "./area.js"; +import { AreaPaths, ExternalOwners } from "./area.js"; /** * Labels that are used to categorize issue for which area they belong to. @@ -197,19 +197,6 @@ export const CommonLabels = { }, }, }, - "external-review": { - description: - "Markers indicating that the external owners of an area have been notified on a PR", - labels: Object.fromEntries( - Object.keys(ExternalOwners).map((area) => [ - externalReviewMarkerLabel(area), - { - color: "ededed", - description: `The external owners of the '${area}' area have been notified on this PR`, - }, - ]), - ), - }, }; export default defineConfig({ diff --git a/eng/common/scripts/labels/automation.ts b/eng/common/scripts/labels/automation.ts index 6b2274a2476..f2aa0552a20 100644 --- a/eng/common/scripts/labels/automation.ts +++ b/eng/common/scripts/labels/automation.ts @@ -1,11 +1,9 @@ import { resolve } from "path"; import { stringify } from "yaml"; -import { externalReviewMarkerLabel } from "../../config/area.js"; import { CheckOptions, syncFile } from "../utils/common.js"; import { expandFolder } from "../utils/find-area-changed.js"; import { PolicyServiceConfig, - addLabel, addReply, and, eventResponderTask, @@ -168,17 +166,17 @@ function createExternalReviewersConfig(config: RepoConfig): PolicyServiceConfig if: [payloadType("Pull_Request")], then: entries.map(([area, owners]) => { const globs = (config.areaPaths[area] ?? []).map(expandFolder); - const marker = externalReviewMarkerLabel(area); const mentions = owners.map((owner) => `@${owner}`).join(" "); - // Only ping the first time the paths match (guarded by the marker label) so - // owners are not re-notified on every subsequent push. + // The area label (added by the prs.triage policy when these paths are + // modified) persists across events, so it doubles as the "already notified" + // guard: owners are pinged the first time the paths match and not on every + // subsequent push. return { - if: [includesModifiedFiles(globs), not(hasLabel(marker))], + if: [includesModifiedFiles(globs), not(hasLabel(area))], then: [ addReply( `${mentions} — this PR modifies files in the \`${area}\` area, which your team owns. Please take a look. (You can't be added as a formal reviewer because you're not a repository collaborator, so this is a heads-up instead.)`, ), - addLabel(marker), ], }; }), From 705337cf2dec93f6cd627066f331fccc117459d0 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Thu, 16 Jul 2026 13:31:47 -0400 Subject: [PATCH 3/3] Ping fionabronwen and add area label guard for external graphql notification --- .../policies/prs.external-reviewers.generated.yml | 13 ++++++++----- eng/common/config/area.ts | 4 ++-- eng/common/scripts/labels/automation.ts | 10 ++++++---- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/.github/policies/prs.external-reviewers.generated.yml b/.github/policies/prs.external-reviewers.generated.yml index cd1120d67dc..404ceb92720 100644 --- a/.github/policies/prs.external-reviewers.generated.yml +++ b/.github/policies/prs.external-reviewers.generated.yml @@ -1,5 +1,5 @@ # This file is generated by the sync-labels script DO NOT EDIT manually -# cspell:ignore steverice swatkatz +# cspell:ignore fionabronwen steverice swatkatz id: prs.external-reviewers name: Notify external area owners on PRs description: GitHub doesn't allow CODEOWNERS entries or formal review requests @@ -23,7 +23,10 @@ configuration: label: emitter:graphql then: - addReply: - reply: "@steverice @swatkatz — this PR modifies files in the `emitter:graphql` - area, which your team owns. Please take a look. (You can't - be added as a formal reviewer because you're not a - repository collaborator, so this is a heads-up instead.)" + reply: "@fionabronwen @swatkatz @steverice — this PR modifies files in the + `emitter:graphql` area, which your team owns. Please take a + look. (You can't be added as a formal reviewer because + you're not a repository collaborator, so this is a heads-up + instead.)" + - addLabel: + label: emitter:graphql diff --git a/eng/common/config/area.ts b/eng/common/config/area.ts index 08801be9c38..f1c12ec9726 100644 --- a/eng/common/config/area.ts +++ b/eng/common/config/area.ts @@ -42,8 +42,8 @@ export const AreaPaths: Record = { * Keyed by the same area names as {@link AreaPaths}; the paths are reused from there. */ export const ExternalOwners: Partial> = { - // cspell:ignore swatkatz - "emitter:graphql": ["steverice", "swatkatz"], + // cspell:ignore swatkatz fionabronwen + "emitter:graphql": ["fionabronwen", "swatkatz", "steverice"], }; /** diff --git a/eng/common/scripts/labels/automation.ts b/eng/common/scripts/labels/automation.ts index f2aa0552a20..da0d1cac0a4 100644 --- a/eng/common/scripts/labels/automation.ts +++ b/eng/common/scripts/labels/automation.ts @@ -4,6 +4,7 @@ import { CheckOptions, syncFile } from "../utils/common.js"; import { expandFolder } from "../utils/find-area-changed.js"; import { PolicyServiceConfig, + addLabel, addReply, and, eventResponderTask, @@ -167,16 +168,17 @@ function createExternalReviewersConfig(config: RepoConfig): PolicyServiceConfig then: entries.map(([area, owners]) => { const globs = (config.areaPaths[area] ?? []).map(expandFolder); const mentions = owners.map((owner) => `@${owner}`).join(" "); - // The area label (added by the prs.triage policy when these paths are - // modified) persists across events, so it doubles as the "already notified" - // guard: owners are pinged the first time the paths match and not on every - // subsequent push. + // The area label doubles as the "already notified" guard: it is added + // below the first time the paths match so owners are pinged once and not + // on every subsequent push (the prs.triage policy also adds it, but adding + // it here guarantees the guard is set even if that policy hasn't run yet). return { if: [includesModifiedFiles(globs), not(hasLabel(area))], then: [ addReply( `${mentions} — this PR modifies files in the \`${area}\` area, which your team owns. Please take a look. (You can't be added as a formal reviewer because you're not a repository collaborator, so this is a heads-up instead.)`, ), + addLabel(area), ], }; }),