feat: Recipe Editor Phase 3 - LSP WebSocket Bridge

[dubadub]

Summary

Phase 3 of the Recipe Editor feature - WebSocket bridge connecting the browser editor to cooklang-language-server.

Depends on: #244 (Phase 2)

Changes

• Add WebSocket feature to Axum and futures-util dependency
• Create LSP bridge module (src/server/lsp_bridge.rs)
• Register WebSocket endpoint at /api/ws/lsp
• Add LSP connection status indicator to editor UI
• Add i18n keys for LSP status (6 locales + new uk-UA locale)

Architecture

Browser (WebSocket) ↔ /api/ws/lsp ↔ Bridge ↔ stdio ↔ cook lsp subprocess

• One LSP subprocess per WebSocket connection
• Bidirectional JSON-RPC message forwarding
• Proper LSP Content-Length framing
• Subprocess cleanup on disconnect

Editor Status Bar

Shows LSP connection status:

• 🟢 Green: Connected
• ⚪ Gray: Disconnected
• 🔴 Red: Error

Auto-reconnects after 3 seconds on disconnect.

Test Plan

• All existing tests pass
• Manual: Start server, open editor, verify "LSP Connected" status
• Manual: Check browser console for LSP handshake messages
• Manual: Stop server, verify "Disconnected" and auto-reconnect

[claude]

Pull Request Review: Recipe Editor Phase 3 - LSP WebSocket Bridge

Summary

This PR implements a WebSocket bridge that connects the browser-based recipe editor to the cooklang-language-server subprocess, enabling LSP features like diagnostics and completions. The implementation follows a clean architecture with proper message framing and bidirectional communication.

✅ Strengths

Architecture & Design

• Clean separation of concerns: The LSP bridge module is well-isolated with clear responsibilities
• Proper LSP protocol handling: Correctly implements Content-Length framing for LSP messages
• Resource management: Good cleanup of subprocess and WebSocket connections on disconnect
• Auto-reconnect: Client-side reconnection logic improves resilience

Code Quality

• Good documentation: Module-level docs clearly explain the architecture
• Appropriate logging: Uses tracing with debug/info/error levels appropriately
• Error handling: Proper error propagation and logging throughout

🔍 Issues & Concerns

1. CRITICAL - Resource Leak & DoS Risk (src/server/lsp_bridge.rs:1022-1034)

Severity: High

Each WebSocket connection spawns a new LSP subprocess without any limiting mechanism. This could lead to:

• Resource exhaustion (memory, file descriptors, CPU)
• Potential DoS attack vector
• Performance degradation under load

Recommendation:

// Add connection pooling or limit concurrent LSP processes
use std::sync::atomic::{AtomicUsize, Ordering};

static ACTIVE_LSP_CONNECTIONS: AtomicUsize = AtomicUsize::new(0);
const MAX_LSP_CONNECTIONS: usize = 10; // Adjust based on server capacity

async fn handle_lsp_connection(socket: WebSocket, state: Arc<AppState>) {
    let current = ACTIVE_LSP_CONNECTIONS.fetch_add(1, Ordering::SeqCst);
    if current >= MAX_LSP_CONNECTIONS {
        ACTIVE_LSP_CONNECTIONS.fetch_sub(1, Ordering::SeqCst);
        error\!("Max LSP connections ({}) reached, rejecting connection", MAX_LSP_CONNECTIONS);
        let _ = socket.close().await;
        return;
    }
    
    // ... existing code ...
    
    // Ensure we decrement on exit
    let _guard = scopeguard::guard((), |_| {
        ACTIVE_LSP_CONNECTIONS.fetch_sub(1, Ordering::SeqCst);
    });
}

2. Security - Process Execution Path (src/server/lsp_bridge.rs:1024)

Severity: Medium

Using std::env::current_exe() assumes the current binary has LSP capabilities. While this works for CookCLI, it could be problematic if:

• The binary is replaced or modified
• Running in containerized/restricted environments
• The executable is moved after startup

Recommendation:
Add validation that the LSP command actually exists:

async fn spawn_lsp_process(base_path: &camino::Utf8Path) -> Result<Child, std::io::Error> {
    let exe_path = std::env::current_exe()?;
    
    // Verify the executable supports LSP
    if \!tokio::fs::metadata(&exe_path).await.is_ok() {
        return Err(std::io::Error::new(
            std::io::ErrorKind::NotFound,
            "LSP executable not found"
        ));
    }
    
    debug\!("Spawning LSP process: {} lsp", exe_path.display());
    // ... rest of implementation
}

3. Bug - Incorrect Response Handling (templates/edit.html:1307-1315)

Severity: Medium

