fix(pi): repair the /mcpctl menu, skills target, and typecheck the extension

The pi extension shipped in `src/pi-ext/` was covered by no tsconfig and no
eslint config, so nothing ever checked it against pi's API. Pointing tsc at
the published @earendil-works/pi-coding-agent types found the command surface
to be inert.

Fixes:

- `/mcpctl` did nothing. `ctx.ui.select` takes `string[]` and returns the
  chosen string; it was called with `{value,label}` objects, so the menu
  rendered five `[object Object]` rows and `choice === "status"` never
  matched any branch. Labels are now plain strings mapped back to actions.
- The headless branch returned a status string from a handler typed
  `Promise<void>`; pi drops it. Reports via notify instead.
- "Sync skills" omitted `--agent pi`, writing into ~/.claude/skills — in an
  integration whose stated purpose is to not depend on ~/.claude — and said
  so in its own success message. It also ran execSync with `stdio: "inherit"`,
  painting raw output over pi's TUI, and interpolated the project name into a
  shell string. Now execFile with `--agent pi` and captured output.
- Tool results typed `content[].type` as `string`; pi's AgentToolResult wants
  the `"text"` literal.
- `callTool` asserted `Promise<unknown>` to `ToolCallResult`.
- Sanitising MCP tool names to `[a-z0-9_]` can collide (`docs.search` vs
  `docs-search`). The colliding tool was silently never registered but still
  reported active, so its calls were forwarded to the first tool. Names are
  now disambiguated and tracked with the MCP tool they forward to.
- `registerWithPi` rewrote settings.json even when nothing changed. Since
  parsing strips `//` comments, a no-op run destroyed them.

Guards, so this class of bug can't return:

- `src/pi-ext/tsconfig.json` checks the extension against the real published
  pi types (dev dependency, not a shim — a shim drifting from the published
  API is the exact failure being guarded). Wired into `pnpm typecheck`.
- eslint now covers `src/pi-ext/*.ts` like every other source file.
- A test fails if the embedded copy in `config/pi-extension.ts` is stale;
  editing the sources without regenerating silently shipped old code.

Also: the branch added `config pi` without regenerating shell completions
(the committed-completions test was failing), and the doc advertised
`mcpctl pi sync-skills`, which does not exist. Both corrected, plus a note
on the session-token vs `mcpctl_pat_` bearer difference that would bite
against an authenticated `mcplocal serve`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017BMXdb2qZbPSh8Q7XpTyjB

src/cli/src/utils/pi-settings.ts

@@ -152,7 +152,12 @@ export async function registerWithPi(
     addedSkills.push(skillsDir);
   }
 
-  await writeSettings(settingsPath, settings);
+  // Only rewrite when something actually changed. Re-serialising drops any
+  // comments the file had (readSettings strips them to parse), so a no-op run
+  // must not silently reformat a hand-maintained settings.json.
+  if (addedExtensions.length > 0 || addedSkills.length > 0) {
+    await writeSettings(settingsPath, settings);
+  }
   return { addedExtensions, addedSkills };
 }
 

src/cli/tests/config/pi-extension-embed.test.ts

@@ -0,0 +1,45 @@
+import { describe, it, expect } from 'vitest';
+import { readFileSync } from 'node:fs';
+import { join } from 'node:path';
+import { PI_EXTENSION_FILES, PI_EXTENSION_FILENAMES } from '../../src/config/pi-extension.js';
+
+/**
+ * `mcpctl config pi` installs the *embedded* copy of the extension, not the
+ * files in src/pi-ext/. Editing the sources without re-running the generator
+ * therefore ships stale code to users while the repo looks correct — and the
+ * embedded copy is the one thing no typecheck covers. Same guarantee the
+ * completions check gives.
+ */
+const repoRoot = join(import.meta.dirname, '..', '..', '..', '..');
+const piExtDir = join(repoRoot, 'src', 'pi-ext');
+
+describe('embedded pi extension', () => {
+  it('matches the sources in src/pi-ext (re-run scripts/generate-pi-extension.ts)', () => {
+    for (const name of PI_EXTENSION_FILENAMES) {
+      const onDisk = readFileSync(join(piExtDir, name), 'utf-8');
+      expect(PI_EXTENSION_FILES[name], `${name} is embedded`).toBeDefined();
+      expect(PI_EXTENSION_FILES[name], `${name} is stale — regenerate the embed`).toBe(onDisk);
+    }
+  });
+
+  it('embeds every file the extension needs to load', () => {
+    // mcpctl-pi.ts imports ./mcp-http.js — installing one without the other
+    // yields an extension pi cannot load.
+    expect(Object.keys(PI_EXTENSION_FILES).sort()).toEqual(['mcp-http.ts', 'mcpctl-pi.ts']);
+    expect(PI_EXTENSION_FILES['mcpctl-pi.ts']).toContain('./mcp-http.js');
+  });
+
+  it('carries the fixes the pi API requires', () => {
+    const main = PI_EXTENSION_FILES['mcpctl-pi.ts'] ?? '';
+    // ctx.ui.select takes string[] and returns the chosen string.
+    expect(main).not.toMatch(/select\([^)]*\[\s*\{\s*value:/);
+    // Skills must land in pi's tree, never ~/.claude: the sync passes
+    // --agent pi and no longer tells the user it wrote to Claude's tree.
+    expect(main).toContain('"--agent", "pi"');
+    expect(main).not.toContain('(into ~/.claude/skills)');
+    // Spawned without a shell, so the project name is never interpolated into
+    // a command string. Matches the call, not prose mentioning it.
+    expect(main).toContain('execFile("mcpctl", args');
+    expect(main).not.toMatch(/execSync\s*\(/);
+  });
+});

src/pi-ext/mcp-http.ts

@@ -220,7 +220,8 @@ export class McpHttpSession {
 
   /** Call a tool. */
   async callTool(name: string, args: Record<string, unknown> = {}): Promise<ToolCallResult> {
-    return this.send('tools/call', { name, arguments: args }) as ToolCallResult;
+    const result = await this.send('tools/call', { name, arguments: args });
+    return result as ToolCallResult;
   }
 
   /** Close the session (DELETE). Safe when no session has been negotiated. */

src/pi-ext/mcpctl-pi.ts

@@ -200,8 +200,8 @@ export default function (pi: ExtensionAPI) {
   let activeProject: string | null = null;
   /** Persistent session for the active project — MUST be reused to keep gate state. */
   let session: McpHttpSession | null = null;
-  /** Pi tool names currently registered for the active project. */
-  const projectToolNames = new Set<string>();
+  /** Registered pi tool name → the MCP tool it forwards to, for the active project. */
+  const projectToolNames = new Map<string, string>();
   let activeCtx: ExtensionContext | null = null;
 
   function endpointUrl(): string {
@@ -234,19 +234,41 @@ export default function (pi: ExtensionAPI) {
     const { tools } = await sess.listTools();
     const names: string[] = [];
     for (const t of tools) {
-      const piName = piToolName(project, t.name);
-      if (!projectToolNames.has(piName)) {
-        pi.registerTool(createTool(piName, t, project));
-        projectToolNames.add(piName);
-      }
+      const piName = uniqueToolName(project, t.name);
+      if (piName === null) continue; // already registered for this same MCP tool
+      pi.registerTool(createTool(piName, t, project));
+      projectToolNames.set(piName, t.name);
       names.push(piName);
     }
+    // Names already registered for tools still in the list stay active.
+    for (const [piName, mcpName] of projectToolNames) {
+      if (!names.includes(piName) && tools.some((t) => t.name === mcpName)) names.push(piName);
+    }
     // Activate built-ins/other extensions + all tools discovered in this project.
     const active = pi.getActiveTools().filter((n) => !n.startsWith("mc_"));
     pi.setActiveTools([...new Set([...active, ...names])]);
     return { tools: names };
   }
 
+  /**
+   * The pi tool name to register `mcpTool` under, or null if it is already
+   * registered. Sanitising MCP names down to `[a-z0-9_]` can collide (e.g.
+   * `docs.search` and `docs-search`); without a suffix the second tool would
+   * silently never be registered while still being reported as available, and
+   * its calls would be forwarded to the first tool instead.
+   */
+  function uniqueToolName(project: string, mcpTool: string): string | null {
+    const base = piToolName(project, mcpTool);
+    if (projectToolNames.get(base) === mcpTool) return null;
+    if (!projectToolNames.has(base)) return base;
+    for (let i = 2; i < 100; i++) {
+      const candidate = `${base}_${String(i)}`;
+      if (projectToolNames.get(candidate) === mcpTool) return null;
+      if (!projectToolNames.has(candidate)) return candidate;
+    }
+    return null;
+  }
+
   function createTool(piName: string, t: ToolInfo, project: string) {
     const mcpName = t.name;
     return {
@@ -271,7 +293,9 @@ export default function (pi: ExtensionAPI) {
           throw new Error(text || `${mcpName} returned an error`);
         }
         return {
-          content: [{ type: "text", text: text || "(no text content)" }],
+          // `type` must be the literal "text" — pi's AgentToolResult content is
+          // a TextContent | ImageContent union, not { type: string }.
+          content: [{ type: "text" as const, text: text || "(no text content)" }],
           details: { mcpTool: mcpName, project, raw: res },
         };
       },
@@ -287,20 +311,34 @@ export default function (pi: ExtensionAPI) {
   }
 
   // ── /mcpctl command ──
+  /**
+   * Menu labels → actions. `ctx.ui.select` takes plain strings and returns the
+   * chosen string, so the mapping has to live here rather than in the options.
+   */
+  const MENU: ReadonlyArray<readonly [label: string, action: string]> = [
+    ["Status", "status"],
+    ["Switch project", "switch"],
+    ["Refresh tools", "refresh"],
+    ["Sync skills into pi's skills dir", "sync"],
+    ["Close", "close"],
+  ];
+
   pi.registerCommand("mcpctl", {
     description: "mcpctl integration: status, switch project, refresh tools",
     handler: async (_args, ctx) => {
       if (!ctx.hasUI) {
-        return `mcpctl: active project = ${activeProject ?? "none"}; mcplocal = ${mcplocalUrl}`;
+        // A command handler resolves to void — pi ignores a returned string, so
+        // headless status has to go through notify.
+        ctx.ui.notify(
+          `mcpctl: active project = ${activeProject ?? "none"}; mcplocal = ${mcplocalUrl}`,
+          "info",
+        );
+        return;
       }
-      const choice = await ctx.ui.select("mcpctl", [
-        { value: "status", label: "Status" },
-        { value: "switch", label: "Switch project" },
-        { value: "refresh", label: "Refresh tools" },
-        { value: "sync skills", label: "Sync skills (mcpctl skills sync)" },
-        { value: "close", label: "Close" },
-      ]);
-      if (!choice) return;
+      const picked = await ctx.ui.select("mcpctl", MENU.map(([label]) => label));
+      if (!picked) return;
+      const choice = MENU.find(([label]) => label === picked)?.[1];
+      if (!choice || choice === "close") return;
 
       if (choice === "status") {
         const cs = await readConfig();
@@ -335,14 +373,28 @@ export default function (pi: ExtensionAPI) {
         return;
       }
 
-      if (choice === "sync skills") {
+      if (choice === "sync") {
         try {
-          const { execSync } = await import("node:child_process");
-          execSync(`mcpctl skills sync${activeProject ? ` -p ${activeProject}` : ""} --quiet`, {
-            stdio: "inherit",
-            cwd: process.cwd(),
+          // `--agent pi` is what keeps skills in ~/.pi/agent/skills; without it
+          // this syncs into ~/.claude/skills, which the whole point of this
+          // integration is to avoid depending on.
+          const args = ["skills", "sync", "--agent", "pi", "--quiet"];
+          if (activeProject) args.push("-p", activeProject);
+          // execFile, not execSync+stdio:"inherit": a shell would need the
+          // project name quoted, and inherited stdio writes raw output over
+          // pi's TUI. Capture instead and report through notify.
+          const { execFile } = await import("node:child_process");
+          const stderr = await new Promise<string>((resolve, reject) => {
+            execFile("mcpctl", args, { timeout: 120_000 }, (err, _stdout, errOut) => {
+              if (err) reject(new Error((errOut || err.message).trim()));
+              else resolve(errOut);
+            });
           });
-          ctx.ui.notify("mcpctl skills synced (into ~/.claude/skills)", "info");
+          const detail = stderr.trim().split("\n").pop();
+          ctx.ui.notify(
+            `mcpctl skills synced into pi's skills dir${detail ? ` — ${detail}` : ""}. Run /reload to pick them up.`,
+            "info",
+          );
         } catch (e) {
           ctx.ui.notify(`skills sync failed: ${(e as Error).message}`, "error");
         }

src/pi-ext/tsconfig.json

@@ -0,0 +1,26 @@
+{
+  "//": [
+    "The pi extension is shipped as source (embedded in the CLI, then written",
+    "into ~/.pi/agent/extensions/) and is therefore never compiled by the CLI's",
+    "own build. Without this project it was typechecked by nothing, which is how",
+    "a ctx.ui.select() call with the wrong option shape shipped.",
+    "",
+    "It is checked against the REAL @earendil-works/pi-coding-agent types (a dev",
+    "dependency) rather than a hand-written shim, because a shim drifting from",
+    "the published API is the exact failure mode this guards against."
+  ],
+  "compilerOptions": {
+    "target": "ES2022",
+    "module": "ESNext",
+    "moduleResolution": "bundler",
+    "lib": ["ES2022"],
+    "types": ["node"],
+    "strict": true,
+    "noImplicitOverride": true,
+    "noUncheckedIndexedAccess": false,
+    "noEmit": true,
+    "skipLibCheck": true,
+    "forceConsistentCasingInFileNames": true
+  },
+  "files": ["mcpctl-pi.ts", "mcp-http.ts"]
+}