Drop the host-side Composio normalisers (tinymemory#18 §B3) - #153
Conversation
tinymemory#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." `providers::normalize` was fifteen files and 2,598 lines that this crate never called. Its only consumer was the host, reaching in through `tinycortex::memory::sync::composio::providers::normalize::*` — which meant a host binding a *different* memory engine could not have Composio sync at all, despite none of this code caring which engine is bound. That is the coupling §B3 names, and it ran through here. Verified dead before removing: the only references to `normalize::` outside the module itself were four doc-links in `common.rs`, plus one `use normalize::NormalizedView` in `store/safety/pii.rs` — which is unicode normalisation, an unrelated `normalize`. The four doc-links are kept and repointed. They explain that `common::pick_str` and the normalisers' `pick_str` deliberately differ — one resolves with `Value::pointer` and coerces numbers, the other walks with `Value::get` and does not — and that warning is worth more now that the two live in different crates than it was when they were siblings. The code now lives in `tinymemory-sync`, which depends on `serde_json`, two logging facades and `chrono`, and links no engine. Validation: - `cargo check --workspace --all-targets` — clean - `cargo test --workspace` — 1396 passed, 0 failed - `cargo fmt --all -- --check` — clean - `cargo clippy --all-targets -- -D warnings` (the CI gate) — clean - `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features` — clean Refs tinyhumansai/tinymemory#18 (§B3)
|
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 selected for processing (17)
💤 Files with no reviewable changes (16)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe PR removes the in-crate Composio normalization modules and their tests. It removes the public ChangesComposio normalizer removal
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR removes unused host-side normalization code and updates its documentation references without changing active behavior; no actionable merge-blocking risk remains after normal checks and review. 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}
How this change flows0 changed behaviours across 2 relationships. 3 surrounding behaviours are shown (60 graph nodes walked). 31 further behaviours left out to keep the diagram readable. flowchart LR
n0["ensure_object"]:::impacted
n1["post_process"]:::impacted
n2["reshape_fetch_history"]:::impacted
n1 -->|calls| n2
n2 -->|calls| 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
the
priority: p3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
The deletion itself is clean and the move is faithful — I verified both independently. The problem is sequencing: this can't land before tinymemory#40.
Blocking: the dependency runs the other way from what the body says
tinymemory main still has 9 live imports into the module this PR deletes:
core/src/sync/composio/providers/helpers.rs:8—use crate::engine::backend::sync::composio::providers::normalize::helpers::pick_strcore/src/sync/composio/providers/mod.rs:284—pub(crate) use …normalize::helpers::pick_str- one
use …normalize::<provider>each inproviders/{gmail,linear,clickup,notion,github,slack}/mod.rs
core/Cargo.toml:24 declares tinycortex = { version = "0.1", features = [… "sync"] }, and .gitmodules pins vendor/tinycortex with branch = main. So the moment this lands on tinycortex main and that gitlink is bumped, tinymemory main stops compiling in 8 files. That's not hypothetical — the submodule tracks main.
The "Merge order" section argues that "tinymemory#40 … does not depend on this merging". That's true and it's the wrong direction — this PR depends on #40, and the section never mentions the tinymemory main edge at all. It's the one question the PR needed to answer.
Major: the destination isn't close to landing. tinymemory#40 is OPEN (mergedAt: null) and its own body says "Stacked on #19 → … → #39" — roughly 21 PRs deep. So this isn't a land-them-together-this-afternoon pair; the window where the code exists nowhere consumable could be weeks.
Suggested order
1. tinymemory #19 → … → #39 (unblock the stack)
2. tinymemory #40 ← must land first: creates tinymemory-sync,
repoints all 9 refs off tinycortex
3. tinycortex #153 ← this PR, safe only once (2) is on tinymemory main
4. openhuman: bump vendor/tinycortex + vendor/tinymemory gitlinks together,
in ONE commit — bumping either alone breaks the build
Nothing needs to land in tinycortex first; the blocker is entirely in tinymemory. Step 4 is a real constraint rather than a formality, since openhuman vendors both.
Simplest resolution: hold this behind #40 and replace the "Merge order" section with an explicit Blocked by tinymemory#40. If you'd rather decouple the repos sooner, the alternative is to deprecate instead of delete — keep pub mod normalize for one release as #[deprecated] re-exports of tinymemory_sync::*, bump to 0.2.0, delete once openhuman's gitlinks have moved. That gets the one-home goal without a broken-main window. Either path works; the current one doesn't.
What I verified and found clean
Content parity is complete, including every test file — this was the risk worth checking and it's spotless. All 15 files landed as sync/src/ with a new sync/Cargo.toml (name = "tinymemory-sync", publish = false). Test bodies go 1,181 → 1,216 lines, a uniform +5 per file (module header), i.e. zero test loss:
| file | tinycortex | tinymemory | Δ |
|---|---|---|---|
| clickup / _tests | 133 / 96 | 133 / 101 | 0 / +5 |
| github / _tests | 130 / 118 | 130 / 123 | 0 / +5 |
| gmail_post_process / _tests | 489 / 354 | 503 / 359 | +14 / +5 |
| helpers / _tests | 50 / 35 | 50 / 40 | 0 / +5 |
| linear / _tests | 157 / 184 | 157 / 189 | 0 / +5 |
| notion / _tests | 115 / 137 | 120 / 142 | +5 / +5 |
| slack_post_process / _tests | 320 / 257 | 323 / 262 | +3 / +5 |
| mod.rs → lib.rs | 23 | 38 | +15 |
The four source deltas are exactly the edits #40 documents (two unwraps removed, if let…else → ?, one scoped expect, doc links unlinked to prose).
The "never called internally" claim holds. I grepped the PR-head tree for providers::normalize and all seven normalize::<module> paths — exactly one hit, the doc-prose line at common.rs:17. No re-exports, no feature-gated call sites, nothing in examples/. The NormalizedView hit in store/safety/pii.rs is genuinely unrelated (unicode normalisation). The openhuman tests/raw_coverage/* hits for gmail_post_process/slack_post_process are false leads — memory_threads_raw_coverage_e2e.rs:70 imports openhuman's own provider, not this normaliser. openhuman and opencompany are both clean of direct references; their only exposure is the vendored tinymemory copy.
common.rs:7-19 is good. The two intra-doc links were correctly converted to plain backtick prose rather than left as links that would silently degrade to text across a crate boundary, and tinymemory_sync::helpers::pick_str is a real path (#40's sync/src/lib.rs exports pub mod helpers). The pick_str divergence warning — Value::pointer with numeric coercion vs Value::get returning None on non-string leaves — is preserved verbatim and correctly re-scoped. Agreed that it's worth more now the two live in different crates.
Smaller things
- Breaking public-API removal with no version bump:
providers/mod.rs:11waspub mod normalize;and the deletednormalize/mod.rshad 7 ×pub mod.Cargo.toml:17stays0.1.1, and consumers requestversion = "0.1"(>=0.1.0, <0.2.0), so cargo treats a semver-breaking change as range-compatible. Mitigated in practice since tinycortex isn't on crates.io and every consumer takes it by path/git/submodule — so this is discipline rather than an outage. Worth either a0.2.0bump or a note that 0.x discipline is deliberately skipped here. - The validation table overstates what it proved.
memory/mod.rs:89gates the whole tree behind#[cfg(feature = "sync")]anddefault = [], socargo check --workspace --all-targets, the 1396-test run, andclippy --all-targetsall ran with the deleted code compiled out, and thecomposio_sync_mock/_livetargets carryrequired-features = ["sync"]so they were skipped. Only thecargo doc --all-featuresrow actually compiled the affected tree. Not a real risk —ci.yml:43,46,49runsbuild/test/docwith--all-featuresand CI is green — but the table reads stronger than it is. - Worth fixing on the other side: tinymemory#40's body names "Companion engine-side removal: tinycortex#150", but #150 is
build(deps): bump rusqlite(dependabot). The real companion is this PR. Anyone tracking the pair lands on a dependabot PR.
For what it's worth on the two green approvals: CodeRabbit generated no comments and tinysweeper reported an intra-repo call graph ("0 changed behaviours across 2 relationships, 60 graph nodes walked"). Neither can see tinymemory, so neither approval speaks to the only risk this PR carries.
|
@oxoxDev — the blocking finding was correct, and it is now satisfied. It also raced the fix by about 90 seconds, which is worth stating plainly rather than letting the timestamps imply the review was wrong:
So The 9 imports are gone. On tinymemory All six Your suggested order was followed, via the stack tips rather than every PR: #32 (A1+A2+C1, Step 4 is recorded in the PR body. You are right that it is a real constraint: openhuman vendors both repos, so Validation table — accepted and corrected. Version bump — taking the note, not the bump, and the reason is stronger than discipline. A The wrong-companion reference (tinymemory#40's body naming #150, a dependabot rusqlite bump) — noted; #40 is closed now, and this PR's body names the relationship explicitly from this side. On the two green approvals: agreed, and worth repeating — neither CodeRabbit nor tinysweeper can see tinymemory, so neither spoke to the only risk this PR carried. Yours was the review that caught it. |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Re-reviewed. The blocker is genuinely resolved — verified independently, not taken on the reply.
On tinymemory main at 5f9052e (the #41 merge):
| Claim | Result |
|---|---|
the 9 normalize:: imports are gone |
0 refs in core/src |
repointed through tinymemory_sync |
exactly 8 files — the same eight that carried the imports |
tinymemory-sync exists |
15 files in sync/src, name = "tinymemory-sync", publish = false, 0.1.0 |
So the deletion no longer strands anything. Thanks for the timeline note — #41 merged at 11:35:16Z and my review posted at 11:36:48Z, so it was accurate when written and stale ~92 seconds later. The mergedAt: null on #40 was the confusing part: closed-as-subsumed reads identically to never-landed through the API, and I'd have drawn the same wrong conclusion again. Worth knowing for the rest of the stack, since #30/#31/#38/#40 are all in that state.
Taking back the version-bump note — your reasoning is right and mine was incomplete. I checked the patch wiring: root Cargo.toml:141 redirects tinycortex to path = "vendor/tinycortex", and four sites declare version = "0.1" against it (core/Cargo.toml:29 and :30, adapters/tinycortex/Cargo.toml:24, crates/tinymemory-module/Cargo.toml:39). A patched crate still has to satisfy the requirement, so 0.2.0 makes all four unsatisfiable simultaneously — it would manufacture the exact coordinated break I was warning about, not document it. publish = false on both crates plus the #149 precedent settles it. Withdrawn.
The validation table now says which rows ran with the tree compiled out and points at ci.yml:43,46,49 plus the --no-default-features --features sync lane at ci.yml:68 — that's the right correction, and the Features (sync) lane is green on this head. Merge order and the openhuman both-gitlinks-in-one-commit constraint are recorded in the body. Useful addition that openhuman's own tinycortex::memory::sync:: uses are StatusListResponse / list_sync_statuses / SyncState, none of which this touches.
Everything I verified in the first pass still holds: faithful move with zero test loss (uniform +5 per file, just a module header), no internal call sites, and the common.rs doc-link conversion to plain prose with the pick_str divergence warning preserved.
Approving. Remaining sequencing is step 4 — openhuman bumping vendor/tinycortex and vendor/tinymemory in a single commit.
2 participants
Companion to tinyhumansai/tinymemory#40, which is where this code now lives.
Summary
tinymemory#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."providers::normalizewas 15 files and 2,598 lines that this crate never called. Its only consumer was the host, reaching in throughtinycortex::memory::sync::composio::providers::normalize::*— which meant a host binding a different memory engine could not have Composio sync at all, despite none of this code caring which engine is bound.Verified dead before removing
The only references to
normalize::outside the module itself:providers/common.rsuse normalize::NormalizedViewinstore/safety/pii.rs— which is unicode normalisation, an unrelatednormalizeNo call sites. The module was host-facing surface sitting inside the engine.
The doc links are kept, and repointed
common::pick_strand the normalisers'pick_strdeliberately differ — one resolves withValue::pointerand coerces numbers, the other walks withValue::getand returnsNonefor non-string leaves. That warning is worth more now the two live in different crates than it was when they were siblings, so the note stays and names the crate instead of a sibling module.Validation
cargo check --workspace --all-targetscargo test --workspacecargo fmt --all -- --checkcargo clippy --all-targets -- -D warnings(theci.ymlgate)RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-featuresCaveat, raised in review and correct:
default = []andmemory/mod.rs:89gates thiswhole tree behind
#[cfg(feature = "sync")], so the three rows marked above ran with thedeleted code compiled out — and the
composio_sync_mock/composio_sync_livetargetscarry
required-features = ["sync"], so they were skipped entirely. Only thecargo doc --all-featuresrow actually compiled it locally.The coverage is real, it just comes from CI rather than from those local commands:
ci.yml:43,46,49runbuild/test/docwith--all-features, and the feature matrixat
ci.yml:68runs a dedicated--no-default-features --features synclane. Both are green.Merge order — this PR is blocked by tinymemory#40, now satisfied
The original wording here argued that tinymemory#40 does not depend on this merging. That
was true and it answered the wrong direction: this PR depends on #40, and the section
never mentioned the tinymemory
mainedge, which was the one question it needed to answer.Raised in review, and correct.
The dependency was real. tinymemory
mainheld 9 live imports into the module this PRdeletes (
normalize::helpers::pick_strfromproviders/helpers.rsandproviders/mod.rs,plus one
normalize::<provider>each in gmail / linear / clickup / notion / github / slack).core/Cargo.tomltakestinycortexwith thesyncfeature and.gitmodulespinsvendor/tinycortexwithbranch = main, so landing this before #40 would have brokentinymemory
mainin 8 files on the next gitlink bump.That precondition is now met. #40's work reached tinymemory
mainviatinymemory#41 (merged
5f9052e), whichcarries #40's commit
da888f7verbatim. On tinymemorymaintoday:git grep normalize:: -- core/src/→ 0 files (all 9 imports gone)tinymemory_syncsync/, and its dependency budget reports20 crates with no engine — CI there fails if it ever reaches
tinycortex|rusqlite|libsqlite|tinymemory-core|tinymemory-apiSo the code has a consumable home, and this removal no longer opens a broken-main window.
The
#[deprecated]re-export alternative suggested in review is no longer needed.Still independent of #149 — different
files, no overlap.
After this lands: openhuman must bump both gitlinks in one commit
openhuman vendors both repos. Bumping
vendor/tinycortexpast this commit without alsobumping
vendor/tinymemorypast5f9052ewould leave openhuman's tinymemory copy importinga module this PR deleted. Neither bump is safe alone. (openhuman is otherwise unaffected: it
never referenced these normalisers directly — its
tinycortex::memory::sync::uses areStatusListResponse,list_sync_statusesandSyncState, none of which this PR touches.)Version discipline: staying at
0.1.1, deliberatelyThis removes public API (
providers/mod.rspub mod normalize;and 7pub modin thedeleted
normalize/mod.rs) without a version bump — raised in review as worth either a0.2.0bump or an explicit note. Taking the note, because the bump is the more dangerousoption here:
publish = false; nothing resolves tinycortex from crates.io.version = "0.1"and redirects it with[patch.crates-io]to apath — tinymemory
core/Cargo.toml:29-30,adapters/tinycortex/Cargo.toml:24, andopenhuman
Cargo.toml:212. A patched crate must still satisfy the version requirement,so bumping to
0.2.0makes all four unsatisfiable at once and requires a simultaneouscross-repo edit — the same coordinated-break shape the section above warns about.
tinycortex-api, a strictly larger breaking removal,and also stayed at
0.1.1.Summary by CodeRabbit
Refactor
Documentation