feat: skill linter and skill-review skill#105
Open
agullon wants to merge 12 commits intoopenshift-eng:mainfrom
Open
feat: skill linter and skill-review skill#105agullon wants to merge 12 commits intoopenshift-eng:mainfrom
agullon wants to merge 12 commits intoopenshift-eng:mainfrom
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a SKILL.md linting system: a Python linter and Bash wrapper, Makefile targets, templates, guidelines and docs, CI/hook integration, review-rule additions, a marketplace plugin, and a user-invocable skill to run the linter. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
plugins/.templates/SKILL.md.template (1)
51-53: Make the side-effect note a placeholder, not a hardcoded claim.This template is used for all skills, but Line 53 asserts every generated skill is read-only. That will be wrong for mutating skills and is easy to leave behind during copy/paste.
♻️ Suggested wording
## Notes -- **Read-only**: This skill does not modify any external state +- Document whether the skill is read-only or, if it changes state, what confirmation/rollback guard it requires🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/.templates/SKILL.md.template` around lines 51 - 53, The template currently hardcodes "- **Read-only**: This skill does not modify any external state" which incorrectly asserts every generated skill is non-mutating; change that line to a placeholder so generated skills can declare their actual side-effect behavior (e.g., replace the hardcoded sentence under "## Notes" with a template variable like a "Side effects" placeholder such as "- **Side effects**: {{SIDE_EFFECTS}}" or similar), ensuring the SKILL.md.template exposes a field contributors must fill when a skill is mutating or read-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/docs/SKILL-GUIDELINES.md`:
- Around line 366-380: Add an explicit checklist item to the Quality Checklist
that requires reviewers to run the contribution quick-check command and confirm
skills pass the guidelines: include the exact command `python3
scripts/lint-skills.py <plugin>/skills/*/SKILL.md` and require the reviewer to
verify and mark that “Skills follow Skill Quality Guidelines
(SKILL-GUIDELINES.md)”; update the checklist block in SKILL-GUIDELINES.md (the
PR reviewers — verify these before approving a new or updated skill list) to
include this new line so the reviewer workflow matches CONTRIBUTING.md.
- Around line 109-112: The guidance in SKILL-GUIDELINES.md conflicts with the
linter: update the paragraph about `allowed-tools` to state that for any
`user-invocable` skill the `allowed-tools` field is required (the linter rule
`check_allowed_tools` emits a warning when it is missing) and retain the
existing permission-minimality advice and the explicit requirement to declare
`Bash`; alternatively, if you prefer the doc as-is, change the
`check_allowed_tools` rule to only warn for non-simple skills and update the
rule name/logic accordingly—refer to `allowed-tools`, `user-invocable`, and
`check_allowed_tools` when making the change.
- Around line 44-49: Add a new linter check (e.g., E008) that enforces the
presence and boolean value of the `user-invocable` field for slash commands:
modify the manifest validation routine (look for the function that
parses/validates command fields such as validateCommandFields or
parseSkillManifest) to emit E008 when `user-invocable` is missing or not a
boolean, update the linter's rule registry/list (the array/object that contains
E001–E007, W001–W012) to include E008, and add/adjust unit tests to cover
missing, empty, and non-boolean `user-invocable` cases so PR gating will fail
when the field is absent or invalid.
- Around line 331-335: Update the example that sets
WORKDIR="/tmp/skill-name-$(date +%Y%m%d)" to recommend creating a unique
ephemeral directory using mktemp-style creation (instead of date-based names)
and ensure cleanup after use; specifically change the guidance around the
WORKDIR variable and the mkdir -p step to instruct using a command that produces
a unique directory (e.g., mktemp -d) and add a note to register cleanup (trap or
explicit rm) so concurrent runs/users cannot collide.
In `@scripts/lint-skills.py`:
- Around line 133-154: The DESTRUCTIVE_KEYWORDS list in scripts/lint-skills.py
includes non-destructive, read-only commands (curl, wget, ssh, scp) that trigger
noisy W008/W009 warnings; remove "curl", "wget", "ssh", and "scp" from the
DESTRUCTIVE_KEYWORDS list so only truly destructive commands remain, then run
the linter/tests that exercise linting rules (and update any test expectations
if needed) to ensure warnings are reduced accordingly.
- Around line 373-445: The three checks (check_side_effect_guard,
check_description_intent_mismatch, check_credential_pattern) currently call
skill.body_outside_code_blocks(), which skips fenced code blocks where
destructive commands and hardcoded credentials often appear; change each check
to also scan the full skill.body (or both body_outside_code_blocks() and body
including code blocks) so matches inside fenced snippets are detected, e.g.,
replace or augment uses of body_outside_code_blocks() with skill.body (or run
the regexes against both) and keep using skill.find_body_line/re.escape to
report the correct line number and flags.
In `@scripts/lint-skills.sh`:
- Around line 56-67: The direct-mode branch currently swallows the Python
linter's exit code by appending "|| true" to the python3 "$SCRIPT_PATH" call and
the script always exits 0; remove "|| true" from the direct-mode invocation (the
python3 call in the else branch) so the linter's exit status propagates, and
move the "exit 0" so it only runs in the JSON-mode path after echoing
LINT_OUTPUT (i.e., keep exit 0 inside the if [[ "$JSON_MODE" == true ]] block);
leave the JSON-mode behavior (capturing stdout to LINT_OUTPUT, discarding
stderr) unchanged.
- Around line 25-39: The current TTY check ([ -t 0 ] || [[ $# -gt 0 ]]) sends
no-arg non-TTY runs (e.g., make lint-skills in CI) into JSON hook mode; change
the branching to treat no-arg runs as direct mode unless stdin actually contains
JSON by peeking at stdin first. Replace the heuristic in scripts/lint-skills.sh
so that you: 1) prefer direct mode when $# == 0 and stdin is not a JSON payload,
2) detect JSON by non-blocking-peeking the first non-whitespace byte from stdin
(e.g., read -r -n1 -t0 or head -c1) and check for “{” or “[”, and 3) only enable
JSON_MODE and parse INPUT when that peek indicates JSON; keep the rest of the
CWD/jq logic the same and update the condition around JSON_MODE accordingly.
---
Nitpick comments:
In `@plugins/.templates/SKILL.md.template`:
- Around line 51-53: The template currently hardcodes "- **Read-only**: This
skill does not modify any external state" which incorrectly asserts every
generated skill is non-mutating; change that line to a placeholder so generated
skills can declare their actual side-effect behavior (e.g., replace the
hardcoded sentence under "## Notes" with a template variable like a "Side
effects" placeholder such as "- **Side effects**: {{SIDE_EFFECTS}}" or similar),
ensuring the SKILL.md.template exposes a field contributors must fill when a
skill is mutating or read-only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 44f4f165-5b2f-423b-a4c0-2ea02bf95e84
📒 Files selected for processing (9)
.claude/settings.json.coderabbit.yamlMakefileplugins/.templates/SKILL.md.templateplugins/docs/CONTRIBUTING.mdplugins/docs/DEVELOPMENT.mdplugins/docs/SKILL-GUIDELINES.mdscripts/lint-skills.pyscripts/lint-skills.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/skills-review/skills/lint/SKILL.md`:
- Line 6: The behavior contract in SKILL.md is inconsistent: the header
"allowed-tools: Bash, Read, Glob, Grep" disallows writes, yet the body (the
"apply corrections" instruction at the section around Line 47) implies editing
and Line 67 describes a read-only posture; either remove/replace the "apply
corrections" instruction to make the doc strictly lint-only, or explicitly add
write-capable tools to the allowed-tools list and add an explicit confirmation
gate in the workflow text (e.g., a clear "Confirm edits?" step) so edits only
occur after explicit approval; update the SKILL.md allowed-tools entry and the
corresponding "apply corrections" and confirmation paragraphs to be consistent.
In `@scripts/lint-skills.py`:
- Around line 622-626: format_json currently returns early when there are no
errors, causing --json mode to emit nothing if only warnings exist; change
format_json so it always writes valid JSON to stdout (or returns a JSON string)
even when errors is empty by emitting an object with an errors array (possibly
empty) and a warnings array built from findings; update any related path that
calls format_json (the warning-only path referenced around lines 715-718) to use
the same JSON output so consumers always receive well-formed JSON. Ensure you
reference the format_json function and the variables findings/errors/warnings
when implementing the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e9b463cb-cdc5-4cc7-b04d-a7c5bc29c4e1
📒 Files selected for processing (5)
.claude-plugin/marketplace.jsonplugins/skills-review/.claude-plugin/plugin.jsonplugins/skills-review/skills/lint/SKILL.mdscripts/lint-skills.pyscripts/lint-skills.sh
✅ Files skipped from review due to trivial changes (1)
- plugins/skills-review/.claude-plugin/plugin.json
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/lint-skills.sh
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/lint-skills.py (2)
362-434:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSecurity checks still miss fenced code blocks (destructive commands + credential patterns).
check_side_effect_guard,check_description_intent_mismatch, andcheck_credential_patternstill scanbody_outside_code_blocks(). That leaves blind spots in fencedbash/jsonexamples, which are common locations for exactly these risks.Suggested fix
def check_side_effect_guard(skill): - text = skill.body_outside_code_blocks().lower() + text = skill.body.lower() @@ def check_description_intent_mismatch(skill): @@ - body_lower = skill.body_outside_code_blocks().lower() + body_lower = skill.body.lower() @@ def check_credential_pattern(skill): - text = skill.body_outside_code_blocks() + text = skill.bodySeverity: medium (policy violation risk and missed security findings).
As per coding guidelines, CONTRIBUTING.md quick checklist requires ensuring “there are no hardcoded credentials/security vulnerabilities.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-skills.py` around lines 362 - 434, These checks currently call skill.body_outside_code_blocks(), which skips fenced code blocks and misses destructive commands and credentials; update check_side_effect_guard, check_description_intent_mismatch, and check_credential_pattern to scan the full skill.body (or both body_outside_code_blocks() plus code blocks) when matching DESTRUCTIVE_KEYWORDS, CREDENTIAL_PATTERNS, and CONFIRMATION_PATTERNS so examples inside fenced bash/json are detected; keep existing use of skill.find_body_line (adding re.IGNORECASE where missing) and preserve the same Finding construction and early-return logic.
623-627:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--jsoncan still emit empty output for warning-only findings.When findings contain warnings but no errors,
main()callsformat_json(all_findings), andformat_json()returns without printing anything. Hook consumers then get empty stdout instead of valid JSON.Suggested fix
def format_json(findings): errors = [f for f in findings if f.severity == "error"] if not errors: - return + print(json.dumps({})) + returnAlso applies to: 715-719
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-skills.py` around lines 623 - 627, format_json currently returns early when there are no error-level findings, causing --json to produce no output for warning-only results; update format_json (and the similar block around the code handling lines 715-719) so it always emits valid JSON: construct a JSON object that includes errors (possibly empty) and warnings derived from the findings argument and print json.dumps(...) of that object instead of returning early; reference the format_json function and the call site where main() calls format_json(all_findings) to ensure the printed output is produced for warning-only cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/lint-skills.py`:
- Around line 362-434: These checks currently call
skill.body_outside_code_blocks(), which skips fenced code blocks and misses
destructive commands and credentials; update check_side_effect_guard,
check_description_intent_mismatch, and check_credential_pattern to scan the full
skill.body (or both body_outside_code_blocks() plus code blocks) when matching
DESTRUCTIVE_KEYWORDS, CREDENTIAL_PATTERNS, and CONFIRMATION_PATTERNS so examples
inside fenced bash/json are detected; keep existing use of skill.find_body_line
(adding re.IGNORECASE where missing) and preserve the same Finding construction
and early-return logic.
- Around line 623-627: format_json currently returns early when there are no
error-level findings, causing --json to produce no output for warning-only
results; update format_json (and the similar block around the code handling
lines 715-719) so it always emits valid JSON: construct a JSON object that
includes errors (possibly empty) and warnings derived from the findings argument
and print json.dumps(...) of that object instead of returning early; reference
the format_json function and the call site where main() calls
format_json(all_findings) to ensure the printed output is produced for
warning-only cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4be275ad-f349-4d66-9b41-250cdd406c80
📒 Files selected for processing (3)
plugins/docs/SKILL-GUIDELINES.mdscripts/lint-skills.pyscripts/lint-skills.sh
✅ Files skipped from review due to trivial changes (1)
- plugins/docs/SKILL-GUIDELINES.md
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/lint-skills.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
scripts/lint-skills.py (2)
623-640:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways emit valid JSON in hook mode.
format_json()returns without printing when only warnings are present, so--jsoncan produce empty stdout even thoughmain()still routes non-empty warning sets here. That breaks hook consumers that expect a JSON payload every time.Suggested fix
def format_json(findings): errors = [f for f in findings if f.severity == "error"] if not errors: - return + print(json.dumps({})) + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-skills.py` around lines 623 - 640, format_json() currently returns early when there are no errors, causing --json hook mode to produce no output; change it to always print a JSON payload by handling the warnings-only case: compute warnings = [f for f in findings if f.severity != "error"], build a reason string that includes either the error lines (if any) or the warning lines (if no errors), and set "decision" to "block" when errors exist or to "allow" (or an appropriate non-blocking value) when only warnings are present; ensure the function always calls print(json.dumps(output)) so callers of format_json (and main()) always receive valid JSON.
362-434:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScan fenced code blocks too.
body_outside_code_blocks()still hides destructive commands and hardcoded credentials when they appear in fenced examples, so W008/W009/W010 can miss the exact security cases this linter is meant to catch. Per CONTRIBUTING.md: "including security checks like no hardcoded credentials."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-skills.py` around lines 362 - 434, The linter currently uses skill.body_outside_code_blocks() which omits fenced code examples and therefore misses destructive commands and hardcoded credentials; update check_side_effect_guard, check_description_intent_mismatch, and check_credential_pattern to scan the full skill body (e.g., skill.body or an equivalent that includes fenced code blocks) instead of body_outside_code_blocks(), preserving the existing case-insensitive searches and re.escape/finding logic (keep calls to skill.find_body_line and re.IGNORECASE where used) so W008/W009/W010 detect issues inside fenced code blocks as well.plugins/skills-review/skills/lint/SKILL.md (1)
47-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the skill read-only, or grant write tools explicitly.
Per CONTRIBUTING.md: "Tooling constraints ... ensure minimal tool set." The body still says to "apply corrections" even though
allowed-toolsonly includesBash, Read, Glob, Grep, so the workflow promises edits it cannot perform.Suggested doc tweak
- If the user wants to fix issues, read the failing SKILL.md files and apply corrections. + If the user wants to fix issues, summarize concrete fixes and propose patches; do not edit files in this skill.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/skills-review/skills/lint/SKILL.md` around lines 47 - 68, The SKILL.md claims the tool will "apply corrections" but the skill's allowed-tools (Bash, Read, Glob, Grep) cannot write; update the SKILL.md so the skill is explicitly read-only (remove or change phrases like "apply corrections" and "It only modifies files if the user explicitly requests fixes") or alternatively declare and add the required write tools in the allowed-tools list and document that fixes require explicit user approval; reference the lint skill invocation examples (e.g., "/skills-review:lint" and "/skills-review:lint --all") and the "Analysis-only by default"/Notes section when making the change so the description and tool permissions are consistent with CONTRIBUTING.md.scripts/lint-skills.sh (1)
63-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the TTY heuristic from hijacking direct runs.
The
-t 0fallback still sends no-arg, non-TTY runs into hook mode, somake lint-skillsin CI can take the JSON path instead of the human-readable path advertised in the script comments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-skills.sh` around lines 63 - 69, The TTY heuristic currently flips JSON_MODE to true for non-TTY, no-arg runs (e.g., CI make lint-skills), so change the conditional around JSON_MODE to treat no-argument direct runs as human-readable: in the conditional that inspects JSON_MODE, check ORIG_ARGC == 0 (not > 0) so that if the script was invoked with no arguments (ORIG_ARGC==0) you force JSON_MODE=false regardless of TTY; specifically update the if-statement that references JSON_MODE, [ -t 0 ], and ORIG_ARGC so it sets JSON_MODE=false when [ -t 0 ] OR ORIG_ARGC == 0, otherwise set JSON_MODE=true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/lint-skills.sh`:
- Around line 118-123: The current fast-path in the JSON_MODE branch uses the
git probe (the git diff origin/main --name-only ... | grep -q .) and treats any
probe failure as "no changes", causing an early exit; change the logic around
JSON_MODE and CHECK_ALL_FILES so that a failing git probe does not cause exit 0
— explicitly capture the exit status of the git diff pipeline and only exit 0
when the command succeeded and reported no SKILL.md changes; if the git probe
fails (non-zero exit) fall back to running the linter (or reuse the Python
linter's ref-resolution logic instead of hardcoding origin/main) so linting
proceeds rather than silently skipping, and update the condition that references
origin/main to use the same ref resolution used by the Python linter.
- Around line 124-131: The current hook treats any non-zero exit from the Python
linter as a crash because the command is run with ||; instead run the linter
capturing stdout into LINT_OUTPUT and stderr into STDERR_FILE, then inspect the
exit code ($?) and treat it as a crash only when stderr is non-empty and
LINT_OUTPUT is empty (i.e., real crash vs normal lint-failure JSON on stdout).
Use STDERR_FILE, LINT_OUTPUT, SCRIPT_PATH and LINT_ARGS to locate the call, and
when emitting the crash JSON escape the stderr safely (e.g., via jq -Rs or other
JSON-quoting) rather than interpolating raw STDERR_CONTENT; remove the || branch
and replace with a conditional that checks exit code + stderr presence before
writing the crash payload.
---
Duplicate comments:
In `@plugins/skills-review/skills/lint/SKILL.md`:
- Around line 47-68: The SKILL.md claims the tool will "apply corrections" but
the skill's allowed-tools (Bash, Read, Glob, Grep) cannot write; update the
SKILL.md so the skill is explicitly read-only (remove or change phrases like
"apply corrections" and "It only modifies files if the user explicitly requests
fixes") or alternatively declare and add the required write tools in the
allowed-tools list and document that fixes require explicit user approval;
reference the lint skill invocation examples (e.g., "/skills-review:lint" and
"/skills-review:lint --all") and the "Analysis-only by default"/Notes section
when making the change so the description and tool permissions are consistent
with CONTRIBUTING.md.
In `@scripts/lint-skills.py`:
- Around line 623-640: format_json() currently returns early when there are no
errors, causing --json hook mode to produce no output; change it to always print
a JSON payload by handling the warnings-only case: compute warnings = [f for f
in findings if f.severity != "error"], build a reason string that includes
either the error lines (if any) or the warning lines (if no errors), and set
"decision" to "block" when errors exist or to "allow" (or an appropriate
non-blocking value) when only warnings are present; ensure the function always
calls print(json.dumps(output)) so callers of format_json (and main()) always
receive valid JSON.
- Around line 362-434: The linter currently uses
skill.body_outside_code_blocks() which omits fenced code examples and therefore
misses destructive commands and hardcoded credentials; update
check_side_effect_guard, check_description_intent_mismatch, and
check_credential_pattern to scan the full skill body (e.g., skill.body or an
equivalent that includes fenced code blocks) instead of
body_outside_code_blocks(), preserving the existing case-insensitive searches
and re.escape/finding logic (keep calls to skill.find_body_line and
re.IGNORECASE where used) so W008/W009/W010 detect issues inside fenced code
blocks as well.
In `@scripts/lint-skills.sh`:
- Around line 63-69: The TTY heuristic currently flips JSON_MODE to true for
non-TTY, no-arg runs (e.g., CI make lint-skills), so change the conditional
around JSON_MODE to treat no-argument direct runs as human-readable: in the
conditional that inspects JSON_MODE, check ORIG_ARGC == 0 (not > 0) so that if
the script was invoked with no arguments (ORIG_ARGC==0) you force
JSON_MODE=false regardless of TTY; specifically update the if-statement that
references JSON_MODE, [ -t 0 ], and ORIG_ARGC so it sets JSON_MODE=false when [
-t 0 ] OR ORIG_ARGC == 0, otherwise set JSON_MODE=true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1f335bcf-34c8-4819-8bd7-04143e5517ea
📒 Files selected for processing (5)
.claude/settings.jsonplugins/docs/SKILL-GUIDELINES.mdplugins/skills-review/skills/lint/SKILL.mdscripts/lint-skills.pyscripts/lint-skills.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .claude/settings.json
- plugins/docs/SKILL-GUIDELINES.md
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
agullon
force-pushed
the
SKILL_testing
branch
2 times, most recently
from
3bd6a86 to
d6f1160
Compare
agullon
changed the title
Define quality standards for SKILL.md files covering frontmatter, description quality, safety guards, orchestration patterns, and anti-patterns. These guidelines are the spec for the automated linter added in the next commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Python linter (lint-skills.py) with 19 checks — 7 errors that block commits and 12 advisory warnings covering frontmatter validation, description quality, safety guards, and structural conventions. Shell wrapper (lint-skills.sh) supports direct invocation for make targets and hook mode (--hook) for Claude Code stop hooks with JSON stdin/stdout protocol. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Expose /skills-review:lint slash command that runs the skill content linter and presents findings. Register plugin in the marketplace. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
- Add stop hook to run lint-skills.sh on modified SKILL.md files - Add CodeRabbit review instructions for SKILL.md files - Add make lint-skills and lint-all-skills targets Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
- Improve SKILL.md template with synopsis, edge cases, and notes - Add skill quality checklist items to CONTRIBUTING.md - Add skill quality section to DEVELOPMENT.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
The previous commit downgraded pr-review to v1.1.0 as part of the yolo-agent removal, which was dropped from this PR. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
brandisher
reviewed
Apr 30, 2026
Contributor
brandisher
left a comment
brandisher
left a comment
There was a problem hiding this comment.
I like the idea behind this PR and it looks like an improvement over the first iteration. Couple of questions:
- Do the guidelines you've laid out adhere to https://agentskills.io/specification ? This is the canonical definition of what skills should follow.
- I see a
lint-skills.pyand alint-skills.sh. Why do we have two of them?
|
@agullon: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
- Updated SKILL-GUIDELINES.md to reflect new constraints: description length reduced to 1024 characters and added requirements for name format and directory matching. - Implemented new linter checks for name format and name-directory matching in lint-skills.py to enforce these guidelines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
- Replaced the shell script `lint-skills.sh` with a Python script `lint-skills.py` for improved functionality and maintainability. - Updated Makefile and documentation to reflect the new Python-based linter. - Adjusted references in CONTRIBUTING.md, DEVELOPMENT.md, and SKILL-GUIDELINES.md to use the new script. - Removed the obsolete shell script. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
- Changed the linter command in settings.json from the shell script `lint-skills.sh` to the Python script `lint-skills.py` to ensure consistency with recent migrations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
pre-commit.check-secrets: ENABLED
- Replaced the --json option with --hook for improved clarity and functionality. - Adjusted the linter's behavior to read from stdin and always exit with code 0 in hook mode. - Removed the deprecated --json option from the argument parser. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
Author
I didn't know about agentskill.io, the guidelines I added were from Antrophic, Cursor and OpenAI official docs. I just updated them with agentskill.io. I created both .py and .sh scripts because of an old habit of using always bash scripts as entry point for python scripts. Bash script is suppose to manage the python venv and usage. But in this repo and for this script I think it's not necessary so I removed the bash one. Please, review the PR again. |
vimauro
reviewed
May 5, 2026
vimauro
reviewed
May 5, 2026
vimauro
reviewed
May 5, 2026
- Adjusted the description quality requirement in .coderabbit.yaml to specify a maximum length of 1024 characters, enhancing clarity and consistency with recent updates to skill guidelines. pre-commit.check-secrets: ENABLED
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
We have 38 SKILL.md files and no automated way to enforce quality. Bad descriptions cause wrong auto-invocation, missing safety guards cause unintended side effects, and oversized skills get partially ignored by Claude. Today the only check is manual review.
What
A content linter for SKILL.md files — 19 checks split into 7 blocking errors and 12 advisory warnings — plus the guidelines doc that defines the rules.
Errors (block commits):
Warnings (advisory):
allowed-tools(unlimited blast radius)How it runs
scripts/lint-skills.sh --hookruns after every Claude Code stop, checks only changed SKILL.md files, blocks on errorsmake lint-skills(changed files) /make lint-all-skills(all 38)/skills-review:lintfor interactive useAlso in this PR
How to review
Commits are split by concern:
refactor: simplify pr-review plugin— standalone cleanup, review independentlydocs: add skill quality guidelines— the spec; read this to understand what the linter enforcesfeat: add skill content linter— Python linter + shell wrapper; the bulk of the PRfeat: add skills-review plugin— thin plugin wiringchore: integrate skill linter into hooks and build— Makefile, settings.json, .coderabbit.yamldocs: update template and contributing docs— docs follow-upValidation
All 38 existing skills pass with zero errors. The 40 warnings are expected (mostly missing
allowed-toolson older skills).