Skip to content

Revert "Revert "Rework skill validator script (#11)"" #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nirinchev wants to merge 1 commit into from
Closed

Revert "Revert "Rework skill validator script (#11)"" #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 0 additions & 70 deletions .github/scripts/validate-skills.sh
View file
Open in desktop

This file was deleted.

9 changes: 2 additions & 7 deletions .github/workflows/validate-skills.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,12 @@ name: Validate Skills

on:
pull_request:
Comment thread
nirinchev marked this conversation as resolved.
paths:
Copy link
Copy Markdown
Collaborator

@dacharyc dacharyc Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only want to run the validator when a PR has changed skills - I don't want to run it when PRs touch unrelated infrastructure. This scope gate lets us avoid unnecessary CI runs.

Copy link
Copy Markdown
Collaborator Author

@nirinchev nirinchev Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI run is super cheap - half a minute where the majority of the time is spent on the setup. By making the check only run on skill changes, we cannot make the validate check required for merging as there's going to be PRs where it doesn't run.

- "skills/**"

jobs:
validate:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
with:
# Full history is needed so git diff can compare against the base branch
fetch-depth: 0

- name: Set up Go
uses: actions/setup-go@v6
Expand All @@ -23,5 +18,5 @@ jobs:
- name: Install skill-validator
run: go install github.com/agent-ecosystem/skill-validator/cmd/skill-validator@latest

- name: Validate changed skills
run: bash .github/scripts/validate-skills.sh "${{ github.base_ref }}"
- name: Validate all skills
Copy link
Copy Markdown
Collaborator

@dacharyc dacharyc Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior was intentional. I only want the CI to validate a changed skill because I don't want the CI to block merging a changed skill if some other skill is now failing. If we want to have some other CI to run a scheduled check to confirm all skills are still known good, I'd recommend setting that up as a separate CI run.

Copy link
Copy Markdown
Collaborator Author

@nirinchev nirinchev Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the validate check required, which means there should be no skills in main that are failing validation. It's not clear to me what we're gaining here by only checking the changed files.

run: bash tools/validate-skills.sh
23 changes: 23 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Contributing

## Adding a new skill

To add a new skill, create a new directory in the `skills` directory with the name of the skill. The directory should contain a `SKILL.md` file with the skill's metadata and instructions. You may find the [`skill-creator`](https://github.com/anthropics/skills/tree/main/skills/skill-creator) skill by Anthropic helpful for creating the initial draft. To install in Claude Code, add the `skill-creator` plugin.

## Testing new skills

### Structural testing

Use the `tools/validate-skills.sh` script to test the structural validity of the skill. This script uses the [`skill-validator`](https://github.com/agent-ecosystem/skill-validator) tool to check the skill's metadata and instructions.
Copy link
Copy Markdown
Collaborator

@dacharyc dacharyc Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we suggest contributors use a tools/validate-skills.sh script for structural validation. I think we should advise folks to use the review-skill skill. The review-skill skill runs the structural validation checks, performs some additional quality checks, and provides optional LLM-as-judge checks, and provides details interpreted by the agent using framing to help SMEs understand what they need to change based on the output of the structural and other checks.

Running the bare tool is fine for folks who are very familiar with what it measures and how to apply it, but I don't think it's contributor-friendly for SMEs who may not be familiar with what a Skill needs or how to add it.

Copy link
Copy Markdown
Collaborator Author

@nirinchev nirinchev Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the script being run on CI and this is what's going to gate PR merges - SMEs should be capable of running the same scripts CI runs to unblock themselves and resolve problems raised by it. I'm happy to also suggest running the review-skill skill, but anything that runs on CI, we should be able to run locally.


### LLM testing

Use the `tools/review-skill` skill to have an agent review the skill and interpret the results. On top of the structural validation offered by the `validate-skills.sh` script, it can also perform LLM scoring and provide a summary of the results.

Exact installation instructions depend on the client, but should be something similar to:

```bash
mkdir -p ~/.claude/skills && ln -s <path-to-agent-skills>/tools/review-skill ~/.claude/skills/review-skill
```

This creates a symlink to the `review-skill` tool in the `~/.claude/skills` directory so that it can be used in Claude Code.
60 changes: 60 additions & 0 deletions tools/validate-skills.sh
View file
Open in desktop
Copy link
Copy Markdown
Collaborator

@dacharyc dacharyc Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get a chance to ask on the original PR, but can you say more about the use case for this move? The original PR description only says:

Moves validate-skills.sh script to tools so that it can more easily be accessed outside of github actions.

If the desire is to run the skill-validator tool outside of CI, I think a separate script with its own entrypoint makes more sense. This script is optimized for CI usage, including these three flags which I would probably not run for local checks or other purposes: --strict --emit-annotations -o markdown

If the desire is to run the skill-validator tool on multiple skills, the script isn't needed at all - skill-validator already handles multi-skill validation. You can just invoke it with skill-validator check skills/ and it will check all skills in the directory.

And finally, if you want a script entrypoint for the tool to ensure the tool is installed before running the script, I'd probably recommend using the enterprise version of the tool, skill-validator-ent, which I wrote as a wrapper around the core lib that works with our AWS Bedrock model provider/identity management. That's the one I refer to in the review-skill skill for our use here. The CI predated development of skill-validator-ent, and also doesn't use any of the enterprise functionality, so I didn't change it here - but if the use case is a script for people to run locally, we should change it.

Copy link
Copy Markdown
Collaborator Author

@nirinchev nirinchev Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was mostly to align what we use locally and what's on CI. While we do output problems on CI in the summary section, it's always easier to be able to run whatever CI is doing and see the impact of your changes without pushing. And while I know the script is a simple wrapper around the skill-validator cli, I believe it still offers some value by abstracting away the API and helping with the installation process. Again, I know that this is information people could discover by reading what the script does, then replicating it manually, I wanted to address the case "Oh my CI failed, let me make some changes, let me run the same command CI is using locally and see if it succeeds".

On the enterprise cli - I have no opinions, fine with moving to that or to some other tool, my main goal is to have the same scripts run on CI available locally so that contributors can validate their work without having to copy-paste commands.

Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#!/bin/bash
# Validates all skill directories.
#
# Exit codes:
# 0 All validated skills passed.
# 1 One or more skills failed validation.

# -e is intentionally omitted: all error paths are handled explicitly,
# so abort-on-error would conflict with the || FAILED=1 accumulator pattern.
set -uo pipefail

# Find repository root (script is at tools/validate-skills.sh)
REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)"

# Check if skill-validator is available
if ! command -v skill-validator &> /dev/null; then
echo "❌ skill-validator is not installed."
echo ""
echo "To install it, run:"
echo " brew tap agent-ecosystem/tap"
echo " brew install skill-validator"
echo "--- or ---"
echo " go install github.com/agent-ecosystem/skill-validator/cmd/skill-validator@latest"
echo ""
exit 1
fi

FAILED=0

# Find and validate all skill directories
for skill_dir in "$REPO_ROOT"/skills/*/; do
# Check if the glob matched anything
[ -d "$skill_dir" ] || continue

Comment thread
nirinchev marked this conversation as resolved.
if [ -n "${GITHUB_ACTIONS:-}" ]; then
# In CI: use markdown output with annotations, filter annotations from summary
skill-validator check --strict --emit-annotations -o markdown "$skill_dir" \
| tee >(grep -v '^::' >> "$GITHUB_STEP_SUMMARY") || FAILED=1
Comment thread
nirinchev marked this conversation as resolved.
else
# Local: simple output
skill-validator check --strict "$skill_dir" || FAILED=1
fi
done

echo ""
if [ $FAILED -ne 0 ]; then
echo "❌ Skill validation failed!"
if [ -n "${GITHUB_ACTIONS:-}" ]; then
echo ""
echo "📋 See the Job Summary for detailed validation results:"
echo " https://github.com/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID"
fi
else
echo "✅ Skill validation passed!"
fi

echo ""

exit $FAILED