Add a behavioural conformance suite for MemoryProvider drivers (#18 §E1) - #20
Conversation
`rust_out` and `tmp` are 3.6 MB Linux x86-64 ELF executables with debug info, committed into a repository developed on macOS. Nothing references either name in source, config, CI, or docs; both are the output of an ad-hoc build that was staged by accident, and every clone has carried 7.3 MB of them since. Ignored with anchored paths so a legitimate `tmp` directory inside a crate is unaffected.
The workspace declared three different answers: 1.96 at the root and for the module crate, 1.85 for `core` and both adapters, and nothing at all for `api`. The low ones were not true — the root facade requires 1.96 and depends on all of them, so nothing in this workspace builds on 1.85, and no CI job checked the claim. Aligning on 1.96 immediately surfaced three lints that the understated MSRV had been suppressing: clippy only suggests `is_multiple_of` when the declared minimum supports it. Those are fixed here rather than allowed, since they are a consequence of this change and not separable from it. `api/` gaining an explicit MSRV is worth noting for downstream: it is the dependency-light crate an embedding host binds directly, so it now states a floor rather than leaving consumers to infer one.
The layout section omitted `tinymemory-core` entirely — the largest crate in the repository, and the one a real host actually depends on. It also omitted `crates/tinymemory-module`. Adds both, and states plainly that `core` is not dependency-light the way `api` is, since that asymmetry is the thing a reader most needs to know before choosing which to depend on. Also documents `git submodule update --init --recursive`. Nothing builds without it, and because `core` names its engines by path through `vendor/`, an uninitialized checkout fails at manifest resolution with an error that does not mention submodules.
`audit_provider` checks that a driver's advertised capabilities match its reachable accessors — a structural check, proving the shape is honest, not the behaviour. Nothing checked that two drivers answer the same question the same way, which is the claim "swap the engine without the host learning anything new" actually rests on. `tinymemory-conformance` is that check. `assert_provider` takes any bound driver and drives the contract: the mandatory three families, upsert on `(namespace, key)`, namespace isolation, provenance preservation, recall limits and scoping, export pagination, import round-tripping, and unicode / empty / 64 KiB / control-character content. It depends on `tinymemory-api` and nothing else of substance. A suite that pulled in an engine could not prove interchangeability, having already chosen one; reaching `tinymemory-core` would drag in a bundled SQLite besides. Ships an in-memory reference driver, for two reasons. A suite that only ever ran against real engines cannot distinguish "the engine is wrong" from "the assertion is wrong", and a driver whose behaviour is obvious by inspection separates those. It also documents the contract by example — the upsert, the verbatim taint, the cursor that terminates on `None` rather than on an empty page. Writing it immediately found a distinction worth naming. The contract permits a driver that accepts writes and discards them; `NullMemoryProvider` is exactly that, and the storage assertions are vacuous for it. Rather than weakening each assertion to tolerate an empty read — which would let a driver that *intends* to retain and silently does not slip through — the suite probes retention once and reports which half it ran. The contract-shape assertions are never skipped. Provenance is the sharp assertion. A driver reading back `Internal` for content stored as `ExternalSync` has laundered external content into internal-trust content, and every downstream gate keyed on taint is then silently wrong. Refs tinyhumansai#18 (§E1)
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tinysweeper
Bot
added
the
priority: p3
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Reviewed this PR's own commit (59178d2). The crate is well-placed — workspace and default member, no required-features, no cfg gates, so a bare cargo test runs it. The manifest's refusal to depend on tinymemory-core is the right call and the comment explaining it is worth keeping: a suite that linked an engine could not prove interchangeability.
Four findings, the first significant.
1. The retention probe swallows the exact failure it claims to catch
if !retains_writes(p).await {
return; // skips all seven storage assertions
}The doc comment argues:
a driver that intends to retain and silently does not is the failure mode worth catching, and a caller-supplied flag would let it through
I think that is inverted. With the probe as written, a driver that intends to retain and silently does not returns false, assert_provider returns early, and the suite reports success — seven assertions skipped, nothing printed. A caller-supplied expectation is precisely what would catch it: the caller declares "TinyCortex retains", the probe disagrees, the test fails.
This matters most for the drivers §E1 actually targets. Run the TinyCortex adapter or Mem0 through this suite with a broken write path and it passes green.
Suggestion that keeps the property you want (don't trust the caller) while closing the hole — have the caller declare, and fail on disagreement rather than branching on the probe alone:
pub enum Retention { Required, Discards }
pub async fn assert_provider(p: Arc<dyn MemoryProvider>, expect: Retention) {
// ... contract-shape assertions, unchanged ...
let retains = retains_writes(&*p).await;
match (expect, retains) {
(Retention::Required, false) => panic!("{}: declared retaining, discarded a write", p.driver_id()),
(Retention::Discards, true) => panic!("{}: declared /dev/null, retained a write", p.driver_id()),
(Retention::Discards, false) => return,
(Retention::Required, true) => {}
}
// ... storage assertions ...
}The probe still runs, so a lying caller is still caught — but silence is no longer a passing outcome.
2. Nothing pins that the reference driver takes the retaining branch
retains_writes is called in exactly one place and asserted nowhere. If InMemoryProvider regressed, the_in_memory_reference_driver_conforms would pass vacuously too — so the calibration subject, whose whole job is to tell "assertion is wrong" from "driver is wrong", cannot currently detect its own breakage. One line fixes it:
assert!(retains_writes(&InMemoryProvider::new()).await, "the reference driver must retain");3. §E1's Unsupported requirement is not implemented
§E1 asks that "every unadvertised family returns MemoryError::Unsupported, never Ok." There are zero occurrences of Unsupported in conformance/. What is here is assert_capability_audit (advertised == reachable) plus the accessor is_none() checks in the test target — that is §E1's audit bullet, which is a different requirement. Worth either implementing or calling out as deferred, so the §E1 checkbox is not read as complete.
4. A soft if let makes the awkward-content assertion skippable
if let Some(got) = provider.get(&ns, key).await... {
assert_eq!(&got.content, content, "{who}: `{key}` content was mangled");
}A driver that retains generally but drops specifically the unicode / empty / 64 KiB / NUL-byte cases returns None, the assert_eq! never runs, and the case passes. The 64 KiB large case is exactly where a real backend silently truncates or rejects. Since this assertion only runs after the driver has already been shown to retain, None here is a defect by definition and should be else { panic!(...) }.
Scope note (not a defect in this PR)
§E1 asks the suite be run against the TinyCortex adapter and the three remote adapters too; only Null and InMemory are wired. I can see why — the adapter needs the host's process-global seams, as #22's engine/test.rs explains — but the §E1 checkbox is not fully met by the stack as it stands, and it would be good to say where that leg joins.
The structure here is good and the two-reference-driver rationale (calibration subject + opposite end) is a genuinely nice design. Finding 1 is the one I would want addressed before this merges, since everything later in the stack is measured against this net.
3 participants
Summary
Step 2 of the sequencing in #18 — §E1, the driver conformance suite. Additive only: no existing crate changes behaviour.
audit_providerchecks that a driver's advertised capabilities match its reachable accessors. That is a structural check — it proves the shape is honest, not that the behaviour is. Nothing checked that two drivers answer the same question the same way, which is the claim "a second engine binds in its place without the host learning anything new" actually rests on.tinymemory-conformanceis that check.assert_providertakes any boundArc<dyn MemoryProvider>and drives the contract:(namespace, key)— a re-store replaces, never duplicateslistandgetExternalSyncsurvives store → get and export → importNonerather than on an empty pageaudit_providerreports nothing advertised-but-unreachableEach sub-assertion is also public, so a driver mid-implementation can run the parts it claims and get a useful failure rather than an unrelated one.
Dependencies
tinymemory-api,async-trait,serde_json,anyhow. Deliberately nothing else: a suite that pulled in an engine could not prove interchangeability, having already chosen one — and reachingtinymemory-corewould drag in a bundled SQLite and the embedded engine besides (§D).The reference driver
InMemoryProvider, for two reasons.A suite that only ever ran against real engines cannot distinguish "the engine is wrong" from "the assertion is wrong". Running it against a driver whose behaviour is obvious by inspection separates those.
It also documents the contract by example — the
(namespace, key)upsert, taint persisted verbatim, the cursor that terminates onNone. It advertises exactly the mandatory three and leaves every optional accessor atNone, which is what makes its own audit pass.One design note worth review
Writing this surfaced a distinction the contract permits but does not name: a driver may accept writes and discard them.
NullMemoryProvideris exactly that, and the storage assertions are vacuous for it.The tempting fix is to weaken each assertion to tolerate an empty read. That is wrong — it would let a driver that intends to retain and silently does not pass the whole suite, which is the failure mode most worth catching. Instead the suite probes retention once (
retains_writes) and skips only the storage half, never the contract-shape half. The probe is deliberately not a flag the caller passes, for the same reason.If you would rather
nullwere simply excluded from the suite, say so and I will invert it — but I think a contract suite that cannot run against the contract's own reference driver is asserting storage rather than the contract.Validation
All four contract commands from
AGENTS.md, from the repository root:cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo build --all-targets --all-featurescargo test --all-featuresThe suite itself: 3 integration tests + 1 doctest, green against both
InMemoryProviderandNullMemoryProvider.Not in this PR
integration/remote-engines/Docker harness wired into CI (§E2). Worth its own PR so a live-engine failure is not confused with a suite bug.Related
Part of #18. This is also the item blocking
OPENCOMPANY_MEMORY=remotein tinyhumansai/opencompany#914 — routing a tenant's memory at an adapter with one happy-path test each is what this suite exists to stop.