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: 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")