From 66085d0080bf952d97783daf6650e903282e2dc7 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 17 Jun 2026 10:16:51 -0400 Subject: [PATCH] fix(#1374): surface diagnostic when configured agent skills all fail to resolve (#1376) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1374): surface diagnostic when configured agent skills all fail to resolve buildAgentSkillsBlock returned '' (only ad-hoc per-path stderr warnings) when an agent configured via agent_skills had paths that all failed to resolve — missing SKILL.md, unsafe path, invalid global name, OR a malformed (non-string/non-array) value. query agent-skills --json reported skills_count>0 with an empty block and no machine-readable signal, so a fully-dropped configuration was indistinguishable from a resolved one. Thread an optional diagnostics collector through buildAgentSkillsBlock: route every skip warning through a warn() helper (stderr + collector), flag truthy-but-malformed config values, emit an aggregate warning when configured paths resolve to zero skills, and surface the collected reasons in a new warnings[] field on the query agent-skills --json IR. Empty arrays and falsy values stay silent (skills_count is honestly 0). skills_count semantics unchanged. Docs updated for the new IR field. Co-Authored-By: Claude Opus 4.8 * chore(#1374): backfill changeset PR number (#1376) Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .changeset/wise-zebras-run.md | 5 ++ docs/CLI-TOOLS.md | 4 +- src/init.cts | 48 +++++++++--- tests/agent-skills.test.cjs | 134 ++++++++++++++++++++++++++++++++++ 4 files changed, 177 insertions(+), 14 deletions(-) create mode 100644 .changeset/wise-zebras-run.md diff --git a/.changeset/wise-zebras-run.md b/.changeset/wise-zebras-run.md new file mode 100644 index 000000000..5774f9db0 --- /dev/null +++ b/.changeset/wise-zebras-run.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1376 +--- +**Misconfigured agent skills no longer fail silently** — when an agent's configured `agent_skills` paths all fail to resolve (e.g. a missing `SKILL.md`), `gsd-tools query agent-skills` now emits an aggregate warning to stderr and adds a `warnings[]` field to its `--json` output, instead of returning an empty block with no signal. (#1376) diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 5112666a9..665ace4cb 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -425,11 +425,11 @@ Emit the skill block for a given agent type. # Emit raw XML skill block (default — safe for shell expansion) node gsd-tools.cjs agent-skills -# Emit typed JSON surface (#455) — { agent_type, block, skills_count } +# Emit typed JSON surface (#455) — { agent_type, block, skills_count, warnings } node gsd-tools.cjs agent-skills --json ``` -The `--json` flag returns a typed IR object suitable for structured consumption and test assertions, while the default (no flag) preserves the raw XML output that workflow shell expansions rely on. +The `--json` flag returns a typed IR object suitable for structured consumption and test assertions, while the default (no flag) preserves the raw XML output that workflow shell expansions rely on. The IR also includes a `warnings` array naming any configured skill paths that were skipped (for example a missing `SKILL.md`); it is empty when every configured skill resolved. --- diff --git a/src/init.cts b/src/init.cts index 5f7626c93..4068e5e92 100644 --- a/src/init.cts +++ b/src/init.cts @@ -1876,7 +1876,13 @@ function buildAgentSkillsBlock( config: Record, agentType: string, projectRoot: string, + diagnostics?: { warnings: string[] }, ): string { + const warn = (message: string): void => { + process.stderr.write(message); + if (diagnostics) diagnostics.warnings.push(message.replace(/\n+$/, '')); + }; + const runtime = (config && (config['runtime'] as string)) || 'claude'; const globalSkillsBase = getGlobalSkillsBase(runtime); @@ -1886,7 +1892,13 @@ function buildAgentSkillsBlock( if (!skillPaths) return ''; if (typeof skillPaths === 'string') skillPaths = [skillPaths]; - if (!Array.isArray(skillPaths) || skillPaths.length === 0) return ''; + if (!Array.isArray(skillPaths)) { + warn( + `[agent-skills] WARNING: Agent "${agentType}" has a malformed agent_skills value (expected string or array, got ${typeof skillPaths}) — ignoring\n`, + ); + return ''; + } + if (skillPaths.length === 0) return ''; // Hoist trusted roots computation before the loop: loadTrustedGlobalRoots does // realpathSync I/O and should run at most once per call, not once per failing skill. @@ -1898,12 +1910,15 @@ function buildAgentSkillsBlock( // Skill-tool directive ({ kind: 'directive', name }) for plugin-provided namespaced skills. const validEntries: Array<{ kind: 'include'; ref: string; display: string } | { kind: 'directive'; name: string }> = []; for (const skillPath of skillPaths) { - if (typeof skillPath !== 'string') continue; + if (typeof skillPath !== 'string') { + warn(`[agent-skills] WARNING: Ignoring non-string skill entry (${typeof skillPath}) — skipping\n`); + continue; + } if (skillPath.startsWith('global:')) { const skillName = skillPath.slice(7); if (!skillName) { - process.stderr.write( + warn( `[agent-skills] WARNING: "global:" prefix with empty skill name — skipping\n`, ); continue; @@ -1911,7 +1926,7 @@ function buildAgentSkillsBlock( // Accept: one or more [A-Za-z0-9_-]+ segments joined by single colons. // Rejects: empty segments (::), leading/trailing colon, dots, slashes, backslashes. if (!/^[A-Za-z0-9_-]+(:[A-Za-z0-9_-]+)*$/.test(skillName)) { - process.stderr.write( + warn( `[agent-skills] WARNING: Invalid global skill name "${skillName}" — skipping\n`, ); continue; @@ -1923,7 +1938,7 @@ function buildAgentSkillsBlock( // Emit a natural-language Skill-tool directive (not a @-include). validEntries.push({ kind: 'directive', name: skillName }); } else { - process.stderr.write( + warn( `[agent-skills] WARNING: Plugin-namespaced skill "global:${skillName}" requires a Skill-tool-capable runtime (claude) — skipping on runtime "${runtime}"\n`, ); } @@ -1931,7 +1946,7 @@ function buildAgentSkillsBlock( } // Non-namespaced bare name: attempt filesystem resolution as before. if (globalSkillsBase === null) { - process.stderr.write( + warn( `[agent-skills] WARNING: Runtime "${runtime}" does not use a skills directory — "global:${skillName}" is not supported on this runtime\n`, ); continue; @@ -1940,7 +1955,7 @@ function buildAgentSkillsBlock( const globalSkillMd = path.join(globalSkillDir, 'SKILL.md'); const displayPath = getGlobalSkillDisplayPath(runtime, skillName); if (!fs.existsSync(globalSkillMd)) { - process.stderr.write( + warn( `[agent-skills] WARNING: Global skill not found at "${displayPath}/SKILL.md" — skipping\n`, ); continue; @@ -1952,11 +1967,13 @@ function buildAgentSkillsBlock( return Boolean(rootCheck['safe']); }); if (!acceptedViaTrustedRoot) { - process.stderr.write( + warn( `[agent-skills] WARNING: Global skill "${skillName}" failed path check (symlink escape?) — skipping\n`, ); continue; } + // Intentionally a direct stderr write, NOT warn(): this is an acceptance + // trace, not a skip, so it must not land in the diagnostics warnings[]. process.stderr.write(`[agent-skills] NOTE: Global skill "${skillName}" accepted via trusted_global_roots (resolves outside the default skills dir)\n`); } validEntries.push({ kind: 'include', ref: `${globalSkillDir}/SKILL.md`, display: displayPath }); @@ -1965,7 +1982,7 @@ function buildAgentSkillsBlock( const pathCheck = validatePath(skillPath, projectRoot) as unknown as Record; if (!pathCheck['safe']) { - process.stderr.write( + warn( `[agent-skills] WARNING: Skipping unsafe path "${skillPath}": ${pathCheck['error'] as string}\n`, ); continue; @@ -1973,7 +1990,7 @@ function buildAgentSkillsBlock( const skillMdPath = path.join(projectRoot, skillPath, 'SKILL.md'); if (!fs.existsSync(skillMdPath)) { - process.stderr.write( + warn( `[agent-skills] WARNING: Skill not found at "${skillPath}/SKILL.md" — skipping\n`, ); continue; @@ -1982,7 +1999,12 @@ function buildAgentSkillsBlock( validEntries.push({ kind: 'include', ref: `${skillPath}/SKILL.md`, display: skillPath }); } - if (validEntries.length === 0) return ''; + if (validEntries.length === 0) { + warn( + `[agent-skills] WARNING: Agent "${agentType}" has ${skillPaths.length} configured skill path(s) but none resolved to a valid skill — all were skipped (see warnings above)\n`, + ); + return ''; + } const lines = validEntries.map((entry) => { if (entry.kind === 'directive') { @@ -2005,10 +2027,12 @@ function cmdAgentSkills( } const config = loadConfig(cwd); + const diagnostics = { warnings: [] as string[] }; const block = buildAgentSkillsBlock( config, agentType, cwd, + diagnostics, ); if (jsonMode) { @@ -2019,7 +2043,7 @@ function cmdAgentSkills( : skillPaths ? [skillPaths] : []; - output({ agent_type: agentType, block: block || '', skills_count: normalizedPaths.length }, raw); + output({ agent_type: agentType, block: block || '', skills_count: normalizedPaths.length, warnings: diagnostics.warnings }, raw); return; } diff --git a/tests/agent-skills.test.cjs b/tests/agent-skills.test.cjs index 7aff32d6a..258f83f68 100644 --- a/tests/agent-skills.test.cjs +++ b/tests/agent-skills.test.cjs @@ -234,6 +234,140 @@ describe('agent-skills command', () => { }); }); +// ─── empty-resolution diagnostics (silent-drop visibility) ──────────────────── +// +// When an agent is CONFIGURED with skill paths but every path fails to resolve +// (missing SKILL.md, unsafe path, invalid global name), buildAgentSkillsBlock +// previously returned '' with only per-path stderr warnings and no aggregate +// signal — so `query agent-skills --json` reported skills_count > 0 with an +// empty block and no indication the configured skills were dropped. +// +// Fix: emit an aggregate stderr WARNING when configured paths all resolve to +// zero skills, and surface every skip reason in a `warnings[]` field on the +// --json IR. +describe('agent-skills empty-resolution diagnostics', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('configured agent whose only skill is missing → warnings[] names the path and the aggregate drop', () => { + writeConfig(tmpDir, { + agent_skills: { 'gsd-phase-researcher': ['references/other-skill'] }, + }); + + const r = runAgentSkillsJson(['agent-skills', 'gsd-phase-researcher'], tmpDir); + assert.ok(r.success, `Command failed: ${r.error}`); + assert.strictEqual(r.ir.block, '', 'block must be empty when the only configured skill is missing'); + assert.strictEqual(r.ir.skills_count, 1, 'skills_count still reflects the configured path count'); + assert.ok(Array.isArray(r.ir.warnings), 'IR must include a warnings array'); + assert.ok(r.ir.warnings.length >= 1, `warnings must be non-empty, got: ${JSON.stringify(r.ir.warnings)}`); + assert.ok( + r.ir.warnings.some((w) => w.includes('references/other-skill')), + `warnings must name the skipped path, got: ${JSON.stringify(r.ir.warnings)}`, + ); + assert.ok( + r.ir.warnings.some((w) => /none resolved to a valid skill/.test(w)), + `warnings must include the aggregate empty-resolution diagnostic, got: ${JSON.stringify(r.ir.warnings)}`, + ); + }); + + test('configured agent with all skills missing → aggregate WARNING on stderr naming the agent', () => { + writeConfig(tmpDir, { + agent_skills: { 'gsd-planner': ['references/a', 'references/b'] }, + }); + + const r = runGsdToolsWithStderr(['agent-skills', '--json', 'gsd-planner'], tmpDir, { + HOME: tmpDir, + USERPROFILE: tmpDir, + }); + assert.ok(r.success, `Command failed (exit ${r.exitCode}): ${r.stderr}`); + assert.ok( + r.stderr.includes('[agent-skills] WARNING') && + r.stderr.includes('gsd-planner') && + r.stderr.includes('none resolved to a valid skill'), + `stderr must carry the aggregate empty-resolution warning naming the agent, got: ${r.stderr}`, + ); + const ir = JSON.parse(r.stdout); + assert.strictEqual(ir.block, ''); + assert.ok(ir.warnings.length >= 2, `warnings must list both skipped paths, got: ${JSON.stringify(ir.warnings)}`); + }); + + test('partial resolution: one valid + one missing → block present, NO aggregate warning, skipped path still listed', () => { + const skillDir = path.join(tmpDir, 'skills', 'present'); + fs.mkdirSync(skillDir, { recursive: true }); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), '# present\n'); + + writeConfig(tmpDir, { + agent_skills: { 'gsd-executor': ['skills/present', 'skills/absent'] }, + }); + + const r = runAgentSkillsJson(['agent-skills', 'gsd-executor'], tmpDir); + assert.ok(r.success, `Command failed: ${r.error}`); + assert.ok(r.ir.block.includes('skills/present/SKILL.md'), 'block must include the resolvable skill'); + assert.strictEqual(r.ir.skills_count, 2, 'skills_count reflects both configured paths'); + assert.ok(Array.isArray(r.ir.warnings), 'IR must include a warnings array'); + assert.ok( + r.ir.warnings.some((w) => w.includes('skills/absent')), + `warnings must list the one skipped path, got: ${JSON.stringify(r.ir.warnings)}`, + ); + assert.ok( + !r.ir.warnings.some((w) => /none resolved to a valid skill/.test(w)), + `aggregate empty-resolution warning must NOT fire when at least one skill resolved, got: ${JSON.stringify(r.ir.warnings)}`, + ); + }); + + test('all skills resolve → warnings[] is empty', () => { + const skillDir = path.join(tmpDir, 'skills', 'only'); + fs.mkdirSync(skillDir, { recursive: true }); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), '# only\n'); + + writeConfig(tmpDir, { + agent_skills: { 'gsd-executor': ['skills/only'] }, + }); + + const r = runAgentSkillsJson(['agent-skills', 'gsd-executor'], tmpDir); + assert.ok(r.success, `Command failed: ${r.error}`); + assert.ok(r.ir.block.includes('skills/only/SKILL.md'), 'block must include the resolved skill'); + assert.ok(Array.isArray(r.ir.warnings), 'IR must include a warnings array'); + assert.strictEqual(r.ir.warnings.length, 0, `warnings must be empty when all skills resolve, got: ${JSON.stringify(r.ir.warnings)}`); + }); + + test('unconfigured agent → warnings[] empty (no skills configured is not a drop)', () => { + writeConfig(tmpDir, { + agent_skills: { 'gsd-executor': ['skills/whatever'] }, + }); + + const r = runAgentSkillsJson(['agent-skills', 'gsd-planner'], tmpDir); + assert.ok(r.success, `Command failed: ${r.error}`); + assert.strictEqual(r.ir.block, ''); + assert.ok(Array.isArray(r.ir.warnings), 'IR must include a warnings array'); + assert.strictEqual(r.ir.warnings.length, 0, 'an agent with no configured skills is not a drop — warnings must be empty'); + }); + + test('malformed (non-array, non-string) configured value → flagged in warnings[], not a silent drop', () => { + // A hand-edited config.json could carry a scalar instead of an array. + // cmdAgentSkills still counts it as a configured path, so it must be surfaced. + writeConfig(tmpDir, { + agent_skills: { 'gsd-executor': 42 }, + }); + + const r = runAgentSkillsJson(['agent-skills', 'gsd-executor'], tmpDir); + assert.ok(r.success, `Command failed: ${r.error}`); + assert.strictEqual(r.ir.block, '', 'block must be empty for a malformed value'); + assert.ok(Array.isArray(r.ir.warnings), 'IR must include a warnings array'); + assert.ok( + r.ir.warnings.some((w) => /malformed agent_skills value/.test(w)), + `malformed scalar config must be flagged in warnings[], got: ${JSON.stringify(r.ir.warnings)}`, + ); + }); +}); + // ─── config-ensure-section includes agent_skills ──────────────────────────── describe('config-ensure-section with agent_skills', () => {