test(e2e): backfill coverage for the memory module-dispatch and maintenance-contract hops - #5973
Conversation
How this change flows0 changed behaviours across 12 relationships. 6 surrounding behaviours are shown (34 graph nodes walked). 23 further behaviours left out to keep the diagram readable. flowchart LR
n0["join"]:::impacted
n1["memory_sources_github_repo_activity_flow"]:::impacted
n2["memory_sources_composio_registry_flow"]:::impacted
n3["memory_sources_crud_and_folder_read_flow"]:::impacted
n4["serve"]:::impacted
n5["memory_sources_validation_rejects_bad_input"]:::impacted
n1 -->|calls| n0
n1 -->|tests| n0
n1 -->|calls| n4
n2 -->|calls| n0
n2 -->|tests| n0
n2 -->|calls| n4
n3 -->|calls| n0
n3 -->|tests| n0
n3 -->|calls| n4
n5 -->|calls| n0
n5 -->|tests| n0
n5 -->|calls| n4
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]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change adds end-to-end regression tests for folder-source path resolution, source synchronization dispatch, and tree maintenance RPC routing. ChangesMemory dispatch contracts
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This test-only change adds focused coverage without altering runtime behavior, and no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: 4490739d25
ℹ️ 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".
| assert!( | ||
| !message.contains("does not serve Maintenance"), |
There was a problem hiding this comment.
This only excludes one early error string and does not observe either maintenance call. If either RPC is reverted to the old host-side implementation, replaced with a successful no-op, or becomes an unknown method, message is empty or different and the test still passes without calling flush_pending or reset_derived_index. Use an instrumented provider or assert a specific post-dispatch result so the claimed contract-hop regression is detectable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed on the substance. Partly fixed in 27341632a, and I am leaving this OPEN because one of your three cases is not closed and I do not want to resolve on a partial answer.
Closed — renamed or removed RPC. unknown method: <name> contains no does not serve Maintenance, so it satisfied the old assertion while nothing dispatched. Now rejected explicitly. Also added a check that the response carries a result or an error rather than neither.
NOT closed — a successful no-op. From the RPC boundary a no-op is indistinguishable from a real call: reset_tree answers tree_rows_deleted / chunks_requeued / jobs_enqueued, and a genuine call against an empty store returns the same zeros. Your suggestion of an instrumented provider is the right shape, but memory/binding.rs exposes no driver-registration seam — I grepped for set_test_provider / register_driver / test_provider and there is none. Adding one is a change to production wiring, which does not belong in a test-backfill PR. If you want that seam, it should be its own change and I am happy to say so on an issue.
Being precise about what I demonstrated, because the revert I ran does not prove my additions. I revert-checked by forcing ModuleMemoryProvider::as_maintenance() to None. It failed at memory_sources_e2e.rs:1047 —
openhuman.memory_tree_flush_now never reached a driver: the bound provider does not serve
the Maintenance capability... Got: flush_now: driver 'tinymemory' does not serve Maintenance
— but line 1047 is the pre-existing assertion, not the ones I added. That revert was already covered. My unknown method assertion is justified by inspection of the error text rather than by that run, and I did not spend a second build under the current memory/disk throttle to exercise it on a thread that stays open regardless. Flagging that rather than letting the green revert-check read as proof of the new lines.
Over to a human on the no-op case.
bfa1e16 to
1b84322
Compare
Both review findings on tinyhumansai#5973 were correct. `sources_sync_...` asserted `!contains("unsupported capability")` and `!contains("source_sync")`. The other way the call is refused before dispatch is `sync_rpc` bailing on an absent capability with the bound memory driver '<id>' does not serve source sync (`memory/sources/rpc_part_01.rs:560-564`). That carries "source sync" with a SPACE, so it contains neither string and both assertions passed while `run_source_sync` was never reached — green for the exact regression the test exists to catch, one layer up from the defaulted body. Now rejected. Proven: forcing `ModuleMemoryProvider::as_source_sync()` to `None` fails the new assertion at memory_sources_e2e.rs:977 with the real message ("...driver 'tinymemory' does not serve source sync"); before the change that revert passed. `tree_reset_and_flush_...` excluded one string, so an RPC that was renamed or removed 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. Not closed, and said so on the thread: a successful no-op is indistinguishable from a real call at the RPC boundary. Detecting it needs an instrumented provider, and `memory/binding.rs` exposes no driver-registration seam; adding one is a production change that does not belong in a test backfill.
…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.
2734163 to
5c1bc7e
Compare
…-w3\n\ntest(e2e): backfill coverage for the memory module-dispatch and maintenance-contract hops\n
1 participant
Summary
run_source_syncmodule member) and Route the recall, reset and flush paths through the contract #5725 (reset_tree/flush_nowrouted through the Maintenance contract).tests/memory_sources_e2e.rs.## Related.Problem
Both paths could break completely today and no lane would go red.
#5808 / #5801.
MemorySourceSync::run_source_syncis a defaulted contract member.ModuleMemoryProviderinherited itsUnsupportedbody instead of bridging it, so "Sync now" answeredunsupported capability: source_syncwhile the module's own scheduler kept syncing fine. A defaulted member that was never bridged is indistinguishable from a bridged one at compile time — which is exactly why the bug shipped.#5725.
reset_treeandflush_nowwere routed throughMaintenance::reset_derived_index/flush_pending. Nothing exercised either afterwards. In the raw-coverage lane both names appear only as string literals fed tomemory::schema::schemas(...)— a schema lookup, not an invocation (memory_threads_raw_coverage_e2e.rs:1153-1158). Inworker_c_modules_e2e.rsthey sit in a 68-method loop whose helperassert_rpc_completedpasses if the response contains eitherresultorerror. Both satisfy a grep and the domain e2e gate while asserting nothing.Solution
Because the compile-time signal is exactly what is missing in both cases, both tests assert the runtime answer, and each keys on a message produced on a specific side of the seam.
sources_sync_dispatches_to_the_module_rather_than_refusing_the_capability—binding::buildbindsmodule_providerwhenever themodulesfeature is on, soopenhuman.memory_sources_syncreally 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. The test adds an enabled folder source (assertingenabledfirst, since the enabled gate insync_rpcreturns before the driver is ever asked) and requires that the answer is not anunsupported capabilityrefusal.tree_reset_and_flush_route_through_the_maintenance_contract— both RPCs resolvebinding.provider().as_maintenance()and bail with"driver '<id>' does not serve Maintenance"before any driver call. That message is therefore the one failure that proves the contract hop did not happen, and it is what the test forbids.Neither test asserts success, deliberately. Success needs a live module artifact fetched over the network, and this lane must not depend on that. The bugs being pinned were never "wrong data" — they were "the call is refused before it is attempted", and that is what these assert.
Submission Checklist
diff-coverto score.## Related— N/A: no matrix feature row applies.## Related.Closes #NNNin the## Relatedsection — N/A: backfills coverage for already-merged PRs; there is no open issue to close.Impact
memory_sources_e2etarget, so no new link step.Related
~/tinyhuman/bugs/W3-test-findings.md— most notably thatflows/.../composio_tests.rs::backend_dispatch_forwards_the_workflow_connection_idis a non-#[ignore]d test that can only pass by downloading a module over the network, which contradicts the mock policy and will fail on an isolated runner.assert_rpc_completeda real assertion; make theraw_coverage_allfeature gating fail loudly instead of skipping silently.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
test/e2e-backfill-w34490739d2Validation Run
pnpm --filter openhuman-app format:check— N/A: no frontend file is touched.pnpm typecheck— N/A: no TypeScript is touched.cargo test --test memory_sources_e2e sources_sync_dispatches_to_the_moduleand... tree_reset_and_flush_route_through— both pass; both revert-checked (table below).cargo fmt -- --check— clean.Validation Blocked
command:the Playwright spec for fix(brain): warn on a failed refresh instead of showing stale data silently #5942, and any test that drives a real module for fix(flows): honor selected Composio account #5837 / Take the connector vocabulary and module from tinyconnectors #5849 / Shed tinycortex and tinymemory-core from the product build (memory over the v1.13.6 module, contract 4.0) #5875error:no module artifact is available without a network download; the host was at its memory and slot limits, so a Playwright app build was not runimpact:those four items are reported as not covered rather than shipped unverifiedBehavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Revert-check evidence
sources_sync_dispatches_to_the_module_rather_than_refusing_the_capabilityrun_source_syncbridge removed frommodules/memory_part_02.rsso the trait default appliesmemory_sources_e2e.rs:952—Got: unsupported capability: source_sync(the #5801 string verbatim)tree_reset_and_flush_route_through_the_maintenance_contractas_maintenance()returnsNoneinmodules/memory_part_01.rsmemory_sources_e2e.rs:1029—Got: flush_now: driver 'tinymemory' does not serve MaintenanceBoth fixes restored afterwards;
git diffagainstmainfor both files is empty.Summary by CodeRabbit