The LSP message handler checks for message.result but doesn't properly identify which request the response is for. This could cause issues when multiple requests are in flight.

Current code:

if (message.id \!== undefined && message.result \!== undefined) {
    // Response to a request
    console.log('LSP response:', message);
    if (message.result && message.result.capabilities) {
        // Initialize response received, send initialized notification
        sendLspNotification('initialized', {});
    }
}

Recommendation:
Track pending requests properly:

const pendingRequests = new Map();

function sendLspRequest(method, params) {
    if (\!lspSocket || lspSocket.readyState \!== WebSocket.OPEN) {
        return null;
    }
    const id = lspMessageId++;
    const message = {
        jsonrpc: '2.0',
        id: id,
        method: method,
        params: params
    };
    
    pendingRequests.set(id, { method, timestamp: Date.now() });
    lspSocket.send(JSON.stringify(message));
    return id;
}

function handleLspMessage(message) {
    if (message.id \!== undefined) {
        const request = pendingRequests.get(message.id);
        if (request) {
            pendingRequests.delete(message.id);
            
            if (request.method === 'initialize' && message.result?.capabilities) {
                sendLspNotification('initialized', {});
                console.log('LSP initialized successfully');
            }
        }
    } else if (message.method) {
        // Server notification
        console.log('LSP notification:', message.method);
    }
}

4. Potential Race Condition (src/server/lsp_bridge.rs:1054-1108)

Severity: Low

The stdout_task reads LSP output but doesn't handle the case where the WebSocket is closed while reading. If the channel tx is dropped, the task will exit silently, but partial messages might be lost.

Recommendation:
Add proper shutdown signaling:

// Use a shutdown token to coordinate cleanup
let (shutdown_tx, mut shutdown_rx) = tokio::sync::watch::channel(false);

let stdout_task = tokio::spawn(async move {
    loop {
        tokio::select\! {
            _ = shutdown_rx.changed() => {
                debug\!("Shutdown signal received, closing stdout reader");
                return;
            }
            result = read_lsp_message(&mut stdout_reader) => {
                // Handle message...
            }
        }
    }
});

5. Missing Error Context (src/server/lsp_bridge.rs:1089)

Severity: Low

Using tokio::io::AsyncReadExt::read_exact directly is correct, but error messages don't include the expected vs actual bytes read, making debugging harder.

Recommendation:

if let Err(e) = tokio::io::AsyncReadExt::read_exact(&mut stdout_reader, &mut content).await {
    error\!("Error reading LSP content ({} bytes expected): {}", content_length, e);
    return;
}

6. Hardcoded Values (templates/edit.html:1260)

Severity: Low

The 3-second reconnect delay is hardcoded. Consider:

• Exponential backoff to reduce server load
• Configuration option for reconnect behavior
• Maximum retry limit

Recommendation:

let reconnectDelay = 1000;
const MAX_RECONNECT_DELAY = 30000;
let reconnectAttempts = 0;

lspSocket.onclose = function() {
    console.log('LSP WebSocket closed');
    updateLspStatus('disconnected');
    
    // Exponential backoff with jitter
    const delay = Math.min(
        reconnectDelay * Math.pow(2, reconnectAttempts) + Math.random() * 1000,
        MAX_RECONNECT_DELAY
    );
    reconnectAttempts++;
    
    setTimeout(() => {
        connectLsp();
    }, delay);
};

lspSocket.onopen = function() {
    reconnectAttempts = 0; // Reset on successful connection
    reconnectDelay = 1000;
    // ... rest of onopen
};

7. Internationalization Not Applied (templates/edit.html:1334, 1340, 1345)

Severity: Low

The PR adds i18n keys (lsp-connected, lsp-disconnected, lsp-error) but the template hardcodes English strings instead of using the Fluent translations.

Current:

text.textContent = 'LSP Connected';

Should be:

text.textContent = '{{ "lsp-connected" | fluent }}';

Or use a data attribute approach if dynamic updates are needed.

📋 Minor Issues

8. Missing WebSocket Close Handling

The server doesn't send a proper close frame before killing the subprocess. Add:

// Before killing the process, try to close WebSocket gracefully
let _ = ws_sender.send(Message::Close(None)).await;
tokio::time::sleep(tokio::time::Duration::from_millis(100)).await;
let _ = lsp_process.kill().await;

9. Cursor Position Not Implemented

The status bar shows Line 1, Col 1 but this is never updated. Either implement it or remove the placeholder.

10. Missing Content-Type Validation

The WebSocket handler doesn't validate that text messages are valid JSON before attempting to forward them to the LSP.

🧪 Test Coverage

Missing Tests:

