-
Notifications
You must be signed in to change notification settings - Fork 445
Add frontmatter skills support with activation-time gh skill install and engine wiring
#42426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
0f2fa96
ff9a6ad
fffec7e
54ee1ac
6c1edb2
a1e89b7
0a451d9
c11044d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,15 @@ | |
| ["ci", "testing"] | ||
| ] | ||
| }, | ||
| "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.", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/grill-with-docs] The 💡 Suggested wording@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
| "items": { | ||
| "type": "string", | ||
| "pattern": "^[A-Za-z0-9_.-]+\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+)*)?@[0-9a-f]{40}$" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot add support for github actions expressions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in |
||
| }, | ||
| "examples": [["githubnext/awesome-skills@1f181b37d3fe5862ab590648f25a292e345b5de6"], ["githubnext/awesome-skills/review/security@1f181b37d3fe5862ab590648f25a292e345b5de6"]] | ||
| }, | ||
| "metadata": { | ||
| "type": "object", | ||
| "description": "Optional metadata field for storing custom key-value pairs compatible with the custom agent spec. Key names are limited to 64 characters, and values are limited to 1024 characters.", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| //go:build !integration | ||
|
|
||
| package workflow | ||
|
|
||
| import ( | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestBuildActivationJob_AddsFrontmatterSkillsInstallSteps(t *testing.T) { | ||
| compiler := NewCompiler(WithVersion("dev")) | ||
| compiler.SetActionMode(ActionModeDev) | ||
|
|
||
| data := &WorkflowData{ | ||
| Name: "skills-workflow", | ||
| On: `"on": | ||
| workflow_dispatch:`, | ||
| AI: "copilot", | ||
| EngineConfig: &EngineConfig{ | ||
| ID: "claude", | ||
| }, | ||
| Skills: []string{ | ||
| "githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de6", | ||
| "githubnext/skills/review/security@1f181b37d3fe5862ab590648f25a292e345b5de6", | ||
| }, | ||
| } | ||
|
|
||
| job, err := compiler.buildActivationJob(data, false, "", "skills.lock.yml") | ||
| require.NoError(t, err) | ||
| require.NotNil(t, job) | ||
|
|
||
| steps := strings.Join(job.Steps, "") | ||
| assert.Contains(t, steps, "Upgrade gh CLI for frontmatter skills") | ||
| assert.Contains(t, steps, "Install frontmatter skills") | ||
| assert.Contains(t, steps, "GH_AW_SKILL_DIR: \".claude/skills\"") | ||
| 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") | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage for expression-based skill specs This test exercises SHA-pinned specs only.
A complementary test case covering these forms would document the expected generated YAML (e.g., that whole-expression specs produce no @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test only covers the happy path (skills present). There is no test verifying that when 💡 Suggested test to addfunc 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -519,6 +519,68 @@ func (c *Compiler) addActivationVersionCheckStep(ctx *activationJobBuildContext) | |
| ctx.steps = append(ctx.steps, generateGitHubScriptWithRequire("check_version_updates.cjs")) | ||
| } | ||
|
|
||
| func (c *Compiler) addActivationSkillInstallSteps(ctx *activationJobBuildContext) { | ||
| if len(ctx.data.Skills) == 0 { | ||
| return | ||
| } | ||
|
|
||
| engineID := "" | ||
| if ctx.data.EngineConfig != nil { | ||
| engineID = ctx.data.EngineConfig.ID | ||
| } | ||
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/codebase-design] Every sibling 💡 Suggested fixChange the signature to return 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 if err := c.addActivationSkillInstallSteps(ctx); err != nil {
return nil, fmt.Errorf("failed to add skill install steps: %w", err)
}@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
| return | ||
| } | ||
| escapedSkillSpecsJSON := strings.ReplaceAll(string(skillSpecsJSON), "'", "''") | ||
|
|
||
| ctx.steps = append(ctx.steps, " - name: Upgrade gh CLI for frontmatter skills\n") | ||
| ctx.steps = append(ctx.steps, " run: |\n") | ||
| ctx.steps = append(ctx.steps, " set -euo pipefail\n") | ||
| ctx.steps = append(ctx.steps, " bash \"${RUNNER_TEMP}/gh-aw/actions/install_gh_cli.sh\"\n") | ||
| ctx.steps = append(ctx.steps, " GH_VERSION=$(gh --version | awk 'NR==1 {print $3}')\n") | ||
| ctx.steps = append(ctx.steps, " REQUIRED=\"2.90.0\"\n") | ||
| ctx.steps = append(ctx.steps, " echo \"gh version: ${GH_VERSION}\"\n") | ||
| ctx.steps = append(ctx.steps, " if ! printf '%s\\n%s\\n' \"$REQUIRED\" \"$GH_VERSION\" | sort -V -C; then\n") | ||
| ctx.steps = append(ctx.steps, " echo \"::error::gh ${GH_VERSION} is older than required ${REQUIRED} (gh skill support requires v2.90+)\"\n") | ||
| ctx.steps = append(ctx.steps, " exit 1\n") | ||
| ctx.steps = append(ctx.steps, " fi\n") | ||
|
|
||
| ctx.steps = append(ctx.steps, " - name: Install frontmatter skills\n") | ||
| 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)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/improve-codebase-architecture] This creates a subtle misleading signal: an operator reading the YAML might think @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
| ctx.steps = append(ctx.steps, " run: |\n") | ||
| ctx.steps = append(ctx.steps, " set -euo pipefail\n") | ||
| ctx.steps = append(ctx.steps, " SKILLS_DST=\"/tmp/gh-aw/${GH_AW_SKILL_DIR}\"\n") | ||
| ctx.steps = append(ctx.steps, " mkdir -p \"${SKILLS_DST}\"\n") | ||
| 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)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: shell injection risk for expression-based skill specs The The GitHub Actions security recommendation is to pass user-controlled inputs through env variables, not directly in 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 @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
| 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)) | ||
| } | ||
|
|
||
| ctx.steps = append(ctx.steps, " SKILL_COUNT=$(find \"${SKILLS_DST}\" -name \"SKILL.md\" | wc -l | tr -d '[:space:]')\n") | ||
| ctx.steps = append(ctx.steps, " echo \"Installed ${SKILL_COUNT} skill file(s)\"\n") | ||
| ctx.steps = append(ctx.steps, " core_summary_path=\"${GITHUB_STEP_SUMMARY:-}\"\n") | ||
| ctx.steps = append(ctx.steps, " if [ -n \"${core_summary_path}\" ]; then\n") | ||
| ctx.steps = append(ctx.steps, " {\n") | ||
| ctx.steps = append(ctx.steps, " echo \"### Frontmatter skills installed\"\n") | ||
| ctx.steps = append(ctx.steps, " echo \"\"\n") | ||
| ctx.steps = append(ctx.steps, " echo \"- Engine skill directory: \\`${GH_AW_SKILL_DIR}\\`\"\n") | ||
| ctx.steps = append(ctx.steps, " echo \"- Requested references: \\`${GH_AW_SKILLS}\\`\"\n") | ||
| ctx.steps = append(ctx.steps, " echo \"- Installed SKILL.md files: ${SKILL_COUNT}\"\n") | ||
| ctx.steps = append(ctx.steps, " } >> \"${core_summary_path}\"\n") | ||
| ctx.steps = append(ctx.steps, " fi\n") | ||
| } | ||
|
|
||
| func (c *Compiler) addActivationTextOutputStep(ctx *activationJobBuildContext) error { | ||
| if !ctx.data.NeedsTextOutput { | ||
| return nil | ||
|
|
@@ -766,15 +828,19 @@ func (c *Compiler) addActivationArtifactUploadStep(ctx *activationJobBuildContex | |
| ctx.steps = append(ctx.steps, " /tmp/gh-aw/aw-prompts/prompt-import-tree.json\n") | ||
| ctx.steps = append(ctx.steps, " /tmp/gh-aw/"+constants.GithubRateLimitsFilename+"\n") | ||
| ctx.steps = append(ctx.steps, " /tmp/gh-aw/base\n") | ||
| // Include the engine-specific sub-agents staging directory (inline sub-agents are enabled by default). | ||
| engineID := "" | ||
| if ctx.data.EngineConfig != nil { | ||
| engineID = ctx.data.EngineConfig.ID | ||
| } | ||
| // Include the engine-specific sub-agent staging directory only when inline agents are enabled. | ||
| if isFeatureEnabled(constants.FeatureFlag("inline-agents"), ctx.data) { | ||
| engineID := "" | ||
| if ctx.data.EngineConfig != nil { | ||
| engineID = ctx.data.EngineConfig.ID | ||
| } | ||
| subAgentDir := GetEngineSubAgentDir(engineID) | ||
| skillDir := GetEngineSkillDir(engineID) | ||
| ctx.steps = append(ctx.steps, fmt.Sprintf(" /tmp/gh-aw/%s\n", subAgentDir)) | ||
| } | ||
| // Always include the engine-specific skill directory when either inline skills are enabled | ||
| // or frontmatter skills are configured. | ||
| if isFeatureEnabled(constants.FeatureFlag("inline-agents"), ctx.data) || len(ctx.data.Skills) > 0 { | ||
| skillDir := GetEngineSkillDir(engineID) | ||
| ctx.steps = append(ctx.steps, fmt.Sprintf(" /tmp/gh-aw/%s\n", skillDir)) | ||
| } | ||
| ctx.steps = append(ctx.steps, " if-no-files-found: ignore\n") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -965,6 +965,12 @@ func (c *Compiler) generateCreateAwInfo(yaml *strings.Builder, data *WorkflowDat | |
| fmt.Fprintf(yaml, " GH_AW_INFO_FEATURES: '%s'\n", escapedFeaturesJSON) | ||
| } | ||
| } | ||
| if len(data.Skills) > 0 { | ||
| if skillsJSON, err := json.Marshal(data.Skills); err == nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] When The features block three lines above follows the exact same pattern — consider at least adding a 💡 Suggested fixif 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
| escapedSkillsJSON := strings.ReplaceAll(string(skillsJSON), "'", "''") | ||
| fmt.Fprintf(yaml, " GH_AW_INFO_SKILLS: '%s'\n", escapedSkillsJSON) | ||
| } | ||
| } | ||
| fmt.Fprintf(yaml, " uses: %s\n", getCachedActionPin("actions/github-script", data)) | ||
| yaml.WriteString(" with:\n") | ||
| yaml.WriteString(" script: |\n") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd]
parseSkillsFromEnvaccepts any non-empty string intoaw_info.jsonwithout validating the spec format. While the Go compiler enforces theowner/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`
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in
a1e89b7.generate_aw_info.test.cjsnow covers malformed JSON, non-array payloads, and non-string items forGH_AW_INFO_SKILLS.