diff --git a/CLAUDE.md b/CLAUDE.md index 5feea99c..9fe87c0c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -248,7 +248,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph ⚠️ **llama-swap endpoints** (one model at a time): the apply routes check `GET /running` and return `requiresConfirmation` before evicting a model another live session uses; `confirmedSwap` and `confirmedContext` are SEPARATE flags and must stay so. Claude alone gets a context floor (`CLAUDE_MIN_SAFE_CONTEXT_TOKENS`); context is parsed from `/running`'s `cmd`, never trusted from `/props`. Backend log lines come from llama-swap's `/api/events` `upstream` source, never `/logs`. → [architecture-invariants#custom-model-endpoint-profiles](docs/architecture-invariants.md#custom-model-endpoint-profiles) -**MCP server sync** (opt-in, `mcpSyncEnabled`, SYNCED, default OFF; `src/mcp-sync.ts`, `GET`/`POST /api/mcp-sync`): copies each installed, enabled CLI's user-level MCP servers into the others. It is the ONE subsystem that writes another CLI's REAL user config (`~/.claude.json`, `~/.codex/config.toml`, `~/.gemini/*`, opencode's), which is why it is opt-in and admin-only in multi-user mode (both verbs 403 for a non-admin, and the Settings group is hidden for them). Where each CLI keeps the file is registry data, `capabilities.mcpConfig` (`{ path, format, relocation? }`), never a branch on the id. ⚠️ ADDITIVE only: a name already defined, in any shape, is never edited or removed (a different same-name definition is a reported conflict), and a server switched off in its own CLI is never copied. ⚠️ Never write a file that did not parse; re-parse the NEW text and require every added server to read back before the tmp+rename (written through a symlink, previous file kept as `.codeman-bak`, one apply at a time, else 409). ⚠️ A file that receives copied `env`/`headers` (secrets) is left `0600`, and so is the backup. ⚠️ Responses carry server NAMES only, never env values, headers or file text: a parse failure is reported by line and column (`describeMcpSyncError`), never the parser's own message (smol-toml and V8 both quote source). ⚠️ `mcpConfig.relocation` names the env var the CLI reads to move its file (`CLAUDE_CONFIG_DIR`, `CODEX_HOME`, `XDG_CONFIG_HOME`, `GEMINI_CLI_HOME`), resolved from the SERVER env at call time; a relative value reports the target `skipped`, never a guessed write, and a per-session `envOverrides` relocation is not followed. Tests must pass `home` (which drops the `process.env` default) or clear those vars first. → `docs/cli-registry.md` (MCP server sync), `docs/api-reference.md`, `docs/wiki/Settings-Reference.md` +**MCP server sync** (opt-in, `mcpSyncEnabled`, SYNCED, default OFF; `src/mcp-sync.ts`, `GET`/`POST /api/mcp-sync`): copies each installed, enabled CLI's user-level MCP servers into the others. It is the ONE subsystem that writes another CLI's REAL user config (`~/.claude.json`, `~/.codex/config.toml`, `~/.gemini/*`, opencode's), which is why it is opt-in and admin-only in multi-user mode (both verbs 403 for a non-admin, and the Settings group is hidden for them). Where each CLI keeps the file is registry data, `capabilities.mcpConfig` (`{ path, format, relocation? }`), never a branch on the id. ⚠️ Tools that sync but are not run modes (GitHub Copilot CLI, the first) are plain data in `MCP_SYNC_ONLY_TOOLS` (`src/mcp-sync-targets.ts`), take part only when installed or already configured, and their companion `settings.json` (`disabledMcpServers`) is read, never written; a test must clear their relocation vars too. ⚠️ ADDITIVE only: a name already defined, in any shape, is never edited or removed (a different same-name definition is a reported conflict), and a server switched off in its own CLI is never copied. ⚠️ Never write a file that did not parse; re-parse the NEW text and require every added server to read back before the tmp+rename (written through a symlink, previous file kept as `.codeman-bak`, one apply at a time, else 409). ⚠️ A file that receives copied `env`/`headers` (secrets) is left `0600`, and so is the backup. ⚠️ Responses carry server NAMES only, never env values, headers or file text: a parse failure is reported by line and column (`describeMcpSyncError`), never the parser's own message (smol-toml and V8 both quote source). ⚠️ `mcpConfig.relocation` names the env var the CLI reads to move its file (`CLAUDE_CONFIG_DIR`, `CODEX_HOME`, `XDG_CONFIG_HOME`, `GEMINI_CLI_HOME`, and `COPILOT_HOME` for Copilot), resolved from the SERVER env at call time; a relative value reports the target `skipped`, never a guessed write, and a per-session `envOverrides` relocation is not followed. Tests must pass `home` (which drops the `process.env` default) or clear those vars first. → `docs/cli-registry.md` (MCP server sync), `docs/api-reference.md`, `docs/wiki/Settings-Reference.md` **Run launch synchronization**: the Run entrypoint holds an in-flight lock and disables `#runBtn` for the whole launch (≥500ms) so a double click cannot create duplicate `w-` sessions; `_ensureCreatedSessionVisible()` runs before `selectSession()` and `_onSessionCreated()` stays an idempotent upsert, so POST-first and SSE-first both render exactly one tab. ⚠️ **Closing has the mirror-image race**: `closeSession()` must read `wasActive` BEFORE its `await` and announce the delete via `_closingSessions`, and `_onSessionDeleted` skips the active-session handoff for ids in that set; never read `activeSessionId` after the fact. The fallback picks the first `sessionOrder` entry still in `sessions`. Tests: `test/session-close-fallback.test.ts`. → [architecture-invariants#run-launch-synchronization](docs/architecture-invariants.md#run-launch-synchronization) diff --git a/docs/api-reference.md b/docs/api-reference.md index 5133ce5d..0a785d11 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -836,7 +836,7 @@ Result (`data`): - `targets[]` — one per enabled CLI that declares an MCP config, plus GitHub Copilot CLI (`id: "copilot"`, a sync-only target that is not a run mode): `id`, `label`, `file`, `status`, `error?`, `servers` (names it already has), `added` (names added, or that would be), `skipped` (names its dialect cannot express, e.g. SSE for Codex and Antigravity). - `status`: `ok`; `absent` (not installed and no config file, so not read or created); `skipped` (the CLI's relocation env var, e.g. `CODEX_HOME`, is set to a relative path in the server's environment, so its file cannot be located safely and is neither read nor written); `unreadable` (the file exists but cannot be parsed safely, so it is not written); `failed` (a read or write error, the file may be unchanged). - `error` says why a target is not `ok`. A parse failure is reported by position only (`not valid TOML (line 3, column 21)`, `not valid JSON`), never with text from the file. - - `file` honours each CLI's own relocation env var as the server process sees it (`CLAUDE_CONFIG_DIR`, `CODEX_HOME`, `XDG_CONFIG_HOME`, `GEMINI_CLI_HOME`); see `docs/cli-registry.md`. + - `file` honours each CLI's own relocation env var as the server process sees it (`CLAUDE_CONFIG_DIR`, `CODEX_HOME`, `XDG_CONFIG_HOME`, `GEMINI_CLI_HOME`, and `COPILOT_HOME` for the sync-only Copilot CLI); see `docs/cli-registry.md`. - `conflicts[]` — names defined differently by different CLIs. Existing definitions are kept; the first CLI's is copied where the name is missing. - `disabled[]` — names left out because every definition is switched off in its own CLI (codex `enabled = false`, opencode `enabled: false`, antigravity `disabled: true`). - `unsupported[]` — labels of enabled agent CLIs with no known MCP config file (nothing is guessed). diff --git a/docs/cli-registry.md b/docs/cli-registry.md index 99310a1e..247e6a32 100644 --- a/docs/cli-registry.md +++ b/docs/cli-registry.md @@ -269,7 +269,7 @@ A module-level const freezes at first import, and the failure is asymmetric: a C **Tools that are not run modes.** GitHub Copilot CLI keeps an MCP list worth syncing but Codeman does not launch it, so it has no registry entry. `src/mcp-sync-targets.ts` declares such tools as plain data (`MCP_SYNC_ONLY_TOOLS`: id, label, config path, dialect, relocation var, the binary whose presence means "installed"). They join the registry CLIs as sync targets (listed after them, so a registry CLI's definition wins a same-name difference), under the same rules: installed or already configured, otherwise `absent`. Copilot's dialect is `copilot-json` (`~/.copilot/mcp-config.json`, relocated by `COPILOT_HOME`; checked against `copilot mcp add` 1.0.94): `mcpServers`, each entry with `tools` (`["*"]` = all), `type` `local` | `http` | `sse`, `command`/`args`/`env` or `url`/`headers`. `copilot mcp disable` does not mark the entry: it lists the name under `disabledMcpServers` in `settings.json` beside the config. Sync reads that list (never writes it) so a disabled server is not copied, and reports the target `unreadable` if `settings.json` is not valid JSON rather than guessing. -`relocation` (`{ envVar, path }`) names the env var the CLI itself reads to move that file: claude `CLAUDE_CONFIG_DIR` (`.claude.json` under it), codex `CODEX_HOME` (`config.toml`), opencode `XDG_CONFIG_HOME` (`opencode/opencode.json`) gemini `GEMINI_CLI_HOME` (`.gemini/settings.json`) and, for the sync-only Copilot CLI, `COPILOT_HOME` (`mcp-config.json`); antigravity follows `$HOME` only, so it declares none. The var is read from the SERVER process env at call time, which is the env the CLIs Codeman spawns inherit. An absolute value moves the file to `/`, an empty one counts as unset (as it does for each CLI), and anything else reports the target `skipped` with the reason instead of writing a file the CLI never reads. A per-session relocation (a session's own `CLAUDE_CONFIG_DIR` in `envOverrides`) is not followed: the sync only knows the server's environment. +`relocation` (`{ envVar, path }`) names the env var the CLI itself reads to move that file: claude `CLAUDE_CONFIG_DIR` (`.claude.json` under it), codex `CODEX_HOME` (`config.toml`), opencode `XDG_CONFIG_HOME` (`opencode/opencode.json`), gemini `GEMINI_CLI_HOME` (`.gemini/settings.json`) and, for the sync-only Copilot CLI, `COPILOT_HOME` (`mcp-config.json`); antigravity follows `$HOME` only, so it declares none. The var is read from the SERVER process env at call time, which is the env the CLIs Codeman spawns inherit. An absolute value moves the file to `/`, an empty one counts as unset (as it does for each CLI), and anything else reports the target `skipped` with the reason instead of writing a file the CLI never reads. A per-session relocation (a session's own `CLAUDE_CONFIG_DIR` in `envOverrides`) is not followed: the sync only knows the server's environment. The rules the module keeps and the tests pin: it only ADDS (a name already defined, in any shape, is never edited or removed; a same-name difference is reported as a conflict); a server switched off in its own CLI is not copied; it never writes a file it could not parse (opencode JSONC with comments, a TOML file with a duplicate table) and re-parses the new text before writing; codex TOML is read with a real parser (`smol-toml`), so CRLF files and inline tables are handled; names such as `__proto__` are ignored and every table keyed by an untrusted name has no prototype; a symlinked config is written through, not replaced; a file that receives `env`/`headers` is left `0600`; only one apply runs at a time; and its result carries server names only, never env values or headers, and never file text: a parse failure is reported by line and column, not by the parser's message (smol-toml prints a code frame of the offending lines and V8's JSON errors quote source, either of which can hold a secret). The schema restricts `path` and `relocation.path` to a relative path without `..`, since sync writes to it. diff --git a/src/mcp-sync.ts b/src/mcp-sync.ts index 14a9f67a..de9ad0a8 100644 --- a/src/mcp-sync.ts +++ b/src/mcp-sync.ts @@ -12,7 +12,8 @@ * does not understand) is never rewritten and nothing is ever removed. Same name with a * different definition is reported as a conflict and left alone. * - A server the user has switched off in its own CLI (codex `enabled = false`, opencode - * `enabled: false`, antigravity `disabled: true`) is not propagated: copying it would + * `enabled: false`, antigravity `disabled: true`, Copilot's `disabledMcpServers` in its + * `settings.json`) is not propagated: copying it would * switch it on in every other CLI. * - A file that does not parse (e.g. opencode JSONC with comments, a TOML file with a * duplicate table) is never written, and a write is only made after the NEW text has been diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index d8dab19a..1793adbe 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -1235,7 +1235,7 @@ Object.assign(CodemanApp.prototype, { show('Apply or Save settings to turn MCP sync on first, then preview or sync.', 'save-first'); return; } - if (apply && !confirm('Add missing MCP servers to every installed, enabled CLI\'s config file? Env values and headers on those servers are copied too.')) return; + if (apply && !confirm('Add missing MCP servers to every installed, enabled CLI\'s config file (and GitHub Copilot CLI\'s, when it is installed)? Env values and headers on those servers are copied too.')) return; show('Working…'); const res = apply ? await this._apiPost('/api/mcp-sync', {}) : await this._api('/api/mcp-sync'); let body = null; diff --git a/test/routes/mcp-sync-routes.test.ts b/test/routes/mcp-sync-routes.test.ts index e38f07b8..59570c7f 100644 --- a/test/routes/mcp-sync-routes.test.ts +++ b/test/routes/mcp-sync-routes.test.ts @@ -16,6 +16,7 @@ import { registerMcpSyncRoutes } from '../../src/web/routes/mcp-sync-routes.js'; import { SETTINGS_PATH } from '../../src/web/route-helpers.js'; import { registryFilePath, reloadCliRegistry } from '../../src/config/cli-registry/registry.js'; import { STOCK_CLIS } from '../../src/config/cli-registry/stock.js'; +import { MCP_SYNC_ONLY_TOOLS } from '../../src/mcp-sync-targets.js'; // Which CLIs are installed on the machine running the tests must not decide the outcome: nothing // is installed, so only a CLI whose config file exists takes part. @@ -43,14 +44,26 @@ vi.mock('../../src/utils/cli-installed-probes.js', () => ({ probeStockCliAvailability: async () => ({}), isCliEntryInstalled: (e: { id: string }) => installed.has(e.id), })); +// The sync-only tools (Copilot) probe the real PATH for their binary; route that through the same set. +vi.mock('../../src/mcp-sync-targets.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + mcpSyncOnlyTargets: (taken: ReadonlySet) => + actual.mcpSyncOnlyTargets(taken, (binary) => installed.has(binary)), + }; +}); // The route follows each CLI's relocation env var (CODEX_HOME, CLAUDE_CONFIG_DIR, XDG_CONFIG_HOME, // ...) from process.env, so the runner's own values (CI images set XDG_CONFIG_HOME) must never // aim a test write outside the temp HOME. Cleared before every test, restored after the file. -const RELOCATION_VARS = STOCK_CLIS.flatMap((e) => { - const envVar = e.capabilities.mcpConfig?.relocation?.envVar; - return envVar ? [envVar] : []; -}); +const RELOCATION_VARS = [ + ...STOCK_CLIS.flatMap((e) => { + const envVar = e.capabilities.mcpConfig?.relocation?.envVar; + return envVar ? [envVar] : []; + }), + ...MCP_SYNC_ONLY_TOOLS.flatMap((t) => (t.relocation ? [t.relocation.envVar] : [])), +]; const savedEnv = Object.fromEntries(RELOCATION_VARS.map((k) => [k, process.env[k]])); afterAll(() => { for (const [k, v] of Object.entries(savedEnv)) {