From 4addf976d09b7d888541cce83023a3df2e819c90 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 21:49:24 -0300 Subject: [PATCH 1/2] fix(pr-validation): run the advisory checks even when a blocking one fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR Size, Auto Labels and PR Metadata were gated on `needs.blocking-checks.result == 'success'`, so a wrong title or a missing signature took them down too — the summary reported three `skipped` rows next to the failure. That came from the 2-tier fail-fast model and saved a runner at the expense of the pull request that most needed the help. None of the three can block a merge, all three are cheap, and a pull request already in trouble is exactly where a size label and an assignee are worth having. `needs:` stays for ordering; the verdict is no longer consulted. Drafts and non-pull_request events still skip, as before. Safe because none of the three can fail the job — each is continue-on-error — and the summary and report jobs already run with always(), so an advisory checkout failure still gets reported. Tests pin all three facts. --- .github/workflows/pr-validation.yml | 11 ++++++- docs/pr-validation.md | 13 ++++++++ .../breaking-change-guard/test-workflow.py | 33 +++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/.github/workflows/pr-validation.yml b/.github/workflows/pr-validation.yml index b0d45a69..6d416f17 100644 --- a/.github/workflows/pr-validation.yml +++ b/.github/workflows/pr-validation.yml @@ -402,11 +402,20 @@ jobs: commit-signatures-outcome: ${{ steps.commit-signatures.outcome || 'skipped' }} # ----------------- Tier 2: Advisory Checks (shared checkout, depends on Tier 1) ----------------- + # Advisory by definition, so a blocking failure must not take them with it. + # They used to be gated on `needs.blocking-checks.result == 'success'`, from + # the 2-tier fail-fast model, which saved a runner at the cost of the exact + # pull request that needed the help: a wrong title or a missing signature + # meant no size label, no auto-labels and no assignee either. Those three are + # cheap, none of them can block, and they are most useful on a pull request + # that is already in trouble. + # + # `needs:` stays for ordering only. The verdict is no longer consulted. advisory-checks: name: Advisory Checks runs-on: ${{ vars.GENERAL_RUNNERS || inputs.runner_type }} needs: [blocking-checks] - if: always() && github.event_name == 'pull_request' && needs.blocking-checks.result == 'success' && github.event.pull_request.draft != true + if: always() && github.event_name == 'pull_request' && github.event.pull_request.draft != true outputs: metadata-result: ${{ steps.collect.outputs.metadata }} size-result: ${{ steps.collect.outputs.size }} diff --git a/docs/pr-validation.md b/docs/pr-validation.md index 99703145..2abd19cc 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -225,6 +225,19 @@ For every non-draft live run, including detector failure or cancellation, the wo feat fix docs style refactor perf test chore ci build revert ``` +### Advisory checks always run + +PR Size, Auto Labels and PR Metadata are advisory — none of them can block a +merge. They are therefore **not** gated on the blocking tier passing. + +They used to be. The 2-tier fail-fast model skipped them whenever a blocking +check failed, which saved a runner at the cost of the pull request that most +needed the help: a wrong title or a missing signature also meant no size label, +no auto-labels and no assignee. A pull request already in trouble is exactly +where that information is worth having. + +They still skip on drafts, and on anything that is not a `pull_request` event. + ## Breaking change approval The author-acknowledgement guard above asks the author to declare the break. The diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 80c63546..c7d00d14 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -778,6 +778,39 @@ def test_analysis_workflows_expose_scoped_test_runner_overrides(self): ) +class AdvisoryChecksTests(unittest.TestCase): + def test_advisory_checks_do_not_cascade_off_a_blocking_failure(self): + job = extract_job(WORKFLOW, "advisory-checks") + condition = next( + line for line in job.splitlines() if line.strip().startswith("if:") + ) + # Advisory by definition: a wrong title or a missing signature must not + # also cost the pull request its size label, its auto-labels and its + # assignee. Those are most useful precisely on a pull request that is + # already in trouble. + self.assertNotIn("needs.blocking-checks.result", condition) + self.assertIn("always()", condition) + self.assertIn("github.event.pull_request.draft != true", condition) + + def test_advisory_checks_cannot_become_a_new_blocker(self): + job = extract_job(WORKFLOW, "advisory-checks") + # Running them unconditionally is only safe while none of them can fail + # the job — otherwise this change would turn three advisory checks into + # a blocking one. + for step_name in ("Check PR metadata", "Check PR size", "Auto-label PR"): + with self.subTest(step=step_name): + self.assertIn("continue-on-error: true", extract_step(job, step_name)) + + def test_the_reports_survive_an_advisory_failure(self): + # The checkout in advisory-checks has no continue-on-error, so the job + # can still fail. Everything that reports must run anyway. + for job_id in ("pr-checks-summary", "pr-validation-report"): + with self.subTest(job=job_id): + job = extract_job(WORKFLOW, job_id) + self.assertIn("advisory-checks", job) + self.assertIn("always()", job) + + class ApprovalGateStructureTests(unittest.TestCase): """The human approval gate — wiring that must not drift. From 28c54ad9eefc7c1f910a8a052b8555af45f711e9 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 21:58:32 -0300 Subject: [PATCH 2/2] docs(pr-validation): correct the advisory conditions left saying otherwise MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #911. The new section described advisory checks running regardless of the blocking result while two other places in the same file still described the old behaviour — the Jobs table and the Tier 2 bullet. Contradictory documentation is worse than none: whoever reads the table believes it. A third one was not flagged and was wrong too: the Tier 1 bullet said a blocking failure prevents advisory checks from running. Also fixes the test that was supposed to make this change safe. It asserted over the whole job text, so a step — or the comment above the assertion, which contains the word always() — satisfied it while the job-level needs and if said something else. It now reads the parsed job. Verified by dropping advisory-checks from a needs list and watching it fail. --- docs/pr-validation.md | 8 +++++--- src/validate/breaking-change-guard/test-workflow.py | 10 +++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/docs/pr-validation.md b/docs/pr-validation.md index 2abd19cc..f01a9d6f 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -284,7 +284,7 @@ Full detail: [`breaking-change-approval.md`](breaking-change-approval.md). |-----|------|------------|-----------| | `breaking-change-guard` | mandatory detection | `breaking-change-guard@v1` | always, including drafts | | `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, blocking-checks passed | +| `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 | | `pr-validation-report` | — | `pr-validation-reporter` | non-draft (upserts single PR comment) | @@ -295,7 +295,7 @@ Full detail: [`breaking-change-approval.md`](breaking-change-approval.md). - Always enforce both the guard job state and its normalized output, including on drafts - Skip existing source branch, title, description, commit signature, and collector steps on drafts; their non-draft behavior is unchanged - On non-drafts, every validation runs even if one fails (`continue-on-error` per step) -- Job fails if the guard job, normalized guard output, or **any** existing blocking check fails, preventing advisory checks from running +- Job fails if the guard job, normalized guard output, or **any** existing blocking check fails. Advisory checks run either way - In dry-run mode, guard state is logged but does not fail `Blocking Checks` ### Fail-closed detection @@ -338,7 +338,9 @@ The step runs with read-only permissions and emits no commit metadata beyond wha ### Advisory checks (Tier 2) - Share a single `checkout` with `fetch-depth: 0` -- Only run if all blocking checks passed +- Run on every non-draft pull request, **regardless of the blocking result** — a + pull request that failed a blocking check still gets its size label, its + auto-labels and its assignee - Never block merge — informational only ## Dry Run Behavior diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index c7d00d14..7cc94227 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -804,11 +804,15 @@ def test_advisory_checks_cannot_become_a_new_blocker(self): def test_the_reports_survive_an_advisory_failure(self): # The checkout in advisory-checks has no continue-on-error, so the job # can still fail. Everything that reports must run anyway. + # + # Read from the parsed job, not from its text: `assertIn` over the whole + # job body is satisfied by a step, or even by this comment, while the + # job-level `needs` and `if` say something else entirely. + jobs = yaml.safe_load(WORKFLOW)["jobs"] for job_id in ("pr-checks-summary", "pr-validation-report"): with self.subTest(job=job_id): - job = extract_job(WORKFLOW, job_id) - self.assertIn("advisory-checks", job) - self.assertIn("always()", job) + self.assertIn("advisory-checks", jobs[job_id]["needs"]) + self.assertIn("always()", jobs[job_id]["if"]) class ApprovalGateStructureTests(unittest.TestCase):