diff --git a/apps/desktop/features/mcp-server/server.js b/apps/desktop/features/mcp-server/server.js index 98abcfe9..058f463c 100644 --- a/apps/desktop/features/mcp-server/server.js +++ b/apps/desktop/features/mcp-server/server.js @@ -616,6 +616,15 @@ function handleMessage(msg) { // re-running peek-mcp-init), the on-disk config drifts from our process // env and behaviour silently goes wrong. Detect drift before each // incoming message and exit(0) so the client respawns with fresh env. +// +// We CANNOT know which registration Claude Code actually used to spawn us — +// a read-only user-scope entry (~/.claude/settings.json) and a scoped +// project entry (/.mcp.json) legitimately coexist and differ. So we +// compare our env against EVERY candidate entry and only respawn when NONE +// of them matches — i.e. our env is genuinely stale. Matching against just +// the first-resolved file was a false-positive machine: whenever the nearest +// file happened to be the OTHER registration, a perfectly good server exited +// on connect and the client reported "can't connect". function readPeekEntry(filePath) { try { @@ -626,52 +635,66 @@ function readPeekEntry(filePath) { } } -function resolveOnDiskPeekEntry() { +function collectOnDiskPeekEntries() { const candidates = [ path.join(process.cwd(), '.mcp.json'), path.join(process.cwd(), '.claude', 'settings.json'), path.join(os.homedir(), '.claude', 'settings.json'), ]; + const entries = []; let sawAnyFile = false; for (const p of candidates) { if (!fs.existsSync(p)) continue; sawAnyFile = true; const entry = readPeekEntry(p); - if (entry) return { path: p, entry, sawAnyFile }; + if (entry) entries.push({ path: p, entry }); } - return { path: null, entry: null, sawAnyFile }; + return { entries, sawAnyFile }; +} + +function entryEnvMatchesCurrent(entry) { + const env = entry.env || {}; + const onScope = (env.PEEK_MCP_SCOPE_TAG || '').trim(); + const onReadonly = env.PEEK_MCP_READONLY === '1'; + const onDb = env.PEEK_DB_PATH || ''; + const curScope = (process.env.PEEK_MCP_SCOPE_TAG || '').trim(); + const curReadonly = process.env.PEEK_MCP_READONLY === '1'; + const curDb = process.env.PEEK_DB_PATH || ''; + return onScope === curScope && onReadonly === curReadonly && onDb === curDb; } let exiting = false; function checkConfigDrift() { if (exiting) return; - const found = resolveOnDiskPeekEntry(); - if (!found.entry) { + const { entries, sawAnyFile } = collectOnDiskPeekEntries(); + if (entries.length === 0) { // No config files at all → can't say; tolerate (might be transient). - if (!found.sawAnyFile) return; - // Files exist but peek entry was removed — toggled off. + if (!sawAnyFile) return; + // Files exist but the peek entry was removed everywhere — toggled off. exiting = true; process.stderr.write('Peek MCP entry removed from settings; exiting so client can clean up.\n'); process.exit(0); } - const env = found.entry.env || {}; - const onScope = (env.PEEK_MCP_SCOPE_TAG || '').trim(); - const onReadonly = env.PEEK_MCP_READONLY === '1'; - const onDb = env.PEEK_DB_PATH || ''; + // Correctly configured as long as SOME on-disk entry agrees with how we + // were spawned. + if (entries.some(({ entry }) => entryEnvMatchesCurrent(entry))) return; + // No entry matches our env → a real value change; respawn with fresh env. + exiting = true; const curScope = (process.env.PEEK_MCP_SCOPE_TAG || '').trim(); const curReadonly = process.env.PEEK_MCP_READONLY === '1'; const curDb = process.env.PEEK_DB_PATH || ''; - if (onScope !== curScope || onReadonly !== curReadonly || onDb !== curDb) { - exiting = true; - process.stderr.write( - `Peek MCP config drift detected at ${found.path}:\n` + - ` PEEK_MCP_SCOPE_TAG: current="${curScope}" on-disk="${onScope}"\n` + - ` PEEK_MCP_READONLY: current=${curReadonly ? '"1"' : '(unset)'} on-disk=${onReadonly ? '"1"' : '(unset)'}\n` + - ` PEEK_DB_PATH: current="${curDb}" on-disk="${onDb}"\n` + - `Exiting so client respawns with fresh env.\n`, - ); - process.exit(0); + let msg = 'Peek MCP config drift detected — no on-disk entry matches our spawn env:\n'; + msg += ` current: PEEK_MCP_SCOPE_TAG="${curScope}" ` + + `PEEK_MCP_READONLY=${curReadonly ? '"1"' : '(unset)'} PEEK_DB_PATH="${curDb}"\n`; + for (const { path: p, entry } of entries) { + const env = entry.env || {}; + msg += ` on-disk ${p}: PEEK_MCP_SCOPE_TAG="${(env.PEEK_MCP_SCOPE_TAG || '').trim()}" ` + + `PEEK_MCP_READONLY=${env.PEEK_MCP_READONLY === '1' ? '"1"' : '(unset)'} ` + + `PEEK_DB_PATH="${env.PEEK_DB_PATH || ''}"\n`; } + msg += 'Exiting so client respawns with fresh env.\n'; + process.stderr.write(msg); + process.exit(0); } // --- Main --- diff --git a/apps/desktop/features/mcp-server/server.test.js b/apps/desktop/features/mcp-server/server.test.js index d71a0e0f..02527d94 100644 --- a/apps/desktop/features/mcp-server/server.test.js +++ b/apps/desktop/features/mcp-server/server.test.js @@ -10,7 +10,7 @@ import { describe, it, before, after, beforeEach } from 'node:test'; import assert from 'node:assert/strict'; import { spawn, execSync } from 'node:child_process'; -import { mkdtempSync, writeFileSync, unlinkSync, existsSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, writeFileSync, unlinkSync, existsSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { fileURLToPath } from 'node:url'; @@ -1374,4 +1374,43 @@ describe('MCP Server config drift detection', () => { assert.equal(outcome, 'still-running'); proc.kill(); }); + + it('does not exit when a mismatching entry resolves first but another entry matches', async () => { + // Regression: two legitimate registrations coexist — a scoped project + // .mcp.json (resolved FIRST) and a read-only user-scope settings.json. + // We were spawned from the read-only one. The old "compare against the + // first-resolved file" logic saw the scoped .mcp.json, declared drift, + // and exit(0)'d on connect → client "can't connect". Match-any must keep + // us alive because SOME on-disk entry agrees with our spawn env. + const dbPath = createTestDb(); + const tmpProject = mkdtempSync(join(tmpdir(), 'peek-mcp-drift-')); + // Resolved first: scoped, writable — does NOT match our spawn env. + writeFileSync(join(tmpProject, '.mcp.json'), JSON.stringify({ + mcpServers: { peek: { command: 'node', args: [SERVER_PATH], + env: { PEEK_DB_PATH: dbPath, PEEK_MCP_SCOPE_TAG: 'peek' } } }, + })); + // HOME is isolated to tmpProject, so this IS the user-scope candidate. + mkdirSync(join(tmpProject, '.claude'), { recursive: true }); + writeFileSync(join(tmpProject, '.claude', 'settings.json'), JSON.stringify({ + mcpServers: { peek: { command: 'node', args: [SERVER_PATH], + env: { PEEK_DB_PATH: dbPath, PEEK_MCP_READONLY: '1' } } }, + })); + + // Spawned from the read-only registration. + const { proc, started, exited } = spawnServerWithCwd( + { PEEK_DB_PATH: dbPath, PEEK_MCP_READONLY: '1' }, tmpProject, + ); + await started; + + proc.stdin.write(JSON.stringify({ + jsonrpc: '2.0', id: 1, method: 'tools/list', params: {}, + }) + '\n'); + + const outcome = await Promise.race([ + exited.then(() => 'exited'), + new Promise((resolve) => setTimeout(() => resolve('still-running'), 500)), + ]); + assert.equal(outcome, 'still-running'); + proc.kill(); + }); }); diff --git a/apps/desktop/main/mcp-config.ts b/apps/desktop/main/mcp-config.ts index 4140c62f..d30357af 100644 --- a/apps/desktop/main/mcp-config.ts +++ b/apps/desktop/main/mcp-config.ts @@ -146,6 +146,11 @@ function writeSettings(data: Record): void { export interface McpStatus extends McpConfig { expectedCommand: string; configuredCommand: string | null; + // True when a peek entry is present but its command/env no longer matches + // what this build generates (e.g. left over from a prior install). The + // entry is still "enabled" — this just flags that it should be re-saved to + // point at the current launcher. + stale: boolean; settingsPath: string; profiles: string[]; configuredAt: number | null; @@ -161,7 +166,15 @@ export function getMcpStatus(): McpStatus { try { configuredAt = fs.statSync(settingsPath()).mtimeMs; } catch {} } return { - enabled: !!configured && snippetsMatch(configured, expected), + // A present peek entry means enabled — do NOT gate this on a byte-exact + // snippet match. A reinstall that changes the generated command path or + // env shape would otherwise flip a working, present config to "not + // configured" (blank Settings), tricking the user into re-enabling with + // defaults and silently dropping their scope tag. Staleness is surfaced + // separately via `stale` so the UI can nudge a re-save without hiding the + // config. + enabled: !!configured, + stale: !!configured && !snippetsMatch(configured, expected), profile: inferred.profile, readonly: inferred.readonly, scopeTag: inferred.scopeTag, diff --git a/apps/desktop/renderer/settings/settings.js b/apps/desktop/renderer/settings/settings.js index 5d7107a2..37c11b67 100644 --- a/apps/desktop/renderer/settings/settings.js +++ b/apps/desktop/renderer/settings/settings.js @@ -613,7 +613,7 @@ const renderIntegrationsSettings = async () => { let mcpState = { enabled: false, profile: 'default', readonly: true, scopeTag: '', - expectedCommand: '', configuredCommand: null, + expectedCommand: '', configuredCommand: null, stale: false, settingsPath: '~/.claude/settings.json', profiles: [], configuredAt: null, }; try { @@ -647,7 +647,11 @@ const renderIntegrationsSettings = async () => { return; } const when = mcpState.configuredAt ? new Date(mcpState.configuredAt).toLocaleString() : 'unknown'; - statusLine.textContent = `Configured at ${mcpState.settingsPath} (last updated ${when}).`; + let text = `Configured at ${mcpState.settingsPath} (last updated ${when}).`; + if (mcpState.stale) { + text += ' ⚠ The saved command is from a previous install — toggle off and on to update it.'; + } + statusLine.textContent = text; }; const syncUI = () => {