Skip to content

Commit bb5d1f7

Browse files
authored
fix: correct gh skill install invocation for frontmatter skills (#42543)
1 parent 34a5b34 commit bb5d1f7

5 files changed

Lines changed: 228 additions & 132 deletions

File tree

.github/workflows/mattpocock-skills-reviewer.lock.yml

Lines changed: 8 additions & 82 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
// @ts-check
2+
/// <reference types="@actions/github-script" />
3+
4+
const fs = require("fs");
5+
const path = require("path");
6+
7+
/**
8+
* @param {string} rawSkills
9+
* @returns {string[]}
10+
*/
11+
function parseSkillSpecs(rawSkills) {
12+
return (rawSkills || "")
13+
.split(/\r?\n/)
14+
.map(skill => skill.trim())
15+
.filter(Boolean);
16+
}
17+
18+
/**
19+
* @typedef {{args: string[]; displaySpec: string}} SkillInstallCommand
20+
*/
21+
22+
/**
23+
* @param {string} skillSpec
24+
* @param {string} skillsDst
25+
* @returns {SkillInstallCommand}
26+
*/
27+
function buildSkillInstallCommand(skillSpec, skillsDst) {
28+
const atIndex = skillSpec.lastIndexOf("@");
29+
const hasPin = atIndex >= 0;
30+
const skillBase = hasPin ? skillSpec.slice(0, atIndex) : skillSpec;
31+
const skillRef = hasPin ? skillSpec.slice(atIndex + 1) : "";
32+
const parts = skillBase.split("/");
33+
const pinArgs = skillRef ? ["--pin", skillRef] : [];
34+
35+
if (parts.length >= 3) {
36+
return {
37+
displaySpec: skillSpec,
38+
args: ["skill", "install", `${parts[0]}/${parts[1]}`, parts.slice(2).join("/"), ...pinArgs, "--dir", skillsDst, "--force"],
39+
};
40+
}
41+
42+
if (parts.length === 2) {
43+
return {
44+
displaySpec: skillSpec,
45+
args: ["skill", "install", skillBase, "--all", ...pinArgs, "--dir", skillsDst, "--force"],
46+
};
47+
}
48+
49+
return {
50+
displaySpec: skillSpec,
51+
args: ["skill", "install", skillSpec, "--dir", skillsDst, "--force"],
52+
};
53+
}
54+
55+
/**
56+
* @param {string} skillsDst
57+
* @returns {number}
58+
*/
59+
function countInstalledSkillFiles(skillsDst) {
60+
if (!fs.existsSync(skillsDst)) {
61+
return 0;
62+
}
63+
64+
let count = 0;
65+
const stack = [skillsDst];
66+
while (stack.length > 0) {
67+
const currentDir = stack.pop();
68+
if (!currentDir) {
69+
continue;
70+
}
71+
for (const entry of fs.readdirSync(currentDir, { withFileTypes: true })) {
72+
const entryPath = path.join(currentDir, entry.name);
73+
if (entry.isDirectory()) {
74+
stack.push(entryPath);
75+
continue;
76+
}
77+
if (entry.isFile() && entry.name === "SKILL.md") {
78+
count++;
79+
}
80+
}
81+
}
82+
83+
return count;
84+
}
85+
86+
/**
87+
* @param {string} skillDir
88+
* @param {string[]} skills
89+
* @param {number} installedSkillCount
90+
* @returns {Promise<void>}
91+
*/
92+
async function writeSkillSummary(skillDir, skills, installedSkillCount) {
93+
core.summary
94+
.addRaw("### Frontmatter skills installed\n\n")
95+
.addRaw(`- Engine skill directory: \`${skillDir}\`\n`)
96+
.addRaw(`- Requested references: \`${JSON.stringify(skills)}\`\n`)
97+
.addRaw(`- Installed SKILL.md files: ${installedSkillCount}\n`);
98+
await core.summary.write();
99+
}
100+
101+
async function main() {
102+
const skillDir = process.env.GH_AW_SKILL_DIR || "";
103+
const skills = parseSkillSpecs(process.env.GH_AW_FRONTMATTER_SKILLS || "");
104+
const skillsDst = path.join("/tmp/gh-aw", skillDir);
105+
106+
fs.mkdirSync(skillsDst, { recursive: true });
107+
108+
core.info(`Installing frontmatter skills to ${skillsDst}`);
109+
core.info("Existing skills at destination may be replaced (--force) to ensure pinned refs are up to date");
110+
111+
for (const skillSpec of skills) {
112+
core.info(`Installing skill reference: ${skillSpec}`);
113+
const command = buildSkillInstallCommand(skillSpec, skillsDst);
114+
await exec.exec("gh", command.args);
115+
}
116+
117+
const installedSkillCount = countInstalledSkillFiles(skillsDst);
118+
core.info(`Installed ${installedSkillCount} skill file(s)`);
119+
await writeSkillSummary(skillDir, skills, installedSkillCount);
120+
}
121+
122+
module.exports = { main, parseSkillSpecs, buildSkillInstallCommand, countInstalledSkillFiles };
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
2+
import fs from "fs";
3+
import os from "os";
4+
import path from "path";
5+
import { createRequire } from "module";
6+
7+
const require = createRequire(import.meta.url);
8+
const script = require("./install_frontmatter_skills.cjs");
9+
10+
describe("install_frontmatter_skills", () => {
11+
let originalEnv;
12+
let originalCore;
13+
let originalExec;
14+
let tempRoot;
15+
16+
beforeEach(() => {
17+
originalEnv = {
18+
GH_AW_SKILL_DIR: process.env.GH_AW_SKILL_DIR,
19+
GH_AW_FRONTMATTER_SKILLS: process.env.GH_AW_FRONTMATTER_SKILLS,
20+
};
21+
originalCore = global.core;
22+
originalExec = global.exec;
23+
tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-frontmatter-skills-"));
24+
25+
global.core = {
26+
info: vi.fn(),
27+
summary: {
28+
addRaw: vi.fn().mockReturnThis(),
29+
write: vi.fn().mockResolvedValue(undefined),
30+
},
31+
};
32+
global.exec = {
33+
exec: vi.fn().mockResolvedValue(0),
34+
};
35+
});
36+
37+
afterEach(() => {
38+
if (originalEnv.GH_AW_SKILL_DIR === undefined) {
39+
delete process.env.GH_AW_SKILL_DIR;
40+
} else {
41+
process.env.GH_AW_SKILL_DIR = originalEnv.GH_AW_SKILL_DIR;
42+
}
43+
if (originalEnv.GH_AW_FRONTMATTER_SKILLS === undefined) {
44+
delete process.env.GH_AW_FRONTMATTER_SKILLS;
45+
} else {
46+
process.env.GH_AW_FRONTMATTER_SKILLS = originalEnv.GH_AW_FRONTMATTER_SKILLS;
47+
}
48+
global.core = originalCore;
49+
global.exec = originalExec;
50+
fs.rmSync(tempRoot, { recursive: true, force: true });
51+
fs.rmSync("/tmp/gh-aw/.claude", { recursive: true, force: true });
52+
});
53+
54+
it("splits repo-level and path-level skill specs into gh skill install arguments", () => {
55+
expect(script.buildSkillInstallCommand("githubnext/skills@abc123", "/tmp/gh-aw/.claude/skills").args).toEqual(["skill", "install", "githubnext/skills", "--all", "--pin", "abc123", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
56+
expect(script.buildSkillInstallCommand("githubnext/skills/review/security@abc123", "/tmp/gh-aw/.claude/skills").args).toEqual([
57+
"skill",
58+
"install",
59+
"githubnext/skills",
60+
"review/security",
61+
"--pin",
62+
"abc123",
63+
"--dir",
64+
"/tmp/gh-aw/.claude/skills",
65+
"--force",
66+
]);
67+
});
68+
69+
it("omits --pin when the resolved skill spec is unpinned", () => {
70+
expect(script.buildSkillInstallCommand("githubnext/skills/review/security", "/tmp/gh-aw/.claude/skills").args).toEqual(["skill", "install", "githubnext/skills", "review/security", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
71+
});
72+
73+
it("reads skill specs from the env var and installs them at runtime", async () => {
74+
process.env.GH_AW_SKILL_DIR = ".claude/skills";
75+
process.env.GH_AW_FRONTMATTER_SKILLS = ["githubnext/skills@abc123", "githubnext/skills/review/security@def456", "${{ inputs.skill_ref }}"].join("\n");
76+
fs.mkdirSync("/tmp/gh-aw/.claude/skills/example", { recursive: true });
77+
fs.writeFileSync("/tmp/gh-aw/.claude/skills/example/SKILL.md", "# test\n", "utf8");
78+
79+
await script.main();
80+
81+
expect(global.exec.exec).toHaveBeenNthCalledWith(1, "gh", ["skill", "install", "githubnext/skills", "--all", "--pin", "abc123", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
82+
expect(global.exec.exec).toHaveBeenNthCalledWith(2, "gh", ["skill", "install", "githubnext/skills", "review/security", "--pin", "def456", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
83+
expect(global.exec.exec).toHaveBeenNthCalledWith(3, "gh", ["skill", "install", "${{ inputs.skill_ref }}", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
84+
expect(global.core.summary.addRaw).toHaveBeenCalledWith(expect.stringContaining("### Frontmatter skills installed"));
85+
expect(global.core.summary.addRaw).toHaveBeenCalledWith(expect.stringContaining('["githubnext/skills@abc123","githubnext/skills/review/security@def456","${{ inputs.skill_ref }}"]'));
86+
});
87+
});

pkg/workflow/activation_skills_step_test.go

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,9 @@ func TestBuildActivationJob_AddsFrontmatterSkillsInstallSteps(t *testing.T) {
3636
assert.Contains(t, steps, "Upgrade gh CLI for frontmatter skills", "expected gh upgrade step in activation job")
3737
assert.Contains(t, steps, "Install frontmatter skills", "expected frontmatter skills install step in activation job")
3838
assert.Contains(t, steps, "GH_AW_SKILL_DIR: \".claude/skills\"", "expected engine skill directory env var")
39-
assert.Contains(t, steps, "GH_AW_SKILLS_SUMMARY: '[\"githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de6\",\"githubnext/skills/review/security@1f181b37d3fe5862ab590648f25a292e345b5de6\"]'", "expected summary env var for requested skills")
40-
assert.Contains(t, steps, "GH_AW_SKILL_SPEC_0: \"githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de6\"", "expected first skill env var")
41-
assert.Contains(t, steps, "GH_AW_SKILL_SPEC_1: \"githubnext/skills/review/security@1f181b37d3fe5862ab590648f25a292e345b5de6\"", "expected second skill env var")
42-
assert.Contains(t, steps, "skill_spec=\"${GH_AW_SKILL_SPEC_0}\"", "expected runtime install loop to read first skill from env")
43-
assert.Contains(t, steps, "install_args+=(--all)", "expected runtime repository-scope detection")
44-
assert.Contains(t, steps, "gh skill install \"${skill_spec}\" \"${install_args[@]}\" --dir \"${SKILLS_DST}\" --force", "expected runtime install command to use quoted env values")
45-
assert.Contains(t, steps, "### Frontmatter skills installed", "expected step summary output")
39+
assert.Contains(t, steps, "GH_AW_FRONTMATTER_SKILLS: \"githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de6\\ngithubnext/skills/review/security@1f181b37d3fe5862ab590648f25a292e345b5de6\"", "expected skills env var")
40+
assert.Contains(t, steps, "const { main } = require('${{ runner.temp }}/gh-aw/actions/install_frontmatter_skills.cjs');", "expected github-script runtime loader for skill install")
41+
assert.NotContains(t, steps, "GH_AW_SKILL_SPEC_0", "expected per-skill env vars to be removed")
4642
}
4743

4844
func TestBuildActivationJob_AddsExpressionSkillInstallSteps(t *testing.T) {
@@ -65,9 +61,8 @@ func TestBuildActivationJob_AddsExpressionSkillInstallSteps(t *testing.T) {
6561
require.NotNil(t, job)
6662

6763
steps := strings.Join(job.Steps, "")
68-
assert.Contains(t, steps, "GH_AW_SKILL_SPEC_0: \"${{ inputs.skill_ref }}\"", "expected whole-expression skill env var")
69-
assert.Contains(t, steps, "GH_AW_SKILL_SPEC_1: \"githubnext/skills@${{ github.sha }}\"", "expected expression-ref skill env var")
70-
assert.NotContains(t, steps, "echo \"Installing skill reference: ${{ inputs.skill_ref }}\"", "expression should not be interpolated directly into the run script")
64+
assert.Contains(t, steps, "GH_AW_FRONTMATTER_SKILLS: \"${{ inputs.skill_ref }}\\ngithubnext/skills@${{ github.sha }}\"", "expected skills env var to preserve expressions for runtime resolution")
65+
assert.NotContains(t, steps, "GH_AW_SKILL_SPEC_0", "expected per-skill env vars to be removed")
7166
}
7267

7368
func TestBuildActivationJob_NoSkillsStepsWhenSkillsAbsent(t *testing.T) {

0 commit comments

Comments
 (0)