feat(mcp): add basic_memory_diagnostics tool for version and system info#963
feat(mcp): add basic_memory_diagnostics tool for version and system info#963groksrc wants to merge 4 commits into
Conversation
Adds a new read-only MCP tool `basic_memory_diagnostics` that surfaces: - Basic Memory package version and API version (from __init__.py) - Python version and platform/architecture details - Config file path and full config.json contents (cloud_api_key redacted) Resolves #187. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29be0299f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from basic_memory.mcp.server import mcp | ||
|
|
||
| # Fields in BasicMemoryConfig that contain secrets and must never be surfaced. | ||
| _SECRET_FIELDS = frozenset({"cloud_api_key"}) |
There was a problem hiding this comment.
When config.json contains database_url for a Postgres deployment, BasicMemoryConfig allows URLs with user:pass@host, but this tool only removes cloud_api_key and then dumps the remaining config. In that scenario, calling basic_memory_diagnostics exposes the database password to any MCP client or copied support transcript; redact database_url or at least strip URI userinfo before serializing the config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this was a real gap. Fixed in cbc2d5f.
What changed:
- Added
_redact_url()helper that strips theuser:passworduserinfo component from a URL usingurlparse/urlunparse, leaving host, port, and database name intact for diagnostic value. - Added
_URL_FIELDS = frozenset({"database_url"})alongside the existing_SECRET_FIELDS, so future credential-bearing URL fields are easy to add. _redact_config()now routes URL fields through_redact_url()rather than dropping them entirely.
Tests added: _redact_url unit tests (password stripped, port/host preserved, no-credential URL unchanged, non-URL path unchanged), _redact_config tests for database_url, and an end-to-end test_diagnostics_redacts_database_url_password that confirms a Postgres password never surfaces in the full diagnostic output.
Extends the existing _SECRET_FIELDS redaction to also strip the user:password userinfo component from any URL-bearing config fields (database_url) before surfacing them in diagnostics output. Adds _redact_url() helper and a _URL_FIELDS set so future credential-bearing URL fields are easy to add. Postgres passwords no longer leak to MCP clients or support transcripts; host/port/db-name remain visible. Fixes the P1 security finding raised in review of PR #963. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flags dotted-host substring checks against URLs (py/incomplete-url-substring-sanitization); exact matches are also stronger assertions. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
…d structured output
- Remove orphaned __api_version__ ('v0' since 2025; real API routes are /v2,
and nothing else reads the constant)
- output_schema=None so FastMCP doesn't duplicate the markdown report into
structuredContent (halves the payload)
- Add title/tags annotations so #963 and #968 are merge-order independent
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
|
This is tested and good to go, but needs to ship in v0.next and we have a maintenance release ahead of it. |
There was a problem hiding this comment.
Pull request overview
Adds a new read-only MCP diagnostics tool intended to help troubleshoot Basic Memory installations by reporting version/system details and a redacted view of the config file.
Changes:
- Added
basic_memory_diagnosticsMCP tool that returns a markdown diagnostics report (version/system/config). - Registered the tool in the MCP tools package init.
- Added unit tests for diagnostics output and redaction helpers.
- Modified
basic_memory/__init__.pyby removing__api_version__.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/basic_memory/mcp/tools/basic_memory_diagnostics.py |
New diagnostics MCP tool + helpers for redacting secrets/URL credentials. |
src/basic_memory/mcp/tools/__init__.py |
Registers the new diagnostics tool for MCP enumeration. |
tests/mcp/test_tool_basic_memory_diagnostics.py |
Adds unit tests for diagnostics output and redaction behavior. |
src/basic_memory/__init__.py |
Removes the package-level __api_version__ constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Package version - updated by release automation | ||
| __version__ = "0.22.0" |
| "## Version", | ||
| f"- basic-memory: {bm_version}", | ||
| "", |
| if parsed.username or parsed.password: | ||
| # Rebuild with credentials replaced by a placeholder. | ||
| netloc = f"***@{parsed.hostname}" | ||
| if parsed.port: | ||
| netloc = f"{netloc}:{parsed.port}" | ||
| redacted = parsed._replace(netloc=netloc) | ||
| return urlunparse(redacted) |
| # --- Configuration --- | ||
| manager = ConfigManager() | ||
| config_file = manager.config_dir / CONFIG_FILE_NAME | ||
| config_exists = config_file.exists() |
| import json | ||
| import platform | ||
| import sys | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import basic_memory | ||
| from basic_memory.mcp.tools.basic_memory_diagnostics import ( | ||
| _redact_config, | ||
| _redact_url, | ||
| basic_memory_diagnostics, | ||
| ) |
Closes #187
Summary
Implements the
basic_memory_diagnosticsMCP tool requested in issue #187, following the converged requirements from the comment thread.What changed:
src/basic_memory/mcp/tools/basic_memory_diagnostics.py— a new read-only MCP tool that surfaces:__version__and__api_version__)cloud_api_keyredactedsrc/basic_memory/mcp/tools/__init__.py(import +__all__entry)tests/mcp/tools/test_basic_memory_diagnostics.pycovering the happy path and the redaction behaviorDesign decisions per issue thread:
basic_memory_diagnostics(renamed from the earlier draft name per reviewer request)How it was tested
uv run pytest tests/mcp/tools/test_basic_memory_diagnostics.pyReview notes
src/basic_memory/mcp/tools/basic_memory_diagnostics.pyrecomputes the config path asmanager.config_dir / CONFIG_FILE_NAME, butConfigManageralready exposesself.config_file(config.py:825) which is the same value. Usingmanager.config_filewould be marginally cleaner, though the current form is correct and arguably clearer about what file it reads.basic_memory_diagnosticsinsrc/basic_memory/mcp/tools/__init__.pyis appended at the end of the import block (after the schema tools) rather than grouped alphabetically with the other tool imports. Purely cosmetic; the__all__list itself is correctly alphabetized.except Exceptionbranch in the tool (malformed config JSON) is marked# pragma: no coverand is not exercised by a test. Low risk since it is defensive, but a malformed-JSON test would close the gap.🤖 Generated with Claude Code