Merge pull request #2126 from MemPalace/fix/2103-onto-develop

fix(mcp): refuse config and ack writes in read-only mode (#2103)

Author: igorls

mempalace/cli.py

@@ -2189,7 +2189,7 @@ def main():
     p_serve.add_argument(
         "--read-only",
         action="store_true",
-        help="Expose recall only: mutating tools are hidden and refused",
+        help="Expose recall only: tools that change state are hidden and refused",
     )
     p_serve.add_argument(
         "--allow-insecure",

mempalace/mcp_server.py

@@ -290,8 +290,8 @@ def _parse_args():
     parser.add_argument(
         "--read-only",
         action="store_true",
-        help="Serve a read-only tool surface: the mutating tools are hidden from "
-        "tools/list and refused at dispatch (env MEMPALACE_MCP_READ_ONLY)",
+        help="Serve a read-only tool surface: the tools that change state are hidden "
+        "from tools/list and refused at dispatch (env MEMPALACE_MCP_READ_ONLY)",
     )
     args, unknown = parser.parse_known_args()
     if unknown:
@@ -313,10 +313,12 @@ def _parse_args():
 
 _config = MempalaceConfig()
 
-# Read-only server mode: when on, the mutating tools are hidden from tools/list
-# and refused at dispatch (-32003). Resolved once at startup from --read-only or
-# MEMPALACE_MCP_READ_ONLY. Computed inline (not via _truthy_env, defined below)
-# so it is available to the request path regardless of import order.
+# Read-only server mode: when on, the tools in _READ_ONLY_REFUSED_TOOLS (defined
+# below) are hidden from tools/list and refused at dispatch (-32003). That is a
+# wider set than the _MUTATING_TOOLS the peer-writer guard uses. Resolved once at
+# startup from --read-only or MEMPALACE_MCP_READ_ONLY. Computed inline (not via
+# _truthy_env, defined below) so it is available to the request path regardless
+# of import order.
 _READ_ONLY = bool(getattr(_args, "read_only", False)) or os.environ.get(
     "MEMPALACE_MCP_READ_ONLY", ""
 ).strip().lower() in {"1", "true", "yes", "on"}
@@ -403,6 +405,42 @@ def _parse_args():
     }
 )
 
+# Read-only mode (#1877) refuses a wider set than the peer-writer guard above.
+#
+# _MUTATING_TOOLS is the *palace-write* set: _mcp_peer_writer_refusal consults it
+# to decide which calls need this process to hold the palace mine lock. A tool
+# that never touches Chroma or the knowledge graph has to stay out of that set,
+# or a server that lost the lease to a peer would start refusing calls the lease
+# has no say over.
+#
+# Two tools are exactly that shape, and read-only has to name both because it is
+# a capability boundary rather than a lock: it exists so a shared server can
+# serve recall to a client that must not change server state.
+#
+#   mempalace_hook_settings, given an argument, writes the server's
+#   ~/.mempalace/config.json through MempalaceConfig.set_hook_setting.
+#   service.WRITE_TOOLS already classifies it as a write, which the daemon uses
+#   as an allowlist, so read-only was the odd one out.
+#
+#   mempalace_memories_filed_away unlinks ~/.mempalace/hook_state/last_checkpoint
+#   on both of its branches. Consuming the file is the contract of the tool, but
+#   it is still a delete of state that outlives the process, on behalf of a
+#   client with no write access. (service.classify_tool calls this one "read",
+#   which is wrong for the same reason.)
+#
+# mempalace_reconnect is deliberately NOT here even though it is not write-free:
+# it clears ChromaBackend._quarantined_paths, so the reopen that follows can let
+# quarantine_stale_hnsw rename a segment directory. It is the only way to pick up
+# an external writer's changes, and _SQLITE_INTEGRITY_ALLOWED_TOOLS already keeps
+# it reachable for recovery, so gating it would strand a read-only server on a
+# stale index. This set means "refuse what a client asked to change", not
+# "nothing past here touches the disk" -- opening the palace or the knowledge
+# graph materialises files on its own, which no name-based gate can express.
+_READ_ONLY_REFUSED_TOOLS = _MUTATING_TOOLS | {
+    "mempalace_hook_settings",
+    "mempalace_memories_filed_away",
+}
+
 
 def _truthy_env(name: str) -> bool:
     return os.environ.get(name, "").strip().lower() in {"1", "true", "yes", "on"}
@@ -4816,15 +4854,19 @@ def _internal_tool_error(req_id, tool_name: str, exc: BaseException = None) -> d
 
 
 def _mcp_read_only_refusal(req_id, tool_name: str):
-    """Refuse mutating tools when the server runs in read-only mode (#1877).
+    """Refuse state-changing tools when the server runs in read-only mode (#1877).
 
     Read-only is an operator-set server mode (``--read-only`` /
     ``MEMPALACE_MCP_READ_ONLY``), distinct from the dynamic peer-writer lock:
     it is an unconditional gate so a shared team server can expose recall
     without write access. Enforced at dispatch, not merely hidden from
     tools/list, so a client that calls a mutating tool by name is still refused.
+
+    Gates on ``_READ_ONLY_REFUSED_TOOLS``, not ``_MUTATING_TOOLS``: a tool can
+    write outside the palace database, which the peer-writer lease has no reason
+    to arbitrate but read-only still has to refuse.
     """
-    if not _READ_ONLY or tool_name not in _MUTATING_TOOLS:
+    if not _READ_ONLY or tool_name not in _READ_ONLY_REFUSED_TOOLS:
         return None
 
     return {
@@ -4896,16 +4938,17 @@ def handle_request(request):
         # Notifications (no id) never get a response per JSON-RPC spec
         return None
     elif method == "tools/list":
-        # In read-only mode, hide the mutating tools so clients don't advertise
+        # In read-only mode, hide the refused tools so clients don't advertise
         # write capabilities they can't use (dispatch also refuses them, #1877).
+        # Same set on both sides, or a tool would be listed and then rejected.
         return {
             "jsonrpc": "2.0",
             "id": req_id,
             "result": {
                 "tools": [
                     {"name": n, "description": t["description"], "inputSchema": t["input_schema"]}
                     for n, t in TOOLS.items()
-                    if not (_READ_ONLY and n in _MUTATING_TOOLS)
+                    if not (_READ_ONLY and n in _READ_ONLY_REFUSED_TOOLS)
                 ]
             },
         }

tests/test_mcp_http_transport.py

@@ -20,6 +20,7 @@
 import http.client
 import json
 import logging
+import os
 import socketserver
 import ssl
 import threading
@@ -201,7 +202,7 @@ def test_bearer_token_enforced_when_configured(monkeypatch):
 
 
 def test_read_only_hides_and_refuses_mutating_tools(http_server, monkeypatch):
-    """Read-only mode (#1877): mutating tools are hidden from tools/list AND
+    """Read-only mode (#1877): the refused tools are hidden from tools/list AND
     refused at dispatch with -32003, while read tools still work."""
     monkeypatch.setattr(mcp, "_READ_ONLY", True)
     port, _ = http_server
@@ -211,7 +212,7 @@ def test_read_only_hides_and_refuses_mutating_tools(http_server, monkeypatch):
     names = {t["name"] for t in json.loads(body)["result"]["tools"]}
     assert "mempalace_search" in names  # read tool stays
     assert "mempalace_add_drawer" not in names  # mutating tool hidden
-    assert names.isdisjoint(mcp._MUTATING_TOOLS)
+    assert names.isdisjoint(mcp._READ_ONLY_REFUSED_TOOLS)
 
     status, body = _post(
         port,
@@ -332,6 +333,117 @@ def fail_bind(host, port):
     assert events == ["bind-failed", "discard", "lease-exit"]
     assert mcp._MCP_WRITER_LOCK_CM is None
 
+def _hook_settings_call(req_id):
+    return {
+        "jsonrpc": "2.0",
+        "id": req_id,
+        "method": "tools/call",
+        "params": {
+            "name": "mempalace_hook_settings",
+            "arguments": {"silent_save": False, "desktop_toast": True},
+        },
+    }
+
+
+def test_read_only_refuses_the_hook_settings_config_write(http_server, monkeypatch, tmp_path):
+    """mempalace_hook_settings writes the server's ~/.mempalace/config.json.
+
+    It touches no palace state, so it is correctly absent from _MUTATING_TOOLS,
+    the palace-write set the peer-writer lease arbitrates. Read-only gated on
+    that set, which let a read-only server persist a config change on behalf of
+    a client that is supposed to have no write access at all.
+
+    The first half is the control: it proves the write really does land here, so
+    the "unchanged" assertion in the second half cannot pass vacuously.
+    """
+    home = tmp_path / "home"
+    (home / ".mempalace").mkdir(parents=True)
+    cfg_file = home / ".mempalace" / "config.json"
+    cfg_file.write_text(
+        json.dumps({"hooks": {"silent_save": True, "desktop_toast": False}}), encoding="utf-8"
+    )
+    monkeypatch.setenv("HOME", str(home))
+    monkeypatch.setenv("USERPROFILE", str(home))
+    monkeypatch.setenv("HOMEDRIVE", os.path.splitdrive(str(home))[0] or "C:")
+    monkeypatch.setenv("HOMEPATH", os.path.splitdrive(str(home))[1] or str(home))
+    pristine = cfg_file.read_bytes()
+
+    port, _ = http_server
+
+    # Control: the gate is off, so the very same call rewrites config.json.
+    # _READ_ONLY is resolved at import from the environment, so pin it rather
+    # than inherit whatever the suite was started with.
+    monkeypatch.setattr(mcp, "_READ_ONLY", False)
+    status, body = _post(port, "/mcp", _hook_settings_call(1))
+    assert status == 200
+    # The handler reports its own failures inside `result` as {"success": false},
+    # not as a JSON-RPC error, so check the payload rather than just the envelope.
+    payload = json.loads(body)
+    assert "error" not in payload
+    assert json.loads(payload["result"]["content"][0]["text"])["success"] is True
+    assert cfg_file.read_bytes() != pristine
+    cfg_file.write_bytes(pristine)
+
+    # Gate on: hidden from tools/list, refused at dispatch, file left alone.
+    monkeypatch.setattr(mcp, "_READ_ONLY", True)
+
+    status, body = _post(port, "/mcp", {"jsonrpc": "2.0", "id": 2, "method": "tools/list"})
+    names = {t["name"] for t in json.loads(body)["result"]["tools"]}
+    assert "mempalace_hook_settings" not in names
+
+    status, body = _post(port, "/mcp", _hook_settings_call(3))
+    assert status == 200
+    assert json.loads(body)["error"]["code"] == -32003
+    assert cfg_file.read_bytes() == pristine
+
+
+def test_read_only_refuses_the_checkpoint_ack_delete(http_server, monkeypatch, tmp_path):
+    """mempalace_memories_filed_away unlinks the Stop hook's checkpoint ack file.
+
+    Consuming that file is the contract of the tool, but it is still a delete of
+    state that outlives the process, done for a client with no write access. Same
+    two-phase shape as the config test: the control proves the delete lands, so
+    the survival assertion afterwards cannot pass vacuously.
+    """
+    home = tmp_path / "home"
+    state_dir = home / ".mempalace" / "hook_state"
+    state_dir.mkdir(parents=True)
+    ack = state_dir / "last_checkpoint"
+    ack.write_text(json.dumps({"msgs": 7, "ts": "2026-01-01T00:00:00"}), encoding="utf-8")
+    monkeypatch.setenv("HOME", str(home))
+    monkeypatch.setenv("USERPROFILE", str(home))
+    monkeypatch.setenv("HOMEDRIVE", os.path.splitdrive(str(home))[0] or "C:")
+    monkeypatch.setenv("HOMEPATH", os.path.splitdrive(str(home))[1] or str(home))
+
+    port, _ = http_server
+    call = {
+        "jsonrpc": "2.0",
+        "id": 1,
+        "method": "tools/call",
+        "params": {"name": "mempalace_memories_filed_away", "arguments": {}},
+    }
+
+    # Control: the gate is off, so the call consumes the ack file.
+    monkeypatch.setattr(mcp, "_READ_ONLY", False)
+    status, body = _post(port, "/mcp", call)
+    assert status == 200
+    assert json.loads(json.loads(body)["result"]["content"][0]["text"])["count"] == 7
+    assert not ack.exists()
+
+    # Gate on: refused, and a fresh ack file survives untouched.
+    ack.write_text(json.dumps({"msgs": 7, "ts": "2026-01-01T00:00:00"}), encoding="utf-8")
+    pristine = ack.read_bytes()
+    monkeypatch.setattr(mcp, "_READ_ONLY", True)
+
+    status, body = _post(port, "/mcp", {"jsonrpc": "2.0", "id": 2, "method": "tools/list"})
+    names = {t["name"] for t in json.loads(body)["result"]["tools"]}
+    assert "mempalace_memories_filed_away" not in names
+
+    status, body = _post(port, "/mcp", dict(call, id=3))
+    assert status == 200
+    assert json.loads(body)["error"]["code"] == -32003
+    assert ack.read_bytes() == pristine
+
 
 @pytest.mark.parametrize(
     "disconnect_exc",

tests/test_mcp_server.py

@@ -5300,6 +5300,85 @@ def forbidden_lock():
     assert '"ok": true' in response["result"]["content"][0]["text"]
 
 
+def test_read_only_refuses_exactly_the_refused_set(monkeypatch):
+    """Ask the gate which tools it refuses instead of restating the set.
+
+    Comparing against the whole TOOLS registry also catches a stale name: a tool
+    renamed or removed while the set still lists it would gate nothing, and the
+    two sides would stop matching.
+    """
+    from mempalace import mcp_server
+
+    monkeypatch.setattr(mcp_server, "_READ_ONLY", True)
+
+    refused = {
+        name for name in mcp_server.TOOLS if mcp_server._mcp_read_only_refusal(1, name) is not None
+    }
+    assert refused == set(mcp_server._READ_ONLY_REFUSED_TOOLS)
+    assert "mempalace_hook_settings" in refused
+    assert "mempalace_memories_filed_away" in refused
+    # Reconnect stays reachable on purpose: it is the only way a read-only
+    # server picks up an external writer's changes.
+    assert "mempalace_reconnect" not in refused
+
+    # The palace-write set the peer-writer lease arbitrates stays the narrower
+    # of the two; see test_peer_writer_guard_does_not_gate_hook_settings.
+    assert mcp_server._MUTATING_TOOLS < mcp_server._READ_ONLY_REFUSED_TOOLS
+    assert "mempalace_hook_settings" not in mcp_server._MUTATING_TOOLS
+
+
+def test_read_only_refuses_every_daemon_write_tool():
+    """Read-only must not be laxer than the daemon's own write classification.
+
+    service.WRITE_TOOLS is a security allowlist: execute_job lets the generic
+    mcp_tool escape hatch run write-classified tools only. A tool the daemon
+    calls a write while read-only serves it is the exact gap this fixes, and
+    mempalace_hook_settings was that tool.
+    """
+    from mempalace import mcp_server, service
+
+    assert service.WRITE_TOOLS <= mcp_server._READ_ONLY_REFUSED_TOOLS
+    assert "mempalace_hook_settings" in service.WRITE_TOOLS
+
+
+def test_peer_writer_guard_does_not_gate_hook_settings(monkeypatch):
+    """The read-only widening must not leak into the peer-writer path.
+
+    mempalace_hook_settings writes the config file and never the palace, so it
+    stays out of _MUTATING_TOOLS and the lease has no say over it. Read-only
+    refuses it through _READ_ONLY_REFUSED_TOOLS instead. Were it moved into
+    _MUTATING_TOOLS, a peer holding the lease would refuse it with -32001,
+    including the no-argument form that only reads the current settings.
+    """
+    from mempalace import mcp_server
+
+    def forbidden_lock():
+        raise AssertionError("hook_settings should not acquire the peer-writer lock")
+
+    monkeypatch.setitem(
+        mcp_server.TOOLS,
+        "mempalace_hook_settings",
+        {
+            "description": "test config tool",
+            "input_schema": {"type": "object", "properties": {}},
+            "handler": lambda: {"ok": True},
+        },
+    )
+    monkeypatch.setattr(mcp_server, "_acquire_mcp_writer_lock", forbidden_lock)
+
+    response = mcp_server.handle_request(
+        {
+            "jsonrpc": "2.0",
+            "id": 9,
+            "method": "tools/call",
+            "params": {"name": "mempalace_hook_settings", "arguments": {}},
+        }
+    )
+
+    assert '"ok": true' in response["result"]["content"][0]["text"]
+    assert "mempalace_hook_settings" not in mcp_server._MUTATING_TOOLS
+
+
 def test_status_tool_does_not_acquire_peer_writer_lock(monkeypatch):
     from mempalace import mcp_server
 

website/guide/remote-server.md

@@ -132,7 +132,7 @@ Output includes the token and the exact client command. Useful flags:
 | `--port` | `8765` | Listen port |
 | `--backend` | config/env | Storage backend (e.g. `qdrant`) |
 | `--tls-cert` / `--tls-key` | _(none)_ | PEM cert + key to terminate **TLS natively** (server speaks `https`) |
-| `--read-only` | off | Expose recall only — the mutating tools are hidden and refused |
+| `--read-only` | off | Expose recall only — the tools that change state are hidden and refused |
 | `--token` | auto | Use a specific bearer token instead of the generated one |
 | `--allow-insecure` | off | Permit a non-loopback bind with no token (only behind a trusted proxy) |