Repository navigation
fix(skill-evals): point write-skill step 5 at its section and check every binding - #1897
Conversation
…very binding apache#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 <[email protected]> Generated-by: Claude Code (Opus 5.5)
potiuk
left a comment
There was a problem hiding this comment.
Rebinding step 5 to its renamed section and adding --check as a prek gate closes the gap where a renamed heading passes its own PR.
The one open point was that the rebound step-5 suite had not been run against a model: its prompt is now the new Step 5 section plus the whole of security-checklist.md, while the cases' expected.json were written against the old inline checklist. I ran it on this head (1eaa98d7) with magpie-run-evals.sh tools/skill-evals/evals/write-skill/step-5-security-checklist/ (Claude Code, Opus 5): 5 of 5 cases pass, so no expected output needed updating and nothing was pushed to the branch. Approving and merging.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Magpie handles maintainer review:
Contributing guide.
Summary
### Step 5 — Apply the security checklistto## Step 5 — Secure it, if it reads outside contentand moved the rules tosecurity-checklist.md.The
write-skill/step-5-security-checkliststep-config still named the old heading, soload_step_configraisedHeading … not foundand the suite failed before any case reached a model.security-checklist.mdthroughalso_include, as other suites already do, becauseoutput-spec.mdrefers to the checklist's patterns.skill-eval <path> --checkbuilds every step's system prompt without running a case, prints one line per fixtures dir whose prompt cannot be built, and exits 1 if there is any.The new
check-eval-promptsprek hook runs it overtools/skill-evals/evalswhenever a Markdown file changes.workspace-pytestskips Markdown-only changes, so before this hook a renamed heading passed the PR that renamed it.Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)tools/skill-evals/evals/write-skill/step-5-security-checklist/fixtures/step-config.json)Test plan
prek run --all-filespasses — run asAGENTS.mddescribes:prek run --all-files --skip workspace-pytest, thenworkspace-pyteston the changed files; both passuv run pytest/ruff check/mypypasses —tools/skill-evals: full suite,ruff check,ruff format --checkand themypy (workspace)hook clean.A direct
mypyrun insidetools/skill-evalsreports 8 errors inrunner.py(lines 586–609, code this PR does not touch);mainreports the same 8.Print mode now builds every prompt in the tree; before the fix it failed on this step.
test_check_reports_a_step_heading_the_skill_no_longer_hasintools/skill-evals/tests/test_runner.pyfailed before the change (unrecognized arguments: --check) and passes after.On the real tree,
skill-eval tools/skill-evals/evals --checktakes about 1.5 s.With the old step-config it exits 1 with one line,
…/write-skill/step-5-security-checklist/fixtures: Heading '### Step 5 — Apply the security checklist' not found in …/write-skill/SKILL.md; with the fix it exits 0.RFC-AI-0004 compliance
No principle touched: the change adds a read-only check and no mutation.
Linked issues
Refs #1331
Notes for reviewers (optional)
files:filter matches every.md, not onlySKILL.md, because step-configs also extract sections fromdocs/adapters/codex.md,docs/setup/agentic-overrides.md,tools/asf-nexus/operations.mdandtools/dev/preflight-block.md.--checkcatchesOSError,ValueErrorandKeyError: whatload_step_configraises for a missing file, a missing heading, a path outside the repo, invalid JSON, or a missing key.Generative AI disclosure: prepared with Claude Code (Opus 5.5); I reviewed and tested the change.