Extract runtime error variants into dedicated runtime::error::Error type by rydrman · Pull Request #1325 · spkenv/spk

rydrman · 2026-02-19T03:11:46Z

Warning

This code and the MR summary were generated by cursor, reviewed by me upon posting

Summary

Introduces crate::runtime::error::Error, a new domain-specific error enum for runtime operations, reducing the monolithic spfs::Error from 47 variants to 34
Follows the existing sub-error pattern (encoding::Error, graph::ObjectError) by adding a #[from] conversion on the root Error, so all ?-based error propagation continues to work unchanged
Updates 20 files across spfs, spfs-cli, spfs-vfs, and spk-cli crates to construct and match on the new type

Motivation

The spfs::Error type is a large catch-all bucket. When a function returns Result<T, spfs::Error>, callers cannot tell at the type level which errors are actually possible. For example, storage trait methods can never produce RuntimeAlreadyEditable, and runtime code can never produce UnknownObject.

This PR is the first step in a series to introduce domain-specific error types for distinct areas of the SPFS API. Runtime errors are the lowest-risk extraction because:

They form a tight, self-contained cluster (13 variants)
They are only constructed in ~5 modules inside crates/spfs/src/ plus a handful of CLI crates
No storage traits or graph database traits ever return them

What Changed

New file: crates/spfs/src/runtime/error.rs

Contains 13 variants moved from the root Error:

Variant	Description
`NothingToCommit`	Empty commit attempt
`NoActiveRuntime`	No SPFS runtime detected
`RuntimeNotInitialized`	Runtime missing owner/init data
`UnknownRuntime`	Named runtime not found in storage
`RuntimeExists`	Runtime name collision
`RuntimeUpperDirAlreadyInUse`	Durable upper dir conflict
`DoesNotSupportDurableRuntimePath`	Repo type incompatible with durability
`RuntimeAlreadyEditable`	Double-edit attempt
`RuntimeReadError`	I/O read failure on runtime paths
`RuntimeWriteError`	I/O write failure on runtime paths
`RuntimeSetPermissionsError`	Chmod failure on runtime paths
`CouldNotCreateSpfsRoot`	Failed to create `/spfs` directory
`RuntimeChangeToDurableError`	Durable migration failure

Root Error changes:

Added Runtime(#[from] runtime::error::Error) variant with #[diagnostic(forward(0))]
Removed the 13 variants listed above
Updated OsError impl to delegate to runtime::error::Error::os_error()

Construction site updates (20 files):

All places that previously constructed Error::RuntimeXxx(...) now construct RuntimeError::RuntimeXxx(...) (with .into() where needed for type conversion). Match arms that destructured runtime variants on the root Error now use nested patterns like Error::Runtime(RuntimeError::NoActiveRuntime).

Backward Compatibility

The root spfs::Error and spfs::Result types remain unchanged as the top-level API
All ? propagation sites work without modification due to the #[from] conversion
Callers who match on specific runtime error variants need to update their patterns (all such sites are updated in this PR)
The OsError trait implementation correctly delegates through the new nesting

Future Work

The next step in this effort would be extracting the 6 object/reference lookup variants (UnknownObject, UnknownReference, AmbiguousReference, InvalidReference, NotCorrectKind, ObjectMissingPayload) into a graph::LookupError type. These are the most API-visible errors used in storage traits, and extracting them would eventually allow narrowing the return types on DatabaseView::read_object, TagStorage::resolve_tag, etc.

jrray · 2026-02-19T04:14:32Z

Hopefully you can get it to make sure the test suite still passes, or at least compiles. 😅

rydrman · 2026-02-19T05:27:37Z

Ya, I was pretty explicit about how to test - but clearly it was using the wrong commands or under-validating

codecov · 2026-02-19T06:56:52Z

Codecov Report

❌ Patch coverage is 11.65049% with 91 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/spfs/src/env.rs	0.00%	40 Missing ⚠️
crates/spfs/src/runtime/storage.rs	24.13%	22 Missing ⚠️
crates/spfs/src/monitor.rs	0.00%	8 Missing ⚠️
crates/spfs/src/runtime/error.rs	0.00%	7 Missing ⚠️
crates/spfs-cli/cmd-render/src/cmd_render.rs	0.00%	3 Missing ⚠️
crates/spfs-cli/common/src/args.rs	0.00%	2 Missing ⚠️
crates/spfs-cli/main/src/cmd_write.rs	0.00%	2 Missing ⚠️
crates/spfs-cli/cmd-clean/src/cmd_clean.rs	0.00%	1 Missing ⚠️
crates/spfs-cli/main/src/cmd_commit.rs	0.00%	1 Missing ⚠️
crates/spfs-cli/main/src/cmd_edit.rs	0.00%	1 Missing ⚠️
... and 4 more

📢 Thoughts on this report? Let us know!

rydrman · 2026-02-19T16:06:01Z

Okay, much better

Copilot

Pull request overview

This pull request extracts 13 runtime-specific error variants from the monolithic spfs::Error enum into a dedicated runtime::Error type, following the established pattern used by encoding::Error and graph::ObjectError. The refactoring reduces the root error enum from 47 to 34 variants while maintaining full backward compatibility through automatic #[from] conversion.

Changes:

Introduces crate::runtime::error::Error with 13 domain-specific runtime error variants
Updates root spfs::Error to wrap runtime::Error using #[from] attribute for seamless error propagation
Refactors 20 files across spfs, spfs-cli, spfs-vfs, and spk-cli crates to use the new error type

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/spfs/src/runtime/error.rs	New runtime-specific error enum with 13 variants and OsError implementation
crates/spfs/src/runtime/mod.rs	Exports the new Error type from the runtime module
crates/spfs/src/error.rs	Removes 13 runtime variants and adds Runtime(#[from] runtime::Error) wrapper variant
crates/spfs/src/status.rs	Updates error construction and documentation to use RuntimeError type
crates/spfs/src/runtime/storage.rs	Updates error construction sites to use RuntimeError throughout
crates/spfs/src/monitor.rs	Updates error construction and pattern matching for runtime errors
crates/spfs/src/find_path.rs	Updates error construction to use RuntimeError::NoActiveRuntime
crates/spfs/src/env_win.rs	Updates RuntimeNotInitialized construction to use RuntimeError
crates/spfs/src/env.rs	Updates numerous error construction sites for runtime write/read errors
crates/spfs/src/commit_test.rs	Updates error pattern matching to use nested Error::Runtime(RuntimeError::...)
crates/spfs/src/commit.rs	Updates NothingToCommit construction and matching to use RuntimeError
crates/spfs/src/storage/tar/repository.rs	Updates RuntimeWriteError construction with explicit .into() conversion
crates/spfs-vfs/src/winfsp/router.rs	Updates RuntimeExists construction to use runtime::Error type
crates/spfs-cli/main/src/cmd_write.rs	Updates RuntimeWriteError construction to use RuntimeError
crates/spfs-cli/main/src/cmd_info.rs	Updates error pattern matching to use nested pattern
crates/spfs-cli/main/src/cmd_edit.rs	Updates RuntimeAlreadyEditable pattern matching to use nested pattern
crates/spfs-cli/main/src/cmd_commit.rs	Updates NothingToCommit construction to use runtime::Error
crates/spfs-cli/common/src/args.rs	Updates error pattern matching in macro to use nested patterns
crates/spfs-cli/cmd-render/src/cmd_render.rs	Updates RuntimeWriteError and RuntimeReadError construction
crates/spfs-cli/cmd-clean/src/cmd_clean.rs	Updates RuntimeWriteError construction to use runtime::Error
crates/spk-cli/group4/src/cmd_view.rs	Updates NoActiveRuntime pattern matching to use nested pattern
crates/spk-cli/common/src/env.rs	Updates NoActiveRuntime pattern matching to use nested pattern

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-19T17:50:47Z

crates/spfs/src/runtime/error.rs

+impl OsError for Error {
+    fn os_error(&self) -> Option<i32> {
+        match self {
+            Error::RuntimeReadError(_, err) => err.os_error(),
+            Error::RuntimeWriteError(_, err) => err.os_error(),
+            _ => None,
+        }
+    }
+}


The OsError implementation is incomplete. It should also include cases for RuntimeSetPermissionsError and CouldNotCreateSpfsRoot, both of which contain an io::Error that should be delegated for OS error code extraction, similar to RuntimeReadError and RuntimeWriteError.

crates/spfs/src/runtime/error.rs

The monolithic spfs::Error enum had 47 variants covering everything from runtime management to object storage to serialization. This makes it difficult for callers to reason about which errors can actually occur in a given context. This change extracts the 13 runtime-related variants into a new crate::runtime::error::Error type, following the existing pattern used by encoding::Error and graph::ObjectError. The new type is included in the root Error via a #[from] conversion, so existing code that propagates errors with `?` continues to work unchanged. Variants moved: - NothingToCommit, NoActiveRuntime, RuntimeNotInitialized - UnknownRuntime, RuntimeExists, RuntimeUpperDirAlreadyInUse - DoesNotSupportDurableRuntimePath, RuntimeAlreadyEditable - RuntimeReadError, RuntimeWriteError, RuntimeSetPermissionsError - CouldNotCreateSpfsRoot, RuntimeChangeToDurableError This is the first step toward more granular error types across the spfs API surface. Signed-off-by: Ryan Bottriell <rbottriell@ilm.com> Co-authored-by: Cursor <cursoragent@cursor.com>

Signed-off-by: Ryan Bottriell <rbottriell@ilm.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Ryan Bottriell <rbottriell@ilm.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Ryan Bottriell <rbottriell@ilm.com> Co-authored-by: Cursor <cursoragent@cursor.com>

The runtime::Error type uses spfs::runtime as its fallback error code, but this anchor was missing from the error codes documentation page. Signed-off-by: Ryan Bottriell <rbottriell@ilm.com> Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-24T03:55:04Z

crates/spfs/src/runtime/error.rs

+    fn os_error(&self) -> Option<i32> {
+        match self {
+            Error::RuntimeReadError(_, err) => err.os_error(),
+            Error::RuntimeWriteError(_, err) => err.os_error(),


The OsError implementation should also handle the CouldNotCreateSpfsRoot variant, which contains an std::io::Error source. This would allow OS error codes from directory creation failures to be properly exposed.

Suggested change

Error::RuntimeWriteError(_, err) => err.os_error(),

Error::RuntimeWriteError(_, err) => err.os_error(),

Error::CouldNotCreateSpfsRoot { source } => source.os_error(),

Copilot · 2026-02-24T03:55:04Z

crates/spfs-cli/cmd-render/src/cmd_render.rs

+            .map_err(|err| RuntimeError::RuntimeWriteError(target.to_owned(), err))?;
        let target_dir = tokio::task::block_in_place(|| dunce::canonicalize(target))
            .map_err(|err| Error::InvalidPath(target.to_owned(), err))?;
        if tokio::fs::read_dir(&target_dir)
            .await
-            .map_err(|err| Error::RuntimeReadError(target_dir.clone(), err))?
+            .map_err(|err| RuntimeError::RuntimeReadError(target_dir.clone(), err))?
            .next_entry()
            .await
-            .map_err(|err| Error::RuntimeReadError(target_dir.clone(), err))?
+            .map_err(|err| RuntimeError::RuntimeReadError(target_dir.clone(), err))?


Using RuntimeError::RuntimeWriteError and RuntimeError::RuntimeReadError for general directory operations in the render command is semantically incorrect. These errors are intended for runtime-specific filesystem operations (e.g., operations on runtime directories like upper_dir, work_dir, etc.), not for general file I/O operations during rendering. Consider using a different error type or the existing Error::StorageWriteError/Error::StorageReadError, or create more appropriate error variants for render operations.

Copilot · 2026-02-24T03:55:04Z

crates/spfs-cli/main/src/cmd_write.rs

+                    .map_err(|err| RuntimeError::RuntimeWriteError(file.clone(), err))?;
                #[cfg(unix)]
                let mode = handle
                    .metadata()
                    .await
-                    .map_err(|err| Error::RuntimeWriteError(file.clone(), err))?
+                    .map_err(|err| RuntimeError::RuntimeWriteError(file.clone(), err))?


Using RuntimeError::RuntimeWriteError for file open and metadata operations in the write command is semantically incorrect. The first error is actually a read error (opening a file for reading), not a write error. These operations are also not runtime-specific filesystem operations. Consider using appropriate error variants like Error::StorageReadError for file opening or a more general I/O error type.

Copilot · 2026-02-24T03:55:04Z

crates/spfs/src/runtime/error.rs

+// SPDX-License-Identifier: Apache-2.0
+// https://github.com/spkenv/spk
+
+#![allow(unused_assignments)]


The #![allow(unused_assignments)] attribute at the top of this file appears unnecessary. There are no assignments in this file, let alone unused ones. This attribute should be removed as it's misleading and could mask future issues.

Suggested change

#![allow(unused_assignments)]

Copilot · 2026-02-24T03:55:05Z

crates/spfs/src/runtime/error.rs

+    fn os_error(&self) -> Option<i32> {
+        match self {
+            Error::RuntimeReadError(_, err) => err.os_error(),
+            Error::RuntimeWriteError(_, err) => err.os_error(),


The OsError implementation is incomplete. The RuntimeSetPermissionsError variant also contains an io::Error source and should be included in the match statement to properly expose OS error codes from permission-setting operations.

Suggested change

Error::RuntimeWriteError(_, err) => err.os_error(),

Error::RuntimeWriteError(_, err) => err.os_error(),

Error::RuntimeSetPermissionsError(_, err) => err.os_error(),

rydrman requested review from dcookspi and jrray February 19, 2026 03:11

rydrman self-assigned this Feb 19, 2026

rydrman added the maintenance Cleanup, upgrades, etc label Feb 19, 2026

rydrman force-pushed the refactor/extract-runtime-error-type branch from 960a179 to fcc9c29 Compare February 19, 2026 06:40

jrray requested a review from Copilot February 19, 2026 17:46

Copilot started reviewing on behalf of jrray February 19, 2026 17:46 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

jrray reviewed Feb 19, 2026

View reviewed changes

crates/spfs/src/runtime/error.rs Show resolved Hide resolved

rydrman and others added 3 commits February 23, 2026 17:21

Add spfs::runtime error code documentation

0987852

The runtime::Error type uses spfs::runtime as its fallback error code, but this anchor was missing from the error codes documentation page. Signed-off-by: Ryan Bottriell <rbottriell@ilm.com> Co-authored-by: Cursor <cursoragent@cursor.com>

rydrman force-pushed the refactor/extract-runtime-error-type branch from f1299d5 to 0987852 Compare February 24, 2026 01:54

rydrman requested a review from jrray February 24, 2026 02:18

jrray requested a review from Copilot February 24, 2026 03:51

Copilot started reviewing on behalf of jrray February 24, 2026 03:52 View session

Copilot AI reviewed Feb 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract runtime error variants into dedicated runtime::error::Error type#1325

Extract runtime error variants into dedicated runtime::error::Error type#1325
rydrman wants to merge 3 commits intomainfrom
refactor/extract-runtime-error-type

rydrman commented Feb 19, 2026 •

edited

Loading

Uh oh!

jrray commented Feb 19, 2026

Uh oh!

rydrman commented Feb 19, 2026

Uh oh!

codecov bot commented Feb 19, 2026

Uh oh!

rydrman commented Feb 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	Error::RuntimeWriteError(_, err) => err.os_error(),
	Error::RuntimeWriteError(_, err) => err.os_error(),
	Error::CouldNotCreateSpfsRoot { source } => source.os_error(),

	Error::RuntimeWriteError(_, err) => err.os_error(),
	Error::RuntimeWriteError(_, err) => err.os_error(),
	Error::RuntimeSetPermissionsError(_, err) => err.os_error(),

Conversation

rydrman commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

What Changed

Backward Compatibility

Future Work

Uh oh!

jrray commented Feb 19, 2026

Uh oh!

rydrman commented Feb 19, 2026

Uh oh!

codecov bot commented Feb 19, 2026

Codecov Report

Uh oh!

rydrman commented Feb 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rydrman commented Feb 19, 2026 •

edited

Loading