mirror of
https://github.com/colbymchenry/codegraph.git
synced 2026-10-17 05:07:37 +08:00
`readJsonFile` copied an unparseable file to `<path>.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.
This commit is contained in:
@@ -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).
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -51,9 +51,16 @@ export function getCodeGraphPermissions(): string[] {
|
||||
/**
|
||||
* Read a JSON file, returning `{}` when missing or unparseable.
|
||||
*
|
||||
* Unparseable files are backed up to `<path>.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 `<path>.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<string, any> {
|
||||
if (!fs.existsSync(filePath)) {
|
||||
@@ -61,13 +68,7 @@ export function readJsonFile(filePath: string): Record<string, any> {
|
||||
}
|
||||
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 `<path>.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<string, any>): void {
|
||||
backupUnparseableJson(filePath);
|
||||
atomicWriteFileSync(filePath, JSON.stringify(data, null, 2) + '\n');
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user