Pass engine-specific agent type to gh skill install#42759
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh skill install
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42759 does not have the 'implementation' label and has only 6 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
| * @param {string} engineID | ||
| * @returns {string} | ||
| */ | ||
| function getSkillInstallAgent(engineID) { |
There was a problem hiding this comment.
Pull request overview
This PR fixes frontmatter skill installation so gh skill install is invoked with an engine-specific --agent flag, allowing the GitHub CLI to select the correct host-specific skill layout. It does this by threading the workflow engine ID into the activation install step and mapping gh-aw engine IDs to gh skill agent identifiers.
Changes:
- Export
GH_AW_INFO_ENGINE_IDduring activation frontmatter skill install steps (with a legacyaifallback). - Add engine→agent mapping in
install_frontmatter_skills.cjsand append--agent <value>togh skill installwhen supported. - Extend Go + JS tests to assert engine env wiring and expected
--agentbehavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_activation_job_builder.go | Threads engine ID (and ai fallback) into activation skill install step env. |
| pkg/workflow/activation_skills_step_test.go | Adds assertions that activation steps include the engine ID env var. |
| actions/setup/js/install_frontmatter_skills.cjs | Maps engine IDs to gh skill --agent values and injects --agent into install commands. |
| actions/setup/js/install_frontmatter_skills.test.cjs | Updates tests to verify --agent is included and mapping logic behaves as expected. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
| if engineID == "" && ctx.data.AI != "" { | ||
| engineID = ctx.data.AI | ||
| } | ||
| skillDir := GetEngineSkillDir(engineID) |
| 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") |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (5 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — commenting with suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
defaultbranch doubles as pass-through and unknown sentinel (install_frontmatter_skills.cjsline 38): known pass-through engines (codex,antigravity,opencode,pi) share the samedefaultarm as genuinely unknown engines. A new engine with a different CLI name could silently pass through and emit the wrong--agentvalue. See inline comment.- Fallback from
AItoengineIDis undocumented (compiler_activation_job_builder.goline 542): the compatibility shim is correct but invisible to the next reader. A one-line comment or ADR entry would anchor the intent. - Test coverage gaps: the mapping test does not exercise case-insensitivity or nil inputs; the
main()integration test does not cover the no-engine path. - Minor duplication: the
(engineID || "").trim().toLowerCase()normalisation appears twice ingetSkillInstallAgent. Extract to a local variable.
Positive Highlights
- Backward compatibility is handled cleanly: unknown engines reliably omit
--agentrather than failing. - Logging added to surface which agent is targeted at install time.
agentArgsis cleanly spliced into all three install command variants with no special-casing.getSkillInstallAgentis small, pure, and independently testable.- Go test assertions are tight and specific, verifying the env var value rather than just its presence.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 55.5 AIC · ⌖ 10.2 AIC · ⊞ 6.6K
Comment /matt to run again
| case "antigravity": | ||
| case "opencode": | ||
| case "pi": | ||
| return (engineID || "").trim().toLowerCase(); |
There was a problem hiding this comment.
[/tdd] The default cases for codex, antigravity, opencode, and pi return engineID.trim().toLowerCase() directly, while copilot, claude, and gemini are explicitly remapped. This is fine for known identifiers, but a new engine added with a different gh-skill CLI name (e.g. openai) would silently pass through and produce an incorrect --agent flag if its CLI name differs from its gh-aw ID.
💡 Suggestion
Consider listing each pass-through case explicitly rather than grouping them in a default that also serves as the "unknown" branch. That way, unknown engines reliably return "" and a new engine that needs remapping is never accidentally emitted:
case "codex":
return "codex";
case "antigravity":
return "antigravity";
case "opencode":
return "opencode";
case "pi":
return "pi";
default:
return "";This also makes the test in install_frontmatter_skills.test.cjs at line 110 (getSkillInstallAgent("crush") === "") a structural guarantee rather than just a happy-path check.
@copilot please address this.
| engineID = ctx.data.EngineConfig.ID | ||
| } | ||
| if engineID == "" && ctx.data.AI != "" { | ||
| engineID = ctx.data.AI |
There was a problem hiding this comment.
[/grill-with-docs] The fallback if engineID == "" && ctx.data.AI != "" silently promotes the AI string field to engine ID without a log or comment. If AI holds a value like "claude-sonnet-4" that differs from a valid getSkillInstallAgent key (e.g. "claude"), the mapping will return "" and the --agent flag will be silently omitted.
💡 Suggestion
Add an inline comment explaining the fallback contract so future maintainers know when this path fires:
// Legacy workflows set AI instead of EngineConfig. Map it to an engine ID
// so getSkillInstallAgent can resolve the --agent flag for gh skill install.
if engineID == "" && ctx.data.AI != "" {
engineID = ctx.data.AI
}Alternatively, document in a CONTEXT.md or ADR that AI is the legacy spelling for EngineConfig.ID so the relationship is captured in the domain model.
@copilot please address this.
| expect(script.getSkillInstallAgent("crush")).toBe(""); | ||
| expect(script.getSkillInstallAgent("")).toBe(""); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] The mapping test covers all supported engines and the known-unsupported "crush" case, but it does not test case-insensitivity or whitespace stripping — both of which are explicitly handled by (engineID || "").trim().toLowerCase() in the implementation.
💡 Suggested additions
it("maps engine IDs case-insensitively and strips whitespace", () => {
expect(script.getSkillInstallAgent("Copilot")).toBe("github-copilot");
expect(script.getSkillInstallAgent(" claude ")).toBe("claude-code");
expect(script.getSkillInstallAgent("GEMINI")).toBe("gemini-cli");
expect(script.getSkillInstallAgent(null)).toBe("");
expect(script.getSkillInstallAgent(undefined)).toBe("");
});This makes the defensive normalisation observable and prevents silent regressions if the normalisation is later removed.
@copilot please address this.
| 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")); |
There was a problem hiding this comment.
[/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.
| * @returns {string} | ||
| */ | ||
| function getSkillInstallAgent(engineID) { | ||
| switch ((engineID || "").trim().toLowerCase()) { |
There was a problem hiding this comment.
[/codebase-design] getSkillInstallAgent duplicates the (engineID || "").trim().toLowerCase() normalisation on line 27 and again on line 38, once for the switch guard and once for pass-through returns. Extracting the normalisation to a local variable keeps the mapping table cleaner and avoids the risk of one copy drifting:
💡 Refactor
function getSkillInstallAgent(engineID) {
const id = (engineID || "").trim().toLowerCase();
switch (id) {
case "copilot":
return "github-copilot";
case "claude":
return "claude-code";
case "gemini":
return "gemini-cli";
case "codex":
case "antigravity":
case "opencode":
case "pi":
return id;
default:
return "";
}
}This is a straightforward simplification with no behaviour change.
@copilot please address this.
There was a problem hiding this comment.
❌ One blocking regression, one non-blocking concern
The core approach is sound — threading GH_AW_INFO_ENGINE_ID through the env and mapping it to gh skill --agent is the right design. One correctness regression needs to be fixed before merging.
### Findings
[HIGH — blocking] addActivationArtifactUploadStep (~line 847) has a second engineID derivation that reads only EngineConfig.ID and was not updated with the AI fallback added in addActivationSkillInstallSteps. For workflows using the legacy ai: field without an engine: block (e.g. ai: claude), skill files get installed to .claude/skills (correctly) but the artifact upload still includes .github/skills (wrong). The installed skills never reach the artifact, so the agent runs without them. Copilot users are unaffected because both GetEngineSkillDir("") and GetEngineSkillDir("copilot") resolve to .github/skills. See inline comment for the fix.
[MEDIUM — non-blocking] codex, antigravity, opencode, and pi are returned as pass-through --agent values with no comment documenting that gh skill install --agent accepts them. If any are not accepted, the install fails silently (warning-only). Should also extract the normalized switch discriminant to a local variable to avoid re-computing it in the return.
🔎 Code quality review by PR Code Quality Reviewer · 110.4 AIC · ⌖ 7.55 AIC · ⊞ 1.6K
Comment /review to run again
| ctx.steps = append(ctx.steps, fmt.Sprintf(" - name: Install frontmatter skill %d\n", i+1)) | ||
| ctx.steps = append(ctx.steps, " env:\n") | ||
| ctx.steps = append(ctx.steps, fmt.Sprintf(" GH_TOKEN: %s\n", tokenExpr)) | ||
| ctx.steps = append(ctx.steps, formatYAMLEnv(" ", "GH_AW_INFO_ENGINE_ID", engineID)) |
There was a problem hiding this comment.
AI fallback applied to install step but not to artifact upload — skills will be missing from the artifact for legacy ai: users.
The fallback at line 541–542 (if engineID == "" && ctx.data.AI != "") now correctly resolves engineID for skill installation. But addActivationArtifactUploadStep (~line 847) derives its own engineID from EngineConfig.ID only — the AI fallback was never added there.
💡 Impact and fix
For a workflow with ai: claude (legacy frontmatter, no engine: block):
- This function resolves
engineID = "claude"→ skills are installed to/tmp/gh-aw/.claude/skills✓ addActivationArtifactUploadStepresolvesengineID = ""→GetEngineSkillDir("")returns.github/skills→ the artifact upload includes/tmp/gh-aw/.github/skillsinstead of.claude/skills✗
The installed skills are never captured in the artifact, so the agent runs without them.
Apply the same fallback in addActivationArtifactUploadStep:
engineID := ""
if ctx.data.EngineConfig != nil {
engineID = ctx.data.EngineConfig.ID
}
if engineID == "" && ctx.data.AI != "" {
engineID = ctx.data.AI
}This affects any non-Copilot engine using the legacy ai: field (ai: claude, ai: codex, ai: gemini, ...) — Copilot is unaffected because GetEngineSkillDir("") and GetEngineSkillDir("copilot") both return .github/skills.
| case "antigravity": | ||
| case "opencode": | ||
| case "pi": | ||
| return (engineID || "").trim().toLowerCase(); |
There was a problem hiding this comment.
Pass-through engine IDs (codex, antigravity, opencode, pi) have no documented contract with gh skill install --agent.
If gh skill install rejects any of these as an unknown --agent value, all skill installs for those engine users will fail. The error is caught as a non-fatal warning, so the breakage is silent from the workflow perspective.
💡 Details and suggested fix
The PR description documents copilot → github-copilot, claude → claude-code, gemini → gemini-cli as explicit remappings, but doesn’t establish that codex, antigravity, opencode, and pi are accepted identically by gh skill. Add a comment pinning this assumption to a source:
// These engine IDs match gh skill agent names directly — see <link to gh skill docs or enum>
case "codex":
case "antigravity":
// ...
return normalized;Also: the switch discriminant already computes (engineID || "").trim().toLowerCase() into the matched value, so return (engineID || "").trim().toLowerCase() re-evaluates it from the raw parameter unnecessarily. Use a local variable:
function getSkillInstallAgent(engineID) {
const normalized = (engineID || "").trim().toLowerCase();
switch (normalized) {
case "copilot": return "github-copilot";
case "claude": return "claude-code";
case "gemini": return "gemini-cli";
case "codex":
case "antigravity":
case "opencode":
case "pi":
return normalized; // reuse, not recompute
default: return "";
}
}Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Review: Pass engine-specific agent type to gh skill install
Overall: looks good — one non-blocking suggestion.
The implementation is clean and correct. The engine→agent mapping, the backward-compat fallback through data.AI, and the GH_AW_INFO_ENGINE_ID env propagation all follow existing patterns in the codebase.
What was checked
| Area | Verdict |
|---|---|
getSkillInstallAgent mapping completeness |
✅ All documented engines handled |
--agent arg ordering in buildSkillInstallCommand |
✅ Placed after --pin (consistent across all 3 path branches) |
Backward-compat fallback (data.AI) |
✅ Mirrors existing GetEngineSkillDir pattern |
Empty-engine guard (no --agent when unset) |
✅ Safe no-op for unrecognised engines |
| Test coverage | ✅ New unit tests cover mapping, per-argument order, and env-var wiring |
| YAML injection safety | ✅ formatYAMLEnv already uses %q, no new vectors |
Non-blocking suggestion
The crush engine is registered and has a skill dir (.crush/skills), but falls through to the default branch — intentionally, and the test confirms it. A one-line comment in getSkillInstallAgent would help future contributors understand this is a deliberate omission, not an oversight. (See inline comment.)
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 87.2 AIC · ⌖ 6.75 AIC · ⊞ 4.9K
| case "pi": | ||
| return (engineID || "").trim().toLowerCase(); | ||
| default: | ||
| return ""; |
There was a problem hiding this comment.
The crush engine is a registered built-in engine (see engine_definition.go) with its own skill dir (.crush/skills), but it silently falls through to the default branch and emits no --agent flag. This is intentional per the test, but future contributors will wonder why crush is absent. A short inline comment would prevent confusion:
default:
// Engines not yet mapped (e.g. "crush") get no --agent flag; gh skill install
// will fall back to its own default layout.
return "";@copilot please address this.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Great work on this one! 🎉 Threading the engine ID through to The PR is well-structured, covers all the engine variants consistently, and the test additions in both This looks ready for review. 🚀
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 2c401e4 and 526bb23. The activation artifact upload now uses the same resolved engine fallback as skill installation, and I added coverage for the legacy |
|
@copilot please run the
|
Frontmatter skill installs were invoking
gh skill installwithout the engine type, so the CLI could not reliably target the correct host-specific skill layout. This change threads the workflow engine ID into the install step and maps it to the correspondinggh skill --agentvalue.Activation step wiring
GH_AW_INFO_ENGINE_IDon frontmatter skill install stepsaifield whenengine.idis not setgh skillinvocationinstall_frontmatter_skills.cjs--agent <value>togh skill installwhen the engine has a supported CLI agent namegh skilluse different identifiers (for examplecopilot→github-copilot,claude→claude-code,gemini→gemini-cli)Coverage updates
--agentflag