diff --git a/RUNBOOK.yaml b/RUNBOOK.yaml index c6629a69..a606d1dc 100644 --- a/RUNBOOK.yaml +++ b/RUNBOOK.yaml @@ -1,7 +1,7 @@ schema_version: '1.0' lock: - active_claim: null - claimed_by: null + active_claim: '6.4' + claimed_by: 2026-04-07-e9b4572b claimed_at: '2026-04-07T01:20:00Z' review: max_rounds: 3 @@ -585,10 +585,10 @@ epics: - id: '6.4' phase: P6 title: DX polish (CLI help, errors, onboarding) - status: pending - branch: null - pr: null - review_round: 0 + status: claimed + branch: epic/6.4-dx-polish + pr: 53 + review_round: 1 description: 'Improve CLI help text, error messages with actionable suggestions, first-run onboarding flow, quickstart documentation. diff --git a/openspace/deploy/preflight.py b/openspace/deploy/preflight.py new file mode 100644 index 00000000..959394a3 --- /dev/null +++ b/openspace/deploy/preflight.py @@ -0,0 +1,182 @@ +"""Pre-flight environment checks for OpenSpace. + +Validates the runtime environment before server startup and provides +actionable error messages with suggested fixes. Designed for DX — a +developer who misconfigures something should know exactly what to do. + +Usage:: + + from openspace.deploy.preflight import preflight_check + + issues = preflight_check(transport="streamable-http") + if issues: + for issue in issues: + print(f" ✗ {issue}") + sys.exit(1) +""" + +from __future__ import annotations + +import logging +import os +import sys +from dataclasses import dataclass, field +from pathlib import Path +from typing import List, Optional + +logger = logging.getLogger(__name__) + + +@dataclass +class PreflightIssue: + """A single pre-flight check failure with actionable fix.""" + + check: str + message: str + suggestion: str + severity: str = "error" # error | warning + + def __str__(self) -> str: + icon = "✗" if self.severity == "error" else "⚠" + return f"{icon} [{self.check}] {self.message}\n → {self.suggestion}" + + +def check_python_version(minimum: tuple = (3, 11)) -> Optional[PreflightIssue]: + """Verify Python version meets minimum requirement.""" + if sys.version_info < minimum: + return PreflightIssue( + check="python-version", + message=f"Python {minimum[0]}.{minimum[1]}+ required, got {sys.version_info.major}.{sys.version_info.minor}", + suggestion=f"Install Python {minimum[0]}.{minimum[1]}+ from https://python.org/downloads/", + ) + return None + + +def check_bearer_token(transport: str) -> Optional[PreflightIssue]: + """Verify bearer token is set for HTTP transports.""" + if transport == "stdio": + return None # No auth needed for stdio + + token_env = "OPENSPACE_MCP_BEARER_TOKEN" + token = os.environ.get(token_env, "").strip() + if not token: + if sys.platform == "win32": + set_cmd = ( + f'$env:{token_env} = python -c "import secrets; print(secrets.token_urlsafe(32))"' + ) + else: + set_cmd = ( + f'export {token_env}=$(python -c "import secrets; print(secrets.token_urlsafe(32))")' + ) + return PreflightIssue( + check="bearer-token", + message=f"{token_env} not set (required for {transport} transport)", + suggestion=( + f"Set {token_env} to a strong random token:\n" + f" {set_cmd}\n" + f" Or use --transport stdio for local-only access (no auth required)" + ), + ) + return None + + +def check_skill_store(path: str) -> Optional[PreflightIssue]: + """Verify skill store directory exists or can be created.""" + p = Path(path) + if p.exists() and not p.is_dir(): + if sys.platform == "win32": + fix_cmd = f"Remove-Item {path}; New-Item -ItemType Directory {path}" + else: + fix_cmd = f"rm {path} && mkdir -p {path}" + return PreflightIssue( + check="skill-store", + message=f"Skill store path exists but is not a directory: {path}", + suggestion=f"Remove the file and create directory:\n {fix_cmd}", + ) + return None + + +def check_port_available(port: int, host: str = "0.0.0.0") -> Optional[PreflightIssue]: + """Check if the target port is likely available (best-effort).""" + import socket + + try: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.settimeout(1) + s.bind((host if host != "0.0.0.0" else "127.0.0.1", port)) + except OSError: + return PreflightIssue( + check="port-available", + message=f"Port {port} appears to be in use on {host}", + suggestion=( + f"Either stop the other process or use a different port:\n" + f" openspace-mcp --port {port + 1}\n" + f" Or set OPENSPACE_MCP_PORT={port + 1}" + ), + severity="warning", + ) + return None + + +def preflight_check( + transport: str = "stdio", + port: int = 8000, + host: str = "0.0.0.0", + skill_store_path: str = "skills/", + check_port: bool = True, +) -> List[PreflightIssue]: + """Run all pre-flight checks and return any issues found. + + Returns an empty list if all checks pass. + + Args: + transport: MCP transport type (stdio, sse, streamable-http) + port: Server port (only checked for HTTP transports) + host: Server bind address + skill_store_path: Path to skill store directory + check_port: Whether to check port availability + """ + issues: List[PreflightIssue] = [] + + result = check_python_version() + if result: + issues.append(result) + + result = check_bearer_token(transport) + if result: + issues.append(result) + + result = check_skill_store(skill_store_path) + if result: + issues.append(result) + + if transport != "stdio" and check_port: + result = check_port_available(port, host) + if result: + issues.append(result) + + return issues + + +def format_preflight_report(issues: List[PreflightIssue]) -> str: + """Format preflight issues into a readable report.""" + errors = [i for i in issues if i.severity == "error"] + warnings = [i for i in issues if i.severity == "warning"] + + lines = ["\n╔══ OpenSpace Pre-flight Check ══╗\n"] + + if errors: + lines.append(f" {len(errors)} error(s) found:\n") + for issue in errors: + lines.append(f" {issue}\n") + + if warnings: + lines.append(f" {len(warnings)} warning(s):\n") + for issue in warnings: + lines.append(f" {issue}\n") + + if not errors: + lines.append(" ✓ All checks passed\n") + + lines.append("╚═══════════════════════════════╝") + return "\n".join(lines) diff --git a/openspace/mcp/server.py b/openspace/mcp/server.py index bcbb77d2..5dce6bf2 100644 --- a/openspace/mcp/server.py +++ b/openspace/mcp/server.py @@ -183,7 +183,84 @@ def _probe_mcp_tools() -> HealthProbe: # --------------------------------------------------------------------------- -# 4. Server entry point +# 4. CLI argument parser (extracted for testability) +# --------------------------------------------------------------------------- +_VERSION = "0.1.0" # TODO: read from pyproject.toml or importlib.metadata + +_EPILOG = """\ +examples: + openspace-mcp # stdio transport (local, no auth) + openspace-mcp --transport streamable-http # HTTP (requires bearer token) + openspace-mcp --transport sse --port 9000 # SSE on custom port + +environment variables: + OPENSPACE_MCP_HOST Bind address (default: 0.0.0.0) + OPENSPACE_MCP_PORT Port number (default: 8000) + OPENSPACE_MCP_TRANSPORT Transport: stdio|sse|streamable-http + OPENSPACE_MCP_BEARER_TOKEN Auth token (REQUIRED for HTTP transports) + OPENSPACE_LOG_LEVEL Logging: DEBUG|INFO|WARNING|ERROR + OPENSPACE_SHUTDOWN_TIMEOUT Graceful shutdown seconds (default: 30) + OPENSPACE_METRICS_ENABLED Prometheus metrics: true|false + +notes: + HTTP transports (sse, streamable-http) require OPENSPACE_MCP_BEARER_TOKEN. + Use --transport stdio for local development without authentication. +""" + + +def _build_arg_parser(deploy_cfg=None): + """Build the CLI argument parser with rich help text. + + Extracted from run_mcp_server for testability. Returns an + argparse.ArgumentParser ready for parse_args(). + """ + import argparse + + defaults = { + "transport": "stdio", + "port": 8000, + "host": "0.0.0.0", + } + if deploy_cfg is not None: + defaults["transport"] = deploy_cfg.mcp_transport + defaults["port"] = deploy_cfg.mcp_port + defaults["host"] = deploy_cfg.mcp_host + + parser = argparse.ArgumentParser( + prog="openspace-mcp", + description="OpenSpace MCP Server — AI agent framework with tool execution", + epilog=_EPILOG, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + parser.add_argument( + "--version", + action="version", + version=f"openspace-mcp {_VERSION}", + ) + parser.add_argument( + "--transport", + choices=["stdio", "sse", "streamable-http"], + default=defaults["transport"], + help="MCP transport protocol (default: %(default)s). " + "HTTP transports require OPENSPACE_MCP_BEARER_TOKEN.", + ) + parser.add_argument( + "--port", + type=int, + default=defaults["port"], + help="Server port for HTTP transports (default: %(default)s)", + ) + parser.add_argument( + "--host", + type=str, + default=defaults["host"], + help="Bind address for HTTP transports (default: %(default)s)", + ) + return parser + + +# --------------------------------------------------------------------------- +# 5. Server entry point # --------------------------------------------------------------------------- def run_mcp_server(mcp=None) -> None: """Console-script entry point for ``openspace-mcp``. @@ -195,8 +272,6 @@ def run_mcp_server(mcp=None) -> None: mcp: Optional pre-created FastMCP instance. If None, creates one via ``create_mcp_app()``. """ - import argparse - import uvicorn from openspace.auth.bearer import ( @@ -214,35 +289,57 @@ def run_mcp_server(mcp=None) -> None: # Load config from environment, then allow CLI overrides deploy_cfg = DeployConfig.from_env() - parser = argparse.ArgumentParser(description="OpenSpace MCP Server") - parser.add_argument( - "--transport", - choices=["stdio", "sse", "streamable-http"], - default=deploy_cfg.mcp_transport, - ) - parser.add_argument("--port", type=int, default=deploy_cfg.mcp_port) - parser.add_argument("--host", type=str, default=deploy_cfg.mcp_host) + parser = _build_arg_parser(deploy_cfg) args = parser.parse_args() + # Pre-flight checks: validate environment before starting + from openspace.deploy.preflight import ( + format_preflight_report, + preflight_check, + ) + + issues = preflight_check( + transport=args.transport, + port=args.port, + host=args.host, + skill_store_path=deploy_cfg.skill_store_path, + check_port=(args.transport != "stdio"), + ) + if issues: + errors = [i for i in issues if i.severity == "error"] + report = format_preflight_report(issues) + # Write directly to console — logger goes to file only, user won't see it + _console = sys.__stderr__ or sys.stderr + _console.write(report + "\n") + _console.flush() + if errors: + sys.exit(1) + if args.transport == "stdio": mcp.run(transport="stdio") return # --- HTTP transports: enforce bearer token auth (fail-closed) --- token = get_bearer_token() + _console = sys.__stderr__ or sys.stderr if not token: - logger.critical( - "FAIL-CLOSED: %s not set. Refusing to start %s transport " - "without authentication. Set the environment variable or " - "use --transport stdio for local-only access.", - BEARER_TOKEN_ENV, - args.transport, + msg = ( + f"FAIL-CLOSED: {BEARER_TOKEN_ENV} not set. Refusing to start " + f"{args.transport} transport without authentication.\n" + f" → Set the environment variable or use --transport stdio " + f"for local-only access.\n" ) + _console.write(msg) + _console.flush() + logger.critical(msg.strip()) sys.exit(1) token_ok, reason = validate_token_strength(token) if not token_ok: - logger.critical("FAIL-CLOSED: %s — %s", BEARER_TOKEN_ENV, reason) + msg = f"FAIL-CLOSED: {BEARER_TOKEN_ENV} — {reason}\n" + _console.write(msg) + _console.flush() + logger.critical(msg.strip()) sys.exit(1) if args.transport == "sse": diff --git a/tests/test_dx_polish.py b/tests/test_dx_polish.py new file mode 100644 index 00000000..ad7fc1be --- /dev/null +++ b/tests/test_dx_polish.py @@ -0,0 +1,330 @@ +"""Tests for Epic 6.4: DX Polish — CLI help, error messages, onboarding. + +Tests cover: +- CLI --help output quality (examples, descriptions) +- Pre-flight checks (env validation, actionable suggestions) +- Error message UX (suggestions included) +- Argument defaults from DeployConfig +""" + +from __future__ import annotations + +import os +from pathlib import Path +from unittest.mock import patch + +import pytest + +ROOT = Path(__file__).resolve().parent.parent + + +# ====================================================================== +# CLI --help quality +# ====================================================================== +class TestCLIHelp: + """openspace-mcp --help must be useful for first-time users.""" + + def _get_help_text(self): + """Get help text from parser directly (stdout redirected on Windows).""" + import io + from openspace.mcp.server import _build_arg_parser + + parser = _build_arg_parser() + buf = io.StringIO() + parser.print_help(buf) + return buf.getvalue() + + def test_help_exits_zero(self): + """--help should not crash the parser.""" + from openspace.mcp.server import _build_arg_parser + + parser = _build_arg_parser() + # parse_args(['--help']) calls sys.exit(0) — just verify parser builds + assert parser is not None + + def test_help_contains_description(self): + text = self._get_help_text() + assert "OpenSpace" in text + assert "MCP" in text + + def test_help_contains_examples(self): + """--help should show usage examples.""" + text = self._get_help_text() + assert "example" in text.lower() or "openspace-mcp" in text + + def test_help_documents_transport_choices(self): + text = self._get_help_text() + assert "stdio" in text + assert "streamable-http" in text + + def test_help_explains_auth_requirement(self): + """Users must know HTTP transports require a bearer token.""" + text = self._get_help_text() + output = text.lower() + assert "bearer" in output or "auth" in output or "token" in output + + def test_help_shows_env_var_names(self): + """Users should see which env vars configure the server.""" + text = self._get_help_text() + assert "OPENSPACE_MCP" in text + + def test_help_has_version_flag(self): + """--version should be registered.""" + from openspace.mcp.server import _build_arg_parser, _VERSION + + parser = _build_arg_parser() + # Check that version action exists + version_actions = [ + a for a in parser._actions + if isinstance(a, __import__("argparse")._VersionAction) + ] + assert version_actions, "--version must be registered" + assert _VERSION in version_actions[0].version + + +# ====================================================================== +# Pre-flight checks +# ====================================================================== +class TestPreflightChecks: + """Environment validation with actionable suggestions.""" + + def test_preflight_passes_for_stdio(self): + from openspace.deploy.preflight import preflight_check + + issues = preflight_check(transport="stdio", check_port=False) + errors = [i for i in issues if i.severity == "error"] + assert not errors, f"stdio should have no errors: {errors}" + + def test_preflight_catches_missing_bearer_token(self): + from openspace.deploy.preflight import preflight_check + + with patch.dict(os.environ, {}, clear=True): + issues = preflight_check(transport="streamable-http", check_port=False) + token_issues = [i for i in issues if i.check == "bearer-token"] + assert len(token_issues) == 1 + assert "OPENSPACE_MCP_BEARER_TOKEN" in token_issues[0].message + + def test_preflight_suggestion_is_actionable(self): + """Suggestions must include a platform-appropriate set command.""" + from openspace.deploy.preflight import preflight_check + + with patch.dict(os.environ, {}, clear=True): + issues = preflight_check(transport="streamable-http", check_port=False) + token_issues = [i for i in issues if i.check == "bearer-token"] + assert token_issues + suggestion = token_issues[0].suggestion + # Must contain a real shell command, not just descriptive text + assert "$env:" in suggestion or "export " in suggestion + + def test_preflight_no_token_check_for_stdio(self): + from openspace.deploy.preflight import check_bearer_token + + result = check_bearer_token("stdio") + assert result is None + + def test_preflight_token_check_passes_when_set(self): + from openspace.deploy.preflight import check_bearer_token + + with patch.dict(os.environ, {"OPENSPACE_MCP_BEARER_TOKEN": "test-token-value"}): + result = check_bearer_token("streamable-http") + assert result is None + + def test_preflight_skill_store_file_not_dir(self, tmp_path): + from openspace.deploy.preflight import check_skill_store + + # Create a file where directory expected + fake_path = tmp_path / "skills" + fake_path.write_text("not a directory") + result = check_skill_store(str(fake_path)) + assert result is not None + assert result.check == "skill-store" + + def test_preflight_skill_store_missing_is_ok(self, tmp_path): + """Missing skill store is OK — it will be created on first use.""" + from openspace.deploy.preflight import check_skill_store + + result = check_skill_store(str(tmp_path / "nonexistent")) + assert result is None + + def test_preflight_python_version_check(self): + from openspace.deploy.preflight import check_python_version + + # Current Python should pass + result = check_python_version(minimum=(3, 11)) + assert result is None + + def test_preflight_python_version_too_old(self): + from openspace.deploy.preflight import check_python_version + + # Require a future version to force failure + result = check_python_version(minimum=(99, 0)) + assert result is not None + assert "99.0" in result.message + + def test_preflight_issue_str_format(self): + from openspace.deploy.preflight import PreflightIssue + + issue = PreflightIssue( + check="test", message="broken", suggestion="fix it" + ) + text = str(issue) + assert "✗" in text + assert "broken" in text + assert "fix it" in text + + def test_preflight_warning_icon(self): + from openspace.deploy.preflight import PreflightIssue + + issue = PreflightIssue( + check="test", message="warning", suggestion="maybe fix", + severity="warning", + ) + assert "⚠" in str(issue) + + def test_preflight_report_format(self): + from openspace.deploy.preflight import ( + PreflightIssue, + format_preflight_report, + ) + + issues = [ + PreflightIssue(check="a", message="err", suggestion="fix", severity="error"), + PreflightIssue(check="b", message="warn", suggestion="maybe", severity="warning"), + ] + report = format_preflight_report(issues) + assert "Pre-flight" in report + assert "1 error" in report + assert "1 warning" in report + + def test_preflight_report_all_clear(self): + from openspace.deploy.preflight import format_preflight_report + + report = format_preflight_report([]) + assert "All checks passed" in report + + def test_preflight_bearer_suggestion_platform_aware(self): + """Suggestion must use platform-appropriate shell syntax.""" + from openspace.deploy.preflight import check_bearer_token + + with patch.dict(os.environ, {}, clear=True): + result = check_bearer_token("streamable-http") + assert result is not None + if os.name == "nt": + assert "$env:" in result.suggestion + else: + assert "export " in result.suggestion + + def test_preflight_skill_store_suggestion_platform_aware(self, tmp_path): + """Skill store fix suggestion must use platform-appropriate commands.""" + from openspace.deploy.preflight import check_skill_store + + fake_path = tmp_path / "skills" + fake_path.write_text("not a directory") + result = check_skill_store(str(fake_path)) + assert result is not None + if os.name == "nt": + assert "Remove-Item" in result.suggestion + else: + assert "rm " in result.suggestion + + +# ====================================================================== +# Preflight visibility (P0 fix) +# ====================================================================== +class TestPreflightVisibility: + """Preflight output must reach the user, not just the log file.""" + + def test_preflight_writes_to_console_not_just_logger(self): + """run_mcp_server must write preflight to sys.__stderr__, not just logger.""" + import inspect + from openspace.mcp.server import run_mcp_server + + source = inspect.getsource(run_mcp_server) + assert "__stderr__" in source, ( + "Preflight output must use sys.__stderr__ to reach console" + ) + + def test_critical_errors_write_to_console(self): + """Bearer token errors must also reach console.""" + import inspect + from openspace.mcp.server import run_mcp_server + + source = inspect.getsource(run_mcp_server) + # Count __stderr__ occurrences — should be used for both preflight AND auth errors + count = source.count("__stderr__") + assert count >= 2, ( + f"Expected __stderr__ used for preflight AND auth errors, found {count} uses" + ) + + +# ====================================================================== +# Error message UX +# ====================================================================== +class TestErrorMessages: + """Error messages must include actionable suggestions.""" + + def test_invalid_transport_error_is_descriptive(self): + from openspace.deploy.config import DeployConfig + + with pytest.raises(ValueError, match="transport"): + DeployConfig(mcp_transport="invalid") + + def test_invalid_port_error_shows_range(self): + from openspace.deploy.config import DeployConfig + + with pytest.raises(ValueError, match="1-65535"): + DeployConfig(mcp_port=99999) + + def test_bad_env_port_error_names_variable(self): + from openspace.deploy.config import DeployConfig + + with patch.dict(os.environ, {"OPENSPACE_MCP_PORT": "xyz"}): + with pytest.raises(ValueError, match="OPENSPACE_MCP_PORT"): + DeployConfig.from_env() + + def test_bad_log_level_shows_valid_options(self): + from openspace.deploy.config import DeployConfig + + with pytest.raises(ValueError, match="DEBUG.*INFO.*WARNING.*ERROR"): + DeployConfig(log_level="VERBOSE") + + +# ====================================================================== +# Entrypoint wiring +# ====================================================================== +class TestEntrypointDX: + """DX improvements are wired into the actual entry point.""" + + def test_preflight_module_importable(self): + from openspace.deploy.preflight import preflight_check + assert callable(preflight_check) + + def test_preflight_wired_into_server(self): + """run_mcp_server must call preflight_check before starting.""" + import inspect + from openspace.mcp.server import run_mcp_server + + source = inspect.getsource(run_mcp_server) + assert "preflight" in source.lower(), ( + "Preflight checks must be wired into run_mcp_server" + ) + + def test_server_argparse_has_epilog(self): + """Argparse must have epilog with examples.""" + from openspace.mcp.server import _build_arg_parser + + parser = _build_arg_parser() + assert parser.epilog, "argparse must have epilog with examples" + assert "example" in parser.epilog.lower() + + def test_server_argparse_has_formatter(self): + """Must use RawDescriptionHelpFormatter for epilog formatting.""" + import argparse + + from openspace.mcp.server import _build_arg_parser + + parser = _build_arg_parser() + assert parser.formatter_class in ( + argparse.RawDescriptionHelpFormatter, + argparse.RawTextHelpFormatter, + ) diff --git a/tests/test_p4_integration.py b/tests/test_p4_integration.py index 45f366cd..388cb29c 100644 --- a/tests/test_p4_integration.py +++ b/tests/test_p4_integration.py @@ -394,7 +394,7 @@ def test_mcp_decomposition_sizes(self): """Verify MCP modules are within expected bounds.""" modules = { "openspace/mcp_server.py": 65, # shim, ~51 (+27% headroom) - "openspace/mcp/server.py": 310, # ~272 (+14% headroom, health probes added 6.1) + "openspace/mcp/server.py": 410, # ~396 (+4% headroom, CLI help + preflight added 6.4) "openspace/mcp/tool_handlers.py": 970, # ~880 (+10% headroom, SLO tool added 6.2) } base = Path(__file__).parent.parent