refactor: Apply codebase audit improvements

[evansenter]

Summary

• Add GITHUB_TOKEN documentation comment to .exports
• Fix hardcoded Homebrew path in LaunchAgent plist (now supports both Intel /usr/local and ARM /opt/homebrew)
• Add strict mode (set -euo pipefail) to bootstrap.sh and uninstall.sh
• Add explicit HISTFILE=~/.zsh_history to .zshrc
• Update /im-lost command to reference linked issues in Context and Suggested Next Actions
• Document vim dark-only theme limitation in root CLAUDE.md
• Fix missing trailing newline in preferences/iTerm Profile.json

Test plan

• Run ./bootstrap.sh -f - verified it completes successfully
• Test on Intel Mac (if available) to verify Homebrew path detection

🤖 Generated with Claude Code

[claude]

PR Review: Codebase Audit Improvements

Overall, this is a solid refactoring PR that addresses several good practices. The changes are well-scoped and improve robustness.

✅ Strengths

1. Strict Mode Addition - Adding set -euo pipefail to both shell scripts is excellent for catching errors early
2. Cross-Architecture Support - The Homebrew path detection for Intel vs ARM Macs is a needed improvement
3. Documentation - Adding the GITHUB_TOKEN comment and vim theme limitation is helpful
4. Defensive Variable Reference - The ${ZSH_VERSION:-} change prevents errors in non-zsh shells

🔍 Code Quality & Best Practices

Suggestions:

1. bootstrap.sh:101-105 - Consider using brew --prefix instead of hardcoding paths for better robustness with custom Homebrew installations

2. home/.zshrc:21 - Adding explicit HISTFILE is good but consider a comment explaining why (it is the zsh default already)

🐛 Potential Issues

1. bootstrap.sh:2 - With set -euo pipefail, test that error handling at line 215 still works correctly. The set -e flag can interact unexpectedly with || operators.

2. bootstrap.sh:107 - The sed command assumes the template file exists. With strict mode, consider adding validation before the sed command.

3. Ensure macOS-specific commands in install_launch_agents() do not cause failures on Linux with strict mode enabled.

🔒 Security

Minor suggestion: Add a stronger warning in .exports not to commit tokens to version control.

🧪 Test Coverage

Recommendations:

1. Test strict mode error handling flows
2. Verify sed works on both GNU sed (Linux) and BSD sed (macOS)
3. Test sourcing .zshrc from bash context
4. Verify LaunchAgent reload with dynamic path

Recommendation

Approve with minor suggestions. The PR is solid and ready to merge after testing error handling with strict mode enabled.

Nice work on the audit!

[claude]

PR Review: refactor: Apply codebase audit improvements

Summary

This PR makes excellent improvements to the dotfiles infrastructure with a focus on robustness, portability, and documentation. Overall, this is a high-quality set of changes that demonstrates careful attention to detail.

---

Strengths

1. Strict Mode Implementation
Adding set -euo pipefail to both bootstrap.sh:2 and uninstall.sh:2 is a best practice that significantly improves script reliability by exiting on errors, undefined variables, and pipe failures.

2. Homebrew Path Detection Logic
The improved Homebrew detection in bootstrap.sh:100-107 is well-designed. It prefers brew --prefix when available, then falls back to architecture-based detection. This handles custom installations and is future-proof.

3. Defensive Variable Checking
The change in bootstrap.sh:217 to use ZSH_VERSION:- is critical for strict mode compatibility and prevents errors when the variable is unset.

4. Documentation Improvements

• Added GITHUB_TOKEN documentation in home/.exports:5-6
• Documented vim dark-only theme limitation in CLAUDE.md:87
• Enhanced /im-lost command to reference linked issues

---

Issues & Concerns

Critical: Potential Resource Leak in LaunchAgent Installation

Location: bootstrap.sh:113-119

The temporary file handling has a critical issue. If any command between mktemp and the final mv/rm fails due to set -e, the temporary file will be leaked.

