docs: fix stale skill validator references#354
Conversation
|
Thanks for your contribution, @nightcityblade - this looks good to approve/merge. The PR satisfies #351:
Thanks for your note on the issue with |
There was a problem hiding this comment.
Hey @nightcityblade! Thanks for adding this!
Just a few things:
Rename is correct and scope is clean (verified tools/skill-and-tool-validator/ exists on main, tools/skill-validator/ does not). Three blockers before merge:
-
Acceptance criterion #1 not met. Issue #351 requires the grep to return zero hits. One stale reference still ships in:
tools/skill-evals/evals/setup-override-upstream/step-6-pr-confirm/fixtures/case-1-shows-all-sections/report.md:25 - [ ] Ran `skill-validate` — passes.The issue flagged this fixture explicitly as "needs judgement, not blind replace" and asked for the
setup-override-upstreameval to be re-run after the edit. I checkedexpected.jsonfor this case and it does not literally referenceskill-validate, so updating the fixture prose should be safe, but the decision needs to be explicit, not silent. -
Grep command in the test plan is broken. As written:
git grep -nE 'skill-validator|skill-validate\\b' ...In a bash single-quoted string
\\bis the literal two chars\b, which in ERE matches an escaped backslash followed byb, not a word boundary. The issue's original command used\b(single backslash = ERE word boundary). The version pasted here silently returns zero hits, which is why the missed fixture above was not caught. If that is just markdown escaping when pasting, the grep that was actually run was the working one — please confirm — but the command as recorded in this PR does not run as the author claims. -
Validator not actually executed. Test plan notes
uvis pinned to 0.10.7 vs required>=0.11.8, which is fine to disclose, but the diff also touches prose inmeta-and-quality-tooling.md(tool name and CLI description, not just commands). Runningskill-and-tool-validateonce on a compatibleuvwould catch any link/frontmatter regressions in that prose change. Please run it before merge.
Once the fixture is updated (or its non-update is justified in a comment) and the eval re-run, this is ready to merge.
| ```bash | ||
| # Validate skill definitions (frontmatter, links, placeholders) | ||
| uv run --project tools/skill-validator --group dev skill-validate | ||
| uv run --project tools/skill-and-tool-validator --group dev skill-and-tool-validate |
There was a problem hiding this comment.
This same rename is missing from tools/skill-evals/evals/setup-override-upstream/step-6-pr-confirm/fixtures/case-1-shows-all-sections/report.md:25 (- [ ] Ran \skill-validate` — passes.). Issue #351 acceptance criterion #1 (git grep` returns no hits) is not met until that fixture is updated or its non-update is justified. The issue called it out explicitly under "Two spots need judgement, not blind replace."
| source: > | ||
| README.md § Skill families (utilities) and AGENTS.md § Reusable skills. | ||
| Implemented by tools/skill-validator/, tools/skill-evals/, | ||
| Implemented by tools/skill-and-tool-validator/, tools/skill-evals/, |
There was a problem hiding this comment.
Prose change here is correct. Reminder from the issue: after updating the setup-override-upstream fixture, re-run uv run --project tools/skill-evals skill-eval tools/skill-evals/evals/setup-override-upstream/ and confirm expected.json still matches. expected.json for case-1-shows-all-sections does not literally contain skill-validate, so the update is most likely safe, but please verify.
|
Thanks for the careful review — I pushed On the grep note: the command I actually ran locally used a single I still have one gap on the validator run itself: this checkout has |
|
Hi @nightcityblade — thanks very much for catching this and putting the fix together. I'm closing this in favour of #352, which @MD-Mushfiqur123 opened about an hour and a half earlier with the same scope. Both PRs touched the exact same 9 files and made essentially the same find-and-replace from Your work is real and helpful — we'd be glad to see more contributions from you. If you'd like a heads-up on what's likely to be picked up next, the open issues with Thanks again, and apologies for the duplicate-effort frustration — that's on us, not on you. |
Summary
tools/skill-and-tool-validatorpathskill-validatereferences with the renamedskill-and-tool-validateCLIskill-validator/skill-validatereferences to the renamedskill-and-tool-validator#351Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)Test plan
prek run --all-filespassesuv run pytest/ruff check/mypypasses(
PYTHONPATH=tools/skill-evals/src python3 -m skill_evals.runner tools/skill-evals/evals/<skill>/)(a regression test for the bug fixed / the behaviour added — see CONTRIBUTING.md)
git grep -nE 'skill-validator|skill-validate\\b' -- ':!tools/skill-and-tool-validator' | grep -v skill-and-tool || trueuv run --project tools/skill-and-tool-validator --group dev skill-and-tool-validate, but localuvis pinned to 0.10.7 and the repo requires>=0.11.8RFC-AI-0004 compliance
<PROJECT>,<tracker>,<upstream>,<security-list>) used in all skill / tool prose (thecheck-placeholdersprek hook is the mechanical gate)Linked issues
Closes #351
Notes for reviewers (optional)
The change is docs-only. I left the test-plan note about the
uvversion mismatch so the failed local validator invocation is explicit rather than silently omitted.