Conversation
PR Review: Interactive Attach/Detach SupportGreat work on this feature! The attach/detach functionality is well-implemented with clean separation of concerns. ✅ Strengths
🔍 Critical Issues1. Terminal State Leak (attach.go:214)If goroutine panics between EnableRawMode and RestoreTerminal, terminal left in raw mode. 2. Race Condition: Double Signal Handling (attach.go:251-261, run.go:267-308)Both attachInteractiveMode and runInteractive set up SIGINT/SIGTERM handlers. 3. Incomplete Error Context (manager.go:100-107)Transient errors treated same as not-found errors during state reconciliation. 4. Resource Leak: Proxy Server (manager.go:626-642)Proxy could leak if SaveMetadata panics. 5. Missing Validation (run.go:210)Interactive without TTY can cause undefined behavior.
|
|
@claude fix the lint errors. The code review issues seem minor. If you have permission, file issues for the serious ones otherwise we'll come back to it. |
|
Claude finished @dpup's task in 4m 36s —— View job Lint fixes completed ✓SummaryFixed all 6 golangci-lint errors and pushed changes to the Changes Made
Commit: Critical Review Issues (Unable to File)I don't have permission to create GitHub issues. Here are the critical findings from the code review that should be tracked:
--- | Branch: cmd |
Code Review: PR #29 - Interactive Modes with Attach/DetachThanks for this substantial feature addition! The attach/detach functionality and run persistence are great additions to moat. I've reviewed the changes and have feedback across several areas. Critical Issues 🔴1. Security: Firewall IP Filtering DisabledFiles: Both firewall implementations explicitly do not filter by destination IP address (see comments in code). While I understand the reasoning (dynamic IPs for host.docker.internal), this means the firewall allows traffic to ANY destination on the proxy port, not just the proxy. Impact: If another service runs on the same port on a different host, the firewall won't block it. For Docker on Linux with localhost binding, there's also no authentication token protection. Recommendations:
2. Potential Resource Leak in Docker AttachFile: The Recommendation: Use a 3. Incomplete Container State HandlingFile: The Recommendation: Add explicit handling for all possible container states and log warnings for unknown states. Moderate Issues 🟡4. Missing Error Handling in Attach CommandFile: Recent logs are displayed but errors are silently ignored. If Recommendation: Log a debug message when recent logs can't be fetched. 5. Windows Support IncompleteFile: Raw mode always returns an error on Windows, which means interactive mode won't work properly. Escape sequences will echo and the UX will be degraded. Recommendation: Either implement Windows raw mode using the Console API, or prominently document this limitation. 6. Duplicate Logic Between attach.go and run.goThe Issue: Violates DRY principle and makes maintenance harder. Bug fixes might need to be applied twice. Recommendation: Extract common interactive attach logic into a shared function in 7. Complex Control Flow in runInteractiveFile: This function has deeply nested select statements and handles multiple concerns: signal handling, escape sequences, attach lifecycle, container exit detection, and context cancellation. Recommendation: Break into smaller functions with clear responsibilities. Consider a state machine pattern for the different modes (attached, detached, exiting, etc.). Test Coverage 🧪8. No Tests for Attach CommandFile: The entire attach command has no unit tests despite handling complex state machines and user interaction. Recommendation: Add unit tests covering:
9. Missing Edge Cases in Escape Proxy TestsFile: Existing tests are good but missing:
10. No E2E Tests for Interactive ModeRecommendation: Add E2E tests that:
Security Notes 🔒11. Proxy Authentication Token in Environment VariableFile: The proxy auth token is passed via Severity: Medium - This is standard practice for HTTP proxy auth, but worth noting as an attack vector. Recommendation: Document this in security docs. Consider certificate-based auth for higher security scenarios. 12. Escape Sequence InjectionFile: If a malicious container could inject control characters matching the escape sequence, it could trigger detach/stop actions. Severity: Low - Requires attacker to control container output, which is already a compromised scenario. Recommendation: Document that escape sequences should not be used in untrusted environments. Performance 🚀13. Polling for Container Exit in Apple ContainersFile:
Recommendation: Investigate if Apple's container CLI has a native blocking wait command, or consider file system watches on container state files. 14. Double Log ProcessingFile: Logs are streamed to stdout during execution, then read again after container exits. For long-running containers with lots of output, this doubles I/O. Recommendation: Consider buffering logs during streaming so they can be written to storage without re-reading. Documentation 📚15. State Transitions UndocumentedFile: Run states are defined but valid transitions aren't documented. Can a run go from Recommendation: Add state transition diagram in comments or docs. 16. Escape Sequence ChoiceFile: Ctrl-/ is chosen as the escape prefix but:
Recommendation: Document the choice and consider making it configurable. Minor Issues17. Silent Error IgnoringMultiple locations use
Silent failures could lead to inconsistent state on disk. Recommendation: At minimum, log errors at debug level. For critical operations like initial metadata save, consider returning the error. 18. Magic Numbers
Recommendation: Extract to named constants with explanatory comments. Positive Aspects ✅
SummaryThis PR adds valuable functionality, but several areas need attention before merge: Must Fix:
Should Fix: Nice to Have: The core functionality appears sound, but the complexity and lack of tests for critical paths (especially attach) is concerning for production use. Great work on this substantial feature! |
|
@claude rebase this branch against main |
Allow specifying a default command in agent.yaml that runs when no command is provided on the CLI. CLI commands take precedence.
- Validate that command[0] is not empty when command is specified - Document default behavior (no command = container exits immediately) - Add security note about shell command patterns and variable interpolation - Consolidate command precedence documentation in Configuration section
Implement attach/detach functionality allowing users to connect to and disconnect from running containers without stopping them. Key features: - `moat attach <run-id>` to reconnect to running containers - `moat run -it` for interactive sessions with TTY - Escape sequences (Ctrl-/ d/k) for detach/stop in interactive mode - Double Ctrl+C (within 500ms) to stop in non-interactive mode - Run persistence across manager restarts via container state reconciliation - Recent logs displayed when re-attaching to interactive sessions - Interactive mode configurable via agent.yaml New files: - cmd/moat/cli/attach.go: Attach command implementation - internal/term/escape.go: Escape sequence detection for detach/stop - internal/term/raw.go: Terminal raw mode for clean escape handling - docs/plans/2026-01-19-interactive-attach-model.md: Design document Runtime interface extended with: - ContainerState(): Query container status for state reconciliation - Attach(): Connect stdin/stdout/stderr to running containers 🤖 Generated with [Claude Code](https://claude.com/claude-code)
When attaching to a running container, `moat attach` now defaults to the same interactive mode that was used when the run was started: - If started with `moat run -it`, attach uses interactive mode - If started without `-it`, attach uses output-only mode Use `-i=false` to force output-only mode even if the run was started interactively. The `-it` flags still work to force interactive mode. Changes: - Add `Interactive` field to run metadata (persisted to disk) - Restore interactive mode when loading persisted runs - Update attach command to check run's interactive flag - Use Cobra's `Changed()` to detect explicit flag usage 🤖 Generated with [Claude Code](https://claude.com/claude-code)
- Check CloseWrite() error in docker.go - Check RestoreTerminal() error in attach.go and run.go - Fix ineffectual assignment warnings by moving lastSigTime update - Fix redundant nil check in escape.go by breaking immediately Co-authored-by: Daniel Pupius <dpup@users.noreply.github.com>
Pull Request Review: Interactive Attach/Detach ModelThis is a significant feature addition that adds interactive mode support, attach/detach capabilities, and run persistence. Overall the implementation is solid with good attention to detail. Below are my findings organized by category: ✅ Strengths
🐛 Potential Bugs
|
🔒 Security Concerns
📊 Performance Considerations
🧪 Test CoverageExcellent test coverage for escape handling:
Missing tests:
📝 Code Quality
Minor suggestions:
|
🎯 Recommendations SummaryHigh Priority:
Medium Priority:
Low Priority:
📋 Conventional Commits Compliance✅ Commit message follows the conventional commits format specified in CLAUDE.md:
Overall Assessment: This is a well-implemented feature with good attention to edge cases and security. The main concerns are around state persistence reliability and test coverage for the attach flows. The code quality is high and follows Go best practices. Recommendation: Approve with suggested improvements to be addressed in follow-up PRs or before merge, depending on severity assessment. |
Pull Request Review - Interactive Mode & Attach/DetachThis is a significant and well-implemented feature that adds interactive modes, attach/detach capabilities, and run persistence. Overall, the code quality is high and follows the project conventions. Here's my detailed feedback: ✅ StrengthsArchitecture & Design
Code Quality
User Experience
🔍 Issues & Concerns1. Race Condition in Proxy Logger Setup (Medium Priority)Location: The atomic store reference pattern is used to avoid data races, but there's a window where network requests can be lost: var storeRef atomic.Value // holds *storage.RunStore
p.SetLogger(func(data proxy.RequestLogData) {
store, _ := storeRef.Load().(*storage.RunStore)
if store == nil {
// Store not yet initialized - early request during container startup.
// This is expected and non-fatal; the request won't be logged.
log.Debug("skipping network log: store not yet initialized", ...)
return
}
...
})Problem: Network requests made before line 644 ( Suggestion:
2. Timing-Dependent Container Exit Detection (Low-Medium Priority)Location: Multiple places use a 200ms delay to detect if a container exited: case <-logsDone:
select {
case err := <-waitDone:
// Container exited
case <-time.After(containerExitCheckDelay):
// Check run state manuallyProblem: This is timing-dependent and could race on slow systems. A fast container exit might not be detected within 200ms, or the state check might race with actual container shutdown. Suggestion:
3. Metadata Save Errors Are Silent (Low Priority)Location: Metadata persistence failures are ignored: _ = r.SaveMetadata()Problem: If disk is full or permissions are wrong, state won't persist across restarts. This could lead to confusion when Suggestion:
4. Double Ctrl+C Window May Be Too Short (UX)Location: doublePressWindow = 500 * time.MillisecondIssue: 500ms might be too fast for users, especially with network latency or when typing over SSH. Common practice is 1-2 seconds for double-press detection. Suggestion:
5. Signal Handling Inconsistency (Minor)Location: In if sig == syscall.SIGTERM {
fmt.Printf("\nStopping run %s...\n", r.ID)
// stops the run
}
// SIGINT is forwarded to containerBut in Suggestion: Extract signal handling into a shared function to ensure consistency and reduce duplication. 6. Potential Resource Leak on Error Paths (Low)Location: if err := secrets.ResolveAll(ctx, opts.Config.Secrets); err != nil {
if proxyServer != nil {
_ = proxyServer.Stop(context.Background())
}
return nil, err
}Issue: Container was created at line 587, but on error at line 434, the container isn't cleaned up. Similar issue with CA directory creation. Suggestion:
7. Test Coverage GapsWhat's Missing:
Suggestion: Add integration tests that:
🎯 Performance ConsiderationsStorage WritesThe atomic store reference pattern is good, but consider:
Container State Polling
🔒 Security ReviewEscape Sequence Handling✅ Well-designed: Ctrl-/ is unlikely to conflict with application input Raw Mode Terminal✅ Proper cleanup with defer Authentication Token for Apple Containers✅ Good use of 32-byte cryptographically random tokens (line 290) 📝 Documentation & StyleFollows CLAUDE.md Guidelines✅ Conventional commit format: Comments & Docs✅ Good architectural documentation in Suggestions
🧪 Testing Recommendations# Scenarios to test manually:
# 1. Detach and reattach multiple times
moat run -i -- bash
# Press Ctrl-/ d to detach
moat attach <run-id>
# 2. Manager restart persistence
moat run -d ./project
# Kill moat manager process
# Restart and verify run is still tracked
# 3. Fast-exiting containers
moat run -- sh -c 'echo hello'
# Verify logs are captured
# 4. Network requests during startup
moat run --grant github -- sh -c 'curl https://api.github.com'
# Verify network.jsonl captures the request✨ Nice Touches
📊 Summary
Overall: 8.3/10 - Strong implementation with minor issues that should be addressed. 🎬 Recommendation✅ Approve with minor changes Must fix before merge:
Should fix soon after merge: Nice to have: Great work on this PR! The attach/detach model is well-designed and the implementation quality is high. The issues I've raised are relatively minor and don't block the core functionality. |
2 participants
Details
Attach/Detach Model
Runs now exist independently of your terminal. By default, moat run attaches to show output:
New Commands
Run Persistence
Runs now survive manager restarts. On startup, the manager reconciles persisted run metadata with actual container state via the new ContainerState() runtime method.
Configuration
Interactive mode can be set in agent.yaml: