From 1eaa98d7407c792659a802f203924e54ffd07ede Mon Sep 17 00:00:00 2001 From: Davide Polato Date: Thu, 29 Oct 2026 10:26:07 +0100 Subject: [PATCH] fix(skill-evals): point write-skill step 5 at its section and check every binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1331 renamed write-skill's "### Step 5 — Apply the security checklist" to "## Step 5 — Secure it, if it reads outside content" and moved the rules to security-checklist.md. The step-5-security-checklist step-config still named the old heading, so the runner could not build the prompt and the suite failed before reaching a model. The step-config now names the new heading and adds security-checklist.md through also_include, since the output spec refers to the checklist's patterns. skill-eval --check builds every step's system prompt without running a case and exits 1 naming each fixtures dir whose prompt cannot be built. The check-eval-prompts prek hook runs it over tools/skill-evals/evals whenever a Markdown file changes, so a renamed heading fails in the PR that renames it. Signed-off-by: Davide Polato Generated-by: Claude Code (Opus 5.5) --- .pre-commit-config.yaml | 11 ++++++++ tools/skill-evals/README.md | 4 +++ .../fixtures/step-config.json | 5 +++- tools/skill-evals/src/skill_evals/runner.py | 19 ++++++++++++++ tools/skill-evals/tests/test_runner.py | 26 +++++++++++++++++++ 5 files changed, 64 insertions(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index c5c3528b9..5b9a9c746 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -488,6 +488,17 @@ repos: entry: uv run --project tools/skill-token-count skill-token-count --check files: ^(skills/.*/SKILL\.md|plugins/magpie-[^/]+/skills/.*/SKILL\.md|tools/dev/preflight-block\.md|tools/skill-token-count/.*|uv\.lock|\.pre-commit-config\.yaml)$ pass_filenames: false + # Every eval step builds its system prompt from the Markdown section its + # step-config.json names. Builds each one without running a case, so a + # renamed heading or a moved file fails here instead of in the next eval run. + - repo: local + hooks: + - id: check-eval-prompts + name: check-eval-prompts (each eval step-config still finds its section) + language: system + entry: uv run --project tools/skill-evals skill-eval tools/skill-evals/evals --check + files: (\.md$|^tools/skill-evals/|^\.pre-commit-config\.yaml$) + pass_filenames: false # Quick-start screenshot set. Three lists have to agree — the harnesses # `capture-screenshot.sh` offers, the ecosystem manifests that make a harness # real, and the PNGs on disk — and none of the existing gates read images, so diff --git a/tools/skill-evals/README.md b/tools/skill-evals/README.md index e3390bf25..b2ba2748b 100644 --- a/tools/skill-evals/README.md +++ b/tools/skill-evals/README.md @@ -535,6 +535,10 @@ Print mode supports manual review, while `--cli` runs the configured model and c CI can use a deterministic stand-in CLI for harness behavior; live model runs remain an explicit pre-flight because they require provider credentials and can be nondeterministic. +`--check` builds every step's system prompt without running a case, +and names each fixtures dir whose prompt cannot be built, such as a `step_heading` the `SKILL.md` no longer has. +The `check-eval-prompts` prek hook runs it over `evals/` whenever a Markdown file changes. + One challenge is model non-determinism. Most cases in this suite have clear, unambiguous correct answers and should pass reliably on a single run. A small number sit closer to a decision boundary — the MEDIUM dedup verdicts and DEFENSE-IN-DEPTH vs NOT-CVE-WORTHY edge cases in particular — and may benefit from being run twice before failing a build. Running every case multiple times would be excessive and slow; the better approach is to tag genuinely borderline cases explicitly and apply a retry budget only to those. Coverage gaps are also more consequential under CI than in manual review. A missing step is invisible to the suite and produces an implicit green signal — worth filling before the suite result is used to gate anything. diff --git a/tools/skill-evals/evals/write-skill/step-5-security-checklist/fixtures/step-config.json b/tools/skill-evals/evals/write-skill/step-5-security-checklist/fixtures/step-config.json index 7949f351e..f915d73ed 100644 --- a/tools/skill-evals/evals/write-skill/step-5-security-checklist/fixtures/step-config.json +++ b/tools/skill-evals/evals/write-skill/step-5-security-checklist/fixtures/step-config.json @@ -1,4 +1,7 @@ { "skill_md": "skills/write-skill/SKILL.md", - "step_heading": "### Step 5 — Apply the security checklist" + "step_heading": "## Step 5 — Secure it, if it reads outside content", + "also_include": [ + "plugins/magpie-utilities/skills/write-skill/security-checklist.md" + ] } diff --git a/tools/skill-evals/src/skill_evals/runner.py b/tools/skill-evals/src/skill_evals/runner.py index 99da58623..726548f3e 100644 --- a/tools/skill-evals/src/skill_evals/runner.py +++ b/tools/skill-evals/src/skill_evals/runner.py @@ -1260,6 +1260,15 @@ def main(argv: list[str] | None = None) -> int: "running prompts." ), ) + parser.add_argument( + "--check", + action="store_true", + help=( + "Build each step's system prompt under path without running a case. " + "Prints one line per fixtures dir whose prompt cannot be built, such as " + "a step_heading the SKILL.md no longer has, and exits 1 if there is any." + ), + ) args = parser.parse_args(argv) grader_explicit = args.grader_cli != DEFAULT_GRADER_CLI @@ -1276,6 +1285,16 @@ def main(argv: list[str] | None = None) -> int: print(f"{tag} {tag_counts[tag]}") return 0 + if args.check: + broken = 0 + for fixtures_dir in dict.fromkeys(fixtures_dir for _case_dir, fixtures_dir in cases): + try: + load_step_config(fixtures_dir) + except (OSError, ValueError, KeyError) as exc: + print(f"{fixtures_dir}: {exc}", file=sys.stderr) + broken += 1 + return 1 if broken else 0 + if args.tag: requested_tags = set(args.tag) cases = [ diff --git a/tools/skill-evals/tests/test_runner.py b/tools/skill-evals/tests/test_runner.py index c05bc21a6..9e2b6a41e 100644 --- a/tools/skill-evals/tests/test_runner.py +++ b/tools/skill-evals/tests/test_runner.py @@ -1293,6 +1293,32 @@ def test_list_tags_no_tags_found(tmp_path: Path, capsys: pytest.CaptureFixture[s assert stdout.strip() == "no tags found" +def test_check_reports_a_step_heading_the_skill_no_longer_has( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +): + """A renamed SKILL.md heading leaves the step-config pointing at nothing; + --check names that fixtures dir without running any case.""" + repo = _make_repo(tmp_path) + (repo / "SKILL.md").write_text("## Step 5 — Secure it\n\nRules.\n") + good = _make_fixtures_dir( + repo / "evals" / "good", step_config={"skill_md": "SKILL.md", "step_heading": "## Step 5 — Secure it"} + ) + stale = _make_fixtures_dir( + repo / "evals" / "stale", + step_config={"skill_md": "SKILL.md", "step_heading": "### Step 5 — Apply the checklist"}, + ) + _make_case(good, "case-1") + _make_case(stale, "case-1") + + rc, stdout, stderr = _run_main(capsys, [str(repo / "evals"), "--check"]) + + assert rc == 1 + assert stdout == "" + assert str(stale) in stderr + assert "### Step 5 — Apply the checklist" in stderr + assert str(good) not in stderr + + def test_collect_tag_counts(tmp_path: Path): fixtures_dir = tmp_path / "fixtures" case1 = _make_case(fixtures_dir, "case-1")