-
Notifications
You must be signed in to change notification settings - Fork 214
feat(mcp): add basic_memory_diagnostics tool for version and system info #963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
groksrc
wants to merge
4
commits into
main
Choose a base branch
from
feat/187-diagnostics-tool
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+383
−3
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
29be029
feat(mcp): add basic_memory_diagnostics tool for version and system info
groksrc cbc2d5f
fix(mcp): redact database_url credentials in diagnostics output
groksrc 8f444f9
test(mcp): use exact-equality assertions for redacted URLs
groksrc 4d0566c
fix(mcp): drop stale API version from diagnostics, suppress duplicate…
groksrc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| """Diagnostic tool for Basic Memory version and system information.""" | ||
|
|
||
| import json | ||
| import platform | ||
| import sys | ||
| from urllib.parse import urlparse, urlunparse | ||
|
|
||
| import basic_memory | ||
| from basic_memory.config import CONFIG_FILE_NAME, ConfigManager | ||
| from basic_memory.mcp.server import mcp | ||
|
|
||
| # Fields in BasicMemoryConfig that contain secrets and must never be surfaced. | ||
| _SECRET_FIELDS = frozenset({"cloud_api_key"}) | ||
|
|
||
| # Fields whose values are URLs that may embed user:password credentials. | ||
| # The userinfo component is stripped before surfacing. | ||
| _URL_FIELDS = frozenset({"database_url"}) | ||
|
|
||
|
|
||
| def _redact_url(url: str) -> str: | ||
| """Strip the userinfo (user:password) from a URL string. | ||
|
|
||
| Replaces any credentials with *** so the host/path remain visible for | ||
| diagnostics (e.g. ``postgresql://***@localhost/mydb``). If the value | ||
| cannot be parsed as a URL it is returned unchanged. | ||
| """ | ||
| try: | ||
| parsed = urlparse(url) | ||
| except Exception: # pragma: no cover — urlparse is very permissive | ||
| return url | ||
|
|
||
| if not parsed.hostname: | ||
| # Not a meaningful URL (e.g. a bare file path); leave it alone. | ||
| return url | ||
|
|
||
| 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) | ||
|
|
||
| return url | ||
|
|
||
|
|
||
| def _redact_config(raw: dict) -> dict: | ||
| """Return a copy of the raw config dict with secret fields removed. | ||
|
|
||
| - Keys in ``_SECRET_FIELDS`` are dropped entirely. | ||
| - Keys in ``_URL_FIELDS`` have their userinfo component stripped so that | ||
| host and database name remain visible for diagnostics. | ||
|
|
||
| Only top-level keys are processed. Nested keys within project entries are | ||
| not currently credential-bearing, but the two sets make the pattern easy | ||
| to extend. | ||
| """ | ||
| result: dict = {} | ||
| for k, v in raw.items(): | ||
| if k in _SECRET_FIELDS: | ||
| # Drop entirely — value has no diagnostic value. | ||
| continue | ||
| if k in _URL_FIELDS and isinstance(v, str): | ||
| result[k] = _redact_url(v) | ||
| else: | ||
| result[k] = v | ||
| return result | ||
|
|
||
|
|
||
| @mcp.tool( | ||
| "basic_memory_diagnostics", | ||
| title="Basic Memory Diagnostics", | ||
| tags={"diagnostics"}, | ||
| annotations={"readOnlyHint": True, "openWorldHint": False}, | ||
| # The report is a markdown string; suppress FastMCP's wrap_result so the | ||
| # payload isn't duplicated into structuredContent. | ||
| output_schema=None, | ||
| ) | ||
| def basic_memory_diagnostics() -> str: | ||
| """Return version, system, and configuration diagnostics for Basic Memory. | ||
|
|
||
| Provides: | ||
| - Basic Memory package version | ||
| - Python version and platform details | ||
| - Config file path and its contents (secrets redacted) | ||
|
|
||
| Useful for troubleshooting installations and gathering information for | ||
| support requests. Read-only; never emits secrets or API keys. | ||
| """ | ||
| # --- Version information --- | ||
| bm_version = basic_memory.__version__ | ||
|
|
||
| # --- System information --- | ||
| python_version = sys.version | ||
| platform_info = platform.platform() | ||
| machine = platform.machine() | ||
|
|
||
| # --- Configuration --- | ||
| manager = ConfigManager() | ||
| config_file = manager.config_dir / CONFIG_FILE_NAME | ||
| config_exists = config_file.exists() | ||
|
|
||
| if config_exists: | ||
| try: | ||
| raw_config = json.loads(config_file.read_text(encoding="utf-8")) | ||
| safe_config = _redact_config(raw_config) | ||
| config_dump = json.dumps(safe_config, indent=2, default=str) | ||
| except Exception as exc: # pragma: no cover | ||
| config_dump = f"<error reading config: {exc}>" | ||
| else: | ||
| config_dump = "<config file not found>" | ||
|
|
||
| lines = [ | ||
| "# Basic Memory Diagnostics", | ||
| "", | ||
| "## Version", | ||
| f"- basic-memory: {bm_version}", | ||
| "", | ||
| "## System", | ||
| f"- Python: {python_version}", | ||
| f"- Platform: {platform_info}", | ||
| f"- Architecture: {machine}", | ||
| "", | ||
| "## Configuration", | ||
| f"- Config path: {config_file}", | ||
| f"- Config exists: {config_exists}", | ||
| "", | ||
| "```json", | ||
| config_dump, | ||
| "```", | ||
| ] | ||
| return "\n".join(lines) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,247 @@ | ||
| """Tests for the basic_memory_diagnostics MCP tool.""" | ||
|
|
||
| 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, | ||
| ) | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Unit tests for _redact_config helper | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_redact_config_removes_cloud_api_key(): | ||
| raw = {"cloud_api_key": "bmc_secret", "default_project": "main", "projects": {}} | ||
| result = _redact_config(raw) | ||
| assert "cloud_api_key" not in result | ||
| assert result["default_project"] == "main" | ||
| assert "projects" in result | ||
|
|
||
|
|
||
| def test_redact_config_passes_through_safe_fields(): | ||
| raw = {"default_project": "main", "log_level": "INFO", "env": "dev"} | ||
| result = _redact_config(raw) | ||
| assert result == raw | ||
|
|
||
|
|
||
| def test_redact_config_empty_dict(): | ||
| assert _redact_config({}) == {} | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Tests for the basic_memory_diagnostics tool | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_diagnostics_returns_string(): | ||
| result = basic_memory_diagnostics() | ||
| assert isinstance(result, str) | ||
|
|
||
|
|
||
| def test_diagnostics_includes_version(): | ||
| result = basic_memory_diagnostics() | ||
| assert basic_memory.__version__ in result | ||
|
|
||
|
|
||
| def test_diagnostics_includes_python_version(): | ||
| result = basic_memory_diagnostics() | ||
| # sys.version can be multi-line; just check the version tuple prefix | ||
| major_minor = f"{sys.version_info.major}.{sys.version_info.minor}" | ||
| assert major_minor in result | ||
|
|
||
|
|
||
| def test_diagnostics_includes_platform(): | ||
| result = basic_memory_diagnostics() | ||
| assert platform.machine() in result | ||
|
|
||
|
|
||
| def test_diagnostics_includes_config_path(tmp_path): | ||
| """Config path section should appear in output.""" | ||
| with patch("basic_memory.mcp.tools.basic_memory_diagnostics.ConfigManager") as MockMgr: | ||
| mock_mgr = MagicMock() | ||
| mock_mgr.config_dir = tmp_path | ||
| MockMgr.return_value = mock_mgr | ||
|
|
||
| config_file = tmp_path / "config.json" | ||
| config_file.write_text(json.dumps({"default_project": "main", "projects": {}})) | ||
|
|
||
| result = basic_memory_diagnostics() | ||
|
|
||
| assert str(tmp_path) in result | ||
| assert "Config path:" in result | ||
|
|
||
|
|
||
| def test_diagnostics_config_exists_with_valid_json(tmp_path): | ||
| """When config file exists, its safe contents should appear as JSON.""" | ||
| config_data = { | ||
| "default_project": "research", | ||
| "projects": {"research": {"path": str(tmp_path / "research")}}, | ||
| } | ||
| with patch("basic_memory.mcp.tools.basic_memory_diagnostics.ConfigManager") as MockMgr: | ||
| mock_mgr = MagicMock() | ||
| mock_mgr.config_dir = tmp_path | ||
| MockMgr.return_value = mock_mgr | ||
|
|
||
| config_file = tmp_path / "config.json" | ||
| config_file.write_text(json.dumps(config_data)) | ||
|
|
||
| result = basic_memory_diagnostics() | ||
|
|
||
| assert "research" in result | ||
| assert "```json" in result | ||
|
|
||
|
|
||
| def test_diagnostics_redacts_cloud_api_key(tmp_path): | ||
| """cloud_api_key must never appear in diagnostic output.""" | ||
| config_data = { | ||
| "default_project": "main", | ||
| "cloud_api_key": "bmc_super_secret_token", | ||
| "projects": {}, | ||
| } | ||
| with patch("basic_memory.mcp.tools.basic_memory_diagnostics.ConfigManager") as MockMgr: | ||
| mock_mgr = MagicMock() | ||
| mock_mgr.config_dir = tmp_path | ||
| MockMgr.return_value = mock_mgr | ||
|
|
||
| config_file = tmp_path / "config.json" | ||
| config_file.write_text(json.dumps(config_data)) | ||
|
|
||
| result = basic_memory_diagnostics() | ||
|
|
||
| assert "bmc_super_secret_token" not in result | ||
| assert "cloud_api_key" not in result | ||
|
|
||
|
|
||
| def test_diagnostics_config_missing(tmp_path): | ||
| """When config file does not exist, output should say so.""" | ||
| with patch("basic_memory.mcp.tools.basic_memory_diagnostics.ConfigManager") as MockMgr: | ||
| mock_mgr = MagicMock() | ||
| mock_mgr.config_dir = tmp_path | ||
| MockMgr.return_value = mock_mgr | ||
|
|
||
| # Ensure no config.json is present | ||
| config_file = tmp_path / "config.json" | ||
| assert not config_file.exists() | ||
|
|
||
| result = basic_memory_diagnostics() | ||
|
|
||
| assert "Config exists: False" in result | ||
| assert "<config file not found>" in result | ||
|
|
||
|
|
||
| def test_diagnostics_output_sections(): | ||
| """All expected section headers should be present.""" | ||
| result = basic_memory_diagnostics() | ||
| assert "# Basic Memory Diagnostics" in result | ||
| assert "## Version" in result | ||
| assert "## System" in result | ||
| assert "## Configuration" in result | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Unit tests for _redact_url helper | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_redact_url_strips_password(): | ||
| url = "postgresql://user:secret@localhost/mydb" | ||
| result = _redact_url(url) | ||
| assert "secret" not in result | ||
| assert "user" not in result | ||
| assert "localhost" in result | ||
| assert "mydb" in result | ||
| assert "***" in result | ||
|
|
||
|
|
||
| def test_redact_url_strips_only_password_when_no_username(): | ||
| # password-only userinfo (unusual but valid per RFC) | ||
| url = "postgresql://:secret@db.example.com/app" | ||
| assert _redact_url(url) == "postgresql://***@db.example.com/app" | ||
|
|
||
|
|
||
| def test_redact_url_preserves_port(): | ||
| url = "postgresql://admin:pw@db.internal:5432/prod" | ||
| assert _redact_url(url) == "postgresql://***@db.internal:5432/prod" | ||
|
|
||
|
|
||
| def test_redact_url_no_credentials_unchanged(): | ||
| url = "postgresql://db.internal:5432/prod" | ||
| assert _redact_url(url) == url | ||
|
|
||
|
|
||
| def test_redact_url_non_url_string_unchanged(): | ||
| # Bare file paths / non-URL values must not be mangled. | ||
| path = "/home/user/.local/share/basic-memory/main.db" | ||
| assert _redact_url(path) == path | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # _redact_config tests for database_url | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_redact_config_scrubs_database_url_credentials(): | ||
| raw = { | ||
| "default_project": "main", | ||
| "database_url": "postgresql://dbuser:dbpass@host.example.com:5432/bm", | ||
| "projects": {}, | ||
| } | ||
| result = _redact_config(raw) | ||
| # Exact match: credentials replaced, host/port/db preserved for diagnostics. | ||
| assert result["database_url"] == "postgresql://***@host.example.com:5432/bm" | ||
|
|
||
|
|
||
| def test_redact_config_leaves_database_url_without_credentials(): | ||
| raw = {"database_url": "sqlite:////tmp/basic-memory/main.db"} | ||
| result = _redact_config(raw) | ||
| assert result["database_url"] == "sqlite:////tmp/basic-memory/main.db" | ||
|
|
||
|
|
||
| def test_redact_config_drops_secret_fields_independently(): | ||
| raw = { | ||
| "cloud_api_key": "bmc_top_secret", | ||
| "database_url": "postgresql://dbuser:dbpassword@host/db", | ||
| "default_project": "main", | ||
| } | ||
| result = _redact_config(raw) | ||
| assert "cloud_api_key" not in result | ||
| assert "dbpassword" not in result["database_url"] | ||
| assert "dbuser" not in result["database_url"] | ||
| assert "main" == result["default_project"] | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Integration: database_url redaction surfaces in diagnostic output | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_diagnostics_redacts_database_url_password(tmp_path): | ||
| """Postgres password in database_url must not appear in diagnostic output.""" | ||
| config_data = { | ||
| "default_project": "main", | ||
| "database_url": "postgresql://pguser:supersecret@db.internal:5432/basicmemory", | ||
| "projects": {}, | ||
| } | ||
| with patch("basic_memory.mcp.tools.basic_memory_diagnostics.ConfigManager") as MockMgr: | ||
| mock_mgr = MagicMock() | ||
| mock_mgr.config_dir = tmp_path | ||
| MockMgr.return_value = mock_mgr | ||
|
|
||
| config_file = tmp_path / "config.json" | ||
| config_file.write_text(json.dumps(config_data)) | ||
|
|
||
| result = basic_memory_diagnostics() | ||
|
|
||
| assert "supersecret" not in result | ||
| assert "pguser" not in result | ||
| # Host and port remain visible for diagnostics. | ||
| assert "db.internal" in result | ||
| assert "5432" in result |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
config.jsoncontainsdatabase_urlfor a Postgres deployment,BasicMemoryConfigallows URLs withuser:pass@host, but this tool only removescloud_api_keyand then dumps the remaining config. In that scenario, callingbasic_memory_diagnosticsexposes the database password to any MCP client or copied support transcript; redactdatabase_urlor at least strip URI userinfo before serializing the config.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — this was a real gap. Fixed in cbc2d5f.
What changed:
_redact_url()helper that strips theuser:passworduserinfo component from a URL usingurlparse/urlunparse, leaving host, port, and database name intact for diagnostic value._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_urlunit tests (password stripped, port/host preserved, no-credential URL unchanged, non-URL path unchanged),_redact_configtests fordatabase_url, and an end-to-endtest_diagnostics_redacts_database_url_passwordthat confirms a Postgres password never surfaces in the full diagnostic output.