feat: Add AI Assistant context menu integration (#225)

[kshivang]

Summary

• Add built-in AI assistants: Claude Code, Codex, Gemini CLI, OpenCode
• Auto-detect installation with assistant-specific paths (e.g., ~/.opencode/bin/opencode, ~/.claude/local/claude)
• YOLO/Auto mode flags enabled by default:
  • Claude Code: --dangerously-skip-permissions
  • Codex: --full-auto
  • Gemini CLI: -y
  • OpenCode: --auto-approve
• Support custom AI assistants with add/edit/delete
• Per-assistant configuration (enable/disable, custom command, custom flags)
• Settings UI under Settings > AI Assistants
• Context menu shows "AI Assistants" submenu with launch/install options
• Auto Node.js installation via nvm (Linux), Homebrew (macOS), winget (Windows)

Test plan

• Right-click in terminal shows "AI Assistants" submenu
• Installed assistants show direct launch option with Auto mode label
• Not installed assistants show Install (Script/npm) and Learn More options
• Settings > AI Assistants shows all assistants with config options
• Toggle Auto mode on/off works correctly
• Add custom assistant dialog works
• Custom assistants appear in context menu

🤖 Generated with Claude Code

[claude]

Code Review: AI Assistant Integration (#225)

Overall, this is a well-structured and comprehensive implementation of AI assistant integration for BossTerm. The code quality is high with good separation of concerns and thoughtful UX design.

Critical Issues to Fix

1. Resource Leak in Process Execution

Location: AIAssistantDetector.kt:180-186

If waitFor() times out, the process continues running. Add process.destroyForcibly() in a finally block to ensure cleanup.

2. Race Condition in Auto-Refresh

Location: AIAssistantDetector.kt:63-70

Multiple rapid calls to startAutoRefresh() could create multiple coroutines. Add synchronization with a lock object.

Important Suggestions

3. Code Duplication

EmbeddableTerminal.kt and TabbedTerminal.kt have 46 lines of identical AI assistant initialization code. Extract to shared composable.

4. Silent Detection Failures

runCommand() catches all exceptions silently. Consider exposing detection errors to help users debug PATH/permission issues.

5. Refresh Interval Too Low

The slider allows 10-second intervals which could cause excessive CPU usage. Increase minimum to 30 seconds.

What I Really Like

• Multi-strategy detection is thorough and smart
• Auto Node.js installation is excellent UX
• Settings UI is polished and intuitive
• Security handled correctly (working directory escaping)
• Proper lifecycle management with Compose
• Extensibility design is impressive

Summary

Verdict: Approve with recommended changes

This PR adds significant value. Please address the two critical issues (resource leak and race condition) before merging.

Files reviewed: 12 files, 1657 additions, 17 deletions

[claude]

Code Review: AI Assistant Context Menu Integration (#225)

Overview

This PR adds comprehensive AI assistant integration to BossTerm's context menu, supporting Claude Code, Codex, Gemini CLI, and OpenCode with auto-detection, launch, and installation capabilities. The implementation is well-structured with good separation of concerns.

---

✅ Strengths

Architecture & Design

• Clean separation of concerns: Detection, launching, menu generation, and settings are properly separated into distinct classes
• Extensible design: The AIAssistantDefinition data class makes it easy to add new assistants without code changes
• Reactive state management: StateFlow integration for installation status is well-implemented
• Platform-aware: Proper platform detection for macOS, Linux, and Windows with appropriate fallbacks

Code Quality

• Comprehensive documentation: All public APIs have clear KDoc comments
• Error handling: Proper try-catch blocks in AIAssistantDetector.runCommand() and URL opening logic
• Resource cleanup: AIAssistantDetector.dispose() properly cancels coroutines

---

🔍 Issues & Concerns

1. CRITICAL SECURITY: Command Injection Vulnerability 🚨

Location: AIAssistantLauncher.kt:28

"cd ${escapeShellArg(workingDirectory)} && $command\n"

Problem: The command variable is NOT escaped, only workingDirectory is. Since command can include user-provided custom commands (via config.customCommand), this creates a command injection vulnerability.

Attack scenario:

// Malicious custom command
customCommand = "claude; rm -rf ~/*"
// Results in: cd '/safe/path' && claude; rm -rf ~/*

Fix: Apply shell escaping to the entire command or use safer execution methods (ProcessBuilder instead of shell strings).

Impact: HIGH - User-provided custom commands can execute arbitrary shell commands

---

2. Security: Dangerous Auto-Install Scripts ⚠️

Location: AIAssistantDefinition.kt:88, 121

installCommand = "curl -fsSL https://claude.ai/install.sh | bash"
installCommand = "curl -fsSL https://opencode.ai/install | bash"

Issues:

• Executes remote scripts with zero verification (no checksum, no signature)
• -fsSL flags hide errors that could warn users
• Piping directly to bash gives script full user permissions
• No HTTPS certificate validation enforcement

Recommendation:

1. Add warning UI before running curl | bash commands
2. Consider two-step install (download → review → execute)
3. Add checksums/signatures if available from upstream
4. Document security implications in UI

---

3. Thread Safety: Mutable Static State ⚠️

Location: AIAssistantDefinition.kt:135

private var _customAssistants: List<AIAssistantDefinition> = emptyList()

fun setCustomAssistants(assistants: List<AIAssistantDefinition>) {
    _customAssistants = assistants.map { it.copy(isBuiltIn = false) }
}

Problem:

• _customAssistants is mutable without synchronization
• ALL getter creates new list on every access (inefficient)
• Concurrent reads during setCustomAssistants() could see partial state

Fix: Use @Volatile or synchronization for thread safety

---

4. Resource Leak: Process Streams Not Closed 🐛

Location: AIAssistantLauncher.kt:134, 138, 180

ProcessBuilder("open", url).start()  // Process never cleaned up
ProcessBuilder("cmd", "/c", "start", "", url).start()
ProcessBuilder(command).redirectErrorStream(true).start()

Problem: Processes are started but never explicitly closed. While short-lived commands may exit quickly, error streams/input streams remain open until GC.

Recommendation: Add comments explaining intentional fire-and-forget behavior, or add explicit cleanup for error cases.

---

5. Performance: Inefficient Detection Strategy 🐌

Location: AIAssistantDetector.kt:97-147

Issues:

• Spawns 2+ processes per assistant per detection (which + shell which)
• No caching between menu opens (despite async refresh)
• 5-second timeout per command × multiple strategies = slow UX

Impact: Checking 4 assistants with 5 strategies each = up to 20 process spawns + 100 seconds of potential timeouts (though parallel execution mitigates this).

Recommendation:

• Add caching layer (cache for 5-10 seconds)
• Short-circuit on first success (already done ✅)
• Reduce timeout to 1-2 seconds per command

---

6. Bug: Incorrect npmPackage Extraction 🐛

Location: AIAssistantLauncher.kt:76-77

val npmPackage = assistant.npmInstallCommand?.removePrefix("npm install -g ")
    ?: assistant.installCommand.removePrefix("npm install -g ")

Problem: If installCommand is "curl ... | bash", removePrefix returns the full string unchanged, resulting in invalid npm commands.

Example: Claude Code has installCommand = "curl..." and npmInstallCommand = "npm install -g @anthropic-ai/claude-code". If npmInstallCommand is null, the fallback breaks.

Fix: Only use npm path if installCommand actually starts with "npm":

val npmPackage = assistant.npmInstallCommand?.removePrefix("npm install -g ")
    ?: assistant.installCommand.takeIf { it.startsWith("npm install -g ") }
        ?.removePrefix("npm install -g ")
    ?: return "echo 'No npm install command available'"

---

7. Missing Null Safety 🐛

Location: Multiple locations with AIAssistants.findById()

fun findById(id: String): AIAssistantDefinition?  // Returns nullable

Problem: Callers don't always check for null when assistant ID is invalid/removed.

Recommendation: Add null checks or use requireNotNull() where assistant must exist.

---

📋 Additional Observations

Code Style

• ✅ Follows Kotlin conventions (data classes, named parameters, extension functions)
• ✅ Proper use of coroutines and StateFlow
• ⚠️ Some long methods (getNpmInstallCommandWithNodeCheck at 40+ lines) could be extracted

Testing

• ❌ No unit tests for critical security functions (escapeShellArg, buildLaunchCommand)
• ❌ No tests for detection logic
• ❌ No tests for command generation

Recommended tests:

@Test
fun testEscapeShellArg_preventsInjection() {
    val malicious = "'; rm -rf /; '"
    val escaped = escapeShellArg(malicious)
    assertFalse(escaped.contains("rm -rf"))
}

@Test
fun testLaunchCommand_escapesCustomCommand() {
    val config = AIAssistantConfigData(customCommand = "claude; echo pwned")
    val cmd = launcher.getLaunchCommand(assistant, config)
    assertFalse(cmd.contains("; echo"))
}

Documentation

• ✅ Good KDoc coverage
• ⚠️ Missing security warnings in user-facing strings
• ⚠️ No README or docs update for new feature

---

🎯 Recommendations Summary

Must Fix Before Merge

1. ✅ Fix command injection vulnerability in AIAssistantLauncher.kt:28
2. ✅ Add @Volatile to _customAssistants or use proper synchronization
3. ✅ Fix npm package extraction bug

Should Fix

4. Add warning UI for curl | bash installation
5. Close process streams or document intentional detachment
6. Add basic unit tests for security-critical functions

Nice to Have

7. Add caching layer for detection
8. Reduce detection timeout from 5s to 1-2s
9. Add null safety checks for findById() usage

---

📊 Overall Assessment

Code Quality: 7/10

• Well-structured, good separation of concerns
• Missing critical security fix

Security: 4/10 ⚠️

• Command injection vulnerability must be fixed
• Unverified remote script execution needs warnings

Test Coverage: 2/10 ⚠️

• No tests for 1,600+ lines of new code
• Critical security functions untested

Performance: 6/10

• Detection could be optimized with caching
• Multiple process spawns per detection

Recommendation: Request Changes - Fix command injection vulnerability before merge, add basic tests for security functions.

---

Great work on the feature implementation! The architecture is solid and extensible. Please address the security issues and consider adding tests. Happy to re-review once updated! 🚀

[claude]

Pull Request Review: AI Assistant Context Menu Integration (#225)

Overview

This PR adds comprehensive AI assistant integration to BossTerm, enabling users to launch and install AI coding assistants (Claude Code, Codex, Gemini CLI, OpenCode) directly from the context menu. The implementation is well-architected with clear separation of concerns across detection, launching, and menu generation.

---

Strengths

Architecture and Design

1. Clean Separation of Concerns: The code is well-organized into distinct responsibilities
2. Reactive State Management: Uses Kotlin Flow for reactive installation status updates
3. Extensibility: The design allows for both built-in and custom AI assistants
4. Platform Awareness: Proper platform-specific handling for Windows, macOS, and Linux

---

CRITICAL: Security Vulnerabilities

1. Command Injection in escapeShellArg (AIAssistantLauncher.kt:202-204)

Severity: HIGH - Could allow arbitrary command execution

The current escaping is insufficient for all shell contexts. Consider inputs with semicolons, backticks, or command substitution. Input like "/tmp; rm -rf /" could still execute dangerous commands.

Recommendation: Use ProcessBuilder with separate arguments instead of shell string concatenation, or change the terminal writer API to accept a list of arguments instead of a raw string.

2. Arbitrary Script Execution (AIAssistantLauncher.kt)

Severity: MEDIUM - Potential for supply chain attacks

The install commands use curl pipe bash pattern without any verification (no checksum verification, no HTTPS certificate pinning). If these domains are compromised, arbitrary code execution is possible.

Recommendation:

• Add prominent warning in UI before running install scripts
• Consider showing script preview before execution
• Document security implications in code comments

3. Process Timeout Without Cleanup (AIAssistantDetector.kt:182-204)

Severity: LOW - Resource leak

The runCommand method has a timeout but does not clean up stderr/stdout pipes before destroying. Should drain streams before destroyForcibly().

---

Code Quality Issues

4. Mutable Global State (AIAssistantDefinition.kt:135-141)

Severity: MEDIUM - Thread safety concerns

The _customAssistants variable is not thread-safe. Multiple threads could call setCustomAssistants concurrently. Should use @volatile and @synchronized, or better yet make this immutable.

5. No Tests

Severity: HIGH - Quality assurance

The PR adds 1,672 lines of new code with zero tests. Need unit tests for:

• escapeShellArg with malicious inputs (CRITICAL!)
• Platform-specific command generation
• Detection logic with mocked file system
• Menu generation with various states

6. Minor Issues

• Magic numbers (30000, 5) should be extracted to named constants
• Inconsistent naming (yoloEnabled vs Auto mode)
• Code duplication between AIAssistantConfig.buildFullCommand and AIAssistantLauncher.buildLaunchCommand

---

Recommendations Summary

Must Fix Before Merge:

1. Add tests for command injection scenarios
2. Fix or document security implications of curl pipe bash installations
3. Add thread safety to AIAssistants.setCustomAssistants

Should Fix:

4. Improve error handling in launch commands
5. Add at least basic unit tests for core logic
6. Consider ProcessBuilder instead of shell string concatenation

Nice to Have:

7. Extract magic numbers to constants
8. Reduce code duplication
9. Add documentation for security considerations

---

Overall Assessment

This is a well-designed feature with good architecture, but has critical security concerns that must be addressed before merging. The lack of tests is also concerning for code that handles command execution. With the security fixes and basic test coverage, this would be ready to merge.

Estimated Risk: MEDIUM-HIGH (due to command injection potential)

Files Reviewed: 12 files, 1,672 additions, 17 deletions

[claude]

Code Review for PR #226: AI Assistant Context Menu Integration

This is a well-architected addition that brings AI coding assistant integration to BossTerm.

Overall Assessment

Verdict: Approve with minor recommendations

This PR demonstrates solid engineering with good separation of concerns, proper lifecycle management, and thoughtful UX design.

Strengths

1. Excellent Architecture

• Clean separation between detection, launching, and UI components
• AIAssistantDefinition provides solid data model with extensibility
• Registry pattern is well-implemented with proper state management

2. Strong Refactoring (Commit 2)

• Excellent extraction of shared state into rememberAIAssistantState()
• Removed 46 lines of duplicate code
• Proper lifecycle management

3. Resource Management Fix (Commit 3)

• Good catch on process cleanup with finally block
• Prevents resource leaks

4. Performance Optimization (Commit 4)

• Removing polling in favor of on-demand detection
• Reduces CPU usage and eliminates excessive process spawning

5. Comprehensive Settings UI

• Well-designed with collapsible advanced options
• Per-assistant configuration is intuitive
• Good visual feedback

Security Concerns

CRITICAL: Command Injection Vulnerability

Location: AIAssistantLauncher.kt:23-31

The workingDirectory parameter uses basic shell escaping that may not be sufficient. While escapeShellArg() uses single-quote wrapping, OSC 7 working directory comes from shell output which could be manipulated.

Recommendations:

1. Validate the working directory path before using it
2. Sanitize the path to only allow safe characters
3. Use ProcessBuilder with directory() parameter instead of cd command

MEDIUM: Process Timeout Handling

Location: AIAssistantDetector.kt:145-166

The catch block catches all exceptions and returns false without logging. Recommend adding error logging and properly handling InterruptedException.

Potential Bugs

1. Race Condition in Custom Assistants Loading

Location: AIAssistantSettingsSection.kt:58-73

AIAssistants.setCustomAssistants() mutates global state but detection happens asynchronously. Recommend chaining operations so detection happens after loading custom assistants.

2. Inconsistent Null Handling

Location: AIAssistantMenuProvider.kt:45

Consider using consistent null handling throughout and providing default configs at initialization.

Performance Considerations

Good: On-demand detection instead of polling

Consider:

1. Detection caching (30-second cache to prevent rapid re-detection)
2. Parallel detection using coroutines (reduce time from 5-10s to 1-2s)

Test Coverage

Missing tests for:

1. Command injection attack scenarios
2. Concurrent detection calls
3. Custom assistant CRUD operations
4. Working directory edge cases

Recommendations Summary

Must Fix (Before Merge)

1. Address command injection vulnerability
2. Fix race condition in custom assistants loading

Should Fix (High Priority)
3. Add detection caching
4. Implement parallel detection
5. Add error logging

Nice to Have (Future PRs)
6. Add unit tests for security-critical paths
7. Extract magic strings to constants
8. Add troubleshooting documentation

Conclusion

This is a high-quality feature implementation with excellent architecture. The main concern is the command injection vulnerability which should be addressed before merge.

Recommendation: Approve with required changes

Great work on this feature!

[claude]

Pull Request Review: AI Assistant Context Menu Integration

This PR adds comprehensive AI assistant integration. Here is my detailed review:

Strengths

1. Well-Architected Design

• Clean separation of concerns with distinct classes
• Excellent use of reactive state with Kotlin Flow
• Proper Compose integration with lifecycle management

2. Robust Detection Strategy
Multi-layered approach: specific paths, common directories, nvm scanning, which command, shell-sourced which

3. Cross-Platform Support
Platform-aware installation with auto Node.js setup

4. Great UX
Auto-close on success, clear errors, YOLO mode indicators, per-assistant config

5. Code Quality
Comprehensive KDoc, proper resource cleanup, serializable data classes

Critical Security Concerns

CRITICAL: Command Injection Vulnerability in AIAssistantLauncher.kt

Lines 37-39 and 76-114: User-provided customCommand and customYoloFlag are used directly without validation. Attack scenarios include malicious commands, command chaining, and dangerous install scripts.

Recommendations:

1. Validate custom commands against whitelist/regex
2. Add warning dialog for custom assistants
3. Sanitize all user inputs before shell execution

MEDIUM: URL Injection in openWebsite()

Lines 120-154: websiteUrl not validated before browser launch. Could expose local files or cause unexpected behavior.

Recommendation: Validate URL scheme (only allow https/http)

Bugs to Fix

1. Race Condition in Detection (EmbeddableTerminal.kt:286-291, TabbedTerminal.kt:603-608)
detectAll() is suspend function called in non-suspending context. Use coroutineScope.launch and add caching with TTL.

2. Process Cleanup on Timeout (AIAssistantDetector.kt:157-158)
Calling exitValue() after timeout throws exception. Add conditional check.

3. Missing Installation Error Handling (AIAssistantInstallDialog.kt:113-135)
Dialog can get stuck in Installing state. Add timeout logic and Cancel button.

4. NVM Path Traversal (AIAssistantDetector.kt:88-97)
listFiles() can return null. Add permission checks and use canonicalPath.

Code Quality Improvements

1. Extract magic strings to constants
2. Add input validation to AIAssistantDefinition (empty id/command)
3. Remove unused variable at line 78
4. Unify settings update pattern

Performance Considerations

1. Blocking detection on menu open causes UI jank - cache results with 30s TTL
2. Redundant setCustomAssistants() calls - add distinctUntilChanged()

Test Coverage

Missing tests for: command escaping, security vectors, detection strategies, URL validation, settings serialization

Verdict

Overall: Excellent architecture and UX, but security vulnerabilities must be fixed before merge.

Required Changes (BLOCKING):

1. Fix command injection in AIAssistantLauncher
2. Validate URL schemes in openWebsite()
3. Fix process timeout logic

Recommended (Non-blocking):
Input validation, detection caching, warning dialogs, unit tests, extract constants

Post-Merge:
Comprehensive tests, security documentation, verified assistants badge system

Great work! Once security issues are resolved, this will be a fantastic addition.

[claude]

Pull Request Review: AI Assistant Context Menu Integration (#225)

Thank you for this substantial feature addition! This PR adds built-in AI assistant integration with auto-detection, launch capabilities, and a settings UI. Overall, the implementation is solid and well-structured. Below are my findings organized by priority.

---

🔴 Critical Issues

1. Command Injection Vulnerability (AIAssistantLauncher.kt:28-31)

Severity: HIGH - Security Risk

The command variable is NOT escaped, only workingDirectory is. If customCommand from settings contains shell metacharacters (e.g., ; rm -rf ~), it could enable command injection.

Fix: Escape the entire command by applying escapeShellArg() to both the base command and YOLO flag.

Rationale: User-controlled input (customCommand, customYoloFlag) must be sanitized before shell execution. This follows OWASP guidelines and the CLAUDE.md directive about preventing command injection.

2. Resource Leak (AIAssistantDetector.kt:145-165)

Severity: MEDIUM - Performance/Stability

The finally block handles cleanup well, but youre not cleaning up the process streams explicitly. While .use {} handles inputStream, the errorStream and outputStream should also be closed to prevent descriptor leaks in long-running sessions.

---

🟡 High-Priority Issues

3. Inadequate Error Handling (Multiple Files)

AIAssistantDetector.kt: Silent failures on all detection strategies could leave users confused.

Recommendation: Add logging (at least for debug mode) to help diagnose detection issues.

4. Race Condition (TabbedTerminal.kt:605-611, EmbeddableTerminal.kt:268-274)

detectAll() is a suspending function that runs on Dispatchers.IO, but its called from a non-suspending lambda. If detection takes longer than the menu render, the menu will show stale data.

Fix: Launch coroutine explicitly using coroutineScope.launch

5. Missing Input Validation (AIAssistantDefinition.kt:38-43)

No validation that baseCommand is not blank. If customCommand is set to empty string and assistant.command is also empty, this could result in executing an empty command.

---

🟢 Moderate-Priority Issues

6. Code Duplication

The AI assistant context menu integration logic is duplicated in TabbedTerminal.kt and EmbeddableTerminal.kt (~30 lines each). Consider extracting to a shared utility function.

7. Magic Strings

Multiple places use System.getProperty(os.name).lowercase() directly. Extract to a Platform utility object.

8. Performance Issue (AIAssistantDetector.kt:87-98)

The nvm directory scan uses listFiles() without early exit. Add return true on first match.

---

✅ Strengths

1. Excellent Architecture: Clean separation between detection, launching, and menu generation
2. Platform Support: Comprehensive cross-platform installation commands
3. Settings Integration: Well-integrated with existing settings system
4. User Experience: Auto-detection on menu open is smart (no polling overhead)
5. Feature Completeness: Covers both built-in and custom assistants

---

🎯 Summary

This is a high-quality feature addition. The critical security issue (#1) must be fixed before merge. Issues #2-5 should be addressed for robustness. The remaining items are nice-to-haves.

Recommendation: Request changes for security fix, then approve after addressing critical/high-priority items.

Great work overall! 🚀

[claude]

Code Review: AI Assistant Integration

Critical Security Issues

1. Command Injection Vulnerability (AIAssistantLauncher.kt:28-30)
SEVERITY: HIGH

The command variable is not escaped when used in shell context. User-supplied commands from config.customCommand could contain shell metacharacters.

Attack: User sets command to: myassistant; rm -rf ~
Fix: Use ProcessBuilder with array arguments instead of shell string interpolation.

2. Timeout Handling Bug (AIAssistantDetector.kt:155)
SEVERITY: MEDIUM

Calling process.exitValue() after timeout throws IllegalThreadStateException.
Fix: Check if completed before calling exitValue().

3. Thread Safety Issue (AIAssistantDefinition.kt:134-140)
SEVERITY: MEDIUM

Mutable global state without thread safety in AIAssistants.setCustomAssistants.
Fix: Use @volatile or AtomicReference.

Strengths

• Excellent architecture with clear separation of concerns
• Great UX with on-demand detection and embedded terminal for installation
• Cross-platform support (nvm/Homebrew/winget)
• Extensible with custom assistant support

Additional Issues

• Complex installation commands are hard to debug
• Missing null safety in TabbedTerminal.kt:587
• Code duplication between EmbeddableTerminal and TabbedTerminal
• No URL validation (could be file:// or javascript:)
• No unit tests for command generation and shell escaping

Recommendations

Must Fix Before Merge:

1. Fix command injection vulnerability
2. Fix timeout handling bug
3. Add thread safety to singleton

Overall: 7.5/10

Good architecture and UX, but security issues must be addressed before merging.

Reviewed by Claude Code - 2026-01-07

[claude]

PR Review: AI Assistant Context Menu Integration (#225)

Overview

This PR adds comprehensive AI assistant integration to BossTerm with context menu support, automatic detection, installation dialogs, and per-assistant configuration. The implementation is well-structured with 1843 additions across 13 files.

---

✅ Strengths

1. Excellent Architecture & Separation of Concerns

• Clean separation: AIAssistantDefinition (data), AIAssistantDetector (detection), AIAssistantLauncher (commands), AIAssistantMenuProvider (UI generation)
• Proper use of Compose state management with rememberAIAssistantState()
• Reactive state with StateFlow for installation status tracking

2. Comprehensive Platform Support

• AIAssistantLauncher.kt:80-114: Platform-specific Node.js installation (Windows/macOS/Linux)
• AIAssistantLauncher.kt:159-200: Robust Linux browser detection with multiple fallbacks
• Proper shell escaping for security (line 202: escapeShellArg())

3. User Experience

• Context menu shows "Launch" for installed assistants, "Install/Learn More" submenu for uninstalled
• Auto mode flags enabled by default with clear labeling
• Installation progress shown in embedded terminal with auto-close on success
• Settings UI allows granular per-assistant configuration

4. Good Code Quality

• Extensive KDoc comments explaining purpose and behavior
• Immutable data classes with @serializable for settings persistence
• Proper Compose lifecycle management with remember{} and LaunchedEffect

---

⚠️ Issues & Concerns

1. Security: Command Injection Vulnerability 🔴 CRITICAL

Location: AIAssistantLauncher.kt:27-28

Problem: workingDirectory is escaped, but command is NOT escaped when user provides custom commands via settings. A malicious or misconfigured custom command could contain shell metacharacters.

Example Attack:
customCommand = "myai; rm -rf ~/"
Results in: "cd /path && myai; rm -rf ~/"

Recommendation:

• Validate/sanitize custom commands in settings
• Consider using ProcessBuilder instead of shell strings for launching assistants
• Add input validation in AIAssistantSettingsSection when adding custom assistants

---

2. Resource Leak: Process Not Cleaned Up 🟡 HIGH

Location: AIAssistantDetector.kt:175-177

Problem: If an exception is thrown after start(), the process is never destroyed. While waitFor() completes quickly, the stdout/stderr streams remain open.

Recommendation: Wrap in try-finally and call destroyForcibly() in finally block to ensure cleanup.

---

3. Performance: Detection Strategy Could Be More Efficient 🟡 MEDIUM

Location: AIAssistantDetector.kt:51-109

Issue: Detection runs 5 strategies sequentially for each assistant:

1. Assistant-specific paths
2. Common paths
3. nvm paths with listFiles() glob
4. which command
5. Shell-sourced which

Problems:

• Steps 4 & 5 spawn processes even if step 1-3 already found the binary
• Step 3 (listFiles() on ~/.nvm/versions/node) could be expensive if many Node versions installed
• All assistants detected sequentially, not in parallel

Recommendation: Use async to detect all assistants in parallel for better performance.

---

4. Error Handling: Silent Failures 🟡 MEDIUM

Locations:

• AIAssistantDetector.kt:155-159: runCommand() catches all exceptions and returns false
• AIAssistantLauncher.kt:150-153: openUrl() catches all exceptions and only prints to stdout

Issues:

• Detection failures are silent (no logging for debugging)
• Users don't see why browser didn't open
• println() goes to stdout, not proper logging

Recommendation: Add proper logging with java.util.logging.Logger for debugging and user feedback.

---

5. Missing Input Validation 🟡 MEDIUM

Location: AIAssistantSettingsSection.kt (custom assistant add/edit dialog)

Missing Validations:

• displayName: No check for empty string or length limits
• command: No validation for shell metacharacters or path injection
• id: No check for duplicate IDs or invalid characters
• websiteUrl: No URL format validation

Recommendation: Add validation function that checks for empty fields, shell metacharacters, and valid formats before saving custom assistants.

---

6. Test Coverage: None 🟡 MEDIUM

No tests found for:

• Command generation logic (especially shell escaping)
• Detection strategies
• Menu item generation based on installation status
• Settings serialization/deserialization

Recommendation: Add unit tests for at least:

• AIAssistantLauncher: escapeShellArg(), getLaunchCommand() with special chars, buildLaunchCommand()
• AIAssistantDetector: detectSingle() finds commands, handles missing commands gracefully

---

7. Code Quality: Minor Issues

a) Inconsistent null-safety: AIAssistantDetector.kt:159 - process variable scope issue in finally block
b) Hardcoded timeouts: AIAssistantDetector.kt:155 - waitFor(5, TimeUnit.SECONDS) should be constant
c) Magic strings: AIAssistantDefinition.kt - URLs and install commands hardcoded
d) Incomplete error messaging: AIAssistantInstallDialog.kt:170-179 - doesn't show stderr on failure

---

📋 Specific File Comments

AIAssistantDetector.kt

• Line 88: listFiles()?.forEach - potential NPE if directory deleted between exists check and listFiles
• Line 106: Shell injection risk if command contains special chars (though unlikely here)

AIAssistantLauncher.kt

• Line 76: npmPackage extraction with removePrefix could fail if format differs
• Line 95: macOS assumes Homebrew installed, no fallback

EmbeddableTerminal.kt / TabbedTerminal.kt

• Lines 261-276 (Embeddable), 580-612 (Tabbed): Duplicate logic for combining menu items - consider extracting to shared function

AIAssistantInstallDialog.kt

• Line 123: onInitialCommandComplete auto-closes on success but user might want to see output

---

🎯 Recommendations Summary

Must Fix (Before Merge)

1. Fix command injection vulnerability in AIAssistantLauncher.getLaunchCommand()
2. Fix process resource leak in AIAssistantDetector.openUrlOnLinux()
3. Add input validation for custom assistant creation

Should Fix (Soon After)

4. Add parallel detection for better performance
5. Add proper logging instead of silent failures
6. Add unit tests for core logic (especially security-sensitive parts)

Nice to Have

7. Extract duplicate menu-combining logic
8. Add configuration file for assistant definitions
9. Show stderr output in installation dialog on failure
10. Make timeouts configurable

---

🏁 Verdict

Recommendation: ✋ Request Changes

The feature is well-designed and adds significant value, but the command injection vulnerability is a security risk that must be addressed before merge. The resource leak and missing validation should also be fixed.

Once these critical issues are resolved, this will be a great addition to BossTerm!

---

Testing Checklist (After Fixes)

• Test custom assistant with special characters in command
• Test with malicious command: "myai; echo INJECTED"
• Verify process cleanup with ps aux during detection
• Test with 10+ Node versions in ~/.nvm to verify performance
• Test browser opening on Linux without xdg-open
• Test installation dialog error handling with invalid commands
• Verify settings persistence across restarts

Great work overall! The architecture is solid and the feature is comprehensive. Just needs these security and robustness fixes. 👍

[claude]

Code Review for PR #226: AI Assistant Integration

I've reviewed this PR implementing AI assistant integration with context menu support. Here's my detailed feedback:

---

✅ Strengths

Architecture & Design

• Well-structured: Clean separation of concerns with dedicated classes for detection, launching, menu generation, and settings
• Composable state management: Good use of remember, LaunchedEffect, and reactive StateFlow patterns
• Extensibility: Nice design allowing both built-in and custom assistants
• On-demand detection: Smart choice to detect on context menu open rather than continuous polling

Code Quality

• Clear documentation: Comprehensive KDoc comments throughout
• Type safety: Good use of Kotlin's type system with data classes and sealed classes
• Error handling: Try-catch blocks in appropriate places (URL opening, process execution)

---

🔴 Critical Issues

1. Command Injection Vulnerability (AIAssistantLauncher.kt:27-32)

Issue: While workingDirectory is escaped, the command and yoloFlag are NOT escaped before shell execution. This creates a command injection vulnerability if custom assistants are loaded from settings with malicious commands or YOLO flags contain shell metacharacters.

Fix: Escape the entire command or use process builder arrays instead of shell strings.

2. Resource Leaks (AIAssistantDetector.kt:145-166)

Issue: The error stream is not consumed. If which produces error output, the buffer can fill and cause the process to block.

Fix: Ensure streams are closed in the finally block.

3. Missing Input Validation (Settings files)

Settings are loaded from ~/.bossterm/settings.json but there's no validation that:

• Custom assistant IDs are unique
• Command/flag strings don't contain malicious content
• URLs are valid HTTP/HTTPS schemes

---

🟡 High Priority Issues

4. Process Timeout Issues (AIAssistantDetector.kt:155)

5-second timeout may be too aggressive for slow systems or network-dependent package managers. Consider making timeout configurable or increasing to 10 seconds.

5. Shell Sourcing May Hang (AIAssistantDetector.kt:105-107)

Login shells (-l) can execute arbitrary code from .bashrc/.zshrc which could take a long time, hang waiting for input, or have side effects. Consider using non-login shells or increasing timeout for this specific check.

6. Platform Detection Anti-Pattern (AIAssistantLauncher.kt:206-210)

Platform detection is repeated in multiple places. Should be centralized in a Platform utility class.

---

🟠 Medium Priority Issues

7. Hardcoded Assistant Definitions (AIAssistantDefinition.kt:80-124)

The built-in assistant list includes placeholders like "Codex (OpenAI)" and "Gemini CLI" which may not exist as actual products with these exact commands. Verify these tools exist before including them as built-in options.

8. Installation Dialog Race Condition (AIAssistantInstallDialog.kt:155)

After successful installation, detection refresh is deferred to the onDismiss callback, creating a race condition where the menu might not immediately show the assistant as installed.

9. No Feedback for Failed URL Opens (AIAssistantLauncher.kt:120-122)

Returns boolean but callers don't check the result. Users get no feedback if the URL fails to open.

10. Terminal Output Injection (TabbedTerminal.kt:648-651)

If assistant.displayName contains single quotes, the echo command breaks. Escape the display name or use double quotes with proper escaping.

11. Duplicate Code (TabbedTerminal.kt & EmbeddableTerminal.kt)

The AI assistant integration code is nearly identical between files. Consider extracting to a shared composable.

---

🔵 Low Priority / Suggestions

12. Node.js Auto-Install Commands are Brittle

The complex shell one-liners for auto-installing Node.js could fail in many edge cases. Consider adding error messages and manual installation instructions.

13. Missing Documentation

No documentation explaining how to add custom assistants, what Auto Mode flags do (security implications), or how to troubleshoot detection failures.

14. Test Coverage

This PR adds 1,843 lines of new code with 0 test files. Critical components that should have tests:

• AIAssistantDetector.detectSingle() - unit tests with mocked processes
• AIAssistantLauncher.getLaunchCommand() - test escaping logic
• AIAssistantConfig.buildFullCommand() - test flag combinations
• Settings serialization/deserialization with malformed input

---

📋 Security Checklist

• ❌ Command injection prevention: Needs fixes
• ⚠️ Input validation: Missing for user-provided settings
• ✅ Resource cleanup: Mostly good, minor issues
• ⚠️ Process timeouts: Could be improved
• ❌ Shell escaping: Incomplete coverage
• ✅ No hardcoded credentials: None found

---

🎯 Recommendations

Must Fix Before Merge:

1. Fix command injection vulnerability (escape all user input)
2. Add input validation for custom assistant settings
3. Fix terminal output injection with display names

Should Fix:

4. Add basic unit tests for core logic
5. Deduplicate AI integration code between TabbedTerminal and EmbeddableTerminal
6. Improve error handling and user feedback

Nice to Have:

7. Add user documentation
8. Centralize platform detection
9. Make timeouts configurable

---

Overall Assessment

This is a solid feature implementation with good architecture and clean code structure. The main concerns are security-related (command injection, input validation) which must be addressed before merging. Once the critical issues are fixed, this will be a valuable addition to BossTerm.

Recommendation: Request changes for security fixes, then approve after verification.

---

Great work on the feature! Let me know if you'd like me to elaborate on any of these points or provide specific code fixes.

[claude]

Code Review for PR #226: AI Assistant Integration

This PR adds comprehensive AI assistant integration. Overall well-structured with 1,961 additions across 16 files.

Strengths

• Clean separation of concerns (Detector, Launcher, MenuProvider)
• Extensible design supporting custom assistants
• Good Compose patterns and state management
• Comprehensive documentation and error handling

Critical Issues

1. Command Injection Vulnerability (AIAssistantLauncher.kt:202)

• Custom command field allows arbitrary shell commands
• RISK: HIGH - malicious assistants could execute arbitrary code
• FIX: Sanitize inputs or use ProcessBuilder with argument arrays

2. Debug Logging in Production (BossEmulator1.kt:475, BossTerminal.kt:404)

• println statements bypass logging framework
• FIX: Replace with LOG.debug()

3. Race Condition (TabbedTerminal.kt:619, EmbeddableTerminal.kt:279)

• Async detection stored in unsynchronized MutableMap
• FIX: Wrap in coroutineScope.launch

4. Resource Leak (AIAssistantDetector.kt:146-166)

• Process streams not explicitly closed on timeout
• FIX: Add stream.close() in finally block

Warnings

5. Hard-coded Shell Commands (AIAssistantLauncher.kt:75-115)

• Complex npm install logic with curl pipe bash
• Security: No checksum verification
• Recommendation: Use installation scripts with verification

6. Missing Input Validation (AIAssistantSettingsSection.kt:573)

• Only checks non-blank, no shell metacharacter validation
• Missing URL format validation
• Missing ID uniqueness check

7. Type Naming Inconsistency

• Both AIAssistantConfig and AIAssistantConfigData exist
• Recommendation: Standardize naming

Testing Concerns

• No unit tests for detection logic
• No tests for shell escaping (security-critical)
• No tests for installation fallback

Performance

• detectAll() runs synchronously on menu open
• Worst case: 20 seconds (4 assistants × 5s timeout each)
• FIX: Use non-blocking coroutines

Recommendations

Must Fix (Before Merge):

1. Remove debug println statements
2. Fix detection race condition
3. Add input validation for custom commands

Should Fix (High Priority):
4. Add unit tests for shell escaping
5. Improve error handling
6. Document security model

Nice to Have:
7. Cache detection results
8. Add telemetry
9. Support API-based assistants

Summary

Category	Rating	Notes
Architecture	⭐⭐⭐⭐	Well-structured
Code Quality	⭐⭐⭐	Debug code leaks
Security	⚠️	Command injection risk
Testing	⚠️	No tests
Documentation	⭐⭐⭐⭐	Excellent KDoc
Performance	⭐⭐⭐	Can be optimized

Main concerns: Security (command injection), debug code in production, missing tests.

Great work on the extensible design and settings UI! 🚀

[claude]

Code Review: AI Assistant Context Menu Integration (#225)

Overview

This PR adds comprehensive AI assistant integration with auto-detection, installation, and launch capabilities. The implementation is generally solid with good architecture, but there are several important issues to address.

---

Critical Issues

1. Security: Command Injection Vulnerabilities

Location: AIAssistantLauncher.kt:233-234

The escapeShellArg function is insufficient and vulnerable to command injection. User-controlled paths in workingDirectory could break out of single quotes.

Example Attack:
workingDirectory = "'; rm -rf ~; echo '"
Results in: cd ''; rm -rf ~; echo '' && claude

Recommendation: Use ProcessBuilder with separate arguments instead of shell string concatenation, OR implement proper shell escaping that handles all special characters.

2. Resource Leak: Process Cleanup

Location: AIAssistantDetector.kt:147-169

The finally block only checks process.isAlive after potential exceptions. If an exception occurs before waitFor(), the process reference might be null or the cleanup might fail.

Recommendation: Wrap process usage in nested try-finally to ensure cleanup even on exceptions.

3. Debug Logging in Production

Location: BossEmulator1.kt:475, BossTerminal.kt:404

Using println() for debug logging pollutes stdout/stderr and stays in production builds.

Recommendation: Revert to proper logging framework with LOG.debug().

---

Important Issues

4. Race Condition: Detection Status Updates

Location: EmbeddableTerminal.kt:299-312, TabbedTerminal.kt:617-626

The detectionResultsHolder workaround suggests a race condition between async detection and menu rendering. This could lead to stale status being displayed.

Recommendation: Use Compose State properly instead of a mutable map hack.

5. Complex Boolean Logic in Command Start Tracking

Location: EmbeddableTerminal.kt:890-915

The command start tracking logic has subtle edge cases. The implementation might still miss rapid commands.

Recommendation: Add explicit state machine with better logging for command lifecycle.

6. Missing Input Validation

Location: AIAssistantSettingsSection.kt (custom assistant dialog)

No validation for custom assistant fields (command, displayName, etc.). Users could enter empty strings, invalid commands, or malicious input.

Recommendation: Add validation before saving custom assistants.

---

Best Practices & Suggestions

7. Missing Tests

The PR adds 2,233 lines of code but no automated tests. At minimum, add:

• Unit tests for escapeShellArg() (especially with malicious input)
• Tests for AIAssistantDetector.isInstalled()
• Tests for command generation in AIAssistantLauncher

8. Detection Performance

The detectAll() function runs sequentially through all assistants. For 4+ assistants, this could take several seconds on slow systems.

Recommendation: Parallelize detection using coroutineScope with async/await.

9. Memory Leak: State Management

Location: EmbeddableTerminalState.kt:499

The aiInstallRequest state is never cleaned up automatically. If the composable is disposed while the dialog is open, this state persists.

Recommendation: Use DisposableEffect to clean up on disposal.

---

Summary

Verdict: Needs Changes Before Merge

The feature implementation is solid architecturally, but the security vulnerabilities and resource leaks must be fixed before merging. The debug logging should also be reverted.

After addressing these critical issues, this will be a valuable addition to BossTerm.

Action Items

1. Fix command injection vulnerability in escapeShellArg()
2. Fix process resource leak in runCommand()
3. Remove debug println statements
4. Address race condition in detection status (important)
5. Add input validation for custom assistants (important)
6. Add unit tests for security-critical code
7. Consider parallelizing detection for better UX

Great work overall! The AI assistant integration is a compelling feature.

[claude]

Code Review for PR #226: AI Assistant Context Menu Integration

This is a comprehensive feature addition integrating AI coding assistants into BossTerm. Overall well-structured with good separation of concerns.

Strengths

Architecture & Design

• Excellent separation: detection (AIAssistantDetector), launching (AIAssistantLauncher), UI (AIAssistantInstallDialog)
• Smart on-demand detection replacing polling is a great performance decision
• Good abstraction with AIAssistantDefinition for extensibility
• Programmatic API (installAIAssistant, isAIAssistantInstalled) provides excellent control

Code Quality Improvements

• Resource leak fix (cb6632b): destroyForcibly() in finally block
• Thread safety (73176ea): AtomicReference for detection results
• Proper stream cleanup prevents resource leaks
• OSC 133 refinement: B->D sequence tracking prevents false positives

User Experience

• Installation dialog with embedded terminal provides excellent visibility
• Fallback to npm when script fails
• Comprehensive settings UI with per-assistant customization

Issues & Concerns

1. Security (HIGH PRIORITY)

Command Injection in escapeShellArg (AIAssistantLauncher.kt:233-235)

• If workingDirectory contains newlines/control characters, could break quoted context
• Recommend: Add validation to reject control characters

Shell Command Construction

• Commands via string interpolation, custom assistants allow arbitrary config
• Recommend: Document security model (user-configured commands are implicitly trusted)

2. Resource Management

Process Cleanup (AIAssistantDetector.kt:155)

• inputStream.bufferedReader().use { it.readText() } before timeout could block indefinitely
• Recommend: Redirect to ProcessBuilder.Redirect.DISCARD since output unused

3. Code Duplication

Installation Dialog Handling (TabbedTerminal.kt:646-693, EmbeddableTerminal.kt:318-367)

• 112 duplicated lines
• Recommend: Extract to shared composable

4. Error Handling

URL Opening (AIAssistantLauncher.kt:158-184)

• Exceptions caught but user gets no feedback
• Recommend: Show notification when URL fails

5. Testing

Missing validation for custom assistants (empty commands, duplicate IDs)
Missing unit tests for core functionality

Performance

• Detection runs 5 strategies per assistant, could be slow
• Recommend: Cache results 5-10 seconds
• Multiple concurrent detectAll() calls could spawn many processes
• Recommend: Debounce or cancel previous detection

Code Quality Notes

Positive: Consistent naming, good docs, proper Kotlin idioms

Improvements: Replace println with LOG.debug, avoid magic strings, handle mutable AIAssistants.ALL

Final Verdict: 4/5 stars

Solid, well-implemented feature with good architecture.

Recommendation: Approve with minor changes

Required before merge:

• Add validation to escapeShellArg for control characters
• Replace debug println with proper logging

Recommended for follow-up:

• Extract duplicate dialog handling
• Add unit tests
• Consider debouncing detection

Great work! The iterative approach in commit history shows excellent development practices.

[claude]

Pull Request Review: AI Assistant Context Menu Integration

This PR adds comprehensive AI assistant integration to BossTerm with context menu support, detection, installation, and settings management. The implementation is well-structured with approximately 2,300 lines of new code across 17 files.

Strengths

Architecture & Design

• Excellent separation of concerns: Detection, launching, menu generation, and UI are cleanly separated
• Composable design with rememberAIAssistantState() providing a clean API
• Platform-aware: Handles Windows, macOS, and Linux installation differences gracefully
• Flexible per-assistant configuration with custom commands and YOLO flags

Code Quality

• Comprehensive KDoc documentation on all major classes
• Proper resource cleanup with AIAssistantDetector.closeStreams()
• Thread safety using AtomicReference for detection results
• Graceful error handling with retry options in installation dialog

User Experience

• Smart multi-strategy detection (specific paths → common paths → nvm → which → shell-sourced)
• Automatic Node.js installation via platform-specific package managers
• Script install with npm fallback and "Try npm" button on failure
• Clear YOLO mode indicators in menu labels

Critical Issues

1. Security - Command Injection Risk ⚠️

Location: AIAssistantLauncher.kt:233-234 and buildLaunchCommand()

Custom commands and YOLO flags are NOT escaped before execution. Users can configure arbitrary commands in settings that execute directly. This could allow malicious settings JSON to inject commands.

Recommendation: Validate custom commands against a whitelist pattern, add security warnings in UI, and consider requiring user confirmation for custom commands.

2. Race Condition - Detection Results 🏁

Location: EmbeddableTerminal.kt:281-297, TabbedTerminal.kt:619-627

onContextMenuOpenAsync is a synchronous lambda but calls the suspend function detectAll(). This appears to be missing a coroutineScope.launch wrapper.

Recommendation: Wrap the detection call in coroutineScope.launch.

3. False B->D Sequence Detection 🐛

Location: EmbeddableTerminal.kt:892-919

The OSC 133 listener only checks for the first B signal, but shells emit B->D for every command. If a user types another command before installation finishes, the wrong D signal will trigger the callback with an incorrect exit code.

Recommendation: Only register the listener after sending the command and unregister it after the first D.

Medium Priority Issues

4. Resource Leak - Process Not Always Killed

Location: AIAssistantDetector.kt:147-177

If process.waitFor() times out, the process may continue running until the outer finally block. Consider using the .use {} pattern for guaranteed cleanup.

5. Missing Validation - Custom Assistant IDs

Location: AIAssistantSettingsSection.kt

No validation prevents duplicate IDs, invalid characters, or empty IDs for custom assistants. This could overwrite built-in assistants or break serialization.

6. Hardcoded URLs - Install Scripts

Location: AIAssistantDefinition.kt:88, 121

Some install script URLs may be placeholders (especially for Codex, Gemini CLI, OpenCode). Consider marking which assistants are verified vs. conceptual.

Performance Considerations

• detectAll() spawns up to 4 processes on every context menu open (up to 20 seconds worst case with 5-second timeouts)
• Suggestion: Cache detection results for 30 seconds to avoid redundant checks

Test Coverage

Missing test scenarios for:

• Shell escaping with special characters
• Process timeout handling
• Duplicate custom ID prevention
• OSC 133 sequence correctness

Recommendations

High Priority:

1. Fix security issue: Validate/escape custom commands
2. Fix race condition: Wrap onContextMenuOpenAsync in coroutine scope
3. Fix false B->D detection: Only fire callback for the actual install command

Medium Priority:
4. Add custom assistant ID validation
5. Improve process cleanup
6. Cache detection results

Summary

This is a solid implementation with excellent architecture and UX. The main concerns are security (custom command validation), correctness (OSC 133 tracking), and compilation (suspend function usage).

Once these critical issues are addressed, this will be a great addition to BossTerm!

Recommendation: Request changes for security and correctness issues, then approve after fixes.

[claude]

Final Code Review: AI Assistant Context Menu Integration (#225)

This PR has undergone multiple rounds of review with several iterations addressing security concerns, resource management, and performance optimizations. Here's my final assessment:

---

Overall Assessment

Verdict: APPROVE - This is a well-architected feature with good separation of concerns, proper lifecycle management, and thoughtful UX design. Previous critical security issues have been addressed through multiple review cycles.

---

Strengths

1. Excellent Architecture: Clean separation between detection (AIAssistantDetector), command generation (AIAssistantLauncher), UI generation (AIAssistantMenuProvider), and settings
2. Robust Detection: Multi-strategy detection with assistant-specific paths, common directories, nvm scanning, which command, and shell-sourced which
3. Cross-Platform Support: Platform-aware installation with auto Node.js setup via nvm (Linux), Homebrew (macOS), and winget (Windows)
4. User Experience: On-demand detection (no polling overhead), embedded terminal for installation with live output, auto-close on success, error feedback with npm fallback, clear YOLO/Auto mode indicators
5. Extensibility: Support for both built-in and custom assistants
6. Documentation: Comprehensive KDoc comments and CLAUDE.md updates

---

Remaining Observations

1. Command Escaping Pattern (AIAssistantLauncher.kt:28-31)

The code uses escapeShellArg() for the working directory. The command variable uses user-configurable customCommand and customYoloFlag. This is acceptable risk because users explicitly configure these commands in settings with full awareness, the terminal already allows arbitrary command execution, and using ProcessBuilder would break shell features users expect.

Recommendation: Document in code comments that custom commands are user-controlled and intentionally not escaped.

2. Process Cleanup (AIAssistantDetector.kt:145-169)

The runCommand() method has a finally block with destroyForcibly() which ensures processes are cleaned up. The 5-second timeout is reasonable.

Minor suggestion: Consider logging detection failures in debug mode to help users troubleshoot PATH issues.

3. Thread Safety (AIAssistantDefinition.kt:135-141)

The _customAssistants variable is called from LaunchedEffect which runs on the main thread. Since settings changes happen on the UI thread and menu generation reads from the same thread, this is thread-safe in practice.

For future hardening: Consider adding @volatile if multi-threaded access is added.

4. Detection Caching

The current implementation uses on-demand detection (no polling). Each context menu open triggers fresh detection (~100-500ms with multiple assistants).

Suggestion for future PR: Add 30-second cache to reduce redundant detections.

5. Test Coverage

This PR adds 1,843 lines without tests. While not ideal, this is consistent with the existing codebase pattern.

Recommendation for future: Add integration tests for detection, command generation, and settings serialization.

---

Code Quality Checklist

• ✅ Follows Kotlin conventions
• ✅ Proper use of Compose lifecycle (remember, LaunchedEffect)
• ✅ Immutable data classes with @serializable
• ✅ Resource cleanup (process.destroyForcibly())
• ✅ KDoc documentation
• ✅ Security: escapeShellArg() for external input
• ✅ Error handling with try-catch blocks
• ✅ Platform-specific handling (Windows/macOS/Linux)
• ⚠️ No unit tests (consistent with project pattern)

---

Conclusion

This PR represents a significant feature addition with excellent engineering quality. The architecture is extensible, the UX is polished, and security concerns have been appropriately addressed through the review process.

Recommendation: APPROVE and MERGE

The identified observations are minor and can be addressed in follow-up PRs if needed. This feature is ready for production use.

Great work on this implementation! The multi-round review process has resulted in a solid, well-thought-out feature.

---

Reviewed via Claude Code - 2026-01-07

[claude]

Code Review - PR 226: AI Assistant Context Menu Integration

This is a substantial and well-architected feature addition with good separation of concerns.

STRENGTHS:

• Excellent modular architecture (Definition, Detector, Launcher, MenuProvider, InstallDialog, CommandInterceptor)
• Clean data model separation between definitions and user configuration
• Reactive design with StateFlow
• Good documentation with KDoc comments
• Platform-specific installation support (macOS Homebrew, Linux nvm, Windows winget)

CRITICAL SECURITY ISSUE - MUST FIX:

1. Command Injection Vulnerabilities in AIAssistantLauncher.kt:28 and AIAssistantInstallDialog.kt:323
   • workingDirectory is escaped but command is NOT
   • Attack scenario: customCommand = 'claude; rm -rf /' or malicious customYoloFlag
   • Recommendation: Validate/sanitize custom commands and flags or escape entire command

OTHER ISSUES TO FIX:
2. Process Resource Leaks (AIAssistantDetector.kt:169-192)

• exitValue() may throw IllegalThreadStateException if process times out
• Check if completed before calling exitValue()

3. Process Cleanup (AIAssistantLauncher.kt:205-218)

   • No timeout on whichProcess.waitFor() could hang indefinitely

4. Buffer Synchronization (AICommandInterceptor.kt:44-80)

   • inputBuffer accessed from multiple threads without synchronization

5. Null Safety (AIAssistantLauncher.kt:76-78)

   • npm package parsing edge case if installCommand doesn't start with expected prefix

MINOR ISSUES:

• Hardcoded curl pipe bash (security risk, add warning)
• Code duplication between EmbeddableTerminal and TabbedTerminal
• Shell compatibility issues with -l flag
• Missing test coverage for security-critical paths

RATING: 7.5/10 (would be 9/10 after security fixes)

Great architecture overall! Please address the command injection vulnerability before merging.

[claude]

Pull Request Review: AI Assistant Context Menu Integration (#225)

This is an impressive feature addition that brings AI coding assistant integration to BossTerm. The implementation is well-structured with good separation of concerns.

Strengths

Architecture & Design

• Clean separation of concerns: Detection, launching, menu generation, and UI are properly separated
• Extensible design: The AIAssistantDefinition system makes it easy to add new assistants
• On-demand detection: Smart move from polling to on-demand detection - reduces CPU usage
• Graceful degradation: OSC 133 command interception works only when shell integration is configured
• Shared composable pattern: rememberAIAssistantState() eliminates code duplication

Code Quality

• Good error handling with process cleanup in finally blocks
• Proper resource management with destroyForcibly() in cleanup paths
• Cross-platform support for macOS, Linux, and Windows
• Comprehensive KDoc comments

User Experience

• Multi-strategy detection with multiple fallback methods
• Installation flexibility with script and npm methods
• Auto-run after install with fresh login shell
• Visual feedback with embedded terminal showing live progress

Issues & Concerns

1. Security: Curl Pipe to Bash (HIGH) - Lines 88, 121 in AIAssistantDefinition.kt
Piping curl output directly to bash is a security anti-pattern. Recommendation: Download to temp file first or add security warning dialog, or default to npm which has better security.

2. Resource Leak: Process Stream Not Closed (MEDIUM) - Lines 33-40 in VersionControlMenuProvider.kt
ProcessBuilder.start() opens streams that must be closed. Use ProcessBuilder.Redirect.DISCARD for stdout/stderr and add timeout with destroyForcibly in finally block (like AIAssistantDetector.kt does correctly).

3. Potential Command Injection (MEDIUM) - Line 325 in AIAssistantInstallDialog.kt
While single-quote escaping is attempted, complex commands with backticks or other metacharacters could be problematic. Use proper shell escaping library or validate input more strictly.

4. Debug Print Statement (LOW) - Line 175 in AIAssistantInstallDialog.kt
Debug println left in production code. Remove or convert to LOG.debug().

5. Silent Error Swallowing (LOW) - Lines 182, 298
URL opening failures are logged to stdout or silently ignored. Show toast notification with actionable error message.

Suggestions

1. Rate limiting for detection - cache results for 30-60 seconds to avoid spawning many processes
2. Installation progress indicator for scripts without output
3. Version Control menu could group less-common commands under More submenus
4. Add automated tests (unit tests for AICommandInterceptor, integration tests for detection)

Performance

Good: On-demand detection, StateFlow caching, Redirect.DISCARD
Could improve: Detection spawns 20-40 processes per context menu open with 4 assistants

Overall Assessment

This is high-quality work with thoughtful design decisions. The feature adds significant value.

Recommendation: Approve with minor fixes

Must-Fix Before Merge:

1. Fix process stream leak in VersionControlMenuProvider.kt
2. Remove debug println statement
3. Add security warning for curl pipe bash or use npm by default

Nice-to-Have (follow-up PRs):

1. Rate limiting for detection
2. Improved error feedback
3. Automated tests
4. More robust shell escaping

Great job on this feature!

[claude]

Code Review: AI Assistant Integration (#225)

This is an impressive and well-architected feature implementation. The code quality is high, with clear separation of concerns and thoughtful design.

---

Strengths

Architecture

• Clean separation of concerns: Detector, Launcher, MenuProvider, Interceptor are well-isolated
• Reactive state management with StateFlow
• Proper LaunchedEffect and DisposableEffect usage
• Graceful OSC 133 degradation
• Platform-specific handling (macOS/Linux/Windows)

Code Quality

• Comprehensive KDoc documentation
• Proper Kotlin nullability
• Resource cleanup with finally blocks
• Good use of data classes and immutable state

User Experience

• Multi-strategy detection (paths → nvm → which → shell-sourced)
• Installation fallback (script with npm backup)
• Auto Node.js installation
• Clean context menu UI with YOLO indicators

---

Critical Issues

1. Command Injection (AIAssistantInstallDialog.kt:325) - HIGH

Line 325 has potential shell metacharacter vulnerability. While single quotes are escaped, shell metacharacters like dollar-parens or backticks could execute arbitrary commands.

Fix: Escape all special chars (dollar, backtick, backslash) or write to temp script file.

2. Unvalidated Install Commands (AIAssistantDefinition.kt) - MEDIUM

Custom assistants allow arbitrary installCommand values piped to bash. Add validation for safe patterns only (curl from known domains, npm, brew) and prominent security warning in UI.

---

Major Issues

3. Race Condition (AIAssistantMenuProvider.kt:45)

StateFlow might be read mid-update during detectAll(). Add detection-in-progress state.

4. Process Timeout Logging (AIAssistantDetector.kt:182)

Add logging when waitFor times out to aid debugging.

---

Minor Issues

5. Menu item generation logic duplicated (AIAssistantMenuProvider.kt:77-114)
6. Hard-coded installation messages (AIAssistantInstallDialog.kt:318,328)
7. No assistant ID uniqueness validation (AIAssistants.kt:140)
8. Missing unit tests for command interception, detection, config logic

---

Performance

9. Detection on Every Menu Open

Each context menu triggers detection for ALL assistants (100-500ms). Consider:

• Cache with 5min TTL
• Background coroutine with stale data
• Per-assistant detection

---

Recommendations

Must Fix: 1, 2, 3
Should Fix: 4, 6, 7
Can Fix Later: 5, 8, 9, 10

---

Overall: 8.5/10

High-quality work with excellent architecture. Security issues are the only blockers. Feature design is thoughtful and maintainable. OSC 133 integration is clever. Great job!

[claude]

Pull Request Review: AI Assistant Integration

This is a substantial and well-architected feature with excellent separation of concerns. Overall rating: 4/5 stars.

Critical Issues (Must Fix)

1. Command Injection in AIAssistantLauncher.kt:28 - The command variable is not escaped before execution. If config.customCommand contains shell metacharacters, it could execute arbitrary code.

2. Shell Injection in AIAssistantDetector.kt:129 - Command parameter is interpolated without escaping in shell -c context.

3. Unbounded Buffer in AICommandInterceptor.kt - inputBuffer can grow without limit. Add size cap (e.g., 1000 chars).

Medium Priority Issues

4. URLs Need Verification - Several assistant URLs may be outdated (Codex deprecated, others unverified)
5. Silent URL Failures - Errors only printed to console
6. Race Condition - _customAssistants accessed without synchronization
7. Code Duplication - Menu generation duplicated in EmbeddableTerminal and TabbedTerminal

Strengths

• Excellent multi-strategy detection (5 approaches)
• Proper resource management with timeouts and finally blocks
• Cross-platform Node.js installation support
• Clean Kotlin/Compose patterns
• Good coroutine usage

Security Concerns

• YOLO mode enabled by default - consider opt-in or warning dialog
• Command injection vulnerabilities need fixing before merge

Test Coverage

Missing unit tests for:

• Command escaping
• Command extraction
• Detection strategies
• Edge cases

Recommendation

Fix the 3 critical security issues, verify URLs, then merge. The architecture is solid and this will be a great addition once security is addressed.

Total Changes: 3,594 additions, 24 deletions across 25 files

[claude]

Pull Request Review: AI Assistant Integration (#225)

Overview: This PR adds comprehensive AI assistant integration with context menu detection, installation dialogs, and command interception. Well-architected with approximately 3600 lines of new code.

STRENGTHS:

1. Excellent Architecture - Clean module separation with AIAssistantDefinition, AIAssistantDetector, AIAssistantLauncher, AICommandInterceptor. Thread-safe caching with AtomicReference.

2. Robust Detection Strategy - Multi-strategy detection including assistant-specific paths, common paths, nvm glob support, which command, and shell-sourced which. Excellent process cleanup with destroyForcibly in finally block.

3. Smart Command Interception - Uses OSC 133 shell integration, only intercepts when isConfirmedNotInstalled, handles Ctrl+C/U/W properly, pipe detection works correctly.

4. Platform-Aware Installation - Auto Node.js install for macOS (Homebrew), Linux (nvm), Windows (winget).

5. Great UX - Embedded terminal with live install output, auto-close on success, confirmation dialogs, YOLO mode enabled by default.

CRITICAL SECURITY ISSUES:

1. Command Injection Vulnerability (AIAssistantLauncher.kt:234) - escapeShellArg NOT used for custom commands in settings or npm package names. Attack vectors include malicious custom commands or npm packages. Recommendation: Validate custom commands with whitelist, sanitize npm packages, use ProcessBuilder arrays.

2. OSC 133 Race Condition (EmbeddableTerminal.kt:1010-1027) - Fast command completion may arrive before listener registration. Recommendation: Use timeout-based approach or track registration timestamp.

MEDIUM PRIORITY ISSUES:

3. Resource Leak in VersionControlMenuProvider - Lines 50-58, 89-98, 106-140 create processes without try-finally cleanup.

4. Inconsistent Error Handling - Uses println instead of logging framework, debug prints scattered.

5. Hardcoded UI Values - Colors and sizes should be extracted to constants.

MINOR ISSUES:

6. Missing Test Coverage - No tests for AIAssistantDetector, AICommandInterceptor parsing, or shell escaping (security-critical).

7. Git Branch Fetching Blocks UI - Could lag on repos with many branches.

RECOMMENDATIONS:

Before Merge (High Priority):

• Fix command injection vulnerability
• Add process cleanup with try-finally
• Add security tests for shell escaping

Post-Merge:

• Add unit tests for core logic
• Extract UI theme constants
• Add git branch fetch timeout
• Use structured logging

CONCLUSION:

Well-designed feature with excellent architecture and UX. Main concerns are security (command injection risk), resource leaks (process cleanup), and testing (no automated tests for security logic).

Recommendation: Request changes for security fixes, then approve.

Great work overall! This is a compelling feature that enhances developer experience significantly.

[claude]

Code Review: AI Assistant Integration (#225)

This is an excellent implementation with proper architecture and UX. The code quality is high and follows project patterns.

Strengths

• Clean separation of concerns across detection, launching, and installation
• Multi-strategy detection with graceful fallbacks
• Excellent documentation with KDoc comments
• Proper error handling and resource management
• Non-blocking detection on context menu open
• Smart OSC 133 integration for command interception

Issues Found

1. Command Injection Risk - HIGH PRIORITY

Location: AIAssistantInstallDialog.kt:324-325

The escape function only handles single quotes. The command is vulnerable to injection via backtick, dollar sign, backslash.

Recommendation: Validate commandToRunAfter more strictly before executing. Consider checking it matches only the assistant command name.

2. Resource Leak - MEDIUM PRIORITY

Location: AIAssistantLauncher.kt:208

The openUrlOnLinux method doesn't ensure process termination if waitFor hangs, unlike AIAssistantDetector which has proper timeout and finally block cleanup.

Recommendation: Apply same pattern - use waitFor with timeout and destroyForcibly in finally block.

3. Race Condition - LOW PRIORITY

Location: TabbedTerminal.kt:401

mutableSetOf is not thread-safe but accessed from LaunchedEffect coroutines.

Recommendation: Use Collections.synchronizedSet or check tab.aiCommandInterceptor != null exclusively.

4. Command Parsing Gap

Location: AICommandInterceptor.kt:123-139

extractCommandName handles pipes but not other operators like &&, semicolon, ||.

Recommendation: Split on all shell operators to detect AI commands in compound statements.

Suggestions

• Document/validate hardcoded install URLs
• Add UI indicator for OSC 133 shell integration status
• Consider adding unit tests for edge cases

Security Summary

• Command injection: HIGH priority - needs fix before merge
• Process leak: MEDIUM priority - should fix
• Race condition: LOW priority - can address in follow-up

Conclusion

Well-implemented feature with excellent architecture. Fix the command injection vulnerability before merge, address process leak, other issues can be follow-up PRs.

Overall Rating: 8.5/10 (would be 9.5/10 after security fix)

Great work!

[claude]

Code Review: AI Assistant Integration (#225)

This is an impressive feature implementation with well-thought-out architecture.

✅ Strengths

Architecture & Design

• Multi-strategy detection (5 levels) in AIAssistantDetector.kt is robust
• Smart move replacing polling with on-demand detection via onContextMenuOpenAsync
• Clean separation between detection, launching, menu provision, and UI
• Proper resource cleanup with DisposableEffect and finally blocks

User Experience

• OSC 133-based command interception is clever with isConfirmedNotInstalled() preventing false positives
• Installation dialog with embedded terminal provides excellent real-time feedback
• npm fallback on script failure is great UX
• Running original command after install using fresh login shell (lines 319-326) is a nice touch

VCS Integration

• Context-aware menus dynamically showing options based on git repo status
• Smart fallback to /proc/pid/cwd when OSC 7 unavailable

I will post detailed issues and suggestions in follow-up comments.

[claude]

🔍 Critical Issues

Issue #1: Process Resource Leak in AIAssistantDetector.kt (lines 169-192)

The current code calls process.exitValue() even when waitFor() times out, which throws IllegalThreadStateException:

return try {
    val completed = process.waitFor(5, TimeUnit.SECONDS)
    completed && process.exitValue() == 0  // exitValue throws if !completed

Fix: Check completed before calling exitValue():

return try {
    val completed = process.waitFor(5, TimeUnit.SECONDS)
    if (completed) process.exitValue() == 0 else false

Issue #2: Security - Command Injection Risk in AIAssistantLauncher.kt

Lines 28-32: workingDirectory is escaped but command (includes user customCommand) is NOT escaped.

Example exploit: User sets customCommand: "claude; rm -rf ~" → executes both commands.

Recommended: Document that custom commands execute as-is and add warning in settings UI.

[claude]

🔍 High Priority Issues

Issue #3: Missing Timeout in VersionControlMenuProvider.kt

isCommandInstalled() (lines 48-57) has no timeout on waitFor() and no destroyForcibly() cleanup.

Fix: Add timeout and cleanup pattern:

private fun isCommandInstalled(command: String): Boolean {
    val process: Process
    try {
        process = ProcessBuilder("which", command)
            .redirectOutput(ProcessBuilder.Redirect.DISCARD)
            .redirectError(ProcessBuilder.Redirect.DISCARD)
            .start()
    } catch (e: Exception) {
        return false
    }
    
    return try {
        val completed = process.waitFor(5, TimeUnit.SECONDS)
        if (completed) process.exitValue() == 0 else false
    } catch (e: Exception) {
        false
    } finally {
        if (process.isAlive) {
            process.destroyForcibly()
        }
    }
}

Issue #4: Same problem in checkIsGitRepo() and fetchGitBranches()

Lines 89-98 and 106-139 both use waitFor() without timeout. Apply same fix pattern.

[claude]

💡 Medium Priority Issues & Suggestions

Issue #5: Debug println left in AIAssistantInstallDialog.kt:175 - use LOG.debug() or remove

Issue #6: openUrl() methods (lines 158-231) swallow exceptions with println - use proper logger

Issue #7: AICommandInterceptor.inputBuffer accessed from multiple threads - consider synchronized block

Performance:

• AIAssistantDetector.getAssistantSpecificPaths() checks 3-4 paths per assistant - consider caching
• VersionControlMenuProvider.fetchGitBranches() runs synchronously on menu open - could add latency in large repos

Code Quality:

• Magic numbers (700.dp, 450.dp, MAX_BRANCHES=10) should be named constants
• Missing tests for detection logic, command interception, install commands
• Missing KDoc for buildCheckoutSubmenu(), buildSwitchSubmenu(), buildGhMenu()

[claude]

🎯 Test Coverage Needed

Before merging, please verify:

1. Command Injection: Try setting customCommand to "claude; echo hacked" in settings
2. Process Cleanup: Run detection in loop and check for zombie processes (ps aux | grep which)
3. OSC 133 Edge Cases: Test command interception without shell integration
4. Timeout Behavior: Simulate slow which command and verify 5-second timeout
5. Branch Fetching: Test in large repos (100+ branches)
6. npm Fallback: Trigger script install failure and verify npm fallback flow

📊 Overall Assessment

Score: 8.5/10

Excellent work with thoughtful architecture and great UX! Main concerns:

1. Process resource management (timeout + cleanup in VCS provider)
2. Command injection documentation/validation
3. Missing timeout handling

Once critical issues #1-4 are addressed, this will be production-ready.

Recommendation: Request changes for critical issues, then approve after verification.

[claude]

Code Review: AI Assistant Integration (#225)

Summary

This is a well-architected feature that adds comprehensive AI assistant integration to BossTerm. The implementation is clean, follows good design patterns, and integrates smoothly with the existing codebase. Overall quality is excellent.

✅ Strengths

Architecture & Design

• Clean separation of concerns: Each component has a single, well-defined responsibility (Detection, Launching, Menu generation, Command interception, UI)
• Immutable data classes: Good use of Kotlin data classes for configuration and state
• Reactive state management: Proper use of StateFlow for installation status tracking
• Graceful degradation: Command interception requires OSC 133 but degrades gracefully when not configured
• Extensibility: Custom assistant support built-in from the start

Code Quality

• Excellent documentation: Every class, function, and parameter is well-documented with clear KDoc comments
• Consistent error handling: try-catch blocks in appropriate places with proper cleanup (e.g., ProcessBuilder cleanup in detector)
• Resource management: Proper process cleanup in AIAssistantDetector.runCommand() with timeout and force-destroy fallback
• Type safety: Good use of sealed classes and enums (InstallDialogState, InstallMethod)

User Experience

• Smart detection strategy: Multi-layer detection (specific paths → common paths → nvm paths → which → shell-sourced which)
• Auto Node.js installation: Excellent UX - automatically installs Node.js if needed for npm packages
• Script + npm fallback: Install dialog tries script first, offers npm on failure
• Post-install command execution: Clever feature to run the original command after install completes
• YOLO mode by default: Sensible default for AI assistants to enable auto-approval

🔍 Code Review Findings

Critical Issues

None found! 🎉

Medium Priority Issues

1. Security: Command injection risk in AIAssistantLauncher.getLaunchCommand() (line 28)

   • The workingDirectory is escaped via escapeShellArg(), which is good
   • However, I don't see the escapeShellArg() function definition in the files reviewed
   • Recommendation: Verify that escapeShellArg() properly handles shell metacharacters, quotes, and newlines
   • Consider using ProcessBuilder instead of string concatenation for cd commands

2. Race condition in command interception (AICommandInterceptor.kt, line 100)

   • Uses isConfirmedNotInstalled() to avoid false positives when detection hasn't run
   • However, if detection runs AFTER user starts typing but BEFORE they press Enter, behavior could be inconsistent
   • Recommendation: Consider tracking detection timestamp and only intercepting if detection is recent (<5s)

3. Process leak potential in AIAssistantDetector (line 169-192)

   • Good: Uses timeout and destroyForcibly() in finally block
   • Potential issue: If waitFor() throws exception before process completes, could leave zombie processes
   • Recommendation: Consider using process.destroyForcibly().waitFor() in catch block to ensure cleanup

Low Priority / Style Issues

4. Hardcoded timeouts (AIAssistantDetector.kt, line 182)

   • 5-second timeout for which commands might be too long for UI responsiveness
   • Recommendation: Consider reducing to 1-2 seconds or making configurable

5. Empty list fallback in getAssistantSpecificPaths() (line 159)

   • Returns empty list for unknown assistants, which is fine but could log a warning
   • Recommendation: Log at debug level when no specific paths are configured

6. Duplicate code in EmbeddableTerminal.kt and TabbedTerminal.kt

   • The AI install confirmation dialog code is duplicated (~80 lines each)
   • Recommendation: Consider extracting to a shared composable (though this is a minor DRY issue)

7. Magic strings for OSC 133 parsing (BossEmulator1.kt, line 475)

   • Debug log message references OSC 133 types as raw strings
   • Recommendation: Consider enum or constants for OSC 133 command types

Performance Considerations

8. Context menu async detection (EmbeddableTerminal.kt, line 377)

   • detectAll() runs on every context menu open, which spawns multiple processes
   • With 4 built-in assistants × 5 detection strategies, this could be slow
   • Good: Detection results are cached in detectionResultsHolder
   • Recommendation: Consider caching detection results for 30-60 seconds to avoid repeated process spawns

9. Version Control status refresh (EmbeddableTerminal.kt, line 386)

   • Refreshes VCS status on every context menu open
   • Recommendation: Consider debouncing or caching VCS status

🛡️ Security Review

Positive Findings

• ✅ Proper shell escaping for working directory in launch commands
• ✅ Installation commands use official sources (curl from claude.ai, npm registry)
• ✅ No arbitrary code execution from user input
• ✅ Process cleanup prevents resource exhaustion

Concerns

• ⚠️ Install script execution: Scripts piped from URLs (curl | bash) inherently risky

  • Mitigated by: User must explicitly trigger installation via dialog
  • Mitigated by: Scripts come from official sources (claude.ai, opencode.ai)
  • Recommendation: Consider adding checksum verification for install scripts

• ⚠️ Auto-approval flags enabled by default: YOLO mode enables --dangerously-skip-permissions

  • Risk: AI assistants can make changes without prompts
  • Mitigated by: User must explicitly launch the assistant
  • Mitigated by: Can be disabled in settings
  • Recommendation: Consider showing a one-time warning when first launching with YOLO mode

🧪 Test Coverage

The PR description mentions a test plan, but I don't see automated tests for:

• AI command interception logic (parsing, word deletion, Ctrl+U handling)
• Detection strategy fallback behavior
• Install dialog state transitions

Recommendation: Add unit tests for:

• AICommandInterceptor.extractCommandName() - edge cases with pipes, quotes, etc.
• AIAssistantDetector.checkInstallation() - mock filesystem and process execution
• InstallDialogState transitions

📝 Documentation

• ✅ CLAUDE.md updated with comprehensive feature documentation
• ✅ Shell integration setup instructions included
• ✅ API documentation in docs/embedding.md and docs/tabbed-terminal.md
• ✅ Inline KDoc comments throughout

Recommendation: Consider adding troubleshooting section for:

• What to do if detection fails
• How to manually configure custom assistant paths
• OSC 133 verification steps

🎯 Suggestions for Future Enhancement

1. Assistant marketplace: Allow importing assistant definitions from a JSON URL
2. Detection caching: Persist detection results to avoid repeated process spawns
3. Installation progress: Show percentage/download speed in install dialog
4. Assistant groups: Category organization (Code Gen, Testing, DevOps)
5. Telemetry: Track which assistants are most used (with user opt-in)

✅ Approval Recommendation

This PR is approved with minor recommendations for follow-up improvements. The code quality is excellent, architecture is sound, and the feature is well-integrated.

Before Merge

• Verify escapeShellArg() implementation handles all shell metacharacters
• Consider adding unit tests for command interception logic
• Consider adding one-time YOLO mode warning

Post-Merge (Optional)

• Add detection result caching (30-60s TTL)
• Extract shared install dialog code
• Add install script checksum verification

Great work on this feature! 🚀

[claude]

Code Review: AI Assistant Integration (#225)

Great work on this comprehensive feature! The implementation is well-structured and follows good patterns.

Strengths

Architecture & Design:

• Clean separation of concerns: Detection, Launcher, MenuProvider, and CommandInterceptor are well-separated
• Extensibility: Support for both built-in and custom assistants
• Graceful degradation: OSC 133 integration works but doesn't break when missing
• Reusable components: Shared AIInstallDialogHost reduces duplication

Implementation Quality:

• Platform awareness: Platform-specific installation (Homebrew/macOS, nvm/Linux, winget/Windows)
• Multi-strategy detection: Fallback chain from specific paths to common paths to nvm to which to shell-sourced which
• Proper resource cleanup: Process cleanup with destroyForcibly() in finally block
• Thread safety: AtomicReference used correctly for detection results

User Experience:

• YOLO mode by default: Smart defaults for auto-approve flags
• Fallback installation: Script to npm to auto Node.js install
• Rich UI: Embedded terminal shows live installation output
• Command interception: Detects AI commands seamlessly

Critical Issues

1. Resource Leak in AIAssistantLauncher.kt (Line 208)

   • Problem: Missing timeout in ProcessBuilder.waitFor() can hang indefinitely
   • Impact: Browser detection on Linux could freeze
   • Fix: Add timeout: whichProcess.waitFor(2, TimeUnit.SECONDS)

2. Command Injection in AIAssistantLauncher.kt (Lines 233-234)

   • Problem: Shell escaping doesn't handle newlines
   • Impact: Directory names with newlines could break out of quotes
   • Fix: Escape special characters including newlines

3. Race Condition in OSC 133 Listener (EmbeddableTerminal.kt:1014-1022)

   • Problem: commandStarted flag never resets
   • Impact: After first command, all subsequent onCommandFinished calls fire callback
   • Fix: Reset flag in onCommandFinished

Minor Issues

4. Missing Ctrl+D handling in AICommandInterceptor.kt
5. VersionControlMenuProvider.kt (637 lines) seems unrelated - consider separate PR
6. Clear detection results when feature disabled

Suggestions

Test Coverage:

• No unit tests for AICommandInterceptor, AIAssistantDetector, AIAssistantLauncher
• Add tests for command extraction, detection fallback, shell escaping

Documentation:

• Excellent CLAUDE.md updates
• Missing inline comments for OSC 133 handling logic

Error Handling:

• Silent failures in openUrl() should log or notify
• No timeout on install dialog terminal

Verdict

Status: Approve with Changes Requested

Fix critical issues:

1. Resource leak (timeout on waitFor)
2. Command injection (shell escaping)
3. Race condition (OSC 133 reset)
4. Add unit tests
5. Consider splitting VCS integration

Metrics:

• Lines: +3,796 / -26
• Files: 26 changed
• Test Coverage: 0%

Great architecture and UX! Address the critical issues and this will be ready to merge.

[claude]

Code Review: AI Assistant Context Menu Integration (#225)

This is a substantial and well-architected feature addition. Here's my comprehensive review:

✅ Strengths

1. Architecture & Design

• Excellent separation of concerns across AIAssistantDefinition, AIAssistantDetector, AIAssistantLauncher, AIAssistantMenuProvider, and AICommandInterceptor
• Reusable patterns with AIInstallDialogParams shared data class
• Composability: AIInstallDialogHost handles common installation dialog logic cleanly
• Version Control integration uses unified approach with AI assistants

2. Robust Detection

AIAssistantDetector uses 5-strategy approach: assistant-specific paths, common paths, nvm paths, which command, shell-sourced which. Good process cleanup with destroyForcibly().

3. Smart Command Interception

• OSC 133 integration for shell state tracking
• False positive prevention with isConfirmedNotInstalled()
• Robust input tracking (backspace, Ctrl+C/U/W)
• Pipe awareness in extractCommandName()

4. Installation UX

• Embedded terminal with live output
• npm fallback on script failure
• Post-install command execution with shell sourcing
• Clear visual feedback

5. Settings Integration

• Per-assistant config with YOLO mode
• Full CRUD for custom assistants
• Real-time detection with manual refresh

🔍 Issues & Recommendations

1. Security: Command Injection Risk ⚠️

Location: AIAssistantInstallDialog.kt:320
Severity: HIGH - Must fix before merge

Escaping only handles single quotes. If user types 'claude; rm -rf ~', the rm will execute.

2. Resource Leak: Process Not Destroyed

Location: VersionControlMenuProvider.kt:57-63
Severity: MEDIUM - Should fix before merge

No timeout or destroyForcibly() if waitFor() hangs. Apply same pattern as AIAssistantDetector.runCommand().

3. Race Condition: Detection Results

Location: EmbeddableTerminal.kt:240, TabbedTerminal.kt:239
Severity: LOW - UX issue

onContextMenuOpenAsync sets results, customContextMenuItemsProvider reads them. Menu may show stale status if opened before async completes.

4. Missing Input Validation

Location: AIAssistantSettingsSection.kt
Severity: LOW

No validation for empty fields, invalid characters, or duplicate IDs in custom assistant dialog.

5. Overly Broad Catch Block

Location: VersionControlMenuProvider.kt:683-685
Severity: LOW

Swallows all exceptions. Should catch specific IOException, URISyntaxException.

🎯 Performance

Positive:

• No polling (on-demand detection)
• Cached status with StateFlow
• Coroutine usage on Dispatchers.IO

Improvement: fetchGitBranches() runs on every menu open. For repos with 1000s of branches, consider limiting to 50 initially.

🔒 Security

Approved:

• No credential storage
• Hardcoded install commands
• No network requests

Concerns:

• Command injection in commandToRunAfter
• curl | bash pattern (industry standard but risky)

🧪 Test Coverage Gaps

Missing unit tests for:

• AICommandInterceptor (input buffer edge cases)
• AIAssistantDetector (path resolution)
• VersionControlMenuProvider (branch parsing)
• Settings serialization

📝 Documentation

Excellent KDoc comments and CLAUDE.md updates. Minor: add @throws tags.

🚀 Overall: APPROVE with Minor Changes

High-quality work with excellent design. Issues are mostly minor except command injection.

Must Fix Before Merge:

1. Fix command injection in commandToRunAfter
2. Add timeout to VersionControlMenuProvider.isCommandInstalled()

Should Fix Soon:
3. Fix detection results race condition
4. Add input validation for custom assistants

Nice to Have:
5. Add unit tests
6. Split VersionControlMenuProvider

Stats: +4,010 / -26 lines, 6 new files, 20 modified files

Great work! 🎉

[claude]

Code Review: AI Assistant Integration (#225)

This is a comprehensive feature implementation that adds AI coding assistant integration to BossTerm. Overall, the code quality is excellent with solid architecture and attention to detail.

---

Blocking Issues (Must Fix Before Merge)

1. Security: Hardcoded Auto-Approval Flags (HIGH PRIORITY)

Location: AIAssistantDefinition.kt lines 96-98, 107, 118, 130

Problem: YOLO flags are enabled by default (yoloEnabled: true), allowing AI assistants to execute arbitrary commands without user approval. This includes file deletions, git force pushes, package installations, etc.

Recommendation: Change default to yoloEnabled: false and show a clear warning dialog when user enables YOLO mode in settings.

2. Security: Unvalidated Install Commands

Location: AIAssistantDefinition.kt lines 98, 144-145

Problems: curl | bash pattern and sudo commands without warnings.

Recommendation: Show confirmation dialogs before running sudo or curl | bash commands. Add platform detection for install commands.

3. Debug Logging in Production

Location: BossEmulator1.kt line 475, BossTerminal.kt line 404, EmbeddableTerminal.kt lines 1046, 1058

Problem: println statements bypass logging framework and clutter stdout.

Recommendation: Replace all println(DEBUG:...) with LOG.debug() or remove before merging.

---

Recommended Improvements (Should Fix)

4. Performance: detectAll() blocks context menu open (100-500ms on slow systems). Run detection in background every 30s instead of on menu open.

5. Error handling: Add timeout fallback for OSC 133 events to prevent indefinite blocking if shell integration breaks.

6. Code organization: Split 721-line VersionControlMenuProvider.kt into smaller files (GitMenuProvider, GitHubMenuProvider, VCSDetector).

7. Race condition: Command interception could fail if detection is still running when user types a command.

---

Strengths

Architecture & Design

• Clean separation of concerns with dedicated AI modules
• Extensible design supporting both built-in and custom assistants
• Proper state management with Compose and Kotlin Flow
• Thread-safe detection using AtomicReference

Command Interception

• Smart OSC 133 integration with graceful fallback
• Proper input tracking (Ctrl+C, Ctrl+U, Ctrl+W, backspace, pipes)
• Avoids false positives with isConfirmedNotInstalled check

Version Control Integration

• Context-aware Git menus adapting to repo state
• Dynamic branch lists with current branch highlighted
• Smart gh CLI handling (detects if gh repo set-default needed)
• Excellent UX with disabled/enabled menu items

Settings UI

• Comprehensive per-assistant configuration
• Clear visual hierarchy for built-in vs custom assistants
• Install command display with copy-to-clipboard
• Manual detection refresh button

---

Test Coverage

Missing Tests:

• Unit tests for AICommandInterceptor (input parsing, state transitions)
• Unit tests for AIAssistantDetector (path resolution, detection logic)
• Integration tests for OSC 133 command state tracking
• Edge case tests: empty git repos, detached HEAD, no remote

Recommendation: Add tests for at least the core interception logic.

---

Final Rating: 8.5/10

This is a well-architected feature with excellent user experience and solid technical implementation. The main concerns are around security defaults (YOLO mode enabled by default) and debug logging cleanup. Once the blocking issues are addressed, this is ready to merge.

The integration with OSC 133, the context-aware menus, and the extensible assistant system are all well-executed. Great work overall!