test: trait-level contract harness scaffold (closes coverage gap class from #3890)

[zmanian]

Summary

Introduces a trait-level contract test harness pattern. One trait, one shared suite of invariants, every impl wired in one line. Failures attribute to a specific impl by name.

This is a scaffolding PR — it establishes the macro/pattern with one concrete adoption (MemoryDocumentRepository) to prove the shape. It does not try to migrate every trait in one go.

Why

Across recent PR review (#3890, #3887, #3908) the same antipattern keeps recurring:

• A trait has 2+ impls (in-memory, filesystem, libsql, postgres, …).
• Every impl is supposed to honor the same isolation / durability / CAS invariants.
• Tests only exercise one impl — often a mock that quietly implements its own invariants and never crosses the layer where a real bug lives.

Per .claude/rules/testing.md ("Test Through the Caller, Not Just the Helper"), a contract test that runs against only one impl proves only that impl — not the contract. #3890 specifically surfaced a search-isolation HIGH on MemoryDocumentRepository that would have been impossible if every impl was forced through the same suite.

Design

Contracts are plain pub async fn in crates/ironclaw_memory/src/contract_tests.rs that take a factory closure Fn() -> R:

pub async fn writes_isolated_across_scopes<R, F>(factory: F)
where R: MemoryDocumentRepository, F: Fn() -> R { … }

Per-impl wiring is a single line via the exported contract_test! macro:

// crates/ironclaw_memory/tests/repo_in_memory_contract.rs
contract_test!(in_memory, InMemoryMemoryDocumentRepository::new);

// crates/ironclaw_memory/tests/repo_filesystem_contract.rs
contract_test!(filesystem, || {
    FilesystemMemoryDocumentRepository::new(Arc::new(InMemoryBackend::new()))
});

The macro expands to one #[tokio::test] per contract, wrapped in a module named after the impl label. Failure output reads:

test filesystem::search_documents_isolated_across_scopes ... FAILED

— clear attribution, no shared mutable state across tests (factory produces a fresh repo per contract).

Contract invariants covered in this scaffold

Contract	Invariant
round_trip_returns_written_bytes	Put then get returns the exact bytes written.
writes_isolated_across_scopes	A write in scope A is invisible to reads in scope B — even at the identical relative path. (The class of bug behind #3890.)
list_documents_honors_scope	list_documents(scope) returns only documents in that scope.
search_documents_isolated_across_scopes	search_documents either returns the documented Unsupported-shaped error, or returns only scope-local hits. This is the exact #3890 HIGH finding.

Both InMemoryMemoryDocumentRepository and FilesystemMemoryDocumentRepository pass all four contracts cleanly. (If either had failed, that would be a real bug surfaced — none found in this scaffold pass.)

Non-goals (explicitly)

• Not migrating every trait. Follow-ups can port IdempotencyLedger, CheckpointStateStore, ProcessStore, ApprovalRequestStore, MemoryDocumentIndexRepository, etc. onto the same shape.
• Not deleting existing tests. memory_backend_contract.rs and memory_filesystem_contract.rs are untouched; deduplication of any tests subsumed by the new suite is a follow-up.
• Not expanding the suite breadth. The first contract set is deliberately minimal — round-trip, scope isolation, list filter, search isolation. CAS, metadata, append-outcome, indexer-coupling contracts can be added as follow-ups once the shape lands.
• Not reshaping MemoryDocumentRepository. The harness fits the trait as-is; the default search_documents impl returns Unsupported, and the contract handles that branch.

Test plan

• cargo fmt clean
• cargo clippy -p ironclaw_memory --all-targets --all-features clean (no warnings, per CLAUDE.md zero-warning policy)
• cargo test -p ironclaw_memory — all existing tests still pass; 8 new contract tests pass (4 per impl)
• New tests attribute by impl: in_memory::… and filesystem::… show up distinctly in test output
• Reviewer: confirm the harness shape (closure factory + contract_test! macro) is the one you want to standardize on before we start porting other traits

Follow-ups

1. Extend the MemoryDocumentRepository contract suite (CAS append semantics, metadata roundtrip, indexer-invariant 6 chunk invalidation).
2. Port IdempotencyLedger to the same shape (the trait at issue in [codex] Route Reborn production builders through factory #3887).
3. Port ProcessStore / CheckpointStateStore (existing contract files already exist in those crates — easy ports).
4. Deduplicate single-impl tests in memory_backend_contract.rs / memory_filesystem_contract.rs that are now subsumed by the trait-level suite.

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[henrypark133]

Single-agent review. (Security · Bugs · Performance · Tests · Conventions · Design)

Verdict: COMMENT — 3 Medium findings, 1 Low finding. No Critical/High issues; the pattern established is sound and valuable. The Mediums below should be addressed before merge since two of them (vacuous search assertion, dead variant check) directly undermine the stated purpose of the PR (making isolation contracts load-bearing by construction).

---

Findings Summary

Tests (2 Medium)

M1 — search_documents_isolated_across_scopes passes vacuously for FilesystemMemoryDocumentRepository (contract_tests.rs:174–224)

The contract writes two documents but no chunk records. FilesystemMemoryDocumentRepository::search_documents queries the .chunks/ subtree over Filter::Fts, not raw document bodies. With no chunks present, the call returns Ok([]), the for-loop iterates zero times, and the scope-isolation assertion is trivially satisfied. This is precisely the vacuous-pass anti-pattern the PR aims to eliminate: the only impl where the Ok branch fires does so on an empty result set, so a broken isolation would never surface. Fix: add replace_document_chunks_if_current calls after the two write_document calls to seed indexed content, then assert that only scope-A hits are returned.

M2 — FilesystemError::Unsupported arm is dead; real error is Backend (contract_tests.rs:210–220)

memory_backend_unsupported() calls memory_error() which returns FilesystemError::Backend { reason: "memory backend does not support search" }, never FilesystemError::Unsupported. The matches!(err, FilesystemError::Unsupported { .. }) branch never fires; the assertion only passes because the string-fallback err.to_string().to_lowercase().contains("not support") catches it. The doc comment says the intent is to enforce "the documented unsupported variant" — but that variant is never actually emitted. Impls that correctly return FilesystemError::Unsupported would pass; current impls silently fall through. Fix: either change memory_backend_unsupported to emit FilesystemError::Unsupported, or replace the matches! arm with matches!(err, FilesystemError::Backend { .. }) and document the gap.

Conventions (1 Medium, 1 Low)

M3 — pub mod contract_tests is unconditionally in the production library (lib.rs:9)

Both ironclaw_llm and ironclaw_agent_loop gate their public test-helper modules behind #[cfg(any(test, feature = "test-support"))]. contract_tests ships into the production binary without a guard, which is inconsistent with the repo convention. Fix: add #[cfg(any(test, feature = "contract-tests"))] (or reuse a test-support feature) and declare it in Cargo.toml.

L1 — RepoFactory<R> type alias is dead code, narrower than the actual bound (contract_tests.rs:51)

The alias pub type RepoFactory<R> = fn() -> R (bare function pointer) is never referenced: contract functions use F: Fn() -> R (trait-bound accepting both fn pointers and closures), the macro uses $factory directly, and neither wiring file names the type. It will generate a dead-code warning and misleads callers into thinking bare fn-pointers are the canonical factory shape when the filesystem contract wires a closure. Remove or replace with a doc note explaining Fn() -> R is the expected bound.

---

What is clearly right

• The pattern itself — single harness, one #[tokio::test] per contract per impl, factory-per-call isolation — is exactly the right shape.
• round_trip_returns_written_bytes, writes_isolated_across_scopes, and list_documents_honors_scope are load-bearing and correct.
• Factory isolation (fresh repo per contract) is properly enforced.
• The existing unit tests in repo/filesystem.rs are excellent and unaffected by this PR.

Approving once M1 and M2 are addressed — those two directly concern the stated goal of the PR.

[henrypark133]

[Conventions/Medium] Both ironclaw_llm and ironclaw_agent_loop gate their public test-helper modules behind #[cfg(any(test, feature = "test-support"))]. This pub mod contract_tests ships into the production binary without a guard, inconsistent with repo convention. Add #[cfg(any(test, feature = "contract-tests"))] and declare the feature in Cargo.toml.

[henrypark133]

[Conventions/Low] pub type RepoFactory<R> = fn() -> R is never referenced: contract functions use F: Fn() -> R (accepts both fn-pointers and closures), the macro uses $factory directly, and neither wiring file names this type. It will generate a dead-code warning and misleads callers. Remove or replace with a doc note explaining Fn() -> R is the expected bound.

[henrypark133]

[Tests/Medium] memory_backend_unsupported() calls memory_error() which returns FilesystemError::Backend { reason: "..." }, never FilesystemError::Unsupported. The matches!(err, FilesystemError::Unsupported { .. }) arm never fires; the assertion only passes via the string-fallback contains("not support"). Fix: either update memory_backend_unsupported to emit FilesystemError::Unsupported, or swap the matches! arm to FilesystemError::Backend { .. }.

[henrypark133]

[Tests/Medium] This contract writes documents but no chunk records. FilesystemMemoryDocumentRepository::search_documents queries the .chunks/ subtree via Filter::Fts, not raw doc bodies. With no chunks, the search returns Ok([]), the for-loop below iterates zero times, and the scope-isolation assertion is trivially satisfied — vacuous pass. Add replace_document_chunks_if_current calls after the two write_document calls so the Ok branch actually validates isolation with real hits.

[henrypark133]

Approving with follow-up — prior COMMENT review (#pullrequestreview-4349702482) captured 3 Medium + 1 Low findings. No Critical/High. Not blocking merge per Med/Low-only rule. Items to address in follow-up:

• (Tests/M) Search isolation contract vacuously correct for FilesystemMemoryDocumentRepository — no chunk records written before search
• (Tests/M) FilesystemError::Unsupported arm is dead; actual error is Backend
• (Conventions/M) pub mod contract_tests unconditional in production binary; should gate behind #[cfg(any(test, feature = "contract-tests"))]
• (Conventions/L) RepoFactory<R> type alias is dead code