diff --git a/get-shit-done/bin/lib/config.cjs b/get-shit-done/bin/lib/config.cjs index 07da8c4e..0ff016c6 100644 --- a/get-shit-done/bin/lib/config.cjs +++ b/get-shit-done/bin/lib/config.cjs @@ -25,6 +25,7 @@ const VALID_CONFIG_KEYS = new Set([ 'workflow.use_worktrees', 'workflow.code_review', 'workflow.code_review_depth', + 'workflow.code_review_command', 'git.branching_strategy', 'git.base_branch', 'git.phase_branch_template', 'git.milestone_branch_template', 'git.quick_branch_template', 'planning.commit_docs', 'planning.search_gitignored', 'workflow.subagent_timeout', @@ -64,6 +65,7 @@ const CONFIG_KEY_SUGGESTIONS = { 'hooks.research_questions': 'workflow.research_before_questions', 'workflow.research_questions': 'workflow.research_before_questions', 'workflow.codereview': 'workflow.code_review', + 'workflow.review_command': 'workflow.code_review_command', 'workflow.review': 'workflow.code_review', 'workflow.code_review_level': 'workflow.code_review_depth', 'workflow.review_depth': 'workflow.code_review_depth', @@ -154,6 +156,7 @@ function buildNewProjectConfig(userChoices) { skip_discuss: false, code_review: true, code_review_depth: 'standard', + code_review_command: null, }, hooks: { context_warnings: true, diff --git a/get-shit-done/templates/config.json b/get-shit-done/templates/config.json index a3f69929..bb7dcb5f 100644 --- a/get-shit-done/templates/config.json +++ b/get-shit-done/templates/config.json @@ -11,7 +11,8 @@ "security_asvs_level": 1, "security_block_on": "high", "discuss_mode": "discuss", - "research_before_questions": false + "research_before_questions": false, + "code_review_command": null }, "planning": { "commit_docs": true, diff --git a/get-shit-done/workflows/ship.md b/get-shit-done/workflows/ship.md index 9a1c0837..218b3145 100644 --- a/get-shit-done/workflows/ship.md +++ b/get-shit-done/workflows/ship.md @@ -159,6 +159,68 @@ Report: "PR #{number} created: {url}" + +**External code review command (automated sub-step):** + +Before prompting the user, check if an external review command is configured: + +```bash +REVIEW_CMD=$(node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" config-get workflow.code_review_command --default "" 2>/dev/null) +``` + +If `REVIEW_CMD` is non-empty and not `"null"`, run the external review: + +1. **Generate diff and stats:** + ```bash + DIFF=$(git diff ${BASE_BRANCH}...HEAD) + DIFF_STATS=$(git diff --stat ${BASE_BRANCH}...HEAD) + ``` + +2. **Load phase context from STATE.md:** + ```bash + STATE_STATUS=$(node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" state load 2>/dev/null | head -20) + ``` + +3. **Build review prompt and pipe to command via stdin:** + Construct a review prompt containing the diff, diff stats, and phase context, then pipe it to the configured command: + ```bash + REVIEW_PROMPT="You are reviewing a pull request.\n\nDiff stats:\n${DIFF_STATS}\n\nPhase context:\n${STATE_STATUS}\n\nFull diff:\n${DIFF}\n\nRespond with JSON: { \"verdict\": \"APPROVED\" or \"REVISE\", \"confidence\": 0-100, \"summary\": \"...\", \"issues\": [{\"severity\": \"...\", \"file\": \"...\", \"line_range\": \"...\", \"description\": \"...\", \"suggestion\": \"...\"}] }" + REVIEW_OUTPUT=$(echo "${REVIEW_PROMPT}" | timeout 120 ${REVIEW_CMD} 2>/tmp/gsd-review-stderr.log) + REVIEW_EXIT=$? + ``` + +4. **Handle timeout (120s) and failure:** + If `REVIEW_EXIT` is non-zero or the command times out: + ```bash + if [ $REVIEW_EXIT -ne 0 ]; then + REVIEW_STDERR=$(cat /tmp/gsd-review-stderr.log 2>/dev/null) + echo "WARNING: External review command failed (exit ${REVIEW_EXIT}). stderr: ${REVIEW_STDERR}" + echo "Continuing with manual review flow..." + fi + ``` + On failure, warn with stderr output and fall through to the manual review flow below. + +5. **Parse JSON result:** + If the command succeeded, parse the JSON output and report the verdict: + ```bash + # Parse verdict and summary from REVIEW_OUTPUT JSON + VERDICT=$(echo "${REVIEW_OUTPUT}" | node -e " + let d=''; process.stdin.on('data',c=>d+=c); process.stdin.on('end',()=>{ + try { const r=JSON.parse(d); console.log(r.verdict); } + catch(e) { console.log('INVALID_JSON'); } + }); + ") + ``` + - If `verdict` is `"APPROVED"`: report approval with confidence and summary. + - If `verdict` is `"REVISE"`: report issues found, list each issue with severity, file, line_range, description, and suggestion. + - If JSON is invalid (`INVALID_JSON`): warn "External review returned invalid JSON" with stderr and continue. + + Regardless of the external review result, fall through to the manual review options below. + +--- + +**Manual review options:** + Ask if user wants to trigger a code review: diff --git a/tests/code-review-command.test.cjs b/tests/code-review-command.test.cjs new file mode 100644 index 00000000..868d5cc7 --- /dev/null +++ b/tests/code-review-command.test.cjs @@ -0,0 +1,140 @@ +/** + * Tests for code_review_command hook in ship workflow (#1876) + * + * Validates that the external code review command integration is properly + * wired into config, templates, and the ship workflow. + */ + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs'); + +const CONFIG_CJS_PATH = path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'config.cjs'); +const SHIP_MD_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'ship.md'); +const CONFIG_TEMPLATE_PATH = path.join(__dirname, '..', 'get-shit-done', 'templates', 'config.json'); + +describe('code_review_command config key', () => { + test('workflow.code_review_command is in VALID_CONFIG_KEYS', () => { + const { VALID_CONFIG_KEYS } = require(CONFIG_CJS_PATH); + assert.ok( + VALID_CONFIG_KEYS.has('workflow.code_review_command'), + 'workflow.code_review_command must be in VALID_CONFIG_KEYS' + ); + }); + + test('config-set accepts workflow.code_review_command', () => { + const tmpDir = createTempProject(); + try { + // Create config.json first + fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ workflow: {} }, null, 2) + ); + + const result = runGsdTools( + ['config-set', 'workflow.code_review_command', 'my-review-tool --review'], + tmpDir, + { HOME: tmpDir } + ); + assert.ok(result.success, 'config-set should succeed'); + + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.updated, true); + assert.strictEqual(parsed.key, 'workflow.code_review_command'); + assert.strictEqual(parsed.value, 'my-review-tool --review'); + } finally { + cleanup(tmpDir); + } + }); +}); + +describe('config template', () => { + test('config.json template has code_review_command under workflow section', () => { + const template = JSON.parse(fs.readFileSync(CONFIG_TEMPLATE_PATH, 'utf-8')); + assert.ok(template.workflow, 'template must have workflow section'); + assert.ok( + 'code_review_command' in template.workflow, + 'workflow section must contain code_review_command key' + ); + assert.strictEqual( + template.workflow.code_review_command, + null, + 'code_review_command default should be null' + ); + }); +}); + +describe('ship workflow code_review_command integration', () => { + const shipContent = fs.readFileSync(SHIP_MD_PATH, 'utf-8'); + + test('ship.md contains code_review_command config check', () => { + assert.ok( + shipContent.includes('code_review_command'), + 'ship.md must reference code_review_command' + ); + }); + + test('ship.md has external review sub-step that reads config', () => { + assert.ok( + shipContent.includes('config-get') && shipContent.includes('workflow.code_review_command'), + 'ship.md must read workflow.code_review_command from config' + ); + }); + + test('ship.md generates diff against base branch for review', () => { + assert.ok( + shipContent.includes('git diff') && shipContent.includes('BASE_BRANCH'), + 'ship.md must generate a diff using BASE_BRANCH for the external review' + ); + }); + + test('ship.md has JSON parsing for external review output', () => { + assert.ok( + shipContent.includes('verdict') && shipContent.includes('APPROVED'), + 'ship.md must parse JSON output with verdict field' + ); + assert.ok( + shipContent.includes('REVISE'), + 'ship.md must handle REVISE verdict' + ); + }); + + test('ship.md has timeout handling for external review command (120s)', () => { + assert.ok( + shipContent.includes('120') || shipContent.includes('timeout'), + 'ship.md must have timeout handling (120s) for external review command' + ); + }); + + test('ship.md has stderr capture on failure', () => { + assert.ok( + shipContent.includes('stderr'), + 'ship.md must capture stderr on external review command failure' + ); + }); + + test('ship.md pipes review prompt to command via stdin', () => { + assert.ok( + shipContent.includes('stdin'), + 'ship.md must pipe the review prompt to the command via stdin' + ); + }); + + test('ship.md includes diff stats in review prompt', () => { + assert.ok( + shipContent.includes('diff --stat') || shipContent.includes('diffstat') || shipContent.includes('--stat'), + 'ship.md must include diff stats in the review prompt' + ); + }); + + test('ship.md falls through to existing review flow on failure', () => { + // The external review should not block the existing manual review options + assert.ok( + shipContent.includes('AskUserQuestion') || shipContent.includes('Skip review'), + 'ship.md must still offer the existing manual review flow after external review' + ); + }); +});