From a977381d4e2c0799036cfe25a765dd7ed1a7d400 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:23:03 -0300 Subject: [PATCH 1/3] feat(pr-validation): publish the gate verdict under one stable check name A reusable workflow's check is reported as ` / ... / `, and the caller's job name lives in the consumer's own repository. Measured across the 68 repositories consuming the umbrellas, the same gate produces six distinct names: 48 validate / PR Metadata / Breaking Change Approval / Breaking Change Approval 18 validate / Breaking Change Approval / Breaking Change Approval 5 ui / PR Metadata / ... 1 validate_mcp / PR Metadata / ... 1 Validate PR / ... 1 Metadata Validation / ... Required status checks match the context exactly, with no wildcard, so no single organisation ruleset can require the gate - and a required check that never arrives leaves the pull request pending forever, so listing all six would block every repository that produces only one. A commit status carries no job path. It is the reason `CodeRabbit` and `Socket Security` read identically everywhere, and it removes the caller from the name: set `BREAKING_APPROVAL_STATUS_CONTEXT` once and one rule covers every repository. Published on every run, including the ones with nothing to approve - out of scope, no breaking commits, superseded all publish success. A status that appears only sometimes cannot be required. `result` decides pass or fail; the state only chooses the wording, and the two are not allowed to disagree. `statuses: write` is deliberately not declared on the job: a called workflow may only ask for permissions the caller already holds, so declaring it would break every consumer that does not grant it - the same reasoning the file already applies to `actions: read`. It goes through MANAGE_TOKEN, and warns rather than failing when it cannot be written. Co-Authored-By: Claude Opus 5 --- .../workflows/breaking-change-approval.yml | 116 ++++++++++++++++- .github/workflows/go-pr-validation.yml | 9 ++ .github/workflows/js-pr-validation.yml | 9 ++ .github/workflows/pr-validation.yml | 14 ++ docs/breaking-change-approval.md | 42 ++++++ docs/go-pr-validation.md | 1 + docs/js-pr-validation.md | 1 + docs/pr-validation.md | 1 + .../breaking-change-guard/test-workflow.py | 123 ++++++++++++++++++ 9 files changed, 311 insertions(+), 5 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 4715d715..db9a83d2 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -192,6 +192,22 @@ on: required: false type: boolean default: true + status_context: + description: >- + Name of a commit status published with the gate's verdict. Fixed + across repositories, which a job name cannot be: a reusable + workflow's check surfaces as ` / … / `, and the + caller's job name lives in the consumer's own file — so the same gate + appears under several different names across an organisation and no + single ruleset can require it. + + Set it and the gate publishes that status on every run, so one + required status check covers every repository. Needs a token with + `statuses: write` — MANAGE_TOKEN, or the caller granting it. Empty + publishes nothing. + required: false + type: string + default: '' enable_comment: description: Post (and keep updated) a single comment carrying the request, the approval link and the outcome. required: false @@ -814,11 +830,13 @@ jobs: runs-on: ${{ vars.GENERAL_RUNNERS || inputs.runner_type }} needs: [detect, request, approval] if: always() - # Deliberately NOT declaring `actions: read`. A called workflow may only ask - # for permissions the caller already holds, so declaring it would break every - # consumer whose entrypoint does not grant it. The approvals API is read - # through MANAGE_TOKEN when present, and degrades to the job conclusion when - # it is not — see the fallback in the step. + # Deliberately NOT declaring `actions: read`, and for the same reason not + # `statuses: write`. A called workflow may only ask for permissions the + # caller already holds, so declaring either would break every consumer whose + # entrypoint does not grant it. Both are reached through MANAGE_TOKEN + # instead, and both degrade rather than fail — the approvals API falls back + # to the job conclusion, and the commit status warns loudly if it cannot be + # published. permissions: pull-requests: write outputs: @@ -1131,6 +1149,94 @@ jobs: echo "::warning::PR #${PR_NUMBER} could not be closed (token lacks 'pull-requests: write'?) — the gate still failed" fi + # One check name, the same in every repository. A reusable workflow's job + # surfaces as ` / … / Breaking Change Approval`, and the + # caller's job name is in the consumer's own file — measured across the + # organisation, the same gate produces six different names, so no single + # organisation ruleset can require it. A commit status carries no job + # path, which is why CodeRabbit and Socket look identical everywhere. + # + # Published on every run, including the ones with nothing to approve: a + # required status check that never arrives leaves the pull request + # pending forever, which is worse than not having the rule. + - name: Publish the gate status + if: always() && inputs.status_context != '' + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.MANAGE_TOKEN || github.token }} + REPO: ${{ github.repository }} + CONTEXT: ${{ inputs.status_context }} + HEAD_SHA: ${{ needs.detect.outputs.head-sha }} + STATE: ${{ steps.verdict.outputs.state }} + RESULT: ${{ steps.verdict.outputs.result }} + REASON: ${{ needs.detect.outputs.reason }} + APPROVERS: ${{ steps.decisions.outputs.approvers || needs.request.outputs.approved-by }} + REJECTERS: ${{ steps.verdict.outputs.rejecters }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + DRY_RUN: ${{ inputs.dry_run && 'true' || 'false' }} + run: | + set -euo pipefail + + if [ -z "${HEAD_SHA:-}" ]; then + echo "::warning::No head SHA resolved, so the '${CONTEXT}' status cannot be published. A required check will stay pending." + exit 0 + fi + + # `pending` is never published. The status exists so a branch rule can + # read it, and a rule reading `pending` blocks — this job runs once + # the verdict is in, so it always has a final answer to give. + case "$STATE" in + approved) + status=success + description="Approved by ${APPROVERS:-a reviewer}" + ;; + rejected) + status=failure + description="Rejected by ${REJECTERS:-a reviewer}" + ;; + none) + status=success + description="No approval needed — ${REASON}" + ;; + superseded) + status=success + description="Superseded by a newer run" + ;; + blocked) + status=failure + description="The gate could not run — see the run log" + ;; + would-request) + status=success + description="Dry run — would hold for approval" + ;; + *) + status=failure + description="Awaiting a decision on the breaking change" + ;; + esac + + # `result` is the authority on pass or fail; the state only chooses + # the wording. Letting the two disagree is how a status says success + # over a verdict that failed. + [ "$RESULT" = "success" ] || [ "$status" = "failure" ] || status=failure + + if [ "$DRY_RUN" = "true" ]; then + echo "::notice::DRY RUN — would publish status '${CONTEXT}' = ${status} (${description})" + exit 0 + fi + + if ! gh api --method POST "repos/${REPO}/statuses/${HEAD_SHA}" \ + -f state="$status" \ + -f context="$CONTEXT" \ + -f description="${description:0:140}" \ + -f target_url="$RUN_URL" >/dev/null 2>&1; then + echo "::warning::Could not publish the '${CONTEXT}' status. It needs a token with \`statuses: write\` — pass MANAGE_TOKEN, or grant it on the caller's job. A branch rule requiring this status will stay pending until it can be written." >&2 + exit 0 + fi + + echo "::notice::Status '${CONTEXT}' = ${status} — ${description}" + - name: Record the verdict if: always() env: diff --git a/.github/workflows/go-pr-validation.yml b/.github/workflows/go-pr-validation.yml index cf6c133a..8d7ac29d 100644 --- a/.github/workflows/go-pr-validation.yml +++ b/.github/workflows/go-pr-validation.yml @@ -179,6 +179,14 @@ on: description: 'Close the pull request when a reviewer rejects the breaking change.' type: boolean default: true + breaking_change_status_context: + description: >- + Publish the gate's verdict as a commit status under this name, so one + organisation-wide required check can cover every repository. Defaults + to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation variable. + required: false + type: string + default: '' breaking_change_fallback_reviewers: description: >- Who answers the breaking-change gate in a repository whose CODEOWNERS @@ -493,6 +501,7 @@ jobs: 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_status_context: ${{ inputs.breaking_change_status_context }} 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 533ccd95..f77a058a 100644 --- a/.github/workflows/js-pr-validation.yml +++ b/.github/workflows/js-pr-validation.yml @@ -187,6 +187,14 @@ on: description: 'Close the pull request when a reviewer rejects the breaking change.' type: boolean default: true + breaking_change_status_context: + description: >- + Publish the gate's verdict as a commit status under this name, so one + organisation-wide required check can cover every repository. Defaults + to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation variable. + required: false + type: string + default: '' breaking_change_fallback_reviewers: description: >- Who answers the breaking-change gate in a repository whose CODEOWNERS @@ -602,6 +610,7 @@ jobs: 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_status_context: ${{ inputs.breaking_change_status_context }} 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 0e973b29..a559ab0a 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_status_context: + description: >- + Publish the gate's verdict as a commit status under this name. A + reusable workflow's check carries the caller's job path, so the same + gate appears under a different name in each repository and an + organisation ruleset cannot require it. A commit status does not, + so one name works everywhere. + + Defaults to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation + variable. Empty publishes nothing. + required: false + type: string + default: '' breaking_change_fallback_reviewers: description: >- Who answers the breaking-change gate in a repository whose CODEOWNERS @@ -342,6 +355,7 @@ jobs: # 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 }} + status_context: ${{ inputs.breaking_change_status_context != '' && inputs.breaking_change_status_context || vars.BREAKING_APPROVAL_STATUS_CONTEXT }} 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 1f2d2cb6..b482a8cb 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -498,6 +498,7 @@ Declare them with [`labels-sync`](labels-sync.md), or pass `label: ''` / | `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. | | `pending_label` | string | `awaiting-breaking-approval` | States that a decision is pending. A state, so it comes off once approved or rejected. Cosmetic. | | `enable_approval` | boolean | `true` | Hold breaking changes at the Environment. `false` keeps the gate reporting and labelling but never asks for a decision — the author acknowledgement is a separate control the caller still enforces, and the comment carrying the line to paste comes from here. Not the same as not calling the workflow. | +| `status_context` | string | *(empty)* | Publish the verdict as a commit status under this name — the one check name that is identical in every repository. Through `pr-validation` this defaults to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation variable. Needs `statuses: write`. Empty publishes nothing. | | `enable_comment` | boolean | `true` | Post and keep the comment updated. | | `dry_run` | boolean | `false` | Report what would be gated without commenting, labelling, requesting an approval, or failing. | | `runner_type` | string | `blacksmith-4vcpu-ubuntu-2404` | Runner label. Overridden by `vars.GENERAL_RUNNERS`. | @@ -575,6 +576,47 @@ if: >- Nothing supplied is still `false` — the gate does not scan out of scope, so there is no answer to report. +## Requiring the gate in a branch rule + +The gate's job surfaces as a check named after the **caller's** job path — +`validate / PR Metadata / Breaking Change Approval / Breaking Change Approval` +in one repository, `validation / Breaking Change Approval / Breaking Change +Approval` in another. The prefix comes from the consumer's own workflow file, +so the same gate appears under several different names across an organisation. +Measured across the repositories consuming the umbrellas: **six distinct +names**. Required status checks match the context exactly, with no wildcard, so +no single organisation ruleset can require it — and a required check that never +arrives leaves every pull request pending forever. + +`status_context` removes the caller from the name. The gate publishes a commit +status, which carries no job path, so the same string works everywhere — the +reason `CodeRabbit` and `Socket Security` look identical in every repository. + +``` +BREAKING_APPROVAL_STATUS_CONTEXT = breaking-change-approval +``` + +Then one organisation ruleset requires `breaking-change-approval` and covers +every repository, whatever its jobs are called. + +Three things worth knowing: + +- **It is published on every run**, including those with nothing to approve. + Out of scope, no breaking commits, superseded — all publish `success`. A + status that only appears sometimes cannot be required. +- **`result` decides pass or fail**, not the state. The state only chooses the + wording, and the two are not allowed to disagree. +- **It needs `statuses: write`.** The job does not declare that permission, and + deliberately: a called workflow may only ask for permissions the caller + already holds, so declaring it would break every consumer that does not grant + it. It goes through `MANAGE_TOKEN` instead. Without a token that can write it + the step warns and carries on rather than failing — but a rule requiring the + status will stay pending, so check the warning before turning the rule on. + +Requiring the job name instead still works for a single repository, and this +repository does exactly that. Prefer the status wherever one rule has to cover +more than one repository. + ## Secrets | Secret | Required | Description | diff --git a/docs/go-pr-validation.md b/docs/go-pr-validation.md index acd4563d..94ddafcf 100644 --- a/docs/go-pr-validation.md +++ b/docs/go-pr-validation.md @@ -72,6 +72,7 @@ This distinguishes the two skips that used to look identical: a pipeline the cal | `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_status_context` | Publish the gate verdict as a commit status under this name, so one organisation-wide required check covers every repository. Defaults to the `BREAKING_APPROVAL_STATUS_CONTEXT` 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 7a24e401..41cb820e 100644 --- a/docs/js-pr-validation.md +++ b/docs/js-pr-validation.md @@ -62,6 +62,7 @@ The `Go Analysis` and `Lib Version` stubs are unaffected: they are not flag-guar | `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_status_context` | Publish the gate verdict as a commit status under this name, so one organisation-wide required check covers every repository. Defaults to the `BREAKING_APPROVAL_STATUS_CONTEXT` 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 d7fcdf75..e8cff308 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -183,6 +183,7 @@ jobs: | `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_status_context` | string | *(empty)* | Publish the gate verdict as a commit status under this name, so one organisation-wide required check covers every repository. Defaults to the `BREAKING_APPROVAL_STATUS_CONTEXT` 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 f770c507..16b45cac 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -75,6 +75,7 @@ "breaking_change_require_protected_environment", "breaking_change_approval_reviewers", "breaking_change_fallback_reviewers", + "breaking_change_status_context", "breaking_change_create_environment", } @@ -448,6 +449,58 @@ def run_detect_normalization(**overrides): return outputs +def run_gate_status(**overrides): + """Run the step that publishes the commit status, and report the call. + + Returns (process, calls). The status is the one check name a branch rule + can require across every repository, so what it publishes per state is the + contract — not something to read off the source. + """ + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + step = next( + s + for s in workflow["jobs"]["outcome"]["steps"] + if s.get("name") == "Publish the gate status" + ) + + env = { + "REPO": "LerianStudio/example", + "CONTEXT": "breaking-change-approval", + "HEAD_SHA": "abc1234567", + "STATE": "approved", + "RESULT": "success", + "REASON": "rule develop <- '*'", + "APPROVERS": "carol", + "REJECTERS": "", + "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", + "DRY_RUN": "false", + "GH_TOKEN": "stub", + } + env.update(overrides) + + with tempfile.TemporaryDirectory() as temp_dir: + calls = Path(temp_dir) / "calls" + calls.touch() + stub = Path(temp_dir) / "gh" + stub.write_text( + "#!/usr/bin/env bash\n" + f'printf \'%s\\n\' "$*" >> "{calls}"\n' + 'if [ -n "${STUB_FAILS:-}" ]; then exit 1; fi\n', + encoding="utf-8", + ) + stub.chmod(0o755) + env["PATH"] = f"{temp_dir}:{os.environ.get('PATH', '/usr/bin:/bin')}" + result = subprocess.run( + ["bash", "-c", step["run"]], + cwd=temp_dir, + env=env, + text=True, + capture_output=True, + check=False, + ) + return result, calls.read_text(encoding="utf-8").splitlines() + + def run_reporter_comment( state, existing_body=None, read_fails=False, legacy_ids="", **overrides ): @@ -2268,6 +2321,76 @@ def test_no_instruction_is_better_than_an_empty_one(self): # The break is still announced — only the unusable instruction is gone. self.assertIn("acknowledgement missing", rendered) + def test_the_gate_publishes_one_status_name_everywhere(self): + # A reusable workflow's check is reported as ` / … / `, + # and the caller's job name lives in the consumer's repository. Measured + # across the organisation, the same gate produces six different names, + # so no single ruleset can require it. A commit status carries no job + # path — which is why CodeRabbit and Socket look identical everywhere. + def published(**overrides): + result, calls = run_gate_status(**overrides) + self.assertEqual(0, result.returncode, result.stderr) + self.assertEqual(1, len(calls), calls) + return calls[0] + + # Pass and fail, by state. + for state, extra, expected in ( + ("approved", {}, "success"), + ("none", {}, "success"), + ("superseded", {}, "success"), + ("rejected", {"RESULT": "failure"}, "failure"), + ("blocked", {"RESULT": "failure"}, "failure"), + ("unresolved", {"RESULT": "failure"}, "failure"), + ): + with self.subTest(state): + call = published(STATE=state, **extra) + self.assertIn(f"state={expected}", call.replace("state ", "state=")) + self.assertIn("statuses/abc1234567", call) + self.assertIn("context=breaking-change-approval", call.replace("context ", "context=")) + + # `result` is the authority: a state that reads as success cannot + # publish success over a verdict that failed. + contradictory = published(STATE="none", RESULT="failure") + self.assertIn("failure", contradictory) + + # Published on every run, including the ones with nothing to approve — + # a required check that never arrives leaves the pull request pending + # forever, which is worse than having no rule. + self.assertIn("statuses/", published(STATE="none")) + + def test_the_status_is_opt_in_and_degrades_instead_of_failing(self): + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + step = next( + s + for s in workflow["jobs"]["outcome"]["steps"] + if s.get("name") == "Publish the gate status" + ) + # Off by default: a repository that never asked for it is untouched. + self.assertIn("inputs.status_context != ''", step["if"]) + self.assertEqual( + "", yaml.safe_load(APPROVAL_WORKFLOW)[True]["workflow_call"]["inputs"][ + "status_context" + ]["default"] + ) + + # `statuses: write` is NOT declared on the job. A called workflow may + # only ask for permissions the caller already holds, so declaring it + # would break every consumer whose entrypoint does not grant it — the + # same reasoning the file already applies to `actions: read`. + self.assertNotIn("statuses", workflow["jobs"]["outcome"]["permissions"]) + + # So a token that cannot write it warns and carries on, rather than + # failing the job over a check the caller may not even require. + result, calls = run_gate_status(STUB_FAILS="1") + self.assertEqual(0, result.returncode) + self.assertIn("statuses: write", result.stdout + result.stderr) + + # And a dry run says what it would publish without publishing it. + preview, dry_calls = run_gate_status(DRY_RUN="true") + self.assertEqual(0, preview.returncode) + self.assertEqual([], dry_calls) + self.assertIn("would publish status", preview.stdout) + def test_a_blocked_gate_names_the_half_that_failed(self): # Measured on a real repository: ungoliant-controller's first breaking # change hit this, and the comment said only "did not reach a verdict" From 1726f426416a48f010269491303fc8da0c4ca1e8 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:01:14 -0300 Subject: [PATCH 2/3] fix(pr-validation): hold the gate status pending, and never publish a dead one Three findings from CodeRabbit on #949, all valid. **A superseded run published failure over its replacement.** The verdict emits `state=superseded` with `result=failure` - confirmed by running it - so the authority line turned the intended success into a failure and published it. GitHub reads the latest status for a context, and a superseded run can finish after the one that replaced it, so this would block a pull request on a decision already given. Superseded now publishes nothing, the same rule the comment and the close already follow. My test could not see it: it passed the helper's default `result=success` for every state, which is not what the verdict emits for this one. Each state now carries its real result. **No pending status before the wait.** The previous run's success stayed current while this run sat at the Environment, so a rule requiring the status would allow the merge on a decision nobody had given. The head SHA does not have to change for that - `edited` re-runs on the same commit, and retargeting a pull request moves it in or out of scope. The status now goes pending in `request`, before `approval` can wait, and only when this run will actually hold. **The warning offered a remedy that does nothing.** It suggested granting `statuses: write` on the caller's job. The job declares its own `permissions:`, which replaces what the caller granted, so only MANAGE_TOKEN can write it. Corrected in the warning, the input description and the guide. Co-Authored-By: Claude Opus 5 --- .../workflows/breaking-change-approval.yml | 72 ++++++++++++++++-- docs/breaking-change-approval.md | 34 ++++++--- .../breaking-change-guard/test-workflow.py | 76 ++++++++++++++++++- 3 files changed, 161 insertions(+), 21 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index db9a83d2..93db1e17 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -202,9 +202,9 @@ on: single ruleset can require it. Set it and the gate publishes that status on every run, so one - required status check covers every repository. Needs a token with - `statuses: write` — MANAGE_TOKEN, or the caller granting it. Empty - publishes nothing. + required status check covers every repository. Needs MANAGE_TOKEN + carrying `statuses: write`: this job declares its own `permissions:`, + so granting it on the caller has no effect. Empty publishes nothing. required: false type: string default: '' @@ -740,6 +740,50 @@ jobs: # run announces "needs a decision", adds the pending label, and then the # verdict takes both back seconds later — a flicker in the timeline for a # decision that was never in question. + # The status goes pending before the wait, and only when there will be + # one. GitHub reads the latest status for a context, so without this the + # previous run's `success` stays current while this run sits at the + # Environment — and a rule requiring it would let the merge through on a + # decision nobody has given yet. + # + # The head SHA does not have to change for that to happen: `edited` + # re-runs on the same commit, and changing a pull request's base moves it + # in or out of scope. A pull request that was out of scope published + # `success`, and retargeting it to `develop` must not keep it. + # + # Only when this run will actually hold. Publishing pending on every run + # would leave the status pending wherever the gate has nothing to ask. + - name: Hold the gate status while the decision is pending + if: >- + inputs.status_context != '' + && needs.detect.result == 'success' + && needs.detect.outputs.in-scope == 'true' + && needs.detect.outputs.has-breaking-changes == 'true' + && steps.reuse.outputs.already-approved != 'true' + && !inputs.dry_run + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.MANAGE_TOKEN || github.token }} + REPO: ${{ github.repository }} + CONTEXT: ${{ inputs.status_context }} + HEAD_SHA: ${{ needs.detect.outputs.head-sha }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + set -euo pipefail + + [ -n "${HEAD_SHA:-}" ] || exit 0 + + if ! gh api --method POST "repos/${REPO}/statuses/${HEAD_SHA}" \ + -f state=pending \ + -f context="$CONTEXT" \ + -f description="Waiting for a decision on the breaking change" \ + -f target_url="$RUN_URL" >/dev/null 2>&1; then + echo "::warning::Could not hold the '${CONTEXT}' status pending. It needs MANAGE_TOKEN carrying \`statuses: write\`." >&2 + exit 0 + fi + + echo "::notice::Status '${CONTEXT}' = pending — waiting for a decision" + - name: Request the approval on the pull request if: >- needs.detect.result == 'success' @@ -1185,6 +1229,17 @@ jobs: # `pending` is never published. The status exists so a branch rule can # read it, and a rule reading `pending` blocks — this job runs once # the verdict is in, so it always has a final answer to give. + # A superseded run must not touch the pull request, and the status is + # the pull request. Its verdict is about a revision that is no longer + # current, and it carries `result=failure` — so publishing it would + # overwrite the live run's success with a failure and block the pull + # request on a decision that was already made. The same rule the + # comment and the close already follow. + if [ "$STATE" = "superseded" ]; then + echo "::notice::Superseded — leaving the '${CONTEXT}' status to the run that replaced this one" + exit 0 + fi + case "$STATE" in approved) status=success @@ -1198,10 +1253,6 @@ jobs: status=success description="No approval needed — ${REASON}" ;; - superseded) - status=success - description="Superseded by a newer run" - ;; blocked) status=failure description="The gate could not run — see the run log" @@ -1231,7 +1282,12 @@ jobs: -f context="$CONTEXT" \ -f description="${description:0:140}" \ -f target_url="$RUN_URL" >/dev/null 2>&1; then - echo "::warning::Could not publish the '${CONTEXT}' status. It needs a token with \`statuses: write\` — pass MANAGE_TOKEN, or grant it on the caller's job. A branch rule requiring this status will stay pending until it can be written." >&2 + # Only MANAGE_TOKEN can fix this. The job declares its own + # `permissions:`, which replaces whatever the caller granted, so + # adding `statuses: write` upstream changes nothing — and the job + # cannot declare it itself without breaking every consumer that + # does not hold it. + echo "::warning::Could not publish the '${CONTEXT}' status. It needs MANAGE_TOKEN carrying \`statuses: write\`; granting it on the caller's job has no effect here. A branch rule requiring this status will stay pending until it can be written." >&2 exit 0 fi diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index b482a8cb..6fde4eaa 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -599,19 +599,31 @@ BREAKING_APPROVAL_STATUS_CONTEXT = breaking-change-approval Then one organisation ruleset requires `breaking-change-approval` and covers every repository, whatever its jobs are called. -Three things worth knowing: - -- **It is published on every run**, including those with nothing to approve. - Out of scope, no breaking commits, superseded — all publish `success`. A - status that only appears sometimes cannot be required. +Four things worth knowing: + +- **It goes `pending` before the gate waits**, and only then. GitHub reads the + latest status for a context, so without it the previous run's `success` stays + current while this run sits at the Environment — and the rule would allow a + merge on a decision nobody has given. The head SHA does not have to change + for that: `edited` re-runs on the same commit, and retargeting a pull request + moves it in or out of the gate's scope. +- **It is published on every run that reaches a verdict**, including those with + nothing to approve. Out of scope and no breaking commits both publish + `success`. A status that only appears sometimes cannot be required. +- **A superseded run publishes nothing.** Its verdict describes a revision that + is no longer current and carries `result: failure`, so publishing it would + overwrite the live run's success and block the pull request on a decision + already given. The same rule the comment and the close already follow. - **`result` decides pass or fail**, not the state. The state only chooses the wording, and the two are not allowed to disagree. -- **It needs `statuses: write`.** The job does not declare that permission, and - deliberately: a called workflow may only ask for permissions the caller - already holds, so declaring it would break every consumer that does not grant - it. It goes through `MANAGE_TOKEN` instead. Without a token that can write it - the step warns and carries on rather than failing — but a rule requiring the - status will stay pending, so check the warning before turning the rule on. +- **It needs `MANAGE_TOKEN` carrying `statuses: write`.** The job declares its + own `permissions:`, which replaces whatever the caller granted — so granting + `statuses: write` upstream has no effect here. Declaring it on the job is not + an option either: a called workflow may only ask for permissions the caller + already holds, so it would break every consumer that does not hold it. Hence + the token. Without one that can write, the step warns and carries on rather + than failing — but a rule requiring the status will stay pending, so check + the warning before turning the rule on. Requiring the job name instead still works for a single repository, and this repository does exactly that. Prefer the status wherever one rule has to cover diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 16b45cac..8739c54f 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -2333,11 +2333,13 @@ def published(**overrides): self.assertEqual(1, len(calls), calls) return calls[0] - # Pass and fail, by state. + # Pass and fail, by state. Each one carries the `result` the verdict + # actually emits for it — the previous version of this test passed the + # helper's default `success` for every state, which is how it missed + # that `superseded` publishes with `result=failure`. for state, extra, expected in ( ("approved", {}, "success"), ("none", {}, "success"), - ("superseded", {}, "success"), ("rejected", {"RESULT": "failure"}, "failure"), ("blocked", {"RESULT": "failure"}, "failure"), ("unresolved", {"RESULT": "failure"}, "failure"), @@ -2358,6 +2360,76 @@ def published(**overrides): # forever, which is worse than having no rule. self.assertIn("statuses/", published(STATE="none")) + def test_a_superseded_run_leaves_the_status_alone(self): + # The verdict emits `superseded` with `result=failure` — confirmed by + # running it — so publishing from such a run would overwrite the live + # run's success with a failure and block the pull request on a decision + # that was already given. GitHub reads the latest status for a context, + # and a superseded run can finish after the one that replaced it. + _, verdict = run_verdict(APPROVAL_RESULT="cancelled", MOCK_NEWER_RUNS="2") + self.assertEqual("superseded", verdict["state"]) + self.assertEqual("failure", verdict["result"]) + + result, calls = run_gate_status( + STATE=verdict["state"], RESULT=verdict["result"] + ) + self.assertEqual(0, result.returncode, result.stderr) + self.assertEqual([], calls, "a superseded run published a status") + + # Same for a head that moved on, the other route to superseded. + _, moved = run_verdict(LIVE_SHA="def456") + _, moved_calls = run_gate_status(STATE=moved["state"], RESULT=moved["result"]) + self.assertEqual([], moved_calls) + + def test_the_status_goes_pending_before_the_gate_waits(self): + # GitHub reads the latest status for a context, so without this the + # previous run's `success` stays current while this run sits at the + # Environment — and a rule requiring it would let the merge through on + # a decision nobody has given. + # + # The head SHA does not have to change for that: `edited` re-runs on + # the same commit, and changing a pull request's base moves it in or + # out of the gate's scope. + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + steps = workflow["jobs"]["request"]["steps"] + names = [s.get("name") for s in steps] + hold = next( + s for s in steps if s.get("name") == "Hold the gate status while the decision is pending" + ) + + # Before the wait, which is the whole point. The hold lives in + # `request`, and `approval` — the job that sits at the Environment — + # needs `request`, so the status is pending before anything waits. + self.assertIn("Hold the gate status while the decision is pending", names) + self.assertIn("request", workflow["jobs"]["approval"]["needs"]) + self.assertLess( + names.index("Hold the gate status while the decision is pending"), + names.index("Request the approval on the pull request") + 1, + ) + + # Only when this run will actually hold: publishing pending wherever + # the gate has nothing to ask would leave the status pending forever. + for condition in ( + "inputs.status_context != ''", + "needs.detect.outputs.in-scope == 'true'", + "needs.detect.outputs.has-breaking-changes == 'true'", + "steps.reuse.outputs.already-approved != 'true'", + "!inputs.dry_run", + ): + self.assertIn(condition, hold["if"], condition) + + # Same context as the final status, or the two describe different + # checks and the rule only ever sees one of them. + publish = next( + s + for s in workflow["jobs"]["outcome"]["steps"] + if s.get("name") == "Publish the gate status" + ) + self.assertEqual( + publish["env"]["CONTEXT"], hold["env"]["CONTEXT"] + ) + self.assertIn("state=pending", " ".join(hold["run"].split())) + def test_the_status_is_opt_in_and_degrades_instead_of_failing(self): workflow = yaml.safe_load(APPROVAL_WORKFLOW) step = next( From 8592f8a3bce5e588b4ad255acc6fa35453979015 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:59:27 -0300 Subject: [PATCH 3/3] fix(pr-validation): let only the newest run speak for the gate status Two more from CodeRabbit on #949, both real. **An older run could publish success over a newer run's pending.** Retarget a pull request to an in-scope base without touching the head SHA: the old out-of-scope run keeps going - `concurrency` covers the approval job alone, not the workflow - reaches `state=none`, and publishes `success` after the new run has set `pending`. GitHub reads the latest status for a context, so a status-only rule would allow the merge before anyone decided. `superseded` does not cover it. The head did not move and nothing was cancelled, so the old run's `none` is a legitimate verdict about a scope that no longer applies. Both writers now read who wrote the context last and stop if that run is newer. The run id is already in the `target_url` this gate writes, so the ordering is readable without asking the runs API, and a status written by anything else has no id to compare and is left alone. **The doc's failure mode was wrong.** It said a rule requiring the status would stay pending if the write failed. That is only true for a commit with no status yet; where the SHA already carries `success` for the context, a failed write leaves that success current and the merge is allowed. Now stated, with the advice to require the gate's job alongside the status until a run has written it at least once. Co-Authored-By: Claude Opus 5 --- .../workflows/breaking-change-approval.yml | 44 ++++++++++++++++++ docs/breaking-change-approval.md | 19 ++++++-- .../breaking-change-guard/test-workflow.py | 45 +++++++++++++++++++ 3 files changed, 105 insertions(+), 3 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 93db1e17..688e27d9 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -767,12 +767,34 @@ jobs: REPO: ${{ github.repository }} CONTEXT: ${{ inputs.status_context }} HEAD_SHA: ${{ needs.detect.outputs.head-sha }} + RUN_ID: ${{ github.run_id }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} run: | set -euo pipefail [ -n "${HEAD_SHA:-}" ] || exit 0 + # Who wrote this context last, and from which run? Runs do not finish + # in the order they start: retarget a pull request and the old + # out-of-scope run keeps going, reaches `state=none`, and publishes + # `success` after the new run has already set `pending`. The status + # is the merge decision, so the older run must not have the last + # word. + # + # The run id is in the `target_url` this gate writes, so the ordering + # is readable without asking the runs API. A status written by + # anything else has no id to compare and is left alone. + latest=$( + gh api "repos/${REPO}/commits/${HEAD_SHA}/statuses" \ + --jq "[.[] | select(.context == \"${CONTEXT}\")] | first | .target_url // empty" 2>/dev/null || true + ) + previous_run=$(grep -oE '[0-9]+$' <<< "${latest:-}" || true) + + if [ -n "$previous_run" ] && [ "$previous_run" -gt "$RUN_ID" ] 2>/dev/null; then + echo "::notice::Run ${previous_run} already spoke for '${CONTEXT}' and is newer than this one — leaving it alone" + exit 0 + fi + if ! gh api --method POST "repos/${REPO}/statuses/${HEAD_SHA}" \ -f state=pending \ -f context="$CONTEXT" \ @@ -1216,6 +1238,7 @@ jobs: REASON: ${{ needs.detect.outputs.reason }} APPROVERS: ${{ steps.decisions.outputs.approvers || needs.request.outputs.approved-by }} REJECTERS: ${{ steps.verdict.outputs.rejecters }} + RUN_ID: ${{ github.run_id }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} DRY_RUN: ${{ inputs.dry_run && 'true' || 'false' }} run: | @@ -1229,6 +1252,27 @@ jobs: # `pending` is never published. The status exists so a branch rule can # read it, and a rule reading `pending` blocks — this job runs once # the verdict is in, so it always has a final answer to give. + # Who wrote this context last, and from which run? Runs do not finish + # in the order they start: retarget a pull request and the old + # out-of-scope run keeps going, reaches `state=none`, and publishes + # `success` after the new run has already set `pending`. The status + # is the merge decision, so the older run must not have the last + # word. + # + # The run id is in the `target_url` this gate writes, so the ordering + # is readable without asking the runs API. A status written by + # anything else has no id to compare and is left alone. + latest=$( + gh api "repos/${REPO}/commits/${HEAD_SHA}/statuses" \ + --jq "[.[] | select(.context == \"${CONTEXT}\")] | first | .target_url // empty" 2>/dev/null || true + ) + previous_run=$(grep -oE '[0-9]+$' <<< "${latest:-}" || true) + + if [ -n "$previous_run" ] && [ "$previous_run" -gt "$RUN_ID" ] 2>/dev/null; then + echo "::notice::Run ${previous_run} already spoke for '${CONTEXT}' and is newer than this one — leaving it alone" + exit 0 + fi + # A superseded run must not touch the pull request, and the status is # the pull request. Its verdict is about a revision that is no longer # current, and it carries `result=failure` — so publishing it would diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 6fde4eaa..e104d758 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -599,7 +599,7 @@ BREAKING_APPROVAL_STATUS_CONTEXT = breaking-change-approval Then one organisation ruleset requires `breaking-change-approval` and covers every repository, whatever its jobs are called. -Four things worth knowing: +Five things worth knowing: - **It goes `pending` before the gate waits**, and only then. GitHub reads the latest status for a context, so without it the previous run's `success` stays @@ -610,6 +610,10 @@ Four things worth knowing: - **It is published on every run that reaches a verdict**, including those with nothing to approve. Out of scope and no breaking commits both publish `success`. A status that only appears sometimes cannot be required. +- **An older run never speaks over a newer one.** Runs do not finish in the + order they start, and the concurrency group covers the approval job alone. + Each write first reads who wrote the context last — the run id is in the + `target_url` the gate writes — and stops if that run is newer. - **A superseded run publishes nothing.** Its verdict describes a revision that is no longer current and carries `result: failure`, so publishing it would overwrite the live run's success and block the pull request on a decision @@ -622,8 +626,17 @@ Four things worth knowing: an option either: a called workflow may only ask for permissions the caller already holds, so it would break every consumer that does not hold it. Hence the token. Without one that can write, the step warns and carries on rather - than failing — but a rule requiring the status will stay pending, so check - the warning before turning the rule on. + than failing. + + **Confirm the token can write statuses before making this the only required + check.** "It will stay pending" is only true for a commit that has no status + yet. If the same SHA already carries `success` for the context — an earlier + run, before the pull request was retargeted into scope — a failed write + leaves that success current, and a status-only rule allows the merge. The + failure is silent on the pull request by design, since the step must not + fail a job over a check the caller may not require. So either require the + gate's job alongside the status, or check that a run has written the status + at least once before relying on it alone. Requiring the job name instead still works for a single repository, and this repository does exactly that. Prefer the status wherever one rule has to cover diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 8739c54f..0a76d9af 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -474,6 +474,7 @@ def run_gate_status(**overrides): "REJECTERS": "", "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", "DRY_RUN": "false", + "RUN_ID": "500", "GH_TOKEN": "stub", } env.update(overrides) @@ -484,6 +485,16 @@ def run_gate_status(**overrides): stub = Path(temp_dir) / "gh" stub.write_text( "#!/usr/bin/env bash\n" + # The read that decides whether an older run may speak. It answers + # the target_url of whoever wrote the context last; LATEST_RUN sets + # which run that was. + 'case "$*" in\n' + ' *"/statuses"*[!0-9])\n' + ' if [ -n "${LATEST_RUN:-}" ]; then\n' + " printf '%s\\n' \"https://github.com/o/r/actions/runs/${LATEST_RUN}\"\n" + " fi\n" + " exit 0 ;;\n" + "esac\n" f'printf \'%s\\n\' "$*" >> "{calls}"\n' 'if [ -n "${STUB_FAILS:-}" ]; then exit 1; fi\n', encoding="utf-8", @@ -2360,6 +2371,40 @@ def published(**overrides): # forever, which is worse than having no rule. self.assertIn("statuses/", published(STATE="none")) + def test_an_older_run_cannot_speak_over_a_newer_one(self): + # Runs do not finish in the order they start, and `concurrency` is on + # the approval job only — the rest of an older run keeps going. + # Retarget a pull request to an in-scope base without touching the head + # SHA and the old out-of-scope run reaches `state=none`, then publishes + # `success` after the new run has already set `pending`. GitHub reads + # the latest status for a context, so a status-only rule would allow + # the merge before anyone decided. + # + # `superseded` does not cover this: the head did not move and nothing + # was cancelled, so the old run's verdict is a legitimate `none`. + _, late = run_gate_status(STATE="none", RUN_ID="500", LATEST_RUN="900") + self.assertEqual([], late, "an older run published over a newer one") + + # A newer run still speaks over an older one, which is the whole point + # of the ordering rather than a blanket refusal. + _, newer = run_gate_status(STATE="none", RUN_ID="500", LATEST_RUN="100") + self.assertTrue(newer) + + # And a context nobody has written yet is published normally. + _, first = run_gate_status(STATE="none", RUN_ID="500") + self.assertTrue(first) + + # The hold applies the same rule, or the pending it writes could be + # the thing an older run overwrites. + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + hold = next( + s + for s in workflow["jobs"]["request"]["steps"] + if s.get("name") == "Hold the gate status while the decision is pending" + ) + self.assertIn("previous_run", hold["run"]) + self.assertIn("RUN_ID", hold["env"]) + def test_a_superseded_run_leaves_the_status_alone(self): # The verdict emits `superseded` with `result=failure` — confirmed by # running it — so publishing from such a run would overwrite the live