From c19a4afacd14214da1df500c0623f530acd03530 Mon Sep 17 00:00:00 2001 From: Daniel Holanda Date: Wed, 9 Sep 2026 06:18:13 -0700 Subject: [PATCH] Select from the merge base, not the base branch tip A pull request event names two commits, and the discover job diffed them directly. That answers how the two trees differ, which stops being the same question as what the branch did the moment the base branch moves on without it: everything merged into the base since the branch left comes back in that diff, in reverse, as though this branch had touched it. amd/skills#207 edits one skill and got behavioral legs for all seven, because its branch predates a base commit touching .github/workflows/evals.yml -- an infra path, so selection re-ran the entire catalog off a file the branch never opened. Two other skills came back the same way and failed on their own. Diff from the merge base instead. It lives in `select --since BASE HEAD` rather than in the workflow's inline Python, because a decision made in YAML cannot be tested or re-run by hand, and this is the one that decides what a run costs. `--changed` still reads a list of paths from stdin for a caller that works them out some other way. There is no merge base for unrelated histories, or in a clone too shallow to hold one. That falls back to the plain diff with a warning: selecting too much costs a slow run, selecting too little ships an untested change. Replaying amd/skills#207 against this build plans one leg (hyperloom-workload-optimizer on its Instinct runner) plus routing, down from eleven behavioral legs. Co-authored-by: Cursor --- .github/workflows/reusable.yml | 4 +- .github/workflows/skill-evals.yml | 30 ++++---- README.md | 2 +- action.yml | 2 +- bootstrap/launch.py | 4 +- docs/usage.md | 23 ++++-- examples/amd-skills-checks.yml | 2 +- pyproject.toml | 2 +- skillscope/__init__.py | 2 +- skillscope/cli.py | 36 ++++++++-- skillscope/select.py | 53 ++++++++++++++ tests/test_skillscope.py | 113 ++++++++++++++++++++++++++++++ 12 files changed, 236 insertions(+), 37 deletions(-) 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."""