Re-export the TinyMemory contract instead of duplicating it (tinymemory#18 §A1) - #149
Conversation
tinymemory#18 §A1. This crate and `tinymemory-api` described the same memory value types — `MemoryEntry`, `MemoryCategory`, `MemoryTaint`, `NamespaceSummary`, `RecallOpts`, `Capability`, the `provider::types` set — because the latter was extracted from the former and held byte-identical. They were the same code and nominally distinct types, so `adapters/tinycortex/src/convert.rs` in tinymemory had to translate between them on every call, and a field added to one had to be added to the other and to the conversion, in three places, or a value was silently dropped. `api/` now re-exports `tinymemory-api` and defines nothing of its own: 33 files deleted, `lib.rs` left as the re-export. Every existing path still resolves — `tinycortex_api::types::MemoryEntry` now *is* `tinymemory_api::types::MemoryEntry` — so no dependant needs an edit to gain the single type set. Two things change on purpose, both checked against the engine before doing this: - The capability vocabulary grows from thirteen families to eighteen. The engine matches on `Capability` in zero places, so nothing here is affected. - `CONTRACT_VERSION` becomes the contract's `(2, 2)` rather than this crate's `(1, 0)`. That value was already stale — hosts bind against the contract, and the engine never reads it. The enums the engine *does* match on are variant-identical (`MemoryCategory` at 16 sites, `MemoryTaint` at 9), so no match arm changes. The dependency is by git, which requires this crate not to publish — hence `publish = false` here and on the engine. That records the state the repository has actually been in rather than changing it: since `api/` was split out, `cargo package -p tinycortex` has failed with `no matching package named 'tinycortex-api' found`, because that crate was never published. The last release predates the split. A host that vendors both patches the git entry to its own checkout, so a build never resolves two copies. Validation, all from a clean clone at 5fdeac9: - `cargo check --workspace --all-targets` — clean - `cargo test --workspace` — 1259 passed, 0 failed - `cargo fmt --all -- --check` — clean - `cargo clippy --all-targets -- -D warnings` (the CI gate) — clean, 0 before and after - `cargo clippy --workspace --all-targets --all-features -- -D warnings` — 6 errors before and 6 after, all pre-existing in `persona/` and `sync/composio/` and untouched by this change Refs tinyhumansai/tinymemory#18 (§A1)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (36)
💤 Files with no reviewable changes (33)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe API crate now uses ChangesShared API contract extraction
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR replaces duplicated contract definitions with a re-export and records the crate's non-publishable status; the reported checks pass and no actionable merge-blocking risk remains after normal review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
The direction is right and the homework in the description is real. I re-ran it from a clean checkout of 34cbb6c rather than taking it on faith, and the load-bearing claims hold:
| Claim | Checked |
|---|---|
the engine matches on Capability in zero places |
grep -rn "Capabilit" src/ tests/ → no hits, so 13 → 18 families touches nothing here |
the engine never reads CONTRACT_VERSION |
grep -rn "CONTRACT_VERSION|is_compatible" src/ tests/ → no hits, so (1,0) → (2,2) is inert in this repo |
| the re-export is surface-complete | old lib.rs exported 13 pub mods + {is_compatible, CONTRACT_VERSION}; the new pub use list is those exact 13 + the same two. Nothing dropped |
cargo fmt --all -- --check |
clean |
cargo clippy --all-targets -- -D warnings (the ci.yml gate) |
clean |
cargo build --all-targets --all-features |
clean |
RUSTDOCFLAGS="-D warnings" cargo doc --all-features --no-deps |
clean |
I also checked the publish = false worry that isn't in the description: publish = false does not make cargo package skip. On this branch cargo package -p tinycortex --no-verify still fails with the same no matching package named 'tinycortex-api' found. So release.yml fails at the Package crate step exactly as it does today — before Push release commit and tag. No new half-released-tag failure mode. Good.
Four things I'd like changed before this lands. The first is substantive; the rest are the file contradicting itself.
1. Pin the git dependency to a rev (api/Cargo.toml)
tinymemory-api = { git = "https://github.com/tinyhumansai/tinymemory" }This floats on tinymemory's main. That is at odds with the change's own premise: the whole point is that there is now exactly one contract, and CONTRACT_VERSION is a real number that hosts bind against — but with no rev, which contract a build gets depends on when the lockfile was last regenerated.
This is not hypothetical. Cargo.lock here pins 4549cda2 (tinymemory#20, 17 Aug 13:28Z). tinymemory main is already at 7cadb942. Anyone who runs cargo update, and any downstream host that resolves this dependency fresh rather than inheriting our lock, silently gets a different contract than the one CI validated — including a different CONTRACT_VERSION, which is precisely the value the description argues hosts bind against.
Please pin the rev the lock already records:
tinymemory-api = { git = "https://github.com/tinyhumansai/tinymemory", rev = "4549cda222de3891b95e2fa58e2565bb2c194328" }Zero behaviour change — that is the commit already resolved — but the contract pointer becomes an explicit, reviewable line in the diff. Given the description's own merge-order note ("needs the gitlink bumped to a commit that carries the re-export"), bumping that pointer is a deliberate act. It should look like one.
2. The module docs still describe the deleted implementation (api/src/lib.rs)
The new //! block at the top is accurate, but the pre-existing block below it was left as-is and now contradicts both the change and the comment 60 lines further down in the same file:
//! - [capabilities]: the **thirteen** [capabilities::Capability] families— the re-export makes it eighteen. The implementation comment in this same file says "grows from thirteen families to eighteen".//! - [provider]: the driver contract — ... plus the **thirteen** capability family traits— same.//! This crate holds the value types, error enum, capability vocabulary, and storage trait— it no longer holds them; that is the change.//! It is deliberately dependency-light (serde / serde_json / chrono / sha2 / anyhow / thiserror / async-trait / uuid only)— the enumeration is now wrong: viatinymemory-apithe tree also carriesschemarsandlog.
cargo doc passes because the intra-doc links all still resolve; it can't catch prose. This is the crate's rustdoc front page, so it's the first thing a driver author reads.
3. api/Cargo.toml keeps eight now-dead dependencies, with a comment justifying them
api/src/ is one file, and that file is pub use tinymemory_api::{...} and nothing else. So anyhow, async-trait, chrono, serde, serde_json, sha2, thiserror, and uuid are all unused by this crate now.
They aren't free. sha2 = "0.10" is retained here while tinymemory-api brings sha2 0.11, so cargo tree -p tinycortex-api currently shows a doubled hash stack:
block-buffer v0.10.4 crypto-common v0.1.7 digest v0.10.7 sha2 v0.10.9
block-buffer v0.12.1 crypto-common v0.2.2 digest v0.11.3 sha2 v0.11.0
Worth fixing on its own terms, but more so because the comment block directly above [dependencies] now asserts something false — it explains that chrono, sha2, and uuid are each "pulled in by a value type that has to keep behaving identically after the move out of the engine crate", naming chunks::Metadata, chunks::chunk_id, and ToolMemoryRule::generate_id. This crate no longer defines any of those. That comment is the guard rail for "keep the contract crate lean", and it should describe the crate as it now is.
4. The [patch] pointer sends the reader to an unrelated table (api/Cargo.toml)
A host that vendors both patches this entry to its own checkout — see the
[patch]table in the workspace root.
The [patch] table in this workspace root is [patch.crates-io] tinyagents = { path = "vendor/tinyagents" } — nothing to do with tinymemory, and a [patch.crates-io] entry cannot override a git dependency anyway; that needs [patch."https://github.com/tinyhumansai/tinymemory"]. The description gets this right ("as tinymemory's workspace root does"); the comment points at the wrong repo. Either say tinymemory's root or spell out the correct table form, so the next person to hit a two-copies build isn't sent to vendor/tinyagents.
None of this touches the substance — the re-export itself is the right call and the deletions are clean. I'm pushing these four fixes to the branch now so this isn't another round trip; please sanity-check the pinned rev is the one you intended.
Four findings from @sanil-23's review, all valid. **Pin the contract by `rev`.** `4549cda222de3891b95e2fa58e2565bb2c194328` is what the lockfile already resolved, so this is zero behaviour change — checked against `Cargo.lock` rather than taken on faith. What it buys is that bumping the contract becomes a reviewable line in a diff rather than whatever the default branch happened to be when someone rebuilt. **Drop the eight now-dead dependencies.** `api/src/` is one file of `pub use tinymemory_api::{...}`, so anyhow, async-trait, chrono, serde, serde_json, sha2, thiserror and uuid were all unused. They were not free while they stayed: `sha2 = "0.10"` here against `sha2 0.11` in the contract gave `cargo tree -p tinycortex-api` a doubled hash stack. Now one version each — block-buffer 0.12.1, crypto-common 0.2.2, digest 0.11.3, sha2 0.11.0. The comment above `[dependencies]` justified chrono, sha2 and uuid by naming `chunks::Metadata`, `chunks::chunk_id` and `ToolMemoryRule::generate_id`. This crate defines none of those any more, so the comment was asserting something false about the very rule it exists to guard. It now describes the crate as it is, and notes that the no-SQLite/no-git2/no-reqwest rule applies transitively through the contract, which enforces it in its own CI. **Correct the module docs.** `cargo doc` passed because the intra-doc *links* still resolved; it cannot catch prose. The front page said this crate "holds" the value types (it names them), listed a dependency set that is no longer accurate, and said "thirteen capability families" twice while an implementation comment sixty lines below said the re-export grows it to eighteen. This is the first thing a driver author reads. **Fix the `[patch]` pointer.** It sent the reader to "the `[patch]` table in the workspace root" — which in *this* repo is `tinyagents`, unrelated. Worse, a `[patch.crates-io]` entry cannot override a git dependency at all. The comment now spells out the correct `[patch."https://github.com/tinyhumansai/tinymemory"]` form and says it is tinymemory's root that carries it. Validation: - `cargo fmt --all -- --check` — clean - `cargo clippy --all-targets -- -D warnings` (the CI gate) — clean - `cargo check --workspace --all-targets` — clean - `cargo test --workspace` — 1259 passed, 0 failed - `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features` — clean - contract purity guard — 0 matches
|
@sanil-23 all four addressed in 1. Pinned rev. Sanity-checked as you asked: 2. Module docs. Corrected — "holds" → "names", the dependency enumeration dropped rather than re-listed (it would just go stale again), and both "thirteen" occurrences now say eighteen. The one remaining "thirteen" is the implementation comment describing the transition itself, which is accurate. You are right that 3. Dead dependencies. Removed all eight. The doubled hash stack is gone: one version each. I also rewrote the comment above 4. Re-ran from clean: fmt, Note CodeRabbit's review errored out on this PR rather than reporting anything; I have not treated that as a pass. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Issue #18 §A1, the tinymemory half. Companion to tinyhumansai/tinycortex#149, which makes `tinycortex-api` re-export this workspace's contract rather than redefining it. `convert.rs` existed because the engine's contract crate and this one described the same values under two names — the same code, in fact, since `api/` was extracted from `tinycortex-api` and held byte-identical. Being nominally distinct meant every call across the seam translated, and a field added to either contract had to be added to the other and to the conversion. Three places, or the value was silently dropped. With one type set there is nothing to convert: `convert.rs` and its tests are deleted, and the adapter's eleven conversion call sites become plain delegation. `list` and `namespace_summaries` lose an `into_iter().collect()` that had become an identity map, and `store`/`store_with_taint` pass their arguments straight through. The gitlink moves to the commit carrying the re-export. That commit also stops publishing tinycortex, which is what makes the git dependency legal — and records the state that repository was already in, since `cargo package` there has failed since `api/` was split out. The workspace root gains a `[patch]` for the git contract dependency. Without it cargo resolves the git copy *and* the path copy as two distinct crates, and `MemoryCategory` from one is not the same type as the other — the exact duplication this change deletes, reintroduced by the fix for it. Found by the compiler at the seam rather than reasoned about, and the patch table is the same mechanism this workspace already uses for tinycortex itself. Acceptance for §A1, checked rather than asserted: - `adapters/tinycortex/src/convert.rs` is deleted - no conversion function remains anywhere in the workspace (grep: 0 hits) - the workspace builds and tests green with no conversion at the seam `cargo test --all-features` reports 1101 passing, down 9 from 1110. That difference is exactly the nine tests in `convert_test.rs`, which tested the layer this change removes. Refs #18 (§A1)
3 participants
Implements the tinycortex half of tinyhumansai/tinymemory#18 §A1.
The problem
This crate and
tinymemory-apidescribe the same memory value types —MemoryEntry,MemoryCategory,MemoryTaint,NamespaceSummary,RecallOpts,Capability, theprovider::typesset. Not by coincidence:tinymemory-apiwas extracted from this crate and held byte-identical.types.rsdiffers between them by 14 lines, and every one is a doc comment, a doctest path, or a single extra parse arm.They are the same code and nominally distinct types. So
adapters/tinycortex/src/convert.rsin tinymemory translates between them on every call, and a field added to one has to be added to the other and to the conversion — three places, or a value is silently dropped.The change
api/re-exportstinymemory-apiand defines nothing of its own. 33 files deleted;lib.rsis the re-export.Every existing path still resolves.
tinycortex_api::types::MemoryEntrynow istinymemory_api::types::MemoryEntry, so no dependant needs an edit to gain the single type set — andconvert.rsdownstream collapses to identity.Two deliberate changes, checked against the engine first
Capabilityin zero placesCONTRACT_VERSION(1,0) → (2,2)The enums the engine does match on are variant-identical —
MemoryCategory(16 sites) andMemoryTaint(9 sites) — so no match arm changes. I checked this before writing the re-export rather than after.Why
publish = falseThe dependency is by git, and cargo refuses a git dependency in a published crate.
This records the state the repository has actually been in rather than changing it. Since
api/was split out, publishing has been broken:tinycortex-apiwas never published, andtinycortexmain depends on it bypath + version. The last release (0.1.1, 29–30 June, 54 downloads) predates the split, so the release workflow would fail the next time it ran.A host that vendors both crates patches the git entry to its own checkout — as tinymemory's workspace root does — so a build never resolves two copies of the contract.
Validation
From a clean clone at
5fdeac9:cargo check --workspace --all-targetscargo test --workspacecargo fmt --all -- --checkcargo clippy --all-targets -- -D warnings(theci.ymlgate)cargo clippy --workspace --all-targets --all-features -- -D warningsThose 6 are pre-existing, in
persona/andsync/composio/, and untouched here — I measured the baseline on the unmodified commit rather than assuming.Merge order
tinymemory's companion change — deleting
convert.rsand simplifying the adapter — depends on this landing first, since it needs the gitlink bumped to a commit that carries the re-export.Summary by CodeRabbit
tinymemory-apicontract.tinycortexand API packages are no longer publishable as standalone packages.