diff --git a/.changeset/humble-geese-wave.md b/.changeset/humble-geese-wave.md new file mode 100644 index 000000000..0d0955f47 --- /dev/null +++ b/.changeset/humble-geese-wave.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3743 +--- +**`/gsd-code-review N --fix` now dispatches the fixer** — `--fix`, `--all`, and `--auto` flags were silently dropped at the workflow initialize step (regression of #2946). diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index b4325f09f..234a36aca 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -1,5 +1,5 @@ { - "generated": "2026-05-18", + "generated": "2026-05-21", "families": { "agents": [ "gsd-advisor-researcher", @@ -266,6 +266,7 @@ "cjs-command-router-adapter.cjs", "cjs-sdk-bridge.cjs", "clusters.cjs", + "code-review-flags.cjs", "command-aliases.generated.cjs", "commands.cjs", "config-schema.cjs", diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 4c4075668..03aa29584 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -361,7 +361,7 @@ The `gsd-planner` agent is decomposed into a core agent plus reference modules t --- -## CLI Modules (72 shipped) +## CLI Modules (73 shipped) Full listing: `get-shit-done/bin/lib/*.cjs`. @@ -374,6 +374,7 @@ Full listing: `get-shit-done/bin/lib/*.cjs`. | `cjs-command-router-adapter.cjs` | Shared compatibility adapter for manifest-backed CJS command-family routers | | `cjs-sdk-bridge.cjs` | Shared SDK runtime-bridge loader (`tryLoadSdk`/`getExecuteForCjs`); consumed by every CJS router and `gsd-tools.cjs` to delegate canonical commands to the SDK in-process | | `clusters.cjs` | Skill cluster definitions for the runtime surface module (ADR-0011 Phase 2) | +| `code-review-flags.cjs` | Typed flag parser for `/gsd:code-review`; exports `parseCodeReviewFlags(argv)` (→ `{ fix, all, auto, depth, files }`) and `resolveCodeReviewWorkflow(flags)` (→ `'code-review.md' \| 'code-review-fix.md'`); canonical dispatch seam for `--fix`/`--all`/`--auto` routing | | `command-aliases.generated.cjs` | Generated CJS alias/subcommand metadata for manifest-backed family routers | | `commands.cjs` | Misc CLI commands (slug, timestamp, todos, scaffolding, stats) | | `config-schema.cjs` | Single source of truth for `VALID_CONFIG_KEYS` and dynamic key patterns; imported by both the validator and the config-schema-docs parity test | diff --git a/get-shit-done/bin/lib/code-review-flags.cjs b/get-shit-done/bin/lib/code-review-flags.cjs new file mode 100644 index 000000000..a0bba0167 --- /dev/null +++ b/get-shit-done/bin/lib/code-review-flags.cjs @@ -0,0 +1,74 @@ +'use strict'; + +/** + * Typed flag parser for the /gsd:code-review command. + * + * This is the canonical IR for code-review argument parsing. The workflow + * (code-review.md) delegates flag dispatch logic to this module so that: + * 1. Tests assert on a structured IR rather than on rendered bash text. + * 2. The dispatch decision is testable without instantiating the workflow. + * + * @typedef {Object} CodeReviewFlags + * @property {boolean} fix - true when --fix is present in argv + * @property {boolean} all - true when --all is present in argv (implies fix) + * @property {boolean} auto - true when --auto is present in argv (implies fix) + * @property {string} depth - depth override value, or '' if not supplied + * @property {string} files - files override value, or '' if not supplied + */ + +/** + * Parse code-review flags from an argv array. + * + * The first positional argument (phase number) is ignored by this function — + * phase validation is handled by `gsd-sdk query init.phase-op`. + * + * @param {string[]} argv - Array of argument strings, e.g. ['2', '--fix', '--all'] + * @returns {CodeReviewFlags} + */ +function parseCodeReviewFlags(argv) { + const flags = { + fix: false, + all: false, + auto: false, + depth: '', + files: '', + }; + + for (const arg of argv) { + if (arg === '--fix') { + flags.fix = true; + } else if (arg === '--all') { + flags.all = true; + } else if (arg === '--auto') { + flags.auto = true; + } else if (arg.startsWith('--depth=')) { + flags.depth = arg.slice('--depth='.length); + } else if (arg.startsWith('--files=')) { + flags.files = arg.slice('--files='.length); + } + } + + // --all and --auto imply --fix + if (flags.all || flags.auto) { + flags.fix = true; + } + + return flags; +} + +/** + * Determine which workflow to dispatch based on parsed flags. + * + * Returns the workflow filename (relative to workflows/) that the orchestrator + * should load: + * - 'code-review-fix.md' when fix=true (--fix, --all, or --auto present) + * - 'code-review.md' otherwise (review-only pass) + * + * @param {CodeReviewFlags} flags + * @returns {'code-review.md' | 'code-review-fix.md'} + */ +function resolveCodeReviewWorkflow(flags) { + return flags.fix ? 'code-review-fix.md' : 'code-review.md'; +} + +module.exports = { parseCodeReviewFlags, resolveCodeReviewWorkflow }; diff --git a/get-shit-done/workflows/code-review.md b/get-shit-done/workflows/code-review.md index 0f8aa14c8..010c57b66 100644 --- a/get-shit-done/workflows/code-review.md +++ b/get-shit-done/workflows/code-review.md @@ -1,5 +1,5 @@ -Review source files changed during a phase for bugs, security issues, and code quality problems. Computes file scope (--files override > SUMMARY.md > git diff fallback), checks config gate, spawns gsd-code-reviewer agent, commits REVIEW.md, and presents results to user. +Review source files changed during a phase for bugs, security issues, and code quality problems. Computes file scope (--files override > SUMMARY.md > git diff fallback), checks config gate, spawns gsd-code-reviewer agent, commits REVIEW.md, and presents results to user. When --fix is passed, delegates to code-review-fix.md after review to auto-apply findings via gsd-code-fixer. @@ -8,6 +8,7 @@ Read all files referenced by the invoking prompt's execution_context before star - gsd-code-reviewer: Reviews source files for bugs and quality issues +- gsd-code-fixer: Applies fixes to code review findings (used via dispatch_fix → code-review-fix.md when --fix is passed) @@ -40,26 +41,25 @@ Error: Phase ${PHASE_ARG} not found. Run /gsd:progress to see available phases. This runs BEFORE config gate check so user errors are surfaced immediately regardless of config state. -Parse optional flags from $ARGUMENTS: +Parse optional flags from $ARGUMENTS using the typed flag parser: -**--depth flag:** ```bash -DEPTH_OVERRIDE="" -for arg in "$@"; do - if [[ "$arg" == --depth=* ]]; then - DEPTH_OVERRIDE="${arg#--depth=}" - fi -done -``` +# Parse all code-review flags into a structured IR via code-review-flags.cjs. +# This is the canonical flag-parsing surface — do not replicate inline bash parsing +# for --fix/--all/--auto here; the module handles all flag extraction and implication +# logic (e.g., --all and --auto imply --fix). +FLAGS_JSON=$(node -e " + const { parseCodeReviewFlags } = require('./get-shit-done/bin/lib/code-review-flags.cjs'); + const flags = parseCodeReviewFlags(process.argv.slice(1)); + process.stdout.write(JSON.stringify(flags)); +" -- "$@" 2>/dev/null) -**--files flag:** -```bash -FILES_OVERRIDE="" -for arg in "$@"; do - if [[ "$arg" == --files=* ]]; then - FILES_OVERRIDE="${arg#--files=}" - fi -done +# Extract individual flag values from the IR +FIX_FLAG=$(echo "$FLAGS_JSON" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf-8')).fix))") +FIX_ALL=$(echo "$FLAGS_JSON" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf-8')).all))") +FIX_AUTO=$(echo "$FLAGS_JSON" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf-8')).auto))") +DEPTH_OVERRIDE=$(echo "$FLAGS_JSON" | node -e "process.stdout.write(JSON.parse(require('fs').readFileSync('/dev/stdin','utf-8')).depth)") +FILES_OVERRIDE=$(echo "$FLAGS_JSON" | node -e "process.stdout.write(JSON.parse(require('fs').readFileSync('/dev/stdin','utf-8')).files)") ``` If FILES_OVERRIDE is set, split by comma into array: @@ -508,6 +508,46 @@ fi ``` + +If the `--fix` flag was passed (`FIX_FLAG=true`), delegate to the `code-review-fix.md` workflow +to auto-apply findings from the REVIEW.md that was just written (or that already existed). + +This step runs AFTER `commit_review` so REVIEW.md is guaranteed to be on disk before the fixer +is invoked. If REVIEW.md was not created (agent failed, scope was empty, etc.), the `code-review-fix.md` +workflow handles the missing-review error and exits cleanly. + +```bash +if [ "$FIX_FLAG" = "true" ]; then + echo "" + echo "─────────────────────────────────────────────────────────────────" + echo " --fix: delegating to code-review-fix.md" + echo "─────────────────────────────────────────────────────────────────" + echo "" + + # Build the fix sub-arguments: pass phase arg plus any --all/--auto flags + FIX_ARGS="${PHASE_ARG}" + if [ "$FIX_ALL" = "true" ]; then + FIX_ARGS="${FIX_ARGS} --all" + fi + if [ "$FIX_AUTO" = "true" ]; then + FIX_ARGS="${FIX_ARGS} --auto" + fi + + # Load and execute the code-review-fix workflow. + # The fix workflow is the canonical implementation for all fix logic: + # gsd-code-fixer agent dispatch, --auto iteration loop, REVIEW-FIX.md commit, + # and result presentation. Do not duplicate that logic here. + Workflow(workflow="get-shit-done/workflows/code-review-fix.md", args="${FIX_ARGS}") + + # Exit after fix workflow completes — present_results is for review-only output. + # The fix workflow has its own present_results step. + # Exit workflow. +fi +``` + +If `FIX_FLAG` is false, skip this step entirely and proceed to `present_results`. + + Read the REVIEW.md YAML frontmatter to extract finding counts. @@ -600,6 +640,7 @@ If `--files` validation fails unexpectedly on macOS, install coreutils or use ab - [ ] Phase validated before config gate check - [ ] Config gate checked (workflow.code_review) +- [ ] --fix/--all/--auto flags parsed via code-review-flags.cjs typed IR (not ad-hoc bash) - [ ] Depth resolved with validation (quick|standard|deep) - [ ] File scope computed with 3 tiers: --files > SUMMARY.md > git diff - [ ] Malformed/missing SUMMARY.md handled gracefully with fallback @@ -609,5 +650,6 @@ If `--files` validation fails unexpectedly on macOS, install coreutils or use ab - [ ] Agent spawned with explicit file list, depth, review_path, diff_base - [ ] Agent failure handled without partial commits - [ ] REVIEW.md committed if created -- [ ] Results presented inline with next step suggestion +- [ ] When --fix: dispatch_fix step delegates to code-review-fix.md with --all/--auto forwarded +- [ ] Results presented inline with next step suggestion (review-only path) diff --git a/tests/bug-3727-code-review-fix-flag-dispatch.test.cjs b/tests/bug-3727-code-review-fix-flag-dispatch.test.cjs new file mode 100644 index 000000000..6bc100d27 --- /dev/null +++ b/tests/bug-3727-code-review-fix-flag-dispatch.test.cjs @@ -0,0 +1,164 @@ +// allow-test-rule: source-text-is-the-product +// The workflow .md IS the product: its text is loaded and interpreted at +// runtime by the agent host. Structural assertions (step existence, flag +// references in the initialize step) verify the deployed dispatch contract. +// Pure-function assertions on parseCodeReviewFlags / resolveCodeReviewWorkflow +// are IR-level (not raw-text-match) per CONTRIBUTING.md L554. + +/** + * Regression tests for #3727 — /gsd-code-review N --fix silently no-ops. + * + * Root cause: code-review.md `initialize` step parses only --depth and + * --files flags. --fix, --all, and --auto are never parsed, so no dispatch + * to code-review-fix.md ever occurs. + * + * Fix: add parseCodeReviewFlags() to code-review-flags.cjs (typed IR), wire + * it into the workflow initialize step, and add a dispatch_fix step that + * delegates to code-review-fix.md when flags.fix is true. + */ + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.resolve(__dirname, '..'); +const FLAGS_LIB = path.join(ROOT, 'get-shit-done', 'bin', 'lib', 'code-review-flags.cjs'); +const WORKFLOW_PATH = path.join(ROOT, 'get-shit-done', 'workflows', 'code-review.md'); + +// --------------------------------------------------------------------------- +// Stage 2 seam: typed IR from parseCodeReviewFlags and resolveCodeReviewWorkflow +// These functions are the production code under test — not text-matching on +// the .md source. +// --------------------------------------------------------------------------- + +const { parseCodeReviewFlags, resolveCodeReviewWorkflow } = require(FLAGS_LIB); + +describe('#3727 — parseCodeReviewFlags: typed IR for code-review argv', () => { + test('code-review workflow parses --fix flag and dispatches gsd-code-fixer', () => { + // This is the invariant being violated: --fix must be captured into the IR + // and must route to the fixer workflow, not be silently dropped. + const flags = parseCodeReviewFlags(['2', '--fix']); + assert.strictEqual(flags.fix, true, + '--fix must set flags.fix = true'); + const workflow = resolveCodeReviewWorkflow(flags); + assert.strictEqual(workflow, 'code-review-fix.md', + '--fix must dispatch to code-review-fix.md, not code-review.md'); + }); + + test('--fix absent: dispatch stays on code-review.md (review-only)', () => { + // Counter-test (Stage 5 contract 6): omitting --fix must NOT dispatch fixer. + const flags = parseCodeReviewFlags(['2']); + assert.strictEqual(flags.fix, false, + 'Without --fix, flags.fix must be false'); + const workflow = resolveCodeReviewWorkflow(flags); + assert.strictEqual(workflow, 'code-review.md', + 'Without --fix, dispatch must stay on code-review.md'); + }); + + test('--all implies --fix', () => { + const flags = parseCodeReviewFlags(['3', '--all']); + assert.strictEqual(flags.fix, true, + '--all must imply flags.fix = true'); + assert.strictEqual(flags.all, true, + '--all must set flags.all = true'); + const workflow = resolveCodeReviewWorkflow(flags); + assert.strictEqual(workflow, 'code-review-fix.md', + '--all must dispatch to code-review-fix.md'); + }); + + test('--auto implies --fix', () => { + const flags = parseCodeReviewFlags(['3', '--auto']); + assert.strictEqual(flags.fix, true, + '--auto must imply flags.fix = true'); + assert.strictEqual(flags.auto, true, + '--auto must set flags.auto = true'); + const workflow = resolveCodeReviewWorkflow(flags); + assert.strictEqual(workflow, 'code-review-fix.md', + '--auto must dispatch to code-review-fix.md'); + }); + + test('--fix --all --auto all set simultaneously', () => { + const flags = parseCodeReviewFlags(['1', '--fix', '--all', '--auto']); + assert.strictEqual(flags.fix, true); + assert.strictEqual(flags.all, true); + assert.strictEqual(flags.auto, true); + assert.strictEqual(resolveCodeReviewWorkflow(flags), 'code-review-fix.md'); + }); + + test('--depth flag is still parsed alongside --fix (no regression)', () => { + const flags = parseCodeReviewFlags(['2', '--depth=deep', '--fix']); + assert.strictEqual(flags.fix, true); + assert.strictEqual(flags.depth, 'deep'); + assert.strictEqual(resolveCodeReviewWorkflow(flags), 'code-review-fix.md'); + }); + + test('--files flag is still parsed alongside --fix (no regression)', () => { + const flags = parseCodeReviewFlags(['2', '--files=src/foo.ts,src/bar.ts', '--fix']); + assert.strictEqual(flags.fix, true); + assert.strictEqual(flags.files, 'src/foo.ts,src/bar.ts'); + }); + + test('empty argv returns all-false IR', () => { + const flags = parseCodeReviewFlags([]); + assert.strictEqual(flags.fix, false); + assert.strictEqual(flags.all, false); + assert.strictEqual(flags.auto, false); + assert.strictEqual(flags.depth, ''); + assert.strictEqual(flags.files, ''); + }); +}); + +// --------------------------------------------------------------------------- +// Docs-parity: the workflow .md must contain the dispatch_fix step and +// reference code-review-flags.cjs in the initialize step. +// These are structural invariants on the deployed product text. +// Source-text-is-the-product exemption applies (see file header). +// --------------------------------------------------------------------------- + +describe('#3727 — code-review.md structural dispatch contract', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + + test('initialize step references code-review-flags.cjs for flag parsing', () => { + const initStart = src.indexOf(''); + const initEnd = src.indexOf('', initStart); + assert.ok(initStart !== -1, 'workflow must have an initialize step'); + const initSection = src.slice(initStart, initEnd); + assert.ok( + initSection.includes('code-review-flags.cjs'), + 'initialize step must reference code-review-flags.cjs to parse --fix/--all/--auto flags' + ); + }); + + test('workflow has a step', () => { + assert.ok( + src.includes(''), + 'code-review.md must have a dispatch_fix step (missing = --fix is silently no-op)' + ); + }); + + test('dispatch_fix step delegates to code-review-fix.md when fix flag is true', () => { + const stepStart = src.indexOf(''); + const stepEnd = src.indexOf('', stepStart); + assert.ok(stepStart !== -1, 'dispatch_fix step must exist'); + const stepSection = src.slice(stepStart, stepEnd); + assert.ok( + stepSection.includes('code-review-fix.md'), + 'dispatch_fix step must reference code-review-fix.md workflow' + ); + }); + + test('dispatch_fix step references gsd-code-fixer agent (via code-review-fix.md chain)', () => { + // The dispatch step must show that fixing will occur, either directly or + // by loading code-review-fix.md (which in turn spawns gsd-code-fixer). + const stepStart = src.indexOf(''); + const stepEnd = src.indexOf('', stepStart); + const stepSection = src.slice(stepStart, stepEnd); + assert.ok( + stepSection.includes('gsd-code-fixer') || stepSection.includes('code-review-fix.md'), + 'dispatch_fix step must reference gsd-code-fixer agent or code-review-fix.md workflow' + ); + }); +});