Uh oh!
There was an error while loading. Please reload this page.
Serve the Composio connection bootstrap over the bus - #105
Conversation
`ComposioProvider::on_connection_created` is the last thing in the memory seam that a host can only reach by naming the engine. OpenHuman's connection-created subscriber builds a `tinymemory_core` `ProviderContext` purely to hand it to that hook, which is why `memory/sync/composio/ providers/context_ext.rs` still imports the engine crate — and why the crate cannot leave the product dependency graph (openhuman#5560). `BootstrapConnection(toolkit, connection_id)` closes it. It sits in `MemorySourceSync` beside `RunConnectionSync`, and deliberately not inside it: a sync moves items and runs many times, a bootstrap establishes what a sync then assumes and runs once. They also fail differently, and a caller can only decline to stop syncing over a failed bootstrap if it can tell the two apart. Caps are left at their defaults rather than accepted as arguments. No provider overrides the hook today and the default impl reads neither `max_items` nor `sync_depth_days` — they cap how much a *sync* walks, and giving a bootstrap a walk budget would imply it walks. The registry init is the part worth reviewing. `get_provider` resolves through a process-global that the *host's* boot used to fill; a `cdylib` has its own statics, so inside the module that registry is empty and `get_provider` answers `None` rather than erroring — every bootstrap would have reported "no composio provider registered" over a working connection, with nothing in a build or a type check saying so. The module now calls `init_default_providers` beside the other seams it installs for exactly this reason. The existing sync pipeline resolves its provider a different way and never depended on it, so this is a new requirement rather than a fix. Two tests, because the failure is invisible to compilation: a unit test that the registry really registers what it claims, and an E2E against the dlopen'd artifact asserting `BootstrapConnection` fails because no client resolves and never because the registry is empty. Those two are one line apart in the engine and worlds apart in meaning. Note for reviewers: `cargo clippy --workspace --all-targets -- -D warnings` is already red on main with six `unwrap`/`unwrap_err` findings in `crates/tinymemory-documents/src/convert/test.rs`. Untouched here.
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Warning Your free Security trial is over. An organization admin can activate billing to continue. 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 flows3 changed behaviours across 10 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 30 further behaviours left out to keep the diagram readable. flowchart LR
n0["...hod_fails_with_its_advertised_family_name<br/>changed"]:::changed
n1["setup<br/>changed"]:::changed
n2["portability_and_lifecycle_round_trip<br/>changed"]:::changed
n3["block_on"]:::impacted
n4["install"]:::impacted
n5["assert"]:::impacted
n6["assert_unsupported"]:::impacted
n7["call"]:::impacted
n8["ModuleConfig"]:::impacted
n0 -->|calls| n3
n0 -->|tests| n3
n0 -->|calls| n5
n0 -->|calls| n6
n0 -->|tests| n6
n1 -->|uses| n8
n2 -->|calls| n5
n2 -->|calls| n7
n2 -->|tests| n7
n4 -->|uses| n8
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. |
The loader E2E required `BootstrapConnection` to fail, on the assumption that a temp workspace has no Composio configured. It does not fail: the module's proxied `ComposioHost` answers through the test harness and the default bootstrap is content with that, so the call returns `Ok` and `expect_err` panicked in CI. Asserting the symptom instead of the mechanism made the test wrong about the one thing it exists to pin. It now accepts either outcome and rejects exactly one: an error naming an empty provider registry. `Ok` means the provider resolved and its bootstrap ran; any other error means it resolved and the run failed on its own terms. Mutation-tested rather than assumed, against a built cdylib with TINYMEMORY_TEST_MODULE set — without that variable the harness aborts before reaching the assertion, which is how the first local check passed judgement on nothing: init present -> pass init removed -> fail, "no composio provider registered for 'gmail'"
Uh oh!
There was an error while loading. Please reload this page.
What
Adds one member,
BootstrapConnection(toolkit, connection_id), toMemorySourceSync.Why
ComposioProvider::on_connection_createdis the last thing in the memory seam a host can only reach by naming the engine. OpenHuman's connection-created subscriber builds atinymemory_coreProviderContextfor the sole purpose of handing it to that hook:That is why
memory/sync/composio/providers/context_ext.rsstill imports the engine crate, and why the crate cannot leave the shipped dependency graph (tinyhumansai/openhuman#5560). It is the last of six remaining references there that needs anything from this repo — the other five are host-side work against members this repo already serves.Design notes
Beside
RunConnectionSync, not inside it. A sync moves items and is expected to run many times; a bootstrap establishes what a sync then assumes and is expected to run once. Folding them together would either re-register triggers on every sync or leave a connection whose first sync silently has no profile behind it. They also fail differently, which is the more practical reason: a bootstrap failure should not stop items syncing, and a caller can only make that choice if it can tell the two apart.No cap arguments.
max_items/sync_depth_daysare left at their defaults. No provider overrides the hook today, and the default impl reads neither — they bound how much a sync walks, and giving a bootstrap a walk budget would imply it walks. Same reasoningRunConnectionSyncalready documents for not carrying budgets.Defaulted on the trait, so this is additive for any other implementor; the null driver takes the default and answers
Unsupported.The part worth reviewing: the provider registry
get_providerresolves through a process-global. The host's boot is what callsinit_default_providers— nothing in this workspace does. Acdylibhas its own statics, so inside the module that registry is empty, andget_provideranswersNonerather than erroring. EveryBootstrapConnectionwould have reportedover a perfectly good connection, and nothing in a build, a type check, or the module's own unit tests would have caught it — they all run in a process the host has already initialised.
The module now calls
init_default_providersbeside the other seams it installs at startup, for exactly that reason. The existing sync pipeline resolves its provider a different way and never depended on this, so it is a new requirement rather than a fix to something broken.Tests
Two, because the failure mode is invisible to compilation:
init_default_providersreally registers what it claims, asserting a known toolkit and that an unregistered one stays unregistered, so it cannot pass against a registry that returns something for everything;dlopen'd artifact assertingBootstrapConnectionfails because no Composio client resolves and never because the registry is empty. Those two outcomes are one line apart in the engine and worlds apart in what they mean.Plus the null-driver
Unsupportedcase and the service-level family refusal, matching the siblings.cargo testis green in both workspaces (the root andcrates/tinymemory-module).Note for reviewers
cargo clippy --workspace --all-targets -- -D warningsis already red onmain: sixunwrap/unwrap_errfindings incrates/tinymemory-documents/src/convert/test.rs. Untouched by this branch.After this merges
A release and a registry re-pin, then the OpenHuman side is a single PR.
is_compatiblehas no call sites and this member joins an already-advertised family, so a host calling it against an older pinned artifact getsUnknownMethodat runtime rather than a capability refusal — the pin has to move before the host asks.