fix: restore main CI contracts after merge#480
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)
📝 WalkthroughWalkthroughThe PR registers a pending-memory application tool, adds randomized backoff to embedding API retries, updates polyphonic recall similarity calculation, and revises related regression coverage. ChangesPending memory tool registration
Embedding retry jitter
Polyphonic recall and regression updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_memory_lifecycle.py (1)
6-11: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the deleted lifecycle regression coverage.
This smoke test does not replace the removed
Mnemosyne.__del__and multi-owner close/reconnection tests: cleanup and reopen regressions can now pass CI unnoticed. Also assert that the remembered record is persisted and retrievable, rather than checking only thatremember()returned a string.As per path instructions,
tests/**must maintain comprehensive, meaningful coverage of lifecycle and cross-session 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_memory_lifecycle.py` around lines 6 - 11, Expand test_mnemosyne_initializes_beam_and_can_remember to verify the remembered record is persisted and retrievable, then restore dedicated regression coverage for Mnemosyne cleanup via __del__ and for closing/reconnecting with multiple owners or sessions. Keep the tests focused on lifecycle behavior and cross-session persistence, using the existing public APIs and asserting records remain available after reopen.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.
Inline comments:
In `@integrations/hermes/src/mnemosyne_hermes/tools.py`:
- Around line 797-802: The Hermes documentation’s tool registry listing is
outdated. Update the tools section in README.md to state 40 tools and add
mnemosyne_apply_pending alongside the existing documented tools, using
ALL_TOOL_SCHEMAS as the source of truth.
In `@tests/test_provider_all_15_tools.py`:
- Around line 52-55: Add focused assertions to test_all_tools_registered using
the existing provider tool metadata: verify mnemosyne_apply_pending is
registered, and validate its schema requires pending_ids as an array whose items
are strings. Keep the aggregate tool-count assertion, but ensure the new checks
independently detect missing or incorrectly defined registration.
---
Outside diff comments:
In `@tests/test_memory_lifecycle.py`:
- Around line 6-11: Expand test_mnemosyne_initializes_beam_and_can_remember to
verify the remembered record is persisted and retrievable, then restore
dedicated regression coverage for Mnemosyne cleanup via __del__ and for
closing/reconnecting with multiple owners or sessions. Keep the tests focused on
lifecycle behavior and cross-session persistence, using the existing public APIs
and asserting records remain available after reopen.
🪄 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: aed422c3-be7f-43ab-89a7-9d1a14baf561
📒 Files selected for processing (4)
integrations/hermes/src/mnemosyne_hermes/tools.pymnemosyne/core/embeddings.pytests/test_memory_lifecycle.pytests/test_provider_all_15_tools.py
| ALL_TOOL_SCHEMAS = [ | ||
| REMEMBER_SCHEMA, RECALL_SCHEMA, SHARED_REMEMBER_SCHEMA, SHARED_RECALL_SCHEMA, | ||
| SHARED_FORGET_SCHEMA, SHARED_STATS_SCHEMA, SLEEP_SCHEMA, STATS_SCHEMA, | ||
| INVALIDATE_SCHEMA, VALIDATE_SCHEMA, GET_SCHEMA, TRIPLE_ADD_SCHEMA, TRIPLE_QUERY_SCHEMA, | ||
| TRIPLE_END_SCHEMA, | ||
| REMEMBER_CANONICAL_SCHEMA, RECALL_CANONICAL_SCHEMA, FORGET_CANONICAL_SCHEMA, MODEL_CARD_SCHEMA, | ||
| REMEMBER_CANONICAL_SCHEMA, RECALL_CANONICAL_SCHEMA, FORGET_CANONICAL_SCHEMA, APPLY_PENDING_SCHEMA, MODEL_CARD_SCHEMA, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files integrations/hermes/src/mnemosyne_hermes/tools.py integrations/hermes/README.md integrations/hermes | sed 's#^`#-` #'
printf '\n== Tool schema list around the cited lines ==\n'
sed -n '780,830p' integrations/hermes/src/mnemosyne_hermes/tools.py | cat -n
printf '\n== README references to tools/count/memory guidance ==\n'
rg -n "tool|Tools|memory|Mnemosyne|disable memory|40|apply_pending|mnemosyne_apply_pending" integrations/hermes/README.md
printf '\n== Count ALL_TOOL_SCHEMAS entries by source text ==\n'
python3 - <<'PY'
import re, pathlib
path = pathlib.Path('integrations/hermes/src/mnemosyne_hermes/tools.py')
text = path.read_text()
m = re.search(r'ALL_TOOL_SCHEMAS\s*=\s*\[(.*?)\n\]', text, re.S)
if not m:
print('ALL_TOOL_SCHEMAS block not found')
raise SystemExit(1)
block = m.group(1)
items = re.findall(r'([A-Z_]+_SCHEMA)\b', block)
print('count_in_block=', len(items))
print('last_10=', items[-10:])
PYRepository: mnemosyne-oss/mnemosyne
Length of output: 7211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- README excerpts ---'
sed -n '1,260p' integrations/hermes/README.md | cat -n | sed -n '1,260p'
echo
echo '--- tool schema names in tools.py ---'
python3 - <<'PY'
import re, pathlib
text = pathlib.Path('integrations/hermes/src/mnemosyne_hermes/tools.py').read_text()
m = re.search(r'ALL_TOOL_SCHEMAS\s*=\s*\[(.*?)\n\]', text, re.S)
if not m:
print('missing ALL_TOOL_SCHEMAS')
raise SystemExit(1)
block = m.group(1)
items = re.findall(r'([A-Z_]+_SCHEMA)\b', block)
print('COUNT', len(items))
for i, item in enumerate(items, 1):
print(f'{i:02d} {item}')
PYRepository: mnemosyne-oss/mnemosyne
Length of output: 11947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'README matches:\n'
rg -n "40 tools|39 tools|mnemosyne_apply_pending|apply_pending|disable memory|memory tools" integrations/hermes/README.md || true
printf '\nALL_TOOL_SCHEMAS count:\n'
python3 - <<'PY'
import re, pathlib
text = pathlib.Path('integrations/hermes/src/mnemosyne_hermes/tools.py').read_text()
m = re.search(r'ALL_TOOL_SCHEMAS\s*=\s*\[(.*?)\n\]', text, re.S)
items = re.findall(r'([A-Z_]+_SCHEMA)\b', m.group(1)) if m else []
print(len(items))
print(items[-6:])
PYRepository: mnemosyne-oss/mnemosyne
Length of output: 634
Update the Hermes docs to match the current tool registry.
integrations/hermes/README.md still says 23 tools, but ALL_TOOL_SCHEMAS now exposes 40. Update the count, and add mnemosyne_apply_pending to the documented tool surface if this section is meant to enumerate it.
🤖 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 `@integrations/hermes/src/mnemosyne_hermes/tools.py` around lines 797 - 802,
The Hermes documentation’s tool registry listing is outdated. Update the tools
section in README.md to state 40 tools and add mnemosyne_apply_pending alongside
the existing documented tools, using ALL_TOOL_SCHEMAS as the source of truth.
Source: Path instructions
| def test_all_tools_registered(self, tmp_path): | ||
| provider = _provider(tmp_path) | ||
| names = _tool_names(provider) | ||
| assert len(names) == 39, f"Expected 39 tools, got {len(names)}" | ||
| assert len(names) == 40, f"Expected 40 tools, got {len(names)}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a focused regression test for mnemosyne_apply_pending.
Updating the aggregate count does not prove the new tool is registered; the test could still pass with a missing tool and an unrelated duplicate. Assert that mnemosyne_apply_pending exists and that its schema requires an array of string pending_ids.
Suggested coverage
def test_all_tools_registered(self, tmp_path):
provider = _provider(tmp_path)
names = _tool_names(provider)
assert len(names) == 40, f"Expected 40 tools, got {len(names)}"
+
+ def test_apply_pending_tool_present(self, tmp_path):
+ provider = _provider(tmp_path)
+ schemas = provider.get_tool_schemas()
+ schema = next(s for s in schemas if s["name"] == "mnemosyne_apply_pending")
+ assert schema["parameters"]["required"] == ["pending_ids"]
+ assert schema["parameters"]["properties"]["pending_ids"] == {
+ "type": "array",
+ "items": {"type": "string"},
+ "description": schema["parameters"]["properties"]["pending_ids"]["description"],
+ }As per path instructions: “Ensure strong coverage of: ... MCP tool surface ... Flag missing or weak test scenarios.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_all_tools_registered(self, tmp_path): | |
| provider = _provider(tmp_path) | |
| names = _tool_names(provider) | |
| assert len(names) == 39, f"Expected 39 tools, got {len(names)}" | |
| assert len(names) == 40, f"Expected 40 tools, got {len(names)}" | |
| def test_all_tools_registered(self, tmp_path): | |
| provider = _provider(tmp_path) | |
| names = _tool_names(provider) | |
| assert len(names) == 40, f"Expected 40 tools, got {len(names)}" | |
| def test_apply_pending_tool_present(self, tmp_path): | |
| provider = _provider(tmp_path) | |
| schemas = provider.get_tool_schemas() | |
| schema = next(s for s in schemas if s["name"] == "mnemosyne_apply_pending") | |
| assert schema["parameters"]["required"] == ["pending_ids"] | |
| assert schema["parameters"]["properties"]["pending_ids"] == { | |
| "type": "array", | |
| "items": {"type": "string"}, | |
| "description": schema["parameters"]["properties"]["pending_ids"]["description"], | |
| } |
🤖 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_provider_all_15_tools.py` around lines 52 - 55, Add focused
assertions to test_all_tools_registered using the existing provider tool
metadata: verify mnemosyne_apply_pending is registered, and validate its schema
requires pending_ids as an array whose items are strings. Keep the aggregate
tool-count assertion, but ensure the new checks independently detect missing or
incorrectly defined registration.
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)
194-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake this assertion hit the field
_estimate_similarityactually reads._hydrate_result_contentonly populatesresult.content, while similarity usesmetadata["content"]; with empty metadata here, the0.0check passes even if hydration breaks. Use overlapping content and assert a non-zero Jaccard score, or put the source text intometadata["content"].🤖 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 194 - 199, Update the test around _hydrate_result_content and _estimate_similarity so the compared results contain source text in metadata["content"], which is the field similarity reads. Use overlapping content and assert the expected non-zero Jaccard score, ensuring the assertion fails if hydration or metadata content handling is broken.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 194-199: Update the test around _hydrate_result_content and
_estimate_similarity so the compared results contain source text in
metadata["content"], which is the field similarity reads. Use overlapping
content and assert the expected non-zero Jaccard score, ensuring the assertion
fails if hydration or metadata content handling is broken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6f4621d-239f-4dc8-8002-40eee8c2f768
📒 Files selected for processing (1)
tests/test_beam_e5_polyphonic_recall.py
|
@AxDSan #480 is ready to merge. Full CI is green across Python 3.10–3.13, plus build, docs, CLA, and CodeRabbit. It restores the post-merge contracts introduced across #456/#476/#478:
No |
|
After #480 lands, I suggest one clean The next substantive item is #474, which I’d treat as a separate design-first effort: make the FastEmbed/sqlite-vec capability state observable end-to-end, avoid silent FTS-only degradation, and reproduce the embedding-storage/FK path before changing it. I’d avoid folding that into another quick stabilization PR. |
2 participants
Restores main CI after the #456/#476/#478 merge sequence.
Validation: focused regression suite (82 passed), py_compile, git diff --check.
Summary
mnemosyne_apply_pending,APPLY_PENDING_SCHEMA). This re-enables the approved “staged/approved pending writes” flow to be committed through the BEAM write path (i.e., it fixes the integration surface contract rather than changing memory semantics).PolyphonicResult.contentwhen present (falling back tometadata["content"]), updating the related regression assertion accordingly.Impact across architecture & guarantees
APPLY_PENDING_SCHEMAcommits existing staged/approved local pending records, and the embedding retry change alters timing only (not request payloads).py_compile, andgit diff --check.