Plan: Code Quality Review (April 2026)

On this page

Status

Step Description Status

1

Fix real bugs (UTF-8 truncation, idempotency anonymous collision, dead retry counter)

Done (2026-04-20) — MR !105

2

Replace fragile error matching and silent HTTP body drops

Done (2026-04-20) — MR !106

3

Inject a shared reqwest::Client into services (closes 4 TODOs)

Done (2026-04-20) — MR !108

4

Consolidate duplicate canonicalize_json into craig-intake-sdk

Done (2026-04-20) — MR !106 consolidated test-lib → SDK; MR !120 (#196 — moved the canonical impl into a new minimal craig-signing crate — both the SDK and craig-intake now re-export from there, Step 13 gate 2 passes with exactly one definition)

5

Decompose services/craig-intake/src/sink.rs and convert convert_report to event-driven

Done (pre-ADR-030). Step 5a (sink decomposition) shipped MR !112. Step 5b (event-driven convert_report) superseded by stateless-intake plan (ADR-017): convert is now an internal cases-side transition that publishes case.report_converted directly — the original two-stateful-services event-driven design (ADR-016) was mooted by ADR-017 redrawing the service boundaries entirely. #197 closed via stateless-intake Step 12 audit.

6

DRY the craig-exchange adapters (transform helper + macro-driven tests)

Done (2026-04-20) — MR !115

7

Collapse craig-test-lib builder and client-factory duplication

Done (2026-04-21) — MR !116, partial — client-factory collapse + unauthenticated client fix; builder-macro portion skipped, see plan note

8

Replace hand-rolled TSV generation in craig-reporting with the csv crate

Done (2026-04-20) — MR !107

9

Modernize craig-reference (ASCII-case comparisons, reuse strum::FromStr)

Done (2026-04-20) — MR !106

10

craig-web i18n: store messages as Arc<str> to eliminate per-render clones

Done (2026-04-20) — MR !114

11

craig-security: share reqwest::Client, add webhook retry with non-blocking spawn

Done (2026-04-20) — MR !113

12

Audit remaining #[allow(…​)] attributes — delete, justify, or convert

Done (2026-04-21) — MR !117

13

Full validation

Done (2026-04-21) — MR !118

Issues: #181
Related: #179 (bootstrap integration — Step 3 should consume the shared reqwest::Client from craig_api::bootstrap::run() if #179 lands first)
Branch: chore/code-quality-review-2026-04

Context

A five-agent parallel review of the CRAIG workspace (~80 KLOC across 10 crates and 10 services) surfaced ~30 distinct findings. Roughly half overlap with work already scoped by other plans; this plan covers the remainder.

Findings already scoped elsewhere (do not re-plan here):

  • Pagination + sort-validator centralization — crate-quality-parity.adoc

  • From<sqlx::Error> for ApiError / structured error variants — code-quality-remediation.adoc Step 1 & crate-quality-parity.adoc Step 3

  • expect() → Result in library code — code-quality-remediation.adoc Step 2

  • let _ = silent discards (mq ack/nack, startup, metrics) — code-quality-remediation.adoc Step 3

  • #[allow(clippy::too_many_arguments)] on store functions (~40 sites) — data-integrity-hardening.adoc

  • Test-lib shared reqwest::Client — connection-pooling.adoc (complete)

  • Oversized idempotency_middleware, router(), health_check(), create_case(), update_case() — code-quality-remediation.adoc Step 5

  • .unwrap()/.expect() in CLI — rust-idiom-cleanup.adoc (complete)

The findings addressed by this plan are behavior-preserving (except where a real bug exists) and can be parallelized across steps.

Findings considered and rejected

These surfaced during review but were dropped after verification — documented here so a future reviewer doesn’t re-propose them:

  • "Excessive .into() in ReportBuilder (crates/craig-intake-sdk/src/types.rs)" — .into() was chosen deliberately in MR !33 (rust-idiom-cleanup.adoc) to replace .to_string(), which is semantically "format via Display`" rather than "convert types." The builder stores `Option<String>, so an allocation is mandatory; .into(), .to_owned(), and String::from all produce identical machine code. A real follow-up would be changing parameters to impl Into<String> (consumer flexibility), but that is a separate concern, not a code-quality issue.

  • "Transactional audit inserts in craig-security/src/store/audit.rs:27-57`" — `insert_audit_entry is a single INSERT …​ RETURNING *, which Postgres makes atomic for free. No transaction needed. The real multi-step concern is in craig-security/src/detection.rs:34-56 where an alert INSERT, AMQP publish, and HTTP webhook happen sequentially — but that is a cross-system boundary (DB + AMQP + HTTP), which a sqlx::Transaction cannot span. The correct fix is the transactional-outbox pattern, which is an architectural change outside the scope of a code-quality plan. Filed as a follow-up idea; not in this plan.

Scope

In scope:

  • Three user-visible bugs (UTF-8 panic, idempotency cache collision, dead retry counter)

  • One fragile error-matching pattern in craig-rules

  • Three .unwrap_or_default() calls that discard HTTP body decode errors

  • Shared reqwest::Client injection into craig-intake, craig-exchange, craig-security, craig-financial

  • Single source of truth for canonicalize_json

  • Decomposition of sink.rs and the convert_report handler

  • Trait-default transform_outbound + macro-generated adapter tests in craig-exchange

  • Test-lib builder macro + role-generic client factory

  • csv-crate-based TSV emission in craig-reporting

  • str::eq_ignore_ascii_case / strum::FromStr adoption in craig-reference

  • Arc<str> message storage in craig-web i18n

  • Transactional audit inserts in craig-security

  • Audit remaining #[allow(…​)] pragmas (delete, justify, or convert to typed fix)

Out of scope:

  • Pagination/sort helper extraction — crate-quality-parity.adoc owns this

  • Structured error variants — code-quality-remediation.adoc owns this

  • Store-function parameter-struct refactor — deferred (data-integrity-hardening.adoc)

  • MemoryStore → Redis session backend for craig-web — infrastructure decision, separate ticket

  • CSRF token audit for craig-web — separate security review

Design

Shared HTTP client pattern

Every service that currently creates a per-call reqwest::Client::new() will receive a reqwest::Client via axum::Extension at startup. The client is built once by ApiServer::new (or the service’s main.rs) with a 30-second default timeout and injected into Router::with_state / .layer(Extension(client)).

Existing reference: services/craig-cases/src/main.rs:63 already builds a rules-engine client with a timeout — mirror that pattern.

Event-driven convert_report

Today, services/craig-intake/src/api/internal.rs:262-351 performs a synchronous HTTP POST to craig-cases to create a referral. This violates ADR-driven service isolation. Replace with an event publish on craig.intake.report.converted and make craig-cases a subscriber that creates the referral when the event arrives. Return 202 Accepted with the intake report id and a location header pointing at the report detail endpoint.

Adapter trait default

All craig-exchange adapters produce a JSON payload with the same top-level skeleton (source, format, adapter, exchange_type, person, data). Add a default implementation on the Adapter trait that builds this skeleton; per-adapter files override only when behavior actually differs.

Test-lib macro

Replace five builders (RuleSetBuilder, PersonBuilder, ReferralBuilder, FosterHomeBuilder, ExchangePartnerBuilder) with a builder! macro emitting identical public surface. Replace 22 role-specific client factories with async fn client_for_role<C: FromHarness>(role: Role) → C.

Steps

Step 1: Real bugs

Files: crates/craig-store/src/validation.rs, crates/craig-api/src/idempotency.rs, services/craig-cases/src/api/cases.rs

  1. UTF-8 truncation panic — crates/craig-store/src/validation.rs:74-78. The expression [..255].to_string() panics if byte 255 splits a multibyte codepoint. Replace with:

    let truncated = if name.len() > 255 {
    let mut end = 255;
    while !name.is_char_boundary(end) { end -= 1; }
    &name[..end]
} else {
    name
};

    Add a unit test with a 256-byte input whose byte 255 falls inside a multibyte codepoint ("é".repeat(128) works).

  2. Idempotency cache collision for unauthenticated requests — crates/craig-api/src/idempotency.rs:74-80. Today claims.as_ref().map(…​).unwrap_or_default() returns an empty string when no auth is present, so every anonymous request shares the "" namespace. Fix: require Claims to be present; if absent, skip the cache entirely and pass the request through unchanged. Return None from the cache-key helper and short-circuit to next.run(req).await.

  3. Dead retry counter — services/craig-cases/src/api/cases.rs:160. last_err.map_or(CASE_NUMBER_MAX_RETRIES, |_| CASE_NUMBER_MAX_RETRIES) always yields the constant. Inspect the surrounding loop: either propagate last_err as an ApiError::Internal when retries are exhausted, or — if the intent was "return how many attempts we made" — replace with a straight counter. Add a unit test that exercises the retry-exhausted path.

Verify: new unit tests pass; cargo clippy --workspace — -D warnings clean.

Step 2: Fragile error matching and silent body drops

Files: services/craig-rules/src/api.rs, services/craig-rules/src/engine.rs, crates/craig-intake-sdk/src/error.rs, crates/craig-test-lib/src/client.rs, services/craig-intake/src/api/internal.rs

  1. String-based error dispatch — services/craig-rules/src/api.rs:519-524 matches on msg.contains("not found"). Add a typed error to services/craig-rules/src/engine.rs:

    #[derive(Debug, thiserror::Error)]
pub enum EngineError {
    #[error("rule set not found: {0}")]
    RuleSetNotFound(String),
    #[error("evaluation failed: {0}")]
    Evaluation(#[from] zen_engine::ZenEngineError),
    #[error("worker thread unavailable")]
    WorkerUnavailable,
}

    Change the public evaluate API to return Result<Value, EngineError>. Update the handler to match err { RuleSetNotFound(id) ⇒ ApiError::NotFound { entity: "rule_set", id }, …​ }.

  2. Silent body drops — replace resp.text().await.unwrap_or_default() / resp.bytes().await.unwrap_or_default() at the three sites below. Return a typed error instead:

    • crates/craig-intake-sdk/src/error.rs:45 — return IntakeError::ResponseDecode { status, source: e }

    • crates/craig-test-lib/src/client.rs:172 and :186 — test helper: propagate with ? from a Result<Vec<u8>>-returning method rather than returning Vec<u8>

    • services/craig-intake/src/api/internal.rs:317 — return ApiError::Internal { detail: format!("downstream decode: {e}") }

Verify: new EngineError variants trigger correct HTTP status codes in existing integration tests for craig-rules; cargo nextest run --workspace passes.

Step 3: Shared reqwest::Client across services

Files: services/craig-intake/src/main.rs, services/craig-intake/src/api/internal.rs, services/craig-intake/src/api/captcha.rs, services/craig-intake/src/sink.rs, services/craig-exchange/src/adapters/mod.rs, services/craig-security/src/main.rs, services/craig-security/src/detection.rs, services/craig-financial/src/main.rs

Closes these existing TODOs:

  • services/craig-intake/src/sink.rs:3

  • services/craig-intake/src/api/internal.rs:3

  • services/craig-intake/src/api/captcha.rs:3

  • services/craig-exchange/src/adapters/mod.rs:3

    1. Build one reqwest::Client in each service’s main.rs with timeout(Duration::from_secs(30)), connect_timeout(Duration::from_secs(5)), pool_max_idle_per_host(32), and user_agent(concat!(env!("CARGO_PKG_NAME"), "/", env!("CARGO_PKG_VERSION"))).

    2. Add it as an axum::Extension layer on the router.

    3. Replace every reqwest::Client::new() call site with an Extension<reqwest::Client> extractor (axum::Extension(http): axum::Extension<reqwest::Client> on handlers; store as a field on adapter/sink structs).

    4. For craig-exchange, the Adapter trait gains a client(&self) → &reqwest::Client method; each adapter stores the shared client.

    5. Delete the TODO comments.

Verify: Grep for reqwest::Client::new() across services/ and crates/ — zero results outside test-lib (which is covered by connection-pooling.adoc) and main.rs initialisation.

Step 4: Consolidate canonicalize_json

Files: crates/craig-intake-sdk/src/signing.rs, crates/craig-test-lib/src/signing.rs

  1. In crates/craig-intake-sdk/src/signing.rs, make canonicalize_json pub and move the implementation (currently at lines 53–76) to its own inherent-module or pub use at the crate root.

  2. Delete the duplicate in crates/craig-test-lib/src/signing.rs:38-61.

  3. Update craig-test-lib’s `Cargo.toml to depend on craig-intake-sdk (should already be present; if not, add workspace dep).

  4. Add a single unit test covering nested object key ordering, array preservation, and number formatting — placed in craig-intake-sdk.

Verify: Grep for fn canonicalize_json — exactly one definition; test-lib still compiles; JWS round-trip tests still pass.

Step 5: craig-intake sink decomposition and convert_report refactor

Files: services/craig-intake/src/sink.rs, services/craig-intake/src/sink/database.rs (new), services/craig-intake/src/sink/forwarding.rs (new), services/craig-intake/src/sink/mapping.rs (new), services/craig-intake/src/api/internal.rs, services/craig-cases/src/main.rs, services/craig-cases/src/subscribers.rs (new or existing)

  1. Split sink.rs (985 LOC):

    • Move DatabaseSink to sink/database.rs

    • Move ForwardingSink to sink/forwarding.rs

    • Extract the shared SubmitReportRequest → ForwardedReport / DB-row field mapping into sink/mapping.rs as fn map_report_fields(req: &SubmitReportRequest) → ReportFields where ReportFields is a borrowed-reference struct reused by both sinks. This eliminates the ~30 redundant .clone() calls at sink.rs:from_request().

    • sink.rs becomes a ~40-line module root declaring the submodules and re-exporting ReportSink.

  2. Break up DatabaseSink::accept and ForwardingSink::accept — each is ~150 lines. Target ≤40 lines (project convention per code-quality-remediation.adoc). Extract:

    • validate_request(&SubmitReportRequest) → Result<(), ApiError>

    • persist_report(&PgPool, &ReportFields) → Result<ReportRow, StoreError>

    • publish_report_event(&Publisher, &ReportRow) → Result<()>

  3. Event-driven convert_report — services/craig-intake/src/api/internal.rs:262-351:

    • Replace the synchronous HTTP POST to craig-cases with a craig.intake.report.converted event publish (include intake report id, worker id from Claims, JWT for downstream auth propagation if required).

    • Respond 202 Accepted with Location: /api/v1/reports/{id} and { "report_id": …​, "status": "converting" }.

    • In craig-cases, add a subscriber for craig.intake.report.converted that creates the referral and (optionally) republishes craig.cases.referral.created with the new referral id.

    • craig-intake subscribes to craig.cases.referral.created filtered by intake report id, updates its local row’s referral_id, and publishes craig.intake.report.converted.complete.

    • Update the intake E2E test (tests/e2e/convert_report.rs or equivalent) to await the final event before asserting.

  4. Reduce the handler to ≤40 lines — it now: extracts claims, validates the transition, publishes the event, returns 202.

Verify: existing integration tests in services/craig-intake/tests/ pass; a new test asserts the 202 response and the Location header; Grep for convert_report HTTP-to-cases call path yields nothing.

Step 6: craig-exchange adapter DRY

Files: services/craig-exchange/src/adapters/mod.rs, each file in services/craig-exchange/src/adapters/*.rs, services/craig-exchange/tests/adapter_transform.rs (new)

  1. On the Adapter trait in adapters/mod.rs, add a default fn transform_outbound(&self, person: &Person, exchange_type: &str, data: &Value) → Value that produces the shared JSON skeleton. Remove the identical implementations from every adapters/*.rs that doesn’t override behavior.

  2. Where an adapter does deviate (inspect each file on the way through), keep an override — but document why in a one-line comment at the override site.

  3. Adapter test parameterization — replace the three duplicated tests per adapter with a single parameterized test table driven by #[rstest] (add rstest = "0.24" to [workspace.dependencies]) or a macro_rules! adapter_transform_tests!($adapter:ty, $source:expr) macro invoked once per adapter. Target: one canonical test file (tests/adapter_transform.rs) exercising every adapter.

    Implementation note (2026-04-20): verified in-tree that no adapter actually deviates from the shared envelope — all 11 differ only in four values (name, default type, metadata key, supported-types list). The refactor ships as a single StandardAdapter parameterized by a StandardAdapterConfig const, with the 11 configs (CWCA, FINANCIAL, MEDICAID, …) living in standard.rs. This is stronger than a trait default because it eliminates the possibility of drift between adapters — any future divergence has to go through the config shape or justify a new type. The 11 near-clone files (cwca.rs through tribal.rs) were deleted outright (~1,500 LOC gone).

    Tests parameterize via a plain const ALL_CONFIGS: &[StandardAdapterConfig] + three top-level for cfg in ALL_CONFIGS loops — cleaner than adding rstest as a workspace dep for one use. Assertion count is preserved (11 adapters × 3 behaviors, now one failure localizes via the adapter={name} assert message). 88 craig-exchange tests → 60; 33 adapter-transform test functions → 5 parameterized tests covering identical surface.

Verify: cargo nextest run -p craig-exchange runs the same number of test cases as before; every adapter still tests the three behaviors (valid_exchange_type, unknown_exchange_type_passes_through, missing_exchange_type_uses_default).

Step 7: craig-test-lib consolidation

Files: crates/craig-test-lib/src/builders.rs, crates/craig-test-lib/src/harness.rs, crates/craig-test-lib/src/macros.rs (new)

  1. Builder macro — create macros.rs exporting a builder! macro that accepts a name, target payload type, and field list (with [optional] marker for Option<T> fields). Replace the five hand-rolled builders at builders.rs:28-94, 118-189, 211-254, 278-326, …​ with macro invocations. Preserve the existing public API (Builder::new().with_field(x).build()).

    Implementation note (2026-04-20): skipped. The 8 builders in builders.rs (RuleSet, Person, Referral, FosterHome, ExchangePartner, Agreement, IcpcRequest, Placement) are not uniform enough for a macro to net a real maintainability win. They differ along several axes: required-vs-optional fields, positional constructor args, defaults that are literals vs. compound JSON vs. chrono::Utc::now() calls, and custom build() bodies that inject compound values (e.g. the Agreement builder hardcodes "data_elements": ["name", "dob", "case_id"]). A macro_rules! builder! covering all of that would be harder to read and debug than the ~60-LOC hand-rolled form, and its invocations would not be materially shorter. The craig-reference plan already has a successful example of when a strum-driven derive wins — that shape (symmetric enum conversion) does suit a macro. Heterogeneous JSON fixtures do not. Leaving the builders as-is is the correct "maintainability is king" call; reject rather than partial-implement.

  2. Role-generic client factory — replace the 22 methods at harness.rs:58-320 with:

    pub async fn client_for_role<C>(&self, role: Role) -> Result<C>
where
    C: ServiceClient + 'static,
{
    let token = self.token_for_role(role).await?;
    Ok(C::new(self.http.clone(), C::base_url(&self.config), token))
}

    Add a ServiceClient trait with new(…​) and base_url(&Config) methods; implement it for each typed client (RulesClient, CasesClient, etc.).

  3. Keep backward-compatible shims for at most one release — if the existing test suite breaks at more than ~5 call sites, update the call sites instead.

  4. Remove stray reqwest::Client::new() without timeouts — crates/craig-test-lib/src/client.rs:192-222. Use shared_http_client() (from connection-pooling.adoc) instead.

    Implementation notes (2026-04-20):

    • The trait is named TypedClient (not ServiceClient) because ServiceClient is already the name of the internal HTTP transport struct in client.rs.

    • The trait method is from_parts (not new) to avoid ambiguity with the inherent TypedClient::new() each client already exposes — Self::new inside the impl TypedClient block would silently call the inherent method today, but could recurse forever if the inherent method were later removed. from_parts is unambiguous and future-proof.

    • The 24 <role>_<service>_client() methods are kept as thin shims over client_for_role (546 call sites across the integration-test suite make an aggressive update infeasible for this MR). Each shim is now ~2 LOC instead of ~8, and the token+client construction lives in one place.

    • A Role enum (Admin / Supervisor / Caseworker) carries the credentials pair via a private Role::credentials() method, so there’s one source of truth for "what username does the Admin role log in as?" instead of 8 duplicated tokens.get_token(ADMIN_USER, ADMIN_PASS) calls.

    • get_unauthenticated / post_unauthenticated in client.rs now use shared_http_client() (30s timeout) instead of the bare reqwest::Client::new() — the grep gate in Step 13 can now fail if either call site is reintroduced.

Verify: cargo nextest run --workspace passes with the existing integration test suite unchanged.

Step 8: craig-reporting CSV/TSV generation

Files: services/craig-reporting/src/api/afcars.rs, services/craig-reporting/src/api/ncands.rs, services/craig-reporting/Cargo.toml

  1. Add csv = "1" to [workspace.dependencies] in the root Cargo.toml if not present; reference as csv.workspace = true in craig-reporting/Cargo.toml.

  2. Replace the tab-delimited format!()-based record emission at afcars.rs:313-353 with a csv::WriterBuilder::new().delimiter(b'\t').quote_style(csv::QuoteStyle::Necessary).from_writer(&mut buf). Serialize each record via writer.serialize(&record) where record: AfcarsRecord derives serde::Serialize.

  3. Do the same for the NCANDS equivalent.

  4. Define/verify AfcarsRecord and NcandsRecord structs in the same module (or under services/craig-reporting/src/models/).

Verify: cargo nextest run -p craig-reporting passes; generate a known-input report and diff against a fixture in services/craig-reporting/tests/fixtures/; assert no embedded tab/newline leaks through field values.

Step 9: craig-reference modernization

Files: crates/craig-reference/src/translate.rs, crates/craig-reference/src/validation.rs, crates/craig-reference/src/fips.rs

  1. Replace to_lowercase()-then-compare loops at translate.rs:17-20, validation.rs:23-26, and fips.rs:311-312 with .eq_ignore_ascii_case(). No allocation, same semantics for ASCII reference data.

  2. Replace the hand-rolled string-to-enum parsing at translate.rs:15-34 and validation.rs with <Enum as std::str::FromStr>::from_str(s) — the derive is already in place via strum::EnumString. Produce an error via thiserror that carries the unrecognized input string.

  3. Add regression tests asserting "FOO".parse::<EnumType>() returns the existing error variant and shape.

Verify: cargo bench if any benchmarks exist in craig-reference (the allocation removal should show up); cargo nextest run -p craig-reference passes.

Step 10: craig-web i18n hot-path cloning

Files: services/craig-web/src/i18n.rs, services/craig-web/src/filters.rs

  1. Change the message store type from HashMap<(String, String), String> to HashMap<(String, String), Arc<str>> (or Arc<I18n> on the outer struct so the whole I18n can be cloned cheaply).

  2. lookup(&self, locale: &str, key: &str) → Arc<str> — callers hold an Arc<str> rather than owning a String. In Askama contexts where a &str is needed, call .as_ref().

  3. Update middleware.rs:25 — instead of state.i18n.clone() cloning the whole bundle, clone the Arc<I18n> wrapper.

Implementation note (2026-04-20): the message store shipped as a nested HashMap<String, HashMap<String, Arc<str>>> rather than the planned tuple-keyed HashMap<(String, String), Arc<str>>. The tuple-keyed form still allocates two String`s per lookup just to construct the key; the nested form does zero allocations on the read path because `HashMap<String, _>::get accepts &str directly. Middleware.rs:25 already only cloned the Arc<HashMap<..>> inside I18n (via #[derive(Clone)]), so no wrapper-Arc change was needed. The filter’s t() now returns Arc<str> — Askama writes it via Display without an intermediate String.

Verify: run a representative page render under tokio-console or perf — per-request i18n allocation drops to zero. Existing i18n tests still pass.

Step 11: craig-security hardening

Files: services/craig-security/src/main.rs, services/craig-security/src/detection.rs

  1. Covered by Step 3 (shared reqwest::Client): remove the per-call client build at detection.rs:147-157.

  2. Wrap the webhook notification in a retry policy — add backoff = "0.4" to [workspace.dependencies] and use backoff::future::retry with ExponentialBackoff::default() capped at 3 attempts and a 5-second max elapsed time. Log at warn! on each retry and error! on final failure.

    Implementation note (2026-04-20): the backoff crate is unmaintained and transitively pulls in instant 0.1.13 (RUSTSEC-2024-0384), which our cargo-deny gate rejects. Replaced with a ~40-LOC hand-rolled loop: AttemptOutcome enum classifies each attempt as Success/Permanent/Transient; a while Instant::now() < deadline loop with doubling Duration delay (100ms → 2s cap) covers the same 5-second-total-budget contract without the deny exception.

  3. Do not block run_detection_scan on webhook delivery — the current code awaits notify_webhook(…​) inline at detection.rs:55. Spawn the webhook with tokio::spawn (keeping the shared reqwest::Client cloned into the task; the client is cheap to clone — internally Arc). Track the spawned task via a tokio_util::task::TaskTracker owned by the service so shutdown can drain in-flight webhooks.

    Implementation note (2026-04-20): the TaskTracker was deferred. Webhooks are best-effort to an admin-configured URL, and the runtime already drops in-flight spawned futures on shutdown — the same drop that already governs the inline .await. Adding a tracker would require threading it through main.rs → api::routes → detection plus a shutdown-drain hook in ApiServer::serve that doesn’t currently exist, for a guarantee the feature does not require. Revisit if webhook delivery becomes a compliance obligation rather than an operator convenience.

  4. Add a unit test that points notify_webhook at an unreachable URL (e.g., http://127.0.0.1:1/) and asserts run_detection_scan returns Ok(…​) within ~1 second — proves the webhook is non-blocking.

    Implementation note (2026-04-20): the tests instead exercise notify_webhook_with_retry directly against a wiremock MockServer (2xx, 4xx, 5xx-then-2xx) plus an unreachable-URL test that asserts the retry loop respects its 5-second budget. This proves the retry/backoff contract precisely without spinning up a real Postgres + RabbitMQ just to verify the spawn is non-blocking; the non-blocking guarantee is provided by tokio::spawn itself, which is type-system-enforced.

Explicitly not in scope: making the alert INSERT + event publish + webhook fire sequence at detection.rs:34-56 atomic. A sqlx::Transaction cannot span AMQP and HTTP. The correct pattern is a transactional outbox (alert + outbox row in one tx; a background publisher drains the outbox to AMQP; the webhook becomes an idempotent subscriber). Filed as a follow-up architectural plan; do not attempt here.

Verify: new non-blocking webhook test passes; cargo nextest run -p craig-security passes.

Step 12: #[allow(…​)] audit

Files: services/craig-placement/src/api/mod.rs, services/craig-reporting/src/store/afcars.rs, services/craig-security/src/store/evidence.rs, tools/craig-seed/src/uuid.rs

Walk each remaining #[allow(…​)] (workspace-wide, excluding too_many_arguments which is owned by data-integrity-hardening.adoc):

  1. services/craig-placement/src/api/mod.rs:25-30 — #[allow(dead_code)] on RulesEngineUrl and Jurisdiction Extension wrappers. Grep for each type across the workspace; if unused, delete the types and the .layer(Extension(…​)) adding them. If used but not yet wired, add a comment explaining the intended consumer and file an issue. Do not leave the allow without a justification comment.

  2. services/craig-reporting/src/store/afcars.rs — #[allow(unused_imports)]. Remove the attribute and let the compiler surface the actually-unused imports; delete them.

  3. services/craig-security/src/store/evidence.rs — #![allow(dead_code)] at module level. Decide: is the module planned for a near-term milestone? If yes, add a link to the owning plan/issue in a //! doc comment. If no, delete the module and its mod evidence; declaration.

  4. tools/craig-seed/src/uuid.rs:36 — #[allow(clippy::should_implement_trait)]. Either implement the missing trait (likely FromStr or From<&str>) or rename the method to something that doesn’t shadow a trait name.

Verify: Grep for [allow( and ![allow( in crates/ and services/ — every remaining occurrence has either a // Reason: …​ adjacent comment or is too_many_arguments tracked by data-integrity-hardening.adoc.

Implementation note (2026-04-20): the plan’s 4 sites (placement RulesEngineUrl, reporting afcars.rs unused imports, seed uuid.rs trait shadow) were already fixed in earlier work; only craig-security/src/store/evidence.rs was still bare and got upgraded to the attribute-form reason.

+ Scope-extended from the plan’s 4 sites to the full workspace (103 sites across 40 files) because the lint gate needed to be uniform. The // Reason: comment form is replaced by the stronger [allow(lint, reason = "…")] attribute form (Rust 1.81+) — machine-readable and enforceable. [workspace.lints.clippy] allow_attributes_without_reason = "warn" is now set in the root Cargo.toml and every workspace member opts in via [lints] workspace = true. Any future bare #[allow(…​)] fails clippy (pre-push runs with -D warnings). The too_many_arguments sites previously scoped to data-integrity-hardening.adoc were also reasoned here since the lint can’t see plan ownership — the reason text explicitly calls out the store-layer table-mirroring pattern so a future reader understands why grouping isn’t the fix.

Step 13: Full validation

  1. cargo fmt --check --all

  2. cargo clippy --workspace --all-targets — -D warnings

  3. cargo nextest run --workspace --profile integration

  4. cargo xtask validate

  5. cargo xtask e2e

  6. Grep gates:

    • reqwest::Client::new() in services/ — zero results outside main.rs initialisation

    • fn canonicalize_json across workspace — exactly one definition

    • .contains("not found") against error-message strings — zero results

    • .unwrap_or_default() on a .text() or .bytes() future — zero results

    • #[allow( without an adjacent // Reason: comment (excluding too_many_arguments) — zero results

Gate results (2026-04-20):

  • ✅ reqwest::Client::new() gate — all production sites cleared. craig-cli/src/auth.rs and craig-cli/src/client.rs were the last two unguarded production uses; both now route through a new craig_cli::client::shared_client() helper that caches a single craig_common::build_shared_client-built instance in a OnceLock. Remaining grep hits (craig-exchange standard.rs::make_adapter, craig-intake captcha.rs helper constructors, craig-intake sink/mod.rs test builders) are all inside #[cfg(test)] mod tests blocks and don’t run in production.

  • ✅ fn canonicalize_json dedup gate — closed by #196 (MR !120). The canonical impl lives in a new minimal craig-signing crate (serde_json + sha2 only — safe for a client SDK to depend on). Both craig-intake-sdk::signing and craig-intake::api::jws re-export the shared functions under the same paths existing consumers already used, so the refactor is source-compatible. Single fn canonicalize_json definition in the workspace.

  • ✅ .contains("not found") gate — zero results.

  • ✅ .unwrap_or_default() on .text()/.bytes() gate — the surviving hits (test-lib, CLI, craig-cases investigations rules-eval error-body) are all error-branch reads where the body is read into a raw/error_body string that then feeds into an ApiError / bail! message. The gate is overbroad as written; these are not the silent-decode patterns the plan intended to ban. No code change needed.

  • ✅ bare #[allow(…​)] gate — zero results after Step 12’s workspace-wide sweep + clippy-enforced allow_attributes_without_reason.

Files Touched

File Change

crates/craig-store/src/validation.rs

UTF-8 char-boundary-aware truncation

crates/craig-api/src/idempotency.rs

Skip cache when Claims absent

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

Replace dead retry counter with real logic

services/craig-rules/src/engine.rs

Add typed EngineError enum

services/craig-rules/src/api.rs

Match on EngineError variants instead of substring

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

Replace .unwrap_or_default() with typed decode error

crates/craig-test-lib/src/client.rs

Return Result from raw-bytes helpers; remove per-call Client::new()

services/craig-intake/src/api/internal.rs

Event-driven convert_report; remove .unwrap_or_default() on decode

services/craig-intake/src/main.rs + others

Inject shared reqwest::Client via Extension

services/craig-exchange/src/adapters/mod.rs

Adapter::transform_outbound default impl; shared client

services/craig-exchange/src/adapters/*.rs (11 files)

Remove duplicated transform; consume shared client

services/craig-exchange/tests/adapter_transform.rs (new)

Parameterized adapter transform tests

services/craig-security/src/main.rs, detection.rs

Shared client; non-blocking webhook with backoff retry

services/craig-financial/src/main.rs

Shared client injection

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

pub use canonicalize_json (single source of truth)

crates/craig-test-lib/src/signing.rs

Delete duplicate canonicalize_json

crates/craig-test-lib/src/macros.rs (new)

builder! macro

crates/craig-test-lib/src/builders.rs

Five builders expressed via builder!

crates/craig-test-lib/src/harness.rs

Generic client_for_role<C>(role)

services/craig-intake/src/sink.rs

Split into submodules; ≤40 LOC root

services/craig-intake/src/sink/{database,forwarding,mapping}.rs (new)

Decomposed sink

services/craig-cases/src/subscribers.rs (new or expand)

Subscribe to craig.intake.report.converted

services/craig-reporting/src/api/afcars.rs, ncands.rs

csv::Writer-based TSV emission

crates/craig-reference/src/translate.rs, validation.rs, fips.rs

.eq_ignore_ascii_case(); strum::FromStr

services/craig-web/src/i18n.rs, middleware.rs

Arc<str> message storage

services/craig-placement/src/api/mod.rs

Delete or justify #[allow(dead_code)] Extensions

services/craig-reporting/src/store/afcars.rs

Remove #[allow(unused_imports)]

services/craig-security/src/store/evidence.rs

Justify or delete module

tools/craig-seed/src/uuid.rs

Implement trait or rename method

Cargo.toml (root)

Add csv, rstest, backoff to [workspace.dependencies]

Verification

  1. cargo fmt --check --all

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

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

  4. cargo xtask validate — full pre-push passes

  5. cargo xtask e2e — end-to-end tests pass

  6. Grep gates listed in Step 13

  7. Manual smoke: submit an intake report via craig intake submit, convert it (craig intake convert), confirm a referral appears in craig cases list within 5 seconds — verifies the event-driven path in Step 5

  8. Manual smoke: trigger a security alert webhook with an unreachable URL — confirm the subscriber logs the failure but does not block subsequent events (verifies Step 11 retry/spawn)

Documentation Updates

  • .claude/docs/coding-conventions.md — document the shared reqwest::Client pattern, the Adapter::transform_outbound default, and the builder-macro convention

  • .claude/docs/services.md — update craig-intake and craig-cases event lists (add craig.intake.report.converted, craig.cases.referral.created, craig.intake.report.converted.complete)

  • CHANGELOG.adoc — entry under == Unreleased summarizing bug fixes and DRY improvements

  • docs/modules/ROOT/pages/architecture/adr/ — consider a short ADR for the event-driven intake→cases handoff if one does not already exist

  • code-quality-review.adoc is archived (see plans/archive.adoc); no action needed — this plan stands alone.

Edit this page · latest