Plan: Code Quality Remediation

On this page

Status

Step Description Status

1

Replace String-wrapped error variants with structured types (ApiError, SigningError, StoreError, RenderError)

Done (2026-04-05) — (ApiError::Internal structured in security-ci-remediation; others verified in code-quality-audit-remediation)

2

Replace expect() with Result propagation in library code

Done (2026-04-05) — (MR !32)

3

Replace silent error discards with tracing::warn or comments

Done (2026-04-05) — (MR !32)

4

Type Typst context serde_json::Value fields, document protocol-level exceptions

Done (2026-04-05) — (all remaining Value fields documented as protocol exceptions with rationale)

5

Decompose idempotency_middleware into sub-40-line functions

Done (2026-04-05) — (extracted extract_cache_key, check_cache, execute_and_cache)

Epic: &45
Issues: TBD
Branch: refactor/structured-error-types (step 1), future branches for steps 4-5
Labels: type::chore, priority::high, program::infrastructure, service::shared-crates

Context

A codebase audit against standardized code quality constraints revealed 5 categories of violations. Two have been shipped (MR !32): expect() in library code replaced with Result propagation, and silent error discards fixed with tracing::warn or explanatory comments.

The remaining highest-priority item is String-wrapped error variants. Four error enums use String fields where structured types would preserve error chains and eliminate the format!("{e}") / .to_string() pattern at ~163 call sites.

Scope

In scope (step 1):

  • ApiError::Internal(String) → Internal { context, #[source] source } with Box<dyn Error>

  • SigningError — 4 String variants → #[source] Box<dyn Error> wrappers

  • StoreError::Config(String) → Config(#[source] Box<dyn Error>); NotFound(String) → NotFound { path }

  • RenderError — ManifestError(String) → ManifestParse([from] toml::de::Error); CompilationFailed/ContextError → [source] Box<dyn Error>

  • All call sites across services and crates

Out of scope (by design):

  • ApiError::NotFound(String) — user-facing RFC 9457 detail text, not a wrapped error

  • ApiError::BadRequest(String) — validation/regulatory messages

  • ApiError::Conflict(String) — state transition messages

  • StoreError::DisallowedContentType(String), InvalidFilename(String) — value strings, not wrapped errors

  • SigningError::InvalidJws(String) — hand-written validation messages (3 sites)

  • SigningError::NoKeyForProgram(String) — program name identifier

In scope (steps 4-5, future branches):

  • Type canopy-typst context serde_json::Value fields

  • Document protocol-level Value exceptions (event envelope, rules engine I/O)

  • Decompose idempotency_middleware (75 lines → 3 helpers)

Design

See plan file at .claude/plans/cryptic-gathering-quokka.md for full type definitions and migration patterns.

Key design decisions:

  1. ApiError::Internal takes { context: &'static str, #[source] source: Box<dyn Error> } with internal() helper constructor

  2. SigningError variants use #[source] Box<dyn Error> (not concrete types, since p256 has many error types)

  3. StoreError::Config wraps Box<dyn Error> (wraps io::Error or object_store builder errors)

  4. RenderError::ManifestError splits into ManifestParse([from] toml::de::Error) + existing Io([from] std::io::Error)

Steps

Step 1: Structured error types

Files: crates/canopy-common/src/error.rs, crates/canopy-signing/src/error.rs, crates/canopy-store/src/error.rs, crates/canopy-typst/src/error.rs, all service API and domain files with ApiError::Internal call sites

See detailed migration plan in .claude/plans/cryptic-gathering-quokka.md.

Step 2: Replace expect() in library code

Status: Complete (MR !32)

Step 3: Fix silent error discards

Status: Complete (MR !32)

Step 4: Type Typst context Value fields

Files: crates/canopy-typst/src/context.rs, crates/canopy-typst/src/engine.rs, crates/canopy-mq/src/envelope.rs, crates/canopy-rules-client/src/lib.rs

  • program_data: serde_json::Value → typed per-program context enum

  • Add code comments on protocol-level Value exceptions (event envelope, rules engine I/O)

Step 5: Decompose idempotency_middleware

Files: crates/canopy-api/src/idempotency.rs

Extract: check_cache, execute_and_cache, build_cached_response.

Files Touched

File Change

crates/canopy-common/src/error.rs

Internal → named-field variant with Box source; add internal() constructor

crates/canopy-signing/src/error.rs

4 String variants → #[source] Box wrappers

crates/canopy-store/src/error.rs

Config → Box source; NotFound → named field

crates/canopy-typst/src/error.rs

ManifestError → ManifestParse(#[from]); CompilationFailed/ContextError → Box source

crates/canopy-typst/src/manifest.rs

.map_err(to_string) → ? operator

All service API/domain files

ApiError::Internal(format!(…​)) → ApiError::internal("ctx", e)

Verification

  1. cargo nextest run --workspace --lib --bins — unit tests pass

  2. cargo xtask dev start --shared-db — devstack healthy

  3. cargo nextest run --workspace --test '*' --profile integration — integration tests pass

  4. cargo clippy --workspace --all-targets — -D warnings — zero warnings

  5. cargo xtask validate — full pre-push validation

Acceptance Criteria

  • Zero expect() or unwrap() in library code (crates/) — done

  • Zero let _ = discarding Result without a code comment or tracing::warn — done

  • Zero format!("{e}") or .to_string() wrapping a source error into a String variant

  • Zero serde_json::Value in application-level data types (step 4)

  • No function exceeds 40 lines without documented exception (step 5)

Documentation Updates

  • .claude/docs/coding-conventions.md — update error handling examples

  • CHANGELOG.adoc — entry under == Unreleased

Edit this page · default