1901 aligning coding standards skill discovery - #2358
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2358 +/- ##
==========================================
- Coverage 82.76% 82.74% -0.03%
==========================================
Files 154 154
Lines 20853 20831 -22
Branches 13 13
==========================================
- Hits 17259 17236 -23
- Misses 3592 3593 +1
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d6d15dc to
b538028
Compare
WilliamBerryiii
left a comment
WilliamBerryiii
left a comment
There was a problem hiding this comment.
It looks like either the eval matrix job is broken, or the discovery system has a bug as the evals weren't run over the new tests. Might want to do a root cause analysis on that and file a bug.
f164d99 to
ff26b45
Compare
|
@WilliamBerryiii thanks for the review! |
add behavior tests - Reframe standards discovery via Workspace/User/Bundled origin precedence and coverage-based stacking - Add code-review stimuli: user-tier precedence, unknown-origin dedup, stacked coverage - Regenerate agent-behavior eval.yaml from partials
replaced with named origin references fixed spellcheck
| * **Define file groups.** For coverage decisions, treat each distinct literal extension in `extensions` as a changed file group. Also treat obvious framework or tool signals from `files` as groups when they appear in filenames or path segments. If a language or framework is uncertain, use the literal extension group rather than inferring a broader group. | ||
| * **Preserve origin evidence.** Keep each candidate skill's exact `name`, `description`, and any source label or path together throughout selection. Treat workspace paths as Workspace, user-profile paths or labels as User, and extension, plugin, or bundled labels as Bundled. If no source label or path is available, mark the origin as Unknown rather than inferring it from content. | ||
| * **Resolve duplicates.** If same-named skills are available and their origin is identifiable, prefer Workspace over User over Bundled. If origin is Unknown, load one same-named skill only, do not merge bodies, and mention the ambiguous duplicate in the one-line return summary. | ||
| * **Load stacked skills.** Load distinct-named matches needed to cover every changed language, framework, or literal extension so their checklists stack. Do not drop a skill if it is the only match for any changed file group. When many skills match, include additional skills only when they cover otherwise-uncovered file groups; drop description-only matches first. |
There was a problem hiding this comment.
The previous step 2 had an explicit ceiling of "up to 8 matching skills." That cap appears to have been removed in this revision. Was that intentional?
The new stacking heuristic (cover every changed file group, drop description-only matches first) is a quality improvement, but on a diff that genuinely touches many distinct languages or frameworks, there is no longer a hard upper bound on how many skills the agent loads. On smaller models, an unbounded load could exhaust the context window.
If the removal was unintentional, consider appending "Load at most 8 skills total." to this sub-bullet to restore the guard.
| ## Extending with Custom Skills | ||
|
|
||
| The Standards and Accessibility perspectives discover skills dynamically at review time. You extend coverage by adding `SKILL.md` files to your repository without modifying the agent itself. See [Language Skills](language-skills.md) for the full guide on built-in skills, skill stacking, and authoring enterprise-specific standards. | ||
| The Standards perspective discovers coding-standards skills dynamically at review time. You extend coverage by adding `SKILL.md` files under a supported repository skill root without modifying the agent itself. See [Language Skills](language-skills.md) for the full guide on built-in skills, skill stacking, same-name precedence, and authoring enterprise-specific standards. |
There was a problem hiding this comment.
The previous sentence also covered the Accessibility perspective: "The Standards and Accessibility perspectives discover skills dynamically at review time." That reference has been removed here.
Was this intentional scoping for this PR? If the Accessibility perspective still supports custom skill authoring, consider keeping a mention rather than dropping it entirely. For example:
The Standards perspective discovers coding-standards skills dynamically at review time; the Accessibility perspective uses the same extension mechanism for accessibility skills. You extend either by adding
SKILL.mdfiles under a supported repository skill root without modifying the agent itself.
4 participants
Pull Request
Description
Aligns the Code Review Standards perspective with platform-supported coding-standards skill discovery roots for issue #1901.
This PR updates the coding-standards standards subagent and docs to clarify that skill discovery starts from supported roots, then semantic matching selects relevant discovered candidates. It also documents same-name de-duplication, repository precedence over the built-in baseline, additive stacking for distinct skill names, and visible handling for contradictory distinct skills.
The change adds advisory Vally coverage for the requested merge-policy scenarios: no conflict, same-name collision, and contradictory distinct skills.
Related Issue(s)
Fixes #1901
Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
AI Artifacts:
prompt-builderagent and addressed all feedback.github/instructions/*.instructions.md).github/prompts/*.prompt.md).github/agents/*.agent.md).github/skills/*/SKILL.md).github/hooks/*/*.json)evals/)Other:
.ps1,.sh,.py)Sample Prompts (for AI Artifact Contributions)
User Request:
Run the Code Review Standards perspective on a Python diff when both built-in and repository-authored coding-standards skills are available.
Execution Flow:
Output Artifacts:
Success Indicators:
For detailed contribution requirements, see:
Testing
Automated validation performed:
pwsh scripts/evals/Build-AgentBehaviorSpec.ps1 -Force- passednpm run eval:lint:schema- passedFocused advisory Vally eval - passed
Command:
npx vally eval --eval-spec evals/agent-behavior/eval.yaml --tag scenario=merge-no-conflict,merge-name-collision,merge-content-contradiction --runs 1 --output-dir /tmp/vally-merge-resultsResult:
agent-behavior: passed (score: 100.0%, threshold: 70.0%)npm run lint:md -- docs/agents/code-review/language-skills.md docs/agents/code-review/README.md .github/agents/coding-standards/subagents/code-review-standards.agent.md- passednpm run lint:frontmatter- passednpm run lint:md-links- passednpm run spell-check- passedgit diff --check -- .github/agents/coding-standards/subagents/code-review-standards.agent.md docs/agents/code-review/README.md docs/agents/code-review/language-skills.md evals/agent-behavior/stimuli/code-review-standards.yml evals/agent-behavior/eval.yaml- passedVS Code diagnostics on touched files - passed
Manual testing was not performed.
Checklist
Required Checks
AI Artifact Contributions
/prompt-analyzeto review contributionprompt-builderreviewRequired Automated Checks
The following validation commands must pass before merging:
npm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run validate:skills(N/A - no skill files changed)npm run lint:md-linksnpm run lint:ps(N/A - no PowerShell files changed)npm run eval:lint:schemanpm run plugin:generatenpm run docs:testSecurity Considerations
GHCP Artifact Maturity
Warning
This PR includes experimental GHCP artifacts that may have breaking changes.
.github/agents/coding-standards/subagents/code-review-standards.agent.md.github/agents/coding-standards/subagents/code-review-standards.agent.mdGHCP Maturity Acknowledgment
Additional Notes
This PR is intentionally scoped to coding-standards discovery and merge behavior only. It does not broaden security or accessibility skill discovery.