Recommendation: Add a trap to ensure cleanup on EXIT.

Medium: Missing Error Handling for Homebrew Prefix

Location: bootstrap.sh:100-107

The logic assumes that if brew exists, brew --prefix will succeed. However, this command can fail in rare cases like corrupted installations or permission issues. Consider adding error handling with fallback to architecture-based detection.

---

Security, Performance, and Testing

Security: No concerns identified. The changes use proper quoting, avoid command injection, and follow best practices.

Performance: No issues. The changes add minimal overhead and maintain efficiency.

Test Coverage: The test plan is reasonable but could benefit from additional edge case testing, particularly for brew --prefix failure and GITHUB_TOKEN missing scenarios.

---

Additional Observations

1. HISTFILE explicit setting in home/.zshrc:21 is good defensive programming
2. /im-lost command improvements will significantly improve workflow awareness
3. Trailing newline fix in iTerm Profile.json follows POSIX standards

---

Recommended Actions

Before Merge:

1. Fix the temporary file leak in bootstrap.sh:113-119 (Critical)
2. Add error handling for brew --prefix failure (Medium)

Optional Improvements:

1. Add --dry-run flag for preview mode
2. Consider logging operations for debugging
3. Validate dark-notify is executable before installing LaunchAgent

---

Overall Assessment

Code Quality: 4/5
Best Practices: 5/5
Security: 5/5
Documentation: 5/5

Recommendation: Approve with minor revisions. The critical issue with temporary file cleanup should be addressed, but this is a well-crafted PR that meaningfully improves the codebase.

Great work on this audit! The strict mode addition and improved Homebrew detection are particularly valuable improvements.

[claude]

PR Review: refactor: Apply codebase audit improvements

Overall, this is a solid PR with excellent improvements to error handling, portability, and documentation.

Strengths

1. Strict mode (set -euo pipefail) - Great addition to both scripts. Catches errors early.
2. Homebrew path detection - Dynamic detection is much better than hardcoded paths.
3. Defensive programming - The ZSH_VERSION parameter expansion prevents errors with set -u.
4. Explicit HISTFILE - Good practice to make configuration explicit.
5. Documentation improvements - GITHUB_TOKEN comment and vim theme limitation note improve UX.

Issues and Suggestions

Critical: Potential error handling issue (bootstrap.sh:101-107)

The brew --prefix command could fail with set -e enabled, causing the script to exit entirely. Consider adding error handling with a fallback or temporarily disabling set -e for this command.

Important: Missing HISTFILE directory check (home/.zshrc:21)

The parent directory for HISTFILE may not exist. Consider ensuring it exists in bootstrap.sh or .zshrc.

Suggestion: sed portability (bootstrap.sh:109)

The sed command should work, but testing on Linux would confirm GNU sed compatibility.

Testing Recommendations

Additional testing needed:

• Intel Mac: Verify Homebrew path detection works with /usr/local
• Linux: Test that LaunchAgent installation is correctly skipped
• Broken Homebrew: Test when brew exists but brew --prefix fails

Summary

Recommendation: Approve with minor fixes

Security: No concerns
Performance: No impact
Test coverage: Needs Intel Mac verification
Code quality: Excellent improvements

Great work! The strict mode addition alone will catch many issues early.

[claude]

Code Review

Great work on this codebase audit! The changes improve robustness, documentation, and cross-platform compatibility. Here's my detailed feedback:

✅ Strengths

1. Excellent error handling improvements - Adding set -euo pipefail to both scripts is a best practice that will catch errors early
2. Smart Homebrew path detection - The fallback strategy (brew --prefix → architecture detection → hardcoded paths) is well-designed
3. Proper cleanup with traps - The trap for temp file cleanup prevents leaks on script failure
4. Good documentation - GITHUB_TOKEN comment in .exports helps users understand requirements
5. Explicit HISTFILE setting - Prevents issues when zsh uses different default history locations

