From 98718cf6dbbca893ec53def44ce9f030619239d9 Mon Sep 17 00:00:00 2001 From: Lukas <103962359+L4XB@users.noreply.github.com> Date: Sun, 27 Sep 2026 14:25:05 +0200 Subject: [PATCH] fix(installer): keep --refresh off the configs of unconfigured agents (#1870) (#1873) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readJsonFile` copied an unparseable file to `.backup` and warned that it was about to be overwritten. That is the right thing to do just before a write, but every target's `detect()` reads its agent's config through the same helper — and `install --refresh` detects every target in order to skip the ones codegraph was never installed into. So an empty `~/.gemini/config/mcp_config.json`, sitting beside an Antigravity install codegraph does not manage, got a 0-byte `mcp_config.json.backup` and a warning about an overwrite that never came. Detection is meant to be read-only. `readJsonFile` is now a pure read. Preserving an unparseable config moves to `writeJsonFile`, the one place a target replaces a JSON config — so the backup still happens wherever it mattered, and only there. Reads that decide not to write (the legacy-entry cleanups, the uninstall sweeps) no longer leave one behind either. Unchanged on purpose: an install that really does overwrite an empty config still backs it up. Skipping the copy when there is nothing to preserve is a separate call. --- CHANGELOG.md | 2 + __tests__/installer-targets.test.ts | 81 ++++++++++++++++++++++++++++- src/installer/targets/shared.ts | 47 +++++++++++++---- src/installer/targets/types.ts | 7 +++ 4 files changed, 126 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3561f2580..1b7e5b027 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -198,6 +198,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - `codegraph install` now honors `CLAUDE_CONFIG_DIR` and `CODEX_HOME` for global Claude Code and Codex setup so CodeGraph loads in your chosen profile (thanks @seanchann; #1627). +- `codegraph install --refresh` no longer leaves a `.backup` file beside the config of an agent it isn't set up for. Checking whether an agent already has CodeGraph meant reading its config, and a config that wasn't valid JSON — an empty `mcp_config.json` next to an Antigravity install, say — got copied aside with a warning that CodeGraph was about to overwrite it, even though the agent was then skipped untouched. Checking is silent now; a config that can't be read is still backed up, but only when CodeGraph really is replacing it. Thanks @Gotman08. (#1870) + - Files opted in with `includeIgnored` now stay indexed on Git older than 2.36, and embedded repositories remain visible to the watcher (thanks @maxmilian and @newshowardz777; #1549). - `codegraph init` and `codegraph index` now list unsupported file extensions and explain that CodeGraph is inactive when no supported source files are found (#1502). diff --git a/__tests__/installer-targets.test.ts b/__tests__/installer-targets.test.ts index 3b740c7ed..bc32f30fa 100644 --- a/__tests__/installer-targets.test.ts +++ b/__tests__/installer-targets.test.ts @@ -14,7 +14,7 @@ * touched. */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; @@ -1761,6 +1761,85 @@ describe('Installer — refreshTargets sweep (codegraph install --refresh)', () }); }); +describe('Installer — detection never writes (#1870)', () => { + let tmpHome: string; + let tmpCwd: string; + let origCwd: string; + let homeRestore: { restore: () => void }; + + // Antigravity present with an MCP config JSON.parse rejects, and + // codegraph never installed into it — the reported setup: an empty + // (0-byte) `~/.gemini/config/mcp_config.json`. + function plantUnparseableAntigravityConfig(content: string): string { + const dir = path.join(tmpHome, '.gemini', 'config'); + fs.mkdirSync(dir, { recursive: true }); + const file = path.join(dir, 'mcp_config.json'); + fs.writeFileSync(file, content); + return file; + } + + beforeEach(() => { + tmpHome = mkTmpDir('detect-home'); + tmpCwd = mkTmpDir('detect-cwd'); + origCwd = process.cwd(); + process.chdir(tmpCwd); + homeRestore = setHome(tmpHome); + }); + + afterEach(() => { + homeRestore.restore(); + process.chdir(origCwd); + fs.rmSync(tmpHome, { recursive: true, force: true }); + fs.rmSync(tmpCwd, { recursive: true, force: true }); + }); + + it('a refresh leaves an unconfigured agent\'s unparseable config alone — no backup, no warning', () => { + const file = plantUnparseableAntigravityConfig(''); + + // Asserted inside the spy: `mockRestore` also clears the recorded + // calls, so a `not.toHaveBeenCalled()` after it always passes. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const reports = refreshTargets(ALL_TARGETS, 'global'); + expect(reports.find((r) => r.id === 'antigravity')!.status).toBe('not-configured'); + expect(fs.readdirSync(path.dirname(file))).toEqual(['mcp_config.json']); + expect(fs.readFileSync(file, 'utf-8')).toBe(''); + expect(warn).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); + + it('detect() on its own touches nothing', () => { + const file = plantUnparseableAntigravityConfig('{ half a config'); + + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + expect(getTarget('antigravity')!.detect('global').alreadyConfigured).toBe(false); + expect(fs.readdirSync(path.dirname(file))).toEqual(['mcp_config.json']); + expect(fs.readFileSync(file, 'utf-8')).toBe('{ half a config'); + expect(warn).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); + + it('an install that really overwrites an unparseable config still backs it up first', () => { + const file = plantUnparseableAntigravityConfig('{ half a config'); + + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + getTarget('antigravity')!.install('global', { autoAllow: false }); + expect(warn).toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + + expect(fs.readFileSync(file + '.backup', 'utf-8')).toBe('{ half a config'); + expect(JSON.parse(fs.readFileSync(file, 'utf-8')).mcpServers.codegraph).toBeDefined(); + }); +}); + describe('Installer — Cursor rules file cleanup on uninstall', () => { let tmpHome: string; let tmpCwd: string; diff --git a/src/installer/targets/shared.ts b/src/installer/targets/shared.ts index 364f40427..52f2c25bd 100644 --- a/src/installer/targets/shared.ts +++ b/src/installer/targets/shared.ts @@ -51,9 +51,16 @@ export function getCodeGraphPermissions(): string[] { /** * Read a JSON file, returning `{}` when missing or unparseable. * - * Unparseable files are backed up to `.backup` BEFORE we return - * `{}` — so an idempotent re-run never silently deletes a user's - * existing config that happened to break JSON parse temporarily. + * Read-only, and deliberately silent: every target's `detect()` reads + * its agent's config on every `codegraph install`, and most of those + * agents codegraph was never installed into. A warning about a file we + * are not going to touch is noise, and a `.backup` beside it is a + * bug — `install --refresh` promises unconfigured agents are "skipped + * untouched", and an empty `~/.gemini/config/mcp_config.json` was + * getting backed up by the detection pass alone (issue #1870). + * + * Preserving an unparseable config is `writeJsonFile`'s job — it runs + * only where one is actually about to be overwritten. */ export function readJsonFile(filePath: string): Record { if (!fs.existsSync(filePath)) { @@ -61,13 +68,7 @@ export function readJsonFile(filePath: string): Record { } try { return JSON.parse(fs.readFileSync(filePath, 'utf-8')); - } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - console.warn(` Warning: Could not parse ${path.basename(filePath)}: ${msg}`); - console.warn(` A backup will be created before overwriting.`); - try { - fs.copyFileSync(filePath, filePath + '.backup'); - } catch { /* ignore backup failure */ } + } catch { return {}; } } @@ -93,11 +94,37 @@ export function atomicWriteFileSync(filePath: string, content: string): void { } } +/** + * Copy a file we're about to overwrite to `.backup` when it isn't + * parseable JSON — `readJsonFile` handed the caller `{}` for it, so the + * write is a replacement, not an edit, and without this the user's + * config would be gone. No-op when the file is absent or parses fine. + */ +function backupUnparseableJson(filePath: string): void { + if (!fs.existsSync(filePath)) return; + try { + JSON.parse(fs.readFileSync(filePath, 'utf-8')); + return; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + console.warn(` Warning: Could not parse ${path.basename(filePath)}: ${msg}`); + console.warn(` A backup will be created before overwriting.`); + } + try { + fs.copyFileSync(filePath, filePath + '.backup'); + } catch { /* ignore backup failure */ } +} + /** * Atomic JSON write. Trailing newline matches the convention every * existing target had — preserves diff-friendly file shape. + * + * The single point where a target replaces a JSON config, so it is also + * where an unparseable one gets preserved (issue #1870) — reading a + * config no longer has that side effect. */ export function writeJsonFile(filePath: string, data: Record): void { + backupUnparseableJson(filePath); atomicWriteFileSync(filePath, JSON.stringify(data, null, 2) + '\n'); } diff --git a/src/installer/targets/types.ts b/src/installer/targets/types.ts index d93680573..746f96473 100644 --- a/src/installer/targets/types.ts +++ b/src/installer/targets/types.ts @@ -93,6 +93,13 @@ export interface AgentTarget { * pair lets the orchestrator skip cleanly with a clear message. */ supportsLocation(loc: Location): boolean; + /** + * Read-only, and quiet. Every target is detected on every run — + * including agents codegraph was never installed into — and + * `install --refresh` skips the unconfigured ones on the strength of + * this answer, so a target that writes or warns here touches a config + * it has no business touching (issue #1870). + */ detect(loc: Location): DetectionResult; install(loc: Location, opts: InstallOptions): WriteResult; /**