Draft: better logs and shell

[stmh]

No description provided.

[claude]

Claude finished @stmh's task —— View job

---

Pull Request Review: Better Logs and Shell

I've completed a comprehensive review of this substantial feature implementation that adds unified output system with logs and shell functionality across 57 files.

Review Tasks

• Analyze core architecture and design patterns
• Review Docker services implementation (logs/shell)
• Evaluate WebSocket authentication and security
• Check error handling and resource management
• Review CLI implementation and UX
• Assess test coverage and quality
• Provide structured feedback and recommendations

---

✅ Overall Assessment: High Quality Implementation

This PR implements a well-architected unified output system that addresses real problems with time-synchronized logging while adding significant new capabilities. The implementation demonstrates strong software engineering practices.

🎯 Strengths

Architecture & Design

• Clean separation of concerns: TaskDetails focused on state, TaskOutput handles streams separately
• Strong type safety: Comprehensive error types (LogStreamError, ShellServiceError) with proper context
• Memory management: Configurable limits with automatic cleanup prevent memory issues
• Future-proofing: Well-designed WebSocket protocol supports both web UI and CLI

Docker Integration

// scotty/src/docker/services/logs.rs:174-198

• Proper container discovery: Robust service-to-container mapping with error handling
• Streaming optimization: Smart buffering (1 line/50ms for follow, 10 lines/100ms for historical)
• Idle timeout: 200ms timeout for historical logs prevents hanging CLI sessions
• Resource cleanup: Proactive stream termination with proper WebSocket notifications

Security Implementation

// scotty/src/api/auth_core.rs:16-52

• Centralized authentication: Single function handles all auth modes (dev/bearer/oauth)
• Token flexibility: Accepts both "Bearer token" and raw token formats
• Permission integration: Proper authorization checks before stream access
• WebSocket security: Message-based authentication flow with proper error handling

CLI User Experience

// scottyctl/src/commands/apps/logs.rs:287-299

• Clear feedback: Progressive status updates and success messages
• Flexible options: Support for --follow, --lines, --timestamps, time filters
• Error recovery: Graceful handling of connection failures and authentication errors
• Performance: Efficient WebSocket streaming with proper connection management

🔍 Areas for Improvement

1. Error Handling Enhancements

// scotty/src/docker/services/logs.rs:356-369

Issue: Docker connection failures could be more specific

// Current
Err(e) => {
    error!("Error reading logs for stream {}: {}", stream_id, e);
    // Generic error sent to client
}

// Suggestion: Add specific error types
match e.kind() {
    bollard::errors::ErrorKind::DockerResponseNotFoundError => {
        // Container stopped/removed
    },
    bollard::errors::ErrorKind::DockerResponseTimeoutError => {
        // Timeout - maybe retry
    },
    _ => // Generic handling
}

2. Memory Management Edge Cases

// scotty-core/src/output/unified_output.rs:122-129

Issue: Line truncation could break multi-byte UTF-8 characters

// Current - potential UTF-8 boundary issue
let truncated_content = if content.len() > self.limits.max_line_length {
    format!("{}... [TRUNCATED]", &content[..self.limits.max_line_length - 15])
} else { content };

// Suggestion: Use char_indices() for safe UTF-8 truncation
let truncated_content = if content.chars().count() > max_chars {
    let truncate_pos = content.char_indices()
        .nth(max_chars - 15)
        .map(|(pos, _)| pos)
        .unwrap_or(content.len());
    format!("{}... [TRUNCATED]", &content[..truncate_pos])
} else { content };

3. Configuration Validation

// scotty-core/src/settings/shell.rs:24-36

Missing: Shell path validation could be stronger

// Add validation method
impl ShellSettings {
    pub fn validate(&self) -> Result<(), String> {
        for shell in &self.allowed_shells {
            if !std::path::Path::new(shell).exists() {
                return Err(format!("Shell not found: {}", shell));
            }
        }
        Ok(())
    }
}

4. Performance Considerations

Log Buffer Optimization (scotty/src/docker/services/logs.rs:123-157):

• Consider using Vec::with_capacity() for better allocation patterns
• Buffer size could be adaptive based on container log volume

WebSocket Message Serialization:

• JSON serialization on every message could be expensive for high-volume logs
• Consider MessagePack or binary protocols for better performance

🧪 Test Coverage Assessment

Strengths:

• Comprehensive Docker service tests with CI-friendly approach
• Good edge case coverage (service not found, container ID missing)
• Mock-friendly design enables testing without Docker dependency

Areas to expand:

• Integration tests for WebSocket authentication flow
• CLI command tests with mock WebSocket server
• Error recovery scenarios (network failures, container restarts)

🔒 Security Review

Well implemented:

• ✅ Centralized authentication prevents duplication
• ✅ Permission checks before accessing app resources
• ✅ Input validation for service names and shell paths
• ✅ Session isolation and cleanup

Consider adding:

• Rate limiting for shell session creation
• Audit logging for shell access
• Container escape sequence filtering for security

🚀 Performance & Scalability

Good patterns:

• Efficient VecDeque operations for log rotation
• Smart buffering reduces WebSocket message overhead
• Proactive cleanup prevents resource leaks

Future considerations:

• Connection pooling for high-volume deployments
• Log compression for long-term storage
• Horizontal scaling of WebSocket connections

📝 Code Quality

Excellent practices:

• Consistent error handling patterns
• Comprehensive documentation and examples
• Clear naming conventions
• Proper separation of concerns

🔧 Minor Issues

1. Dead code annotations: Some fields marked as #[allow(dead_code)] should be reviewed
2. Magic numbers: Buffer sizes (50ms, 200ms) could be configurable
3. Logging verbosity: Some debug logs might be too verbose for production

---

✅ Recommendation: APPROVE with Minor Improvements

This is a high-quality implementation that significantly enhances Scotty's capabilities. The architecture is sound, security is well-handled, and the user experience is thoughtfully designed. The suggested improvements are minor and could be addressed in follow-up PRs.

Key achievements:

• ✅ Solves real time-synchronicity problems
• ✅ Adds valuable real-time log streaming
• ✅ Implements secure shell access
• ✅ Maintains backward compatibility
• ✅ Provides excellent CLI experience

The implementation follows the documented PRD closely and represents substantial value for users managing containerized applications.

---