Refactor Rust SDK errors to use structs with a kind() method#1400
Open
heaths wants to merge 4 commits into
Open
Refactor Rust SDK errors to use structs with a kind() method#1400heaths wants to merge 4 commits into
kind() method#1400heaths wants to merge 4 commits into
Conversation
The conventional `#[non_exhaustive] enum Error { ... }` pattern appears
safe but creates problems as a library evolves. This PR changes all
error types to the struct-with-`kind()` pattern, which also aligns with
the Azure SDK for Rust error design.
Why not a flat error enum:
- `#[non_exhaustive]` on the enum prevents exhaustive matching, but
individual variants are still fixed. Adding a field to any variant —
even just to improve an error message with a line number or file path
— is a breaking change.
- Adding context data is harder than it looks. With a flat enum, new
fields touch every affected variant and all match arms across the
codebase. With a struct, new fields are added in one place and callers
who don't use them are unaffected.
- A single enum conflates all failure modes, making it impossible to
document or guarantee which variants a given function can actually
return. Callers must handle unrelated variants they will never see, or
accept a wildcard arm that silently swallows future additions.
The struct + kind pattern:
| Concern | Flat enum | Struct + `kind()` |
|---|---|---|
| Categorization | Match directly on variant | Call `.kind()` → `&*Kind` |
| Adding context | Breaking: add fields to variant | Non-breaking: add fields to struct |
| `non_exhaustive` | On enum; variants are fixed | Not needed on struct with only private fields |
| Simple display | Must match all variants | `format!("{err}")` — no match needed |
Callers who only want to display or propagate an error with `?` do not
need to call `.kind()` at all. Only callers who need to inspect the
failure category call `.kind()`, and they get a stable, scoped `*Kind`
enum to match against.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the Rust SDK error model to a structured Error/ErrorKind design (and similar *Kind + error wrapper types), removing thiserror usage and updating call sites, tests, examples, and docs accordingly.
Changes:
- Introduces
rust/src/errors.rswithErrorKind,ProtocolErrorKind,SessionErrorKind, and a shared internal representation for error messages/sources. - Updates SDK code/tests/examples to match on
*.kind()and construct errors viawith_message(...)/From<...>conversions. - Adjusts test configuration to require
test-supportfor selected integration tests; updates editor/LSP configuration.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/tests/session_test.rs | Updates session tests to assert on err.kind() and to_string() instead of enum variants. |
| rust/tests/protocol_version_test.rs | Updates protocol version tests to match on ErrorKind::Protocol(...). |
| rust/tests/e2e/session_fs_sqlite.rs | Migrates FS error construction to new FsError/FsErrorKind API. |
| rust/src/types.rs | Switches duplicate tool handler errors to Error::with_message(ErrorKind::InvalidConfig, ...). |
| rust/src/tool.rs | Updates tool tests to match on err.kind() (ErrorKind::Json). |
| rust/src/subscription.rs | Introduces RecvErrorKind + RecvError wrapper and manual Display/Error impls. |
| rust/src/session_fs.rs | Reworks FS errors into FsErrorKind + FsError wrapper with wire mapping logic. |
| rust/src/session.rs | Constructs/propagates session errors via ErrorKind::Session(SessionErrorKind::...). |
| rust/src/resolve.rs | Returns BinaryNotFound via ErrorKind (with optional hint) and updates docs. |
| rust/src/lib.rs | Moves crate error types out to errors.rs, re-exports them, and updates many signatures to Result<T>. |
| rust/src/jsonrpc.rs | Updates JSON-RPC parsing/transport errors to new ErrorKind model. |
| rust/src/errors.rs | Adds new centralized error system (Error, ErrorKind, ProtocolErrorKind, SessionErrorKind, StopErrors, etc.). |
| rust/src/embeddedcli.rs | Migrates embedded CLI extraction/install errors off thiserror to internal Repr model. |
| rust/examples/session_fs.rs | Updates example to construct FsError via FsErrorKind. |
| rust/examples/manual_tool_resume.rs | Adjusts imports due to subscription error type visibility changes. |
| rust/Cargo.toml | Removes thiserror; gates selected integration tests behind test-support. |
| .vscode/settings.json | Configures rust-analyzer to use all features and clippy checks. |
| .github/skills/rust-coding-skill/SKILL.md | Updates Rust skill guidance to reference ErrorKind vs prior Error enum variants. |
| .github/lsp.json | Adds rust-analyzer to repo LSP configuration. |
| .github/copilot-instructions.md | Adds guidance to use configured LSP operations (find refs/rename) instead of pattern matching. |
Sticking with `*Kind` as a convention for error enums.
Enhanced the Error struct to include an optional backtrace, which is captured only when `RUST_BACKTRACE` is set. This change helps in debugging by providing context on error occurrences without inflating the Error size unnecessarily.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The conventional
#[non_exhaustive] enum Error { ... }pattern appearssafe but creates problems as a library evolves. This PR changes all
error types to the struct-with-
kind()pattern, which also aligns withthe Azure SDK for Rust error design.
Why not a flat error enum:
#[non_exhaustive]on the enum prevents exhaustive matching, butindividual variants are still fixed. Adding a field to any variant —
even just to improve an error message with a line number or file path
— is a breaking change.
fields touch every affected variant and all match arms across the
codebase. With a struct, new fields are added in one place and callers
who don't use them are unaffected.
document or guarantee which variants a given function can actually
return. Callers must handle unrelated variants they will never see, or
accept a wildcard arm that silently swallows future additions.
The struct + kind pattern:
kind().kind()→&*Kindnon_exhaustiveformat!("{err}")— no match neededCallers who only want to display or propagate an error with
?do notneed to call
.kind()at all. Only callers who need to inspect thefailure category call
.kind(), and they get a stable, scoped*Kindenum to match against.