From c8a971928179287a95050fa186308fe9736e312f 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 1/4] 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 121604a89bcf6821a254737e4fa656d055f030a9 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 2/4] 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 2e417324..be87e051 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 6fda01b233f1c0620edf2d1587570cc17efa8b29 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:41:22 -0300 Subject: [PATCH 3/4] docs(pr-validation): drop the description of a job that no longer exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the CodeRabbit review on #918. "Feedback comments" still described, in ten lines of detail, how the acknowledgement guard creates, updates and deletes its comment — a job this PR removes. It sat directly above the section explaining that the gate owns the single comment, so the file contradicted itself. This is the same mistake I flagged and fixed on #911: adding a section for the new behaviour and leaving the old description in place, where whoever reads it believes it. Also lists `acknowledgement` in the `state` input description. The README already documented it; the action's own contract did not. --- docs/pr-validation.md | 15 +++++++-------- .../breaking-change-approval-reporter/action.yml | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/docs/pr-validation.md b/docs/pr-validation.md index 2bb1a90e..9dbb5dbd 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -210,14 +210,13 @@ The guard is mandatory on every target branch. There is no opt-out. The action s ### Feedback comments -For every non-draft live run, including detector failure or cancellation, the workflow uses `github.token` to manage one best-effort comment owned only when the author is exactly `github-actions[bot]` and the body starts with the exact marker line: - -- A breaking change creates or updates the comment with `Author acknowledged` or `Awaiting author acknowledgement` state and the exact acknowledgement. -- Detection or guard-job failure replaces stale acknowledgement with one `Detection failed` comment and deletes duplicate owned comments. -- Duplicate owned marker comments are deleted. -- When breaking changes disappear, all owned guard comments are deleted. -- Before any comment mutation, the workflow fetches the current PR and stops if its current head SHA differs from the event head SHA. -- Fork token write restrictions and comment API failures never affect enforcement. The workflow never falls back to another bot identity. +The guard posts nothing of its own. Everything an author needs to read about a +breaking change — including the exact acknowledgement line to paste — lives in +the single comment described under [One comment, not two](#one-comment-not-two), +written by the breaking-change gate. + +Enforcement never depends on that comment: a comment API failure leaves the +verdict untouched, and the author still sees `Blocking Checks` fail. ### Default PR Title Types diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index e6412c68..977bddf3 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -13,7 +13,7 @@ inputs: required: false default: ${{ github.repository }} state: - description: "What to report — `waiting`, `approved`, `rejected`, or `unresolved` when the approval ended without a recorded decision. Any other value fails the step." + description: "What to report — `waiting`, `approved`, `rejected`, `unresolved` when the approval ended without a recorded decision, or `acknowledgement` where no approval is required and only the author's acknowledgement is at stake. Any other value fails the step." required: true reason: description: Why the pull request is gated, as resolved by `pr-gate-scope`. From 4590dfe2e9cbffecc7204067ec3ab30842b2657e Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:59:30 -0300 Subject: [PATCH 4/4] docs(pr-validation): stop promising a failure that does not always happen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the CodeRabbit review on #918. The sentence said a comment API failure leaves the verdict untouched "and the author still sees Blocking Checks fail" — but an acknowledged breaking change passes it, as the same file states four paragraphs earlier and `pr-validation.yml:266` implements. The point was that enforcement does not depend on the comment. It now says that, without inventing a consequence that holds in one case out of two. I wrote the sentence in the previous round, while fixing a different inaccuracy in the same section. --- docs/pr-validation.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/pr-validation.md b/docs/pr-validation.md index 9dbb5dbd..ffcd66c1 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -215,8 +215,9 @@ breaking change — including the exact acknowledgement line to paste — lives the single comment described under [One comment, not two](#one-comment-not-two), written by the breaking-change gate. -Enforcement never depends on that comment: a comment API failure leaves the -verdict untouched, and the author still sees `Blocking Checks` fail. +Enforcement never depends on that comment. A comment API failure leaves the +verdict exactly as it was — passing or failing on the acknowledgement alone, +which is what `Blocking Checks` decides either way. ### Default PR Title Types