From 0abae119592281b22a925c58ae1df3754bb4f2dc Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Thu, 22 Oct 2026 01:06:12 +0000 Subject: [PATCH] fix(installer): a symlinked instructions or config file is written through, not replaced (#2768) Root cause: every installer write goes through one helper that writes `.tmp.` and renames it onto the path. A rename replaces a symbolic link itself, not the file it points to. So a `~/.codex/AGENTS.md` or `~/.claude.json` linked from a dotfiles repo became a detached regular copy on install: the shared file never got CodeGraph's settings, and later edits to it no longer reached the agent. This held for every target (Claude Code, Codex, opencode, Cursor, Gemini, Antigravity, Kiro, Hermes and the Copilot family) and for install, the refresh that `codegraph upgrade` runs, and uninstall. Uninstall also deleted a link whose file it had emptied, leaving CodeGraph's entry behind in the shared file. Fix: - The write helper resolves a link to the file at the end of its chain and writes and renames there, so the link stays exactly as it was, relative or absolute. - A link to a file that doesn't exist yet gets that file created, but only in a folder that already exists, as writing through it in a shell would. No folder is ever created where a link points. - A dangling chain is followed hop by hop from the real folder holding each link, so a relative link under a linked folder lands where the OS would put it. - When the linked file can't be written (a read-only file, a folder that doesn't exist), the error names the link and its target and nothing changes. - Where uninstall used to delete a user's file it had emptied (an instructions file, Codex's config.toml, Copilot CLI's mcp-config.json, the legacy project .claude.json), a link is kept and the emptied content is written through it. - The refresh sweep catches a failure per agent, reports it, still refreshes the others, and exits non-zero, so the upgrade warns. Decided: links to CodeGraph's own legacy files (Cursor's codegraph.mdc, Kiro's steering doc), which install only ever deletes, are still removed. Validation: - 28 new tests. 25 are in installer-targets.test.ts: - every target and location: install and uninstall keep each of its files that is a link (18 cases); - the shared files' content through install and uninstall for Claude Code, Codex, opencode (two agents sharing one AGENTS.md) and Cursor; - relative links under a linked folder; - a link into a missing folder; - a link to a read-only file; - uninstall emptying through links; - a failing link in a refresh; - a folder link guard. The other 3: - 1 in upgrade.test.ts, where the refresh runs the built CLI; - 2 in a new installer-links-simulated.test.ts, which simulates the link with spied lstat/realpath/readlink, so it runs on machines that can't create file symlinks. With main's sources put back, 27 fail; the folder-link guard passes on both. All pass with the fix. The real-link tests are skipped, with the reason in their names, where file symlinks can't be created (Windows without Developer Mode or admin). - This Windows machine has Developer Mode on, so the real-link tests ran for real. - The real CLI against a private home (`install --yes`, `install --refresh`, `uninstall --yes --keep-cli`), 10 linked files for five agents plus relative, dangling and read-only links: - on main, all 29 link checks found the link replaced by a copy; - with the fix, none did, every link's text was unchanged, the shared files got CodeGraph's settings and lost them again on uninstall, and the missing-folder and read-only cases exited 1 with the new message and changed nothing. Fixes #2721 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + __tests__/installer-links-simulated.test.ts | 76 +++++ __tests__/installer-targets.test.ts | 289 ++++++++++++++++++++ __tests__/upgrade.test.ts | 84 ++++++ src/bin/codegraph.ts | 10 +- src/installer/index.ts | 21 +- src/installer/targets/claude.ts | 3 +- src/installer/targets/codex.ts | 3 +- src/installer/targets/copilot-cli.ts | 5 +- src/installer/targets/shared.ts | 91 +++++- 10 files changed, 571 insertions(+), 12 deletions(-) create mode 100644 __tests__/installer-links-simulated.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 57a82ae6e..b3546c451 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - Upgrading CodeGraph while an agent session is open no longer leaves the old version's background server in charge of your project: the first session started from the new install stops it and starts a current one in its place, even while sessions opened before the upgrade are still running. That old server could no longer load the language parsers the upgrade removed, so it saved every file it re-indexed with no symbols, while new sessions could only read the index beside it without keeping it up to date. A background server from a newer install is never stopped, and sessions opened before the upgrade keep the old version until you restart them. Thanks @lipchey for the report. (#2335) - A file is no longer saved with no symbols when its language parser can't be loaded, which is what happened to every file a background server re-indexed after an upgrade removed its install: the file keeps what it had and is indexed again once the parser loads, and files an earlier version emptied this way are re-indexed by the next sync. A background server also exits on its own once its install is upgraded or removed, so the next session starts one from the current install. Thanks @lipchey for the report. (#2335) - `codegraph upgrade` no longer wipes your agent instructions file, such as a global `AGENTS.md`, `CLAUDE.md` or `GEMINI.md`, when it refreshes CodeGraph's section in it: a stray `` line above that section, like one left behind when part of an older section was deleted by hand, made the refresh replace everything in between, often the whole file. CodeGraph now rewrites only its own section, in `codegraph install` and `codegraph uninstall` too, and keeps everything around it exactly as it was, Windows line endings included. Thanks @pzaj2 for the report. (#2506) +- `codegraph install`, `upgrade` and `uninstall` now write through an instructions or config file that is a symbolic link, such as a `~/.codex/AGENTS.md` or `~/.claude.json` linked from your dotfiles, instead of replacing the link with a copy: the shared file gets CodeGraph's settings, uninstall takes them out again, and the link stays in place. A link CodeGraph can't write through, like one to a read-only file or into a folder that doesn't exist, is now reported by name and left alone, and an upgrade still refreshes your other agents. Thanks @jguerra6 for the report and @danusha2345 for the first fix. (#2721) - `codegraph status` no longer says the index is up to date while indexed files are missing their symbols: it now names files the parser couldn't read and files stored without their symbols (which `codegraph sync` repairs), `status --json` counts both, and `codegraph files --json` lists each file's recorded errors. Thanks @lipchey for the report. (#2336) - Indexing large Python projects is much faster again and needs less memory: since 1.6.2, resolving Python references re-read source files over and over, so a project the size of CPython took several times as long to index. The graph it builds is unchanged. Thanks @bompus for the report. (#2332) - In VB.NET, reading or setting a `Shared` field or property through its class or module name, like `AppSession.SessionId`, `AppSession.CurrentUser = "demo"` or `AppSession.Items(0)`, now counts as a use of that member and of the class, and so do reading an `Enum` value like `Mode.Fast` and calling a `Shared` function without parentheses. Before, only calls written with parentheses were linked, so `codegraph callers` on such a field, property or class came back empty and impact missed most of the code that depends on it. A local, parameter or field that only shares a class's name is not mistaken for the class. Thanks @serkanince for the report and @ChrisPrapas. (#2305) diff --git a/__tests__/installer-links-simulated.test.ts b/__tests__/installer-links-simulated.test.ts new file mode 100644 index 000000000..d63ccedb2 --- /dev/null +++ b/__tests__/installer-links-simulated.test.ts @@ -0,0 +1,76 @@ +/** + * The installer writes through a symbolic link instead of replacing it + * (#2721) — checked here with the link SIMULATED, so it runs on every machine. + * + * The real-link tests in `installer-targets.test.ts` need to create file + * symlinks, which Windows only allows with Developer Mode or admin rights; + * where it can't, they are skipped. These two still run: the file system + * reports one path as a link (lstat) and says where it points (realpath / + * readlink), and the write must land there, leaving the link's own path alone. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { atomicWriteFileSync } from '../src/installer/targets/shared'; + +// A plain copy of `fs`, so single functions can be spied on. +vi.mock('fs', async (importOriginal) => ({ ...await importOriginal() })); + +describe('Installer — writes land on the file a link points to (link simulated) (#2721)', () => { + let dir: string; + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-link-sim-')); + }); + afterEach(() => { + vi.restoreAllMocks(); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + /** Make the file system report `linkPath` as a symbolic link. */ + function reportAsLink(linkPath: string, opts: { realpath: () => string; readlink?: string }): void { + const realLstat = fs.lstatSync; + const realRealpath = fs.realpathSync; + const realReadlink = fs.readlinkSync; + vi.spyOn(fs, 'lstatSync').mockImplementation(((p: fs.PathLike, ...rest: unknown[]) => + p === linkPath ? ({ isSymbolicLink: () => true } as fs.Stats) : (realLstat as any)(p, ...rest)) as typeof fs.lstatSync); + vi.spyOn(fs, 'realpathSync').mockImplementation(((p: fs.PathLike, ...rest: unknown[]) => + p === linkPath ? opts.realpath() : (realRealpath as any)(p, ...rest)) as typeof fs.realpathSync); + vi.spyOn(fs, 'readlinkSync').mockImplementation(((p: fs.PathLike, ...rest: unknown[]) => + p === linkPath ? opts.readlink : (realReadlink as any)(p, ...rest)) as typeof fs.readlinkSync); + } + + it('a live link: the file it points to is replaced, and the link\'s own path is never written', () => { + const linkPath = path.join(dir, 'AGENTS.md'); + const target = path.join(dir, 'dotfiles', 'AGENTS.md'); + fs.mkdirSync(path.dirname(target)); + fs.writeFileSync(linkPath, 'stands in for the link'); + fs.writeFileSync(target, '# shared\n'); + reportAsLink(linkPath, { realpath: () => target }); + + atomicWriteFileSync(linkPath, '# shared\nwith codegraph\n'); + + expect(fs.readFileSync(target, 'utf-8')).toBe('# shared\nwith codegraph\n'); + expect(fs.readFileSync(linkPath, 'utf-8')).toBe('stands in for the link'); + expect(fs.readdirSync(path.dirname(target))).toEqual(['AGENTS.md']); + }); + + it('a dangling relative link: its file is created relative to the link\'s folder, and a missing folder is an error', () => { + const linkPath = path.join(dir, 'AGENTS.md'); + fs.writeFileSync(linkPath, 'stands in for the link'); + fs.mkdirSync(path.join(dir, 'dotfiles')); + const enoent = (): never => { throw Object.assign(new Error('ENOENT: no such file'), { code: 'ENOENT' }); }; + reportAsLink(linkPath, { realpath: enoent, readlink: path.join('dotfiles', 'AGENTS.md') }); + + atomicWriteFileSync(linkPath, 'block\n'); + expect(fs.readFileSync(path.join(dir, 'dotfiles', 'AGENTS.md'), 'utf-8')).toBe('block\n'); + expect(fs.readFileSync(linkPath, 'utf-8')).toBe('stands in for the link'); + + vi.restoreAllMocks(); + reportAsLink(linkPath, { realpath: enoent, readlink: path.join('not-cloned', 'AGENTS.md') }); + expect(() => atomicWriteFileSync(linkPath, 'block\n')).toThrow(/is a symbolic link to .*not-cloned.*its folder does not exist/); + expect(fs.existsSync(path.join(dir, 'not-cloned'))).toBe(false); + expect(fs.readFileSync(linkPath, 'utf-8')).toBe('stands in for the link'); + }); +}); diff --git a/__tests__/installer-targets.test.ts b/__tests__/installer-targets.test.ts index 760991259..c303d4eca 100644 --- a/__tests__/installer-targets.test.ts +++ b/__tests__/installer-targets.test.ts @@ -1988,6 +1988,295 @@ describe('Installer — install --refresh keeps each agent\'s instructions file } }); +// --------------------------------------------------------------------------- +// A symbolic link is written through, never replaced (#2721) +// +// Instructions and config files are often symbolic links into a dotfiles or +// team repo (`~/.codex/AGENTS.md -> ~/dotfiles/AGENTS.md`). Every installer +// write went to `.tmp.` and was renamed onto the path, and a +// rename replaces the link itself: install left a detached copy, so the +// shared file never got CodeGraph's entry and later edits to it never reached +// the agent. Uninstall deleted a link whose file it had emptied. +// --------------------------------------------------------------------------- + +/** + * Whether this machine can create a file symlink. On Windows that needs + * Developer Mode or admin rights; where it can't, the link tests are skipped, + * with the reason in their name, rather than passing without running. + */ +function canCreateFileSymlink(): boolean { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-symlink-probe-')); + try { + fs.writeFileSync(path.join(dir, 'target'), ''); + fs.symlinkSync(path.join(dir, 'target'), path.join(dir, 'link'), 'file'); + return true; + } catch { + return false; + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} +const FILE_SYMLINKS = canCreateFileSymlink(); +const NO_FILE_SYMLINKS = ' — skipped: creating a file symlink needs Developer Mode or admin rights on Windows'; + +describe.skipIf(!FILE_SYMLINKS)(`Installer — a symbolic link is written through, never replaced (#2721)${FILE_SYMLINKS ? '' : NO_FILE_SYMLINKS}`, () => { + let tmpHome: string; + let tmpCwd: string; + let origCwd: string; + let homeRestore: { restore: () => void }; + let restoreWritable: Array<() => void>; + + beforeEach(() => { + tmpHome = mkTmpDir('link-home'); + tmpCwd = mkTmpDir('link-cwd'); + origCwd = process.cwd(); + process.chdir(tmpCwd); + homeRestore = setHome(tmpHome); + restoreWritable = []; + }); + + afterEach(() => { + for (const restore of restoreWritable) restore(); + homeRestore.restore(); + process.chdir(origCwd); + fs.rmSync(tmpHome, { recursive: true, force: true }); + fs.rmSync(tmpCwd, { recursive: true, force: true }); + }); + + const isLink = (p: string): boolean => { + try { return fs.lstatSync(p).isSymbolicLink(); } catch { return false; } + }; + /** Make `linkPath` a file symlink whose text is `text`, creating the folder that holds it. */ + function link(linkPath: string, text: string): void { + fs.mkdirSync(path.dirname(linkPath), { recursive: true }); + fs.symlinkSync(text, linkPath, 'file'); + } + /** A file in the shared folder the links point into, written with `content` when given. */ + function sharedFile(name: string, content?: string): string { + const file = path.join(tmpHome, 'dotfiles', name); + fs.mkdirSync(path.dirname(file), { recursive: true }); + if (content !== undefined) fs.writeFileSync(file, content); + return file; + } + + // Every file each agent's install writes, at both locations: whatever the + // installer does, it never replaces or removes a link. + for (const target of ALL_TARGETS) { + for (const location of (['global', 'local'] as const).filter((l) => target.supportsLocation(l))) { + it(`${target.id} (${location}): install and uninstall keep every one of its files that is a link`, () => { + const links = target.describePaths(location).map((p, i) => { + const dest = sharedFile(`${i}-${path.basename(p)}`); + link(p, dest); + return { path: p, dest }; + }); + + const result = target.install(location, { autoAllow: true }); + for (const l of links) { + expect(isLink(l.path), l.path).toBe(true); + expect(fs.readlinkSync(l.path)).toBe(l.dest); + } + for (const f of result.files.filter((x) => x.action === 'created' || x.action === 'updated')) { + const written = links.find((l) => l.path === f.path); + expect(written, f.path).toBeDefined(); + expect(fs.existsSync(written!.dest), written!.dest).toBe(true); + } + expect(target.detect(location).alreadyConfigured).toBe(true); + + target.uninstall(location); + for (const l of links) { + expect(isLink(l.path), l.path).toBe(true); + expect(fs.readlinkSync(l.path)).toBe(l.dest); + } + expect(target.detect(location).alreadyConfigured).toBe(false); + }); + } + } + + it('the shared files get CodeGraph\'s entries next to the user\'s own, and uninstall takes only those out again', () => { + const files = { + claudeJson: sharedFile('claude.json', '{\n "numStartups": 3\n}\n'), + claudeMd: sharedFile('CLAUDE.md', '# my claude notes\n'), + agentsMd: sharedFile('AGENTS.md', '# shared rules\n\nBe terse.\n'), + codexToml: sharedFile('codex.toml', 'model = "o3"\n'), + opencode: sharedFile('opencode.jsonc', '{\n // my theme\n "theme": "tokyonight"\n}\n'), + cursor: sharedFile('cursor-mcp.json', '{\n "mcpServers": {\n "other": {\n "command": "x"\n }\n }\n}\n'), + }; + const original = Object.fromEntries(Object.entries(files).map(([k, f]) => [k, fs.readFileSync(f, 'utf-8')])); + link(path.join(tmpHome, '.claude.json'), files.claudeJson); + link(path.join(tmpHome, '.claude', 'CLAUDE.md'), files.claudeMd); + // Two agents sharing one instructions file. + link(path.join(tmpHome, '.codex', 'AGENTS.md'), files.agentsMd); + link(path.join(tmpHome, '.config', 'opencode', 'AGENTS.md'), files.agentsMd); + link(path.join(tmpHome, '.codex', 'config.toml'), files.codexToml); + link(path.join(tmpHome, '.config', 'opencode', 'opencode.jsonc'), files.opencode); + link(path.join(tmpHome, '.cursor', 'mcp.json'), files.cursor); + const ids = ['claude', 'codex', 'opencode', 'cursor'] as const; + + for (const id of ids) getTarget(id)!.install('global', { autoAllow: false }); + + const read = (f: string) => fs.readFileSync(f, 'utf-8'); + expect(JSON.parse(read(files.claudeJson))).toMatchObject({ numStartups: 3, mcpServers: { codegraph: {} } }); + expect(read(files.claudeMd)).toBe(`# my claude notes\n\n${CODEGRAPH_INSTRUCTIONS_BLOCK}\n`); + expect(read(files.agentsMd)).toBe(`# shared rules\n\nBe terse.\n\n${CODEGRAPH_INSTRUCTIONS_BLOCK}\n`); + expect(read(files.codexToml)).toContain('model = "o3"'); + expect(read(files.codexToml)).toContain('[mcp_servers.codegraph]'); + expect(read(files.opencode)).toContain('// my theme'); + expect(parseJsonc(read(files.opencode)).mcp.servers.codegraph).toBeDefined(); + expect(JSON.parse(read(files.cursor)).mcpServers).toMatchObject({ other: { command: 'x' }, codegraph: {} }); + // Re-running is a no-op through the links too. + for (const id of ids) { + for (const f of getTarget(id)!.install('global', { autoAllow: false }).files) expect(f.action, f.path).toBe('unchanged'); + } + + for (const id of ids) getTarget(id)!.uninstall('global'); + + expect(JSON.parse(read(files.claudeJson))).toEqual({ numStartups: 3 }); + expect(read(files.claudeMd)).toBe(original.claudeMd); + expect(read(files.agentsMd)).toBe(original.agentsMd); + expect(read(files.codexToml)).toBe(original.codexToml); + expect(parseJsonc(read(files.opencode))).toEqual({ $schema: 'https://opencode.ai/config.json', theme: 'tokyonight' }); + expect(read(files.opencode)).toContain('// my theme'); + expect(JSON.parse(read(files.cursor))).toEqual({ mcpServers: { other: { command: 'x' } } }); + }); + + it('a relative link keeps its text, and is followed from the folder that really holds it', () => { + // `~/.codex` is itself a folder link into the dotfiles repo, and the + // config inside it points one level up, relative to the real folder. + const realCodex = path.join(tmpHome, 'dotfiles', 'codex'); + fs.mkdirSync(realCodex, { recursive: true }); + fs.symlinkSync(realCodex, path.join(tmpHome, '.codex'), 'junction'); + const agentsText = path.join('..', 'AGENTS.md'); + const tomlText = path.join('..', 'codex.toml'); + link(path.join(realCodex, 'AGENTS.md'), agentsText); + link(path.join(realCodex, 'config.toml'), tomlText); + fs.writeFileSync(path.join(tmpHome, 'dotfiles', 'codex.toml'), 'model = "o3"\n'); + // Only the AGENTS.md target is missing, so it is created where the link + // really points: dotfiles/AGENTS.md, not a lexical ~/AGENTS.md. + + getTarget('codex')!.install('global', { autoAllow: false }); + + expect(fs.readlinkSync(path.join(realCodex, 'AGENTS.md'))).toBe(agentsText); + expect(fs.readlinkSync(path.join(realCodex, 'config.toml'))).toBe(tomlText); + expect(isLink(path.join(tmpHome, '.codex'))).toBe(true); + expect(fs.readFileSync(path.join(tmpHome, 'dotfiles', 'AGENTS.md'), 'utf-8')).toBe(`${CODEGRAPH_INSTRUCTIONS_BLOCK}\n`); + expect(fs.existsSync(path.join(tmpHome, 'AGENTS.md'))).toBe(false); + expect(fs.readFileSync(path.join(tmpHome, 'dotfiles', 'codex.toml'), 'utf-8')).toContain('[mcp_servers.codegraph]'); + }); + + it('a link into a folder that does not exist is an error naming the link, and nothing is created', () => { + const agentsMd = path.join(tmpHome, '.codex', 'AGENTS.md'); + const missing = path.join(tmpHome, 'dotfiles-not-cloned-yet', 'AGENTS.md'); + link(agentsMd, missing); + + expect(() => getTarget('codex')!.install('global', { autoAllow: false })).toThrow( + /AGENTS\.md is a symbolic link to .*dotfiles-not-cloned-yet.*its folder does not exist.*never replaces one with a copy/, + ); + expect(isLink(agentsMd)).toBe(true); + expect(fs.readlinkSync(agentsMd)).toBe(missing); + expect(fs.existsSync(path.dirname(missing))).toBe(false); + }); + + // POSIX refuses the write in a read-only folder, except to root; Windows + // refuses replacing a file whose read-only attribute is set. + const POSIX_ROOT = process.platform !== 'win32' && process.getuid?.() === 0; + it.skipIf(POSIX_ROOT)(`a link to a read-only file is an error naming the link; the file is unchanged and no temp file is left${POSIX_ROOT ? ' — skipped: root can write into a read-only folder' : ''}`, () => { + const shared = sharedFile(path.join('managed', 'AGENTS.md'), '# managed by home-manager\n'); + const lock = process.platform === 'win32' ? shared : path.dirname(shared); + fs.chmodSync(lock, process.platform === 'win32' ? 0o444 : 0o555); + restoreWritable.push(() => fs.chmodSync(lock, process.platform === 'win32' ? 0o666 : 0o755)); + const agentsMd = path.join(tmpHome, '.codex', 'AGENTS.md'); + link(agentsMd, shared); + + expect(() => getTarget('codex')!.install('global', { autoAllow: false })).toThrow(/AGENTS\.md is a symbolic link to .*it is read-only/); + expect(isLink(agentsMd)).toBe(true); + expect(fs.readFileSync(shared, 'utf-8')).toBe('# managed by home-manager\n'); + expect(fs.readdirSync(path.dirname(shared)).filter((n) => n.includes('.tmp.'))).toEqual([]); + }); + + it('uninstall empties a linked file that held only CodeGraph\'s part, instead of deleting the link', () => { + const agentsMd = sharedFile('AGENTS.md'); + const codexToml = sharedFile('codex.toml'); + const copilot = sharedFile('copilot.json'); + link(path.join(tmpHome, '.codex', 'AGENTS.md'), agentsMd); + link(path.join(tmpHome, '.codex', 'config.toml'), codexToml); + link(path.join(tmpHome, '.copilot', 'mcp-config.json'), copilot); + getTarget('codex')!.install('global', { autoAllow: false }); + getTarget('copilot-cli')!.install('global', { autoAllow: false }); + expect(fs.readFileSync(agentsMd, 'utf-8')).toBe(`${CODEGRAPH_INSTRUCTIONS_BLOCK}\n`); + + getTarget('codex')!.uninstall('global'); + getTarget('copilot-cli')!.uninstall('global'); + + for (const p of ['.codex/AGENTS.md', '.codex/config.toml', '.copilot/mcp-config.json']) { + expect(isLink(path.join(tmpHome, p)), p).toBe(true); + } + expect(fs.readFileSync(agentsMd, 'utf-8')).toBe(''); + expect(fs.readFileSync(codexToml, 'utf-8')).toBe(''); + expect(JSON.parse(fs.readFileSync(copilot, 'utf-8'))).toEqual({}); + }); + + it('a link that can\'t be written fails only its own agent in a refresh: the others are still refreshed', () => { + getTarget('codex')!.install('global', { autoAllow: false }); + getTarget('gemini')!.install('global', { autoAllow: false }); + // Codex's instructions file is now a link into a folder that is gone, + // and Gemini's block is stale. + const agentsMd = path.join(tmpHome, '.codex', 'AGENTS.md'); + fs.unlinkSync(agentsMd); + link(agentsMd, path.join(tmpHome, 'gone', 'AGENTS.md')); + const geminiMd = path.join(tmpHome, '.gemini', 'GEMINI.md'); + fs.writeFileSync(geminiMd, `${LEGACY_BLOCK}\n`); + + const reports = refreshTargets(ALL_TARGETS, 'global'); + + const codex = reports.find((r) => r.id === 'codex')!; + expect(codex.status).toBe('failed'); + expect(codex.error).toMatch(/AGENTS\.md is a symbolic link to .*gone.*its folder does not exist/); + const gemini = reports.find((r) => r.id === 'gemini')!; + expect(gemini.status).toBe('refreshed'); + expect(fs.readFileSync(geminiMd, 'utf-8')).toBe(`${CODEGRAPH_INSTRUCTIONS_BLOCK}\n`); + expect(isLink(agentsMd)).toBe(true); + }); +}); + +describe('Installer — a config folder that is a link keeps working (#2721)', () => { + let tmpHome: string; + let tmpCwd: string; + let origCwd: string; + let homeRestore: { restore: () => void }; + + beforeEach(() => { + tmpHome = mkTmpDir('dirlink-home'); + tmpCwd = mkTmpDir('dirlink-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 }); + }); + + // A folder link needs no special rights: a junction on Windows, a symlink elsewhere. + it('~/.codex as a folder link: files are written inside the real folder and the link stays', () => { + const real = path.join(tmpHome, 'dotfiles', 'codex'); + fs.mkdirSync(real, { recursive: true }); + const dirLink = path.join(tmpHome, '.codex'); + fs.symlinkSync(real, dirLink, 'junction'); + + getTarget('codex')!.install('global', { autoAllow: false }); + expect(fs.lstatSync(dirLink).isSymbolicLink()).toBe(true); + expect(fs.readFileSync(path.join(real, 'config.toml'), 'utf-8')).toContain('[mcp_servers.codegraph]'); + expect(fs.readFileSync(path.join(real, 'AGENTS.md'), 'utf-8')).toBe(`${CODEGRAPH_INSTRUCTIONS_BLOCK}\n`); + + getTarget('codex')!.uninstall('global'); + expect(fs.lstatSync(dirLink).isSymbolicLink()).toBe(true); + expect(fs.existsSync(path.join(real, 'AGENTS.md'))).toBe(false); + }); +}); + describe('Installer — detection never writes (#1870)', () => { let tmpHome: string; let tmpCwd: string; diff --git a/__tests__/upgrade.test.ts b/__tests__/upgrade.test.ts index 53f6a8958..403271b7f 100644 --- a/__tests__/upgrade.test.ts +++ b/__tests__/upgrade.test.ts @@ -895,6 +895,90 @@ describe('post-upgrade refresh keeps the user\'s instructions files (#2506)', () }, 90_000); }); +// --------------------------------------------------------------------------- +// A symbolic link the refresh can't write through (#2721) +// --------------------------------------------------------------------------- + +/** File symlinks need Developer Mode or admin rights on Windows. */ +function canCreateFileSymlink(): boolean { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-symlink-probe-')); + try { + fs.writeFileSync(path.join(dir, 'target'), ''); + fs.symlinkSync(path.join(dir, 'target'), path.join(dir, 'link'), 'file'); + return true; + } catch { + return false; + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} +const FILE_SYMLINKS = canCreateFileSymlink(); + +describe.skipIf(!FILE_SYMLINKS)(`post-upgrade refresh with a link it can't write through (#2721)${FILE_SYMLINKS ? '' : ' — skipped: creating a file symlink needs Developer Mode or admin rights on Windows'}`, () => { + const BIN = path.resolve(__dirname, '../dist/bin/codegraph.js'); + let home: string; + + beforeEach(() => { + home = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-upgrade-2721-')); + }); + + afterEach(() => { + fs.rmSync(home, { recursive: true, force: true }); + }); + + it('warns, keeps the link, and still refreshes every other agent', async () => { + // Codex and Gemini configured globally. Codex's AGENTS.md is a link into a + // dotfiles folder that isn't there; Gemini's block is out of date. + fs.mkdirSync(path.join(home, '.codex'), { recursive: true }); + fs.writeFileSync(path.join(home, '.codex', 'config.toml'), '[mcp_servers.codegraph]\ncommand = "codegraph"\nargs = ["serve", "--mcp"]\n'); + const agentsMd = path.join(home, '.codex', 'AGENTS.md'); + const missing = path.join(home, 'dotfiles', 'AGENTS.md'); + fs.symlinkSync(missing, agentsMd, 'file'); + fs.mkdirSync(path.join(home, '.gemini'), { recursive: true }); + fs.writeFileSync(path.join(home, '.gemini', 'settings.json'), JSON.stringify({ mcpServers: { codegraph: { type: 'stdio', command: 'codegraph', args: ['serve', '--mcp'] } } }, null, 2) + '\n'); + const geminiMd = path.join(home, '.gemini', 'GEMINI.md'); + fs.writeFileSync(geminiMd, '\n## CodeGraph\n\nPrefer `codegraph_search` over grep.\n\n'); + + const work = path.join(home, 'work'); + fs.mkdirSync(work); + const env: NodeJS.ProcessEnv = { + ...process.env, + HOME: home, + USERPROFILE: home, + XDG_CONFIG_HOME: path.join(home, '.config'), + APPDATA: path.join(home, 'AppData', 'Roaming'), + LOCALAPPDATA: path.join(home, 'AppData', 'Local'), + CODEGRAPH_NO_DAEMON: '1', + CODEGRAPH_TELEMETRY: '0', + CODEGRAPH_NO_UPDATE_CHECK: '1', + DO_NOT_TRACK: '1', + NO_COLOR: '1', + }; + for (const key of ['CODEX_HOME', 'COPILOT_HOME', 'HERMES_HOME', 'CLAUDE_CONFIG_DIR', 'CODEGRAPH_NO_INSTALL_REFRESH']) delete env[key]; + + const { deps, calls } = makeDeps({ + method: { kind: 'npm', scope: 'global' }, + currentVersion: '0.9.8', + hasCommand: (cmd) => cmd === 'codegraph', + }); + let refreshOutput = ''; + deps.run = (cmd, args, runEnv) => { + calls.runs.push({ cmd, args, env: runEnv }); + if (cmd !== 'codegraph') return 0; + const r = spawnSync(process.execPath, [BIN, ...args], { cwd: work, env, encoding: 'utf-8', timeout: 60_000 }); + refreshOutput = `${r.stdout}\n${r.stderr}`; + return r.status ?? -1; + }; + + expect(await runUpgrade({}, deps)).toBe(0); + expect(calls.logs.join('\n')).toMatch(/Could not refresh the installed agent surfaces/); + expect(refreshOutput).toMatch(/Codex CLI: could not refresh — .*AGENTS\.md is a symbolic link to .*its folder does not exist/); + expect(fs.lstatSync(agentsMd).isSymbolicLink()).toBe(true); + expect(fs.existsSync(path.dirname(missing))).toBe(false); + expect(fs.readFileSync(geminiMd, 'utf-8')).toBe(CODEGRAPH_INSTRUCTIONS_BLOCK + '\n'); + }, 90_000); +}); + // --------------------------------------------------------------------------- // Post-upgrade version probe — does the PATH-resolved `codegraph` serve the // version we just installed, in THIS terminal? diff --git a/src/bin/codegraph.ts b/src/bin/codegraph.ts index dd0287ddd..296d950d1 100644 --- a/src/bin/codegraph.ts +++ b/src/bin/codegraph.ts @@ -2737,17 +2737,25 @@ program ? [opts.location as 'global' | 'local'] : ['global', 'local']; let changed = 0; + let failed = 0; for (const loc of locs) { for (const report of refreshTargets(ALL_TARGETS, loc)) { + if (report.status === 'failed') { + failed += 1; + error(`${report.displayName}: could not refresh — ${report.error}`); + } for (const p of report.changedPaths) { changed += 1; console.log(` ${report.displayName}: refreshed ${p}`); } } } - if (changed === 0) { + if (changed === 0 && failed === 0) { console.log('All configured agent surfaces are already current.'); } + // Non-zero so `codegraph upgrade` tells the user to look; every other + // agent was still refreshed. + if (failed > 0) process.exitCode = 1; return; } diff --git a/src/installer/index.ts b/src/installer/index.ts index 8930e48a7..579d59593 100644 --- a/src/installer/index.ts +++ b/src/installer/index.ts @@ -22,7 +22,7 @@ import { getTarget, resolveTargetFlag, } from './targets/registry'; -import type { AgentTarget, Location, TargetId } from './targets/types'; +import type { AgentTarget, Location, TargetId, WriteResult } from './targets/types'; // Import the lightweight submodules directly (not the ../sync barrel, which // re-exports FileWatcher and would transitively pull in ../extraction — the // installer must stay importable even when native modules can't load). @@ -380,13 +380,13 @@ export function uninstallTargets( }); } -export type RefreshStatus = 'refreshed' | 'unchanged' | 'not-configured' | 'unsupported'; +export type RefreshStatus = 'refreshed' | 'unchanged' | 'not-configured' | 'unsupported' | 'failed'; /** * Per-target outcome of a refresh sweep. `refreshed` means at least one * filesystem entry was created, updated, or removed; `unchanged` means the target was - * already current (every write reported byte-identical); the other two - * mirror `UninstallStatus`. + * already current (every write reported byte-identical); `failed` means a + * write threw (`error` says why); the other two mirror `UninstallStatus`. */ export interface RefreshReport { id: TargetId; @@ -395,6 +395,8 @@ export interface RefreshReport { status: RefreshStatus; /** Absolute paths created, updated, or removed by the refresh. */ changedPaths: string[]; + /** Why the refresh of this target failed (status `failed` only). */ + error?: string; } /** @@ -428,7 +430,16 @@ export function refreshTargets( if (!target.detect(location).alreadyConfigured) { return { ...base, status: 'not-configured' as const, changedPaths: [] }; } - const result = target.install(location, { autoAllow: false, promptHook: undefined }); + let result: WriteResult; + try { + result = target.install(location, { autoAllow: false, promptHook: undefined }); + } catch (err) { + // One agent's file that can't be written (a symbolic link to a + // read-only file, say: #2721) must not stop the rest of the sweep + // `codegraph upgrade` runs. + const error = err instanceof Error ? err.message : String(err); + return { ...base, status: 'failed' as const, changedPaths: [], error }; + } const changedPaths = result.files .filter((f) => f.action === 'created' || f.action === 'updated' || f.action === 'removed') .map((f) => f.path); diff --git a/src/installer/targets/claude.ts b/src/installer/targets/claude.ts index 3c789849d..1f2c43489 100644 --- a/src/installer/targets/claude.ts +++ b/src/installer/targets/claude.ts @@ -35,6 +35,7 @@ import { getMcpServerConfig, jsonDeepEqual, readJsonFile, + removeFileUnlessLinked, removeMarkedSection, writeJsonFile, upsertInstructionsEntry, @@ -295,7 +296,7 @@ function cleanupLegacyLocalMcp(): WriteResult['files'][number] | null { delete config.mcpServers.codegraph; if (Object.keys(config.mcpServers).length === 0) delete config.mcpServers; if (Object.keys(config).length === 0) { - try { fs.unlinkSync(file); } catch { /* ignore */ } + removeFileUnlessLinked(file, '{}\n'); } else { writeJsonFile(file, config); } diff --git a/src/installer/targets/codex.ts b/src/installer/targets/codex.ts index 797acff1e..d6506282c 100644 --- a/src/installer/targets/codex.ts +++ b/src/installer/targets/codex.ts @@ -42,6 +42,7 @@ import { import { atomicWriteFileSync, getMcpServerConfig, + removeFileUnlessLinked, removeMarkedSection, upsertInstructionsEntry, } from './shared'; @@ -134,7 +135,7 @@ class CodexTarget implements AgentTarget { const { content: nextContent, action } = removeTomlTable(content, TOML_HEADER); if (action === 'removed') { if (nextContent.trim() === '') { - try { fs.unlinkSync(tomlPath); } catch { /* ignore */ } + removeFileUnlessLinked(tomlPath, ''); } else { atomicWriteFileSync(tomlPath, nextContent.trimEnd() + '\n'); } diff --git a/src/installer/targets/copilot-cli.ts b/src/installer/targets/copilot-cli.ts index c355ca29b..bbffd0108 100644 --- a/src/installer/targets/copilot-cli.ts +++ b/src/installer/targets/copilot-cli.ts @@ -42,6 +42,7 @@ import { getMcpServerConfig, jsonDeepEqual, readJsonFile, + removeFileUnlessLinked, writeJsonFile, } from './shared'; @@ -158,7 +159,9 @@ class CopilotCliTarget implements AgentTarget { // 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); + // A symbolic link is the user's, so it is kept with `{}` written + // through it (#2721). + removeFileUnlessLinked(file, '{}\n'); } else { writeJsonFile(file, config); } diff --git a/src/installer/targets/shared.ts b/src/installer/targets/shared.ts index dca78989c..050ba068a 100644 --- a/src/installer/targets/shared.ts +++ b/src/installer/targets/shared.ts @@ -78,10 +78,35 @@ export function readJsonFile(filePath: string): Record { * * Prevents corruption if the process crashes mid-write. The temp * file is cleaned up on rename failure. + * + * A symbolic link is written through, never replaced (#2721). Renaming + * onto the link's own path swapped the link for a regular copy, so a + * `~/.codex/AGENTS.md -> ~/dotfiles/AGENTS.md` kept in a dotfiles repo + * stopped reaching the shared file. The write lands on the file at the end + * of the link instead, and the link itself, relative or absolute, is left + * as it was. A link to a file that doesn't exist yet gets that file + * created, but only in a folder that already exists, as writing through it + * in a shell would: no folder is ever created where a link points. When the + * linked file can't be written, the error names the link and nothing + * changes. */ export function atomicWriteFileSync(filePath: string, content: string): void { + if (!isSymbolicLink(filePath)) { + writeThenRename(filePath, content, true); + return; + } + let target = filePath; + try { + target = linkTarget(filePath); + writeThenRename(target, content, false); + } catch (err) { + throw linkWriteError(filePath, target, err); + } +} + +function writeThenRename(filePath: string, content: string, createFolder: boolean): void { const dir = path.dirname(filePath); - if (!fs.existsSync(dir)) { + if (createFolder && !fs.existsSync(dir)) { fs.mkdirSync(dir, { recursive: true }); } const tmpPath = filePath + '.tmp.' + process.pid; @@ -94,6 +119,65 @@ export function atomicWriteFileSync(filePath: string, content: string): void { } } +/** True when `p` itself is a symbolic link (on Windows, also a junction). */ +function isSymbolicLink(p: string): boolean { + try { + return fs.lstatSync(p).isSymbolicLink(); + } catch { + return false; + } +} + +/** + * The file a symbolic link ends at, through any chain of links. It may not + * exist yet: each hop of a dangling chain is resolved from the real folder + * that holds the link, as the OS resolves it, not by joining names. + */ +function linkTarget(link: string): string { + try { + return fs.realpathSync(link); + } catch { + let current = link; + for (let hop = 0; hop < 40 && isSymbolicLink(current); hop++) { + current = path.resolve(fs.realpathSync(path.dirname(current)), fs.readlinkSync(current)); + } + if (isSymbolicLink(current)) { + throw Object.assign(new Error('too many levels of symbolic links'), { code: 'ELOOP' }); + } + return current; + } +} + +function linkWriteError(link: string, target: string, cause: unknown): Error { + const code = (cause as NodeJS.ErrnoException | undefined)?.code; + const why = code === 'ENOENT' ? 'its folder does not exist' + : code === 'EACCES' || code === 'EROFS' ? 'it is read-only' + : code === 'EPERM' || code === 'EBUSY' ? 'it is read-only or in use' + : cause instanceof Error ? cause.message : String(cause); + const message = target === link + ? `${link} is a symbolic link that can't be followed (${why}).` + : `${link} is a symbolic link to ${target}, which can't be written (${why}).`; + return Object.assign( + new Error(`${message} CodeGraph writes through links and never replaces one with a copy: fix the link or make its file writable, then run this again.`, { cause }), + { code }, + ); +} + +/** + * Delete a file that codegraph's change left with nothing of the user's in + * it. A symbolic link is kept instead and `emptied` is written through it: + * the link is the user's, usually into a shared dotfiles folder, and + * deleting it would leave codegraph's entry behind in the shared file + * (#2721). + */ +export function removeFileUnlessLinked(filePath: string, emptied: string): void { + if (isSymbolicLink(filePath)) { + atomicWriteFileSync(filePath, emptied); + return; + } + try { fs.unlinkSync(filePath); } catch { /* ignore */ } +} + /** * Copy a file we're about to overwrite to `.backup` when it isn't * parseable JSON — `readJsonFile` handed the caller `{}` for it, so the @@ -344,7 +428,8 @@ export function stripMarkedSections(content: string, startMarker: string, endMar * Inverse of `replaceOrAppendMarkedSection`. Strips the marker * block from `filePath` if present. If the file becomes empty after * removal, deletes the file entirely (matches the existing Claude - * uninstall behavior). + * uninstall behavior) — unless it is a symbolic link, which is kept + * and emptied through. * * Returns `removed` when content was stripped, `not-found` when * the markers weren't present, `kept` when the file didn't exist. @@ -367,7 +452,7 @@ export function removeMarkedSection( if (rest === null) return 'not-found'; if (isBlank(rest)) { - try { fs.unlinkSync(filePath); } catch { /* ignore */ } + removeFileUnlessLinked(filePath, rest); } else { atomicWriteFileSync(filePath, rest); }