diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 4715d715..688e27d9 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 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: '' enable_comment: description: Post (and keep updated) a single comment carrying the request, the approval link and the outcome. required: false @@ -724,6 +740,72 @@ 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_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" \ + -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' @@ -814,11 +896,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 +1215,128 @@ 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_ID: ${{ github.run_id }} + 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. + # 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 + # 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 + description="Approved by ${APPROVERS:-a reviewer}" + ;; + rejected) + status=failure + description="Rejected by ${REJECTERS:-a reviewer}" + ;; + none) + status=success + description="No approval needed — ${REASON}" + ;; + 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 + # 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 + + 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..e104d758 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,72 @@ 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. + +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 + 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. +- **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 + 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 `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. + + **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 +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..0a76d9af 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,69 @@ 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", + "RUN_ID": "500", + "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" + # 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", + ) + 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 +2332,182 @@ 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. 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"), + ("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_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 + # 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( + 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"