From ef3054836e4a28be4889d71753e2d36623473982 Mon Sep 17 00:00:00 2001 From: Ryan Lyell Date: Fri, 24 Jul 2026 03:36:52 -0400 Subject: [PATCH 1/3] feat(recall): fuse BM25 into hybrid ranking, add command smoke suite, honest benchmarks (v2.29.0) Retrieval: - Fuse FTS5 BM25 into hybrid recall ranking (65% semantic / 35% lexical) instead of discarding the lexical score after candidate filtering. - Vectorize chunk-aware scoring into one matrix op (identical results). - Apply temporal decay consistently on the text-search fallback lane; the deterministic exact/lexical lane still preserves BM25 order. Reliability: - Lock the managed key file to the executing Windows identity (whoami) instead of an environment-derived username, fixing a create-then-read failure. Shared helper used by key init and auto HTTP key creation. Testing/tooling: - Add scripts/test-scripts/smoke-commands.py + pytest module covering the full marm-memory command surface; Docker and destructive tiers opt-in. - Restore module-generation isolation in load_isolated_server (conftest). Benchmarks: - bench_hotpath recall-scaling section times only shipped code paths (recall_similar, _fetch_and_score_embedding_rows) via one async dispatch; README performance tables refreshed from a single run. Co-Authored-By: Claude Opus 4.8 --- .gitignore | 3 + AGENTS.md | 1 + CHANGELOG.md | 23 + README.md | 49 ++- docs/INSTALL-DOCKER.md | 2 +- docs/INSTALL-LINUX.md | 4 +- docs/INSTALL-PLATFORMS.md | 2 +- docs/INSTALL-WINDOWS.md | 4 +- docs/TECHNICAL-OVERVIEW.md | 2 +- marm-mcp-server/Dockerfile | 2 +- marm-mcp-server/README.md | 2 +- marm-mcp-server/docker-compose.yml | 2 +- marm-mcp-server/marm-docs/README.md | 2 +- marm-mcp-server/marm_mcp_server/__init__.py | 4 +- .../marm_mcp_server/config/settings.py | 23 +- .../marm_mcp_server/core/memory_recall.py | 76 +++- .../marm_mcp_server/core/memory_scoring.py | 92 ++-- marm-mcp-server/marm_mcp_server/server.py | 2 +- .../services/key_management.py | 20 +- .../marm_mcp_server/utils/security.py | 29 ++ marm-mcp-server/pyproject.toml | 6 +- marm-mcp-server/server.json | 6 +- marm-mcp-server/tests/conftest.py | 11 +- marm-mcp-server/tests/test_chunking.py | 32 ++ marm-mcp-server/tests/test_command_smoke.py | 396 ++++++++++++++++++ marm-mcp-server/tests/test_hybrid_search.py | 90 +++- marm-mcp-server/tests/test_runtime_cli.py | 23 + .../tests/test_temporal_weighting.py | 97 +++++ .../benchmarking/performance/bench_hotpath.py | 191 +++------ scripts/test-scripts/README.md | 17 + scripts/test-scripts/smoke-commands.py | 66 +++ 31 files changed, 1015 insertions(+), 264 deletions(-) create mode 100644 marm-mcp-server/tests/test_command_smoke.py create mode 100644 scripts/test-scripts/smoke-commands.py diff --git a/.gitignore b/.gitignore index c400ef11..002c9dc2 100644 --- a/.gitignore +++ b/.gitignore @@ -31,6 +31,7 @@ __pycache__/ # MCP server database marm-mcp-server/*.db marm-mcp-server/marm_memory.db +marm-mcp-server/pip/ # Logs and runtime data logs/ @@ -117,6 +118,8 @@ docs/future/ marm-mcp-server/.pytest_cache/ marm-mcp-server/.pytest_tmp/ marm-mcp-server/.pytest_tmp*/ +marm-mcp-server/.pytest-review-*/ +marm-mcp-server/.pytest-smoke-*/ marm_usage_analytics.db dump.md dump2.md diff --git a/AGENTS.md b/AGENTS.md index f94afef9..d6fc2252 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -58,6 +58,7 @@ Semver: MAJOR = breaking (schema renames, parameter removals), MINOR = new tools ## Testing - Tests live in `marm-mcp-server/tests/`; run with `pytest` from `marm-mcp-server/`. +- Run `python scripts/test-scripts/smoke-commands.py` from the repo root for the local CLI smoke suite. It uses `smoke`, `smoke_lifecycle`, `smoke_docker`, and `smoke_destructive` markers. `--docker` and `--destructive` are explicit opt-ins; destructive mode uses a disposable virtual environment rather than the active package. - Hit real FastAPI endpoints and real SQLite. Mock only when it meaningfully speeds the test AND matches real behavior with at least 95% fidelity. - Every new MARM Console API route needs at least one happy-path FastAPI response-contract test with the MCP adapter stubbed. This verifies the actual response model without requiring a live graph backend. - No existence-check or coded-to-pass tests. Deep tests that exercise real paths beat broad shallow coverage. diff --git a/CHANGELOG.md b/CHANGELOG.md index c267c711..5ed2bcef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ ## Version 2 - MARM Protocol to Universal MCP Server Evolution +
+July 24th, 2026: Hybrid Recall Fusion, Windows Key Fix, and Command Smoke Suite (v2.29.0) + +### Hybrid Recall Now Fuses Lexical Relevance + +- Semantic recall now blends the FTS5 BM25 keyword score into ranking instead of using it only as a candidate pre-filter and then discarding it. On the hybrid path, relevance combines semantic similarity (65%) with the normalized BM25 score (35%) before temporal weighting, so exact-term matches such as identifiers, config keys, and error strings surface more reliably. This changes recall ordering; it is backward compatible and needs no migration. +- The chunk-aware scorer was vectorized into a single matrix operation. Results are identical to the previous per-chunk loop, with less per-query work on large scans. +- Temporal decay is now applied consistently on the text-search fallback lane, so newer results are preferred when the semantic model is unavailable. The deterministic exact/lexical lane still returns matches in BM25 order, unaffected by age. + +### Windows Managed-Key Reliability + +- The managed API-key file (`~/.marm/.env`) is now locked to the executing Windows identity (resolved via `whoami`) rather than an environment-derived username. This fixes a case where a key created under one resolved identity could not be read back by the same process. Both `key init` and automatic HTTP key creation use the same tested helper. + +### Local Command Smoke Suite + +- Added `scripts/test-scripts/smoke-commands.py` and a pytest module that exercise the entire `marm-memory` command surface: every help route, safe read-only dispatches, an isolated HTTP start/health/stop lifecycle, and a managed-key round trip. A real Docker lifecycle (`--docker`) and an uninstall/reinstall inside a disposable virtual environment (`--destructive`) are explicit opt-ins that never touch the active install. A static inventory check fails if a newly added command has no smoke coverage. + +### Benchmark Integrity + +- The recall-scaling benchmark now times only shipped code paths (`recall_similar` and `_fetch_and_score_embedding_rows`); the previous benchmark-local reimplementations were removed so published numbers reflect what a caller actually runs. Both compared paths use the same async dispatch and exclude the constant query-encode cost. The README performance tables were refreshed from a single run. + +
+
July 24th, 2026: Restored PyPI And Registry Publishing (v2.28.2) diff --git a/README.md b/README.md index df23107a..a55db5ea 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ width="900" height="250"> -

MARM: Local-First Persistent Multi-Agent Memory Layer for MCP Clients v2.28.2

+

MARM: Local-First Persistent Multi-Agent Memory Layer for MCP Clients v2.29.0

[![License](https://img.shields.io/badge/license-Apache--2.0-blue)](https://github.com/Lyellr88/marm-memory/blob/MARM-main/LICENSE) [![Python](https://img.shields.io/badge/python-3.10%2B-blue)](https://www.python.org/) @@ -159,7 +159,6 @@ When a managed key is active, `marm-memory console --import-key` opens a local C Use `marm-memory upgrade --check` to compare the installed package with PyPI. `marm-memory upgrade` previews a safe native upgrade; `--yes` performs it only where the active installer can be replaced safely. `marm-memory uninstall` similarly previews package removal and always preserves `~/.marm`, including memory databases, graph indexes, keys, logs, and configuration. On Windows, editable installs, or pipx installs, MARM prints the exact manual command rather than attempting to replace an active launcher. - ### Upgrade Existing Embeddings The Jina v2 Small default uses 512-dimensional embeddings; older `all-MiniLM-L6-v2` data is 384-dimensional and must be re-embedded after upgrading. Stop every MARM HTTP and STDIO process, then run: @@ -174,40 +173,46 @@ The command refuses to continue when it detects a live HTTP server, but STDIO pr MARM is tuned for fast recall first, even as memory grows and long memories are chunked behind the scenes. -These measurements use the fastembed-backed `jinaai/jina-embeddings-v2-small-en` encoder and a throwaway local SQLite database. +These measurements use the fastembed-backed `jinaai/jina-embeddings-v2-small-en` encoder and a throwaway local SQLite database. Every timed path calls the shipped `MARMMemory` code, not a benchmark-local reimplementation. All numbers below come from a single run of [`scripts/benchmarking/performance/bench_hotpath.py`](scripts/benchmarking/performance/bench_hotpath.py) on local hardware; absolute milliseconds vary by machine, so treat the scaling shape as the signal. ### 1. Retrieval Latency Scaling +End-to-end `recall_similar` latency (includes query encoding). + | Session Size ($N$) | Min Latency | Median Latency | p95 Latency | | :--- | :--- | :--- | :--- | -| **N = 100** | 6.6 ms | 7.4 ms | 8.0 ms | -| **N = 500** | 7.1 ms | 8.1 ms | 10.0 ms | -| **N = 1,000** | 7.6 ms | 8.5 ms | 9.0 ms | -| **N = 2,000** | 9.3 ms | 10.5 ms | 11.6 ms | -| **N = 4,000** | 11.5 ms | 12.1 ms | 13.4 ms | +| **N = 100** | 6.3 ms | 6.5 ms | 8.1 ms | +| **N = 500** | 7.2 ms | 7.4 ms | 8.0 ms | +| **N = 1,000** | 8.0 ms | 8.2 ms | 9.9 ms | +| **N = 2,000** | 9.2 ms | 9.7 ms | 10.8 ms | +| **N = 4,000** | 11.4 ms | 12.0 ms | 14.8 ms | ### 2. Encoder + Concurrency -- **Cold model load:** `887ms` -- **Warm encode:** median `4.0ms`, p95 `4.4ms` -- **Concurrent recall:** 10 gathered recalls completed in `616.3ms` vs `440.7ms` serial. The current path is intentionally serialized around shared encoder/SQLite work, so gathering calls does not create parallel speedup. +- **Cold model load:** `934ms` +- **Warm encode:** median `4.2ms`, p95 `4.8ms` +- **Concurrent recall:** 10 gathered recalls completed in `609.5ms` vs `411.3ms` serial. The current path is intentionally serialized around shared encoder/SQLite work, so gathering calls does not create parallel speedup. ### 3. Write-Time Ingestion Cost -- **Consolidation off:** median `6.8ms`, p95 `7.9ms` -- **Consolidation on:** median `51.3ms`, p95 `93.7ms` -- **Tradeoff:** write-time dedupe/clustering adds `7.6x` median cost so recall stays fast and cleaner over time. +- **Consolidation off:** median `5.9ms`, p95 `7.5ms` +- **Consolidation on:** median `61.2ms`, p95 `105.3ms` +- **Tradeoff:** write-time dedupe/clustering adds `10.3x` median cost so recall stays fast and cleaner over time. -### 4. Hybrid Search Scaling +### 4. Recall Scaling: Full Scan vs Production Hybrid -| Session Size ($N$) | Pure Semantic | Production Hybrid | FTS Filter -> Rerank | Speedup vs Pure | -| :--- | :--- | :--- | :--- | :--- | -| **N = 100** | 2.3 ms | 7.9 ms | 1.9 ms | 1.2x | -| **N = 1,000** | 23.1 ms | 9.2 ms | 2.5 ms | 9.2x | -| **N = 4,000** | 106.5 ms | 13.2 ms | 4.7 ms | 22.8x | -| **N = 10,000** | 267.1 ms | 13.6 ms | 5.3 ms | 50.4x | +Why recall stays roughly flat as memory grows: instead of scoring every stored vector, production recall uses an FTS keyword pre-filter to a bounded candidate set, then re-ranks that set by semantic + BM25 + temporal score. Both columns are real code paths (`_fetch_and_score_embedding_rows` for the full scan, `recall_similar` for hybrid), dispatched through the same async path and timed with the query vector precomputed so the constant encode cost from section 1 is excluded from both. -These Jina v2 Small benchmarks used a throwaway real SQLite database and the live configured encoder on local hardware. Reproduce them: [`scripts/benchmarking/performance/bench_hotpath.py`](scripts/benchmarking/performance/bench_hotpath.py) +| Session Size ($N$) | Full Semantic Scan | Production Hybrid | Speedup | +| :--- | :--- | :--- | :--- | +| **N = 100** | 3.3 ms | 4.0 ms | 0.8x | +| **N = 500** | 15.8 ms | 4.4 ms | 3.6x | +| **N = 1,000** | 33.5 ms | 6.4 ms | 5.2x | +| **N = 2,000** | 69.3 ms | 6.3 ms | 11.0x | +| **N = 4,000** | 127.3 ms | 7.2 ms | 17.7x | +| **N = 10,000** | 320.6 ms | 8.7 ms | 37.0x | + +The full scan grows roughly linearly with $N$ while hybrid recall stays near-flat, so the advantage widens with session size. At very small $N$ the pre-filter overhead is not yet worth it (hybrid is marginally slower at N = 100); the win appears once there is enough to skip. Reproduce with [`scripts/benchmarking/performance/bench_hotpath.py`](scripts/benchmarking/performance/bench_hotpath.py). ### 5. LoCoMo Retrieval Accuracy diff --git a/docs/INSTALL-DOCKER.md b/docs/INSTALL-DOCKER.md index 82f9f634..0af0d47b 100644 --- a/docs/INSTALL-DOCKER.md +++ b/docs/INSTALL-DOCKER.md @@ -2,7 +2,7 @@ ## Universal Memory Intelligence Platform for AI Agents -**MARM v2.28.2** - Memory Accurate Response Mode +**MARM v2.29.0** - Memory Accurate Response Mode *Docker deployment guide for Windows, Mac, and Linux* --- diff --git a/docs/INSTALL-LINUX.md b/docs/INSTALL-LINUX.md index efd5957a..fc5deb25 100644 --- a/docs/INSTALL-LINUX.md +++ b/docs/INSTALL-LINUX.md @@ -2,7 +2,7 @@ ## Universal Memory Intelligence Platform for AI Agents -**MARM v2.28.2** - Memory Accurate Response Mode +**MARM v2.29.0** - Memory Accurate Response Mode *Complete Linux installation guide* --- @@ -320,7 +320,7 @@ curl -s http://localhost:8001/health { "status": "healthy", "service": "MARM MCP Server", - "version": "2.28.2", + "version": "2.29.0", "timestamp": "2026-01-01T00:00:00+00:00", "database": "connected", "semantic_search": "available" diff --git a/docs/INSTALL-PLATFORMS.md b/docs/INSTALL-PLATFORMS.md index 68374605..b8081ae2 100644 --- a/docs/INSTALL-PLATFORMS.md +++ b/docs/INSTALL-PLATFORMS.md @@ -1,4 +1,4 @@ -# MARM v2.28.2 MCP Server - Platform Integration Guide +# MARM v2.29.0 MCP Server - Platform Integration Guide ## Table of Contents diff --git a/docs/INSTALL-WINDOWS.md b/docs/INSTALL-WINDOWS.md index 986bbe7f..d6f4efa8 100644 --- a/docs/INSTALL-WINDOWS.md +++ b/docs/INSTALL-WINDOWS.md @@ -2,7 +2,7 @@ ## Universal Memory Intelligence Platform for AI Agents -**MARM v2.28.2** - Memory Accurate Response Mode +**MARM v2.29.0** - Memory Accurate Response Mode *Complete Windows installation guide* --- @@ -294,7 +294,7 @@ Invoke-WebRequest -Uri http://localhost:8001/health { "status": "healthy", "service": "MARM MCP Server", - "version": "2.28.2", + "version": "2.29.0", "timestamp": "2026-01-01T00:00:00+00:00", "database": "connected", "semantic_search": "available" diff --git a/docs/TECHNICAL-OVERVIEW.md b/docs/TECHNICAL-OVERVIEW.md index f8319b65..8f4f4767 100644 --- a/docs/TECHNICAL-OVERVIEW.md +++ b/docs/TECHNICAL-OVERVIEW.md @@ -1,6 +1,6 @@ # MARM Technical Overview -> Current implementation: MARM MCP Server v2.28.2 +> Current implementation: MARM MCP Server v2.29.0 This document explains what MARM is, why it is built this way, and how information moves through the system from an agent writing something to that information being recalled later. It is intended as a technical product overview, not a source-code reference. diff --git a/marm-mcp-server/Dockerfile b/marm-mcp-server/Dockerfile index ccaa8299..98d43ab8 100644 --- a/marm-mcp-server/Dockerfile +++ b/marm-mcp-server/Dockerfile @@ -73,7 +73,7 @@ HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \ LABEL org.opencontainers.image.title="MARM Universal MCP Server" LABEL org.opencontainers.image.description="Production-ready Universal MCP Server with advanced AI memory capabilities, semantic search, and professional-grade architecture" -LABEL org.opencontainers.image.version="2.28.2" +LABEL org.opencontainers.image.version="2.29.0" LABEL org.opencontainers.image.authors="Ryan Lyell - marm-memory" LABEL org.opencontainers.image.url="https://marmsystems.com" LABEL org.opencontainers.image.source="https://github.com/Lyellr88/marm-memory" diff --git a/marm-mcp-server/README.md b/marm-mcp-server/README.md index 2093d06f..8679be54 100644 --- a/marm-mcp-server/README.md +++ b/marm-mcp-server/README.md @@ -7,7 +7,7 @@ mcp-name: io.github.Lyellr88/marm-mcp-server width="900" height="250"> -

MARM: Local-First Persistent Multi-Agent Memory Layer for MCP Clients v2.28.2

+

MARM: Local-First Persistent Multi-Agent Memory Layer for MCP Clients v2.29.0

[![License](https://img.shields.io/badge/license-Apache--2.0-blue)](https://github.com/Lyellr88/marm-memory/blob/MARM-main/LICENSE) [![Python](https://img.shields.io/badge/python-3.10%2B-blue)](https://www.python.org/) diff --git a/marm-mcp-server/docker-compose.yml b/marm-mcp-server/docker-compose.yml index f55bb2e5..0a569411 100644 --- a/marm-mcp-server/docker-compose.yml +++ b/marm-mcp-server/docker-compose.yml @@ -18,7 +18,7 @@ services: environment: - SERVER_HOST=0.0.0.0 - SERVER_PORT=8001 - - SERVER_VERSION=2.28.2 + - SERVER_VERSION=2.29.0 - ENVIRONMENT=production - LOG_LEVEL=INFO diff --git a/marm-mcp-server/marm-docs/README.md b/marm-mcp-server/marm-docs/README.md index 09e7768a..2973e909 100644 --- a/marm-mcp-server/marm-docs/README.md +++ b/marm-mcp-server/marm-docs/README.md @@ -1,4 +1,4 @@ -# MARM: Local-First Persistent Multi-Agent Memory Layer for MCP Clients v2.28.2 +# MARM: Local-First Persistent Multi-Agent Memory Layer for MCP Clients v2.29.0 ## Table of Contents diff --git a/marm-mcp-server/marm_mcp_server/__init__.py b/marm-mcp-server/marm_mcp_server/__init__.py index 5d48a418..a1323432 100644 --- a/marm-mcp-server/marm_mcp_server/__init__.py +++ b/marm-mcp-server/marm_mcp_server/__init__.py @@ -14,10 +14,10 @@ - Production-grade performance Author: Ryan Lyell - marm-memory -Version: 2.28.2 +Version: 2.29.0 """ -__version__ = "2.28.2" +__version__ = "2.29.0" __author__ = "Ryan Lyell" __email__ = "lyell@marmsystems.com" diff --git a/marm-mcp-server/marm_mcp_server/config/settings.py b/marm-mcp-server/marm_mcp_server/config/settings.py index 02d5f144..5cc98913 100644 --- a/marm-mcp-server/marm_mcp_server/config/settings.py +++ b/marm-mcp-server/marm_mcp_server/config/settings.py @@ -5,7 +5,7 @@ import sys from pathlib import Path -from ..utils.security import generate_api_key +from ..utils.security import generate_api_key, restrict_windows_file_to_current_user def _safe_int(env_key: str, default: int) -> int: @@ -122,7 +122,7 @@ def get_analytics_db_path(): f"WARNING: SERVER_PORT={_raw_port} out of [1, 65535], clamped to {SERVER_PORT}", file=sys.stderr, ) -SERVER_VERSION = "2.28.2" +SERVER_VERSION = "2.29.0" GRAPH_ENABLED = os.environ.get("GRAPH_ENABLED", "true").lower() != "false" @@ -398,24 +398,7 @@ def _load_key_from_file() -> str: except OSError: pass if sys.platform == "win32": - try: - import getpass - import subprocess - - user = getpass.getuser() - subprocess.run( - [ - "icacls", - str(_MARM_ENV_PATH), - "/inheritance:r", - "/grant:r", - f"{user}:(F)", - ], - check=False, - capture_output=True, - ) - except Exception: - pass + restrict_windows_file_to_current_user(_MARM_ENV_PATH) except Exception as _e: print(f"WARNING: Could not save API key to {_MARM_ENV_PATH}: {_e}") diff --git a/marm-mcp-server/marm_mcp_server/core/memory_recall.py b/marm-mcp-server/marm_mcp_server/core/memory_recall.py index 9f6b4373..1a3c2b92 100644 --- a/marm-mcp-server/marm_mcp_server/core/memory_recall.py +++ b/marm-mcp-server/marm_mcp_server/core/memory_recall.py @@ -9,6 +9,7 @@ TEMPORAL_WEIGHT, TEMPORAL_HALF_LIFE_DAYS, FTS_CANDIDATE_LIMIT, + HYBRID_SEARCH_TEXT_WEIGHT, ) from .memory_utils import ( _safe_print, @@ -187,7 +188,13 @@ def _wrap(results, truncated): _recall_debug("semantic model unavailable → text-search fallback") return _wrap( await _recall_text_search( - mem, query, session, limit, project=project, platform=platform + mem, + query, + session, + limit, + project=project, + platform=platform, + apply_temporal=True, ), False, ) @@ -199,10 +206,10 @@ def _wrap(results, truncated): query_embedding = await asyncio.to_thread(mem._encode_sync, query) fts_query = _safe_fts_query(query) - candidate_ids: list[str] = [] + candidates: list[tuple[str, float]] = [] if fts_query: try: - candidate_ids = await asyncio.to_thread( + candidates = await asyncio.to_thread( _fetch_fts_candidate_ids, mem.db_path, session, @@ -212,7 +219,7 @@ def _wrap(results, truncated): platform, ) _recall_debug( - f"FTS filter: {len(candidate_ids)} candidates for '{fts_query}'" + f"FTS filter: {len(candidates)} candidates for '{fts_query}'" ) except Exception as e: _safe_print( @@ -220,12 +227,13 @@ def _wrap(results, truncated): ) _recall_debug("FTS filter failed → semantic fallback") + bm25_by_id = dict(candidates) use_semantic_fallback = True - if candidate_ids: + if candidates: similarities, dim_skipped = await asyncio.to_thread( _fetch_and_score_by_ids, mem.db_path, - candidate_ids, + [cid for cid, _ in candidates], query_embedding, ) if similarities: @@ -257,12 +265,24 @@ def _wrap(results, truncated): f"recall_similar: skipped {dim_skipped} memories with wrong embedding dimension (expected {len(query_embedding)})" ) + # Lexical fusion only applies on the FTS filter->rerank path, where every + # scored row has a BM25 score. The semantic fallback scan has no lexical + # signal, so it keeps the pure semantic+temporal blend. + apply_bm25 = not use_semantic_fallback and bool(bm25_by_id) + combined: dict[str, tuple] = {} for mem_row, vec_score in similarities: t_score = _temporal_score(mem_row["timestamp"], TEMPORAL_HALF_LIFE_DAYS) + if apply_bm25: + bm25_score = bm25_by_id.get(mem_row["id"], 0.0) + relevance = ( + 1 - HYBRID_SEARCH_TEXT_WEIGHT + ) * vec_score + HYBRID_SEARCH_TEXT_WEIGHT * bm25_score + else: + relevance = vec_score combined[mem_row["id"]] = ( mem_row, - (1 - TEMPORAL_WEIGHT) * vec_score + TEMPORAL_WEIGHT * t_score, + (1 - TEMPORAL_WEIGHT) * relevance + TEMPORAL_WEIGHT * t_score, ) ranked = sorted(combined.values(), key=lambda x: x[1], reverse=True)[:limit] @@ -292,7 +312,13 @@ def _wrap(results, truncated): _recall_debug(f"semantic search exception → text-search fallback: {e}") return _wrap( await _recall_text_search( - mem, query, session, limit, project=project, platform=platform + mem, + query, + session, + limit, + project=project, + platform=platform, + apply_temporal=True, ), False, ) @@ -305,24 +331,42 @@ async def _recall_text_search( limit: int = 5, project: str = None, platform: str = None, + apply_temporal: bool = False, ) -> List[Dict]: - """Text search via FTS5 BM25 ranking, with LIKE fallback for unsanitizable queries.""" + """Text search via FTS5 BM25 ranking, with LIKE fallback for unsanitizable queries. + + apply_temporal blends recency into the score and re-ranks by it. It is opt-in + because the exact lane (_recall_exact) must return lexical hits in BM25 order, + unaffected by age; only the semantic-intent fallbacks turn it on so their + ranking stays consistent with the main semantic lane. + """ _recall_debug(f"text-search path: query='{query[:50]}', session={session}") + + def _blend_temporal(base_sim: float, timestamp: str) -> float: + if not apply_temporal: + return base_sim + t_score = _temporal_score(timestamp, TEMPORAL_HALF_LIFE_DAYS) + return (1 - TEMPORAL_WEIGHT) * base_sim + TEMPORAL_WEIGHT * t_score + fts_query = _safe_fts_query(query) if fts_query is not None: + # Temporal re-ranking must see beyond the top-`limit` BM25 rows, or a + # newer result ranked just outside the cutoff could never be promoted. + # The exact lane keeps the requested limit (pure BM25 order). + fetch_limit = max(limit, FTS_CANDIDATE_LIMIT) if apply_temporal else limit try: fts_rows = await asyncio.to_thread( _fetch_and_score_fts_rows, mem.db_path, session, fts_query, - limit, + fetch_limit, project, platform, ) if fts_rows: _recall_debug(f"FTS5 returned {len(fts_rows)} results") - return [ + results = [ { "id": row["id"], "session_name": row["session_name"], @@ -332,13 +376,17 @@ async def _recall_text_search( "metadata": json.loads(row["metadata"]) if row["metadata"] else {}, - "similarity": float(score), + "similarity": _blend_temporal(float(score), row["timestamp"]), "project": row["project"], "platform": row["platform"], "retrieval_mode": "exact_fts", } for row, score in fts_rows ] + if apply_temporal: + results.sort(key=lambda r: r["similarity"], reverse=True) + results = results[:limit] + return results except Exception as e: _safe_print(f"FTS5 search failed, falling back to LIKE: {e}") _recall_debug("FTS5 failed → LIKE fallback") @@ -376,11 +424,13 @@ async def _recall_text_search( "timestamp": row[3], "context_type": row[4], "metadata": json.loads(row[5]) if row[5] else {}, - "similarity": 0.8, + "similarity": _blend_temporal(0.8, row[3]), "project": row[6], "platform": row[7], "retrieval_mode": "exact_like", } ) + if apply_temporal: + results.sort(key=lambda r: r["similarity"], reverse=True) return results diff --git a/marm-mcp-server/marm_mcp_server/core/memory_scoring.py b/marm-mcp-server/marm_mcp_server/core/memory_scoring.py index d748b763..c9fffaf1 100644 --- a/marm-mcp-server/marm_mcp_server/core/memory_scoring.py +++ b/marm-mcp-server/marm_mcp_server/core/memory_scoring.py @@ -4,6 +4,21 @@ import numpy as np +def _normalize_bm25(raw_scores: list[float]) -> list[float]: + """Map raw BM25 scores (more-negative = better) to [0, 1] with 1.0 best. + + Single-row or all-equal result sets collapse to 1.0 so a lone lexical hit + still contributes its full text weight. + """ + if not raw_scores: + return [] + min_s, max_s = min(raw_scores), max(raw_scores) + if max_s == min_s: + return [1.0 for _ in raw_scores] + span = max_s - min_s + return [(max_s - s) / span for s in raw_scores] + + def _score_embedding_rows(rows, query_embedding, limit: int): """Score embedding rows in one NumPy batch instead of a Python cosine loop.""" if limit <= 0: @@ -67,32 +82,16 @@ def _score_chunk_aware( normalized_query = query_vec / query_norm dim_skipped = 0 - results = [] - - for mem in memories: - mem_id = mem["id"] - chunk_embs = chunks_by_id.get(mem_id) - - if chunk_embs: - best_score = None - for emb_bytes in chunk_embs: - try: - vec = np.frombuffer(emb_bytes, dtype=np.float32) - except Exception: - continue - if vec.shape[0] != expected_dim: - dim_skipped += 1 - continue - norm = np.linalg.norm(vec) - if norm == 0: - continue - score = float(np.dot(vec / norm, normalized_query)) - if best_score is None or score > best_score: - best_score = score - if best_score is not None: - results.append((mem, best_score)) - else: - emb_bytes = mem["embedding"] + vectors: list[np.ndarray] = [] + owners: list[int] = [] + + for mem_index, mem in enumerate(memories): + chunk_embs = chunks_by_id.get(mem["id"]) + # Chunked memories score max-over-chunks; unchunked fall back to the + # parent embedding. A memory never mixes both. + candidate_embs = chunk_embs if chunk_embs else [mem["embedding"]] + + for emb_bytes in candidate_embs: if emb_bytes is None: continue try: @@ -105,8 +104,25 @@ def _score_chunk_aware( norm = np.linalg.norm(vec) if norm == 0: continue - results.append((mem, float(np.dot(vec / norm, normalized_query)))) + vectors.append(vec / norm) + owners.append(mem_index) + if not vectors: + return [], dim_skipped + + matrix = np.vstack(vectors).astype(np.float32, copy=False) + scores = matrix @ normalized_query + + # Collapse to one score per memory (max over its chunk rows). + owner_arr = np.asarray(owners) + best_scores = np.full(len(memories), -np.inf, dtype=np.float32) + np.maximum.at(best_scores, owner_arr, scores) + + results = [ + (memories[i], float(best_scores[i])) + for i in range(len(memories)) + if best_scores[i] != -np.inf + ] results.sort(key=lambda x: x[1], reverse=True) return results, dim_skipped @@ -203,13 +219,7 @@ def _fetch_and_score_fts_rows( if not rows: return [] - raw_scores = [row["score"] for row in rows] - min_s, max_s = min(raw_scores), max(raw_scores) - if max_s == min_s: - normalized = [1.0 for _ in raw_scores] - else: - span = max_s - min_s - normalized = [(max_s - s) / span for s in raw_scores] + normalized = _normalize_bm25([row["score"] for row in rows]) return list(zip(rows, normalized)) @@ -220,12 +230,16 @@ def _fetch_fts_candidate_ids( limit: int, project: str | None = None, platform: str | None = None, -) -> list[str]: - """Return top N memory IDs from FTS5 by BM25 rank. No scoring needed.""" +) -> list[tuple[str, float]]: + """Return top N (memory_id, normalized_bm25) from FTS5 by BM25 rank. + + The BM25 score is normalized to [0, 1] (1.0 = best lexical match) so callers + can fuse the lexical signal into the semantic blend instead of discarding it. + """ conn = sqlite3.connect(db_path, timeout=30.0) try: base = """ - SELECT m.id + SELECT m.id, bm25(memories_fts) AS score FROM memories_fts JOIN memories m ON memories_fts.rowid = m.rowid WHERE memories_fts MATCH ? @@ -244,10 +258,12 @@ def _fetch_fts_candidate_ids( base += " ORDER BY bm25(memories_fts) LIMIT ?" params.append(limit) rows = conn.execute(base, params).fetchall() - return [row[0] for row in rows] finally: conn.close() + normalized = _normalize_bm25([row[1] for row in rows]) + return [(row[0], score) for row, score in zip(rows, normalized)] + def _fetch_and_score_by_ids( db_path: str, diff --git a/marm-mcp-server/marm_mcp_server/server.py b/marm-mcp-server/marm_mcp_server/server.py index a5fe174c..c92b5e5c 100644 --- a/marm-mcp-server/marm_mcp_server/server.py +++ b/marm-mcp-server/marm_mcp_server/server.py @@ -5,7 +5,7 @@ FastAPI application, compliant with the MCP protocol via FastApiMCP. Author: Lyell - marm-memory -Version: 2.28.2 +Version: 2.29.0 """ import os diff --git a/marm-mcp-server/marm_mcp_server/services/key_management.py b/marm-mcp-server/marm_mcp_server/services/key_management.py index b6657eef..0c50af08 100644 --- a/marm-mcp-server/marm_mcp_server/services/key_management.py +++ b/marm-mcp-server/marm_mcp_server/services/key_management.py @@ -2,12 +2,12 @@ from __future__ import annotations -import sys import os import stat +import sys from pathlib import Path -from ..utils.security import generate_api_key +from ..utils.security import generate_api_key, restrict_windows_file_to_current_user def managed_key_path() -> Path: @@ -42,21 +42,7 @@ def _protect_key_file(path: Path) -> bool: except OSError: return False try: - import getpass - import subprocess - - result = subprocess.run( - [ - "icacls", - str(path), - "/inheritance:r", - "/grant:r", - f"{getpass.getuser()}:(F)", - ], - check=False, - capture_output=True, - ) - return result.returncode == 0 + return restrict_windows_file_to_current_user(path) except OSError: return False diff --git a/marm-mcp-server/marm_mcp_server/utils/security.py b/marm-mcp-server/marm_mcp_server/utils/security.py index c7295d27..45310510 100644 --- a/marm-mcp-server/marm_mcp_server/utils/security.py +++ b/marm-mcp-server/marm_mcp_server/utils/security.py @@ -2,6 +2,9 @@ import secrets import string +import subprocess +import sys +from pathlib import Path def generate_api_key(length: int = 40) -> str: @@ -17,3 +20,29 @@ def generate_api_key(length: int = 40) -> str: key += [secrets.choice(alphabet) for _ in range(length - 4)] secrets.SystemRandom().shuffle(key) return "".join(key) + + +def restrict_windows_file_to_current_user(path: Path) -> bool: + """Grant the executing Windows identity exclusive access to a sensitive file.""" + if sys.platform != "win32": + return True + try: + identity = subprocess.run( + ["whoami"], check=False, capture_output=True, text=True + ).stdout.strip() + if not identity: + return False + result = subprocess.run( + [ + "icacls", + str(path), + "/inheritance:r", + "/grant:r", + f"{identity}:(F)", + ], + check=False, + capture_output=True, + ) + return result.returncode == 0 + except OSError: + return False diff --git a/marm-mcp-server/pyproject.toml b/marm-mcp-server/pyproject.toml index f479f7b1..ea84d464 100644 --- a/marm-mcp-server/pyproject.toml +++ b/marm-mcp-server/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "marm-mcp-server" -version = "2.28.2" +version = "2.29.0" description = "Local-first 3-in-1 AI memory layer & MCP server for Claude Code, Codex, Grok, Gemini, VS Code and Cursor. Fuses session history, codebase indices & concept graphs in SQLite. Enables zero-cloud, privacy-first context & instant recall also works with multi-agent swarms." readme = "README.md" license = "Apache-2.0" @@ -109,6 +109,10 @@ addopts = "-p no:cacheprovider" markers = [ "docker: Docker transport tests skipped by scripts/run-tests.py unless --docker is used", "slow_stdio: subprocess-based STDIO transport tests skipped by scripts/run-tests.py --fast", + "smoke: local command parse, dispatch, and isolated lifecycle smoke tests", + "smoke_lifecycle: HTTP runtime and managed-key lifecycle smoke tests", + "smoke_docker: smoke tests that launch a real Docker container (need a daemon)", + "smoke_destructive: smoke tests that uninstall/reinstall MARM in a disposable virtual environment", ] [tool.ruff] diff --git a/marm-mcp-server/server.json b/marm-mcp-server/server.json index 3626b068..256f85cf 100644 --- a/marm-mcp-server/server.json +++ b/marm-mcp-server/server.json @@ -3,7 +3,7 @@ "_schema_date": "2025-12-11", "name": "io.github.Lyellr88/marm-mcp-server", "description": "Universal MCP Server with advanced AI memory capabilities and semantic search.", - "version": "2.28.2", + "version": "2.29.0", "author": "Ryan Lyell - marm-memory", "license": "Apache-2.0", "homepage": "https://marmsystems.com", @@ -17,12 +17,12 @@ { "registryType": "pypi", "identifier": "marm-mcp-server", - "version": "2.28.2", + "version": "2.29.0", "transport": { "type": "stdio" } }, { "registryType": "oci", - "identifier": "lyellr88/marm-mcp-server:2.28.2", + "identifier": "lyellr88/marm-mcp-server:2.29.0", "transport": { "type": "stdio" } } ], diff --git a/marm-mcp-server/tests/conftest.py b/marm-mcp-server/tests/conftest.py index d0641576..536a927a 100644 --- a/marm-mcp-server/tests/conftest.py +++ b/marm-mcp-server/tests/conftest.py @@ -58,10 +58,17 @@ def isolated_cbm_store(tmp_path_factory): def load_isolated_server(monkeypatch, tmp_path, api_key="", write_queue_enabled=False): - """Import the server after pointing global state at a temporary database.""" + """Import the server after pointing global state at a temporary database. + + Modules are dropped via monkeypatch.delitem, not a bare del, so the original + module objects are restored at teardown. Otherwise the isolated re-import + below leaves a new module generation in sys.modules for the rest of the + session, and later tests that bound symbols (e.g. MARMMemory) at import time + would silently reference the stale generation. + """ for name in list(sys.modules): if name == "marm_mcp_server" or name.startswith("marm_mcp_server."): - del sys.modules[name] + monkeypatch.delitem(sys.modules, name) monkeypatch.setenv("MARM_DB_PATH", str(tmp_path / "marm_memory.db")) monkeypatch.setenv("MARM_ANALYTICS_DB_PATH", str(tmp_path / "analytics.db")) diff --git a/marm-mcp-server/tests/test_chunking.py b/marm-mcp-server/tests/test_chunking.py index 5c1b5257..a4e353ca 100644 --- a/marm-mcp-server/tests/test_chunking.py +++ b/marm-mcp-server/tests/test_chunking.py @@ -222,6 +222,38 @@ def test_score_chunk_aware_handles_mixed_chunked_and_unchunked(): assert len(results) == 2 +def test_score_chunk_aware_batched_path_counts_wrong_dim_chunk_as_skipped(): + """Batched scoring must still skip a wrong-dimension chunk, score the + remaining good chunk, and count the skip -- the collapse cannot silently + drop dim mismatches.""" + unit_vec = _make_unit_vec() + wrong_dim = np.ones(DEFAULT_SEMANTIC_DIM + 1, dtype=np.float32) + wrong_dim /= np.linalg.norm(wrong_dim) + mem_id = str(uuid.uuid4()) + row = _make_sqlite_row(mem_id, None) + chunks_by_id = {mem_id: [wrong_dim.tobytes(), unit_vec.tobytes()]} + + results, skipped = _score_chunk_aware([row], chunks_by_id, unit_vec) + + assert skipped == 1 + assert len(results) == 1 + assert abs(results[0][1] - 1.0) < 1e-4 + + +def test_score_chunk_aware_batched_path_excludes_wrong_dim_parent(): + """A memory whose only embedding is the wrong dimension is excluded and + counted as skipped, even in the batched path.""" + unit_vec = _make_unit_vec() + wrong_dim = np.ones(DEFAULT_SEMANTIC_DIM + 1, dtype=np.float32) + mem_id = str(uuid.uuid4()) + row = _make_sqlite_row(mem_id, wrong_dim.tobytes()) + + results, skipped = _score_chunk_aware([row], {}, unit_vec) + + assert results == [] + assert skipped == 1 + + # --- DB schema tests --- diff --git a/marm-mcp-server/tests/test_command_smoke.py b/marm-mcp-server/tests/test_command_smoke.py new file mode 100644 index 00000000..a3806ee3 --- /dev/null +++ b/marm-mcp-server/tests/test_command_smoke.py @@ -0,0 +1,396 @@ +"""Local smoke coverage for the complete marm-memory command surface.""" + +from __future__ import annotations + +import argparse +import os +import re +import socket +import subprocess +import sys +import time +import urllib.error +import urllib.request +import uuid +import venv +from pathlib import Path + +import pytest + + +PACKAGE_ROOT = Path(__file__).resolve().parents[1] +TRACEBACK = "Traceback (most recent call last)" +PRODUCT_ENTRYPOINT = ( + "import sys\n" + "from marm_mcp_server.cli import main\n" + "sys.argv = ['marm-memory', *sys.argv[1:]]\n" + "main()\n" +) + +# This is deliberately static. A parser-derived list would approve a newly added +# command without requiring an explicit smoke decision. +COMMAND_HELP_PATHS = ( + ("start",), + ("http",), + ("fast-start-http",), + ("stdio",), + ("stop",), + ("restart",), + ("status",), + ("console",), + ("logs",), + ("doctor",), + ("knowledge",), + ("knowledge", "status"), + ("knowledge", "build"), + ("projects",), + ("projects", "list"), + ("projects", "index"), + ("projects", "status"), + ("projects", "remove"), + ("maintenance",), + ("maintenance", "status"), + ("maintenance", "embeddings"), + ("maintenance", "embeddings", "migrate"), + ("key",), + ("key", "generate"), + ("key", "init"), + ("key", "path"), + ("key", "reveal"), + ("docker",), + ("docker", "status"), + ("docker", "pull"), + ("docker", "run"), + ("docker", "command"), + ("docker", "compose"), + ("docker", "stdio-command"), + ("docker", "logs"), + ("docker", "stop"), + ("docker", "upgrade"), + ("docker", "maintenance"), + ("docker", "maintenance", "embeddings"), + ("docker", "maintenance", "embeddings", "migrate"), + ("upgrade",), + ("update",), + ("uninstall",), + ("version",), +) + +SAFE_DISPATCHES = ( + ("version",), + ("status",), + ("logs", "--lines", "1"), + ("doctor", "--json"), + ("knowledge", "status"), + ("maintenance", "status", "--json"), + ("key", "generate"), + ("key", "path"), + ("uninstall",), + ("docker", "command"), + ("docker", "compose"), + ("docker", "stdio-command"), + ("docker", "run", "--dry-run"), +) +SAFE_DISPATCH_EXIT_CODES = { + ("doctor", "--json"): {0, 1}, +} + + +def _free_port() -> int: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: + sock.bind(("127.0.0.1", 0)) + return int(sock.getsockname()[1]) + + +def _isolated_environment(tmp_path: Path, *, port: int | None = None) -> dict[str, str]: + home = tmp_path / "home" + (home / ".marm").mkdir(parents=True) + environment = os.environ.copy() + environment.update( + { + "HOME": str(home), + "USERPROFILE": str(home), + "MARM_DB_PATH": str(home / ".marm" / "memory.db"), + "MARM_ANALYTICS_DB_PATH": str(home / ".marm" / "analytics.db"), + "MARM_CONCEPT_DB_PATH": str(home / ".marm" / "index" / "concepts.db"), + "MARM_DOCS_DB_PATH": str(home / ".marm" / "docs" / "marm_docs.db"), + "PIP_CACHE_DIR": str(tmp_path / "pip-cache"), + "SERVER_HOST": "127.0.0.1", + "SERVER_PORT": str(port or _free_port()), + } + ) + environment.pop("MARM_API_KEY", None) + return environment + + +def _run_product( + arguments: tuple[str, ...] | list[str], + environment: dict[str, str], + *, + python_executable: Path | None = None, + cwd: Path | None = None, + timeout: int = 30, +) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [ + str(python_executable or sys.executable), + "-c", + PRODUCT_ENTRYPOINT, + *arguments, + ], + cwd=cwd or PACKAGE_ROOT, + env=environment, + capture_output=True, + text=True, + timeout=timeout, + ) + + +def _assert_clean(result: subprocess.CompletedProcess[str]) -> None: + output = result.stdout + result.stderr + assert TRACEBACK not in output, output + + +def _parser_command_paths(parser: argparse.ArgumentParser) -> set[tuple[str, ...]]: + paths: set[tuple[str, ...]] = set() + + def visit(current: argparse.ArgumentParser, prefix: tuple[str, ...]) -> None: + for action in current._actions: + if not isinstance(action, argparse._SubParsersAction): + continue + for name, child in action.choices.items(): + path = (*prefix, name) + paths.add(path) + visit(child, path) + + visit(parser, ()) + return paths + + +@pytest.mark.smoke +def test_static_inventory_matches_registered_product_parser(): + from marm_mcp_server.cli import _product_parser + + assert set(COMMAND_HELP_PATHS) == _parser_command_paths(_product_parser()) + + +@pytest.mark.smoke +def test_root_help_lists_every_registered_top_level_command(): + from marm_mcp_server.cli import _product_help, _product_parser + + help_text = _product_help() + top_level = {path[0] for path in _parser_command_paths(_product_parser())} + documented = set() + command_reference, _, _ = help_text.partition("\nExamples:") + for line in command_reference.splitlines(): + match = re.match(r"^ {2}([a-z][a-z-]*(?:\|[a-z][a-z-]*)*)\s", line) + if match: + documented.update(match.group(1).split("|")) + + assert documented == top_level + + +@pytest.mark.smoke +def test_root_help_and_version_parse_without_a_traceback(tmp_path): + environment = _isolated_environment(tmp_path) + for arguments in (("--help",), ("-V",), ("version",)): + result = _run_product(arguments, environment) + assert result.returncode == 0, result.stderr + _assert_clean(result) + + +@pytest.mark.smoke +@pytest.mark.parametrize("command_path", COMMAND_HELP_PATHS) +def test_every_command_help_route_parses_cleanly(tmp_path, command_path): + result = _run_product((*command_path, "--help"), _isolated_environment(tmp_path)) + assert result.returncode == 0, result.stderr + _assert_clean(result) + + +@pytest.mark.smoke +@pytest.mark.parametrize("arguments", SAFE_DISPATCHES) +def test_safe_command_dispatches_exit_cleanly(tmp_path, arguments): + result = _run_product(arguments, _isolated_environment(tmp_path), timeout=45) + expected_codes = SAFE_DISPATCH_EXIT_CODES.get(arguments, {0}) + assert result.returncode in expected_codes, result.stderr + _assert_clean(result) + + +@pytest.mark.smoke +@pytest.mark.smoke_lifecycle +def test_http_foreground_reaches_health_then_stops(tmp_path): + port = _free_port() + environment = _isolated_environment(tmp_path, port=port) + process = subprocess.Popen( + [sys.executable, "-c", PRODUCT_ENTRYPOINT, "http"], + cwd=PACKAGE_ROOT, + env=environment, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + base_url = f"http://127.0.0.1:{port}" + try: + deadline = time.monotonic() + 30 + last_error: Exception | None = None + while time.monotonic() < deadline: + try: + with urllib.request.urlopen( + f"{base_url}/health", timeout=2 + ) as response: + if response.status == 200: + break + except (urllib.error.URLError, OSError) as exc: + last_error = exc + time.sleep(0.2) + else: + raise AssertionError( + f"MARM HTTP runtime did not become healthy: {last_error}" + ) + + stop_result = _run_product(("stop",), environment) + assert stop_result.returncode == 0, stop_result.stderr + _assert_clean(stop_result) + process.wait(timeout=15) + finally: + if process.poll() is None: + process.terminate() + process.wait(timeout=10) + + +@pytest.mark.smoke +@pytest.mark.smoke_lifecycle +def test_managed_key_round_trip_stays_inside_isolated_home(tmp_path): + environment = _isolated_environment(tmp_path) + init = _run_product(("key", "init"), environment) + assert init.returncode == 0, init.stderr + _assert_clean(init) + + reveal = _run_product(("key", "reveal"), environment) + assert reveal.returncode == 0, reveal.stderr + key = reveal.stdout.strip() + assert key + assert "terminal capture" in reveal.stderr + + path = _run_product(("key", "path"), environment) + assert path.returncode == 0, path.stderr + assert key not in path.stdout + assert Path(path.stdout.strip()).exists() + + +def _docker_available() -> bool: + try: + result = subprocess.run( + ["docker", "ps"], capture_output=True, text=True, timeout=20 + ) + except OSError: + return False + return result.returncode == 0 + + +@pytest.mark.smoke_docker +def test_real_docker_command_lifecycle(tmp_path): + if not _docker_available(): + pytest.skip("Docker daemon is not available") + + environment = _isolated_environment(tmp_path) + container = f"marm-command-smoke-{uuid.uuid4().hex[:10]}" + port = _free_port() + data_dir = tmp_path / "docker-data" + data_dir.mkdir() + run_arguments = ( + "docker", + "run", + "--name", + container, + "--port", + str(port), + "--data-dir", + str(data_dir), + ) + try: + pull = _run_product(("docker", "pull"), environment, timeout=180) + assert pull.returncode == 0, pull.stderr + run = _run_product(run_arguments, environment, timeout=90) + assert run.returncode == 0, run.stderr + status = _run_product(("docker", "status", "--name", container), environment) + assert status.returncode == 0, status.stderr + assert '"state": "running"' in status.stdout + logs = _run_product(("docker", "logs", "--name", container), environment) + assert logs.returncode == 0, logs.stderr + finally: + stop = _run_product(("docker", "stop", "--name", container), environment) + assert stop.returncode == 0, stop.stderr + + +@pytest.mark.smoke_destructive +def test_destructive_uninstall_reinstalls_in_disposable_environment(tmp_path): + wheel_dir = tmp_path / "wheel" + pip_environment = os.environ.copy() + pip_environment["PIP_CACHE_DIR"] = str(tmp_path / "pip-cache") + build = subprocess.run( + [ + sys.executable, + "-m", + "build", + "--wheel", + "--no-isolation", + "--outdir", + str(wheel_dir), + ], + cwd=PACKAGE_ROOT, + capture_output=True, + text=True, + env=pip_environment, + timeout=240, + ) + assert build.returncode == 0, build.stderr + wheel = next(wheel_dir.glob("marm_mcp_server-*.whl")) + + environment_dir = tmp_path / "destructive-venv" + venv.EnvBuilder(with_pip=True, system_site_packages=True).create(environment_dir) + python = environment_dir / ( + "Scripts/python.exe" if os.name == "nt" else "bin/python" + ) + install = subprocess.run( + [str(python), "-m", "pip", "install", "--no-deps", str(wheel)], + capture_output=True, + text=True, + env=pip_environment, + timeout=180, + ) + assert install.returncode == 0, install.stderr + + environment = _isolated_environment(tmp_path) + environment["VIRTUAL_ENV"] = str(environment_dir) + environment["PATH"] = str(python.parent) + os.pathsep + environment["PATH"] + result = _run_product( + ("uninstall", "--yes"), + environment, + python_executable=python, + cwd=tmp_path, + timeout=120, + ) + _assert_clean(result) + + if os.name == "nt": + assert result.returncode == 1 + assert "not safe" in result.stderr + return + + try: + assert result.returncode == 0, result.stderr + finally: + reinstall = subprocess.run( + [str(python), "-m", "pip", "install", "--no-deps", str(wheel)], + capture_output=True, + text=True, + env=pip_environment, + timeout=180, + ) + assert reinstall.returncode == 0, reinstall.stderr + + version = _run_product( + ("version",), environment, python_executable=python, cwd=tmp_path + ) + assert version.returncode == 0, version.stderr + assert version.stdout.strip() diff --git a/marm-mcp-server/tests/test_hybrid_search.py b/marm-mcp-server/tests/test_hybrid_search.py index 39b23856..b2e0a3a2 100644 --- a/marm-mcp-server/tests/test_hybrid_search.py +++ b/marm-mcp-server/tests/test_hybrid_search.py @@ -382,7 +382,9 @@ async def test_recall_similar_falls_back_when_fts_candidates_have_no_embeddings( # Return a non-existent ID so ID-bounded fetch finds nothing scoreable monkeypatch.setattr( - memory_recall_module, "_fetch_fts_candidate_ids", lambda *_: ["non-existent-id"] + memory_recall_module, + "_fetch_fts_candidate_ids", + lambda *_: [("non-existent-id", 1.0)], ) results = await memory.recall_similar( @@ -423,7 +425,7 @@ async def test_recall_similar_falls_back_when_fts_candidates_are_all_wrong_dimen # Force FTS to return only the wrong-dimension ID monkeypatch.setattr( - memory_recall_module, "_fetch_fts_candidate_ids", lambda *_: [wrong_id] + memory_recall_module, "_fetch_fts_candidate_ids", lambda *_: [(wrong_id, 1.0)] ) query_vec = correct_vec.copy() @@ -558,7 +560,7 @@ async def test_recall_similar_debug_logs_filter_rerank_path(monkeypatch, tmp_pat debug_calls: list[str] = [] monkeypatch.setattr(memory_recall_module, "_recall_debug", debug_calls.append) monkeypatch.setattr( - memory_recall_module, "_fetch_fts_candidate_ids", lambda *_: [embed_id] + memory_recall_module, "_fetch_fts_candidate_ids", lambda *_: [(embed_id, 1.0)] ) await mem.recall_similar( @@ -596,3 +598,85 @@ async def test_recall_similar_debug_logs_semantic_fallback_path(monkeypatch, tmp ) assert any("semantic fallback" in msg for msg in debug_calls) + + +@pytest.mark.asyncio +async def test_recall_similar_fuses_bm25_into_ranking(monkeypatch, tmp_path): + """Two candidates with identical embeddings must be ordered by their BM25 + score, proving the lexical signal is fused into the blend rather than + discarded after the FTS filter.""" + from marm_mcp_server.core import memory_recall as memory_recall_module + + memory = MARMMemory(str(tmp_path / "memory.db")) + memory._encoder_failed = True + + dim = 384 + vec = np.ones(dim, dtype=np.float32) + vec /= np.linalg.norm(vec) + + with memory.get_connection() as conn: + strong_id = _insert_with_embedding(conn, "fuse", "docker deployment", vec) + weak_id = _insert_with_embedding(conn, "fuse", "docker sidebar", vec) + + # Identical embeddings -> identical vec_score. Only the BM25 term differs, + # so any ordering must come from lexical fusion. + monkeypatch.setattr( + memory_recall_module, + "_fetch_fts_candidate_ids", + lambda *_: [(strong_id, 1.0), (weak_id, 0.0)], + ) + + results = await memory.recall_similar( + "docker", session="fuse", limit=5, query_vec=vec.copy() + ) + ids = [r["id"] for r in results] + + assert ids.index(strong_id) < ids.index(weak_id) + strong_sim = results[ids.index(strong_id)]["similarity"] + weak_sim = results[ids.index(weak_id)]["similarity"] + assert strong_sim > weak_sim + + +def test_normalize_bm25_maps_more_negative_to_higher_score(): + """BM25 is more-negative = better, so normalization must invert: the most + negative raw score becomes 1.0 and the least negative becomes 0.0.""" + from marm_mcp_server.core.memory_scoring import _normalize_bm25 + + assert _normalize_bm25([-5.0, -3.0, -1.0]) == [1.0, 0.5, 0.0] + assert _normalize_bm25([-2.0]) == [1.0] + assert _normalize_bm25([]) == [] + # All-equal scores collapse to 1.0 so a lone lexical hit keeps full weight. + assert _normalize_bm25([-2.0, -2.0]) == [1.0, 1.0] + + +def test_fetch_fts_candidate_ids_returns_normalized_bm25_from_real_index(tmp_path): + """Exercises the real SQLite bm25() -> normalization path end to end: the + tighter lexical match must score highest, and every score must land in + [0, 1] with 1.0 as the best. Guards against an inverted or out-of-range + normalization that the fusion test's stub cannot catch.""" + from marm_mcp_server.core.memory_scoring import _fetch_fts_candidate_ids + + vec = np.ones(384, dtype=np.float32) + vec /= np.linalg.norm(vec) + + memory = MARMMemory(str(tmp_path / "memory.db")) + memory._encoder_failed = True + + with memory.get_connection() as conn: + # Tight match: the query term is essentially the whole document. + tight_id = _insert_with_embedding(conn, "norm", "python", vec) + # Diluted match: the query term is buried among many others. + loose_id = _insert_with_embedding( + conn, + "norm", + "python programming language tutorial guide reference manual notes", + vec, + ) + + pairs = _fetch_fts_candidate_ids(memory.db_path, "norm", '"python"', 10) + scores = dict(pairs) + + assert set(scores) == {tight_id, loose_id} + assert all(0.0 <= s <= 1.0 for s in scores.values()) + assert max(scores.values()) == 1.0 + assert scores[tight_id] > scores[loose_id] diff --git a/marm-mcp-server/tests/test_runtime_cli.py b/marm-mcp-server/tests/test_runtime_cli.py index 36bf7a99..4499c5b4 100644 --- a/marm-mcp-server/tests/test_runtime_cli.py +++ b/marm-mcp-server/tests/test_runtime_cli.py @@ -220,6 +220,29 @@ def test_managed_key_init_reuses_existing_credential(monkeypatch, tmp_path): assert active_key_management.read_managed_key(path) == "first-key" +def test_windows_key_acl_uses_the_executing_identity(monkeypatch, tmp_path): + from marm_mcp_server.utils import security + + calls = [] + + class Completed: + def __init__(self, returncode=0, stdout=""): + self.returncode = returncode + self.stdout = stdout + + def run(command, **kwargs): + calls.append(command) + if command == ["whoami"]: + return Completed(stdout="DOMAIN\\runtime-user\r\n") + return Completed() + + monkeypatch.setattr(security.sys, "platform", "win32") + monkeypatch.setattr(security.subprocess, "run", run) + + assert security.restrict_windows_file_to_current_user(tmp_path / ".env") + assert calls[1][-1] == "DOMAIN\\runtime-user:(F)" + + def test_key_path_and_reveal_keep_output_intentional(monkeypatch, capsys, tmp_path): active_key_management = importlib.import_module( "marm_mcp_server.services.key_management" diff --git a/marm-mcp-server/tests/test_temporal_weighting.py b/marm-mcp-server/tests/test_temporal_weighting.py index d94efccc..f4a4ea40 100644 --- a/marm-mcp-server/tests/test_temporal_weighting.py +++ b/marm-mcp-server/tests/test_temporal_weighting.py @@ -154,3 +154,100 @@ def _insert(ts, content): assert ids.index(old_id) < ids.index(new_id), ( "At TEMPORAL_WEIGHT=0 the FTS-dominant old memory should rank above the new non-matching one" ) + + +@pytest.mark.asyncio +async def test_text_search_fallback_applies_temporal_but_exact_lane_does_not(tmp_path): + """When the encoder is unavailable, semantic-intent recall falls back to text + search WITH temporal decay so newer wins. The exact lane must NOT be reordered + by age -- identical lexical hits keep BM25 (insertion) order.""" + import sqlite3 as _sqlite3 + import uuid + + mem = MARMMemory(str(tmp_path / "memory.db")) + mem._encoder_failed = True # forces the text-search fallback path + + base = datetime.now(timezone.utc) + + def _insert(ts): + mid = str(uuid.uuid4()) + with _sqlite3.connect(str(tmp_path / "memory.db")) as conn: + conn.execute( + "INSERT INTO memories (id, session_name, content, timestamp, context_type, metadata) " + "VALUES (?, 'fallback-temporal', 'temporal keyword content', ?, 'general', '{}')", + (mid, ts), + ) + conn.execute( + "INSERT INTO memories_fts(rowid, content) " + "SELECT rowid, content FROM memories WHERE id = ?", + (mid,), + ) + return mid + + old_id = _insert((base - timedelta(days=120)).isoformat()) + new_id = _insert(base.isoformat()) + + # Natural-language query -> auto lane -> encoder unavailable -> temporal fallback + semantic = await mem.recall_similar( + "temporal keyword content", session="fallback-temporal", limit=5 + ) + sem_ids = [r["id"] for r in semantic] + assert sem_ids.index(new_id) < sem_ids.index(old_id), ( + "semantic-intent fallback must rank the newer memory first" + ) + + # Exact lane -> identical BM25 for identical content -> insertion order, age ignored + exact = await mem.recall_similar( + "temporal keyword content", + session="fallback-temporal", + limit=5, + exact_mode="exact", + ) + ex_ids = [r["id"] for r in exact] + assert ex_ids.index(old_id) < ex_ids.index(new_id), ( + "exact lane must preserve BM25/insertion order, not re-rank by age" + ) + + +@pytest.mark.asyncio +async def test_temporal_fallback_promotes_newer_result_outside_bm25_limit(tmp_path): + """With limit=1 on the temporal fallback lane, a newer result ranked just + outside the top-1 BM25 row must still be promoted. This fails if the lane + fetches only `limit` BM25 rows before re-ranking, so it guards the widened + candidate pool.""" + import sqlite3 as _sqlite3 + import uuid + + mem = MARMMemory(str(tmp_path / "memory.db")) + mem._encoder_failed = True # forces the text-search fallback path + + base = datetime.now(timezone.utc) + + def _insert(ts): + mid = str(uuid.uuid4()) + with _sqlite3.connect(str(tmp_path / "memory.db")) as conn: + conn.execute( + "INSERT INTO memories (id, session_name, content, timestamp, context_type, metadata) " + "VALUES (?, 'limit-cutoff', 'temporal keyword content', ?, 'general', '{}')", + (mid, ts), + ) + conn.execute( + "INSERT INTO memories_fts(rowid, content) " + "SELECT rowid, content FROM memories WHERE id = ?", + (mid,), + ) + return mid + + # Identical content -> identical BM25. Old is inserted first, so it is the + # single row a limit=1 BM25 fetch would return; new sits just outside it. + _insert((base - timedelta(days=120)).isoformat()) + new_id = _insert(base.isoformat()) + + results = await mem.recall_similar( + "temporal keyword content", session="limit-cutoff", limit=1 + ) + + assert len(results) == 1 + assert results[0]["id"] == new_id, ( + "temporal fallback must promote the newer row from outside the BM25 limit cutoff" + ) diff --git a/scripts/benchmarking/performance/bench_hotpath.py b/scripts/benchmarking/performance/bench_hotpath.py index 0f498fa2..fe0d4d3c 100644 --- a/scripts/benchmarking/performance/bench_hotpath.py +++ b/scripts/benchmarking/performance/bench_hotpath.py @@ -5,13 +5,18 @@ 2. recall_similar latency vs session size N (FTS filter + bounded embedding rerank) 3. event-loop blocking: concurrent recalls via asyncio.gather vs serial sum 4. write latency with consolidation OFF vs ON (double-encode + scan-per-write) - 5. HYBRID SEARCH: FTS5 filter→re-rank vs weighted fusion vs pure semantic + 5. RECALL SCALING: production full semantic scan vs production hybrid recall + +Every timed path calls the shipped MARMMemory code (recall_similar, +_fetch_and_score_embedding_rows). No scoring is reimplemented in this script, +so the numbers reflect what a caller actually gets. Run from repo root: python scripts/benchmarking/performance/bench_hotpath.py Uses a throwaway temp DB; never touches ~/.marm. """ import asyncio +import importlib.util import os import sqlite3 import statistics @@ -37,12 +42,7 @@ from marm_mcp_server.core import memory_ops # noqa: E402 from marm_mcp_server.config.settings import DEFAULT_SEMANTIC_DIM # noqa: E402 -try: - import numpy as np - - NUMPY_AVAILABLE = True -except ImportError: - NUMPY_AVAILABLE = False +NUMPY_AVAILABLE = importlib.util.find_spec("numpy") is not None def _pct(values, p): @@ -203,10 +203,16 @@ async def bench_connection_overhead(db_path, iters=30): async def bench_hybrid_strategies(mem, sizes=None, iters=15): - """Benchmark three hybrid search strategies: - 1. Pure Semantic (baseline brute-force) - 2. Current Production (weighted fusion: vector scan + FTS merge) - 3. Filter→Re-rank (FTS top 50 → semantic re-rank only those 50) + """Compare two REAL production recall paths as session size N grows: + + 1. Full semantic scan -- _fetch_and_score_embedding_rows scores every + embedding row (the cost with no keyword pre-filter). + 2. Production hybrid -- recall_similar: FTS pre-filter to a bounded + candidate set, then semantic + BM25 + temporal re-rank. + + Both are timed with the query vector precomputed, so encode cost (constant + in N, reported in section 1) is excluded and the numbers isolate the + scan-vs-prefilter scaling difference. No scoring is reimplemented here. """ if sizes is None: sizes = [100, 500, 1000, 2000, 4000, 10000] @@ -215,12 +221,12 @@ async def bench_hybrid_strategies(mem, sizes=None, iters=15): print(" [SKIPPED: numpy not available]") return None + from marm_mcp_server.core.memory_scoring import _fetch_and_score_embedding_rows + mem._load_encoder_lazily() results = { - "pure_semantic": {}, + "full_scan": {}, "production_hybrid": {}, - "filter_rerank": {}, - "fts_only": {}, "fts_hit_rate": {}, } @@ -234,116 +240,44 @@ async def bench_hybrid_strategies(mem, sizes=None, iters=15): for n in sizes: seed(mem, n) - pure_samples = [] + scan_samples = [] prod_samples = [] - filter_samples = [] - fts_samples = [] fts_hits = [] for iter_num in range(iters): query = test_queries[iter_num % len(test_queries)] - query_emb = mem.encoder.encode(query) + query_vec = mem._encode_sync(query) fts_query = _safe_fts_query(query) - # 1. Pure Semantic (disable FTS in production code temporarily) + # 1. Full semantic scan (production scorer, no pre-filter). + # Dispatched via asyncio.to_thread to match how recall_similar runs + # its DB/scoring work, so both columns share one execution model. t0 = time.perf_counter() - conn = sqlite3.connect(mem.db_path, timeout=30.0) - try: - conn.row_factory = sqlite3.Row - rows = conn.execute( - """SELECT id, content, embedding FROM memories - WHERE session_name = 'bench' AND embedding IS NOT NULL - ORDER BY timestamp DESC LIMIT 10000""" - ).fetchall() - - if rows: - similarities = [] - for row in rows: - emb_bytes = row["embedding"] - if emb_bytes: - emb = np.frombuffer(emb_bytes, dtype=np.float32) - if len(emb) == len(query_emb): - sim = float(np.dot(query_emb, emb)) - similarities.append((row["id"], row["content"], sim)) - similarities.sort(key=lambda x: x[2], reverse=True) - # Top results available in similarities[:5] - finally: - conn.close() - pure_samples.append((time.perf_counter() - t0) * 1000) - - # 2. Production Hybrid (current recall_similar implementation) + await asyncio.to_thread( + _fetch_and_score_embedding_rows, mem.db_path, "bench", n, query_vec, 5 + ) + scan_samples.append((time.perf_counter() - t0) * 1000) + + # 2. Production hybrid recall (precomputed vector excludes encode) t0 = time.perf_counter() - await mem.recall_similar(query, session="bench", limit=5) + await mem.recall_similar( + query, session="bench", limit=5, query_vec=query_vec + ) prod_samples.append((time.perf_counter() - t0) * 1000) - # 3. Filter→Re-rank Strategy (dump.md approach) - t0 = time.perf_counter() - conn = sqlite3.connect(mem.db_path, timeout=30.0) - try: - conn.row_factory = sqlite3.Row - - # Step 1: FTS filter to top 50 candidates - candidates = ( - conn.execute( - """SELECT m.id, m.content, m.embedding - FROM memories_fts - JOIN memories m ON memories_fts.rowid = m.rowid - WHERE memories_fts MATCH ? AND m.session_name = 'bench' - ORDER BY bm25(memories_fts) LIMIT 50""", - (fts_query,), - ).fetchall() - if fts_query - else [] - ) - - fts_hits.append(len(candidates)) - - # Fallback: if FTS finds nothing, take top 50 by timestamp - if not candidates: - candidates = conn.execute( - """SELECT id, content, embedding FROM memories - WHERE session_name = 'bench' AND embedding IS NOT NULL - ORDER BY timestamp DESC LIMIT 50""" - ).fetchall() - - # Step 2: Re-rank only those 50 by semantic similarity - if candidates: - scores = [] - for row in candidates: - emb_bytes = row["embedding"] - if emb_bytes: - emb = np.frombuffer(emb_bytes, dtype=np.float32) - if len(emb) == len(query_emb): - score = float(np.dot(query_emb, emb)) - scores.append((row["id"], row["content"], score)) - - scores.sort(key=lambda x: x[2], reverse=True) - # Top results available in scores[:5] - finally: - conn.close() - filter_samples.append((time.perf_counter() - t0) * 1000) - - # 4. FTS-only (baseline keyword search) + # Informational only: how many candidates the FTS pre-filter matched. if fts_query: - t0 = time.perf_counter() - conn = sqlite3.connect(mem.db_path, timeout=30.0) - try: - conn.row_factory = sqlite3.Row - conn.execute( - """SELECT m.id, m.content FROM memories_fts + with mem.get_connection() as conn: + matched = conn.execute( + """SELECT COUNT(*) FROM memories_fts JOIN memories m ON memories_fts.rowid = m.rowid - WHERE memories_fts MATCH ? AND m.session_name = 'bench' - ORDER BY bm25(memories_fts) LIMIT 5""", + WHERE memories_fts MATCH ? AND m.session_name = 'bench'""", (fts_query,), - ).fetchall() - finally: - conn.close() - fts_samples.append((time.perf_counter() - t0) * 1000) + ).fetchone()[0] + fts_hits.append(min(matched, 50)) - results["pure_semantic"][n] = pure_samples + results["full_scan"][n] = scan_samples results["production_hybrid"][n] = prod_samples - results["filter_rerank"][n] = filter_samples - results["fts_only"][n] = fts_samples if fts_samples else [0.0] * iters results["fts_hit_rate"][n] = (sum(fts_hits) / len(fts_hits)) if fts_hits else 0 return results @@ -386,9 +320,10 @@ async def main(): conn_overhead = await bench_connection_overhead(mem.db_path) print(_stat_line("connect + close", conn_overhead), "\n") - print("=== 6. HYBRID SEARCH STRATEGIES COMPARISON ===") + print("=== 6. RECALL SCALING: full semantic scan vs production hybrid ===") print( - "Testing: Pure Semantic | Production Hybrid (weighted fusion) | Filter→Re-rank\n" + "Both paths call shipped code, timed with the query vector precomputed\n" + "(encode is constant in N, see section 1).\n" ) hybrid = await bench_hybrid_strategies( @@ -397,38 +332,32 @@ async def main(): if hybrid: print( - f"{'Size':<8} {'Pure Sem':<12} {'Prod Hybrid':<14} {'Filter→Rerank':<16} {'FTS-Only':<12} {'Speedup':<12} {'FTS Hits'}" + f"{'Size':<8} {'Full Scan':<14} {'Prod Hybrid':<14} {'Speedup':<10} {'FTS Hits'}" ) - print("-" * 100) + print("-" * 60) for n in [100, 500, 1000, 2000, 4000, 10000]: - pure_med = statistics.median(hybrid["pure_semantic"][n]) + scan_med = statistics.median(hybrid["full_scan"][n]) prod_med = statistics.median(hybrid["production_hybrid"][n]) - filt_med = statistics.median(hybrid["filter_rerank"][n]) - fts_med = ( - statistics.median(hybrid["fts_only"][n]) - if hybrid["fts_only"][n][0] > 0 - else 0 - ) - - # Speedup: filter→re-rank vs pure semantic - speedup = pure_med / filt_med if filt_med > 0 else 0 + speedup = scan_med / prod_med if prod_med > 0 else 0 fts_hit_rate = hybrid["fts_hit_rate"][n] print( - f"{n:<8} {pure_med:>7.1f}ms {prod_med:>7.1f}ms " - f"{filt_med:>7.1f}ms {fts_med:>7.1f}ms " - f"{speedup:>5.1f}x {fts_hit_rate:>4.1f}/50" + f"{n:<8} {scan_med:>7.1f}ms {prod_med:>7.1f}ms " + f"{speedup:>5.1f}x {fts_hit_rate:>4.1f}/50" ) - print("\n📊 Key Insights:") - print(" • Pure Semantic: O(N) brute-force vector scan (baseline)") - print(" • Prod Hybrid: Weighted fusion (65% vector + 35% FTS scores)") + print("\nKey points:") + print( + " • Full Scan: _fetch_and_score_embedding_rows over all N rows (no prefilter)" + ) + print( + " • Prod Hybrid: recall_similar -- FTS prefilter + semantic/BM25/temporal rerank" + ) + print(" • Speedup = full scan / production hybrid (both real code paths)") print( - " • Filter→Re-rank: FTS narrows to 50 → semantic re-rank (dump.md strategy)" + " • FTS Hits: avg candidates the keyword prefilter matched (capped at 50)\n" ) - print(" • Speedup shows Filter→Re-rank advantage over Pure Semantic") - print(" • FTS Hits shows avg candidates found by keyword filter\n") if __name__ == "__main__": diff --git a/scripts/test-scripts/README.md b/scripts/test-scripts/README.md index 1c2d5811..8f77adaf 100644 --- a/scripts/test-scripts/README.md +++ b/scripts/test-scripts/README.md @@ -15,6 +15,23 @@ Compaction smoke paths use the public `marm_compaction(action=...)` endpoint/too | Test full compaction stage/apply/idempotency paths | `compaction-worker-smoke.py` | Compaction Worker | | Simulate small local swarms writing to MARM | `swarm-smoke.py` | Swarm | | Test natural compaction trigger from swarm writes | `swarm-smoke.py` | Swarm + Compaction | +| Smoke every `marm-memory` command path | `smoke-commands.py` | Command Surface | + +### Command Surface + +Run the complete safe command parse, dispatch, HTTP lifecycle, and managed-key smoke suite: + +```powershell +python scripts\test-scripts\smoke-commands.py +``` + +Add a real Docker lifecycle only when a Docker daemon is available. `--destructive` builds a local wheel and exercises uninstall/reinstall inside a disposable virtual environment; it does not modify the active development environment. + +```powershell +python scripts\test-scripts\smoke-commands.py --docker +python scripts\test-scripts\smoke-commands.py --destructive +python scripts\test-scripts\smoke-commands.py --skip lifecycle +``` ## Quick Choice diff --git a/scripts/test-scripts/smoke-commands.py b/scripts/test-scripts/smoke-commands.py new file mode 100644 index 00000000..58dbdc1b --- /dev/null +++ b/scripts/test-scripts/smoke-commands.py @@ -0,0 +1,66 @@ +"""Run local smoke coverage for the marm-memory command surface.""" + +from __future__ import annotations + +import argparse +import subprocess +import sys +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parents[2] +PACKAGE_ROOT = REPO_ROOT / "marm-mcp-server" +TEST_TARGET = "tests/test_command_smoke.py" + + +def _marker_expression(args: argparse.Namespace) -> str: + groups = ["smoke"] + if args.docker: + groups.append("smoke_docker") + if args.destructive: + groups.append("smoke_destructive") + expression = " or ".join(f"({group})" for group in groups) + if "lifecycle" in args.skip: + expression = f"({expression}) and not smoke_lifecycle" + return expression + + +def main() -> int: + parser = argparse.ArgumentParser( + description="Run local marm-memory command smoke tests." + ) + parser.add_argument( + "--docker", + action="store_true", + help="Include real Docker lifecycle coverage when a daemon is available.", + ) + parser.add_argument( + "--destructive", + action="store_true", + help="Include the opt-in uninstall/reinstall lifecycle test.", + ) + parser.add_argument( + "--skip", + action="append", + choices=("lifecycle",), + default=[], + help="Skip a selected smoke tier. May be repeated.", + ) + args = parser.parse_args() + + command = [ + sys.executable, + "-m", + "pytest", + TEST_TARGET, + "-m", + _marker_expression(args), + "-ra", + "-v", + ] + print(f"Running command smoke tests: {' '.join(command)}") + return subprocess.run(command, cwd=PACKAGE_ROOT, check=False).returncode + + +if __name__ == "__main__": + raise SystemExit(main()) From c561724221fc24e7a9c44ca208ec706b5daa1a28 Mon Sep 17 00:00:00 2001 From: Ryan Lyell Date: Fri, 24 Jul 2026 05:15:50 -0400 Subject: [PATCH 2/3] fix(review): address PR #111 findings and refresh benchmark numbers CodeRabbit + Codex review of the v2.29.0 branch: - Gate smoke_docker/smoke_destructive behind MARM_SMOKE_DOCKER / MARM_SMOKE_DESTRUCTIVE at collection time so plain pytest and the publish test step no longer run the Docker or uninstall/reinstall lifecycle. run-tests.py and smoke-commands.py set the gates. - Add build>=1.2.2 to [dev] so the destructive wheel/reinstall smoke can actually run. - settings.py: warn when restrict_windows_file_to_current_user() fails instead of silently leaving the key file inheritable. - security.py: resolve whoami/icacls via absolute SystemRoot\System32 paths to avoid an untrusted-search-path hijack (CWE-426); test updated to match. - test_temporal_weighting.py: drop the undefined SQLite tie-order assumption on the exact lane; assert timestamp-invariant similarity and retrieval_mode instead. Benchmark: alternate which path runs first each iteration so neither gets a consistent warm-cache advantage; re-ran and refreshed README section 4 (numbers held: 39.4x at N=10,000). Scripts: --tests flag for check-file-length; clean-pytest-artifacts discovers .pytest-review-*/.pytest-smoke-*/marm-pytest-* dirs. Co-Authored-By: Claude Opus 4.8 --- .gitignore | 3 ++ README.md | 14 +++--- .../marm_mcp_server/config/settings.py | 6 ++- .../marm_mcp_server/core/memory_recall.py | 8 ++- .../marm_mcp_server/utils/security.py | 13 ++++- marm-mcp-server/pyproject.toml | 4 +- marm-mcp-server/tests/conftest.py | 16 ++++++ marm-mcp-server/tests/test_command_smoke.py | 10 +++- marm-mcp-server/tests/test_hybrid_search.py | 19 +++++++ marm-mcp-server/tests/test_runtime_cli.py | 9 +++- .../tests/test_temporal_weighting.py | 13 +++-- .../benchmarking/performance/bench_hotpath.py | 50 +++++++++++++------ scripts/check-file-length.py | 12 ++++- scripts/clean-pytest-artifacts.py | 4 ++ scripts/run-tests.py | 5 +- scripts/test-scripts/smoke-commands.py | 10 +++- 16 files changed, 156 insertions(+), 40 deletions(-) diff --git a/.gitignore b/.gitignore index 002c9dc2..7a7bf454 100644 --- a/.gitignore +++ b/.gitignore @@ -111,6 +111,8 @@ docs/future/ .impeccable .cursor .pytest_cache/ +.pytest-review-*/ +.pytest-smoke-*/ .pytest_tmp*/ .pytest_tmp_dashboard/ .pytest_tmp_review/ @@ -120,6 +122,7 @@ marm-mcp-server/.pytest_tmp/ marm-mcp-server/.pytest_tmp*/ marm-mcp-server/.pytest-review-*/ marm-mcp-server/.pytest-smoke-*/ +marm-pytest-*/ marm_usage_analytics.db dump.md dump2.md diff --git a/README.md b/README.md index a55db5ea..e631825e 100644 --- a/README.md +++ b/README.md @@ -201,16 +201,16 @@ End-to-end `recall_similar` latency (includes query encoding). ### 4. Recall Scaling: Full Scan vs Production Hybrid -Why recall stays roughly flat as memory grows: instead of scoring every stored vector, production recall uses an FTS keyword pre-filter to a bounded candidate set, then re-ranks that set by semantic + BM25 + temporal score. Both columns are real code paths (`_fetch_and_score_embedding_rows` for the full scan, `recall_similar` for hybrid), dispatched through the same async path and timed with the query vector precomputed so the constant encode cost from section 1 is excluded from both. +Why recall stays roughly flat as memory grows: instead of scoring every stored vector, production recall uses an FTS keyword pre-filter to a bounded candidate set, then re-ranks that set by semantic + BM25 + temporal score. Both columns are real code paths (`_fetch_and_score_embedding_rows` for the full scan, `recall_similar` for hybrid), dispatched through the same async path and timed with the query vector precomputed so the constant encode cost from section 1 is excluded from both. Each iteration alternates which path runs first so neither one consistently benefits from the other's warmed cache. | Session Size ($N$) | Full Semantic Scan | Production Hybrid | Speedup | | :--- | :--- | :--- | :--- | -| **N = 100** | 3.3 ms | 4.0 ms | 0.8x | -| **N = 500** | 15.8 ms | 4.4 ms | 3.6x | -| **N = 1,000** | 33.5 ms | 6.4 ms | 5.2x | -| **N = 2,000** | 69.3 ms | 6.3 ms | 11.0x | -| **N = 4,000** | 127.3 ms | 7.2 ms | 17.7x | -| **N = 10,000** | 320.6 ms | 8.7 ms | 37.0x | +| **N = 100** | 3.5 ms | 3.8 ms | 0.9x | +| **N = 500** | 15.9 ms | 4.2 ms | 3.8x | +| **N = 1,000** | 34.4 ms | 4.9 ms | 7.0x | +| **N = 2,000** | 67.6 ms | 5.7 ms | 11.9x | +| **N = 4,000** | 132.5 ms | 7.3 ms | 18.0x | +| **N = 10,000** | 330.4 ms | 8.4 ms | 39.4x | The full scan grows roughly linearly with $N$ while hybrid recall stays near-flat, so the advantage widens with session size. At very small $N$ the pre-filter overhead is not yet worth it (hybrid is marginally slower at N = 100); the win appears once there is enough to skip. Reproduce with [`scripts/benchmarking/performance/bench_hotpath.py`](scripts/benchmarking/performance/bench_hotpath.py). diff --git a/marm-mcp-server/marm_mcp_server/config/settings.py b/marm-mcp-server/marm_mcp_server/config/settings.py index 5cc98913..69cec5fb 100644 --- a/marm-mcp-server/marm_mcp_server/config/settings.py +++ b/marm-mcp-server/marm_mcp_server/config/settings.py @@ -397,8 +397,10 @@ def _load_key_from_file() -> str: _MARM_ENV_PATH.chmod(0o600) except OSError: pass - if sys.platform == "win32": - restrict_windows_file_to_current_user(_MARM_ENV_PATH) + if sys.platform == "win32" and not restrict_windows_file_to_current_user( + _MARM_ENV_PATH + ): + print(f"WARNING: Could not restrict API key file: {_MARM_ENV_PATH}") except Exception as _e: print(f"WARNING: Could not save API key to {_MARM_ENV_PATH}: {_e}") diff --git a/marm-mcp-server/marm_mcp_server/core/memory_recall.py b/marm-mcp-server/marm_mcp_server/core/memory_recall.py index 1a3c2b92..29c66da1 100644 --- a/marm-mcp-server/marm_mcp_server/core/memory_recall.py +++ b/marm-mcp-server/marm_mcp_server/core/memory_recall.py @@ -379,7 +379,9 @@ def _blend_temporal(base_sim: float, timestamp: str) -> float: "similarity": _blend_temporal(float(score), row["timestamp"]), "project": row["project"], "platform": row["platform"], - "retrieval_mode": "exact_fts", + "retrieval_mode": ( + "semantic_fallback_fts" if apply_temporal else "exact_fts" + ), } for row, score in fts_rows ] @@ -427,7 +429,9 @@ def _blend_temporal(base_sim: float, timestamp: str) -> float: "similarity": _blend_temporal(0.8, row[3]), "project": row[6], "platform": row[7], - "retrieval_mode": "exact_like", + "retrieval_mode": ( + "semantic_fallback_like" if apply_temporal else "exact_like" + ), } ) diff --git a/marm-mcp-server/marm_mcp_server/utils/security.py b/marm-mcp-server/marm_mcp_server/utils/security.py index 45310510..83704cef 100644 --- a/marm-mcp-server/marm_mcp_server/utils/security.py +++ b/marm-mcp-server/marm_mcp_server/utils/security.py @@ -1,5 +1,6 @@ """Cryptographic utilities — no imports from settings, no side effects.""" +import os import secrets import string import subprocess @@ -27,14 +28,22 @@ def restrict_windows_file_to_current_user(path: Path) -> bool: if sys.platform != "win32": return True try: + system_root = os.environ.get("SystemRoot") + if not system_root: + return False + system32 = Path(system_root) / "System32" + whoami = system32 / "whoami.exe" + icacls = system32 / "icacls.exe" + if not whoami.is_absolute() or not icacls.is_absolute(): + return False identity = subprocess.run( - ["whoami"], check=False, capture_output=True, text=True + [str(whoami)], check=False, capture_output=True, text=True ).stdout.strip() if not identity: return False result = subprocess.run( [ - "icacls", + str(icacls), str(path), "/inheritance:r", "/grant:r", diff --git a/marm-mcp-server/pyproject.toml b/marm-mcp-server/pyproject.toml index ea84d464..451c4120 100644 --- a/marm-mcp-server/pyproject.toml +++ b/marm-mcp-server/pyproject.toml @@ -67,6 +67,7 @@ dev = [ "pytest>=7.0.0", "pytest-asyncio>=0.21.0", "pytest-cov>=4.0.0", + "build>=1.2.2", "jsonschema>=4.0.0", "requests>=2.31.0", "black>=22.0.0", @@ -106,13 +107,14 @@ disallow_untyped_defs = true testpaths = ["tests"] python_files = ["test_*.py"] addopts = "-p no:cacheprovider" +tmp_path_retention_policy = "none" markers = [ "docker: Docker transport tests skipped by scripts/run-tests.py unless --docker is used", "slow_stdio: subprocess-based STDIO transport tests skipped by scripts/run-tests.py --fast", "smoke: local command parse, dispatch, and isolated lifecycle smoke tests", "smoke_lifecycle: HTTP runtime and managed-key lifecycle smoke tests", "smoke_docker: smoke tests that launch a real Docker container (need a daemon)", - "smoke_destructive: smoke tests that uninstall/reinstall MARM in a disposable virtual environment", + "smoke_destructive: opt-in smoke tests that uninstall/reinstall MARM in a disposable virtual environment", ] [tool.ruff] diff --git a/marm-mcp-server/tests/conftest.py b/marm-mcp-server/tests/conftest.py index 536a927a..7d6bc9dd 100644 --- a/marm-mcp-server/tests/conftest.py +++ b/marm-mcp-server/tests/conftest.py @@ -34,6 +34,22 @@ def _resolve_cbm_binary() -> str | None: ) +def pytest_collection_modifyitems(items): + opt_in_markers = { + "smoke_docker": "MARM_SMOKE_DOCKER", + "smoke_destructive": "MARM_SMOKE_DESTRUCTIVE", + } + for item in items: + for marker, environment_name in opt_in_markers.items(): + if ( + item.get_closest_marker(marker) + and os.environ.get(environment_name) != "1" + ): + item.add_marker( + pytest.mark.skip(reason=f"{marker} requires {environment_name}=1") + ) + + @pytest.fixture(autouse=True, scope="session") def isolated_cbm_store(tmp_path_factory): """Keep test indexes out of the developer's real code graph. diff --git a/marm-mcp-server/tests/test_command_smoke.py b/marm-mcp-server/tests/test_command_smoke.py index a3806ee3..61e3680a 100644 --- a/marm-mcp-server/tests/test_command_smoke.py +++ b/marm-mcp-server/tests/test_command_smoke.py @@ -13,6 +13,7 @@ import urllib.request import uuid import venv +import warnings from pathlib import Path import pytest @@ -319,7 +320,8 @@ def test_real_docker_command_lifecycle(tmp_path): assert logs.returncode == 0, logs.stderr finally: stop = _run_product(("docker", "stop", "--name", container), environment) - assert stop.returncode == 0, stop.stderr + if stop.returncode != 0: + warnings.warn(f"Docker smoke cleanup failed: {stop.stderr}", stacklevel=2) @pytest.mark.smoke_destructive @@ -387,7 +389,11 @@ def test_destructive_uninstall_reinstalls_in_disposable_environment(tmp_path): env=pip_environment, timeout=180, ) - assert reinstall.returncode == 0, reinstall.stderr + if reinstall.returncode != 0: + warnings.warn( + f"Destructive smoke reinstall cleanup failed: {reinstall.stderr}", + stacklevel=2, + ) version = _run_product( ("version",), environment, python_executable=python, cwd=tmp_path diff --git a/marm-mcp-server/tests/test_hybrid_search.py b/marm-mcp-server/tests/test_hybrid_search.py index b2e0a3a2..17865516 100644 --- a/marm-mcp-server/tests/test_hybrid_search.py +++ b/marm-mcp-server/tests/test_hybrid_search.py @@ -174,6 +174,25 @@ async def test_recall_text_search_falls_back_to_like_for_punctuation_only_query( assert any(r["id"] == mem_id for r in results) +@pytest.mark.asyncio +async def test_semantic_fallback_like_marks_its_retrieval_mode(tmp_path): + memory = MARMMemory(str(tmp_path / "memory.db")) + memory._encoder_failed = True + + mem_id = await memory.store_memory( + "content with --- dashes inside", session="semantic-like" + ) + + results = await memory.recall_similar( + "---", session="semantic-like", limit=5, exact_mode="semantic" + ) + + assert any(result["id"] == mem_id for result in results) + assert {result["retrieval_mode"] for result in results} == { + "semantic_fallback_like" + } + + @pytest.mark.asyncio async def test_recall_text_search_falls_back_to_like_when_fts5_raises( monkeypatch, tmp_path diff --git a/marm-mcp-server/tests/test_runtime_cli.py b/marm-mcp-server/tests/test_runtime_cli.py index 4499c5b4..4a9214c1 100644 --- a/marm-mcp-server/tests/test_runtime_cli.py +++ b/marm-mcp-server/tests/test_runtime_cli.py @@ -230,16 +230,23 @@ def __init__(self, returncode=0, stdout=""): self.returncode = returncode self.stdout = stdout + system_root = tmp_path / "windows" + whoami = system_root / "System32" / "whoami.exe" + icacls = system_root / "System32" / "icacls.exe" + def run(command, **kwargs): calls.append(command) - if command == ["whoami"]: + if command == [str(whoami)]: return Completed(stdout="DOMAIN\\runtime-user\r\n") return Completed() monkeypatch.setattr(security.sys, "platform", "win32") + monkeypatch.setenv("SystemRoot", str(system_root)) monkeypatch.setattr(security.subprocess, "run", run) assert security.restrict_windows_file_to_current_user(tmp_path / ".env") + assert calls[0] == [str(whoami)] + assert calls[1][0] == str(icacls) assert calls[1][-1] == "DOMAIN\\runtime-user:(F)" diff --git a/marm-mcp-server/tests/test_temporal_weighting.py b/marm-mcp-server/tests/test_temporal_weighting.py index f4a4ea40..82b69c07 100644 --- a/marm-mcp-server/tests/test_temporal_weighting.py +++ b/marm-mcp-server/tests/test_temporal_weighting.py @@ -196,17 +196,20 @@ def _insert(ts): "semantic-intent fallback must rank the newer memory first" ) - # Exact lane -> identical BM25 for identical content -> insertion order, age ignored + assert {result["retrieval_mode"] for result in semantic} == { + "semantic_fallback_fts" + } + + # Exact lane preserves lexical scores rather than applying temporal decay. exact = await mem.recall_similar( "temporal keyword content", session="fallback-temporal", limit=5, exact_mode="exact", ) - ex_ids = [r["id"] for r in exact] - assert ex_ids.index(old_id) < ex_ids.index(new_id), ( - "exact lane must preserve BM25/insertion order, not re-rank by age" - ) + exact_by_id = {result["id"]: result for result in exact} + assert {result["retrieval_mode"] for result in exact} == {"exact_fts"} + assert exact_by_id[old_id]["similarity"] == exact_by_id[new_id]["similarity"] @pytest.mark.asyncio diff --git a/scripts/benchmarking/performance/bench_hotpath.py b/scripts/benchmarking/performance/bench_hotpath.py index fe0d4d3c..72c9fe84 100644 --- a/scripts/benchmarking/performance/bench_hotpath.py +++ b/scripts/benchmarking/performance/bench_hotpath.py @@ -249,21 +249,41 @@ async def bench_hybrid_strategies(mem, sizes=None, iters=15): query_vec = mem._encode_sync(query) fts_query = _safe_fts_query(query) - # 1. Full semantic scan (production scorer, no pre-filter). - # Dispatched via asyncio.to_thread to match how recall_similar runs - # its DB/scoring work, so both columns share one execution model. - t0 = time.perf_counter() - await asyncio.to_thread( - _fetch_and_score_embedding_rows, mem.db_path, "bench", n, query_vec, 5 - ) - scan_samples.append((time.perf_counter() - t0) * 1000) - - # 2. Production hybrid recall (precomputed vector excludes encode) - t0 = time.perf_counter() - await mem.recall_similar( - query, session="bench", limit=5, query_vec=query_vec - ) - prod_samples.append((time.perf_counter() - t0) * 1000) + # Alternate order so one path does not always benefit from a warmed cache. + if iter_num % 2 == 0: + t0 = time.perf_counter() + await asyncio.to_thread( + _fetch_and_score_embedding_rows, + mem.db_path, + "bench", + n, + query_vec, + 5, + ) + scan_samples.append((time.perf_counter() - t0) * 1000) + + t0 = time.perf_counter() + await mem.recall_similar( + query, session="bench", limit=5, query_vec=query_vec + ) + prod_samples.append((time.perf_counter() - t0) * 1000) + else: + t0 = time.perf_counter() + await mem.recall_similar( + query, session="bench", limit=5, query_vec=query_vec + ) + prod_samples.append((time.perf_counter() - t0) * 1000) + + t0 = time.perf_counter() + await asyncio.to_thread( + _fetch_and_score_embedding_rows, + mem.db_path, + "bench", + n, + query_vec, + 5, + ) + scan_samples.append((time.perf_counter() - t0) * 1000) # Informational only: how many candidates the FTS pre-filter matched. if fts_query: diff --git a/scripts/check-file-length.py b/scripts/check-file-length.py index e55a6ff1..fe2a0108 100644 --- a/scripts/check-file-length.py +++ b/scripts/check-file-length.py @@ -22,6 +22,9 @@ ROOT / "marm-console" / "artifacts" / "marm-console" / "src", ), ] +TEST_DIRS = [ + ("marm-mcp-server/tests", ROOT / "marm-mcp-server" / "tests"), +] EXTENSIONS = {".py", ".toml", ".md", ".txt", ".json", ".ts", ".tsx", ".css"} # Build artifacts, not source: the bundled spaCy pipeline and the compiled @@ -61,14 +64,21 @@ def main() -> int: action="store_true", help="Also run find-versions after the check", ) + parser.add_argument( + "--tests", + action="store_true", + help="Also scan test folders (marm-mcp-server/tests)", + ) args = parser.parse_args() threshold = args.threshold print(f"{CYAN}=== File Length Check (>{threshold} lines) ==={RESET}\n") + scan_dirs = SCAN_DIRS + TEST_DIRS if args.tests else SCAN_DIRS + results: dict[str, list[tuple[int, str, str]]] = defaultdict(list) - for label, base in SCAN_DIRS: + for label, base in scan_dirs: if not base.exists(): print(f"{YELLOW}Warning: {label}/ not found, skipping{RESET}") continue diff --git a/scripts/clean-pytest-artifacts.py b/scripts/clean-pytest-artifacts.py index 02f46170..fffeb0fc 100644 --- a/scripts/clean-pytest-artifacts.py +++ b/scripts/clean-pytest-artifacts.py @@ -45,8 +45,12 @@ def discover_targets() -> list[Path]: for base in (ROOT, SERVER_ROOT, CONSOLE_ROOT): add_existing_directory(base / ".pytest_cache", targets) add_child_directories(base, ".pytest_tmp*", targets) + add_child_directories(base, ".pytest-review-*", targets) + add_child_directories(base, ".pytest-smoke-*", targets) add_child_directories(base / "tmp", "pytest-*", targets) + add_child_directories(ROOT, "marm-pytest-*", targets) + add_existing_directory(Path(r"C:\tmp\marm-pytest"), targets) safe_roots = existing_safe_roots() diff --git a/scripts/run-tests.py b/scripts/run-tests.py index b3fd718e..8f09106c 100644 --- a/scripts/run-tests.py +++ b/scripts/run-tests.py @@ -94,9 +94,12 @@ def pytest_base_command(args: argparse.Namespace) -> list[str]: def run_pytest_all(args: argparse.Namespace) -> bool: + environment = pytest_env() + if args.docker or args.slow: + environment["MARM_SMOKE_DOCKER"] = "1" command = pytest_base_command(args) command.append("tests") - return run_step("Pytest suite", command, SERVER_ROOT, env=pytest_env()) + return run_step("Pytest suite", command, SERVER_ROOT, env=environment) def run_compile_check(cwd: Path, *targets: str) -> bool: diff --git a/scripts/test-scripts/smoke-commands.py b/scripts/test-scripts/smoke-commands.py index 58dbdc1b..89956bef 100644 --- a/scripts/test-scripts/smoke-commands.py +++ b/scripts/test-scripts/smoke-commands.py @@ -3,6 +3,7 @@ from __future__ import annotations import argparse +import os import subprocess import sys from pathlib import Path @@ -59,7 +60,14 @@ def main() -> int: "-v", ] print(f"Running command smoke tests: {' '.join(command)}") - return subprocess.run(command, cwd=PACKAGE_ROOT, check=False).returncode + environment = os.environ.copy() + if args.docker: + environment["MARM_SMOKE_DOCKER"] = "1" + if args.destructive: + environment["MARM_SMOKE_DESTRUCTIVE"] = "1" + return subprocess.run( + command, cwd=PACKAGE_ROOT, env=environment, check=False + ).returncode if __name__ == "__main__": From 1f7b1209fd9f506b26e587b48476b16211afb50e Mon Sep 17 00:00:00 2001 From: Ryan Lyell Date: Fri, 24 Jul 2026 05:43:21 -0400 Subject: [PATCH 3/3] fix(security): native Win32 DACL for key-file lockdown, warn on stderr Second CodeRabbit pass on PR #111: - security.py: replace the subprocess whoami/icacls approach with a native Win32 ctypes DACL replacement (OpenProcessToken -> token SID -> SetEntriesInAclW -> SetNamedSecurityInfoW). Spawns no external binaries, so the SystemRoot/untrusted-search-path exposure (CWE-426) is gone entirely. Verified on Windows: the file ACL collapses to a single owner-only full-control entry with inheritance removed. - settings.py: send the ACL-failure warning to stderr so it cannot corrupt the STDIO JSON-RPC stream. - test_runtime_cli.py: keep the delegation test and add a real Windows-gated test that runs the ctypes path and asserts the on-disk DACL is owner-only (skipped off win32). Co-Authored-By: Claude Opus 4.8 --- .../marm_mcp_server/config/settings.py | 5 +- .../marm_mcp_server/utils/security.py | 166 ++++++++++++++---- marm-mcp-server/tests/test_runtime_cli.py | 56 +++--- 3 files changed, 175 insertions(+), 52 deletions(-) diff --git a/marm-mcp-server/marm_mcp_server/config/settings.py b/marm-mcp-server/marm_mcp_server/config/settings.py index 69cec5fb..c4bfa30d 100644 --- a/marm-mcp-server/marm_mcp_server/config/settings.py +++ b/marm-mcp-server/marm_mcp_server/config/settings.py @@ -400,7 +400,10 @@ def _load_key_from_file() -> str: if sys.platform == "win32" and not restrict_windows_file_to_current_user( _MARM_ENV_PATH ): - print(f"WARNING: Could not restrict API key file: {_MARM_ENV_PATH}") + print( + f"WARNING: Could not restrict API key file: {_MARM_ENV_PATH}", + file=sys.stderr, + ) except Exception as _e: print(f"WARNING: Could not save API key to {_MARM_ENV_PATH}: {_e}") diff --git a/marm-mcp-server/marm_mcp_server/utils/security.py b/marm-mcp-server/marm_mcp_server/utils/security.py index 83704cef..85f5cb52 100644 --- a/marm-mcp-server/marm_mcp_server/utils/security.py +++ b/marm-mcp-server/marm_mcp_server/utils/security.py @@ -1,9 +1,8 @@ """Cryptographic utilities — no imports from settings, no side effects.""" -import os +import ctypes import secrets import string -import subprocess import sys from pathlib import Path @@ -23,35 +22,142 @@ def generate_api_key(length: int = 40) -> str: return "".join(key) +def _set_windows_owner_only_dacl(path: Path) -> bool: + """Replace a file DACL with one full-control entry for the process user.""" + from ctypes import wintypes + + token_query = 0x0008 + token_user_class = 1 + set_access = 2 + trustee_is_sid = 0 + trustee_is_unknown = 0 + generic_all = 0x10000000 + no_inheritance = 0 + se_file_object = 1 + dacl_security_information = 0x00000004 + protected_dacl_security_information = 0x80000000 + + class SidAndAttributes(ctypes.Structure): + _fields_ = [("sid", wintypes.LPVOID), ("attributes", wintypes.DWORD)] + + class TokenUser(ctypes.Structure): + _fields_ = [("user", SidAndAttributes)] + + class Trustee(ctypes.Structure): + _fields_ = [ + ("multiple_trustee", wintypes.LPVOID), + ("multiple_trustee_operation", wintypes.DWORD), + ("trustee_form", wintypes.DWORD), + ("trustee_type", wintypes.DWORD), + ("name", wintypes.LPWSTR), + ] + + class ExplicitAccess(ctypes.Structure): + _fields_ = [ + ("access_permissions", wintypes.DWORD), + ("access_mode", wintypes.DWORD), + ("inheritance", wintypes.DWORD), + ("trustee", Trustee), + ] + + try: + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + advapi32 = ctypes.WinDLL("advapi32", use_last_error=True) + kernel32.GetCurrentProcess.argtypes = [] + kernel32.GetCurrentProcess.restype = wintypes.HANDLE + kernel32.CloseHandle.argtypes = [wintypes.HANDLE] + kernel32.CloseHandle.restype = wintypes.BOOL + kernel32.LocalFree.argtypes = [wintypes.LPVOID] + kernel32.LocalFree.restype = wintypes.LPVOID + advapi32.OpenProcessToken.argtypes = [ + wintypes.HANDLE, + wintypes.DWORD, + ctypes.POINTER(wintypes.HANDLE), + ] + advapi32.OpenProcessToken.restype = wintypes.BOOL + advapi32.GetTokenInformation.argtypes = [ + wintypes.HANDLE, + wintypes.DWORD, + wintypes.LPVOID, + wintypes.DWORD, + ctypes.POINTER(wintypes.DWORD), + ] + advapi32.GetTokenInformation.restype = wintypes.BOOL + advapi32.SetEntriesInAclW.argtypes = [ + wintypes.DWORD, + ctypes.POINTER(ExplicitAccess), + wintypes.LPVOID, + ctypes.POINTER(wintypes.LPVOID), + ] + advapi32.SetEntriesInAclW.restype = wintypes.DWORD + advapi32.SetNamedSecurityInfoW.argtypes = [ + wintypes.LPWSTR, + wintypes.DWORD, + wintypes.DWORD, + wintypes.LPVOID, + wintypes.LPVOID, + wintypes.LPVOID, + wintypes.LPVOID, + ] + advapi32.SetNamedSecurityInfoW.restype = wintypes.DWORD + token = wintypes.HANDLE() + if not advapi32.OpenProcessToken( + kernel32.GetCurrentProcess(), token_query, ctypes.byref(token) + ): + return False + try: + size = wintypes.DWORD() + advapi32.GetTokenInformation( + token, token_user_class, None, 0, ctypes.byref(size) + ) + if not size.value: + return False + buffer = ctypes.create_string_buffer(size.value) + if not advapi32.GetTokenInformation( + token, token_user_class, buffer, size, ctypes.byref(size) + ): + return False + token_user = ctypes.cast(buffer, ctypes.POINTER(TokenUser)).contents + access = ExplicitAccess( + generic_all, + set_access, + no_inheritance, + Trustee( + None, + 0, + trustee_is_sid, + trustee_is_unknown, + ctypes.cast(token_user.user.sid, wintypes.LPWSTR), + ), + ) + dacl = wintypes.LPVOID() + if advapi32.SetEntriesInAclW( + 1, ctypes.byref(access), None, ctypes.byref(dacl) + ): + return False + try: + return ( + advapi32.SetNamedSecurityInfoW( + str(path), + se_file_object, + dacl_security_information | protected_dacl_security_information, + None, + None, + dacl, + None, + ) + == 0 + ) + finally: + kernel32.LocalFree(dacl) + finally: + kernel32.CloseHandle(token) + except (AttributeError, OSError): + return False + + def restrict_windows_file_to_current_user(path: Path) -> bool: """Grant the executing Windows identity exclusive access to a sensitive file.""" if sys.platform != "win32": return True - try: - system_root = os.environ.get("SystemRoot") - if not system_root: - return False - system32 = Path(system_root) / "System32" - whoami = system32 / "whoami.exe" - icacls = system32 / "icacls.exe" - if not whoami.is_absolute() or not icacls.is_absolute(): - return False - identity = subprocess.run( - [str(whoami)], check=False, capture_output=True, text=True - ).stdout.strip() - if not identity: - return False - result = subprocess.run( - [ - str(icacls), - str(path), - "/inheritance:r", - "/grant:r", - f"{identity}:(F)", - ], - check=False, - capture_output=True, - ) - return result.returncode == 0 - except OSError: - return False + return _set_windows_owner_only_dacl(path) diff --git a/marm-mcp-server/tests/test_runtime_cli.py b/marm-mcp-server/tests/test_runtime_cli.py index 4a9214c1..f437b05d 100644 --- a/marm-mcp-server/tests/test_runtime_cli.py +++ b/marm-mcp-server/tests/test_runtime_cli.py @@ -220,34 +220,48 @@ def test_managed_key_init_reuses_existing_credential(monkeypatch, tmp_path): assert active_key_management.read_managed_key(path) == "first-key" -def test_windows_key_acl_uses_the_executing_identity(monkeypatch, tmp_path): +def test_windows_key_acl_delegates_to_dacl_replacement(monkeypatch, tmp_path): from marm_mcp_server.utils import security - calls = [] + calls: list[Path] = [] - class Completed: - def __init__(self, returncode=0, stdout=""): - self.returncode = returncode - self.stdout = stdout + monkeypatch.setattr(security.sys, "platform", "win32") + monkeypatch.setattr( + security, + "_set_windows_owner_only_dacl", + lambda path: calls.append(path) or True, + ) - system_root = tmp_path / "windows" - whoami = system_root / "System32" / "whoami.exe" - icacls = system_root / "System32" / "icacls.exe" + assert security.restrict_windows_file_to_current_user(tmp_path / ".env") + assert calls == [tmp_path / ".env"] - def run(command, **kwargs): - calls.append(command) - if command == [str(whoami)]: - return Completed(stdout="DOMAIN\\runtime-user\r\n") - return Completed() - monkeypatch.setattr(security.sys, "platform", "win32") - monkeypatch.setenv("SystemRoot", str(system_root)) - monkeypatch.setattr(security.subprocess, "run", run) +@pytest.mark.skipif(sys.platform != "win32", reason="Windows DACL API only") +def test_windows_dacl_replacement_locks_file_to_current_user(tmp_path): + """Exercise the real ctypes DACL path and confirm the on-disk ACL ends up + owner-only. Guards the security-critical replacement, not just delegation.""" + from marm_mcp_server.utils import security - assert security.restrict_windows_file_to_current_user(tmp_path / ".env") - assert calls[0] == [str(whoami)] - assert calls[1][0] == str(icacls) - assert calls[1][-1] == "DOMAIN\\runtime-user:(F)" + target = tmp_path / ".env" + target.write_text("SECRET=abc\n", encoding="utf-8") + + before = subprocess.run( + ["icacls", str(target)], capture_output=True, text=True + ).stdout + assert "(I)" in before, "fixture file should start with inherited ACEs" + + assert security._set_windows_owner_only_dacl(target) is True + + after = subprocess.run( + ["icacls", str(target)], capture_output=True, text=True + ).stdout + identity = subprocess.run(["whoami"], capture_output=True, text=True).stdout.strip() + + acl_lines = [line for line in after.splitlines() if ":(" in line] + assert len(acl_lines) == 1, f"expected a single ACE, got: {acl_lines}" + assert identity.lower() in acl_lines[0].lower() + assert "(F)" in acl_lines[0] + assert "(I)" not in after, "inheritance must be removed" def test_key_path_and_reveal_keep_output_intentional(monkeypatch, capsys, tmp_path):