Let configuration select the memory engine, gated by the registry (#18 §A5) - #23
Let configuration select the memory engine, gated by the registry (#18 §A5)#23YellowSnnowmann wants to merge 7 commits into
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)
Issue tinyhumansai#18 §E3. `AGENTS.md` mandates a `tests/` directory exercising only the public API; the repository had none at the root, and `core/tests/` holds fixtures with no test files. Four targets, matching the four §E3 names: `driver_selection.rs` pins admission: reserved ids resolve to fixed classes, an external driver with no entry is refused fail-closed, an untrusted external driver is refused even with one, a reserved id's class cannot be overridden by config, and a class typo is echoed back so the operator can find the line. `capability_negotiation.rs` covers both directions of the bind-time negotiation, including a deliberately lying provider that advertises a summary tree it has no accessor for — the failure `audit_provider` exists to catch, and which was previously asserted only in unit tests inside the contract crate. `taint_end_to_end.rs` drives provenance through store, get, list, recall, and the export/import round trip, at every driver this workspace ships. It also pins the fail-closed reading of an unknown persisted value, which is the one direction that cannot be undone. `null_provider.rs` asserts the compiled-out configuration is genuinely usable: every mandatory method answers rather than panicking, it reports Ready rather than a fault, and no optional family is either advertised or reachable. Two of the four are deliberately narrower than §E3 describes, and both say so in their module docs. `driver_selection.rs` cannot yet assert that a bound provider's `driver_id()` matches configuration, because nothing selects an engine from config — that is §A5. `taint_end_to_end.rs` cannot drive the sync path, because sync is welded to the engine until §B. Both are written against current behaviour, per the sequencing note in the issue, and each names where its missing leg joins. Refs tinyhumansai#18 (§E3)
Issue tinyhumansai#18 §C3. The adapter advertised Core, Recall and Portability and nothing else, which is the reason anything wanting a summary tree, entities, or a diff ledger reached past the contract to the engine directly — the families existed, but not through `MemoryProvider`. `crates/tinymemory-module` had grown all of them, because it needed them and nowhere else had them. They were never module-specific. Every one delegates to `tinymemory-core` on a blocking thread, and the two types they hold — `MemoryClient` and the host config — are core and contract types respectively. So the whole `ModuleMemoryProvider` moves down to `tinymemory-tinycortex` as `engine::TinycortexProvider`, and the module crate keeps only the thing that genuinely is its own: turning a `ModuleConfig` into the engine's runtime configuration. Its `provider.rs` goes from 2189 lines to 38. Nineteen family implementations move: documents, ingest, graph, goals, tool-memory, tree, entities, diff, sources, maintenance, people, chunks, retrieval, profile, and episodic, alongside the mandatory three. The diff family gets a `memory-git` feature rather than riding along unconditionally. It is what drags `git2` / `libgit2-sys` / `libz-sys` — a native build — into the graph, and this adapter had no such dependency before today; making it unconditional would hand every consumer a libgit2 build they never asked for. `cargo tree --no-default-features` confirms none is linked. The gate reaches `capabilities()`, not just the accessor. A build without `memory-git` neither advertises nor reaches `Diff`, so `audit_provider` still passes — which is the whole reason that audit exists. That rule is extracted as `engine::advertised_capabilities` so it can be tested directly: constructing a provider needs a `MemoryClient`, which needs the host's process-global seams installed, and a test that installs a process global is order-dependent. The new module is `engine`, not `provider`: the crate already has a `provider` function returning the mandatory-only driver, and both are worth keeping — a host with no workspace, config, or client still has the lighter one. Refs tinyhumansai#18 (§C3)
Issue tinyhumansai#18 §A5. The driver registry could answer "is this driver id real, and is it allowed to answer for memory", and nothing asked it: `DriverRegistry::admit` had no caller outside its own tests, `MemoryHostConfig::memory_provider()` had no reader, and the memory client factory constructed TinyCortex unconditionally. Configuration could not choose an engine. `DriverRegistry::select` closes that: it reads the engine from the host's configuration and puts it through `admit`, so both surfaces are now live. Two corrections to the issue, both load-bearing. §A5 names `memory_provider()` as the selector. That method is a `provider:model` routing string for the memory *workload* — which language model does summarisation and entity extraction — not the store the memory lives in. Reading it would have let a model change repoint a company's storage. Selection reads a new `memory_driver()` instead, defaulted to `None` so it breaks no existing implementation, and a test pins that the two fields stay independent. §A5 also asks that `create_memory_*` return a bound `Arc<dyn MemoryProvider>`. It cannot, and the reason is structural rather than unfinished: since §C3 `adapters/tinycortex` depends on `tinymemory-core`, so a core factory returning a constructed adapter provider is a dependency cycle. Selection therefore resolves the decision and the host constructs — which is what `src/registry`'s module docs have said all along: "It resolves the class, not the instance." A configuration naming no engine gets the reserved embedded default, so adding selection does not turn "I configured nothing" into a host that fails to start. Going through `select` does not loosen admission either: an external engine named in config is still refused without endpoint, credential and trust. Refs tinyhumansai#18 (§A5)
|
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 |
How this change flows0 changed behaviours across 5 relationships. 4 surrounding behaviours are shown (60 graph nodes walked). 37 further behaviours left out to keep the diagram readable. flowchart LR
n0["MemoryProvider"]:::impacted
n1["assert_provider"]:::impacted
n2["the_in_memory_reference_driver_conforms"]:::impacted
n3["the_null_driver_conforms"]:::impacted
n1 -->|uses| n0
n2 -->|calls| n1
n2 -->|tests| n1
n3 -->|calls| n1
n3 -->|tests| n1
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]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Reviewed this PR's own commit (4ab4d55). One significant finding, plus credit where it is due.
The two deviations from §A5 are both good judgment
memory_provider() vs memory_driver() is the best catch in the stack. §A5 literally names memory_provider() as the selector; you found it is a provider:model routing string for the memory workload — which model does summarisation and entity extraction — and that reading it would let a model change repoint a company's storage. Adding a separate memory_driver() and pinning the independence with a test is the right correction, and the issue is wrong, not this PR. Worth folding back into #18 so the next reader does not re-introduce it.
The dependency cycle is real: since #22 made adapters/tinycortex depend on tinymemory-core, a core factory returning a constructed adapter provider cannot compile. "Selection resolves the decision, the host constructs" is a defensible split and matches what src/registry's module docs already said.
I also checked the default path specifically, since a wrong answer there means the wrong backend serves memory: config.memory_driver().unwrap_or(TINYCORTEX_DRIVER_ID), with memory_driver() defaulted to None so no existing implementor breaks, and #21 already pins that a reserved embedded id is admitted with no config entry. An unconfigured host still binds. That is correct.
Finding: select() has no production caller, so selection is not yet live
The commit says select closes the gap "so both surfaces are now live". I do not think that holds yet:
callers of select( → src/registry/test.rs (5), tests/driver_selection.rs (4)
callers of admit( → the same two test files, plus one doc comment in src/lib.rs
I checked every PR in the stack: core/src/store/factories.rs is untouched by all eleven, and at the top of the stack (#29) create_memory_client_with_local_ai still constructs UnifiedMemory unconditionally. No production type overrides memory_driver() either — EngineRuntimeConfig implements MemoryHostConfig and inherits the None default; the only override is TestHostConfig.
So §1.3's complaint was "admit has no caller outside its own tests", and after this PR "select has no caller outside its own tests". The dead code moved up a level rather than being wired, and §A5's acceptance criterion — "config.memory_provider() selects the engine and DriverRegistry::admit gates it" — is met in tests but not at runtime.
To be clear about the blast radius: nothing is silently mis-routed today. Because no production config type exposes the key, an operator cannot set it at all, so there is no wrong-backend risk in this PR. The finding is about the claim and about §A5's status, not about user-visible behaviour.
What I would suggest:
- Soften the commit/PR wording — something like "makes selection expressible and tested; wiring the host factory follows in §A3" — so a reader of the merge history does not conclude configuration now selects the engine.
- Leave §A5 open in #18 rather than checking it off, and note in the body that the factory wiring is blocked on §A3 removing core's direct engine call sites.
The code itself I have no changes to request — select is a thin, correct composition over admit, and going through it does not loosen admission (an external engine named in config is still refused without endpoint, credential and trust — #21 covers that).
2 participants
Summary
Issue #18 §A5.
The driver registry could answer "is this driver id real, and is it allowed to answer for memory" — and nothing asked it.
DriverRegistry::admithad no caller outside its own tests,MemoryHostConfig::memory_provider()had no reader anywhere, andcreate_memory_client_with_local_aiconstructed TinyCortex unconditionally. Configuration could not choose an engine, which is §1.3's finding.DriverRegistry::selectcloses it: reads the engine from the host's configuration, puts it throughadmit. Both surfaces are now live.Two corrections to the issue, both load-bearing
1.
memory_provider()is not the selector. §A5 names it, but its own doc reads "provider:modelrouting string for the memory workload" — which language model does summarisation and entity extraction. That is a different axis from which store the memory lives in, and reading it would have let a model change repoint a company's storage.Selection reads a new
MemoryHostConfig::memory_driver(), defaulted toNoneso it breaks no existing implementation. A test pins that the two fields stay independent.2.
create_memory_*cannot return a bound provider. §A5's last clause asks for it. It is not possible, and the reason is structural rather than unfinished: since §C3 (#22)adapters/tinycortexdepends ontinymemory-core, so a core factory returning a constructed adapter provider is a dependency cycle.Selection therefore resolves the decision and the host constructs. That is not a workaround — it is what
src/registry's module docs have said all along:If you would rather core owned construction, that needs §C3 reverted or the adapter split — worth deciding explicitly rather than by accident.
Behaviour worth checking in review
driversentry. Adding selection must not turn "I configured nothing" into a host that fails to start, and there is a test for exactly that.selectdoes not loosenadmit. An external engine named in config is still refused fail-closed without endpoint, credential and trust. Tested throughselect, not justadmit, so the new path cannot become a way around the gate.Public API changes
Additive:
MemoryHostConfig::memory_driver()— defaulted, so every existing implementation compiles unchangedDriverRegistry::selectTestHostConfig::memory_driverfield — the struct is#[non_exhaustive], so adding a field is non-breakingtinymemory-api'stest-supportValidation
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo clippy -p tinymemory-tinycortex --all-targets --no-default-features -- -D warningscargo build --all-targets --all-featurescargo test --all-featurestest --lib/build --locked --releaseRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features#21's
tests/driver_selection.rsgains the selection leg its scope note promised, and that note is rewritten rather than left stale.Related
Part of #18. This is the item the OpenCompany cross-link named first:
OPENCOMPANY_MEMORY=remotehad "nothing to drive" because admission was never invoked. It now is.