fix(memory): bridge the four defaulted module members instead of refusing - #5808
Conversation
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe memory provider now forwards diagnosis and source-sync operations through the module bus. Coding-session ingestion uses a budget-based timeout with a 30-second grace period. A disabled-host regression test covers the forwarded methods. ChangesMemory provider forwarding and ingestion timing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This is a localized bridge fix that routes four previously refused calls to the module; the reported tests and checks are clean, and no actionable merge-blocking risk remains beyond normal review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The PR implements the three required ModuleMemoryProvider source-sync bridge calls from issue Full details: Out of Scope Changes checkExplanation The PR includes changes outside issue Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9a89b2b84
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| module_call!(self, "purge_all", "PurgeAll", ()) | ||
| } | ||
| async fn diagnose(&self) -> Result<Diagnosis, MemoryError> { | ||
| module_call!(self, "diagnose", "Diagnose", ()) |
There was a problem hiding this comment.
The four new dispatches hard-code "Diagnose", "RunSourceSync", "BootstrapConnection", and "IsToolkitSyncable" even though these members have canonical constants in the module contract. The disabled-host test fails before .call(...), so it cannot detect a misspelling or an upstream rename; such drift will compile successfully and surface only as MemberNotFound in deployed module-backed flows. Import and use the contract constants for all four calls instead.
AGENTS.md reference: AGENTS.md:L241-L243
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…sing `MemorySourceSync::run_source_sync`, `bootstrap_connection` and `is_toolkit_syncable`, and `MemoryMaintenance::diagnose`, each carry a default body that returns `Unsupported`. `ModuleMemoryProvider` implemented every required member and silently inherited all four defaults, so each call was answered by the default rather than dispatched to the module. A defaulted trait member cannot break an implementor at compile time, which is why this was invisible: when the members were added to the contract the bridge kept compiling and began refusing at runtime. The pinned artifact serves all four. They are declared in the module's METHODS list (tinymemory-module/src/lib.rs:774) and their wire names are canonical bus constants (tinymemory-bus/src/names.rs:287-300). The host simply never sent the call: before this change `grep -rn RunSourceSync src/` returned nothing. Symptom: the manual "Sync now" / "All in" button failed for every source with `unsupported capability: source_sync`, while the module's own scheduler kept ingesting normally because it runs inside the module and never crosses this bridge. The capability list is untouched. It was already correct: `as_source_sync()` returned `Some` and the refusal came one call later. Closes tinyhumansai#5801
f9a89b2 to
4404c8f
Compare
|
Rebased onto What conflicted: only But #5803 surfaces a real problem with
|
| source | items | duration |
|---|---|---|
Gmail ca_ml7K8z4dTbIB |
250 | 1m 24s |
GitHub ca_tnftuGIRcr_I |
979 | 1m 17s |
Both roughly 3x the 30 s default. And unlike ingest_coding_sessions, source sync has no RPC-level budget at all — neither sync_rpc (sources/rpc.rs:563) nor apply_all_in_rpc (:1023) wraps the call in tokio::time::timeout. The flat 30 s is the only deadline in the system.
So once this PR lands, a real Sync will dispatch to the module, the module will sync correctly, and the caller will be released at 30 s with a timeout error while the work completes at ~80 s. That is #5802's shape exactly, and #5803's own comment says why it cannot be waved off: "A timeout does not cancel the remote work — tinybus cannot — it stops waiting and frees the caller."
This is still strictly better than today (the sync currently never runs at all), so it is not a reason to hold this PR. But it means clicking Sync may still show a failure for a sync that succeeded, and whoever verifies should expect that rather than read it as this fix not working.
I did not fix it here on purpose. The fix needs a budget to derive from, and there is no ingest_budget equivalent for source sync — sizing one means deciding how long a user should wait before being told a sync is still running, which is a product call, not a transcription. Inventing a constant tuned to the two durations above would be a heuristic fitted to one session's data.
Recommended shape, mirroring what #5803 established:
- a
sync_budget()inmemory::sources::rpcsized to the work (source kind and/or item count), used as the RPC-leveltokio::time::timeoutso the clean structured message wins; run_source_sync's bus deadline derived assync_budget() + a grace, reusingINGEST_BUS_GRACEor a sibling, so the two cannot drift.
bootstrap_connection plausibly needs the same treatment (it fetches a profile and registers triggers). is_toolkit_syncable and diagnose are quick reads and are fine on the default.
Happy to file this as its own issue, or to implement it here if you would rather it ship together — say which.
|
@coderabbitai review |
|
…memory sources Two e2e gaps found in the coverage audit of recently merged PRs. Both paths could break completely today without a single lane going red. tinyhumansai#5808 / tinyhumansai#5801 — `MemorySourceSync::run_source_sync` is a DEFAULTED contract member. `ModuleMemoryProvider` inherited its `Unsupported` body instead of bridging, so "Sync now" answered `unsupported capability: source_sync` on a build whose module could sync fine. A defaulted member that was never bridged is indistinguishable from a bridged one at compile time, which is why it shipped — so the new test asserts the RUNTIME answer. The discriminator: `binding::build` binds `module_provider` whenever the `modules` feature is on, so this RPC really does reach the bridged member. An unbridged member refuses the capability BEFORE any transport is attempted; a bridged one gets as far as the module. Those two failures differ in the message, and only the second is correct. tinyhumansai#5725 — `reset_tree` and `flush_now` were routed through `Maintenance::reset_derived_index` / `flush_pending`. Nothing exercised either afterwards: in the raw-coverage lane both names appear only as string literals fed to `memory::schema::schemas(...)`, and in `worker_c_modules_e2e.rs` they sit in a 68-method loop whose helper passes on an error response. The new test drives both RPCs and asserts they reach a driver that serves Maintenance, keying on the host-side `"does not serve Maintenance"` refusal — the one failure produced before any driver call, so it is what proves the hop happened. Neither test asserts success. That would need a live module artifact fetched over the network, which this lane must not depend on; the bugs these pin were never "wrong data" but "the call is refused before it is attempted". Revert-checked, both: run_source_sync bridge removed -> FAILED at memory_sources_e2e.rs:952, "Got: unsupported capability: source_sync" (the tinyhumansai#5801 string verbatim) as_maintenance() -> None -> FAILED at memory_sources_e2e.rs:1029, "Got: flush_now: driver 'tinymemory' does not serve Maintenance" Both restored afterwards; the fix files are byte-identical to main.
…memory sources Two e2e gaps found in the coverage audit of recently merged PRs. Both paths could break completely today without a single lane going red. tinyhumansai#5808 / tinyhumansai#5801 — `MemorySourceSync::run_source_sync` is a DEFAULTED contract member. `ModuleMemoryProvider` inherited its `Unsupported` body instead of bridging, so "Sync now" answered `unsupported capability: source_sync` on a build whose module could sync fine. A defaulted member that was never bridged is indistinguishable from a bridged one at compile time, which is why it shipped — so the new test asserts the RUNTIME answer. The discriminator: `binding::build` binds `module_provider` whenever the `modules` feature is on, so this RPC really does reach the bridged member. An unbridged member refuses the capability BEFORE any transport is attempted; a bridged one gets as far as the module. Those two failures differ in the message, and only the second is correct. tinyhumansai#5725 — `reset_tree` and `flush_now` were routed through `Maintenance::reset_derived_index` / `flush_pending`. Nothing exercised either afterwards: in the raw-coverage lane both names appear only as string literals fed to `memory::schema::schemas(...)`, and in `worker_c_modules_e2e.rs` they sit in a 68-method loop whose helper passes on an error response. The new test drives both RPCs and asserts they reach a driver that serves Maintenance, keying on the host-side `"does not serve Maintenance"` refusal — the one failure produced before any driver call, so it is what proves the hop happened. Neither test asserts success. That would need a live module artifact fetched over the network, which this lane must not depend on; the bugs these pin were never "wrong data" but "the call is refused before it is attempted". Revert-checked, both: run_source_sync bridge removed -> FAILED at memory_sources_e2e.rs:952, "Got: unsupported capability: source_sync" (the tinyhumansai#5801 string verbatim) as_maintenance() -> None -> FAILED at memory_sources_e2e.rs:1029, "Got: flush_now: driver 'tinymemory' does not serve Maintenance" Both restored afterwards; the fix files are byte-identical to main.
…memory sources Two e2e gaps found in the coverage audit of recently merged PRs. Both paths could break completely today without a single lane going red. tinyhumansai#5808 / tinyhumansai#5801 — `MemorySourceSync::run_source_sync` is a DEFAULTED contract member. `ModuleMemoryProvider` inherited its `Unsupported` body instead of bridging, so "Sync now" answered `unsupported capability: source_sync` on a build whose module could sync fine. A defaulted member that was never bridged is indistinguishable from a bridged one at compile time, which is why it shipped — so this asserts the RUNTIME answer. The discriminator: `binding::build` binds `module_provider` whenever the `modules` feature is on, so this RPC really does reach the bridged member. An unbridged member refuses the capability BEFORE any transport is attempted; a bridged one gets as far as the module. tinyhumansai#5725 — `reset_tree` and `flush_now` were routed through `Maintenance::reset_derived_index` / `flush_pending`. Nothing exercised either afterwards: in the raw-coverage lane both names appear only as string literals fed to `memory::schema::schemas(...)`, and in `worker_c_modules_e2e.rs` they sit in a 68-method loop whose helper passes on an error response. This commit also carries the two review findings raised on the PR, which were both correct and were fixed in a follow-up now folded in by the rebase: - `sources_sync_...` excluded only "unsupported capability" and "source_sync". The other pre-dispatch refusal — `sync_rpc` bailing with "the bound memory driver '<id>' does not serve source sync" (`memory/sources/rpc_part_01.rs:560-564`) — spells it with a SPACE, so it matched neither string and the test passed green while `run_source_sync` was never reached. Now rejected. Proven: forcing `ModuleMemoryProvider::as_source_sync()` to `None` fails the new assertion with the real message; before the change that same revert passed. - `tree_reset_and_flush_...` excluded one string, so a renamed or removed RPC answered `unknown method: <name>`, contained no "does not serve Maintenance", and passed without dispatching. Now rejected, plus a check that the response is a result or an error rather than neither. Neither test asserts success. That would need a live module artifact fetched over the network, which this lane must not depend on; the bugs these pin were never "wrong data" but "the call is refused before it is attempted". Rebased onto edee560. The conflict with the merged relative-folder-path tests (tinyhumansai#5959) was textual, not semantic: both sides append independent tests to the tail of this file and share the same setup boilerplate, which is what git interleaved. Resolved by taking main's file whole and appending these two tests, so both sets survive intact.
…ridge-source-sync\n\nfix(memory): bridge the four defaulted module members instead of refusing\n
2 participants
Summary
module_call!arms the host's module bridge was missing, so they dispatch to the module instead of being answered by a refusing trait default.unsupported capability: source_syncwhile scheduled sync worked normally.run_source_sync,bootstrap_connection,is_toolkit_syncableanddiagnose.ARTIFACT_CAPABILITIESorassume_full_capabilities. The capability list was already correct; changing it would have papered over the real gap.Problem
MemorySourceSyncandMemoryMaintenancecarry members with default bodies that unconditionally refuse:ModuleMemoryProviderimplemented every required member and silently inherited the defaults. A defaulted trait member cannot break an implementor at compile time, so when these were added to the contract the bridge kept compiling and began refusing at runtime.The refusal is raised inside the host and never reaches the wire. In the reported session three sources refused in 13ms total, while a genuine module round-trip in the same session took 30s to time out.
The artifact was never the problem. All four are declared in the module's
METHODSlist (tinymemory-module/src/lib.rs:770-776) and their wire names are canonical bus constants (tinymemory-bus/src/names.rs:287-300). Before this change:Solution
Four
module_call!arms following the pattern the neighbouring members already use, placed to mirror the trait's own member order:MemorySourceSync::run_source_syncRunSourceSyncsync.rs:153MemorySourceSync::bootstrap_connectionBootstrapConnectionsync.rs:193MemorySourceSync::is_toolkit_syncableIsToolkitSyncablesync.rs:227MemoryMaintenance::diagnoseDiagnoserecords.rs:470Plus the
Diagnosisimportdiagnoseneeds. That is the whole production change.How the siblings were found
Rather than guess, I diffed every trait member carrying a refusing default against what the bridge implements. 17 members across 8 traits have such a default; 13 are implemented; these 4 were not. The other three defaulted
MemorySourceSyncmembers andMemoryMaintenance::diagnosewere the complete set — no others need a decision.Blast radius beyond the reported button
All four have live host call sites, all dead before this change:
run_source_sync—memory/sources/rpc.rs:574(per-source Sync) andrpc.rs:1055(Sources → All in). The reported symptom.is_toolkit_syncable—memory/sync/composio/bus.rs:137. The host could not determine whether a toolkit was syncable.bootstrap_connection—memory/sync/composio/bus.rs:811. First-time bootstrap of a newly authorised connection failed silently.diagnose— the typed maintenance diagnosis an operator or agent reads.Testing
The test asserts dispatch, not a live module
With the module host disabled,
proxy()fails withMemoryError::Other(memory.rs:342-357), while an unbridged member returnsMemoryError::Unsupported. Asserting "notUnsupported" therefore asserts that the call went throughmodule_call!at all — which is precisely the thing that was missing — and needs no module, no network and no artifact.Revert-check, one member at a time
A single combined revert would stop at the first assertion and prove only one member. Each of the four was reverted individually and the failure names that member:
run_source_syncrun_source_sync answered from the trait default ... Got Unsupported { capability: "source_sync" }bootstrap_connectionbootstrap_connection answered from the trait default ... Got Unsupported { capability: "source_sync" }is_toolkit_syncableis_toolkit_syncable answered from the trait default ... Got Unsupported { capability: "source_sync" }diagnosediagnose answered from the trait default ... Got Unsupported { capability: "maintenance" }diagnosereportingmaintenancerather thansource_syncis the check that the test discriminates per member rather than catching one capability generically. Therun_source_syncfailure reproduces the production error string exactly.With the fix in place all four pass, and
openhuman::modules::is green at 79 passed / 0 failed.cargo fmt --checkandcargo clippy --no-deps --libare clean.The trap: no lane can catch this end to end
CI cannot prove this fixed. The lanes run with no module or with a test artifact, which is exactly why the bug reached a user. The test above proves the dispatch exists; it cannot prove the module answers it. The only end-to-end proof is the real app, and the maintainer can confirm it by clicking Sync on Brain → Sources and seeing a non-zero
sync_triggeredin[memory_sources] apply_all_in_rpc: complete.Impact
Restores the manual sync trigger, Composio toolkit-syncability checks, first-time connection bootstrap, and the typed maintenance diagnosis. No behaviour change for anything already working: scheduled sync runs inside the module and never crossed this bridge.
Left for a separate decision, deliberately not in this PR
1.
GuardedSourceSyncandGuardedMaintenancehave the identical hole in all four members. It is latent, not reachable: every call site resolves throughbinding.provider(), which returns the unguarded provider (binding.rs:93, the same field asunguarded_provider), so no current caller reaches it.I did not fix it here because the guard's forwards are not mechanical — each one must choose
admit_writeoradmit_read, and that is a security-tier decision, not a transcription. Getting it wrong lets areadonlyoperator trigger a sync that spends money. The current behaviour fails closed (denies), so leaving it is safe; adding a forward with the wrong tier would fail open. My reading, for whoever takes it:run_source_syncandbootstrap_connectionare writes (both ingest or persist),is_toolkit_syncableanddiagnoseare reads.2. The defaults themselves are the root cause and will hide the next member. The durable fix is to remove the default bodies from these members so the compiler names every implementor that is missing one. That is a breaking trait change in
tinymemory-apitouching every implementor, so it belongs in its own change with its own review.3. Nothing in CI compares the artifact's
METHODSlist against the host'smodule_call!arms. A test doing that would have caught all four statically.is_compatiblehas no call sites (grep -rn is_compatible src/returns two doc comments), but note it would not have caught this: it compares host and artifact capability sets that here already agreed.Related
feat/5560-queue-and-recall-through-the-contract), which routed the manual trigger onto the contract and moved the pin so the module serves it, but did not add the host-side arm.Submission Checklist
the_defaulted_members_dispatch_to_the_module_instead_of_refusingcovers all four members. The failure path is the per-member revert-check table above.module_call!arms plus one import; all four are executed by the new test, which fails if any is removed.Closes #5801.Impact (platform)
Desktop. No migration, no schema change, no config change.
AI Authored PR Metadata
Linear Issue
Commit & Branch
fix/5801-module-bridge-source-syncf9a89b2b8Validation Run
pnpm --filter openhuman-app format:check— no frontend files changed.pnpm typecheck— no TypeScript changed.cargo test -p openhuman --lib -- openhuman::modules::— 79 passed, 0 failed. Plus the four individual revert-checks above.cargo fmt -p openhuman -- --checkclean;cargo clippy -p openhuman --no-deps --libclean.Validation Blocked
command:end-to-end verification against the real pinned artifacterror:not blocked by a failure — no lane loads the real module, by designimpact:the dispatch is proven by test; that the module answers it is provable only in the running app. See the trap note above.Behavior Changes
Unsupported.Parity Contract
as_source_sync()already returnedSomeand still does. Only the member dispatch changed.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests