diff --git a/.changeset/agile-goats-roam.md b/.changeset/agile-goats-roam.md new file mode 100644 index 000000000..dd080012d --- /dev/null +++ b/.changeset/agile-goats-roam.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1005 +--- +**`/gsd-code-review`, `/gsd-code-review --fix`, and `/gsd-eval-review` now inject configured `agent_skills` into their subagents** — these review-family workflows previously spawned their reviewer/fixer/auditor agents (including the `--auto` re-review/re-fix loops) without the project-configured skill and rule context, so any `agent_skills` set for `gsd-code-reviewer`, `gsd-code-fixer`, or `gsd-eval-auditor` were silently ignored. They now query and inject those skills like the ~20 sibling workflows. diff --git a/gsd-core/workflows/code-review-fix.md b/gsd-core/workflows/code-review-fix.md index 581f8d632..d9b4fea18 100644 --- a/gsd-core/workflows/code-review-fix.md +++ b/gsd-core/workflows/code-review-fix.md @@ -21,6 +21,8 @@ _GSD_SHIM_NAME="gsd-tools.cjs"; _GSD_RUNTIME_ROOT="${RUNTIME_DIR:-$(git rev-pars PHASE_ARG="${1}" INIT=$(gsd_run query init.phase-op "${PHASE_ARG}") if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi +AGENT_SKILLS_FIXER=$(gsd_run query agent-skills gsd-code-fixer) +AGENT_SKILLS_REVIEWER=$(gsd_run query agent-skills gsd-code-reviewer) ``` Parse from init JSON: `phase_found`, `phase_dir`, `phase_number`, `phase_name`, `padded_phase`, `commit_docs`. @@ -205,7 +207,7 @@ iteration: 1 Read REVIEW.md findings, apply fixes, commit each atomically, write REVIEW-FIX.md. Do NOT commit REVIEW-FIX.md (orchestrator handles that). -") +${AGENT_SKILLS_FIXER}") ``` > **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. @@ -283,7 +285,7 @@ ${FILES_CONFIG} Re-review the phase at ${REVIEW_DEPTH} depth. Write findings to ${REVIEW_PATH}. Do NOT commit the output — the orchestrator handles that. -") +${AGENT_SKILLS_REVIEWER}") # ORCHESTRATOR RULE — CODEX RUNTIME: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result before proceeding. # Check new REVIEW.md status @@ -322,7 +324,7 @@ iteration: ${ITERATION} Read REVIEW.md findings, apply fixes, commit each atomically, write REVIEW-FIX.md (overwrite previous). Do NOT commit REVIEW-FIX.md. -") +${AGENT_SKILLS_FIXER}") # ORCHESTRATOR RULE — CODEX RUNTIME: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result before proceeding. # Check if fixer succeeded diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index 7910328d6..a3e7bbf62 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -21,6 +21,7 @@ _GSD_SHIM_NAME="gsd-tools.cjs"; _GSD_RUNTIME_ROOT="${RUNTIME_DIR:-$(git rev-pars PHASE_ARG="${1}" INIT=$(gsd_run query init.phase-op "${PHASE_ARG}") if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi +AGENT_SKILLS_REVIEWER=$(gsd_run query agent-skills gsd-code-reviewer) ``` Parse from init JSON: `phase_found`, `phase_dir`, `phase_number`, `phase_name`, `padded_phase`, `commit_docs`. @@ -462,7 +463,7 @@ ${CONFIG_FILES} Review the listed source files at ${REVIEW_DEPTH} depth. Write findings to ${REVIEW_PATH}. Do NOT commit the output — the orchestrator handles that. -") +${AGENT_SKILLS_REVIEWER}") ``` > **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. diff --git a/gsd-core/workflows/eval-review.md b/gsd-core/workflows/eval-review.md index b8379482c..06e451f5a 100644 --- a/gsd-core/workflows/eval-review.md +++ b/gsd-core/workflows/eval-review.md @@ -22,6 +22,7 @@ Parse: `phase_dir`, `phase_number`, `phase_name`, `phase_slug`, `padded_phase`, ```bash AUDITOR_MODEL=$(gsd_run query resolve-model gsd-eval-auditor 2>/dev/null | jq -r '.model' 2>/dev/null || true) +AGENT_SKILLS_AUDITOR=$(gsd_run query agent-skills gsd-eval-auditor) ``` Display banner: @@ -101,6 +102,8 @@ phase_name: {phase_name} padded_phase: {padded_phase} state: {A or B} + +${AGENT_SKILLS_AUDITOR} ``` Spawn as Task with model `AUDITOR_MODEL`. diff --git a/tests/code-review-agent-skills.test.cjs b/tests/code-review-agent-skills.test.cjs new file mode 100644 index 000000000..a6e211c17 --- /dev/null +++ b/tests/code-review-agent-skills.test.cjs @@ -0,0 +1,72 @@ +// allow-test-rule: source-text-is-the-product +// The agent_skills injection for the review-family workflows lives as text in +// the workflow .md files — that text IS what the orchestrating runtime loads +// and executes. There is no intermediate runtime that parses these workflows +// into a prompt we could assert on structurally, so the deployed contract is +// the workflow text itself. +// +// Regression guard for #991: code-review.md / code-review-fix.md / +// eval-review.md were the lone outliers among ~20 workflows that never +// injected the project-configured agent_skills into the subagents they spawn. +// Subagents do not inherit the orchestrator's auto-loaded context, so this +// injection is the ONLY channel for reviewer/fixer/auditor rule context. + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const WORKFLOWS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows'); + +// Workflow file -> EVERY subagent type it spawns and must inject skills for. +// A workflow can spawn more than one agent type: code-review-fix.md spawns the +// fixer (twice) AND re-spawns the reviewer in its --auto loop, so it must +// inject skills for BOTH. Listing every spawned type here is what catches a +// partially-fixed workflow (the gap Codex flagged on the first pass at #991). +const REVIEW_FAMILY = [ + { file: 'code-review.md', agentTypes: ['gsd-code-reviewer'] }, + { file: 'code-review-fix.md', agentTypes: ['gsd-code-fixer', 'gsd-code-reviewer'] }, + { file: 'eval-review.md', agentTypes: ['gsd-eval-auditor'] }, +]; + +function escapeRe(s) { + return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +const cases = REVIEW_FAMILY.flatMap(({ file, agentTypes }) => + agentTypes.map((agentType) => ({ file, agentType })), +); + +describe('agent_skills injection — review-family workflows (#991)', () => { + for (const { file, agentType } of cases) { + test(`${file} queries + injects agent_skills for every ${agentType} spawn`, () => { + const content = fs.readFileSync(path.join(WORKFLOWS_DIR, file), 'utf8'); + + // 1. Must query the project-configured skills for this agent type, using + // the same `gsd_run query agent-skills ` idiom as the ~20 + // sibling workflows (plan-phase, execute-phase, secure-phase, ...). + const assignRe = new RegExp( + '([A-Z][A-Z0-9_]*)=\\$\\(\\s*gsd_run query agent-skills ' + escapeRe(agentType) + '\\s*\\)', + ); + const m = content.match(assignRe); + assert.ok( + m, + `${file}: missing \`VAR=$(gsd_run query agent-skills ${agentType})\` — configured agent_skills are never queried (#991)`, + ); + const varName = m[1]; + + // 2. The queried block must be interpolated into the spawn prompt for + // EVERY spawn of this agent type. code-review-fix.md spawns the fixer + // twice (initial + auto-iteration re-spawn); both must inject, or a + // spawn runs under-equipped. + const interpolations = content.split('${' + varName + '}').length - 1; + const spawnCount = ( + content.match(new RegExp('subagent_type=["\']' + escapeRe(agentType) + '["\']', 'g')) || [] + ).length; + assert.ok( + interpolations >= Math.max(1, spawnCount), + `${file}: \${${varName}} is interpolated ${interpolations}x but ${agentType} is spawned ${spawnCount}x — every spawn must inject the skills block (#991)`, + ); + }); + } +});