diff --git a/CHANGELOG.md b/CHANGELOG.md index 578629589..00f0a9a5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added +- **Review model configuration** — Per-CLI model selection for /gsd-review via `review.models.` config keys. Falls back to CLI defaults when not set. (#1849) + ## [1.34.2] - 2026-04-06 ### Changed diff --git a/get-shit-done/bin/lib/config.cjs b/get-shit-done/bin/lib/config.cjs index d02945388..ba6bfe155 100644 --- a/get-shit-done/bin/lib/config.cjs +++ b/get-shit-done/bin/lib/config.cjs @@ -47,6 +47,8 @@ function isValidConfigKey(keyPath) { if (VALID_CONFIG_KEYS.has(keyPath)) return true; // Allow agent_skills. with any agent type string if (/^agent_skills\.[a-zA-Z0-9_-]+$/.test(keyPath)) return true; + // Allow review.models. for per-CLI model selection in /gsd-review + if (/^review\.models\.[a-zA-Z0-9_-]+$/.test(keyPath)) return true; // Allow features. — dynamic namespace for feature flags. // Intentionally open-ended so new flags (e.g., features.global_learnings) work // without updating VALID_CONFIG_KEYS each time. @@ -64,6 +66,7 @@ const CONFIG_KEY_SUGGESTIONS = { 'workflow.review': 'workflow.code_review', 'workflow.code_review_level': 'workflow.code_review_depth', 'workflow.review_depth': 'workflow.code_review_depth', + 'review.model': 'review.models.', }; function validateKnownConfigKeyPath(keyPath) { diff --git a/get-shit-done/references/planning-config.md b/get-shit-done/references/planning-config.md index 34db2749e..f84de6be0 100644 --- a/get-shit-done/references/planning-config.md +++ b/get-shit-done/references/planning-config.md @@ -235,6 +235,7 @@ Generated from `CONFIG_DEFAULTS` (core.cjs) and `VALID_CONFIG_KEYS` (config.cjs) | `context_window` | number | `200000` | `200000`, `1000000` | Context window size; set `1000000` for 1M-context models | | `resolve_model_ids` | boolean\|string | `false` | `false`, `true`, `"omit"` | Map model aliases to full Claude IDs; `"omit"` returns empty string | | `context` | string\|null | `null` | `"dev"`, `"research"`, `"review"` | Execution context profile that adjusts agent behavior: `"dev"` for development tasks, `"research"` for investigation/exploration, `"review"` for code review workflows | +| `review.models.` | string\|null | `null` | Any model ID string | Per-CLI model override for /gsd-review (e.g., `review.models.gemini`). Falls back to CLI default when null. | ### Workflow Fields diff --git a/get-shit-done/workflows/review.md b/get-shit-done/workflows/review.md index 734ca1ad4..f726fbbf9 100644 --- a/get-shit-done/workflows/review.md +++ b/get-shit-done/workflows/review.md @@ -139,26 +139,47 @@ Write to a temp file: `/tmp/gsd-review-prompt-{phase}.md` +Read model preferences from planning config. Null/missing values fall back to CLI defaults. + +```bash +GEMINI_MODEL=$(node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" config-get review.models.gemini --raw 2>/dev/null || true) +CLAUDE_MODEL=$(node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" config-get review.models.claude --raw 2>/dev/null || true) +CODEX_MODEL=$(node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" config-get review.models.codex --raw 2>/dev/null || true) +OPENCODE_MODEL=$(node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" config-get review.models.opencode --raw 2>/dev/null || true) +``` + For each selected CLI, invoke in sequence (not parallel — avoid rate limits): **Gemini:** ```bash -gemini -p "$(cat /tmp/gsd-review-prompt-{phase}.md)" 2>/dev/null > /tmp/gsd-review-gemini-{phase}.md +if [ -n "$GEMINI_MODEL" ] && [ "$GEMINI_MODEL" != "null" ]; then + gemini -m "$GEMINI_MODEL" -p "$(cat /tmp/gsd-review-prompt-{phase}.md)" 2>/dev/null > /tmp/gsd-review-gemini-{phase}.md +else + gemini -p "$(cat /tmp/gsd-review-prompt-{phase}.md)" 2>/dev/null > /tmp/gsd-review-gemini-{phase}.md +fi ``` **Claude (separate session):** ```bash -claude -p "$(cat /tmp/gsd-review-prompt-{phase}.md)" 2>/dev/null > /tmp/gsd-review-claude-{phase}.md +if [ -n "$CLAUDE_MODEL" ] && [ "$CLAUDE_MODEL" != "null" ]; then + claude --model "$CLAUDE_MODEL" -p "$(cat /tmp/gsd-review-prompt-{phase}.md)" 2>/dev/null > /tmp/gsd-review-claude-{phase}.md +else + claude -p "$(cat /tmp/gsd-review-prompt-{phase}.md)" 2>/dev/null > /tmp/gsd-review-claude-{phase}.md +fi ``` **Codex:** ```bash -codex exec --skip-git-repo-check "$(cat /tmp/gsd-review-prompt-{phase}.md)" 2>/dev/null > /tmp/gsd-review-codex-{phase}.md +if [ -n "$CODEX_MODEL" ] && [ "$CODEX_MODEL" != "null" ]; then + codex exec --model "$CODEX_MODEL" --skip-git-repo-check "$(cat /tmp/gsd-review-prompt-{phase}.md)" 2>/dev/null > /tmp/gsd-review-codex-{phase}.md +else + codex exec --skip-git-repo-check "$(cat /tmp/gsd-review-prompt-{phase}.md)" 2>/dev/null > /tmp/gsd-review-codex-{phase}.md +fi ``` **CodeRabbit:** -Note: CodeRabbit reviews the current git diff/working tree — it does not accept a prompt. It may take up to 5 minutes. Use `timeout: 360000` on the Bash tool call. +Note: CodeRabbit reviews the current git diff/working tree — it does not accept a prompt or model flag. It may take up to 5 minutes. Use `timeout: 360000` on the Bash tool call. ```bash coderabbit review --prompt-only 2>/dev/null > /tmp/gsd-review-coderabbit-{phase}.md @@ -166,7 +187,11 @@ coderabbit review --prompt-only 2>/dev/null > /tmp/gsd-review-coderabbit-{phase} **OpenCode (via GitHub Copilot):** ```bash -cat /tmp/gsd-review-prompt-{phase}.md | opencode run - 2>/dev/null > /tmp/gsd-review-opencode-{phase}.md +if [ -n "$OPENCODE_MODEL" ] && [ "$OPENCODE_MODEL" != "null" ]; then + cat /tmp/gsd-review-prompt-{phase}.md | opencode run --model "$OPENCODE_MODEL" - 2>/dev/null > /tmp/gsd-review-opencode-{phase}.md +else + cat /tmp/gsd-review-prompt-{phase}.md | opencode run - 2>/dev/null > /tmp/gsd-review-opencode-{phase}.md +fi if [ ! -s /tmp/gsd-review-opencode-{phase}.md ]; then echo "OpenCode review failed or returned empty output." > /tmp/gsd-review-opencode-{phase}.md fi diff --git a/tests/review-model-config.test.cjs b/tests/review-model-config.test.cjs new file mode 100644 index 000000000..a98b42df7 --- /dev/null +++ b/tests/review-model-config.test.cjs @@ -0,0 +1,107 @@ +/** + * Review Model Config Tests (#1849) + * + * Verifies the review.models. dynamic config key pattern: + * - isValidConfigKey accepts review.models. + * - validateKnownConfigKeyPath suggests review.models. for review.model + * - End-to-end round-trip via config-set / config-get for both model IDs and null + */ + +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); + +describe('review.models. config key', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + // Ensure config exists for set/get + runGsdTools('config-ensure-section', tmpDir, { HOME: tmpDir, USERPROFILE: tmpDir }); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('isValidConfigKey accepts review.models.gemini', () => { + // Exercised via config-set, which calls isValidConfigKey internally and + // errors out if the key is not valid. + const result = runGsdTools( + ['config-set', 'review.models.gemini', 'gemini-3.1-pro-preview'], + tmpDir, + { HOME: tmpDir, USERPROFILE: tmpDir } + ); + assert.ok(result.success, `config-set should succeed for review.models.gemini: ${result.error}`); + }); + + test('isValidConfigKey accepts review.models.codex', () => { + const result = runGsdTools( + ['config-set', 'review.models.codex', 'gpt-5-codex'], + tmpDir, + { HOME: tmpDir, USERPROFILE: tmpDir } + ); + assert.ok(result.success, `config-set should succeed for review.models.codex: ${result.error}`); + }); + + test('review.model is rejected and suggests review.models.', () => { + // The suggestion path goes through validateKnownConfigKeyPath, which is + // called before isValidConfigKey in cmdConfigSet. + const result = runGsdTools( + ['config-set', 'review.model', 'gemini-3.1-pro-preview'], + tmpDir, + { HOME: tmpDir, USERPROFILE: tmpDir } + ); + assert.ok(!result.success, 'config-set should fail for review.model'); + assert.ok( + result.error.includes('review.models.'), + `error should suggest review.models., got: ${result.error}` + ); + }); + + test('round-trip: config-set then config-get for a model ID', () => { + const setResult = runGsdTools( + ['config-set', 'review.models.gemini', 'gemini-3.1-pro-preview'], + tmpDir, + { HOME: tmpDir, USERPROFILE: tmpDir } + ); + assert.ok(setResult.success, `config-set failed: ${setResult.error}`); + + const getResult = runGsdTools( + ['config-get', 'review.models.gemini', '--raw'], + tmpDir, + { HOME: tmpDir, USERPROFILE: tmpDir } + ); + assert.ok(getResult.success, `config-get failed: ${getResult.error}`); + assert.strictEqual( + getResult.output, + 'gemini-3.1-pro-preview', + 'config-get should return the value set via config-set' + ); + }); + + test('round-trip: config-set null then config-get returns "null"', () => { + // The issue spec documents null as the "fall back to CLI default" sentinel. + // cmdConfigSet does not parse 'null' as JSON null — it stores the literal + // string 'null'. config-get --raw returns the string 'null', and the + // workflow's `[ "$VAR" != "null" ]` guard handles this. + const setResult = runGsdTools( + ['config-set', 'review.models.gemini', 'null'], + tmpDir, + { HOME: tmpDir, USERPROFILE: tmpDir } + ); + assert.ok(setResult.success, `config-set null failed: ${setResult.error}`); + + const getResult = runGsdTools( + ['config-get', 'review.models.gemini', '--raw'], + tmpDir, + { HOME: tmpDir, USERPROFILE: tmpDir } + ); + assert.ok(getResult.success, `config-get failed: ${getResult.error}`); + assert.strictEqual( + getResult.output, + 'null', + 'config-get should return the literal string "null" so the workflow guard can match it' + ); + }); +});