feat(review): add per-CLI model selection via config (#1859)

* feat(review): add per-CLI model selection via config - Add review.models.<cli> dynamic config keys to VALID_CONFIG_KEYS - Update review.md to read model preferences via config-get at runtime - Null/missing values fall back to CLI defaults (backward compatible) - Add key suggestion for common typo (review.model) - Update planning-config reference doc Closes #1849 * fix(review): handle absent and null model config gracefully Address PR #1859 review feedback from @trek-e: 1. Add `|| true` to all four config-get subshell invocations in review.md so that an absent review.models.<cli> key does not produce a non-zero exit from the subshell. cmdConfigGet calls error() (process.exit(1)) when the key path is missing; the 2>/dev/null suppresses the message but the exit code was still discarded silently. The || true makes the fall-through explicit and survives future set -e adoption. 2. Add `&& [ "$VAR" != "null" ]` to all four if guards. cmdConfigSet does not parse the literal 'null' as JSON null — it stores the string 'null' — and cmdConfigGet --raw returns the literal text 'null' for that value. Without the extra guard the workflow would pass `-m "null"` to the CLI, which crashes. The issue spec documents null as the "fall back to CLI default" sentinel, so this restores the contract. 3. Add tests/review-model-config.test.cjs covering all five cases trek-e listed: - isValidConfigKey accepts review.models.gemini (via config-set) - isValidConfigKey accepts review.models.codex (via config-set) - review.model is rejected and suggests review.models.<cli-name> - config-set then config-get round-trip with a model ID - config-set then config-get round-trip with null (returns "null") Tests follow the node:test + node:assert/strict pattern from tests/agent-skills.test.cjs and use runGsdTools from helpers.cjs. Closes #1849
2026-04-10 15:44:15 +01:00
parent dcb503961a
commit 7e2217186a
5 changed files with 144 additions and 5 deletions
--- 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.<cli>` config keys. Falls back to CLI defaults when not set. (#1849)
+
 ## [1.34.2] - 2026-04-06

 ### Changed
--- 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.<agent-type> with any agent type string
  if (/^agent_skills\.[a-zA-Z0-9_-]+$/.test(keyPath)) return true;
+  // Allow review.models.<cli-name> for per-CLI model selection in /gsd-review
+  if (/^review\.models\.[a-zA-Z0-9_-]+$/.test(keyPath)) return true;
  // Allow features.<feature_name> — 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.<cli-name>',
 };

 function validateKnownConfigKeyPath(keyPath) {
--- 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.<cli>` | 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

--- 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`
 </step>

 <step name="invoke_reviewers">
+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
--- a/tests/review-model-config.test.cjs
+++ b/tests/review-model-config.test.cjs
@@ -0,0 +1,107 @@
+/**
+ * Review Model Config Tests (#1849)
+ *
+ * Verifies the review.models.<cli> dynamic config key pattern:
+ *   - isValidConfigKey accepts review.models.<cli-name>
+ *   - validateKnownConfigKeyPath suggests review.models.<cli-name> 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.<cli> 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.<cli-name>', () => {
+    // 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.<cli-name>'),
+      `error should suggest review.models.<cli-name>, 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'
+    );
+  });
+});