From a72bebb3791f5c5d3294683223d35182deed9388 Mon Sep 17 00:00:00 2001 From: forfrossen <43250388+forfrossen@users.noreply.github.com> Date: Thu, 23 Apr 2026 18:40:56 +0200 Subject: [PATCH] fix(workflows): agent-skills query keys must match subagent_type (follow-up to #2555) (#2616) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(workflows): agent-skills query keys must match subagent_type Eight workflow files called `gsd-sdk query agent-skills ` with a key that did not match any `subagent_type` Task() spawns in the same workflow (or any existing `agents/.md`): - research-phase.md:45 — gsd-researcher → gsd-phase-researcher - plan-phase.md:36 — gsd-researcher → gsd-phase-researcher - plan-phase.md:38 — gsd-checker → gsd-plan-checker - quick.md:145 — gsd-checker → gsd-plan-checker - verify-work.md:36 — gsd-checker → gsd-plan-checker - new-milestone.md:207 — gsd-synthesizer → gsd-research-synthesizer - new-project.md:63 — gsd-synthesizer → gsd-research-synthesizer - ui-review.md:21 — gsd-ui-reviewer → gsd-ui-auditor - discuss-phase.md:114 — gsd-advisor → gsd-advisor-researcher Effect before this fix: users configuring `agent_skills.` in .planning/config.json got no injection on these paths because the workflow asked the SDK for a different (non-existent) key. The SDK correctly returned "" for the unknown key, which then interpolated as an empty string into the Task() prompt. Silent no-op. The discuss-phase advisor case is a subtle variant — the spawn site uses `subagent_type="general-purpose"` and loads the agent role via `Read(~/.claude/agents/gsd-advisor-researcher.md)`. The injection key must follow the agent identity (gsd-advisor-researcher), not the technical spawn type. This is a follow-up to #2555 — the SDK-side fix in that PR (#2587) only becomes fully effective once the call sites use the right keys. Adds `sdk/src/workflow-agent-skills-consistency.test.ts` as a contract test: every `agent-skills ` invocation in `get-shit-done/workflows/**/*.md` must reference an existing `agents/.md`. Fails loudly on future key typos. Closes #2615 * test: harden workflow agent-skills regex per review feedback Review (#2616): CodeRabbit flagged the `agent-skills ` pattern as too permissive (can match prose mentions of the string) and the per-line scan as brittle (misses commands wrapped across lines). - Require full `gsd-sdk query agent-skills` prefix before capture + `\b` around the pattern so prose references no longer match. - Scan each file's full content (not line-by-line) so `\s+` can span newlines; resolve 1-based line number from match index. - Add JSDoc on helpers and on QUERY_KEY_PATTERN. Verified: RED against base (`f30da83`) produces the same 9 violations as before; GREEN on fixed tree. --------- Co-authored-by: forfrossen --- get-shit-done/workflows/discuss-phase.md | 2 +- get-shit-done/workflows/new-milestone.md | 2 +- get-shit-done/workflows/new-project.md | 2 +- get-shit-done/workflows/plan-phase.md | 4 +- get-shit-done/workflows/quick.md | 2 +- get-shit-done/workflows/research-phase.md | 2 +- get-shit-done/workflows/ui-review.md | 2 +- get-shit-done/workflows/verify-work.md | 2 +- .../workflow-agent-skills-consistency.test.ts | 98 +++++++++++++++++++ 9 files changed, 107 insertions(+), 9 deletions(-) create mode 100644 sdk/src/workflow-agent-skills-consistency.test.ts diff --git a/get-shit-done/workflows/discuss-phase.md b/get-shit-done/workflows/discuss-phase.md index 3304c799..fc7e7bba 100644 --- a/get-shit-done/workflows/discuss-phase.md +++ b/get-shit-done/workflows/discuss-phase.md @@ -111,7 +111,7 @@ Phase number from argument (required). ```bash INIT=$(gsd-sdk query init.phase-op "${PHASE}") if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi -AGENT_SKILLS_ADVISOR=$(gsd-sdk query agent-skills gsd-advisor 2>/dev/null) +AGENT_SKILLS_ADVISOR=$(gsd-sdk query agent-skills gsd-advisor-researcher 2>/dev/null) ``` Parse JSON for: `commit_docs`, `phase_found`, `phase_dir`, `phase_number`, `phase_name`, `phase_slug`, `padded_phase`, `has_research`, `has_context`, `has_plans`, `has_verification`, `plan_count`, `roadmap_exists`, `planning_exists`, `response_language`. diff --git a/get-shit-done/workflows/new-milestone.md b/get-shit-done/workflows/new-milestone.md index 045ce441..cd51afe3 100644 --- a/get-shit-done/workflows/new-milestone.md +++ b/get-shit-done/workflows/new-milestone.md @@ -204,7 +204,7 @@ gsd-sdk query commit "docs: start milestone v[X.Y] [Name]" .planning/PROJECT.md INIT=$(gsd-sdk query init.new-milestone) if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi AGENT_SKILLS_RESEARCHER=$(gsd-sdk query agent-skills gsd-project-researcher 2>/dev/null) -AGENT_SKILLS_SYNTHESIZER=$(gsd-sdk query agent-skills gsd-synthesizer 2>/dev/null) +AGENT_SKILLS_SYNTHESIZER=$(gsd-sdk query agent-skills gsd-research-synthesizer 2>/dev/null) AGENT_SKILLS_ROADMAPPER=$(gsd-sdk query agent-skills gsd-roadmapper 2>/dev/null) ``` diff --git a/get-shit-done/workflows/new-project.md b/get-shit-done/workflows/new-project.md index 3ecf4bda..5b387a48 100644 --- a/get-shit-done/workflows/new-project.md +++ b/get-shit-done/workflows/new-project.md @@ -60,7 +60,7 @@ The document should describe what you want to build. INIT=$(gsd-sdk query init.new-project) if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi AGENT_SKILLS_RESEARCHER=$(gsd-sdk query agent-skills gsd-project-researcher 2>/dev/null) -AGENT_SKILLS_SYNTHESIZER=$(gsd-sdk query agent-skills gsd-synthesizer 2>/dev/null) +AGENT_SKILLS_SYNTHESIZER=$(gsd-sdk query agent-skills gsd-research-synthesizer 2>/dev/null) AGENT_SKILLS_ROADMAPPER=$(gsd-sdk query agent-skills gsd-roadmapper 2>/dev/null) ``` diff --git a/get-shit-done/workflows/plan-phase.md b/get-shit-done/workflows/plan-phase.md index 16a40e3c..9378b266 100644 --- a/get-shit-done/workflows/plan-phase.md +++ b/get-shit-done/workflows/plan-phase.md @@ -33,9 +33,9 @@ Load all context in one call (paths only to minimize orchestrator context): ```bash INIT=$(gsd-sdk query init.plan-phase "$PHASE") if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi -AGENT_SKILLS_RESEARCHER=$(gsd-sdk query agent-skills gsd-researcher 2>/dev/null) +AGENT_SKILLS_RESEARCHER=$(gsd-sdk query agent-skills gsd-phase-researcher 2>/dev/null) AGENT_SKILLS_PLANNER=$(gsd-sdk query agent-skills gsd-planner 2>/dev/null) -AGENT_SKILLS_CHECKER=$(gsd-sdk query agent-skills gsd-checker 2>/dev/null) +AGENT_SKILLS_CHECKER=$(gsd-sdk query agent-skills gsd-plan-checker 2>/dev/null) CONTEXT_WINDOW=$(gsd-sdk query config-get context_window 2>/dev/null || echo "200000") TDD_MODE=$(gsd-sdk query config-get workflow.tdd_mode 2>/dev/null || echo "false") ``` diff --git a/get-shit-done/workflows/quick.md b/get-shit-done/workflows/quick.md index 70918488..1df8961c 100644 --- a/get-shit-done/workflows/quick.md +++ b/get-shit-done/workflows/quick.md @@ -142,7 +142,7 @@ INIT=$(gsd-sdk query init.quick "$DESCRIPTION") if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi AGENT_SKILLS_PLANNER=$(gsd-sdk query agent-skills gsd-planner 2>/dev/null) AGENT_SKILLS_EXECUTOR=$(gsd-sdk query agent-skills gsd-executor 2>/dev/null) -AGENT_SKILLS_CHECKER=$(gsd-sdk query agent-skills gsd-checker 2>/dev/null) +AGENT_SKILLS_CHECKER=$(gsd-sdk query agent-skills gsd-plan-checker 2>/dev/null) AGENT_SKILLS_VERIFIER=$(gsd-sdk query agent-skills gsd-verifier 2>/dev/null) ``` diff --git a/get-shit-done/workflows/research-phase.md b/get-shit-done/workflows/research-phase.md index 7c93dc47..2c254a16 100644 --- a/get-shit-done/workflows/research-phase.md +++ b/get-shit-done/workflows/research-phase.md @@ -42,7 +42,7 @@ If exists: Offer update/view/skip options. INIT=$(gsd-sdk query init.phase-op "${PHASE}") if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi # Extract: phase_dir, padded_phase, phase_number, state_path, requirements_path, context_path -AGENT_SKILLS_RESEARCHER=$(gsd-sdk query agent-skills gsd-researcher 2>/dev/null) +AGENT_SKILLS_RESEARCHER=$(gsd-sdk query agent-skills gsd-phase-researcher 2>/dev/null) ``` ## Step 4: Spawn Researcher diff --git a/get-shit-done/workflows/ui-review.md b/get-shit-done/workflows/ui-review.md index b637666f..2385df56 100644 --- a/get-shit-done/workflows/ui-review.md +++ b/get-shit-done/workflows/ui-review.md @@ -18,7 +18,7 @@ Valid GSD subagent types (use exact names — do not fall back to 'general-purpo ```bash INIT=$(gsd-sdk query init.phase-op "${PHASE_ARG}") if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi -AGENT_SKILLS_UI_REVIEWER=$(gsd-sdk query agent-skills gsd-ui-reviewer 2>/dev/null) +AGENT_SKILLS_UI_REVIEWER=$(gsd-sdk query agent-skills gsd-ui-auditor 2>/dev/null) ``` Parse: `phase_dir`, `phase_number`, `phase_name`, `phase_slug`, `padded_phase`, `commit_docs`. diff --git a/get-shit-done/workflows/verify-work.md b/get-shit-done/workflows/verify-work.md index 8f412406..5b43399d 100644 --- a/get-shit-done/workflows/verify-work.md +++ b/get-shit-done/workflows/verify-work.md @@ -33,7 +33,7 @@ If $ARGUMENTS contains a phase number, load context: INIT=$(gsd-sdk query init.verify-work "${PHASE_ARG}") if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi AGENT_SKILLS_PLANNER=$(gsd-sdk query agent-skills gsd-planner 2>/dev/null) -AGENT_SKILLS_CHECKER=$(gsd-sdk query agent-skills gsd-checker 2>/dev/null) +AGENT_SKILLS_CHECKER=$(gsd-sdk query agent-skills gsd-plan-checker 2>/dev/null) ``` Parse JSON for: `planner_model`, `checker_model`, `commit_docs`, `phase_found`, `phase_dir`, `phase_number`, `phase_name`, `has_verification`, `uat_path`. diff --git a/sdk/src/workflow-agent-skills-consistency.test.ts b/sdk/src/workflow-agent-skills-consistency.test.ts new file mode 100644 index 00000000..7c61a392 --- /dev/null +++ b/sdk/src/workflow-agent-skills-consistency.test.ts @@ -0,0 +1,98 @@ +/** + * Contract test: every `gsd-sdk query agent-skills ` invocation in + * `get-shit-done/workflows/**\/*.md` must reference a slug that exists as + * `agents/.md` at the repository root. + * + * A mismatch produces a silent no-op at runtime — the SDK returns `""` for an + * unknown key, and the workflow interpolates the empty string into the spawn + * prompt, so any `agent_skills.` configuration in + * `.planning/config.json` is silently ignored. This test prevents regression. + * + * Related: https://github.com/gsd-build/get-shit-done/issues/2615 + */ +import { describe, it, expect } from 'vitest'; +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { dirname, join, relative } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const repoRoot = join(__dirname, '..', '..'); +const workflowsDir = join(repoRoot, 'get-shit-done', 'workflows'); +const agentsDir = join(repoRoot, 'agents'); + +/** + * Matches a full `gsd-sdk query agent-skills ` invocation and captures + * the slug. Requires a token boundary before `gsd-sdk` and a word boundary + * after the slug so that prose references (e.g. documentation mentioning the + * string "agent-skills") do not produce false positives. The `\s+` between + * tokens accepts newlines, so commands wrapped across lines still match. + */ +const QUERY_KEY_PATTERN = /\bgsd-sdk\s+query\s+agent-skills\s+([a-z][a-z0-9-]*)\b/g; + +interface QueryUsage { + readonly file: string; + readonly line: number; + readonly slug: string; +} + +/** Recursively collects all `.md` file paths under `dir`. */ +function walkMarkdown(dir: string): string[] { + const out: string[] = []; + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + out.push(...walkMarkdown(full)); + } else if (entry.endsWith('.md')) { + out.push(full); + } + } + return out; +} + +/** Returns the set of agent slugs defined by `.md` files in `dir`. */ +function collectAgentSlugs(dir: string): Set { + return new Set( + readdirSync(dir) + .filter((name) => name.endsWith('.md')) + .map((name) => name.replace(/\.md$/, '')), + ); +} + +/** + * Extracts every `gsd-sdk query agent-skills ` usage from the given + * markdown files. Runs the regex over each file's full content (not line by + * line) so wrapped commands still match, then resolves the 1-based line number + * from the match index. + */ +function collectQueryUsages(files: readonly string[]): QueryUsage[] { + const usages: QueryUsage[] = []; + for (const file of files) { + const content = readFileSync(file, 'utf8'); + for (const match of content.matchAll(QUERY_KEY_PATTERN)) { + const index = match.index ?? 0; + const line = content.slice(0, index).split('\n').length; + usages.push({ file, line, slug: match[1]! }); + } + } + return usages; +} + +describe('workflow agent-skills query consistency', () => { + it('every `agent-skills ` query refers to an existing `agents/.md`', () => { + const validSlugs = collectAgentSlugs(agentsDir); + const workflowFiles = walkMarkdown(workflowsDir); + const usages = collectQueryUsages(workflowFiles); + const invalid = usages.filter((u) => !validSlugs.has(u.slug)); + + const report = invalid + .map((u) => ` ${relative(repoRoot, u.file)}:${u.line} — unknown slug '${u.slug}'`) + .join('\n'); + + expect( + invalid, + invalid.length + ? `Found ${invalid.length} agent-skills query keys with no matching agents/.md:\n${report}` + : '', + ).toHaveLength(0); + }); +});