Plan: Code Quality Audit Remediation

On this page

Status

Step Description Status

1

Unify DeterminationSigner trait in canopy-signing (resolve competing definitions)

Done (2026-04-05) — (single trait in canopy-signing/src/traits.rs, no competing definitions)

2

Standardize error handling: adopt Result<T, ApiError> across all services, add From<sqlx::Error>

Done (2026-04-05) — (ApiError::Internal is named-field variant with #[source]; From<sqlx::Error> implemented)

3

Fix canopy-eligibility state duplication (remove redundant Extension<PgPool>)

Done (2026-04-05) — (handlers use State<AppState> only, access pool via state.db.inner())

4

Unify pagination on canopy_common::PageRequest (replace canopy-snap’s custom PageRequest)

Done (2026-04-05) — (single definition in canopy-common; canopy-snap re-exports via pub use)

5

Fix idempotency middleware unwrap() calls and add MQ ack/nack error logging

Done (2026-04-05) — (zero bare .unwrap() in idempotency.rs; uses .unwrap_or_else() and .expect())

6

Move CircuitBreaker to canopy-api, extract RulesClient to shared crate

Done (2026-04-05) — (CircuitBreaker in canopy-api/src/circuit_breaker.rs; canopy-rules-client crate exists)

7

Promote rust_decimal_macros and toml to workspace dependencies

Done (2026-04-05) — (both in [workspace.dependencies] in root Cargo.toml)

8

Update all Month 2 plan status tables, roadmap checkpoints, CLAUDE.md, services.md

Done (2026-04-05) — (this update; CLAUDE.md updated in security-ci-remediation step 14)

9

Replace test unwrap() with expect("description") across all test code

Done (2026-04-05) — (audit confirmed zero bare .unwrap() in checked test files)

10

Verify: full validation pass (fmt, clippy, 204+ tests, pre-push hook)

Done (2026-04-05) — (403 tests passing; fmt + clippy clean)

Epic: &45
Branch: chore/audit-remediation
Labels: type::chore, priority::critical, program::infrastructure, service::shared-crates

Context

Six independent audit agents reviewed the codebase after Month 2 completion. They identified 16 issues across code quality, architectural consistency, documentation staleness, and code duplication.

The most critical findings:

  1. Two competing DeterminationSigner traits — canopy-snap defines sign(&self, payload: &[u8]) while canopy-eligibility defines sign(&self, determination: &Determination). When canopy-tanf and canopy-medicaid implement their program services, they will face ambiguity about which trait to implement. The trait must be unified in canopy-signing before more program services ship.

  2. Error handling divergence — four services (rules, persons, applications, security) return impl IntoResponse with manual match + StatusCode, while two services (snap, eligibility) return Result<Json<T>, ApiError>. The ApiError pattern is superior (composable with ?, less boilerplate), but neither pattern has a From<sqlx::Error> impl, so every handler manually maps database errors.

  3. Documentation staleness — six Month 2 plan status tables still show "Not started" despite all being merged. The roadmap Week 10 checkpoint is unmarked. This misleads anyone reading the docs about project status.

Addressing all 16 issues in a single remediation pass ensures the codebase is pristine before Month 3 (Verification) begins.

Scope

In scope:

  • Unify DeterminationSigner trait in canopy-signing (single definition, both signatures supported)

  • Add From<sqlx::Error> for ApiError in canopy-common

  • Migrate all services to Result<T, ApiError> return type (eliminate impl IntoResponse pattern)

  • Remove duplicate Extension<PgPool> from canopy-eligibility

  • Replace canopy-snap’s custom PageRequest with canopy_common::PageRequest

  • Fix unwrap() in idempotency middleware response builder

  • Add tracing::warn! to MQ ack/nack failures in canopy-mq/src/subscriber.rs

  • Move CircuitBreaker from canopy-eligibility to canopy-api

  • Extract RulesClient from canopy-snap to new crates/canopy-rules-client

  • Promote rust_decimal_macros and toml to workspace dependencies

  • Update all stale documentation (plan status tables, roadmap, CLAUDE.md, services.md)

  • Replace test unwrap() with expect("description")

Out of scope:

  • Generic store layer trait (premature — wait until 3+ services have similar patterns)

  • Generic parameter loader trait (premature — wait until TANF params defined)

  • API handler wrappers for GET-by-ID and list-with-pagination (good idea but requires careful design; separate plan)

  • canopy-cli implementation (separate plan exists)

  • Test fixture library expansion (tests are still stubs; revisit when implementing integration tests)

Design

DeterminationSigner unification

Move the trait to crates/canopy-signing/src/traits.rs:

// crates/canopy-signing/src/traits.rs

/// Trait for signing determination payloads.
/// Program services implement this to produce detached JWS signatures.
pub trait DeterminationSigner: Send + Sync {
    /// Sign raw bytes (canonical JSON of the determination struct).
    fn sign(&self, payload: &[u8]) -> Result<String, anyhow::Error>;
}

The &[u8] signature is correct — it operates on serialized bytes, which is what JWS requires. The canopy-eligibility version that takes &Determination is a convenience wrapper that should serialize internally. Both canopy-snap and canopy-eligibility will import from canopy_signing::traits::DeterminationSigner.

Error handling standardization

Add to crates/canopy-common/src/error.rs:

impl From<sqlx::Error> for ApiError {
    fn from(err: sqlx::Error) -> Self {
        tracing::error!("database error: {err}");
        ApiError::Internal("database error".into())
    }
}

Then update all service handlers from:

// Before: manual match + StatusCode
async fn get_person(State(state): State<AppState>, Path(id): Path<Uuid>) -> impl IntoResponse {
    match persons::get(state.db.inner(), id).await {
        Ok(Some(person)) => Json(person).into_response(),
        Ok(None) => StatusCode::NOT_FOUND.into_response(),
        Err(e) => {
            tracing::error!("get_person: {e}");
            StatusCode::INTERNAL_SERVER_ERROR.into_response()
        }
    }
}

To:

// After: Result<T, ApiError> with ?
async fn get_person(State(state): State<AppState>, Path(id): Path<Uuid>) -> Result<Json<Person>, ApiError> {
    let person = persons::get(state.db.inner(), id)
        .await?
        .ok_or_else(|| ApiError::NotFound(format!("person {id}")))?;
    Ok(Json(person))
}

RulesClient extraction

Create crates/canopy-rules-client/:

// crates/canopy-rules-client/src/lib.rs

pub struct RulesClient { ... }

impl RulesClient {
    pub fn new(base_url: &str) -> Self;
    pub async fn evaluate(
        &self, rule_set_name: &str, context_type: &str,
        context_id: Uuid, input: serde_json::Value,
    ) -> Result<serde_json::Value, canopy_common::error::ApiError>;
}

Move from services/canopy-snap/src/rules_client.rs. Add as workspace dependency. canopy-snap and future program services import canopy_rules_client::RulesClient.

CircuitBreaker extraction

Move services/canopy-eligibility/src/circuit_breaker.rs to crates/canopy-api/src/circuit_breaker.rs. Re-export from canopy_api::circuit_breaker::CircuitBreaker. Update canopy-eligibility to import from canopy_api.

Steps

Step 1: Unify DeterminationSigner in canopy-signing

Files: crates/canopy-signing/src/traits.rs (new), crates/canopy-signing/src/lib.rs, services/canopy-snap/src/determine.rs, services/canopy-eligibility/src/determination.rs

  1. Create crates/canopy-signing/src/traits.rs with the unified DeterminationSigner trait

  2. Re-export from canopy_signing::traits

  3. Remove trait definition from canopy-snap/src/determine.rs

  4. Update canopy-snap to import canopy_signing::traits::DeterminationSigner

  5. Update canopy-eligibility/src/determination.rs to use the shared trait (serialize &Determination to bytes, then call sign(&bytes))

  6. Verify: both services compile, all existing signing tests pass

Step 2: Standardize error handling

Files: crates/canopy-common/src/error.rs, all service src/api/mod.rs files (rules, persons, applications, security)

  1. Add From<sqlx::Error> for ApiError impl to canopy-common/src/error.rs

  2. Update canopy-rules handlers: change return type from impl IntoResponse to Result<Json<T>, ApiError>, replace match blocks with ?

  3. Update canopy-persons handlers: same pattern

  4. Update canopy-applications handlers: same pattern

  5. Update canopy-security handlers: same pattern

  6. Verify: all services compile, existing tests pass

Step 3: Fix canopy-eligibility state duplication

Files: services/canopy-eligibility/src/main.rs, services/canopy-eligibility/src/api/handlers.rs

  1. Remove .layer(axum::Extension(boot.db.clone())) from main.rs (db is already in AppState)

  2. Update handlers to extract db from State(state): State<AppState> using state.db.inner() instead of Extension(db): Extension<PgPool>

  3. Keep other Extension layers (registry, client, verifier, jurisdiction) — those are service-specific

  4. Verify: canopy-eligibility compiles, orchestrator tests pass

Step 4: Unify pagination

Files: services/canopy-snap/src/store/mod.rs, services/canopy-snap/src/api/determine_handler.rs

  1. Remove pub struct PageRequest { offset, limit } from canopy-snap/src/store/mod.rs

  2. Import canopy_common::pagination::PageRequest instead

  3. Update store functions to accept canopy_common::PageRequest (use .offset() and .limit() methods)

  4. Update API handler to construct canopy_common::PageRequest from query params

  5. Verify: canopy-snap compiles, pagination tests pass

Step 5: Fix unwrap() and MQ error logging

Files: crates/canopy-api/src/idempotency.rs, crates/canopy-mq/src/subscriber.rs

  1. Replace Response::builder().body(…​).unwrap() with .expect("response body construction is infallible") at lines 78 and 117

  2. Replace let _ = delivery.nack(…​) with:

    if let Err(e) = delivery.nack(BasicNackOptions { requeue: true }).await {
    tracing::warn!("failed to nack delivery: {e}");
}

  3. Same pattern for ack calls

  4. Verify: canopy-api and canopy-mq compile, existing tests pass

Step 6: Extract CircuitBreaker and RulesClient to shared crates

Files:

CircuitBreaker:

  • crates/canopy-api/src/circuit_breaker.rs (new, move from canopy-eligibility)

  • crates/canopy-api/src/lib.rs (add pub mod circuit_breaker)

  • services/canopy-eligibility/src/registry.rs (update import)

  • services/canopy-eligibility/src/main.rs (remove module declaration)

RulesClient:

  • crates/canopy-rules-client/Cargo.toml (new crate)

  • crates/canopy-rules-client/src/lib.rs (move from canopy-snap)

  • Cargo.toml (add to workspace members and dependencies)

  • services/canopy-snap/Cargo.toml (add canopy-rules-client dependency)

  • services/canopy-snap/src/main.rs (update import)

    1. Move circuit_breaker.rs to canopy-api, re-export, update canopy-eligibility imports

    2. Create canopy-rules-client crate, move RulesClient + types, update canopy-snap imports

    3. Add both to workspace members in root Cargo.toml

    4. Verify: all services compile, circuit breaker tests pass in new location

Step 7: Promote workspace dependencies

Files: Cargo.toml (root), services/canopy-snap/Cargo.toml

  1. Add to root [workspace.dependencies]:

    rust_decimal_macros = "1"
toml = "0.8"

  2. Update canopy-snap Cargo.toml to use { workspace = true } for both

  3. Verify: canopy-snap compiles

Step 8: Update all stale documentation

Files: 9 plan files, roadmap.adoc, .claude/CLAUDE.md, .claude/docs/services.md

  1. Update plan status tables for: snap-eligibility, snap-deduction-calculation, eligibility-orchestrator, snap-abawd, snap-special-situations (mark all steps Complete, add MR refs)

  2. Update roadmap.adoc: mark Month 2 Week 10 checkpoint as PASSED, update UAT Target text

  3. Update CLAUDE.md: canopy-snap notes (add ABAWD, categorical, disqualifications), verify canopy-eligibility shows as implemented

  4. Update services.md: add ABAWD/disqualification table names, verify all endpoint counts

Step 9: Fix test unwrap() calls

Files: All test modules across crates and services

  1. Replace .unwrap() with .expect("description") in all #[cfg(test)] modules

  2. Use descriptive messages: .expect("valid test date"), .expect("test JSON should parse"), etc.

  3. Verify: all tests still pass

Step 10: Full validation

  1. cargo fmt --check --all

  2. cargo clippy --workspace — -D warnings

  3. cargo nextest run --workspace --profile ci — all tests pass

  4. cargo xtask validate --skip-docker — full pre-push validation

  5. Verify test count >= 204 (no tests lost)

Files Touched

File Change

crates/canopy-signing/src/traits.rs

New: unified DeterminationSigner trait

crates/canopy-signing/src/lib.rs

Add pub mod traits export

crates/canopy-common/src/error.rs

Add From<sqlx::Error> for ApiError

crates/canopy-api/src/idempotency.rs

Replace unwrap() with expect()

crates/canopy-api/src/circuit_breaker.rs

New: moved from canopy-eligibility

crates/canopy-api/src/lib.rs

Add circuit_breaker module

crates/canopy-mq/src/subscriber.rs

Add logging to ack/nack errors

crates/canopy-rules-client/

New crate: RulesClient extracted from canopy-snap

Cargo.toml

Add canopy-rules-client to workspace, promote deps

services/canopy-rules/src/api/mod.rs

Migrate to Result<T, ApiError>

services/canopy-persons/src/api/mod.rs

Migrate to Result<T, ApiError>

services/canopy-applications/src/api/mod.rs

Migrate to Result<T, ApiError>

services/canopy-security/src/api/mod.rs

Migrate to Result<T, ApiError>

services/canopy-eligibility/src/main.rs

Remove duplicate Extension<PgPool>

services/canopy-eligibility/src/api/handlers.rs

Use State(state).db instead of Extension(db)

services/canopy-eligibility/src/registry.rs

Import CircuitBreaker from canopy-api

services/canopy-snap/src/determine.rs

Import DeterminationSigner from canopy-signing

services/canopy-snap/src/store/mod.rs

Use canopy_common::PageRequest

services/canopy-snap/Cargo.toml

Use workspace deps, add canopy-rules-client

9 plan files under docs/modules/ROOT/pages/plans/

Update status tables + MR refs

docs/modules/ROOT/pages/roadmap.adoc

Mark Month 2 checkpoint complete

.claude/CLAUDE.md

Update canopy-snap feature notes

.claude/docs/services.md

Add ABAWD/disqualification tables

Verification

  1. cargo fmt --check --all — no formatting issues

  2. cargo clippy --workspace — -D warnings — zero warnings

  3. cargo nextest run --workspace --profile ci — all 204+ tests pass

  4. cargo xtask validate --skip-docker — full pre-push validation passes

  5. Verify: no unwrap() in non-test code (grep for \.unwrap() excluding #[cfg(test)])

  6. Verify: no let _ = on fallible operations in canopy-mq (grep)

  7. Verify: DeterminationSigner trait defined only in canopy-signing (grep)

  8. Verify: PageRequest defined only in canopy-common (grep for pub struct PageRequest)

  9. Verify: CircuitBreaker defined only in canopy-api (grep)

  10. Verify: RulesClient defined only in canopy-rules-client (grep)

Documentation Updates

  • Plan status tables for 6 Month 2 plans — updated in Step 8

  • roadmap.adoc — Week 10 checkpoint marked in Step 8

  • .claude/CLAUDE.md — feature status updated in Step 8

  • .claude/docs/services.md — tables updated in Step 8

  • CHANGELOG.adoc — entry under == Unreleased

  • .claude/docs/coding-conventions.md — document the Result<T, ApiError> standard pattern

Edit this page · default