refactor(errors): handle error classification, codes, and exits#698
Conversation
✅ Deploy Preview for devsydev canceled.
|
✅ Deploy Preview for images-devsy-sh canceled.
|
📝 WalkthroughWalkthroughThe PR centralizes CLI error classification, standardizes wrapped error propagation and retryable exit codes, updates MCP and desktop error payloads, and refactors agent workspace discovery, cloning, and daemon activity handling. ChangesCLI error handling and workspace refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant clierr
participant Output
CLI->>clierr: Classify failure
clierr-->>Output: Provide code and message
Output-->>CLI: Render error and select exit code
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
60cc74c to
ef0edb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/agent/workspace.go (1)
435-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant gRPC error message wrapping.
Wrapping a gRPC status error with its own message using
%wwill result in a duplicated error string (e.g.,<message>: rpc error: code = ... desc = <message>). Since the error is properly wrapped with context at the end of this block (line 447), this intermediate wrapping can be safely removed to prevent redundant log and CLI output.♻️ Proposed fix
- if statusErr, ok := status.FromError(err); ok && statusErr.Message() != "" { - err = fmt.Errorf("%s: %w", statusErr.Message(), err) - }🤖 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 `@pkg/agent/workspace.go` around lines 435 - 437, Remove the intermediate status-message wrapping in the error handling block around status.FromError, leaving err unchanged there; retain the existing contextual wrapping at the end of the block so gRPC errors are reported without duplicated messages.
🤖 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 `@cmd/root.go`:
- Around line 105-116: Move the collector declaration before the panic-recovery
defer in run, initialize it once telemetry BootstrapCLI completes, and record
the recovered panic through the collector inside the defer before assigning the
structured exit code. Preserve the existing logging and exit behavior.
---
Nitpick comments:
In `@pkg/agent/workspace.go`:
- Around line 435-437: Remove the intermediate status-message wrapping in the
error handling block around status.FromError, leaving err unchanged there;
retain the existing contextual wrapping at the end of the block so gRPC errors
are reported without duplicated messages.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a40951a-ea18-4161-ab53-ad62e8870f1e
📒 Files selected for processing (34)
cmd/internal/agentworkspace/build.gocmd/internal/agentworkspace/setup_gpg.gocmd/mcp/errors.gocmd/pro/start.gocmd/provider/configure_shared.gocmd/root.gocmd/root_test.godesktop/src/main/__tests__/cli.test.tsdesktop/src/renderer/src/lib/components/ErrorCard.sveltedesktop/src/shared/cli-error.tspkg/agent/agent.gopkg/agent/dockerless.gopkg/agent/workspace.gopkg/client/clientimplementation/workspace_client.gopkg/clierr/errors.gopkg/clierr/errors_test.gopkg/command/command.gopkg/credentials/request.gopkg/daemon/platform/workspace_watcher.gopkg/errors/errors.gopkg/errors/errors_test.gopkg/errors/patterns.gopkg/exitcode/codes.gopkg/log/logger.gopkg/provider/version_cache.gopkg/provider/versions_github.gopkg/provider/versions_github_test.gopkg/selfupdate/errors.gopkg/selfupdate/errors_test.gopkg/selfupdate/source.gopkg/telemetry/collect.gopkg/tunnel/browser.gopkg/tunnel/browser_test.gopkg/workspace/workspace.go
💤 Files with no reviewable changes (7)
- pkg/errors/errors.go
- cmd/internal/agentworkspace/build.go
- pkg/provider/version_cache.go
- pkg/agent/agent.go
- desktop/src/shared/cli-error.ts
- pkg/errors/errors_test.go
- pkg/errors/patterns.go
…ndling
Make CLIError{code,message} the single structured error surface, built only in pkg/clierr. Libraries return sentinel/typed/wrapped errors; classification happens once at the boundary via clierr.Classify(err).
- Rename pkg/errors -> pkg/clierr so it no longer shadows the stdlib errors package; importers use it unaliased alongside stdlib errors.
- Canonical sentinels (ErrWorkspaceNotFound, ErrRateLimited) live in pkg/clierr like io.EOF; workspace, provider, and selfupdate wrap them, and Classify maps them to codes inline (no registry, no init indirection).
- Preserve error chains: add Unwrap() to command.Error and pro.Error; wrap the gRPC clone site with %w.
- ProviderExecError feeds provider-init stderr to the fingerprint table; classification stays init-scoped so other commands keep transparent child exit codes.
- Exit codes: 0 success, 1 failure, 75 WorkspaceNotFound (consumed by the backhaul SSH retry).
- Unify the MCP error surface onto the shared Code vocabulary; slim CLIError to code+message.
- Root-level panic recovery; propagate the workspace-watcher panic instead of swallowing it; stop double-logging (log-and-return -> wrap-and-return).
- Reduce comments to non-obvious rationale.
ef0edb9 to
184528f
Compare
- Decompose CloneRepositoryForWorkspace into a params struct plus focused helpers (satisfies cyclop/funlen/argument-limit). - Replace the write-and-exec noexec probe in isDirExecutable with a build-tagged Statfs check (execmount_linux.go / execmount_other.go), dropping four gosec suppressions and a temp-file race. - Fix latent bugs: grpc client leak, dead net.Dialer, unbounded append aliasing, defer-in-loop and partial-failure temp-key leak in setupSSHKey, unclosed .devsyignore file. - Static errors use errors.New; rename folder vernacular to dir/directory across the agent home API and callers; make log and error statements direct and consistent; strip narrative comments. - BREAKING(internal): FindAgentHomeFolder/GetAgentDaemonLogFolder/PrepareAgentHomeFolder/ErrFindAgentHomeFolder renamed to *Dir.
- Rename doOnce -> patrolOnce (doOnce implied run-once semantics; it runs every tick). - Extract workspaceConfigs() shared by patrolOnce and initialTouch; build the glob with filepath.Join instead of string concatenation. - Handle FindAgentHomeDir failures consistently (was silently swallowed) and unify glob-error wording. - Use a time.Ticker for the patrol loop; extract pollInterval() and warn on an unparseable --interval. - Add context to the previously bare shutdown-environment error; pass latestActivity by value; name the busy-file grace period; strip narrative comments.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/internal/agent_daemon.go (1)
190-204: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPrevent data races on combined output buffer.
Using a single
&bytes.Buffer{}for bothStdoutandStderrcan lead to a data race (and a runtime panic) ifclientimplementation.RunCommandstreams output concurrently, which is standard behavior whenos/execis used under the hood.Use separate buffers for
StdoutandStderrto guarantee thread safety and to provide clearer error logging.🔒️ Proposed fix
- buf := &bytes.Buffer{} + stdoutBuf := &bytes.Buffer{} + stderrBuf := &bytes.Buffer{} err = clientimplementation.RunCommand(clientimplementation.RunCommandOptions{ Ctx: ctx, Command: workspace.Agent.Exec.Shutdown, Environ: environ, - Stdout: buf, - Stderr: buf, + Stdout: stdoutBuf, + Stderr: stderrBuf, }) if err != nil { - log.Errorf("run shutdown command %s: %v (output: %s)", shutdown, err, buf.String()) + log.Errorf("run shutdown command %s: %v (stdout: %s, stderr: %s)", shutdown, err, stdoutBuf.String(), stderrBuf.String()) return } - log.Infof("ran shutdown command: %s", buf.String()) + log.Infof("ran shutdown command (stdout: %s, stderr: %s)", stdoutBuf.String(), stderrBuf.String())🤖 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 `@cmd/internal/agent_daemon.go` around lines 190 - 204, Update the shutdown command flow to use separate buffers for stdout and stderr instead of sharing one buffer between clientimplementation.RunCommand’s Stdout and Stderr. Adjust the success and error log messages to include the appropriate captured output, preserving the existing shutdown command handling.
🤖 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 `@cmd/internal/agent_daemon.go`:
- Around line 255-259: Update the busy-workspace branch in the activity
calculation to use the current time, such as time.Now() or
time.Now().Add(busyGracePeriod), instead of stat.ModTime(). Preserve
stat.ModTime() for non-busy workspaces so active busy workspaces do not become
eligible for shutdown solely due to an unchanged config timestamp.
- Around line 88-98: Update DaemonCmd.pollInterval to validate that the parsed
duration is strictly positive before returning it. For zero or negative values,
log the invalid interval and return defaultPatrolInterval, preserving the
existing fallback behavior used for parse errors.
---
Outside diff comments:
In `@cmd/internal/agent_daemon.go`:
- Around line 190-204: Update the shutdown command flow to use separate buffers
for stdout and stderr instead of sharing one buffer between
clientimplementation.RunCommand’s Stdout and Stderr. Adjust the success and
error log messages to include the appropriate captured output, preserving the
existing shutdown command handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eeaf4002-285b-4ed0-8ff7-b83540c210c2
📒 Files selected for processing (1)
cmd/internal/agent_daemon.go
…nterval panic - Use separate stdout/stderr buffers for the shutdown command: RunCommand's emulated-shell path (single-arg commands) can write both streams concurrently, and bytes.Buffer is not concurrency-safe. Also logs the streams separately. - Reject non-positive --interval values in pollInterval; time.NewTicker panics on d<=0 and time.ParseDuration accepts "0s"/"-1s" without error. Skipped the busy-workspace ModTime suggestion: it changes long-standing semantics (a busy workspace would never auto-stop) and is unrelated to this refactor.
…stop The machine agent daemon judged inactivity solely from the workspace config mtime, which interactive sessions never update. Meanwhile devsy ssh runs ssh-server --track-activity, heartbeating ContainerActivityFile every 10s on both containers and machines; the container daemon reads it but the machine daemon did not — so a machine with an active SSH session (and no build) could be auto-stopped mid-use. patrolOnce now also considers the activity-file mtime, taking whichever signal is more recent. Additive by construction: it can only delay a stop for a genuinely-active machine, never hasten one, and it self-heals since the heartbeat stops when the session's ssh-server process exits. Full consolidation of the machine and container auto-stop daemons onto a single heartbeat model is left as follow-up; it needs validation on a real machine provider.
Add unit tests for getActivity (shutdown-configured, no-shutdown, busy-file grace, read error), findLatestActivity, and effectiveActivity (fresh heartbeat honored; absent/stale falls back to config activity). Make activityFilePath an overridable package var and extract effectiveActivity so the heartbeat decision is testable without touching real /tmp.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/internal/agent_daemon_test.go (1)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
assert.Equalfortime.Timecomparisons.Multiple tests use
assert.True(t, a.Equal(b))which obfuscates the actual timestamps on failure. Testify'sassert.Equalnatively supportstime.Timeequality checks and provides helpful diffs (e.g.expected 2026-07-21 15:00:00, got 2026-07-21 15:00:00.123).
cmd/internal/agent_daemon_test.go#L45-L45: Replaceassert.Truewithassert.Equal(t, stat.ModTime(), *activity).cmd/internal/agent_daemon_test.go#L68-L68: Replaceassert.Truewithassert.Equal(t, stat.ModTime().Add(busyGracePeriod), *activity).cmd/internal/agent_daemon_test.go#L109-L109: Replaceassert.Truewithassert.Equal(t, configActivity, effectiveActivity(configActivity)).cmd/internal/agent_daemon_test.go#L118-L118: Replaceassert.Truewithassert.Equal(t, configActivity, effectiveActivity(configActivity)).🛠️ Proposed fixes
cmd/internal/agent_daemon_test.go#L45-L45
- assert.True(t, activity.Equal(stat.ModTime())) + assert.Equal(t, stat.ModTime(), *activity)cmd/internal/agent_daemon_test.go#L68-L68
- assert.True(t, activity.Equal(stat.ModTime().Add(busyGracePeriod))) + assert.Equal(t, stat.ModTime().Add(busyGracePeriod), *activity)cmd/internal/agent_daemon_test.go#L109-L109
- assert.True(t, effectiveActivity(configActivity).Equal(configActivity)) + assert.Equal(t, configActivity, effectiveActivity(configActivity))cmd/internal/agent_daemon_test.go#L118-L118
- assert.True(t, effectiveActivity(configActivity).Equal(configActivity)) + assert.Equal(t, configActivity, effectiveActivity(configActivity))🤖 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 `@cmd/internal/agent_daemon_test.go` at line 45, Replace the boolean time comparisons in cmd/internal/agent_daemon_test.go at lines 45, 68, 109, and 118 with assert.Equal calls: compare stat.ModTime() to *activity, stat.ModTime().Add(busyGracePeriod) to *activity, and configActivity to effectiveActivity(configActivity) at both remaining sites.
🤖 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 `@cmd/internal/agent_daemon_test.go`:
- Around line 81-90: Prevent filesystem timestamp precision flakiness in the
mtime tests: in cmd/internal/agent_daemon_test.go lines 81-90, truncate oldTime
and recentTime to seconds before os.Chtimes and use assert.Equal for the
recentTime comparison; in lines 112-115, truncate freshTime to seconds and use
assert.Equal for its comparison. Apply these changes to the relevant
findLatestActivity test cases.
---
Nitpick comments:
In `@cmd/internal/agent_daemon_test.go`:
- Line 45: Replace the boolean time comparisons in
cmd/internal/agent_daemon_test.go at lines 45, 68, 109, and 118 with
assert.Equal calls: compare stat.ModTime() to *activity,
stat.ModTime().Add(busyGracePeriod) to *activity, and configActivity to
effectiveActivity(configActivity) at both remaining sites.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 174391e6-2ddc-4617-96b5-7bc658f7e737
📒 Files selected for processing (2)
cmd/internal/agent_daemon.gocmd/internal/agent_daemon_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/internal/agent_daemon.go
Truncate os.Chtimes times to whole seconds so mtime round-trips exactly on coarse-granularity filesystems (also strips the monotonic clock reading), then use assert.Equal for clearer failure diffs. Safe now that truncation removes the monotonic/precision mismatch that would otherwise break testify's reflect-based time comparison.
There was a problem hiding this comment.
Pull request overview
This PR refactors the CLI/desktop error surface by introducing a new minimal structured error contract (pkg/clierr) and wiring it through CLI exit handling, telemetry, MCP, and desktop UI, while also consolidating retryable exit codes and tightening error-wrapping/unwrap behavior across several subsystems.
Changes:
- Introduces
pkg/clierr(codes + JSON/log shape) and updates CLI, telemetry, logging, MCP, and desktop parsing/UI to use the new{code,message}contract. - Consolidates exit codes into
pkg/exitcode(including a retryable75) and updates backhaul retry logic accordingly. - Refactors agent workspace directory discovery/exec-mount checks, git clone flow, and daemon/workspace watcher reliability improvements with new/updated tests.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workspace/workspace.go | Updates workspace-not-found sentinel documentation to align with retryable exit behavior. |
| pkg/tunnel/browser.go | Switches transient backhaul retry detection to the new retryable exit code. |
| pkg/tunnel/browser_test.go | Updates retry/exit-code test to use the new retryable constant. |
| pkg/telemetry/collect.go | Records error codes via clierr.Classify instead of the removed classifier. |
| pkg/selfupdate/source.go | Routes GitHub API errors through a rate-limit classifier helper. |
| pkg/selfupdate/errors.go | Adds GitHub rate-limit error classification into clierr.ErrRateLimited. |
| pkg/selfupdate/errors_test.go | Tests rate-limit wrapping and chain preservation for self-update. |
| pkg/provider/versions_github.go | Uses clierr.ErrRateLimited as the canonical sentinel for rate limits. |
| pkg/provider/versions_github_test.go | Updates provider rate-limit test to assert clierr.ErrRateLimited. |
| pkg/provider/version_cache.go | Removes the old provider-specific rate-limit sentinel. |
| pkg/provider/dir.go | Updates agent-home function naming in comments. |
| pkg/log/logger.go | Updates JSON error logging to the new clierr.CLIError shape. |
| pkg/exitcode/codes.go | Replaces old single code with a small set of named exit codes (incl. Retryable=75). |
| pkg/errors/patterns.go | Deletes the old fingerprint table implementation. |
| pkg/errors/errors.go | Deletes the old structured error type + classifier. |
| pkg/errors/errors_test.go | Deletes tests tied to the removed legacy error classifier. |
| pkg/daemon/platform/workspace_watcher.go | Propagates watcher goroutine panics to the caller instead of swallowing them. |
| pkg/credentials/request.go | Converts log-and-return into wrap-and-return for request retry failures. |
| pkg/config/pathmanager.go | Updates agent-home function naming in comments. |
| pkg/command/command.go | Adds Unwrap() to preserve error chains for command errors. |
| pkg/clierr/errors.go | Adds the new minimal structured CLI error contract and classifier. |
| pkg/clierr/errors_test.go | Adds tests for CLI error codes, wrapping, and JSON/log shapes. |
| pkg/client/clientimplementation/workspace_client.go | Removes double-logging; wraps and returns errors with context. |
| pkg/agent/workspace.go | Refactors agent dir discovery, git clone flow, and ignore handling; improves cleanup + error wrapping. |
| pkg/agent/execmount_other.go | Adds non-Linux exec-mount probe stub. |
| pkg/agent/execmount_linux.go | Adds Linux exec-mount probe using Statfs and ST_NOEXEC. |
| pkg/agent/dockerless.go | Converts log-and-return into wrap-and-return with captured stderr context. |
| pkg/agent/agent.go | Renames agent-home sentinel and improves error wrapping around rerun-as-root path. |
| desktop/src/shared/cli-error.ts | Simplifies the desktop-side CLIError wire contract to {code,message}. |
| desktop/src/renderer/src/lib/components/ErrorCard.svelte | Removes hint/doc UI elements to match the simplified wire contract. |
| desktop/src/main/tests/cli.test.ts | Updates CLI stderr parsing test to match the new error payload shape. |
| cmd/root.go | Centralizes classification at exit boundary; adds panic recovery and new exit-code mapping. |
| cmd/root_test.go | Adds exit-code tests and adjusts Cobra silencing assertions. |
| cmd/provider/configure_shared.go | Removes legacy stderr fingerprinting/classification during provider init and wraps errors. |
| cmd/pro/start.go | Adds Unwrap() to preserve error chains for command errors. |
| cmd/mcp/errors.go | Switches MCP tool error payload classification to clierr. |
| cmd/internal/agentworkspace/up.go | Updates clone invocation to new CloneWorkspaceParams struct. |
| cmd/internal/agentworkspace/setup_gpg.go | Converts multiple log-and-return sites into wrap-and-return. |
| cmd/internal/agentworkspace/logs_daemon.go | Renames daemon log folder->dir helper and adds a #nosec rationale. |
| cmd/internal/agentworkspace/build.go | Removes double-logging; wraps and returns build errors. |
| cmd/internal/agentcontainer/setup.go | Updates clone invocation to new CloneWorkspaceParams struct. |
| cmd/internal/agent_daemon.go | Refactors patrol loop, validates intervals, improves activity detection (incl. heartbeat file), and tightens logging/errors. |
| cmd/internal/agent_daemon_test.go | Adds unit tests for activity selection, busy grace period, and heartbeat precedence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Stay transparent for unclassified child-process exits (e.g. `devsy ssh -- cmd`). | ||
| if cliErr.Code == clierr.CodeUnknown { | ||
| if code, ok := passthroughExitCode(err, machineMode); ok { | ||
| return code | ||
| } | ||
| } |
| if err != nil { | ||
| return cliErrors.Classify(fmt.Errorf("init: %w", err), cliErrors.ClassifyContext{ | ||
| Provider: provider.Name, | ||
| Stderr: stderrBuf.String(), | ||
| }) | ||
| return fmt.Errorf("init: %w", err) | ||
| } |
Summary
Reworks CLI error handling into a single, idiomatic pipeline: libraries return sentinel/typed/wrapped errors, and classification happens once at the boundary.
CLIError{code,message}is the only structured error crossing process boundaries, and it is constructed only inpkg/errors.errors.Classify(err)resolves an embeddedCLIError→ known sentinel → provider-stderr fingerprint →UNKNOWN. The CLI exit path, MCP, and telemetry all funnel through it.ErrWorkspaceNotFound,ErrRateLimited) live inpkg/errorslikeio.EOF;workspace,provider, andselfupdatewrap them, andClassifymaps them to codes inline — no registry or init-time indirection.Unwrap()where it was missing and replaced chain-breaking reconstruction with%w.init(where config/creds errors surface), so other provider commands keep transparent child exit codes.0/1+75(WORKSPACE_NOT_FOUND, consumed by the backhaul SSH retry).Draft: opening for review.
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX
Tests