feat: Use external editor for command editing instead of inline editor

[alvinunreal]

• Replaces readline inline editor with external editor (Git-like approach)
• Uses $EDITOR or $VISUAL environment variables
• Falls back to common editors (vim, vi, nano, emacs) if env vars not set
• Creates temporary file for editing with command pre-filled
• Properly handles stdin/stdout/stderr for terminal editor support

Fixes #76

[alvinunreal]

@claude fix ci

[claude]

Claude finished @alvinunreal's task —— View job

---

✅ CI Fixed!

Todo List:

• Check CI status and identify failing checks
• Review the code changes in internal/confirm.go
• Fix the identified CI issues
• Commit and push the fixes

Summary:

Fixed the golangci-lint errcheck issues in internal/confirm.go:

1. ✅ Line 91: Added proper error handling for os.Remove in defer
2. ✅ Line 96: Handled tmpFile.Close error with _ =
3. ✅ Line 99: Added explicit error check for tmpFile.Close

The changes have been pushed to the branch and CI should now pass.

--- |

[claude]

Code Review for PR #77: Use external editor for command editing

Summary

This PR successfully replaces the inline readline editor with an external editor approach (similar to Git), addressing issue #76. The implementation is clean and follows good practices overall.

✅ Strengths

1. Good fallback logic: The code properly checks EDITOR and VISUAL environment variables before falling back to common editors
2. Proper resource cleanup: Uses defer for temporary file removal
3. Terminal handling: Correctly sets stdin/stdout/stderr for the external editor
4. Error handling: Most error cases are properly handled with informative messages

🔍 Code Quality & Best Practices

1. Temporary file permissions (Security)

The temporary file is created with default permissions. Consider setting more restrictive permissions:

tmpFile, err := os.CreateTemp("", "tmuxai-edit-*.sh")
if err != nil {
    // ...
}
// Set restrictive permissions immediately after creation
os.Chmod(tmpFile.Name(), 0600)

2. Resource leak prevention

In internal/confirm.go:94, consider using defer tmpFile.Close() immediately after successful creation to ensure the file handle is always closed:

tmpFile, err := os.CreateTemp("", "tmuxai-edit-*.sh")
if err != nil {
    // ...
}
defer tmpFile.Close()

🐛 Potential Issues

1. Windows compatibility

The fallback editors list (vim, vi, nano, emacs) is Unix-centric. Consider adding Windows editors:

editors := []string{"vim", "vi", "nano", "emacs"}
if runtime.GOOS == "windows" {
    editors = []string{"notepad", "notepad++"}
}

2. Empty editor scenario

Good error handling when no editor is found, providing clear user guidance.

⚡ Performance Considerations

• The implementation is efficient with minimal overhead
• File I/O is necessary for external editor integration
• Consider caching the editor lookup result to avoid repeated exec.LookPath calls if this function is called frequently

🔒 Security Considerations

1. Command injection: The temporary file name pattern is safe from injection
2. File permissions: As mentioned above, consider setting restrictive permissions (0600)
3. Race condition: There's a small window between file creation and removal where the file exists with the command content. The restrictive permissions would mitigate this

🧪 Test Coverage

Missing tests: The confirm.go file lacks a corresponding test file. Consider adding tests for:

• Editor selection logic (EDITOR/VISUAL env vars)
• Fallback editor detection
• Temporary file creation and cleanup
• Error handling paths
• Mock external editor execution

Example test structure:

func TestConfirmedToExecFn_EditCommand(t *testing.T) {
    // Test EDITOR env var
    // Test VISUAL env var  
    // Test fallback to common editors
    // Test no editor found scenario
    // Test file operations
}

📝 Additional Suggestions

1. Consider adding a configuration option to disable external editor and keep inline editing as a fallback
2. Add logging for debugging which editor was selected
3. Consider adding a timeout for the editor command to prevent hanging
4. Document the editor selection priority in comments or README

✅ Overall Assessment

The implementation is solid and addresses the issue well. With the security improvement for file permissions and addition of tests, this would be production-ready. The code is clean, readable, and follows Go idioms properly.

Great work on this feature! The Git-like approach is much more user-friendly than inline editing.

[claude]

Code Review for PR #77: External Editor for Command Editing

Thanks for this contribution! I've reviewed the changes and here's my feedback:

✅ Strengths

• Clean implementation that follows Git's familiar editor pattern
• Proper fallback chain: $EDITOR → $VISUAL → common editors
• Correctly handles stdin/stdout/stderr for terminal editor support
• Good error handling with descriptive messages
• Proper cleanup with deferred file removal

🔍 Code Quality & Best Practices

1. Temporary File Security: Consider using more restrictive permissions:

   • The temp file currently uses default permissions. For security, consider setting 0600 explicitly after creation

2. Editor Validation: The editor lookup is good, but consider validating it's executable by using the full path returned from exec.LookPath

🐛 Potential Issues

1. Empty Command Handling: When the edited file becomes empty, it silently returns false. Consider providing user feedback like "Command cancelled: empty command"

2. File Close Error: The code closes the temp file twice in the error path - once explicitly and once in the normal close. Consider using tmpFile.Sync() instead of the second close to avoid potential issues.

⚡ Performance Considerations

• The implementation is efficient with minimal overhead
• Temporary file creation is fast and cleanup is guaranteed via defer
• Editor lookup could be cached if this function is called frequently

🔒 Security Concerns

1. Command Injection: While exec.Command doesn't use shell interpretation, ensure the editor path is validated
2. Temp File Location: Consider allowing TMPDIR override for environments with specific security requirements
3. File Permissions: Explicitly set 0600 permissions for the temp file to prevent other users from reading the command being edited

🧪 Test Coverage

Missing Tests: The confirm.go file lacks a corresponding test file (confirm_test.go). Consider adding:

• Unit tests for the editor selection logic
• Tests for error conditions (no editor found, file creation failure)
• Mock tests for the exec.Command interaction
• Tests for the edit command flow with different editor scenarios

💡 Suggestions for Improvement

1. Add Windows Support: Consider supporting Windows editors like notepad for cross-platform compatibility

2. Configuration Option: Consider adding a config option in config.yaml for users to specify their preferred editor directly

3. Editor Arguments: Some users might want to pass arguments to their editor (e.g., syntax highlighting flags)

✨ Overall Assessment

This is a solid implementation that improves the user experience by leveraging familiar external editors. The code is clean and follows Go best practices. With the security enhancements and test coverage additions suggested above, this would be an excellent addition to the project.

The change successfully addresses issue #76 and provides a more robust editing experience than the inline readline approach. The Git-like approach is intuitive for developers and the fallback mechanism ensures it works out of the box for most users.