Add --allow-repo-traversal and remove body line count checks - #524
Conversation
|
/evaluate |
There was a problem hiding this comment.
Pull request overview
This PR adds configurable validation options to the skill-validator “check” pipeline so users can relax certain spec constraints (line limits and .. path traversal) when validating repo-specific skills/agents, while keeping existing defaults unchanged.
Changes:
- Introduces
CheckOptions(max declaration/agent line limits, allow parent traversal) and wires it intoCheckConfig. - Adds new CLI flags (
--max-declaration-lines,--max-agent-lines,--allow-repo-traversal) and propagates options throughCheckCommand. - Updates
SkillProfilerandAgentProfilerto respect the configured limits and traversal behavior.
Show a summary per file
| File | Description |
|---|---|
| eng/skill-validator/src/Check/SkillProfiler.cs | Adds CheckOptions support for max body lines and optional .. traversal allowance in file references. |
| eng/skill-validator/src/Check/Models.cs | Introduces CheckOptions model and adds it to CheckConfig. |
| eng/skill-validator/src/Check/CheckCommand.cs | Adds CLI options, builds CheckOptions, and plumbs it through validation calls. |
| eng/skill-validator/src/Check/AgentProfiler.cs | Adds CheckOptions support for max agent body line overrides. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 4
|
As noted in the runtime PR, checking the number of lines isn't useful. Tokens is what we want. Should we remove the line check? |
- Reject --allow-repo-traversal with --plugin mode (plugins must be portable) - Validate --max-declaration-lines and --max-agent-lines are positive integers - Add unit tests for MaxDeclarationLines override in SkillProfiler - Add unit tests for AllowRepoTraversal suppression and depth check preservation - Add unit tests for MaxAgentLines override and fallback to MaxDeclarationLines
…-lines options Line count checks are redundant with BPE token-based complexity classification, which already provides better guidance. Keep --allow-repo-traversal only.
There was a problem hiding this comment.
Pull request overview
This PR introduces a CheckOptions plumbing path through the skill-validator and adds a CLI flag intended to relax SKILL.md file-reference traversal rules for repo-specific validation scenarios.
Changes:
- Added
CheckOptions(currently onlyAllowRepoTraversal) and threaded it throughcheckexecution paths. - Added
--allow-repo-traversalCLI flag and rejected it in--pluginmode to preserve plugin portability constraints. - Removed the previous 500-line body line-limit validation (and associated unit tests) for both skills and agents; added tests around
AllowRepoTraversal.
Show a summary per file
| File | Description |
|---|---|
| eng/skill-validator/src/Check/Models.cs | Introduces CheckOptions and adds it to CheckConfig. |
| eng/skill-validator/src/Check/CheckCommand.cs | Adds --allow-repo-traversal, maps it into config, and blocks it with --plugin. |
| eng/skill-validator/src/Check/SkillProfiler.cs | Adds optional CheckOptions parameter and uses it to allow .. in references. |
| eng/skill-validator/src/Check/AgentProfiler.cs | Adds optional CheckOptions parameter and removes body line-limit validation. |
| eng/skill-validator/tests/Check/SkillProfileTests.cs | Removes body line-count tests; adds AllowRepoTraversal tests. |
| eng/skill-validator/tests/Check/AgentPluginTests.cs | Removes agent body line-count tests. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
eng/skill-validator/src/Check/SkillProfiler.cs:126
AllowRepoTraversalskips the explicit..rejection, but the subsequent depth check (immediately below this hunk) still usessegments.Length - 1, which will count..as directory depth. That means../other-skill/SKILL.mdis still likely to fail with a "directories deep" error even though parent traversal is allowed. Consider normalizing/collapsing./..before computing depth, or skipping the depth rule for paths that traverse upward whenAllowRepoTraversalis enabled.
// Reject parent-directory traversals
if (!allowRepoTraversal && segments.Any(s => s == ".."))
{
errors.Add($"File reference '{refMatch.Groups[1].Value}' uses parent-directory traversal — references must stay within the skill directory.");
continue;
- Files reviewed: 6/6 changed files
- Comments generated: 4
…aversal test - Remove unused CheckOptions parameter from AgentProfiler.AnalyzeAgent and RunAgentsCheckCore since line checks were removed - Fix AllowRepoTraversalSuppressesParentTraversalError test to use a non-deep path (../SKILL.md) and assert no file-ref errors at all
JanKrivanek
changed the title
Skill Validation Results
[1] (Isolated) Quality unchanged but weighted score is -0.5% due to: tokens (53491 → 74813)
Model: claude-opus-4.6 | Judge: claude-opus-4.6 🔍 Full Results - additional metrics and failure investigation steps ▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
Pull request was closed
|
/evaluate |
Skill Validation Results
[1] (Plugin) Quality unchanged but weighted score is -4.1% due to: tokens (48820 → 74743), time (62.4s → 86.7s), tool calls (5 → 6)
Model: claude-opus-4.6 | Judge: claude-opus-4.6 🔍 Full Results - additional metrics and failure investigation steps ▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
) * Add validator options * Address PR review: validate options and add tests - Reject --allow-repo-traversal with --plugin mode (plugins must be portable) - Validate --max-declaration-lines and --max-agent-lines are positive integers - Add unit tests for MaxDeclarationLines override in SkillProfiler - Add unit tests for AllowRepoTraversal suppression and depth check preservation - Add unit tests for MaxAgentLines override and fallback to MaxDeclarationLines * Remove body line count checks and --max-declaration-lines/--max-agent-lines options Line count checks are redundant with BPE token-based complexity classification, which already provides better guidance. Keep --allow-repo-traversal only. * Address review: remove unused CheckOptions from AgentProfiler, fix traversal test - Remove unused CheckOptions parameter from AgentProfiler.AnalyzeAgent and RunAgentsCheckCore since line checks were removed - Fix AllowRepoTraversalSuppressesParentTraversalError test to use a non-deep path (../SKILL.md) and assert no file-ref errors at all
Motivation
--allow-repo-traversaloption for checking repo-specific skills/agents. Plugin skills/agents should not traverse past their dirs (they can be distributed in isolation), but repo-specific agents/skills are placed in a folder structure and disallowing them to refer to precise locations of docs, configs, scripts etc. forces an extra discovery turn.Changes
CheckOptionswithAllowRepoTraversaland wires it intoCheckConfig--allow-repo-traversalCLI flag, rejected in--pluginmode (plugins must be portable)SkillProfilerto respect the traversal optionSkillProfilerandAgentProfiler(superseded by BPE token warnings)FYI @ViktorHofer, @danmoseley