fix(polyphonic): hydrate content before diversity ranking#471
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 (1)
📝 WalkthroughWalkthroughPolyphonic recall results now store source content hydrated from memory tables before diversity re-ranking. The recall flow and regression tests cover database-backed content, unmapped IDs, and similarity estimation. ChangesPolyphonic content hydration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RecallEngine
participant SQLiteMemory
participant DiversityRerank
RecallEngine->>RecallEngine: Fuse voice results
RecallEngine->>SQLiteMemory: Fetch source content by result IDs
SQLiteMemory-->>RecallEngine: Return matching content
RecallEngine->>DiversityRerank: Rank hydrated results
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test_beam_e5_polyphonic_recall.py`:
- Around line 178-199: Expand the related polyphonic recall tests around
PolyphonicRecallEngine.recall and _hydrate_result_content to cover grouped
integration cases: verify recall hydrates content before diversity reranking,
hydration works when conn=None via a temporary connection, unmapped synthetic
IDs retain empty content and produce zero similarity, and candidate sets larger
than the hydration batch size are fully hydrated. Preserve the existing fixture
style and assert retrieval behavior rather than only testing the helper
directly.
🪄 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: efaa45f4-f697-49b7-9c92-e2c3eae621e8
📒 Files selected for processing (2)
mnemosyne/core/polyphonic_recall.pytests/test_beam_e5_polyphonic_recall.py
| def test_diversity_hydrates_source_content(self, temp_db): | ||
| """Content Jaccard must use DB content, not a missing result attribute.""" | ||
| from mnemosyne.core.polyphonic_recall import ( | ||
| PolyphonicRecallEngine, | ||
| RecallResult, | ||
| ) | ||
|
|
||
| beam = BeamMemory(session_id="e5-content", db_path=temp_db) | ||
| first_id = beam.remember("Alice deployed the auth service") | ||
| second_id = beam.remember("Bob fixed the billing dashboard") | ||
| engine = PolyphonicRecallEngine(db_path=temp_db, conn=beam.conn) | ||
| combined = engine._combine_voices([ | ||
| RecallResult(first_id, 0.8, "vector", {}), | ||
| RecallResult(second_id, 0.7, "vector", {}), | ||
| ]) | ||
|
|
||
| engine._hydrate_result_content(combined) | ||
| assert combined[first_id].content == "Alice deployed the auth service" | ||
| assert combined[second_id].content == "Bob fixed the billing dashboard" | ||
| assert engine._estimate_similarity( | ||
| combined[first_id], combined[second_id] | ||
| ) < 0.8 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Cover the recall integration and hydration edge cases.
This meaningfully tests the helper, but not the regression path through recall(). Add grouped cases for: (1) recall() hydrating before diversity reranking, (2) conn=None temporary-connection hydration, (3) unmapped synthetic IDs retaining empty content with zero similarity, and (4) a candidate set above the hydration batch size.
As per path instructions, "tests/**: Group ALL suggestions for related fixtures/test files together" and verify meaningful coverage of retrieval behavior.
🤖 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/test_beam_e5_polyphonic_recall.py` around lines 178 - 199, Expand the
related polyphonic recall tests around PolyphonicRecallEngine.recall and
_hydrate_result_content to cover grouped integration cases: verify recall
hydrates content before diversity reranking, hydration works when conn=None via
a temporary connection, unmapped synthetic IDs retain empty content and produce
zero similarity, and candidate sets larger than the hydration batch size are
fully hydrated. Preserve the existing fixture style and assert retrieval
behavior rather than only testing the helper directly.
Source: Path instructions
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_beam_e5_polyphonic_recall.py (1)
197-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the exact Jaccard result.
The two seeded contents have no shared words, so the expected similarity is
0.0;< 0.8would also accept incorrect results such as0.79.Proposed assertion
- ) < 0.8 + ) == 0.0🤖 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/test_beam_e5_polyphonic_recall.py` around lines 197 - 199, Update the assertion for engine._estimate_similarity(combined[first_id], combined[second_id]) to require the exact expected Jaccard similarity of 0.0 instead of merely checking that it is below 0.8.Source: Path instructions
🤖 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.
Outside diff comments:
In `@tests/test_beam_e5_polyphonic_recall.py`:
- Around line 197-199: Update the assertion for
engine._estimate_similarity(combined[first_id], combined[second_id]) to require
the exact expected Jaccard similarity of 0.0 instead of merely checking that it
is below 0.8.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a6cfda31-b011-420b-a720-3043bd966d87
📒 Files selected for processing (1)
tests/test_beam_e5_polyphonic_recall.py
2 participants
Summary
Fixes the regression introduced by #389:
PolyphonicResultdid not carry acontentattribute, so the new content-Jaccard diversity scorer crashed and polyphonic recall returned no results.The engine now hydrates source content in two bounded queries (working + episodic) after RRF fusion and before diversity ranking. Synthetic/unmapped results retain empty content and are handled safely.
Verification
PolyphonicResult has no attribute contentCI failure.40 passed, 4 skipped.py_compileandgit diff --checkpass.Summary
Fixes a polyphonic recall regression where
PolyphonicResultlacked acontentattribute, breaking the content-Jaccard diversity re-ranking and potentially yielding empty polyphonic recall output. The polyphonic recall pipeline now hydratesPolyphonicResult.contentfrom Mnemosyne’s local tiered memories immediately after RRF fusion and before diversity re-ranking; synthetic/unmapped IDs remain safe via empty content.Architecture impact
Tiered memory (working/episodic/BEAM):
content: str = ""toPolyphonicResult._hydrate_result_content()which populatesresult.contentby performing local SQLite reads fromworking_memoryandepisodic_memory(does not change BEAM storage/schema).if result is not None and not result.content).Retrieval strategies / ranking:
self._hydrate_result_content(combined)right after combining voices and before_diversity_rerank(), so content-Jaccard operates on real source text rather than missing attributes.IN (...)placeholder query.Local-first / privacy posture:
working_memory,episodic_memory); no external network calls or external I/O are introduced.sqlite3.OperationalErrorand leavingcontentas""for unmapped/synthetic IDs.Agent integration surfaces (Hermes, MCP, CLI):
PolyphonicResult.content) used for ranking, without altering request/response semantics at the integration layer.Veracity / sync layer / consolidation pipeline:
Maintainability / resilience:
_hydrate_result_content()uses the engine’s sharedconnwhen available, otherwise creates a short-lived connection and closes it.Verification
pytest.approx(1/9)).content == ""and similarity for identical empty content inputs returns0.0.py_compileandgit diff --checkalso pass.