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..f01a9d6f 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 @@ -271,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) | @@ -282,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 @@ -325,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 80c63546..7cc94227 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -778,6 +778,43 @@ 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. + # + # 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): + self.assertIn("advisory-checks", jobs[job_id]["needs"]) + self.assertIn("always()", jobs[job_id]["if"]) + + class ApprovalGateStructureTests(unittest.TestCase): """The human approval gate — wiring that must not drift.