Plan: Code Quality Remediation

On this page

Status: COMPLETE (2026-04-22). All 6 steps shipped via MRs !130 (Step 1), !131 (Step 2), !132 (Step 3), !133 (Step 4), !134 (Step 5), !135 (Step 6 audit). Issue #180 closed. See Archive for cross-reference.

Plan refreshed 2026-04-21 (MR !129). Each of the 6 steps audited against current main after the CQR April sweep (MRs !104-!118) + the craig-signing extraction (!120). Real targets:

  • Step 1 — ApiError and StoreError still use String tuple variants (both in NotFound, BadRequest, Conflict, Internal shapes). IntakeError already has { detail: String } named-field form; only missing a structured entity: &'static str + optional field. Scope slightly narrower than the original plan.

  • Step 2 — only ~4 real targets after filtering out doc-example `expect()`s, test code, and infallible-type-serialization (panic-as-assertion). Scope narrowed.

  • Step 3 — 4 subscriber.rs ack/nack discards + 3 update_last_used fire-and-forget spawns + 6 service main.rs let _ = db (these get a single-line // Reason: comment; no tracing::warn needed — the pattern is "hold for drop-timing lifetime").

  • Step 4 — 3 Value fields in craig-intake-sdk/src/types.rs + their 3 builder methods. Unchanged.

  • Step 5 — five functions confirmed over 40 lines: idempotency_middleware (88), router (58), health_check (54), create_case (77), update_case (64). Numbers below updated.

Step bodies rewritten with accurate file:line references. Original plan text preserved under Original (historical) subheadings where scope changed.

Status

Status column reflects audit at 2026-04-21.

Step Description Status

1

Replace String-wrapped error variants with structured error data

Done (2026-04-21) — MR !130 — ApiError rewrote to named-field struct variants (NotFound { entity: &'static str, id: String }, BadRequest { detail: String, field: Option<&'static str> }, Conflict { entity: &'static str, detail: String }, Internal { detail: String }). StoreError similarly converted. IntakeError already had named-field form — its existing shape is compatible with the rest. Helper constructors (ApiError::not_found, ::bad_request, ::bad_request_for, ::conflict, ::internal, and StoreError::not_found/::disallowed_content_type/::invalid_filename/::config) collapse ~600 call-site migrations into terse one-liners. ProblemDetails gained an optional field member so RFC 9457 responses for BadRequest { field: Some(_) } can target a specific form control.

2

Replace expect() with Result propagation in library code

Done (2026-04-21) — MR !131 — JwksProvider::new/with_fetch_url and IntakeClient::new both now return Result<Self>. Every kept expect() in crates/ has an adjacent // Reason: / // Panic: / // Test-only: comment explaining why it’s an assertion rather than a real failure mode (OTEL-exporter startup, infallible JSON serialization of static maps, HMAC-SHA256’s any-length key guarantee, test harness setup). A workspace-wide grep gate now enforces the "reasoned expect" contract going forward.

3

Replace silent error discards with tracing::warn

Done (2026-04-21) — MR !132 — 4 subscriber.rs ack/nack sites + 2 update_last_used sites now log on failure via tracing::warn with structured context (error, key_id). Rules engine req.reply.send dropped receiver + test-lib cleanup + 6 non-critical let _ = db + metrics-encoder sites annotated with // Reason: / // Test-only: comments explaining intent. Workspace-wide grep gate now enforces that every let _ = ` in library code (excluding `tests/ directories and #[cfg(test)] blocks) has an adjacent reasoning comment — the gate passes cleanly.

4

Type AFCARS Value fields, document protocol-level Value exceptions

Done (2026-04-22) — MR !133 — added Child, Adult, Narrative typed structs to craig-intake-sdk/src/types.rs mirroring the server-side services/craig-intake/src/store/models.rs::{ChildEntry, AdultEntry, Narrative} shapes. ReportSubmission.children/adults/narrative moved from serde_json::Value to Vec<Child>/Vec<Adult>/Option<Narrative>; ReportBuilder signatures and defaults updated to match. Wire format unchanged — server-side SubmitReportRequest keeps its Value slot so loose-JSON clients stay compatible. EventEnvelope.payload and craig-signing’s canonicalize_json remain `Value as protocol-level generics (documented in scope).

5

Decompose functions exceeding 40-line limit

Done (2026-04-22) — MR !134 — all five target functions decomposed and now well under the 40-line ceiling: idempotency_middleware 88→8 (extracted extract_idempotency_key, check_cache, capture_and_cache); router 58→20 (extracted build_protected_routes, apply_global_layers); health_check 54→24 (extracted check_database, check_rabbitmq); create_case 77→23 (extracted resolve_referral_id, insert_case_with_unique_retry); update_case 64→37 (extracted validate_update_payload, publish_case_change_event). 131/131 craig-api + craig-cases tests still green.

6

Full validation pass

Done (2026-04-22) — MR !135 — all gates green: cargo fmt --check --all, cargo clippy --workspace --locked — -D warnings, cargo nextest run --workspace --lib --locked (258/258), pre-push e2e + perf + security regression. Manual grep audit confirms every .expect( in crates/ carries a // Reason: / // Panic: / // Test-only: marker or sits inside #[cfg(test)] / /// doc; every let _ = ` in `crates/ and services/ (excluding tests/ directories) likewise; zero remaining ApiError::NotFound(String::from(…​)) legacy-construction patterns. Drive-by: bumped xtask validate clippy step to --all-targets so this drift class can’t recur, and cleaned the 21 pre-existing bool_assert_comparison / err_expect / needless_borrows_for_generic_args warnings the new gate surfaced.

Issues: #180
Related: #179 (bootstrap integration — Phase 1 residual from archived code-quality-review plan)
Branch: chore/code-quality-remediation

Context

Audit of CRAIG codebase against standardized code quality constraints revealed violations that need remediation before rebasing against claude-quickstart template conventions.

Five categories of issues were found: string-wrapped error variants that lose structured context, expect() calls in library code that can panic on production inputs, silent error discards (let _ =) that hide failures in critical paths, serde_json::Value usage where typed structs should exist, and functions exceeding 40 lines that are difficult to test and review.

Scope

In scope (refreshed 2026-04-21):

  • 3 error types: ApiError (full rewrite from tuple variants), StoreError (tuple → structured), IntakeError (named-field → add entity discriminator)

  • 2 reqwest::Client::builder().build().expect(…​) sites (jwks.rs:50, intake-sdk/client.rs:59) — propagate via Result. Sites originally listed as "15 expect()" reduced to 2 after filtering out doc examples, tests, infallible static-JSON serialization, and the OTEL-exporter startup path.

  • 11 let _ = patterns (4 critical subscriber ack/nack + 2 update_last_used + 1 store cleanup + 2 craig-api best-effort + 6 service-main held-for-lifetime — last 6 need only a comment, not tracing).

  • 3 serde_json::Value fields in craig-intake-sdk/src/types.rs, plus 3 builder methods.

  • 5 functions over 40 lines — extract helpers.

Out of scope:

  • expect() in startup code (shutdown_signal, OTEL-exporter-build in telemetry.rs:86,103) — acceptable with adjacent // Panic: irrecoverable startup failure comment

  • expect() in test-lib, #[cfg(test)] modules, and docstring code blocks — acceptable as test-only / illustrative

  • expect() on infallible static-JSON serialization (e.g. serde_json::to_string(&header).expect("…​") where header is a 2-key map literal) — the expect message is the assertion; conversion to Result would add boilerplate without exposing a real failure mode. Rewriting this class of expect is a separate code-style decision, not a correctness fix.

  • serde_json::Value in EventEnvelope.payload — protocol-level generic; wildcard subscribers need the untyped form

  • serde_json::Value in JWS canonicalization (craig-signing) — protocol-level; signing contract shared across language SDKs

  • craig-web route handlers over 40 lines — Askama-heavy, extract less cleanly; tracked separately in route-cleanup work if it materializes

Design

Structured Error Variants

Replace:

ApiError::NotFound(String)

With:

ApiError::NotFound { entity: &'static str, id: String }

The IntoResponse impl for ApiError already produces RFC 9457 Problem Details. The structured fields provide better detail text without changing the HTTP response shape.

Result Propagation

Replace:

let client = reqwest::Client::builder().build().expect("failed to build HTTP client");

With:

let client = reqwest::Client::builder().build().map_err(|e| anyhow::anyhow!("HTTP client: {e}"))?;

Functions that currently return Self must change to return Result<Self>.

Steps

Step 1: String-wrapped error variants

Files: crates/craig-common/src/error.rs, crates/craig-intake-sdk/src/error.rs, crates/craig-store/src/error.rs

  1. ApiError: Replace NotFound(String) with NotFound { entity: &'static str, id: String }, BadRequest(String) with BadRequest { detail: String, field: Option<&'static str> }, Conflict(String) with Conflict { entity: &'static str, detail: String }, Internal(String) with Internal { detail: String }

  2. Update IntoResponse impl to construct detail from structured fields

  3. Update all call sites across 8 services (grep for ApiError::NotFound(, etc.)

  4. IntakeError: Same pattern — replace String detail with structured fields

  5. StoreError: Same pattern

  6. Run cargo build --workspace to find all broken call sites

  7. Fix each call site with the structured constructor

Step 2: expect() → Result propagation

Files (refreshed): crates/craig-auth/src/jwks.rs, crates/craig-intake-sdk/src/client.rs. Originally listed signing.rs/publisher.rs/telemetry.rs — audit reclassified those as acceptable (infallible static-JSON serialization and OTEL-exporter startup).

  1. crates/craig-auth/src/jwks.rs:50 — reqwest::Client::builder().build().expect("failed to build HTTP client") inside JwksProvider::new(). Change the constructor to return Result<Self>; propagate the build error with .map_err(|e| anyhow::anyhow!("HTTP client: {e}"))?. Update the 1 call site in craig-api bootstrap.

  2. crates/craig-intake-sdk/src/client.rs:59 — same pattern inside IntakeClient::new(). Same fix.

  3. Annotate the kept expects with an adjacent // Reason: comment explaining why they’re acceptable:

    • crates/craig-common/src/telemetry.rs:86,103 — OTEL-exporter-build in the startup path. If the exporter fails to build, the service can’t observe itself; panicking is equivalent to failing startup.

    • crates/craig-mq/src/publisher.rs:30 — serde_json::to_vec(&EventEnvelope).expect(…​). The envelope contains a serde_json::Value payload; unless the payload is produced by a buggy impl Serialize, this can’t fail. Treat as an assertion.

    • crates/craig-intake-sdk/src/signing.rs:75 — 2-key map literal serialization. Infallible by construction.

  4. For crates/craig-api/src/bootstrap.rs shutdown_signal: same treatment — add // Panic: irrecoverable startup failure.

  5. For crates/craig-test-lib/src/client.rs and token.rs: add // Test-only: panic on infra failure.

Verify: grep -rn '\.expect(' crates/ --include='*.rs' — every remaining occurrence has an adjacent // Reason: or // Panic: or // Test-only: comment. Pre-push runs the new jwks::JwksProvider::new() error path in at least one integration test.

Step 3: Silent error discards → tracing::warn

Files (refreshed line numbers):

Critical — must log on error:

  1. crates/craig-mq/src/subscriber.rs:137,144,153,157 — let _ = delivery.ack(…​) / let _ = delivery.nack(…​). Rewrite as if let Err(e) = delivery.ack(BasicAckOptions::default()).await { tracing::warn!(error = %e, "failed to ack AMQP delivery"); }. Dropping an ack silently means the broker re-delivers on the next consumer-up, causing duplicate processing — exactly the bug the warn is supposed to surface.

  2. services/craig-intake/src/api/api_key_lookup.rs:60 — let _ = store::api_keys::update_last_used(…​). Fire-and-forget last-used-timestamp update; add tracing::warn!(error = %e, key_id = %key_id, "failed to update api_key last_used") on error.

  3. services/craig-intake/src/api/jws.rs:112 — same pattern for signer_keys::update_last_used.

  4. crates/craig-store/src/store.rs:103 — best-effort cleanup on a failure path; still worth a warn so the leak is observable.

Non-critical — add a // Reason: comment, no tracing::warn:

  1. crates/craig-api/src/rate_limit.rs:93 — let _ = limiter.check_key(&ip) inside a test helper that warms the limiter. Comment: // Reason: test warmup — check() return is unused on purpose.

  2. crates/craig-api/src/lib.rs:299 — let _ = encoder.encode(…​) in the /metrics handler’s Prometheus-text encoder. Comment: // Reason: metrics encoder failure is surfaced via empty body; HTTP 200 preserves scrape compatibility.

  3. crates/craig-api/src/bootstrap.rs:182-187 — let _ = &br.db; let _ = &br.auth; …. These hold refs so Drop timing is documented; comment as a block: // Reason: held-for-lifetime; explicit binding documents that BootstrapResult fields are kept alive until main() returns.

  4. Service main.rs files (craig-cases/main.rs:142, craig-exchange/main.rs:108, craig-placement/main.rs:95, 3 others): let _ = db binding pattern — comment: // Reason: held-for-lifetime; DbPool kept in scope so connections stay live for the axum server.

Verify: grep gate in Step 6 — every let _ = ` in `crates/ and services/ has either a // Reason: comment on the line above / same line, OR the discarded expression is a Result being handled via if let Err(e) just below.

Step 4: serde_json::Value → typed structs

Files: crates/craig-intake-sdk/src/types.rs

  1. Define Child, Adult, Narrative structs with AFCARS-aligned fields

  2. Replace children: Value, adults: Value, narrative: Value with typed fields

  3. Add #[serde(default)] for backward compatibility

  4. Add comments to EventEnvelope.payload and canonicalize_json explaining why Value is acceptable there

Step 5: Functions over 40 lines → split

Files (refreshed line numbers):

  1. idempotency_middleware (88 lines, crates/craig-api/src/idempotency.rs:63-151) → extract check_cache(&cache, &cache_key) → Option<Response>, capture_and_cache(parts, body, …) → Response. Middleware body collapses to the POST/key-extraction guards + the two extracted-fn calls.

  2. router (58 lines, crates/craig-api/src/lib.rs:90) → extract build_middleware_stack() that returns the configured ServiceBuilder, and build_core_routes() that wires the /healthz + /metrics + /docs endpoints. router() becomes a merge of those two plus the user-supplied service_routes.

  3. health_check (54 lines, crates/craig-api/src/lib.rs:234) → extract check_database(&db) → HealthStatus and check_rabbitmq(&mq) → HealthStatus. health_check composes the two into the final JSON response.

  4. create_case (77 lines, services/craig-cases/src/api/cases.rs) → extract validate_case_payload(&body) → Result<ValidatedCase, ApiError> and persist_case(&db, &validated, &claims) → Result<Case, ApiError>. Handler drops to auth check + the two fn calls + event publish + JSON response.

  5. update_case (64 lines, same file) → extract validate_status_transition(current, next) → Result<(), ApiError> and apply_case_update(&db, id, patch) → Result<Case, ApiError>. Transition tests already exist in transitions.rs; the extract preserves them.

Out of scope (even though 40+ lines): services/craig-web/src/routes/* handlers. They’re Askama-heavy — the template struct population is the body and extracting it doesn’t reduce cognitive load proportional to the churn.

Step 6: Full validation

  1. cargo fmt --check --all

  2. cargo clippy --workspace — -D warnings

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

  4. cargo xtask e2e — E2E tests pass

  5. Grep: every .expect( in crates/ is either:

    • on an adjacent line below a // Reason: / // Panic: / // Test-only: comment, OR

    • inside a #[cfg(test)] module, OR

    • inside a docstring code block (/// … .expect(…))

      Pre-push clippy additionally enforces clippy::expect_used on non-test code (to add later — not in this plan’s scope).

  6. Grep: every let _ = ` in `crates/ and services/ is either:

    • on an adjacent line below a // Reason: comment, OR

    • the discarded value is immediately-rematched via if let Err(e) = … on the next statement.

  7. Grep: zero ApiError::NotFound(String::from( or ApiError::NotFound("…".to_string()) pattern — all constructors go through the structured variant.

Files Touched

File Change

crates/craig-common/src/error.rs

Structured error variants, From<sqlx::Error>

crates/craig-intake-sdk/src/error.rs

Structured error variants

crates/craig-store/src/error.rs

Structured error variants

crates/craig-auth/src/jwks.rs

expect() → Result

crates/craig-intake-sdk/src/client.rs

expect() → Result

crates/craig-intake-sdk/src/signing.rs

expect() → Result

crates/craig-mq/src/publisher.rs

expect() → Result

crates/craig-mq/src/subscriber.rs

let _ = → tracing::warn

crates/craig-common/src/telemetry.rs

expect() → Result

crates/craig-api/src/idempotency.rs

Split into helper functions

crates/craig-api/src/lib.rs

Split router() and health_check()

crates/craig-intake-sdk/src/types.rs

Value → typed AFCARS structs

services/craig-cases/src/api/cases.rs

Split create_case() and update_case()

All service API modules

Update ApiError call sites for structured variants

Verification

  1. cargo nextest run --workspace --profile integration — all tests pass

  2. cargo xtask e2e — E2E tests pass

  3. cargo xtask validate — full pre-push passes

  4. Grep: grep -rn "expect(" crates/ --include="*.rs" | grep -v test-lib | grep -v "shutdown_signal" — zero results

  5. Grep: grep -rn "let _ =" crates/ services/ --include="*.rs" | grep -v "// " — zero uncommented discards

Documentation Updates

  • .claude/docs/coding-conventions.md — document structured error pattern, expect() policy

  • CHANGELOG.adoc — entry under == Unreleased

Edit this page · latest