Skip to content

refactor(errors): handle error classification, codes, and exits#698

Merged
skevetter merged 8 commits into
mainfrom
refactor/error-handling
Jul 21, 2026
Merged

refactor(errors): handle error classification, codes, and exits#698
skevetter merged 8 commits into
mainfrom
refactor/error-handling

Conversation

@skevetter

@skevetter skevetter commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Reworks CLI error handling into a single, idiomatic pipeline: libraries return sentinel/typed/wrapped errors, and classification happens once at the boundary.

  • One surface: CLIError{code,message} is the only structured error crossing process boundaries, and it is constructed only in pkg/errors.
  • Classify at the boundary: errors.Classify(err) resolves an embedded CLIError → known sentinel → provider-stderr fingerprint → UNKNOWN. The CLI exit path, MCP, and telemetry all funnel through it.
  • Canonical sentinels (ErrWorkspaceNotFound, ErrRateLimited) live in pkg/errors like io.EOF; workspace, provider, and selfupdate wrap them, and Classify maps them to codes inline — no registry or init-time indirection.
  • Chain integrity: added Unwrap() where it was missing and replaced chain-breaking reconstruction with %w.
  • Provider fingerprinting is scoped to init (where config/creds errors surface), so other provider commands keep transparent child exit codes.
  • Exit codes: 0 / 1 + 75 (WORKSPACE_NOT_FOUND, consumed by the backhaul SSH retry).
  • Panics are recovered at the root and surfaced as a structured error; the workspace-watcher goroutine now propagates panics instead of swallowing them.
  • Removed log-and-return double-logging (converted to wrap-and-return) and trimmed comments to non-obvious rationale.

Draft: opening for review.

Summary by CodeRabbit

  • New Features

    • Standardized CLI error classification with stable codes (rate-limited, panic, unknown) and simplified JSON error payloads.
    • Panic handling now converts failures into consistent exit codes.
  • Bug Fixes

    • Workspace-not-found now maps to a retryable exit status.
    • Build (including dockerless) and GPG setup failures now stop immediately and return clearer, contextual errors.
    • Transient backhaul failures now key off the retryable exit status.
    • Provider init/version-list rate-limit behavior maps consistently.
  • UI/UX

    • Simplified error cards by removing hint/doc “Learn more” content.
  • Tests

    • Expanded coverage for exit-code mapping and rate-limit error parsing/classification.

@netlify

netlify Bot commented Jul 20, 2026

Copy link
Copy Markdown

Deploy Preview for devsydev canceled.

Name Link
🔨 Latest commit c6ee869
🔍 Latest deploy log https://app.netlify.com/projects/devsydev/deploys/6a5fcd6596056d0008c7753f

@netlify

netlify Bot commented Jul 20, 2026

Copy link
Copy Markdown

Deploy Preview for images-devsy-sh canceled.

Name Link
🔨 Latest commit c6ee869
🔍 Latest deploy log https://app.netlify.com/projects/images-devsy-sh/deploys/6a5fcd657274c60009e4205c

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The 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.

Changes

CLI error handling and workspace refactor

