diff --git a/.devcontainer/on_create.py b/.devcontainer/on_create.py index a6ffb16..8ac9fc1 100644 --- a/.devcontainer/on_create.py +++ b/.devcontainer/on_create.py @@ -118,7 +118,7 @@ def refresh_npm_tools(workspace: Path) -> None: def configure_mcp(workspace: Path) -> None: """Regenerate the machine-local MCP configs when node is available.""" if shutil.which("node") is not None: - code = run_child(["bash", ".llm/mcp/sync-mcp.sh", str(workspace)], workspace) + code = run_child([sys.executable, ".llm/mcp/sync_mcp.py", str(workspace)], workspace) if code != 0: print("⚠️ MCP configuration reported warnings; run 'npm run mcp:sync' to retry.") return diff --git a/.devcontainer/post_start.py b/.devcontainer/post_start.py index a39d3f4..ae69092 100644 --- a/.devcontainer/post_start.py +++ b/.devcontainer/post_start.py @@ -222,7 +222,7 @@ def main(argv: Sequence[str]) -> int: env = child_env_with_env_local(workspace) repair_credential_aliases(workspace, env) - run_child_strict(["bash", ".llm/mcp/sync-mcp.sh", str(workspace)], workspace, env) + run_child_strict([sys.executable, ".llm/mcp/sync_mcp.py", str(workspace)], workspace, env) if shutil.which("opencode") is not None: recycle_stale_service(workspace) diff --git a/.devcontainer/update_content.py b/.devcontainer/update_content.py index e92fa12..393ee01 100644 --- a/.devcontainer/update_content.py +++ b/.devcontainer/update_content.py @@ -32,7 +32,7 @@ def sync_mcp_configs(workspace: Path) -> None: """Re-derive the machine-local MCP configs when node is available.""" if shutil.which("node") is None: return - run_child_strict(["bash", ".llm/mcp/sync-mcp.sh", str(workspace)], workspace) + run_child_strict([sys.executable, ".llm/mcp/sync_mcp.py", str(workspace)], workspace) def main(argv: Sequence[str]) -> int: diff --git a/.llm/mcp/README.md b/.llm/mcp/README.md index 8a7d076..8f8d8a0 100644 --- a/.llm/mcp/README.md +++ b/.llm/mcp/README.md @@ -138,8 +138,8 @@ Security: the bridge listens on all interfaces by default (Linux Docker cannot r - **Port already in use / dynamic ports.** Re-running the bridge is a no-op while a healthy instance holds the port; if something else owns it, the bridge binds the next free port, persists `UNITY_MCP_HTTP_PORT` to `.env.local`, and re-syncs every frontend config automatically. No fixed port is required anywhere: `configure.mjs`, the devcontainer health check, and the doctors all read the persisted value. -- **`npm run mcp:sync` on the host.** It runs `configure.mjs` directly (pure node, cross-platform); the bash wrapper `sync-mcp.sh` remains for container hooks. `hasCommand` uses - `where` on Windows so `uvx`-gated servers register on the host too. +- **`npm run mcp:sync` on the host.** It runs `configure.mjs` directly (pure node, cross-platform); container hooks use the typed Python wrapper `sync_mcp.py` + (`uv run .llm/mcp/sync_mcp.py [workspace_dir] [flags...]`). `hasCommand` uses `where` on Windows so `uvx`-gated servers register on the host too. - **Shared configs are environment-agnostic.** Sync produces identical files from the host and the container (verified by hash): the unity bridge URL always uses `host.docker.internal` (resolves on the host under Docker Desktop too), and zai-image uses PATH-resolved `node` with a workspace-relative script. Never reintroduce `127.0.0.1` or absolute binaries into shared entries — they silently break the other environment. diff --git a/.llm/mcp/sync-mcp.sh b/.llm/mcp/sync-mcp.sh deleted file mode 100755 index 4be09f6..0000000 --- a/.llm/mcp/sync-mcp.sh +++ /dev/null @@ -1,32 +0,0 @@ -#!/usr/bin/env bash -# Sync MCP server configuration into every supported agent frontend. -# Idempotent; safe to run at any time (container hooks and `npm run mcp:sync`). -# -# Usage: -# bash sync-mcp.sh [workspace_dir] [configure.mjs flags...] -# -# A leading-dash first argument (e.g. `--check`) is forwarded to -# configure.mjs instead of being treated as the workspace directory. - -set -euo pipefail - -WORKSPACE_DIR="" -if [ "$#" -gt 0 ] && [ -n "$1" ] && [ "${1#-}" = "$1" ]; then - WORKSPACE_DIR="$1" - shift -fi -# Resolve the configurator path before cd-ing: the workspace argument may be -# any directory, so a script-relative invocation would break after the cd. -script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -if [ -z "${WORKSPACE_DIR}" ]; then - WORKSPACE_DIR="$(cd "${script_dir}/../.." && pwd)" -fi -cd "${WORKSPACE_DIR}" -export DATAVIZ_MCP_WORKSPACE="${WORKSPACE_DIR}" - -if ! command -v node >/dev/null 2>&1; then - echo "❌ node is required to sync MCP configuration." >&2 - exit 1 -fi - -exec node "${script_dir}/configure.mjs" "$@" diff --git a/.llm/mcp/sync_mcp.py b/.llm/mcp/sync_mcp.py new file mode 100644 index 0000000..a2dc3d8 --- /dev/null +++ b/.llm/mcp/sync_mcp.py @@ -0,0 +1,46 @@ +#!/usr/bin/env python3 +"""Sync frontend MCP configs from an optional workspace (issue #171). + +Usage: uv run .llm/mcp/sync_mcp.py [workspace_dir] [configure.mjs flags...] +A leading-dash argument belongs to the configurator, not the workspace. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import sys +from pathlib import Path +from typing import Sequence + + +def main(arguments: Sequence[str]) -> int: + """Run the adjacent Node configurator with the wrapper's workspace contract.""" + args = list(arguments) + # Trust entries use the workspace spelling opened by the agent, so retain + # symlinks in both an explicit workspace and a script-relative default. + script_dir = Path(os.path.abspath(__file__)).parent + workspace = script_dir.parent.parent + if args and args[0] and not args[0].startswith("-"): + workspace = Path(os.path.abspath(args.pop(0))) + node = shutil.which("node") + if node is None: + print("❌ node is required to sync MCP configuration.", file=sys.stderr) + return 1 + env = dict(os.environ) + env["DATAVIZ_MCP_WORKSPACE"] = str(workspace) + try: + return subprocess.run( + [node, str(script_dir / "configure.mjs"), *args], + cwd=workspace, + env=env, + check=False, + ).returncode + except OSError as error: + print(f"❌ Cannot sync MCP configuration: {error}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/tests/test_devcontainer_lifecycle.py b/scripts/tests/test_devcontainer_lifecycle.py index 3633f05..8cb632c 100644 --- a/scripts/tests/test_devcontainer_lifecycle.py +++ b/scripts/tests/test_devcontainer_lifecycle.py @@ -77,7 +77,7 @@ def __init__(self) -> None: directory.mkdir(parents=True) self.marker = self.root / "marker.log" shutil.copy(LIFECYCLE_DIR / "env-local.sh", self.ws / ".devcontainer" / "env-local.sh") - for relative in (".devcontainer/install_npm_tools.py", ".devcontainer/ai_backends.py", ".llm/mcp/sync-mcp.sh"): + for relative in (".devcontainer/install_npm_tools.py", ".devcontainer/ai_backends.py", ".llm/mcp/sync_mcp.py"): self.write_stub(self.ws / relative, STUB_RECORD) self.write_stub(self.stubs / "node", STUB_RECORD) # Credential-shaped variables are scrubbed from the child environment: @@ -222,7 +222,7 @@ def case_on_create_bootstraps_and_is_idempotent() -> None: == [ "STUB install_npm_tools.py| --offline-ok", "STUB ai_backends.py| install", - f"STUB sync-mcp.sh| {sandbox.ws}", + f"STUB sync_mcp.py| {sandbox.ws}", ], f"on_create should call the three children with the documented arguments, got {stub_calls(sandbox.marker_lines())}", ) @@ -250,7 +250,7 @@ def case_on_create_tolerates_npm_failure_only() -> None: "⚠️ npm tool refresh failed at create time; image-baked versions remain installed." in output, f"the npm failure should warn, got:\n{output}", ) - assert_true("STUB sync-mcp.sh" in "\n".join(sandbox.marker_lines()), "the sync should still run after the npm warning") + assert_true("STUB sync_mcp.py" in "\n".join(sandbox.marker_lines()), "the sync should still run after the npm warning") sandbox.write_stub(sandbox.ws / ".devcontainer/ai_backends.py", FAILING_STUB) code, output = sandbox.run("on_create.py") @@ -326,7 +326,7 @@ def case_post_start_replaces_block_and_loads_env() -> None: f"the npm and backend installs run before the load, got {calls}", ) assert_true("STUB node| .llm/mcp/mcp-doctor.mjs --repair" in calls[2], f"the doctor runs before the sync, got {calls}") - assert_true(calls[3].startswith("STUB sync-mcp.sh|"), f"the sync runs after the doctor, got {calls}") + assert_true(calls[3].startswith("STUB sync_mcp.py|"), f"the sync runs after the doctor, got {calls}") assert_true("GITHUB_PERSONAL_ACCESS_TOKEN=stale-value;" in envs[0], f"children before the load see the exported value, got {envs[0]}") assert_true("GITHUB_PERSONAL_ACCESS_TOKEN=file-value;" in envs[3], f"children after the load see the file value, got {envs[3]}") print(output) @@ -340,7 +340,7 @@ def case_update_content_restores_and_syncs_only_when_present() -> None: code, output = sandbox.run("update_content.py") assert_exit_code(0, "update_content should succeed in a sandbox", code) assert_true( - stub_calls(sandbox.marker_lines()) == [f"STUB sync-mcp.sh| {sandbox.ws}"], + stub_calls(sandbox.marker_lines()) == [f"STUB sync_mcp.py| {sandbox.ws}"], f"update_content should only sync without the dotnet manifest, got {stub_calls(sandbox.marker_lines())}", ) @@ -353,7 +353,7 @@ def case_update_content_restores_and_syncs_only_when_present() -> None: lines = sandbox.marker_lines() assert_true(len(lines) == 2, f"update_content should restore and then sync, got {lines}") assert_true(lines[0] == "dotnet tool restore attempt 1", f"the restore should be recorded first, got {lines}") - assert_true(lines[1].startswith(f"STUB sync-mcp.sh| {sandbox.ws} |ENV:"), f"the sync should be recorded second, got {lines}") + assert_true(lines[1].startswith(f"STUB sync_mcp.py| {sandbox.ws} |ENV:"), f"the sync should be recorded second, got {lines}") sandbox.env["PATH"] = "/bin" sandbox.marker.unlink() diff --git a/scripts/tests/test_mcp_credentials.py b/scripts/tests/test_mcp_credentials.py index d22dfa2..3f6014f 100644 --- a/scripts/tests/test_mcp_credentials.py +++ b/scripts/tests/test_mcp_credentials.py @@ -38,7 +38,7 @@ TESTS_DIR = Path(__file__).resolve().parent REPO_ROOT = TESTS_DIR.parent.parent -SYNC = REPO_ROOT / ".llm" / "mcp" / "sync-mcp.sh" +SYNC = REPO_ROOT / ".llm" / "mcp" / "sync_mcp.py" DOCTOR = REPO_ROOT / ".llm" / "mcp" / "mcp-doctor.mjs" CONFIGURE = REPO_ROOT / ".llm" / "mcp" / "configure.mjs" diff --git a/scripts/tests/test_mcp_sync.py b/scripts/tests/test_mcp_sync.py index c12b414..2adf2a6 100644 --- a/scripts/tests/test_mcp_sync.py +++ b/scripts/tests/test_mcp_sync.py @@ -1,17 +1,16 @@ #!/usr/bin/env python3 """mcp-sync contract tests (typed Python port of ``test-mcp-sync.sh``, issue #171). -Every case from the bash original is kept with the same assertions; only -the driver language changed. The subject stays the sync entry -(``sync-mcp.sh`` driving ``configure.mjs``), so each case still syncs into +Every case from the bash original is kept with the same assertions. +The Python sync entry drives the unchanged Node configurator +(``sync_mcp.py`` driving ``configure.mjs``), so each case still syncs into a throwaway workspace with throwaway HOME and CODEX_HOME, and asserts that every supported agent frontend receives the same server fleet with placeholder-only credentials (no secrets on disk). The bash suite's ``jq`` probes become stdlib ``json`` reads, which also removes the jq dependency the old suite silently required. -Cases skip when bash or node is unavailable, like the previous wrapper's -bash guard. +Integration cases require node; the wrapper itself requires only Python. """ from __future__ import annotations @@ -23,7 +22,7 @@ import subprocess import sys from pathlib import Path -from typing import Any, Dict, List, Optional +from typing import Any, List, Tuple sys.dont_write_bytecode = True @@ -31,7 +30,7 @@ TESTS_DIR = Path(__file__).resolve().parent REPO_ROOT = TESTS_DIR.parent.parent -SYNC = REPO_ROOT / ".llm" / "mcp" / "sync-mcp.sh" +SYNC = REPO_ROOT / ".llm" / "mcp" / "sync_mcp.py" # The unity bridge URL port is configurable: unity-mcp-host.mjs binds a dynamic # port when 9020 is busy and persists UNITY_MCP_HTTP_PORT to .env.local. Export @@ -41,15 +40,6 @@ SCRUBBED_KEYS = ("GITHUB_PERSONAL_ACCESS_TOKEN", "GITHUB_MCP_PAT", "GITHUB_TOKEN", "ZAI_API_KEY", "Z_AI_API_KEY") -def bash_path(path: Path) -> str: - """Render a path bash accepts on every host. - - Windows-created fixture paths reach Git Bash through MSYS, which reads - drive-letter paths with forward slashes; the forward form is used - everywhere (a no-op on POSIX). - """ - return str(path).replace("\\", "/") - LEGACY_OPENCODE_JSON = """{ "$schema": "https://opencode.ai/config.json", "theme": "system", @@ -79,8 +69,6 @@ def __init__(self) -> None: self.stderr = "" def run_sync(self) -> None: - bash = shutil.which("bash") - assert_true(bash is not None, "bash must be on PATH; the sync subject is a bash script") env = dict(os.environ) for key in SCRUBBED_KEYS: env.pop(key, None) @@ -98,10 +86,7 @@ def run_sync(self) -> None: # names pointing inside the sandbox. env["USERPROFILE"] = str(self.home) completed = subprocess.run( - # Resolve bash through PATH explicitly: on Windows a bare "bash" - # in CreateProcess's search order hits System32's WSL launcher - # before any PATH entry, and WSL without a distro fails the sync. - [bash, bash_path(SYNC), bash_path(self.root)], + [sys.executable, str(SYNC), str(self.root)], cwd=str(self.root), env=env, capture_output=True, @@ -306,13 +291,86 @@ def case_generated_configs_contain_no_literal_secrets() -> None: ) +def case_wrapper_forwards_workspace_flags_and_exit_status() -> None: + root = new_temp_root(prefix="mcp sync wrapper-") + try: + script_dir = root / ".llm" / "mcp" + script_dir.mkdir(parents=True) + wrapper = script_dir / SYNC.name + shutil.copyfile(SYNC, wrapper) + (script_dir / "configure.mjs").write_text( + "console.log(JSON.stringify({args:process.argv.slice(2),cwd:process.cwd()," + "workspace:process.env.DATAVIZ_MCP_WORKSPACE}));process.exit(7);\n", + encoding="utf-8", + ) + other = root / "other workspace" + other.mkdir() + cases: List[Tuple[Path, List[str], Path, Path, List[str]]] = [ + (wrapper, [], other, root, []), + (wrapper, ["--check"], other, root, ["--check"]), + (wrapper, ["other workspace", "--check", "two words"], root, other, ["--check", "two words"]), + (wrapper, [""], other, root, [""]), + ] + if os.name == "posix": + alias = root / "workspace alias" + alias.symlink_to(root, target_is_directory=True) + cases.extend([ + (wrapper, [str(alias), "--check"], other, alias, ["--check"]), + (alias / ".llm" / "mcp" / SYNC.name, ["--check"], other, alias, ["--check"]), + ]) + else: + print(" SKIP: workspace alias cases require a POSIX symlink fixture.") + for entry, arguments, working_directory, expected_workspace, flags in cases: + completed = subprocess.run( + [sys.executable, str(entry), *arguments], + cwd=working_directory, + env={**os.environ, "DATAVIZ_MCP_WORKSPACE": "ignored inherited workspace"}, + capture_output=True, + text=True, + encoding="utf-8", + check=False, + ) + assert_eq(f"wrapper exit for {arguments}", 7, completed.returncode, completed.stderr) + record = json.loads(completed.stdout) + assert_eq("configurator flags", flags, record["args"]) + assert_eq("configurator cwd", expected_workspace.resolve(), Path(record["cwd"]).resolve()) + assert_eq("workspace environment preserves logical path", os.path.abspath(expected_workspace), record["workspace"]) + failed = subprocess.run( + [sys.executable, str(wrapper), str(root / "missing workspace")], + capture_output=True, + text=True, + encoding="utf-8", + check=False, + ) + assert_eq("missing workspace exits one", 1, failed.returncode) + assert_true("Cannot sync MCP configuration:" in failed.stderr, failed.stderr) + assert_true("Traceback" not in failed.stderr, failed.stderr) + finally: + remove_temp_root(root) + + +def case_wrapper_fails_without_node() -> None: + completed = subprocess.run( + [sys.executable, str(SYNC), "--check"], + env={**os.environ, "PATH": ""}, + capture_output=True, + text=True, + encoding="utf-8", + check=False, + ) + assert_eq("missing node exits one", 1, completed.returncode) + assert_true("node is required to sync MCP configuration." in completed.stderr, completed.stderr) + + def main() -> int: assert_true(SYNC.is_file(), f"sync script not found at {SYNC}") print("== mcp-sync contract tests ==") - missing = shutil.which("bash") is None or shutil.which("node") is None + run_case("case_wrapper_fails_without_node", case_wrapper_fails_without_node) + missing = shutil.which("node") is None if missing: - print(" SKIP: bash and node are both required to drive the sync entry.") - return 0 + print(" SKIP: node is required to drive the configurator.") + return 1 if failure_count() else 0 + run_case("case_wrapper_forwards_workspace_flags_and_exit_status", case_wrapper_forwards_workspace_flags_and_exit_status) for case in CASES: run_case(case.__name__, case) failures = failure_count()