From 241d656e43d06f82206d9eb54b2205911affcc30 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:23:05 -0300 Subject: [PATCH 1/2] fix(pr-validation): keep the breaking-change label after the approval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The label was being removed the moment the change was approved, which gets it backwards: `breaking-change` describes the commits, and the break does not stop being a break because somebody agreed to it. The repository's own labels.yml says as much — "Callers must update their workflow configuration after this change" is a fact about the change, not a step in a workflow. Removing it erased the record exactly when it started to matter: now the break is both real and agreed. Two labels now, expiring at different times: breaking-change a fact. Applied throughout, removed only when the breaking commits are gone awaiting-breaking-approval a state. On while a decision is pending, off the moment one is made, either way Found while implementing this: the cleanup for the fact label did not exist at all. An earlier rewrite of the verdict job dropped it, so a reword that removed the `!` left `breaking-change` on the pull request for good. Restored, and now removing both. awaiting-breaking-approval added to .github/labels.yml. Both are cosmetic — a repository that never declared them still gets its gate. --- .github/labels.yml | 4 +++ .../workflows/breaking-change-approval.yml | 34 +++++++++++++++++-- docs/breaking-change-approval.md | 12 +++++-- .../README.md | 18 +++++++++- .../action.yml | 31 ++++++++++++++--- .../breaking-change-guard/test-workflow.py | 27 +++++++++++++++ 6 files changed, 116 insertions(+), 10 deletions(-) diff --git a/.github/labels.yml b/.github/labels.yml index 8498552c..6e04c717 100644 --- a/.github/labels.yml +++ b/.github/labels.yml @@ -82,6 +82,10 @@ color: "b60205" description: Callers must update their workflow configuration after this change +- name: awaiting-breaking-approval + color: "fbca04" + description: A breaking change on this PR is waiting on a human decision + # ── Capabilities ────────────────────────────────────────────────────────────── - name: build diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 25cf40e2..bd443ec9 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -144,10 +144,23 @@ on: type: boolean default: true label: - description: Label applied while the pull request waits for its approval, and removed once resolved. Cosmetic — failure to apply it never fails the job. Empty disables it. + description: >- + Label stating that this pull request carries a breaking change. A fact + about the change, so it survives the approval — the break does not go + away because somebody agreed to it. Removed only when the breaking + commits are gone. Empty disables it. required: false type: string default: breaking-change + pending_label: + description: >- + Label stating that the breaking change is waiting on a decision. A + state, so it comes off once approved or rejected. Separate from + `label` because the two stop being true at different moments. + Empty disables it. + required: false + type: string + default: awaiting-breaking-approval enable_comment: description: Post (and keep updated) a single comment carrying the request, the approval link and the outcome. required: false @@ -512,7 +525,8 @@ jobs: head-sha: ${{ needs.detect.outputs.head-sha }} acknowledged: ${{ needs.detect.outputs.acknowledged }} author-acknowledgement: ${{ inputs.author_acknowledgement }} - label: '' + label: ${{ inputs.label }} + pending-label: '' enabled: ${{ inputs.enable_comment && 'true' || 'false' }} - name: Request the approval on the pull request @@ -536,6 +550,7 @@ jobs: run-url: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} close-on-rejection: ${{ inputs.close_on_rejection && 'true' || 'false' }} label: ${{ inputs.label }} + pending-label: ${{ inputs.pending_label }} enabled: ${{ inputs.enable_comment && 'true' || 'false' }} # ----------------- The gate itself ----------------- @@ -625,6 +640,8 @@ jobs: APPROVERS: ${{ steps.decisions.outputs.approvers }} REJECTERS: ${{ steps.decisions.outputs.rejecters }} API_READABLE: ${{ steps.decisions.outputs.api-readable }} + LABEL: ${{ inputs.label }} + PENDING_LABEL: ${{ inputs.pending_label }} DRY_RUN: ${{ inputs.dry_run && 'true' || 'false' }} run: | set -euo pipefail @@ -705,6 +722,18 @@ jobs: result=success state=none emit + + # The only moment the break actually stopped existing, so the only + # place the fact label comes off. A reword dropping the `!` left + # `breaking-change` on the pull request for good — the cleanup was + # lost in an earlier rewrite of this job and is restored here. + if [ "$DRY_RUN" != "true" ] && [ -n "${PR_NUMBER:-}" ]; then + for stale in "$LABEL" "$PENDING_LABEL"; do + [ -n "$stale" ] || continue + gh pr edit "$PR_NUMBER" --repo "$REPO" --remove-label "$stale" >/dev/null 2>&1 || true + done + fi + echo "::notice::Nothing to approve on PR #${PR_NUMBER} — ${REASON}" exit 0 fi @@ -763,6 +792,7 @@ jobs: rejection-comment: ${{ steps.decisions.outputs.rejection-comment }} close-on-rejection: ${{ inputs.close_on_rejection && 'true' || 'false' }} label: ${{ inputs.label }} + pending-label: ${{ inputs.pending_label }} enabled: ${{ inputs.enable_comment && 'true' || 'false' }} # Only on a recorded rejection, never on a cancellation. Closing a pull diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index be87e051..942d373b 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -408,8 +408,16 @@ ends as the record of who decided what: > 👉 **[Review the pending deployment](#)** — *Review deployments* → > `breaking-approval`. -The `breaking-change` label is applied while it waits and removed once resolved — -cosmetic, and a repository that never declared it still gets its gate. +Two labels, and they expire at different times. `breaking-change` states a fact +about the commits and **survives the approval** — the break does not stop being a +break because somebody agreed to it, and a search for that label months later +should still find this pull request. It comes off only when the breaking commits +are gone. `awaiting-breaking-approval` states that a decision is pending, and +comes off the moment one is made. + +Both are cosmetic: a repository that never declared them still gets its gate. +Declare them with [`labels-sync`](labels-sync.md), or pass `label: ''` / +`pending_label: ''` to turn them off. ## Inputs diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index 4af911ef..0d3cc91c 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -82,6 +82,21 @@ may be gated by different people than the caller configured, and naming the wron ones is worse than naming none — with neither available, the line is omitted rather than guessed. +## Two labels, because they expire at different times + +`breaking-change` is a **fact about the commits**. Approving does not make the +break go away, and somebody filtering for that label three months from now wants +this pull request in the results. It is applied in every state this composite +handles, and only the caller removes it — at the one moment the fact stops being +true, when the breaking commits are gone. + +`awaiting-breaking-approval` is a **state**. It goes on while a decision is +pending and comes off the moment one is made, either way. + +Collapsing the two is tempting and wrong: removing `breaking-change` on approval +erases the record exactly when it starts to matter, because now the break is +both real and agreed. + ## What it does not do It never closes, merges or approves anything. `close-on-rejection` only changes @@ -113,7 +128,8 @@ side effect in the caller is what makes this safe to run with | `rejecters` | Comma-separated logins that rejected. | No | `''` | | `rejection-comment` | Comment left with the rejection. | No | `''` | | `close-on-rejection` | Whether the caller closes on rejection. Wording only. | No | `true` | -| `label` | Applied while waiting, removed once resolved. Cosmetic. | No | `''` | +| `label` | States that the PR carries a breaking change. A fact — stays once applied. Cosmetic. | No | `''` | +| `pending-label` | States that the breaking change is waiting on a decision. Removed once approved or rejected. | No | `''` | | `enabled` | `false` skips commenting while still managing the label. | No | `true` | ## Outputs diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 977bddf3..bb948c30 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -80,7 +80,11 @@ inputs: required: false default: 'true' label: - description: Label applied while waiting and removed once resolved. Cosmetic — failing to apply it never fails the step. Empty disables it. + description: 'Label stating that this pull request carries a breaking change. A fact about the change, so it stays once applied — approving does not make the break go away. The caller removes it when the breaking commits are gone. Cosmetic; failing to apply it never fails the step. Empty disables it.' + required: false + default: '' + pending-label: + description: 'Label stating that the breaking change is waiting on a decision. A state, so it is removed once approved or rejected. Kept separate from `label` because "this breaks things" and "nobody has decided yet" stop being true at different moments. Empty disables it.' required: false default: '' enabled: @@ -120,6 +124,7 @@ runs: REJECTION_COMMENT: ${{ inputs.rejection-comment }} CLOSE_ON_REJECTION: ${{ inputs.close-on-rejection }} LABEL: ${{ inputs.label }} + PENDING_LABEL: ${{ inputs.pending-label }} ENABLED: ${{ inputs.enabled }} run: | set -euo pipefail @@ -356,14 +361,30 @@ runs: fi # Cosmetic and deliberately last — a repository that never declared the - # label still gets its gate. + # labels still gets its gate. + # + # Two of them, because they stop being true at different moments. The + # breaking change is a property of the commits: approving it does not + # make the break go away, and somebody filtering for `breaking-change` + # three months from now wants this pull request in the results. Waiting + # on a decision is a state, and it ends. + # + # Every state reaching this composite means a breaking change is + # present — the caller skips it entirely when there is none — so the + # fact label is applied throughout and only the caller removes it. if [ -n "$LABEL" ]; then + if ! gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "$LABEL" >/dev/null 2>&1; then + echo "::notice::Label '${LABEL}' not applied — it is not declared in ${REPO}. Cosmetic only." + fi + fi + + if [ -n "$PENDING_LABEL" ]; then if [ "$STATE" = "waiting" ]; then - if ! gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "$LABEL" >/dev/null 2>&1; then - echo "::notice::Label '${LABEL}' not applied — it is not declared in ${REPO}. Cosmetic only." + if ! gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "$PENDING_LABEL" >/dev/null 2>&1; then + echo "::notice::Label '${PENDING_LABEL}' not applied — it is not declared in ${REPO}. Cosmetic only." fi else - gh pr edit "$PR_NUMBER" --repo "$REPO" --remove-label "$LABEL" >/dev/null 2>&1 || true + gh pr edit "$PR_NUMBER" --repo "$REPO" --remove-label "$PENDING_LABEL" >/dev/null 2>&1 || true fi fi diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index d731c86c..92810945 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1219,6 +1219,33 @@ def test_an_unevaluated_acknowledgement_is_not_reported_as_missing(self): self.assertIn("Author acknowledgement", rendered) self.assertIn("missing", rendered) + def test_the_fact_label_outlives_the_decision(self): + reporter = ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + # `breaking-change` describes the commits, not the process: approving + # does not make the break go away, and somebody filtering for it months + # later wants this pull request in the results. Only the pending label + # is a state, and only it comes off on a decision. + self.assertIn("PENDING_LABEL", reporter) + add_fact = reporter.index('--add-label "$LABEL"') + self.assertNotIn('--remove-label "$LABEL"', reporter) + self.assertIn('--remove-label "$PENDING_LABEL"', reporter) + self.assertGreater(add_fact, 0) + + def test_both_labels_go_when_the_break_does(self): + body = extract_run_body( + extract_job(APPROVAL_WORKFLOW, "outcome"), "Resolve the verdict" + ) + # The one moment the fact stops being true. This cleanup was lost in an + # earlier rewrite, leaving `breaking-change` on a pull request whose + # breaking commit had been reworded away. + self.assertIn('for stale in "$LABEL" "$PENDING_LABEL"', body) + none_at = body.index("state=none") + cleanup_at = body.index('for stale in "$LABEL" "$PENDING_LABEL"') + self.assertLess(none_at, cleanup_at) + def test_the_fingerprint_comes_from_the_breaking_commits(self): detect = extract_job(APPROVAL_WORKFLOW, "detect") self.assertIn("src/validate/breaking-change-commits@", detect) From 23403cd27364cbd2cbd5d4098d896e9a0950ed29 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:40:26 -0300 Subject: [PATCH 2/2] test(pr-validation): run the label lifecycle instead of reading it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the CodeRabbit review on #919. The label test asserted on the composite's text, which cannot say which state made which call. Moving `--add-label "$LABEL"` into the `waiting` branch would have left every string in place and stopped the fact label from surviving a decision — the one thing this PR exists to fix. `run_reporter_labels(state)` executes the reporter with `gh` stubbed and returns the `gh pr edit` calls. Each state is checked for the fact label being added and never removed, and for the pending label being added only while waiting. Verified by that exact mutation: three subtests fail. Worth saying plainly: I ran this check by hand while implementing, and pasted its output as evidence. Running it once and versioning it are not the same thing, and only the second survives the next refactor. Also documents the behaviour change for callers who set `label` as a temporary waiting marker — move it to `pending_label` and clear `label`. Callers on the default need nothing. --- .../workflows/breaking-change-approval.yml | 4 + docs/breaking-change-approval.md | 8 ++ .../action.yml | 2 +- .../breaking-change-guard/test-workflow.py | 117 ++++++++++++++++-- 4 files changed, 117 insertions(+), 14 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index bd443ec9..08ab0dcf 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -149,6 +149,10 @@ on: about the change, so it survives the approval — the break does not go away because somebody agreed to it. Removed only when the breaking commits are gone. Empty disables it. + + Behaviour change: this label used to come off on approval. A caller + that set it as a temporary waiting marker should move that value to + `pending_label` and set this one to an empty string. required: false type: string default: breaking-change diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 942d373b..38e5aebb 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -419,6 +419,14 @@ Both are cosmetic: a repository that never declared them still gets its gate. Declare them with [`labels-sync`](labels-sync.md), or pass `label: ''` / `pending_label: ''` to turn them off. +> **Behaviour change.** `label` used to be removed once the change was approved +> or rejected. If you set it as a temporary waiting marker, move that value to +> `pending_label` and set `label: ''` — otherwise the marker now stays on the +> pull request after the decision. +> +> Callers on the default need nothing: `breaking-change` staying put is the +> point, and `awaiting-breaking-approval` takes over the transient role. + ## Inputs | Input | Type | Default | Description | diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index bb948c30..b21f6e18 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -80,7 +80,7 @@ inputs: required: false default: 'true' label: - description: 'Label stating that this pull request carries a breaking change. A fact about the change, so it stays once applied — approving does not make the break go away. The caller removes it when the breaking commits are gone. Cosmetic; failing to apply it never fails the step. Empty disables it.' + description: 'Label stating that this pull request carries a breaking change. A fact about the change, so it stays once applied — approving does not make the break go away. The caller removes it when the breaking commits are gone. Cosmetic; failing to apply it never fails the step. Empty disables it. Behaviour change: it used to be removed on a decision — a caller using it as a temporary waiting marker should move that value to `pending-label` and clear this one.' required: false default: '' pending-label: diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 92810945..23c8255f 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -347,6 +347,79 @@ def run_verdict(**overrides): return result, parse_github_output(output_path.read_text(encoding="utf-8")) +def run_reporter_labels(state, **overrides): + """Run the reporter end to end and return the `gh pr edit` calls it made. + + `render_reporter` stops before the label block, so asserting on the + composite's text was the only coverage the lifecycle had — and text cannot + tell which state made which call. Moving `--add-label "$LABEL"` into the + `waiting` branch would keep every string in place and break the behaviour. + """ + action = yaml.safe_load( + ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + ) + body = action["runs"]["steps"][0]["run"] + + env = { + "REPO": "LerianStudio/example", + "PR_NUMBER": "42", + "STATE": state, + "REASON": "rule develop <- '*'", + "HEAD_SHA": "abc1234567", + "ACKNOWLEDGED": "false", + "ACK_LINE": "Breaking change acknowledged: …", + "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", + "REVIEWERS": "team:devops", + "SERVER_URL": "https://github.com", + "BADGE_URL": "https://img.shields.io/badge/x-1f6feb", + "DOCS_URL": "https://example.invalid/docs", + "APPROVERS": "carol", + "REJECTERS": "dave", + "REJECTION_COMMENT": "", + "CLOSE_ON_REJECTION": "true", + "LABEL": "breaking-change", + "PENDING_LABEL": "awaiting-breaking-approval", + "ENABLED": "false", + "FINGERPRINT": "", + "RUN_ID": "1", + "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" + 'case "$*" in\n' + " *\"pr edit\"*)" + " printf '%s\\n' \"$*\" | grep -oE '(add|remove)-label [a-z-]+'" + f' >> "{calls}" ;;\n' + " *\"/commits\"*) printf '%s\\n' 'feat(x)!: breaks' ;;\n" + " *) exit 1 ;;\n" + "esac\n", + encoding="utf-8", + ) + stub.chmod(0o755) + env["PATH"] = f"{temp_dir}:{os.environ.get('PATH', '/usr/bin:/bin')}" + env["GITHUB_OUTPUT"] = str(Path(temp_dir) / "out") + result = subprocess.run( + ["bash", "-c", body], + cwd=temp_dir, + env=env, + text=True, + capture_output=True, + check=False, + ) + if result.returncode != 0: + raise AssertionError(f"reporter failed for {state!r}: {result.stderr}") + return calls.read_text(encoding="utf-8").split() + + def parse_github_output(output): values = {} for line in output.splitlines(): @@ -1220,19 +1293,37 @@ def test_an_unevaluated_acknowledgement_is_not_reported_as_missing(self): self.assertIn("missing", rendered) def test_the_fact_label_outlives_the_decision(self): - reporter = ( - REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" - / "action.yml" - ).read_text(encoding="utf-8") - # `breaking-change` describes the commits, not the process: approving - # does not make the break go away, and somebody filtering for it months - # later wants this pull request in the results. Only the pending label - # is a state, and only it comes off on a decision. - self.assertIn("PENDING_LABEL", reporter) - add_fact = reporter.index('--add-label "$LABEL"') - self.assertNotIn('--remove-label "$LABEL"', reporter) - self.assertIn('--remove-label "$PENDING_LABEL"', reporter) - self.assertGreater(add_fact, 0) + # Run it, per state, and read the `gh pr edit` calls. Asserting on the + # composite's text cannot tell which state made which call: moving + # `--add-label "$LABEL"` into the `waiting` branch would leave every + # string in place and stop the fact label from surviving a decision. + for state in ("waiting", "approved", "rejected", "acknowledgement"): + with self.subTest(state=state): + calls = run_reporter_labels(state) + pairs = list(zip(calls[::2], calls[1::2])) + + self.assertIn( + ("add-label", "breaking-change"), + pairs, + "the fact label belongs on every state the gate reports", + ) + self.assertNotIn( + ("remove-label", "breaking-change"), + pairs, + "approving does not make the break go away", + ) + + expected = "add-label" if state == "waiting" else "remove-label" + self.assertIn((expected, "awaiting-breaking-approval"), pairs) + + def test_the_two_labels_can_be_turned_off_independently(self): + # Cosmetic throughout: a repository that declared neither still gets + # its gate, and one that wants only the fact can say so. + self.assertEqual([], run_reporter_labels("waiting", LABEL="", PENDING_LABEL="")) + self.assertEqual( + ["add-label", "breaking-change"], + run_reporter_labels("waiting", PENDING_LABEL=""), + ) def test_both_labels_go_when_the_break_does(self): body = extract_run_body(