diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index c299af5b..9183030b 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -125,6 +125,20 @@ on: required: false type: string default: '' + fallback_reviewers: + description: >- + Who may answer the gate when `approval_reviewers` is empty and + CODEOWNERS yields nobody — same `team:` / `user:` form. + + Most repositories have a CODEOWNERS without a catch-all `*` rule, so + a repository-wide gate finds no owner to ask. Without a fallback the + Environment cannot be created, the preflight fails, and the first + breaking change in that repository is blocked by a gate nobody can + answer. Empty keeps that behaviour — refusing rather than inventing a + reviewer. + required: false + type: string + default: '' create_approval_environment: description: >- Create `breaking-approval` when it is missing, using `approval_reviewers`. @@ -675,7 +689,14 @@ jobs: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} environment: breaking-approval create-if-missing: ${{ inputs.create_approval_environment && 'true' || 'false' }} - reviewers: ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers || steps.codeowners.outputs.reviewers }} + # Explicit, then CODEOWNERS, then the fallback. The caller's own + # answer wins; the fallback only covers a repository that has no + # catch-all rule to read. + reviewers: >- + ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers + || (steps.codeowners.outputs.reviewers != '' + && steps.codeowners.outputs.reviewers + || inputs.fallback_reviewers) }} # Out of scope, so no approval is coming — but the acknowledgement is # enforced on every pull request, and an author who omits it otherwise @@ -722,7 +743,14 @@ jobs: subjects: ${{ needs.detect.outputs.subjects }} author-acknowledgement: ${{ inputs.author_acknowledgement }} fingerprint: ${{ needs.detect.outputs.fingerprint }} - reviewers: ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers || steps.codeowners.outputs.reviewers }} + # Explicit, then CODEOWNERS, then the fallback. The caller's own + # answer wins; the fallback only covers a repository that has no + # catch-all rule to read. + reviewers: >- + ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers + || (steps.codeowners.outputs.reviewers != '' + && steps.codeowners.outputs.reviewers + || inputs.fallback_reviewers) }} actual-reviewers: ${{ steps.environment.outputs.reviewers }} reason: ${{ needs.detect.outputs.reason }} head-sha: ${{ needs.detect.outputs.head-sha }} diff --git a/.github/workflows/go-pr-validation.yml b/.github/workflows/go-pr-validation.yml index eb56a31d..39bf3da7 100644 --- a/.github/workflows/go-pr-validation.yml +++ b/.github/workflows/go-pr-validation.yml @@ -179,6 +179,15 @@ on: description: 'Close the pull request when a reviewer rejects the breaking change.' type: boolean default: true + breaking_change_fallback_reviewers: + description: >- + Who answers the breaking-change gate in a repository whose CODEOWNERS + has no catch-all `*` rule. Defaults to the + `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation variable — see + docs/breaking-change-approval.md. + required: false + type: string + default: '' breaking_change_approval_reviewers: description: 'Who may answer the breaking-change gate, as comma-separated `team:` or `user:`. Empty takes the repository own CODEOWNERS. Used only to create the `breaking-approval` Environment when it does not exist.' type: string @@ -472,6 +481,7 @@ jobs: breaking_change_skip_labels: ${{ inputs.breaking_change_skip_labels }} breaking_change_close_on_rejection: ${{ inputs.breaking_change_close_on_rejection }} breaking_change_approval_reviewers: ${{ inputs.breaking_change_approval_reviewers }} + breaking_change_fallback_reviewers: ${{ inputs.breaking_change_fallback_reviewers }} breaking_change_create_environment: ${{ inputs.breaking_change_create_environment }} breaking_change_require_protected_environment: ${{ inputs.breaking_change_require_protected_environment }} secrets: diff --git a/.github/workflows/js-pr-validation.yml b/.github/workflows/js-pr-validation.yml index 90d8f2a4..533ccd95 100644 --- a/.github/workflows/js-pr-validation.yml +++ b/.github/workflows/js-pr-validation.yml @@ -187,6 +187,15 @@ on: description: 'Close the pull request when a reviewer rejects the breaking change.' type: boolean default: true + breaking_change_fallback_reviewers: + description: >- + Who answers the breaking-change gate in a repository whose CODEOWNERS + has no catch-all `*` rule. Defaults to the + `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation variable — see + docs/breaking-change-approval.md. + required: false + type: string + default: '' breaking_change_approval_reviewers: description: 'Who may answer the breaking-change gate, as comma-separated `team:` or `user:`. Empty takes the repository own CODEOWNERS. Used only to create the `breaking-approval` Environment when it does not exist.' type: string @@ -592,6 +601,7 @@ jobs: breaking_change_skip_labels: ${{ inputs.breaking_change_skip_labels }} breaking_change_close_on_rejection: ${{ inputs.breaking_change_close_on_rejection }} breaking_change_approval_reviewers: ${{ inputs.breaking_change_approval_reviewers }} + breaking_change_fallback_reviewers: ${{ inputs.breaking_change_fallback_reviewers }} breaking_change_create_environment: ${{ inputs.breaking_change_create_environment }} breaking_change_require_protected_environment: ${{ inputs.breaking_change_require_protected_environment }} secrets: diff --git a/.github/workflows/pr-validation.yml b/.github/workflows/pr-validation.yml index 789f0488..0e973b29 100644 --- a/.github/workflows/pr-validation.yml +++ b/.github/workflows/pr-validation.yml @@ -158,6 +158,19 @@ on: description: 'Close the pull request when a reviewer rejects the breaking change.' type: boolean default: true + breaking_change_fallback_reviewers: + description: >- + Who answers the breaking-change gate in a repository whose CODEOWNERS + has no catch-all `*` rule — the common case, which otherwise leaves + the Environment uncreatable and the first breaking change blocked by + a gate nobody can answer. + + Defaults to the `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation + variable, so it is set once for the organisation rather than per + repository. Same `team:` / `user:` form. + required: false + type: string + default: '' breaking_change_approval_reviewers: description: >- Who may answer the breaking-change gate, as comma-separated `team:` @@ -326,6 +339,9 @@ jobs: skip_labels: ${{ inputs.breaking_change_skip_labels }} close_on_rejection: ${{ inputs.breaking_change_close_on_rejection }} approval_reviewers: ${{ inputs.breaking_change_approval_reviewers }} + # The caller's value wins; the organisation variable is the default, so + # a repository that never heard of this gate still has someone to ask. + fallback_reviewers: ${{ inputs.breaking_change_fallback_reviewers != '' && inputs.breaking_change_fallback_reviewers || vars.BREAKING_APPROVAL_FALLBACK_REVIEWERS }} create_approval_environment: ${{ inputs.breaking_change_create_environment }} require_protected_environment: ${{ inputs.breaking_change_require_protected_environment }} dry_run: ${{ inputs.dry_run }} diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 77e81466..1f2d2cb6 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -84,6 +84,40 @@ guard job already ran the same composite over the same commits, so a second full-history checkout would buy nothing. The acknowledgement state is shown to the reviewer as context. +## Who answers the gate + +Three sources, in order. The first that yields anybody wins: + +1. **`approval_reviewers`** — the caller named them explicitly. +2. **CODEOWNERS**, read from the pull request's **base** ref. Reading the head + would let an author edit who is allowed to approve their own change. +3. **`fallback_reviewers`** — through `pr-validation`, the + `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation variable. + +The third exists because of what the second actually finds. +`codeowners-reviewers` reads the **catch-all `*` rule**, which is the right +question for a repository-wide gate — but most CODEOWNERS files do not have +one. Measured across the consuming repositories before this was added: four of +five carried rules only for specific paths (`/.github/`, `/.releaserc.yml`), +and no `*`. + +In those four the gate resolved no reviewer at all. An Environment cannot be +created without one — `environment-protection` refuses, because an Environment +created without required reviewers is a gate that approves itself — so the +preflight failed, and the first breaking change in each of those repositories +would have been blocked by a gate nobody could answer. + +Set the organisation variable once and that stops being a per-repository +problem: + +``` +BREAKING_APPROVAL_FALLBACK_REVIEWERS = team:your-devops-team +``` + +Leaving it unset keeps the old behaviour: no reviewer, no Environment, and a +refusal rather than an invented approver. That is deliberate — a gate that +quietly picks someone to ask is worse than one that says it cannot run. + ## Setup ### The Environment creates itself, owned by CODEOWNERS @@ -458,6 +492,7 @@ Declare them with [`labels-sync`](labels-sync.md), or pass `label: ''` / | `author_acknowledgement` | string | the `pr-validation` line | Shown to the reviewer as context. Never substitutes for the approval. | | `close_on_rejection` | boolean | `true` | Close the pull request when a reviewer rejects. | | `approval_reviewers` | string | *(empty)* | Who may answer the gate, as comma-separated `team:` or `user:`. Empty takes the repository's CODEOWNERS. Used only to create the Environment when it does not exist. | +| `fallback_reviewers` | string | *(empty)* | Who answers when `approval_reviewers` is empty **and** CODEOWNERS yields nobody. Through `pr-validation` this defaults to the `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation variable. Empty refuses rather than inventing a reviewer. | | `create_approval_environment` | boolean | `true` | Create `breaking-approval` when missing. Needs a token with repository administration rights. | | `require_protected_environment` | boolean | `true` | Refuse to gate when `breaking-approval` exists without required reviewers. Leave on. | | `label` | string | `breaking-change` | States that the change is breaking. A fact, so it survives the decision and comes off only when the breaking commits are gone. Cosmetic. | diff --git a/docs/go-pr-validation.md b/docs/go-pr-validation.md index bd1df4ba..1bd344d8 100644 --- a/docs/go-pr-validation.md +++ b/docs/go-pr-validation.md @@ -71,6 +71,7 @@ This distinguishes the two skips that used to look identical: a pipeline the cal | `breaking_change_close_on_rejection` | Close the PR when a reviewer rejects the breaking change | boolean | `true` | | `breaking_change_require_protected_environment` | Refuse to gate when `breaking-approval` has no required reviewers | boolean | `true` | | `breaking_change_approval_reviewers` | Who may answer the gate, as `team:` or `user:`. Empty takes the repository's CODEOWNERS | string | *(empty)* | +| `breaking_change_fallback_reviewers` | Who answers when the above is empty **and** CODEOWNERS yields nobody. Defaults to the `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation variable | string | *(empty)* | | `breaking_change_create_environment` | Create `breaking-approval` when missing. Needs a `MANAGE_TOKEN` with administration rights | boolean | `true` | | `go_version` | Go version | string | `1.23` | | `golangci_lint_version` | GolangCI-Lint version | string | `v1.62.2` | diff --git a/docs/js-pr-validation.md b/docs/js-pr-validation.md index f21bb6f3..7a24e401 100644 --- a/docs/js-pr-validation.md +++ b/docs/js-pr-validation.md @@ -61,6 +61,7 @@ The `Go Analysis` and `Lib Version` stubs are unaffected: they are not flag-guar | `breaking_change_close_on_rejection` | Close the PR when a reviewer rejects the breaking change | boolean | `true` | | `breaking_change_require_protected_environment` | Refuse to gate when `breaking-approval` has no required reviewers | boolean | `true` | | `breaking_change_approval_reviewers` | Who may answer the gate, as `team:` or `user:`. Empty takes the repository's CODEOWNERS | string | *(empty)* | +| `breaking_change_fallback_reviewers` | Who answers when the above is empty **and** CODEOWNERS yields nobody. Defaults to the `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation variable | string | *(empty)* | | `breaking_change_create_environment` | Create `breaking-approval` when missing. Needs a `MANAGE_TOKEN` with administration rights | boolean | `true` | | `node_version` | Node.js version | string | `22` | | `package_manager` | Package manager (`npm`, `yarn`, `pnpm`) | string | `npm` | diff --git a/docs/pr-validation.md b/docs/pr-validation.md index ffcd66c1..d7fcdf75 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -182,6 +182,7 @@ jobs: | `breaking_change_close_on_rejection` | boolean | `true` | Close the PR when a reviewer rejects the breaking change | | `breaking_change_require_protected_environment` | boolean | `true` | Refuse to gate when `breaking-approval` has no required reviewers, instead of silently passing | | `breaking_change_approval_reviewers` | string | *(empty)* | Who may answer the gate, as comma-separated `team:` or `user:`. Empty takes the repository's CODEOWNERS | +| `breaking_change_fallback_reviewers` | string | *(empty)* | Who answers when the above is empty **and** CODEOWNERS yields nobody — the common case, since most CODEOWNERS have no catch-all `*` rule. Defaults to the `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation variable | | `breaking_change_create_environment` | boolean | `true` | Create `breaking-approval` when missing. Needs a `MANAGE_TOKEN` with repository administration rights | The breaking change guard has no enable input, target-branch input, acknowledgement input, or opt-out. It applies to every caller and every PR target branch. `dry_run: true` remains a global preview mode without guard enforcement; it is not a guard-specific opt-out and does not change normal `dry_run: false` operation. Existing callers must migrate their `pull_request.types` list to include both `edited` and `ready_for_review`; otherwise body edits and draft-to-ready transitions do not rerun validation. diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index b61c0af2..1cb0b6fc 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -74,6 +74,7 @@ "breaking_change_close_on_rejection", "breaking_change_require_protected_environment", "breaking_change_approval_reviewers", + "breaking_change_fallback_reviewers", "breaking_change_create_environment", } @@ -1259,16 +1260,66 @@ def test_reviewers_come_from_codeowners_on_the_base_ref(self): # branch would let the author choose who approves their own change. self.assertIn("ref: ${{ needs.detect.outputs.base-ref }}", step) self.assertNotIn("head", step) - # Explicit reviewers win; CODEOWNERS is the fallback. - preflight = extract_step( - extract_job(APPROVAL_WORKFLOW, "request"), - "Verify the breaking-approval Environment is protected", + # Explicit reviewers win, then CODEOWNERS, then the fallback. Order + # matters: a caller that named its reviewers must not be overridden by + # a repository-wide default, and the default must not be reached while + # CODEOWNERS still has an answer. + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + preflight = next( + s + for s in workflow["jobs"]["request"]["steps"] + if s.get("name") == "Verify the breaking-approval Environment is protected" ) - self.assertIn( - "inputs.approval_reviewers != '' && inputs.approval_reviewers" - " || steps.codeowners.outputs.reviewers", - preflight, + expression = " ".join(preflight["with"]["reviewers"].split()) + self.assertEqual( + "${{ inputs.approval_reviewers != '' && inputs.approval_reviewers" + " || (steps.codeowners.outputs.reviewers != ''" + " && steps.codeowners.outputs.reviewers" + " || inputs.fallback_reviewers) }}", + expression, + ) + + def test_a_repository_without_a_catch_all_rule_still_has_a_reviewer(self): + # Measured across the consumers before adding this: four of five + # CODEOWNERS files carry no `*` rule, and `codeowners-reviewers` reads + # the catch-all by design — a repository-wide gate should ask the + # repository-wide owner. So those four resolved nobody, the Environment + # could not be created, and the first breaking change in each would + # have been blocked by a gate with no one able to answer it. + gate = yaml.safe_load(APPROVAL_WORKFLOW) + self.assertIn("fallback_reviewers", gate[True]["workflow_call"]["inputs"]) + # Empty by default: refusing is still better than inventing a reviewer. + self.assertEqual( + "", gate[True]["workflow_call"]["inputs"]["fallback_reviewers"]["default"] + ) + + # The umbrellas expose it, and `pr-validation` sources the default from + # an organisation variable rather than naming a team in the code. + validation = yaml.safe_load( + (REPO_ROOT / ".github" / "workflows" / "pr-validation.yml").read_text( + encoding="utf-8" + ) ) + forwarded = validation["jobs"]["breaking-change-approval"]["with"][ + "fallback_reviewers" + ] + self.assertIn("vars.BREAKING_APPROVAL_FALLBACK_REVIEWERS", forwarded) + self.assertIn("inputs.breaking_change_fallback_reviewers", forwarded) + + for name in ("go-pr-validation", "js-pr-validation"): + umbrella = yaml.safe_load( + (REPO_ROOT / ".github" / "workflows" / f"{name}.yml").read_text( + encoding="utf-8" + ) + ) + self.assertIn( + "breaking_change_fallback_reviewers", + umbrella[True]["workflow_call"]["inputs"], + name, + ) + + # No team name in the code — the organisation variable is the source. + self.assertNotIn("G_Github_Devops", APPROVAL_WORKFLOW) def test_the_waiting_comment_links_instead_of_mentioning(self): rendered = render_reporter("waiting")