Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions .github/AUTOMATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ workflows change.

| Workflow | File | Trigger | Responsibility | GitHub state |
| --- | --- | --- | --- | --- |
| Validate repository | [`workflows/validate.yml`](workflows/validate.yml) | `pull_request` | Set up Python 3.13, validate both Skills' metadata (`scripts/validation/validate-skill-metadata.py`), run `python -m unittest discover -s tests` | Read-only (`contents: read`) |
| 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 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`) |
Expand All @@ -36,8 +36,11 @@ Runs on the pull request, read-only. `validate.yml` and the `Release
worthiness` `release-gate` job are the required status checks on the
`main` ruleset ([`../docs/RELEASE.md`](../docs/RELEASE.md)):

- **`validate.yml`** — Skill metadata validation plus the full
`tests/` suite.
- **`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:
[`../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
headings/fields, unresolved placeholders), checking out the validator
Expand Down
40 changes: 40 additions & 0 deletions .github/workflows/validate.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ name: Validate repository

on:
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
Expand All @@ -20,6 +24,33 @@ jobs:
with:
python-version: "3.13"

- name: Route tests (FAST/FULL)
id: route
# The router is trusted from the base, never the head: a PR that
# introduces or edits it is FULL (#533). A separate blobless clone keeps
# the checkout under test unchanged; any failure leaves the tier FULL.
continue-on-error: true
env:
EVENT_NAME: ${{ github.event_name }}
BASE_SHA: ${{ github.event.pull_request.base.sha }}
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
REPO_URL: ${{ github.server_url }}/${{ github.repository }}
run: |
route_repo="$RUNNER_TEMP/ci-route"
router="$RUNNER_TEMP/ci_test_route.py"
if [ "$EVENT_NAME" != "pull_request" ]; then
reason="non-pull_request event ($EVENT_NAME)"
elif git init -q "$route_repo" \
&& git -C "$route_repo" remote add origin "$REPO_URL" \
&& git -C "$route_repo" fetch -q --no-tags --filter=blob:none origin "$BASE_SHA" "$HEAD_SHA" \
&& git -C "$route_repo" show "$BASE_SHA:scripts/validation/ci_test_route.py" > "$router"; then
exec python "$router" route --event-name "$EVENT_NAME" --base "$BASE_SHA" --head "$HEAD_SHA" --repo "$route_repo"
else
reason="no router at the base commit, or the base/head could not be fetched"
fi
printf '### CI test route\n\n- Tier: **FULL**\n- Reason: %s\n' "$reason" >> "$GITHUB_STEP_SUMMARY"
echo "tier=full" >> "$GITHUB_OUTPUT"

- name: Set up Node.js
# The distribution consumer check drives the pinned `skills` CLI (#511).
uses: actions/setup-node@v4
Expand All @@ -45,10 +76,19 @@ jobs:
for tree in dist/skills/*/; do agentskills validate "$tree"; done

- name: Run repository tests
# FULL unless the router positively selected FAST.
if: ${{ steps.route.outputs.tier != 'fast' }}
env:
DISTRIBUTION_INSTALL_CHECK: "1"
run: python -m unittest discover -s tests -t .

- name: Run repository tests except tests.integration (FAST tier)
if: ${{ steps.route.outputs.tier == 'fast' }}
env:
DISTRIBUTION_INSTALL_CHECK: "1"
# The base's router, extracted by the route step, selects the tests.
run: python "$RUNNER_TEMP/ci_test_route.py" run-fast

skill-tree-parity:
name: Skill tree (${{ matrix.shell }} on ${{ matrix.os }})
runs-on: ${{ matrix.os }}
Expand Down
7 changes: 5 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,11 @@ bot replies. Maintainers can tune the three named `CLAIM_*` values in
3. Implement the issue without adding unrelated changes.
4. Run the checks relevant to your change from
[README § Local validation before opening a PR](README.md#local-validation-before-opening-a-pr).
The full test suite is not required locally before every push or PR — it
runs on every PR in CI, which is the authoritative regression gate.
The full test suite is not required locally before every push or PR — CI
is the authoritative regression gate. It runs the full suite unless every
changed path is provably not an integration input, in which case it omits
only `tests/integration/`
([routed CI tests](policies/validation-and-clean-exit.md#routed-ci-tests)).
5. Open a pull request against this repository and use `Fixes #<issue>` when
the pull request should close the issue.

Expand Down
10 changes: 7 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,14 @@ python -m pip install -r requirements-dev.txt

Local validation is **targeted**, not a fixed sequence: run only the checks
relevant to what you changed, for fast feedback before pushing. The full
suite below is **not** required locally before every push or PR — it runs
automatically in [`.github/workflows/validate.yml`](.github/workflows/validate.yml)
on every PR, which remains the authoritative regression gate before merge
suite below is **not** required locally before every push or PR — CI runs
it in [`.github/workflows/validate.yml`](.github/workflows/validate.yml),
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
(see [`policies/validation-and-clean-exit.md`](policies/validation-and-clean-exit.md#routed-ci-tests)).

| Change | Run |
| --- | --- |
Expand Down
49 changes: 44 additions & 5 deletions policies/validation-and-clean-exit.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,51 @@ relevant to the change — see
targeted mapping from change type to command. The full suite
(`python3 -m unittest discover -s tests -t .` plus both metadata
validations, link validation, and packaging) is not a local precondition;
it runs on every PR in `.github/workflows/validate.yml`, which is the
CI runs it through `.github/workflows/validate.yml`, which is the
authoritative regression gate before merge
([`git-pr-merge-policy.md`](git-pr-merge-policy.md)). Running the full
sequence locally beforehand is optional, never required, and appropriate
when a change genuinely spans multiple areas or a shared cross-cutting
contract.
([`git-pr-merge-policy.md`](git-pr-merge-policy.md)), routed as described
under [Routed CI tests](#routed-ci-tests). Running the full sequence
locally beforehand is optional, never required, and appropriate when a
change genuinely spans multiple areas or a shared cross-cutting contract.

### Routed CI tests

The required `test` job in `validate.yml` routes every PR to exactly one
tier. **FULL is the default**:

- **FULL** runs the full suite exactly as above
(`python -m unittest discover -s tests -t .`).
- **FAST** runs the same discovery minus exactly the tests whose ID starts
with `tests.integration.`. Every unit, policy, and repository test — and
any new top-level test directory — still runs, as do metadata
validation, the canonical build, Agent Skills spec validation, and the
shell/PowerShell parity jobs. FAST never removes a contract or
correctness test.

A PR is FAST only when **every** changed path (a three-dot merge-base
diff, renames counted as both paths) is on the allowlist in
[`../scripts/validation/ci_test_route.py`](../scripts/validation/ci_test_route.py),
the single canonical home of that list. Every entry must have positive,
repository-backed evidence that no integration test copies, reads, or
packages it; anything unknown, mixed, empty, or erroring is FULL, and
there is no label or flag that selects FAST. The `test` job extracts the
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.

`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
`tests/integration/packaging/_shared.py`) or a literal
`REPO_ROOT / "…"` read under `tests/integration/` overlaps the allowlist.
It does not see an integration test's own inline copy list, other path
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.

Repository-owned Python that any of these steps touch follows
[`python_scripts_coding_policy.md`](python_scripts_coding_policy.md).
Expand Down
161 changes: 161 additions & 0 deletions scripts/validation/ci_test_route.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
#!/usr/bin/env python3
"""Route a CI run to the FAST or FULL test tier from its changed paths.

FULL is the default. FAST omits only `tests.integration.*`, and only when every
changed path is on the allowlist below; see policies/validation-and-clean-exit.md.
"""

from __future__ import annotations

import argparse
import os
import subprocess
import sys
import unittest
from dataclasses import dataclass
from pathlib import Path
from typing import Iterator, Sequence

FAST = "fast"
FULL = "full"

# Exact, case-sensitive matches. Each entry needs positive evidence that no
# integration test copies, reads, or packages it (issue #533).
FAST_FILES = frozenset(
{
".gitignore",
"AGENTS.md",
"CLAUDE.md",
"CONTRIBUTING.md",
"SECURITY.md",
"README.md",
".github/PULL_REQUEST_TEMPLATE.md",
".github/AUTOMATION.md",
}
)
FAST_DIRS = ("policies/", ".github/ISSUE_TEMPLATE/")

INTEGRATION_PREFIX = "tests.integration."


@dataclass(frozen=True)
class Route:
tier: str
reason: str
first_full_path: str | None = None


def is_fast_path(path: str) -> bool:
return path in FAST_FILES or any(path.startswith(d) and len(path) > len(d) for d in FAST_DIRS)


def classify(paths: Sequence[str]) -> Route:
if not paths:
return Route(FULL, "empty change set")
for path in paths:
if not is_fast_path(path):
return Route(FULL, "a changed path is not on the FAST allowlist", path)
return Route(FAST, "every changed path is on the FAST allowlist")


def changed_paths(repo: Path, base: str, head: str) -> list[str]:
# Three-dot: the PR's own changes since the merge-base; renames split into D + A.
out = subprocess.run(
["git", "-C", str(repo), "diff", "--name-only", "--no-renames", "-z", f"{base}...{head}"],
capture_output=True,
check=True,
).stdout
return [p for p in out.decode("utf-8", "surrogateescape").split("\0") if p]


def route(event_name: str, repo: Path, base: str | None, head: str | None) -> Route:
if event_name != "pull_request":
return Route(FULL, f"non-pull_request event ({event_name or 'unknown'})")
if not base or not head:
return Route(FULL, "missing base or head SHA")
try:
paths = changed_paths(repo, base, head)
except (OSError, subprocess.CalledProcessError):
return Route(FULL, "git diff failed")
return classify(paths)


def safe_route(event_name: str, repo: Path, base: str | None, head: str | None) -> Route:
try:
result = route(event_name, repo, base, head)
except Exception as exc: # noqa: BLE001 - any router failure must resolve to FULL
return Route(FULL, f"router exception ({type(exc).__name__})")
if result.tier not in (FAST, FULL):
return Route(FULL, f"unrecognized tier {result.tier!r}")
return result


def summary_markdown(result: Route) -> str:
lines = ["### CI test route", "", f"- Tier: **{result.tier.upper()}**", f"- Reason: {result.reason}"]
if result.first_full_path is not None:
lines.append(f"- First path that forced FULL: `{result.first_full_path}`")
return "\n".join(lines) + "\n"


def _append(path: str | None, text: str) -> None:
if path:
with open(path, "a", encoding="utf-8") as handle:
handle.write(text)


def _iter_tests(suite: unittest.TestSuite) -> Iterator[unittest.TestCase]:
for item in suite:
if isinstance(item, unittest.TestSuite):
yield from _iter_tests(item)
else:
yield item


def fast_suite(start_dir: str | Path = "tests", top_level_dir: str | Path = ".") -> unittest.TestSuite:
# Same discovery as FULL, minus exactly the integration tests.
discovered = unittest.defaultTestLoader.discover(str(start_dir), top_level_dir=str(top_level_dir))
return unittest.TestSuite(t for t in _iter_tests(discovered) if not t.id().startswith(INTEGRATION_PREFIX))


def _cmd_route(args: argparse.Namespace) -> int:
result = safe_route(args.event_name, Path(args.repo), args.base, args.head)
print(summary_markdown(result), end="")
_append(args.step_summary, summary_markdown(result))
# Last, so a failure anywhere above leaves the tier unset, which runs FULL.
_append(args.github_output, f"tier={result.tier}\n")
return 0


def _cmd_run_fast(args: argparse.Namespace) -> int:
suite = fast_suite()
if args.list:
for test in _iter_tests(suite):
print(test.id())
return 0
result = unittest.TextTestRunner().run(suite)
return 0 if result.wasSuccessful() and result.testsRun > 0 else 1


def main(argv: Sequence[str] | None = None) -> int:
parser = argparse.ArgumentParser(description=__doc__)
commands = parser.add_subparsers(dest="command", required=True)

route_cmd = commands.add_parser("route", help="classify the change set and emit tier=fast|full")
route_cmd.add_argument("--event-name", required=True)
route_cmd.add_argument("--base")
route_cmd.add_argument("--head")
route_cmd.add_argument("--repo", default=".")
route_cmd.add_argument("--github-output", default=os.environ.get("GITHUB_OUTPUT"))
route_cmd.add_argument("--step-summary", default=os.environ.get("GITHUB_STEP_SUMMARY"))
route_cmd.set_defaults(func=_cmd_route)

fast_cmd = commands.add_parser("run-fast", help="run tests/ from the repository root minus tests.integration.*")
fast_cmd.add_argument("--list", action="store_true", help="print the FAST test IDs instead of running them")
fast_cmd.set_defaults(func=_cmd_run_fast)

args = parser.parse_args(argv)
return args.func(args)


if __name__ == "__main__":
sys.exit(main())
4 changes: 4 additions & 0 deletions tests/integration/packaging/_shared.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ def _reference_module_path(name: str) -> Path:
GITHUB_SKILL_DIR = REPO_ROOT / "skills" / "github-pr-review"
DIST_DIR = REPO_ROOT / "dist"

# Repository paths copied into the temp build roots of the packaging and
# release version tests; the CI routing guard keeps these off the FAST allowlist.
TEMP_ROOT_INPUTS = ("skills", "shared", "scripts", "capabilities", "docs", "LICENSE", "CHANGELOG.md")

# The reference modules this test guards — none is a runtime dependency.
REFERENCE_TEST_MODULES = (
"current_evidence.py",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import tests.unit.package_domain._shared # noqa: F401 - sys.path wiring

from package_domain.version import frontmatter_version, newest_release_version
from tests.integration.packaging._shared import TEMP_ROOT_INPUTS
from tests.support.paths import REPO_ROOT

PACKAGE = "scripts/packaging/package-skills.sh"
Expand All @@ -45,7 +46,7 @@ def setUp(self) -> None:
tmp = tempfile.TemporaryDirectory()
self.addCleanup(tmp.cleanup)
self.root = Path(tmp.name)
for rel in ("skills", "shared", "scripts", "capabilities", "docs", "LICENSE", "CHANGELOG.md"):
for rel in TEMP_ROOT_INPUTS:
_copy(rel, self.root)

def _set_committed_version(self, version: str) -> None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import zipfile
from pathlib import Path

from tests.integration.packaging._shared import TEMP_ROOT_INPUTS
from tests.support.paths import REPO_ROOT

SCRIPT = "scripts/release/verify-skill-archives.sh"
Expand Down Expand Up @@ -96,8 +97,9 @@ def setUp(self) -> None:
tmp = tempfile.TemporaryDirectory()
self.addCleanup(tmp.cleanup)
self.root = Path(tmp.name)
for rel in ("skills", "shared", "scripts", "capabilities", "docs", "LICENSE"):
_copy(rel, self.root)
for rel in TEMP_ROOT_INPUTS:
if rel != "CHANGELOG.md": # each test writes its own rolled changelog
_copy(rel, self.root)

def _roll_changelog(self, version: str) -> None:
(self.root / "CHANGELOG.md").write_text(
Expand Down
Loading
Loading