Repository navigation
test(reborn): add memory_search/memory_tree int-tier scenarios (T0-MEMQ) - #5434
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTwo new integration test scenarios are added to ChangesMemory Group E2E Scenario Expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
github-actions
Bot
added
risk: low
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two new integration test scenarios to the reborn_group_memory test suite: scenario_memory_search_finds_seeded and scenario_memory_tree_reflects_structure. These scenarios verify that documents written by one thread are correctly searchable and visible in the directory tree structure from a different thread/conversation sharing the same underlying store. There are no review comments, so no feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/reborn_group_memory/scenario_memory_search_finds_seeded.rs`:
- Around line 54-69: The test in scenario_memory_search_finds_seeded is too weak
to prove that builtin.memory_search is query-selective because it only checks
for an unwritten token; strengthen the guard by seeding a second unrelated
document and asserting its unique marker is not present in the search result for
the target query, using searcher.assert_tool_result_contains (or an equivalent
negative check) around the existing memory_search assertion, or else soften the
comment to describe intent rather than a guarantee.
In `@tests/reborn_group_memory/scenario_memory_tree_reflects_structure.rs`:
- Around line 52-55: The current assertions in the scenario only check for the
presence of “atlas/” and “runbook.md”, which can still pass if the tree is
flattened or mis-parented. Update the test in
scenario_memory_tree_reflects_structure to verify the hierarchy by asserting the
parent path is reflected as well, and/or add a second memory_tree call scoped to
“projects/atlas” so the leaf is validated through its real parent. Use the
existing lister assertions around assert_tool_result_contains and the
memory_tree invocation to keep the structure check tied to the actual
parent-child relationship.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f80d2901-a1bd-43df-8be8-2c8e65287b6e
📒 Files selected for processing (3)
tests/reborn_group_memory/main.rstests/reborn_group_memory/scenario_memory_search_finds_seeded.rstests/reborn_group_memory/scenario_memory_tree_reflects_structure.rs
| // The hit's snippet includes the marker → search located the seeded doc. | ||
| searcher.assert_tool_result_contains("osprey-meridian-7").await?; | ||
|
|
||
| // Committed negative guard (non-vacuity): a marker that was never written | ||
| // must be ABSENT from the search result, so `assert_tool_result_contains` | ||
| // is proven to discriminate rather than pass unconditionally (e.g. if the | ||
| // search silently returned every document or an empty-but-stringified set). | ||
| if searcher | ||
| .assert_tool_result_contains("tungsten-mirage-88") | ||
| .await | ||
| .is_ok() | ||
| { | ||
| return Err( | ||
| "negative guard failed: search result must not contain an unwritten marker".into(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This does not prove memory_search is query-selective.
The current negative guard only proves the response does not contain an unwritten token. A broken builtin.memory_search that returns every indexed document would still pass because the seeded marker is present and tungsten-mirage-88 is absent. Seed a second unrelated document and assert that its marker is not returned for this query, or soften the guarantee in the comment.
As per path instructions, "Comments that promise guarantees across layers must either be enforced by code/tests or softened to describe intent."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/reborn_group_memory/scenario_memory_search_finds_seeded.rs` around
lines 54 - 69, The test in scenario_memory_search_finds_seeded is too weak to
prove that builtin.memory_search is query-selective because it only checks for
an unwritten token; strengthen the guard by seeding a second unrelated document
and asserting its unique marker is not present in the search result for the
target query, using searcher.assert_tool_result_contains (or an equivalent
negative check) around the existing memory_search assertion, or else soften the
comment to describe intent rather than a guarantee.
Source: Path instructions
| // The serialized tree array must contain both the intermediate directory | ||
| // and the leaf file, proving the structure was reflected (not dropped). | ||
| lister.assert_tool_result_contains("atlas/").await?; | ||
| lister.assert_tool_result_contains("runbook.md").await?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
These asserts do not verify the hierarchy.
Checking only "atlas/" and "runbook.md" allows a flattened or mis-parented tree to pass. If this scenario is meant to prove projects/atlas/runbook.md is reflected structurally, assert on the parent directory as well and/or drive a second memory_tree call at path: "projects/atlas" so the leaf is verified through its real parent.
As per path instructions, "Comments that promise guarantees across layers must either be enforced by code/tests or softened to describe intent."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/reborn_group_memory/scenario_memory_tree_reflects_structure.rs` around
lines 52 - 55, The current assertions in the scenario only check for the
presence of “atlas/” and “runbook.md”, which can still pass if the tree is
flattened or mis-parented. Update the test in
scenario_memory_tree_reflects_structure to verify the hierarchy by asserting the
parent path is reflected as well, and/or add a second memory_tree call scoped to
“projects/atlas” so the leaf is validated through its real parent. Use the
existing lister assertions around assert_tool_result_contains and the
memory_tree invocation to keep the structure check tied to the actual
parent-child relationship.
Source: Path instructions
|
🚅 Deployed to the ironclaw-pr-5434 environment in ironclaw-ci-preview
|
Add two integration-tier scenarios to the `reborn_group_memory` group, covering the `builtin.memory_search` and `builtin.memory_tree` first-party tools that `builtin_tools()` already registers but no int-tier test exercised. Both mirror the existing cross-thread write_then_read pattern: a writer conversation seeds a document via `memory_write`, then a different conversation over the shared store exercises the tool and asserts on a distinctive marker. Each carries a committed negative guard (an unwritten marker / uncreated directory) so the positive assertion is proven to discriminate rather than pass vacuously. memory_search relies on the FTS chunk projection persisting in the shared RootFilesystem, so cross-thread search hits the writer's chunks end to end. Mutation-verified: forcing search to return empty results turns only the search scenario RED; forcing the tree to drop nodes turns only the tree scenario RED — each for the right reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…T0-MEMQ) Style-only, no behavior change: rustfmt line-wrap fixes in the two new test scenario files added by this PR. clippy --all-targets --all-features -D warnings was already clean on these files; no lint fixes were needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
abe5d62 to
456db08
Compare
There was a problem hiding this comment.
Pull request overview
Adds missing integration-tier (int-tier) coverage for the first-party builtin.memory_search and builtin.memory_tree tools within the existing reborn_group_memory sequential group harness, exercising cross-conversation visibility through the shared workspace/memory store.
Changes:
- Add
memory_search_finds_seededscenario that writes a distinctive marker viabuiltin.memory_writein one conversation, then finds it viabuiltin.memory_searchin another (with a negative guard). - Add
memory_tree_reflects_structurescenario that writes a nested document path in one conversation, then verifiesbuiltin.memory_treein another reflects the intermediate directory + leaf (with a negative guard). - Wire both scenarios into
tests/reborn_group_memory/main.rsand update the module-level rationale comment accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/reborn_group_memory/main.rs | Registers and executes the two new memory scenarios in the existing sequential group runner. |
| tests/reborn_group_memory/scenario_memory_search_finds_seeded.rs | New int-tier cross-thread scenario covering builtin.memory_search against a seeded marker. |
| tests/reborn_group_memory/scenario_memory_tree_reflects_structure.rs | New int-tier cross-thread scenario covering builtin.memory_tree structural output for nested paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add Reborn integration-tier tests for builtin.memory_search and builtin.memory_tree shared-store memory scenarios.
Stats: 0 net-new findings (from 4 raw reviewer findings, 2 after overlap dedup, 0 after live-thread dedupe) across 0 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Dedupe Notes
- The search negative-guard/comment concern is already covered by the unresolved CodeRabbit thread on
tests/reborn_group_memory/scenario_memory_search_finds_seeded.rs. - The tree hierarchy/comment concern is already covered by the unresolved CodeRabbit thread on
tests/reborn_group_memory/scenario_memory_tree_reflects_structure.rs.
No additional net-new issues survived current-head validation.
What
Closes T0-MEMQ from the Reborn backend coverage roadmap (
docs/reborn/reborn-backend-coverage-roadmap.md, Tier 0). Adds two integration-tier scenarios to thereborn_group_memorygroup, covering thebuiltin.memory_searchandbuiltin.memory_treefirst-party tools — already registered bybuiltin_tools()but previously exercised only at trace/QA tier in-process int-tier.Scenarios added
memory_search_finds_seeded(tests/reborn_group_memory/scenario_memory_search_finds_seeded.rs) — writer conversation seeds a distinctive sentence viamemory_write(target: "memory"); a different conversation issuesmemory_searchover the shared store and asserts the hit's snippet surfaces the marker (osprey-meridian-7). This exercises the write→reindex→FTS-chunk-projection→search path end to end. Cross-thread search works because the chunk records + FTS index live in the sharedRootFilesystem.memory_tree_reflects_structure(tests/reborn_group_memory/scenario_memory_tree_reflects_structure.rs) — writer seeds a nested path (projects/atlas/runbook.md); a different conversation lists viamemory_tree(root, depth 3) and asserts both the intermediate directory (atlas/) and the leaf (runbook.md) appear.Both mirror the existing
scenario_write_then_read_cross_thread.rspattern (writer +assert_tool_invoked, thenassert_tool_result_containson the consumer) and each carries a committed negative guard (an unwritten marker / uncreated directory) so the positive assertion is proven to discriminate rather than pass vacuously.Mutation proof (protocol #4)
Code-under-test:
crates/ironclaw_host_runtime/src/first_party_tools/memory.rs.search_response_to_valueto emit empty results → onlymemory_search_finds_seededwent RED: "no recorded capability result containing osprey-meridian-7". Reverted.tree_response_to_valueto return an empty array → onlymemory_tree_reflects_structurewent RED: "no recorded capability result containing atlas/". Reverted.Each mutation reddened exactly the intended scenario for the right reason; the other scenarios stayed green.
Verification
cargo build --tests --all-features— greencargo clippy --all --tests --all-features -- -D warnings— cleanreborn_group_memorygreen under--features libsqland--all-features(5× consecutive under all-features, no flake)Notes / deferred
main(efcacdc50).🤖 Generated with Claude Code