Extract the Composio normalisers into an engine-neutral crate (#18 §B3) - #40
Closed
YellowSnnowmann wants to merge 11 commits into
Closed
Conversation
Issue tinyhumansai#18 §E5. The loader E2E drove three of the eighteen families the module advertises, and nothing checked that a write reached the workspace it was given. **Every declared method is routed.** The module advertises `Capabilities::all()` and declares 88 methods; the existing manifest test compares two lists, so it passes for a method that is declared, listed, and answers "unknown member" — the bus-level version of a capability set that overstates its accessors. This calls each declared method and distinguishes the kind of refusal: a wired method rejects the empty argument list with `ai.tinyhumans.tinybus.Error.BadArguments`, while an unwired one is `ai.tinyhumans.tinybus.Error.UnknownMethod`. The two names are what make the test discriminate rather than pass vacuously, and the unrouted shape is taken from the build under test rather than hard-coded. Eighteen bespoke round trips would assert more, and would also need eighteen sets of valid arguments and eighteen engine preconditions. This asserts the one thing that is true of all of them and is cheap to keep true. **A write lands in the host's workspace.** Every other test here stores and reads back inside one admission, so a module that kept its store in a temporary directory of its own, or in memory, passes all of them — and the difference shows on the user's next launch. §E5 asks for a shutdown/restart cycle, and it is not here because it cannot be: TinyBus never unloads a library, so a second admission in the same process is refused with `ModuleRefused { reason: "module initialization failed" }`. That is the same constraint that already forces every test in this file to be the only one in its process. A real restart needs a second process against a shared workspace, which is a change to the CI loop rather than a test, so the directory assertion covers what restart would have been checking and the limitation is written down rather than left as a gap someone rediscovers. Verified the way CI runs them — every ignored test in its own process, all ten green. Refs tinyhumansai#18 (§E5)
Issue tinyhumansai#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 tinyhumansai#18 (§A1)
Issue tinyhumansai#18 §A2. `tinymemory_core::traits` re-exported `Memory` and the memory value types from `tinycortex::memory` — from the *engine*. That is §1.1's finding: `tinymemory_core::MemoryEntry` was the engine's type rather than the contract's, so a second engine could not be bound without translating, and the crate was engine-neutral in name only. It now names `tinymemory-api` directly. Since §A1 the engine re-exports that same contract, so both spellings resolve to one type either way — but reaching an engine-neutral contract *through an engine* is precisely what would have to be undone before a second engine could be bound, and undoing it later is harder than not doing it. §A2's second clause comes along with the first: `UnifiedMemory` implements `crate::traits::Memory`, which is now the contract's trait, so it implements the TinyMemory trait directly with no further edit. This was not possible before §A1. The same repoint attempted then produced 10 compile errors, 9 of which wanted taint conversions added — the three-place edit §A1 exists to delete. With one type set behind both names it is a 20-line documentation change and a re-export, and the workspace compiles unchanged. Refs tinyhumansai#18 (§A2)
Issue tinyhumansai#18 §C1. Eighty-three files named the `tinycortex` crate, in two hundred and ninety-six places. That made the engine's shape an ambient fact of the whole crate rather than a dependency anyone had chosen, and made "what would a second engine have to provide?" a question nobody could answer without reading all of them. Every one now goes through `core/src/engine/backend.rs`, which re-exports the engine's memory surface and is the only file outside the seam that names it. Outside `core/src/engine/`, references to the crate are zero. Re-exporting rather than wrapping is deliberate. A wrapper over three hundred call sites would be a second surface to keep in step with the first, which is the failure §A1 had just finished deleting. What this buys is not insulation — the call sites use the engine's API verbatim — but a single enumerable place that names it. The seam is `engine`, not `tinycortex`. §C1's text says `core/src/tinycortex/`, but its own acceptance criterion asks that `grep -rl tinycortex core/src` match only files under that module, and those two cannot both hold: with the module named after the engine, every call site reads `crate::tinycortex::…` and matches. Naming the seam for its role rather than for one engine satisfies the criterion and is the better name regardless — a seam named after the thing it is meant to make replaceable is the coupling this section removes. The module was `pub` but had no consumer outside this crate, so the rename breaks nothing. The workspace root and the module crate each gain a `[patch]` for the contract's git dependency. Patch tables apply only from the root being built, and the module crate is its own root; without its own entry cargo resolves the git copy alongside the path copy and `MemoryTaint` from one is not the same type as from the other. Found by the compiler, not predicted. What is left matching `tinycortex` outside the seam is 82 log-message strings and a `tinycortex_kv` accessor name. Neither is a crate reference, and renaming the accessor is cosmetic churn §C1 does not ask for. Two stale doc comments describing a "thirteen-family `tinycortex_api` contract" are corrected — since §A1 the contract is `tinymemory-api`, and it has eighteen families. Acceptance, measured: 83 files naming the engine crate outside the seam, now 0. Refs tinyhumansai#18 (§C1)
The module crate is its own workspace root, and a patch table applies only from the root being built — so the parent workspace's entry does not reach it. Without its own, cargo resolves the git copy of `tinymemory-api` alongside the path copy and `MemoryTaint` from one is not the same type as from the other. Belongs in this commit rather than a later one: this is where the git dependency arrives, so this is where its consequence has to be handled. CI caught it — the module lane failed with eight type mismatches while every other lane was green, because it is the only job that builds from that root. Refs tinyhumansai#18 (§A1)
…-one-memory-trait
…e-engine # Conflicts: # crates/tinymemory-module/Cargo.toml
Issue tinyhumansai#18's acceptance criterion 5. The suite ran against the in-memory reference driver and the null driver — both written alongside it, so passing proved the assertions were self-consistent and not much else. The premise the whole issue rests on, that an engine other than TinyCortex can satisfy the contract, had never been exercised. Each adapter now runs the full `assert_provider` over a real TCP socket against a double that speaks its own HTTP shapes and retains what it is sent. That retention is the point: `failure_test`'s doubles only have to misbehave, while these have to work, because the suite writes and reads back. All three pass — eleven assertions each, including taint preservation, upsert identity, namespace isolation, and export/import round trip. Each adapter is paired with a second test asserting its double genuinely retains, and that pairing earned itself immediately. `cognee_upholds_the_ contract` passed while `the_cognee_double_actually_retains` failed: the suite returns early when a driver does not retain, so it had run four assertions and skipped the seven that matter, and reported success. Without the probe this would have been reported as "Cognee passes". The cause was the double, not the adapter. Cognee's data listing has to carry a `name` ending `.tinymemory[.json]` — the adapter skips anything else, because Cognee's own text loader strips the extension — and the listing returned only `id`, so every record was filtered out before the fetch. What this proves is narrower than "the hosted engines uphold the contract", and the module docs say so: nobody here can prove that about someone else's service. It is that *the adapter* does, given a backend answering its own documented shapes. A violation on the adapter's side of the wire — a dropped taint, a non-terminating export cursor, an upsert that duplicates — is caught. `retains_writes` is exported from the conformance crate for this: a caller standing up its own backend double needs it, for exactly the reason above. Not covered here: the TinyCortex adapter. It needs `require_embedding_host()`, a process-global, so driving it means installing host seams — which makes the test order-dependent unless it is isolated in its own target. Criterion 5 names it alongside the three, so it remains open. Refs tinyhumansai#18 (§E1, acceptance criterion 5)
Completes issue tinyhumansai#18's acceptance criterion 5. With the three hosted adapters already covered, this is the last driver the criterion names. `crate::provider` needs only a `tinycortex::memory::Memory` backend, so the suite runs against the engine's own `InMemoryMemoryStore` with no host seams. That is also the sharper test: it is the engine's simplest backend, so anything the suite catches is the adapter's behaviour rather than the storage engine's. It failed on the first run, which is the point of running it: tinycortex: store of `empty` failed: memory content cannot be empty The reference driver, the null driver and all three hosted adapters accept empty content. TinyCortex refuses it. The contract settles which is right — `MemoryCore::store` documents `MemoryError::Invalid` "for caller input the driver rejects" — so refusing is conformant and the *suite* was over-asserting. It required every content shape to round trip, which the contract never promised. `assert_awkward_content_round_trips` now allows a driver to refuse a shape, and still requires that a shape it *accepts* comes back unmangled. A guard keeps that from becoming vacuous: a driver that refused all four shapes fails, because it would otherwise pass having stored nothing. That correction surfaced a second finding, left open deliberately. The refusal arrives as `MemoryError::Other`, not `Invalid`: DIAG variant = Other(memory content cannot be empty) The engine's typed error is flattened through `anyhow` before the mandatory composition sees it, so a validation refusal is indistinguishable from a backend failure. Recovering it would need downcasting or string matching, and the real fix is §A4 — one error type across the contract. The suite says so where the assertion is, so the tightening to require `Invalid` has an obvious home rather than being rediscovered. Not weakened to get green: the reference driver accepts empty content and is still held to round-tripping it faithfully, as are the three hosted adapters. Refs tinyhumansai#18 (§E1, acceptance criterion 5)
Issue tinyhumansai#18 §B3: "Payload normalisers are pure `Value -> Value` transforms with no engine dependency. Move them back into a `tinymemory-sync` crate — so a non-TinyCortex engine gets Composio sync for free." They were not in this workspace at all. They lived inside the TinyCortex engine, and `tinymemory-core` reached in through `tinycortex::memory::sync::composio::providers::normalize::*` to use them. A host binding a different memory engine therefore could not have Composio sync, despite none of this code caring which engine is bound. That is the coupling §B3 names, and it ran through the engine rather than around it. `tinymemory-sync` is fifteen files and 2,598 lines, depending on `serde_json`, two logging facades, and `chrono`. It links no engine, no storage, no async runtime — and no contract either. Two things found while moving, both stated rather than smoothed over. The crate is not quite the pure function of its input that §B3 describes. `format_email_local_time` renders in `chrono::Local`, so it reads the host's timezone, and `notion::now_ms` reads the clock. Both are deliberate upstream — the agent presents local times without doing UTC arithmetic, and Notion payloads carry no ingestion timestamp — and the raw UTC field is preserved alongside, so sorting and deduplication stay UTC-based. Documented at the crate root and at each function rather than left for someone whose output moves when they change `TZ`. The two logging facades are also inherited: `gmail_post_process` traces through `tracing`, `slack_post_process` through `log`. Preserved rather than unified, because §B3 is a move and swapping a facade changes where a host's log lines surface — a behaviour change hiding inside a relocation. The move is otherwise verbatim, with four exceptions, all forced by this workspace's lint configuration being stricter than the engine's gate reached: two `unwrap`s removed by checking presence immutably before fetching mutably, one `if let ... else { return None }` rewritten as `?`, and one `unwrap` in `ensure_object` turned into a scoped `expect` with the invariant spelled out — the case `AGENTS.md` explicitly permits. Doc links pointing at engine-internal paths are unlinked to prose, since this crate deliberately cannot see them. Acceptance, measured: `cargo tree -p tinymemory-sync` links zero of `tinycortex`, `rusqlite`, `tinymemory-core`, `tinymemory-api`. Core no longer names the engine's normalisers anywhere. The engine keeps its copy until tinyhumansai/tinycortex removes it; that side is a companion change, and the module is dead code there — its only remaining references are four doc links. Refs tinyhumansai#18 (§B3)
|
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]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
tinycortex#149 landed as a squash (8401346b), discarding the branch head this pin pointed at; 34cbb6c is diverged from tinycortex main rather than an ancestor of it. The merged commit is also the one that deletes the 33 duplicated files under api/src/, which is the state this stack depends on.
Contributor
Author
|
Superseded by #41, which merged as 5f9052e and carries this branch's work in full — its commit Checked before closing: the one file that looked unique to this branch ( §B3 is on Closing rather than merging: replaying it would re-apply changes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #18 §B3 — "Payload normalisers … are pure
Value → Valuetransforms with no engine dependency. Move them back into atinymemory-synccrate … so a non-TinyCortex engine gets Composio sync for free."They were not in this workspace at all. They lived inside the TinyCortex engine, and
tinymemory-corereached in throughtinycortex::memory::sync::composio::providers::normalize::*. A host binding a different memory engine therefore could not have Composio sync — despite none of this code caring which engine is bound. That is exactly the coupling §B3 names, and it ran through the engine rather than around it.tinymemory-syncis 15 files, 2,598 lines. It links no engine, no storage, no async runtime — and no contract either.Acceptance, measured
Two things found while moving
Both stated rather than smoothed over, because both would otherwise surprise someone later.
The crate is not quite "pure".
format_email_local_timerenders inchrono::Local, so it reads the host's timezone;notion::now_msreads the clock. Both are deliberate upstream — the agent presents local times without doing UTC arithmetic, and Notion payloads carry no ingestion timestamp — and the raw UTC field is preserved alongside, so sorting and deduplication stay UTC-based. Documented at the crate root and at each function, rather than left for whoever notices their output moves when they changeTZ.Two logging facades.
gmail_post_processtraces throughtracing,slack_post_processthroughlog. Both inherited. Preserved rather than unified, because §B3 is a move and swapping a facade changes where a host's log lines surface — a behaviour change hiding inside a relocation. Worth reconciling in its own change.What is not verbatim, and why
Four edits, all forced by this workspace's lints being stricter than the engine's gate reached:
unwraps removedif let … else { return None }→?clippy::question_markunwrap→ scopedexpectinensure_objectAGENTS.mdnames: "genuinely unreachable states — whereexpectmust carry a message explaining the invariant"The scoped
#[allow]is on one function with the invariant spelled out — not blanket, and every other inheritedunwrapwas removed rather than allowed.Public API changes
tinymemory-sync, a workspace membertinymemory-coregains a dependency on it and drops those reaches into the engineValidation
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-featuresRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-featurescargo run --example basic./scripts/ci/dependency-budget.shcargo test --libThe new crate carries 107 tests, moved with it.
Why this one first
§B3 is the cheapest probe of §A3's open boundary question.
tinymemory-coreis 217 files: 112 touch SQL or the engine, and 105 touch neither. If core really is two crates wearing one name, the 26 Composio files are the easiest 26 to prove it with — and success has a hard test the compiler enforces, rather than a judgement call.It worked, which is evidence for splitting the remaining 79 the same way rather than moving §A3's boundary in either direction.
Related
Part of #18 (§B3). Moves toward acceptance criterion 6 — "Composio sync completes end to end against a non-TinyCortex driver" — which is now a wiring question rather than a coupling one.