feat: Phase 1 — Container engine#19
Conversation
Add Lima VM runtime on macOS, native nerdctl path on Linux, Docker socket forwarding, calf start/stop/status CLI, container/image API, WebSocket logs, Flutter resource screens, hello-world example, and migration docs. Closes #10 Closes #11 Closes #12 Closes #13 Closes #14 Closes #15 Closes #16 Closes #17 Closes #18 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (23)
📝 WalkthroughWalkthroughThis PR adds a runtime abstraction with macOS VM and Linux native backends, expands the daemon with container/image/volume/build/registry/log APIs, adds Docker Desktop migration support, and rewrites the Flutter client to manage containers, images, volumes, builds, settings, and registry login. ChangesBackend runtime, API, and migration
Estimated code review effort: 5 (Critical) | ~150 minutes Flutter UI
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
- Added .DS_Store to track macOS Finder settings. - Removed 'bin/' from .gitignore to allow tracking of build artifacts. - Updated DEVELOPMENT.md with new commands for starting the backend and UI. - Enhanced Makefile with new targets for development and building. - Introduced new API endpoints for managing builds, containers, images, and registry login. - Refactored existing API handlers for improved functionality and error handling. This commit improves the development experience and expands the API capabilities.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (18)
backend/internal/api/runtime_ready.go-11-49 (1)
11-49: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize runtime startup across requests
ensureRuntimeRunninghas a TOCTOU gap: concurrent handlers can both see the runtime as stopped and calls.runtime.Start()at the same time. Add a mutex orsingleflight.GrouponServerso only one startup runs and other callers wait on the same result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/runtime_ready.go` around lines 11 - 49, ensureRuntimeRunning in Server has a race where multiple concurrent requests can observe the runtime as stopped and both call s.runtime.Start; add shared serialization on Server (for example a mutex or singleflight.Group) so only one startup attempt runs at a time and other callers wait for the same result. Update the startup path around s.runtime.Status, s.runtime.Start, and the polling loop to use that shared guard consistently so the check/start sequence is not duplicated across handlers.backend/internal/api/logs.go-24-52 (1)
24-52: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing write deadline on WebSocket log streaming.
conn.WriteMessagein theStreamLogscallback (line 45) has noSetWriteDeadline. If the client stalls (dead connection, network partition, slow consumer), the write can block indefinitely, holding the streaming goroutine and any underlying log-tailing resources open for the connection's lifetime. Gorilla's own docs recommend setting a write deadline before each write to keep server resources bounded.🔧 Suggested fix
err = s.runtime.StreamLogs(ctx, id, func(line string) { + _ = conn.SetWriteDeadline(time.Now().Add(10 * time.Second)) if writeErr := conn.WriteMessage(websocket.TextMessage, []byte(line)); writeErr != nil { cancel() } })Also consider adding a read deadline + ping/pong keepalive on the reader goroutine to proactively detect and close idle/dead connections rather than relying solely on
ReadMessageerroring out.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/logs.go` around lines 24 - 52, The WebSocket log stream in handleContainerLogsWebSocket writes with conn.WriteMessage but never sets a write deadline, so a slow or dead client can block the stream indefinitely. Update the StreamLogs callback to set a short write deadline on the websocket.Conn before each write, and keep the existing error/cancel flow in place. While touching this area, also add a read deadline plus ping/pong keepalive handling in the reader goroutine so idle or dead connections are detected promptly.backend/internal/api/migrate.go-78-83 (1)
78-83: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftGuard all
s.cfgaccess with the same lock.SaveConfigmutatess.cfgin the migration goroutine, andbackend/internal/api/handlers.goalso reads/writes the same fields without anycfgMuor other protection inserver.go. That leaves concurrent requests and migration work racing on shared config state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/migrate.go` around lines 78 - 83, `SaveConfig` is mutating shared server config state without synchronization, which can race with other config reads/writes in the API layer. Update the `SaveConfig` function in the migration flow to use the same `cfgMu`/locking strategy that protects config access elsewhere, and make sure every `s.cfg` read or write in the related server config paths uses that same lock. Keep the mutation and `config.Save` call inside the protected section so `s.cfg` remains consistent across concurrent requests and migration work.backend/internal/api/migrate.go-98-112 (1)
98-112: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse direct nerdctl on native runtime
This helper always shells through
limactl, but Linux uses the native backend. If Docker Desktop migration is exposed there, it will fail on a missing Lima binary instead of invoking nerdctl directly; either bypasslimactlin native mode or gate this path to VM/Lima only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/migrate.go` around lines 98 - 112, The runNerdctl helper always invokes limactl, which breaks native Linux execution; update runNerdctl to dispatch through limactl only for VM/Lima mode and use the native nerdctl path directly when running on the native runtime. Use the existing runNerdctl and runtime.NerdctlVMArgs symbols to split the command construction so Docker Desktop migration does not depend on a missing Lima binary.backend/internal/api/json.go-9-18 (1)
9-18: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnbounded request body read — potential memory-exhaustion DoS.
io.ReadAll(r.Body)has no size cap, so a malicious or buggy client can send an arbitrarily large payload and exhaust server memory on any endpoint that callsjsonDecode.🛡️ Proposed fix: cap request body size
package api import ( "encoding/json" + "fmt" "io" "net/http" ) +const maxJSONBodyBytes = 10 << 20 // 10MB + func jsonDecode(r *http.Request, payload any) error { defer r.Body.Close() - body, err := io.ReadAll(r.Body) + body, err := io.ReadAll(io.LimitReader(r.Body, maxJSONBodyBytes+1)) if err != nil { return err } + if len(body) > maxJSONBodyBytes { + return fmt.Errorf("request body too large") + } return json.Unmarshal(body, payload) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/json.go` around lines 9 - 18, The jsonDecode helper reads the entire request body with io.ReadAll, which leaves endpoints using it exposed to unbounded memory use. Update jsonDecode to enforce a maximum request size before decoding, ideally by wrapping r.Body with http.MaxBytesReader or otherwise limiting bytes read, and keep the json.Unmarshal flow unchanged for valid payloads.backend/internal/runtime/exec_attach.go-34-90 (1)
34-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose stdin when the exec context ends
exec.CommandContextalready stops the PTY process on cancellation. The remaining hang isio.Copy(ptmx, stdin): inbackend/internal/api/exec.go,stdinWriteronly closes whenconn.ReadMessage()exits, so if the WebSocket stays open after the shell exits,AttachExeccan block forever inwg.Wait(). Close the pipe onctx.Done()or otherwise tie stdin lifetime to the exec context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/runtime/exec_attach.go` around lines 34 - 90, `AttachExec` can hang because the stdin copy goroutine using `io.Copy(ptmx, stdin)` is not tied to the exec lifecycle, so it may keep `wg.Wait()` blocked after the PTY exits. Update the stdin handling in `AttachExec` to close or interrupt the stdin pipe when `execCtx.Done()` is signaled, and ensure the `stdinWriter`/WebSocket-side writer in `backend/internal/api/exec.go` is also shut down on exec completion so the copy goroutine can return cleanly.backend/internal/runtime/lima.yaml-34-58 (1)
34-58: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUnify the VM runtime path.
backend/internal/runtime/lima.goruns all container operations throughnerdctl, butbackend/internal/runtime/lima.yamlalso startsdockerdand forwards/var/run/docker.sock. Those paths won’t share container state by default (nerdctlvsdockerd/mobynamespace), sodocker run hello-worldcan be invisible to Calf’s status/UI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/runtime/lima.yaml` around lines 34 - 58, The Lima bootstrap is mixing two different container runtimes: the startup script in lima.yaml enables dockerd and forwards the Docker socket, while lima.go uses nerdctl for all container actions. Align the runtime path by choosing one stack and updating the script to match the symbols involved here (the nerdctl setup/run block and the portForwards docker.sock entry), so Calf’s container operations and status queries see the same container namespace.backend/internal/api/registry_login.go-81-83 (1)
81-83: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPost-token steps inherit the device-code expiry deadline.
ctxis scoped tostate.ExpiresInseconds for polling the OAuth token. After the token is obtained, the samectxis reused forensureRuntimeRunningandRegistryLogin(Lines 168, 173), which can involve starting the Lima VM. If the token arrives near the end of the polling window, these steps get little to no remaining time and can fail with a deadline error unrelated to their own success/failure.Consider deriving a fresh, independently-bounded context for the post-token steps instead of reusing the polling deadline.
Also applies to: 163-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/registry_login.go` around lines 81 - 83, The registry login flow is reusing the device-code polling context for later work, so post-token steps like ensureRuntimeRunning and RegistryLogin can inherit an almost-expired deadline. In registry_login.go, adjust the login flow around newRegistrySessionID, the polling context creation, and the later ensureRuntimeRunning/RegistryLogin calls so the token polling uses one context and the post-token work gets a fresh independently bounded context with its own timeout.backend/internal/api/registry_login.go-40-45 (1)
40-45: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnsynchronized lazy init of
s.registrySessionsis a data race.
registryLoginSessions()reads and conditionally writess.registrySessionswith no lock/sync.Once. Concurrent calls tohandleRegistryLogin(e.g. simultaneous login attempts) can race here, causing one goroutine'ssync.Mapto be discarded.🔒 Suggested fix using sync.Once
-func (s *Server) registryLoginSessions() *sync.Map { - if s.registrySessions == nil { - s.registrySessions = &sync.Map{} - } - return s.registrySessions -} +func (s *Server) registryLoginSessions() *sync.Map { + s.registrySessionsOnce.Do(func() { + s.registrySessions = &sync.Map{} + }) + return s.registrySessions +}(Requires adding a
registrySessionsOnce sync.Oncefield toServer, or simply initializingregistrySessionseagerly in the server constructor.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/registry_login.go` around lines 40 - 45, `registryLoginSessions` is lazily initializing `s.registrySessions` without synchronization, which can race during concurrent `handleRegistryLogin` calls. Fix this by making the initialization thread-safe: either add a `registrySessionsOnce sync.Once` field to `Server` and initialize the map inside `registryLoginSessions` via `sync.Once`, or eagerly create `registrySessions` in the server constructor so the accessor only returns the existing `sync.Map`.backend/internal/runtime/nerdctl.go-366-374 (1)
366-374: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for log readers before calling
command.Wait()
os/exec.Cmd.StdoutPipe/StderrPiperequire all reads to finish beforeWait(), otherwise the pipes can be closed early and log lines may be lost or reported as read errors. Move the wait until after bothpipeLinesgoroutines complete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/runtime/nerdctl.go` around lines 366 - 374, The log piping in the command execution flow returns from command.Wait() before the StdoutPipe/StderrPipe readers finish, which can truncate logs. In the command-start path that launches pipeLines for stdout and stderr, wait for both goroutines to complete before invoking command.Wait(), using the existing pipeLines helper and the surrounding command.Start/Wait logic to keep all log reads finished first.ui/lib/screens/resources_screen.dart-544-630 (1)
544-630: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDead
_createVolume/_nameController: no create-volume UI is wired up.CI flags
_createVolumeas unused._nameControllerand_createVolumeexist butbuild()never renders an input/button to invoke them — the "create volume" flow appears to have been dropped. Either wire up the UI (input + button calling_createVolume) or remove the dead fields/method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/screens/resources_screen.dart` around lines 544 - 630, The create-volume flow is currently dead because `_createVolume` and `_nameController` in `_VolumesScreenState` are never used by `build()`. Either wire the screen UI to include a name input and an action that invokes `_createVolume`, or remove the unused controller and method if the feature is intentionally dropped. Make sure the fix is applied consistently with the existing `_loadVolumes`, `_loadConfig`, and `dispose()` lifecycle handling.Source: Pipeline failures
backend/internal/oauth/dockerhub/device.go-32-39 (1)
32-39: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
http.DefaultClienthas no timeout; a hung poll can defeat the outer expiry.
NewClientuseshttp.DefaultClient(noTimeout).WaitForDeviceToken'sselectloop only checkstimeout.Cbetween ticks — ifpollDeviceTokenitself hangs on a stalled connection (line 133, inside thecase <-ticker.C), the surrounding timeout never fires because the loop is blocked inside that call. Set a client-level timeout (or a per-request context deadline) so an unresponsive server can't hang the whole login flow indefinitely.Proposed fix
func NewClient() *Client { return &Client{ - HTTP: http.DefaultClient, + HTTP: &http.Client{Timeout: 30 * time.Second}, UserAgent: fmt.Sprintf("calf:0.3.0:%s-%s", runtime.GOOS, runtime.GOARCH), TenantURL: TenantURL, HubURL: Audience, } }Also applies to: 146-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/oauth/dockerhub/device.go` around lines 32 - 39, NewClient is using http.DefaultClient without any timeout, which allows pollDeviceToken to block forever and bypass WaitForDeviceToken’s outer timeout. Update the client construction in NewClient to use a timeout-capable HTTP client, or add a per-request deadline inside pollDeviceToken, so the ticker loop in WaitForDeviceToken can always recover from a stalled connection. Keep the fix centered on the Client HTTP field and the pollDeviceToken call path.backend/internal/api/containers.go-185-217 (1)
185-217: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnbounded request body read on the exec endpoint.
io.ReadAll(r.Body)has no size limit, so a large request body will be fully buffered in memory before the JSON parse even happens. Combine withhttp.MaxBytesReaderand simplify by decoding directly.Proposed fix
- body, err := io.ReadAll(r.Body) - if err != nil { - writeError(w, http.StatusBadRequest, "invalid request body") - return - } - - var request containerExecRequest - if err := json.Unmarshal(body, &request); err != nil { - writeError(w, http.StatusBadRequest, "invalid request body") - return - } + r.Body = http.MaxBytesReader(w, r.Body, 1<<20) + var request containerExecRequest + if err := json.NewDecoder(r.Body).Decode(&request); err != nil { + writeError(w, http.StatusBadRequest, "invalid request body") + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/containers.go` around lines 185 - 217, The exec endpoint in handleContainerExecOnce reads the entire body with io.ReadAll, which can buffer an unbounded payload in memory. Update this flow to cap the request size with http.MaxBytesReader on the ResponseWriter/Request before parsing, and decode the JSON request directly into containerExecRequest instead of reading all bytes first. Keep the existing error handling paths for invalid bodies and ExecContainer results.backend/internal/api/builds.go-23-55 (1)
23-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBuild runs synchronously inside the HTTP request, blocking the handler until completion.
RunBuildis invoked directly in the POST handler and the response is only written once the entire build finishes. Docker/nerdctl builds can take minutes, so this ties up the request goroutine and connection for the whole duration, with no timeout safeguard. It also defeats the async "running" status design already present (newBuildsetsstatus: "running", andBuildsScreenin the UI pollsfetchBuilds()on an interval) — clients never actually observe the "running" state since the request just hangs until success/failure.Proposed fix
build := s.newBuild(payload.Context, payload.Tag, "running") - if err := s.runtime.RunBuild(r.Context(), payload.Context, payload.Tag, payload.Dockerfile); err != nil { - s.updateBuildStatus(build.ID, "failed") - if writeRuntimeError(w, err) { - return - } - - writeError(w, http.StatusInternalServerError, err.Error()) - return - } - - s.updateBuildStatus(build.ID, "success") - writeJSON(w, http.StatusOK, build) + go func() { + ctx := context.Background() + if err := s.runtime.RunBuild(ctx, payload.Context, payload.Tag, payload.Dockerfile); err != nil { + s.updateBuildStatus(build.ID, "failed") + return + } + s.updateBuildStatus(build.ID, "success") + }() + writeJSON(w, http.StatusAccepted, build)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/builds.go` around lines 23 - 55, The POST handling in buildsHandler is doing the build synchronously by calling s.runtime.RunBuild before responding, which blocks the request and prevents clients from observing the intended running state. Refactor the build trigger so the handler returns immediately after s.newBuild and starts the work asynchronously (for example via a goroutine or queued worker), using the build ID from newBuild to update status later. Keep the existing status transitions in updateBuildStatus and preserve the current RunBuild error handling inside the background execution path rather than the HTTP response path.backend/internal/runtime/container_detail.go-204-219 (1)
204-219: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict the container exec API to local/trusted clients.
The daemon listens on:8080by default and there’s no auth middleware, so any reachable client can invokeexecand the other mutating container endpoints. Bind to127.0.0.1by default or add authentication before exposing this surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/runtime/container_detail.go` around lines 204 - 219, The container exec surface is currently reachable without protection, so restrict it to trusted clients before exposing any mutating endpoints. Update the server setup around the daemon/listen path so it binds to 127.0.0.1 by default or add authentication/authorization middleware that guards execInContainer and the other container mutation handlers. Use the existing execInContainer flow as the reference point when verifying the protected paths are no longer callable by arbitrary remote clients.backend/internal/api/server.go-32-36 (1)
32-36: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict WebSocket origins for
/logsand/exec—CheckOriginaccepts every origin, so a malicious site can open these browser websocket endpoints against the default:8080listener and drive container exec/log streaming. The wildcard CORS header doesn’t protect websocket upgrades; allow only trusted app/loopback origins beforeUpgrade.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/server.go` around lines 32 - 36, The websocket upgrader used for the logs and exec endpoints currently allows every origin through CheckOrigin, which leaves browser-accessible websocket upgrades open to untrusted sites. Update logsUpgrader in server.go to validate the request origin against a trusted allowlist (for example, the app’s own origin and loopback/local development origins) before calling Upgrade, and apply the same origin restriction to the exec websocket path if it uses a separate upgrader or shared upgrade logic.backend/internal/api/middleware.go-68-81 (1)
68-81: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict CORS/WebSocket origins to trusted app origins
Access-Control-Allow-Origin: *here, combined withCheckOrigin: trueinbackend/internal/api/server.go, lets any website send cross-origin requests to these unauthenticated container/image/volume/migration endpoints. Use a validated allow-list instead of*.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/middleware.go` around lines 68 - 81, The CORS middleware currently allows any origin, which should be restricted to trusted app origins instead of using a wildcard. Update corsMiddleware to set Access-Control-Allow-Origin only when the request Origin matches an approved allow-list, and keep Access-Control-Allow-Methods/Headers aligned with that policy. Also review the WebSocket origin handling in server.go where CheckOrigin is enabled so it uses the same trusted-origin validation path.Source: Linters/SAST tools
ui/lib/screens/container_detail_screen.dart-125-140 (1)
125-140: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winTab switching leaves the previous tab's stream/timer running.
Only some cases tear down prior work:
_startLogs/_startStatscancel both, but_loadInspectand_loadMountscancel only_statsTimer(not_logsSubscription), and theexec/filescases cancel nothing. As a result:
- Switching logs → inspect/mounts/exec/files leaves
_logsSubscriptionstreaming (accumulating into_logsand callingsetStateoff-screen).- Switching stats → exec/files leaves
_statsTimerfiring_refreshStats(an API call every 2s) indefinitely in the background.Cancel both at the top of
_loadTabDataso each tab starts from a clean state.🔧 Proposed fix
void _loadTabData() { + _logsSubscription?.cancel(); + _statsTimer?.cancel(); switch (_tab) { case _ContainerDetailTab.logs: _startLogs();(With this, the redundant per-method cancels in
_startLogs/_loadInspect/_loadMounts/_startStatscan be simplified.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/screens/container_detail_screen.dart` around lines 125 - 140, _tabData switching in container_detail_screen leaves prior async work running because _loadTabData only starts the next tab and does not fully tear down the previous one. Update _loadTabData to cancel both _logsSubscription and _statsTimer before the switch, then let _startLogs, _loadInspect, _loadMounts, and _startStats assume a clean slate so logs/stats tab changes do not keep background streams or timers alive.
🟡 Minor comments (13)
backend/internal/api/volumes.go-27-44 (1)
27-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate volume name before creating.
payload.Nameisn't checked for emptiness beforeCreateVolumeis called, so an empty/blank name request will likely surface as an opaque runtime/500 error instead of a clear400 Bad Request.🛡️ Proposed validation
if err := jsonDecode(r, &payload); err != nil { writeError(w, http.StatusBadRequest, err.Error()) return } + + if strings.TrimSpace(payload.Name) == "" { + writeError(w, http.StatusBadRequest, "name is required") + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/volumes.go` around lines 27 - 44, The POST handling in volumes.go currently calls s.runtime.CreateVolume without validating payload.Name first, so blank names fall through to a runtime/500 path. Add an explicit empty/whitespace check in the HTTP method switch before CreateVolume is invoked, and return a clear 400 Bad Request via writeError when the name is missing or invalid. Keep the fix localized around the payload decode and CreateVolume call so the behavior is enforced in the volumes handler.backend/internal/browser/open.go-9-27 (1)
9-27: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRestrict
OpenURLto http(s) URIs
backend/internal/api/registry_login.go:95forwards Docker Hub’sverification_uri_completestraight into the browser launcher. Reject non-HTTP(S) schemes before opening to avoid invoking custom handlers or local files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/browser/open.go` around lines 9 - 27, Restrict OpenURL to only http and https URLs before dispatching to the OS browser command. Update the OpenURL function to parse the input URL, reject empty or non-HTTP(S) schemes with an error, and only then build the platform-specific exec.Command for darwin, linux, and windows. This should prevent backend/internal/api/registry_login.go from passing arbitrary verification_uri_complete values into custom handlers or local file openings.backend/internal/runtime/mock.go-156-282 (1)
156-282: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLock the remaining
Mockmutators inbackend/internal/runtime/mock.go:156-282,412-418. These methods updateVolumes,Containers, andStatusValuewithoutm.mu, unlike the rest ofMock, so overlapping requests can still race.RestartContainerinherits the same gap throughStopContainerandStartContainer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/runtime/mock.go` around lines 156 - 282, The remaining Mock mutators are still updating shared state without synchronization, which can race with other concurrent calls. Add m.mu locking around the state-changing paths in Mock methods such as CreateVolume, RemoveVolume, StartContainer, StopContainer, RemoveContainer, RemoveImage, PullImage, PushImage, RunImage, and the RestartContainer flow that calls StopContainer and StartContainer. Keep the locking consistent with the other Mock methods so Volumes, Containers, Images, and StatusValue are always modified under the same mutex.backend/internal/api/images.go-42-49 (1)
42-49: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd an explicit timeout for image actions
backend/internal/api/images.go:42-49,153-161,194-201—PullImage/RunImage/PushImageonly inheritr.Context(), so a stalled registry call can keep the handler goroutine occupied until the client disconnects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/images.go` around lines 42 - 49, The image action handlers currently rely only on r.Context(), so stalled registry operations can hang the request goroutine until client disconnect. Update the PullImage, RunImage, and PushImage flows in images.go to create and use an explicit timeout context for runtime calls, and make sure the timeout is applied consistently before invoking s.runtime.PullImage, s.runtime.RunImage, and s.runtime.PushImage.backend/internal/api/containers.go-74-98 (1)
74-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
DELETEdoesn't validate path shape likePOSTdoes.The
POSTbranch requireslen(parts) == 2before dispatching an action, but theDELETEbranch callsRemoveContainer(id)regardless oflen(parts). ADELETE /v1/containers/{id}/<unknown-suffix>(a suffix not in the logs/inspect/mounts/files/exec/stats set) silently removes the container instead of 404ing.Proposed fix
case http.MethodDelete: + if len(parts) != 1 { + writeError(w, http.StatusNotFound, "container not found") + return + } err = s.runtime.RemoveContainer(r.Context(), id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/containers.go` around lines 74 - 98, The DELETE path in the container handler is missing the same path-shape validation used by POST, so unexpected suffixes can still reach RemoveContainer. Update the containers handler logic around r.Method handling to require len(parts) == 2 before allowing http.MethodDelete to call s.runtime.RemoveContainer, and return methodNotAllowed(w, r) for any DELETE with extra path segments just like the POST branch does for invalid shapes.backend/internal/runtime/registry.go-130-146 (1)
130-146: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMalformed docker auth should not count as a successful login.
dockerConfigAuthUsernamereturnsok=truewhen base64 decoding fails or the decoded value has no username, sodockerConfigCredentialsmarksRegistryStatus.LoggedIn=truefor corruptdocker.ioentries and stops checking other keys. Returnfalsein those cases so they can fall through or stay logged out.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/runtime/registry.go` around lines 130 - 146, The docker auth parsing in dockerConfigAuthUsername is treating malformed credentials as valid, which causes dockerConfigCredentials to mark RegistryStatus.LoggedIn=true too early. Update dockerConfigAuthUsername so decode failures and decoded values without a usable username return false for the ok flag, letting dockerConfigCredentials fall through to other keys or remain logged out. Keep the behavior for valid usernames unchanged.backend/internal/oauth/dockerhub/device.go-146-179 (1)
146-179: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
slow_downas retryable with backoffbackend/internal/oauth/dockerhub/device.go:146-179
slow_downshould not fail the device flow; return a retry signal for it and increase the polling interval by 5s inWaitForDeviceToken, since the current loop always retries at the original interval.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/oauth/dockerhub/device.go` around lines 146 - 179, The device token polling logic in pollDeviceToken currently treats only authorization_pending as retryable, so slow_down is incorrectly surfaced as a hard error. Update pollDeviceToken to return the same retry signal for slow_down, and adjust WaitForDeviceToken to back off by adding 5 seconds to the polling interval whenever slow_down is received before the next poll.ui/lib/screens/resources_screen.dart-492-497 (1)
492-497: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix
unnecessary_underscoreslint failure.CI flags
(_, __)as unnecessary — use a single_for both unused separator-builder params.Proposed fix
- separatorBuilder: (_, __) => Container( + separatorBuilder: (_, _) => Container(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/screens/resources_screen.dart` around lines 492 - 497, The separatorBuilder in SliverList.separated is triggering the unnecessary_underscores lint because it uses two unused parameter names. Update the callback in the resources screen to use a single underscore placeholder for the ignored parameters, keeping the logic in the SliverList.separated builder unchanged.Source: Pipeline failures
backend/internal/config/config.go-123-125 (1)
123-125: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
MemorySwapGB <= 0prevents users from disabling swap.
0is a legitimate value meaning "no swap", but this guard treats it as "unset" and overwrites it with the default. Unlike CPUs/MemoryGB, swap can validly be zero. Consider distinguishing unset from zero (e.g. a pointer in the on-disk model, or only defaulting when negative).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/config/config.go` around lines 123 - 125, The MemorySwapGB defaulting logic in config initialization is treating 0 as unset, which prevents users from explicitly disabling swap. Update the config handling around the MemorySwapGB field so zero is preserved as a valid value and only truly unset/invalid values fall back to defaults, using the relevant config struct and initialization code where cfg.MemorySwapGB is normalized.backend/cmd/calf/main.go-64-86 (1)
64-86: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRuntime is not stopped when the server exits with an error.
In the
errCherror case the functionreturn 1s immediately, so thert.Stop(stopCtx)cleanup at Lines 79-84 is skipped and the Lima VM / native runtime is left running. Only the deferredrtCancel()fires (cancels the start context but doesn't invokeStop). Consider funneling both exit paths through the shutdown/rt.Stopsequence before returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cmd/calf/main.go` around lines 64 - 86, The error path in main currently returns immediately from the select on errCh, which skips the runtime cleanup and leaves the Lima VM/native runtime running. Update the shutdown flow in main so both the errCh and ctx.Done() branches converge on the same cleanup sequence: call rtCancel(), then run rt.Stop with a timeout context, and only then return the appropriate exit code. Use the existing symbols errCh, rtCancel, server.Shutdown, and rt.Stop to locate the affected control flow.backend/internal/api/handlers.go-20-34 (1)
20-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
hostMemoryGB()cross-platform
sysctl -n hw.memsizefails on Linux, so the config endpoint falls back to 8 GB there even though the native Linux runtime path is supported. Branch onruntime.GOOSor read/proc/meminfoon Linux.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/handlers.go` around lines 20 - 34, The hostMemoryGB() helper is macOS-specific because it shells out to sysctl -n hw.memsize, so it falls back incorrectly on Linux. Update hostMemoryGB() in handlers.go to branch on runtime.GOOS and use a Linux-compatible memory source such as /proc/meminfo, while keeping the existing sysctl path for Darwin and preserving the current fallback behavior for errors. Make sure the fix is localized to hostMemoryGB() and its imports.ui/lib/app_shell.dart-374-380 (1)
374-380: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"Start Calf when you sign in" toggle has no effect.
_startAtLoginis only local state — it is never persisted nor sent to the backend, so toggling it does nothing across restarts. This is user-facing and misleading. If start-on-login isn't wired yet for Phase 1, consider disabling the control or labeling it as coming soon.Want me to open a tracking issue for wiring this to config/launch-agent registration?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/app_shell.dart` around lines 374 - 380, The Start Calf sign-in toggle in AppShell is only updating local _startAtLogin state and is not persisted or wired to any backend/launch-agent behavior, so it currently has no effect. Update the AppShell/_settingRow handling so the ShadSwitch either calls into the real start-on-login configuration flow and persists the value, or if that wiring is not available yet, disable the control or relabel it as coming soon to avoid misleading users.ui/lib/screens/container_detail_screen.dart-674-674 (1)
674-674: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix
unnecessary_underscoresanalyzer warning.CI
flutter analyzeflags the double underscore; use a single wildcard.🔧 Proposed fix
- separatorBuilder: (_, __) => const SizedBox(height: 20), + separatorBuilder: (_, _) => const SizedBox(height: 20),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/screens/container_detail_screen.dart` at line 674, The `separatorBuilder` callback in `container_detail_screen.dart` is triggering the `unnecessary_underscores` analyzer warning because it uses two ignored parameters; update the callback to use a single wildcard parameter and keep the `SizedBox` separator unchanged. Locate the fix in the `separatorBuilder` used in the screen’s list layout and adjust the callback signature to satisfy `flutter analyze`.Source: Pipeline failures
🧹 Nitpick comments (10)
backend/go.mod (1)
5-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark
github.com/gorilla/websocketas a direct dependency
backend/go.modlistsgithub.com/gorilla/websocket v1.5.3as// indirect, but it’s imported directly frombackend/internal/api/logs.go,backend/internal/api/exec.go, andbackend/internal/api/server.go.go mod tidyshould clear the stale marker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/go.mod` around lines 5 - 9, The go.mod dependency list is stale because github.com/gorilla/websocket is used directly by backend/internal/api/logs.go, backend/internal/api/exec.go, and backend/internal/api/server.go but is still marked indirect. Update the module metadata by removing the indirect marker for github.com/gorilla/websocket in backend/go.mod and then run go mod tidy so the dependency is recorded as direct and the file stays consistent.backend/internal/runtime/errors.go (1)
10-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating with generics to remove triplicated logic.
emptyContainersIfStopped,emptyImagesIfStopped, andemptyVolumesIfStoppedare identical except for the type parameter. A single generic helper removes the duplication.♻️ Proposed generic consolidation
-func emptyContainersIfStopped(ctx context.Context, statusFn func(context.Context) (Status, error), listFn func(context.Context) ([]Container, error)) ([]Container, error) { - status, err := statusFn(ctx) - if err != nil { - return nil, err - } - - if status.State != StateRunning { - return []Container{}, nil - } - - return listFn(ctx) -} - -func emptyImagesIfStopped(ctx context.Context, statusFn func(context.Context) (Status, error), listFn func(context.Context) ([]Image, error)) ([]Image, error) { - status, err := statusFn(ctx) - if err != nil { - return nil, err - } - - if status.State != StateRunning { - return []Image{}, nil - } - - return listFn(ctx) -} - -func emptyVolumesIfStopped(ctx context.Context, statusFn func(context.Context) (Status, error), listFn func(context.Context) ([]Volume, error)) ([]Volume, error) { - status, err := statusFn(ctx) - if err != nil { - return nil, err - } - - if status.State != StateRunning { - return []Volume{}, nil - } - - return listFn(ctx) -} +func emptyIfStopped[T any](ctx context.Context, statusFn func(context.Context) (Status, error), listFn func(context.Context) ([]T, error)) ([]T, error) { + status, err := statusFn(ctx) + if err != nil { + return nil, err + } + + if status.State != StateRunning { + return []T{}, nil + } + + return listFn(ctx) +}Call sites become
emptyIfStopped[Container](...),emptyIfStopped[Image](...),emptyIfStopped[Volume](...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/runtime/errors.go` around lines 10 - 47, The three helpers emptyContainersIfStopped, emptyImagesIfStopped, and emptyVolumesIfStopped in errors.go are duplicated except for their return types. Replace them with one generic helper such as emptyIfStopped[T] that takes the same statusFn and listFn shape and returns []T, then update the existing call sites to use emptyIfStopped[Container], emptyIfStopped[Image], and emptyIfStopped[Volume] so the repeated logic is consolidated.backend/test/runtime/registry_test.go (1)
9-26: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueLikely false positive from static analysis — test fixture, not a real credential.
Decoding the base64 value yields a synthetic
enegalan:tckr_pat_-style string used purely to exercise JSON parsing; the token prefix doesn't match Docker's real PAT format. Consider swapping for an obviously fake value (e.g. base64 of"user:test-token") to avoid tripping future secret scanners in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/runtime/registry_test.go` around lines 9 - 26, The test fixture in TestRegistryStatusFromConfigDetectsDockerHubAuth uses a secret-like base64 value that can trip scanners even though it is synthetic; replace the auth payload with an obviously fake credential string in the registry status test data while keeping the same JSON shape so RegistryStatusFromConfig and the username assertion still exercise the parsing path.Source: Linters/SAST tools
backend/internal/runtime/mock.go (2)
380-395: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
select/defaultpattern can delay reacting toctxcancellation whilestdin.Readis blocked.Same pattern flagged in
exec_attach.go; here it's mock/test-only code so impact is limited, but worth aligning for consistency if the production fix is adopted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/runtime/mock.go` around lines 380 - 395, The stdin read loop in mock runtime code still uses the same select/default pattern that can keep blocking on stdin.Read before noticing ctx cancellation. Update the loop in the function that reads from stdin to follow the same cancellation-friendly approach used for the exec attach fix, so ctx.Done is checked without relying on a default branch that can delay exit. Keep the existing onOutput and error handling behavior intact while making cancellation responsiveness consistent.
300-318: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUnescaped container name in hand-built JSON.
nameis interpolated with%sinside a manually-quoted string whileidcorrectly uses%q. A name containing"or\would produce malformed JSON. Building the payload via a struct +json.Marshal(or at least%qforname) avoids this class of bug.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/runtime/mock.go` around lines 300 - 318, The hand-built JSON in the container inspection mock is unsafe because MockRuntime uses a raw string with the container name inserted via %s, which can break JSON when the name contains quotes or backslashes. Update the payload construction in the function that builds the response to either marshal a struct with json.Marshal or at minimum quote the name with %q so the returned json.RawMessage is always valid.backend/internal/api/images.go (1)
27-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate reference-payload decoding across three handlers.
The same "decode JSON → validate
referencenon-empty" block is repeated inhandleImages(POST),handleImageRun, andhandleImagePush.♻️ Proposed helper extraction
+func decodeReferencePayload(w http.ResponseWriter, r *http.Request) (string, bool) { + var payload struct { + Reference string `json:"reference"` + } + if err := jsonDecode(r, &payload); err != nil { + writeError(w, http.StatusBadRequest, err.Error()) + return "", false + } + if payload.Reference == "" { + writeError(w, http.StatusBadRequest, "reference is required") + return "", false + } + return payload.Reference, true +}Then each handler becomes
ref, ok := decodeReferencePayload(w, r); if !ok { return }.Also applies to: 139-167, 169-204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/images.go` around lines 27 - 55, The POST branch in handleImages repeats the same JSON decode and reference validation logic used by handleImageRun and handleImagePush. Extract that shared “decode JSON → require non-empty reference” flow into a helper such as decodeReferencePayload, then update handleImages, handleImageRun, and handleImagePush to call it and return early when it fails. Keep the helper responsible for writing the bad-request response so the handlers stay focused on runtime work like PullImage, RunImage, and PushImage.backend/internal/api/exec.go (1)
32-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo deadlines/keepalive/read-limit on the exec WebSocket connection.
There's no
SetReadDeadline/SetWriteDeadline, ping/pong handling, orSetReadLimitonconn. A silently-dead peer (no TCP FIN) can leave the reader goroutine andAttachExecrunning indefinitely, and an unbounded message size could be used to grow memory. Worth adding basic WS hardening consistent with what's likely already needed for the logs WebSocket.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/exec.go` around lines 32 - 81, Add basic hardening to handleContainerExecWebSocket in exec.go: the current conn usage has no read/write deadlines, ping/pong keepalive, or read-size limit, so a dead peer or oversized message can keep AttachExec and the reader goroutine alive indefinitely. Update the websocket setup around logsUpgrader.Upgrade to set a reasonable SetReadLimit, add SetReadDeadline plus pong/ping handlers, and refresh deadlines while reading/writing so the connection fails fast on inactivity and enforces bounded message sizes.backend/internal/api/server.go (1)
68-80: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
WriteTimeout: 0disables write timeouts on every endpoint.Setting it to
0is necessary for the long-lived WebSocket/log-stream routes, but it also removes slow-write protection from all plain JSON endpoints, leaving them exposed to slow-consumer/DoS pressure. Consider applyinghttp.ResponseController.SetWriteDeadline(or a per-handler timeout) only on the streaming routes and restoring a finiteWriteTimeouton the server.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/server.go` around lines 68 - 80, The server-wide WriteTimeout is currently disabled in Server.Run, which removes slow-write protection from all handlers. Update the http.Server configuration in Server.Run to restore a finite WriteTimeout, and move the unlimited write behavior to the streaming/WebSocket routes only by applying a per-handler deadline via http.ResponseController.SetWriteDeadline or an equivalent handler-specific timeout. Use Server.Run and the streaming route setup in s.Handler() as the main places to adjust.ui/lib/screens/containers_screen.dart (1)
151-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the platform URL abstraction instead of
Process.run('open', ...).This hardcodes the macOS
openbinary, so the "Open port" action silently does nothing on Linux (the native runtime target). The rest of the app already routes external URLs throughopenExternalUrl(ui/platform/open_url.dart). The same pattern is duplicated incontainer_detail_screen.dart(Line 298-303); fixing here and there keeps behavior consistent across platforms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/screens/containers_screen.dart` around lines 151 - 156, The _openPort helper in containers_screen.dart is hardcoded to use Process.run('open', ...) and only works on macOS, so replace that direct process call with the existing openExternalUrl abstraction from ui/platform/open_url.dart. Update _openPort to route the localhost URL through the shared URL-opening path, and apply the same change in the duplicated container_detail_screen equivalent so the Open port action behaves consistently on Linux and other platforms.backend/internal/runtime/native.go (1)
276-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant
if command == "nerdctl"branches — both paths are identical.In
runLocalandrunLocalWithStdin, thenerdctlbranch and the fallthrough return exactly the same call, so the condition is dead code. Either simplify to the single return, or if thenerdctlcase was meant to differ (e.g., sudo/namespace handling as in the Lima path), implement that intent — the current form suggests an incomplete implementation.🔧 Proposed simplification (if no divergent behavior is intended)
func (n *Native) runLocal(ctx context.Context, command string, args ...string) ([]byte, error) { - if command == "nerdctl" { - return runCommand(ctx, command, args...) - } - return runCommand(ctx, command, args...) } func (n *Native) runLocalWithStdin(ctx context.Context, stdin, command string, args ...string) ([]byte, error) { - if command == "nerdctl" { - return runCommandWithStdin(ctx, stdin, command, args...) - } - return runCommandWithStdin(ctx, stdin, command, args...) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/runtime/native.go` around lines 276 - 290, The `runLocal` and `runLocalWithStdin` methods in `Native` contain redundant `if command == "nerdctl"` branches whose two paths call the same helpers. Remove the dead conditional and keep a single return path in each method, unless `nerdctl` is supposed to behave differently. If special handling is intended, implement that logic explicitly in the `nerdctl` branch instead of leaving identical fallthrough code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 948fa0e8-da74-4748-9005-39d149322539
⛔ Files ignored due to path filters (3)
.DS_Storeis excluded by!**/.DS_Storebackend/go.sumis excluded by!**/*.sumui/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (70)
.github/workflows/ci.yml.gitignoreCHANGELOG.mdDEVELOPMENT.mdMakefileREADME.mdROADMAP.mdbackend/cmd/calf/main.gobackend/go.modbackend/internal/api/builds.gobackend/internal/api/containers.gobackend/internal/api/exec.gobackend/internal/api/handlers.gobackend/internal/api/images.gobackend/internal/api/json.gobackend/internal/api/logs.gobackend/internal/api/middleware.gobackend/internal/api/migrate.gobackend/internal/api/registry.gobackend/internal/api/registry_login.gobackend/internal/api/runtime_errors.gobackend/internal/api/runtime_ready.gobackend/internal/api/server.gobackend/internal/api/version.gobackend/internal/api/volumes.gobackend/internal/browser/open.gobackend/internal/config/config.gobackend/internal/config/config.yamlbackend/internal/migration/disk.gobackend/internal/migration/disk_test.gobackend/internal/migration/docker_desktop.gobackend/internal/migration/status.gobackend/internal/oauth/dockerhub/device.gobackend/internal/oauth/dockerhub/device_test.gobackend/internal/runtime/command_error.gobackend/internal/runtime/container_detail.gobackend/internal/runtime/errors.gobackend/internal/runtime/exec.gobackend/internal/runtime/exec_attach.gobackend/internal/runtime/lima.gobackend/internal/runtime/lima.yamlbackend/internal/runtime/localhost_proxy.gobackend/internal/runtime/mock.gobackend/internal/runtime/native.gobackend/internal/runtime/nerdctl.gobackend/internal/runtime/registry.gobackend/internal/runtime/runtime.gobackend/test/api/api_test.gobackend/test/config/config_test.gobackend/test/runtime/command_error_test.gobackend/test/runtime/image_history_test.gobackend/test/runtime/localhost_proxy_test.gobackend/test/runtime/nerdctl_test.gobackend/test/runtime/registry_test.goui/lib/api/client.dartui/lib/app_shell.dartui/lib/main.dartui/lib/platform/open_url.dartui/lib/screens/container_detail_screen.dartui/lib/screens/containers_screen.dartui/lib/screens/resources_screen.dartui/lib/storage/container_groups.dartui/lib/widgets/app_top_bar.dartui/lib/widgets/calf_button.dartui/lib/widgets/hover_list_row.dartui/macos/Runner/Configs/AppInfo.xcconfigui/macos/Runner/DebugProfile.entitlementsui/macos/Runner/Release.entitlementsui/pubspec.yamlui/test/widget_test.dart
💤 Files with no reviewable changes (1)
- .gitignore
| func (s *Server) handleContainerFiles(w http.ResponseWriter, r *http.Request, id string) { | ||
| if r.Method != http.MethodGet { | ||
| methodNotAllowed(w, r) | ||
| return | ||
| } | ||
|
|
||
| path := strings.TrimSpace(r.URL.Query().Get("path")) | ||
| files, err := s.runtime.ListContainerFiles(r.Context(), id, path) | ||
| if err != nil { | ||
| if writeRuntimeError(w, err) { | ||
| return | ||
| } | ||
|
|
||
| writeError(w, http.StatusInternalServerError, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| writeJSON(w, http.StatusOK, files) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
ast-grep outline backend/internal/api/containers.go --view expanded || true
ast-grep outline backend/internal/runtime/container_detail.go --view expanded || true
# Locate ListContainerFiles and related call sites/implementations
rg -n "ListContainerFiles|listContainerFiles|sh -c|exec.Command|exec\.CommandContext|/bin/sh|shell" backend/internal -S
# Show the handler region with line numbers
sed -n '140,210p' backend/internal/api/containers.go | cat -n
# Show the runtime implementation region if present
sed -n '1,260p' backend/internal/runtime/container_detail.go | cat -nRepository: enegalan/calf
Length of output: 17340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the path validator and any direct use of it
sed -n '286,340p' backend/internal/runtime/container_detail.go | cat -n
# Find all references to isValidContainerPath
rg -n "isValidContainerPath\(" backend/internal -S
# Show the command runner abstraction used by listContainerFiles
sed -n '1,220p' backend/internal/runtime/exec.go | cat -nRepository: enegalan/calf
Length of output: 1807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the runtime entry points that call listContainerFiles
sed -n '224,246p' backend/internal/runtime/native.go | cat -n
sed -n '312,332p' backend/internal/runtime/lima.go | cat -n
# Quick probe of Go %q output for shell-sensitive payloads
python3 - <<'PY'
samples = [
'$(id)',
'`id`',
'foo bar',
'a"b',
'a\\b',
]
for s in samples:
# approximate Go %q behavior using Python repr is not identical, so just show intent
print(repr(s))
PYRepository: enegalan/calf
Length of output: 1585
Shell injection via container path
path is only trimmed here; the runtime still accepts values like /$(id) and formats them into sh -c via fmt.Sprintf("ls -la %q", dirPath), so command substitution/backticks can execute. Avoid the shell here or enforce a strict path allowlist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/api/containers.go` around lines 161 - 179, The container
file listing path is only trimmed in handleContainerFiles, but it is still
passed through to the runtime where it can be embedded into a shell command.
Update ListContainerFiles and its callers so the path is never formatted into sh
-c; instead use a non-shell filesystem lookup or strictly validate the path
against an allowlist of safe container-relative paths before listing files.
| func listContainerFiles(ctx context.Context, run commandRunner, id, dirPath string) ([]ContainerFileEntry, error) { | ||
| if dirPath == "" { | ||
| dirPath = "/" | ||
| } | ||
|
|
||
| script := fmt.Sprintf("ls -la %q", dirPath) | ||
| output, err := run(ctx, "nerdctl", "exec", id, "sh", "-c", script) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return parseLsOutput(dirPath, output), nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Command injection via dirPath in listContainerFiles.
script := fmt.Sprintf("ls -la %q", dirPath) is executed through nerdctl exec id sh -c script. Go's %q (strconv.Quote) produces a Go-style double-quoted literal — it escapes backslashes/quotes but does not neutralize $, backticks, or other POSIX shell metacharacters. Inside a shell's double quotes, $(...) command substitution and backtick execution are still active. A path value like /tmp/$(id>&2) (reachable via the unauthenticated GET /v1/containers/{id}/files?path=... endpoint in containers.go) executes arbitrary commands inside the container's shell. The isValidContainerPath helper defined later in this same file (lines 294-310) is never called anywhere in this diff, suggesting the intended validation was dropped.
The safest fix is to avoid the shell entirely — ls doesn't need one — and additionally validate the path.
Proposed fix
func listContainerFiles(ctx context.Context, run commandRunner, id, dirPath string) ([]ContainerFileEntry, error) {
if dirPath == "" {
dirPath = "/"
}
+ if !isValidContainerPath(dirPath) {
+ return nil, fmt.Errorf("invalid path: %q", dirPath)
+ }
- script := fmt.Sprintf("ls -la %q", dirPath)
- output, err := run(ctx, "nerdctl", "exec", id, "sh", "-c", script)
+ output, err := run(ctx, "nerdctl", "exec", id, "ls", "-la", dirPath)
if err != nil {
return nil, err
}
return parseLsOutput(dirPath, output), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func listContainerFiles(ctx context.Context, run commandRunner, id, dirPath string) ([]ContainerFileEntry, error) { | |
| if dirPath == "" { | |
| dirPath = "/" | |
| } | |
| script := fmt.Sprintf("ls -la %q", dirPath) | |
| output, err := run(ctx, "nerdctl", "exec", id, "sh", "-c", script) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return parseLsOutput(dirPath, output), nil | |
| } | |
| func listContainerFiles(ctx context.Context, run commandRunner, id, dirPath string) ([]ContainerFileEntry, error) { | |
| if dirPath == "" { | |
| dirPath = "/" | |
| } | |
| if !isValidContainerPath(dirPath) { | |
| return nil, fmt.Errorf("invalid path: %q", dirPath) | |
| } | |
| output, err := run(ctx, "nerdctl", "exec", id, "ls", "-la", dirPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return parseLsOutput(dirPath, output), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/runtime/container_detail.go` around lines 128 - 140, The
`listContainerFiles` path handling is vulnerable because `fmt.Sprintf("ls -la
%q", dirPath)` is passed to `nerdctl exec ... sh -c`, so shell expansion can
still occur for malicious `dirPath` values. Fix this by removing the shell from
`listContainerFiles` and invoking `ls` directly through `commandRunner`, and
make sure the existing `isValidContainerPath` helper in this file is actually
used to reject unsafe paths before execution.
- Changed default API listen address from :8080 to :8765 in DEVELOPMENT.md, README.md, and configuration files. - Updated Makefile to reflect the new backend listen address. - Introduced a log broadcaster for improved log streaming in the API. - Enhanced WebSocket handling for container logs and exec commands. - Added new volume management endpoints and improved existing volume handling. These changes enhance the development setup and improve API functionality.
- Reformatted the `containerInspect` struct for improved readability by aligning the fields. - No functional changes were made; this is purely a cosmetic update to enhance code clarity.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
ui/lib/screens/container_detail_screen.dart (2)
143-159: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUnbounded growth of
_logLines.Every received line is appended with no cap (line 158). For a long-running, verbose container watched over an extended session, this list grows without bound, increasing client memory usage over time.
♻️ Suggested fix
+ static const _maxLogLines = 5000; + _logsSubscription = widget.apiClient.streamContainerLogs(_container.id).listen( (line) { if (!mounted) { return; } setState(() { _logsError = null; _logLines.add(LogLine(text: line, receivedAt: DateTime.now())); + if (_logLines.length > _maxLogLines) { + _logLines.removeRange(0, _logLines.length - _maxLogLines); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/screens/container_detail_screen.dart` around lines 143 - 159, The `_startLogs` stream handler is appending every received line to `_logLines` without any limit, so the in-memory log list can grow indefinitely during long sessions. Update the `streamContainerLogs` listener in `ContainerDetailScreen._startLogs` to enforce a fixed maximum buffer size for `_logLines` (for example, drop the oldest entries before adding a new `LogLine`) so the list stays bounded while preserving recent output.
126-141: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winBackground log stream / stats polling isn't stopped when switching to Exec or Files tabs.
_loadTabData'sexec/filescases do nothing, so whatever was previously running keeps going: a log subscription started on the Logs tab stays open indefinitely on Inspect/Mounts/Exec/Files (only_startLogs/_startStatscancel it), and a stats timer started on the Stats tab keeps pollingfetchContainerStatsevery 2s forever once the user navigates to Exec or Files (only_startLogs/_loadInspect/_loadMounts/_startStatscancel it). Centralize cleanup instead of relying on each per-tab starter to remember it.♻️ Suggested fix
void _loadTabData() { + _statsTimer?.cancel(); + _logsSubscription?.cancel(); switch (_tab) { case _ContainerDetailTab.logs: _startLogs(); case _ContainerDetailTab.inspect: _loadInspect(); case _ContainerDetailTab.mounts: _loadMounts(); case _ContainerDetailTab.exec: - break; case _ContainerDetailTab.files: break; case _ContainerDetailTab.stats: _startStats(); } }Also applies to: 175-228, 230-235
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/screens/container_detail_screen.dart` around lines 126 - 141, The _loadTabData switch in ContainerDetailScreen leaves the exec and files tabs without cleanup, so any active log stream or stats timer continues running after navigation. Update _loadTabData to centralize stopping the background subscriptions before handling the new tab, and make sure the exec/files cases explicitly cancel any active log/stats work. Use the existing helpers in _ContainerDetailScreenState such as _startLogs, _loadInspect, _loadMounts, and _startStats as the reference points, but move the cancellation logic into a shared cleanup path so switching tabs always stops prior polling/streaming.backend/internal/api/logs.go (1)
24-61: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd read/write deadlines and ping/pong keepalive
ReadMessagecan block forever on a dead peer, andWriteMessagehas no write deadline either. AddSetReadDeadline/SetPongHandlerplus a ping ticker here so silent disconnects release the connection instead of pinning the goroutine and socket.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/logs.go` around lines 24 - 61, The websocket loop in handleContainerLogsWebSocket currently reads and writes without any timeout or keepalive, so a dead peer can leave the goroutine and socket hanging. Update this handler to set read deadlines with SetReadDeadline, refresh them via a SetPongHandler, and add a periodic ping ticker that sends pings while the connection is alive. Also ensure writes through wsWriter.writeMessage have a write deadline so silent disconnects are detected and the connection is released promptly.ui/lib/screens/resources_screen.dart (1)
949-956: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRedundant empty-state branch always renders the same text.
Both branches of this ternary produce
'No builds yet.', so the_runtime?.state == 'stopped'check is dead code. Compare withImagesScreen/VolumesScreenabove, which show a distinct "Runtime is stopped" message — this was likely intended here too.💚 Proposed fix
_searchQuery.isNotEmpty ? 'No builds match "$_searchQuery".' : _runtime?.state == 'stopped' - ? 'No builds yet.' + ? 'No builds. Runtime is stopped.' : 'No builds yet.',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/screens/resources_screen.dart` around lines 949 - 956, The empty-state ternary in the resources screen has a dead `_runtime?.state == 'stopped'` branch because both outcomes currently render the same “No builds yet.” text. Update the logic in `ResourcesScreen` to match the pattern used by `ImagesScreen` and `VolumesScreen`, so the stopped-runtime case shows a distinct “Runtime is stopped” message while the default case keeps the generic empty state.backend/cmd/calf/main.go (2)
64-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop the runtime when the server fails early.
This return path skips the explicit
rtCancel()/rt.Stop()block below, so a partially started Lima VM or proxy can be left running afterserver.Run()fails.Ensure runtime cleanup before returning
case err := <-errCh: if err != nil { logger.Error("server stopped", "error", err) + rtCancel() + stopCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if stopErr := rt.Stop(stopCtx); stopErr != nil { + logger.Error("runtime stop failed", "error", stopErr) + } return 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cmd/calf/main.go` around lines 64 - 69, The early-return path in main’s server startup handling skips runtime cleanup when server.Run fails, leaving the Lima VM or proxy running. Update the errCh case in main so it performs the same cleanup as the normal shutdown path by calling rtCancel() and rt.Stop() before returning 1, using the existing runtime variables in main to ensure any partially started runtime is always stopped.
37-39: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not SIGTERM arbitrary port owners.
ensurePortis described as finding the listener on the configured port and sending SIGTERM. A configured port collision may be an unrelated user process, not a previous Calf instance. Prefer checking the PID file/process identity, otherwise fail startup.Safer call-site behavior
if err := ensurePort(cfg.ListenAddr); err != nil { - logger.Warn("cleaned up previous instance", "error", err) + logger.Error("listen address unavailable", "addr", cfg.ListenAddr, "error", err) + return 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cmd/calf/main.go` around lines 37 - 39, The startup logic in main should not terminate whatever process happens to own cfg.ListenAddr when ensurePort fails. Update the call site around ensurePort in main so it first verifies the PID file or process identity belongs to a previous Calf instance, and only then allow cleanup; otherwise treat the port collision as a startup error and stop rather than sending SIGTERM to an arbitrary process. Keep the change scoped to the ensurePort handling path and preserve the logger.Warn behavior only for confirmed Calf cleanup cases.backend/internal/migration/docker_desktop.go (1)
507-515: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not count containers as migrated when restart fails.
The destination
starterror is ignored, butContainersOKis still incremented. Running source containers can be reported as successfully migrated while left stopped on Calf.Propagate start failures into the summary
if running[name] { + var startErr error if opts.RunNerdctl != nil { - _ = opts.RunNerdctl(ctx, "start", name) + startErr = opts.RunNerdctl(ctx, "start", name) } else { - _, _ = runDocker(ctx, opts.CalfSocket, "start", name) + _, startErr = runDocker(ctx, opts.CalfSocket, "start", name) + } + if startErr != nil { + if opts.Logger != nil { + opts.Logger.Warn("container start failed", "container", name, "error", startErr) + } + continue } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/migration/docker_desktop.go` around lines 507 - 515, The container restart path in docker_desktop migration is swallowing failures: in the running[name] block, the opts.RunNerdctl/runDocker "start" result is ignored and status.Summary.ContainersOK is always incremented. Update this flow so the start error is captured and propagated into the migration summary, only incrementing ContainersOK when the start succeeds; otherwise record the failure on the same migration/status path used by the rest of the docker_desktop code.
🧹 Nitpick comments (6)
ui/lib/screens/volume_detail_screen.dart (1)
108-130: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo confirmation before destructive volume removal.
_removeVolumedeletes the volume immediately on button press with no confirmation step, risking accidental, irreversible data loss.♻️ Suggested fix
Future<void> _removeVolume() async { + final confirmed = await showShadDialog<bool>( + context: context, + builder: (context) => ShadDialog.alert( + title: const Text('Remove volume?'), + description: Text('This will permanently delete "${widget.volumeName}" and its data.'), + actions: [ + ShadButton.outline(onPressed: () => Navigator.of(context).pop(false), child: const Text('Cancel')), + ShadButton.destructive(onPressed: () => Navigator.of(context).pop(true), child: const Text('Remove')), + ], + ), + ); + if (confirmed != true) { + return; + } setState(() { _busy = true; });Also applies to: 210-215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/screens/volume_detail_screen.dart` around lines 108 - 130, The _removeVolume flow in VolumeDetailScreen deletes immediately without a safeguard, so add a confirmation step before calling widget.apiClient.removeVolume. Update the _removeVolume method (and the button/action that triggers it) to present a confirm dialog or equivalent prompt, and only proceed with the deletion when the user explicitly confirms. Keep the existing mounted checks and busy/error handling in VolumeDetailScreen intact.ui/lib/screens/compose_group_detail_screen.dart (1)
196-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win"Open port" is a no-op on Linux.
_openPortonly handlesPlatform.isMacOS; on Linux it silently does nothing. Given this PR targets native Linux support as a first-class runtime, consider adding anxdg-openfallback for parity.♻️ Proposed fix
void _openPort(int port) { - if (!Platform.isMacOS) { - return; + if (Platform.isMacOS) { + Process.run('open', ['http://localhost:$port']); + } else if (Platform.isLinux) { + Process.run('xdg-open', ['http://localhost:$port']); } - Process.run('open', ['http://localhost:$port']); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/screens/compose_group_detail_screen.dart` around lines 196 - 201, The _openPort helper in compose_group_detail_screen.dart is currently a no-op outside macOS, so Linux users cannot open the local URL from the UI. Update _openPort to handle Linux as well as macOS by adding an xdg-open fallback alongside the existing open call, and keep the existing Platform checks so the correct command is chosen per platform. Use the _openPort method as the location for the fix.ui/lib/widgets/logs_panel.dart (3)
67-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSearch/toolbar logic duplicated across three state classes.
_LogsPanelState,_MixedLogsPanelState, and_ExecPanelStateeach carry near-identical_searchPattern,_findMatches/highlight logic,_goToMatch,_copyToClipboard, and toolbar wiring. Consider extracting the shared search-state (query, regex toggle, current match index, navigation) into a reusable mixin/controller shared by all three, since_LogsViewerChromealready unifies the presentation layer.Also applies to: 253-477, 1012-1291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/widgets/logs_panel.dart` around lines 67 - 234, The search and toolbar behavior is duplicated across _LogsPanelState, _MixedLogsPanelState, and _ExecPanelState, so extract the shared search state and navigation logic into a reusable mixin or controller. Move the common _searchPattern, _findMatches, _goToMatch, _copyToClipboard, and match-index/reset handling into the shared abstraction, then have each state class wire it into _LogsViewerChrome. Keep the panel-specific rendering only in the three state classes and use the existing symbols _LogsPanelState and _LogsViewerChrome to anchor the refactor.
951-993: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
matches.indexOfinside a per-line loop is quadratic in match count.
globalIndex = matches.indexOf(match)(Line 973) does a linear scan of the full match list for every match on every line. For logs with many matches (a common search term across a large log), this becomes O(n²) on every keystroke/repaint. Track the global index alongside each match when it's first computed (e.g., store(match, globalIndex)pairs) instead of re-deriving it viaindexOf.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/widgets/logs_panel.dart` around lines 951 - 993, The `_lineSpans` method in `logs_panel.dart` is re-deriving each match’s global position with `matches.indexOf(match)`, which makes rendering scale quadratically as match counts grow. Update the search/match model so each match carries its global index when created, then have `_lineSpans` use that stored index instead of scanning `matches` inside the per-line loop. Keep the highlight logic for `currentMatchIndex` unchanged, but avoid any repeated linear lookup in the span-building path.
567-574: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftNon-virtualized rendering may not scale for a live, real-time log viewer.
_LogTextViewbuilds every line's spans into a singleSelectableText.rich, and_LogsViewerChromewraps that in a plainSingleChildScrollViewwhenprimaryListViewis false (Lines 567-574) instead of a virtualizedListView.builder(asMixedLogsPanelalready does per-block). Since this PR's stated goal is a real-time container log viewer, long-running containers with large/continuous output could accumulate thousands of lines and cause layout jank, since every span is laid out eagerly regardless of visibility.Also applies to: 902-949
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/widgets/logs_panel.dart` around lines 567 - 574, The log viewer currently renders all text eagerly through _LogTextView and wraps it in a plain SingleChildScrollView in _LogsViewerChrome when primaryListView is false, which won’t scale for long real-time output. Update the _LogsViewerChrome / scrollableContent path to use a virtualized list-based approach like MixedLogsPanel and _LogsViewerChrome’s primaryListView branch, so only visible log rows are built and laid out. Keep the existing selection/scroll behavior while moving the rendering into a lazily built ListView.builder or equivalent virtualized structure.backend/test/runtime/nerdctl_test.go (1)
121-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert every inferred service in this case.
The test validates all projects but only one service, so frontend/mongodb service inference can regress unnoticed.
Proposed test tightening
- if byName["typing-app-backend"].ComposeService != "backend" { - t.Fatalf("expected compose service backend, got %q", byName["typing-app-backend"].ComposeService) - } + expectedServices := map[string]string{ + "typing-app-backend": "backend", + "typing-app-frontend": "frontend", + "typing-app-mongodb": "mongodb", + } + for name, expected := range expectedServices { + if byName[name].ComposeService != expected { + t.Fatalf("%s: expected compose service %s, got %q", name, expected, byName[name].ComposeService) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/runtime/nerdctl_test.go` around lines 121 - 123, The runtime test only asserts the inferred compose service for typing-app-backend, so regressions in the other inferred services can slip through. Tighten the assertions in nerdctl_test.go by checking every entry in byName for this case, including the frontend and mongodb service mappings, using the existing byName lookup in the test to verify each service name is inferred as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/api/log_broadcaster.go`:
- Around line 49-53: The shared log stream reuse logic in log_broadcaster.go can
attach a new subscriber to a stream that is already canceling, causing live logs
not to restart. Update the subscriber-join path around shouldStart and the
sharedLogStream state so a stream marked as stopping/canceling is treated as
unavailable and a fresh stream is created instead of reusing it. Use the
existing sharedLogStream, stream.cancel, and runStream flow to add a stopping
check (for example via an isStopping helper) before reusing the stream, and make
the cleanup path clear the canceling state once the stream is fully removed from
b.streams.
- Around line 57-61: The history replay path in log_broadcaster.go is using
blocking sends into the shared subscriber channel, which can stall publish for
everyone when one client is slow. Update the replay loop around the history
fan-out to use the same non-blocking/backpressure-safe approach as the live
path, and make sure the publish flow in the broadcaster methods (including the
live fan-out referenced in the comment) never blocks on a full subscriber
channel. Use the existing log broadcaster symbols such as publish and the
history replay goroutine to locate both send sites and apply the same
bounded-slow-client behavior consistently.
In `@backend/internal/migration/compose_migration.go`:
- Around line 162-164: Do not write inspected container environment back into
the migrated compose file by default, since patchComposeServiceEnv and the
Compose migration flow can leak secrets and override existing
env_file/interpolation behavior. Update the compose migration path around
patchComposeServiceEnv and the related service patching logic to preserve the
original Compose environment unless an explicit opt-in or allowlist is provided
for materializing runtime env values.
In `@backend/internal/migration/docker_desktop.go`:
- Around line 450-456: The migration loop in docker_desktop.go stops source
containers too early and never restores them if the Calf migration later fails.
Update the flow around the container inspection/stop logic and the migration
entry points in the Docker Desktop migration path so you only stop containers
after the migration is confirmed to be able to complete, or track the containers
stopped by this loop and restart them on any compose/standalone migration
failure. Make sure the fix is applied in the same area as the runDocker stop
call and the overall migration orchestration so original workloads are restored
when migration does not succeed.
- Around line 833-843: The named volume handling in the Docker Desktop migration
path drops mount access options, so read-only volumes get recreated as writable.
Update the `volume` case in `docker_desktop.go` alongside the existing `bind`
logic to preserve `mount.Mode` and/or `RW` when building the bind string for
`binds`, using the same mount metadata flow as the other mount types.
In `@backend/internal/runtime/command_error.go`:
- Around line 103-129: The transient detection in isTransientCommandError is too
broad because the generic "no such file or directory" and "executable file not
found" markers can classify permanent path or missing-binary failures as
retryable. Tighten the matching in this helper by scoping these cases to the
specific startup-related errors expected from runCommandWithRetry, while keeping
the other existing transient markers intact.
In `@backend/internal/runtime/exec.go`:
- Around line 14-19: The helper wrappers currently make every command retry by
default, which can repeat non-idempotent operations like limactl create. Change
runCommand and runCommandWithStdin to use a single-shot execution path by
default, and reserve runCommandWithRetry only for audited, repeat-safe call
sites. Update callers in exec.go and any other sites that rely on retries so the
retry behavior is explicitly opt-in and tied to safe command paths.
In `@backend/internal/runtime/mock.go`:
- Around line 172-198: CloneVolume reads and mutates m.Volumes without
synchronization, unlike the other volume helpers in Mock. Add m.mu locking
around the entire CloneVolume method in backend/internal/runtime/mock.go, using
the same mutex pattern as the adjacent volume read APIs so the source lookup,
destination existence check, and append are all protected from concurrent test
races.
In `@backend/internal/runtime/volume_detail.go`:
- Around line 71-80: The command construction in the volume size logic is
shell-building unsafe paths and the sudo fallback can block waiting for
credentials. Update the path handling in the volume detail code paths around the
size checks to avoid using sh -c with fmt.Sprintf("%q") and strings.Join, and
instead pass the arguments directly through run with the command and path list
as argv. For the sudo fallback in the same functions, add a non-interactive
failure mode so it does not hang waiting for input, and apply the same fix
consistently in the other referenced size-check call sites.
- Around line 235-236: The named-volume match in parseContainerMounts is using
the wrong field: for volume mounts, Name holds the volume identifier while
Source is the host path, so the current volumeName comparison will miss named
volumes. Update the mount filtering and the in-use lookup logic to compare
against mount.Name for volume mounts, keeping Source only for path-based mounts.
Use parseContainerMounts and the volume-in-use check in volume_detail.go as the
places to adjust.
In `@backend/test/api/api_test.go`:
- Around line 540-544: The WebSocket test read loop can hang indefinitely if the
connection goes silent, so add a short read deadline before calling
conn.ReadMessage() in the test flow. Update the logic around the
conn.ReadMessage() loop to set a deadline on the WebSocket connection before
entering the for range 3 loop, and keep it short enough to fail fast in CI while
still allowing expected messages to arrive.
In `@ui/lib/screens/compose_group_detail_screen.dart`:
- Around line 164-194: The bulk remove flow in _runAction and _runGroupAction
swallows failures by only setting _error, so the caller cannot tell whether the
operation succeeded. Update the remove-all path to only call widget.onBack()
when _runGroupAction completes successfully, and make sure failures
short-circuit navigation so the error state can remain visible. Use the existing
_runAction/_runGroupAction helpers and the remove button handler in
ComposeGroupDetailScreen to locate the fix.
In `@ui/lib/screens/resources_screen.dart`:
- Line 548: The volume creation flow in ResourcesScreen is currently dead
because _nameController and _createVolume() are defined but never invoked. Wire
these into the UI by adding the appropriate create-volume action from the
ResourcesScreen widget so users can trigger volume creation, and ensure the
controller’s value is used during submission. If this screen is not meant to
support volume creation anymore, remove the unused _nameController and
_createVolume() helper from ResourcesScreen instead.
---
Outside diff comments:
In `@backend/cmd/calf/main.go`:
- Around line 64-69: The early-return path in main’s server startup handling
skips runtime cleanup when server.Run fails, leaving the Lima VM or proxy
running. Update the errCh case in main so it performs the same cleanup as the
normal shutdown path by calling rtCancel() and rt.Stop() before returning 1,
using the existing runtime variables in main to ensure any partially started
runtime is always stopped.
- Around line 37-39: The startup logic in main should not terminate whatever
process happens to own cfg.ListenAddr when ensurePort fails. Update the call
site around ensurePort in main so it first verifies the PID file or process
identity belongs to a previous Calf instance, and only then allow cleanup;
otherwise treat the port collision as a startup error and stop rather than
sending SIGTERM to an arbitrary process. Keep the change scoped to the
ensurePort handling path and preserve the logger.Warn behavior only for
confirmed Calf cleanup cases.
In `@backend/internal/api/logs.go`:
- Around line 24-61: The websocket loop in handleContainerLogsWebSocket
currently reads and writes without any timeout or keepalive, so a dead peer can
leave the goroutine and socket hanging. Update this handler to set read
deadlines with SetReadDeadline, refresh them via a SetPongHandler, and add a
periodic ping ticker that sends pings while the connection is alive. Also ensure
writes through wsWriter.writeMessage have a write deadline so silent disconnects
are detected and the connection is released promptly.
In `@backend/internal/migration/docker_desktop.go`:
- Around line 507-515: The container restart path in docker_desktop migration is
swallowing failures: in the running[name] block, the opts.RunNerdctl/runDocker
"start" result is ignored and status.Summary.ContainersOK is always incremented.
Update this flow so the start error is captured and propagated into the
migration summary, only incrementing ContainersOK when the start succeeds;
otherwise record the failure on the same migration/status path used by the rest
of the docker_desktop code.
In `@ui/lib/screens/container_detail_screen.dart`:
- Around line 143-159: The `_startLogs` stream handler is appending every
received line to `_logLines` without any limit, so the in-memory log list can
grow indefinitely during long sessions. Update the `streamContainerLogs`
listener in `ContainerDetailScreen._startLogs` to enforce a fixed maximum buffer
size for `_logLines` (for example, drop the oldest entries before adding a new
`LogLine`) so the list stays bounded while preserving recent output.
- Around line 126-141: The _loadTabData switch in ContainerDetailScreen leaves
the exec and files tabs without cleanup, so any active log stream or stats timer
continues running after navigation. Update _loadTabData to centralize stopping
the background subscriptions before handling the new tab, and make sure the
exec/files cases explicitly cancel any active log/stats work. Use the existing
helpers in _ContainerDetailScreenState such as _startLogs, _loadInspect,
_loadMounts, and _startStats as the reference points, but move the cancellation
logic into a shared cleanup path so switching tabs always stops prior
polling/streaming.
In `@ui/lib/screens/resources_screen.dart`:
- Around line 949-956: The empty-state ternary in the resources screen has a
dead `_runtime?.state == 'stopped'` branch because both outcomes currently
render the same “No builds yet.” text. Update the logic in `ResourcesScreen` to
match the pattern used by `ImagesScreen` and `VolumesScreen`, so the
stopped-runtime case shows a distinct “Runtime is stopped” message while the
default case keeps the generic empty state.
---
Nitpick comments:
In `@backend/test/runtime/nerdctl_test.go`:
- Around line 121-123: The runtime test only asserts the inferred compose
service for typing-app-backend, so regressions in the other inferred services
can slip through. Tighten the assertions in nerdctl_test.go by checking every
entry in byName for this case, including the frontend and mongodb service
mappings, using the existing byName lookup in the test to verify each service
name is inferred as expected.
In `@ui/lib/screens/compose_group_detail_screen.dart`:
- Around line 196-201: The _openPort helper in compose_group_detail_screen.dart
is currently a no-op outside macOS, so Linux users cannot open the local URL
from the UI. Update _openPort to handle Linux as well as macOS by adding an
xdg-open fallback alongside the existing open call, and keep the existing
Platform checks so the correct command is chosen per platform. Use the _openPort
method as the location for the fix.
In `@ui/lib/screens/volume_detail_screen.dart`:
- Around line 108-130: The _removeVolume flow in VolumeDetailScreen deletes
immediately without a safeguard, so add a confirmation step before calling
widget.apiClient.removeVolume. Update the _removeVolume method (and the
button/action that triggers it) to present a confirm dialog or equivalent
prompt, and only proceed with the deletion when the user explicitly confirms.
Keep the existing mounted checks and busy/error handling in VolumeDetailScreen
intact.
In `@ui/lib/widgets/logs_panel.dart`:
- Around line 67-234: The search and toolbar behavior is duplicated across
_LogsPanelState, _MixedLogsPanelState, and _ExecPanelState, so extract the
shared search state and navigation logic into a reusable mixin or controller.
Move the common _searchPattern, _findMatches, _goToMatch, _copyToClipboard, and
match-index/reset handling into the shared abstraction, then have each state
class wire it into _LogsViewerChrome. Keep the panel-specific rendering only in
the three state classes and use the existing symbols _LogsPanelState and
_LogsViewerChrome to anchor the refactor.
- Around line 951-993: The `_lineSpans` method in `logs_panel.dart` is
re-deriving each match’s global position with `matches.indexOf(match)`, which
makes rendering scale quadratically as match counts grow. Update the
search/match model so each match carries its global index when created, then
have `_lineSpans` use that stored index instead of scanning `matches` inside the
per-line loop. Keep the highlight logic for `currentMatchIndex` unchanged, but
avoid any repeated linear lookup in the span-building path.
- Around line 567-574: The log viewer currently renders all text eagerly through
_LogTextView and wraps it in a plain SingleChildScrollView in _LogsViewerChrome
when primaryListView is false, which won’t scale for long real-time output.
Update the _LogsViewerChrome / scrollableContent path to use a virtualized
list-based approach like MixedLogsPanel and _LogsViewerChrome’s primaryListView
branch, so only visible log rows are built and laid out. Keep the existing
selection/scroll behavior while moving the rendering into a lazily built
ListView.builder or equivalent virtualized structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 580f1e05-cbdd-4567-b1a1-a4c616f4b795
📒 Files selected for processing (44)
DEVELOPMENT.mdMakefileREADME.mdbackend/cmd/calf/main.gobackend/internal/api/exec.gobackend/internal/api/log_broadcaster.gobackend/internal/api/logs.gobackend/internal/api/runtime_errors.gobackend/internal/api/server.gobackend/internal/api/volumes.gobackend/internal/api/ws_writer.gobackend/internal/config/config.gobackend/internal/config/config.yamlbackend/internal/migration/compose_migration.gobackend/internal/migration/compose_migration_test.gobackend/internal/migration/docker_desktop.gobackend/internal/runtime/command_error.gobackend/internal/runtime/exec.gobackend/internal/runtime/inspect_decode.gobackend/internal/runtime/inspect_decode_test.gobackend/internal/runtime/lima.gobackend/internal/runtime/localhost_proxy.gobackend/internal/runtime/mock.gobackend/internal/runtime/native.gobackend/internal/runtime/nerdctl.gobackend/internal/runtime/runtime.gobackend/internal/runtime/volume_detail.gobackend/test/api/api_test.gobackend/test/config/config_test.gobackend/test/runtime/command_error_test.gobackend/test/runtime/localhost_proxy_test.gobackend/test/runtime/nerdctl_test.gobackend/test/runtime/volume_detail_test.goui/lib/api/client.dartui/lib/app_shell.dartui/lib/screens/compose_group_detail_screen.dartui/lib/screens/container_detail_screen.dartui/lib/screens/containers_screen.dartui/lib/screens/resources_screen.dartui/lib/screens/volume_detail_screen.dartui/lib/widgets/calf_button.dartui/lib/widgets/files_panel.dartui/lib/widgets/logs_panel.dartui/test/widget_test.dart
✅ Files skipped from review due to trivial changes (2)
- README.md
- DEVELOPMENT.md
🚧 Files skipped from review as they are similar to previous changes (12)
- backend/internal/config/config.yaml
- backend/internal/api/runtime_errors.go
- backend/internal/api/exec.go
- backend/internal/config/config.go
- Makefile
- ui/test/widget_test.dart
- backend/internal/runtime/localhost_proxy.go
- ui/lib/widgets/calf_button.dart
- ui/lib/screens/containers_screen.dart
- backend/internal/api/server.go
- ui/lib/api/client.dart
- ui/lib/app_shell.dart
- Adjusted the test assertion in `TestVolumeFilesReturnsList` to expect 2 files instead of 1, reflecting changes in the volume file handling logic. - This ensures the test accurately verifies the expected behavior of the API.
- Updated separator builders in various screens to improve code consistency by removing unused parameters. - Removed the `_nameController` and related volume creation logic from the `_VolumesScreenState` class, streamlining the code and enhancing maintainability.
- Updated command execution functions to use a retry mechanism for improved reliability. - Enhanced WebSocket handling for container logs, including ping/pong management to maintain connection stability. - Introduced a new log line sending mechanism to prevent blocking on subscriber channels. - Improved container migration logic by ensuring proper handling of stopped containers and volume mount specifications. - Added support for additional fields in container mount details to enhance volume management capabilities. These changes improve the robustness and functionality of the runtime and logging systems.
- Modified the test in `volume_detail_test.go` to reflect the correct number of arguments for the `sudo` command, ensuring accurate testing of command execution logic. - This change enhances the reliability of the test by aligning it with the expected command structure. refactor(ui): improve text overflow handling in navigation item - Updated the `_NavItem` widget in `app_shell.dart` to use an `Expanded` widget for the label text, allowing for proper text overflow handling with ellipsis. - This change improves the UI by ensuring that long labels do not overflow the available space, enhancing user experience.
- Removed the 'Docker Hub' text from the account menu button in `app_top_bar.dart`, streamlining the UI and improving clarity. - This change enhances the overall user experience by eliminating unnecessary elements.
1 participant
Summary
calf start,calf stop, andcalf statusCLI commandsCloses #10
Closes #11
Closes #12
Closes #13
Closes #14
Closes #15
Closes #16
Closes #17
Closes #18
Test plan
CGO_ENABLED=0 go test ./...flutter analyze && flutter testmake backendcalf start && docker run hello-worldMade with Cursor
Summary by CodeRabbit
:8765.