Skip to content

Commit 97ef445

Browse files
committed
Merge branch 'feat/mcp-s1-config' into feat/mcp-s2-client
2 parents decf490 + 3c0412e commit 97ef445

3 files changed

Lines changed: 33 additions & 7 deletions

File tree

docs/MCP.md

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,10 +114,15 @@ Every MCP tool call goes through `ctx.approve({ kind: 'mcp', … })` by default.
114114

115115
### G3 — Autopilot must not silently run third-party tools
116116
Autopilot exists to skip *our own* vetted commands. Default: MCP calls **still prompt under autopilot**.
117-
A per-tool allow-list (`"github__list_issues": "allow"`) lets users opt specific tools out. If MCP tool
118-
annotations (`readOnlyHint` / `destructiveHint`) are present we may auto-allow read-only ones — but
119-
annotations are **server-supplied and therefore untrusted**, so they can only ever *relax* toward asking,
120-
never justify running something destructive silently.
117+
The **only** thing that grants `allow` is the user's per-tool allow-list (`"github__list_issues": "allow"`).
118+
119+
Server-supplied annotations (`readOnlyHint` / `destructiveHint`) are **untrusted** and may therefore only
120+
ever *tighten*, never loosen:
121+
- `destructiveHint: true` **forces the prompt**, overriding an allow-list entry — worst case one extra
122+
prompt, and a hostile server gains nothing by lying.
123+
- `readOnlyHint: true` grants **nothing** on its own — a server could simply claim it.
124+
125+
That is exactly what `classifyMcpTool` implements (S1); the tests pin both directions.
121126

122127
### G4 — Untrusted text reaching the model
123128
MCP **tool descriptions** are third-party strings injected into the tools block the model reads — a known

extensions/levelcode-ai/mcpConfig.js

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,12 @@ function loadServerConfig(opts) {
109109
}
110110
};
111111

112-
// 1. User setting first — see the precedence note above.
113-
add(serverMapOf({ mcpServers: o.settings }), 'settings', 'settings');
112+
// 1. User setting first — see the precedence note above. A MISSING setting is an empty map, not an
113+
// empty wrapper: {mcpServers: undefined} would fall through serverMapOf's bare-map branch and get
114+
// reported as a phantom server literally named "mcpServers" — a spurious error for every user who
115+
// has no MCP config at all.
116+
const settingsRaw = (o.settings && typeof o.settings === 'object' && !Array.isArray(o.settings)) ? o.settings : null;
117+
add(settingsRaw ? serverMapOf(settingsRaw) : {}, 'settings', 'settings');
114118

115119
// 2. Then each workspace folder's file.
116120
for (const f of (Array.isArray(o.folders) ? o.folders : [])) {
@@ -156,7 +160,7 @@ function sanitizeSegment(raw, fallback) {
156160
function namespaceToolName(server, tool) {
157161
const full = sanitizeSegment(server, 'server') + NAME_SEPARATOR + sanitizeSegment(tool, 'tool');
158162
if (full.length <= MAX_TOOL_NAME) { return full; }
159-
const tag = shortHash(String(server) + '
163+
const tag = shortHash(String(server) + '\u0000' + String(tool));
160164
return full.slice(0, MAX_TOOL_NAME - tag.length - 1) + '_' + tag;
161165
}
162166

extensions/levelcode-ai/test/mcpConfig.test.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,23 @@ test('CONFIG: a throwing readFile, absent file, and no config at all are all tol
185185
assert.strictEqual(M.loadServerConfig().servers.length, 0);
186186
});
187187

188+
test('CONFIG: no MCP config at all reports NO problems (the common case must be silent)', () => {
189+
// Regression: a missing `settings` used to be wrapped as {mcpServers: undefined}, which fell through
190+
// to the bare-map branch and reported a phantom server named "mcpServers" — an error for every user
191+
// who has never configured MCP. Assert on problems, not just servers.
192+
for (const arg of [undefined, {}, { settings: undefined }, { settings: null }, { settings: {} }]) {
193+
const r = M.loadServerConfig(arg);
194+
assert.deepStrictEqual(r.servers, [], JSON.stringify(arg));
195+
assert.deepStrictEqual(r.problems, [], 'expected no problems for ' + JSON.stringify(arg) + ', got ' + JSON.stringify(r.problems));
196+
}
197+
});
198+
199+
test('CONFIG: a settings object may itself use the {mcpServers:…} wrapper', () => {
200+
const { servers, problems } = M.loadServerConfig({ settings: { mcpServers: { wrapped: { command: 'x' } } } });
201+
assert.deepStrictEqual(servers.map((s) => s.name), ['wrapped']);
202+
assert.deepStrictEqual(problems, []);
203+
});
204+
188205
test('CONFIG: the server cap is enforced', () => {
189206
const settings = {};
190207
for (let i = 0; i < M.MAX_SERVERS + 3; i++) { settings['s' + i] = { command: 'x' }; }

0 commit comments

Comments
 (0)