-
Notifications
You must be signed in to change notification settings - Fork 227
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.
Open
Changes from 1 commit
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
Some comments aren't visible on the classic Files Changed page.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| """Diagnostic tool for Basic Memory version and system information.""" | ||
|
|
||
| import json | ||
| import platform | ||
| import sys | ||
|
|
||
| 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"}) | ||
|
|
||
|
|
||
| def _redact_config(raw: dict) -> dict: | ||
| """Return a copy of the raw config dict with secret fields removed. | ||
|
|
||
| Only top-level keys are redacted. Nested secret-looking keys within | ||
| project entries are not currently present, but the pattern is explicit | ||
| so it is easy to extend. | ||
| """ | ||
| return {k: v for k, v in raw.items() if k not in _SECRET_FIELDS} | ||
|
|
||
|
|
||
| @mcp.tool( | ||
| "basic_memory_diagnostics", | ||
| annotations={"readOnlyHint": True, "openWorldHint": False}, | ||
| ) | ||
| def basic_memory_diagnostics() -> str: | ||
| """Return version, system, and configuration diagnostics for Basic Memory. | ||
|
|
||
| Provides: | ||
| - Basic Memory package version and API 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__ | ||
| api_version = basic_memory.__api_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() | ||
|
Comment on lines
+98
to
+101
|
||
|
|
||
| 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}", | ||
| f"- API version: {api_version}", | ||
| "", | ||
|
Comment on lines
+116
to
+118
|
||
| "## 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,146 @@ | ||
| """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, | ||
| basic_memory_diagnostics, | ||
| ) | ||
|
Comment on lines
+3
to
+13
|
||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # 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 | ||
| assert basic_memory.__api_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 | ||
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.