[claude]

Important Issue #2: bootstrap.sh:115 - Trap cleared inconsistently

The trap is cleared at line 131 inside the success branch, but if the else branch executes line 129, the trap remains active. While functional, this is inconsistent.

Important Issue #3: bootstrap.sh:192 - Variable expansion inconsistency

The install_claude_mcp_servers function references GITHUB_TOKEN without defensive expansion. With strict mode (set -u), this will fail if unset. Should match the defensive pattern used at line 219 for ZSH_VERSION.

Suggestions

#1. bootstrap.sh:9 - Line 9 uses BASH_SOURCE while line 16 uses BASH_SOURCE[0]. Prefer explicit array index for consistency.

#2. brew --prefix failure - Silent fallback at line 101. Consider logging when brew --prefix fails for debugging.

#3. iTerm Profile.json - Trailing newline fix is correct, but this is a generated file. Document in CLAUDE.md to prevent future manual edits.

[claude]

🎯 Test Coverage

The PR indicates manual testing on the bootstrap script, which is good. Consider adding:

1. Test on Intel Mac - Already noted in the test plan
2. Test GITHUB_TOKEN warning - Verify the warning appears when token is unset
3. Test trap cleanup - Intentionally cause a failure after mktemp to ensure temp file is cleaned up

📝 Documentation

The CLAUDE.md updates are helpful and accurately describe the new behavior. Well done!

Performance & Security

• Performance: No concerns - changes are minimal overhead
• Security:
  • ✅ Strict mode prevents many common bash pitfalls
  • ✅ Temp file cleanup prevents information leaks
  • ✅ No new security vulnerabilities introduced

Summary

This is high-quality work that materially improves the dotfiles robustness. The strict mode addition and Homebrew path detection are particularly valuable improvements. The issues I identified are minor - mostly consistency and edge case handling.

Recommendation: Approve with minor suggested improvements. The code is safe to merge as-is, but addressing the Important items would further improve code quality.

Nice work! 🎉

[claude]

PR Review: Codebase Audit Improvements

Overall, this is a well-executed refactoring that improves robustness and documentation. The changes are focused, tested, and follow shell scripting best practices. Here's my detailed feedback:

---

✅ Strengths

1. Strict Mode Addition - Adding set -euo pipefail to both shell scripts is excellent:

   • Catches uninitialized variables
   • Propagates errors from pipelines
   • Prevents silent failures

2. Homebrew Path Detection - The fix for ARM vs Intel Macs is well-designed:

   • Tries brew --prefix first (authoritative)
   • Falls back to architecture-based detection
   • Properly handles errors with 2>/dev/null

3. Trap Cleanup - Good use of trap 'rm -f "$tmp_plist"' EXIT to ensure temp file cleanup

4. Documentation Improvements - All the CLAUDE.md updates are valuable additions

---

🔍 Issues Found

Critical

None found.

Important

#1. Trap Exit Code Handling (bootstrap.sh:114-131)

The trap is being cleared with trap - EXIT before returning from the function, which means if the function exits unexpectedly, the trap won't fire. Consider this pattern instead:

local tmp_plist
tmp_plist=$(mktemp)
# Clean up on exit, but preserve exit code
trap 'rm -f "$tmp_plist"' EXIT

if [[ \! -f "$dest_plist" ]] || \! cmp -s "$tmp_plist" "$dest_plist"; then
    echo "Installing LaunchAgent: $plist"
    mv "$tmp_plist" "$dest_plist"
    # mv consumed the file, no cleanup needed
    trap - EXIT
    
    # Reload the agent
    launchctl bootout "gui/$(id -u)/com.user.dark-notify" 2>/dev/null || true
    launchctl bootstrap "gui/$(id -u)" "$dest_plist"
    
    # Trigger theme sync
    "$HOME/.bin/toggle-btop-theme"
