feat: support configurable text editor for editing modelfiles

[sammcj]

• feat: support configurable text editor for editing modelfiles

For example, in your ~/.config/gollama/config.json you could add:

  "editor": "/Applications/Visual Studio Code.app/Contents/Resources/app/bin/code",

[llamapreview]

Auto Pull Request Review from LlamaPReview

1. Overview

1.1 Core Changes

• Primary purpose and scope: Add configurable text editor support for modelfile editing
• Key components modified:
  • App state management (app_model.go)
  • Editor configuration handling (config/*)
  • Editor execution logic (operations.go)
  • UI views (app_model.go)
• Cross-component impacts: Affects model editing workflow, configuration system, and UI state management
• Business value alignment: Enhances UX by supporting external editors like VS Code, improving productivity

1.2 Technical Architecture

• System design modifications: Added new UI state for external editor workflow
• Component interaction changes: Extended editor handling logic in operations.go
• Integration points impact: Requires secure file handling and process execution
• Dependency changes and implications: No new dependencies added

2. Critical Findings

2.1 Must Fix (P0🔴)

Issue: Insecure temporary file handling (predictable filenames)

• Analysis Confidence: High
• Impact: Vulnerable to symlink attacks and race conditions
• Resolution: Use os.CreateTemp() with random filenames

Issue: Command injection vulnerability

• Analysis Confidence: High
• Impact: Potential arbitrary command execution
• Resolution: Avoid shell invocation, use exec.Command() directly with sanitized arguments

Issue: VS Code flag handling flaw

• Analysis Confidence: Medium
• Impact: Incorrect editor configuration persistence
• Resolution: Apply flags at execution time instead of modifying config value

2.2 Should Fix (P1🟡)

Issue: Error handling gaps in editor startup

• Analysis Confidence: High
• Impact: Temporary files not cleaned up on error
• Suggested Solution: Add file cleanup in error handling blocks

Issue: Parameter parsing vulnerability

• Analysis Confidence: Medium
• Impact: Doesn't handle quoted values in modelfiles
• Suggested Solution: Implement quoted value parsing in parameter extraction

Issue: State management flaw

• Analysis Confidence: High
• Impact: Inconsistent state transitions during editor cancellation
• Suggested Solution: Create unified state reset method

2.3 Consider (P2🟢)

Area: Editor path validation

• Analysis Confidence: Medium
• Improvement Opportunity: Prevent runtime errors by validating editor paths at config load

Area: Windows path handling in documentation

• Analysis Confidence: High
• Improvement Opportunity: Improve Windows user experience with platform-specific examples

2.4 Summary of Action Items

• Immediate (P0): Fix security vulnerabilities in file handling and command execution
• High Priority (P1): Improve error handling and state management
• Nice-to-have (P2): Add editor validation and Windows docs

3. Technical Analysis

3.1 Code Logic Analysis

📁 operations.go - startExternalEditor

• Submitted PR Code:

    newModelfilePath := filepath.Join(tempDir, fmt.Sprintf("%s_modelfile.txt", modelName))
    // ...
    if strings.Contains(editor, " ") {
        cmd = exec.Command("sh", "-c", fullCommand)
    }

• Analysis:
  • Predictable temp filenames enable symlink attacks
  • Shell invocation with user input enables command injection
  • VS Code flag handling modifies config value instead of execution context
  • Error cases don't clean up temporary files
• LlamaPReview Suggested Improvements:

    tempFile, err := os.CreateTemp(tempDir, "modelfile_*.txt")
    newModelfilePath := tempFile.Name()
    // ...
    parts := strings.Fields(editor)
    if len(parts) > 1 {
        args := append(parts[1:], newModelfilePath)
        if strings.Contains(editor, "code") {
            args = append([]string{"--wait"}, args...)
        }
        cmd = exec.Command(parts[0], args...)
    } else {
        cmd = exec.Command(editor, newModelfilePath)
    }

• Improvement rationale:
  • Prevents symlink attacks with random filenames
  • Eliminates command injection risk
  • Properly associates flags with execution
  • Maintains configuration integrity

📁 operations.go - finishExternalEdit

• Submitted PR Code:

    parts := strings.SplitN(line, " ", 3)

• Analysis:
  • Doesn't handle quoted parameter values
  • Will incorrectly split values containing spaces
• LlamaPReview Suggested Improvements:

    if strings.HasPrefix(strings.ToUpper(line), "PARAMETER") {
        line = strings.TrimSpace(strings.TrimPrefix(line, "PARAMETER"))
        firstSpace := strings.Index(line, " ")
        if firstSpace == -1 {
            continue
        }
        paramName := strings.ToLower(line[:firstSpace])
        paramValue := strings.TrimSpace(line[firstSpace+1:])
        // Handle quoted values
        if len(paramValue) > 1 && paramValue[0] == '"' && paramValue[len(paramValue)-1] == '"' {
            paramValue = paramValue[1 : len(paramValue)-1]
        }

• Improvement rationale:
  • Correctly handles values with spaces
  • Maintains modelfile specification compliance

📁 app_model.go - Key handlers

• Submitted PR Code:

    if m.view == ExternalEditorView {
        m.externalEditing = false
        m.externalEditorFile = ""
    }

• Analysis:
  • State reset logic duplicated across multiple handlers
  • Risk of inconsistent state management
• LlamaPReview Suggested Improvements:

    // Add to AppModel
    func (m *AppModel) resetEditorState() {
        m.externalEditing = false
        m.externalEditorFile = ""
        m.externalEditorModel = ""
    }
    
    // Update call sites:
    m.resetEditorState()

• Improvement rationale:
  • Centralizes state management
  • Reduces error-prone duplication
  • Ensures consistent state transitions

3.2 Key Quality Aspects

• Testing strategy and coverage: Add tests for editor path validation and quoted parameter handling
• Documentation needs: Include Windows path examples in README.md

4. Overall Evaluation

• Technical assessment: Valuable feature with security and robustness concerns
• Business impact: Significantly improves UX for power users
• Risk evaluation: High risk due to security vulnerabilities in current implementation
• Notable positive aspects:
  • Clean configuration approach
  • Good fallback behavior
  • Comprehensive VS Code support
• Implementation quality: Good architectural design but needs security hardening
• Final recommendation: Request Changes (Address P0 issues before merging)

---

💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.