Skip to content

Commit 40e1b3d

Browse files
committed
fix(cli): address auto-update follow-ups and include formatting cleanups
Signed-off-by: phernandez <paul@basicmachines.co>
1 parent 6aefd30 commit 40e1b3d

6 files changed

Lines changed: 128 additions & 42 deletions

File tree

src/basic_memory/cli/auto_update.py

Lines changed: 11 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ def detect_install_source(executable: str | None = None) -> InstallSource:
7070
return InstallSource.HOMEBREW
7171
if "uv/tools/basic-memory" in normalized:
7272
return InstallSource.UV_TOOL
73-
if "/uv/archive-" in normalized or "/uv/archive-v" in normalized:
73+
if "/uv/archive-" in normalized:
7474
return InstallSource.UVX
7575
return InstallSource.UNKNOWN
7676

@@ -94,39 +94,18 @@ def _run_subprocess(
9494
capture_output: bool,
9595
) -> subprocess.CompletedProcess[str]:
9696
"""Run a subprocess with explicit stdio behavior for protocol safety."""
97-
# Trigger: caller needs command output for decision-making (e.g., brew outdated).
98-
# Why: update-availability checks parse command output.
99-
# Outcome: stdout/stderr are captured in-memory (never forwarded to terminal).
100-
if capture_output:
101-
return subprocess.run(
102-
command,
103-
stdin=subprocess.DEVNULL,
104-
stdout=subprocess.PIPE,
105-
stderr=subprocess.PIPE,
106-
text=True,
107-
timeout=timeout_seconds,
108-
check=False,
109-
)
110-
111-
if silent:
112-
# Trigger: silent operation (MCP/background) with no need for subprocess output.
113-
# Why: prevent protocol/terminal pollution from child process output.
114-
# Outcome: stdout/stderr are discarded.
115-
return subprocess.run(
116-
command,
117-
stdin=subprocess.DEVNULL,
118-
stdout=subprocess.DEVNULL,
119-
stderr=subprocess.DEVNULL,
120-
text=True,
121-
timeout=timeout_seconds,
122-
check=False,
123-
)
97+
# Trigger: silent operation (MCP/background) with no need for subprocess output.
98+
# Why: prevent protocol/terminal pollution from child process output.
99+
# Outcome: stdout/stderr are discarded unless explicit capture is requested.
100+
use_devnull = silent and not capture_output
101+
stdout_target = subprocess.DEVNULL if use_devnull else subprocess.PIPE
102+
stderr_target = subprocess.DEVNULL if use_devnull else subprocess.PIPE
124103

125104
return subprocess.run(
126105
command,
127106
stdin=subprocess.DEVNULL,
128-
stdout=subprocess.PIPE,
129-
stderr=subprocess.PIPE,
107+
stdout=stdout_target,
108+
stderr=stderr_target,
130109
text=True,
131110
timeout=timeout_seconds,
132111
check=False,
@@ -240,7 +219,7 @@ def run_auto_update(
240219
# Trigger: mixed naive/aware datetimes from manual config edits.
241220
# Why: datetime subtraction fails for mixed tz-awareness.
242221
# Outcome: ignore the gate once and continue with a forced check path.
243-
logger.warning("Auto-update interval gate skipped due incompatible timestamp format")
222+
logger.warning("Auto-update interval gate skipped due to incompatible timestamp format")
244223
else:
245224
if elapsed < timedelta(seconds=config.update_check_interval):
246225
return AutoUpdateResult(
@@ -349,6 +328,7 @@ def run_auto_update(
349328
except (
350329
RuntimeError,
351330
urllib.error.URLError,
331+
ValueError,
352332
TimeoutError,
353333
subprocess.SubprocessError,
354334
OSError,

src/basic_memory/cli/commands/mcp.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,12 @@ def _run_background_auto_update() -> None:
9191
elif result.status == AutoUpdateStatus.FAILED and result.error:
9292
logger.warning(f"MCP background auto-update failed: {result.error}")
9393

94-
threading.Thread(target=_run_background_auto_update, daemon=True).start()
94+
# Trigger: stdio transport corresponds to local user installs.
95+
# Why: server transports (HTTP/SSE) run in managed environments where
96+
# package-manager self-upgrades are inappropriate.
97+
# Outcome: background auto-update runs only for local stdio MCP sessions.
98+
if transport == "stdio":
99+
threading.Thread(target=_run_background_auto_update, daemon=True).start()
95100

96101
# Run the MCP server (blocks)
97102
# Lifespan handles: initialization, migrations, file sync, cleanup

src/basic_memory/cli/commands/project.py

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -227,9 +227,7 @@ async def _list_projects(ws: str | None = None):
227227

228228
console.print(table)
229229
if cloud_error is not None:
230-
console.print(
231-
f"[yellow]Cloud project discovery failed: {cloud_error}[/yellow]"
232-
)
230+
console.print(f"[yellow]Cloud project discovery failed: {cloud_error}[/yellow]")
233231
console.print(
234232
"[dim]Showing local projects only. "
235233
"Run 'bm cloud login' or 'bm cloud api-key save <key>' if this is a credentials issue.[/dim]"

tests/api/v2/test_schema_router.py

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -668,7 +668,8 @@ async def test_validate_reads_schema_from_file_not_database(
668668

669669
# Overwrite the file on disk with validation=strict
670670
file_path = Path(file_service.base_path) / schema_entity.file_path
671-
file_path.write_text(dedent("""\
671+
file_path.write_text(
672+
dedent("""\
672673
---
673674
title: Editable Schema
674675
permalink: schemas/editable-schema
@@ -685,7 +686,8 @@ async def test_validate_reads_schema_from_file_not_database(
685686
686687
## Observations
687688
- [note] Schema that will be edited on disk
688-
"""))
689+
""")
690+
)
689691

690692
# Create a note missing "role" — strict mode should produce errors, not warnings
691693
note_entity, _ = await entity_service.create_or_update_entity(
@@ -749,7 +751,8 @@ async def test_validate_falls_back_to_db_on_incomplete_frontmatter(
749751

750752
# Overwrite file with frontmatter missing the 'schema' key
751753
file_path = Path(file_service.base_path) / schema_entity.file_path
752-
file_path.write_text(dedent("""\
754+
file_path.write_text(
755+
dedent("""\
753756
---
754757
title: Incomplete Schema
755758
permalink: schemas/incomplete-schema
@@ -761,7 +764,8 @@ async def test_validate_falls_back_to_db_on_incomplete_frontmatter(
761764
762765
## Observations
763766
- [note] Mid-edit state
764-
"""))
767+
""")
768+
)
765769

766770
# Create a note to validate against this schema
767771
note_entity, _ = await entity_service.create_or_update_entity(

tests/cli/test_auto_update.py

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@
99
from rich.console import Console
1010

1111
from basic_memory.cli.auto_update import (
12+
AutoUpdateResult,
1213
AutoUpdateStatus,
1314
InstallSource,
15+
_is_interactive_session,
1416
detect_install_source,
1517
maybe_run_periodic_auto_update,
1618
run_auto_update,
@@ -43,6 +45,25 @@ def _base_config(tmp_path) -> BasicMemoryConfig:
4345
return BasicMemoryConfig(projects={"main": {"path": str(tmp_path / "main")}})
4446

4547

48+
def _result(
49+
status: AutoUpdateStatus,
50+
*,
51+
message: str | None,
52+
error: str | None = None,
53+
) -> AutoUpdateResult:
54+
return AutoUpdateResult(
55+
status=status,
56+
source=InstallSource.UV_TOOL,
57+
checked=True,
58+
update_available=status in {AutoUpdateStatus.UPDATE_AVAILABLE, AutoUpdateStatus.UPDATED},
59+
updated=status == AutoUpdateStatus.UPDATED,
60+
latest_version="9.9.9",
61+
message=message,
62+
error=error,
63+
restart_recommended=status == AutoUpdateStatus.UPDATED,
64+
)
65+
66+
4667
def test_detect_install_source_variants():
4768
assert (
4869
detect_install_source("/opt/homebrew/Cellar/basic-memory/0.18.0/bin/python")
@@ -271,3 +292,83 @@ def test_maybe_run_periodic_auto_update_non_interactive_has_no_console_output():
271292
)
272293
assert result is None
273294
assert buf.getvalue() == ""
295+
296+
297+
def test_maybe_run_periodic_auto_update_prints_updated(monkeypatch):
298+
console, buf = _capture_console()
299+
monkeypatch.setattr(
300+
"basic_memory.cli.auto_update.run_auto_update",
301+
lambda **kwargs: _result(
302+
AutoUpdateStatus.UPDATED,
303+
message="Basic Memory was updated successfully.",
304+
),
305+
)
306+
307+
result = maybe_run_periodic_auto_update("status", is_interactive=True, console=console)
308+
assert result is not None
309+
assert result.status == AutoUpdateStatus.UPDATED
310+
assert "updated successfully" in buf.getvalue().lower()
311+
312+
313+
def test_maybe_run_periodic_auto_update_prints_available(monkeypatch):
314+
console, buf = _capture_console()
315+
monkeypatch.setattr(
316+
"basic_memory.cli.auto_update.run_auto_update",
317+
lambda **kwargs: _result(
318+
AutoUpdateStatus.UPDATE_AVAILABLE,
319+
message="Update available (latest: 9.9.9).",
320+
),
321+
)
322+
323+
result = maybe_run_periodic_auto_update("status", is_interactive=True, console=console)
324+
assert result is not None
325+
assert result.status == AutoUpdateStatus.UPDATE_AVAILABLE
326+
assert "update available" in buf.getvalue().lower()
327+
328+
329+
def test_maybe_run_periodic_auto_update_prints_failed_with_error(monkeypatch):
330+
console, buf = _capture_console()
331+
monkeypatch.setattr(
332+
"basic_memory.cli.auto_update.run_auto_update",
333+
lambda **kwargs: _result(
334+
AutoUpdateStatus.FAILED,
335+
message="Automatic update check failed.",
336+
error="network timeout",
337+
),
338+
)
339+
340+
result = maybe_run_periodic_auto_update("status", is_interactive=True, console=console)
341+
assert result is not None
342+
assert result.status == AutoUpdateStatus.FAILED
343+
output = buf.getvalue().lower()
344+
assert "automatic update check failed" in output
345+
assert "network timeout" in output
346+
347+
348+
def test_maybe_run_periodic_auto_update_uses_interactive_probe_when_not_overridden(monkeypatch):
349+
console, buf = _capture_console()
350+
monkeypatch.setattr("basic_memory.cli.auto_update._is_interactive_session", lambda: True)
351+
monkeypatch.setattr(
352+
"basic_memory.cli.auto_update.run_auto_update",
353+
lambda **kwargs: _result(
354+
AutoUpdateStatus.UP_TO_DATE,
355+
message="Basic Memory is up to date.",
356+
),
357+
)
358+
359+
result = maybe_run_periodic_auto_update("status", console=console)
360+
assert result is not None
361+
assert result.status == AutoUpdateStatus.UP_TO_DATE
362+
# UP_TO_DATE is intentionally silent for periodic checks.
363+
assert buf.getvalue() == ""
364+
365+
366+
def test_is_interactive_session_handles_closed_stdio(monkeypatch):
367+
class _BrokenStream:
368+
def isatty(self) -> bool:
369+
raise ValueError("I/O operation on closed file")
370+
371+
monkeypatch.setattr("basic_memory.cli.auto_update.sys.stdin", _BrokenStream())
372+
monkeypatch.setattr("basic_memory.cli.auto_update.sys.stdout", _BrokenStream())
373+
374+
assert _is_interactive_session() is False

tests/cli/test_cloud_status.py

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,7 @@ def is_token_valid(self, t):
6363
monkeypatch.setattr(
6464
"basic_memory.cli.commands.cloud.core_commands.ConfigManager", FakeConfigManager
6565
)
66-
monkeypatch.setattr(
67-
"basic_memory.cli.commands.cloud.core_commands.CLIAuth", FakeAuth
68-
)
66+
monkeypatch.setattr("basic_memory.cli.commands.cloud.core_commands.CLIAuth", FakeAuth)
6967
monkeypatch.setattr(
7068
"basic_memory.cli.commands.cloud.core_commands.get_cloud_config",
7169
lambda: ("cid", "domain", "https://cloud.example.com"),

0 commit comments

Comments
 (0)