diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index ebd7156e..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 @@ -314,9 +287,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 @@ -336,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 { @@ -510,6 +494,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 +526,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/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/docs/pr-validation.md b/docs/pr-validation.md index af9d86cd..ffcd66c1 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -210,14 +210,14 @@ 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: +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. -- 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. +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 @@ -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..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`. @@ -24,9 +24,13 @@ 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: + 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 @@ -202,9 +207,24 @@ 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" = "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" + printf '
\n\n' + fi ;; approved) printf '> [!NOTE]\n' @@ -226,6 +246,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..d731c86c 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") @@ -1252,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)