• No automated tests for LSP bridge functionality
• No WebSocket connection/disconnection tests
• No LSP protocol framing tests
• No error handling tests (malformed messages, subprocess crashes)

Recommendation:
Add integration tests:

#[cfg(test)]
mod tests {
    use super::*;
    
    #[tokio::test]
    async fn test_lsp_websocket_connection() {
        // Test WebSocket upgrade and LSP subprocess spawn
    }
    
    #[tokio::test]
    async fn test_lsp_message_framing() {
        // Test Content-Length header parsing
    }
    
    #[tokio::test]
    async fn test_concurrent_connection_limit() {
        // Test connection limiting
    }
}

🎯 Performance Considerations

1. Memory usage: Each connection allocates buffers and spawns a subprocess. Monitor memory under load.
2. Channel buffering: The mpsc channel has a buffer of 32. This seems reasonable but may need tuning.
3. Subprocess overhead: Consider process pooling for high-traffic scenarios.

🔒 Security Recommendations

1. Add authentication: WebSocket endpoint has no auth. Consider requiring session cookies or tokens.
2. Rate limiting: Add per-IP rate limiting to prevent abuse.
3. Input validation: Validate all WebSocket messages before forwarding.
4. Timeout handling: Add timeouts for subprocess operations to prevent hanging.

✨ Suggestions

1. Add WebSocket heartbeat/ping-pong to detect dead connections early
2. Log LSP subprocess crashes with stderr capture for debugging
3. Add metrics for LSP connection count, message throughput, error rates
4. Consider LSP process pooling for better resource utilization

📝 Documentation

The plan document (docs/plans/2026-01-19-recipe-editor-phase3.md) is excellent and very detailed. However:

• Consider adding architecture diagrams
• Document the connection limit and resource constraints
• Add troubleshooting section for common issues

Overall Assessment

Code Quality: ✅ Good
Architecture: ✅ Good
Security: ⚠️ Needs attention (connection limits, validation)
Testing: ❌ Missing automated tests
Documentation: ✅ Good

Recommendation

APPROVE with requested changes

