Skip to content

fix(test): run agent_retrieval_e2e, and point it at the shipped ingest path - #5983

Merged
M3gA-Mind merged 2 commits into
tinyhumansai:mainfrom
M3gA-Mind:fix/agent-retrieval-e2e
Sep 2, 2026
Merged

fix(test): run agent_retrieval_e2e, and point it at the shipped ingest path#5983
M3gA-Mind merged 2 commits into
tinyhumansai:mainfrom
M3gA-Mind:fix/agent-retrieval-e2e

Conversation

@M3gA-Mind

@M3gA-Mind M3gA-Mind commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • tests/agent_retrieval_e2e.rs had two defects at once: both of its async tests carried #[ignore], so neither ran; and both ingested through tinymemory_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.
  • Both are fixed: the #[ignore]s are gone and ingest goes through memory::tree::tree::rpc::ingest_rpc, the handler behind openhuman.memory_tree_ingest.
  • Un-ignoring then exposed two further defects inside the provenance test, which are also fixed here. Neither was visible while the test was skipped.
  • All three tests in the file now run and pass, and both async tests are mutation-checked.

Problem

#[ignore] gave the reason "the currently pinned artifact predates SearchEntities/RetrieveLeaves". That is no longer true — the file's own ensure_memory_seams binds 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:

["agent://session/phase1-flush-test/segment/seg-18cdb8ff55cafe88c4a6eeb9#ep1-3",
 "agent://session/phase1-recap-test/segment/seg-18cdb8ff55ed6338909210e5#ep1-3"]

rows belonging to something else entirely.

And its assertion could not tell the difference. It 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. A provenance test that passes when provenance is deleted is not a provenance test.

Solution

  • #[ignore] removed from both tests. No replacement suppression.
  • Ingest via ingest_rpc (SourceKind::Chat / SourceKind::Email), which is payload → chat_items / email_items → the bound driver's Ingest family.
  • The chunk listing moves to read_rpc::list_chunks_rpc, scoped to the thread's own source_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.
  • The provenance assertion now requires the message-id set at ingest to reach the citation, rather than requiring the field to be non-null.

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.

test mutation result
cross_chat_entity_index_spans_source_boundaries chat_items emits empty content FAILED at the cross-source entity assertion
fetch_leaves_hydrates_source_ref_for_cited_chunks email_items sets source_ref: None FAILEDgot 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_email calls 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 the email_items mutation 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

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy — two previously-dead tests now run, both mutation-checked
  • Diff coverage ≥ 80%N/A: no production lines changed; the diff is one test file
  • Coverage matrix updated — N/A: no feature rows added, removed or renamed
  • All affected feature IDs from the matrix are listed in the PR description under ## RelatedN/A: no coverage-matrix feature IDs affected
  • No new external network dependencies introduced (mock backend used per Testing Strategy) — no new dependency of any kind
  • Manual smoke checklist updated if this touches release-cut surfaces — N/A: test-only change, no release-cut surface touched
  • Linked issue closed via Closes #NNN in the ## Related section — N/A: fixes a test defect found during a coverage audit; no issue filed

Impact

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_e2e target runs in ~5s.

Related

  • Follows the coverage audit behind test(e2e): backfill coverage for the flush latch, source-scope rendering, erased tool extensions and theme import #5965; this is the item recorded there as "an #[ignore]d lane that also tests the wrong path".
  • Follow-up worth a maintainer's eye, not fixed here: an unfiltered list_chunks_rpc in a fresh temp workspace returned agent://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

  • Tests
    • Expanded end-to-end retrieval coverage for chat and email sources.
    • Retrieval tests now run as part of the standard test suite.
    • Improved validation that cited chunks preserve accurate source references.
    • Added coverage for retrieving chunks within the correct ingested source scope and across source boundaries.

@M3gA-Mind
M3gA-Mind requested a review from a team September 2, 2026 16:40

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinysweeper found nothing blocking. Approving.

$0.0000 · 0 in / 0 out

@tinysweeper

tinysweeper Bot commented Sep 2, 2026

Copy link
Copy Markdown

How this change flows

3 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
Loading

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 0.1.0

@tinysweeper tinysweeper Bot added the priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. label Sep 2, 2026
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 0eae9228-bab7-448c-a29b-a6d3068b683d

📥 Commits

Reviewing files that changed from the base of the PR and between 8e65c40 and 2c5fdd0.

📒 Files selected for processing (1)
  • tests/agent_retrieval_e2e.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Two 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.

Changes

Retrieval RPC test coverage

Layer / File(s) Summary
RPC ingestion setup
tests/agent_retrieval_e2e.rs
The tests replace direct chat and email ingest helpers with ingest_rpc and typed IngestRequest payloads.
Retrieval and provenance assertions
tests/agent_retrieval_e2e.rs
Both tests are enabled. The email test filters chunks by source ID and requires cited source_ref values to contain the expected message references.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 2c5fd

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: sanil-23

Poem

A rabbit sends payloads through RPC,
Chat trails cross boundaries cleanly.
Email leaves carry refs,
Cited chunks reveal their threads,
And skipped tests hop free.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: enabling the agent retrieval end-to-end tests and routing them through the shipped ingest path.

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +264 to +265
ingest_rpc(
&cfg,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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.
@M3gA-Mind
M3gA-Mind force-pushed the fix/agent-retrieval-e2e branch from 2c5fdd0 to f5dcecf Compare September 2, 2026 17:16
@M3gA-Mind
M3gA-Mind merged commit ad84233 into tinyhumansai:main Sep 2, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant