From ce7decfe6e077b39988d3f8bf3c67d195dfb1225 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 18:57:06 -0300 Subject: [PATCH 1/3] feat(pr-validation): mark the breaking change so it reads at a glance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The alert block said what happened in prose, which meant reading it to know. A 💥 in an `##` heading inside the block makes the state legible from the timeline, before anyone opens anything — and the table gains marks on the three rows that matter, so it can be scanned instead of read. Each state now gets the alert that fits it: IMPORTANT while a decision is pending, NOTE once approved, CAUTION on a rejection that closed the pull request, WARNING when the decision was never recorded. Colour and icon come from GitHub; the emoji says what the block is about. --- .../action.yml | 35 ++++++++++++------- .../breaking-change-guard/test-workflow.py | 18 ++++++++++ 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 9768c32d..1b126aeb 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -173,9 +173,9 @@ runs: case "$STATE" in waiting) printf '> [!IMPORTANT]\n' - printf '> **Breaking change detected — this pull request needs a decision**\n' + printf '> ## 💥 Breaking change detected\n' printf '>\n' - printf '> Merging it makes the next release a **major** version. That is often deliberate; this just asks someone to confirm it before it ships.\n\n' + printf '> **This pull request needs a decision.** Merging it makes the next release a **major** version — often deliberate, so this just asks someone to confirm it before it ships.\n\n' # A shields.io badge renders as an actual button, which nothing in # GitHub-flavoured Markdown does on its own. The alt text carries @@ -190,28 +190,37 @@ runs: fi printf '| | |\n|:--|:--|\n' - [ -n "$commit_link" ] && printf '| **Commit** | %s |\n' "$commit_link" - [ -n "$mentions" ] && printf '| **Who can decide** | %s |\n' "$mentions" - printf '| **Author acknowledgement** | %s |\n\n' \ + [ -n "$commit_link" ] && printf '| 💥 **Breaking commit** | %s |\n' "$commit_link" + [ -n "$mentions" ] && printf '| 👤 **Who can decide** | %s |\n' "$mentions" + printf '| %s **Author acknowledgement** | %s |\n\n' \ + "$([ "$ACKNOWLEDGED" = "true" ] && echo "✅" || echo "⬜")" \ "$([ "$ACKNOWLEDGED" = "true" ] && echo "present" || echo "missing")" ;; approved) - printf '### ✅ Breaking change approved\n\n' - printf 'Approved by **%s**. The next release will be a major version.\n\n' \ + printf '> [!NOTE]\n' + printf '> ## ✅ Breaking change approved\n' + printf '>\n' + printf '> Approved by **%s**. 💥 The next release will be a major version.\n\n' \ "${APPROVERS:-a reviewer}" ;; rejected) - printf '### Breaking change rejected\n\n' - printf 'Rejected by **%s**' "${REJECTERS:-a reviewer}" + printf '> [!CAUTION]\n' + printf '> ## ⛔ Breaking change rejected\n' + printf '>\n' + printf '> Rejected by **%s**' "${REJECTERS:-a reviewer}" [ -n "$REJECTION_COMMENT" ] && printf ' — *%s*' "$REJECTION_COMMENT" - printf '.\n\n' + printf '.\n' if [ "$CLOSE_ON_REJECTION" = "true" ]; then - printf 'This pull request was closed. The branch is untouched — rework it without the break and reopen.\n\n' + printf '>\n' + printf '> This pull request was closed. The branch is untouched — rework it without the break and reopen.\n' fi + printf '\n' ;; unresolved) - printf '### Breaking change — decision not recorded\n\n' - printf 'The approval did not complete.' + printf '> [!WARNING]\n' + printf '> ## 💥 Breaking change — decision not recorded\n' + printf '>\n' + printf '> The approval did not complete.' [ -n "$RUN_URL" ] && printf ' [Open the run](%s) to answer it.' "$RUN_URL" printf '\n\n' ;; diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 72cee214..30b081e7 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -904,6 +904,24 @@ def test_the_button_degrades_when_the_badge_cannot_load(self): # And an empty badge-url falls back to a plain heading link. self.assertIn('elif [ -n "$RUN_URL" ]; then', reporter) + def test_each_state_carries_its_own_alert_and_mark(self): + reporter = ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + # GitHub alerts colour the block and supply the icon; the emoji marks + # what the block is about, so the state is readable from the timeline + # without opening anything. + for alert, mark in ( + ("[!IMPORTANT]", "## 💥 Breaking change detected"), + ("[!NOTE]", "## ✅ Breaking change approved"), + ("[!CAUTION]", "## ⛔ Breaking change rejected"), + ("[!WARNING]", "## 💥 Breaking change — decision not recorded"), + ): + with self.subTest(alert=alert): + self.assertIn(alert, reporter) + self.assertIn(mark, reporter) + def test_the_undo_example_uses_the_real_commit_subjects(self): reporter = ( REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" From 1c162b2006a83e1035fcc8a398e0cec362847dd2 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:04:29 -0300 Subject: [PATCH 2/3] test(pr-validation): render the comment instead of grepping the script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #909. The state assertions searched the whole reporter script, so every alert and heading was present somewhere no matter which branch used it. Swapping two states' alerts would have kept the suite green while every comment said the wrong thing. The suggested fix pulled each `case` branch out with a regex anchored to the exact indentation. That trades one fragility for another — any reindentation breaks the pattern and the test reports a problem that is not there — and it still reads the source rather than the output. So the tests now run the composite and read the Markdown it produced. `render_reporter(state)` executes the body with `gh` stubbed, and each state is checked for its own alert and mark *and* for the absence of the other three; exclusivity is what catches a swap. Verified by swapping IMPORTANT for NOTE on `waiting` and watching it fail. The link assertions moved the same way: they matched shell fragments like `${SERVER_URL}/${REPO}/commit/...` and now match rendered URLs, so the no-bare-mention rule holds however the composite spells it. --- .../breaking-change-guard/test-workflow.py | 142 +++++++++++++----- 1 file changed, 105 insertions(+), 37 deletions(-) diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 30b081e7..6172af4f 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -8,6 +8,8 @@ import unittest from pathlib import Path +import yaml + REPO_ROOT = Path(__file__).resolve().parents[3] WORKFLOW_PATH = REPO_ROOT / ".github" / "workflows" / "pr-validation.yml" @@ -213,6 +215,71 @@ def execute_body(body, env): return result, parse_github_output(output) +def render_reporter(state, **overrides): + """Render the reporter's comment for one state and return the Markdown. + + Asserting against the composite's source only proves a string exists + somewhere in it — swapping two states' alerts would keep every string + present and every assertion green. Running the thing and reading what it + produced is the only check that ties a state to its own output. + """ + action = yaml.safe_load( + ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + ) + # Everything up to the posting is the part that builds the Markdown. + body = action["runs"]["steps"][0]["run"].split('comment_id=""')[0] + + env = { + "REPO": "LerianStudio/example", + "PR_NUMBER": "42", + "STATE": state, + "REASON": "rule develop <- '*'", + "HEAD_SHA": "abc1234567", + "ACKNOWLEDGED": "false", + "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", + "ENABLED": "true", + "GH_TOKEN": "stub", + } + env.update(overrides) + + with tempfile.TemporaryDirectory() as stub_dir: + stub = Path(stub_dir) / "gh" + stub.write_text( + "#!/usr/bin/env bash\n" + "case \"$*\" in\n" + " *\"/commits\"*) printf '%s\\n'" + " 'feat(scope)!: a breaking subject' 'chore: an ordinary one' ;;\n" + " *) exit 1 ;;\n" + "esac\n", + encoding="utf-8", + ) + stub.chmod(0o755) + env["PATH"] = f"{stub_dir}:{os.environ.get('PATH', '/usr/bin:/bin')}" + result = subprocess.run( + ["bash", "-c", body + '\ncat "$body_file"'], + cwd=stub_dir, + env=env, + text=True, + capture_output=True, + check=False, + ) + if result.returncode != 0: + raise AssertionError(f"reporter failed for state {state!r}: {result.stderr}") + return result.stdout + + def parse_github_output(output): values = {} for line in output.splitlines(): @@ -822,32 +889,24 @@ def test_reviewers_come_from_codeowners_on_the_base_ref(self): ) def test_the_waiting_comment_links_instead_of_mentioning(self): - reporter = ( - REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" - / "action.yml" - ).read_text(encoding="utf-8") + rendered = render_reporter("waiting") # Links to the commit, the decision, the reviewers and the docs. for fragment in ( - "${SERVER_URL}/${REPO}/commit/${HEAD_SHA}", - "[![Review the pending deployment](%s)](%s)", - "${SERVER_URL}/orgs/${owner}/teams/${slug}", - "[How this gate works](${DOCS_URL})", + "](https://github.com/LerianStudio/example/commit/abc1234567)", + "[![Review the pending deployment](https://img.shields.io/badge/", + "](https://github.com/orgs/LerianStudio/teams/devops)", + "[How this gate works](https://example.invalid/docs)", ): with self.subTest(fragment=fragment): - self.assertIn(fragment, reporter) + self.assertIn(fragment, rendered) + # The pending deployment already notifies the reviewers; a bare @mention - # would ping the whole team again for the same thing. - # - # Checked as "every occurrence sits inside Markdown link text", not as - # one forbidden spelling: an assertion pinned to the exact characters - # around `@` passes the day somebody writes the mention differently, - # which is worse than no assertion at all. - for match in re.finditer(r"@\$\{owner\}/", reporter): - preceding = reporter[: match.start()] + # would ping the whole team again for the same thing. Checked on the + # rendered comment, so it holds however the composite spells it. + for match in re.finditer(r"@LerianStudio/", rendered): self.assertTrue( - preceding.endswith("["), - f"bare team mention at offset {match.start()}: " - f"...{reporter[max(0, match.start() - 40):match.end() + 20]}...", + rendered[: match.start()].endswith("["), + f"bare team mention: ...{rendered[max(0, match.start() - 40):match.end() + 30]}...", ) def test_only_one_approval_request_stays_open_per_pull_request(self): @@ -904,23 +963,32 @@ def test_the_button_degrades_when_the_badge_cannot_load(self): # And an empty badge-url falls back to a plain heading link. self.assertIn('elif [ -n "$RUN_URL" ]; then', reporter) - def test_each_state_carries_its_own_alert_and_mark(self): - reporter = ( - REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" - / "action.yml" - ).read_text(encoding="utf-8") - # GitHub alerts colour the block and supply the icon; the emoji marks - # what the block is about, so the state is readable from the timeline - # without opening anything. - for alert, mark in ( - ("[!IMPORTANT]", "## 💥 Breaking change detected"), - ("[!NOTE]", "## ✅ Breaking change approved"), - ("[!CAUTION]", "## ⛔ Breaking change rejected"), - ("[!WARNING]", "## 💥 Breaking change — decision not recorded"), - ): - with self.subTest(alert=alert): - self.assertIn(alert, reporter) - self.assertIn(mark, reporter) + # GitHub alerts colour the block and supply the icon; the emoji marks what + # the block is about, so the state is readable from the timeline without + # opening anything. + STATE_MARKS = ( + ("waiting", "[!IMPORTANT]", "## 💥 Breaking change detected"), + ("approved", "[!NOTE]", "## ✅ Breaking change approved"), + ("rejected", "[!CAUTION]", "## ⛔ Breaking change rejected"), + ("unresolved", "[!WARNING]", "## 💥 Breaking change — decision not recorded"), + ) + + def test_each_state_renders_its_own_alert_and_mark(self): + # Rendered, not grepped. Asserting against the source only proves a + # string exists somewhere in it, so swapping two states' alerts would + # keep every assertion green while every comment said the wrong thing. + for state, alert, mark in self.STATE_MARKS: + with self.subTest(state=state): + rendered = render_reporter(state) + self.assertIn(alert, rendered) + self.assertIn(mark, rendered) + + # Exclusivity is what actually catches a swap. + for other_state, other_alert, other_mark in self.STATE_MARKS: + if other_state == state: + continue + self.assertNotIn(other_alert, rendered) + self.assertNotIn(other_mark, rendered) def test_the_undo_example_uses_the_real_commit_subjects(self): reporter = ( From 72ee2dd2b6ba8baec5b09b63318c7514e8450389 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:09:23 -0300 Subject: [PATCH 3/3] test(pr-validation): assert the alert block, not two loose strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the third CodeRabbit review on #909. The state test read the rendered comment but checked `[!IMPORTANT]` and `## 💥 …` as separate strings. A GitHub alert only renders inside a blockquote, so dropping the `>` would leave both assertions green while the comment rendered as plain text — getting the content right and the presentation wrong, which is the thing this PR is about. Now asserted as adjacent quoted lines. Verified by removing the `>` from `waiting` and watching it fail. --- src/validate/breaking-change-guard/test-workflow.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 6172af4f..80c63546 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -980,8 +980,11 @@ def test_each_state_renders_its_own_alert_and_mark(self): for state, alert, mark in self.STATE_MARKS: with self.subTest(state=state): rendered = render_reporter(state) - self.assertIn(alert, rendered) - self.assertIn(mark, rendered) + # As adjacent blockquote lines, not as two strings that happen + # to be somewhere: a GitHub alert only renders inside `>`, so + # dropping the quote would leave both assertions green and the + # comment looking like plain text. + self.assertIn(f"> {alert}\n> {mark}", rendered) # Exclusivity is what actually catches a swap. for other_state, other_alert, other_mark in self.STATE_MARKS: