fix: sanitize error responses and remove sys.exit from library code#139
fix: sanitize error responses and remove sys.exit from library code#139bensig merged 3 commits intoMemPalace:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to harden MemPalace’s library/MCP surfaces by reducing sensitive details in error responses and removing process-terminating behavior from importable code, alongside updates to packaging and expanded test coverage.
Changes:
- Sanitize MCP tool-dispatch error responses and remove palace path from
_no_palace()errors. - Replace
sys.exit(1)inmempalace.searcher.search()withreturnto avoid terminating the host process. - Update packaging configuration (move to
hatchling, removerequirements.txt) and add/expand tests.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
mempalace/mcp_server.py |
Removes palace path from a common error payload and sanitizes dispatch-layer error messages. |
mempalace/searcher.py |
Removes sys.exit usage from the search function to make it safer as library code. |
mempalace/__init__.py |
Updates package version constant to 3.0.0. |
pyproject.toml |
Switches build backend to hatchling, adds wheel packaging config, updates dev deps config. |
requirements.txt |
Removes legacy dependency file. |
tests/conftest.py |
Adds fixtures + environment isolation to keep tests from touching user data. |
tests/test_mcp_server.py |
Adds unit/integration tests for MCP server handlers and dispatch. |
tests/test_searcher.py |
Adds tests for the programmatic search_memories API. |
tests/test_knowledge_graph.py |
Adds coverage for temporal KG operations and queries. |
tests/test_dialect.py |
Adds coverage for AAAK dialect compression/encoding/decoding utilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| print(f"\n No palace found at {palace_path}") | ||
| print(" Run: mempalace init <dir> then mempalace mine <dir>") | ||
| sys.exit(1) | ||
| return |
There was a problem hiding this comment.
Replacing sys.exit(1) with return inside search() changes CLI exit codes: mempalace search will now exit 0 even when the palace is missing or a query fails, whereas previously it exited non-zero. To avoid terminating processes when used as a library while preserving CLI behavior, consider having search() raise a typed exception or return a success flag, and have cmd_search (or main) convert failures into sys.exit(1).
| except Exception as e: | ||
| logger.error(f"Tool error in {tool_name}: {e}") |
There was a problem hiding this comment.
handle_request now sanitizes the tool error message returned to the client (good), but the logging line only includes str(e) and drops the traceback, which contradicts the PR description that the “full exception” is still logged. Consider using logger.exception(...) or logger.error(..., exc_info=True) so stderr logs retain stack traces for debugging while keeping the client-facing message generic.
| except Exception as e: | |
| logger.error(f"Tool error in {tool_name}: {e}") | |
| except Exception: | |
| logger.exception(f"Tool error in {tool_name}") |
| def _no_palace(): | ||
| return { | ||
| "error": "No palace found", | ||
| "palace_path": _config.palace_path, | ||
| "hint": "Run: mempalace init <dir> && mempalace mine <dir>", | ||
| } |
There was a problem hiding this comment.
This removes _config.palace_path from _no_palace() responses, but MCP mempalace_search can still leak the local palace path when the palace is missing because tool_search delegates to search_memories(), which formats {"error": f"No palace found at {palace_path}: {e}"}. If the goal is to avoid exposing filesystem paths to MCP clients, consider sanitizing search_memories errors (and any other tool handlers) similarly.
| def _patch_mcp_server(monkeypatch, config, palace_path, kg): | ||
| """Patch the mcp_server module globals to use test fixtures.""" | ||
| from mempalace import mcp_server | ||
|
|
There was a problem hiding this comment.
_patch_mcp_server accepts palace_path but doesn’t use it (it only patches _config and _kg). Dropping the unused parameter (or using it to assert _config.palace_path matches) would reduce confusion and keep the helper’s signature aligned with its behavior.
| if getattr(config, "palace_path", None) != palace_path: | |
| raise AssertionError( | |
| f"config.palace_path ({getattr(config, 'palace_path', None)!r}) " | |
| f"does not match palace_path fixture ({palace_path!r})" | |
| ) |
|
Hey Igor — CI is failing, looks like it needs a rebase on main (we've merged a bunch of changes recently). Can you rebase and push? |
- Remove palace_path from _no_palace() error response (prevents leaking filesystem paths to the LLM) - Replace str(e) with generic 'Internal tool error' in MCP dispatch catch block (full error is still logged server-side via stderr) - Replace sys.exit(1) with return in searcher.search() CLI function (prevents process termination if called from library context) - Remove unused sys import from searcher.py Findings: MemPalace#12 (HIGH), MemPalace#5 (MEDIUM), MemPalace#15 (LOW) Includes test infrastructure from PR MemPalace#131. 92 tests pass.
… validate fixture
igorls
force-pushed
the
fix/error-handling
branch
from
0ec54dd to
5ac4947
Compare
igorls
force-pushed
the
fix/error-handling
branch
from
5ac4947 to
45c2c92
Compare
…nused fixture param
Changes
1. Remove filesystem path from error response (LOW)
def _no_palace(): return { "error": "No palace found", - "palace_path": _config.palace_path, "hint": "Run: mempalace init <dir> && mempalace mine <dir>", }2. Sanitize dispatch error messages (HIGH)
except Exception as e: logger.error(f"Tool error in {tool_name}: {e}") - return {"error": {"message": str(e)}} + return {"error": {"message": "Internal tool error"}}The full exception is still logged to stderr for debugging.
3. Replace sys.exit(1) with return (MEDIUM)
searcher.search()is a CLI-facing function that previously calledsys.exit(1)on error. If this function is ever called from library code (e.g., imported by another module), it would terminate the process. Replaced withreturnwhich preserves the same CLI behavior (no output = error) without the risk.92 tests pass (includes test infrastructure from PR #131).