The implementation is solid and well-structured, but should address the critical resource leak issue (#1) and the security concerns before merging. The other issues are less urgent but should be tracked for follow-up.

Priority fixes:

1. ⚠️ Must fix: Add LSP connection limit (DoS prevention)
2. ⚠️ Should fix: Implement proper request tracking in JavaScript
3. ✨ Nice to have: Add exponential backoff, tests, i18n integration

Great work on Phase 3! The LSP bridge architecture is clean and extensible for Phase 4's completion and diagnostic features.

---

Review generated by Claude Code

[claude]

PR Review: Recipe Editor Phase 3 - LSP WebSocket Bridge

Summary

This PR implements a WebSocket bridge connecting the browser editor to the cooklang-language-server subprocess. The architecture is sound and follows best practices for async Rust with Axum and Tokio.

✅ Strengths

Architecture & Design

• Clean separation: Bridge module is well-isolated with clear responsibilities
• Resource management: Proper subprocess cleanup with lsp_process.kill() on disconnect
• LSP protocol compliance: Correct Content-Length framing implementation
• Auto-reconnect: Frontend implements 3-second reconnect on disconnect
• One subprocess per connection: Appropriate isolation model for LSP

Code Quality

• Good error handling: Proper error propagation and logging throughout
• Async best practices: Uses tokio::select! for concurrent task management
• Clear documentation: Module-level docs explain the architecture
• I18n support: Comprehensive translations including new Ukrainian locale

🔍 Issues & Concerns

1. Security: Potential Resource Exhaustion ⚠️

Severity: High

The WebSocket endpoint spawns a new subprocess per connection without rate limiting or connection limits. This could lead to:

• DoS attacks by opening many connections
• Resource exhaustion on the server

Recommendation:

// Add to AppState
pub struct AppState {
    // ... existing fields
    active_lsp_connections: Arc<AtomicUsize>,
    max_lsp_connections: usize, // e.g., 10
}

// In lsp_websocket handler
async fn lsp_websocket(ws: WebSocketUpgrade, State(state): State<Arc<AppState>>) -> impl IntoResponse {
    let current = state.active_lsp_connections.load(Ordering::Relaxed);
    if current >= state.max_lsp_connections {
        return (StatusCode::TOO_MANY_REQUESTS, "Too many LSP connections").into_response();
    }
    // ... rest of handler
}

2. Bug: Incorrect rootUri in Initialize Request

Severity: Medium

In templates/edit.html:1240:

rootUri: 'file://' + window.location.pathname,

This uses the HTTP path (e.g., /edit/recipe.cook) instead of the filesystem path. The LSP server expects the actual base path.

Fix:

// Pass base_path from server to template
rootUri: {{ base_path_uri|json }}, // Server should provide this

Or use a dedicated API endpoint to get the proper URI.

3. Potential Race Condition in Header Parsing

Severity: Low-Medium

In src/server/lsp_bridge.rs:1054-1080, the header parsing logic:

• Uses a single content_length variable that persists across loop iterations
• Could theoretically carry over stale values if parsing fails

Recommendation:
Move content_length initialization inside the outer loop (it's already there at line 1056, so this is actually fine - but consider adding a comment explaining the reset).

4. Missing Error Recovery in Frontend

Severity: Low

The handleLspMessage function doesn't handle malformed responses or error responses from the LSP server.

Recommendation:

function handleLspMessage(message) {
    if (message.error) {
        console.error('LSP error response:', message.error);
        // Could show user-facing error here
        return;
    }
    // ... rest of handler
}

5. No Timeout for LSP Subprocess

Severity: Low

If the LSP process hangs or becomes unresponsive, the bridge will wait indefinitely.

Recommendation:
Add timeout handling:

tokio::select! {
    _ = stdout_task => {},
    _ = stdin_task => {},
    _ = ws_send_task => {},
    _ = tokio::time::sleep(Duration::from_secs(300)) => {
        warn!("LSP connection timeout after 5 minutes of inactivity");
    }
}

6. I18n Keys Not Used in Template

Severity: Low

The new LSP status i18n keys (lines 700-702, 713-715, etc.) are defined but the template at templates/edit.html:1207 uses hardcoded English strings:

<span id="lsp-text">Disconnected</span>

Recommendation:
Use Fluent templates or pass translations to JavaScript:

const i18n = {
    lspConnected: {{ "lsp-connected"|fluent|json }},
    lspDisconnected: {{ "lsp-disconnected"|fluent|json }},
    lspError: {{ "lsp-error"|fluent|json }}
};

🎯 Performance Considerations

Good:

• Channel buffer size (32) is reasonable for LSP message throughput
• Proper use of flush() after writes prevents buffering delays
• Task splitting allows concurrent read/write operations

Potential Optimization:

• Consider using BufWriter for stdin writes to batch small writes
• The debug! logging might be verbose in production; consider using trace!

🧪 Testing

Manual Test Plan ✅

The PR includes a comprehensive manual test plan in the phase 3 plan document.

Missing:

• No automated tests for the LSP bridge
• Should add integration tests that:
  • Verify WebSocket connection establishment
  • Test message forwarding in both directions
  • Verify subprocess cleanup on disconnect
  • Test error conditions (invalid LSP binary, subprocess crash, etc.)

Recommendation:

#[cfg(test)]
mod tests {
    use super::*;
    
    #[tokio::test]
    async fn test_lsp_bridge_cleanup() {
        // Test that subprocess is killed when connection closes
    }
    
    #[tokio::test]
    async fn test_message_forwarding() {
        // Test bidirectional message passing
    }
}

📋 Minor Issues

Code Style:

1. Line 1089: Could use read_exact directly from AsyncReadExt trait (already imported via use statement) - current code is fine but mixing trait imports
2. The headers string in the original plan (line 267) is created but reset each iteration - the actual implementation (line 1056) correctly declares it inside the loop

Documentation:

• Consider adding examples of LSP message flow to module docs
• Document the expected LSP server behavior (what happens if cook lsp binary is missing?)

🔒 Security Summary

1. ✅ No SQL injection risks (no database)
2. ✅ No XSS risks (JSON-RPC over WebSocket, no HTML rendering)
3. ⚠️ DoS risk: Unlimited subprocess spawning
4. ✅ Subprocess isolation is good (separate process per connection)
5. ✅ No command injection (uses current_exe() and hardcoded args)

📝 Recommendations Priority

Must Fix Before Merge:

1. Add connection limiting/rate limiting for LSP WebSocket endpoint
2. Fix rootUri to use correct filesystem path

Should Fix:
3. Use i18n keys in frontend instead of hardcoded strings
4. Add error response handling in handleLspMessage
5. Add automated tests for LSP bridge

Nice to Have:
6. Add timeout for inactive connections
7. Add BufWriter optimization for LSP stdin
8. Improve debug logging verbosity control

🎉 Conclusion

This is a well-architected feature with clean async Rust code. The main concern is the lack of connection limiting which could lead to resource exhaustion. With the security fixes applied, this would be a solid addition to the codebase.

The code quality is high, error handling is thorough, and the separation of concerns is excellent. Great work on Phase 3!

Recommendation: Request Changes (for security fix) → then Approve