diff --git a/server/ws/groupManager.js b/server/ws/groupManager.js index 9ecab55..073e1b6 100644 --- a/server/ws/groupManager.js +++ b/server/ws/groupManager.js @@ -161,6 +161,15 @@ export function getGroup(groupId) { return groups.get(groupId) || null; } +// Facade-only lookup for the MCP layer: repo_info needs the group's project +// directory (and nothing else), so the facade exposes just the cwd instead of +// the raw group object (which carries controlBroker socket paths, handoff +// channels, handoffQueue, allowedCwds -- internals an LLM-facing tool must not +// reach). +export function getGroupCwd(groupId) { + return groups.get(groupId)?.cwd ?? null; +} + // Declare a group fully assembled (all initial members spawned): from here on // the "no live members" auto-destroy in onSessionExit applies. Called by the // POST /groups handler after the last member is registered. No-op for groups @@ -750,6 +759,7 @@ const groupManagerApi = { listGroupMembers, isSessionInGroup, getRoleForSession, + getGroupCwd, setCurrentTurn, pushHandoff, takeHandoff, diff --git a/server/ws/mcpConfig.js b/server/ws/mcpConfig.js index 615345d..d8fff11 100644 --- a/server/ws/mcpConfig.js +++ b/server/ws/mcpConfig.js @@ -16,6 +16,10 @@ // sessions in the same cwd cannot collide). // opencode -> OPENCODE_CONFIG_CONTENT env var (deep-merged with project // config, no file written). +// copilot -> nothing. copilot has no CLI-arg/env MCP injection (its config +// is file-based only), so `buildMcpConfigArgsAndEnv` never +// assembles an injection for it -- passing `--mcp-config` would +// make the binary error out with "unknown option". // // The optional `{ notify }` descriptor adds the ccserver-notify MCP server: // { mode, sockPath, identity? } @@ -58,6 +62,15 @@ export function buildMcpConfigArgsAndEnv(app, { groupMcp = true, notify } = {}) const notifySockEnv = notify ? { CCSANDBOX_NOTIFY_MCP_SOCK: notify.sockPath } : {}; const notifyIdentityEnv = notify?.identity ? { CCSERVER_NOTIFY_IDENTITY: JSON.stringify(notify.identity) } : {}; + if (app === 'copilot') { + // No CLI-arg/env MCP injection exists for copilot: assembling one would + // reach the binary as `--mcp-config` and die with "unknown option". The + // function is the single assembly point, so refusing here guarantees no + // copilot launch path ever injects (group launches already refuse copilot + // at open_tab / addMember). + return { args: [], env: {} }; + } + if (app === 'opencode') { const mcp = {}; if (groupMcp) mcp.ccserver = { type: 'local', command: [MCP_BRIDGE_COMMAND] }; diff --git a/server/ws/mcpConfig.test.js b/server/ws/mcpConfig.test.js index 3f4dabf..07f7b2f 100644 --- a/server/ws/mcpConfig.test.js +++ b/server/ws/mcpConfig.test.js @@ -35,6 +35,33 @@ test('unknown app falls back to the claude-style CLI arg (default branch)', () = assert.ok(JSON.parse(args[1]).mcpServers.ccserver); }); +// copilot has no CLI-arg/env MCP injection (file-based config only), so the +// assembly point must produce nothing for it -- otherwise the `--mcp-config` +// flag reaches the binary and it errors with "unknown option". +test('copilot gets no injection at all: empty args and env, even with groupMcp', () => { + const { args, env } = buildMcpConfigArgsAndEnv('copilot'); + assert.deepEqual(args, [], 'no CLI args (--mcp-config must never appear)'); + assert.deepEqual(env, {}, 'no env injection'); + assert.ok(!args.join(' ').includes('--mcp-config')); +}); + +test('copilot + notify(sandbox): nothing is assembled for either server', () => { + const { args, env } = buildMcpConfigArgsAndEnv('copilot', { + groupMcp: true, + notify: { mode: 'sandbox', sockPath: '/run/user/1000/ccserver-notify.sock', identity }, + }); + assert.deepEqual(args, [], 'no CLI args (--mcp-config must never appear)'); + assert.deepEqual(env, {}, 'no env injection'); +}); + +test('copilot + notify(host): nothing is assembled for either server', () => { + const { args, env } = buildMcpConfigArgsAndEnv('copilot', { + notify: { mode: 'host', sockPath: '/run/user/1000/ccserver-notify.sock', identity }, + }); + assert.deepEqual(args, [], 'no CLI args (--mcp-config must never appear)'); + assert.deepEqual(env, {}, 'no env injection'); +}); + // ccserver-notify injection (see notify.js): the optional `{ notify }` // descriptor adds the notify server to the same registration, with the bridge // command switching on the session's sandbox mode. sessionManager always diff --git a/server/ws/mcpTools.js b/server/ws/mcpTools.js index b14cd1e..dbd9a63 100644 --- a/server/ws/mcpTools.js +++ b/server/ws/mcpTools.js @@ -185,8 +185,9 @@ export function handoffToOrchestrator(deps, { summary, status = 'done', nextRole // --- repo_info ------------------------------------------------------------- // Shallow repository facts for the orchestrator (control server only). // Security/cost posture, mirroring read_output: -// - cwd is the group's project directory (group.cwd) -- never taken from -// the wire, so there is no path argument to traverse with. +// - cwd is the group's project directory -- obtained through the group +// facade's getGroupCwd(groupId), never from the wire, so there is no path +// argument to traverse with. // - read-only: no writes; the only command execution is the fixed git // invocations below (`git -C ` with a whitelisted argument list, // never caller input). @@ -290,11 +291,10 @@ async function gitState(cwd) { } export async function repoInfo(deps) { - const group = deps.groupManager.getGroup(deps.groupId); - if (!group) { + const cwd = deps.groupManager.getGroupCwd(deps.groupId); + if (!cwd) { return { error: 'group-not-found', message: 'group not found' }; } - const cwd = group.cwd; return { cwd, root: await rootListing(cwd), diff --git a/server/ws/mcpTools.test.js b/server/ws/mcpTools.test.js index 001f753..1921354 100644 --- a/server/ws/mcpTools.test.js +++ b/server/ws/mcpTools.test.js @@ -62,6 +62,19 @@ function controlDeps(groupId) { }; } +// The production deps shape for broker servers: groupManager arrives as the +// narrow groupManagerApi facade (groupManager.js), which deliberately exposes +// getGroupCwd but NOT the raw getGroup (the group object carries controlBroker +// socket paths, handoff channels, etc. that LLM-facing tools must not reach). +// repo_info must work against exactly this shape. +function prodFacadeDeps(groupId) { + return { + groupId, + groupManager: { getGroupCwd: (id) => groupManager.getGroupCwd(id) }, + sessionManager: { getSession: () => null, writeToSession: () => false, waitUntilSettled: async () => ({ settled: true }) }, + }; +} + // deps the way mcpServer would build them for a worker's handoff socket: // sessionId comes from the closure (here a fake registered id), never from args function handoffDeps(groupId, role, sessionId) { @@ -732,3 +745,32 @@ test('repoInfo: caps bite (root 100 entries, README 8KB, package keys 50)', asyn assert.equal(out.packageJson.scripts.length, 50, 'scripts keys capped at 50'); assert.equal(out.packageJson.dependencies.length, 50, 'dependencies keys capped at 50'); }); + +// Regression: production broker deps hand repo_info the narrow groupManager +// facade (getGroupCwd only -- the full module's getGroup is never reachable), +// which used to crash with "deps.groupManager.getGroup is not a function". +test('repoInfo works against the production facade shape (no getGroup on groupManager)', async () => { + const dir = makeTmpRepo('facade'); + writeFileSync(join(dir, 'README.md'), '# Facade Project'); + const gid = randomUUID(); + await groupManager.createGroup({ groupId: gid, cwd: dir, orchestratorDir: join(dir, '..', 'orch') }); + groupsToDestroy.push(gid); + + const deps = prodFacadeDeps(gid); + assert.equal(typeof deps.groupManager.getGroup, 'undefined', 'facade shape must not expose getGroup'); + assert.equal(typeof deps.groupManager.getGroupCwd, 'function'); + + const out = await tools.repoInfo(deps); + assert.equal(out.error, undefined); + assert.equal(out.cwd, dir); + assert.equal(out.readme.file, 'README.md'); + assert.ok(out.readme.text.includes('Facade Project')); + assert.ok(out.root.files.includes('README.md')); +}); + +test('repoInfo: group-not-found also works against the production facade shape', async () => { + const deps = prodFacadeDeps('no-such-group'); + assert.equal(typeof deps.groupManager.getGroup, 'undefined', 'facade shape must not expose getGroup'); + const out = await tools.repoInfo(deps); + assert.equal(out.error, 'group-not-found'); +}); diff --git a/server/ws/notify.js b/server/ws/notify.js index a88647d..1e8bb14 100644 --- a/server/ws/notify.js +++ b/server/ws/notify.js @@ -82,9 +82,11 @@ export function notifyEnabled() { // - shells (app null) never get it, // - workers (groupRole !== 'orchestrator') never get it -- only the // orchestrator of a combo and standalone agent sessions do, +// - copilot never gets it (no CLI-arg/env MCP injection; the notify server +// would be unreachable), even as a standalone agent, // - nothing is injected when the feature is disabled. export function shouldInjectNotify({ shell, app, groupId, groupRole, notifyEnabled }) { - return !shell && app != null && !!notifyEnabled + return !shell && app != null && app !== 'copilot' && !!notifyEnabled && (groupId == null || groupRole === 'orchestrator'); } diff --git a/server/ws/notify.test.js b/server/ws/notify.test.js index 1884ad5..555df6d 100644 --- a/server/ws/notify.test.js +++ b/server/ws/notify.test.js @@ -70,6 +70,12 @@ test('shouldInjectNotify: standalone agents and combo orchestrators only', () => assert.equal(shouldInjectNotify({ ...base, notifyEnabled: false }), false, 'feature disabled -> never'); }); +test('shouldInjectNotify: copilot is never injected (no CLI-arg/env MCP injection)', () => { + const base = { shell: false, app: 'copilot', groupId: null, groupRole: null, notifyEnabled: true }; + assert.equal(shouldInjectNotify(base), false, 'standalone copilot never gets the notify server'); + assert.equal(shouldInjectNotify({ ...base, groupId: 'g1', groupRole: 'orchestrator' }), false, 'copilot as combo orchestrator also never'); +}); + test('subscribe/unsubscribe/list persist to the state file and restore', async () => { await withNotifyConfig( { notify: { subscriptions: [{ url: 'https://seed.example/webhook', name: 'seed' }] } },