Unify skill reference parsing and classification - #51473
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
|
|
Nice refactor unifying skill reference parsing and classification into a single parseSkillRefSpec source of truth. Good problem framing, focused scope limited to skill reference handling, and new tests covering malformed/unpinned/SHA-pinned reference cases. Looks aligned with project quality standards and ready for review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Reviewed PR #51473 for over-engineering (ponytail-review skill). The diff unifies three duplicated parsing implementations into a single parseSkillRefSpec function/struct, reducing duplication rather than adding complexity. No speculative abstractions, dead flexibility, or hand-rolled stdlib replacements found. Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #51473 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (90 additions detected across 4 files).
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
There was a problem hiding this comment.
Pull request overview
Unifies skill-reference classification across validation, SHA resolution, and activation checkout.
Changes:
- Adds a shared parsed skill-reference representation.
- Reuses classification for validation, pinning, and local checkout paths.
- Adds parser coverage for major reference forms.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/skills_frontmatter.go |
Adds shared parsing and classification. |
pkg/workflow/skills_ref_resolution.go |
Uses parsed refs for SHA pinning. |
pkg/workflow/skills_frontmatter_test.go |
Tests reference classifications. |
pkg/workflow/compiler_activation_job.go |
Reuses classification for local checkout paths. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean refactor. The unified parsedSkillRefSpec struct correctly replaces the scattered isLocalSkillRef calls and inline strings.Cut logic. The behavioral change in resolveSkillRefSpec (now gating on parsed.isRemote instead of just hasAt) is a correctness improvement — malformed remote specs are now skipped consistently rather than only partially filtered. The new TestParseSkillRefSpec table covers the key classification cases. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.1 AIC · ⌖ 9.24 AIC · ⊞ 5.5K
🧪 Test Quality Sentinel — PR #51473Score: 82/100 ✅ Excellent Summary: One new behavioral test function ( Metrics
📋 Per-test classification
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — approving with one minor test-coverage note.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Single source of truth:
parseSkillRefSpeccleanly replaces scatteredstrings.Cut/strings.HasPrefixlogic across three call sites — a textbook deep-module improvement. - ✅ Behaviour preserved: validation rules, warning messages, and resolution paths are identical; only the parsing mechanism is unified.
- ✅ Imports cleaned up: removing
stringsandgitutilfromskills_ref_resolution.gocorrectly narrows each package's dependency surface. - ✅ Good test coverage: six table-driven cases cover local, expression, malformed-remote, unpinned, and SHA-pinned specs.
Finding
- One inline comment: missing empty-spec case in
TestParseSkillRefSpec(minor, non-blocking).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 33.4 AIC · ⌖ 7.54 AIC · ⊞ 7.1K
Comment /matt to run again
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| parsed := parseSkillRefSpec(tt.spec) | ||
| require.Equal(t, tt.local, parsed.isLocal) |
There was a problem hiding this comment.
[/tdd] The TestParseSkillRefSpec table is missing an empty-spec case (spec: ""). parseSkillRefSpec has an explicit early-return for trimmed == "", so it is a distinct code path that should be covered.
💡 Suggested test row
{name: "empty", spec: ""},Without this entry, a future change to the early-return branch would silently regress.
@copilot please address this.
|
@copilot Please refresh this branch if needed, address any remaining review feedback, and run the pr-finisher skill.
|
…seSkillRefSpec Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the review feedback: moved expression detection before the local-path classification in |
PR Triage
|
Skill references were parsed independently during frontmatter validation and compile-time SHA pinning, allowing accepted syntax and resolution behavior to drift.
parseSkillRefSpecfor local paths, expressions, valid remote specs, empty refs, and full SHA refs.run: https://github.com/github/gh-aw/actions/runs/31293411688> Generated by 👨🍳 PR Sous Chef · gpt54 · 3.76 AIC · ⌖ 6.55 AIC · ⊞ 8.5K · ◷