From 32a82923e2a71514129ac49649f601f5191911aa Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 09:39:50 -0300 Subject: [PATCH 1/2] fix(pr-validation): give the comment job permission to comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The breaking change guard comment has never been posted. Every run since 2026-08-06 took a 403 and reported success: RequestError [HttpError]: Resource not accessible by integration POST /repos/{owner}/{repo}/issues/{n}/comments The job declared `pull-requests: read` with `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. A job-level block replaces the workflow's rather than merging with it, so declaring `read` there was not least privilege; it was below what the step needs to do anything. The cost was not cosmetic. An author who omits the acknowledgement line sees Blocking Checks fail with no explanation, because the comment that would explain it is the one that never arrives. Two months of silence were possible for two reasons, both addressed: the step is continue-on-error, so the 403 never surfaced. It stays — losing a comment should not fail validation — but a failed post now emits a ::warning:: instead of vanishing a test pinned the permission set to exactly {pull-requests: read, issues: write}. It did not merely miss the bug, it held it in place. Swept the other workflows for the same shape: frontend-pr-analysis and go-pr-coverage-comment both comment on pull requests and both inherit `pull-requests: write` from their workflow level, so they are fine. A test now covers every job that comments, so the next one cannot repeat this. --- .github/workflows/pr-validation.yml | 22 ++++++- docs/pr-validation.md | 2 +- .../breaking-change-guard/test-workflow.py | 62 ++++++++++++++++--- 3 files changed, 74 insertions(+), 12 deletions(-) diff --git a/.github/workflows/pr-validation.yml b/.github/workflows/pr-validation.yml index 6d416f17..36f1c6e1 100644 --- a/.github/workflows/pr-validation.yml +++ b/.github/workflows/pr-validation.yml @@ -490,12 +490,21 @@ jobs: 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: read - issues: write + 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: @@ -574,6 +583,15 @@ jobs: 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 f01a9d6f..af9d86cd 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -286,7 +286,7 @@ 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 | +| `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/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index f6e2ab6b..05545707 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -686,15 +686,20 @@ def test_draft_pull_requests_still_reach_guard_enforcement(self): def test_comment_job_is_write_minimal_and_does_not_checkout(self): job = extract_job(WORKFLOW, "breaking-change-comment") - permissions = re.search( - r"(?m)^ permissions:\n(?P(?: [^\n]+\n?)+)", job - ) - self.assertIsNotNone(permissions) - self.assertEqual( - {"pull-requests: read", "issues: write"}, - {line.strip() for line in permissions.group("body").splitlines()}, - ) - self.assertNotIn("contents:", permissions.group("body")) + 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) @@ -840,6 +845,45 @@ def test_analysis_workflows_expose_scoped_test_runner_overrides(self): ) +class CommentPermissionTests(unittest.TestCase): + def test_jobs_that_comment_on_a_pull_request_can_write_to_it(self): + # Commenting on a pull request needs `pull-requests: write`. The + # issue-comments endpoint is the one used, but GitHub authorises the + # subject as a pull request, so `issues: write` is not enough — it + # covers real issues only. The mistake fails as a 403 at runtime, and + # these steps are continue-on-error, so it reports success. + workflow = yaml.safe_load(WORKFLOW) + top_level = workflow.get("permissions") or {} + + for job_id, job in workflow["jobs"].items(): + if not isinstance(job, dict): + continue + body = yaml.safe_dump(job) + if not any( + marker in body + for marker in ("createComment", "updateComment", "issues/comments") + ): + continue + + with self.subTest(job=job_id): + # A job-level block replaces the workflow's, it does not merge, + # so an explicit block has to carry the permission itself. + effective = job.get("permissions") or top_level + self.assertEqual( + "write", + effective.get("pull-requests"), + 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 4a06ad2bc9cbce5be75edd80b81d0a7295465383 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 15:42:43 -0300 Subject: [PATCH 2/2] docs(pr-validation): state the continue-on-error exception to the dry_run rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the CodeRabbit review on #913, which flagged the ::warning:: this PR adds as a breach of "dry_run: false must be silent". Keeping the warning: the rule's own wording is "let failures surface via exit codes", and a continue-on-error step has deliberately given that channel up. With no exit code and no annotation a failure is invisible — which is precisely how the 403 this PR fixes ran for two months with every job green. Twenty-two steps already in this repository annotate failures on a real run, so the practice predates the question. What the review did expose is that the rule never says any of this. It does now, with the case that proves it. --- .agents/skills/workflow/SKILL.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.agents/skills/workflow/SKILL.md b/.agents/skills/workflow/SKILL.md index d48ed4bd..552a3c91 100644 --- a/.agents/skills/workflow/SKILL.md +++ b/.agents/skills/workflow/SKILL.md @@ -211,6 +211,21 @@ The two modes have opposite goals — design them accordingly: **Real run (`false`):** no extra `echo`, no debug flags, let failures surface via exit codes, one `::notice::` summary on success at most. +**Exception — `continue-on-error` steps must annotate their own failures.** The +rule above leans on the exit code to carry a failure. A step that deliberately +swallows its exit code has no such channel, so a failure there is invisible: the +job reports success and nobody looks at the log. Emit `::warning::` on the +failure path. + +This is not hypothetical. `pr-validation.yml`'s breaking-change comment took a +403 on every run from 2026-08-06 and reported success each time, because the +step was `continue-on-error` and said nothing. The comment it posts explains to +an author what to add; for two months it never arrived, and the only symptom was +a blocking check failing with no explanation. + +Gate the annotation on the failure itself (`if: steps..outcome == 'failure'`) +so a successful run stays silent. + ## Step section titles When a job has more than one logical group of steps, separate each group with a titled section comment: