From 912954466cb2e6f317a4f70ffe522bd976c2ce96 Mon Sep 17 00:00:00 2001 From: wallstop Date: Wed, 14 Oct 2026 17:37:45 +0000 Subject: [PATCH 1/2] MCP sync remove the standalone shell wrapper The script migration still leaves lifecycle MCP sync in Bash. The wrapper is executed, not sourced, so the earlier sourced-shell exception does not apply to it. Port the wrapper to typed, stdlib-only Python and call it through each lifecycle entry point's current interpreter. Keep the adjacent Node configurator and npm's direct Node entry unchanged. Preserve workspace selection, leading-dash flag forwarding, child cwd and environment, missing-node diagnostics, and child exit status. All five original sync integration cases and assertions are unchanged. Add wrapper checks for default roots from another cwd, relative paths with spaces, multiword flags, an empty first argument, exit propagation, missing Node, and a missing workspace. The sync suite no longer skips merely because Bash is absent. Update lifecycle stub expectations. Validation: all 38 self-test files pass; scoped fast check and all ten applicable gate steps pass. Strict pyright reports zero errors on the wrapper, three lifecycle files, and three affected test files. No old sync-mcp.sh references remain in tracked tooling. No Unity behavior or packaged asset changes; Unity suites and npm pack do not apply. Refs #171. --- .devcontainer/on_create.py | 2 +- .devcontainer/post_start.py | 2 +- .devcontainer/update_content.py | 2 +- .llm/mcp/README.md | 4 +- .llm/mcp/sync-mcp.sh | 32 ------- .llm/mcp/sync_mcp.py | 44 +++++++++ scripts/tests/test_devcontainer_lifecycle.py | 12 +-- scripts/tests/test_mcp_credentials.py | 2 +- scripts/tests/test_mcp_sync.py | 98 +++++++++++++++----- 9 files changed, 129 insertions(+), 69 deletions(-) delete mode 100755 .llm/mcp/sync-mcp.sh create mode 100644 .llm/mcp/sync_mcp.py diff --git a/.devcontainer/on_create.py b/.devcontainer/on_create.py index a6ffb16f..8ac9fc1e 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 a39d3f4b..ae69092d 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 e92fa123..393ee019 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 8a7d0763..8f8d8a0b 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 4be09f6f..00000000 --- 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 00000000..dc73a605 --- /dev/null +++ b/.llm/mcp/sync_mcp.py @@ -0,0 +1,44 @@ +#!/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) + script_dir = Path(__file__).resolve().parent + workspace = script_dir.parent.parent + if args and args[0] and not args[0].startswith("-"): + workspace = Path(args.pop(0)).resolve() + 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 3633f05b..8cb632c9 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 d22dfa25..3f6014fa 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 c12b4145..c9e68998 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 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,76 @@ 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() + for arguments, working_directory, expected_workspace, flags in ( + ([], other, root, []), + (["--check"], other, root, ["--check"]), + (["other workspace", "--check", "two words"], root, other, ["--check", "two words"]), + ([""], other, root, [""]), + ): + completed = subprocess.run( + [sys.executable, str(wrapper), *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", expected_workspace.resolve(), Path(record["workspace"]).resolve()) + 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() From 7d9eda0983f15a7ee78b08068664bd5308eb3e9f Mon Sep 17 00:00:00 2001 From: wallstop Date: Wed, 14 Oct 2026 19:06:53 +0000 Subject: [PATCH 2/2] MCP sync preserve workspace symlink identity Agent trust and approval entries use the workspace path as a key. The Python wrapper resolved symlinks before exporting that path, unlike the old shell wrapper, so opening an alias could lose its trust identity. Use lexical absolute paths for both explicit workspaces and the default script-relative workspace. Extend the existing wrapper table to cover both alias paths on POSIX. Compare the exported path verbatim instead of resolving both sides, which hid this regression in the old assertion. Windows retains its existing path/argument coverage and explicitly skips the POSIX symlink fixture. Validation: the added alias case fails against the old wrapper and passes with this change. Strict pyright and check:fast pass. The combined stack gate passes all ten applicable steps, including the full 38-file suite. No Unity or packaged payload changes. Resolve the finding in PR #237 discussion_r4275057123. Refs #171. --- .llm/mcp/sync_mcp.py | 6 ++++-- scripts/tests/test_mcp_sync.py | 28 +++++++++++++++++++--------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/.llm/mcp/sync_mcp.py b/.llm/mcp/sync_mcp.py index dc73a605..a2dc3d8b 100644 --- a/.llm/mcp/sync_mcp.py +++ b/.llm/mcp/sync_mcp.py @@ -18,10 +18,12 @@ def main(arguments: Sequence[str]) -> int: """Run the adjacent Node configurator with the wrapper's workspace contract.""" args = list(arguments) - script_dir = Path(__file__).resolve().parent + # 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(args.pop(0)).resolve() + 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) diff --git a/scripts/tests/test_mcp_sync.py b/scripts/tests/test_mcp_sync.py index c9e68998..2adf2a62 100644 --- a/scripts/tests/test_mcp_sync.py +++ b/scripts/tests/test_mcp_sync.py @@ -22,7 +22,7 @@ import subprocess import sys from pathlib import Path -from typing import Any, List +from typing import Any, List, Tuple sys.dont_write_bytecode = True @@ -305,14 +305,24 @@ def case_wrapper_forwards_workspace_flags_and_exit_status() -> None: ) other = root / "other workspace" other.mkdir() - for arguments, working_directory, expected_workspace, flags in ( - ([], other, root, []), - (["--check"], other, root, ["--check"]), - (["other workspace", "--check", "two words"], root, other, ["--check", "two words"]), - ([""], other, root, [""]), - ): + 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(wrapper), *arguments], + [sys.executable, str(entry), *arguments], cwd=working_directory, env={**os.environ, "DATAVIZ_MCP_WORKSPACE": "ignored inherited workspace"}, capture_output=True, @@ -324,7 +334,7 @@ def case_wrapper_forwards_workspace_flags_and_exit_status() -> None: 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", expected_workspace.resolve(), Path(record["workspace"]).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,