Просмотр исходного кода

fix(installer): copilot-cli detection false-positived on VS Code's ~/.copilot/ide locks

The VS Code Copilot Chat extension writes MCP socket-handoff lock files
into ~/.copilot/ide/ on launch, so `existsSync(~/.copilot)` reported the
Copilot CLI as installed on any machine that merely has the VS Code
extension (caught live on the maintainer's Mac). Detection now counts
the dir as a CLI footprint only when it holds something besides `ide`.

Also: uninstalling a from-scratch install now deletes mcp-config.json
instead of leaving a `{}` husk that would keep detect() reporting the
CLI as installed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Colby McHenry 1 месяц назад
Родитель
Сommit
234dfe60cd
2 измененных файлов с 70 добавлено и 5 удалено
  1. 42 3
      __tests__/installer-targets.test.ts
  2. 28 2
      src/installer/targets/copilot-cli.ts

+ 42 - 3
__tests__/installer-targets.test.ts

@@ -2162,13 +2162,26 @@ describe('Installer targets — Copilot family', () => {
     expect(after.banner).toBe('never');
   });
 
-  it('copilot-cli: uninstall drops an emptied mcpServers wrapper', () => {
+  it('copilot-cli: uninstall of a from-scratch install deletes the file — no `{}` husk to fool detect()', () => {
     const t = getTarget('copilot-cli')!;
     t.install('global', { autoAllow: true });
     t.uninstall('global');
     const file = path.join(tmpHome, '.copilot', 'mcp-config.json');
+    // A leftover empty mcp-config.json would count as a CLI footprint
+    // and keep the target showing as detected after uninstall.
+    expect(fs.existsSync(file)).toBe(false);
+  });
+
+  it('copilot-cli: uninstall keeps the file when unrelated top-level keys remain', () => {
+    const t = getTarget('copilot-cli')!;
+    const file = path.join(tmpHome, '.copilot', 'mcp-config.json');
+    fs.mkdirSync(path.dirname(file), { recursive: true });
+    fs.writeFileSync(file, JSON.stringify({ banner: 'never' }, null, 2) + '\n');
+    t.install('global', { autoAllow: true });
+    t.uninstall('global');
     const after = JSON.parse(fs.readFileSync(file, 'utf-8'));
     expect(after.mcpServers).toBeUndefined();
+    expect(after.banner).toBe('never');
   });
 
   it('copilot-cli: uninstall when never installed reports not-found, no throw', () => {
@@ -2184,15 +2197,41 @@ describe('Installer targets — Copilot family', () => {
     expect(t.uninstall('global').files[0].action).toBe('not-found');
   });
 
-  it('copilot-cli: detect() reports installed from the ~/.copilot dir alone', () => {
+  it('copilot-cli: detect() reports installed from CLI artifacts in ~/.copilot', () => {
     const t = getTarget('copilot-cli')!;
     // The tmp PATH may or may not carry a real `copilot` binary; only
-    // assert the positive signal we control.
+    // assert the positive signal we control. The CLI writes config.json
+    // on first run — that's the footprint.
     fs.mkdirSync(path.join(tmpHome, '.copilot'), { recursive: true });
+    fs.writeFileSync(path.join(tmpHome, '.copilot', 'config.json'), '{}');
     expect(t.detect('global').installed).toBe(true);
     expect(t.detect('global').alreadyConfigured).toBe(false);
   });
 
+  it('copilot-cli: detect() is NOT fooled by the VS Code extension\'s ~/.copilot/ide/ locks', () => {
+    // The VS Code Copilot Chat extension writes MCP socket-handoff lock
+    // files into ~/.copilot/ide/ on every launch — a machine with only
+    // the extension has ~/.copilot with a lone `ide` entry and no CLI.
+    const t = getTarget('copilot-cli')!;
+    const ideDir = path.join(tmpHome, '.copilot', 'ide');
+    fs.mkdirSync(ideDir, { recursive: true });
+    fs.writeFileSync(path.join(ideDir, 'some-uuid.lock'), '{"socketPath":"/tmp/mcp.sock"}');
+
+    // Pin PATH to an empty dir so a real `copilot` binary on the host
+    // can't turn this negative assertion into a false failure.
+    const prevPath = process.env.PATH;
+    process.env.PATH = ideDir;
+    try {
+      expect(t.detect('global').installed).toBe(false);
+
+      // An empty ~/.copilot (no CLI footprint at all) is also not enough.
+      fs.rmSync(ideDir, { recursive: true });
+      expect(t.detect('global').installed).toBe(false);
+    } finally {
+      process.env.PATH = prevPath;
+    }
+  });
+
   it('copilot-cli: printConfig matches what install writes; local variant points at --location=global', () => {
     const t = getTarget('copilot-cli')!;
     const printed = snippetJson(t.printConfig('global'));

+ 28 - 2
src/installer/targets/copilot-cli.ts

@@ -55,6 +55,25 @@ function mcpConfigPath(): string {
   return path.join(configDir(), 'mcp-config.json');
 }
 
+/**
+ * `~/.copilot` existing is NOT proof the CLI is installed: the VS Code
+ * Copilot Chat extension drops MCP socket-handoff lock files into
+ * `~/.copilot/ide/` on launch, so a machine with only the VS Code
+ * extension still has the dir (with a lone `ide` entry). Count the dir
+ * as a CLI footprint only when it holds anything besides `ide` — the
+ * CLI writes `config.json` (and later `mcp-config.json`, history state)
+ * on first run.
+ */
+function cliConfigDirPresent(): boolean {
+  let entries: string[];
+  try {
+    entries = fs.readdirSync(configDir());
+  } catch {
+    return false;
+  }
+  return entries.some((e) => e !== 'ide');
+}
+
 /**
  * Best-effort check that the `copilot` binary is reachable on PATH.
  * A plain fs scan (no shell-out) — cheap enough to run inside
@@ -97,7 +116,7 @@ class CopilotCliTarget implements AgentTarget {
     const file = mcpConfigPath();
     const config = readJsonFile(file);
     const alreadyConfigured = !!config.mcpServers?.codegraph;
-    const installed = fs.existsSync(configDir()) || copilotOnPath();
+    const installed = cliConfigDirPresent() || copilotOnPath();
     return { installed, alreadyConfigured, configPath: file };
   }
 
@@ -129,7 +148,14 @@ class CopilotCliTarget implements AgentTarget {
     if (Object.keys(config.mcpServers).length === 0) {
       delete config.mcpServers;
     }
-    writeJsonFile(file, config);
+    if (Object.keys(config).length === 0) {
+      // Nothing left but the `{}` we'd write back — delete the file so
+      // uninstall fully reverses a from-scratch install. A leftover
+      // empty file would keep detect() reporting the CLI as installed.
+      fs.unlinkSync(file);
+    } else {
+      writeJsonFile(file, config);
+    }
     return { files: [{ path: file, action: 'removed' }] };
   }