Add workspace-level integration tests against the public API (#18 §E3) - #21
Add workspace-level integration tests against the public API (#18 §E3)#21YellowSnnowmann wants to merge 5 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)
|
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 4 relationships. 5 surrounding behaviours are shown (60 graph nodes walked). 42 further behaviours left out to keep the diagram readable. flowchart LR
n0["MemoryProvider"]:::impacted
n1["assert_provider"]:::impacted
n2["cleanup"]:::impacted
n3["ns"]:::impacted
n4["InMemoryProvider"]:::impacted
n1 -->|uses| n0
n2 -->|uses| n0
n3 -->|uses| n0
n4 -->|implements| 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]](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 (9c9a61e). Clean — nothing to change.
It adds exactly the four files §E3 names (driver_selection.rs, capability_negotiation.rs, taint_end_to_end.rs, null_provider.rs), at the workspace root against the public API only, with no cfg gates and no required-features. I checked: they execute on a bare cargo test.
The assertions are real rather than smoke tests — I specifically looked for is_ok()-style padding and did not find it. an_external_driver_without_an_entry_is_refused_fail_closed, an_untrusted_external_driver_is_refused_even_with_an_entry, a_reserved_id_cannot_have_its_class_overridden_by_config, an_unknown_driver_id_is_refused_rather_than_defaulted and an_empty_driver_id_is_refused would each fail if the implementation were wrong, and the last three are the cases where a fail-open bug would actually be dangerous.
Also appreciated that the file documents its own incompleteness honestly — noting that §E3's "the bound provider's driver_id() matches" leg "joins it when §A5 lands". That turns out to be the right call; see my note on #23 about how much of §A5 actually landed.
2 participants
Summary
Issue #18 §E3, the second half of step 2 in the issue's sequencing. Additive only — no existing crate changes behaviour.
AGENTS.mdmandates atests/directory exercising only the public API. The repository had none at the root, andcore/tests/contains fixtures with no test files. This adds the four targets §E3 names:driver_selection.rs(9 tests) — 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 an entry; a reserved id's class cannot be overridden by config; a class typo is echoed back so the operator can find the line to fix; blank and unknown ids are refused rather than defaulted.capability_negotiation.rs(5 tests) — both directions of the bind-time negotiation, plus a deliberately lying provider that advertises a summary tree it has no accessor for. That is the exact failureaudit_providerexists to catch, and it was previously asserted only in unit tests inside the contract crate — never through the facade a host actually binds.taint_end_to_end.rs(5 tests) — provenance through store, get, list, recall, and the export/import round trip, at every driver this workspace ships.getis the easy path; a driver that rebuilds entries on the list and recall paths can drop provenance on exactly those, which is where a policy gate reads it. Also pins the fail-closed reading of an unknown persisted value — the one direction that cannot be undone.null_provider.rs(5 tests) — the compiled-out configuration is genuinely usable: every mandatory method answers rather than panicking, health isReadyrather than a fault ("memory is switched off" is a configuration, not a failure), and no optional family is either advertised or reachable.Two targets are deliberately narrower than §E3 describes
Both say so in their module docs, and both name where the missing leg joins.
driver_selection.rscannot yet assert that a bound provider'sdriver_id()matches configuration, because nothing selects an engine from config —create_memory_client_with_local_aistill constructs TinyCortex unconditionally. That is §A5.taint_end_to_end.rscannot drive the sync path, because sync is welded to the engine (§1.4) until §B. What is assertable now is the seam sync will hand to.Both are written against current behaviour, per the issue's own note that these tests come first as "the safety net for everything below".
Public API / behavior changes
None. New
tests/targets and one dev-dependency on the (unpublished) conformance crate.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 four new targets contribute 24 of those.
A note on the lint headers
Each file opens with
#![allow(clippy::expect_used, clippy::unwrap_used, clippy::panic)]and the same rationale comment assrc/registry/test.rs: a failing assertion in a test is a panic, and those crate-wide lints exist to keep the library from panicking, not the tests. This follows the existing convention rather than introducing one.Related
Part of #18. With §F (#19), §E1 (#20) and §E3 (this) landed, step 2 of the sequencing is complete and the safety net is in place for §A1+§A2 — which is step 3, and which needs a companion PR in
tinyhumansai/tinycortex(see below).