Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .github/workflows/mattpocock-skills-reviewer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 11 additions & 5 deletions actions/setup/js/install_frontmatter_skills.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -22,33 +22,35 @@ function parseSkillSpecs(rawSkills) {
/**
* @param {string} skillSpec
* @param {string} skillsDst
* @param {string} skillInstallAgent
* @returns {SkillInstallCommand}
*/
function buildSkillInstallCommand(skillSpec, skillsDst) {
function buildSkillInstallCommand(skillSpec, skillsDst, skillInstallAgent = "") {
const atIndex = skillSpec.lastIndexOf("@");
const hasPin = atIndex >= 0;
const skillBase = hasPin ? skillSpec.slice(0, atIndex) : skillSpec;
const skillRef = hasPin ? skillSpec.slice(atIndex + 1) : "";
const parts = skillBase.split("/");
const pinArgs = skillRef ? ["--pin", skillRef] : [];
const agentArgs = skillInstallAgent ? ["--agent", skillInstallAgent] : [];

if (parts.length >= 3) {
return {
displaySpec: skillSpec,
args: ["skill", "install", `${parts[0]}/${parts[1]}`, parts.slice(2).join("/"), ...pinArgs, "--dir", skillsDst, "--force"],
args: ["skill", "install", `${parts[0]}/${parts[1]}`, parts.slice(2).join("/"), ...pinArgs, ...agentArgs, "--dir", skillsDst, "--force"],
};
}

if (parts.length === 2) {
return {
displaySpec: skillSpec,
args: ["skill", "install", skillBase, "--all", ...pinArgs, "--dir", skillsDst, "--force"],
args: ["skill", "install", skillBase, "--all", ...pinArgs, ...agentArgs, "--dir", skillsDst, "--force"],
};
}

return {
displaySpec: skillSpec,
args: ["skill", "install", skillSpec, "--dir", skillsDst, "--force"],
args: ["skill", "install", skillSpec, ...agentArgs, "--dir", skillsDst, "--force"],
};
}

Expand Down Expand Up @@ -139,20 +141,24 @@ async function writeSkillSummary(skillDir, skills, installedSkillCount, failures

async function main() {
const skillDir = process.env.GH_AW_SKILL_DIR || "";
const skillInstallAgent = process.env.GH_AW_GH_SKILL_AGENT_NAME || "";
const skills = parseSkillSpecs(process.env.GH_AW_FRONTMATTER_SKILLS || "");
const skillsDst = path.join("/tmp/gh-aw", skillDir);

fs.mkdirSync(skillsDst, { recursive: true });

core.info(`Installing frontmatter skills to ${skillsDst}`);
if (skillInstallAgent) {
core.info(`Installing frontmatter skills for gh skill agent ${skillInstallAgent}`);
}
core.info("Existing skills at destination may be replaced (--force) to ensure pinned refs are up to date");

/** @type {Array<{skill: string; error: string}>} */
const failures = [];

for (const skillSpec of skills) {
core.info(`Installing skill reference: ${skillSpec}`);
const command = buildSkillInstallCommand(skillSpec, skillsDst);
const command = buildSkillInstallCommand(skillSpec, skillsDst, skillInstallAgent);
try {
await exec.exec("gh", command.args);
} catch (err) {
Expand Down
56 changes: 52 additions & 4 deletions actions/setup/js/install_frontmatter_skills.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ describe("install_frontmatter_skills", () => {
beforeEach(() => {
originalEnv = {
GH_AW_SKILL_DIR: process.env.GH_AW_SKILL_DIR,
GH_AW_INFO_ENGINE_ID: process.env.GH_AW_INFO_ENGINE_ID,
GH_AW_GH_SKILL_AGENT_NAME: process.env.GH_AW_GH_SKILL_AGENT_NAME,
GH_AW_FRONTMATTER_SKILLS: process.env.GH_AW_FRONTMATTER_SKILLS,
};
originalCore = global.core;
Expand All @@ -41,6 +43,16 @@ describe("install_frontmatter_skills", () => {
} else {
process.env.GH_AW_SKILL_DIR = originalEnv.GH_AW_SKILL_DIR;
}
if (originalEnv.GH_AW_INFO_ENGINE_ID === undefined) {
delete process.env.GH_AW_INFO_ENGINE_ID;
} else {
process.env.GH_AW_INFO_ENGINE_ID = originalEnv.GH_AW_INFO_ENGINE_ID;
}
if (originalEnv.GH_AW_GH_SKILL_AGENT_NAME === undefined) {
delete process.env.GH_AW_GH_SKILL_AGENT_NAME;
} else {
process.env.GH_AW_GH_SKILL_AGENT_NAME = originalEnv.GH_AW_GH_SKILL_AGENT_NAME;
}
if (originalEnv.GH_AW_FRONTMATTER_SKILLS === undefined) {
delete process.env.GH_AW_FRONTMATTER_SKILLS;
} else {
Expand All @@ -54,7 +66,19 @@ describe("install_frontmatter_skills", () => {
});

it("splits repo-level and path-level skill specs into gh skill install arguments", () => {
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"]);
expect(script.buildSkillInstallCommand("githubnext/skills@abc123", "/tmp/gh-aw/.claude/skills", "claude-code").args).toEqual([
"skill",
"install",
"githubnext/skills",
"--all",
"--pin",
"abc123",
"--agent",
"claude-code",
"--dir",
"/tmp/gh-aw/.claude/skills",
"--force",
]);
expect(script.buildSkillInstallCommand("githubnext/skills/review/security@abc123", "/tmp/gh-aw/.claude/skills").args).toEqual([
"skill",
"install",
Expand All @@ -66,6 +90,19 @@ describe("install_frontmatter_skills", () => {
"/tmp/gh-aw/.claude/skills",
"--force",
]);
expect(script.buildSkillInstallCommand("githubnext/skills/review/security@abc123", "/tmp/gh-aw/.claude/skills", "claude-code").args).toEqual([
"skill",
"install",
"githubnext/skills",
"review/security",
"--pin",
"abc123",
"--agent",
"claude-code",
"--dir",
"/tmp/gh-aw/.claude/skills",
"--force",
]);
});

it("omits --pin when the resolved skill spec is unpinned", () => {
Expand All @@ -74,19 +111,30 @@ describe("install_frontmatter_skills", () => {

it("reads skill specs from the env var and installs them at runtime", async () => {
process.env.GH_AW_SKILL_DIR = ".claude/skills";
process.env.GH_AW_INFO_ENGINE_ID = "claude";
process.env.GH_AW_GH_SKILL_AGENT_NAME = "claude-code";
process.env.GH_AW_FRONTMATTER_SKILLS = ["githubnext/skills@abc123", "githubnext/skills/review/security@def456", "${{ inputs.skill_ref }}"].join("\n");
fs.mkdirSync("/tmp/gh-aw/.claude/skills/example", { recursive: true });
fs.writeFileSync("/tmp/gh-aw/.claude/skills/example/SKILL.md", "# test\n", "utf8");

await script.main();

expect(global.exec.exec).toHaveBeenNthCalledWith(1, "gh", ["skill", "install", "githubnext/skills", "--all", "--pin", "abc123", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
expect(global.exec.exec).toHaveBeenNthCalledWith(2, "gh", ["skill", "install", "githubnext/skills", "review/security", "--pin", "def456", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
expect(global.exec.exec).toHaveBeenNthCalledWith(3, "gh", ["skill", "install", "${{ inputs.skill_ref }}", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
expect(global.exec.exec).toHaveBeenNthCalledWith(1, "gh", ["skill", "install", "githubnext/skills", "--all", "--pin", "abc123", "--agent", "claude-code", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
expect(global.exec.exec).toHaveBeenNthCalledWith(2, "gh", ["skill", "install", "githubnext/skills", "review/security", "--pin", "def456", "--agent", "claude-code", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
expect(global.exec.exec).toHaveBeenNthCalledWith(3, "gh", ["skill", "install", "${{ inputs.skill_ref }}", "--agent", "claude-code", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
expect(global.core.summary.addRaw).toHaveBeenCalledWith(expect.stringContaining("### Frontmatter skills installed"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The main() integration test only exercises the claude engine path. A complementary test for an engine with no mapping (e.g. engineID = "") would confirm that --agent is correctly omitted when the engine is unknown — guarding the backward-compatibility contract explicitly.

💡 Suggested test
it("omits --agent when engine ID has no mapping", async () => {
  process.env.GH_AW_SKILL_DIR = ".claude/skills";
  // No GH_AW_INFO_ENGINE_ID set (or set to an unknown value)
  process.env.GH_AW_FRONTMATTER_SKILLS = "githubnext/skills@abc123";

  await script.main();

  expect(global.exec.exec).toHaveBeenCalledWith("gh", [
    "skill", "install", "githubnext/skills", "--all", "--pin", "abc123",
    "--dir", "/tmp/gh-aw/.claude/skills", "--force",
  ]);
  // Critically: no --agent flag in the call
});

@copilot please address this.

expect(global.core.summary.addRaw).toHaveBeenCalledWith(expect.stringContaining('["githubnext/skills@abc123","githubnext/skills/review/security@def456","${{ inputs.skill_ref }}"]'));
});

it("omits --agent when no gh skill agent name is provided", async () => {
process.env.GH_AW_SKILL_DIR = ".claude/skills";
process.env.GH_AW_FRONTMATTER_SKILLS = "githubnext/skills@abc123";

await script.main();

expect(global.exec.exec).toHaveBeenCalledWith("gh", ["skill", "install", "githubnext/skills", "--all", "--pin", "abc123", "--dir", "/tmp/gh-aw/.claude/skills", "--force"]);
});

it("records failures without throwing when skill install fails", async () => {
process.env.GH_AW_SKILL_DIR = ".claude/skills";
process.env.GH_AW_FRONTMATTER_SKILLS = "bad/repo@abc123";
Expand Down
50 changes: 50 additions & 0 deletions pkg/workflow/activation_skills_step_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ func TestBuildActivationJob_AddsFrontmatterSkillsInstallSteps(t *testing.T) {
assert.Contains(t, steps, "ensure_gh_cli_min_version.sh", "expected gh upgrade step to delegate to ensure_gh_cli_min_version.sh")
assert.Contains(t, steps, "Install frontmatter skill 1", "expected first frontmatter skill install step in activation job")
assert.Contains(t, steps, "Install frontmatter skill 2", "expected second frontmatter skill install step in activation job")
assert.Contains(t, steps, "GH_AW_INFO_ENGINE_ID: \"claude\"", "expected engine id env var for gh skill agent mapping")
assert.Contains(t, steps, "GH_AW_GH_SKILL_AGENT_NAME: \"claude-code\"", "expected gh skill agent env var for claude")
assert.Contains(t, steps, "GH_AW_SKILL_DIR: \".claude/skills\"", "expected engine skill directory env var")
assert.Contains(t, steps, "GH_AW_FRONTMATTER_SKILLS: \"githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de6\"", "expected first skill env var")
assert.Contains(t, steps, "GH_AW_FRONTMATTER_SKILLS: \"githubnext/skills/review/security@1f181b37d3fe5862ab590648f25a292e345b5de6\"", "expected second skill env var")
Expand Down Expand Up @@ -80,11 +82,59 @@ func TestBuildActivationJob_AddsPerSkillAuthSteps(t *testing.T) {

steps := strings.Join(job.Steps, "")
assert.Contains(t, steps, "GH_TOKEN: ${{ secrets.SKILL_PAT }}", "expected first skill install step to use per-skill github-token")
assert.Contains(t, steps, "GH_AW_INFO_ENGINE_ID: \"copilot\"", "expected AI fallback engine id env var for gh skill agent mapping")
assert.Contains(t, steps, "GH_AW_GH_SKILL_AGENT_NAME: \"github-copilot\"", "expected gh skill agent env var for copilot fallback")
assert.Contains(t, steps, "Generate GitHub App token for frontmatter skill 2", "expected app token mint step for second skill")
assert.Contains(t, steps, "id: frontmatter-skill-app-token-2", "expected deterministic app token step id")
assert.Contains(t, steps, "GH_TOKEN: ${{ steps.frontmatter-skill-app-token-2.outputs.token }}", "expected second skill install step to use minted app token")
}

func TestBuildActivationJob_DefaultsSkillInstallEngineToCopilot(t *testing.T) {
compiler := NewCompiler(WithVersion("dev"))
compiler.SetActionMode(ActionModeDev)

data := &WorkflowData{
Name: "skills-workflow",
On: `"on":
workflow_dispatch:`,
Skills: []string{
"githubnext/skills@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, "GH_AW_INFO_ENGINE_ID: \"copilot\"", "expected default engine id env var for frontmatter skill installs")
assert.Contains(t, steps, "GH_AW_GH_SKILL_AGENT_NAME: \"github-copilot\"", "expected default gh skill agent env var for frontmatter skill installs")
assert.Contains(t, steps, "GH_AW_SKILL_DIR: \".github/skills\"", "expected default engine skill directory env var")
}

func TestBuildActivationJob_UsesLegacyAIEngineForSkillArtifactPath(t *testing.T) {
compiler := NewCompiler(WithVersion("dev"))
compiler.SetActionMode(ActionModeDev)

data := &WorkflowData{
Name: "skills-workflow",
On: `"on":
workflow_dispatch:`,
AI: "claude",
Skills: []string{
"githubnext/skills@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, "/tmp/gh-aw/.claude/skills", "expected activation artifact upload to include the legacy ai engine skill directory")
assert.NotContains(t, steps, "/tmp/gh-aw/.github/skills", "expected legacy ai workflows to avoid the default copilot skill directory")
}

func TestBuildActivationJob_AppIgnoreIfMissingFallsBackToActivationToken(t *testing.T) {
compiler := NewCompiler(WithVersion("dev"))
compiler.SetActionMode(ActionModeDev)
Expand Down
9 changes: 9 additions & 0 deletions pkg/workflow/agentic_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ type Engine interface {

// IsExperimental returns true if this engine is experimental
IsExperimental() bool

// GetGHSkillAgentName returns the gh skill --agent value for this engine.
// Returns an empty string when gh skill install does not support the engine.
GetGHSkillAgentName() string
}

// EngineCapabilities captures optional engine features.
Expand Down Expand Up @@ -314,6 +318,7 @@ type BaseEngine struct {
displayName string
description string
experimental bool
ghSkillAgentName string
capabilities EngineCapabilities
dedicatedLLMGatewayPort int
}
Expand All @@ -334,6 +339,10 @@ func (e *BaseEngine) IsExperimental() bool {
return e.experimental
}

func (e *BaseEngine) GetGHSkillAgentName() string {
return e.ghSkillAgentName
}

func (e *BaseEngine) GetCapabilities() EngineCapabilities {
return e.capabilities
}
Expand Down
9 changes: 5 additions & 4 deletions pkg/workflow/antigravity_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ type AntigravityEngine struct {
func NewAntigravityEngine() *AntigravityEngine {
return &AntigravityEngine{
BaseEngine: BaseEngine{
id: "antigravity",
displayName: "Antigravity CLI",
description: "Antigravity CLI with headless mode and LLM gateway support",
experimental: true,
id: "antigravity",
displayName: "Antigravity CLI",
description: "Antigravity CLI with headless mode and LLM gateway support",
experimental: true,
ghSkillAgentName: "antigravity",
capabilities: EngineCapabilities{
ToolsAllowlist: true,
MaxTurns: true,
Expand Down
9 changes: 5 additions & 4 deletions pkg/workflow/claude_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,11 @@ type ClaudeEngine struct {
func NewClaudeEngine() *ClaudeEngine {
return &ClaudeEngine{
BaseEngine: BaseEngine{
id: "claude",
displayName: "Claude Code",
description: "Uses Claude Code with full MCP tool support and allow-listing",
experimental: false,
id: "claude",
displayName: "Claude Code",
description: "Uses Claude Code with full MCP tool support and allow-listing",
experimental: false,
ghSkillAgentName: "claude-code",
capabilities: EngineCapabilities{
ToolsAllowlist: true,
MaxTurns: true, // Claude supports max-turns feature
Expand Down
9 changes: 5 additions & 4 deletions pkg/workflow/codex_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,11 @@ type CodexEngine struct {
func NewCodexEngine() *CodexEngine {
return &CodexEngine{
BaseEngine: BaseEngine{
id: "codex",
displayName: "Codex",
description: "Uses OpenAI Codex CLI with MCP server support",
experimental: false,
id: "codex",
displayName: "Codex",
description: "Uses OpenAI Codex CLI with MCP server support",
experimental: false,
ghSkillAgentName: "codex",
capabilities: EngineCapabilities{
ToolsAllowlist: true,
MaxTurns: true, // AWF max-turns is supported for Codex runs
Expand Down
Loading
Loading