Files
msd-core/get-shit-done/bin/lib/config.cjs
Bill Huang 99c089bfbf feat: add /gsd:code-review and /gsd:code-review-fix commands (#1630)
* feat: add /gsd:code-review and /gsd:code-review-fix commands

Closes #1636

Add two new slash commands that close the gap between phase execution
and verification. After /gsd:execute-phase completes, /gsd:code-review
reviews produced code for bugs, security issues, and quality problems.
/gsd:code-review-fix then auto-fixes issues found by the review.

## New Files

- agents/gsd-code-reviewer.md — Review agent with 3 depth levels
  (quick/standard/deep) and structured REVIEW.md output
- agents/gsd-code-fixer.md — Fix agent with atomic git rollback,
  3-tier verification, per-finding atomic commits, logic-bug flagging
- commands/gsd/code-review.md — Slash command definition
- commands/gsd/code-review-fix.md — Slash command definition
- get-shit-done/workflows/code-review.md — Review orchestration:
  3-tier file scoping, repo-boundary path validation, config gate
- get-shit-done/workflows/code-review-fix.md — Fix orchestration:
  --all/--auto flags, 3-iteration cap, artifact backup across iterations
- tests/code-review.test.cjs — 35 tests covering agents, commands,
  workflows, config, integration, rollback strategy, and logic-bug flagging

## Modified Files

- get-shit-done/bin/lib/config.cjs — Register workflow.code_review and
  workflow.code_review_depth with defaults and typo suggestions
- get-shit-done/workflows/execute-phase.md — Add code_review_gate step
  (PIPE-01): runs after aggregate_results, advisory only, non-blocking
- get-shit-done/workflows/quick.md — Add Step 6.25 code review (PIPE-03):
  scopes via git diff, uses gsd-code-reviewer, advisory only
- get-shit-done/workflows/autonomous.md — Add Step 3c.5 review+fix chain
  (PIPE-02): auto-chains code-review-fix --auto when issues found

## Design Decisions

- Rollback uses git checkout -- {file} (atomic) not Write tool (partial write risk)
- Logic-bug fixes flagged "requires human verification" (syntax check cannot verify semantics)
- Path traversal guard rejects --files paths outside repo root
- Fail-closed scoping: no HEAD~N heuristics when scope is ambiguous

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat: add /gsd:code-review and /gsd:code-review-fix commands

Closes #1636

Add two new slash commands that close the gap between phase execution
and verification. After /gsd:execute-phase completes, /gsd:code-review
reviews produced code for bugs, security issues, and quality problems.
/gsd:code-review-fix then auto-fixes issues found by the review.

## New Files

- agents/gsd-code-reviewer.md — Review agent: 3 depth levels, REVIEW.md
- agents/gsd-code-fixer.md — Fix agent: git rollback, 3-tier verification,
  logic-bug flagging, per-finding atomic commits
- commands/gsd/code-review.md, code-review-fix.md — Slash command definitions
- get-shit-done/workflows/code-review.md — Review orchestration: 3-tier
  file scoping, path traversal guard, config gate
- get-shit-done/workflows/code-review-fix.md — Fix orchestration:
  --all/--auto flags, 3-iteration cap, artifact backup
- tests/code-review.test.cjs — 35 tests: agents, commands, workflows,
  config, integration, rollback, logic-bug flagging

## Modified Files

- get-shit-done/bin/lib/config.cjs — Register workflow.code_review and
  workflow.code_review_depth config keys
- get-shit-done/workflows/execute-phase.md — Add code_review_gate step
  (PIPE-01): after aggregate_results, advisory, non-blocking
- get-shit-done/workflows/quick.md — Add Step 6.25 code review (PIPE-03):
  git diff scoping, gsd-code-reviewer, advisory
- get-shit-done/workflows/autonomous.md — Add Step 3c.5 review+fix chain
  (PIPE-02): auto-chains code-review-fix --auto when issues found

## Design decisions

- Rollback uses git checkout -- {file} (atomic) not Write tool
- Logic-bug fixes flagged requires human verification (syntax != semantics)
- --files paths validated within repo root (path traversal guard)
- Fail-closed: no HEAD~N heuristics when scope ambiguous

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: resolve contradictory rollback instructions in gsd-code-fixer

rollback_strategy said git checkout, critical_rules said Write tool.
Align all three sections (rollback_strategy, execution_flow step b,
critical_rules) to use git checkout -- {file} consistently.

Also remove in-memory PRE_FIX_CONTENT capture — no longer needed
since git checkout is the rollback mechanism.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address all review feedback from rounds 3-4

Blocking (bash compatibility):
- Replace mapfile -t with portable while IFS= read -r loops in both
  workflows (mapfile is bash 4+; macOS ships bash 3.2 by default)
- Add macOS bash version note to platform_notes

Blocking (quick.md scope heuristic):
- Replace fragile HEAD~$(wc -l SUMMARY.md) with git log --grep based
  diff, matching the more robust approach in code-review.md

Security (path traversal):
- Document realpath -m macOS behavior in platform_notes; guard remains
  fail-closed on macOS without coreutils

Logic / correctness:
- Fix REVIEW_PATH / FIX_REPORT_PATH interpolation in node -e strings;
  use process.env.REVIEW_PATH via env var prefix to avoid single-quote
  path injection risk
- Add iteration semantics comment clarifying off-by-one behavior
- Remove duplicate "3. Determine changed files" heading in gsd-code-reviewer.md

Agent:
- Add logic-bug limitation section to gsd-code-fixer verification_strategy

Tests (39 total, up from 32):
- Add rollback uses git checkout test
- Add success_criteria consistency test (must not say Write tool)
- Add logic-bug flagging test
- Add files_reviewed_list spec test
- Add path traversal guard structural test
- Add mapfile-in-bash-blocks tests (bash 3.2 compatibility)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: add gsd-code-reviewer to quick.md available_agent_types and copilot install test

- quick.md Step 6.25 spawns gsd-code-reviewer but the workflow's
  <available_agent_types> block did not list it, failing the spawn
  consistency CI check (#1357)
- copilot-install.test.cjs hardcoded agent list was missing
  gsd-code-fixer.agent.md and gsd-code-reviewer.agent.md, failing
  the Copilot full install verification test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: replace /gsd: colon refs with /gsd- hyphen format in new files

Fixes stale-colon-refs CI test (#1748). All 19 violations replaced:
- agents/gsd-code-fixer.md (2): description + role spawned-by text
- agents/gsd-code-reviewer.md (4): description + role + fallback note + error msg
- get-shit-done/workflows/code-review-fix.md (7): error msgs + retry suggestions
- get-shit-done/workflows/code-review.md (5): error msgs + retry suggestions
- get-shit-done/workflows/execute-phase.md (1): code_review_gate suggestion

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-05 19:43:45 -04:00

470 lines
16 KiB
JavaScript

/**
* Config — Planning config CRUD operations
*/
const fs = require('fs');
const path = require('path');
const { output, error, planningRoot, CONFIG_DEFAULTS } = require('./core.cjs');
const {
VALID_PROFILES,
getAgentToModelMapForProfile,
formatAgentToModelMapAsTable,
} = require('./model-profiles.cjs');
const VALID_CONFIG_KEYS = new Set([
'mode', 'granularity', 'parallelization', 'commit_docs', 'model_profile',
'search_gitignored', 'brave_search', 'firecrawl', 'exa_search',
'workflow.research', 'workflow.plan_check', 'workflow.verifier',
'workflow.nyquist_validation', 'workflow.ui_phase', 'workflow.ui_safety_gate',
'workflow.auto_advance', 'workflow.node_repair', 'workflow.node_repair_budget',
'workflow.text_mode',
'workflow.research_before_questions',
'workflow.discuss_mode',
'workflow.skip_discuss',
'workflow._auto_chain_active',
'workflow.use_worktrees',
'workflow.code_review',
'workflow.code_review_depth',
'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',
'hooks.context_warnings',
'features.thinking_partner',
'context',
'project_code', 'phase_naming',
'manager.flags.discuss', 'manager.flags.plan', 'manager.flags.execute',
'response_language',
]);
/**
* Check whether a config key path is valid.
* Supports exact matches from VALID_CONFIG_KEYS plus dynamic patterns
* like `agent_skills.<agent-type>` where the sub-key is freeform.
*/
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 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.
if (/^features\.[a-zA-Z0-9_]+$/.test(keyPath)) return true;
return false;
}
const CONFIG_KEY_SUGGESTIONS = {
'workflow.nyquist_validation_enabled': 'workflow.nyquist_validation',
'agents.nyquist_validation_enabled': 'workflow.nyquist_validation',
'nyquist.validation_enabled': 'workflow.nyquist_validation',
'hooks.research_questions': 'workflow.research_before_questions',
'workflow.research_questions': 'workflow.research_before_questions',
'workflow.codereview': 'workflow.code_review',
'workflow.review': 'workflow.code_review',
'workflow.code_review_level': 'workflow.code_review_depth',
'workflow.review_depth': 'workflow.code_review_depth',
};
function validateKnownConfigKeyPath(keyPath) {
const suggested = CONFIG_KEY_SUGGESTIONS[keyPath];
if (suggested) {
error(`Unknown config key: ${keyPath}. Did you mean ${suggested}?`);
}
}
/**
* Build a fully-materialized config object for a new project.
*
* Merges (increasing priority):
* 1. Hardcoded defaults — every key that loadConfig() resolves, plus mode/granularity
* 2. User-level defaults from ~/.gsd/defaults.json (if present)
* 3. userChoices — the settings the user explicitly selected during /gsd-new-project
*
* Uses the canonical `git` namespace for branching keys (consistent with VALID_CONFIG_KEYS
* and the settings workflow). loadConfig() handles both flat and nested formats, so this
* is backward-compatible with existing projects that have flat keys.
*
* Returns a plain object — does NOT write any files.
*/
function buildNewProjectConfig(userChoices) {
const choices = userChoices || {};
const homedir = require('os').homedir();
// Detect API key availability
const braveKeyFile = path.join(homedir, '.gsd', 'brave_api_key');
const hasBraveSearch = !!(process.env.BRAVE_API_KEY || fs.existsSync(braveKeyFile));
const firecrawlKeyFile = path.join(homedir, '.gsd', 'firecrawl_api_key');
const hasFirecrawl = !!(process.env.FIRECRAWL_API_KEY || fs.existsSync(firecrawlKeyFile));
const exaKeyFile = path.join(homedir, '.gsd', 'exa_api_key');
const hasExaSearch = !!(process.env.EXA_API_KEY || fs.existsSync(exaKeyFile));
// Load user-level defaults from ~/.gsd/defaults.json if available
const globalDefaultsPath = path.join(homedir, '.gsd', 'defaults.json');
let userDefaults = {};
try {
if (fs.existsSync(globalDefaultsPath)) {
userDefaults = JSON.parse(fs.readFileSync(globalDefaultsPath, 'utf-8'));
// Migrate deprecated "depth" key to "granularity"
if ('depth' in userDefaults && !('granularity' in userDefaults)) {
const depthToGranularity = { quick: 'coarse', standard: 'standard', comprehensive: 'fine' };
userDefaults.granularity = depthToGranularity[userDefaults.depth] || userDefaults.depth;
delete userDefaults.depth;
try {
fs.writeFileSync(globalDefaultsPath, JSON.stringify(userDefaults, null, 2), 'utf-8');
} catch { /* intentionally empty */ }
}
}
} catch {
// Ignore malformed global defaults
}
const hardcoded = {
model_profile: CONFIG_DEFAULTS.model_profile,
commit_docs: CONFIG_DEFAULTS.commit_docs,
parallelization: CONFIG_DEFAULTS.parallelization,
search_gitignored: CONFIG_DEFAULTS.search_gitignored,
brave_search: hasBraveSearch,
firecrawl: hasFirecrawl,
exa_search: hasExaSearch,
git: {
branching_strategy: CONFIG_DEFAULTS.branching_strategy,
phase_branch_template: CONFIG_DEFAULTS.phase_branch_template,
milestone_branch_template: CONFIG_DEFAULTS.milestone_branch_template,
quick_branch_template: CONFIG_DEFAULTS.quick_branch_template,
},
workflow: {
research: true,
plan_check: true,
verifier: true,
nyquist_validation: true,
auto_advance: false,
node_repair: true,
node_repair_budget: 2,
ui_phase: true,
ui_safety_gate: true,
text_mode: false,
research_before_questions: false,
discuss_mode: 'discuss',
skip_discuss: false,
code_review: true,
code_review_depth: 'standard',
},
hooks: {
context_warnings: true,
},
project_code: null,
phase_naming: 'sequential',
agent_skills: {},
};
// Three-level deep merge: hardcoded <- userDefaults <- choices
return {
...hardcoded,
...userDefaults,
...choices,
git: {
...hardcoded.git,
...(userDefaults.git || {}),
...(choices.git || {}),
},
workflow: {
...hardcoded.workflow,
...(userDefaults.workflow || {}),
...(choices.workflow || {}),
},
hooks: {
...hardcoded.hooks,
...(userDefaults.hooks || {}),
...(choices.hooks || {}),
},
agent_skills: {
...hardcoded.agent_skills,
...(userDefaults.agent_skills || {}),
...(choices.agent_skills || {}),
},
};
}
/**
* Command: create a fully-materialized .planning/config.json for a new project.
*
* Accepts user-chosen settings as a JSON string (the keys the user explicitly
* configured during /gsd-new-project). All remaining keys are filled from
* hardcoded defaults and optional ~/.gsd/defaults.json.
*
* Idempotent: if config.json already exists, returns { created: false }.
*/
function cmdConfigNewProject(cwd, choicesJson, raw) {
const planningBase = planningRoot(cwd);
const configPath = path.join(planningBase, 'config.json');
// Idempotent: don't overwrite existing config
if (fs.existsSync(configPath)) {
output({ created: false, reason: 'already_exists' }, raw, 'exists');
return;
}
// Parse user choices
let userChoices = {};
if (choicesJson && choicesJson.trim() !== '') {
try {
userChoices = JSON.parse(choicesJson);
} catch (err) {
error('Invalid JSON for config-new-project: ' + err.message);
}
}
// Ensure .planning directory exists
try {
if (!fs.existsSync(planningBase)) {
fs.mkdirSync(planningBase, { recursive: true });
}
} catch (err) {
error('Failed to create .planning directory: ' + err.message);
}
const config = buildNewProjectConfig(userChoices);
try {
fs.writeFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8');
output({ created: true, path: '.planning/config.json' }, raw, 'created');
} catch (err) {
error('Failed to write config.json: ' + err.message);
}
}
/**
* Ensures the config file exists (creates it if needed).
*
* Does not call `output()`, so can be used as one step in a command without triggering `exit(0)` in
* the happy path. But note that `error()` will still `exit(1)` out of the process.
*/
function ensureConfigFile(cwd) {
const planningBase = planningRoot(cwd);
const configPath = path.join(planningBase, 'config.json');
// Ensure .planning directory exists
try {
if (!fs.existsSync(planningBase)) {
fs.mkdirSync(planningBase, { recursive: true });
}
} catch (err) {
error('Failed to create .planning directory: ' + err.message);
}
// Check if config already exists
if (fs.existsSync(configPath)) {
return { created: false, reason: 'already_exists' };
}
const config = buildNewProjectConfig({});
try {
fs.writeFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8');
return { created: true, path: '.planning/config.json' };
} catch (err) {
error('Failed to create config.json: ' + err.message);
}
}
/**
* Command to ensure the config file exists (creates it if needed).
*
* Note that this exits the process (via `output()`) even in the happy path; use
* `ensureConfigFile()` directly if you need to avoid this.
*/
function cmdConfigEnsureSection(cwd, raw) {
const ensureConfigFileResult = ensureConfigFile(cwd);
if (ensureConfigFileResult.created) {
output(ensureConfigFileResult, raw, 'created');
} else {
output(ensureConfigFileResult, raw, 'exists');
}
}
/**
* Sets a value in the config file, allowing nested values via dot notation (e.g.,
* "workflow.research").
*
* Does not call `output()`, so can be used as one step in a command without triggering `exit(0)` in
* the happy path. But note that `error()` will still `exit(1)` out of the process.
*/
function setConfigValue(cwd, keyPath, parsedValue) {
const configPath = path.join(planningRoot(cwd), 'config.json');
// Load existing config or start with empty object
let config = {};
try {
if (fs.existsSync(configPath)) {
config = JSON.parse(fs.readFileSync(configPath, 'utf-8'));
}
} catch (err) {
error('Failed to read config.json: ' + err.message);
}
// Set nested value using dot notation (e.g., "workflow.research")
const keys = keyPath.split('.');
let current = config;
for (let i = 0; i < keys.length - 1; i++) {
const key = keys[i];
if (current[key] === undefined || typeof current[key] !== 'object') {
current[key] = {};
}
current = current[key];
}
const previousValue = current[keys[keys.length - 1]]; // Capture previous value before overwriting
current[keys[keys.length - 1]] = parsedValue;
// Write back
try {
fs.writeFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8');
return { updated: true, key: keyPath, value: parsedValue, previousValue };
} catch (err) {
error('Failed to write config.json: ' + err.message);
}
}
/**
* Command to set a value in the config file, allowing nested values via dot notation (e.g.,
* "workflow.research").
*
* Note that this exits the process (via `output()`) even in the happy path; use `setConfigValue()`
* directly if you need to avoid this.
*/
function cmdConfigSet(cwd, keyPath, value, raw) {
if (!keyPath) {
error('Usage: config-set <key.path> <value>');
}
validateKnownConfigKeyPath(keyPath);
if (!isValidConfigKey(keyPath)) {
error(`Unknown config key: "${keyPath}". Valid keys: ${[...VALID_CONFIG_KEYS].sort().join(', ')}, agent_skills.<agent-type>`);
}
// Parse value (handle booleans, numbers, and JSON arrays/objects)
let parsedValue = value;
if (value === 'true') parsedValue = true;
else if (value === 'false') parsedValue = false;
else if (!isNaN(value) && value !== '') parsedValue = Number(value);
else if (typeof value === 'string' && (value.startsWith('[') || value.startsWith('{'))) {
try { parsedValue = JSON.parse(value); } catch { /* keep as string */ }
}
const VALID_CONTEXT_VALUES = ['dev', 'research', 'review'];
if (keyPath === 'context' && !VALID_CONTEXT_VALUES.includes(String(parsedValue))) {
error(`Invalid context value '${value}'. Valid values: ${VALID_CONTEXT_VALUES.join(', ')}`);
}
const setConfigValueResult = setConfigValue(cwd, keyPath, parsedValue);
output(setConfigValueResult, raw, `${keyPath}=${parsedValue}`);
}
function cmdConfigGet(cwd, keyPath, raw) {
const configPath = path.join(planningRoot(cwd), 'config.json');
if (!keyPath) {
error('Usage: config-get <key.path>');
}
let config = {};
try {
if (fs.existsSync(configPath)) {
config = JSON.parse(fs.readFileSync(configPath, 'utf-8'));
} else {
error('No config.json found at ' + configPath);
}
} catch (err) {
if (err.message.startsWith('No config.json')) throw err;
error('Failed to read config.json: ' + err.message);
}
// Traverse dot-notation path (e.g., "workflow.auto_advance")
const keys = keyPath.split('.');
let current = config;
for (const key of keys) {
if (current === undefined || current === null || typeof current !== 'object') {
error(`Key not found: ${keyPath}`);
}
current = current[key];
}
if (current === undefined) {
error(`Key not found: ${keyPath}`);
}
output(current, raw, String(current));
}
/**
* Command to set the model profile in the config file.
*
* Note that this exits the process (via `output()`) even in the happy path.
*/
function cmdConfigSetModelProfile(cwd, profile, raw) {
if (!profile) {
error(`Usage: config-set-model-profile <${VALID_PROFILES.join('|')}>`);
}
const normalizedProfile = profile.toLowerCase().trim();
if (!VALID_PROFILES.includes(normalizedProfile)) {
error(`Invalid profile '${profile}'. Valid profiles: ${VALID_PROFILES.join(', ')}`);
}
// Ensure config exists (create if needed)
ensureConfigFile(cwd);
// Set the model profile in the config
const { previousValue } = setConfigValue(cwd, 'model_profile', normalizedProfile, raw);
const previousProfile = previousValue || 'balanced';
// Build result value / message and return
const agentToModelMap = getAgentToModelMapForProfile(normalizedProfile);
const result = {
updated: true,
profile: normalizedProfile,
previousProfile,
agentToModelMap,
};
const rawValue = getCmdConfigSetModelProfileResultMessage(
normalizedProfile,
previousProfile,
agentToModelMap
);
output(result, raw, rawValue);
}
/**
* Returns the message to display for the result of the `config-set-model-profile` command when
* displaying raw output.
*/
function getCmdConfigSetModelProfileResultMessage(
normalizedProfile,
previousProfile,
agentToModelMap
) {
const agentToModelTable = formatAgentToModelMapAsTable(agentToModelMap);
const didChange = previousProfile !== normalizedProfile;
const paragraphs = didChange
? [
`✓ Model profile set to: ${normalizedProfile} (was: ${previousProfile})`,
'Agents will now use:',
agentToModelTable,
'Next spawned agents will use the new profile.',
]
: [
`✓ Model profile is already set to: ${normalizedProfile}`,
'Agents are using:',
agentToModelTable,
];
return paragraphs.join('\n\n');
}
module.exports = {
VALID_CONFIG_KEYS,
cmdConfigEnsureSection,
cmdConfigSet,
cmdConfigGet,
cmdConfigSetModelProfile,
cmdConfigNewProject,
};