feat(recall): set keyword ranking weight from benchmark data (v2.32.0) - #118
Conversation
v2.31.0 activated keyword candidates for natural-language recall but left keyword scores out of ranking, because the old 0.35 weight had been chosen while that path never ran. This sweeps it and turns it on. Defaults, both measured on LoCoMo (1,977 questions, 5,882 memories, top-5): - HYBRID_SEARCH_TEXT_WEIGHT 0.0 -> 0.05. Swept over 13 points; any-hit peaks across a broad 0.04-0.08 plateau at 62.0-62.5% against 57.4% with keyword scoring off and 57.6% at the old 0.35. 0.05 is the plateau centre rather than the argmax, so the default is not fitted to one corpus. High weights are actively harmful: single-hop falls 56.9% -> 47.3% between 0.05 and 0.35. - FTS_CANDIDATE_LIMIT 50 -> 200. This was the lever on the multi-hop regression v2.31.0 introduced; 200 recovers multi-hop any-hit 34.8% -> 39.3%, matching the pre-v2.31.0 baseline, lifts single-hop 1.1pp, leaves adversarial precision unchanged, and costs ~3ms per recall. 500 buys 1.1pp more multi-hop but erodes the adversarial gain, since a pool that large stops narrowing anything. Combined vs v2.31.0 on the same corpus: any-hit 57.4% -> 62.5%, all-hit 47.6% -> 52.6%, evidence recall 51.9% -> 56.9%. Every category improves. Also fixes non-reproducible candidate selection, found because two identical benchmark runs scored 0.5pp apart while disagreeing on 18 questions. Ties at the candidate cutoff are the normal case (53.7% of queries) and SQLite ordered those rows differently between processes, so the pool varied across restarts. Candidate selection now breaks ties on memory ID. Accuracy is unchanged within measurement error; results are now reproducible, which also drops the benchmark's resolution floor from ~0.5pp to ~0.1pp. The fusion bake-off compares methods that can differ by less than the old floor, so it could not have been measured on the previous harness. New setting FTS_LONE_HIT_SCORE (default 1.0) sets the keyword score for a candidate set too degenerate to rank. It ships as a no-op: swept over 0.0/0.3/0.5/1.0 with no measurable effect, because only 1 of 1,982 queries produced such a set on a store this size. Exposed for small stores, where a query matching exactly one memory is routine. The spec had called the previous hardcoded 1.0 an active ranking bug; measurement says otherwise, since under wide OR a degenerate set means one memory held the only match for any query term, which is discriminative rather than weak evidence. No schema change, no migration, no re-embedding. Recall ordering changes by design; explicit env overrides are unaffected.
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesMARM v2.32.0 updates hybrid retrieval defaults, adds configurable BM25 lone-hit scoring, stabilizes FTS candidate tie-breaking, exposes the setting through diagnostics, aligns benchmarks, refreshes documentation, and synchronizes release metadata. Retrieval tuning and validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f863d6d43e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # roughly 3ms more per recall. 500 buys another 1.1pp of multi-hop but starts | ||
| # giving back the adversarial gain, because a pool that large stops acting as a | ||
| # precision gate. | ||
| _raw_fcl = _safe_int("FTS_CANDIDATE_LIMIT", 200) |
There was a problem hiding this comment.
With this new default, recall_similar fetches up to 200 candidates, but scripts/benchmarking/performance/bench_hotpath.py:297,367,379 still clamps the diagnostic with min(matched, 50) and labels it /50 and “capped at 50.” Consequently, queries with at least 50 matches always appear to saturate the production pool even when only a quarter of the new pool is filled, making the benchmark output misleading for the performance and breadth change this release advertises; derive the reporting cap from FTS_CANDIDATE_LIMIT instead.
AGENTS.md reference: AGENTS.md:L76-L76
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
marm-mcp-server/tests/test_hybrid_search.py (1)
924-936: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest reimplements the clamp formula instead of exercising the real settings path.
This asserts
_safe_floatoutput against a manually re-derivedmax(0.0, min(1.0, ...)), rather than reloadingsettings.pyand checking the actualFTS_LONE_HIT_SCOREconstant (and its stderr warning). A divergence between the documented formula and the real clamp/warning logic insettings.pywouldn't be caught here.♻️ Suggested approach exercising the real module path
-def test_fts_lone_hit_score_clamped_to_unit_range(monkeypatch): - """Out-of-range values clamp rather than propagate an invalid weight into - ranking, matching the neighbouring settings' clamp-and-warn contract.""" - from marm_mcp_server.config.settings import _safe_float - - monkeypatch.setenv("FTS_LONE_HIT_SCORE", "2.5") - assert max(0.0, min(1.0, _safe_float("FTS_LONE_HIT_SCORE", 1.0))) == 1.0 - - monkeypatch.setenv("FTS_LONE_HIT_SCORE", "-1") - assert max(0.0, min(1.0, _safe_float("FTS_LONE_HIT_SCORE", 1.0))) == 0.0 - - monkeypatch.setenv("FTS_LONE_HIT_SCORE", "not-a-number") - assert _safe_float("FTS_LONE_HIT_SCORE", 1.0) == 1.0 +def test_fts_lone_hit_score_clamped_to_unit_range(monkeypatch): + """Out-of-range values clamp via the real settings module, matching the + neighbouring settings' clamp-and-warn contract.""" + import importlib + from marm_mcp_server.config import settings + + monkeypatch.setenv("FTS_LONE_HIT_SCORE", "2.5") + importlib.reload(settings) + assert settings.FTS_LONE_HIT_SCORE == 1.0 + + monkeypatch.setenv("FTS_LONE_HIT_SCORE", "-1") + importlib.reload(settings) + assert settings.FTS_LONE_HIT_SCORE == 0.0 + + monkeypatch.delenv("FTS_LONE_HIT_SCORE") + importlib.reload(settings) # restore defaults for other tests🤖 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 `@marm-mcp-server/tests/test_hybrid_search.py` around lines 924 - 936, Update test_fts_lone_hit_score_clamped_to_unit_range to reload the real settings module after each monkeypatched FTS_LONE_HIT_SCORE value, then assert the module’s actual FTS_LONE_HIT_SCORE constant and captured stderr warning. Remove the manually reimplemented max/min assertions and preserve coverage for high, low, and invalid values using the settings module path.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.
Nitpick comments:
In `@marm-mcp-server/tests/test_hybrid_search.py`:
- Around line 924-936: Update test_fts_lone_hit_score_clamped_to_unit_range to
reload the real settings module after each monkeypatched FTS_LONE_HIT_SCORE
value, then assert the module’s actual FTS_LONE_HIT_SCORE constant and captured
stderr warning. Remove the manually reimplemented max/min assertions and
preserve coverage for high, low, and invalid values using the settings module
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 220f443a-1470-41c5-865f-a05e1d6793c7
📒 Files selected for processing (22)
CHANGELOG.mdREADME.mddocs/FAQ.mddocs/INSTALL-DOCKER.mddocs/INSTALL-LINUX.mddocs/INSTALL-PLATFORMS.mddocs/INSTALL-WINDOWS.mddocs/TECHNICAL-OVERVIEW.mdmarm-mcp-server/Dockerfilemarm-mcp-server/README.mdmarm-mcp-server/docker-compose.ymlmarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/marm_mcp_server/resources/marm-docs/FAQ.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/README.mdmarm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/marm_mcp_server/services/cli_output.pymarm-mcp-server/marm_mcp_server/services/runtime_status.pymarm-mcp-server/pyproject.tomlmarm-mcp-server/server.jsonmarm-mcp-server/tests/test_hybrid_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{py,json,md,toml,yml,yaml,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Keep changes surgical: modify only what the task requires, match existing style, and preserve behavior during refactors.
Files:
docs/INSTALL-DOCKER.mddocs/FAQ.mddocs/INSTALL-LINUX.mdmarm-mcp-server/marm_mcp_server/server.pydocs/TECHNICAL-OVERVIEW.mdmarm-mcp-server/pyproject.tomldocs/INSTALL-WINDOWS.mdmarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/docker-compose.ymldocs/INSTALL-PLATFORMS.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/FAQ.mdCHANGELOG.mdmarm-mcp-server/server.jsonmarm-mcp-server/marm_mcp_server/services/cli_output.pymarm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/marm_mcp_server/services/runtime_status.pyREADME.mdmarm-mcp-server/README.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/README.mdmarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/tests/test_hybrid_search.py
**/*.{md,py}
📄 CodeRabbit inference engine (AGENTS.md)
When adding or removing an MCP tool, update every full tool list and tool count in the README, protocol documentation, FAQ, and maintained
marm-docscopies; add tests covering both transports.
Files:
docs/INSTALL-DOCKER.mddocs/FAQ.mddocs/INSTALL-LINUX.mdmarm-mcp-server/marm_mcp_server/server.pydocs/TECHNICAL-OVERVIEW.mddocs/INSTALL-WINDOWS.mdmarm-mcp-server/marm_mcp_server/__init__.pydocs/INSTALL-PLATFORMS.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/FAQ.mdCHANGELOG.mdmarm-mcp-server/marm_mcp_server/services/cli_output.pymarm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/marm_mcp_server/services/runtime_status.pyREADME.mdmarm-mcp-server/README.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/README.mdmarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/tests/test_hybrid_search.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Never commit changes without an explicit user request.
Use semantic versioning: MAJOR for breaking changes, MINOR for new tools, parameters, or features, and PATCH for fixes and documentation updates.
Files:
docs/INSTALL-DOCKER.mddocs/FAQ.mddocs/INSTALL-LINUX.mdmarm-mcp-server/marm_mcp_server/server.pydocs/TECHNICAL-OVERVIEW.mdmarm-mcp-server/Dockerfilemarm-mcp-server/pyproject.tomldocs/INSTALL-WINDOWS.mdmarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/docker-compose.ymldocs/INSTALL-PLATFORMS.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/FAQ.mdCHANGELOG.mdmarm-mcp-server/server.jsonmarm-mcp-server/marm_mcp_server/services/cli_output.pymarm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/marm_mcp_server/services/runtime_status.pyREADME.mdmarm-mcp-server/README.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/README.mdmarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/tests/test_hybrid_search.py
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Only flag documentation issues that are materially wrong, misleading for installation/release behavior, or inconsistent with live MCP behavior. Skip style, phrasing, formatting, and wording preferences.
Files:
docs/INSTALL-DOCKER.mddocs/FAQ.mddocs/INSTALL-LINUX.mddocs/TECHNICAL-OVERVIEW.mddocs/INSTALL-WINDOWS.mddocs/INSTALL-PLATFORMS.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/FAQ.mdCHANGELOG.mdREADME.mdmarm-mcp-server/README.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/README.md
marm-mcp-server/marm_mcp_server/server.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep HTTP MCP tools listed in
MCP_TOOL_OPERATIONS; tools absent from this whitelist must not be exposed over HTTP.
Files:
marm-mcp-server/marm_mcp_server/server.py
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: All memory writes must use the serialized asynchronous write queue; do not add bypass paths.
A semantic-store failure duringmarm_log_entrymust never fail the corresponding log write.
Keep the memory database and isolated concept-graph database on separate connection pools; never share connections between them.
Graph and concept failures must never break the seven core memory tools.
marm_smart_recallmust keep primary memory ranking authoritative; graph enrichment is bounded, read-only, fail-open, and must be trimmed before primary results when enforcing response limits.
Use one lazy-loadedjinaai/jina-embeddings-v2-small-enencoder with 512 dimensions, serialized behind a lock; writes must succeed if the encoder is unavailable.
Keep orchestration in its current owner file and extract modules only at real boundaries consistent with the existing endpoint split.
Comments must be minimal, explain only non-obvious reasons, and never narrate the next line.
Files:
marm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/marm_mcp_server/services/cli_output.pymarm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/marm_mcp_server/services/runtime_status.pymarm-mcp-server/marm_mcp_server/config/settings.py
marm-mcp-server/{pyproject.toml,server.json,marm_mcp_server/__init__.py,marm_mcp_server/config/settings.py,marm_mcp_server/server.py,Dockerfile,docker-compose.yml}
📄 CodeRabbit inference engine (AGENTS.md)
When bumping the version, update every listed package, manifest, server, Docker, and configuration version occurrence, and audit with
python scripts/find-versions.py.
Files:
marm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/Dockerfilemarm-mcp-server/pyproject.tomlmarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/docker-compose.ymlmarm-mcp-server/server.jsonmarm-mcp-server/marm_mcp_server/config/settings.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
marm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/marm_mcp_server/services/cli_output.pymarm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/marm_mcp_server/services/runtime_status.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/tests/test_hybrid_search.py
marm-mcp-server/{server.py,server_stdio.py,server.json}
📄 CodeRabbit inference engine (AGENTS.md)
When adding or removing an MCP tool, update the endpoint implementation, HTTP route and whitelist, STDIO registration/wrapper, and
server.jsontools array.
Files:
marm-mcp-server/server.json
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Run tests withpytestfrommarm-mcp-server/; tests should exercise real FastAPI endpoints and real SQLite, using mocks only when they preserve at least 95% fidelity and provide meaningful speedup.
Every new MARM Console API route must have at least one happy-path FastAPI response-contract test with the MCP adapter stubbed.
Do not write existence-check or coded-to-pass tests; prefer deep tests exercising real paths over broad shallow coverage.
Usepytest.mark.skiponly for genuinely unavailable dependencies, never merely to avoid implementation effort.
Files:
marm-mcp-server/tests/test_hybrid_search.py
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path. Skip minor naming, comments, and layout preferences.
Files:
marm-mcp-server/tests/test_hybrid_search.py
🔇 Additional comments (23)
marm-mcp-server/marm_mcp_server/config/settings.py (2)
253-328: LGTM!
147-147: Version bump is already propagated to all mandated files.marm-mcp-server/marm_mcp_server/core/memory_scoring.py (1)
6-27: LGTM!Also applies to: 223-283
marm-mcp-server/tests/test_hybrid_search.py (1)
163-187: LGTM!Also applies to: 777-779, 813-923
CHANGELOG.md (1)
5-26: LGTM!README.md (1)
8-8: LGTM!Also applies to: 844-844, 982-987
docs/FAQ.md (1)
153-153: LGTM!marm-mcp-server/README.md (1)
10-10: LGTM!Also applies to: 844-844, 982-987
marm-mcp-server/marm_mcp_server/resources/marm-docs/FAQ.md (1)
153-153: LGTM!marm-mcp-server/marm_mcp_server/resources/marm-docs/README.md (1)
1-1: LGTM!Also applies to: 810-810, 948-953
marm-mcp-server/marm_mcp_server/services/runtime_status.py (1)
24-24: LGTM!Also applies to: 234-234
marm-mcp-server/marm_mcp_server/services/cli_output.py (1)
102-102: LGTM!docs/INSTALL-DOCKER.md (1)
5-5: LGTM!docs/INSTALL-LINUX.md (1)
5-5: LGTM!Also applies to: 323-323
docs/INSTALL-PLATFORMS.md (1)
1-1: LGTM!docs/INSTALL-WINDOWS.md (1)
5-5: LGTM!Also applies to: 297-297
docs/TECHNICAL-OVERVIEW.md (1)
3-3: LGTM!marm-mcp-server/Dockerfile (1)
76-76: LGTM!marm-mcp-server/docker-compose.yml (1)
8-8: LGTM!Also applies to: 21-21
marm-mcp-server/marm_mcp_server/__init__.py (1)
17-20: LGTM!marm-mcp-server/marm_mcp_server/server.py (1)
8-8: LGTM!marm-mcp-server/pyproject.toml (1)
7-7: LGTM!marm-mcp-server/server.json (1)
6-6: LGTM!Also applies to: 20-26
…uilder Addresses PR #118 review. The hot-path benchmark's "FTS Hits" column was wrong in two ways, both of which made it useless for judging exactly the breadth change this release ships: - It clamped and labelled against a hardcoded 50, so with the new default of 200 any query matching 50+ candidates reported a saturated "50/50" while a quarter of the real pool was filled. Cap and labels now derive from FTS_CANDIDATE_LIMIT. Measured effect: a 100-row corpus averages 85.8 candidates, previously reported as 50/50. - It counted matches for _safe_fts_query while timing recall_similar, which runs _wide_fts_query. The three benchmark queries are all non-exact, so the diagnostic had been describing a query the measured path never issues since v2.31.0. Now uses the same builder as the path being timed. Also replaces the FTS_LONE_HIT_SCORE clamp test, which re-derived max(0.0, min(1.0, ...)) in the test body rather than exercising real code, so a divergence between the two would not have been caught. The clamp and its warning now live in a _safe_unit_float helper the setting is built from and the test calls directly, covering out-of-range, unparseable, in-range, and unset input. Reloading settings to reach the constant is not usable here: it raises ImportError once another test has evicted the module, which is why _safe_choice and _csv_frozenset are structured the same way. Five other settings still inline the same clamp and are candidates for the helper, left alone as pre-existing and out of scope.
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 `@scripts/benchmarking/performance/bench_hotpath.py`:
- Line 303: Update the FTS hit counting and reporting logic around fts_hits and
the corresponding benchmark output to use the production effective candidate
cap, max(5, FTS_CANDIDATE_LIMIT), consistently. Apply the same cap at the
additional indicated occurrences so metrics and denominators match
recall_similar(..., limit=5).
In `@SECURITY.md`:
- Line 7: Replace the personal Gmail contact in SECURITY.md with a monitored
vulnerability-reporting alias on an organization-controlled domain. Use the
established security mailbox if one exists; otherwise, define an
organization-owned alias with appropriate monitoring and access controls.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 069b1fb8-1d1b-4ee5-a4f4-4a9ad49e554c
📒 Files selected for processing (4)
SECURITY.mdmarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/tests/test_hybrid_search.pyscripts/benchmarking/performance/bench_hotpath.py
🚧 Files skipped from review as they are similar to previous changes (2)
- marm-mcp-server/marm_mcp_server/config/settings.py
- marm-mcp-server/tests/test_hybrid_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,json,md,toml,yml,yaml,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Keep changes surgical: modify only what the task requires, match existing style, and preserve behavior during refactors.
Files:
SECURITY.mdscripts/benchmarking/performance/bench_hotpath.py
**/*.{md,py}
📄 CodeRabbit inference engine (AGENTS.md)
When adding or removing an MCP tool, update every full tool list and tool count in the README, protocol documentation, FAQ, and maintained
marm-docscopies; add tests covering both transports.
Files:
SECURITY.mdscripts/benchmarking/performance/bench_hotpath.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Never commit changes without an explicit user request.
Use semantic versioning: MAJOR for breaking changes, MINOR for new tools, parameters, or features, and PATCH for fixes and documentation updates.
Files:
SECURITY.mdscripts/benchmarking/performance/bench_hotpath.py
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Only flag documentation issues that are materially wrong, misleading for installation/release behavior, or inconsistent with live MCP behavior. Skip style, phrasing, formatting, and wording preferences.
Files:
SECURITY.md
scripts/benchmarking/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Do not publish performance claims that cannot be supported by the repository benchmark scripts.
Files:
scripts/benchmarking/performance/bench_hotpath.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
scripts/benchmarking/performance/bench_hotpath.py
🔇 Additional comments (2)
scripts/benchmarking/performance/bench_hotpath.py (2)
40-46: LGTM!
253-256: LGTM!
| Please report security issues privately by emailing: | ||
|
|
||
| **support@marmsystems.com** | ||
| **lyellr88@gmail.com** |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SECURITY.md relevant lines =="
if [ -f SECURITY.md ]; then
nl -ba SECURITY.md | sed -n '1,40p'
else
echo "SECURITY.md not found"
fi
echo
echo "== README/repository docs mentioning vulnerability/security reporting or email =="
rg -n --hidden -i 'security advisories|security policy|vulnerability|vulnerabilities|lyellr88@gmail.com|`@github`\.com|email|reporting' -g 'SECURITY.md' -g '*.md' -g 'README*' . 2>/dev/null || true
echo
echo "== repo metadata =="
git rev-parse --show-toplevel 2>/dev/null || true
git remote -v 2>/dev/null || trueRepository: Lyellr88/marm-memory
Length of output: 231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, re
path='SECURITY.md'
content=open(path, 'r', encoding='utf-8').read()
emails=re.findall(r'[\w.+-]+@[\w.-]+', content)
print("EMAILS_IN_SECURITY_MD")
for e in emails: print(e)
print("HAS_GMAIL_EQUIVALENT_OR_ORG_DOMAIN_MAILBOXS")
print(any(e.endswith('`@gmail.com`') for e in emails))
print("EMAIL_COUNT", len(emails))
PYRepository: Lyellr88/marm-memory
Length of output: 262
Security Misconfiguration (CWE-16)
Reachability: External
Use an organization-controlled vulnerability mailbox.
Use a monitored alias on an owned organization/project domain instead of lyellr88@gmail.com, unless this Gmail address is explicitly covered by documented monitoring, access control, retention, and continuity controls.
🤖 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 `@SECURITY.md` at line 7, Replace the personal Gmail contact in SECURITY.md
with a monitored vulnerability-reporting alias on an organization-controlled
domain. Use the established security mailbox if one exists; otherwise, define an
organization-owned alias with appropriate monitoring and access controls.
…e cap Second round of PR #118 review. - Report keyword-pool saturation against max(RECALL_LIMIT, FTS_CANDIDATE_LIMIT), the cap recall_similar actually applies, rather than FTS_CANDIDATE_LIMIT alone. Identical at any sane setting; they diverge only when the limit is configured below the requested top-K, where production still fetches top-K rows and the old denominator would have been one production never used. Both values are now named constants so the counter and the two labels cannot drift apart again, and the timed recalls use the same RECALL_LIMIT they report against. - Add a test that imports settings.py in a clean subprocess with FTS_LONE_HIT_SCORE set, asserting the constant the rest of the code imports and the stderr warning. The previous test exercised _safe_unit_float only, so changing the shipped assignment to _safe_float would have dropped the clamp with both tests still green; verified the new test fails on exactly that substitution while the helper test passes. A fresh interpreter is used because monkeypatching after import is too late and importlib.reload raises ImportError once another test has evicted the module. This also covers a non-default in-range value, which nothing previously did. - Stop describing the degenerate-set diagnostic as a count of LoCoMo or benchmark queries. 1,982 is the number of offline FTS calls the probe made; the harness scores 1,977 questions. Different filters, different numbers, and conflating them overstated what the benchmark measured. Docs and changelog already said this correctly; settings.py and the test docstring did not.
Addresses PR #118 review. The two full-scan calls passed a literal 5 while the production calls they are timed against used RECALL_LIMIT. No behavior change today since the constant is 5, but the two arms of a comparison should not read their top-K from different places: changing RECALL_LIMIT would have silently made the baseline and the production path request different limits, and the resulting speedup ratio would have compared two different workloads. Verified both timed paths still run: full scan 4.3ms/17.1ms and production hybrid 6.1ms/11.0ms at N=100/500, keyword pool unchanged at 85.8 and 200.0.
1 participant
v2.31.0 activated keyword candidates for natural-language recall but left keyword scores out of ranking, because the old 0.35 weight had been chosen while that path never ran. This sweeps it and turns it on.
Defaults, both measured on LoCoMo (1,977 questions, 5,882 memories, top-5):
Combined vs v2.31.0 on the same corpus: any-hit 57.4% -> 62.5%, all-hit 47.6% -> 52.6%, evidence recall 51.9% -> 56.9%. Every category improves.
Also fixes non-reproducible candidate selection, found because two identical benchmark runs scored 0.5pp apart while disagreeing on 18 questions. Ties at the candidate cutoff are the normal case (53.7% of queries) and SQLite ordered those rows differently between processes, so the pool varied across restarts. Candidate selection now breaks ties on memory ID. Accuracy is unchanged within measurement error; results are now reproducible, which also drops the benchmark's resolution floor from ~0.5pp to ~0.1pp. The fusion bake-off compares methods that can differ by less than the old floor, so it could not have been measured on the previous harness.
New setting FTS_LONE_HIT_SCORE (default 1.0) sets the keyword score for a candidate set too degenerate to rank. It ships as a no-op: swept over 0.0/0.3/0.5/1.0 with no measurable effect, because only 1 of 1,982 queries produced such a set on a store this size. Exposed for small stores, where a query matching exactly one memory is routine. The spec had called the previous hardcoded 1.0 an active ranking bug; measurement says otherwise, since under wide OR a degenerate set means one memory held the only match for any query term, which is discriminative rather than weak evidence.
No schema change, no migration, no re-embedding. Recall ordering changes by design; explicit env overrides are unaffected.
Summary by CodeRabbit
FTS_LONE_HIT_SCOREto control scoring for single/tied keyword matches.