From 2a7bba11ea6638d717b8f5e2143a50b2a63084d0 Mon Sep 17 00:00:00 2001 From: Michal Date: Mon, 10 Aug 2026 12:15:46 +0100 Subject: [PATCH 1/3] fix(claude): stop pre-migration .mcp.json residue outranking a project switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mcpctl config claude --project X` writes user scope and never rewrites a checkout's `.mcp.json`. The status line, however, preferred that file unconditionally — so a legacy project-named entry an older mcpctl left behind (`homeautomation` -> `mcpctl mcp -p homeautomation`) kept naming the old project for good, and every switch looked like it had done nothing. Reproduced live: with user scope on `sre`, `mcpctl statusline --directory ~/developer/michalzxc/claude/debug` printed `mcpctl:homeautomation` — a project Claude Code also had in `disabledMcpServers` for that directory, so the line named a server that was not even mounted. Rank the sources by how deliberate each one is instead: a canonical `mcpctl` pin, then user scope, then legacy residue, then the marker. A pin is a decision and still wins; residue is not and no longer does. At every step, skip a server Claude Code has switched off for that directory. `config claude` now also warns when the working directory's `.mcp.json` contradicts the switch, naming the file — the two scopes are merged rather than chosen between, so nothing else would tell you. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019wUmrfkVQR6CKcYKxENq7k --- docs/claude-integration.md | 49 +++++++-- src/cli/src/commands/config.ts | 48 +++++++++ src/cli/src/commands/statusline.ts | 81 ++++++++++++--- src/cli/src/config/claude-mcp.ts | 57 ++++++++-- src/cli/tests/commands/claude.test.ts | 42 ++++++++ src/cli/tests/commands/statusline.test.ts | 121 ++++++++++++++++++++++ src/cli/tests/config/claude-mcp.test.ts | 47 +++++++++ 7 files changed, 414 insertions(+), 31 deletions(-) create mode 100644 src/cli/tests/commands/statusline.test.ts diff --git a/docs/claude-integration.md b/docs/claude-integration.md index d0163b5..425a65e 100644 --- a/docs/claude-integration.md +++ b/docs/claude-integration.md @@ -101,13 +101,48 @@ to open, and re-scoping it would silently change which skills sync into it. ⏵⏵ bypass permissions on · ← for agents ``` -`mcpctl statusline` resolves the project from a directory-scoped `.mcp.json` -first (a repo that pinned itself wins), then the user-scope entry in -`.claude.json`, then a `.mcpctl-project` marker up the tree so a checkout that is scoped but not yet -wired still reports. It reads the directory from the JSON Claude Code pipes in, -so it follows `/cwd` rather than reporting wherever the binary was launched, and -prints **nothing** when no project is active — an empty status line beats one -saying "none" on every unrelated repo. +`mcpctl statusline` reads the directory from the JSON Claude Code pipes in, so it +follows `/cwd` rather than reporting wherever the binary was launched, and prints +**nothing** when no project is active — an empty status line beats one saying +"none" on every unrelated repo. + +It then takes the project from the most deliberate source that names one: + +1. a canonical `mcpctl` entry in that directory's `.mcp.json` — a repo that + pinned itself wins, and it is the scope Claude Code itself prefers when both + define that server name; +2. the user-scope entry in `.claude.json` — what `config claude --project` + writes, so a switch takes effect everywhere it is not overridden; +3. a **legacy** project-named entry in `.mcp.json` (`homeautomation`, + `docmost`, …), left by an mcpctl older than the constant server name; +4. a `.mcpctl-project` marker up the tree, so a checkout that is scoped but not + yet wired still reports. + +> **Legacy entries rank below user scope on purpose.** They used to outrank it, +> which made switching look broken: a user-scope switch never rewrites a +> checkout's `.mcp.json`, so the leftover kept naming the old project for good. +> A pin is a decision; residue is not. + +A server Claude Code has switched off for that directory (`disabledMcpServers` / +`disabledMcpjsonServers`) is skipped at every step — a disabled server is not +mounted, so naming its project would be a lie. A `.mcp.json` server that is in +neither list is still awaiting its approval prompt and does count, since blanking +the status line on a fresh checkout is the more confusing failure. + +### When a directory contradicts a switch + +Claude Code merges the two scopes rather than picking one, so switching in user +scope cannot clean up what a directory declares. `config claude` says so rather +than reporting plain success: + +``` +Warning: /path/to/repo/.mcp.json still registers 'homeautomation' for this +directory — mounted alongside 'sre', not replaced by it. + Re-run with --scope project to retire it, or delete the entry by hand. +``` + +A canonical entry pinned to another project gets the stronger wording — it +*overrides* the switch in that directory rather than sitting beside it. ### It is never installed over yours diff --git a/src/cli/src/commands/config.ts b/src/cli/src/commands/config.ts index 8623513..917d551 100644 --- a/src/cli/src/commands/config.ts +++ b/src/cli/src/commands/config.ts @@ -46,6 +46,8 @@ import { mergeUserScopeServer, userScopeProject, activeProjectIn, + canonicalProjectIn, + legacyEntriesIn, claudeJsonPath, type McpJson, type ClaudeJson, @@ -104,11 +106,51 @@ function readMcpJson(path: string): McpJson | null { } } +/** + * Warnings about a `.mcp.json` in `dir` that contradicts a user-scope switch to + * `project`. + * + * Claude Code merges the two scopes rather than picking one, so a + * directory-scoped entry does not go away when you switch globally: + * - a canonical `mcpctl` entry shares the name, and project scope wins — the + * switch has no effect in this directory at all; + * - a legacy project-named entry has a *different* name, so it is simply + * mounted alongside and the old project keeps answering here. + * Either way the user is owed the file path, because nothing else will tell + * them. Exported for tests. + */ +export function shadowWarnings(dir: string, project: string | undefined): string[] { + if (project === undefined || project === '') return []; + const path = join(dir, '.mcp.json'); + const parsed = readMcpJson(path); + if (parsed === null) return []; + + const pinned = canonicalProjectIn(parsed); + if (pinned !== null && pinned !== project) { + return [ + `Warning: ${path} pins '${MCPCTL_SERVER_NAME}' to '${pinned}' for this directory, which overrides the switch here.`, + ` Re-run with --scope project to repoint it, or delete the '${MCPCTL_SERVER_NAME}' entry to follow the user-scope project.`, + ]; + } + + const stale = legacyEntriesIn(parsed).filter((e) => e.project !== project); + if (stale.length > 0) { + const names = stale.map((e) => `'${e.server}'`).join(', '); + return [ + `Warning: ${path} still registers ${names} for this directory — mounted alongside '${project}', not replaced by it.`, + ` Re-run with --scope project to retire ${stale.length === 1 ? 'it' : 'them'}, or delete the ${stale.length === 1 ? 'entry' : 'entries'} by hand.`, + ]; + } + return []; +} + export interface ConfigCommandDeps { configDeps: Partial; log: (...args: string[]) => void; /** API client for the skills sync side-effect of `config claude --project`. Optional so existing call sites work; without it we skip the sync step. */ apiClient?: ApiClient; + /** Working directory to check for a shadowing `.mcp.json`. Injectable so tests need not chdir. */ + cwd?: () => string; } export interface ConfigApiDeps { @@ -124,6 +166,7 @@ const defaultDeps: ConfigCommandDeps = { export function createConfigCommand(deps?: Partial, apiDeps?: ConfigApiDeps): Command { const { configDeps, log } = { ...defaultDeps, ...deps }; + const cwd = deps?.cwd ?? ((): string => process.cwd()); // PR-5: api client used by `mcpctl config claude --project` to run the // initial skills sync after wiring the .mcp.json. Threaded through from // index.ts; falls back to apiDeps.client when not explicitly passed (the @@ -406,6 +449,11 @@ export function createConfigCommand(deps?: Partial, apiDeps?: if (userScope) { // The whole point of user scope: you do this once, not per checkout. log('This applies in every directory — no need to re-run it per repo.'); + // ...except where a directory-scoped entry contradicts it. That file + // is never rewritten by a user-scope switch, so staying silent is how + // a switch ends up looking like it did nothing: the status line keeps + // naming the old project, and its server keeps answering here. + for (const line of shadowWarnings(cwd(), opts.project)) log(line); } // PR-5: write project marker, run initial skills sync, install diff --git a/src/cli/src/commands/statusline.ts b/src/cli/src/commands/statusline.ts index 7f22ede..9d04ef0 100644 --- a/src/cli/src/commands/statusline.ts +++ b/src/cli/src/commands/statusline.ts @@ -2,7 +2,16 @@ import { Command } from 'commander'; import { readFileSync } from 'node:fs'; import { join, resolve } from 'node:path'; import { homedir } from 'node:os'; -import { activeProjectIn, claudeJsonPath, userScopeProject, type McpJson, type ClaudeJson } from '../config/claude-mcp.js'; +import { + MCPCTL_SERVER_NAME, + canonicalProjectIn, + claudeJsonPath, + disabledServersFor, + legacyEntriesIn, + userScopeProject, + type McpJson, + type ClaudeJson, +} from '../config/claude-mcp.js'; import { findProjectMarker } from '../utils/project-marker.js'; /** @@ -16,9 +25,10 @@ import { findProjectMarker } from '../utils/project-marker.js'; * `setStatus`. * * Claude Code pipes a JSON blob in on stdin (session id, model, workspace). We - * only need the directory — the project is whatever `.mcp.json` there mounts, - * falling back to a `.mcpctl-project` marker up the tree so a checkout that is - * scoped but not yet wired still reports. + * only need the directory; the project is then resolved from the most + * deliberate source that names one — see the ranking in the action below. + * Whatever it reports has to be a project that is genuinely mounted, so a + * server Claude Code has switched off for that directory is skipped. * * Prints nothing at all when no project is active: an empty status line is * better than one that says "none" on every unrelated repo you open. @@ -53,25 +63,47 @@ export function resolveDirectory(input: StatusLineInput, fallback: string): stri return input.workspace?.current_dir ?? input.workspace?.project_dir ?? input.cwd ?? fallback; } -/** The project Claude Code's user-scope config mounts, or null. */ -export function projectFromUserScope(path: string): string | null { +/** Claude Code's user-scope config, or null if it is missing or unreadable. */ +export function readClaudeJson(path: string): ClaudeJson | null { try { - return userScopeProject(JSON.parse(readFileSync(path, 'utf-8')) as ClaudeJson); + return JSON.parse(readFileSync(path, 'utf-8')) as ClaudeJson; } catch { return null; } } -/** The project `.mcp.json` in `dir` mounts, or null. */ -export function projectFromMcpJson(dir: string): string | null { +/** The project Claude Code's user-scope config mounts, or null. */ +export function projectFromUserScope(doc: ClaudeJson | null): string | null { + return userScopeProject(doc); +} + +/** The `.mcp.json` in `dir`, or null if there isn't a readable one. */ +export function readDirMcpJson(dir: string): McpJson | null { try { - const parsed = JSON.parse(readFileSync(join(dir, '.mcp.json'), 'utf-8')) as McpJson; - return activeProjectIn(parsed); + return JSON.parse(readFileSync(join(dir, '.mcp.json'), 'utf-8')) as McpJson; } catch { return null; } } +/** + * The project the canonical `mcpctl` entry in `dir`'s `.mcp.json` pins, or null + * — skipped when Claude Code has that server switched off for `dir`. + */ +export function projectFromDirPin(mcpJson: McpJson | null, disabled: Set): string | null { + if (disabled.has(MCPCTL_SERVER_NAME)) return null; + return canonicalProjectIn(mcpJson); +} + +/** + * The project a *legacy* project-named entry in `dir`'s `.mcp.json` mounts, or + * null. Disabled entries are skipped, so a leftover the user already turned off + * in `/mcp` stops being reported. + */ +export function projectFromDirLegacy(mcpJson: McpJson | null, disabled: Set): string | null { + return legacyEntriesIn(mcpJson).find((e) => !disabled.has(e.server))?.project ?? null; +} + /** Format for the status line. Empty string means "render nothing". */ export function formatStatus(project: string | null, prefix: string): string { return project !== null && project !== '' ? `${prefix}${project}` : ''; @@ -96,12 +128,29 @@ export function createStatuslineCommand(deps?: Partial): Command const input = opts.directory !== undefined ? {} : await readStdinJson(); const dir = opts.directory !== undefined ? resolve(opts.directory) : resolveDirectory(input, cwd()); - // Directory-scoped wiring wins: a repo with its own .mcp.json entry has - // deliberately pinned itself, and that beats the global default. - let project = projectFromMcpJson(dir) ?? projectFromUserScope(claudeJsonPath()); + const claudeJson = readClaudeJson(claudeJsonPath()); + const mcpJson = readDirMcpJson(dir); + const disabled = disabledServersFor(claudeJson, dir); + + // Ranked by how deliberate each source is, because a switch has to be + // able to win: + // 1. a canonical `mcpctl` entry in this directory's .mcp.json — a + // deliberate pin, and the scope Claude Code itself prefers when both + // define the same server name; + // 2. user scope — what `config claude --project` writes, so switching + // projects must beat anything less deliberate than a pin; + // 3. a *legacy* project-named entry in .mcp.json. This used to outrank + // user scope, which made a switch look like it had done nothing: the + // residue an older mcpctl left in a checkout is not a pin, and never + // gets rewritten by a user-scope switch, so it reported the old + // project forever; + // 4. the .mcpctl-project marker — the other thing `config claude` + // writes, and what skills sync already trusts. + let project = + projectFromDirPin(mcpJson, disabled) + ?? projectFromUserScope(claudeJson) + ?? projectFromDirLegacy(mcpJson, disabled); if (project === null) { - // Not wired here (or wired above this directory) — the marker is the - // other thing `config claude` writes, and skills sync already trusts it. const marker = await findProjectMarker(dir, homeDir()).catch(() => null); project = marker?.project ?? null; } diff --git a/src/cli/src/config/claude-mcp.ts b/src/cli/src/config/claude-mcp.ts index d426b26..2452e93 100644 --- a/src/cli/src/config/claude-mcp.ts +++ b/src/cli/src/config/claude-mcp.ts @@ -100,16 +100,30 @@ export function isLegacyMcpctlEntry(name: string, entry: unknown): boolean { return projectOfEntry(entry) === name; } +/** The project the canonical `mcpctl` entry mounts, or null if there isn't one. */ +export function canonicalProjectIn(config: Pick | null | undefined): string | null { + return projectOfEntry(config?.mcpServers?.[MCPCTL_SERVER_NAME]); +} + +/** + * Legacy project-named entries still present, in file order. + * + * Kept separate from the canonical entry because the two mean different things + * to a reader: the canonical entry is a deliberate pin, a legacy entry is + * residue from an older mcpctl that nothing has cleaned up yet. Callers that + * rank sources (the status line) must be able to tell them apart. + */ +export function legacyEntriesIn(config: Pick | null | undefined): { server: string; project: string }[] { + const servers = config?.mcpServers; + if (!servers) return []; + return Object.entries(servers) + .filter(([name, entry]) => isLegacyMcpctlEntry(name, entry)) + .map(([name]) => ({ server: name, project: name })); +} + /** The project currently mounted by `.mcp.json`, preferring the canonical entry. */ export function activeProjectIn(config: Pick | null | undefined): string | null { - const servers = config?.mcpServers; - if (!servers) return null; - const canonical = projectOfEntry(servers[MCPCTL_SERVER_NAME]); - if (canonical !== null) return canonical; - for (const [name, entry] of Object.entries(servers)) { - if (isLegacyMcpctlEntry(name, entry)) return name; - } - return null; + return canonicalProjectIn(config) ?? legacyEntriesIn(config)[0]?.project ?? null; } export interface MergeResult { @@ -169,12 +183,39 @@ export function claudeJsonPath(env: NodeJS.ProcessEnv = process.env, homeDir?: s : join(home, '.claude.json'); } +/** Per-directory state Claude Code keeps in `.claude.json`'s `projects` map. */ +export interface ClaudeProjectEntry { + /** Servers switched off for this directory, whatever scope they came from. */ + disabledMcpServers?: string[]; + /** `.mcp.json` servers declined at the approval prompt. */ + disabledMcpjsonServers?: string[]; + [key: string]: unknown; +} + /** Shape of the bits of `.claude.json` we touch. Everything else is preserved. */ export interface ClaudeJson { mcpServers?: Record; + projects?: Record; [key: string]: unknown; } +/** + * Server names Claude Code has switched off in `dir`. + * + * A disabled server is not mounted, so naming its project as "active" is a + * plain lie — this is what lets the status line skip one. Only *explicit* + * disables count: a `.mcp.json` server in neither list is pending its approval + * prompt, and treating pending as off would blank the status line on a fresh + * checkout, which is the more confusing failure. + */ +export function disabledServersFor(doc: ClaudeJson | null | undefined, dir: string): Set { + const entry = doc?.projects?.[dir]; + return new Set([ + ...(Array.isArray(entry?.disabledMcpServers) ? entry.disabledMcpServers : []), + ...(Array.isArray(entry?.disabledMcpjsonServers) ? entry.disabledMcpjsonServers : []), + ]); +} + /** * Set the user-scope entry, returning the new document and any legacy * project-named entries retired from it. diff --git a/src/cli/tests/commands/claude.test.ts b/src/cli/tests/commands/claude.test.ts index 9f43bd6..f29a00b 100644 --- a/src/cli/tests/commands/claude.test.ts +++ b/src/cli/tests/commands/claude.test.ts @@ -359,4 +359,46 @@ describe('config claude — user scope', () => { process.exitCode = prevExit; expect(output.join('\n')).toContain("unknown --scope 'global'"); }); + + // A user-scope switch never rewrites a directory's .mcp.json, so anything of + // ours left in one keeps answering in that directory. Saying so is the only + // way the user finds out — the switch otherwise reports plain success. + describe('warns when the working directory contradicts the switch', () => { + const switchTo = async (project: string): Promise => { + await createConfigCommand({ configDeps: {}, log, cwd: () => tmpDir }) + .parseAsync(['claude', '--project', project, '--skip-skills', '--skip-ui'], { from: 'user' }); + return output.join('\n'); + }; + + it('names a legacy entry that stays mounted alongside the new project', async () => { + writeFileSync(join(tmpDir, '.mcp.json'), JSON.stringify({ + mcpServers: { homeautomation: { command: 'mcpctl', args: ['mcp', '-p', 'homeautomation'] } }, + })); + const out = await switchTo('sre'); + expect(out).toContain(join(tmpDir, '.mcp.json')); + expect(out).toContain("'homeautomation'"); + expect(out).toContain('mounted alongside'); + }); + + it('says a canonical pin overrides the switch in that directory', async () => { + writeFileSync(join(tmpDir, '.mcp.json'), JSON.stringify({ + mcpServers: { mcpctl: { command: 'mcpctl', args: ['mcp', '-p', 'docmost'] } }, + })); + expect(await switchTo('sre')).toContain('overrides the switch here'); + }); + + it('stays quiet when the directory already agrees, or wires nothing of ours', async () => { + writeFileSync(join(tmpDir, '.mcp.json'), JSON.stringify({ + mcpServers: { + mcpctl: { command: 'mcpctl', args: ['mcp', '-p', 'sre'] }, + 'their-server': { command: 'docker', args: ['run', 'x'] }, + }, + })); + expect(await switchTo('sre')).not.toContain('Warning:'); + }); + + it('stays quiet when there is no .mcp.json at all', async () => { + expect(await switchTo('sre')).not.toContain('Warning:'); + }); + }); }); diff --git a/src/cli/tests/commands/statusline.test.ts b/src/cli/tests/commands/statusline.test.ts new file mode 100644 index 0000000..da6b40b --- /dev/null +++ b/src/cli/tests/commands/statusline.test.ts @@ -0,0 +1,121 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { createStatuslineCommand } from '../../src/commands/statusline.js'; + +/** + * The status line is what tells you which project you are in, so the property + * under test throughout is: after a switch, does it name the project you + * switched to? + * + * These drive the real command rather than the resolution helpers, because the + * bug they cover was in the *ranking* of sources, not in any one source. + */ + +const bridge = (project: string): Record => ({ + command: 'mcpctl', + args: ['mcp', '-p', project], +}); + +let home: string; +let dir: string; + +/** Claude Code's user-scope config, at the path `claudeJsonPath()` resolves. */ +function writeClaudeJson(doc: unknown): void { + writeFileSync(join(home, '.claude.json'), JSON.stringify(doc)); +} + +function writeMcpJson(doc: unknown): void { + writeFileSync(join(dir, '.mcp.json'), JSON.stringify(doc)); +} + +/** Run `statusline` for `dir` and return exactly what it printed. */ +async function statusline(): Promise { + const out: string[] = []; + const cmd = createStatuslineCommand({ log: (l) => out.push(l), cwd: () => dir, homeDir: () => home }); + await cmd.parseAsync(['--directory', dir], { from: 'user' }); + return out.join(''); +} + +beforeEach(() => { + home = mkdtempSync(join(tmpdir(), 'mcpctl-statusline-home-')); + dir = mkdtempSync(join(tmpdir(), 'mcpctl-statusline-dir-')); + // Point claudeJsonPath() at the fake home; the CLI reads $CLAUDE_CONFIG_DIR + // first, which keeps this off the developer's real ~/.claude.json. + process.env['CLAUDE_CONFIG_DIR'] = home; +}); + +afterEach(() => { + delete process.env['CLAUDE_CONFIG_DIR']; + rmSync(home, { recursive: true, force: true }); + rmSync(dir, { recursive: true, force: true }); +}); + +describe('mcpctl statusline', () => { + it('reports the user-scope project when the directory wires nothing', async () => { + writeClaudeJson({ mcpServers: { mcpctl: bridge('sre') } }); + expect(await statusline()).toBe('mcpctl:sre'); + }); + + it('prints nothing at all when no project is active', async () => { + writeClaudeJson({ mcpServers: {} }); + expect(await statusline()).toBe(''); + }); + + it('lets a canonical .mcp.json pin override the user-scope project', async () => { + // Same server name in both scopes: Claude Code prefers project scope, so a + // deliberate pin is genuinely what is mounted here. + writeClaudeJson({ mcpServers: { mcpctl: bridge('sre') } }); + writeMcpJson({ mcpServers: { mcpctl: bridge('docmost') } }); + expect(await statusline()).toBe('mcpctl:docmost'); + }); + + it('does not let a legacy project-named entry outrank a user-scope switch', async () => { + // The regression: an older mcpctl wrote `homeautomation` into a checkout, + // and a user-scope switch never rewrites that file — so the status line + // reported the old project forever and the switch looked like a no-op. + writeClaudeJson({ mcpServers: { mcpctl: bridge('sre') } }); + writeMcpJson({ mcpServers: { homeautomation: bridge('homeautomation') } }); + expect(await statusline()).toBe('mcpctl:sre'); + }); + + it('still reports a legacy entry when nothing more deliberate names a project', async () => { + writeClaudeJson({ mcpServers: {} }); + writeMcpJson({ mcpServers: { homeautomation: bridge('homeautomation') } }); + expect(await statusline()).toBe('mcpctl:homeautomation'); + }); + + it('skips a directory server Claude Code has switched off', async () => { + // A disabled server is not mounted, so naming its project is a lie. + writeClaudeJson({ + mcpServers: {}, + projects: { [dir]: { disabledMcpServers: ['homeautomation'] } }, + }); + writeMcpJson({ mcpServers: { homeautomation: bridge('homeautomation') } }); + expect(await statusline()).toBe(''); + }); + + it('skips a disabled pin and falls through to the user-scope project', async () => { + writeClaudeJson({ + mcpServers: { mcpctl: bridge('sre') }, + projects: { [dir]: { disabledMcpjsonServers: ['mcpctl'] } }, + }); + writeMcpJson({ mcpServers: { mcpctl: bridge('docmost') } }); + expect(await statusline()).toBe('mcpctl:sre'); + }); + + it('falls back to a .mcpctl-project marker when nothing is wired', async () => { + writeClaudeJson({ mcpServers: {} }); + writeFileSync(join(dir, '.mcpctl-project'), 'lab\n'); + expect(await statusline()).toBe('mcpctl:lab'); + }); + + it('honours a custom prefix', async () => { + writeClaudeJson({ mcpServers: { mcpctl: bridge('sre') } }); + const out: string[] = []; + const cmd = createStatuslineCommand({ log: (l) => out.push(l), cwd: () => dir, homeDir: () => home }); + await cmd.parseAsync(['--directory', dir, '--prefix', 'proj '], { from: 'user' }); + expect(out.join('')).toBe('proj sre'); + }); +}); diff --git a/src/cli/tests/config/claude-mcp.test.ts b/src/cli/tests/config/claude-mcp.test.ts index 45a1a0a..d4541da 100644 --- a/src/cli/tests/config/claude-mcp.test.ts +++ b/src/cli/tests/config/claude-mcp.test.ts @@ -5,6 +5,9 @@ import { projectOfEntry, isLegacyMcpctlEntry, activeProjectIn, + canonicalProjectIn, + legacyEntriesIn, + disabledServersFor, } from '../../src/config/claude-mcp.js'; const bridge = (project: string): Record => ({ @@ -62,6 +65,50 @@ describe('activeProjectIn', () => { }); }); +describe('canonicalProjectIn / legacyEntriesIn', () => { + it('tells a deliberate pin apart from pre-migration residue', () => { + const config = { mcpServers: { [MCPCTL_SERVER_NAME]: bridge('sre'), homeautomation: bridge('homeautomation') } }; + expect(canonicalProjectIn(config)).toBe('sre'); + expect(legacyEntriesIn(config)).toEqual([{ server: 'homeautomation', project: 'homeautomation' }]); + }); + + it('reports no canonical entry when only legacy ones are present', () => { + const config = { mcpServers: { docmost: bridge('docmost') } }; + expect(canonicalProjectIn(config)).toBeNull(); + expect(legacyEntriesIn(config)).toEqual([{ server: 'docmost', project: 'docmost' }]); + }); + + it('leaves servers that are not ours out of both', () => { + const config = { mcpServers: { other: { command: 'echo' } } }; + expect(canonicalProjectIn(config)).toBeNull(); + expect(legacyEntriesIn(config)).toEqual([]); + expect(legacyEntriesIn(null)).toEqual([]); + }); +}); + +describe('disabledServersFor', () => { + const doc = { + projects: { + '/repo': { disabledMcpServers: ['homeautomation'], disabledMcpjsonServers: ['mcpctl'] }, + '/other': { disabledMcpServers: ['sre'] }, + }, + }; + + it('unions both of Claude Code\'s disable lists for that directory', () => { + expect([...disabledServersFor(doc, '/repo')].sort()).toEqual(['homeautomation', 'mcpctl']); + }); + + it('is scoped to the directory asked about', () => { + expect([...disabledServersFor(doc, '/other')]).toEqual(['sre']); + expect([...disabledServersFor(doc, '/unknown')]).toEqual([]); + expect([...disabledServersFor(null, '/repo')]).toEqual([]); + }); + + it('survives a malformed entry rather than throwing on the status line', () => { + expect([...disabledServersFor({ projects: { '/repo': { disabledMcpServers: 'nope' } } }, '/repo')]).toEqual([]); + }); +}); + describe('mergeMcpctlServers', () => { it('writes one constant entry regardless of project', () => { const { config } = mergeMcpctlServers(null, { project: 'my-fancy-project' }); -- 2.49.1 From dd29f98f82b8ac8b3a62f17bbc0ba97d3370026f Mon Sep 17 00:00:00 2001 From: Michal Date: Mon, 10 Aug 2026 16:38:19 +0100 Subject: [PATCH 2/3] build: refuse to package from a branch that is behind main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Everyone branches off main and builds from their own branch. A branch that is behind main still builds and installs perfectly — it just packages a binary missing whatever landed on main meanwhile, and `rpm -U --force` overwrites the good one with it. Nothing reports an error; the release succeeds and the feature simply vanishes from the installed CLI. That is what happened today: a build from a stale checkout replaced /usr/bin/mcpctl with one that has no `statusline` command at all, months after the status line landed on main (`mcpctl statusline` -> "unknown command"). `check-main-sync.sh` fetches main, compares, and fails before any work happens, listing the commits the branch is missing and the merge that fixes it. Sourced by build-rpm.sh and build-deb.sh — both are run standalone, so neither can rely on the other having checked. A hard failure rather than a warning: a warning scrolls past in a build log, and the whole point is to stop before the artifact exists. MCPCTL_ALLOW_BEHIND_MAIN=1 is the escape hatch for a deliberate old-tree build; MCPCTL_BASE_BRANCH retargets the comparison. Offline it degrades to the last fetched origin/main, then a local main, saying which it used; outside a git checkout it skips. Verified: passes on this branch (up to date with main); against a synthetic ref one commit ahead, build-rpm.sh aborts with exit 1 before ensure_build_deps and before any compilation; the escape hatch bypasses it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019wUmrfkVQR6CKcYKxENq7k --- docs/project-summary.md | 18 +++++++ scripts/build-deb.sh | 5 ++ scripts/build-rpm.sh | 5 ++ scripts/check-main-sync.sh | 99 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+) create mode 100755 scripts/check-main-sync.sh diff --git a/docs/project-summary.md b/docs/project-summary.md index 95552bd..9ec9bd7 100644 --- a/docs/project-summary.md +++ b/docs/project-summary.md @@ -884,6 +884,24 @@ All pushed to `mysources.co.uk/michal/` registry. source .env && bash scripts/release.sh ``` +**The build refuses to run from a branch that is behind `main`.** Everyone +branches off main, so a stale branch still builds and installs cleanly — it just +ships a binary missing whatever landed on main meanwhile, and `rpm -U --force` +overwrites the good one with it. That happened on 2026-08-10: a build from a +stale checkout replaced `/usr/bin/mcpctl` with one that had no `statusline` +command, months after the status line landed. `scripts/check-main-sync.sh` +(sourced by `build-rpm.sh` and `build-deb.sh`) fetches `main`, compares, and +fails before any work happens, listing the commits you are missing. + +```bash +git merge main # the fix +MCPCTL_ALLOW_BEHIND_MAIN=1 bash scripts/release.sh # deliberate old-tree build +MCPCTL_BASE_BRANCH=release-2.x bash scripts/build-rpm.sh # compare to another branch +``` + +Offline it falls back to the last fetched `origin/main`, then to a local `main`, +and says which it used; outside a git checkout it skips entirely. + Installs via nfpm: - `/usr/bin/mcpctl` — CLI binary (bun compiled) - `/usr/bin/mcpctl-local` — Local proxy binary (bun compiled) diff --git a/scripts/build-deb.sh b/scripts/build-deb.sh index e8b1d07..5d0013d 100755 --- a/scripts/build-deb.sh +++ b/scripts/build-deb.sh @@ -19,6 +19,11 @@ source "$SCRIPT_DIR/arch-helper.sh" resolve_arch "${MCPCTL_TARGET_ARCH:-}" # Sets: NFPM_ARCH, BUN_TARGET, ARCH_SUFFIX +# Same guard as build-rpm.sh: this script is also run on its own, so it cannot +# rely on that one having checked. +source "$SCRIPT_DIR/check-main-sync.sh" +check_main_sync + # Check and install missing build dependencies source "$SCRIPT_DIR/ensure-deps.sh" ensure_build_deps diff --git a/scripts/build-rpm.sh b/scripts/build-rpm.sh index f2db0f1..2f45a98 100755 --- a/scripts/build-rpm.sh +++ b/scripts/build-rpm.sh @@ -19,6 +19,11 @@ source "$SCRIPT_DIR/arch-helper.sh" resolve_arch "${MCPCTL_TARGET_ARCH:-}" # Sets: NFPM_ARCH, BUN_TARGET, ARCH_SUFFIX +# Before anything expensive: a branch behind main packages a binary missing +# whatever landed there, and installing it silently downgrades the machine. +source "$SCRIPT_DIR/check-main-sync.sh" +check_main_sync + # Check and install missing build dependencies source "$SCRIPT_DIR/ensure-deps.sh" ensure_build_deps diff --git a/scripts/check-main-sync.sh b/scripts/check-main-sync.sh new file mode 100755 index 0000000..246ada2 --- /dev/null +++ b/scripts/check-main-sync.sh @@ -0,0 +1,99 @@ +#!/bin/bash +# Refuse to build a package from a branch that main has already moved past. +# +# WHY +# +# Everyone branches off main and builds from their own branch. A branch that is +# behind main still builds and installs perfectly — it just quietly ships a +# binary missing whatever landed on main in the meantime, and `rpm -U --force` +# overwrites the good one with it. +# +# That is not hypothetical: on 2026-08-10 a build from a stale checkout replaced +# /usr/bin/mcpctl with one that had no `statusline` command at all, months after +# the status line landed on main. Nothing reported an error — the release +# succeeded, the feature just vanished from the installed CLI. +# +# So this is a hard failure rather than a warning. A warning scrolls past in a +# build log; the whole point is to stop before the artifact exists. +# +# ESCAPE HATCH +# +# MCPCTL_ALLOW_BEHIND_MAIN=1 build anyway (deliberate build of an old tree) +# MCPCTL_BASE_BRANCH= compare against something other than main +# +# Skips itself entirely outside a git checkout, so tarball builds still work. + +# Resolve the ref to compare against, echoing it on stdout. Prefers a fresh +# fetch; falls back to whatever is already on disk so an offline build is +# degraded rather than blocked. Returns 1 when there is nothing to compare to. +_main_sync_ref() { + local base="$1" remote="$2" + + if [ -n "$remote" ] && git fetch --quiet "$remote" "$base" 2>/dev/null; then + # FETCH_HEAD rather than refs/remotes//: it is what this fetch + # just wrote, so it cannot be a stale opportunistic update. + echo "FETCH_HEAD" + return 0 + fi + + if [ -n "$remote" ] && git rev-parse --verify --quiet "refs/remotes/$remote/$base" >/dev/null; then + echo " (could not reach $remote — comparing against the last fetched $remote/$base)" >&2 + echo "refs/remotes/$remote/$base" + return 0 + fi + + if git rev-parse --verify --quiet "refs/heads/$base" >/dev/null; then + echo " (no reachable remote — comparing against local $base)" >&2 + echo "refs/heads/$base" + return 0 + fi + + return 1 +} + +check_main_sync() { + local base="${MCPCTL_BASE_BRANCH:-main}" + + if ! git rev-parse --git-dir >/dev/null 2>&1; then + return 0 # not a checkout; nothing to be behind + fi + + if [ "${MCPCTL_ALLOW_BEHIND_MAIN:-}" = "1" ]; then + echo "==> Skipping the '$base' sync check (MCPCTL_ALLOW_BEHIND_MAIN=1)" + return 0 + fi + + echo "==> Checking this branch is not behind '$base'..." + + local remote ref + remote="$(git remote | head -1)" + if ! ref="$(_main_sync_ref "$base" "$remote")"; then + echo " (no '$base' branch found anywhere — skipping)" + return 0 + fi + + local behind + behind="$(git rev-list --count "HEAD..$ref" 2>/dev/null || echo 0)" + if [ "$behind" -eq 0 ]; then + echo " up to date with $base" + return 0 + fi + + local branch + branch="$(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo HEAD)" + echo "" >&2 + echo "ERROR: '$branch' is $behind commit(s) behind $base — refusing to build." >&2 + echo "" >&2 + echo " Building now would package a binary without these, and installing it" >&2 + echo " would overwrite a good one with a version missing them:" >&2 + echo "" >&2 + git log --oneline --no-decorate "HEAD..$ref" | head -15 | sed 's/^/ /' >&2 + if [ "$behind" -gt 15 ]; then + echo " … and $((behind - 15)) more" >&2 + fi + echo "" >&2 + echo " Fix it: git merge $base # or: git rebase $base" >&2 + echo " Anyway: MCPCTL_ALLOW_BEHIND_MAIN=1 $0" >&2 + echo "" >&2 + return 1 +} -- 2.49.1 From 96e27c871654812738b5c008263c28f198845eaa Mon Sep 17 00:00:00 2001 From: Michal Date: Mon, 10 Aug 2026 16:41:26 +0100 Subject: [PATCH 3/3] build: extend the main-sync gate to the image build and the k8s deploy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same hazard as the package build, with the cluster on the receiving end: a branch behind main builds images missing whatever landed there, and deploy-k8s.sh pins that sha in Pulumi — making the stale build the cluster's source of truth. build-mcpd.sh gets its own call because it is run standalone as well as from deploy-k8s.sh, so neither can rely on the other having checked. `--dry-run` is exempt. It builds and cuts over nothing, and blocking a read-only inspection is exactly what teaches people to export MCPCTL_ALLOW_BEHIND_MAIN=1 permanently — which would disable the gate for the real deploys too. The failure text is now artifact-agnostic ("produce an artifact" / "shipping it"), since one helper now speaks for packages, images and deploys. Verified against a synthetic ref one commit ahead: build-mcpd.sh exits 1 before any docker work, and deploy-k8s.sh exits 1 before the test gate, the pg_dump, the image build and pulumi. Neither the working tree nor HEAD was moved to test this — the ref was built with git commit-tree and deleted afterwards. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019wUmrfkVQR6CKcYKxENq7k --- docs/project-summary.md | 10 ++++++++-- scripts/build-mcpd.sh | 5 +++++ scripts/check-main-sync.sh | 6 ++++-- scripts/deploy-k8s.sh | 14 ++++++++++++++ 4 files changed, 31 insertions(+), 4 deletions(-) diff --git a/docs/project-summary.md b/docs/project-summary.md index 9ec9bd7..23e7a75 100644 --- a/docs/project-summary.md +++ b/docs/project-summary.md @@ -890,8 +890,14 @@ ships a binary missing whatever landed on main meanwhile, and `rpm -U --force` overwrites the good one with it. That happened on 2026-08-10: a build from a stale checkout replaced `/usr/bin/mcpctl` with one that had no `statusline` command, months after the status line landed. `scripts/check-main-sync.sh` -(sourced by `build-rpm.sh` and `build-deb.sh`) fetches `main`, compares, and -fails before any work happens, listing the commits you are missing. +fetches `main`, compares, and fails before any work happens, listing the commits +you are missing. It gates every path that produces something others consume: +`build-rpm.sh`, `build-deb.sh`, `build-mcpd.sh` (each is also run standalone, so +none can rely on another having checked) and `deploy-k8s.sh` — where a stale +branch would pin its sha in Pulumi and make it the cluster's source of truth. +`deploy-k8s.sh --dry-run` skips the check: it builds and cuts over nothing, and +blocking a read-only inspection only teaches people to export the escape hatch +permanently, disabling the gate for real deploys too. ```bash git merge main # the fix diff --git a/scripts/build-mcpd.sh b/scripts/build-mcpd.sh index 08a7f3a..99ece3a 100755 --- a/scripts/build-mcpd.sh +++ b/scripts/build-mcpd.sh @@ -16,6 +16,11 @@ if [ -f .env ]; then set -a; source .env; set +a fi +# This pushes an image to the registry, so the same staleness gate as the package +# builds applies. Run standalone as well as from deploy-k8s.sh, hence its own copy. +source "$SCRIPT_DIR/check-main-sync.sh" +check_main_sync + # Push directly to internal address (external proxy has body size limit) REGISTRY="10.0.0.194:3012" IMAGE="mcpd" diff --git a/scripts/check-main-sync.sh b/scripts/check-main-sync.sh index 246ada2..05c0e3f 100755 --- a/scripts/check-main-sync.sh +++ b/scripts/check-main-sync.sh @@ -84,8 +84,10 @@ check_main_sync() { echo "" >&2 echo "ERROR: '$branch' is $behind commit(s) behind $base — refusing to build." >&2 echo "" >&2 - echo " Building now would package a binary without these, and installing it" >&2 - echo " would overwrite a good one with a version missing them:" >&2 + # Deliberately artifact-agnostic: the same helper gates RPM/DEB packages, the + # mcpd image, and the k8s deploy. + echo " Building now would produce an artifact without these, and shipping it" >&2 + echo " would replace a good one with a version missing them:" >&2 echo "" >&2 git log --oneline --no-decorate "HEAD..$ref" | head -15 | sed 's/^/ /' >&2 if [ "$behind" -gt 15 ]; then diff --git a/scripts/deploy-k8s.sh b/scripts/deploy-k8s.sh index e41efad..eff872a 100755 --- a/scripts/deploy-k8s.sh +++ b/scripts/deploy-k8s.sh @@ -80,6 +80,20 @@ cat <