From 8b03f84620d11376b033f1094b09fcbb72fdf2d2 Mon Sep 17 00:00:00 2001 From: amirbena Date: Thu, 24 Sep 2026 20:25:50 +0300 Subject: [PATCH] Make repository validation PR-only (#538) Drop validate.yml's push-to-main trigger so a merge no longer re-runs the FULL suite and cross-platform Skill-tree builds; pushes to main stay reserved for release-publish.yml. Update the docs that promised FULL on every push, and pin the pull_request-only trigger in the routing test. Co-Authored-By: Claude Opus 5.5 --- .github/AUTOMATION.md | 5 +++-- .github/workflows/validate.yml | 8 ++++---- README.md | 3 ++- policies/validation-and-clean-exit.md | 13 ++++++++++--- tests/policy/governance/test_ci_test_routing.py | 7 +++---- 5 files changed, 22 insertions(+), 14 deletions(-) diff --git a/.github/AUTOMATION.md b/.github/AUTOMATION.md index 8290267..3c258c8 100644 --- a/.github/AUTOMATION.md +++ b/.github/AUTOMATION.md @@ -21,7 +21,7 @@ workflows change. | Workflow | File | Trigger | Responsibility | GitHub state | | --- | --- | --- | --- | --- | -| Validate repository | [`workflows/validate.yml`](workflows/validate.yml) | `pull_request`, `push` to `main` | `test` (required): set up Python 3.13, extract the trusted router (`scripts/validation/ci_test_route.py`) from the PR base SHA through a separate blobless clone and pick the FAST or FULL tier (FULL by default, on any error, and on every push to `main`), validate both Skills' metadata (`scripts/validation/validate-skill-metadata.py`), build and spec-validate the Skill trees, then run `python -m unittest discover -s tests -t .` (FULL) or the same discovery minus `tests.integration.*` (FAST). Skill-tree shell/PowerShell parity on both tiers | Read-only (`contents: read`) | +| Validate repository | [`workflows/validate.yml`](workflows/validate.yml) | `pull_request` | `test` (required): set up Python 3.13, extract the trusted router (`scripts/validation/ci_test_route.py`) from the PR base SHA through a separate blobless clone and pick the FAST or FULL tier (FULL by default and on any error), validate both Skills' metadata (`scripts/validation/validate-skill-metadata.py`), build and spec-validate the Skill trees, then run `python -m unittest discover -s tests -t .` (FULL) or the same discovery minus `tests.integration.*` (FAST). Skill-tree shell/PowerShell parity on both tiers | Read-only (`contents: read`) | | Validate PR description length | [`workflows/pr-description-length.yml`](workflows/pr-description-length.yml) | `pull_request` (opened, edited, synchronize) | Check out the trusted validator from the PR base SHA (bootstrapping from head only for the PR that introduces the script), enforce the useful-content limit and the canonical PR-template structure via `scripts/validation/pr_description_length.py` | Read-only (`contents: read`) | | Sync Engineering Task labels | [`workflows/sync-issue-labels.yml`](workflows/sync-issue-labels.yml) | `issues` (opened, edited) | Compute managed-label changes from the issue body (`scripts/governance/sync_issue_labels.py`), then `gh issue edit` to apply the add/remove set; per-issue `concurrency` with cancel-in-progress | Mutates issue labels (`issues: write`) | | Claim contribution issue | [`workflows/claim-issue.yml`](workflows/claim-issue.yml) | `issue_comment` (created) | On `/claim` or `/unclaim` on a non-PR issue: check out trusted default-branch automation, read the issue and comment history, plan via `scripts/governance/claim_issue.py` with churn/cooldown thresholds, persist a trusted receipt and a reconciled-state checkpoint comment, then project state onto the `claimed` label; repo-wide serialized `concurrency` queue | Mutates issue comments + the `claimed` label (`issues: write`) | @@ -39,7 +39,8 @@ worthiness` `release-gate` job are the required status checks on the - **`validate.yml`** — Skill metadata validation plus the `tests/` suite, routed per PR: FULL by default, or FAST (omitting only `tests/integration/`) when every changed path is on the router's - allowlist; every push to `main` runs FULL. Contract: + allowlist. Pull-request-only: a push to `main` runs the release + lifecycle (`release-publish.yml`), not this workflow again. Contract: [`../policies/validation-and-clean-exit.md`](../policies/validation-and-clean-exit.md#routed-ci-tests). - **`pr-description-length.yml`** — enforces the PR-description useful-content limit and the canonical PR-template structure (required diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index a3b8eb1..df3de3e 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -1,11 +1,9 @@ name: Validate repository on: + # Pull requests only: repository validation is the pre-merge boundary, and a + # push to main is reserved for release-publish.yml (#538). pull_request: - # Every merged commit runs the full suite (the router resolves any - # non-pull_request event to FULL), so a stale FAST allowlist turns main red. - push: - branches: [main] permissions: contents: read @@ -38,6 +36,8 @@ jobs: run: | route_repo="$RUNNER_TEMP/ci-route" router="$RUNNER_TEMP/ci_test_route.py" + # Defensive: only pull_request triggers this workflow, but any other + # event would still resolve to FULL. if [ "$EVENT_NAME" != "pull_request" ]; then reason="non-pull_request event ($EVENT_NAME)" elif git init -q "$route_repo" \ diff --git a/README.md b/README.md index 50e1f5e..7380fd0 100644 --- a/README.md +++ b/README.md @@ -192,7 +192,8 @@ which remains the authoritative regression gate before merge (see [`policies/git-pr-merge-policy.md`](policies/git-pr-merge-policy.md)). CI routes each PR to FULL (the default) or to FAST, which omits only `tests/integration/` when every changed path is provably not an -integration input; every push to `main` runs FULL +integration input. It runs on pull requests only — a push to `main` +triggers the release lifecycle, not repository validation again (see [`policies/validation-and-clean-exit.md`](policies/validation-and-clean-exit.md#routed-ci-tests)). | Change | Run | diff --git a/policies/validation-and-clean-exit.md b/policies/validation-and-clean-exit.md index 93053d3..e085cf6 100644 --- a/policies/validation-and-clean-exit.md +++ b/policies/validation-and-clean-exit.md @@ -108,6 +108,12 @@ router from the PR's base commit (via a separate blobless clone, so the checkout under test is unchanged), so a PR that edits it (or `validate.yml`) is FULL. +`validate.yml` runs on `pull_request` only. Repository validation is the +pre-merge boundary; a push to `main` does not re-run it, and is reserved +for the release lifecycle in `release-publish.yml`, whose own build, +provenance, distribution, tag, and release verification are unaffected +([#538](https://github.com/amirbena/code-review-skill/issues/538)). + `tests/policy/governance/test_ci_test_routing.py` is a narrow tripwire, not proof that an allowlisted path is inert. On every PR it fails if the shared temp-root copy list (`TEMP_ROOT_INPUTS` in @@ -118,9 +124,10 @@ forms (`joinpath`, `Path(REPO_ROOT, …)`, f-strings), or files read by a script an integration test invokes. Admitting a path to the allowlist therefore stays a maintainer decision made in its own PR with that positive evidence gathered by hand -([#533](https://github.com/amirbena/code-review-skill/issues/533)); the -safety net for a missed consumer is that every push to `main` runs FULL, -so a stale entry surfaces at the first merge that exposes it. +([#533](https://github.com/amirbena/code-review-skill/issues/533)). There +is no post-merge FULL run: a missed consumer surfaces on the next PR that +routes FULL (the default for any change outside the allowlist) and +exercises it. Repository-owned Python that any of these steps touch follows [`python_scripts_coding_policy.md`](python_scripts_coding_policy.md). diff --git a/tests/policy/governance/test_ci_test_routing.py b/tests/policy/governance/test_ci_test_routing.py index 83939f3..f5e7ff3 100644 --- a/tests/policy/governance/test_ci_test_routing.py +++ b/tests/policy/governance/test_ci_test_routing.py @@ -62,10 +62,9 @@ def test_no_path_filters(self) -> None: with self.subTest(event=event): self.assertFalse({"paths", "paths-ignore"} & set(config or {})) - def test_runs_full_on_push_to_main(self) -> None: - on = _on(self.workflow) - self.assertIn("pull_request", on) - self.assertEqual(on["push"], {"branches": ["main"]}) + def test_runs_on_pull_requests_only(self) -> None: + # A push to main is reserved for release-publish.yml (#538). + self.assertEqual(set(_on(self.workflow)), {"pull_request"}) def test_router_runs_from_the_base_sha_outside_the_checkout(self) -> None: test = self.jobs["test"]