Plan H: Idiomatic Rust + Clippy Strictness

On this page

Status

Step Description Status

1

Plan filing — body lands in the docs-only Plan D refresh MR alongside Plans D/G/I/J/K. nav.adoc + CHANGELOG. No code changes.

Not started

2

F-031 implementation: expand [workspace.lints.clippy] from 1 lint (allow_attributes_without_reason) to the full curated set: pedantic, cargo, cognitive_complexity, too_many_lines, panicking-call lints (unwrap_used/expect_used/panic/todo/unimplemented/unreachable/dbg_macro), missing_docs_in_private_items, silent-discard lints (let_underscore_must_use/ignored_unit_patterns). New clippy.toml at repo root with too-many-lines-threshold = 40. Three pedantic sub-lints retained at allow priority=1 for the transition (module_name_repetitions, must_use_candidate, missing_errors_doc, missing_panics_doc — orthogonal to §Style rules). 31 crate-root #![allow(…​)] transitional blocks added (every lib.rs + binary main.rs) for the lints with scheduled sweeps (pedantic, cargo, panicking-call lints, missing-docs-private, too_many_lines, cognitive_complexity, silent-discard lints) — each subsequent sweep MR removes the relevant lint from the allow list. Test-code clippy warnings (tests/*.rs integration tests are separate crate roots; per-crate src/lib.rs allows don’t cascade) surface naturally and are addressed by Plan H Step 3 burn-down per lint family.

Done (2026-05-15)

3

F-031 followup: burn down the warning flood from Step 2. Likely 1 MR per lint family. Common candidates: needless_pass_by_value, too_many_lines-driven decomposition (overlaps with Plan I Step 2), cognitive_complexity, unnecessary_to_owned / unnecessary_clone, missing_docs_in_private_items (the largest category — virtually no internal fn/struct documented today). Run cargo clippy --workspace --all-targets — -D warnings clean by end of step.

Done (2026-05-21) — first-pass via !362 (2026-05-19); test-code warning sweep + missing-docs blanket-allow removal completed during the Step 9 Phase A + Phase B flip (!370 + !376). cargo clippy --workspace --all-targets — -D warnings returns zero violations.

First-pass burn-down done 2026-05-19 — production lib targets now pass cargo clippy --workspace — -D warnings. 4 distinct non-test warnings fixed:

* xtask/src/cmd/lints.rs:82 — clippy::unnecessary_sort_by → `sort_by_key(

b

std::cmp::Reverse(b.1.counted_methods)). * `xtask/src/cmd/lints.rs:220 — clippy::nonminimal_bool + clippy::collapsible_if (same site, 2 lints) → merged the outer if/inner if into one &&-chained boolean and rewrote !recv.mutability.is_some() as recv.mutability.is_none(). * services/craig-security/src/api/partners.rs:463 — clippy::explicit_auto_deref → drop the manual &mut tx reborrow; sqlx executor accepts the tx reference directly post the F-027 transaction-helper migration. * crates/craig-test-lib/src/lib.rs crate-root — added clippy::module_inception to the existing transitional ![allow(…​)] block with a structured reason: the <svc>/<svc>.rs layout (e.g. clients/cases/cases.rs) is deliberate — the client implementation lives in its named file rather than mod.rs so files are searchable by their service name. Renaming would churn imports across ~140 call-site files.

Remaining work for Step 3 close:**

* Test-code warning burn-down (cargo clippy --workspace --all-targets): tests under services//tests/, crates//tests/, and tests/properties/ are separate crate roots; the per-crate src/lib.rs ![allow] blocks don’t cascade to integration-test files. The pedantic / unwrap_used / expect_used warnings surface naturally in those files. Per-test-file #![allow] blocks would close it. * missing_docs_in_private_items — the largest category by warning count (in the thousands). Still suppressed at every crate root pending the Step 9 lint-policy flip; if Step 3 is to make --all-targets — -D warnings clean, the missing-docs blanket adds must come off and every internal fn / struct / enum gains a doc comment. Multi-MR scope; outside this first-pass.

4

F-030 implementation: convert ALL panicking calls in non-tests/ code (scope expanded 2026-05-15 per coding-conventions.md §Style "Use of unwrap, expect, panic, unimplemented!(), unreachable!(), todo!() … not allowed"). Baseline re-anchored 2026-05-18: 1007 .unwrap() + 138 .expect() + 62 panic! + 2 unreachable! + 1 todo! = 1210 sites total workspace-wide (≈2.4× the 2026-05-15 ~501 figure; growth from Plan I + Plan G test/tooling surface). Distribution skews heavily to non-handler code — see §Step 4 for the per-service breakdown. Bootstrap-time invariants previously acceptable as .expect("documented invariant") now must propagate via Result to main — main becomes the only legal panic site (and only at the OS exit boundary). Per-service batches. Sweep starts with the smallest service (craig-rules) to establish the pattern + land foundation From<X> for ApiError impls in crates/craig-common/src/error.rs. crates/craig-test-lib is out of scope as a test-support crate (its `.unwrap()`s ARE the assertion mechanism for callers).

Done (2026-05-18) — all 17 in-scope crates clear: craig-rules + craig-exchange + craig-financial + craig-cli + craig-auth + craig-authz + craig-common + craig-crypto + craig-mq + craig-signing + craig-web + craig-intake + craig-cases + craig-api + craig-mock-server + xtask + craig-seed across 7 MRs

5

F-032 implementation: lock the error-handling policy to typed-everywhere (per coding-conventions.md §Style "Error types cannot be strings", "Do not return Box<dyn std::error::Error>"). Three sub-deliverables: (a) anyhow::Error returns at every public API boundary → thiserror-derived typed enum (~221 sites baseline; expect heavy collapse via shared error types in craig-common); (b) craig-mq::Subscriber trait-bound parameterization is mandatory, not optional — current Result<(), anyhow::Error> → Result<(), Self::Error> with type Error: std::error::Error + Send + Sync + 'static; (c) error-variant audit — every #[error(…​)] variant whose payload is String converts to typed inner. (d) std::io::ErrorKind::Other audit — replace with typed variant per call site. Document in docs/modules/ROOT/pages/architecture.adoc § Error Handling.

Done (2026-05-21) — (a) per-crate sweeps + upstream finale via !364/!365/!366/!368/!371/!372/!373/!377; (b) Subscriber parameterization !361; (c)/(d) zero-violation audits 2026-05-18.

Step 5(b) — Subscriber parameterization (done 2026-05-18): crates/craig-mq/src/subscriber.rs — all 6 subscribe* / internal-helper signatures generic over E (new type param) with bound E: std::fmt::Display + std::fmt::Debug + Send + Sync + 'static.

Deviation from spec: the spec mandates std::error::Error + Send + Sync + 'static, but anyhow::Error (which every current handler closure returns transitively) does not implement std::error::Error — by design, to avoid coherence issues with anyhow’s blanket From<E: StdError> impl. Forcing the strict StdError bound at this point would cascade typed-error definitions through every service’s main.rs subscribe closure body, which is the scope of sub-deliverable (a) and warrants its own per-crate MRs. The relaxed Display + Debug bound captures the spirit of the spec (no longer hardcoded to anyhow::Error; accepts any printable error type) while tolerating the transitional state. Tightening to StdError is gated on sub-deliverable (a) landing across all subscribe call sites.

