diff --git a/.github/workflows/reusable.yml b/.github/workflows/reusable.yml index 0f97f8a..87fa7b4 100644 --- a/.github/workflows/reusable.yml +++ b/.github/workflows/reusable.yml @@ -66,9 +66,10 @@ on: description: >- The skills to grade: a directory, or a glob matching several, one per line or comma-separated, relative to the repository root. Each one is - graded on a runner of its own. + graded on a runner of its own. The default is every directory at the + root; a repo that keeps its skills together says so with `skills/*`. type: string - default: "skills/*" + default: "./*" structural: description: >- @@ -297,7 +298,7 @@ jobs: # by default; `min_accuracy` lowers that bar and `routing: optional` # removes it. args: >- - --routing-skills '${{ matrix.skill }}' + --routing-room '${{ matrix.skill }}' --min-accuracy '${{ inputs.min_accuracy }}' --output routing-report.json --keep-logs routing-logs diff --git a/.github/workflows/selftest.yml b/.github/workflows/selftest.yml index 79d93f9..8b30b7c 100644 --- a/.github/workflows/selftest.yml +++ b/.github/workflows/selftest.yml @@ -51,18 +51,21 @@ jobs: steps: - uses: actions/checkout@v4 + # Laid out the way the default glob expects one: skills at the root, so + # not a step below passes `skills`, which is the configuration a repo + # starting from nothing has. - name: Build a repo to test shell: bash run: | set -euo pipefail - mkdir -p fixture/skills/demo-skill/evals - cat > fixture/skills/demo-skill/SKILL.md <<'EOF' + mkdir -p fixture/demo-skill/evals + cat > fixture/demo-skill/SKILL.md <<'EOF' --- name: demo-skill description: Does demonstrable things, for a test that needs a skill. --- EOF - cat > fixture/skills/demo-skill/skill-card.md <<'EOF' + cat > fixture/demo-skill/skill-card.md <<'EOF' # Skill Card ## Description @@ -77,7 +80,7 @@ jobs: MIT EOF - cat > fixture/skills/demo-skill/evals/evals.json <<'EOF' + cat > fixture/demo-skill/evals/evals.json <<'EOF' { "evaluations": [ {"id": "demo-a", "skill_should_trigger": true, "prompt": "do the demo thing"}, @@ -105,8 +108,9 @@ jobs: # This repo has no skills, so what is left is its own prose -- which is # exactly the half a harness for checking references should not be - # allowed to get wrong about itself. Offline, so nothing here depends on - # the network being polite today. + # allowed to get wrong about itself. `--docs` is also what makes a run + # that finds no skill a real run rather than one that graded nothing. + # Offline, so nothing here depends on the network being polite today. - name: Check this repo's own references uses: ./ with: @@ -119,7 +123,7 @@ jobs: uses: ./ with: command: select - args: --all --routing-skills demo-skill + args: --all --routing-room demo-skill repo: fixture source: . @@ -137,7 +141,7 @@ jobs: uses: ./ with: command: select - args: --all --routing-skills none + args: --all --routing-room none repo: fixture source: . diff --git a/.github/workflows/skill-evals.yml b/.github/workflows/skill-evals.yml index 03d133a..7b01c71 100644 --- a/.github/workflows/skill-evals.yml +++ b/.github/workflows/skill-evals.yml @@ -20,7 +20,7 @@ name: skill-evals # secrets: inherit # with: # skill_globs: skills/* -# routing_skills: my-skill,its-neighbour +# routing_room: my-skill,its-neighbour # # The caller's `with:` block is the whole configuration. There is no config file # in the repo under test: a repo that runs these evals already has a workflow @@ -32,7 +32,7 @@ name: skill-evals # in YAML cannot be tested and cannot be run locally. What is left here is job # topology, which is the one thing that has to be YAML. # -# * routing installs the skills named in `routing_skills` side by side and +# * routing installs the skills named in `routing_room` side by side and # grades which one fires. Who a skill competes against is what the score # means, so wherever there is a choice it is a decision a human makes # rather than one this workflow guesses; a repo with a single skill has no @@ -64,10 +64,12 @@ on: # --- what this repo looks like --------------------------------------- skill_globs: description: >- - Globs naming the directories that hold skills, comma-separated. + Globs naming the directories that are skills, comma-separated. The + default is every directory at the repository root; a repo that keeps + its skills together says so with `skills/*`. type: string - default: "skills/*" - routing_skills: + default: "./*" + routing_room: description: >- Skills to install side by side for the routing run, comma-separated, `all` for every skill with a dataset, or `none` to skip routing. @@ -305,7 +307,7 @@ jobs: IGNORE_GATES: ${{ inputs.ignore_gates && '--ignore-gates' || '' }} BASE: ${{ github.event.pull_request.base.sha }} HEAD: ${{ github.event.pull_request.head.sha }} - ROUTING_SKILLS: ${{ inputs.routing_skills }} + ROUTING_ROOM: ${{ inputs.routing_room }} INFRA_PATHS: ${{ inputs.infra_paths }} BEHAVIOR_RUNNER: ${{ inputs.behavior_runner }} BEHAVIOR_OS: ${{ inputs.behavior_os }} @@ -340,10 +342,10 @@ jobs: args += ["--ignore-gates", extended] # What this repo looks like. Planning is the only step that needs all - # of it, so this is the one place that spells it out; `--skills` goes - # to the action instead, because the launcher needs it too. + # of it, so this is the one place that spells it out; `--skills-dir` + # goes to the action instead, because the launcher needs it too. for flag, name in ( - ("--routing-skills", "ROUTING_SKILLS"), + ("--routing-room", "ROUTING_ROOM"), ("--infra-paths", "INFRA_PATHS"), ("--behavior-runner", "BEHAVIOR_RUNNER"), ("--behavior-os", "BEHAVIOR_OS"), @@ -508,7 +510,7 @@ jobs: # the way a shell would, and a comma-separated list a caller wrote # with spaces in it would otherwise arrive as several arguments. args: >- - --routing-skills '${{ inputs.routing_skills }}' + --routing-room '${{ inputs.routing_room }}' --min-accuracy '${{ inputs.min_accuracy }}' ${{ needs.discover.outputs.extended }} ${{ inputs.only && format('--only ''{0}''', inputs.only) || '' }} diff --git a/README.md b/README.md index f76978c..79bd0b9 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ uv tool install git+https://github.com/amd/skillscope skillscope structural # no agent, no tokens skillscope structural --external # the same, plus checking external URLs skillscope behavioral --skill my-skill # needs an authenticated `claude` CLI -skillscope routing --routing-skills my-skill,its-neighbour +skillscope routing --routing-room my-skill,its-neighbour ``` ## Run it from your CI diff --git a/action.yml b/action.yml index 81befa3..4c4ec91 100644 --- a/action.yml +++ b/action.yml @@ -44,9 +44,10 @@ inputs: default: "" skills: description: >- - Globs naming the directories that hold skills, comma-separated. The + Globs naming the directories that are skills, comma-separated. The launcher needs these to find a skill's version pin before it has fetched - anything, and passes them on to the harness. Default: skills/*. + anything, and passes them on to the harness. Default: ./*, every + directory at the repo root. required: false default: "" repo: diff --git a/bootstrap/launch.py b/bootstrap/launch.py index 792ae59..61d0023 100644 --- a/bootstrap/launch.py +++ b/bootstrap/launch.py @@ -24,7 +24,7 @@ SKILLSCOPE_COMMAND the subcommand, e.g. "structural" SKILLSCOPE_ARGS further arguments, shell-quoted SKILLSCOPE_REPO root of the repo under test (default ".") - SKILLSCOPE_SKILLS globs naming the directories that hold skills + SKILLSCOPE_SKILLS globs naming the directories that are skills SKILLSCOPE_SOURCE owner/repo (or a local path) to install from SKILLSCOPE_REQUESTED an explicit version, which wins outright SKILLSCOPE_VERSION a version from the environment diff --git a/bootstrap/resolve_version.py b/bootstrap/resolve_version.py index 2c043aa..48310fb 100644 --- a/bootstrap/resolve_version.py +++ b/bootstrap/resolve_version.py @@ -36,7 +36,9 @@ import sys from pathlib import Path -DEFAULT_SKILL_GLOBS = ["skills/*"] +# Kept in step with skillscope.config by hand: this script is deliberately +# ignorant of the package it is about to fetch, so it cannot import the value. +DEFAULT_SKILL_GLOBS = ["./*"] DATASET_RELPATH = "evals/evals.json" VERSION_KEY = "skillscope_version" @@ -114,17 +116,17 @@ def main(argv: list[str] | None = None) -> int: "--default", default="", help="Fallback when nothing else pins a version." ) parser.add_argument( - "--skills", + "--skills-dir", default="", help=( - "Comma-separated globs naming the directories that hold skills, " - "so a repo that keeps them somewhere unusual is still searched for " - f"the pin. Default: {','.join(DEFAULT_SKILL_GLOBS)}." + "Comma-separated globs naming the directories that are skills, so " + "a repo that keeps them somewhere other than its root is still " + f"searched for the pin. Default: {','.join(DEFAULT_SKILL_GLOBS)}." ), ) args = parser.parse_args(argv) - globs = [g.strip() for g in args.skills.split(",") if g.strip()] + globs = [g.strip() for g in args.skills_dir.split(",") if g.strip()] version = resolve( root=Path(args.repo).expanduser().resolve(), requested=args.version, diff --git a/docs/authoring-evals.md b/docs/authoring-evals.md index 35ae51d..92ef3f6 100644 --- a/docs/authoring-evals.md +++ b/docs/authoring-evals.md @@ -49,7 +49,7 @@ skills in one workspace, so it is the same assertion either way, and filing it under the neighbour keeps `false` meaning "nothing fires". Your dataset's prompts are graded in a routing run only when your skill is in -the room — one the workflow lists in `routing_skills`, or the only skill the +the room — one the workflow lists in `routing_room`, or the only skill the repo has, which needs no listing. How the room is chosen is in [usage.md](usage.md#who-a-skill-competes-against-is-listed-not-inferred). A prompt expecting a skill that is not in the room could only ever lose, and a @@ -207,7 +207,7 @@ holds. ```bash skillscope structural # your folder, prose, and dataset skillscope behavioral --skill # your skill, end to end -skillscope routing --routing-skills , +skillscope routing --routing-room , skillscope routing --only --keep-logs logs # one case, keeping the transcript ``` diff --git a/docs/usage.md b/docs/usage.md index ed1cc20..f09592d 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -39,8 +39,8 @@ places to disagree. | Flag | Workflow input | Default | What it decides | | --- | --- | --- | --- | -| `--skills` | `skill_globs` | `skills/*` | Globs naming the directories that hold skills. A skill is a directory with a `SKILL.md`, and its directory name is its identity. | -| `--routing-skills` | `routing_skills` | the only skill, if there is one | The skills a routing run installs side by side. `all` means every skill with a dataset, `none` means no routing run, and blank means a repo with one skill runs that skill while a repo with several has to choose. | +| `--skills-dir` | `skill_globs` | the directory you are in | Globs naming the directories that *are* skills, relative to the repo root. A skill is a directory with a `SKILL.md`, and its directory name is its identity. A repo that keeps its skills together passes `skills/*`. | +| `--routing-room` | `routing_room` | `--skill` if given, else the only skill, if there is one | The skills a routing run installs side by side. `all` means every skill with a dataset, `none` means no routing run, and blank means `--skill` is the room, a repo with one skill runs that skill, and a repo with several and no `--skill` has to choose. | | `--infra-paths` | `infra_paths` | none | Paths that change the harness rather than one skill, so touching one re-runs every skill instead of guessing at the blast radius. Your own workflow file belongs here. | | `--docs` | `doc_globs` | none | Markdown outside the skills whose references should be checked too: a README, a docs tree. The skills themselves are always checked. | | `--exclude-url` | `excluded_urls` | none | Regexes matching URLs the external reference check leaves alone. For hosts that are auth-gated or that answer a runner's IP with a 403. | @@ -60,9 +60,28 @@ whole command. Routing adds `--jobs`, `--case-timeout`, `--max-tool-calls`, `--max-budget-usd`, `--keep-logs`, and `--min-accuracy`. `--help` is the authority on all of them. +### Where the skills are + +Every path in the table is relative to the repository root, which is the only +base a workflow input, a line of `git diff --name-only`, and a root-relative +markdown link can all agree on. `skills/*` in a workflow means the same thing +wherever its runner happens to have started. + +The one thing measured from somewhere else is `--skills-dir` when you do not +pass it: then it is every directory in the one you ran the command from. +Standing in a tree of skills and typing `skillscope structural` can only mean +these ones, and a repo that keeps them a level down works with a `cd` rather +than a flag. Under CI the two bases coincide, because the launcher runs from +the repo root — so a run with no `--skills-dir` grades the directories at the +root, and a repo whose skills live anywhere else names them. + +Either way it looks one level down and no further. Searching a whole tree for +every `SKILL.md` finds vendored copies, fixtures, and a contributor's local +install, and each of those silently changes a routing score. + ### Who a skill competes against is listed, not inferred -`routing_skills` is the one input with no useful default, because the answer is +`routing_room` is the one input with no useful default, because the answer is what the score *means*. Install every skill on disk and a work-in-progress directory drops everyone's number; install only the skill under review and it wins every prompt by walkover. A skill you leave off the list still gets its @@ -70,13 +89,15 @@ dataset checked and its behavioral cases run — it just does not move anybody's routing score. Listing them also makes the change visible: a skill joining or leaving the room moves every other skill's number, and that deserves a diff. -A repo with one skill has no such choice, so leave `routing_skills` blank and +A repo with one skill has no such choice, so leave `routing_room` blank and its only skill is the room. The score is then the half of the question that can be answered alone — does the skill fire on its own prompts, and does it stay quiet on its near misses and the shared negatives — and it stops meaning that the moment a second skill shows up, at which point the flag becomes required -again rather than quietly picking a room for you. To turn routing off instead, -say so: `routing_skills: none`. +again rather than quietly picking a room for you. Naming skills with `--skill` +and leaving `--routing-room` off is still a listing, not a guess: those skills +are the room. The workflow input has no `--skill`, so a repo with several still +has to choose. To turn routing off instead, say so: `routing_room: none`. ## What the structural check asserts @@ -95,8 +116,11 @@ agent that simply never uses the skill. So every `SKILL.md` is read first: | `description` | non-empty, at most 1024 characters | | body | at most 500 lines — past that it is reference material, and an agent reads it in full every time the skill loads | -A directory your skill globs match that holds no `SKILL.md` is reported too. -Either the file is missing or the glob is too wide, and both are worth one line. +A directory that holds no `SKILL.md` is simply not a skill, and is passed over +without a word. Matching *no* skill at all is the case that is reported, since +a run that graded nothing and called itself green is the one way this harness +can lie about a repo; a run given `--docs` is exempt, having been asked to +check a repo's own prose. Whatever else your repo asks of a skill is policy rather than format, so it is configuration: @@ -222,7 +246,7 @@ Naming several, and holding them to different bars: | Input | Default | What it decides | | --- | --- | --- | -| `skills` | `skills/*` | The skills to grade: a directory, or a glob matching several, one per line or comma-separated. | +| `skills` | `./*` | The skills to grade: a directory, or a glob matching several, one per line or comma-separated. The default is every directory at the repo root. | | `structural` | `required` | `required`, `optional`, or `off`. | | `routing` | `required` | `required`, `optional`, or `off`. | | `behavioral` | `required` | `required`, `optional`, or `off`. | @@ -260,7 +284,7 @@ jobs: uses: amd/skillscope/.github/workflows/skill-evals.yml@main secrets: inherit with: - routing_skills: my-skill,its-neighbour + routing_room: my-skill,its-neighbour api_key_secret: MY_MODEL_API_KEY ``` diff --git a/skillscope/cli.py b/skillscope/cli.py index f44d3d3..c88c38d 100644 --- a/skillscope/cli.py +++ b/skillscope/cli.py @@ -9,7 +9,7 @@ * ``structural`` -- every skill folder, every dataset, and every reference the skill's markdown makes. No agent, no tokens, instant. - * ``routing`` -- installs the skills named by ``--routing-skills`` side by + * ``routing`` -- installs the skills named by ``--routing-room`` side by side and checks that the right one fires (and that nothing fires when nothing should). Cheap, no hardware, and it pools those skills' prompts so each one's positives are the others' negatives. A repo with a single skill @@ -40,11 +40,11 @@ skillscope behavioral --skill serving-llms-on-epyc # what CI runs: a routing miss fails the run, like a behavioral miss does - skillscope routing --routing-skills local-ai-use,tracelens --no-extended + skillscope routing --routing-room local-ai-use,tracelens --no-extended skillscope behavioral --skill local-ai-use --no-extended # a routing run that reports its score instead of gating on it - skillscope routing --routing-skills all --min-accuracy 0 + skillscope routing --routing-room all --min-accuracy 0 # a repo with one skill: the room is that skill, so nothing names it skillscope routing @@ -119,17 +119,24 @@ def _report_failures(errors: list[str]) -> None: print(f" - {err}", file=sys.stderr) -def _structural_or_exit() -> list[references.Reference]: +def _structural_or_exit(skills: list[str] | None = None) -> list[references.Reference]: """Fail before any tokens are spent if the skills are not in shape. The skill folders, the datasets, and the internal references: everything that costs nothing to check. External URLs are left out on purpose: they fail for reasons that have nothing to do with the run being gated. + + Given `skills`, only those are read. A graded run passes the skills it is + about to install, because that is the run it is gating: a neighbour's + malformed dataset says nothing about whether this run can proceed, and + holding it back would make one skill's mistake everybody else's. The + repo-wide answer is ``skillscope structural``, which passes nothing here + and is the check that gates a merge. """ - found = references.collect() + found = references.collect(skills=skills) errors = ( - structure.errors() - + datasets.structural_errors() + structure.errors(skills) + + datasets.structural_errors(skills) + references.internal_errors(found) ) if errors: @@ -260,7 +267,7 @@ def cmd_select(args: argparse.Namespace) -> int: def _empty_room(args: argparse.Namespace) -> None: """A routing run was asked for with nobody in the room. - ``--routing-skills none`` on this command is a contradiction: the command + ``--routing-room none`` on this command is a contradiction: the command is the routing run, and that flag empties the room. Skipping routing is done by not invoking ``routing`` -- ``select`` still takes ``none`` so CI can leave the job off the plan. @@ -272,17 +279,17 @@ def _empty_room(args: argparse.Namespace) -> None: skill is the exception, and never reaches this function -- there is only one room its skill can be in. """ - if config.wants_no_skills(args.routing_skills): + if config.wants_no_skills(args.routing_room): raise SystemExit( "error: `routing` asks for a routing run and " - "`--routing-skills none` empties the room. Name the skills that " + "`--routing-room none` empties the room. Name the skills that " "go in it, or skip this command." ) available = ", ".join(datasets.skills_with_datasets()) or "(none)" raise SystemExit( "error: `routing` needs the skills that go in the room: " - "`--routing-skills a,b` or `all` for every skill with a dataset. " + "`--routing-room a,b` or `all` for every skill with a dataset. " "Only a repo with one skill gets a default, because who a skill " "competes against is what its routing score means. Skills with a " f"dataset here: {available}." @@ -298,37 +305,44 @@ def _fail_if_expired() -> int | None: return 1 -def _prepare_graded_run(args: argparse.Namespace) -> list[str]: - """Structural checks, model pin, and API reachability. Shared by both graders.""" +def _prepare_graded_run( + args: argparse.Namespace, scope: list[str] | None = None +) -> list[str]: + """Structural checks, model pin, and API reachability. Shared by both graders. + + The structural gate reads `scope`, defaulting to the skills ``--skill`` + selected -- the ones about to be graded, and nobody else. A routing run + passes the room instead, because the room is what it installs and what its + score is about; ``--skill`` there only narrows which of the room's cases + are reported on. + """ if (code := _fail_if_expired()) is not None: raise SystemExit(code) - _structural_or_exit() + selected = _selected_skills(args.skill) + _structural_or_exit(selected if scope is None else sorted(set(scope))) args.model = enforce_model_policy(args.model) or args.model if not args.skip_preflight: ok, detail = check_api_reachable(args.model) if not ok: raise SystemExit(f"error: claude API not reachable -- {detail}") - return _selected_skills(args.skill) + return selected def cmd_routing(args: argparse.Namespace) -> int: - _prepare_graded_run(args) - started = time.time() - + # Who is in the room decides what the structural gate covers, so it is + # settled before anything is checked or any token is spent. routing_set = config.active().routing_set if not routing_set: _empty_room(args) - if not args.routing_skills.strip(): - only = next(iter(routing_set)) - print( - f"[routing] --routing-skills was not given, and {only} is the " - "only skill here with a dataset, so it is the room." - ) + _prepare_graded_run(args, list(routing_set)) + started = time.time() # Pool the routing set's cases: skill Y's positives are skill X's # negatives, which is where most of the false-trigger coverage comes - # from. --skill narrows what is *reported on*, not what is installed. + # from. --skill narrows what is *reported on*, not what is installed, + # unless the room was left unnamed, in which case it is both -- either + # the skills --skill named, or the only skill here with a dataset. cases = datasets.routing_cases(list(routing_set), extended=args.extended) if args.only: cases = datasets.filter_cases(cases, args.only) @@ -443,12 +457,14 @@ def cmd_behavioral(args: argparse.Namespace) -> int: def _add_skills_argument(parser: argparse.ArgumentParser) -> None: parser.add_argument( - "--skills", + "--skills-dir", default="", metavar="GLOB[,GLOB]", help=( - "Globs naming the directories that hold skills. " - f"Default: {','.join(config.DEFAULT_SKILL_GLOBS)}." + "Globs naming the directories that are skills, relative to the " + "repo root: 'skills/*' for a repo that keeps them together, or a " + "path to a single one. Default: every directory in the one this " + "command was run from." ), ) @@ -465,7 +481,7 @@ def _add_docs_argument(parser: argparse.ArgumentParser) -> None: ) -def _add_routing_skills_argument( +def _add_routing_room_argument( parser: argparse.ArgumentParser, *, skip_allowed: bool = True ) -> None: if skip_allowed: @@ -479,13 +495,13 @@ def _add_routing_skills_argument( else: help_text = ( "Skills to install side by side: a list, or `all` for every skill " - "with a dataset. Left out, a repo with one skill runs that skill " - "and a repo with several stops -- who a skill competes against is " - "what its routing score means, so there is nothing sensible to " - "assume." + "with a dataset. Left out, --skill is the room if it was given; " + "otherwise a repo with one skill runs that skill and a repo with " + "several stops -- who a skill competes against is what its routing " + "score means, so there is nothing sensible to assume." ) parser.add_argument( - "--routing-skills", + "--routing-room", default="", metavar="A,B,C", help=help_text, @@ -554,7 +570,7 @@ def _add_timeout_argument(parser: argparse.ArgumentParser) -> None: def _add_routing_arguments(parser: argparse.ArgumentParser) -> None: _add_graded_arguments(parser) - _add_routing_skills_argument(parser, skip_allowed=False) + _add_routing_room_argument(parser, skip_allowed=False) parser.add_argument( "--jobs", type=int, @@ -698,7 +714,7 @@ def build_parser() -> argparse.ArgumentParser: "routing", help="Grade which skill fires, with several installed together.", description=( - "Install the skills named by --routing-skills side by side and " + "Install the skills named by --routing-room side by side and " "grade the trigger decision for every evaluation those skills own. " "A repo with one skill need not name it; a repo with several must, " "because who is in the room is what the score means." @@ -727,7 +743,7 @@ def build_parser() -> argparse.ArgumentParser: formatter_class=argparse.RawDescriptionHelpFormatter, ) _add_skills_argument(select_parser) - _add_routing_skills_argument(select_parser) + _add_routing_room_argument(select_parser) 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.") @@ -842,18 +858,22 @@ def build_parser() -> argparse.ArgumentParser: def _configure(args: argparse.Namespace) -> None: """Make the flags this subcommand was given the active config. - Built twice for a subcommand that takes ``--routing-skills``, because two + Built twice for a subcommand that takes ``--routing-room``, because two of the answers that flag accepts -- ``all``, and the single skill a repo with one of them never had to name -- are questions about which skills ship a dataset, and that is itself answered through the config. The first pass is what makes the repo readable, the second records the answer. + + A blank room with ``--skill`` set is filled from ``--skill`` before that + second pass: naming the skills to grade also names who they sit with, + and an explicit ``--routing-room`` still wins. """ root = Path(args.repo).expanduser() if args.repo else None settings = { name: getattr(args, name, None) for name in ( - "skills", - "routing_skills", + "skills_dir", + "routing_room", "infra_paths", "docs", "excluded_urls", @@ -869,7 +889,11 @@ def _configure(args: argparse.Namespace) -> None: settings["version"] = getattr(args, "version", None) config.use(config.build(root, **settings)) - if settings["routing_skills"] is not None: + if settings["routing_room"] is not None: + if not str(settings["routing_room"]).strip(): + skill = getattr(args, "skill", "") or "" + if skill.strip(): + settings["routing_room"] = skill config.use( config.build(root, **settings, dataset_skills=datasets.skills_with_datasets()) ) diff --git a/skillscope/config.py b/skillscope/config.py index f204351..d5130d0 100644 --- a/skillscope/config.py +++ b/skillscope/config.py @@ -13,8 +13,8 @@ the repo's own workflow:: skillscope select --changed \\ - --skills 'skills/*' \\ - --routing-skills local-ai-use,serving-llms-on-instinct \\ + --skills-dir 'skills/*' \\ + --routing-room local-ai-use,serving-llms-on-instinct \\ --behavior-runner '["self-hosted", "strix_halo"]' \\ --infra-paths .github/workflows/evals.yml @@ -23,13 +23,17 @@ credentials; splitting the other half of the same decision into a second file means two places to read, two places to change, and a file whose only reader is the workflow next to it. Every flag has a default, so a repo that keeps its -skills in ``skills/`` and grades nothing on special hardware passes none of -them. - -The defaults are modest rather than clever. Guessing where skills live by -scanning a whole tree finds vendored copies, fixtures, and a contributor's -local install, and each of those silently changes a routing score; naming the -directory costs one flag and cannot drift. +skills where the command is run and grades nothing on special hardware passes +none of them. + +Every path a flag carries is relative to the repo root, which is the only base +a workflow input, a line of ``git diff --name-only``, and a root-relative +markdown link can all agree on. The one default that is not is +``--skills-dir``, which falls back to the directory the command was typed in: +see `default_skill_globs`. Either way it looks one level down and no further, +because searching a whole tree for every ``SKILL.md`` finds vendored copies, +fixtures, and a contributor's local install, and each of those silently +changes a routing score. """ from __future__ import annotations @@ -47,18 +51,22 @@ # `SKILLSCOPE_SKILLS` is where the skills are, and it is an environment # variable rather than only a flag because the launcher needs the same answer # this does: it looks in a skill's dataset for the version pin before it has -# fetched the harness that could parse a flag. `--skills` still wins. +# fetched the harness that could parse a flag. `--skills-dir` still wins. REPO_ENV = "SKILLSCOPE_REPO" VERSION_ENV = "SKILLSCOPE_VERSION" SKILLS_ENV = "SKILLSCOPE_SKILLS" -DEFAULT_SKILL_GLOBS = ("skills/*",) +# Every directory in the one the command was run from, and no deeper. See +# `default_skill_globs`, which is what actually resolves it; this is the form +# it takes at the repo root, and the fallback when the command was run from +# outside the repo it was pointed at. +DEFAULT_SKILL_GLOBS = ("./*",) # GitHub-hosted Linux, which is what a repo with no runners of its own has. DEFAULT_BEHAVIOR_RUNNER = ("ubuntu-latest",) DEFAULT_BEHAVIOR_OS = ("Linux",) -# The two answers `--routing-skills` takes instead of a list of names: every +# The two answers `--routing-room` takes instead of a list of names: every # skill in the repo that ships a dataset, and no routing run at all. Both are # words rather than shapes of an empty flag, because saying nothing means "work # it out" -- a repo with one skill has only one room its skill can be in -- and @@ -73,7 +81,7 @@ class Config: root: Path - # Globs naming the directories that hold skills. + # Globs naming the directories that are skills. skill_globs: tuple[str, ...] = DEFAULT_SKILL_GLOBS # The skills a routing run installs side by side, in the order given, and @@ -81,7 +89,7 @@ class Config: # never named has become that skill, and `none` -- or a repo with several # skills that said nothing about which of them compete -- has become empty, # which is no routing run. - routing_skills: tuple[str, ...] = () + routing_room: tuple[str, ...] = () # Paths that change the harness rather than one skill, so touching one # re-runs every skill instead of guessing at the blast radius. @@ -143,7 +151,7 @@ def skills(self) -> dict[str, Path]: f"error: two skills are both named '{path.name}' " f"({previous} and {path}). A skill's directory name is " "its identity, so the names have to be unique across " - "the globs passed to --skills." + "the globs passed to --skills-dir." ) found[path.name] = path return found @@ -168,14 +176,14 @@ def routing_set(self) -> dict[str, Path]: the report harder to compare against the flag that produced it. """ skills = self.skills - unknown = [name for name in self.routing_skills if name not in skills] + unknown = [name for name in self.routing_room if name not in skills] if unknown: raise SystemExit( - f"error: --routing-skills names {', '.join(unknown)}, which " + f"error: --routing-room names {', '.join(unknown)}, which " f"{'is' if len(unknown) == 1 else 'are'} not in this repo. " f"Found: {', '.join(sorted(skills)) or '(none)'}." ) - return {name: skills[name] for name in self.routing_skills} + return {name: skills[name] for name in self.routing_room} def base_labels(self, extra: list[str] | tuple[str, ...]) -> list[str]: """The `runs-on` labels a leg starts from, given what it asked for. @@ -213,6 +221,28 @@ def find_root(start: Path | None = None) -> Path: return here +def default_skill_globs(root: Path) -> tuple[str, ...]: + """Every directory in the current one, as a glob relative to `root`. + + A glob that was *passed* is relative to the repo root, because that is the + only base a workflow input, a line of ``git diff --name-only``, and a + root-relative markdown link can all agree on. A glob nobody passed is + relative to the directory the command was typed in, because standing in a + tree of skills and running ``skillscope structural`` can only mean these + ones. Under CI the two are the same: the launcher runs from the repo root. + + The answer is still expressed against `root`, so everything downstream has + one base to reason about rather than two. + """ + try: + here = Path.cwd().resolve().relative_to(root) + except (OSError, ValueError): + # Outside the repo entirely, which is what `--repo` somewhere else + # looks like. Its root is the only directory that can be meant. + return DEFAULT_SKILL_GLOBS + return (f"{here.as_posix()}/*",) + + def _items(value: object, flag: str) -> tuple[str, ...]: """Parse a list-valued flag: a JSON array, or comma-separated values. @@ -251,25 +281,25 @@ def _items(value: object, flag: str) -> tuple[str, ...]: def _is_sentinel(value: object, word: str) -> bool: - items = _items(value, "--routing-skills") + items = _items(value, "--routing-room") return len(items) == 1 and items[0].lower() == word def wants_all_skills(value: object) -> bool: - """Whether a ``--routing-skills`` value is the ``all`` shorthand.""" + """Whether a ``--routing-room`` value is the ``all`` shorthand.""" return _is_sentinel(value, ALL_SKILLS) def wants_no_skills(value: object) -> bool: - """Whether a ``--routing-skills`` value is the ``none`` shorthand.""" + """Whether a ``--routing-room`` value is the ``none`` shorthand.""" return _is_sentinel(value, NO_SKILLS) def build( root: Path | None = None, *, - skills: object = None, - routing_skills: object = None, + skills_dir: object = None, + routing_room: object = None, infra_paths: object = None, docs: object = None, excluded_urls: object = None, @@ -296,11 +326,11 @@ def build( root = (root or find_root()).resolve() globs = ( - _items(skills, "--skills") + _items(skills_dir, "--skills-dir") or _items(os.environ.get(SKILLS_ENV, ""), SKILLS_ENV) - or DEFAULT_SKILL_GLOBS + or default_skill_globs(root) ) - routing = _items(routing_skills, "--routing-skills") + routing = _items(routing_room, "--routing-room") if wants_all_skills(routing): routing = tuple(dataset_skills if dataset_skills is not None else ()) elif wants_no_skills(routing): @@ -315,7 +345,7 @@ def build( return Config( root=root, skill_globs=globs, - routing_skills=routing, + routing_room=routing, infra_paths=frozenset(_items(infra_paths, "--infra-paths")), doc_globs=_items(docs, "--docs"), excluded_urls=_items(excluded_urls, "--exclude-url"), diff --git a/skillscope/datasets.py b/skillscope/datasets.py index a41cce3..b399cc9 100644 --- a/skillscope/datasets.py +++ b/skillscope/datasets.py @@ -519,15 +519,26 @@ def filter_cases(cases: list[Case], only: str) -> list[Case]: return selected -def structural_errors() -> list[str]: +def structural_errors(skills: list[str] | None = None) -> list[str]: """Every structural problem across every dataset, as human-readable strings. Run by CI before any tokens are spent, so a malformed dataset fails in seconds rather than halfway through a paid run. + + Given `skills`, only those skills' datasets are read, plus the shared pool + that every run draws on. That is the scope a graded run gates on: a run + asked for one skill has nothing to say about a neighbour's dataset, and + failing on it would leave the skill under test ungradeable until somebody + else's file is fixed. The repo-wide answer is what ``skillscope + structural`` is for, and it is the check CI gates a merge on. """ errors: list[str] = [] - cases = load_all_cases(errors, extended=True) - declared = set(declared_skills()) + scope = declared_skills() if skills is None else sorted(set(skills)) + cases: list[Case] = [] + for skill in scope: + if dataset_path(skill).is_file(): + cases.extend(load_dataset(skill, errors, extended=True)) + cases.extend(load_shared_negatives(errors)) for case_id in duplicate_ids(cases): errors.append( @@ -545,7 +556,7 @@ def structural_errors() -> list[str]: f"`{case.skill}/{case.workspace}`, which is not a directory." ) - for skill in sorted(declared): + for skill in scope: errors.extend( tier0_errors(skill, [c for c in cases if c.skill == skill and not c.extended]) ) diff --git a/skillscope/references.py b/skillscope/references.py index 41a3fb5..6f10e6d 100644 --- a/skillscope/references.py +++ b/skillscope/references.py @@ -141,17 +141,27 @@ def is_local(self) -> bool: return not parts.scheme and not parts.netloc and bool(parts.path or parts.fragment) -def markdown_files() -> list[Path]: - """Every markdown file the reference checks read, in a stable order.""" +def markdown_files(skills: list[str] | None = None) -> list[Path]: + """Every markdown file the reference checks read, in a stable order. + + Given `skills`, only those skills' folders are read, and ``--docs`` is + left out with them: a run narrowed to one skill is gated on that skill's + prose, not on a neighbour's or on the repo's docs tree. + """ cfg = config.active() + folders = ( + cfg.skills + if skills is None + else {name: cfg.skill_path(name) for name in sorted(set(skills))} + ) found: list[Path] = [] - for _, folder in sorted(cfg.skills.items()): + for _, folder in sorted(folders.items()): found.extend( path for path in sorted(folder.rglob("*")) if path.suffix.lower() in MARKDOWN_SUFFIXES and path.is_file() ) - for pattern in cfg.doc_globs: + for pattern in cfg.doc_globs if skills is None else (): found.extend( path for path in sorted(cfg.root.glob(pattern)) @@ -211,11 +221,17 @@ def _targets(line: str) -> list[str]: return [target for target in cleaned if target] -def collect(files: list[Path] | None = None) -> list[Reference]: - """Every reference in `files`, deduplicated per file, line, and target.""" +def collect( + files: list[Path] | None = None, *, skills: list[str] | None = None +) -> list[Reference]: + """Every reference in `files`, deduplicated per file, line, and target. + + Without `files`, the markdown :func:`markdown_files` finds, narrowed to + `skills` when a caller only wants those skills held to the bar. + """ found: list[Reference] = [] seen: set[tuple[Path, int, str]] = set() - for path in markdown_files() if files is None else files: + for path in markdown_files(skills) if files is None else files: try: text = path.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError): diff --git a/skillscope/routing.py b/skillscope/routing.py index 1981710..fbaac65 100644 --- a/skillscope/routing.py +++ b/skillscope/routing.py @@ -16,7 +16,7 @@ * false trigger -- no skill was expected and one activated (over-triggering). Which skills are in the room is the workflow's decision, passed in as -``--routing-skills``. Wherever there is a choice it has to be a decision +``--routing-room``. Wherever there is a choice it has to be a decision someone makes deliberately, because that set is what the number means: a skill tested alongside two neighbours is answering a harder question than one tested alongside none. A repo with a single skill has no choice to make and so makes diff --git a/skillscope/select.py b/skillscope/select.py index 3767719..266dde5 100644 --- a/skillscope/select.py +++ b/skillscope/select.py @@ -172,7 +172,7 @@ def routing_needed(changed: set[str], extended: bool = True) -> bool: runs. """ cfg = config.active() - routing_set = set(cfg.routing_skills) + routing_set = set(cfg.routing_room) if not routing_set: return False if changed & infra_paths(): @@ -218,7 +218,7 @@ def plan( skills, labels, ignore_gates=ignore_gates, extended=extended ) return { - "routing": routing and bool(config.active().routing_skills), + "routing": routing and bool(config.active().routing_room), "extended": extended, # Routing installs several skills in one session, so it runs at the # version this run is already using; a per-skill pin governs that diff --git a/skillscope/structure.py b/skillscope/structure.py index b4a9b1b..171eaf5 100644 --- a/skillscope/structure.py +++ b/skillscope/structure.py @@ -65,13 +65,36 @@ _SECTION = re.compile(r"^##\s+(?P.+?)\s*$") -def errors() -> list[str]: - """Every structural problem with every skill folder in the repo.""" +def errors(skills: list[str] | None = None) -> list[str]: + """Every structural problem with every skill folder in the repo. + + Given `skills`, only those folders. Otherwise the survey is repo-wide, and + finding no skill at all is itself the finding. + """ cfg = config.active() found: list[str] = [] - for skill in sorted(cfg.skills): + for skill in sorted(cfg.skills if skills is None else set(skills)): found.extend(skill_errors(skill)) - found.extend(_undeclared(cfg)) + if skills is None and not cfg.skills and not cfg.doc_globs: + # A directory the globs matched that holds no SKILL.md is simply not a + # skill, and saying so for every one of them would be noise. Matching + # none of them is different: the run graded nothing and reported that + # as a pass, which is the one way a green check here can lie about a + # repo. A run given --docs still has prose to check, and a repo that + # keeps prose and no skills is entitled to check it. + # + # Suggesting a glob to a caller who just passed one would be + # suggesting the glob that found nothing. + example = ( + ", such as 'skills/*'" + if cfg.skill_globs == config.default_skill_globs(cfg.root) + else "" + ) + found.append( + f"no skill found under {', '.join(cfg.skill_globs)} in {cfg.root}. " + f"A skill is a directory holding a {SKILL_FILE}, and --skills-dir " + f"names those directories rather than the one above them{example}." + ) return found @@ -264,26 +287,3 @@ def sections(text: str) -> dict[str, str]: found[title] = "\n".join(body).strip() return found - -def _undeclared(cfg: config.Config) -> list[str]: - """Directories the skill globs match that hold no SKILL.md. - - Discovery ignores them, which is the problem: a skill whose SKILL.md was - never added, or was added under another name, is not graded, not routed, - and not reported -- it simply is not there. Saying so costs one line and - the fix is either the missing file or a narrower glob. - """ - found: set[str] = set() - for pattern in cfg.skill_globs: - for path in cfg.root.glob(pattern): - if not path.is_dir() or path.name.startswith("."): - continue - if (path / SKILL_FILE).is_file(): - continue - shown = path.relative_to(cfg.root).as_posix() - found.add( - f"{shown}: no {SKILL_FILE}, so nothing in this directory is " - "graded. Add one, or narrow --skills so the directory is not " - "taken for a skill." - ) - return sorted(found) diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index cb34881..3afb294 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -117,7 +117,7 @@ def skill( hooks: str | None = None, workspace: dict[str, str] | None = None, description: str = "", - where: str = "skills", + where: str = ".", ) -> Path: folder = self.root / where / name (folder / "evals").mkdir(parents=True, exist_ok=True) @@ -146,6 +146,15 @@ def skill( # the repo mid-test to see what the structural checks make of the result. return folder + def chdir(self, relative: str = ".") -> None: + """Run the rest of the test from a directory inside this repo. + + Registered after the temp directory's own cleanup, so it is undone + first: Windows will not remove a directory that is the cwd. + """ + self.test.addCleanup(os.chdir, os.getcwd()) + os.chdir(self.root / relative) + def activate(self, **settings) -> config.Config: """Configure the harness for this repo the way the CLI's flags would.""" self.settings = settings @@ -280,7 +289,7 @@ def setUp(self) -> None: self.repo.activate(behavior_os=["Linux", "Windows"]) def plan(self, text: str) -> dict: - path = self.repo.root / "skills" / "demo-skill" / "evals" / "machine.yml" + path = self.repo.root / "demo-skill" / "evals" / "machine.yml" path.write_text(text, encoding="utf-8") return datasets.machine_plan("demo-skill") @@ -326,7 +335,7 @@ def test_the_minimum_useful_files(self) -> None: def test_structural_checks_report_a_broken_machine_file_rather_than_raising(self) -> None: # `skillscope structural` has to survey every skill, so one bad file is # a reported error, not an abandoned run. - (self.repo.root / "skills" / "demo-skill" / "evals" / "machine.yml").write_text( + (self.repo.root / "demo-skill" / "evals" / "machine.yml").write_text( "labels: nope\n", encoding="utf-8" ) self.assertTrue(any("`labels`" in e for e in datasets.structural_errors())) @@ -338,22 +347,63 @@ class TestConfig(unittest.TestCase): def test_a_repo_that_configures_nothing_still_works(self) -> None: repo = Repo(self) repo.skill("demo-skill", dataset=tier0_dataset("demo")) + repo.chdir() cfg = repo.activate() self.assertEqual(cfg.skill_globs, config.DEFAULT_SKILL_GLOBS) - self.assertEqual(cfg.routing_skills, ()) + self.assertEqual(cfg.routing_room, ()) self.assertEqual(datasets.declared_skills(), ["demo-skill"]) def test_skills_can_live_anywhere_the_globs_say(self) -> None: repo = Repo(self) repo.skill("shipped", dataset=tier0_dataset("shipped"), where="agents/skills") - repo.activate(skills="agents/skills/*") + repo.activate(skills_dir="agents/skills/*") self.assertEqual(datasets.declared_skills(), ["shipped"]) self.assertTrue(datasets.dataset_path("shipped").is_file()) + def test_the_default_looks_one_level_down_and_no_further(self) -> None: + # Deep enough for a repo that keeps its skills where you are standing, + # shallow enough that a vendored copy further down is not found and + # silently graded. + repo = Repo(self) + repo.skill("at-the-root", dataset=tier0_dataset("root")) + repo.skill("buried", dataset=tier0_dataset("buried"), where="vendor/skills") + repo.chdir() + repo.activate() + self.assertEqual(datasets.declared_skills(), ["at-the-root"]) + + def test_the_default_is_the_directory_the_command_was_run_from(self) -> None: + # A repo whose skills sit a level down is the usual layout, and + # `cd skills && skillscope structural` is what a person does about it. + # The glob is still reported against the root, which is the base every + # other path in a run is measured from. + repo = Repo(self) + repo.skill("shipped", dataset=tier0_dataset("shipped"), where="skills") + repo.chdir("skills") + cfg = repo.activate() + self.assertEqual(cfg.skill_globs, ("skills/*",)) + self.assertEqual(datasets.declared_skills(), ["shipped"]) + + def test_a_command_run_from_outside_the_repo_gets_that_repos_root(self) -> None: + # `--repo somewhere-else` is not standing anywhere in it, so the only + # directory it can mean is the root it was handed. + repo = Repo(self) + repo.skill("at-the-root", dataset=tier0_dataset("root")) + self.assertEqual(repo.activate().skill_globs, config.DEFAULT_SKILL_GLOBS) + self.assertEqual(datasets.declared_skills(), ["at-the-root"]) + + def test_a_glob_that_was_passed_is_relative_to_the_root_not_the_cwd(self) -> None: + # Every other path flag is root-relative, and a workflow that names + # `skills/*` means the same thing wherever its runner happens to be. + repo = Repo(self) + repo.skill("shipped", dataset=tier0_dataset("shipped"), where="skills") + repo.chdir("skills") + repo.activate(skills_dir="skills/*") + self.assertEqual(datasets.declared_skills(), ["shipped"]) + def test_a_directory_without_a_skill_file_is_not_a_skill(self) -> None: repo = Repo(self) repo.skill("real-skill", dataset=tier0_dataset("real")) - (repo.root / "skills" / "notes").mkdir(parents=True) + (repo.root / "notes").mkdir(parents=True) repo.activate() self.assertEqual(datasets.declared_skills(), ["real-skill"]) @@ -391,7 +441,8 @@ def test_the_skill_globs_can_come_from_the_environment(self) -> None: with mock.patch.dict(os.environ, {config.SKILLS_ENV: "agents/skills/*"}): self.assertEqual(config.build(repo.root).skill_globs, ("agents/skills/*",)) self.assertEqual( - config.build(repo.root, skills="skills/*").skill_globs, ("skills/*",) + config.build(repo.root, skills_dir="skills/*").skill_globs, + ("skills/*",), ) def test_the_version_comes_from_the_environment_when_unset(self) -> None: @@ -410,12 +461,12 @@ def setUp(self) -> None: self.repo.skill("two", dataset=tier0_dataset("two")) def test_the_listed_skills_are_what_gets_installed(self) -> None: - cfg = self.repo.activate(routing_skills="one,two") + cfg = self.repo.activate(routing_room="one,two") self.assertEqual(list(cfg.routing_set), ["one", "two"]) - self.assertEqual(cfg.routing_set["one"], self.repo.root / "skills" / "one") + self.assertEqual(cfg.routing_set["one"], self.repo.root / "one") def test_an_unlisted_skill_is_not_in_the_room(self) -> None: - cfg = self.repo.activate(routing_skills="one") + cfg = self.repo.activate(routing_room="one") self.assertEqual(list(cfg.routing_set), ["one"]) def test_listing_nothing_where_there_is_a_choice_means_no_routing_run(self) -> None: @@ -428,10 +479,10 @@ def test_all_stands_for_every_skill_with_a_dataset(self) -> None: # Resolved by the CLI, which is where "has a dataset" can be answered. cfg = config.build( self.repo.root, - routing_skills="all", + routing_room="all", dataset_skills=["one", "two"], ) - self.assertEqual(cfg.routing_skills, ("one", "two")) + self.assertEqual(cfg.routing_room, ("one", "two")) def test_none_says_no_routing_run_outright(self) -> None: # The one way to turn routing off, and it survives a repo having only @@ -439,18 +490,18 @@ def test_none_says_no_routing_run_outright(self) -> None: for available in (["one"], ["one", "two"]): with self.subTest(available=available): cfg = config.build( - self.repo.root, routing_skills="none", dataset_skills=available + self.repo.root, routing_room="none", dataset_skills=available ) - self.assertEqual(cfg.routing_skills, ()) + self.assertEqual(cfg.routing_room, ()) def test_a_skill_that_does_not_exist_is_refused(self) -> None: - cfg = self.repo.activate(routing_skills="one,ghost") + cfg = self.repo.activate(routing_room="one,ghost") with self.assertRaises(SystemExit) as caught: cfg.routing_set # noqa: B018 -- the property is the assertion self.assertIn("ghost", str(caught.exception)) def test_the_listed_order_is_kept(self) -> None: - cfg = self.repo.activate(routing_skills="two,one") + cfg = self.repo.activate(routing_room="two,one") self.assertEqual(list(cfg.routing_set), ["two", "one"]) @@ -477,7 +528,7 @@ def test_it_is_the_room_for_planning_too(self) -> None: # Otherwise the flag would be redundant on the runner and still # required for CI to schedule the job that runs it. self.resolve(infra_paths=".github/workflows/evals.yml") - self.assertTrue(select_module.routing_needed({"skills/only-skill/SKILL.md"})) + self.assertTrue(select_module.routing_needed({"only-skill/SKILL.md"})) self.assertTrue( select_module.plan(["only-skill"], routing=True, labels=set())["routing"] ) @@ -493,7 +544,7 @@ def test_a_skill_without_a_dataset_is_not_a_candidate(self) -> None: self.assertEqual(list(self.resolve().routing_set), ["only-skill"]) def test_saying_none_still_turns_routing_off(self) -> None: - cfg = self.resolve(routing_skills="none") + cfg = self.resolve(routing_room="none") self.assertEqual(cfg.routing_set, {}) self.assertFalse( select_module.plan(["only-skill"], routing=True, labels=set())["routing"] @@ -508,7 +559,7 @@ def test_the_cli_resolves_the_room_before_the_command_runs(self) -> None: ["--repo", str(self.repo.root), "routing"] ) ) - self.assertEqual(config.active().routing_skills, ("only-skill",)) + self.assertEqual(config.active().routing_room, ("only-skill",)) class TestARoutingRunWithNobodyInTheRoom(unittest.TestCase): @@ -532,8 +583,31 @@ def test_a_repo_with_a_choice_to_make_is_told_what_its_options_are(self) -> None def test_asking_for_routing_and_emptying_the_room_is_a_contradiction(self) -> None: with self.assertRaises(SystemExit) as caught: - cli._empty_room(self.args("--routing-skills", "none")) - self.assertIn("--routing-skills none", str(caught.exception)) + cli._empty_room(self.args("--routing-room", "none")) + self.assertIn("--routing-room none", str(caught.exception)) + + def configure(self, *argv) -> None: + cli._configure( + cli.build_parser().parse_args( + ["--repo", str(self.repo.root), "routing", *argv] + ) + ) + + def test_skill_names_the_room_when_the_room_was_not(self) -> None: + self.configure("--skill", "one") + self.assertEqual(config.active().routing_room, ("one",)) + + def test_skill_can_name_several_as_the_room(self) -> None: + self.configure("--skill", "two,one") + self.assertEqual(config.active().routing_room, ("two", "one")) + + def test_an_explicit_room_wins_over_skill(self) -> None: + self.configure("--routing-room", "two", "--skill", "one") + self.assertEqual(config.active().routing_room, ("two",)) + + def test_none_is_an_explicit_room_and_stays_empty(self) -> None: + self.configure("--routing-room", "none", "--skill", "one") + self.assertEqual(config.active().routing_room, ()) class TestCommands(unittest.TestCase): @@ -558,7 +632,7 @@ def test_neither_grader_has_a_mode_flag(self) -> None: def test_behavioral_does_not_take_routing_flags(self) -> None: with contextlib.redirect_stderr(io.StringIO()): with self.assertRaises(SystemExit): - cli.build_parser().parse_args(["behavioral", "--routing-skills", "all"]) + cli.build_parser().parse_args(["behavioral", "--routing-room", "all"]) with self.assertRaises(SystemExit): cli.build_parser().parse_args(["behavioral", "--min-accuracy", "0"]) @@ -748,13 +822,13 @@ def setUp(self) -> None: self.repo.skill("alpha", dataset=self.graded) self.repo.skill("beta", dataset=tier0_dataset("beta")) self.repo.activate( - routing_skills="alpha,beta", + routing_room="alpha,beta", infra_paths=".github/workflows/evals.yml", ) def gpu(self, skill: str = "alpha") -> None: """Give `skill` a machine.yml asking for hardware this repo rations.""" - (self.repo.root / "skills" / skill / "evals" / "machine.yml").write_text( + (self.repo.root / skill / "evals" / "machine.yml").write_text( "labels: [gpu]\n", encoding="utf-8" ) self.repo.reactivate( @@ -769,7 +843,7 @@ def test_only_skills_with_gradeable_behavior_get_a_leg(self) -> None: def test_a_touched_skill_is_selected(self) -> None: self.assertEqual( - select_module.select_from_changes({"skills/alpha/SKILL.md"}), ["alpha"] + select_module.select_from_changes({"alpha/SKILL.md"}), ["alpha"] ) def test_an_infra_path_selects_everything(self) -> None: @@ -784,21 +858,21 @@ def test_an_unrelated_change_selects_nothing(self) -> None: self.assertEqual(select_module.select_from_changes({"README.md"}), []) def test_a_description_change_buys_a_routing_run(self) -> None: - self.assertTrue(select_module.routing_needed({"skills/alpha/SKILL.md"})) + self.assertTrue(select_module.routing_needed({"alpha/SKILL.md"})) def test_a_dataset_change_buys_a_routing_run(self) -> None: - self.assertTrue(select_module.routing_needed({"skills/beta/evals/evals.json"})) + self.assertTrue(select_module.routing_needed({"beta/evals/evals.json"})) def test_a_reference_file_under_a_skill_does_not(self) -> None: - self.assertFalse(select_module.routing_needed({"skills/alpha/reference.md"})) + self.assertFalse(select_module.routing_needed({"alpha/reference.md"})) def test_an_unlisted_skills_description_is_not_a_routing_input(self) -> None: self.repo.skill("draft", dataset=tier0_dataset("draft")) - self.assertFalse(select_module.routing_needed({"skills/draft/SKILL.md"})) + self.assertFalse(select_module.routing_needed({"draft/SKILL.md"})) def test_with_no_routing_set_nothing_buys_a_routing_run(self) -> None: - self.repo.reactivate(routing_skills="") - self.assertFalse(select_module.routing_needed({"skills/alpha/SKILL.md"})) + self.repo.reactivate(routing_room="") + self.assertFalse(select_module.routing_needed({"alpha/SKILL.md"})) self.assertFalse( select_module.routing_needed({".github/workflows/evals.yml"}) ) @@ -836,7 +910,7 @@ def test_credentials_split_the_two_matrices(self) -> None: def test_hardware_with_no_environment_stays_in_one_matrix(self) -> None: # Only credentials force a second job. A repo that rations a pool but # pays for it out of the same key should not get an extra one. - (self.repo.root / "skills" / "alpha" / "evals" / "machine.yml").write_text( + (self.repo.root / "alpha" / "evals" / "machine.yml").write_text( "labels: [gpu]\n", encoding="utf-8" ) self.repo.reactivate(scoped_runner="self-hosted", scoped_gate="enable_gpu_ci") @@ -1065,7 +1139,7 @@ def setUp(self) -> None: workspace={"evals/files/stub/main.py": "print('hi')\n"}, ) self.repo.skill("beta", dataset=tier0_dataset("beta")) - self.repo.activate(routing_skills="alpha,beta") + self.repo.activate(routing_room="alpha,beta") def test_a_healthy_repo_checks_out_clean(self) -> None: self.assertEqual(datasets.structural_errors(), []) @@ -1101,7 +1175,7 @@ def test_every_skill_with_a_dataset_is_a_declared_skill(self) -> None: def test_every_listed_skill_brings_prompts_to_the_routing_run(self) -> None: # A skill in the room with no gradeable prompt of its own would silently # drop out of the score rather than failing. - listed = config.active().routing_skills + listed = config.active().routing_room cases = datasets.routing_cases(list(listed)) covered = {case.expect_skill for case in cases if case.expect_skill} self.assertEqual(sorted(covered), sorted(listed)) @@ -1134,6 +1208,85 @@ def test_template_is_a_valid_dataset(self) -> None: self.assertEqual(datasets.tier0_errors("alpha", cases), []) +class TestTheGateAPaidRunPassesFirst(unittest.TestCase): + """Which skills the structural checks cover before any tokens are spent. + + A run asked for one skill is gated on that skill. Holding it back for a + neighbour's mistake would make one skill's malformed file everybody else's + problem, and the repo-wide answer is what `structural` is for. + """ + + def setUp(self) -> None: + self.repo = Repo(self) + self.repo.skill("alpha", dataset=tier0_dataset("alpha")) + # The neighbour, with a machine.yml naming a key that does not exist. + self.repo.skill( + "beta", dataset=tier0_dataset("beta"), machine="runner_type: mi300x\n" + ) + self.repo.activate(routing_room="alpha,beta") + + def args(self, *argv) -> argparse.Namespace: + return cli.build_parser().parse_args([*argv, "--skip-preflight"]) + + def fails(self, call, *arguments) -> str: + """The stderr of a gate that stopped the run.""" + stderr = io.StringIO() + with contextlib.redirect_stderr(stderr), self.assertRaises(SystemExit): + call(*arguments) + return stderr.getvalue() + + def test_a_neighbours_mistake_does_not_stop_a_run_of_one_skill(self) -> None: + selected = cli._prepare_graded_run(self.args("behavioral", "--skill", "alpha")) + self.assertEqual(selected, ["alpha"]) + + def test_the_selected_skills_own_mistake_still_stops_it(self) -> None: + stderr = self.fails( + cli._prepare_graded_run, self.args("behavioral", "--skill", "beta") + ) + self.assertIn("runner_type", stderr) + + def test_routing_is_gated_on_everyone_in_the_room(self) -> None: + # A routing score is about all of them, so all of them are read -- + # whichever one --skill narrows the report to. + stderr = self.fails( + cli._prepare_graded_run, + self.args("routing", "--skill", "alpha"), + ["alpha", "beta"], + ) + self.assertIn("runner_type", stderr) + + def test_routing_reads_the_room_and_not_the_repo(self) -> None: + # A routing run leaves --skill off, so the gate has to take its scope + # from the room rather than from "every skill that has a dataset". + self.repo.reactivate(routing_room="alpha") + args = self.args("routing", "--routing-room", "alpha") + self.assertEqual(cli._prepare_graded_run(args, ["alpha"]), ["alpha", "beta"]) + + def test_the_repo_wide_check_still_covers_the_repo(self) -> None: + self.assertIn("runner_type", self.fails(cli._structural_or_exit)) + + def test_a_neighbours_broken_link_is_not_this_runs_problem(self) -> None: + (self.repo.root / "beta" / "reference.md").write_text( + "[gone](./nowhere.md)\n", encoding="utf-8" + ) + self.assertIn("nowhere.md", self.fails(cli._structural_or_exit)) + self.assertEqual(cli._structural_or_exit(["alpha"]), []) + + def test_the_docs_tree_waits_for_the_repo_wide_check(self) -> None: + # --docs is a repo's own markdown, so it belongs to no skill's run. + self.repo.reactivate(docs="*.md") + (self.repo.root / "README.md").write_text("[gone](./nowhere.md)\n", encoding="utf-8") + self.assertIn("README.md", self.fails(cli._structural_or_exit)) + self.assertEqual(cli._structural_or_exit(["alpha"]), []) + + def test_a_duplicate_id_outside_the_scope_is_the_repo_wide_checks_business(self) -> None: + self.repo.skill("gamma", dataset=tier0_dataset("alpha")) + self.assertEqual(datasets.structural_errors(["alpha"]), []) + self.assertTrue( + any("duplicate case id" in e for e in datasets.structural_errors()) + ) + + class TestSkillStructure(unittest.TestCase): """The skill folder itself: what the format requires, and what a repo adds.""" @@ -1143,9 +1296,7 @@ def setUp(self) -> None: self.repo.activate() def write(self, text: str, skill: str = "demo-skill") -> None: - (self.repo.root / "skills" / skill / "SKILL.md").write_text( - text, encoding="utf-8" - ) + (self.repo.root / skill / "SKILL.md").write_text(text, encoding="utf-8") def declares(self, frontmatter: str, body: str = "\n# Demo\n") -> None: self.write(f"---\n{frontmatter}\n---\n{body}") @@ -1214,14 +1365,12 @@ def test_blank_lines_around_the_body_do_not_count_against_it(self) -> None: self.declares("name: demo-skill\ndescription: Does things.", f"\n\n{lines}\n\n\n") self.assertEqual(structure.errors(), []) - def test_a_globbed_directory_with_no_skill_file_is_reported(self) -> None: - # It is not a skill, so nothing grades it, routes it, or reports on it. - # Silence there is the failure: either the file is missing or the glob - # is too wide, and both are worth one line. - (self.repo.root / "skills" / "notes").mkdir(parents=True) - errors = structure.errors() - self.assertEqual(len(errors), 1, errors) - self.assertIn("skills/notes", errors[0]) + def test_a_globbed_directory_with_no_skill_file_is_passed_over(self) -> None: + # A directory holding no SKILL.md is not a skill, and the default glob + # matches every directory in the repo, so reporting one would be a line + # per README folder. + (self.repo.root / "notes").mkdir(parents=True) + self.assertEqual(structure.errors(), []) def test_a_repo_that_requires_nothing_extra_requires_nothing_extra(self) -> None: self.assertEqual(structure.errors(), []) @@ -1270,6 +1419,41 @@ def test_a_malformed_skill_stops_a_run_before_it_spends_anything(self) -> None: cli._structural_or_exit() +class TestARepoWhereNoSkillWasFound(unittest.TestCase): + """Grading nothing is reported, because a green check for it would lie.""" + + def test_finding_no_skill_is_itself_the_finding(self) -> None: + repo = Repo(self) + repo.activate() + errors = structure.errors() + self.assertEqual(len(errors), 1, errors) + self.assertIn("no skill found", errors[0]) + + def test_the_globs_that_matched_nothing_are_named(self) -> None: + # A glob pointing somewhere the skills are not is the usual cause, so + # the message has to say which one was used. + repo = Repo(self) + repo.skill("shipped", dataset=tier0_dataset("shipped"), where="agents/skills") + repo.activate(skills_dir="skills/*") + self.assertIn("skills/*", structure.errors()[0]) + + def test_an_example_glob_is_offered_only_to_a_caller_who_passed_none(self) -> None: + # Suggesting one to a caller who just passed one would be suggesting + # the glob that found nothing. + repo = Repo(self) + repo.activate() + self.assertIn("such as", structure.errors()[0]) + repo.reactivate(skills_dir="skills/*") + self.assertNotIn("such as", structure.errors()[0]) + + def test_a_run_asked_for_docs_still_has_work_to_do(self) -> None: + # --docs is a repo checking its own prose, which is a real run in a + # repo that ships no skill at all. + repo = Repo(self) + repo.activate(docs="*.md") + self.assertEqual(structure.errors(), []) + + def targets(text: str) -> list[str]: """Every reference the extractor finds in one markdown document.""" with tempfile.TemporaryDirectory() as tmp: @@ -1410,10 +1594,10 @@ def test_a_fragment_on_something_that_is_not_markdown_is_left_alone(self) -> Non self.assertEqual(self.errors(), []) def test_root_relative_links_resolve_from_the_repo_root(self) -> None: - self.write("SKILL.md", "[ok](/skills/demo/SKILL.md) [no](/skills/demo/gone.md)\n") + self.write("SKILL.md", "[ok](/demo/SKILL.md) [no](/demo/gone.md)\n") errors = self.errors() self.assertEqual(len(errors), 1, errors) - self.assertIn("/skills/demo/gone.md", errors[0]) + self.assertIn("/demo/gone.md", errors[0]) def test_percent_encoding_is_decoded_before_the_file_is_looked_for(self) -> None: self.write("a file.md", "# Spaced\n") @@ -1686,7 +1870,7 @@ def test_the_routing_set_lands_in_the_workspace_and_nothing_else(self) -> None: repo.skill("one", dataset=tier0_dataset("one")) repo.skill("two", dataset=tier0_dataset("two")) repo.skill("unlisted", dataset=tier0_dataset("unlisted")) - cfg = repo.activate(routing_skills="one,two") + cfg = repo.activate(routing_room="one,two") workspace = routing.stage_workspace(cfg.routing_set) try: staged = sorted(p.name for p in (workspace / ".claude" / "skills").iterdir()) @@ -2018,7 +2202,7 @@ def setUp(self) -> None: self.repo = Repo(self) self.repo.skill("listed", dataset=tier0_dataset("listed")) self.repo.skill("unlisted", dataset=tier0_dataset("unlisted")) - self.repo.activate(routing_skills="listed") + self.repo.activate(routing_room="listed") def test_a_listed_skill_brings_both_kinds_of_prompt(self) -> None: # Its positives are the room's positives; its near misses assert that diff --git a/tools/claude_eval.py b/tools/claude_eval.py index ef7d72c..4636934 100644 --- a/tools/claude_eval.py +++ b/tools/claude_eval.py @@ -246,12 +246,13 @@ def main() -> None: help="Root of the repo holding the skills. Default: the enclosing checkout.", ) parser.add_argument( - "--skills", + "--skills-dir", default="", metavar="GLOB[,GLOB]", help=( - "Globs naming the directories that hold skills. " - f"Default: {','.join(config.DEFAULT_SKILL_GLOBS)}." + "Globs naming the directories that are skills, relative to the " + "repo root. Default: every directory in the one this command was " + "run from." ), ) parser.add_argument( @@ -293,7 +294,8 @@ def main() -> None: extra_args = [a for a in extra_args if a != "--"] config.use( config.build( - Path(args.repo).expanduser() if args.repo else None, skills=args.skills + Path(args.repo).expanduser() if args.repo else None, + skills_dir=args.skills_dir, ) ) diff --git a/tools/compare_skill.py b/tools/compare_skill.py index aa5b88c..c9471b0 100644 --- a/tools/compare_skill.py +++ b/tools/compare_skill.py @@ -208,12 +208,13 @@ def main() -> None: help="Root of the repo holding the skills. Default: the enclosing checkout.", ) parser.add_argument( - "--skills", + "--skills-dir", default="", metavar="GLOB[,GLOB]", help=( - "Globs naming the directories that hold skills. " - f"Default: {','.join(config.DEFAULT_SKILL_GLOBS)}." + "Globs naming the directories that are skills, relative to the " + "repo root. Default: every directory in the one this command was " + "run from." ), ) parser.add_argument( @@ -272,7 +273,8 @@ def main() -> None: args = parser.parse_args() config.use( config.build( - Path(args.repo).expanduser() if args.repo else None, skills=args.skills + Path(args.repo).expanduser() if args.repo else None, + skills_dir=args.skills_dir, ) ) diff --git a/tools/verify_selection_parity.py b/tools/verify_selection_parity.py index 363f4c9..48b9ee8 100644 --- a/tools/verify_selection_parity.py +++ b/tools/verify_selection_parity.py @@ -31,9 +31,9 @@ # What amd/skills' workflow passes. Mirrors .github/workflows/evals.yml there. SETTINGS = [ - "--skills", + "--skills-dir", "skills/*", - "--routing-skills", + "--routing-room", "local-ai-use,local-ai-app-integration,serving-llms-on-instinct," "tracelens-analysis-orchestrator,hyperloom-workload-optimizer", "--infra-paths",