diff --git a/BUG_REPORT.md b/BUG_REPORT.md new file mode 100644 index 000000000..9ead3fa06 --- /dev/null +++ b/BUG_REPORT.md @@ -0,0 +1,268 @@ +# Bug Report - Get Shit Done Codebase Review + +**Date:** 2026-01-31 +**Reviewer:** Claude Code Agent +**Scope:** Full codebase review for bugs, logic errors, and edge cases + +--- + +## Critical Bugs (High Priority) + +### 1. Missing error handling in statusline.js for file system operations + +**File:** `hooks/gsd-statusline.js:51-54` +**Severity:** High +**Type:** Runtime error / crash + +**Issue:** +The statusline reads the todos directory without error handling. If there's a permission issue or a race condition where a file gets deleted between `readdirSync` and `statSync`, the statusline will crash. + +**Current code:** +```javascript +if (session && fs.existsSync(todosDir)) { + const files = fs.readdirSync(todosDir) // Can throw on permission errors + .filter(f => f.startsWith(session) && f.includes('-agent-') && f.endsWith('.json')) + .map(f => ({ name: f, mtime: fs.statSync(path.join(todosDir, f)).mtime })) // Can throw if file deleted + .sort((a, b) => b.mtime - a.mtime); +``` + +The try-catch at line 57 only wraps the JSON.parse, not the directory operations. + +**Fix:** +Wrap the entire directory reading block in try-catch: +```javascript +if (session && fs.existsSync(todosDir)) { + try { + const files = fs.readdirSync(todosDir) + .filter(f => f.startsWith(session) && f.includes('-agent-') && f.endsWith('.json')) + .map(f => ({ name: f, mtime: fs.statSync(path.join(todosDir, f)).mtime })) + .sort((a, b) => b.mtime - a.mtime); + + if (files.length > 0) { + try { + const todos = JSON.parse(fs.readFileSync(path.join(todosDir, files[0].name), 'utf8')); + const inProgress = todos.find(t => t.status === 'in_progress'); + if (inProgress) task = inProgress.activeForm || ''; + } catch (e) {} + } + } catch (e) { + // Silently fail - don't break statusline on file system errors + } +} +``` + +--- + +### 2. Fragile JSON parsing in bash workflows + +**Files:** +- `get-shit-done/workflows/execute-phase.md:20` +- `commands/gsd/execute-phase.md:45` +- `get-shit-done/workflows/execute-phase.md:62` +- `agents/gsd-executor.md:47` + +**Severity:** High +**Type:** Logic error / silent failure + +**Issue:** +The workflows use fragile grep/sed patterns to extract JSON values instead of proper JSON parsing. These patterns will fail silently if JSON formatting varies. + +**Examples:** +```bash +# Fragile - fails if JSON is minified or has different spacing +MODEL_PROFILE=$(cat .planning/config.json 2>/dev/null | grep -o '"model_profile"[[:space:]]*:[[:space:]]*"[^"]*"' | grep -o '"[^"]*"$' | tr -d '"' || echo "balanced") + +COMMIT_PLANNING_DOCS=$(cat .planning/config.json 2>/dev/null | grep -o '"commit_docs"[[:space:]]*:[[:space:]]*[^,}]*' | grep -o 'true\|false' || echo "true") + +BRANCHING_STRATEGY=$(cat .planning/config.json 2>/dev/null | grep -o '"branching_strategy"[[:space:]]*:[[:space:]]*"[^"]*"' | sed 's/.*:.*"\([^"]*\)"/\1/' || echo "none") +``` + +**Problems:** +- Fails if JSON is minified (no spaces) +- Fails if values aren't quoted (e.g., `true` vs `"true"`) +- Fails if there are escaped quotes in the value +- Fails if spacing is different than expected + +**Fix:** +Use `jq` for robust JSON parsing: +```bash +# Robust JSON parsing +MODEL_PROFILE=$(jq -r '.model_profile // "balanced"' .planning/config.json 2>/dev/null || echo "balanced") + +COMMIT_PLANNING_DOCS=$(jq -r '.commit_docs // true' .planning/config.json 2>/dev/null | grep -o 'true\|false' || echo "true") + +BRANCHING_STRATEGY=$(jq -r '.branching_strategy // "none"' .planning/config.json 2>/dev/null || echo "none") + +PHASE_BRANCH_TEMPLATE=$(jq -r '.phase_branch_template // "gsd/phase-{phase}-{slug}"' .planning/config.json 2>/dev/null || echo "gsd/phase-{phase}-{slug}") +``` + +**Impact:** +Without this fix, configuration settings may silently fall back to defaults even when explicitly configured, leading to unexpected behavior. + +--- + +### 3. Violation of stated git commit rules + +**File:** `commands/gsd/execute-phase.md:94` +**Severity:** Medium +**Type:** Inconsistency with documented rules + +**Issue:** +The workflow uses `git add -u` which violates the explicitly stated rule "NEVER use git add . or git add -A or git add src/". + +**Current code:** +```bash +git add -u && git commit -m "fix({phase}): orchestrator corrections" +``` + +**Fix:** +Either: +1. Remove this step if orchestrator corrections shouldn't happen +2. Explicitly enumerate the files to stage: +```bash +# List modified files and stage individually +git status --porcelain | grep '^ M' | cut -c4- | while read file; do + git add "$file" +done +git commit -m "fix({phase}): orchestrator corrections" +``` + +Or better yet, avoid making corrections at the orchestrator level. + +--- + +## Medium Priority Issues + +### 4. Missing hex color validation in install.js + +**File:** `bin/install.js:437-441` +**Severity:** Medium +**Type:** Data validation + +**Issue:** +The code accepts hex color values without validation: + +```javascript +} else if (colorValue.startsWith('#')) { + // Already hex, keep as is + newLines.push(line); +} +``` + +**Fix:** +Add validation for hex color format: +```javascript +} else if (colorValue.startsWith('#')) { + // Validate hex color format (#RGB or #RRGGBB) + if (/^#[0-9A-Fa-f]{3}$|^#[0-9A-Fa-f]{6}$/.test(colorValue)) { + newLines.push(line); + } + // Skip invalid hex colors +} +``` + +--- + +### 5. Potential issue with branch variable expansion + +**File:** `get-shit-done/workflows/execute-phase.md:100-103` +**Severity:** Low +**Type:** Shell safety + +**Issue:** +Phase name is used in shell variable without proper quoting in some places: + +```bash +PHASE_NAME=$(basename "$PHASE_DIR" | sed 's/^[0-9]*-//') +``` + +The variable is properly quoted, but the subsequent sed operations should also be reviewed for edge cases with special characters in phase names. + +**Fix:** +Ensure all variable expansions are properly quoted, especially in sed operations. + +--- + +### 6. Missing CONTEXT.md reference documentation + +**File:** `agents/gsd-executor.md:69` +**Severity:** Low +**Type:** Documentation gap + +**Issue:** +The executor mentions that plans can reference CONTEXT.md but doesn't explain how it should be passed or read. + +**Current text:** +```markdown +**If plan references CONTEXT.md:** The CONTEXT.md file provides the user's vision for this phase — how they imagine it working, what's essential, and what's out of scope. Honor this context throughout execution. +``` + +**Fix:** +Add clarity about how CONTEXT.md is accessed: +```markdown +**If plan references CONTEXT.md:** Read .planning/phases/{phase}/CONTEXT.md for the user's vision. Honor this context throughout execution. The file provides how they imagine it working, what's essential, and what's out of scope. +``` + +--- + +## Low Priority / Code Quality Issues + +### 7. Inconsistent error handling patterns + +**Files:** Multiple +**Severity:** Low +**Type:** Code quality + +**Issue:** +Error handling is inconsistent across different files: +- Some functions have comprehensive try-catch blocks +- Others rely on optional chaining or existence checks +- Some fail silently, others propagate errors + +**Recommendation:** +Establish consistent error handling patterns across the codebase, especially for: +- File system operations +- JSON parsing +- Git operations +- External command execution + +--- + +### 8. Hardcoded paths in multiple locations + +**Files:** Multiple +**Severity:** Low +**Type:** Maintainability + +**Issue:** +Paths like `~/.claude/`, `.planning/`, etc. are hardcoded in many places. Changes to directory structure would require updates in multiple files. + +**Examples:** +- `hooks/gsd-statusline.js:49` - hardcoded `~/.claude/todos` +- `hooks/gsd-check-update.js:12` - hardcoded `~/.claude/` +- Multiple workflow files reference `.planning/` + +**Recommendation:** +Consider centralizing path constants in a shared configuration module. + +--- + +## Summary + +**Total bugs found:** 8 + +**By severity:** +- Critical: 3 (statusline error handling, JSON parsing, git rules violation) +- Medium: 3 (color validation, variable expansion, documentation) +- Low: 2 (error handling patterns, hardcoded paths) + +**Recommended immediate actions:** +1. Fix statusline.js error handling (prevents crashes) +2. Replace grep/sed JSON parsing with jq (prevents silent configuration failures) +3. Fix or document the git add -u usage (consistency with stated rules) + +**Next steps:** +- Prioritize fixes based on user impact +- Add unit tests for critical paths (especially JSON parsing and file operations) +- Consider adding integration tests for workflow orchestration +- Establish error handling and coding standards documentation diff --git a/FIXES_APPLIED.md b/FIXES_APPLIED.md new file mode 100644 index 000000000..49959df46 --- /dev/null +++ b/FIXES_APPLIED.md @@ -0,0 +1,198 @@ +# Fixes Applied - Bug Report Follow-up + +**Date:** 2026-01-31 +**Related:** BUG_REPORT.md + +--- + +## Critical Bugs Fixed + +### 1. ✅ Fixed: Missing error handling in statusline.js + +**File:** `hooks/gsd-statusline.js` +**Change:** Wrapped directory reading operations in try-catch block + +**Before:** +```javascript +if (session && fs.existsSync(todosDir)) { + const files = fs.readdirSync(todosDir) // Could crash + .filter(...) + .map(f => ({ name: f, mtime: fs.statSync(...).mtime })) // Could crash +``` + +**After:** +```javascript +if (session && fs.existsSync(todosDir)) { + try { + const files = fs.readdirSync(todosDir) + .filter(...) + .map(f => ({ name: f, mtime: fs.statSync(...).mtime })) + // ... rest of logic + } catch (e) { + // Silently fail on file system errors - don't break statusline + } +} +``` + +**Impact:** Prevents statusline crashes from file system permission issues or race conditions. + +--- + +### 2. ✅ Fixed: Hex color validation in install.js + +**File:** `bin/install.js` +**Change:** Added validation for hex color format + +**Before:** +```javascript +} else if (colorValue.startsWith('#')) { + // Already hex, keep as is + newLines.push(line); +} +``` + +**After:** +```javascript +} else if (colorValue.startsWith('#')) { + // Validate hex color format (#RGB or #RRGGBB) + if (/^#[0-9a-f]{3}$|^#[0-9a-f]{6}$/i.test(colorValue)) { + // Already hex and valid, keep as is + newLines.push(line); + } + // Skip invalid hex colors +} +``` + +**Impact:** Prevents invalid hex color values from being written to config files. + +--- + +### 3. ✅ Fixed: Git add rules violation in execute-phase.md + +**File:** `commands/gsd/execute-phase.md` +**Change:** Replaced `git add -u` with individual file staging + +**Before:** +```bash +git add -u && git commit -m "fix({phase}): orchestrator corrections" +``` + +**After:** +```bash +# Stage each modified file individually (never use git add -u, git add ., or git add -A) +git status --porcelain | grep '^ M' | cut -c4- | while read file; do + git add "$file" +done +git commit -m "fix({phase}): orchestrator corrections" +``` + +**Impact:** Maintains consistency with documented git commit rules and prevents accidental staging of unwanted files. + +--- + +## Known Issues Remaining + +### Fragile JSON Parsing (High Priority - Not Fixed) + +**Status:** ⚠️ Documented but not fixed +**Reason:** Requires more extensive refactoring to use `jq` or alternative JSON parser +**Location:** Multiple workflow files + +**Files affected:** +- `get-shit-done/workflows/execute-phase.md:20, 62, 76-77` +- `commands/gsd/execute-phase.md:45, 100` +- `agents/gsd-executor.md:47` + +**Current approach:** +```bash +MODEL_PROFILE=$(cat .planning/config.json 2>/dev/null | grep -o '"model_profile"[[:space:]]*:[[:space:]]*"[^"]*"' | grep -o '"[^"]*"$' | tr -d '"' || echo "balanced") +``` + +**Recommended fix:** +```bash +MODEL_PROFILE=$(jq -r '.model_profile // "balanced"' .planning/config.json 2>/dev/null || echo "balanced") +``` + +**Workaround for users:** +- Ensure `.planning/config.json` is properly formatted with consistent spacing +- Always quote string values in JSON +- Avoid special characters in configuration values + +**Next steps:** +- Evaluate if `jq` can be a required dependency +- Or create a Node.js helper script for JSON parsing that workflows can call +- Or document the JSON formatting requirements clearly for users + +--- + +## Testing Recommendations + +### 1. Statusline Error Handling +**Test case:** Delete files while statusline is reading them +```bash +# Terminal 1: Watch statusline +while true; do node hooks/gsd-statusline.js; sleep 1; done + +# Terminal 2: Create and delete files rapidly +mkdir -p ~/.claude/todos +while true; do + touch ~/.claude/todos/test-file.json + sleep 0.1 + rm ~/.claude/todos/test-file.json + sleep 0.1 +done +``` + +**Expected:** Statusline continues working without crashes + +### 2. Color Validation +**Test case:** Invalid hex colors in frontmatter +```markdown +--- +color: #ZZZ +--- +``` +**Expected:** Invalid color is skipped during installation + +### 3. Git Operations +**Test case:** Verify individual file staging +```bash +# Create some changes +touch file1.txt file2.txt +git add file1.txt file2.txt +git commit -m "test files" + +echo "change" > file1.txt +echo "change" > file2.txt + +# Run the orchestrator commit logic +# Should stage each file individually +``` + +**Expected:** Files are staged one at a time, not in bulk + +--- + +## Future Improvements + +1. **Centralize JSON parsing**: Create a helper utility for all JSON config reading +2. **Add unit tests**: Test critical paths like statusline, installer, config parsing +3. **Establish error handling patterns**: Document and enforce consistent error handling +4. **Path constants**: Centralize hardcoded paths in a configuration module +5. **Integration tests**: Test full workflow orchestration end-to-end + +--- + +## Changelog Entry + +```markdown +## [Unreleased] + +### Fixed +- **hooks/gsd-statusline.js**: Added error handling for file system operations to prevent crashes +- **bin/install.js**: Added validation for hex color values to prevent invalid config +- **commands/gsd/execute-phase.md**: Fixed git staging to use individual files instead of git add -u + +### Known Issues +- JSON config parsing uses fragile grep/sed patterns - will be addressed in future release +``` diff --git a/bin/install.js b/bin/install.js index 3b1fe7d8d..d741c4653 100755 --- a/bin/install.js +++ b/bin/install.js @@ -500,8 +500,12 @@ function convertClaudeToOpencodeFrontmatter(content) { if (hexColor) { newLines.push(`color: "${hexColor}"`); } else if (colorValue.startsWith('#')) { - // Already hex, keep as is - newLines.push(line); + // Validate hex color format (#RGB or #RRGGBB) + if (/^#[0-9a-f]{3}$|^#[0-9a-f]{6}$/i.test(colorValue)) { + // Already hex and valid, keep as is + newLines.push(line); + } + // Skip invalid hex colors } // Skip unknown color names continue; diff --git a/commands/gsd/execute-phase.md b/commands/gsd/execute-phase.md index 21350d26b..481e3b38a 100644 --- a/commands/gsd/execute-phase.md +++ b/commands/gsd/execute-phase.md @@ -89,9 +89,13 @@ Phase: $ARGUMENTS git status --porcelain ``` - **If changes exist:** Orchestrator made corrections between executor completions. Commit them: + **If changes exist:** Orchestrator made corrections between executor completions. Stage and commit them individually: ```bash - git add -u && git commit -m "fix({phase}): orchestrator corrections" + # Stage each modified file individually (never use git add -u, git add ., or git add -A) + git status --porcelain | grep '^ M' | cut -c4- | while read file; do + git add "$file" + done + git commit -m "fix({phase}): orchestrator corrections" ``` **If clean:** Continue to verification. diff --git a/hooks/gsd-statusline.js b/hooks/gsd-statusline.js index c9cc2d904..fa8889f97 100755 --- a/hooks/gsd-statusline.js +++ b/hooks/gsd-statusline.js @@ -48,17 +48,21 @@ process.stdin.on('end', () => { const homeDir = os.homedir(); const todosDir = path.join(homeDir, '.claude', 'todos'); if (session && fs.existsSync(todosDir)) { - const files = fs.readdirSync(todosDir) - .filter(f => f.startsWith(session) && f.includes('-agent-') && f.endsWith('.json')) - .map(f => ({ name: f, mtime: fs.statSync(path.join(todosDir, f)).mtime })) - .sort((a, b) => b.mtime - a.mtime); + try { + const files = fs.readdirSync(todosDir) + .filter(f => f.startsWith(session) && f.includes('-agent-') && f.endsWith('.json')) + .map(f => ({ name: f, mtime: fs.statSync(path.join(todosDir, f)).mtime })) + .sort((a, b) => b.mtime - a.mtime); - if (files.length > 0) { - try { - const todos = JSON.parse(fs.readFileSync(path.join(todosDir, files[0].name), 'utf8')); - const inProgress = todos.find(t => t.status === 'in_progress'); - if (inProgress) task = inProgress.activeForm || ''; - } catch (e) {} + if (files.length > 0) { + try { + const todos = JSON.parse(fs.readFileSync(path.join(todosDir, files[0].name), 'utf8')); + const inProgress = todos.find(t => t.status === 'in_progress'); + if (inProgress) task = inProgress.activeForm || ''; + } catch (e) {} + } + } catch (e) { + // Silently fail on file system errors - don't break statusline } }