From f8d5506708515e26445546293467fb48b85ecdaa Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:59:15 -0300 Subject: [PATCH 1/5] feat(pr-validation)!: smoke test the breaking change approval gate Deliberately marked breaking so the gate fires. This pull request exists to watch it work end to end and is not meant to be merged. --- docs/breaking-change-approval.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index cd86d24b..1f6385c2 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -404,3 +404,5 @@ python3 src/validate/breaking-change-guard/test-workflow.py orchestrated composite exists with its README and is pinned, that the preflight runs before the gate, that `outcome` does not declare a permission it cannot escalate to, and that a pull request is closed only on a recorded rejection. + + From 607848bd3c62c0511bb597a1bca1b496935e40e3 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 18:59:08 -0300 Subject: [PATCH 2/5] test(pr-validation): probe whether a push cancels the pending approval --- docs/breaking-change-approval.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 60c7f854..9295ff7d 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -426,3 +426,5 @@ python3 src/validate/breaking-change-guard/test-workflow.py escalate to, and that a pull request is closed only on a recorded rejection. + + From d578fe0b4699e93a44d35c48a7f943a089128492 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:14:31 -0300 Subject: [PATCH 3/5] feat(pr-validation): one comment for the breaking change, not two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported on #906: two bot comments about the same breaking change, one headed "Breaking change guard — ✅ Author acknowledged" and one asking for a decision. The ✅ reads as the approval. It is not: it says the author declared the break, and the small print saying so is small print. The second comment only appeared yesterday, when #913 fixed the 403 that had kept it silent since August. Fixing the permission put a ✅ next to a pending request without my noticing the two would collide. `breaking-change-comment` is removed. The gate's comment already carries the acknowledgement state, with the decision as the headline and the acknowledgement as a row — the hierarchy the two controls actually have. To replace what the removed comment covered, the gate now comments wherever a breaking change is found, not only inside its approval scope. Outside it there is no decision to make, so the comment reports the acknowledgement and the exact line to paste. The waiting comment carries that line too, folded away, since Blocking Checks fails on a missing acknowledgement whether or not an approval is pending. pr-validation now passes the acknowledgement literal to the gate instead of relying on the default to coincide. They were equal by hand across two files; a drift would have had the gate teaching a line that fails the check it was explaining. --- .../workflows/breaking-change-approval.yml | 35 +++++- .github/workflows/pr-validation.yml | 117 ++---------------- docs/pr-validation.md | 13 +- .../README.md | 4 + .../action.yml | 35 +++++- .../breaking-change-guard/test-workflow.py | 84 +++---------- 6 files changed, 109 insertions(+), 179 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index ebd7156e..b36887d6 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -314,9 +314,18 @@ jobs: detection_succeeded=false if [ "$IN_SCOPE" != "true" ]; then - # Nothing was scanned, and nothing needs to be: an out-of-scope pull - # request is a successful no-op, not a failed detection. + # Out of scope means no approval is required — it does not mean + # nothing was found. When the caller already detected, keep that + # answer: the acknowledgement is still enforced everywhere, and the + # author needs to be told which line to add. Scanning here is what + # would be pointless, not knowing. detection_succeeded=true + case "$SUPPLIED_HAS_BREAKING_CHANGES" in + true | false ) + has_breaking_changes="$SUPPLIED_HAS_BREAKING_CHANGES" + [ "$SUPPLIED_ACKNOWLEDGED" = "true" ] && acknowledged=true + ;; + esac elif [ -n "$SUPPLIED_HAS_BREAKING_CHANGES" ]; then # The caller already ran the same composite over the same commits. # Anything but a clean boolean is a wiring mistake, and a wiring @@ -510,6 +519,27 @@ jobs: create-if-missing: ${{ inputs.create_approval_environment && 'true' || 'false' }} reviewers: ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers || steps.codeowners.outputs.reviewers }} + # Out of scope, so no approval is coming — but the acknowledgement is + # enforced on every pull request, and an author who omits it otherwise + # sees Blocking Checks fail with nothing explaining what to add. + - name: Report the acknowledgement requirement + if: >- + needs.detect.result == 'success' + && needs.detect.outputs.in-scope != 'true' + && needs.detect.outputs.has-breaking-changes == 'true' + && !inputs.dry_run + continue-on-error: true + uses: LerianStudio/github-actions-shared-workflows/src/notify/breaking-change-approval-reporter@develop + with: + github-token: ${{ secrets.MANAGE_TOKEN || github.token }} + pr-number: ${{ needs.detect.outputs.pr-number }} + state: acknowledgement + head-sha: ${{ needs.detect.outputs.head-sha }} + acknowledged: ${{ needs.detect.outputs.acknowledged }} + author-acknowledgement: ${{ inputs.author_acknowledgement }} + label: '' + enabled: ${{ inputs.enable_comment && 'true' || 'false' }} + - name: Request the approval on the pull request if: >- needs.detect.result == 'success' @@ -521,6 +551,7 @@ jobs: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} pr-number: ${{ needs.detect.outputs.pr-number }} state: waiting + author-acknowledgement: ${{ inputs.author_acknowledgement }} fingerprint: ${{ needs.detect.outputs.fingerprint }} reviewers: ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers || steps.codeowners.outputs.reviewers }} actual-reviewers: ${{ steps.environment.outputs.reviewers }} diff --git a/.github/workflows/pr-validation.yml b/.github/workflows/pr-validation.yml index 36f1c6e1..ac4c5338 100644 --- a/.github/workflows/pr-validation.yml +++ b/.github/workflows/pr-validation.yml @@ -296,7 +296,17 @@ jobs: uses: ./.github/workflows/breaking-change-approval.yml with: detected_breaking_changes: ${{ needs.breaking-change-guard.result == 'success' && needs.breaking-change-guard.outputs.detection-succeeded == 'true' && needs.breaking-change-guard.outputs.has-breaking-changes || 'unknown' }} + # The acknowledgement state travels with it, so one comment can speak for + # both controls. It replaces `breaking-change-comment`, removed here: two + # bot comments about the same breaking change, one headed by a ✅ for the + # author's acknowledgement and one asking for a decision, read as a + # contradiction. The ✅ was taken for the approval. detected_acknowledged: ${{ needs.breaking-change-guard.outputs.approved }} + # Passed explicitly rather than left to the gate's default. The comment + # tells the author which line to paste, and the detector decides whether + # what they pasted counts — two literals in two files, previously kept + # equal by hand. A drift would have the gate teaching a line that fails. + author_acknowledgement: 'Breaking change acknowledged: I understand that this PR intentionally introduces a breaking change and requires the next release to be a major version.' scope_rules: ${{ inputs.breaking_change_scope_rules }} additional_scope_rules: ${{ inputs.breaking_change_additional_scope_rules }} force_labels: ${{ inputs.breaking_change_force_labels }} @@ -485,113 +495,6 @@ jobs: dry-run: ${{ inputs.dry_run && 'true' || 'false' }} # ----------------- Breaking Change Feedback Comment (best effort) ----------------- - breaking-change-comment: - name: Breaking Change Comment - runs-on: ${{ vars.GENERAL_RUNNERS || inputs.runner_type }} - needs: [breaking-change-guard] - if: always() && github.event_name == 'pull_request' && github.event.pull_request.draft != true && !inputs.dry_run - # `pull-requests: write`, not `issues: write`. The step posts through the - # issue-comments endpoint, but the subject is a pull request — and GitHub - # authorises those as pull requests. `issues: write` covers real issues - # only, so this job spent from 2026-08-06 taking a 403 on every run and - # reporting success, because the step is continue-on-error. - # - # The job-level block narrows the workflow's own `pull-requests: write`, so - # declaring `read` here was not least privilege — it was below what the step - # needs to work at all. - permissions: - pull-requests: write - - steps: - - name: Manage breaking change guard comment - id: comment - continue-on-error: true - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - GUARD_JOB_RESULT: ${{ needs.breaking-change-guard.result }} - DETECTION_SUCCEEDED: ${{ needs.breaking-change-guard.outputs.detection-succeeded }} - HAS_BREAKING_CHANGES: ${{ needs.breaking-change-guard.outputs.has-breaking-changes }} - BREAKING_CHANGE_ACKNOWLEDGED: ${{ needs.breaking-change-guard.outputs.approved }} - EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - ACKNOWLEDGEMENT: 'Breaking change acknowledged: I understand that this PR intentionally introduces a breaking change and requires the next release to be a major version.' - with: - github-token: ${{ github.token }} - script: | - const marker = ''; - const guardJobSucceeded = process.env.GUARD_JOB_RESULT === 'success'; - const detectionSucceeded = process.env.DETECTION_SUCCEEDED === 'true'; - const hasBreakingChanges = process.env.HAS_BREAKING_CHANGES === 'true'; - const acknowledged = process.env.BREAKING_CHANGE_ACKNOWLEDGED === 'true'; - const eventHeadSha = process.env.EVENT_HEAD_SHA; - const acknowledgement = process.env.ACKNOWLEDGEMENT; - - const current = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.issue.number, - }); - if (!eventHeadSha || current.data.head.sha !== eventHeadSha) { - core.warning(`Skipping stale breaking change comment update: event head ${eventHeadSha || 'missing'}, current head ${current.data.head.sha}.`); - return; - } - - const comments = await github.paginate(github.rest.issues.listComments, { - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - per_page: 100, - }); - const owned = comments.filter(comment => - comment.user?.login === 'github-actions[bot]' - && (comment.body === marker || comment.body?.startsWith(`${marker}\n`)) - ); - const deleteComment = comment => github.rest.issues.deleteComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: comment.id, - }); - const upsertOwned = async body => { - const params = { owner: context.repo.owner, repo: context.repo.repo, body }; - - if (owned.length > 0) { - await github.rest.issues.updateComment({ ...params, comment_id: owned[0].id }); - await Promise.all(owned.slice(1).map(deleteComment)); - return; - } - - await github.rest.issues.createComment({ - ...params, - issue_number: context.issue.number, - }); - }; - - if (!guardJobSucceeded || !detectionSucceeded) { - const body = `${marker}\n## Breaking change guard\n\n**Status:** ⚠️ Detection failed\n\nThe workflow could not determine whether this pull request contains breaking changes. Blocking Checks failed closed.`; - await upsertOwned(body); - return; - } - - if (!hasBreakingChanges) { - await Promise.all(owned.map(deleteComment)); - return; - } - - const state = acknowledged ? '✅ Author acknowledged' : '🚫 Awaiting author acknowledgement'; - const detail = acknowledged - ? 'The exact visible author acknowledgement line is present in the pull request description. It records intentional awareness and does not grant maintainer permission.' - : 'The PR author can acknowledge intentional awareness by adding this exact visible line to the pull request description:'; - const body = `${marker}\n## Breaking change guard\n\n**Status:** ${state}\n\n${detail}\n\n\`${acknowledgement}\``; - await upsertOwned(body); - - # Cosmetic enough to keep continue-on-error — losing the comment should - # not fail validation — but not so cosmetic that it should fail in - # silence. Swallowing it is what let a 403 run for two months while every - # job reported green. - - name: Surface a failed comment - if: always() && steps.comment.outcome == 'failure' - run: | - echo "::warning::Could not post the breaking change guard comment. The validation verdict is unaffected, but the author gets no explanation of what to add. Check the step log above — a 403 here usually means the job lacks 'pull-requests: write'." - # ----------------- PR Validation Summary Comment ----------------- pr-validation-report: name: PR Validation Report diff --git a/docs/pr-validation.md b/docs/pr-validation.md index af9d86cd..2bb1a90e 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -238,6 +238,18 @@ where that information is worth having. They still skip on drafts, and on anything that is not a `pull_request` event. +## One comment, not two + +The acknowledgement guard used to post its own comment. It no longer does: the +breaking-change gate's comment carries both controls, because two bot comments +about the same breaking change read as a contradiction — one headed by a ✅ for +the author's acknowledgement, one asking for a decision. The ✅ gets taken for +the approval. + +The gate now comments wherever a breaking change is found, including on branches +outside its approval scope. There it reports only the acknowledgement and the +exact line to paste, since that is the only thing being enforced. + ## Breaking change approval The author-acknowledgement guard above asks the author to declare the break. The @@ -286,7 +298,6 @@ Full detail: [`breaking-change-approval.md`](breaking-change-approval.md). | `blocking-checks` | 1 (fail-fast) | guard enforcement, `pr-source-branch`, `pr-title`, `pr-description`, `pr-commit-signatures` | always; drafts run guard enforcement only | | `advisory-checks` | 2 (informational) | `pr-metadata`, `pr-size`, `pr-labels` | non-draft, regardless of the blocking result | | `pr-checks-summary` | — | `pr-checks-summary` | always (writes to step summary) | -| `breaking-change-comment` | — | `actions/github-script` | every non-draft live run, including detector failure or cancellation. Needs `pull-requests: write` — commenting on a PR is authorised as a pull request, not as an issue | | `pr-validation-report` | — | `pr-validation-reporter` | non-draft (upserts single PR comment) | | `notify` | — | `slack-notify.yml` | non-draft, `!dry_run` | diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index 2050fcb2..4af911ef 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -20,6 +20,7 @@ comment, and it ends as the record of who decided what — nothing is deleted. | `state` | What the comment says | |---|---| | `waiting` | A breaking change was found; here is the link to the pending deployment, and what approving or rejecting will do | +| `acknowledgement` | A breaking change was found on a branch outside the approval scope — no decision is needed, only the author's acknowledgement, with the exact line to paste | | `approved` | Approved by whom, and that the next release will be a major version | | `rejected` | Rejected by whom, the reason they gave, and what to do next | | `unresolved` | The approval ended without a recorded decision — open the run | @@ -99,6 +100,9 @@ side effect in the caller is what makes this safe to run with | `reason` | Why the pull request is gated, from `pr-gate-scope`. | No | `''` | | `head-sha` | Head commit the report refers to. | No | `''` | | `acknowledged` | Whether the author acknowledgement line is present. | No | `false` | +| `author-acknowledgement` | The exact line the author must add when it is missing. | No | `''` | +| `fingerprint` | Digest of the breaking-change commit set. Recorded when the state is `approved`, with the run that carried the approval. | No | `''` | +| `run-id` | Run recorded alongside the fingerprint, so a later run can verify the approval against the Environment log. | No | `${{ github.run_id }}` | | `run-url` | Link to the run carrying the pending deployment. | No | `''` | | `reviewers` | Who can answer, as a `team:` / `user:` list. | No | `''` | | `actual-reviewers` | Reviewers read from the Environment itself. Preferred over `reviewers` when present. | No | `''` | diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 8ee8a41c..3349ed7c 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -27,6 +27,10 @@ inputs: description: Whether the author acknowledgement line is present in the description (true/false). Shown to the reviewer as context. required: false default: 'false' + author-acknowledgement: + description: 'The exact line the author must add to the description. Shown when it is missing — a check that fails without saying what to add is a check people route around.' + required: false + default: '' run-url: description: Link to the run carrying the pending deployment. required: false @@ -103,6 +107,7 @@ runs: REASON: ${{ inputs.reason }} HEAD_SHA: ${{ inputs.head-sha }} ACKNOWLEDGED: ${{ inputs.acknowledged }} + ACK_LINE: ${{ inputs.author-acknowledgement }} RUN_URL: ${{ inputs.run-url }} REVIEWERS: ${{ inputs.actual-reviewers != '' && inputs.actual-reviewers || inputs.reviewers }} SERVER_URL: ${{ inputs.server-url }} @@ -120,9 +125,9 @@ runs: set -euo pipefail case "$STATE" in - waiting | approved | rejected | unresolved ) ;; + waiting | approved | rejected | unresolved | acknowledgement ) ;; * ) - echo "state must be one of: waiting, approved, rejected, unresolved; got '${STATE}'." >&2 + echo "state must be one of: waiting, approved, rejected, unresolved, acknowledgement; got '${STATE}'." >&2 exit 1 ;; esac @@ -205,6 +210,15 @@ runs: printf '| %s **Author acknowledgement** | %s |\n\n' \ "$([ "$ACKNOWLEDGED" = "true" ] && echo "✅" || echo "⬜")" \ "$([ "$ACKNOWLEDGED" = "true" ] && echo "present" || echo "missing")" + + # Blocking Checks fails on a missing acknowledgement whether or + # not an approval is pending, so the line to add belongs here too. + if [ "$ACKNOWLEDGED" != "true" ] && [ -n "$ACK_LINE" ]; then + printf '
\nBlocking Checks also needs the author acknowledgement\n\n' + printf 'Add this exact line to the pull request description:\n\n' + printf '```\n%s\n```\n\n' "$ACK_LINE" + printf '
\n\n' + fi ;; approved) printf '> [!NOTE]\n' @@ -226,6 +240,23 @@ runs: fi printf '\n' ;; + acknowledgement) + # No approval is required here — only the author's own + # acknowledgement, which Blocking Checks enforces everywhere. + if [ "$ACKNOWLEDGED" = "true" ]; then + printf '> [!NOTE]\n' + printf '> ## 💥 Breaking change acknowledged\n' + printf '>\n' + printf '> The author has declared this break in the description. No approval is required on this branch.\n\n' + else + printf '> [!IMPORTANT]\n' + printf '> ## 💥 Breaking change — acknowledgement missing\n' + printf '>\n' + printf '> This pull request carries a breaking change, so the next release becomes a **major** version. Add this exact line to the description:\n' + printf '>\n' + printf '> ```\n> %s\n> ```\n\n' "$ACK_LINE" + fi + ;; unresolved) printf '> [!WARNING]\n' printf '> ## 💥 Breaking change — decision not recorded\n' diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 04ae60fc..0f1c6ce5 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -240,6 +240,9 @@ def render_reporter(state, **overrides): "REASON": "rule develop <- '*'", "HEAD_SHA": "abc1234567", "ACKNOWLEDGED": "false", + "ACK_LINE": "Breaking change acknowledged: I understand that this PR " + "intentionally introduces a breaking change and requires the next " + "release to be a major version.", "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", "REVIEWERS": "team:devops", "SERVER_URL": "https://github.com", @@ -644,22 +647,30 @@ def test_guard_uses_fixed_author_acknowledgement_exact_mode_and_released_action( self.assertIn("does not grant maintainer permission", outputs) def test_detector_and_comment_acknowledgement_literals_are_identical(self): + # One literal tells the author what to paste, the other decides whether + # what they pasted counts. A drift between them leaves the gate teaching + # a line that fails the check it is explaining. detector_step = extract_step( extract_job(WORKFLOW, "breaking-change-guard"), "Detect breaking changes" ) - comment_job = extract_job(WORKFLOW, "breaking-change-comment") detector_literal = re.search( r"(?m)^\s*breaking-change-acknowledgement: '(?P[^']*)'$", detector_step, ) - comment_literal = re.search( - r"(?m)^\s*ACKNOWLEDGEMENT: '(?P[^']*)'$", comment_job - ) self.assertIsNotNone(detector_literal) - self.assertIsNotNone(comment_literal) self.assertEqual(ACKNOWLEDGEMENT, detector_literal.group("value")) + + # The gate is handed the same literal explicitly — relying on its + # default to coincide is what made these drift-prone in the first place. + gate_job = extract_job(WORKFLOW, "breaking-change-approval") + gate_literal = re.search( + r"(?m)^\s*author_acknowledgement: '(?P[^']*)'$", gate_job + ) + self.assertIsNotNone( + gate_literal, "pr-validation must pass the acknowledgement explicitly" + ) self.assertEqual( - detector_literal.group("value"), comment_literal.group("value") + detector_literal.group("value"), gate_literal.group("value") ) def test_guard_checkout_has_full_history_without_persisted_credentials(self): @@ -686,58 +697,6 @@ def test_draft_pull_requests_still_reach_guard_enforcement(self): self.assertRegex(enforcement, r"(?m)^ if: always\(\)$") self.assertIn("github.event.pull_request.draft != true", collection) - def test_comment_job_is_write_minimal_and_does_not_checkout(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - permissions = yaml.safe_load(WORKFLOW)["jobs"]["breaking-change-comment"][ - "permissions" - ] - # `pull-requests: write` is the minimum that works, not a widening. - # This set used to be {pull-requests: read, issues: write}, which looks - # narrower and is simply wrong: commenting on a pull request is - # authorised as a pull request, so the step took a 403 on every run - # from 2026-08-06 and reported success anyway. - # - # The lesson is in the shape of the old assertion, not just its value — - # it pinned a permission set nobody had watched work, and so kept the - # bug in place rather than catching it. - self.assertEqual({"pull-requests": "write"}, permissions) - self.assertNotIn("contents", permissions) - self.assertNotIn("actions/checkout", job) - self.assertIn("github-token: ${{ github.token }}", job) - - def test_comment_job_owns_only_exact_bot_marker_comments(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - self.assertIn("comment.user?.login === 'github-actions[bot]'", job) - self.assertIn("comment.body === marker", job) - self.assertIn("comment.body?.startsWith(`${marker}\\n`)", job) - self.assertNotIn("comment.body?.includes(marker)", job) - self.assertEqual(2, job.count("const body = `${marker}\\n")) - - def test_comment_job_uses_author_acknowledgement_not_authorization(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - self.assertIn("'✅ Author acknowledged'", job) - self.assertIn("'🚫 Awaiting author acknowledgement'", job) - self.assertIn("does not grant maintainer permission", job) - self.assertNotIn("'✅ Approved'", job) - self.assertNotIn("'🚫 Awaiting approval'", job) - - def test_comment_job_rejects_stale_head_updates(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - self.assertIn("EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }}", job) - self.assertIn("github.rest.pulls.get", job) - self.assertIn("current.data.head.sha !== eventHeadSha", job) - - def test_comment_job_reconciles_detector_failure(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - self.assertIn("if (!guardJobSucceeded || !detectionSucceeded)", job) - self.assertIn("**Status:** ⚠️ Detection failed", job) - self.assertIn("Blocking Checks failed closed.", job) - failure_branch = job.index("if (!guardJobSucceeded || !detectionSucceeded)") - no_breaking_branch = job.index("if (!hasBreakingChanges)") - self.assertIn( - "await upsertOwned(body);", job[failure_branch:no_breaking_branch] - ) - def test_reporter_and_summary_receive_guard_and_blocking_runtime_results(self): for job_id, step_name in ( ("pr-checks-summary", "PR Checks Summary"), @@ -877,15 +836,6 @@ def test_jobs_that_comment_on_a_pull_request_can_write_to_it(self): f"{job_id} comments on a pull request and would take a 403", ) - def test_a_failed_comment_is_visible_rather_than_swallowed(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - # continue-on-error is right for a cosmetic comment, but silence is how - # a 403 ran for two months with every job green. - self.assertIn("continue-on-error: true", job) - self.assertIn("::warning::", job) - self.assertIn("steps.comment.outcome == 'failure'", job) - - class AdvisoryChecksTests(unittest.TestCase): def test_advisory_checks_do_not_cascade_off_a_blocking_failure(self): job = extract_job(WORKFLOW, "advisory-checks") From 691eb27ff002cc8053f6b5ab2cd52b0e106882c6 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:27:28 -0300 Subject: [PATCH 4/5] refactor(pr-validation): detect through the API, check out nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL flagged `actions/untrusted-checkout` at the second checkout, and the alert was fair. The gate runs on `pull_request`, so it is not privileged — but it was still placing a contributor's tree on disk in a job that carries MANAGE_TOKEN. Nothing executed that tree, and nothing needed it: the only thing read from it was commit messages, which the API serves. The `breaking-change-commits` composite was already running here for the fingerprint, reading exactly those commits through the API with patterns kept identical to the detector. It now does the detecting too, when the caller has not detected already. Two checkouts, the Git-reading composite, the tree, the token exposure and the alert all go together. `breaking-change-guard` remains the canonical detector and still runs in pr-validation over the real commit range — which is the verdict this gate is handed in the path that matters. One consequence worth stating: on the standalone path the acknowledgement is no longer evaluated, because the detector that reads the description lives in pr-validation. The state is reported as unknown rather than missing, and the comment omits the row. Telling an author to add a line that no check is asking for is worse than saying nothing. --- .../workflows/breaking-change-approval.yml | 71 ++++++------------- docs/breaking-change-approval.md | 19 +++-- .../action.yml | 16 +++-- .../breaking-change-guard/test-workflow.py | 17 +++++ 4 files changed, 65 insertions(+), 58 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index b36887d6..25cf40e2 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -246,49 +246,25 @@ jobs: echo "::notice::PR #${PR_NUMBER} (${HEAD_REF} → ${BASE_REF}) is not gated — ${REASON}" fi - # The composite reads Git history, so it needs `origin/` and a HEAD - # that is the pull request. The second checkout exists because the first - # one leaves HEAD on the event's commit, which is only the pull request - # under `pull_request`. `clean: false` keeps what the first one fetched. - - name: Check out full history - id: checkout - if: steps.scope.outputs.in-scope == 'true' && inputs.detected_breaking_changes == '' - continue-on-error: true - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v6 - with: - fetch-depth: 0 - persist-credentials: false - - - name: Move HEAD onto the pull request head - id: checkout-head - if: steps.scope.outputs.in-scope == 'true' && steps.checkout.outcome == 'success' - continue-on-error: true - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v6 - with: - ref: refs/pull/${{ steps.scope.outputs.pr-number }}/head - fetch-depth: 0 - clean: false - persist-credentials: false - - # Nothing from the checked-out tree is executed — the composite only reads - # commit messages — so fork code never runs here. - - name: Detect breaking changes - id: guard - if: steps.scope.outputs.in-scope == 'true' && steps.checkout-head.outcome == 'success' && inputs.detected_breaking_changes == '' - continue-on-error: true - uses: LerianStudio/github-actions-shared-workflows/src/validate/breaking-change-guard@v1 - with: - base-ref: ${{ steps.scope.outputs.base-ref }} - breaking-change-acknowledgement: ${{ inputs.author_acknowledgement }} - acknowledgement-match-mode: exact-visible-line - # Which commits tripped the gate, and a digest of the set. The digest is # what lets an approval survive a push that did not touch the breaking # change — see the `request` job. + # + # This also detects, when the caller did not. It replaced a pair of + # checkouts and the Git-reading composite: CodeQL flagged them as + # `actions/untrusted-checkout`, and it was right to — a non-privileged + # workflow was putting a contributor's tree on disk beside MANAGE_TOKEN. + # Nothing executed it, but nothing needed it either. Reading the API + # removes the tree, the token exposure and the alert together. + # + # `breaking-change-guard` stays the canonical detector and still runs in + # pr-validation over the real commit range; the patterns here are kept + # identical to it and tested against the same negative cases. - name: List the breaking-change commits id: commits if: steps.scope.outputs.in-scope == 'true' continue-on-error: true + # No checkout: it reads the pull request through the API. uses: LerianStudio/github-actions-shared-workflows/src/validate/breaking-change-commits@develop with: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} @@ -301,11 +277,8 @@ jobs: IN_SCOPE: ${{ steps.scope.outputs.in-scope }} SUPPLIED_HAS_BREAKING_CHANGES: ${{ inputs.detected_breaking_changes }} SUPPLIED_ACKNOWLEDGED: ${{ inputs.detected_acknowledged }} - CHECKOUT_OUTCOME: ${{ steps.checkout.outcome }} - CHECKOUT_HEAD_OUTCOME: ${{ steps.checkout-head.outcome }} - GUARD_OUTCOME: ${{ steps.guard.outcome }} - RAW_HAS_BREAKING_CHANGES: ${{ steps.guard.outputs.has-breaking-changes }} - RAW_ACKNOWLEDGED: ${{ steps.guard.outputs.approved }} + COMMITS_OUTCOME: ${{ steps.commits.outcome }} + BREAKING_COUNT: ${{ steps.commits.outputs.count }} run: | set -euo pipefail @@ -345,15 +318,17 @@ jobs: echo "detected_breaking_changes must be 'true', 'false', 'unknown' or empty; got '${SUPPLIED_HAS_BREAKING_CHANGES}'." >&2 ;; esac - elif [ "$CHECKOUT_OUTCOME" = "success" ] && [ "$CHECKOUT_HEAD_OUTCOME" = "success" ] \ - && [ "$GUARD_OUTCOME" = "success" ] \ - && { [ "$RAW_HAS_BREAKING_CHANGES" = "true" ] || [ "$RAW_HAS_BREAKING_CHANGES" = "false" ]; } \ - && { [ "$RAW_ACKNOWLEDGED" = "true" ] || [ "$RAW_ACKNOWLEDGED" = "false" ]; }; then - has_breaking_changes="$RAW_HAS_BREAKING_CHANGES" - acknowledged="$RAW_ACKNOWLEDGED" + elif [ "$COMMITS_OUTCOME" = "success" ] && [ -n "$BREAKING_COUNT" ]; then + [ "$BREAKING_COUNT" -gt 0 ] && has_breaking_changes=true detection_succeeded=true + # Left empty on purpose, not false. Without the caller's verdict + # the acknowledgement was never evaluated here — the detector that + # reads the description lives in pr-validation — and reporting + # "missing" for something nobody looked at would send the author + # after a line no check is asking for. + acknowledged="" else - echo "Breaking change detection failed: checkout=${CHECKOUT_OUTCOME} checkout_head=${CHECKOUT_HEAD_OUTCOME} guard=${GUARD_OUTCOME}" >&2 + echo "Breaking change detection failed: commits=${COMMITS_OUTCOME} count=${BREAKING_COUNT:-missing}" >&2 fi { diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 1616cdef..67e75745 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -52,7 +52,7 @@ own: | Composite | Responsibility | |---|---| | [`pr-gate-scope`](../src/validate/pr-gate-scope/README.md) | Is this pull request in scope, and why — base → head rules, label overrides, author vetoes | -| [`breaking-change-guard`](../src/validate/breaking-change-guard/README.md) | Does the branch carry a breaking-change commit | +| [`breaking-change-guard`](../src/validate/breaking-change-guard/README.md) | The canonical detector, over the real commit range. Runs in `pr-validation.yml`, whose verdict this gate is handed | | [`breaking-change-commits`](../src/validate/breaking-change-commits/README.md) | Which commits carry the break, and a digest of the set | | [`codeowners-reviewers`](../src/validate/codeowners-reviewers/README.md) | Who owns this repository, per its CODEOWNERS | | [`environment-protection`](../src/validate/environment-protection/README.md) | Does `breaking-approval` exist with required reviewers — creating it when it does not | @@ -177,10 +177,19 @@ jobs: MANAGE_TOKEN: ${{ secrets.MANAGE_TOKEN }} ``` -Called this way the workflow detects on its own, with two checkouts: a plain one -for the `origin/` remote-tracking refs, then `refs/pull//head` with -`clean: false` to move `HEAD` onto the pull request. Nothing from the tree is -executed — only commit messages are read — so fork code never runs. +Called this way the workflow detects on its own, through the API — it checks +out nothing at all. + +It used to use two checkouts and read Git directly. CodeQL flagged that as +`actions/untrusted-checkout`, and the alert was fair: a non-privileged workflow +was placing a contributor's tree on disk next to `MANAGE_TOKEN`. Nothing ever +executed it, but nothing needed it either, and the tree, the token exposure and +the alert all went away together. + +The acknowledgement is **not** evaluated on this path — the detector that reads +the pull request description runs in `pr-validation.yml`. The comment omits that +row rather than reporting it missing, because telling an author to add a line no +check is asking for is worse than saying nothing. ### Testing before adoption diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 3349ed7c..e6412c68 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -24,7 +24,7 @@ inputs: required: false default: '' acknowledged: - description: Whether the author acknowledgement line is present in the description (true/false). Shown to the reviewer as context. + description: 'Whether the author acknowledgement line is present in the description — `true`, `false`, or empty when nobody evaluated it. Empty omits the row entirely: reporting "missing" for something unchecked sends the author after a line no check is asking for.' required: false default: 'false' author-acknowledgement: @@ -207,13 +207,19 @@ runs: printf '| | |\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")" + # Empty means nobody evaluated it — only the caller that enforces + # the acknowledgement can say. Printing "missing" there would send + # the author after a line no check is asking for. + if [ -n "$ACKNOWLEDGED" ]; then + printf '| %s **Author acknowledgement** | %s |\n' \ + "$([ "$ACKNOWLEDGED" = "true" ] && echo "✅" || echo "⬜")" \ + "$([ "$ACKNOWLEDGED" = "true" ] && echo "present" || echo "missing")" + fi + printf '\n' # Blocking Checks fails on a missing acknowledgement whether or # not an approval is pending, so the line to add belongs here too. - if [ "$ACKNOWLEDGED" != "true" ] && [ -n "$ACK_LINE" ]; then + if [ "$ACKNOWLEDGED" = "false" ] && [ -n "$ACK_LINE" ]; then printf '
\nBlocking Checks also needs the author acknowledgement\n\n' printf 'Add this exact line to the pull request description:\n\n' printf '```\n%s\n```\n\n' "$ACK_LINE" diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 0f1c6ce5..d731c86c 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1202,6 +1202,23 @@ def test_the_approval_record_survives_the_next_run(self): # Only an approval writes a new record. self.assertIn('if [ "$STATE" = "approved" ] && [ -n "$FINGERPRINT" ]', reporter) + def test_the_gate_checks_out_nothing(self): + # CodeQL flagged `actions/untrusted-checkout` here, and it was right: + # a non-privileged workflow was putting a contributor's tree on disk + # beside MANAGE_TOKEN. Nothing executed it, but nothing needed it — + # the commits are readable through the API. + self.assertNotIn("actions/checkout", APPROVAL_WORKFLOW) + + def test_an_unevaluated_acknowledgement_is_not_reported_as_missing(self): + # The detector that reads the description lives in pr-validation. Where + # it did not run, the state is unknown — and "missing" would send the + # author after a line no check is asking for. + rendered = render_reporter("waiting", ACKNOWLEDGED="") + self.assertNotIn("Author acknowledgement", rendered) + rendered = render_reporter("waiting", ACKNOWLEDGED="false") + self.assertIn("Author acknowledgement", rendered) + self.assertIn("missing", rendered) + 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 ff8d64caffbb12ae85c795b999d6285aeab5e598 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:57:39 -0300 Subject: [PATCH 5/5] test(pr-validation): probe the two-label lifecycle on a live PR --- docs/breaking-change-approval.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 5711951c..db01c8c6 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -540,3 +540,5 @@ python3 src/validate/breaking-change-guard/test-workflow.py + +