From beec6932c35f2fa007605e7f1f520e172efa03dc Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Thu, 16 Jul 2026 20:09:04 +0000 Subject: [PATCH 1/2] feat(cli): add bm config command group for get/set/list/unset Every user-facing setting previously required hand-editing config.json or knowing the BASIC_MEMORY_* env-var convention. `bm config list|get|set|unset` covers the scalar subset of BasicMemoryConfig (derived from the model, not a hand-maintained list), validates `set` through the model so invalid values fail with the Pydantic error, and marks BASIC_MEMORY_* env overrides. Secret-redaction rules (cloud_api_key never printed, database_url credentials masked) are factored out of the existing basic_memory_diagnostics MCP tool into a shared basic_memory/redaction.py module so both surfaces stay in sync. Closes #991 Co-authored-by: Paul Hernandez <60959+phernandez@users.noreply.github.com> Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> --- AGENTS.md | 6 + README.md | 5 + src/basic_memory/cli/app.py | 1 + src/basic_memory/cli/commands/__init__.py | 2 + src/basic_memory/cli/commands/config.py | 218 ++++++++++++ .../mcp/tools/basic_memory_diagnostics.py | 98 +----- src/basic_memory/redaction.py | 103 ++++++ tests/cli/test_config_command.py | 319 ++++++++++++++++++ 8 files changed, 656 insertions(+), 96 deletions(-) create mode 100644 src/basic_memory/cli/commands/config.py create mode 100644 src/basic_memory/redaction.py create mode 100644 tests/cli/test_config_command.py diff --git a/AGENTS.md b/AGENTS.md index aeabd1871..52a76f97c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -358,6 +358,12 @@ See `.claude/commands/release/release.md` (and `beta.md`, `release-check.md`, `c - Tool access: `basic-memory tool` (provides CLI access to MCP tools) - Continue: `basic-memory tool continue-conversation --topic="search"` +**Config Management:** +- List all settings (effective values, env overrides marked): `basic-memory config list` +- Get one setting: `basic-memory config get cli_output_style` +- Set a setting (validated through the config model): `basic-memory config set cli_output_style plain` +- Revert a setting to its default: `basic-memory config unset cli_output_style` + **Project Management:** - List projects: `basic-memory project list` - Add project: `basic-memory project add "name" ~/path` diff --git a/README.md b/README.md index 8374c13d2..4de05a1d3 100644 --- a/README.md +++ b/README.md @@ -511,6 +511,11 @@ basic-memory project add research ~/research basic-memory project set-cloud research # route through cloud basic-memory project set-local research # revert +# Config +basic-memory config list # all settings, effective values, env overrides +basic-memory config set cli_output_style plain # validated through the config model +basic-memory config unset cli_output_style # revert to default + # Health & maintenance basic-memory status basic-memory doctor # file <-> DB consistency check diff --git a/src/basic_memory/cli/app.py b/src/basic_memory/cli/app.py index ad38cd7ac..dc5565498 100644 --- a/src/basic_memory/cli/app.py +++ b/src/basic_memory/cli/app.py @@ -92,6 +92,7 @@ def _post_command_messages() -> None: "status", "sync", "project", + "config", "tool", "reset", "reindex", diff --git a/src/basic_memory/cli/commands/__init__.py b/src/basic_memory/cli/commands/__init__.py index d0daefaee..0f0225ac1 100644 --- a/src/basic_memory/cli/commands/__init__.py +++ b/src/basic_memory/cli/commands/__init__.py @@ -7,6 +7,7 @@ man, tool, project, + config, format, schema, update, @@ -26,6 +27,7 @@ "import_chatgpt", "tool", "project", + "config", "format", "schema", "update", diff --git a/src/basic_memory/cli/commands/config.py b/src/basic_memory/cli/commands/config.py new file mode 100644 index 000000000..a6d2b3e91 --- /dev/null +++ b/src/basic_memory/cli/commands/config.py @@ -0,0 +1,218 @@ +"""CLI commands for managing Basic Memory's config.json (issue #991). + +Every user-facing config option previously required hand-editing +``config.json`` or knowing the ``BASIC_MEMORY_*`` env-var naming convention. +This module exposes ``bm config list|get|set|unset`` for the scalar subset of +``BasicMemoryConfig`` fields, validating ``set`` through the model itself so +invalid values fail with the same Pydantic error a malformed config.json +would produce. +""" + +import json +import os +import types +import typing +from enum import Enum +from typing import Any + +import typer +from pydantic import ValidationError +from rich.console import Console +from rich.table import Table + +from basic_memory.cli.app import app +from basic_memory.config import BasicMemoryConfig, ConfigManager +from basic_memory.redaction import SECRET_FIELDS, URL_FIELDS, redact_url + +console = Console() + +config_app = typer.Typer(help="Manage Basic Memory's config.json settings") +app.add_typer(config_app, name="config") + +_SCALAR_TYPES = (str, int, float, bool) +_UNION_ORIGINS = (typing.Union, types.UnionType) + + +def _is_scalar_annotation(annotation: Any) -> bool: + """Whether a field's type annotation is a plain scalar (or Optional/Literal/Enum of one). + + Excludes structured fields (dict, list, nested models, datetime) that need + their own dedicated commands (e.g. `projects` -> `bm project add`) or + richer input parsing than a single CLI string argument can offer. + """ + origin = typing.get_origin(annotation) + if origin in _UNION_ORIGINS: + args = [a for a in typing.get_args(annotation) if a is not type(None)] + return len(args) == 1 and _is_scalar_annotation(args[0]) + if origin is typing.Literal: + return True + if origin is not None: + return False + return isinstance(annotation, type) and issubclass(annotation, (*_SCALAR_TYPES, Enum)) + + +def _configurable_fields() -> tuple[str, ...]: + """Scalar BasicMemoryConfig fields settable via `bm config`, derived from the model.""" + return tuple( + sorted( + name + for name, field in BasicMemoryConfig.model_fields.items() + if _is_scalar_annotation(field.annotation) + ) + ) + + +# Derived at import time so the allowlist tracks BasicMemoryConfig automatically +# rather than drifting from a hand-maintained list. +CONFIGURABLE_FIELDS: tuple[str, ...] = _configurable_fields() + + +def _env_var_name(key: str) -> str: + return f"BASIC_MEMORY_{key.upper()}" + + +def _mask(key: str, raw: str) -> str: + """Apply the same secret/URL redaction rules as `basic_memory_diagnostics` (#963).""" + if key in SECRET_FIELDS: + return "********" + if key in URL_FIELDS: + return redact_url(raw) + return raw + + +def _display_value(key: str, value: Any) -> str: + """Render an effective config value for display, masking secrets/URL credentials.""" + if value is None: + return "(not set)" + if key in SECRET_FIELDS: + return "********" + if isinstance(value, Enum): + value = value.value + return _mask(key, str(value)) + + +def _unknown_key_error(key: str) -> None: + console.print(f"[red]Error: '{key}' is not a recognized setting.[/red]") + console.print("[dim]Run 'bm config list' to see all available settings.[/dim]") + + +def _require_known_key(key: str) -> None: + if key not in CONFIGURABLE_FIELDS: + _unknown_key_error(key) + raise typer.Exit(1) + + +@config_app.command("list") +def config_list( + json_output: bool = typer.Option(False, "--json", help="Output in JSON format"), +) -> None: + """List every configurable setting with its effective value and source. + + Source is one of: default (unset), file (set in config.json), or env + (overridden by a BASIC_MEMORY_* environment variable, which always wins). + """ + config_manager = ConfigManager() + config = config_manager.config + + raw_file_data: dict = {} + if config_manager.config_file.exists(): + try: + raw_file_data = json.loads(config_manager.config_file.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): # pragma: no cover - config.json validated on load + raw_file_data = {} + + rows = [] + for key in CONFIGURABLE_FIELDS: + value = getattr(config, key) + env_var = _env_var_name(key) + if env_var in os.environ: + source = f"env ({env_var})" + elif key in raw_file_data: + source = "file" + else: + source = "default" + rows.append({"key": key, "value": _display_value(key, value), "source": source}) + + if json_output: + print(json.dumps(rows, indent=2)) + return + + table = Table(title="Basic Memory Configuration") + table.add_column("Setting", style="cyan") + table.add_column("Value", style="green") + table.add_column("Source", style="yellow") + for row in rows: + table.add_row(row["key"], row["value"], row["source"]) + console.print(table) + + +@config_app.command("get") +def config_get( + key: str = typer.Argument(..., help="Config setting name (see 'bm config list')"), +) -> None: + """Show the effective value of one config setting.""" + _require_known_key(key) + + config = ConfigManager().config + value = getattr(config, key) + console.print(f"{key} = {_display_value(key, value)}") + + env_var = _env_var_name(key) + if env_var in os.environ: + env_value = _mask(key, os.environ[env_var]) + console.print(f"[yellow]Overridden by ${env_var} = {env_value}[/yellow]") + + +@config_app.command("set") +def config_set( + key: str = typer.Argument(..., help="Config setting name (see 'bm config list')"), + value: str = typer.Argument(..., help="New value"), +) -> None: + """Set a config value, validated through BasicMemoryConfig before writing. + + Invalid values (e.g. `cli_output_style` outside rich|plain) fail with the + Pydantic validation error instead of being written to config.json. + """ + _require_known_key(key) + + config_manager = ConfigManager() + config = config_manager.load_config() + + candidate = config.model_dump(mode="json") + candidate[key] = value + + try: + validated = BasicMemoryConfig.model_validate(candidate) + except ValidationError as e: + console.print(f"[red]Error: invalid value for '{key}':[/red]") + console.print(f"[red]{e}[/red]") + raise typer.Exit(1) + + setattr(config, key, getattr(validated, key)) + config_manager.save_config(config) + + console.print(f"[green]{key} = {_display_value(key, getattr(config, key))}[/green]") + + env_var = _env_var_name(key) + if env_var in os.environ: + console.print( + f"[yellow]Note: ${env_var} is set and will override this file value " + "until the environment variable is unset.[/yellow]" + ) + + +@config_app.command("unset") +def config_unset( + key: str = typer.Argument(..., help="Config setting name (see 'bm config list')"), +) -> None: + """Revert a config setting to its default value.""" + _require_known_key(key) + + config_manager = ConfigManager() + config = config_manager.load_config() + + default_value = BasicMemoryConfig.model_fields[key].get_default(call_default_factory=True) + setattr(config, key, default_value) + config_manager.save_config(config) + + console.print(f"[green]{key} reverted to default: {_display_value(key, default_value)}[/green]") diff --git a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py index d97a47e47..697cba444 100644 --- a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py +++ b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py @@ -3,106 +3,12 @@ import json import platform import sys -from urllib.parse import parse_qsl, urlencode, urlparse, urlunparse import basic_memory from basic_memory.config import CONFIG_FILE_NAME, resolve_data_dir from basic_memory.mcp.server import mcp - -# Fields in BasicMemoryConfig that contain secrets and must never be surfaced. -_SECRET_FIELDS = frozenset({"cloud_api_key", "semantic_embedding_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", "semantic_embedding_api_base"}) - -_SECRET_QUERY_KEYS = frozenset( - { - "api_key", - "apikey", - "credential", - "password", - "passwd", - "pwd", - "secret", - "secret_key", - "sslpassword", - "token", - } -) - - -def _query_key_is_secret(key: str) -> bool: - normalized = key.strip().lower().replace("-", "_") - return normalized in _SECRET_QUERY_KEYS or normalized.endswith( - ("_password", "_secret", "_token", "_key") - ) - - -def _redact_query_secrets(query: str) -> str: - """Mask credential-bearing query values while preserving diagnostic options.""" - pairs = parse_qsl(query, keep_blank_values=True) - if not any(_query_key_is_secret(key) for key, _ in pairs): - return query - return urlencode([(key, "***" if _query_key_is_secret(key) else value) for key, value in pairs]) - - -def _redact_url(url: str) -> str: - """Strip userinfo and credential-bearing query values 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 ValueError: - # A malformed authority can still contain credentials. Redact the - # userinfo and query conservatively rather than returning a secret unchanged. - base, query_separator, query = url.partition("?") - safe_url = f"{base}?{_redact_query_secrets(query)}" if query_separator else base - scheme, separator, remainder = safe_url.partition("://") - if separator and "@" in remainder: - _, _, authority = remainder.rpartition("@") - return f"{scheme}://***@{authority}" - return safe_url - - redacted_query = _redact_query_secrets(parsed.query) - if "@" not in parsed.netloc and redacted_query == parsed.query: - # Neither URL userinfo nor known secret query parameters are present. - return url - - redacted_netloc = parsed.netloc - if "@" in parsed.netloc: - # Preserve the authority verbatim after the final @. In particular, using - # parsed.hostname here would discard the brackets required around IPv6 hosts. - _, _, authority = parsed.netloc.rpartition("@") - redacted_netloc = f"***@{authority}" - - return urlunparse(parsed._replace(netloc=redacted_netloc, query=redacted_query)) - - -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 userinfo and credential-bearing query values - stripped so that safe host, database, and connection options remain visible. - - 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 +from basic_memory.redaction import redact_config as _redact_config +from basic_memory.redaction import redact_url as _redact_url # noqa: F401 (re-exported for tests) @mcp.tool( diff --git a/src/basic_memory/redaction.py b/src/basic_memory/redaction.py new file mode 100644 index 000000000..753037491 --- /dev/null +++ b/src/basic_memory/redaction.py @@ -0,0 +1,103 @@ +"""Shared secret-redaction rules for surfacing ``BasicMemoryConfig`` values. + +Used by the ``basic_memory_diagnostics`` MCP tool (#963) and the ``bm config`` +CLI command group (#991) so both surfaces apply identical scrubbing rules +instead of drifting apart over time. +""" + +from urllib.parse import parse_qsl, urlencode, urlparse, urlunparse + +# Fields in BasicMemoryConfig that contain secrets and must never be surfaced. +SECRET_FIELDS = frozenset({"cloud_api_key", "semantic_embedding_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", "semantic_embedding_api_base"}) + +_SECRET_QUERY_KEYS = frozenset( + { + "api_key", + "apikey", + "credential", + "password", + "passwd", + "pwd", + "secret", + "secret_key", + "sslpassword", + "token", + } +) + + +def _query_key_is_secret(key: str) -> bool: + normalized = key.strip().lower().replace("-", "_") + return normalized in _SECRET_QUERY_KEYS or normalized.endswith( + ("_password", "_secret", "_token", "_key") + ) + + +def _redact_query_secrets(query: str) -> str: + """Mask credential-bearing query values while preserving diagnostic options.""" + pairs = parse_qsl(query, keep_blank_values=True) + if not any(_query_key_is_secret(key) for key, _ in pairs): + return query + return urlencode([(key, "***" if _query_key_is_secret(key) else value) for key, value in pairs]) + + +def redact_url(url: str) -> str: + """Strip userinfo and credential-bearing query values 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 ValueError: + # A malformed authority can still contain credentials. Redact the + # userinfo and query conservatively rather than returning a secret unchanged. + base, query_separator, query = url.partition("?") + safe_url = f"{base}?{_redact_query_secrets(query)}" if query_separator else base + scheme, separator, remainder = safe_url.partition("://") + if separator and "@" in remainder: + _, _, authority = remainder.rpartition("@") + return f"{scheme}://***@{authority}" + return safe_url + + redacted_query = _redact_query_secrets(parsed.query) + if "@" not in parsed.netloc and redacted_query == parsed.query: + # Neither URL userinfo nor known secret query parameters are present. + return url + + redacted_netloc = parsed.netloc + if "@" in parsed.netloc: + # Preserve the authority verbatim after the final @. In particular, using + # parsed.hostname here would discard the brackets required around IPv6 hosts. + _, _, authority = parsed.netloc.rpartition("@") + redacted_netloc = f"***@{authority}" + + return urlunparse(parsed._replace(netloc=redacted_netloc, query=redacted_query)) + + +def redact_config(raw: dict) -> dict: + """Return a copy of a raw config dict with secret fields removed. + + - Keys in ``SECRET_FIELDS`` are dropped entirely. + - Keys in ``URL_FIELDS`` have userinfo and credential-bearing query values + stripped so that safe host, database, and connection options remain visible. + + 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 diff --git a/tests/cli/test_config_command.py b/tests/cli/test_config_command.py new file mode 100644 index 000000000..82864c356 --- /dev/null +++ b/tests/cli/test_config_command.py @@ -0,0 +1,319 @@ +"""Tests for the `bm config` command group (issue #991).""" + +import json +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from basic_memory.cli.app import app +from basic_memory.config import BasicMemoryConfig + +# Importing registers the config subcommands on the shared app instance. +import basic_memory.cli.commands.config as config_cmd # noqa: F401 + + +@pytest.fixture +def runner(): + return CliRunner() + + +@pytest.fixture +def write_config(tmp_path, monkeypatch): + """Write config.json under a temporary HOME and return the file path.""" + + def _write(config_data: dict) -> Path: + from basic_memory import config as config_module + + config_module._CONFIG_CACHE = None + config_module._CONFIG_MTIME = None + config_module._CONFIG_SIZE = None + + config_dir = tmp_path / ".basic-memory" + config_dir.mkdir(parents=True, exist_ok=True) + config_file = config_dir / "config.json" + config_file.write_text(json.dumps(config_data, indent=2)) + monkeypatch.setenv("HOME", str(tmp_path)) + return config_file + + return _write + + +def _base_config(**overrides) -> dict: + data = { + "env": "dev", + "projects": {"main": {"path": "/tmp/main", "mode": "local"}}, + "default_project": "main", + } + data.update(overrides) + return data + + +# --------------------------------------------------------------------------- +# CONFIGURABLE_FIELDS derivation +# --------------------------------------------------------------------------- + + +def test_configurable_fields_excludes_structured_types(): + """Structured fields need dedicated commands or richer parsing, so they're excluded.""" + assert "projects" not in config_cmd.CONFIGURABLE_FIELDS + assert "formatters" not in config_cmd.CONFIGURABLE_FIELDS + assert "auto_update_last_checked_at" not in config_cmd.CONFIGURABLE_FIELDS + + +def test_configurable_fields_includes_scalar_settings(): + """Scalar settings (str/bool/int/float/Literal/Enum) are derived from the model.""" + for expected in ("cli_output_style", "log_level", "kebab_filenames", "database_backend"): + assert expected in config_cmd.CONFIGURABLE_FIELDS + + +def test_configurable_fields_matches_model_field_count(): + """Every configurable field name must be a real BasicMemoryConfig field.""" + assert set(config_cmd.CONFIGURABLE_FIELDS) <= set(BasicMemoryConfig.model_fields) + + +# --------------------------------------------------------------------------- +# get / list default behavior +# --------------------------------------------------------------------------- + + +def test_config_get_default_value(runner, write_config): + write_config(_base_config()) + + result = runner.invoke(app, ["config", "get", "cli_output_style"]) + + assert result.exit_code == 0, result.output + assert "cli_output_style = rich" in result.output + + +def test_config_get_unknown_key(runner, write_config): + write_config(_base_config()) + + result = runner.invoke(app, ["config", "get", "not_a_real_setting"]) + + assert result.exit_code == 1 + assert "not a recognized setting" in result.output + + +def test_config_get_renders_enum_value_not_repr(runner, write_config): + """Enum-typed settings (e.g. database_backend) must show their value, not `Class.MEMBER`.""" + write_config(_base_config()) + + result = runner.invoke(app, ["config", "get", "database_backend"]) + + assert result.exit_code == 0, result.output + assert "database_backend = sqlite" in result.output + assert "DatabaseBackend" not in result.output + + +def test_config_list_shows_default_source(runner, write_config): + write_config(_base_config()) + + result = runner.invoke(app, ["config", "list"]) + + assert result.exit_code == 0, result.output + assert "cli_output_style" in result.output + assert "default" in result.output + + +def test_config_list_shows_file_source_for_set_field(runner, write_config): + write_config(_base_config(log_level="DEBUG")) + + result = runner.invoke(app, ["config", "list", "--json"]) + + assert result.exit_code == 0, result.output + rows = {row["key"]: row for row in json.loads(result.output)} + assert rows["log_level"]["value"] == "DEBUG" + assert rows["log_level"]["source"] == "file" + + +def test_config_list_json_output_is_valid_json(runner, write_config): + write_config(_base_config()) + + result = runner.invoke(app, ["config", "list", "--json"]) + + assert result.exit_code == 0, result.output + rows = json.loads(result.output) + assert any(row["key"] == "cli_output_style" for row in rows) + + +# --------------------------------------------------------------------------- +# set: validation through BasicMemoryConfig +# --------------------------------------------------------------------------- + + +def test_config_set_valid_value_round_trips(runner, write_config): + config_file = write_config(_base_config()) + + result = runner.invoke(app, ["config", "set", "cli_output_style", "plain"]) + assert result.exit_code == 0, result.output + assert "cli_output_style = plain" in result.output + + on_disk = json.loads(config_file.read_text()) + assert on_disk["cli_output_style"] == "plain" + + get_result = runner.invoke(app, ["config", "get", "cli_output_style"]) + assert "cli_output_style = plain" in get_result.output + + +def test_config_set_invalid_value_fails_with_pydantic_error(runner, write_config): + config_file = write_config(_base_config()) + + result = runner.invoke(app, ["config", "set", "cli_output_style", "bogus"]) + + assert result.exit_code == 1 + assert "invalid value" in result.output.lower() + assert "cli_output_style" in result.output + + # Config file must be untouched by a failed validation. + on_disk = json.loads(config_file.read_text()) + assert "cli_output_style" not in on_disk + + +def test_config_set_coerces_bool_from_string(runner, write_config): + write_config(_base_config()) + + result = runner.invoke(app, ["config", "set", "format_on_save", "true"]) + + assert result.exit_code == 0, result.output + assert "format_on_save = True" in result.output + + +def test_config_set_rejects_structured_field(runner, write_config): + write_config(_base_config()) + + result = runner.invoke(app, ["config", "set", "projects", '{"x": "/tmp/x"}']) + + assert result.exit_code == 1 + assert "not a recognized setting" in result.output + + +def test_config_set_unknown_key(runner, write_config): + write_config(_base_config()) + + result = runner.invoke(app, ["config", "set", "not_a_real_setting", "value"]) + + assert result.exit_code == 1 + assert "not a recognized setting" in result.output + + +# --------------------------------------------------------------------------- +# unset: revert to default +# --------------------------------------------------------------------------- + + +def test_config_unset_reverts_to_default(runner, write_config): + write_config(_base_config(cli_output_style="plain")) + + result = runner.invoke(app, ["config", "unset", "cli_output_style"]) + + assert result.exit_code == 0, result.output + assert "reverted to default: rich" in result.output + + get_result = runner.invoke(app, ["config", "get", "cli_output_style"]) + assert "cli_output_style = rich" in get_result.output + + +def test_config_unset_unknown_key(runner, write_config): + write_config(_base_config()) + + result = runner.invoke(app, ["config", "unset", "not_a_real_setting"]) + + assert result.exit_code == 1 + assert "not a recognized setting" in result.output + + +# --------------------------------------------------------------------------- +# Redaction: cloud_api_key must never print, database_url credentials masked +# --------------------------------------------------------------------------- + + +def test_config_get_never_prints_cloud_api_key(runner, write_config): + write_config(_base_config(cloud_api_key="bmc_super_secret_token")) + + result = runner.invoke(app, ["config", "get", "cloud_api_key"]) + + assert result.exit_code == 0, result.output + assert "bmc_super_secret_token" not in result.output + assert "cloud_api_key = ********" in result.output + + +def test_config_list_never_prints_cloud_api_key(runner, write_config): + write_config(_base_config(cloud_api_key="bmc_super_secret_token")) + + result = runner.invoke(app, ["config", "list", "--json"]) + + assert result.exit_code == 0, result.output + assert "bmc_super_secret_token" not in result.output + rows = {row["key"]: row for row in json.loads(result.output)} + assert rows["cloud_api_key"]["value"] == "********" + + +def test_config_get_masks_database_url_credentials(runner, write_config): + write_config(_base_config(database_url="postgresql://dbuser:dbpass@host.example.com:5432/bm")) + + result = runner.invoke(app, ["config", "get", "database_url"]) + + assert result.exit_code == 0, result.output + assert "dbpass" not in result.output + assert "dbuser" not in result.output + assert "host.example.com" in result.output + + +def test_config_get_shows_not_set_for_unset_secret(runner, write_config): + write_config(_base_config()) + + result = runner.invoke(app, ["config", "get", "cloud_api_key"]) + + assert result.exit_code == 0, result.output + assert "cloud_api_key = (not set)" in result.output + + +# --------------------------------------------------------------------------- +# Env var overrides +# --------------------------------------------------------------------------- + + +def test_config_get_shows_env_override(runner, write_config, monkeypatch): + write_config(_base_config()) + monkeypatch.setenv("BASIC_MEMORY_CLI_OUTPUT_STYLE", "plain") + + result = runner.invoke(app, ["config", "get", "cli_output_style"]) + + assert result.exit_code == 0, result.output + assert "cli_output_style = plain" in result.output + assert "BASIC_MEMORY_CLI_OUTPUT_STYLE" in result.output + + +def test_config_get_masks_secret_env_override(runner, write_config, monkeypatch): + write_config(_base_config()) + monkeypatch.setenv("BASIC_MEMORY_CLOUD_API_KEY", "bmc_env_secret_token") + + result = runner.invoke(app, ["config", "get", "cloud_api_key"]) + + assert result.exit_code == 0, result.output + assert "bmc_env_secret_token" not in result.output + assert "********" in result.output + + +def test_config_list_shows_env_source(runner, write_config, monkeypatch): + write_config(_base_config()) + monkeypatch.setenv("BASIC_MEMORY_CLI_OUTPUT_STYLE", "plain") + + result = runner.invoke(app, ["config", "list", "--json"]) + + assert result.exit_code == 0, result.output + rows = {row["key"]: row for row in json.loads(result.output)} + assert rows["cli_output_style"]["value"] == "plain" + assert "env" in rows["cli_output_style"]["source"] + assert "BASIC_MEMORY_CLI_OUTPUT_STYLE" in rows["cli_output_style"]["source"] + + +def test_config_set_warns_when_env_var_overrides(runner, write_config, monkeypatch): + write_config(_base_config()) + monkeypatch.setenv("BASIC_MEMORY_CLI_OUTPUT_STYLE", "plain") + + result = runner.invoke(app, ["config", "set", "cli_output_style", "rich"]) + + assert result.exit_code == 0, result.output + assert "BASIC_MEMORY_CLI_OUTPUT_STYLE is set" in result.output From 11e7ae2486b93639f03c41c3c10e85cca673de81 Mon Sep 17 00:00:00 2001 From: phernandez Date: Thu, 16 Jul 2026 18:49:14 -0500 Subject: [PATCH 2/2] refactor(cli): tighten bm config to house style Self-review follow-up on the generated code. config.py worked (24 tests green) but did not read like the rest of the codebase: - Introduce a frozen/slots ConfigSetting dataclass for the list rows instead of anonymous dict literals threaded through list/get output (docs/ENGINEERING_STYLE asks for dataclasses over ad-hoc dict[str, Any] shapes). JSON output is unchanged (asdict -> key/value/source). - Fully type the helpers and collections (list[ConfigSetting], frozenset[str]); redaction.py signatures are dict[str, Any] rather than bare dict. - Drop the silent 'except (OSError, json.JSONDecodeError)' fallback when reading config.json for source labelling. ConfigManager already parsed the file to load the config, so a read failure is a real inconsistency that should surface, not be swallowed into mislabelling every setting as a default (fail-fast rule). - De-duplicate secret masking: _render_value delegates to _redact_for_display instead of re-checking SECRET_FIELDS; name the display sentinels (SECRET_MASK / NOT_SET). - Collapse the two-step _unknown_key_error/_require_known_key into one, add section headers. Behavior and the 24 existing tests are unchanged; the model-derived allowlist is kept (it is the correct way to track BasicMemoryConfig). Signed-off-by: phernandez --- src/basic_memory/cli/commands/config.py | 139 +++++++++++++++--------- src/basic_memory/redaction.py | 5 +- 2 files changed, 89 insertions(+), 55 deletions(-) diff --git a/src/basic_memory/cli/commands/config.py b/src/basic_memory/cli/commands/config.py index a6d2b3e91..4edfbb623 100644 --- a/src/basic_memory/cli/commands/config.py +++ b/src/basic_memory/cli/commands/config.py @@ -1,17 +1,17 @@ """CLI commands for managing Basic Memory's config.json (issue #991). -Every user-facing config option previously required hand-editing -``config.json`` or knowing the ``BASIC_MEMORY_*`` env-var naming convention. -This module exposes ``bm config list|get|set|unset`` for the scalar subset of -``BasicMemoryConfig`` fields, validating ``set`` through the model itself so -invalid values fail with the same Pydantic error a malformed config.json -would produce. +Every user-facing config option previously required hand-editing ``config.json`` +or knowing the ``BASIC_MEMORY_*`` env-var naming convention. This module exposes +``bm config list|get|set|unset`` for the scalar subset of ``BasicMemoryConfig`` +fields, validating ``set`` through the model itself so invalid values fail with +the same Pydantic error a malformed config.json would produce. """ import json import os import types import typing +from dataclasses import asdict, dataclass from enum import Enum from typing import Any @@ -29,16 +29,24 @@ config_app = typer.Typer(help="Manage Basic Memory's config.json settings") app.add_typer(config_app, name="config") +# Display sentinels — kept as named constants so the CLI and its tests agree on the +# exact strings users see for masked secrets and unset values. +SECRET_MASK = "********" +NOT_SET = "(not set)" + _SCALAR_TYPES = (str, int, float, bool) _UNION_ORIGINS = (typing.Union, types.UnionType) +# --- Configurable-field discovery --- + + def _is_scalar_annotation(annotation: Any) -> bool: """Whether a field's type annotation is a plain scalar (or Optional/Literal/Enum of one). Excludes structured fields (dict, list, nested models, datetime) that need - their own dedicated commands (e.g. `projects` -> `bm project add`) or - richer input parsing than a single CLI string argument can offer. + their own dedicated commands (e.g. `projects` -> `bm project add`) or richer + input parsing than a single CLI string argument can offer. """ origin = typing.get_origin(annotation) if origin in _UNION_ORIGINS: @@ -67,82 +75,107 @@ def _configurable_fields() -> tuple[str, ...]: CONFIGURABLE_FIELDS: tuple[str, ...] = _configurable_fields() +# --- Value resolution and display --- + + +@dataclass(frozen=True, slots=True) +class ConfigSetting: + """One configurable setting resolved for display. + + ``value`` is already redacted and stringified for output; ``source`` is one of + ``default`` (unset), ``file`` (present in config.json), or ``env (VAR)`` (an + environment variable is overriding the file value, which always wins). + """ + + key: str + value: str + source: str + + def _env_var_name(key: str) -> str: return f"BASIC_MEMORY_{key.upper()}" -def _mask(key: str, raw: str) -> str: - """Apply the same secret/URL redaction rules as `basic_memory_diagnostics` (#963).""" +def _redact_for_display(key: str, raw: str) -> str: + """Mask secrets and URL credentials the same way `basic_memory_diagnostics` does (#963).""" if key in SECRET_FIELDS: - return "********" + return SECRET_MASK if key in URL_FIELDS: return redact_url(raw) return raw -def _display_value(key: str, value: Any) -> str: +def _render_value(key: str, value: Any) -> str: """Render an effective config value for display, masking secrets/URL credentials.""" if value is None: - return "(not set)" - if key in SECRET_FIELDS: - return "********" + return NOT_SET if isinstance(value, Enum): value = value.value - return _mask(key, str(value)) - + return _redact_for_display(key, str(value)) -def _unknown_key_error(key: str) -> None: - console.print(f"[red]Error: '{key}' is not a recognized setting.[/red]") - console.print("[dim]Run 'bm config list' to see all available settings.[/dim]") +def _file_keys(config_manager: ConfigManager) -> frozenset[str]: + """Top-level keys actually present in config.json, to tell `file` from `default`. -def _require_known_key(key: str) -> None: - if key not in CONFIGURABLE_FIELDS: - _unknown_key_error(key) - raise typer.Exit(1) - + config.json was already parsed by ConfigManager to build the loaded config, so a + read here does not swallow errors: if the file exists it is valid JSON, and a + genuine read failure should surface rather than silently mislabel every setting. + """ + config_file = config_manager.config_file + if not config_file.exists(): + return frozenset() + return frozenset(json.loads(config_file.read_text(encoding="utf-8"))) -@config_app.command("list") -def config_list( - json_output: bool = typer.Option(False, "--json", help="Output in JSON format"), -) -> None: - """List every configurable setting with its effective value and source. - Source is one of: default (unset), file (set in config.json), or env - (overridden by a BASIC_MEMORY_* environment variable, which always wins). - """ +def _resolve_settings() -> list[ConfigSetting]: + """Resolve every configurable setting to its effective value and source.""" config_manager = ConfigManager() config = config_manager.config + file_keys = _file_keys(config_manager) - raw_file_data: dict = {} - if config_manager.config_file.exists(): - try: - raw_file_data = json.loads(config_manager.config_file.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError): # pragma: no cover - config.json validated on load - raw_file_data = {} - - rows = [] + settings: list[ConfigSetting] = [] for key in CONFIGURABLE_FIELDS: - value = getattr(config, key) env_var = _env_var_name(key) if env_var in os.environ: source = f"env ({env_var})" - elif key in raw_file_data: + elif key in file_keys: source = "file" else: source = "default" - rows.append({"key": key, "value": _display_value(key, value), "source": source}) + settings.append( + ConfigSetting(key=key, value=_render_value(key, getattr(config, key)), source=source) + ) + return settings + + +def _require_known_key(key: str) -> None: + """Exit with guidance unless `key` is a configurable scalar setting.""" + if key not in CONFIGURABLE_FIELDS: + console.print(f"[red]Error: '{key}' is not a recognized setting.[/red]") + console.print("[dim]Run 'bm config list' to see all available settings.[/dim]") + raise typer.Exit(1) + + +# --- Commands --- + + +@config_app.command("list") +def config_list( + json_output: bool = typer.Option(False, "--json", help="Output in JSON format"), +) -> None: + """List every configurable setting with its effective value and source.""" + settings = _resolve_settings() if json_output: - print(json.dumps(rows, indent=2)) + print(json.dumps([asdict(setting) for setting in settings], indent=2)) return table = Table(title="Basic Memory Configuration") table.add_column("Setting", style="cyan") table.add_column("Value", style="green") table.add_column("Source", style="yellow") - for row in rows: - table.add_row(row["key"], row["value"], row["source"]) + for setting in settings: + table.add_row(setting.key, setting.value, setting.source) console.print(table) @@ -154,12 +187,11 @@ def config_get( _require_known_key(key) config = ConfigManager().config - value = getattr(config, key) - console.print(f"{key} = {_display_value(key, value)}") + console.print(f"{key} = {_render_value(key, getattr(config, key))}") env_var = _env_var_name(key) if env_var in os.environ: - env_value = _mask(key, os.environ[env_var]) + env_value = _redact_for_display(key, os.environ[env_var]) console.print(f"[yellow]Overridden by ${env_var} = {env_value}[/yellow]") @@ -178,9 +210,10 @@ def config_set( config_manager = ConfigManager() config = config_manager.load_config() + # Validate the whole config with the candidate applied, so `value` is coerced and + # constrained by the same rules that guard a hand-edited config.json. candidate = config.model_dump(mode="json") candidate[key] = value - try: validated = BasicMemoryConfig.model_validate(candidate) except ValidationError as e: @@ -191,7 +224,7 @@ def config_set( setattr(config, key, getattr(validated, key)) config_manager.save_config(config) - console.print(f"[green]{key} = {_display_value(key, getattr(config, key))}[/green]") + console.print(f"[green]{key} = {_render_value(key, getattr(config, key))}[/green]") env_var = _env_var_name(key) if env_var in os.environ: @@ -215,4 +248,4 @@ def config_unset( setattr(config, key, default_value) config_manager.save_config(config) - console.print(f"[green]{key} reverted to default: {_display_value(key, default_value)}[/green]") + console.print(f"[green]{key} reverted to default: {_render_value(key, default_value)}[/green]") diff --git a/src/basic_memory/redaction.py b/src/basic_memory/redaction.py index 753037491..2b4e649a6 100644 --- a/src/basic_memory/redaction.py +++ b/src/basic_memory/redaction.py @@ -5,6 +5,7 @@ instead of drifting apart over time. """ +from typing import Any from urllib.parse import parse_qsl, urlencode, urlparse, urlunparse # Fields in BasicMemoryConfig that contain secrets and must never be surfaced. @@ -80,7 +81,7 @@ def redact_url(url: str) -> str: return urlunparse(parsed._replace(netloc=redacted_netloc, query=redacted_query)) -def redact_config(raw: dict) -> dict: +def redact_config(raw: dict[str, Any]) -> dict[str, Any]: """Return a copy of a raw config dict with secret fields removed. - Keys in ``SECRET_FIELDS`` are dropped entirely. @@ -91,7 +92,7 @@ def redact_config(raw: dict) -> dict: not currently credential-bearing, but the two sets make the pattern easy to extend. """ - result: dict = {} + result: dict[str, Any] = {} for k, v in raw.items(): if k in SECRET_FIELDS: # Drop entirely — value has no diagnostic value.