From 4c10eb22536d35abdc47fc68d9869529dc2ac2a8 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 10 Jun 2026 14:07:23 -0400 Subject: [PATCH] fix(#991): inject configured agent_skills into code-review family subagents (#1005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#991): inject configured agent_skills into code-review family subagents code-review.md, code-review-fix.md, and eval-review.md spawned their subagents (gsd-code-reviewer / gsd-code-fixer / gsd-eval-auditor) without querying or injecting the project-configured agent_skills, while ~20 sibling workflows do. Subagents don't inherit the orchestrator's auto-loaded context, so this injection is the only channel — reviewers/fixers/auditors silently ran without the configured rule/skill context. Mirror the established sibling idiom: add `VAR=$(gsd_run query agent-skills )` in each workflow's initialize step and interpolate `${VAR}` into every Agent() spawn of that type. This covers all spawn sites, including code-review-fix.md's --auto loop which re-spawns gsd-code-reviewer in addition to the two gsd-code-fixer spawns. Regression test reads the workflow text (source-text-is-the-product) and asserts each file queries agent-skills for every agent type it spawns and interpolates the result at least once per spawn. Co-Authored-By: Claude Opus 4.8 * chore(#991): add changeset for code-review agent_skills injection fix Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- .changeset/agile-goats-roam.md | 5 ++ gsd-core/workflows/code-review-fix.md | 8 +-- gsd-core/workflows/code-review.md | 3 +- gsd-core/workflows/eval-review.md | 3 ++ tests/code-review-agent-skills.test.cjs | 72 +++++++++++++++++++++++++ 5 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 .changeset/agile-goats-roam.md create mode 100644 tests/code-review-agent-skills.test.cjs 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)`, + ); + }); + } +});