Make the full driver retain, so its conformance run is not vacuous - #150
Conversation
…t vacuous The driver landed accepting writes and discarding them: `store` returned `Ok(())`, `get` returned `Ok(None)`. That is a legitimate binding — the contract allows it and `NullMemoryProvider` is exactly that — so `assert_provider` probed `retains_writes`, got false, and skipped all ten storage assertions. `the_full_driver_conforms` went green having asserted nothing about storage. The suite's own docs warned about this, and the crate exports `retains_writes` so a caller can catch it: > a double that silently dropped writes would let a whole run pass vacuously. > Probing for that directly is how a caller proves its harness is real. Found by bumping OpenHuman's submodule to this crate and running that host's memory suite, where `driver_memory_round_trips_through_the_bound_driver` fails against a driver that does not round-trip. That test is about the binding, not the engine, so it should have passed. So: entries are stored, and `get` / `forget` / `list` / `namespaces` / `recall` / `export_page` / `import_records` read them. `list` applies namespace, category and session, which are the contract's isolation rules rather than query semantics. Nothing ranks, scores or searches beyond the case-insensitive substring `InMemoryProvider` already documents as "enough for did-the-write- land-and-come-back, and deliberately not enough to test ordering". Canned answers still win where a caller set one — `with_recall_result` and `with_namespace_summaries` exist so a host can drive a known result set without writing rows, and several do. `the_full_driver_retains_writes` is the regression guard. Without it this failure mode is invisible: the suite passes either way, which is what made it worth a test of its own rather than a comment. The portability tier is now one implementation shared by both drivers (`export_entries_page`, `decode_export_record`). The cursor arithmetic, the `Invalid` on an unrecognised cursor and the `None`-terminates rule belong to the contract rather than to either driver, and writing them twice was two chances to get the terminator wrong in different ways — which is the same duplication that produced three `InMemoryProvider`s across two repositories. Refs tinyhumansai#147
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0179 · 103,626 in / 23,447 out · 11,546 cached (11%) · openrouter/openai/text-embedding-3-small, deepseek/deepseek-v4-flash, z-ai/glm-5.2 · 551 embedded
critique: $0.0069 · 41,704 in / 18,144 out · 2,048 cached (5%) · deepseek/deepseek-v4-flash
security: $0.0088 · 37,076 in / 5,154 out · 9,498 cached (26%) · deepseek/deepseek-v4-flash, z-ai/glm-5.2
tests: $0.0014 · 16,025 in / 88 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0008 · 8,821 in / 61 out · 0 cached (0%) · deepseek/deepseek-v4-flash
| content: content.to_string(), | ||
| namespace: Some(namespace.to_string()), | ||
| category, | ||
| timestamp: "1970-01-01T00:00:00Z".to_string(), |
There was a problem hiding this comment.
Preserve original timestamp during import
The import_records method sets timestamp to a fixed string "1970-01-01T00:00:00Z" instead of reading it from the export record's payload. This breaks timestamp preservation across export/import round-trips. The conformance suite's portability tests likely check that imported entries match the original, including timestamp. The decode_export_record function should return the timestamp, and it should be used in the MemoryEntry construction.
[RULE] data-loss ·
| if scope.is_some_and(SourceScope::is_empty) { | ||
| return Ok(Vec::new()); | ||
| } | ||
| let needle = query.to_lowercase(); |
There was a problem hiding this comment.
Apply category and session_id filters in recall
The recall method only filters by namespace and does not apply the category and session_id fields from _opts. This means that recall with those filters set will return entries that should be excluded, causing conformance tests that rely on those filters to fail. The list method correctly applies these filters, so the inconsistency is likely an oversight. Add filter closures for category and session_id similar to the ones in list.
[RULE] unimplemented-filter ·
tinysweeper
Bot
added
the
priority: p1
…ady does `OwnedRecallOpts` carries `namespace`, `category` and `session_id`, and recall honoured only the first. `list` has always applied all three. A caller that narrowed a recall by category and got rows from another one has been told something false about its own store, and nothing in the suite would have said so. Both drivers had it, so both are fixed. The reference driver's copy is pre-existing rather than something this branch introduced, but leaving one driver narrowing and the other not is exactly the divergence that makes a conformance crate stop meaning anything. `min_score` is deliberately still ignored. That one is ranking, and neither driver ranks — `recall` here is a case-insensitive substring match, documented as enough for "did the write land and come back" and not enough to test ordering. Raised by tinysweeper on tinyhumansai#150.
|
Both findings checked against the code. One is a false positive, one is real and now fixed. ❌ high — "Preserve original timestamp during import" — false positiveThe suggested fix is not possible, because there is no timestamp in an export record to preserve.
payload: serde_json::json!({
"key": e.key,
"content": e.content,
"category": e.category.to_string(),
"session_id": e.session_id,
}),No The finding's stated reason is that "the conformance suite's portability tests likely check that imported And the value is not novel. timestamp: "1970-01-01T00:00:00Z".to_string(),So the behaviour matches the driver already shipped beside it. There may be a case for carrying timestamps ✅ medium — "Apply category and session_id filters in recall" — correct, fixed in 21082d5This one is right, and its reasoning is the right reasoning: Two notes on the fix:
Verification after the fix
One thing worth flagging for the tool rather than this PR: the run reports |
There was a problem hiding this comment.
The previously-blocking findings are resolved. Clearing the changes request.
$0.0589 · 82,381 in / 31,480 out · 29,686 cached (36%) · openrouter/openai/text-embedding-3-small, deepseek/deepseek-v4-flash, z-ai/glm-5.2 · 576 embedded
critique: $0.0040 · 30,179 in / 7,494 out · 0 cached (0%) · deepseek/deepseek-v4-flash
security: $0.0342 · 26,562 in / 17,302 out · 9,956 cached (37%) · deepseek/deepseek-v4-flash, z-ai/glm-5.2
tests: $0.0093 · 16,010 in / 2,509 out · 12,520 cached (78%) · z-ai/glm-5.2
description: $0.0114 · 9,630 in / 4,175 out · 7,210 cached (75%) · z-ai/glm-5.2
| records, | ||
| next_cursor, | ||
| }) | ||
| let rows: Vec<MemoryEntry> = self.rows()?.values().cloned().collect(); |
There was a problem hiding this comment.
Sort rows before computing export page
export_entries_page documents that rows must be a stable order (sorted by id), but the caller collects HashMap values without sorting. Because HashMap iteration order is non-deterministic, the cursor offset may refer to a different record across calls, causing duplicate or skipped records in paginated export.
[RULE] unsorted-rows ·
How this change flows0 changed behaviours across 5 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 45 further behaviours left out to keep the diagram readable. flowchart LR
n0["Result"]:::impacted
n1["rows"]:::impacted
n2["export_page"]:::impacted
n3["import_records"]:::impacted
n4["recall"]:::impacted
n5["namespaces"]:::impacted
n1 -->|uses| n0
n2 -->|uses| n0
n3 -->|uses| n0
n4 -->|uses| n0
n5 -->|uses| n0
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
tinysweeper
Bot
added
priority: p2
Four families accepted writes and answered as if none had happened: `put_document` stored while `list_documents` returned `[]`, `put_tool_rule` stored while `tool_rules` returned `[]`, `set_goals` stored while `goals` returned the default, and `put_relation` stored while `relations` returned `[]`. `delete_tool_rule` answered `false` for a rule that was there. This is the same defect #150 fixed for the entry tier, in the families #150 did not check. The shape is worth naming because it is the one this driver keeps reproducing: a family advertised through `as_*()`, its writes accepted, its reads empty — which passes every structural check the suite makes and fails only when a host asks for the row back. Found downstream. tinyhumansai/openhuman#6161 binds this driver in place of its in-process engine, and 22 of that host's handler tests fail on the round trip — `doc_put` then `doc_list`, `put_tool_rule` then `tool_rules`. They are host tests, not engine tests, so the answer is a driver that stores rather than tests that go. `the_full_driver_retains_every_family_it_serves` is the guard, and it is a probe rather than a suite assertion for the same reason `retains_writes` is: `assert_provider` gates optional families on `as_*()`, and a driver that advertises one and discards its writes satisfies every shape check it makes. Nothing in the suite can see this class. One probe per storing family can. Scope held deliberately: `relations` filters on namespace, subject and predicate — the contract's own selectors — and nothing ranks or scores. `delete_tool_rule` requires the id *and* the tool name to match, because the pair is what the caller asserted even though the id alone is unique. Refs #147
`memory/test_support` built a real `TinycortexProvider` over a temp workspace for 59 call sites, and that is what kept `tinycortex` and `tinymemory-core` — 133k lines — on this crate's test critical path long after they left the product build (#5560). Its docstring justified the engine on the grounds that the only alternative was the bus, and a `dlopen`ed module is a process singleton that hangs when a second test loads it. That was a false choice: the third option is a driver that is neither the engine nor the bus. `tinymemory-conformance` now ships one, held to the same contract as TinyCortex by `assert_provider` — the engine is run against those same assertions upstream, so what a test observes here is contract behaviour rather than one engine's behaviour. The fixture is renamed with it. `install_tinycortex_for_test` installing a conformance fake would be the kind of name that costs someone an hour. 45 tests are deleted rather than repointed. Each one fails against a driver that does not filter, rank, aggregate or extract — which is to say each one was asserting engine behaviour through a host wrapper, and upstream owns every one of those behaviours. Repointing them would have been worse than deleting them: they would then assert the fake's filtering, which is code written to make the test pass. The line was drawn by measurement, not by reading. Binding a driver that answers empty separates the two populations by construction — a test that passes with no data is asserting what this crate does, and one that fails is asserting data the driver returned. 997 of 1030 memory tests passed that way, so the repoint is free for everything that stays and the decision surface was 45 tests, not the ~300 a file-by-file reading would have suggested. Two of those 45 turned out not to belong there. `driver_memory_round_trips_ through_the_bound_driver` and `experience_store_round_trips_over_the_bound_ driver` are about the binding rather than the engine, and they failed only because the fake had a defect — it accepted writes and discarded them (tinyhumansai/tinymemory#150). With that fixed they pass, and they stay. The engine is still a dev-dependency at this commit, deliberately. Both drivers are constructible in the same build until the manifest is cut, which is what makes the comparison above repeatable rather than a claim. Refs #6161
1 participant
Summary
The driver added in #148 accepted writes and discarded them, so its conformance run was vacuous. This makes it
retain, and adds the guard that would have caught it.
Related issue
Follow-up to #147 / #148. Unblocks tinyhumansai/openhuman#6161.
API or behavior changes
RecordingProvidernow stores what it is given at the entry tier. Reads (get,forget,list,namespaces,recall,export_page,import_records) answer from that store instead of returning empty.Canned answers still take precedence where a caller set one —
with_recall_resultandwith_namespace_summariesexist so a host can drive a known result set without writing rows, and several do.Not breaking: every previous answer was empty, so nothing that passed before can start failing on a value it
did not expect.
What went wrong
storereturnedOk(())andgetreturnedOk(None). That is a legitimate binding — the contract allowsit, and
NullMemoryProvideris exactly that — soassert_providerprobedretains_writes, gotfalse, andskipped all ten storage assertions.
the_full_driver_conformswent green having asserted nothing aboutstorage.
The suite's own module docs describe this trap, and the crate exports
retains_writesprecisely so a callercan check for it:
I did not spend it on our own driver.
the_full_driver_retains_writesnow does. Without that test the failuremode is invisible — the suite passes either way, which is what makes it worth a test rather than a comment.
How it was found: bumping OpenHuman's submodule to #148 and running that host's memory suite.
driver_memory_round_trips_through_the_bound_driverfails against a driver that does not round-trip, and thattest is about the binding rather than the engine, so it should have passed.
Scope of the storage
listapplies namespace, category and session — the contract's isolation rules, not query semantics.recallis a case-insensitive substring match over content, which is what
InMemoryProvideralready documents as"enough for did-the-write-land-and-come-back, and deliberately not enough to test ranking". Nothing ranks,
scores or searches. A host test that needs more than this is asserting engine behaviour and belongs upstream of
the host.
Validation
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo build --all-targets --all-featurescargo test --all-featuresPlus:
cargo test -p tinymemory-tinycortex --test full_provider_conformance— 35 passed. This is thetarget that runs
assert_provideragainst the real engine, so it is what proves the fake and TinyCortexstill agree now that the storage assertions actually execute.
the_full_driver_conformsnow runs the full storage path rather than returning early at the retentionprobe.
Tests
the_full_driver_retains_writes— the regression guard.the_full_driver_conformsbecomes meaningful for the first time: it now executesassert_store_get_round_trip,assert_upsert_replaces_rather_than_duplicates,assert_list_filters_narrow,assert_taint_is_preserved,assert_recall_respects_limit_and_namespace,assert_namespaces_preserve_their_section,assert_recall_respects_source_scope,assert_export_import_round_trip,assert_awkward_content_round_tripsandassert_kv_round_trip, none ofwhich ran before.
Each of those failures was fixed in turn rather than worked around — recall returning empty, then a stored
entry not appearing in any export page.
Documentation
The
entriesfield documents the vacuous-pass failure and points at the guard. The portability helpersdocument why they are shared. The
recallandlistbodies state what they deliberately do not do.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the description