forked from molecule-ai/molecule-core
fix: address 5-axis review findings on PR #2413
Critical:
- ExternalConnectModal.tsx: filledUniversalMcp substitution searched
for WORKSPACE_AUTH_TOKEN but the snippet's placeholder is now
MOLECULE_WORKSPACE_TOKEN (changed in the previous polish commit
876c0bfc). Operators copy-pasting the MCP tab would have gotten a
literal "<paste from create response>" instead of the token. Fix
the substitution to match the new placeholder name.
Important:
- mcp_cli._platform_register: 401/403 from initial register now hard-
exits with code 3 + an actionable stderr message pointing the
operator at the canvas Tokens tab. Pre-fix: warning log + continue,
which made a bad-token startup silently fail (heartbeat 401's
forever, every tool call also 401's, no clear surfacing in the
operator's MCP client). 500/503 still log + continue (transient
platform blips shouldn't abort the MCP loop).
- a2a_mcp_server.cli_main docstring: removed stale claim that this is
the wheel's console-script entry-point target. The actual target is
mcp_cli.main since 2026-04-30. Wheel-smoke pins both names so the
functionality was correct, but the doc was lying.
Test coverage: 3 new mcp_cli tests:
- register 401 exits code=3 + stderr mentions canvas Tokens tab
- register 403 (C18 hijack rejection) takes same path
- register 500/503 does NOT exit — only auth errors hard-fail
Findings deferred to follow-up (acceptable per review rubric):
- Code dedup across mcp_cli / heartbeat.py / molecule_agent SDK
- Pooled httpx.Client for connection reuse
- Heartbeat exponential backoff
- Token-resolution ordering parity (env-first vs file-first)
between mcp_cli.main and platform_auth.get_token
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang
parent
876c0bfcd4
commit
b54ceb799f
@ -97,12 +97,16 @@ export function ExternalConnectModal({ info, onClose }: Props) {
|
||||
'MOLECULE_WORKSPACE_TOKENS=<paste auth_token from create response>',
|
||||
`MOLECULE_WORKSPACE_TOKENS=${info.auth_token}`,
|
||||
);
|
||||
// Universal MCP snippet uses the same "<paste from create response>"
|
||||
// placeholder pattern as the curl tab — the auth token is exported
|
||||
// as WORKSPACE_AUTH_TOKEN and reused inline for `molecule-mcp`.
|
||||
// Universal MCP snippet uses MOLECULE_WORKSPACE_TOKEN as the env-var
|
||||
// name passed through to molecule-mcp via `claude mcp add ... -- env
|
||||
// MOLECULE_WORKSPACE_TOKEN=...`. The placeholder must match the
|
||||
// template's literal — pre-2026-04-30 polish this looked for
|
||||
// WORKSPACE_AUTH_TOKEN (carryover from the curl tab), which silently
|
||||
// skipped the substitution and left "<paste from create response>"
|
||||
// visible in the operator's clipboard.
|
||||
const filledUniversalMcp = info.universal_mcp_snippet?.replace(
|
||||
'WORKSPACE_AUTH_TOKEN="<paste from create response>"',
|
||||
`WORKSPACE_AUTH_TOKEN="${info.auth_token}"`,
|
||||
'MOLECULE_WORKSPACE_TOKEN="<paste from create response>"',
|
||||
`MOLECULE_WORKSPACE_TOKEN="${info.auth_token}"`,
|
||||
);
|
||||
|
||||
return (
|
||||
|
||||
@ -202,30 +202,19 @@ async def main(): # pragma: no cover
|
||||
|
||||
|
||||
def cli_main() -> None: # pragma: no cover
|
||||
"""Synchronous entry point for the ``molecule-mcp`` console script.
|
||||
"""Synchronous wrapper around the async MCP stdio loop.
|
||||
|
||||
Declared in scripts/build_runtime_package.py as the wheel's
|
||||
entry-point target (``molecule-mcp = "molecule_runtime.a2a_mcp_server:cli_main"``).
|
||||
External-runtime operators install ``molecule-ai-workspace-runtime``
|
||||
and register this binary as an MCP server in their agent's config —
|
||||
Claude Code, hermes, codex, anything that speaks MCP stdio.
|
||||
Called by ``mcp_cli.main`` (the ``molecule-mcp`` console-script
|
||||
entry point in scripts/build_runtime_package.py) AFTER env
|
||||
validation and the standalone register + heartbeat thread setup.
|
||||
Direct callers (in-container code that already validated env and
|
||||
runs heartbeat.py separately) can also invoke this — it's the
|
||||
smallest possible "run the MCP stdio JSON-RPC loop" surface.
|
||||
|
||||
Required environment:
|
||||
WORKSPACE_ID — this workspace's UUID (must
|
||||
already be registered on the
|
||||
platform).
|
||||
PLATFORM_URL — base URL of the Molecule
|
||||
platform (e.g. https://your-
|
||||
tenant.staging.moleculesai.app).
|
||||
MOLECULE_WORKSPACE_TOKEN — bearer token for this workspace
|
||||
(issued by the platform on
|
||||
first /registry/register).
|
||||
|
||||
Mirrors the ``main_sync`` pattern in main.py — wheel-smoke gates
|
||||
in scripts/wheel_smoke.py assert this name is importable so a
|
||||
silent rename can't break every external-runtime operator's MCP
|
||||
install (the 0.1.16 main_sync incident is the cautionary
|
||||
precedent).
|
||||
Wheel-smoke gates in scripts/wheel_smoke.py pin the importability
|
||||
of this name (alongside ``mcp_cli.main``) so a silent rename can't
|
||||
break every external-runtime operator's MCP install — the 0.1.16
|
||||
``main_sync`` rename incident is the cautionary precedent.
|
||||
"""
|
||||
asyncio.run(main())
|
||||
|
||||
|
||||
@ -53,15 +53,22 @@ HEARTBEAT_INTERVAL_SECONDS = 20.0
|
||||
|
||||
|
||||
def _platform_register(platform_url: str, workspace_id: str, token: str) -> None:
|
||||
"""Best-effort one-shot register at startup.
|
||||
"""One-shot register at startup; fails fast on auth errors.
|
||||
|
||||
Lifts the workspace from ``awaiting_agent`` to ``online`` for
|
||||
operators who never ran the curl-register snippet. Safe to call
|
||||
repeatedly: the platform's register handler is an upsert that just
|
||||
refreshes ``url``, ``agent_card``, and ``status``. Skips silently
|
||||
on transport/HTTP errors so a misconfigured PLATFORM_URL doesn't
|
||||
abort the MCP loop — the heartbeat thread will keep retrying and
|
||||
surface the persistent failure that way.
|
||||
repeatedly: the platform's register handler is an upsert that
|
||||
just refreshes ``url``, ``agent_card``, and ``status``.
|
||||
|
||||
Failure model (post-review):
|
||||
- 401 / 403 → ``sys.exit(3)`` immediately. The operator's
|
||||
token is wrong; silently looping in a broken state would
|
||||
make this hard to diagnose because the MCP tools would 401
|
||||
on every call too. Hard-fail is the kindest option.
|
||||
- Other 4xx/5xx → log a warning + continue. The heartbeat
|
||||
thread will surface persistent failures; transient platform
|
||||
blips shouldn't abort the MCP loop.
|
||||
- Network / transport errors → log + continue. Same reasoning.
|
||||
|
||||
Origin header is required by the SaaS edge WAF; without it
|
||||
/registry/register currently still works (it's on the WAF
|
||||
@ -94,6 +101,14 @@ def _platform_register(platform_url: str, workspace_id: str, token: str) -> None
|
||||
json=payload,
|
||||
headers=headers,
|
||||
)
|
||||
if resp.status_code in (401, 403):
|
||||
print(
|
||||
f"molecule-mcp: register rejected with HTTP {resp.status_code} — "
|
||||
f"the token in MOLECULE_WORKSPACE_TOKEN is invalid for workspace "
|
||||
f"{workspace_id}. Regenerate from the canvas → Tokens tab.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
sys.exit(3)
|
||||
if resp.status_code >= 400:
|
||||
logger.warning(
|
||||
"molecule-mcp: register POST returned HTTP %d: %s",
|
||||
@ -105,6 +120,8 @@ def _platform_register(platform_url: str, workspace_id: str, token: str) -> None
|
||||
"molecule-mcp: registered workspace %s with platform",
|
||||
workspace_id,
|
||||
)
|
||||
except SystemExit:
|
||||
raise
|
||||
except Exception as exc: # noqa: BLE001
|
||||
logger.warning("molecule-mcp: register POST failed: %s", exc)
|
||||
|
||||
|
||||
@ -311,6 +311,97 @@ def test_token_resolved_from_file_when_no_env(monkeypatch, tmp_path):
|
||||
assert captured_token["t"] == "file-token"
|
||||
|
||||
|
||||
def test_register_401_exits_with_actionable_error(monkeypatch, capsys):
|
||||
"""Bad token at startup must hard-fail. Otherwise the operator
|
||||
sees no error in their MCP client (which spawns the binary in a
|
||||
subprocess), the heartbeat thread silently 401's forever, and
|
||||
every tool call also 401's — needle-in-haystack debugging.
|
||||
Hard-exiting prints a clear pointer to the canvas Tokens tab."""
|
||||
|
||||
class FakeResp:
|
||||
status_code = 401
|
||||
text = "invalid workspace auth token"
|
||||
|
||||
class FakeClient:
|
||||
def __init__(self, **_kwargs): pass
|
||||
def __enter__(self): return self
|
||||
def __exit__(self, *_a): return False
|
||||
def post(self, *_a, **_kw): return FakeResp()
|
||||
|
||||
import types
|
||||
fake_httpx = types.ModuleType("httpx")
|
||||
fake_httpx.Client = FakeClient
|
||||
monkeypatch.setitem(sys.modules, "httpx", fake_httpx)
|
||||
|
||||
with pytest.raises(SystemExit) as exc_info:
|
||||
mcp_cli._platform_register(
|
||||
"https://test.moleculesai.app",
|
||||
"ws-bad-token",
|
||||
"wrong-token",
|
||||
)
|
||||
assert exc_info.value.code == 3
|
||||
err = capsys.readouterr().err
|
||||
assert "401" in err
|
||||
assert "ws-bad-token" in err
|
||||
assert "Tokens tab" in err or "canvas" in err.lower()
|
||||
|
||||
|
||||
def test_register_403_also_exits(monkeypatch, capsys):
|
||||
"""403 is the C18 hijack-prevention rejection — same operator
|
||||
action (regenerate token) as 401."""
|
||||
|
||||
class FakeResp:
|
||||
status_code = 403
|
||||
text = "C18: live tokens exist; bearer didn't match"
|
||||
|
||||
class FakeClient:
|
||||
def __init__(self, **_kwargs): pass
|
||||
def __enter__(self): return self
|
||||
def __exit__(self, *_a): return False
|
||||
def post(self, *_a, **_kw): return FakeResp()
|
||||
|
||||
import types
|
||||
fake_httpx = types.ModuleType("httpx")
|
||||
fake_httpx.Client = FakeClient
|
||||
monkeypatch.setitem(sys.modules, "httpx", fake_httpx)
|
||||
|
||||
with pytest.raises(SystemExit) as exc_info:
|
||||
mcp_cli._platform_register(
|
||||
"https://test.moleculesai.app",
|
||||
"ws-hijack",
|
||||
"stolen-token",
|
||||
)
|
||||
assert exc_info.value.code == 3
|
||||
|
||||
|
||||
def test_register_500_does_not_exit(monkeypatch):
|
||||
"""Transient platform errors (500, 503) must NOT hard-fail —
|
||||
those clear on retry and the heartbeat thread will surface
|
||||
persistent failures via warning logs."""
|
||||
|
||||
class FakeResp:
|
||||
status_code = 503
|
||||
text = "service unavailable"
|
||||
|
||||
class FakeClient:
|
||||
def __init__(self, **_kwargs): pass
|
||||
def __enter__(self): return self
|
||||
def __exit__(self, *_a): return False
|
||||
def post(self, *_a, **_kw): return FakeResp()
|
||||
|
||||
import types
|
||||
fake_httpx = types.ModuleType("httpx")
|
||||
fake_httpx.Client = FakeClient
|
||||
monkeypatch.setitem(sys.modules, "httpx", fake_httpx)
|
||||
|
||||
# Should return cleanly, no SystemExit raised
|
||||
mcp_cli._platform_register(
|
||||
"https://test.moleculesai.app",
|
||||
"ws-ok",
|
||||
"tok",
|
||||
)
|
||||
|
||||
|
||||
def test_register_payload_shape(monkeypatch):
|
||||
"""The register POST body must use the field names the workspace-
|
||||
server expects (id/url/agent_card/delivery_mode), and must include
|
||||
|
||||
Loading…
Reference in New Issue
Block a user