Layer / File(s) Summary
CLI error classifier and unwrap contracts
pkg/clierr/*, pkg/command/command.go, cmd/pro/start.go, pkg/log/logger.go
Adds stable CLI error codes, classification, panic conversion, JSON/log encoding, and standard error unwrapping.
CLI entrypoint and payload integration
cmd/root.go, cmd/mcp/errors.go, pkg/telemetry/collect.go, cmd/provider/configure_shared.go, desktop/src/...
Routes failures through clierr, simplifies output to code/message, updates exit-code handling, and removes hint/documentation fields from desktop and MCP payloads.
Rate-limit and retryable outcome contracts
pkg/provider/..., pkg/selfupdate/..., pkg/exitcode/..., pkg/tunnel/..., pkg/workspace/workspace.go
Uses shared rate-limit classification and standardizes success, failure, and retryable outcomes.
Agent home, workspace cloning, and daemon activity
pkg/agent/..., cmd/internal/agent_daemon.go, cmd/internal/agentcontainer/setup.go, cmd/internal/agentworkspace/{logs_daemon.go,up.go}, cmd/internal/agent_daemon_test.go
Renames agent directory APIs, adds platform-specific executable checks, refactors cloning parameters and helpers, updates daemon patrol/activity handling, and adds activity tests.
Operational failure and panic propagation
cmd/internal/agentworkspace/{build.go,setup_gpg.go}, pkg/agent/{agent.go,dockerless.go}, pkg/client/..., pkg/credentials/..., pkg/daemon/...
Adds contextual error wrapping across operational paths and returns watcher goroutine panics through its outer function.

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
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.62% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the PR’s main theme: refactoring error classification and exit-code handling.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@skevetter
skevetter force-pushed the refactor/error-handling branch 10 times, most recently from 60cc74c to ef0edb9 Compare July 21, 2026 02:31
@skevetter
skevetter marked this pull request as ready for review July 21, 2026 03:40

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
pkg/agent/workspace.go (1)

435-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove redundant gRPC error message wrapping.

Wrapping a gRPC status error with its own message using %w will 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

📥 Commits

Reviewing files that changed from the base of the PR and between dcd9c20 and ef0edb9.

📒 Files selected for processing (34)
  • cmd/internal/agentworkspace/build.go
  • cmd/internal/agentworkspace/setup_gpg.go
  • cmd/mcp/errors.go
  • cmd/pro/start.go
  • cmd/provider/configure_shared.go
  • cmd/root.go
  • cmd/root_test.go
  • desktop/src/main/__tests__/cli.test.ts
  • desktop/src/renderer/src/lib/components/ErrorCard.svelte
  • desktop/src/shared/cli-error.ts
  • pkg/agent/agent.go
  • pkg/agent/dockerless.go
  • pkg/agent/workspace.go
  • pkg/client/clientimplementation/workspace_client.go
  • pkg/clierr/errors.go
  • pkg/clierr/errors_test.go
  • pkg/command/command.go
  • pkg/credentials/request.go
  • pkg/daemon/platform/workspace_watcher.go
  • pkg/errors/errors.go
  • pkg/errors/errors_test.go
  • pkg/errors/patterns.go
  • pkg/exitcode/codes.go
  • pkg/log/logger.go
  • pkg/provider/version_cache.go
  • pkg/provider/versions_github.go
  • pkg/provider/versions_github_test.go
  • pkg/selfupdate/errors.go
  • pkg/selfupdate/errors_test.go
  • pkg/selfupdate/source.go
  • pkg/telemetry/collect.go
  • pkg/tunnel/browser.go
  • pkg/tunnel/browser_test.go
  • pkg/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

Comment thread cmd/root.go Outdated
…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.
@skevetter
skevetter force-pushed the refactor/error-handling branch from ef0edb9 to 184528f Compare July 21, 2026 04:09
@skevetter
skevetter marked this pull request as draft July 21, 2026 04:09
- 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.
@skevetter
skevetter marked this pull request as ready for review July 21, 2026 06:29
- 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Prevent data races on combined output buffer.

Using a single &bytes.Buffer{} for both Stdout and Stderr can lead to a data race (and a runtime panic) if clientimplementation.RunCommand streams output concurrently, which is standard behavior when os/exec is used under the hood.

Use separate buffers for Stdout and Stderr to 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

📥 Commits

Reviewing files that changed from the base of the PR and between c885fda and b4d85f8.

📒 Files selected for processing (1)
  • cmd/internal/agent_daemon.go

Comment thread cmd/internal/agent_daemon.go
Comment thread cmd/internal/agent_daemon.go
@skevetter
skevetter marked this pull request as draft July 21, 2026 16:03
…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.
@skevetter
skevetter marked this pull request as ready for review July 21, 2026 17:39

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
cmd/internal/agent_daemon_test.go (1)

45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer assert.Equal for time.Time comparisons.

Multiple tests use assert.True(t, a.Equal(b)) which obfuscates the actual timestamps on failure. Testify's assert.Equal natively supports time.Time equality 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: Replace assert.True with assert.Equal(t, stat.ModTime(), *activity).
  • cmd/internal/agent_daemon_test.go#L68-L68: Replace assert.True with assert.Equal(t, stat.ModTime().Add(busyGracePeriod), *activity).
  • cmd/internal/agent_daemon_test.go#L109-L109: Replace assert.True with assert.Equal(t, configActivity, effectiveActivity(configActivity)).
  • cmd/internal/agent_daemon_test.go#L118-L118: Replace assert.True with assert.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

📥 Commits

Reviewing files that changed from the base of the PR and between b4d85f8 and decb42b.

📒 Files selected for processing (2)
  • cmd/internal/agent_daemon.go
  • cmd/internal/agent_daemon_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/internal/agent_daemon.go

Comment thread cmd/internal/agent_daemon_test.go Outdated
@skevetter
skevetter marked this pull request as draft July 21, 2026 17:47
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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 retryable 75) 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.

Comment thread cmd/root.go
Comment on lines +200 to 205
// 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
}
}
Comment on lines 177 to 179
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)
}
@skevetter
skevetter marked this pull request as ready for review July 21, 2026 19:06
@skevetter skevetter changed the title refactor(errors): centralize error classification, codes, and exit handling refactor(errors): error classification, codes, and exit handling Jul 21, 2026
@skevetter skevetter changed the title refactor(errors): error classification, codes, and exit handling refactor(errors): handle classification, codes, and exits Jul 21, 2026
@skevetter skevetter changed the title refactor(errors): handle classification, codes, and exits refactor(errors): handle error classification, codes, and exits Jul 21, 2026
@skevetter
skevetter merged commit c5735b7 into main Jul 21, 2026
66 checks passed
@skevetter
skevetter deleted the refactor/error-handling branch July 21, 2026 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants