Skip to content

Commit a806c4f

Browse files
DeepfreezechillBrian KrafftCopilot
authored
Epic 6.4: DX Polish - CLI help, preflight checks, error UX (#53)
* feat: Epic 6.4 - DX polish (CLI help, preflight checks, error UX) - Extract _build_arg_parser() with rich --help: epilog with examples, env var docs, --version flag, RawDescriptionHelpFormatter - Add openspace/deploy/preflight.py: PreflightIssue, check_python_version, check_bearer_token, check_skill_store, check_port_available, format_report - Wire preflight_check() into run_mcp_server() before server start - Preflight catches: missing bearer token, bad skill store, port in use - All errors include actionable suggestions (commands to run) - 28 new DX tests, 1953 total passed - Bump server.py size guard from 310 to 410 (CLI help + preflight growth) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address R1 review findings for Epic 6.4 DX polish - preflight visibility, platform-aware suggestions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Brian Krafft <bkrafft@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent c10818e commit a806c4f

5 files changed

Lines changed: 634 additions & 25 deletions

File tree

RUNBOOK.yaml

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
schema_version: '1.0'
22
lock:
3-
active_claim: null
4-
claimed_by: null
3+
active_claim: '6.4'
4+
claimed_by: 2026-04-07-e9b4572b
55
claimed_at: '2026-04-07T01:20:00Z'
66
review:
77
max_rounds: 3
@@ -585,10 +585,10 @@ epics:
585585
- id: '6.4'
586586
phase: P6
587587
title: DX polish (CLI help, errors, onboarding)
588-
status: pending
589-
branch: null
590-
pr: null
591-
review_round: 0
588+
status: claimed
589+
branch: epic/6.4-dx-polish
590+
pr: 53
591+
review_round: 1
592592
description: 'Improve CLI help text, error messages with actionable suggestions,
593593
first-run onboarding flow, quickstart documentation.
594594

openspace/deploy/preflight.py

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
"""Pre-flight environment checks for OpenSpace.
2+
3+
Validates the runtime environment before server startup and provides
4+
actionable error messages with suggested fixes. Designed for DX — a
5+
developer who misconfigures something should know exactly what to do.
6+
7+
Usage::
8+
9+
from openspace.deploy.preflight import preflight_check
10+
11+
issues = preflight_check(transport="streamable-http")
12+
if issues:
13+
for issue in issues:
14+
print(f" ✗ {issue}")
15+
sys.exit(1)
16+
"""
17+
18+
from __future__ import annotations
19+
20+
import logging
21+
import os
22+
import sys
23+
from dataclasses import dataclass, field
24+
from pathlib import Path
25+
from typing import List, Optional
26+
27+
logger = logging.getLogger(__name__)
28+
29+
30+
@dataclass
31+
class PreflightIssue:
32+
"""A single pre-flight check failure with actionable fix."""
33+
34+
check: str
35+
message: str
36+
suggestion: str
37+
severity: str = "error" # error | warning
38+
39+
def __str__(self) -> str:
40+
icon = "✗" if self.severity == "error" else "⚠"
41+
return f"{icon} [{self.check}] {self.message}\n{self.suggestion}"
42+
43+
44+
def check_python_version(minimum: tuple = (3, 11)) -> Optional[PreflightIssue]:
45+
"""Verify Python version meets minimum requirement."""
46+
if sys.version_info < minimum:
47+
return PreflightIssue(
48+
check="python-version",
49+
message=f"Python {minimum[0]}.{minimum[1]}+ required, got {sys.version_info.major}.{sys.version_info.minor}",
50+
suggestion=f"Install Python {minimum[0]}.{minimum[1]}+ from https://python.org/downloads/",
51+
)
52+
return None
53+
54+
55+
def check_bearer_token(transport: str) -> Optional[PreflightIssue]:
56+
"""Verify bearer token is set for HTTP transports."""
57+
if transport == "stdio":
58+
return None # No auth needed for stdio
59+
60+
token_env = "OPENSPACE_MCP_BEARER_TOKEN"
61+
token = os.environ.get(token_env, "").strip()
62+
if not token:
63+
if sys.platform == "win32":
64+
set_cmd = (
65+
f'$env:{token_env} = python -c "import secrets; print(secrets.token_urlsafe(32))"'
66+
)
67+
else:
68+
set_cmd = (
69+
f'export {token_env}=$(python -c "import secrets; print(secrets.token_urlsafe(32))")'
70+
)
71+
return PreflightIssue(
72+
check="bearer-token",
73+
message=f"{token_env} not set (required for {transport} transport)",
74+
suggestion=(
75+
f"Set {token_env} to a strong random token:\n"
76+
f" {set_cmd}\n"
77+
f" Or use --transport stdio for local-only access (no auth required)"
78+
),
79+
)
80+
return None
81+
82+
83+
def check_skill_store(path: str) -> Optional[PreflightIssue]:
84+
"""Verify skill store directory exists or can be created."""
85+
p = Path(path)
86+
if p.exists() and not p.is_dir():
87+
if sys.platform == "win32":
88+
fix_cmd = f"Remove-Item {path}; New-Item -ItemType Directory {path}"
89+
else:
90+
fix_cmd = f"rm {path} && mkdir -p {path}"
91+
return PreflightIssue(
92+
check="skill-store",
93+
message=f"Skill store path exists but is not a directory: {path}",
94+
suggestion=f"Remove the file and create directory:\n {fix_cmd}",
95+
)
96+
return None
97+
98+
99+
def check_port_available(port: int, host: str = "0.0.0.0") -> Optional[PreflightIssue]:
100+
"""Check if the target port is likely available (best-effort)."""
101+
import socket
102+
103+
try:
104+
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
105+
s.settimeout(1)
106+
s.bind((host if host != "0.0.0.0" else "127.0.0.1", port))
107+
except OSError:
108+
return PreflightIssue(
109+
check="port-available",
110+
message=f"Port {port} appears to be in use on {host}",
111+
suggestion=(
112+
f"Either stop the other process or use a different port:\n"
113+
f" openspace-mcp --port {port + 1}\n"
114+
f" Or set OPENSPACE_MCP_PORT={port + 1}"
115+
),
116+
severity="warning",
117+
)
118+
return None
119+
120+
121+
def preflight_check(
122+
transport: str = "stdio",
123+
port: int = 8000,
124+
host: str = "0.0.0.0",
125+
skill_store_path: str = "skills/",
126+
check_port: bool = True,
127+
) -> List[PreflightIssue]:
128+
"""Run all pre-flight checks and return any issues found.
129+
130+
Returns an empty list if all checks pass.
131+
132+
Args:
133+
transport: MCP transport type (stdio, sse, streamable-http)
134+
port: Server port (only checked for HTTP transports)
135+
host: Server bind address
136+
skill_store_path: Path to skill store directory
137+
check_port: Whether to check port availability
138+
"""
139+
issues: List[PreflightIssue] = []
140+
141+
result = check_python_version()
142+
if result:
143+
issues.append(result)
144+
145+
result = check_bearer_token(transport)
146+
if result:
147+
issues.append(result)
148+
149+
result = check_skill_store(skill_store_path)
150+
if result:
151+
issues.append(result)
152+
153+
if transport != "stdio" and check_port:
154+
result = check_port_available(port, host)
155+
if result:
156+
issues.append(result)
157+
158+
return issues
159+
160+
161+
def format_preflight_report(issues: List[PreflightIssue]) -> str:
162+
"""Format preflight issues into a readable report."""
163+
errors = [i for i in issues if i.severity == "error"]
164+
warnings = [i for i in issues if i.severity == "warning"]
165+
166+
lines = ["\n╔══ OpenSpace Pre-flight Check ══╗\n"]
167+
168+
if errors:
169+
lines.append(f" {len(errors)} error(s) found:\n")
170+
for issue in errors:
171+
lines.append(f" {issue}\n")
172+
173+
if warnings:
174+
lines.append(f" {len(warnings)} warning(s):\n")
175+
for issue in warnings:
176+
lines.append(f" {issue}\n")
177+
178+
if not errors:
179+
lines.append(" ✓ All checks passed\n")
180+
181+
lines.append("╚═══════════════════════════════╝")
182+
return "\n".join(lines)

openspace/mcp/server.py

Lines changed: 115 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,84 @@ def _probe_mcp_tools() -> HealthProbe:
183183

184184

185185
# ---------------------------------------------------------------------------
186-
# 4. Server entry point
186+
# 4. CLI argument parser (extracted for testability)
187+
# ---------------------------------------------------------------------------
188+
_VERSION = "0.1.0" # TODO: read from pyproject.toml or importlib.metadata
189+
190+
_EPILOG = """\
191+
examples:
192+
openspace-mcp # stdio transport (local, no auth)
193+
openspace-mcp --transport streamable-http # HTTP (requires bearer token)
194+
openspace-mcp --transport sse --port 9000 # SSE on custom port
195+
196+
environment variables:
197+
OPENSPACE_MCP_HOST Bind address (default: 0.0.0.0)
198+
OPENSPACE_MCP_PORT Port number (default: 8000)
199+
OPENSPACE_MCP_TRANSPORT Transport: stdio|sse|streamable-http
200+
OPENSPACE_MCP_BEARER_TOKEN Auth token (REQUIRED for HTTP transports)
201+
OPENSPACE_LOG_LEVEL Logging: DEBUG|INFO|WARNING|ERROR
202+
OPENSPACE_SHUTDOWN_TIMEOUT Graceful shutdown seconds (default: 30)
203+
OPENSPACE_METRICS_ENABLED Prometheus metrics: true|false
204+
205+
notes:
206+
HTTP transports (sse, streamable-http) require OPENSPACE_MCP_BEARER_TOKEN.
207+
Use --transport stdio for local development without authentication.
208+
"""
209+
210+
211+
def _build_arg_parser(deploy_cfg=None):
212+
"""Build the CLI argument parser with rich help text.
213+
214+
Extracted from run_mcp_server for testability. Returns an
215+
argparse.ArgumentParser ready for parse_args().
216+
"""
217+
import argparse
218+
219+
defaults = {
220+
"transport": "stdio",
221+
"port": 8000,
222+
"host": "0.0.0.0",
223+
}
224+
if deploy_cfg is not None:
225+
defaults["transport"] = deploy_cfg.mcp_transport
226+
defaults["port"] = deploy_cfg.mcp_port
227+
defaults["host"] = deploy_cfg.mcp_host
228+
229+
parser = argparse.ArgumentParser(
230+
prog="openspace-mcp",
231+
description="OpenSpace MCP Server — AI agent framework with tool execution",
232+
epilog=_EPILOG,
233+
formatter_class=argparse.RawDescriptionHelpFormatter,
234+
)
235+
parser.add_argument(
236+
"--version",
237+
action="version",
238+
version=f"openspace-mcp {_VERSION}",
239+
)
240+
parser.add_argument(
241+
"--transport",
242+
choices=["stdio", "sse", "streamable-http"],
243+
default=defaults["transport"],
244+
help="MCP transport protocol (default: %(default)s). "
245+
"HTTP transports require OPENSPACE_MCP_BEARER_TOKEN.",
246+
)
247+
parser.add_argument(
248+
"--port",
249+
type=int,
250+
default=defaults["port"],
251+
help="Server port for HTTP transports (default: %(default)s)",
252+
)
253+
parser.add_argument(
254+
"--host",
255+
type=str,
256+
default=defaults["host"],
257+
help="Bind address for HTTP transports (default: %(default)s)",
258+
)
259+
return parser
260+
261+
262+
# ---------------------------------------------------------------------------
263+
# 5. Server entry point
187264
# ---------------------------------------------------------------------------
188265
def run_mcp_server(mcp=None) -> None:
189266
"""Console-script entry point for ``openspace-mcp``.
@@ -195,8 +272,6 @@ def run_mcp_server(mcp=None) -> None:
195272
mcp: Optional pre-created FastMCP instance. If None, creates one
196273
via ``create_mcp_app()``.
197274
"""
198-
import argparse
199-
200275
import uvicorn
201276

202277
from openspace.auth.bearer import (
@@ -214,35 +289,57 @@ def run_mcp_server(mcp=None) -> None:
214289
# Load config from environment, then allow CLI overrides
215290
deploy_cfg = DeployConfig.from_env()
216291

217-
parser = argparse.ArgumentParser(description="OpenSpace MCP Server")
218-
parser.add_argument(
219-
"--transport",
220-
choices=["stdio", "sse", "streamable-http"],
221-
default=deploy_cfg.mcp_transport,
222-
)
223-
parser.add_argument("--port", type=int, default=deploy_cfg.mcp_port)
224-
parser.add_argument("--host", type=str, default=deploy_cfg.mcp_host)
292+
parser = _build_arg_parser(deploy_cfg)
225293
args = parser.parse_args()
226294

295+
# Pre-flight checks: validate environment before starting
296+
from openspace.deploy.preflight import (
297+
format_preflight_report,
298+
preflight_check,
299+
)
300+
301+
issues = preflight_check(
302+
transport=args.transport,
303+
port=args.port,
304+
host=args.host,
305+
skill_store_path=deploy_cfg.skill_store_path,
306+
check_port=(args.transport != "stdio"),
307+
)
308+
if issues:
309+
errors = [i for i in issues if i.severity == "error"]
310+
report = format_preflight_report(issues)
311+
# Write directly to console — logger goes to file only, user won't see it
312+
_console = sys.__stderr__ or sys.stderr
313+
_console.write(report + "\n")
314+
_console.flush()
315+
if errors:
316+
sys.exit(1)
317+
227318
if args.transport == "stdio":
228319
mcp.run(transport="stdio")
229320
return
230321

231322
# --- HTTP transports: enforce bearer token auth (fail-closed) ---
232323
token = get_bearer_token()
324+
_console = sys.__stderr__ or sys.stderr
233325
if not token:
234-
logger.critical(
235-
"FAIL-CLOSED: %s not set. Refusing to start %s transport "
236-
"without authentication. Set the environment variable or "
237-
"use --transport stdio for local-only access.",
238-
BEARER_TOKEN_ENV,
239-
args.transport,
326+
msg = (
327+
f"FAIL-CLOSED: {BEARER_TOKEN_ENV} not set. Refusing to start "
328+
f"{args.transport} transport without authentication.\n"
329+
f" → Set the environment variable or use --transport stdio "
330+
f"for local-only access.\n"
240331
)
332+
_console.write(msg)
333+
_console.flush()
334+
logger.critical(msg.strip())
241335
sys.exit(1)
242336

243337
token_ok, reason = validate_token_strength(token)
244338
if not token_ok:
245-
logger.critical("FAIL-CLOSED: %s — %s", BEARER_TOKEN_ENV, reason)
339+
msg = f"FAIL-CLOSED: {BEARER_TOKEN_ENV}{reason}\n"
340+
_console.write(msg)
341+
_console.flush()
342+
logger.critical(msg.strip())
246343
sys.exit(1)
247344

248345
if args.transport == "sse":

0 commit comments

Comments
 (0)