feat(tools): add inference-based skill validator#1981
Merged
Conversation
LLM-readable validation prompt covering 19 rules across 6 categories: SKILL.md frontmatter, workflow.md hygiene, path resolution, step file structure, sequential execution, and file reference integrity. Designed to catch anti-patterns from mechanical workflow-to-skill conversions (installed_path abuse, intra-skill path variables, metadata in wrong frontmatter). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🤖 Augment PR SummarySummary: This PR introduces an inference-based validator prompt to review Agent Skills directories for common structural and reference integrity issues. Changes:
Technical Notes: The validator is designed to be applied by an LLM over a recursively scanned skill directory, using explicit detection heuristics per rule and emitting findings grouped by rule ID. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
|
||
| --- | ||
|
|
||
| ## Rule Catalog |
There was a problem hiding this comment.
This rule catalog defines 22 rule IDs (SKILL/WF/PATH/STEP/SEQ/REF); consider stating the total in-file (and keeping any external references in sync) to avoid confusion if other docs mention a different count.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| - **Severity:** HIGH | ||
| - **Applies to:** `SKILL.md` | ||
| - **Rule:** The `name` value must use only lowercase letters, numbers, and hyphens. Max 64 characters. Must not contain "anthropic" or "claude". | ||
| - **Detection:** Regex test: `^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$`. String search for forbidden substrings. |
There was a problem hiding this comment.
The regex ^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$ enforces a minimum length of 2 characters, but SKILL-04 only specifies a max length; if 1-character names are valid, this would incorrectly fail them.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| - **Severity:** HIGH | ||
| - **Applies to:** step files | ||
| - **Rule:** Each step must clearly state its goal. Look for a heading like `## YOUR TASK`, `## STEP GOAL`, `## INSTRUCTIONS`, `## INITIALIZATION`, `## EXECUTION`, `# Step N:`, or a frontmatter `goal:` field. |
There was a problem hiding this comment.
STEP-02 detection treats headings like ## INITIALIZATION/## EXECUTION as evidence of a goal, but those sections can exist without stating an explicit goal, so the validator may miss goal-less step files.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
📝 WalkthroughWalkthroughIntroduces a new skill validator specification document ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
tools/skill-validator.md (1)
5-12: No test cases or examples of passing/failing skills.The document defines rules but provides no concrete examples showing:
- A minimal valid skill directory structure
- Sample violations for each rule category
- Expected validator output for a skill with mixed findings
The PR objectives mention running the validator against
bmad-create-architecture(expected failures) andbmad-dev-story(expected pass), but this context isn't captured in the specification itself. Future users won't know what "good" looks like.Add an Examples appendix with:
- Minimal valid skill (directory tree, file contents)
- Sample violation for each high-severity rule (with expected finding)
- Sample report output for a skill with 2-3 findings
This makes the spec testable and reduces ambiguity in rule interpretation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/skill-validator.md` around lines 5 - 12, Add an "Examples" appendix to tools/skill-validator.md that demonstrates a minimal valid skill, sample violations for each high-severity rule, and an expected report for a mixed-results run; specifically update the "How to Use" section (heading "How to Use") by appending an "Examples" section that includes (1) a minimal valid skill directory tree and representative file contents, (2) one concrete failing example per high-severity rule with the exact violation text and which rule it triggers, and (3) an example findings report showing 2–3 findings (including rule IDs, file paths, line snippets, and severity) so reviewers can run the validator against bmad-create-architecture and bmad-dev-story and compare expected pass/fail outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/skill-validator.md`:
- Line 18: Update the "Skill directory" definition to explicitly require
bmad-skill-manifest.yaml in addition to SKILL.md and supporting files: change
the text for the "Skill directory" entry to read that the folder contains
SKILL.md, bmad-skill-manifest.yaml, and all supporting files (e.g., workflow.md,
steps/, templates/), so the documentation and downstream validation rules
reflect that the skill self-registers via its own manifest.
- Around line 167-175: The STEP-03 rule's terminal-step detection is ambiguous
and circular; change the rule to require deterministic detection: declare that a
step is terminal only if it both contains no "## NEXT" section and is either the
highest-numbered step within its branch (determine branch by shared workflow ID
or directory) OR the file includes an explicit terminal marker (e.g.,
frontmatter key terminal: true). Update the STEP-03 description to state these
two alternative criteria (no "## NEXT" + highest-numbered-in-branch OR explicit
terminal: true) and clarify how to resolve branch membership (by workflow
frontmatter or directory naming) so automated validators can unambiguously
determine terminal vs non-terminal steps.
- Around line 121-131: The PATH-02 rule currently flags any occurrence of
`{installed_path}` or `installed_path:` even when used in explanatory prose;
update the detection in tools/skill-validator.md so PATH-02 only triggers on
actual definitions or actionable uses by requiring: (1) frontmatter key
`installed_path:` or backticked/inline occurrences that are part of a path
expression (e.g., `{installed_path}/` or `` `installed_path` =` ``), and (2)
ignore prose mentions that are explicitly negating or warning (preceded by words
like "don't", "never", "avoid", "deprecated", "legacy", or phrases like "must
not"), implemented via regex/negative-context checks or an exception list for
phrases; keep the existing Fix instructions for real usages.
- Around line 103-120: PATH-01 currently assumes a single "./" resolution
convention and will produce false positives for skills using the
bmad-quick-dev-new-preview convention; update the validator logic that enforces
PATH-01 to first detect skill type (e.g., check for markers like presence of
domain-steps/, steps/, or a manifest field such as skill-type/manifest metadata)
and apply the appropriate resolution rule per type, or alternatively document in
PATH-01 that the rule assumes the converted-workflow convention; specifically
modify the PATH-01 enforcement branch in the skill path validation module to (1)
implement a detector that examines repository layout/manifest, (2) choose the
resolution strategy (step-file-relative vs skill-root-relative) accordingly, and
(3) run path normalization and validation against that chosen strategy so
bmad-quick-dev-new-preview-style skills are not incorrectly flagged.
- Around line 16-25: Add a new definition "Skill manifest file" describing
bmad-skill-manifest.yaml as the file in the skill directory used for
self-registration, and add three rules: SKILL-06 requiring
bmad-skill-manifest.yaml to exist in every skill directory; SKILL-07 validating
the manifest contains required fields (at minimum canonicalId, name, version,
entryPoint, and any registry metadata) and fail if missing; and SKILL-08
enforcing canonicalId naming/uniqueness rules (must start with the "bmad-"
prefix and follow the project's canonical ID pattern to avoid collisions).
Ensure the new definition and rules are placed alongside the existing
Definitions and rule catalog entries and reference "bmad-skill-manifest.yaml",
"SKILL-06", "SKILL-07", "SKILL-08", and the field name "canonicalId" so they are
discoverable and machine-validateable.
- Around line 151-157: The STEP-01 rule currently allows single-letter branching
but lacks constraints; add a new STEP-01b rule (or an explicit note that
branching is out of scope) that specifies: allowed branch letters (e.g., a..z)
and max branches per step, required presence rules (e.g., branch 'a' must exist
before 'b'/'c'), whether branches are terminal or may rejoin, and how STEP-03
(next-step references) resolves across branches (e.g., next-step can reference
step-02 or step-01b); update the validator to check for missing sibling
branches, orphan branches (e.g., step-01c without 01a/01b), and cyclic branch
references, and reference the existing filename regex
^step-\d{2}[a-z]?-[a-z0-9-]+\.md$ when describing naming/branch-letter parsing.
- Around line 151-157: Add a new validation rule "STEP-01b — No Duplicate Step
Numbers" that scans files in the steps/ directory, extracts the two-digit step
number (NN) from names matching the existing STEP-01 regex
(`step-NN[variant]?-...`), and reports a HIGH-severity error when the same NN
appears in more than one file unless they are distinct variant suffixes (e.g.,
allow 02a vs 02b but not 02 and 02a together); implement detection by collecting
NN and optional suffix for each filename, flagging collisions where the same NN
appears with both a suffix and a plain form or appears in multiple non-variant
files, and update the rule message to instruct renaming/renumbering conflicting
step files.
- Around line 88-100: WF-03's detection is ambiguous; update the WF-03 rule text
in tools/skill-validator.md (the "WF-03 — workflow.md Frontmatter Variables Must
Be Config or Runtime Only" section) to use concrete syntactic patterns: treat
values that start with "{project-root}" or "{planning_artifacts}" (or other
documented config placeholders) as config variables; treat empty, "null", or a
single placeholder token like "{...}" as runtime variables; treat any value that
is a relative path starting with "./", "../", or a bare filename (no braces) as
a forbidden intra-skill path; and if you prefer not to enforce these statically,
mark WF-03 explicitly as a heuristic/manual-review rule and document that
template expressions (e.g., "{planning_artifacts}/{spec_file}") are ambiguous
and will only generate a flagged finding for reviewer inspection.
- Around line 54-60: SKILL-04's regex currently requires two characters and thus
forbids single-character names; update the regex in the SKILL-04 rule from
`^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$` to allow a single-character name while
preserving the 1–64 length and "no leading/trailing hyphen" constraints, e.g.
`^[a-z0-9](?:[a-z0-9-]{0,62}[a-z0-9])?$`; alternatively, if single-character
names should be disallowed, change the rule text to explicitly state "2–64
characters" instead of only "Max 64 characters."
- Around line 238-275: Add a "Formatting Conventions" subsection to the "Report
Template" (near the "Findings" section) that codifies the ambiguous points:
state that rule groups are sorted by severity (CRITICAL, HIGH, MEDIUM, LOW)
descending and then by rule ID ascending; state that multiple instances of the
same rule (e.g., PATH-01) are combined under one heading (single "### {RULE-ID}
— {Rule Title}" with a bulleted list of findings); specify line notation as
either a single number "42" or a range "42-48" (no prefixes like "L" or
"lines"); and require the Summary table to always include all severities with
zero counts shown (e.g., "| MEDIUM | 0 |").
- Around line 228-235: REF-01 — File References Must Resolve currently declares
HIGH severity but claims external config-variable references can’t be resolved,
creating a contradiction; update the rule (REF-01) to either (A) downgrade
severity to MEDIUM and change the Detection text to say “Flag external
references that use malformed variable syntax (e.g., `{unknown_var}`) or
suspicious traversal (e.g., `../../etc/passwd`). Cannot verify if config
variable paths resolve correctly—this check is best-effort.” and keep Fix as
“Correct the path or remove the dead reference,” or (B) keep HIGH but add a
Requirement section that the validator accepts a mapping of config variable
values (stateful verification) and describe how to use that mapping for
definitive checks; ensure the rule text for REF-01 clearly documents which
checks are heuristic vs definitive and adjust the Detection and Severity fields
accordingly.
- Around line 151-207: Add a new HIGH-severity rule "STEP-00 — Step Files Must
Be In steps/ Directory" before STEP-01 that applies to the skill directory and
enforces that all files matching step-*.md live under a steps/ subdirectory;
specify the Rule ("All files matching `step-*.md` must be located in the
`steps/` subdirectory. No step files may exist at skill root or other
locations."), Detection (search for files matching `step-*.md` and verify their
path is under `steps/`), and Fix ("Move misplaced step files into `steps/`"),
and ensure documentation references in STEP-01..STEP-07 reflect that STEP-00
validates presence of the steps/ directory and rejects step files outside it.
- Around line 210-217: Update the SEQ-01 rule to explicitly differentiate "skip"
from "conditional routing": treat a conditional as allowed routing only when it
is exhaustive and mutually exclusive (e.g., an if/else or switch where each
branch maps to different subsequent step labels and covers all logical cases);
treat it as a forbidden skip when a single conditional directs to a later step
without alternative branches (no else/alternate labels), or when it contains
fast-path language like "skip to step" or "skip ahead" that bypasses
intermediate sequential steps. Modify the detection logic that scans for phrases
(the existing SEQ-01 detection list) to (1) require presence of explicit
else/alternate branches or step-label pairs for allowed routing, (2) flag
single-branch conditionals that reference a later step number and do not mention
alternatives as skips, and (3) keep negation handling (e.g., "do NOT skip") as
currently excluded; update examples and tests for SEQ-01 to cover exhaustive
branching vs single-branch skip cases.
- Around line 184-191: The STEP-05 rule exemption for "navigation/dispatch
sections" needs concrete detection criteria: update STEP-05 to list explicit
heading patterns (e.g., `## NAVIGATION`, `## DISPATCH`, `## RESUME FROM`, `##
ROUTING`) and/or structural indicators (e.g., a bulleted or numbered menu of
resumption targets like "- Resume at step-NN" or checkboxes "[ ] Resume at
step-NN") that will be treated as allowed dispatch sections; alternatively state
that if no such heading/structure exists the exemption does not apply (or remove
the exemption entirely). Modify the rule text in STEP-05 to include these exact
heading examples and the structural patterns so validators can reliably identify
dispatch sections versus prohibited forward-loading references.
- Around line 72-100: Add a new validation rule WF-00 that ensures workflow.md
exists in the skill directory: define WF-00 — "workflow.md Must Exist" with
Severity: CRITICAL, Applies to: skill directory, Rule: the skill directory must
contain workflow.md as the canonical entry file, Detection: check for the
presence of workflow.md (file existence), and Fix: instruct to create
workflow.md or correct the skill structure; update the validator registry
alongside WF-01, WF-02, and WF-03 so WF rules fail when workflow.md is missing.
---
Nitpick comments:
In `@tools/skill-validator.md`:
- Around line 5-12: Add an "Examples" appendix to tools/skill-validator.md that
demonstrates a minimal valid skill, sample violations for each high-severity
rule, and an expected report for a mixed-results run; specifically update the
"How to Use" section (heading "How to Use") by appending an "Examples" section
that includes (1) a minimal valid skill directory tree and representative file
contents, (2) one concrete failing example per high-severity rule with the exact
violation text and which rule it triggers, and (3) an example findings report
showing 2–3 findings (including rule IDs, file paths, line snippets, and
severity) so reviewers can run the validator against bmad-create-architecture
and bmad-dev-story and compare expected pass/fail outcomes.
🪄 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: Pro
Run ID: bdb79e82-3239-443c-928c-989583f9e7f4
📒 Files selected for processing (1)
tools/skill-validator.md
Comment on lines
+16
to
+25
| ## Definitions | ||
|
|
||
| - **Skill directory**: the folder containing `SKILL.md` and all supporting files. | ||
| - **Internal reference**: a file path from one file in the skill to another file in the same skill. | ||
| - **External reference**: a file path from a skill file to a file outside the skill directory. | ||
| - **Originating file**: the file that contains the reference (path resolution is relative to this file's location). | ||
| - **Config variable**: a name-value pair whose value comes from the project config file (e.g., `planning_artifacts`, `implementation_artifacts`, `communication_language`). | ||
| - **Runtime variable**: a name-value pair whose value is set during workflow execution (e.g., `spec_file`, `date`, `status`). | ||
| - **Intra-skill path variable**: a frontmatter variable whose value is a path to another file within the same skill — this is an anti-pattern. | ||
|
|
There was a problem hiding this comment.
Critical omission: No validation of bmad-skill-manifest.yaml.
The Definitions section (and the entire rule catalog) never mentions bmad-skill-manifest.yaml, yet learnings confirm this file is required for skill self-registration in the BMAD-METHOD architecture. A skill directory containing only SKILL.md without the manifest cannot be discovered by the installation pipeline.
Add:
- Definition for "Skill manifest file"
- Rule SKILL-06: Manifest file must exist
- Rule SKILL-07: Manifest must contain required fields (canonicalId, etc.)
- Rule SKILL-08: canonicalId must follow naming convention (based on learnings about bmad- prefix)
Based on learnings: "The skill self-registers via its own bmad-skill-manifest.yaml in its directory" and "canonicalId uniqueness across bmad-skill-manifest.yaml files is considered enforced by the naming convention (all IDs carry the bmad- prefix)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/skill-validator.md` around lines 16 - 25, Add a new definition "Skill
manifest file" describing bmad-skill-manifest.yaml as the file in the skill
directory used for self-registration, and add three rules: SKILL-06 requiring
bmad-skill-manifest.yaml to exist in every skill directory; SKILL-07 validating
the manifest contains required fields (at minimum canonicalId, name, version,
entryPoint, and any registry metadata) and fail if missing; and SKILL-08
enforcing canonicalId naming/uniqueness rules (must start with the "bmad-"
prefix and follow the project's canonical ID pattern to avoid collisions).
Ensure the new definition and rules are placed alongside the existing
Definitions and rule catalog entries and reference "bmad-skill-manifest.yaml",
"SKILL-06", "SKILL-07", "SKILL-08", and the field name "canonicalId" so they are
discoverable and machine-validateable.
|
|
||
| ## Definitions | ||
|
|
||
| - **Skill directory**: the folder containing `SKILL.md` and all supporting files. |
There was a problem hiding this comment.
Incomplete definition of "Skill directory."
Line 18 defines a skill directory as "the folder containing SKILL.md and all supporting files." But learnings show that skills also require bmad-skill-manifest.yaml for registration. The definition should be:
Skill directory: the folder containing
SKILL.md,bmad-skill-manifest.yaml, and all supporting files (workflow.md, steps/, templates, etc.).
This aligns the definition with the actual skill architecture and sets up the missing manifest validation rules.
Based on learnings: "The skill self-registers via its own bmad-skill-manifest.yaml in its directory."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/skill-validator.md` at line 18, Update the "Skill directory" definition
to explicitly require bmad-skill-manifest.yaml in addition to SKILL.md and
supporting files: change the text for the "Skill directory" entry to read that
the folder contains SKILL.md, bmad-skill-manifest.yaml, and all supporting files
(e.g., workflow.md, steps/, templates/), so the documentation and downstream
validation rules reflect that the skill self-registers via its own manifest.
Comment on lines
+54
to
+60
| ### SKILL-04 — `name` Format | ||
|
|
||
| - **Severity:** HIGH | ||
| - **Applies to:** `SKILL.md` | ||
| - **Rule:** The `name` value must use only lowercase letters, numbers, and hyphens. Max 64 characters. Must not contain "anthropic" or "claude". | ||
| - **Detection:** Regex test: `^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$`. String search for forbidden substrings. | ||
| - **Fix:** Rename to comply with the format. |
There was a problem hiding this comment.
SKILL-04 regex forbids single-character skill names.
The regex ^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$ requires start character + middle (0-62 chars) + end character, forcing a minimum of 2 characters. Single-character skill names (e.g., x, q) are rejected, yet the rule text only specifies "Max 64 characters" without documenting this minimum-length constraint.
If single-character names should be forbidden, update the rule text to state "2–64 characters." Otherwise, fix the regex:
-^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$
+^[a-z0-9]([a-z0-9-]{0,62}[a-z0-9])?$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/skill-validator.md` around lines 54 - 60, SKILL-04's regex currently
requires two characters and thus forbids single-character names; update the
regex in the SKILL-04 rule from `^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$` to allow a
single-character name while preserving the 1–64 length and "no leading/trailing
hyphen" constraints, e.g. `^[a-z0-9](?:[a-z0-9-]{0,62}[a-z0-9])?$`;
alternatively, if single-character names should be disallowed, change the rule
text to explicitly state "2–64 characters" instead of only "Max 64 characters."
Comment on lines
+72
to
+100
| ### WF-01 — workflow.md Must NOT Have `name` in Frontmatter | ||
|
|
||
| - **Severity:** HIGH | ||
| - **Applies to:** `workflow.md` (if it exists) | ||
| - **Rule:** The `name` field belongs only in `SKILL.md`. If `workflow.md` has YAML frontmatter, it must not contain `name:`. | ||
| - **Detection:** Parse frontmatter and check for `name:` key. | ||
| - **Fix:** Remove the `name:` line from workflow.md frontmatter. | ||
|
|
||
| ### WF-02 — workflow.md Must NOT Have `description` in Frontmatter | ||
|
|
||
| - **Severity:** HIGH | ||
| - **Applies to:** `workflow.md` (if it exists) | ||
| - **Rule:** The `description` field belongs only in `SKILL.md`. If `workflow.md` has YAML frontmatter, it must not contain `description:`. | ||
| - **Detection:** Parse frontmatter and check for `description:` key. | ||
| - **Fix:** Remove the `description:` line from workflow.md frontmatter. | ||
|
|
||
| ### WF-03 — workflow.md Frontmatter Variables Must Be Config or Runtime Only | ||
|
|
||
| - **Severity:** HIGH | ||
| - **Applies to:** `workflow.md` frontmatter | ||
| - **Rule:** Every variable defined in workflow.md frontmatter must be either: | ||
| - A config variable (value references `{project-root}` or a config-derived variable like `{planning_artifacts}`) | ||
| - A runtime variable (value is empty, a placeholder, or set during execution) | ||
| - A legitimate external path expression | ||
|
|
||
| It must NOT be a path to a file within the skill directory. | ||
| - **Detection:** For each frontmatter variable, check if its value resolves to a file inside the skill (e.g., starts with `./`, `{installed_path}`, or is a bare relative path to a sibling file). If so, it is an intra-skill path variable. | ||
| - **Fix:** Remove the variable. Use a hardcoded relative path inline where the file is referenced. | ||
|
|
There was a problem hiding this comment.
Missing rule: workflow.md existence validation.
WF-01, WF-02, and WF-03 all apply to workflow.md (lines 75, 83, 91), yet no rule validates that this file exists in the skill directory. Learnings confirm that workflow.md is the "canonical entry file for verbatim skill directories."
If a skill is missing workflow.md, the WF rules will silently pass (no file to check), but the skill will be broken. Add:
WF-00 — workflow.md Must Exist
- Severity: CRITICAL
- Applies to: skill directory
- Rule: The skill directory must contain
workflow.mdas the canonical entry file. - Detection: Check for file existence.
- Fix: Create workflow.md or verify the skill structure.
Based on learnings: "workflow.md is always the canonical entry file for verbatim skill directories."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/skill-validator.md` around lines 72 - 100, Add a new validation rule
WF-00 that ensures workflow.md exists in the skill directory: define WF-00 —
"workflow.md Must Exist" with Severity: CRITICAL, Applies to: skill directory,
Rule: the skill directory must contain workflow.md as the canonical entry file,
Detection: check for the presence of workflow.md (file existence), and Fix:
instruct to create workflow.md or correct the skill structure; update the
validator registry alongside WF-01, WF-02, and WF-03 so WF rules fail when
workflow.md is missing.
Comment on lines
+88
to
+100
| ### WF-03 — workflow.md Frontmatter Variables Must Be Config or Runtime Only | ||
|
|
||
| - **Severity:** HIGH | ||
| - **Applies to:** `workflow.md` frontmatter | ||
| - **Rule:** Every variable defined in workflow.md frontmatter must be either: | ||
| - A config variable (value references `{project-root}` or a config-derived variable like `{planning_artifacts}`) | ||
| - A runtime variable (value is empty, a placeholder, or set during execution) | ||
| - A legitimate external path expression | ||
|
|
||
| It must NOT be a path to a file within the skill directory. | ||
| - **Detection:** For each frontmatter variable, check if its value resolves to a file inside the skill (e.g., starts with `./`, `{installed_path}`, or is a bare relative path to a sibling file). If so, it is an intra-skill path variable. | ||
| - **Fix:** Remove the variable. Use a hardcoded relative path inline where the file is referenced. | ||
|
|
There was a problem hiding this comment.
WF-03 detection logic is vague and partially unexecutable.
The rule requires distinguishing config variables from runtime variables from intra-skill path variables, but the detection guidance conflates static syntax analysis with runtime semantics:
- "value is empty, a placeholder, or set during execution" — how does static analysis determine what gets "set during execution"?
- "resolves to a file inside the skill" — requires evaluating relative paths, but variable values can be template expressions (e.g.,
{planning_artifacts}/{spec_file}) whose components are unknowable at validation time.
Either:
- Provide concrete syntax patterns for each category (e.g., "starts with
{project-root}or{planning_artifacts}→ config; empty string ornull→ runtime; starts with./or../→ forbidden intra-skill"), or - Mark this as a heuristic rule that produces findings for manual review rather than definitive violations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/skill-validator.md` around lines 88 - 100, WF-03's detection is
ambiguous; update the WF-03 rule text in tools/skill-validator.md (the "WF-03 —
workflow.md Frontmatter Variables Must Be Config or Runtime Only" section) to
use concrete syntactic patterns: treat values that start with "{project-root}"
or "{planning_artifacts}" (or other documented config placeholders) as config
variables; treat empty, "null", or a single placeholder token like "{...}" as
runtime variables; treat any value that is a relative path starting with "./",
"../", or a bare filename (no braces) as a forbidden intra-skill path; and if
you prefer not to enforce these statically, mark WF-03 explicitly as a
heuristic/manual-review rule and document that template expressions (e.g.,
"{planning_artifacts}/{spec_file}") are ambiguous and will only generate a
flagged finding for reviewer inspection.
Comment on lines
+167
to
+175
| ### STEP-03 — Step Must Reference Next Step | ||
|
|
||
| - **Severity:** MEDIUM | ||
| - **Applies to:** step files (except the final step) | ||
| - **Rule:** Each non-terminal step must contain a reference to the next step file for sequential execution. | ||
| - **Detection:** Look for `## NEXT` section or inline reference to a next step file. Remember to resolve the reference from the originating file's directory (PATH-01 applies here too). | ||
| - **Fix:** Add a `## NEXT` section with the relative path to the next step. | ||
| - **Note:** A terminal step is one that has no next-step reference and either contains completion/finalization language or is the highest-numbered step. If a workflow branches, there may be multiple terminal steps. | ||
|
|
There was a problem hiding this comment.
STEP-03 terminal-step detection is circular and unexecutable.
The rule says: "A terminal step is one that has no next-step reference" (line 174). But STEP-03 is the rule that requires next-step references. This creates a logical circle:
- To apply the rule, you must identify non-terminal steps.
- To identify non-terminal steps, you must know which steps are terminal.
- To know which steps are terminal, you check for missing next-step references.
- But that's what the rule is trying to validate.
The note attempts to resolve this ("contains completion/finalization language or is the highest-numbered step") but introduces new ambiguities:
- What phrases constitute "finalization language"?
- Highest-numbered step assumes linear sequencing, but line 156 allows branching.
Provide deterministic terminal-step detection logic:
- "A step is terminal if it is the highest-numbered step in its branch AND contains no
## NEXTsection." - Or require explicit terminal-step marking (e.g., frontmatter
terminal: true).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/skill-validator.md` around lines 167 - 175, The STEP-03 rule's
terminal-step detection is ambiguous and circular; change the rule to require
deterministic detection: declare that a step is terminal only if it both
contains no "## NEXT" section and is either the highest-numbered step within its
branch (determine branch by shared workflow ID or directory) OR the file
includes an explicit terminal marker (e.g., frontmatter key terminal: true).
Update the STEP-03 description to state these two alternative criteria (no "##
NEXT" + highest-numbered-in-branch OR explicit terminal: true) and clarify how
to resolve branch membership (by workflow frontmatter or directory naming) so
automated validators can unambiguously determine terminal vs non-terminal steps.
Comment on lines
+184
to
+191
| ### STEP-05 — No Forward Loading | ||
|
|
||
| - **Severity:** HIGH | ||
| - **Applies to:** step files | ||
| - **Rule:** A step must not load or read future step files until the current step is complete. Just-in-time loading only. | ||
| - **Detection:** Look for instructions to read multiple step files simultaneously, or unconditional references to step files with higher numbers than the current step. Exempt locations: `## NEXT` sections, navigation/dispatch sections that list valid resumption targets, and conditional routing branches. | ||
| - **Fix:** Remove premature step loading. Ensure only the current step is active. | ||
|
|
There was a problem hiding this comment.
STEP-05 exemption for "dispatch sections" is undefined.
Line 189 exempts "navigation/dispatch sections that list valid resumption targets" from the forward-loading prohibition, but provides no detection criteria. What syntactic or semantic patterns identify a dispatch section vs prohibited forward loading?
Without clear markers (e.g., a ## DISPATCH or ## NAVIGATION heading), the exemption is unenforceable — every multi-step reference could be argued as "navigation."
Specify:
- Heading patterns that mark dispatch sections (e.g.,
## NAVIGATION,## RESUME FROM,## ROUTING), or - Structural indicators (e.g., bulleted list of
[X] Resume at step-NNoptions under a menu), or - Remove the exemption and require dispatch logic to reference steps just-in-time.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~189-~189: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ection:** Look for instructions to read multiple step files simultaneously, or unconditional ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/skill-validator.md` around lines 184 - 191, The STEP-05 rule exemption
for "navigation/dispatch sections" needs concrete detection criteria: update
STEP-05 to list explicit heading patterns (e.g., `## NAVIGATION`, `## DISPATCH`,
`## RESUME FROM`, `## ROUTING`) and/or structural indicators (e.g., a bulleted
or numbered menu of resumption targets like "- Resume at step-NN" or checkboxes
"[ ] Resume at step-NN") that will be treated as allowed dispatch sections;
alternatively state that if no such heading/structure exists the exemption does
not apply (or remove the exemption entirely). Modify the rule text in STEP-05 to
include these exact heading examples and the structural patterns so validators
can reliably identify dispatch sections versus prohibited forward-loading
references.
Comment on lines
+210
to
+217
| ### SEQ-01 — No Skip Instructions | ||
|
|
||
| - **Severity:** HIGH | ||
| - **Applies to:** all files | ||
| - **Rule:** No file should instruct the agent to skip steps or optimize step order. Sequential execution is mandatory. | ||
| - **Detection:** Scan for phrases like "skip to step", "jump to step", "skip ahead", "optimize the order", "you may skip". Exclude negation context (e.g., "do NOT skip steps", "NEVER skip") — these are enforcement instructions, not skip instructions. | ||
| - **Exception:** Conditional routing (e.g., "if X, go to step N; otherwise step M") is valid workflow branching, not skipping. | ||
|
|
There was a problem hiding this comment.
SEQ-01 skip vs conditional-routing distinction is underspecified.
The rule forbids "skip to step" but exempts "conditional routing." The detection guidance doesn't explain how to distinguish:
- Skip (forbidden): "If you already have X, skip to step 5."
- Route (allowed): "If X exists, go to step-05a; otherwise step-05b."
Both use similar syntax ("if X, go to step N"). The validator needs concrete differentiation logic:
- Route = exhaustive branching (if/else covering all cases, each branch leading to a different next step)
- Skip = optional fast-path that bypasses intermediate steps in a linear sequence
Without this, the rule will either under-flag (miss skips disguised as routes) or over-flag (mark valid branching as violations).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/skill-validator.md` around lines 210 - 217, Update the SEQ-01 rule to
explicitly differentiate "skip" from "conditional routing": treat a conditional
as allowed routing only when it is exhaustive and mutually exclusive (e.g., an
if/else or switch where each branch maps to different subsequent step labels and
covers all logical cases); treat it as a forbidden skip when a single
conditional directs to a later step without alternative branches (no
else/alternate labels), or when it contains fast-path language like "skip to
step" or "skip ahead" that bypasses intermediate sequential steps. Modify the
detection logic that scans for phrases (the existing SEQ-01 detection list) to
(1) require presence of explicit else/alternate branches or step-label pairs for
allowed routing, (2) flag single-branch conditionals that reference a later step
number and do not mention alternatives as skips, and (3) keep negation handling
(e.g., "do NOT skip") as currently excluded; update examples and tests for
SEQ-01 to cover exhaustive branching vs single-branch skip cases.
tools/skill-validator.md
Outdated
Comment on lines
+228
to
+235
| ### REF-01 — File References Must Resolve | ||
|
|
||
| - **Severity:** HIGH | ||
| - **Applies to:** all files | ||
| - **Rule:** All file path references within the skill (markdown links, backtick paths, frontmatter values) should point to files that plausibly exist. | ||
| - **Detection:** For internal references, verify the target file exists in the skill directory. For external references using config variables, verify the path structure is plausible (you cannot resolve config variables, but you can check that the path after the variable looks reasonable — e.g., `{planning_artifacts}/*.md` is plausible, `{planning_artifacts}/../../etc/passwd` is not). | ||
| - **Fix:** Correct the path or remove the dead reference. | ||
|
|
There was a problem hiding this comment.
REF-01 external reference verification is unexecutable as written.
Line 233 admits: "you cannot resolve config variables, but you can check that the path structure is plausible." This makes external reference validation a best-effort heuristic, not a definitive check.
Yet the rule severity is HIGH and the fix is "Correct the path or remove the dead reference" — implying certainty. The rule should either:
- Downgrade to MEDIUM with detection text: "Flag external references that use malformed variable syntax (e.g.,
{unknown_var}) or suspicious path traversal (e.g.,../../etc/passwd). Cannot verify if config variable paths resolve correctly." - Require a separate verification step where config variable values are provided to the validator (making it stateful).
As written, an LLM following this rule cannot confidently distinguish a broken external path from a valid one.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~232-~232: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ... file path references within the skill (markdown links, backtick paths, frontmatter valu...
(MARKDOWN_NNP)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/skill-validator.md` around lines 228 - 235, REF-01 — File References
Must Resolve currently declares HIGH severity but claims external
config-variable references can’t be resolved, creating a contradiction; update
the rule (REF-01) to either (A) downgrade severity to MEDIUM and change the
Detection text to say “Flag external references that use malformed variable
syntax (e.g., `{unknown_var}`) or suspicious traversal (e.g.,
`../../etc/passwd`). Cannot verify if config variable paths resolve
correctly—this check is best-effort.” and keep Fix as “Correct the path or
remove the dead reference,” or (B) keep HIGH but add a Requirement section that
the validator accepts a mapping of config variable values (stateful
verification) and describe how to use that mapping for definitive checks; ensure
the rule text for REF-01 clearly documents which checks are heuristic vs
definitive and adjust the Detection and Severity fields accordingly.
Comment on lines
+238
to
+275
| ## Report Template | ||
|
|
||
| When reporting findings, use this format: | ||
|
|
||
| ```markdown | ||
| # Skill Validation Report: {skill-name} | ||
|
|
||
| **Directory:** {path} | ||
| **Date:** {date} | ||
| **Files scanned:** {count} | ||
|
|
||
| ## Summary | ||
|
|
||
| | Severity | Count | | ||
| |----------|-------| | ||
| | CRITICAL | N | | ||
| | HIGH | N | | ||
| | MEDIUM | N | | ||
| | LOW | N | | ||
|
|
||
| ## Findings | ||
|
|
||
| ### {RULE-ID} — {Rule Title} | ||
|
|
||
| - **Severity:** {severity} | ||
| - **File:** `{relative-path-within-skill}` | ||
| - **Line:** {line number or range, if identifiable} | ||
| - **Detail:** {what was found} | ||
| - **Fix:** {specific fix for this instance} | ||
|
|
||
| --- | ||
|
|
||
| (repeat for each finding, grouped by rule ID) | ||
|
|
||
| ## Passed Rules | ||
|
|
||
| (list rule IDs that produced no findings) | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Report template formatting is underspecified.
The template provides structure but leaves key formatting decisions ambiguous:
-
Findings sort order (line 270): "grouped by rule ID" — but should rule groups be ordered by severity (CRITICAL first) or alphabetically (PATH-01, REF-01, SEQ-01, SKILL-01...)?
-
Multiple instances per rule: If PATH-01 fires on 5 different files, does the report show:
- One
### PATH-01section with 5 bulleted findings, or - Five separate
### PATH-01sections?
- One
-
Line range format: "Line: {line number or range, if identifiable}" — what syntax?
42,42-48,L42:L48, orlines 42-48? -
Zero-count severity rows: Should the summary table include
| MEDIUM | 0 |if no MEDIUM findings exist, or omit zero rows?
Add a Formatting Conventions subsection specifying:
- Sort: severity desc, then rule ID asc
- Multiple instances: combine under one rule heading as bulleted list
- Line range:
42(single) or42-48(range) - Summary table: include all severities, show 0 for empty categories
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/skill-validator.md` around lines 238 - 275, Add a "Formatting
Conventions" subsection to the "Report Template" (near the "Findings" section)
that codifies the ambiguous points: state that rule groups are sorted by
severity (CRITICAL, HIGH, MEDIUM, LOW) descending and then by rule ID ascending;
state that multiple instances of the same rule (e.g., PATH-01) are combined
under one heading (single "### {RULE-ID} — {Rule Title}" with a bulleted list of
findings); specify line notation as either a single number "42" or a range
"42-48" (no prefixes like "L" or "lines"); and require the Summary table to
always include all severities with zero counts shown (e.g., "| MEDIUM | 0 |").
…d rules
SKILL-05 checks that SKILL.md name matches the directory name.
REF-01 checks that every {variable} traces to frontmatter, config,
or runtime — exempts {{double-curly}} template placeholders.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 16, 2026
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
1 participant
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.
Summary
tools/skill-validator.md— an LLM-readable inference-based validation prompt for skills following the Agent Skills open standardTest plan
bmad-create-architectureskill (known anti-patterns) — should catch installed_path usage and intra-skill path variablesbmad-dev-storyskill (relatively clean) — should pass with zero or minimal findings🤖 Generated with Claude Code