feat(scripts): add Python port of validate-skills

[dewa1981]

What

Adds scripts/validate-skills-py — a dependency-light Python port of scripts/validate-skills.

Why

The Ruby script cannot run on every machine:

• Linux servers and slim CI images frequently ship Python but not Ruby.
  hooks/pre-commit calls scripts/validate-skills, so on those machines the
  guardrail silently does nothing.
• The Ruby discovery glob is skills/*/SKILL.md (one level, flat). Skill pools
  that nest by category — <category>/[<subcategory>/]<skill>/SKILL.md — are
  common, and against those the Ruby script validates zero files and still
  exits 0, which looks like a pass.

Parity with the Ruby version

• front matter must start on line 1 and close with ---
• front matter must parse as a YAML mapping
• name and description must be non-empty strings
• gh secret set ... --body - is rejected

Deliberate differences

1. Recursive discovery — validates nested pools instead of silently skipping them.
2. Global duplicate-name detection — name is the routing key, so two skills
   sharing one name is a collision regardless of which directory they sit in.
   (The Ruby version tracks duplicates only within the single scanned level.)
3. Block-scalar descriptions (description: | that failed to parse) are
   reported specifically instead of as a generic missing-description error.

Verification

Run against a real 491-skill nested pool and this repo's own skills/:

$ scripts/validate-skills-py --root skills
Validated 54 skill(s).
$ scripts/validate-skills-py --root <nested-pool>
Validated 491 skill(s).

On that pool it immediately surfaced two real defects that the flat glob never
reported:

• one skill whose description contained an unquoted Source: owner/repo, making
  the YAML invalid — the skill never routed at all
• two skills in different directories sharing one name

Notes

• No new dependencies: PyYAML is used when available, with a minimal fallback
  parser otherwise. Python 3.8+.
• --root, --only-changed, --json, --quiet flags for pre-commit/CI use.
• Happy to fold this into scripts/validate-skills as a runtime-selected
  implementation, or keep the Ruby script canonical and treat this purely as an
  alternative for Python-only machines — your call.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 29, 2026, 1:36 AM ET / 05:36 UTC.

ClawSweeper review

What this changes

Adds two Python skill validators, a Python pre-commit hook and commit helper, and documentation for a nested Hermes skill pool.

Merge readiness

⛔ Blocked before merge - 15 items remain

The Python validator addresses a real portability gap in the existing Ruby-only hook, but the submitted branch is not ready to merge: its new hook cannot find the validator, its fallback parser can accept invalid YAML, and the added commit helper can include files it did not check. This repository’s conservative onboarding policy also precludes automatic PR closure.

Priority: P2
Reviewed head: f02e604ec8ed9789620d0375b3eb6ffc9b3fcb48
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The validator has a useful real-pool demonstration, but broken wiring and commit safety defects make the current patch unready.
Proof confidence	🦐 gold shrimp (3/6)	Needs stronger real behavior proof before merge: The PR body supplies copied terminal results from runs of the Python validator against 54 and 491 skills, which supports that entrypoint's full-scan behavior. It does not show the introduced hook or commit helper working after the change, nor a no-PyYAML or changed-only scenario. A redacted terminal trace of those paths is needed; redact private paths, endpoints, keys, and other sensitive details. Updating the PR body should trigger re-review; otherwise a maintainer can request @clawsweeper re-review. No stored-data contract changes were found. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🧂 unranked krab (1/6)	Security review found an item that needs attention.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The PR body supplies copied terminal results from runs of the Python validator against 54 and 491 skills, which supports that entrypoint's full-scan behavior. It does not show the introduced hook or commit helper working after the change, nor a no-PyYAML or changed-only scenario. A redacted terminal trace of those paths is needed; redact private paths, endpoints, keys, and other sensitive details. Updating the PR body should trigger re-review; otherwise a maintainer can request @clawsweeper re-review. No stored-data contract changes were found. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	8 items	Existing validation contract: The current main hook invokes scripts/validate-skills, and the README documents that validator as the supported skill check. New hook target: The introduced hook executes /opt/data/scripts/validate_skills.py, a path absent from the five-file introduced diff; the PR adds scripts/validate-skills-py and scripts/validate-skills-python instead. Fallback parser: Without PyYAML, the parser ignores lines it cannot match as simple key-value pairs and still returns a mapping; malformed YAML can therefore pass.
Findings	5 actionable findings	[P1] Point the Python hook at the shipped validator [P1] Do not skip validation when the helper cannot find it [P1] Limit the commit to the files that passed the safety check
Security	Needs attention	Unreviewed staged files can reach the remote: The helper checks only selected path basenames but invokes unrestricted git commit followed by a default push; previously staged sensitive files can cross the repository boundary.

How this fits together

Skill files enter a local validation script before a Git commit; validation errors should stop the commit. The existing hook invokes the Ruby validator, while this PR adds separate Python entrypoints and a helper that stages, commits, and optionally pushes files.

flowchart LR
  A[Skill files] --> B[Pre-commit hook]
  B --> C[Validator selection]
  C --> D[Front matter checks]
  D --> E{Errors found?}
  E -->|Yes| F[Block commit]
  E -->|No| G[Allow commit]

Loading

Decision needed

Question	Recommendation
Should this repository support Python-only skill validation through its shared hook, and should the Hermes-specific commit helper be part of that change?	Narrow shared fallback: Keep one Python validator and wire it into the shared hook when Ruby is unavailable; leave the Hermes-specific committer out of this PR.

Why: The PR offers two different Python validators and adds a separate commit workflow, while current documentation names the Ruby validator as the shared entrypoint.

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The PR body supplies copied terminal results from runs of the Python validator against 54 and 491 skills, which supports that entrypoint's full-scan behavior. It does not show the introduced hook or commit helper working after the change, nor a no-PyYAML or changed-only scenario. A redacted terminal trace of those paths is needed; redact private paths, endpoints, keys, and other sensitive details. Updating the PR body should trigger re-review; otherwise a maintainer can request @clawsweeper re-review. No stored-data contract changes were found. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Point the Python hook at the shipped validator (P1) - The hook executes /opt/data/scripts/validate_skills.py, but neither that path nor that filename is added. Installing this hook makes commits fail before any skill validation runs. Resolve the validator relative to the repository and use the actual shipped entrypoint.
• Do not skip validation when the helper cannot find it (P1) - SKILL_VALIDATOR names validate_skills.py, which this PR does not add; when skills are selected, the helper prints a warning and continues to commit. Point it at the selected validator and fail closed if validation cannot run.
• Limit the commit to the files that passed the safety check (P1) - The helper checks only its requested filenames, then runs an unrestricted git commit. Git includes unrelated files already in the index, so a pre-staged credential or other file can be committed and pushed despite the helper's safety claim.
• Fail on YAML the fallback parser cannot validate (P1) - On the Python-only machines this port targets, PyYAML may be absent. The fallback ignores malformed or unsupported front-matter lines and still returns a dictionary, making invalid YAML appear valid. Use a real parser or return a validation error when parsing cannot be established.
• Include new skills in changed-only validation (P2) - git diff --name-only HEAD omits untracked files, and filtering before duplicate-name indexing also misses collisions with unchanged skills. A newly added skill can therefore yield Validated 0 skill(s) and exit successfully under the documented changed-only mode.
• Resolve security concern: Unreviewed staged files can reach the remote - The helper checks only selected path basenames but invokes unrestricted git commit followed by a default push; previously staged sensitive files can cross the repository boundary.
• Resolve merge risk (P1) - The Python-only pre-commit path currently fails because its configured executable is not included in the PR.
• Resolve merge risk (P1) - The commit helper can silently skip validation and commit already-staged files, including sensitive files outside its basename check.
• Resolve merge risk (P1) - The no-PyYAML path does not enforce the claimed YAML contract, and changed-only mode can report success without checking a new skill or a global name collision.
• Complete next step (P2) - Resolve the validator scope and entrypoint choice, fix the blocking hook, parser, changed-only, and commit-safety findings, then add redacted real-path proof before merge.
• Improve patch quality - Use one Python validator and prove the repository hook blocks an invalid skill in a real commit attempt.
• Improve patch quality - Remove or repair the commit helper so it cannot include pre-staged files or skip validation.
• Improve patch quality - Show no-PyYAML and changed-only behavior with malformed YAML, a new skill, and a duplicate name.
• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

• [P1] Point the Python hook at the shipped validator — hooks/pre-commit-python:6
• [P1] Do not skip validation when the helper cannot find it — scripts/committer:35-36
• [P1] Limit the commit to the files that passed the safety check — scripts/committer:169-170
• [medium] Unreviewed staged files can reach the remote — scripts/committer:170

Agent review details

Security

Needs attention: The new commit helper can commit and push already-staged files that its secret-name check never examines.

Review metrics

Metric	Value	Why it matters
Added lines	production +617, tests +0, documentation +84	Most new production code consists of two similar validators and an additional commit workflow, with no committed regression coverage.

Merge-risk options

Maintainer options:

1. Repair the shared validation path (recommended)
   Use one checked Python implementation, correct the hook path, and prove invalid skills block commits.
2. Remove the commit helper
   Move the Hermes-specific helper to a separate proposal so its staging and secret-handling contract can be reviewed independently.

Technical review

Best possible solution:

Choose one Python validator, make runtime selection work from the repository hook, reject unsupported YAML rather than passing it, and keep any Hermes-specific commit workflow in a focused follow-up.

Do we have a high-confidence way to reproduce the issue?

Yes for the introduced defects: the hook's missing executable, permissive fallback parser, and unrestricted commit operation are traceable directly from the new source. The reported 491-skill pool itself is not available in this checkout.

Is this the best way to solve the issue?

No. A single Python validator connected to the existing hook is narrower and avoids maintaining two divergent ports and an unrelated commit workflow.

Full review comments:

• [P1] Point the Python hook at the shipped validator — hooks/pre-commit-python:6
  The hook executes /opt/data/scripts/validate_skills.py, but neither that path nor that filename is added. Installing this hook makes commits fail before any skill validation runs. Resolve the validator relative to the repository and use the actual shipped entrypoint.
  Confidence: 0.99
• [P1] Do not skip validation when the helper cannot find it — scripts/committer:35-36
SKILL_VALIDATOR names validate_skills.py, which this PR does not add; when skills are selected, the helper prints a warning and continues to commit. Point it at the selected validator and fail closed if validation cannot run.
  Confidence: 0.99
• [P1] Limit the commit to the files that passed the safety check — scripts/committer:169-170
  The helper checks only its requested filenames, then runs an unrestricted git commit. Git includes unrelated files already in the index, so a pre-staged credential or other file can be committed and pushed despite the helper's safety claim.
  Confidence: 0.97
• [P1] Fail on YAML the fallback parser cannot validate — scripts/validate-skills-py:84-95
  On the Python-only machines this port targets, PyYAML may be absent. The fallback ignores malformed or unsupported front-matter lines and still returns a dictionary, making invalid YAML appear valid. Use a real parser or return a validation error when parsing cannot be established.
  Confidence: 0.96
• [P2] Include new skills in changed-only validation — scripts/validate-skills-py:119-130
git diff --name-only HEAD omits untracked files, and filtering before duplicate-name indexing also misses collisions with unchanged skills. A newly added skill can therefore yield Validated 0 skill(s) and exit successfully under the documented changed-only mode.
  Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 3f8c6a33f911.

Labels

Label changes:

• add P2: This is a useful tooling improvement with concrete merge blockers but limited immediate user impact.
• add merge-risk: 🚨 security-boundary: The commit helper's selected-file secret check does not cover files already staged for the unrestricted git commit.
• add merge-risk: 🚨 automation: The new hook points to a nonexistent executable, so selecting it would stop commits rather than validate skills.
• add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦐 gold shrimp and patch quality is 🧂 unranked krab.
• add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body supplies copied terminal results from runs of the Python validator against 54 and 491 skills, which supports that entrypoint's full-scan behavior. It does not show the introduced hook or commit helper working after the change, nor a no-PyYAML or changed-only scenario. A redacted terminal trace of those paths is needed; redact private paths, endpoints, keys, and other sensitive details. Updating the PR body should trigger re-review; otherwise a maintainer can request @clawsweeper re-review. No stored-data contract changes were found. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

• P2: This is a useful tooling improvement with concrete merge blockers but limited immediate user impact.
• merge-risk: 🚨 automation: The new hook points to a nonexistent executable, so selecting it would stop commits rather than validate skills.
• merge-risk: 🚨 security-boundary: The commit helper's selected-file secret check does not cover files already staged for the unrestricted git commit.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦐 gold shrimp and patch quality is 🧂 unranked krab.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body supplies copied terminal results from runs of the Python validator against 54 and 491 skills, which supports that entrypoint's full-scan behavior. It does not show the introduced hook or commit helper working after the change, nor a no-PyYAML or changed-only scenario. A redacted terminal trace of those paths is needed; redact private paths, endpoints, keys, and other sensitive details. Updating the PR body should trigger re-review; otherwise a maintainer can request @clawsweeper re-review. No stored-data contract changes were found. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

• [medium] Unreviewed staged files can reach the remote — scripts/committer:170
  The helper checks only selected path basenames but invokes unrestricted git commit followed by a default push; previously staged sensitive files can cross the repository boundary.
  Confidence: 0.97

What I checked:

• Existing validation contract: The current main hook invokes scripts/validate-skills, and the README documents that validator as the supported skill check. (hooks/pre-commit:7, 3f8c6a33f911)
• New hook target: The introduced hook executes /opt/data/scripts/validate_skills.py, a path absent from the five-file introduced diff; the PR adds scripts/validate-skills-py and scripts/validate-skills-python instead. (hooks/pre-commit-python:6, f02e604ec8ed)
• Fallback parser: Without PyYAML, the parser ignores lines it cannot match as simple key-value pairs and still returns a mapping; malformed YAML can therefore pass. (scripts/validate-skills-py:84, f02e604ec8ed)
• Changed-only selection: The changed-only mode uses git diff HEAD and filters discovered files before building the duplicate-name index. Untracked skills are omitted, and collisions with unchanged skills are not checked. (scripts/validate-skills-py:121, f02e604ec8ed)
• Commit helper safety: The helper references another absent validator filename, proceeds when that file is missing, and later runs git commit without limiting the commit to its selected paths; already-staged files can be included. (scripts/committer:35, f02e604ec8ed)
• Existing hook history: Blame ties the current hook invocation to the original hook commit; its raw recorded parent has no hooks/pre-commit path. (hooks/pre-commit:7, 2a96db8a61eb)

Likely related people:

• Peter Steinberger: Raw commit 2a96db8 adds hooks/pre-commit:7 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 2a96db8a61eb; files: hooks/pre-commit)
• chaochaoweb3: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

[steipete]

Thanks for documenting the Python-only and nested-pool use cases. I’m closing this because the proposed parallel validator and Hermes-specific tooling do not fit this repository’s single canonical guardrail. The diff also includes two validator copies, an unrelated auto-pushing commit helper, and a hook pointing at /opt/data/scripts/validate_skills.py, which is not installed by this repository.

The validation contract would also weaken: without PyYAML, malformed YAML and non-string values can pass; --only-changed joins repository-relative paths to the skills root, can check zero changed skills, and misses name collisions with unchanged skills. This needs a unified validator design rather than landing the alternate scripts as provided.