Call sites updated: 1 (services/craig-rules/src/main.rs:114 subscribe_exclusive::<_, , anyhow::Error> turbofish so type inference resolves at the return Ok)) before engine.reload_all().await? is reached). All other subscribe call sites infer E = anyhow::Error from the closure body without annotation. Step 5(c) — String-payload variant audit (done 2026-05-18): workspace grep [error(…​)] variants with String, payload. Two hits surface in crates/craig-auth/src/actor_verifier.rs:UnknownKey{iss, kid} + WrongAudience{expected, got} — these are typed identifier fields (issuer / key-id / audience claim), not free-form String error messages. They’re correctly structured per the spec’s intent ("typed inner" — the field is a String value representing a typed identifier; the spec target is variants whose ONLY payload is an unconstrained String message). Zero violations. Step 5(d) — io::ErrorKind::Other audit (done 2026-05-18): workspace grep ErrorKind::Other / Error::other. One hit in crates/craig-rules-client/src/lib.rs:275 constructs a marker io::Error inside test infrastructure — not at an API boundary. Zero production-handler hits. Compliant. Step 5(a) — remaining work: workspace audit at branch time surfaces 36 pub fn / pub async fn returning anyhow::Result in non-test code (much less than the 2026-05-15 spec baseline of ~221; that figure was pre-Plan-H-batches when more sites still used anyhow at the boundary). Only 1 of 36 sits at the HTTP-handler boundary (services/craig-intake/src/api/hashing.rs::hash_ip — added by Plan H F-030 batch 4 and acceptable since it propagates to handlers that map via .map_err(ApiError::internal)). The remaining 35 are at internal-module boundaries (store-layer crud, engine spawn helpers, config accessors, telemetry init). Per-crate typed-error conversion is a meaningful refactor; per-service batches. Step 5(a) batch 1 — craig-intake config accessors (done 2026-05-19): services/craig-intake/src/config.rs — 4 pub fn accessors (cases_url, security_url, service_principal, ip_hash_secret) widened from anyhow::Result<> to Result<_, ConfigError>, where the new ConfigError::MissingField { field: &'static str } enum (one variant, typed 'static slug) captures the only legitimate failure shape. All 5 caller sites in services/craig-intake/src/main.rs continue to propagate via ?; main returns anyhow::Result<()> and the From<E: StdError> blanket impl coerces the typed error at the boundary. Tests 143/143 in craig-intake post-migration. Step 5(a) batch 2 — small one-offs across 3 crates (done 2026-05-19): 5 sites across 3 crates each get their own typed enum within a single MR. - craig-mq publisher — new PublishError enum with two [from] variants (Serialize(serde_json::Error), Amqp(lapin::Error)); Publisher::publish + publish_dlx widened from anyhow::Result<()> to Result<(), PublishError>. Sole caller is outbox::run_once which already destructures the error via match/.to_string(); no change required. - craig-common telemetry — new TelemetryInitError enum (otel-feature-gated variants for SpanExporter + PrometheusExporter); telemetry::init widened from anyhow::Result<_> to Result<_, TelemetryInitError>. Callers (services/craig-web/main.rs, crates/craig-api/bootstrap.rs) propagate via ? into anyhow main, coerced by the blanket impl. - craig-cases screening_policy — new ScreeningPolicyDecodeError enum with 3 variants (MissingContent { ruleset_name }, MissingMetadata { ruleset_name }, InvalidMetadata { ruleset_name, [source] source: serde_json::Error }); decode() widened to Result<ScreeningPolicy, ScreeningPolicyDecodeError>. Caller at screening_policy.rs:246 (inside still-anyhow fetch) propagates via ?. NB: ScreeningPolicyClient::fetch itself + the sibling matching::ruleset_metadata::decode remain on anyhow — they have richer multi-step error shape and warrant their own batch. - craig-rules engine — extended the existing EngineError enum with CompileRuleSet([from] serde_json::Error); RulesEngine::reload_all + compile_rule_set widened from anyhow::Result<_> to Result<_, EngineError> (single sqlx + serde_json error source set, both already representable via the existing Db variant + new CompileRuleSet variant). RulesEngine::new itself is still anyhow (multiple distinct error sources from runtime/thread/store layers — own batch). Verification: 437/437 tests across craig-mq + craig-common + craig-cases + craig-rules nextest. Workspace build clean. craig-reporting/store/issues.rs sibling-fn cluster (also anyhow::Result) was originally scoped into batch 2 but dropped on inspection — converting count_unresolved_by_severity alone would leave the file half-converted. Grouped with the craig-security/store batch (the store-layer typed-error sweep) instead. Step 5(a) batch 3 — store-layer typed-error sweep (done 2026-05-19): 79 anyhow::Result return-type sites collapse to Result<_, sqlx::Error> across 11 craig-security/store files + 4 craig-reporting/store files. Pure sweep (Python regex pass + verified no triple-nested generics), since every site fails only via sqlx::Error propagation — ApiError::Db([from] sqlx::Error) already exists in craig_common::error, so handlers continue to work without changes. One exception — services/craig-security/src/store/partners.rs::rotate_key had anyhow::bail! for the cross-partner-ownership case. Replaced with a typed RotateKeyError enum (Db([from] sqlx::Error) + OwnershipMismatch { key_id, partner_id }) plus a From<RotateKeyError> for ApiError impl mapping OwnershipMismatch → 400 and Db → 500 (via the existing sqlx→ApiError bridge). thiserror = { workspace = true } added to services/craig-security/Cargo.toml. Tests 254/254 in craig-security + craig-reporting nextest. craig-reporting test count unchanged (no test surface for store fn signature changes since the test layer goes through HTTP handlers, not raw store). Step 5(a) progress: ~80 of ~90 effective sites migrated (the audit floor of 36 single-line was a substantial undercount; multi-line signatures + helper fns + the rotate_key carve-out put the real total around 90). Remaining: the multi-line cluster in crates/craig-mq::inbox::handle_idempotently, tools/craig-seed::*, and a handful of multi-line signatures across handler/api code that the single-line audit missed. Subsequent batches target those. Step 5(a) batch 4 — remaining simple boundaries (done 2026-05-20): 12 more sites converted to typed sqlx/lapin errors. - services/craig-security/src/detection.rs — 7 sites (1 pub run_detection_scan + 6 private count_* helpers); all sqlx-only. - services/craig-cases/src/matching/seed.rs — 3 sites (collect_candidates, collect_allegations, collect_case_household); all sqlx-only. - crates/craig-api/src/bootstrap.rs::connect_database — anyhow::Result<DbPool> → Result<DbPool, sqlx::Error> (drops the .context("failed to connect to database") wrapping; sqlx::Error’s Display already includes the underlying cause). - crates/craig-api/src/bootstrap.rs::connect_mq — anyhow::Result<_> → Result<_, lapin::Error> (same — drops the now-redundant .context()). - 2 companion test assertions in bootstrap::tests::connect_{database,mq}_invalid_url_returns_err updated to match on the typed-error variant (sqlx::Error::{PoolTimedOut,Io,Database}; lapin::ErrorKind::{IOError,InvalidConnectionState}) instead of the dropped wrapping string. Follow-up status (filed during batch 4; closed 2026-05-21): - 466 — workspace test-crate Plan H Step 2 transitional allow sweep — CLOSED via !369 (56 integration test crate roots + cargo_common_metadata workspace demotion; 208 clippy errors → 0). - #467 — crates/craig-mq::inbox::handle_idempotently generic-E parameterization — CLOSED via !372 (new InboxError<E> enum; 5 test sites turbofished; 6 service main.rs caller closures resolve E via inference). - #468 — services/craig-cases/src/matching multi-source MatchingError enum — CLOSED via !371 (10-variant enum covering transport/serde/db/rules-engine-domain/ruleset-shape failures; 4 fn signatures converted). - #469 — crates/craig-api/src/bootstrap.rs multi-source — CLOSED via !373 (11-variant BootstrapError umbrella; 3 fns converted; 6 variants carry [source] anyhow::Error transitionally pending upstream craig-auth/craig-common typed-error sweeps). - tools/craig-seed::* (9 sites) — explicitly carved out as a build-time tool, not an API boundary. Step 5(a) status (2026-05-21): complete. All originally-in-scope sites migrated to typed errors, then the upstream craig-auth / craig-common sweep closed the last gap (!377): * craig-common::http::build_shared_client → Result<reqwest::Client, reqwest::Error> * craig-auth::oidc_discovery — new OidcDiscoveryError 4-variant enum + 4 fn signatures typed * craig-auth::jwks — new JwksError 10-variant enum + 5 fn signatures typed * craig-auth::keypair_env — new KeypairEnvError 7-variant enum + 2 fn signatures typed; 3 anyhow::bail! sites converted * craig-api::bootstrap::BootstrapError — 6 transitional [source] anyhow::Error variants tightened to typed inner sources (OidcDiscovery, JwksRefresh, JwksProvider, PeerJwksEnv, SigningKeypairEnv, HttpClient) The ClaimsExtractor trait boundary’s BoxedClaimsFuture keeps Result<Claims, anyhow::Error> — the middleware only logs the error before returning 401, so no semantic gain from typing the trait-object boundary. Typed leaves auto-convert via .map_err(Into::into) (anyhow’s From<E: StdError> blanket impl). == Plan H Step 9 Phase A (done 2026-05-21) -D warnings restored on xtask validate. The 7 fully-swept lint families flipped from warn to deny at workspace level via !370: * clippy::unwrap_used — F-030 swept across the workspace * clippy::expect_used — F-030 * clippy::panic — F-030 * clippy::todo — zero production occurrences * clippy::unimplemented — zero production occurrences * clippy::unreachable — zero production occurrences * clippy::let_underscore_must_use — F-051 + Phase A residual sweep ![cfg_attr(test, allow(…​] added to 29 lib roots scopes test-only suppression to [cfg(test)] mod tests blocks while production stays deny-enforced. crates/craig-test-lib remains permanently exempt per Plan H Step 4 §scope (test-support crate; panics ARE the assertion mechanism).

== Plan H Step 9 Phase B (done 2026-05-21)

The remaining 8 warn-level lints flipped to deny at workspace level via !376. Zero code changes — the F-031 burn-down (!311)
Phase A sweep (!370) + Plan I F-033 decomposition collectively brought workspace clippy to zero warnings before this MR. Promotion from warn to deny is a no-op for current code but a hard gate against regressions: new code that violates any of these lints fails cargo clippy locally, not just CI’s -D warnings.

* clippy::allow_attributes_without_reason — every [allow] must carry reason = "…​" * clippy::pedantic (group) — pedantic style sublints across the workspace * clippy::cargo (group) — Cargo.toml metadata + dep hygiene * clippy::cognitive_complexity — function-complexity ceiling * clippy::too_many_lines — function-length ceiling (Plan I F-033) * clippy::dbg_macro — no dbg!() in production code * clippy::missing_docs_in_private_items — every private item documented per coding-conventions.md §Style * clippy::ignored_unit_patterns — no let _ = () / _ = () patterns