fi
# If we didn't mv the file, trap will clean it up

Current behavior: If an error occurs after setting the trap but before explicitly removing it (e.g., launchctl fails), the file might not be cleaned up due to set -e exiting early.

#2. ZSH_VERSION Check Redundancy (bootstrap.sh:220)

The change from [[ -n "$ZSH_VERSION" ]] to [[ -n "${ZSH_VERSION:-}" ]] is good for strict mode compliance, but there's a logic issue:

if [[ -n "${ZSH_VERSION:-}" ]]; then
    source ~/.zshrc 2>/dev/null || echo "Restart your terminal or run: source ~/.zshrc"
fi

Problem: This script runs in bash (shebang is #\!/usr/bin/env bash), so ZSH_VERSION will always be empty. This check can never be true unless the script is sourced in zsh (not recommended).

Recommendation: Remove this check entirely. The user should manually restart their shell or source the file:

echo "✓ Installation complete\!"
echo "  Restart your terminal or run: source ~/.zshrc"

---

Suggestions

#3. HISTFILE Documentation (home/.zshrc:21)

Adding explicit HISTFILE=~/.zsh_history is good defensive programming, but consider adding a comment explaining why:

# Explicitly set history file (should be zsh default, but be defensive)
HISTFILE=~/.zsh_history

#4. Error Message Specificity (bootstrap.sh:102-106)

The homebrew detection could provide better error messaging if dark-notify is installed but homebrew is not found:

if command -v brew >/dev/null 2>&1 && homebrew_prefix="$(brew --prefix 2>/dev/null)"; then
    : # homebrew_prefix set by condition
elif [[ "$(uname -m)" == "arm64" ]]; then
    homebrew_prefix="/opt/homebrew"
    echo "Warning: brew command not found, assuming ARM Homebrew path: $homebrew_prefix"
else
    homebrew_prefix="/usr/local"
    echo "Warning: brew command not found, assuming Intel Homebrew path: $homebrew_prefix"
fi

#5. /im-lost Command Enhancement (home/.claude/commands/im-lost.md:30)

The instruction to "parse the PR body for issue references" is good, but could be more specific about the regex patterns Claude should use:

Parse the PR body for issue references using these patterns:
- Keywords: `Fixes #123`, `Closes #45`, `Resolves #67`
- Direct references: `#123`
- Full URLs: `https://github.com/owner/repo/issues/123`

---

🧪 Test Coverage

• ✅ Tested ./bootstrap.sh -f successfully
• ⚠️ Intel Mac testing pending (understandable limitation)
• Suggestion: Add a test case in the PR description for the strict mode behavior:
  • Verify script fails fast on errors (e.g., temporarily break a command and verify it exits immediately)

---

🔒 Security Considerations

#6. GITHUB_TOKEN in ~/.extra (home/.exports:5-6)

Good call documenting this requirement. Consider adding additional security guidance:

# Variables expected in ~/.extra (not tracked):
#   GITHUB_TOKEN - Required for Claude Code GitHub MCP server
#                  Use a fine-grained PAT with minimal scopes
#                  Rotate periodically and never commit to git

---

📊 Performance

No performance concerns. The changes are lightweight and the homebrew detection optimization (trying brew --prefix first) is efficient.

---

🎯 Summary

Recommended Actions:

1. Must Fix: Issue Add Claude Code GitHub Workflow #2 (ZSH_VERSION check is non-functional)
2. Should Fix: Issue Add dark mode theme switching for btop and iTerm2 #1 (trap cleanup edge case)
3. Nice to Have: Suggestions Refactor sync to use home/ directory #3-6

Overall Assessment: ✅ Approve with minor fixes

This PR significantly improves the robustness of the bootstrap scripts and adds valuable documentation. The strict mode addition alone makes this worthwhile. With the two "Important" issues addressed, this is ready to merge.

---

Great work on the comprehensive codebase audit! 🎉