Skip to content

Commit 5fa2415

Browse files
authored
fix: mcp agent policies and deduplicate blocks when merging unrolled assistants (#8376)
* fix: deduplicate blocks when merging config yaml * fix: tool policies for mcp servers in agent mode * fix: syntax error * fix: cli tests * fix: don't log * fix: lint/format * fix: better comments for tool policies * fix: one test left * fix: failing services test * fix: lint and format
1 parent 749916d commit 5fa2415

File tree

11 files changed

+772
-121
lines changed

11 files changed

+772
-121
lines changed

extensions/cli/src/onboarding.test.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -108,11 +108,8 @@ name: "Incomplete Config"
108108
});
109109

110110
test("should handle empty string config path", async () => {
111-
// Empty string should be treated differently from undefined
112-
// Note: empty string triggers onboarding flow, but should still fail in our error format
113-
await expect(
114-
initializeWithOnboarding(mockAuthConfig, ""),
115-
).rejects.toThrow();
111+
// Loads default agent with no error
112+
await initializeWithOnboarding(mockAuthConfig, "");
116113
});
117114

118115
test("should not fall back to default config when explicit config fails", async () => {

extensions/cli/src/services/MCPService.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ export class MCPService
5252
private isShuttingDown = false;
5353

5454
getDependencies(): string[] {
55-
return [SERVICE_NAMES.AUTH, SERVICE_NAMES.API_CLIENT, SERVICE_NAMES.CONFIG];
55+
return [SERVICE_NAMES.CONFIG];
5656
}
5757
constructor() {
5858
super("MCPService", {

extensions/cli/src/services/ToolPermissionService.ts

Lines changed: 85 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { parseAgentFileTools } from "@continuedev/config-yaml";
1+
import { ALL_BUILT_IN_TOOLS } from "src/tools/allBuiltIns.js";
22

33
import { ensurePermissionsYamlExists } from "../permissions/permissionsYamlLoader.js";
44
import { resolvePermissionPrecedence } from "../permissions/precedenceResolver.js";
@@ -11,7 +11,11 @@ import { logger } from "../util/logger.js";
1111

1212
import { BaseService, ServiceWithDependencies } from "./BaseService.js";
1313
import { serviceContainer } from "./ServiceContainer.js";
14-
import { AgentFileServiceState, SERVICE_NAMES } from "./types.js";
14+
import {
15+
AgentFileServiceState,
16+
MCPServiceState,
17+
SERVICE_NAMES,
18+
} from "./types.js";
1519

1620
export interface InitializeToolServiceOverrides {
1721
allow?: string[];
@@ -61,55 +65,94 @@ export class ToolPermissionService
6165
* Declare dependencies on other services
6266
*/
6367
getDependencies(): string[] {
64-
return [SERVICE_NAMES.AGENT_FILE];
68+
return [SERVICE_NAMES.AGENT_FILE, SERVICE_NAMES.MCP];
6569
}
6670

6771
/**
6872
* Generate agent-file-specific policies if an agent file is active
6973
*/
70-
private generateAgentFilePolicies(
74+
generateAgentFilePolicies(
7175
agentFileServiceState?: AgentFileServiceState,
76+
mcpServiceState?: MCPServiceState,
7277
): undefined | ToolPermissionPolicy[] {
73-
if (!agentFileServiceState?.agentFile?.tools) {
78+
const parsedTools = agentFileServiceState?.parsedTools;
79+
if (!parsedTools?.tools.length) {
7480
return undefined;
7581
}
7682

77-
const parsedTools = parseAgentFileTools(
78-
agentFileServiceState.agentFile.tools,
79-
);
80-
if (parsedTools.tools.length === 0) {
81-
return undefined;
82-
}
8383
const policies: ToolPermissionPolicy[] = [];
84+
const servers = Array.from(mcpServiceState?.connections?.values() ?? []);
85+
for (const mcpServer of parsedTools.mcpServers) {
86+
const server = servers?.find(
87+
(s) => s.config?.sourceSlug && s.config.sourceSlug === mcpServer,
88+
);
89+
if (!server) {
90+
logger.warn("No connected MCP server found ");
91+
continue;
92+
}
93+
94+
const specificTools = parsedTools.tools.filter(
95+
(t) => t.mcpServer && t.toolName && t.mcpServer === mcpServer,
96+
);
8497

85-
for (const toolRef of parsedTools.tools) {
86-
if (toolRef.mcpServer) {
87-
if (toolRef.toolName) {
88-
policies.push({
89-
tool: `mcp:${toolRef.mcpServer}:${toolRef.toolName}`,
90-
permission: "allow",
91-
});
92-
} else {
93-
policies.push({
94-
tool: `mcp:${toolRef.mcpServer}:*`,
95-
permission: "allow",
96-
});
97-
}
98-
} else if (toolRef.toolName) {
99-
policies.push({ tool: toolRef.toolName, permission: "allow" });
98+
// In the `tools` key of an agent is comma separated strings like
99+
// mcp/server, mcp/server2:tool_name, Bash
100+
// - this would give the agent access to ALL tools from mcp/server, ONLY tool_name from mcp/server2, and Bash, and no other tools
101+
// mcp/server
102+
// - this would give the agent access to ALL tools from mcp/server, and no built-ins
103+
// built_in, mcp/server
104+
// - "built_in" keyword can be used to include all built ins
105+
// Blank = all built-in tools
106+
107+
// Handle MCP first
108+
// If ANY mcp tools are specifically called out, only allow those ones for that mcp server
109+
// Otherwise the blanket allow will cover all MCP tools
110+
if (specificTools.length) {
111+
const specificSet = new Set(specificTools.map((t) => t.toolName));
112+
const notMentioned = server.tools.filter(
113+
(t) => !specificSet.has(t.name),
114+
);
115+
const allowed: ToolPermissionPolicy[] = specificTools.map((t) => ({
116+
tool: t.toolName!,
117+
permission: "allow",
118+
}));
119+
policies.push(...allowed);
120+
const disallowed: ToolPermissionPolicy[] = notMentioned.map((t) => ({
121+
tool: t.name,
122+
permission: "exclude",
123+
}));
124+
policies.push(...disallowed);
100125
}
101126
}
102127

103-
if (policies.length > 0) {
104-
logger.debug(`Generated ${policies.length} agent file tool policies`);
128+
// If mcp servers or specific built-in tools are specified
129+
// then we only inclue listed built-in tools and exclude all others
130+
const hasMcp = !!parsedTools.mcpServers?.length;
131+
const specificBuiltIns = parsedTools.tools
132+
.filter((t) => !t.mcpServer)
133+
.map((t) => t.toolName!);
134+
if (specificBuiltIns.length || (hasMcp && !parsedTools.allBuiltIn)) {
135+
const allowed: ToolPermissionPolicy[] = specificBuiltIns.map((tool) => ({
136+
tool,
137+
permission: "allow",
138+
}));
139+
policies.push(...allowed);
140+
const specificBuiltInSet = new Set(specificBuiltIns);
141+
const notMentioned = ALL_BUILT_IN_TOOLS.map((t) => t.name).filter(
142+
(name) => !specificBuiltInSet.has(name),
143+
);
144+
const disallowed: ToolPermissionPolicy[] = notMentioned.map((tool) => ({
145+
tool,
146+
permission: "exclude",
147+
}));
148+
policies.push(...disallowed);
105149
}
106150

107-
if (parsedTools.allBuiltIn) {
108-
policies.push({ tool: "*", permission: "allow" });
109-
policies.push({ tool: "mcp:*", permission: "exclude" });
110-
} else {
111-
policies.push({ tool: "*", permission: "exclude" });
112-
}
151+
// Allow all other tools
152+
policies.push({
153+
tool: "*",
154+
permission: "allow",
155+
});
113156

114157
return policies;
115158
}
@@ -165,6 +208,7 @@ export class ToolPermissionService
165208
initializeSync(
166209
runtimeOverrides?: InitializeToolServiceOverrides,
167210
agentFileServiceState?: AgentFileServiceState,
211+
mcpServiceState?: MCPServiceState,
168212
): ToolPermissionServiceState {
169213
logger.debug("Synchronously initializing ToolPermissionService");
170214

@@ -180,6 +224,7 @@ export class ToolPermissionService
180224

181225
const agentFilePolicies = this.generateAgentFilePolicies(
182226
agentFileServiceState,
227+
mcpServiceState,
183228
);
184229
const modePolicies = this.generateModePolicies();
185230

@@ -224,11 +269,16 @@ export class ToolPermissionService
224269
async doInitialize(
225270
runtimeOverrides?: InitializeToolServiceOverrides,
226271
agentFileServiceState?: AgentFileServiceState,
272+
mcpServiceState?: MCPServiceState,
227273
): Promise<ToolPermissionServiceState> {
228274
await ensurePermissionsYamlExists();
229275

230276
// Use the synchronous version after ensuring the file exists
231-
return this.initializeSync(runtimeOverrides, agentFileServiceState);
277+
return this.initializeSync(
278+
runtimeOverrides,
279+
agentFileServiceState,
280+
mcpServiceState,
281+
);
232282
}
233283

234284
/**

extensions/cli/src/services/index.test.ts

Lines changed: 8 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1,71 +1,10 @@
1-
import { Configuration, DefaultApi } from "@continuedev/sdk/dist/api";
21
import { vi } from "vitest";
32

4-
import { initializeServices, services } from "./index.js";
5-
6-
// Mock the onboarding module
7-
vi.mock("../onboarding.js", () => ({
8-
initializeWithOnboarding: vi.fn().mockResolvedValue({ wasOnboarded: false }),
9-
createOrUpdateConfig: vi.fn().mockResolvedValue(undefined),
10-
}));
3+
import { MCPService } from "./MCPService.js";
114

12-
// Mock auth module
13-
vi.mock("../auth/workos.js", () => ({
14-
loadAuthConfig: vi.fn().mockReturnValue({}),
15-
getConfigUri: vi.fn().mockReturnValue(null),
16-
}));
17-
18-
// Mock the config loader
19-
vi.mock("../configLoader.js", () => ({
20-
loadConfiguration: vi.fn().mockResolvedValue({
21-
config: { name: "test", version: "1.0.0" },
22-
source: { type: "test" },
23-
}),
24-
}));
5+
import { initializeServices, services } from "./index.js";
256

267
describe("initializeServices", () => {
27-
// let mockToolPermissionsService: any;
28-
29-
beforeEach(() => {
30-
// Reset all mocks
31-
vi.clearAllMocks();
32-
33-
// Mock service methods to avoid actual initialization
34-
vi.spyOn(services.auth, "initialize").mockResolvedValue({
35-
authConfig: {
36-
accessToken: "",
37-
expiresAt: 123,
38-
organizationId: "",
39-
refreshToken: "",
40-
userEmail: "",
41-
},
42-
isAuthenticated: false,
43-
});
44-
vi.spyOn(services.apiClient, "initialize").mockResolvedValue({
45-
apiClient: new DefaultApi(
46-
new Configuration({
47-
basePath: "",
48-
accessToken: "",
49-
}),
50-
),
51-
});
52-
vi.spyOn(services.agentFile, "initialize").mockResolvedValue({
53-
slug: null,
54-
agentFile: null,
55-
agentFileModel: null,
56-
parsedRules: null,
57-
parsedTools: null,
58-
});
59-
vi.spyOn(services.config, "initialize").mockResolvedValue({
60-
config: { name: "test", version: "1.0.0" },
61-
configPath: undefined,
62-
});
63-
});
64-
65-
afterEach(() => {
66-
vi.restoreAllMocks();
67-
});
68-
698
describe("mode conversion", () => {
709
it("should pass only auto flag for auto mode", async () => {
7110
const spy = vi.spyOn(services.toolPermissions, "initialize");
@@ -94,6 +33,12 @@ describe("initializeServices", () => {
9433
parsedRules: null,
9534
parsedTools: null,
9635
},
36+
{
37+
connections: [],
38+
mcpService: expect.any(MCPService),
39+
prompts: [],
40+
tools: [],
41+
},
9742
);
9843
});
9944
});

extensions/cli/src/services/index.ts

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,10 @@ export async function initializeServices(initOptions: ServiceInitOptions = {}) {
113113
serviceContainer.register(
114114
SERVICE_NAMES.TOOL_PERMISSIONS,
115115
async () => {
116-
const agentFileState = await serviceContainer.get<AgentFileServiceState>(
117-
SERVICE_NAMES.AGENT_FILE,
118-
);
116+
const [mcpState, agentFileState] = await Promise.all([
117+
serviceContainer.get<AuthServiceState>(SERVICE_NAMES.MCP),
118+
serviceContainer.get<AgentFileServiceState>(SERVICE_NAMES.AGENT_FILE),
119+
]);
119120

120121
// Initialize mode service with tool permission overrides
121122
if (initOptions.toolPermissionOverrides) {
@@ -128,24 +129,28 @@ export async function initializeServices(initOptions: ServiceInitOptions = {}) {
128129
exclude: overrides.exclude,
129130
isHeadless: initOptions.headless,
130131
};
131-
132132
// Only set the boolean flag that corresponds to the mode
133133
if (overrides.mode) {
134134
initArgs.mode = overrides.mode;
135135
}
136136
// If mode is "normal" or undefined, no flags are set
137-
return await toolPermissionService.initialize(initArgs, agentFileState);
137+
return await toolPermissionService.initialize(
138+
initArgs,
139+
agentFileState,
140+
mcpState,
141+
);
138142
} else {
139143
// Even if no overrides, we need to initialize with defaults
140144
return await toolPermissionService.initialize(
141145
{
142146
isHeadless: initOptions.headless,
143147
},
144148
agentFileState,
149+
mcpState,
145150
);
146151
}
147152
},
148-
[SERVICE_NAMES.AGENT_FILE],
153+
[SERVICE_NAMES.AGENT_FILE, SERVICE_NAMES.MCP],
149154
);
150155

151156
// Initialize SystemMessageService with command options
@@ -257,7 +262,10 @@ export async function initializeServices(initOptions: ServiceInitOptions = {}) {
257262
if (!configState.config) {
258263
throw new Error("Config not available for MCP service");
259264
}
260-
return mcpService.initialize(configState.config, initOptions.headless);
265+
return mcpService.initialize(
266+
configState.config,
267+
initOptions.headless || initOptions.options?.agent,
268+
);
261269
},
262270
[SERVICE_NAMES.CONFIG], // Depends on config
263271
);
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
import { editTool } from "./edit.js";
2+
import { exitTool } from "./exit.js";
3+
import { fetchTool } from "./fetch.js";
4+
import { listFilesTool } from "./listFiles.js";
5+
import { multiEditTool } from "./multiEdit.js";
6+
import { readFileTool } from "./readFile.js";
7+
import { runTerminalCommandTool } from "./runTerminalCommand.js";
8+
import { searchCodeTool } from "./searchCode.js";
9+
import { statusTool } from "./status.js";
10+
import { writeChecklistTool } from "./writeChecklist.js";
11+
import { writeFileTool } from "./writeFile.js";
12+
13+
// putting in here for circular import issue
14+
export const ALL_BUILT_IN_TOOLS = [
15+
readFileTool,
16+
editTool,
17+
multiEditTool,
18+
writeFileTool,
19+
listFilesTool,
20+
searchCodeTool,
21+
runTerminalCommandTool,
22+
fetchTool,
23+
writeChecklistTool,
24+
exitTool,
25+
statusTool,
26+
];

0 commit comments

Comments
 (0)