Fix copilot --mcp-config injection and repo_info facade mismatch - #39
Merged
Conversation
Bug A: copilot has no CLI-arg/env MCP injection, so buildMcpConfigArgsAndEnv must never assemble one -- the claude-style else branch was emitting '--mcp-config <JSON>' for copilot standalone sessions, making the binary die with 'unknown option'. Add an explicit copilot branch returning empty args/env (the function is the single assembly point, so no launch path can inject), and exclude copilot in shouldInjectNotify (no notify-server injection either). Bug B: repo_info called deps.groupManager.getGroup, but production broker deps carry the narrow groupManagerApi facade which never exposed getGroup -- crashing every repo_info call with 'getGroup is not a function'. Add getGroupCwd to the facade (cwd only, keeping the internal group object out of LLM reach) and switch repoInfo to it; group-not-found behavior preserved. Tests: copilot injection-empty cases (mcpConfig), copilot shouldInjectNotify exclusion (notify), repo_info against the production facade shape incl. group-not-found (mcpTools). Full suite: 299 pass / 1 pre-existing environment-dependent sandbox-resolve failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two bugs reported against the current server.
Bug A: copilot launch fails with
error: unknown option '--mcp-config'buildMcpConfigArgsAndEnv()only branched onopencode; every other app (copilot included) fell into the claude-style else branch that assembles--mcp-config <JSON>. A standalone copilot session with notify enabled reached it viasessionManager.js→shouldInjectNotify()returning true for standalone agents → the flag was passed to the copilot binary, which errors out.server/ws/mcpConfig.js: explicitcopilotbranch returning{ args: [], env: {} }— the function is the single assembly point, so no copilot launch path can ever receive--mcp-config.server/ws/notify.js:shouldInjectNotify()now excludes copilot (it has no CLI-arg/env MCP injection, so the notify server would be unreachable; also avoids binding the notify socket into its sandbox).Bug B: ccserver_repo_info fails with
deps.groupManager.getGroup is not a functionrepoInfo()calleddeps.groupManager.getGroup(), but production deps pass the deliberately narrowgroupManagerApifacade (groupManager.js) which never includedgetGroup— onlygroup.cwdwas needed. Unit tests passed the full module namespace, so the mismatch went unnoticed since repo_info was introduced (05bb3bc).server/ws/groupManager.js: addedgetGroupCwd(groupId)to the facade (returnsgroup?.cwd ?? null), keeping the raw group object (controlBroker socket paths, handoff channels, etc.) out of LLM-facing reach.server/ws/mcpTools.js:repoInfo()now readsdeps.groupManager.getGroupCwd(deps.groupId).Tests
+6 unit tests: copilot gets no injection (plain / notify sandbox / notify host),
shouldInjectNotifyexcludes copilot, andrepoInforegression tests against the production facade shape (nogetGroup).npm test: 300 tests, 299 pass — the single failure (sandbox-resolve.test.js) is a pre-existing environment-dependent failure unrelated to this change.