Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions server/ws/groupManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -750,6 +759,7 @@ const groupManagerApi = {
listGroupMembers,
isSessionInGroup,
getRoleForSession,
getGroupCwd,
setCurrentTurn,
pushHandoff,
takeHandoff,
Expand Down
13 changes: 13 additions & 0 deletions server/ws/mcpConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -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? }
Expand Down Expand Up @@ -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] };
Expand Down
27 changes: 27 additions & 0 deletions server/ws/mcpConfig.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions server/ws/mcpTools.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <cwd>` with a whitelisted argument list,
// never caller input).
Expand Down Expand Up @@ -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),
Expand Down
42 changes: 42 additions & 0 deletions server/ws/mcpTools.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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');
});
4 changes: 3 additions & 1 deletion server/ws/notify.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}

Expand Down
6 changes: 6 additions & 0 deletions server/ws/notify.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' }] } },
Expand Down