diff --git a/.github/workflows/reusable.yml b/.github/workflows/reusable.yml index 68e3dd3..3e5d073 100644 --- a/.github/workflows/reusable.yml +++ b/.github/workflows/reusable.yml @@ -8,7 +8,7 @@ name: reusable # # jobs: # evals: -# uses: amd/skillscope/.github/workflows/reusable.yml@v0.1.1 +# uses: amd/skillscope/.github/workflows/reusable.yml@v0.1.2 # secrets: # api_key: ${{ secrets.ANTHROPIC_API_KEY }} # with: @@ -63,7 +63,7 @@ name: reusable # let a reusable workflow interpolate that pin into `uses: amd/skillscope@...`, # so each job checks out this repository at `job.workflow_sha` (the commit the # caller referenced) and runs the composite action from that tree. Pinning -# `@v0.1.1` grades with v0.1.1, `@main` grades with main, and a branch grades +# `@v0.1.2` grades with v0.1.2, `@main` grades with main, and a branch grades # with that branch -- so there is no release ritual and nothing to keep in step. on: diff --git a/.github/workflows/skill-evals.yml b/.github/workflows/skill-evals.yml index 58f02cb..0720bc5 100644 --- a/.github/workflows/skill-evals.yml +++ b/.github/workflows/skill-evals.yml @@ -16,7 +16,7 @@ name: skill-evals # # jobs: # skill-evals: -# uses: amd/skillscope/.github/workflows/skill-evals.yml@v0.1.1 +# uses: amd/skillscope/.github/workflows/skill-evals.yml@v0.1.2 # secrets: inherit # with: # skill_globs: skills/* @@ -62,7 +62,7 @@ name: skill-evals # let a reusable workflow interpolate that pin into `uses: amd/skillscope@...`, # so each job checks out this repository at `job.workflow_sha` (the commit the # caller referenced) and runs the composite action from that tree. Pinning -# `@v0.1.1` grades with v0.1.1, `@main` grades with main, and a branch grades +# `@v0.1.2` grades with v0.1.2, `@main` grades with main, and a branch grades # with that branch -- so there is no release ritual and nothing to keep in step. on: @@ -273,7 +273,8 @@ jobs: - name: Check out repository uses: actions/checkout@v4 with: - # Need the merge base so `git diff` can see what the pull request changed. + # Both commits' full history, so selection can find their merge base + # and diff what the pull request changed from there. fetch-depth: 0 - name: Check out skillscope @@ -302,6 +303,10 @@ jobs: # it was asked for. Written in Python so the arguments are quoted rather # than pasted -- a label like `don't merge` would otherwise arrive at the # CLI in pieces. + # + # Which commits changed what is left to `select --since`, along with + # every other decision: it diffs from the merge base, so a branch whose + # base has moved on is still planned for what the branch did. - name: Work out how to select id: how shell: python @@ -323,22 +328,18 @@ jobs: run: | import os import shlex - import subprocess event = os.environ["EVENT"] skills = os.environ.get("SKILLS", "").strip() base, head = os.environ.get("BASE", ""), os.environ.get("HEAD", "") extended = os.environ["EXTENDED"] - changed = "" if event == "pull_request" and base: - changed = subprocess.run( - ["git", "diff", "--name-only", base, head], - check=True, - capture_output=True, - text=True, - ).stdout - args = ["--changed", "--labels", os.environ.get("LABELS", ""), extended] + args = [ + "--since", base, head, + "--labels", os.environ.get("LABELS", ""), + extended, + ] if os.environ.get("IGNORE_GATES"): args.append("--ignore-gates") else: @@ -363,10 +364,6 @@ jobs: if value: args += [flag, value] - with open("changed-files.txt", "w", encoding="utf-8") as handle: - handle.write(changed) - print("Changed files:\n" + (changed or "(none)")) - # Quoted rather than pasted: a JSON label array and a pull-request # label like `don't merge` both have to survive the trip intact. line = "args=" + " ".join(shlex.quote(a) for a in args) @@ -381,7 +378,6 @@ jobs: command: select args: ${{ steps.how.outputs.args }} skills: ${{ inputs.skill_globs }} - stdin: changed-files.txt python-version: ${{ inputs.python_version }} - name: Emit the plan diff --git a/README.md b/README.md index 73bf7f3..3beaa72 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,7 @@ Simply add an `evals.json` file to your skill and a workflow that points to our ```yaml jobs: evals: - uses: amd/skillscope/.github/workflows/reusable.yml@v0.1.1 + uses: amd/skillscope/.github/workflows/reusable.yml@v0.1.2 secrets: api_key: ${{ secrets.ANTHROPIC_API_KEY }} with: diff --git a/action.yml b/action.yml index 70261d1..6d1ee8b 100644 --- a/action.yml +++ b/action.yml @@ -9,7 +9,7 @@ description: >- # Pin the tag you want to run, the same way you pin the reusable workflow: # -# - uses: amd/skillscope@v0.1.1 +# - uses: amd/skillscope@v0.1.2 # # The Python in that checkout is the harness. There is no second version to # resolve, and nothing here fetches a different ref. diff --git a/bootstrap/launch.py b/bootstrap/launch.py index 19cdac6..26e7193 100644 --- a/bootstrap/launch.py +++ b/bootstrap/launch.py @@ -5,7 +5,7 @@ """Run one skillscope command from the composite action's own checkout. Callers pin a tag on the action or on a reusable workflow in this repo -(``amd/skillscope@v0.1.1``, ``.../reusable.yml@v0.1.1``). This script installs +(``amd/skillscope@v0.1.2``, ``.../reusable.yml@v0.1.2``). This script installs *that* checkout with ``uvx`` and execs the command. It does not fetch some other ref: the ``uses:`` pin is the harness. @@ -91,7 +91,7 @@ def main() -> int: if not (source / "pyproject.toml").is_file(): raise SystemExit( f"error: {source} has no pyproject.toml. The action must run from " - "a skillscope checkout (for example amd/skillscope@v0.1.1)." + "a skillscope checkout (for example amd/skillscope@v0.1.2)." ) version = packaged_version(source) or "unknown" diff --git a/docs/usage.md b/docs/usage.md index 6de894c..8c6825e 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -216,7 +216,7 @@ one runner per skill: ```yaml jobs: evals: - uses: amd/skillscope/.github/workflows/reusable.yml@v0.1.1 + uses: amd/skillscope/.github/workflows/reusable.yml@v0.1.2 secrets: api_key: ${{ secrets.ANTHROPIC_API_KEY }} with: @@ -280,7 +280,7 @@ pays for. ```yaml jobs: skill-evals: - uses: amd/skillscope/.github/workflows/skill-evals.yml@v0.1.1 + uses: amd/skillscope/.github/workflows/skill-evals.yml@v0.1.2 secrets: inherit with: routing_room: my-skill,its-neighbour @@ -295,7 +295,7 @@ workflow file documents every one. To run a single command instead of a pipeline, use the action directly: ```yaml -- uses: amd/skillscope@v0.1.1 +- uses: amd/skillscope@v0.1.2 with: command: structural ``` @@ -304,7 +304,20 @@ Deciding what to run by hand is also possible: `select` emits the plan for a change as JSON. ```bash -git diff --name-only main HEAD | skillscope select --changed +skillscope select --since main HEAD +``` + +`--since` takes the two commits a pull request names and works the changed +paths out from their merge base, so a branch is planned for what it changed +rather than for how it differs from a base that has moved on without it. +Without that, everything merged into the base since the branch left it comes +back in the diff, and a base commit touching an [infra +path](#configuring-the-repo-under-test) re-runs the whole catalog. + +Pass a list of paths instead when it was worked out some other way: + +```bash +git diff --name-only main...HEAD | skillscope select --changed ``` ## Versions @@ -315,7 +328,7 @@ the tag you want to run: ```yaml jobs: evals: - uses: amd/skillscope/.github/workflows/reusable.yml@v0.1.1 + uses: amd/skillscope/.github/workflows/reusable.yml@v0.1.2 ``` That tag's checkout is what grades your skills. Bump the ref in that one line diff --git a/examples/amd-skills-checks.yml b/examples/amd-skills-checks.yml index f9e4534..bc2cba3 100644 --- a/examples/amd-skills-checks.yml +++ b/examples/amd-skills-checks.yml @@ -21,7 +21,7 @@ permissions: jobs: evals: name: AMD Skills Checks - uses: amd/skillscope/.github/workflows/reusable.yml@v0.1.1 + uses: amd/skillscope/.github/workflows/reusable.yml@v0.1.2 secrets: api_key: ${{ secrets.ANTHROPIC_API_KEY }} with: diff --git a/pyproject.toml b/pyproject.toml index 8a2ec72..150e2e4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,7 +8,7 @@ build-backend = "setuptools.build_meta" [project] name = "skillscope" -version = "0.1.1" +version = "0.1.2" description = "Routing and behavioral test harness for agent skills." readme = "README.md" requires-python = ">=3.10" diff --git a/skillscope/__init__.py b/skillscope/__init__.py index 98d320b..55c78d8 100644 --- a/skillscope/__init__.py +++ b/skillscope/__init__.py @@ -15,4 +15,4 @@ __all__ = ["__version__"] -__version__ = "0.1.1" +__version__ = "0.1.2" diff --git a/skillscope/cli.py b/skillscope/cli.py index fdb70af..9a64987 100644 --- a/skillscope/cli.py +++ b/skillscope/cli.py @@ -53,7 +53,10 @@ skillscope routing --only qwen-on-mi300x --keep-logs eval-logs # what CI should run for a change - git diff --name-only BASE HEAD | skillscope select --changed + skillscope select --since BASE HEAD + + # the same, from a list of paths worked out some other way + git diff --name-only BASE...HEAD | skillscope select --changed Reports go to stdout as markdown, to ``$GITHUB_STEP_SUMMARY`` under Actions, and to a JSON artifact under ``.skillscope/runs/`` in the repo under test. @@ -228,6 +231,20 @@ def cmd_template(args: argparse.Namespace) -> int: return 0 +def _changed_for_select(args: argparse.Namespace) -> set[str]: + """The changed paths to plan from, however the caller chose to say them.""" + if args.since: + lines = select_module.changed_paths(*args.since) + # The plan is the only thing on stdout, so what it was decided from + # goes to stderr, where a CI log still shows it. + print("Changed files:", file=sys.stderr) + for path in lines or ["(none)"]: + print(f" {path}", file=sys.stderr) + else: + lines = sys.stdin.read().splitlines() + return {line.strip().replace("\\", "/") for line in lines if line.strip()} + + def cmd_select(args: argparse.Namespace) -> int: available = datasets.skills_with_datasets() if args.all: @@ -240,11 +257,7 @@ def cmd_select(args: argparse.Namespace) -> int: return 1 skills, needs_routing = requested, True else: - changed = { - line.strip().replace("\\", "/") - for line in sys.stdin.read().splitlines() - if line.strip() - } + changed = _changed_for_select(args) skills = select_module.select_from_changes(changed) needs_routing = select_module.routing_needed(changed, args.extended) @@ -747,6 +760,17 @@ def build_parser() -> argparse.ArgumentParser: mode = select_parser.add_mutually_exclusive_group(required=True) mode.add_argument("--all", action="store_true", help="Every skill with a dataset.") mode.add_argument("--changed", action="store_true", help="Read changed paths from stdin.") + mode.add_argument( + "--since", + nargs=2, + metavar=("BASE", "HEAD"), + help=( + "The two commits a pull request names. Changed paths are worked " + "out from their merge base, so a branch is planned for what it " + "changed rather than for how it differs from a base that has moved " + "on. Both commits' history has to be in the checkout." + ), + ) mode.add_argument( "--names", metavar="A,B,C", help="An explicit comma-separated skill list." ) diff --git a/skillscope/select.py b/skillscope/select.py index f0df66a..e8b926c 100644 --- a/skillscope/select.py +++ b/skillscope/select.py @@ -54,6 +54,8 @@ from __future__ import annotations import json +import subprocess +import sys from pathlib import Path from . import config, datasets @@ -65,6 +67,57 @@ EXTENDED_SUFFIX = "/" + datasets.EXTENDED_DATASET_RELPATH.as_posix() +def changed_paths(base: str, head: str) -> list[str]: + """What a branch changed, given the two commits a pull request names. + + ``git diff base head`` answers a different question -- how those two trees + differ -- and the difference matters as soon as the base branch moves on + without the branch. Everything merged into the base since the branch left + it comes back in that diff, in reverse, as though this branch had touched + it. One skill's pull request then re-runs its neighbours, and a base commit + that happened to touch an infra path re-runs the entire catalog. + + So diff from the merge base, which is the only commit both sides agree on + and so the only one that makes the answer "what did this branch do". + + Falls back to the plain diff when there is no common ancestor to be found + -- a clone shallow enough not to contain one, or histories that really are + unrelated -- because selecting too much costs a slow run, and selecting too + little ships an untested change. + """ + root = config.active().root + + def git(*args: str) -> subprocess.CompletedProcess: + return subprocess.run( + ["git", "-C", str(root), *args], + capture_output=True, + text=True, + encoding="utf-8", + check=False, + ) + + fork_point = git("merge-base", base, head) + if fork_point.returncode == 0 and fork_point.stdout.strip(): + base = fork_point.stdout.strip() + else: + print( + f"warning: no merge base for {base} and {head}, so selection is " + "falling back to the plain diff between them. It may name files " + "this branch never touched.\n" + f"{fork_point.stderr.strip()}", + file=sys.stderr, + ) + + diff = git("diff", "--name-only", base, head) + if diff.returncode != 0: + raise SystemExit( + f"error: could not diff {base}..{head} in {root}. CI needs the " + "history of both commits to work out what changed, so check out " + "with fetch-depth: 0.\n" + diff.stderr.strip() + ) + return [line.strip() for line in diff.stdout.splitlines() if line.strip()] + + def infra_paths() -> set[str]: """Paths that change the shared engine rather than one skill. diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 4f49a72..a3bc5ff 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -25,6 +25,7 @@ import os import re import runpy +import subprocess import tempfile import time import unittest @@ -735,6 +736,15 @@ def test_select_does_not_take_a_timeout(self) -> None: args = cli.build_parser().parse_args(["select", "--all"]) self.assertFalse(hasattr(args, "timeout")) + def test_select_takes_a_commit_pair_or_a_list_of_paths_but_not_both(self) -> None: + args = cli.build_parser().parse_args(["select", "--since", "base", "head"]) + self.assertEqual(args.since, ["base", "head"]) + with contextlib.redirect_stderr(io.StringIO()): + with self.assertRaises(SystemExit): + cli.build_parser().parse_args( + ["select", "--since", "base", "head", "--changed"] + ) + class TestDeadline(unittest.TestCase): """The command-level --timeout, distinct from a routing case's own cap.""" @@ -917,6 +927,109 @@ def test_hardware_with_no_environment_stays_in_one_matrix(self) -> None: ) +class TestWhatABranchChanged(unittest.TestCase): + """Selection is planned from the merge base, not from the base branch tip. + + A pull request names two commits, and the difference between their trees is + not the same question as what the branch did. As soon as the base moves on + without the branch, everything merged into it comes back in that diff, in + reverse -- so a change to one skill re-runs its neighbours, and a base + commit that touched an infra path re-runs the whole catalog. + """ + + def setUp(self) -> None: + self.repo = Repo(self) + self.repo.skill("alpha", dataset=tier0_dataset("alpha")) + self.repo.skill("beta", dataset=tier0_dataset("beta")) + self.repo.activate( + routing_room="alpha,beta", + infra_paths=".github/workflows/evals.yml", + ) + self.git("init", "--quiet", "--initial-branch", "main") + self.git("config", "user.email", "selftest@example.invalid") + self.git("config", "user.name", "skillscope selftest") + self.base = self.commit("everything so far") + + def git(self, *args: str) -> str: + done = subprocess.run( + ["git", "-C", str(self.repo.root), *args], + capture_output=True, + text=True, + encoding="utf-8", + check=True, + ) + return done.stdout.strip() + + def commit(self, message: str) -> str: + self.git("add", "--all") + self.git("commit", "--quiet", "--allow-empty", "-m", message) + return self.git("rev-parse", "HEAD") + + def write(self, relative: str, text: str) -> None: + path = self.repo.root / relative + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + + def branch_behind_an_advanced_base(self) -> tuple[str, str]: + """A branch off `main`, with `main` moving on after it left. + + Returns the base branch's new tip and the branch's head -- the pair a + pull request event hands CI. + """ + self.git("checkout", "--quiet", "-b", "touches-alpha") + self.write("alpha/SKILL.md", "---\nname: alpha\ndescription: Does alpha things now.\n---\n") + head = self.commit("edit one skill") + + self.git("checkout", "--quiet", "main") + self.write(".github/workflows/evals.yml", "name: evals\n") + base = self.commit("something else lands on the base branch") + return base, head + + def test_a_branch_is_planned_for_its_own_commits(self) -> None: + base, head = self.branch_behind_an_advanced_base() + self.assertEqual(select_module.changed_paths(base, head), ["alpha/SKILL.md"]) + + def test_the_base_branchs_own_commits_are_not_this_branchs(self) -> None: + # The regression this pins: the plain diff between the two commits + # names a file only the base branch touched, and that file is an infra + # path, so planning from it re-runs every skill in the repo. + base, head = self.branch_behind_an_advanced_base() + self.assertIn( + ".github/workflows/evals.yml", + self.git("diff", "--name-only", base, head).splitlines(), + ) + self.assertEqual( + select_module.select_from_changes(set(select_module.changed_paths(base, head))), + ["alpha"], + ) + + def test_a_branch_that_does_touch_an_infra_path_still_re_runs_everything(self) -> None: + self.git("checkout", "--quiet", "-b", "touches-the-harness") + self.write(".github/workflows/evals.yml", "name: evals\n") + head = self.commit("edit the harness") + self.assertEqual( + select_module.select_from_changes( + set(select_module.changed_paths(self.base, head)) + ), + ["alpha", "beta"], + ) + + def test_unrelated_histories_fall_back_to_the_plain_diff(self) -> None: + # No common ancestor to diff from -- a clone too shallow to hold one, + # or histories that really are unrelated. Selecting too much costs a + # slow run; selecting too little ships an untested change. + self.git("checkout", "--quiet", "--orphan", "elsewhere") + self.git("rm", "-rq", "--cached", ".") + self.write("beta/SKILL.md", "---\nname: beta\ndescription: Does beta things now.\n---\n") + head = self.commit("a history of its own") + + stderr = io.StringIO() + with contextlib.redirect_stderr(stderr): + changed = select_module.changed_paths(self.base, head) + self.assertIn("no merge base", stderr.getvalue()) + self.assertIn("beta/SKILL.md", changed) + + class TestCaseExpectations(unittest.TestCase): """`skill_should_trigger` is the whole expectation."""