Pre-existing priority=1 allows for 5 known-noise sublints are preserved: module_name_repetitions, must_use_candidate, missing_errors_doc, missing_panics_doc, cargo_common_metadata.

Plan H Step 9 → Done. The lint-enforcement track of the Idiomatic-Rust plan is complete; every clippy lint called out in coding-conventions.md §Style is now deny at workspace level.

Verification:

* cargo clippy --workspace --all-targets clean under deny * cargo xtask validate --skip-docker: green * CI -D warnings enforces these as errors on every MR

6

F-050 implementation: sync/async mixing audit (NEW per coding-conventions.md §Style — no mixing sync/blocking code with async code). Grep targets: std::fs / std::io / std::thread::sleep inside async fn; block_on in non-test code; Mutex::lock in async fn (parking_lot’s sync lock is acceptable for short critical sections per parking_lot docs, but tokio::sync::Mutex is the right async-aware choice). Output: per-site classification (convert to async equivalent / wrap in spawn_blocking / accept with documented rationale).

Done (2026-05-18)

Audit done 2026-05-18 across all non-test code in services/, crates/, tools/. Surfaced 7 in-scope sites + 1 block_on site. Resolutions:

* Step 6b (1 site converted): services/craig-cases/src/main.rs:141 — futures::executor::block_on(ZenAuthzEngine::boot(InMemory)) inside an .or_else(|_| { …​ }) fallback chain. Single-threaded runtime deadlock risk. Refactored to an async match chain so the final-fallback path runs as a native .await rather than re-entering the executor via block_on. * Step 6a (6 sites classified SYNC-OK with inline // SYNC-OK (Plan H F-050): <reason> comments): - crates/craig-store/src/store.rs:26 — std::fs::create_dir_all inside pub fn from_config (boot-time, before tokio runtime). - services/craig-intake/src/api/api_key_lookup.rs:115 — std::fs::read_to_string inside pub fn from_file (boot-time partner-keys load). - services/craig-cli/src/auth.rs:74,77 — std::fs::create_dir_all + std::fs::write inside pub async fn login (one-shot CLI command, no concurrent tasks to starve; small JSON write). - services/craig-cli/src/auth.rs:91 — std::fs::read_to_string inside pub fn load_cached_token (sync function; CLI sync caller). - services/craig-cli/src/cmd/login.rs:26 — std::io::stdin().read_line(…​) inside pub async fn run (interactive prompt; blocking IS the intent). - services/craig-cli/src/cmd/icpc.rs:254 — std::fs::read(file) inside pub async fn run (one-shot CLI attach; user-supplied file). * Step 6c (no in-scope sites): the parking_lot::Mutex/std::sync::Mutex usages all live in crates/craig-test-lib (test-support, out of F-030/F-050 scope) — none are held across .await in production code. * Non-issues filtered out: crates/craig-rules-client/src/lib.rs:275 (std::io::Error::other(…​) is a type constructor, not an I/O call); crates/craig-api/src/lib.rs:192 (std::io::Error in a return-type position, not a blocking call).

7

F-051 implementation: silent-discard audit (NEW per coding-conventions.md §Style — no silent runtime failures). Grep targets: let _ = <expr> where <expr> is Result<_, _>; .ok(); on Result (silently drops error); .unwrap_or(…​) / .unwrap_or_default() where the discarded Err shape carries diagnostic value. Per-site triage: convert to ? propagation OR tracing::warn!(error = %e, …​) OR documented // SILENT-OK: <reason> accepting the discard. The lint also lives in F-031 as clippy::ignored_unit_patterns + clippy::let_underscore_must_use (warn).

Done (2026-05-18)

Audit done 2026-05-18 via cargo clippy --workspace --all-targets reading the let_underscore_must_use + ignored_unit_patterns warnings. Zero non-test hits surfaced by the lints in production code.

Manual grep for .ok(); (statement-terminated Result discards) surfaced 6 sites; all are intentional best-effort idioms now documented with inline // SILENT-OK (Plan H F-051): <reason> comments:

* services/craig-web/src/main.rs:49, services/craig-intake/src/main.rs:47, crates/craig-api/src/bootstrap.rs:58 — dotenvy::dotenv().ok(); (devstack-only file; production env via orchestration). * services/craig-web/src/main.rs:565, services/craig-intake/src/main.rs:367 — tokio::signal::ctrl_c().await.ok(); (degrade-not-crash on signal-handler kernel failure; shutdown proceeds anyway). * crates/craig-common/src/telemetry.rs:109 — METRICS_REGISTRY.set(registry).ok(); (OnceLock::set Err only fires on re-init; programmer-error case, existing registry preserved).

Not real silent discards (.ok() used as Result<T, E> → Option<T> conversion preserving the success value):

* services/craig-placement/src/api/placements.rs:120, services/craig-placement/src/api/kinship.rs:92 — Uuid::parse_str(…​).ok(); bound to claim_sub_uuid for later use. * crates/craig-common/src/telemetry.rs:77 — std::env::var("OTEL_EXPORTER_OTLP_ENDPOINT").ok(); bound to otel_endpoint.

Manual grep for let _ = <Result-typed expr> over-matched: most let _ = ident; sites are "keep alive for side effect, suppress unused warning" (e.g. let _ = publisher;) and don’t drop a Result. The let_underscore_must_use clippy lint correctly distinguishes these and reports zero true positives. The let _ = X.await?; form is ?-propagation discarding only the success value (typically ()), also not a silent discard.

8

F-052 implementation: std::sync::Mutex → parking_lot::Mutex migration. Per coding-conventions.md §Style the expect("not poisoned") idiom is forbidden; parking_lot’s mutex doesn’t poison so the call site becomes direct lock() without Result. Grep targets: std::sync::Mutex field types + Mutex::new constructors in CRAIG-authored code (third-party uses out of scope; see coding-conventions.md §Style). Add parking_lot to Cargo.toml workspace deps (verify via cargo tree if it’s already transitively present). Per-service batches.

Done (2026-05-17)

9

F-053 implementation: interior-mutability audit in CRAIG-authored types (NEW per coding-conventions.md §Style — interior mutability for &self mutation requires explicit approval). Third-party types OK (DashMap, governor, parking_lot, tokio sync). Grep CRAIG-authored RefCell / Cell / Mutex<…​> field types. Per-site decision: refactor to &mut self ownership / convert to an approved third-party type with documented rationale / file approval note in ADR (next free ADR number).

Done (2026-05-18)

Audit done 2026-05-18 across all non-test code in services/, crates/, tools/. Grep target: RefCell / UnsafeCell / Cell< / std::sync::Mutex / parking_lot::Mutex / tokio::sync::Mutex / OnceLock / OnceCell / LazyLock field-type usages.

Zero CRAIG-authored interior-mutability types found. 9 hits surfaced, all use the approved third-party / std types per the §Step 9 carve-out:

* tokio::sync::RwLock (6 sites — explicitly allow-listed by the §Step 9 spec): - services/craig-cases/src/screening_policy.rs:164 — cache: Arc<RwLock<HashMap<String, CachedEntry>>> - crates/craig-auth/src/service_token.rs:62 — cache: Arc<RwLock<Option<CachedToken>>> - crates/craig-auth/src/oidc_discovery.rs:58 — cache: Arc<RwLock<Option<CachedDoc>>> - crates/craig-auth/src/jwks.rs:42 — keys: Arc<RwLock<Option<JwkSet>>> - services/craig-intake/src/api/signer_auth.rs:52 — cache: Arc<RwLock<HashMap<String, CachedEntry>>> - services/craig-intake/src/api/partner_auth.rs:64 — cache: Arc<RwLock<HashMap<String, CachedEntry>>>

* std::sync::OnceLock (3 sites — std-provided lazy-init primitive; functionally equivalent to the §Step 9 spec’s "third-party types OK" carve-out): - crates/craig-api/src/lib.rs:298 — START_TIME: OnceLock<Instant> for /livez uptime - crates/craig-common/src/telemetry.rs:32 — METRICS_REGISTRY: OnceLock<prometheus::Registry> - services/craig-cli/src/client.rs — CLIENT: OnceLock<reqwest::Client> (F-030 batch 2 conversion)

All 9 sites are cache / lazy-init patterns using approved primitives. No CRAIG-authored type uses interior mutability for &self mutation that would require an ADR carve-out. No code changes required.

10

Epic: &30 (epic: Idiomatic Rust + Clippy Strictness (Plan H))
Issues: #421 (Step 2) · #422 (Step 3) · #423 (Step 4) · #424 (Step 5) · #449 (Step 6 — F-050 sync/async) · #450 (Step 7 — F-051 silent-discard) · #451 (Step 8 — F-052 parking_lot) · #452 (Step 9 — F-053 interior-mutability) · #425 (Step 10 — plan completion)
Branch prefix: chore/idiomatic-rust- / refactor/idiomatic-rust-
Milestone: TBD

Context

Seven idiomatic-Rust gaps, three from the 2026-05-15 post-Plan-C/F audit and four added 2026-05-15-pm when the project-wide style doctrine was tightened (see .claude/docs/coding-conventions.md §Style):

  • F-030 (P0): 1210 baseline (re-anchored 2026-05-18) = 1007 .unwrap() + 138 .expect() + 62 panic! + 2 unreachable! + 1 todo! in non-test code workspace-wide. The 2026-05-15 figure (~501) was correct at the time; growth comes from Plan I (handler-module-decomposition; new test surfaces + new domain client splits) and Plan G (DRY+YAGNI helpers + new xtask infrastructure). Each per-service batch should re-count at branch time via rg -n '\.unwrap\(\)|\.expect\(|panic!|unreachable!|unimplemented!|todo!' <crate>/src/ | rg -v '#\[cfg\(test\)\]'. Scope expanded 2026-05-15 per coding-conventions.md §Style "Use of unwrap, expect, panic, unimplemented!(), unreachable!(), todo!() … is not allowed. All errors and options must be propagated or handled." Previously, bootstrap-time .expect("documented invariant") was accepted; now every panicking call propagates via Result to main. Only main may panic — and only at the OS exit boundary via eprintln! + std::process::exit(1), not a literal panic!.

  • F-031 (P1): workspace [workspace.lints.clippy] enforces ONE lint (allow_attributes_without_reason = "warn"). Missing the full curated set: pedantic, cargo, cognitive_complexity, too_many_lines, plus the panicking-call lints (unwrap_used, expect_used, panic, todo, unimplemented, unreachable), plus dbg_macro, plus missing_docs_in_private_items (coding-conventions.md §Style requires all functions documented). Function-size threshold = 40 lines per coding-conventions.md §Style ("Almost no function should be more than 40 lines… Less than 10% of total functions should be more than 40 lines"). Subagent triages "important function" overrides per-site.

  • F-032 (P1): 221 anyhow::Error occurrences vs 19 thiserror::Error derives. craig-mq::Subscriber hardcodes anyhow::Error in 6 trait bounds; downstream subscribers can’t surface typed errors. Policy locked 2026-05-15 to typed-everywhere per coding-conventions.md §Style "Error types cannot be strings", "Do not return Box<dyn std::error::Error>. Return concrete rust error types." Subscriber trait-bound parameterization is mandatory, not optional. Also includes String-payload variant audit + std::io::ErrorKind::Other audit.

  • F-050 (P0) NEW: sync/async mixing audit. Per coding-conventions.md §Style: sync/blocking code is never mixed with async code — no nuance. Grep targets per Step 6 row.

  • F-051 (P1) NEW: silent-discard audit. Per coding-conventions.md §Style: no silent runtime failures. Every let _ = result or .ok() on a Result triages to propagate / warn / explicit // SILENT-OK: <reason>.

  • F-052 (P1) NEW: std::sync::Mutex → parking_lot::Mutex migration. The lock().expect("not poisoned") idiom is no longer acceptable; parking_lot doesn’t poison so the call becomes direct lock(). Per Q2 of style review, parking_lot is canonical for CRAIG sync mutexes (tokio::sync::Mutex remains for async-aware paths).

  • F-053 (P2) NEW: interior-mutability audit in CRAIG-authored types. Per coding-conventions.md §Style: interior mutability (RefCell, Cell, Mutex for mutation behind &self) is not allowed without explicit approval. Third-party types (DashMap, governor, parking_lot, tokio sync) are pre-approved per the §Style concurrency-primitives section; this audit scope is CRAIG-authored types only.

Cross-cutting invariants

  1. Step 2 lights up the warning flood deliberately. Expect a large initial warning count after adding pedantic + panicking-call lints + missing_docs_in_private_items. The CI policy for Step 2 specifically: set top-level warnings = "warn" (not deny) until Step 3 drains the flood. After Step 3 lands, flip to warnings = "deny".

  2. Step 4 is per-handler audit, not bulk sed. No site is automatically "correct" anymore — even mutex.lock().expect("not poisoned") is out (Step 8 swaps to parking_lot which doesn’t poison). Bootstrap-time .expect("documented invariant") converts to Result propagation to main. Each conversion to ? requires adding a From<X> for ApiError impl if not already present — that’s part of the work.

  3. Step 5 is policy-locked. Per coding-conventions.md §Style the policy is typed-everywhere. The MR’s job is to execute, not re-litigate. Anyhow at API boundaries is forbidden; the question is which typed error type per crate.

  4. main is the only legal panic site, and even then via process::exit. Every service main becomes: parse config, init tracing, build app, .await the server, log + exit non-zero on error. No panic!, no unwrap. This is the Application Termination Boundary — the one place Result<(), E> resolves.

  5. Step 6 (sync/async) blocks no other steps. Sync/async mixing is an audit — converting a block_on call to spawn_blocking might be a one-line change or might force restructuring a whole handler. Per-site analysis.

  6. Step 8 (parking_lot) ships before Step 4 finishes. mutex.lock().expect("not poisoned") sites stay broken until parking_lot lands; otherwise Step 4 would have to convert them to verbose unwrap_or_else(|e| e.into_inner()) only to be re-simplified by Step 8.

  7. Order matters. Step 2 (lints, warn level) → Step 8 (parking_lot — clears the expect idiom) → {Step 3 (burn-down) || Step 4 (panicking-call sweep)} → Step 5 (error policy execution) → Step 6 (sync/async audit) → Step 7 (silent-discard audit) → Step 9 (interior-mutability audit) → flip lints to deny. Step 3 and Step 4 commute: Step 3’s burn-down explicitly defers _used lints to Step 4 (line in §Step 3 below), so the only binding precedence is Step 8 before Step 4. Steps 6/7/9 may run in any order; they don’t depend on 2-5.

Scope

In scope (7 findings):

  • F-030 panicking-call audit + conversion (ALL unwrap/expect/panic/unimplemented/unreachable/todo)

  • F-031 workspace clippy lint expansion (panicking-call lints + missing_docs_in_private_items + too_many_lines.max_lines = 40) + burn-down

  • F-032 error-handling policy execution (typed-everywhere; mandatory Subscriber trait-bound parameterization; String-payload variant audit; ErrorKind::Other audit)

  • F-050 sync/async mixing audit

  • F-051 silent-discard audit

  • F-052 std::sync::Mutex → parking_lot migration

  • F-053 interior-mutability audit in CRAIG-authored types

Out of scope:

  • DTO type discipline (strum continuation) — Plan D

  • DRY refactors / tx-boilerplate extraction — Plan G

  • Handler / module decomposition — Plan I

  • Env-var sprawl — Plan J

  • Canopy xtask + hook backports — Plan K

  • Partner typed schemas — Plan L

Steps

Step 2: F-031 workspace lint expansion

Files:

  • Cargo.toml — workspace [workspace.lints.clippy] section grows from 1 lint to the full curated set:

    [workspace.lints.clippy]
allow_attributes_without_reason = "warn"
pedantic = "warn"
cargo = "warn"
cognitive_complexity = "warn"
too_many_lines = "warn"
# Panicking calls forbidden per coding-conventions.md §Style
unwrap_used = "warn"
expect_used = "warn"
panic = "warn"
todo = "warn"
unimplemented = "warn"
unreachable = "warn"
dbg_macro = "warn"
# All functions documented per coding-conventions.md §Style
missing_docs_in_private_items = "warn"
# No silent runtime failures per coding-conventions.md §Style
let_underscore_must_use = "warn"
ignored_unit_patterns = "warn"
# Allow noisy-but-style-orthogonal lints with reason (priority=1 escapes pedantic-group)
module_name_repetitions = { level = "allow", priority = 1 }

    NOTE the previously-allowed missing_errors_doc, missing_panics_doc, must_use_candidate are DROPPED — coding-conventions.md §Style mandates docs and the _used lints cover the panic/error doc concerns directly.

  • clippy.toml (NEW or EDIT at repo root) — function-size threshold per coding-conventions.md §Style:

    too-many-lines-threshold = 40

Branch: chore/idiomatic-rust-step2-clippy-lints

MR title: chore(workspace): expand clippy lints — panicking calls, missing-docs, silent-discard, max_lines=40 [Step 2 of idiomatic-rust]

Crate-level allow for _used lints (mandatory in this same MR): Step 2 lights up clippy::unwrap_used + expect_used + panic + todo + unimplemented + unreachable workspace-wide, but Step 4 doesn’t do the sweep — so Step 2 must also land per-crate #[allow(clippy::unwrap_used, clippy::expect_used, reason = "scheduled for Plan H Step 4 sweep")] at every crate’s lib.rs / main.rs top. Without these allows, cargo clippy is unusable between Step 2 and Step 4 lands.

Workspace-warn override mechanism: the [workspace.lints.clippy] block above uses "warn" not "deny". The warnings = "deny" flip happens via:

# Cargo.toml workspace [lints.rust] section, after Step 9 closes:
[workspace.lints.rust]
warnings = "deny"

This is a workspace-level lint table (Rust 2024+; see cargo-features if pinning is required). NOT a -W warnings clippy flag (which would only apply to per-invocation), NOT a .gitlab-ci.yml env-var override (CI invocation reuses workspace lints).

Verification:

  1. cargo clippy --workspace --all-targets --locked runs cleanly (warnings expected); no error-level lints surface immediately

  2. Workspace CHANGELOG note explains the warning flood will drain in Step 3 + subsequent finding-specific steps

  3. [workspace.lints.rust] warnings = "warn" (the default; explicit override not needed during Steps 2-9 since the lints themselves are "warn" level)

  4. After Step 9 closes (final finding audit complete), set [workspace.lints.rust] warnings = "deny"

Step 3: F-031 followup — burn down the warning flood

1 MR per lint family. Expected categories:

  • missing_docs_in_private_items — almost every internal fn / struct. Largest single category; could be its own multi-batch effort. Per-crate batches.

  • needless_pass_by_value — String/HashMap parameters that could be &str/&HashMap

  • too_many_lines (threshold 40) — fat handlers; overlap with Plan I F-033. Plan I Step 2 lands first if both run in parallel; Step 3 here adopts Plan I’s decomposed handlers. Subagent triages "important function" exceptions per-site (coding-conventions.md §Style: <10% may exceed 40 lines).

  • cognitive_complexity — convoluted control flow; overlap with Plan I Step 2

  • unnecessary_to_owned / unnecessary_clone — clone proliferation

  • pedantic sub-lints (redundant_closure_for_method_calls, manual_let_else, etc.) — minor stylistic cleanups

  • _used lints (unwrap_used, expect_used) — these surface every site Step 4 will sweep; Step 3 acknowledges them via #[allow(clippy::unwrap_used, reason = "scheduled for Step 4 sweep")] at the crate or fn level so Step 3’s burn-down doesn’t have to convert before Step 8’s parking_lot lands

Branch (per family): chore/idiomatic-rust-step3-burndown-<lint-family>

Verification (final):

  1. cargo clippy --workspace --all-targets — warning count drops to zero except the _used lints (those are Step 4’s job)

  2. Per-family MR documents starting warning count + ending warning count

Step 4: F-030 panicking-call audit + conversion

Scope (re-anchored 2026-05-18):

  • 1007 .unwrap() (was 413 on 2026-05-15)

  • 138 .expect() (was 88 on 2026-05-15)

  • 62 panic! + 2 unreachable! + 1 todo! (0 unimplemented!)

  • Total: 1210 sites workspace-wide

  • All non-tests/ code is in scope. Test code is out of scope; tests may panic as the assertion mechanism. crates/craig-test-lib is out of scope as a test-support crate — its `.unwrap()`s ARE the assertion surface for its callers.

Per-service distribution (2026-05-18 — TRUE in-scope counts, excluding inline #[cfg(test)] mod tests blocks within src/ files):

The initial 2026-05-18 table over-counted because it included .unwrap()/.expect() calls inside [cfg(test)] mod tests modules embedded in src/ files (test code, OUT OF SCOPE per the §Scope rule). Re-counted 2026-05-18-pm by filtering each file’s hits against the position of its first [cfg(test)] marker:

Crate True in-scope sites Notes

services/craig-rules

0

Done 2026-05-18 batch 1 (3 sites cleared)

services/craig-cases

1 → 0

Done 2026-05-18 batch 5 (validate_link_request refactored to a local typed Role enum so the later match is exhaustive — no unreachable!)

services/craig-cli

1 → 0

Done 2026-05-18 batch 2 (shared_client() returns Result; ApiClient::new propagates)

services/craig-exchange

1 → 0

Done 2026-05-18 batch 2 (semaphore acquire — bail gracefully on shutdown)

crates/craig-auth

2 → 0

Done 2026-05-18 batch 3 (both sites were inside //!-doc-comments — already strict-clippy clean; no code change needed)

crates/craig-authz

2 → 0

Done 2026-05-18 batch 3 (spawn_eval_thread returns Result; runtime build moved outside thread::spawn)

crates/craig-common

2 → 0

Done 2026-05-18 batch 3 (telemetry::init returns Result; OTLP + Prometheus exporter build errors ?-propagate)

crates/craig-crypto

2 → 0

Done 2026-05-18 batch 3 (FieldEncryptor::hmac returns Result; HKDF + HMAC errors ?-propagate)

crates/craig-mq

2 → 0

Done 2026-05-18 batch 3 (Publisher::publish + publish_dlx return anyhow::Result; serde_json errors ?-propagate)

crates/craig-signing

2 → 0

Done 2026-05-18 batch 3 (canonicalize_json returns Result; serde_json errors ?-propagate)

services/craig-financial

2 → 0

Done 2026-05-18 batch 2 (end_of_month chained-fallback)

services/craig-web

5 → 0

Done 2026-05-18 batch 4 (main.rs reqwest builder ?-propagates; i18n.rs skip-with-warn replaces panic! on unparseable locale name; 3 multipart fallback unwraps replaced with Part::bytes(Vec::new()) — reqwest defaults the MIME)

tools/craig-mock-server

5 → 0

Done 2026-05-18 batch 5 (added require_str_field helper in validation.rs returning Result<&str, ValidationError>; replaces .as_str().unwrap() post-validation pattern at 5 mock-route sites)

services/craig-intake

7 → 0

Done 2026-05-18 batch 4 (5 IntakeSettings post-validate accessors now return Result; build_router widened to anyhow::Result<Router>; hash_ip widened to Result<String> (HMAC documented-infallible); build_rate_limiter swapped NonZeroU32::new(x.max(1)).expect(…​) for unwrap_or(NonZeroU32::MIN))

crates/craig-api

10 → 0

Done 2026-05-18 batch 5 (8 idempotency.rs Response::builder()…​.body().unwrap() sites collapsed via new finalize_response helper with non-panicking error!-traced fallback; 2 bootstrap.rs SIGTERM/ctrl-c .expect() sites degraded to log-and-fall-back so signal-handler registration failure logs an error! and the service continues running rather than crashing)

xtask

13 → 0

Done 2026-05-18 batch 6 (reconcile.rs 2 — Reference post-validate expect`s → `.ok_or_else(|| anyhow!(…​))?; quality_budgets.rs 4 — regex compile .expect("regex compiles") → .context(…​)? propagation; coverage_matrix.rs 7 — 5 regex compile .expect → .context(…​)? + 2 caps.get(1).unwrap() collapsed via if let Some(caps) = …​ && let Some(fn_name_match) = caps.get(1) chain)

tools/craig-seed

294 → 0

Done 2026-05-18 batch 7 (fixture-generator CLI sweep — main/lib/generate/write_output/render_manifest/6× render_*_sql widened to anyhow::Result<…​>; 252 writeln!()…​.unwrap() calls bulk-replaced via Edit replace_all with writeln!()…​? and tail String returns wrapped with Ok(…​); 26 NaiveDate::from_ymd_opt(…​).unwrap() + 3 Utc.with_ymd_and_hms(…​).unwrap() literals replaced via new ymd(y, m, d) / ymd_hms(…​) helpers using unwrap_or_default() per Plan H allow-list; 7 case_id.unwrap() Option calls collapsed via let c_id = c.id binding before the push; remaining 4 sites refactored to let-else { continue } + filter_map chains)

TOTAL in-scope (estimated)

~408

crates/craig-test-lib

—

Out of scope (test-support crate)

services/craig-reporting

—

All hits inside inline #[cfg(test)] mod tests

services/craig-placement

—

All hits inside inline #[cfg(test)] mod tests

crates/craig-store / db / matching / reference / security

—

Mostly test-inline or none after filter

Methodology note: per-batch recount uses for f in $(rg -l '…​' <crate>/src); do n=$(rg -n '…​' "$f" | awk -F: -v b=$(grep -n '^#\[cfg(test)\]' "$f" \| head -1 \| cut -d: -f1) '\$1 < b'); done. The 2026-05-15 baseline (~501) was correct under the same methodology (no test-inline filter wasn’t on yet because counted differently). The 2026-05-18 morning recount of "1210" was unfiltered (included inline tests); the corrected count is ~408. Each per-service batch should still re-count at branch time.

Batching strategy: one MR per crate, ordered smallest → largest within service/library tier. Multi-crate "small-batch" MRs OK when each crate has only 1-3 sites and the conversion patterns are similar (e.g. batch 2 above bundled 3 crates × 4 total sites). Tools (xtask + craig-seed + mock-server, ~312 sites) ship in their own batches at the end since they’re CLI binaries where main is the legal panic boundary — the conversion shape there is "propagate to main, exit non-zero on err" rather than HTTP handler chains.

Files: per-service batches; sites identified by cargo clippy …​ — -W clippy::unwrap_used -W clippy::expect_used -W clippy::panic reading the warnings list.

Process per site:

  1. Classify against one rule: this site exists in a function whose return type is Result<T, E> or can be made so?

    1. If yes (overwhelming default): propagate via ? + appropriate From<X> for ApiError impl

    2. If no (e.g. axum’s IntoResponse for some structured error path): wrap in Result at the outermost boundary; propagate inward

  2. mutex.lock().expect(…​) sites: SKIP — Step 8 swaps to parking_lot which doesn’t poison; converting now means converting twice

  3. Bootstrap-time expect("documented invariant") in service main: convert to Result propagation; main returns Result<(), MainError>; the OS exit happens via the bootstrap shim’s match on the Result

Branch (per service): refactor/idiomatic-rust-step4-panicking-calls-<service>

Verification:

  1. cargo nextest run --workspace — no test regression

  2. Per-service cargo clippy -p <svc> — -D clippy::unwrap_used -D clippy::expect_used -D clippy::panic -D clippy::todo -D clippy::unimplemented -D clippy::unreachable clean

  3. New From<X> for ApiError impls land in crates/craig-common/src/error.rs as needed

Step 5: F-032 error-handling policy execution

Process:

  1. Policy lock (no decision left to make): typed-everywhere per coding-conventions.md §Style.

  2. Three sub-deliverables:

    1. (a) anyhow::Error returns at API boundaries → thiserror-derived typed enum. Estimated ~221 sites. Per-crate batches.

    2. (b) craig-mq::Subscriber trait-bound parameterization (mandatory):

      // Before
pub trait Subscriber: Send + Sync {
    fn process(&self, payload: ...) -> Result<(), anyhow::Error>;
}

// After
pub trait Subscriber: Send + Sync {
    type Error: std::error::Error + Send + Sync + 'static;
    fn process(&self, payload: ...) -> Result<(), Self::Error>;
}

      Cascading migration shape: the trait-change MR ships first (craig-mq only). Then N per-service impl Subscriber migrations land as small follow-up MRs. Surfaced impl sites at branch time via rg -n 'impl.*Subscriber.*for' services/ crates/. Expected count: 6+ per craig-mq’s current bound-count, including the wildcard audit subscriber in `services/craig-security. One trait-change MR + ~6 per-service impl MRs. The trait-change MR uses type Error = anyhow::Error as the per-impl default during the transition; each follow-up MR tightens to a typed <Service>SubscriberError enum.

    3. (c) error-variant audit — every #[error("…​")] variant whose payload is String must convert to a typed inner error type. Per-crate grep -rE '\{[^}]+:\s*String\s*\}' crates//src/error.rs services//src/error.rs to enumerate.

    4. (d) std::io::ErrorKind::Other audit — replace each construction with a typed variant. grep -rn "ErrorKind::Other" services/ crates/.

  3. Document policy in docs/modules/ROOT/pages/architecture.adoc § Error Handling (new section).

Branch: refactor/idiomatic-rust-step5-typed-errors

MR title: refactor: lock error-handling policy to typed-everywhere; parameterize Subscriber trait bound [Step 5 of idiomatic-rust]

Verification:

  1. cargo build --workspace clean after Subscriber trait bound change

  2. cargo nextest run --workspace — no regression

  3. grep -rn "anyhow::Error" services/ crates/ returns zero matches in pub fn / pub async fn signatures (internal use may persist where it doesn’t cross the API boundary)

  4. grep -rn "Box<dyn .*Error" services/ crates/ returns zero matches in return types

  5. grep -rn ".: String," crates//src/error.rs services/*/src/error.rs shows zero unconverted String-payload variants

Step 6: F-050 sync/async mixing audit (NEW; splits into 6a/6b/6c)

Sub-divided because "every async fn that contains a blocking call site" is potentially every handler in the codebase. Each sub-step is its own MR:

  • Step 6a: std::fs/std::io inside async fn → mechanical conversion to tokio::fs / tokio::io::AsyncRead-or-AsyncWrite. Often one-line edits + import swaps. Largest expected category.

  • Step 6b: block_on audit (structural) — tokio::runtime::Handle::block_on / futures::executor::block_on in non-test code. Each site needs restructuring (either move work to an async fn boundary OR spawn_blocking if it’s CPU-bound).

  • Step 6c: Mutex-across-.await audit — std::sync::Mutex / parking_lot::Mutex held across an .await is a deadlock risk; convert to tokio::sync::Mutex. Step 8 (parking_lot migration) happens-before; this sub-step focuses on async-held mutexes specifically.

Files: every async fn that contains a blocking call site; surfaced via audit grep.

Audit grep targets:

# std::io / std::fs blocking calls inside async
rg -t rust 'std::(fs|io)::' --files-with-matches | xargs rg -l 'async fn'
# std::thread::sleep inside async
rg 'std::thread::sleep' --type rust
# block_on outside test code
rg 'tokio::runtime::Handle::block_on|futures::executor::block_on' services/ crates/
# Mutex::lock inside async fn (parking_lot's mutex is brief-critical-section-acceptable per its docs;
# tokio::sync::Mutex is the right async-aware choice for held-across-await locks)
rg 'std::sync::Mutex|parking_lot::Mutex' --type rust | xargs rg -B5 'async fn'

Process per site:

  1. Classify: brief sync call that won’t yield ≥ ~10µs (acceptable per Tokio docs — e.g. String::new()) / blocking call that should be async (convert to tokio::fs / tokio::io::AsyncRead / etc.) / blocking call that has no async equivalent (wrap in tokio::task::spawn_blocking)

  2. Document the decision per site with a // SYNC-OK: <reason> comment for accepted brief-sync; otherwise convert

  3. Special case: parking_lot::Mutex acquired briefly inside async is OK; held across .await is a deadlock risk — convert to tokio::sync::Mutex in that case

Branch: refactor/idiomatic-rust-step6-sync-async-audit

MR title: refactor: sync/async mixing audit + conversions per coding-conventions.md §Style [Step 6 of idiomatic-rust]

Verification:

  1. cargo nextest run --workspace — no test regression

  2. cargo clippy --workspace --all-targets — no new warnings introduced

  3. Audit report committed as MR body or .claude/docs/sync-async-audit.md

Step 7: F-051 silent-discard audit (NEW)

Files: every let _ = result_expr / .ok(); / .unwrap_or_default() on a Result; surfaced via the new clippy lints from Step 2.

Audit grep targets: These greps are exploratory — they over-match (e.g. let _ = expensive_compute() where the type is (), not Result). Manual inspection per site for the actual Result-discard cases. The authoritative gate is the clippy lints from Step 2 (let_underscore_must_use + ignored_unit_patterns), NOT the grep counts.

# Bare let _ = (expression returning Result) — manually inspect each hit
rg 'let _ =' services/ crates/
# .ok() on a Result silently drops the error
rg '\.ok\(\);' services/ crates/
# .unwrap_or_default() — discards error but may hide bugs
rg '\.unwrap_or_default\(\)' services/ crates/

Process per site:

  1. Classify: is the discard intentional (e.g. cleanup-best-effort, idempotent retry, fire-and-forget log)?

    1. If yes → convert to explicit if let Err(e) = expr { tracing::warn!(error = %e, "context"); } + // SILENT-OK: <reason> comment

    2. If no → propagate via ? + appropriate error mapping

Branch: refactor/idiomatic-rust-step7-silent-discard-audit

Verification:

  1. cargo clippy --workspace --all-targets — -D clippy::let_underscore_must_use -D clippy::ignored_unit_patterns clean

  2. grep -c "SILENT-OK:" services/ crates/ — count of accepted discards documented in MR body

Step 8: F-052 std::sync::Mutex → parking_lot migration

Files: every CRAIG-authored std::sync::Mutex field or local; surfaced via:

rg 'std::sync::Mutex' services/ crates/
rg 'use std::sync::Mutex' services/ crates/

(Third-party uses are out of scope per Q2 of the style review.)

Per-site change:

  1. Replace std::sync::Mutex<T> field type with parking_lot::Mutex<T>

  2. Replace mutex.lock().expect("not poisoned") / .unwrap() with mutex.lock() (no Result, no panic)

  3. If the site holds the lock across .await: convert to tokio::sync::Mutex<T> instead — parking_lot’s mutex is brief-critical-section only

Cargo.toml workspace deps: verify parking_lot is present via cargo tree -i parking_lot. If transitively-present, add as explicit workspace dep with cargo add parking_lot --package <root> (or hand-edit the workspace Cargo.toml per the delivery-protocol standard’s Architectural-Recommendations guidance (check existing infra/tooling first)).

Branch: refactor/idiomatic-rust-step8-parking-lot-mutex

MR title: refactor: std::sync::Mutex → parking_lot::Mutex (no poisoning, no expect idiom) [Step 8 of idiomatic-rust]

Verification:

  1. cargo nextest run --workspace — no regression

  2. grep -rn "std::sync::Mutex" services/ crates/ returns zero matches in non-test, non-FFI code

  3. grep -rn "expect(\"not poisoned\")\|expect(\"poisoned\")" services/ crates/ returns zero matches

Step 9: F-053 interior-mutability audit (NEW)

Files: every CRAIG-authored RefCell, Cell, or Mutex<T> field exhibiting &self mutation; surfaced via:

rg 'RefCell|Cell<' services/ crates/
# Mutex sites already audited in Step 8; this catches the remaining
# CRAIG-authored cases where Mutex provides &self-mutation deliberately
rg 'parking_lot::Mutex|tokio::sync::Mutex' services/ crates/  # post-Step-8

(Third-party types — DashMap, governor, parking_lot internals, tokio sync — are explicitly approved per Q2 of the style review; this audit covers CRAIG-authored types only.)

Per-site decision:

  1. Refactor to &mut self ownership (preferred — eliminates the interior-mutability requirement)

  2. Convert to an approved third-party type (DashMap for concurrent map mutation, parking_lot::Mutex for short critical sections, tokio::sync::Mutex for async-held locks)

  3. File an approval note in a new ADR if interior mutability is genuinely required (rare — typically only for Cell in static-init paths)

Branch: refactor/idiomatic-rust-step9-interior-mutability-audit

Verification:

  1. cargo nextest run --workspace — no regression

  2. grep -rn "RefCell\|Cell<" services/ crates/ --include='*.rs' | grep -v 'tests/' | grep -v 'examples/' returns zero matches OR each remaining match has an ADR reference comment

  3. After this step, set warnings = "deny" workspace-wide (delivers F-031 final flip)

Step 10: Plan completion audit + archive

Mirror Plan B Step 8 / Plan C Step 18 / Plan F Step 6 pattern.

Final flip checklist (per the project_plan_h_transition_state memory):

  1. Cargo.toml [workspace.lints.clippy] — 15 lints flipped from warn to deny (Phase A !370 swept 7 + Phase B !376 swept the remaining 8). Done.

  2. xtask/src/cmd/validate.rs — -D warnings restored on clippy invocation via !370.

  3. Per-crate ![allow(…​)] transitional blocks — all removed; ![cfg_attr(test, allow(…​))] on 29 lib roots scopes test-only suppression to #[cfg(test)] mod tests. crates/craig-test-lib + crates/craig-auth/src/jwks.rs::test_fixtures remain permanently exempt as test-support code.

  4. clippy::wildcard_enum_match_arm = "deny" — shipped post-archive 2026-05-21 via !380. 5 production sites fixed (serde_json::Value matches enumerated to primitives; EngineError variant list spelled out; fn-level reasoned-allows for object_store::Error non_exhaustive + syn::Type 14-variant cases). Test code allow-listed via existing cfg_attr(test, allow) pattern. Workspace lint count: 15 → 16 lints deny.

  5. Per-library-crate #![deny(unreachable_pub)] — shipped post-archive 2026-05-21 via !381 across 15 lib roots. 1 production site fixed (crates/craig-api/src/otel.rs::otel_propagation → pub(crate)).

  6. Per-library-crate #![warn(unused_crate_dependencies)] — deferred. Initial probe suggested zero hits but --all-targets clippy surfaces ~15 across the workspace (dev-deps used only in tests/*.rs, stale [dependencies] entries). Requires per-crate cleanup MR.

  7. Per-library-crate #![warn(missing_docs)] — still deferred. 2400+ hits; cleanup-first work on a much larger surface than the other two lints.

  8. cargo clippy --workspace --all-targets — -D warnings returns 0 violations. ✓

  9. cargo xtask validate --skip-docker green. ✓

Completion-audit findings (2026-05-21): see [§Plan H completion audit](#_plan_h_completion_audit) below.

Plan H completion audit (2026-05-21)

Spawned a plan-completion-audit subagent per delivery-protocol.md § Plan Completion Audit. Findings:

All 9 substantive steps shipped. Per-step verification against the git history:

  • Step 2 (F-031 workspace lints) — !311 → de558dd (2026-05-15)

  • Step 3 (F-031 burn-down) — !362 first-pass (2026-05-19) + test-code residual sweep folded into !370/!376

  • Step 4 (F-030 panicking-call sweep) — 7 MRs across 17 crates (2026-05-18)

  • Step 5 (F-032 typed-everywhere) — !361 (Subscriber) + !364/!365/!366/!368 (per-crate batches 1-4) + !371/!372/!373 (multi-source follow-ups #467/#468/#469) + !377 (upstream craig-auth + craig-common finale)

  • Step 6 (F-050 sync/async) — !358 (2026-05-18)

  • Step 7 (F-051 silent-discard) — !359 (2026-05-18) + !370 Phase A residual sweep

  • Step 8 (F-052 parking_lot) — !346 + !350 (2026-05-17/18)

  • Step 9 (F-053 interior-mutability + lint-enforcement flip) — !360 (interior-mutability audit; zero CRAIG-authored hits) + !370 (Phase A: 7 lints flipped to deny) + !376 (Phase B: 8 lints flipped to deny)

Acceptance criteria from §After this plan lands:

Criterion Status

Workspace clippy enforces pedantic + cargo + complexity + panicking-call + missing-docs + silent-discard lints with warnings = "deny"

✓

Function-size threshold = 40 lines (clippy.toml)

✓

All panicking-call sites converted; main is the only legal panic site

✓ (F-030 batches 1-7)

Error-handling policy: typed-everywhere; zero anyhow::Error at API boundaries

✓ (Step 5 + upstream sweep !377)

craig-mq::Subscriber trait-bound parameterized

✓ (!361)

Zero sync/async mixing (or each site SYNC-OK)

✓ (Step 6 !358)

Zero silent-discard (or each site SILENT-OK)

✓ (Step 7 !359 + !370)

Zero std::sync::Mutex in CRAIG code

✓ (Step 8 !346 + !350)

Zero CRAIG-authored interior-mutability (or each has an ADR ref)

✓ (Step 9 !360 — zero hits found)

Issues closed: #421 (Step 2), #422 (Step 3), #423 (Step 4), #424 (Step 5), #449 (Step 6), #450 (Step 7), #451 (Step 8), #452 (Step 9), #425 (Step 10 — this MR). Plus 4 follow-ups closed during the 3-day sweep: #465, #466, #467, #468, #469.

Plan archived to docs/modules/ROOT/pages/plans/archive.adoc § Code Quality and removed from nav.adoc § Planned. Phase Status row added to .claude/CLAUDE.md.

Files Touched

File Step Change

Cargo.toml

2

EDIT (workspace lints — full curated set)

clippy.toml

2

NEW or EDIT (max_lines = 40)

Per-service src/**

3,4,6,7,8,9

EDIT (lint burn-down + panicking-call conversions + sync/async + silent-discard + parking_lot + interior-mutability)

crates/craig-common/src/error.rs

4,5

EDIT (new From<X> for ApiError impls + typed-error variants)

crates/craig-mq/src/subscriber.rs

5

EDIT (trait-bound parameterization — mandatory)

Per-crate src/error.rs files

5

EDIT (String-payload variant audit + typed inner types)

docs/modules/ROOT/pages/architecture.adoc

5

EDIT (error-handling policy section)

Workspace Cargo.toml

8

EDIT (add parking_lot if not transitively present)

Verification

After every step: cargo xtask validate --skip-docker + cargo nextest run --workspace.

Risks

Risk Mitigation

Step 2 surfaces so many warnings that Step 3 burn-down becomes its own multi-week effort

Split into Plan H1 (steps 2+4 — lints on + unwrap audit) and Plan H2 (step 3 — burn-down) if scope explodes

Step 4 ? conversion forces new From<X> for ApiError impls that proliferate the error type

Accept the surface growth; the error type is the right place to centralize. Use #[from] attribute on each variant

Step 5 picks a policy that invalidates a chunk of existing code (e.g. typed-everywhere forces a sweep of all 221 anyhow sites)

If typed-everywhere is the chosen policy, defer to a separate sub-plan; ship policy doc + Subscriber trait change only in Step 5

Step 2’s lint allow-list grows large enough to defeat the purpose

Audit allow-list quarterly; remove allows as code-style improves

After this plan lands

  • Workspace clippy enforces pedantic + cargo + complexity + panicking-call + missing-docs + silent-discard lints with warnings = "deny"

  • Function-size threshold = 40 lines (strict per coding-conventions.md §Style; subagent-triaged "important" overrides documented)

  • All 501 panicking-call sites (unwrap + expect + panic + unimplemented + unreachable + todo) converted to Result propagation; main is the only legal panic site

  • Error-handling policy: typed-everywhere. Zero anyhow::Error at API boundaries; zero Box<dyn Error> returns; zero String-payload error variants

  • craig-mq::Subscriber trait bound parameterized (mandatory)

  • Zero sync/async mixing — every site converted or documented SYNC-OK

  • Zero silent-discard — every site converted or documented SILENT-OK

  • Zero std::sync::Mutex in CRAIG code (parking_lot for sync; tokio::sync::Mutex for async-held)

  • Zero CRAIG-authored interior-mutability sites (or each has an ADR reference)

Appendix: Conversion patterns reference (Steps 4/6/7)

Each per-site Plan H sweep produces the same recurring conversion shapes. This reference is consulted by per-batch MRs so the conversion choice is canonical and reviewable, not improvised.

Step 4 (F-030) — panicking-call conversions

Pattern Before After First-shipped reference

Static HTTP header value

"application/json".parse().expect("static")

HeaderValue::from_static("application/json")

craig-rules/api.rs:687 (batch 1)

Runtime HTTP header value

format!(…​).parse().expect("…​")

HeaderValue::try_from(format!(…​))? + new From<InvalidHeaderValue> for ApiError in craig-common

craig-rules/api.rs:693 (batch 1)

Tokio runtime build inside thread::spawn

Builder::build().expect(…​) inside the spawn closure

let rt = Builder::build()?; thread::Builder::new().spawn(move || rt.block_on(…​))? — current-thread runtime is Send

craig-rules/engine.rs:80, craig-authz/eval_thread.rs:33 (batch 1/3)

Bootstrap config-validated expect

config_field.expect("validated as present")

Propagate via Result from the constructor; cascade with ? to main

craig-cli/client.rs, craig-intake/config.rs (batch 2/4)

OnceLock::get_or_init with fallible init

OnceLock::get_or_init(|| build().expect(…​))

Manual race-tolerant try-init: optimistic get() → fallback build()? + set() → final get-or-bail!. get_or_try_init is unstable as of Rust 1.94

craig-cli/client.rs (batch 2)

Semaphore acquire_owned inside loop

.acquire_owned().await.expect("never closes")

match acq.await { Ok(p) ⇒ p, Err(_) ⇒ { debug!(…​); break; } } (worker-shutdown bail)

craig-exchange/send_worker.rs:140 (batch 2)

Chrono arithmetic chain

from_ymd_opt(…​).expect().pred_opt().expect()

.and_then(|d| d.pred_opt()).unwrap_or(date) — unwrap_or is non-panicking and on the Plan H allow-list

craig-financial/main.rs:481 (batch 2)

Documented-infallible crypto / serde

.expect("infallible")

.map_err(|e| anyhow::anyhow!("op: {e}"))? — propagate even when practically unreachable

craig-crypto/lib.rs::hmac, craig-mq/publisher.rs (batch 3)

serde_json::to_string on String / Value

serde_json::to_string(…​).expect(…​)

serde_json::to_string(…​)? + change fn signature to Result<String, serde_json::Error>

craig-signing/lib.rs::canonicalize_json (batch 3)

OTLP / Prometheus exporter build

.build().expect("…​")

.build().map_err(|e| anyhow::anyhow!(…​))? + cascade init() to return anyhow::Result

craig-common/telemetry.rs (batch 3)

Doc-comment artifact

Literal todo!() / .expect(…​) inside //! or /// block

No code change — clippy lints don’t fire on doc-comments. Verify with cargo clippy …​ — -D clippy::unwrap_used first

craig-auth/lib.rs (batch 3)

unreachable!() after a validated invariant

_ ⇒ unreachable!("role validated above") at the end of a match

PREFERRED: local typed enum that lifts validation into the parse step so the later match is exhaustive. Eliminates BOTH the unreachable! AND the duplicate validation gate. Alternative: #[allow(clippy::unreachable, reason = "…​")] with structured justification

craig-cases/api/report_persons.rs::validate_link_request (batch 5)

CLI main + lib widening (whole-crate cascade)

fn main() { …​expect("…​") } + pub fn helper() → T

fn main() → anyhow::Result<()> { …​? } + pub fn helper() → anyhow::Result<T>. For seed/fixture CLIs with many writeln!() sites: bulk Edit replace_all .unwrap(); → ?;, then wrap render-tail expressions with Ok(…​). Tests cascaded via .unwrap() (out of F-030 scope)

craig-seed (batch 7 — 294 sites)

Chrono date constants in fixture data

NaiveDate::from_ymd_opt(2024, 1, 1).unwrap() + Utc.with_ymd_and_hms(…​).unwrap() repeated dozens of times

Module-local helpers using unwrap_or_default() (Plan H allow-list): fn ymd(y, m, d) → NaiveDate { NaiveDate::from_ymd_opt(y, m, d).unwrap_or_default() } + ditto for ymd_hms. Bulk-rewrite via sed. The fallback (1970-01-01) is unreachable for literal-constant args

craig-seed/datagen.rs (batch 7)

Axum Response::builder().body(…​).unwrap()

8 sites in idempotency.rs constructing Response from string-literal headers + already-validated `HeaderValue`s

Single helper finalize_response(builder: Builder, body: Body) → Response using .unwrap_or_else(|e| { error!(error = %e, "…​"); Response::new(Body::empty()) }). Empty-body fallback + error! trace

craig-api/idempotency.rs (batch 5)

Option<T>::unwrap() after Option-becoming-Some assignment

case_id = Some(c.id); …​ let x = case_id.unwrap(); (multiple sites inside one function)

Bind a non-Option local before the assignment: let c_id = c.id; case_id = Some(c_id); …​ let x = c_id;. Move-aware: bind BEFORE data.cases.push(c) to avoid use-after-move

craig-seed/datagen.rs::generate_family (batch 7)

Option::unwrap() after .is_some() filter

.filter(|p| p.foster_home_id.is_some()).map(|p| (…​, p.foster_home_id.unwrap()))

Collapse to .filter_map(|p| p.foster_home_id.map(|fhid| (…​, fhid))). For iter().take(N) with pre-filtered Option: let Some(x) = opt else { continue };

craig-seed/datagen.rs (batch 7)

Validation helper for mock-server JSON extraction

require_fields(&body, &["x"])?; let x = body["x"].as_str().unwrap();

Add pub fn require_str_field<'a>(body: &'a Value, field: &str) → Result<&'a str, ValidationError> that combines existence + string-type check. Returns structured 400 instead of panicking on non-string types

craig-mock-server/validation.rs (batch 5)

Subscriber generic parameterization (Plan H Step 5b)

Fut: Future<Output = Result<(), anyhow::Error>> (hardcoded)

Fut: Future<Output = Result<(), E>> + E: std::fmt::Display + std::fmt::Debug + Send + Sync + 'static. NB: bound relaxed from spec’s std::error::Error — anyhow::Error doesn’t impl StdError. Single call site needs turbofish subscribe::<_, _, anyhow::Error> when closure body returns Ok(()) BEFORE the ? propagation that would otherwise resolve E. Tightening to StdError gates on Step 5(a)

craig-mq/subscriber.rs + craig-rules/main.rs (Step 5 MR !361)

Step 6 (F-050) — sync/async mixing

Pattern Before After First-shipped reference

block_on inside .or_else() fallback closure

.or_else(|_| { futures::executor::block_on(async_fn(…​)) }) — deadlock risk on single-threaded runtime

Restructure to async match chain: replace nested .or_else with explicit match ZenAuthzEngine::boot(…​).await { Ok(p) ⇒ p, Err(_) ⇒ fallback_async().await? }

craig-cases/main.rs (Step 6b)

SIGTERM / ctrl_c registration .expect

let mut sigterm = signal(SignalKind::terminate()).expect("…​");

Degrade-not-crash match: match signal(SignalKind::terminate()) { Ok(mut s) ⇒ tokio::select! { …​ }, Err(e) ⇒ { error!(error = %e, "…​"); ctrl_c.await.ok(); } }. Non-Unix: std::future::pending().await after log on error

craig-api/bootstrap.rs::shutdown_signal (batch 5)

Brief sync I/O in async fn (SYNC-OK classification)

std::fs::write(…​) inside pub async fn login (one-shot CLI)

KEEP the std::fs call; add inline // SYNC-OK (Plan H F-050): <reason> comment. Common reasons: (a) boot-time pub fn before tokio runtime accepts traffic; (b) one-shot CLI with no concurrent tasks to starve

craig-cli/{auth, cmd/login, cmd/icpc}.rs, craig-intake/api/api_key_lookup.rs (Step 6a)

Step 7 (F-051) — silent-discard

Pattern Before After First-shipped reference

.ok(); Result discard (SILENT-OK classification)

dotenvy::dotenv().ok(); / tokio::signal::ctrl_c().await.ok(); / OnceLock::set(…​).ok();

KEEP the .ok(); call; add inline // SILENT-OK (Plan H F-051): <reason> comment. Common idioms: dotenv-optional (production env via orchestration); ctrl_c-degrade-on-failure; OnceLock-idempotent (Err only on re-init)

craig-{web,intake,api/bootstrap}.rs, craig-common/telemetry.rs (Step 7)

Process notes

  • Cascade-management: production callers cascade with ? (typically already in anyhow::Result or Result<_, ApiError>). Test sites (anywhere in tests/ OR inside #[cfg(test)] mod tests) get .unwrap() appended — out of F-030 scope. crates/craig-test-lib/src/…​ is the test-support crate; out of scope; .unwrap() on its own internal calls is fine.

  • Non-panicking ops on the allow-list: unwrap_or, unwrap_or_else, unwrap_or_default, ok_or / ok_or_else, ?, let-else { bail!(…​) } are all Plan H-compliant.

  • Verify before working: run cargo clippy -p <crate> — -D clippy::unwrap_used -D clippy::expect_used -D clippy::panic -D clippy::todo -D clippy::unimplemented -D clippy::unreachable first. If clean, no work needed — even if rg finds occurrences in doc-comments.

  • Count in-scope sites properly: rg alone over-counts. Filter each file’s hits against its first #[cfg(test)] marker position:

    boundary=$(grep -n "^#\[cfg(test)\]" "$f" | head -1 | cut -d: -f1)
rg -n '...' "$f" | awk -F: -v b="$boundary" '$1 < b' | wc -l

  • Foundation craig-common::error variants added (batch 1, reusable): ApiError::Header(#[from] axum::http::header::InvalidHeaderValue) — maps to 500 in IntoResponse (invalid header is server-side data-integrity bug, never user-driven).

  • Per-batch shipping pattern: branch refactor/idiomatic-rust-step4-<scope> off fresh main; convert sites; cargo fmt --all; build + nextest + strict clippy; refresh §Step 4 status + CHANGELOG; Q1-Q8 walkthrough; commit + push; force-merge runbook; pull main + branch cleanup.

Edit this page · latest