Data Integrity & Code Hardening Plan

On this page

Status

COMPLETE (core phases) — Phases 1 (soft-delete migration, MR !24), 2 (transaction wrapping, MR !25), and 4 (forbid unsafe_code) are merged to main. Phase 3 (HTTP client pooling) is DEFERRED — low priority, tracked separately in the code-quality-security-hardening plan (#38).

JWS integrity verification (Phase 2e) adds a complementary integrity layer — cryptographic proof that partner-submitted reports were not altered in transit. See ADR-010.

Context

A principal-level code review identified six categories of issues in the CRAIG codebase, ordered by severity. The shared crates and overall architecture are excellent — these issues concentrate in service-layer patterns that, left unaddressed, create compliance risk (hard deletes in a child welfare system) and data integrity gaps (multi-write handlers without transactions).

Scope: 6 issues across 4 phases, producing 4 GitLab issues and 4 MRs.

Phase 1: Soft-Delete Migration (Compliance — HIGH)

Problem

11 entities use DELETE FROM (hard delete) while only 3 use soft-delete (SET active = false). For a CCWIS subject to 2 CFR § 200.334 record retention requirements, hard-deleting case contacts, court orders, and financial records is a compliance liability. Deleted records have no audit trail and cannot be recovered.

Hard-Delete Entities to Migrate

Service Store File Function Table

craig-cases

store/contacts.rs

delete_contact()

contacts

craig-cases

store/court_orders.rs

delete_court_order()

court_orders

craig-cases

store/case_plans.rs

delete_task()

case_plan_tasks

craig-cases

store/contact_attachments.rs

delete_attachment()

contact_attachments

craig-placement

store/kinship.rs

delete_kinship_option()

kinship_options

craig-placement

store/foster_homes.rs

delete_training()

foster_home_training

craig-financial

store/rates.rs

delete_rate()

rate_tables

craig-security

store/nist.rs

delete_control()

nist_controls

craig-security

store/reviews.rs

delete_review()

security_reviews

Entities to KEEP as Hard Delete (justified)

Service Store File Function Justification

craig-exchange

store/agreements.rs

delete_agreement()

Only deletes status = 'draft' — drafts are pre-commitment, safe to hard-delete

craig-financial

store/adjustments.rs

delete_adjustment()

Only deletes status = 'pending' — unapproved adjustments are safe to remove

Implementation

For each of the 9 tables above:

1. Migration

Add active BOOLEAN NOT NULL DEFAULT true column (if not present) and add deleted_at TIMESTAMPTZ column for audit:

-- Example: YYYYMMDDHHMMSS_soft_delete_contacts.sql
ALTER TABLE contacts ADD COLUMN IF NOT EXISTS active BOOLEAN NOT NULL DEFAULT true;
ALTER TABLE contacts ADD COLUMN IF NOT EXISTS deleted_at TIMESTAMPTZ;

2. Store function

Change DELETE FROM to UPDATE …​ SET active = false, deleted_at = now():

// Before (contacts.rs:102-107)
pub async fn delete_contact(pool: &PgPool, id: Uuid) -> Result<Option<Contact>> {
    sqlx::query_as::<_, Contact>("DELETE FROM contacts WHERE id = $1 RETURNING *")

// After
pub async fn delete_contact(pool: &PgPool, id: Uuid) -> Result<Option<Contact>> {
    sqlx::query_as::<_, Contact>(
        "UPDATE contacts SET active = false, deleted_at = now() WHERE id = $1 AND active = true RETURNING *"
    )

3. List/count queries

Add AND active = true filter to all SELECT queries for that table. Pattern from admin_units.rs:

SELECT * FROM contacts WHERE case_id = $1 AND active = true ORDER BY ...

4. GET queries

Keep returning inactive records (soft-deleted records visible via direct GET, filtered from LIST — matches existing project convention per CLAUDE.md).

5. Model

Add active: bool and deleted_at: Option<DateTime<Utc>> to the sqlx model struct. Mark with #[serde(skip_serializing_if = "Option::is_none")] on deleted_at.

Files Modified

Migrations (9 new files) in each service’s migrations/ directory:

  • services/craig-cases/migrations/YYYYMMDDHHMMSS_soft_delete_contacts.sql

  • services/craig-cases/migrations/YYYYMMDDHHMMSS_soft_delete_court_orders.sql

  • services/craig-cases/migrations/YYYYMMDDHHMMSS_soft_delete_case_plan_tasks.sql

  • services/craig-cases/migrations/YYYYMMDDHHMMSS_soft_delete_contact_attachments.sql

  • services/craig-placement/migrations/YYYYMMDDHHMMSS_soft_delete_kinship_options.sql

  • services/craig-placement/migrations/YYYYMMDDHHMMSS_soft_delete_foster_home_training.sql

  • services/craig-financial/migrations/YYYYMMDDHHMMSS_soft_delete_rate_tables.sql

  • services/craig-security/migrations/YYYYMMDDHHMMSS_soft_delete_nist_controls.sql

  • services/craig-security/migrations/YYYYMMDDHHMMSS_soft_delete_security_reviews.sql

Store files (9 modified):

  • services/craig-cases/src/store/contacts.rs — change delete, add active = true to list/count

  • services/craig-cases/src/store/court_orders.rs — same

  • services/craig-cases/src/store/case_plans.rs — same (for tasks)

  • services/craig-cases/src/store/contact_attachments.rs — same

  • services/craig-placement/src/store/kinship.rs — same

  • services/craig-placement/src/store/foster_homes.rs — same (for training)

  • services/craig-financial/src/store/rates.rs — same

  • services/craig-security/src/store/nist.rs — same

  • services/craig-security/src/store/reviews.rs — same

Model files: Add active and deleted_at fields to each model struct in store/models.rs for each affected service.

GitLab

  • Issue: feat: Migrate hard-delete entities to soft-delete for compliance

  • Branch: feature/soft-delete-migration

  • Labels: feat, P1-high

  • Weight: 5

Phase 2: Transaction Wrapping for Multi-Step Operations (Data Integrity — MEDIUM)

Problem

Zero uses of sqlx::Transaction exist in the codebase. Several handlers perform multiple sequential writes without atomicity guarantees. The highest-risk case is foster home placement, where a capacity check + occupancy increment + placement creation can leave inconsistent state if any step fails.

Critical Handlers to Wrap

2a. create_placement — services/craig-placement/src/api/placements.rs:73-138

Current flow (3 writes, no transaction):

  1. get_foster_home() — READ (capacity check)

  2. increment_occupancy() — WRITE

  3. create_placement() — WRITE

Race condition: Between step 1 (capacity check) and step 2 (increment), another request can place a child, overallocating the home. If step 2 succeeds but step 3 fails, occupancy is incremented without a placement.

Fix: Wrap steps 1-3 in a transaction. Use SELECT …​ FOR UPDATE on the foster home row to lock it during the capacity check:

let mut tx = app.db.inner().begin().await.map_err(|e| ApiError::Internal(e.to_string()))?;

// Lock foster home row for update
let home = sqlx::query_as::<_, FosterHome>(
    "SELECT * FROM foster_homes WHERE id = $1 FOR UPDATE"
)
.bind(home_id)
.fetch_optional(&mut *tx)
.await?;

// Capacity check against locked row
// increment_occupancy(&mut *tx, home_id)
// create_placement(&mut *tx, ...)

tx.commit().await.map_err(|e| ApiError::Internal(e.to_string()))?;

2b. update_placement (end placement) — services/craig-placement/src/api/placements.rs (~line 238+)

Current flow when ending a placement:

  1. get_placement() — READ

  2. update_placement() — WRITE (status → ended)

  3. decrement_occupancy() — WRITE (conditional)

Fix: Same transaction pattern. Lock placement row with FOR UPDATE, update status and decrement occupancy atomically.

2c. retry_transaction — services/craig-exchange/src/api/transactions.rs (~line 225+)

Current flow:

  1. get_transaction() — READ

  2. update_transaction_status() to "retry" — WRITE

  3. update_transaction_status() to "pending" — WRITE

Fix: Combine into a single UPDATE or wrap in transaction. Simplest fix: single SQL statement UPDATE …​ SET status = 'pending', retried_at = now() WHERE id = $1 AND status = 'failed' RETURNING *.

Implementation Pattern

Add a begin() convenience method to DbPool in crates/craig-db/src/lib.rs:

impl DbPool {
    pub async fn begin(&self) -> Result<sqlx::Transaction<'_, sqlx::Postgres>, sqlx::Error> {
        self.inner().begin().await
    }
}

Store functions that participate in transactions need impl sqlx::Executor as their first parameter instead of &PgPool, so they can accept either a pool or a transaction:

// Before
pub async fn create_placement(pool: &PgPool, ...) -> Result<Placement> {

// After
pub async fn create_placement<'e, E>(executor: E, ...) -> Result<Placement>
where
    E: sqlx::Executor<'e, Database = sqlx::Postgres>,
{
Only change the signature for functions that participate in transactions (the 6 functions in the 3 handlers above). Don’t refactor all 40+ store functions — that’s scope creep.

Files Modified

  • crates/craig-db/src/lib.rs — add begin() method

  • services/craig-placement/src/store/placements.rs — generalize create_placement(), update_placement() executor param

  • services/craig-placement/src/store/foster_homes.rs — generalize get_foster_home(), increment_occupancy(), decrement_occupancy() executor param

  • services/craig-placement/src/api/placements.rs — wrap create_placement and update_placement handlers in transactions

  • services/craig-exchange/src/store/transactions.rs — combine retry into single UPDATE

  • services/craig-exchange/src/api/transactions.rs — simplify retry_transaction handler

GitLab

  • Issue: feat: Add transaction wrapping for multi-step database operations

  • Branch: feature/transaction-wrapping

  • Labels: feat, P1-high

  • Weight: 5

Phase 3: HTTP Client Pooling in Production Code (Code Quality — LOW)

Problem

CLAUDE.md mandates "shared reqwest::Client, NOT Client::new() per request." Several production code paths still create per-request clients.

Instances to Fix

File Line Context Fix

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

288

Cross-service referral POST

Inject shared client from main.rs via Extension

services/craig-web/src/routes/report.rs

201

Public report submission

Use state.api (existing ApiClient) instead of new client

services/craig-web/src/routes/report.rs

286

Report status lookup

Use state.api instead of new client

Instances to LEAVE (justified)

File Line Context Justification

services/craig-cli/src/client.rs

16

CLI ApiClient

CLI is short-lived process, one client per invocation is correct

services/craig-cli/src/auth.rs

40

Token fetch

Same — CLI lifecycle

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

177,193

get/post_unauthenticated()

Intentionally separate for negative auth tests

crates/craig-test-lib/src/clients/intake.rs

57,84

PublicReportClient

Test-only, no auth needed

Implementation

craig-intake internal.rs: The shared client already exists in main.rs (line 75-78, built with 30s timeout). Pass it as Extension<reqwest::Client> to the internal routes and extract it in the handler instead of creating a new one.

craig-web report.rs: The AppState already contains an ApiClient with a shared reqwest::Client. The report handlers currently bypass it and create raw clients. Refactor to call state.api.post(…​) instead.

Files Modified

  • services/craig-intake/src/main.rs — add shared client as Extension to internal routes

  • services/craig-intake/src/api/internal.rs — extract Extension<reqwest::Client> instead of Client::new()

  • services/craig-web/src/routes/report.rs — use state.api instead of reqwest::Client::new()

GitLab

  • Issue: fix: Use shared reqwest::Client in intake and web services

  • Branch: feature/shared-http-clients

  • Labels: fix, P2-medium

  • Weight: 2

Phase 4: Code Hygiene — #![forbid(unsafe_code)] (Hardening — VERY LOW)

Problem

Zero crates in the workspace declare #![forbid(unsafe_code)], despite zero unsafe blocks existing. Adding the attribute codifies this as a compile-time guarantee and prevents future regressions.

Implementation

Add #![forbid(unsafe_code)] to the top of lib.rs (for library crates) or main.rs (for binary crates) in all 20 workspace members:

Shared crates (lib.rs):

  • crates/craig-common/src/lib.rs

  • crates/craig-auth/src/lib.rs

  • crates/craig-db/src/lib.rs

  • crates/craig-mq/src/lib.rs

  • crates/craig-api/src/lib.rs

  • crates/craig-store/src/lib.rs

  • crates/craig-reference/src/lib.rs

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

  • crates/craig-intake-sdk/src/lib.rs

Services (main.rs):

  • services/craig-rules/src/main.rs

  • services/craig-cases/src/main.rs

  • services/craig-placement/src/main.rs

  • services/craig-exchange/src/main.rs

  • services/craig-financial/src/main.rs

  • services/craig-reporting/src/main.rs

  • services/craig-security/src/main.rs

  • services/craig-intake/src/main.rs

  • services/craig-cli/src/main.rs (or lib.rs)

  • services/craig-web/src/main.rs

Tool:

  • tools/craig-seed/src/main.rs

GitLab

  • Issue: chore: Add #![forbid(unsafe_code)] to all workspace crates

  • Branch: feature/forbid-unsafe

  • Labels: chore, P3-low

  • Weight: 1

Deferred (Not in This Plan)

Store Parameter Structs

40 store functions use #[allow(clippy::too_many_arguments)]. Converting these to parameter structs is a large mechanical refactor (40 functions + all call sites) with low correctness risk. The current pattern works — it’s a readability concern. Deferring to avoid scope creep. Track as a separate refactor: issue.

Event Outbox Pattern

Event publishing is fire-and-forget (errors logged, not retried). An outbox pattern would guarantee delivery but requires new infrastructure (outbox table, polling loop or CDC). This is a significant architectural change best addressed as its own plan when the project moves to multi-node deployment. The current approach is pragmatic for single-node devstack.

Implementation Order

  1. Phase 4 (forbid unsafe) — 30 minutes, zero risk, one-line changes

  2. Phase 3 (HTTP clients) — 1-2 hours, low risk, 3 files

  3. Phase 1 (soft-delete) — largest scope, 9 migrations + 9 store files + models

  4. Phase 2 (transactions) — most complex, requires careful testing of concurrent scenarios

Phases 4 and 3 can be done on the same branch if desired (both are small). Phases 1 and 2 should be separate branches.

Testing Strategy

All phases — full test battery per CLAUDE.md:

  1. cargo nextest run --workspace --lib

  2. cargo xtask dev restart (schema changes in Phase 1) or cargo xtask dev reload (code-only in Phases 2-4)

  3. cargo nextest run --workspace

  4. cargo xtask e2e

Phase 1 additional: Verify soft-deleted records are excluded from LIST endpoints but still returned by GET.

Phase 2 additional: Manually test concurrent placement creation (two terminals, same foster home at capacity-1) to verify the transaction + FOR UPDATE lock prevents overallocation.

Documentation Updates

Per Documentation Update Checklist:

  • .claude/docs/services.md — note soft-delete behavior on affected entities

  • CHANGELOG.adoc — entries under == Unreleased for each phase

  • docs/modules/ROOT/pages/implementation-guide.adoc — document transaction pattern and soft-delete convention

  • .claude/docs/shared-crates.md — document DbPool::begin() addition

  • .claude/CLAUDE.md — update Known Issues if any, update phase stats on last MR only

Edit this page · latest