fix(test): run agent_retrieval_e2e, and point it at the shipped ingest path - #5983
Conversation
How this change flows3 changed behaviours across 13 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 47 further behaviours left out to keep the diagram readable. flowchart LR
n0["...chat_entity_index_spans_source_boundaries<br/>changed"]:::changed
n1["...aves_hydrates_source_ref_for_cited_chunks<br/>changed"]:::changed
n2["orchestrator_reaches_memory_agent_on_demand<br/>changed"]:::changed
n3["vec"]:::impacted
n4["any"]:::impacted
n5["format"]:::impacted
n6["ChatBatch"]:::impacted
n7["ensure_memory_seams"]:::impacted
n8["set_workspace_env"]:::impacted
n0 -->|calls| n3
n0 -->|tests| n3
n0 -->|uses| n6
n0 -->|calls| n7
n0 -->|calls| n8
n1 -->|calls| n3
n1 -->|tests| n3
n1 -->|calls| n7
n1 -->|calls| n8
n2 -->|calls| n4
n2 -->|tests| n4
n2 -->|calls| n5
n2 -->|tests| n5
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. |
|
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; 3 remain after this review. 📝 WalkthroughWalkthroughTwo retrieval end-to-end tests now use RPC ingestion. Both tests run instead of being ignored. The email test filters chunks by source ID and verifies that ingest-time references remain in cited chunks. ChangesRetrieval RPC test coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change only restores and corrects end-to-end test coverage, with no production behavior or runtime impact. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c5fdd0c41
ℹ️ 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".
| ingest_rpc( | ||
| &cfg, |
There was a problem hiding this comment.
Keep module ingestion inside the temporary workspace
When this test runs through the product-feature E2E runner, modules is enabled and the integration target links openhuman_core with cfg(test) = false; consequently, binding::module_provider explicitly ignores cfg.workspace_dir and loads TinyMemory using the process-wide boot policy. ensure_memory_seams publishes that policy from Config::default(), whose workspace is the real ~/.openhuman/workspace, so this newly enabled ingest_rpc call writes the synthetic Alice/Phoenix records into a developer's actual memory store rather than tmp. The fixed source IDs also let later runs pass against stale data. Publish an isolated temp-backed module policy before the first binding, or otherwise ensure the module itself is rooted in the test workspace.
Useful? React with 👍 / 👎.
…ingest path
Two defects in one file. Both tests carried `#[ignore]`, so neither ran;
and both ingested through `tinymemory_core::ingest_pipeline::{ingest_chat,
ingest_email}` — the in-process engine call that tinyhumansai#5560/tinyhumansai#5779 replaced —
so if they had run they would have exercised a path the product no longer
takes.
They now go through `memory::tree::tree::rpc::ingest_rpc`, which is what
the `openhuman.memory_tree_ingest` RPC serves: payload -> `chat_items` /
`email_items` -> the bound driver's Ingest family. The chunk listing moves
to `read_rpc::list_chunks_rpc` for the same reason — reading the engine's
in-process store directly would not be reading what the product reads.
The ignore reason ("the currently pinned artifact predates
SearchEntities/RetrieveLeaves") no longer holds: all three tests pass
against the pinned module.
… the test wrote Two problems the un-ignoring exposed, both in the provenance test. It listed chunks unfiltered and took the first two. Those were not the messages it had just ingested — the fetched leaves came back as `agent://session/phase1-flush-test/segment/...`, rows this test never wrote. The listing is now scoped to the thread's own source id. And the assertion only checked that a `source_ref` field was inhabited, which those foreign chunks satisfied. Measured: with `email_items` dropping `source_ref` outright, the test still passed. It now asserts the message-id set at ingest actually reaches the citation.
2c5fdd0 to
f5dcecf
Compare
Summary
tests/agent_retrieval_e2e.rshad two defects at once: both of its async tests carried#[ignore], so neither ran; and both ingested throughtinymemory_core::ingest_pipeline::{ingest_chat, ingest_email}— the in-process engine call that Route memory tool and query paths through the module seam so tinymemory-core leaves the build #5560/Route the last six memory handlers through the contract (#5560 phase 2) #5779 replaced — so if they had run they would have exercised a path the product no longer takes.#[ignore]s are gone and ingest goes throughmemory::tree::tree::rpc::ingest_rpc, the handler behindopenhuman.memory_tree_ingest.Problem
#[ignore]gave the reason "the currently pinned artifact predates SearchEntities/RetrieveLeaves". That is no longer true — the file's ownensure_memory_seamsbinds the module driver, and the tests pass against the pinned artifact.Meanwhile the file read as coverage of the ingest route to a grep and to the string-match domain gate, while running nothing.
What surfaced once it ran:
The provenance test was fetching chunks it never wrote. It listed chunks with no filter and took the first two rows. Those were not its emails — the leaves came back as:
rows belonging to something else entirely.
And its assertion could not tell the difference. It only checked that a
source_reffield was inhabited, which those foreign chunks satisfied. Measured: withemail_itemsdroppingsource_refoutright, the test still passed. A provenance test that passes when provenance is deleted is not a provenance test.Solution
#[ignore]removed from both tests. No replacement suppression.ingest_rpc(SourceKind::Chat/SourceKind::Email), which is payload →chat_items/email_items→ the bound driver's Ingest family.read_rpc::list_chunks_rpc, scoped to the thread's ownsource_id. Reading the engine's in-process store directly would not be reading what the product reads, and reading it unfiltered is what fetched somebody else's rows.Mutation checks
Each async test was checked against a mutation of the behaviour it pins; both fail naming their own assertion, and both pass again when restored.
cross_chat_entity_index_spans_source_boundarieschat_itemsemits empty contentfetch_leaves_hydrates_source_ref_for_cited_chunksemail_itemssetssource_ref: Nonegot refs []Restored:
3 passed; 0 failed; 0 ignored.One negative result worth recording. Reverting only the path change — putting the direct-engine
ingest_chat/ingest_emailcalls back while keeping the host-side read — left all three tests passing. The two ingest routes converge on the same store at this level, so this file cannot serve as a regression guard for which route is taken. It is still correct for it to drive the shipped one — that is what makes theemail_itemsmutation above visible at all — but the path change is not itself pinned by an assertion, and I would rather say so than let the green be read as proof.Submission Checklist
N/A: no production lines changed; the diff is one test fileN/A: no feature rows added, removed or renamed## Related—N/A: no coverage-matrix feature IDs affectedN/A: test-only change, no release-cut surface touchedCloses #NNNin the## Relatedsection —N/A: fixes a test defect found during a coverage audit; no issue filedImpact
No runtime, platform, security or migration impact — one test file, no production code. CI gains two tests that previously did not execute. The
agent_retrieval_e2etarget runs in ~5s.Related
#[ignore]d lane that also tests the wrong path".list_chunks_rpcin a fresh temp workspace returnedagent://session/phase1-*rows this test never wrote. Scoping the filter is the right fix for this test either way, but it does not explain where those rows came from, and if workspace isolation is leaking between tests that is worth knowing independently.Summary by CodeRabbit