diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 32baad1cd..a8bcf53f5 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -165,12 +165,14 @@ describe('registerPtyHandlers', () => { getPathMock.mockReturnValue('/tmp/orca-user-data') existsSyncMock.mockReturnValue(true) statSyncMock.mockReturnValue({ isDirectory: () => true, mode: 0o755 }) - openCodeBuildPtyEnvMock.mockReturnValue({ + openCodeBuildPtyEnvMock.mockImplementation((_ptyId: string, existingConfigDir?: string) => ({ ORCA_OPENCODE_HOOK_PORT: '4567', ORCA_OPENCODE_HOOK_TOKEN: 'opencode-token', ORCA_OPENCODE_PTY_ID: 'test-pty', - OPENCODE_CONFIG_DIR: '/tmp/orca-opencode-config' - }) + OPENCODE_CONFIG_DIR: existingConfigDir + ? '/tmp/orca-opencode-overlay' + : '/tmp/orca-opencode-config' + })) buildAgentHookEnvMock.mockReturnValue({ ORCA_AGENT_HOOK_PORT: '5678', ORCA_AGENT_HOOK_TOKEN: 'agent-token' @@ -513,9 +515,15 @@ describe('registerPtyHandlers', () => { expect(env.ORCA_OPENCODE_HOOK_PORT).toBe('4567') }) - it('preserves a user-provided OPENCODE_CONFIG_DIR on the daemon path', async () => { + it('mirrors a user-provided OPENCODE_CONFIG_DIR into a per-PTY overlay on the daemon path', async () => { const env = await daemonSpawnAndGetEnv({ OPENCODE_CONFIG_DIR: '/user/custom/opencode' }) - expect(env.OPENCODE_CONFIG_DIR).toBe('/user/custom/opencode') + // Why: OpenCode loads config from a single dir, so the user's path is + // mirrored into a per-PTY overlay rather than passed through literally. + expect(openCodeBuildPtyEnvMock).toHaveBeenCalledWith( + expect.any(String), + '/user/custom/opencode' + ) + expect(env.OPENCODE_CONFIG_DIR).toBe('/tmp/orca-opencode-overlay') }) it('injects Pi overlay env (PI_CODING_AGENT_DIR) on the daemon path', async () => { diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 3b4a1748c..3f55bb6ab 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -184,16 +184,13 @@ export function buildPtyHostEnv( baseEnv.OPENCODE_CONFIG_DIR ?? process.env.OPENCODE_CONFIG_DIR const preexistingPiAgentDir = baseEnv.PI_CODING_AGENT_DIR ?? process.env.PI_CODING_AGENT_DIR - const openCodeHookEnv = openCodeHookService.buildPtyEnv(id) - if (preexistingOpenCodeConfigDir) { - // Why: OPENCODE_CONFIG_DIR is a singular extra config root. Replacing a - // user-provided directory would silently hide their custom OpenCode - // config, so preserve it. The Orca status plugin will not load, so the - // dashboard falls back to a blank status for that pane until the user - // unsets their override. - delete openCodeHookEnv.OPENCODE_CONFIG_DIR - } - Object.assign(baseEnv, openCodeHookEnv) + // Why: OPENCODE_CONFIG_DIR is a singular path, not a colon-list, so a user + // value cannot coexist with an Orca-only injection. Hand the user's value + // (when present) to the hook service and let it materialize a per-PTY + // mirror overlay that lets the user's plugins and Orca's status plugin + // load together — same pattern Pi uses below for PI_CODING_AGENT_DIR. See + // docs/opencode-config-dir-collision.md. + Object.assign(baseEnv, openCodeHookService.buildPtyEnv(id, preexistingOpenCodeConfigDir)) // Why: Claude/Codex native hooks run inside the shell process, so Orca // must inject the loopback receiver coordinates before the agent starts. @@ -1166,12 +1163,9 @@ export function registerPtyHandlers( // driver gate; pty:reportGeometry never resizes the PTY, only refreshes // the restore-target cache. See docs/mobile-fit-hold.md. ipcMain.removeAllListeners('pty:reportGeometry') - ipcMain.on( - 'pty:reportGeometry', - (_event, args: { id: string; cols: number; rows: number }) => { - runtime?.recordRendererGeometry(args.id, args.cols, args.rows) - } - ) + ipcMain.on('pty:reportGeometry', (_event, args: { id: string; cols: number; rows: number }) => { + runtime?.recordRendererGeometry(args.id, args.cols, args.rows) + }) // Why: fire-and-forget — clears the DaemonPtyAdapter's sticky cold restore // cache after the renderer has consumed the data. No-op for non-daemon providers. diff --git a/src/main/opencode/hook-service.test.ts b/src/main/opencode/hook-service.test.ts index 21fb9229a..80e311a06 100644 --- a/src/main/opencode/hook-service.test.ts +++ b/src/main/opencode/hook-service.test.ts @@ -1,5 +1,20 @@ -import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest' -import { existsSync, mkdtempSync, rmSync, readFileSync } from 'fs' +/* eslint-disable max-lines -- Why: this suite covers four orthogonal regimes + (plugin source, id guards, legacy per-PTY round-trip, and overlay mode for + user-set OPENCODE_CONFIG_DIR). Splitting them across files would scatter + tightly coupled fixtures (userData mock, hooks/overlay roots) and obscure + the docs/opencode-config-dir-collision.md regression matrix. */ +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' +import { + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + rmSync, + symlinkSync, + writeFileSync +} from 'fs' import { tmpdir } from 'os' import { join } from 'path' @@ -157,7 +172,7 @@ describe('OpenCodeHookService buildPtyEnv / clearPty round-trip', () => { // Why: the primitives above only prove the helpers work in isolation. This // suite exercises the public surface against a real filesystem so a future // regression — e.g. re-tightening the id guard or desyncing the path used by - // writePluginConfig vs clearPty — fails loudly. Before #1148 the service + // writeLegacyPluginConfig vs clearPty — fails loudly. Before #1148 the service // silently returned {} for daemon-shaped ids; these tests lock that in. const daemonSessionId = '50c010a2-bc8e-4eb1-8847-5812133ad6df::/Users/thebr/ghostx/workspaces/noqa/autoheal@@a1b2c3d4' @@ -179,8 +194,8 @@ describe('OpenCodeHookService buildPtyEnv / clearPty round-trip', () => { }) afterEach(() => { - const hooksRoot = join(userDataDir, 'opencode-hooks') - rmSync(hooksRoot, { recursive: true, force: true }) + rmSync(join(userDataDir, 'opencode-hooks'), { recursive: true, force: true }) + rmSync(join(userDataDir, 'opencode-config-overlays'), { recursive: true, force: true }) }) it('writes OPENCODE_CONFIG_DIR for a daemon-shaped sessionId and installs the plugin file', () => { @@ -211,9 +226,24 @@ describe('OpenCodeHookService buildPtyEnv / clearPty round-trip', () => { it('buildPtyEnv returns {} for an unusable id and creates nothing on disk', () => { const service = new OpenCodeHookService() const hooksRoot = join(userDataDir, 'opencode-hooks') + const overlaysRoot = join(userDataDir, 'opencode-config-overlays') expect(service.buildPtyEnv('')).toEqual({}) expect(existsSync(hooksRoot)).toBe(false) + expect(existsSync(overlaysRoot)).toBe(false) + }) + + it('buildPtyEnv preserves a user-set OPENCODE_CONFIG_DIR when the id is unusable', () => { + // Why: defense-in-depth — if the bounds guard rejects the id, we still + // must not blow away the user's own OPENCODE_CONFIG_DIR. The status + // plugin is forfeited, but the user's plugins/auth/keymap keep loading. + const service = new OpenCodeHookService() + const userDir = mkdtempSync(join(tmpdir(), 'orca-opencode-userdir-')) + try { + expect(service.buildPtyEnv('', userDir)).toEqual({ OPENCODE_CONFIG_DIR: userDir }) + } finally { + rmSync(userDir, { recursive: true, force: true }) + } }) it('works end-to-end for a plain UUID id (non-daemon path)', () => { @@ -231,3 +261,249 @@ describe('OpenCodeHookService buildPtyEnv / clearPty round-trip', () => { expect(existsSync(env.OPENCODE_CONFIG_DIR!)).toBe(false) }) }) + +describe('OpenCodeHookService overlay mode (user OPENCODE_CONFIG_DIR set)', () => { + // Why: locks in docs/opencode-config-dir-collision.md — when the user has + // their own OPENCODE_CONFIG_DIR (e.g. a company-wide opencode config repo), + // Orca must mirror it into a per-PTY overlay rather than `delete` its own + // injection (the prior bug) or overwrite the user's value (Superset's + // failure mode). The user's auth/models/keymap and Orca's status plugin + // both load via a single OPENCODE_CONFIG_DIR. + const ptyId = 'overlay-pty-1' + let userDataDir: string + let userConfigDir: string + + beforeAll(() => { + userDataDir = mkdtempSync(join(tmpdir(), 'orca-opencode-overlay-userdata-')) + getPathMock.mockImplementation((name: string) => { + if (name === 'userData') { + return userDataDir + } + throw new Error(`unexpected getPath(${name})`) + }) + }) + + afterAll(() => { + rmSync(userDataDir, { recursive: true, force: true }) + }) + + beforeEach(() => { + userConfigDir = mkdtempSync(join(tmpdir(), 'orca-opencode-overlay-userconfig-')) + // Realistic user config: top-level files plus a plugins/ dir with a user plugin. + writeFileSync(join(userConfigDir, 'opencode.json'), '{"userTheme":"solarized"}') + writeFileSync(join(userConfigDir, 'auth.json'), 'user-auth-token') + mkdirSync(join(userConfigDir, 'plugins'), { recursive: true }) + writeFileSync(join(userConfigDir, 'plugins', 'user-plugin.js'), 'export default () => {}') + }) + + afterEach(() => { + rmSync(userConfigDir, { recursive: true, force: true }) + rmSync(join(userDataDir, 'opencode-hooks'), { recursive: true, force: true }) + rmSync(join(userDataDir, 'opencode-config-overlays'), { recursive: true, force: true }) + }) + + function expectUserConfigIntact(): void { + expect(readFileSync(join(userConfigDir, 'opencode.json'), 'utf8')).toBe( + '{"userTheme":"solarized"}' + ) + expect(readFileSync(join(userConfigDir, 'auth.json'), 'utf8')).toBe('user-auth-token') + expect(readFileSync(join(userConfigDir, 'plugins', 'user-plugin.js'), 'utf8')).toBe( + 'export default () => {}' + ) + } + + it('builds an overlay under userData and exposes user config + Orca plugin together', () => { + const service = new OpenCodeHookService() + const env = service.buildPtyEnv(ptyId, userConfigDir) + + expect(env.OPENCODE_CONFIG_DIR).toBe( + join(userDataDir, 'opencode-config-overlays', toSafeDirName(ptyId)) + ) + expect(env.OPENCODE_CONFIG_DIR).not.toBe(userConfigDir) + + // Mirrored user files reachable via the overlay. + expect(readFileSync(join(env.OPENCODE_CONFIG_DIR!, 'opencode.json'), 'utf8')).toBe( + '{"userTheme":"solarized"}' + ) + expect(readFileSync(join(env.OPENCODE_CONFIG_DIR!, 'auth.json'), 'utf8')).toBe( + 'user-auth-token' + ) + expect(readFileSync(join(env.OPENCODE_CONFIG_DIR!, 'plugins', 'user-plugin.js'), 'utf8')).toBe( + 'export default () => {}' + ) + + // Orca's status plugin is a sibling, not a replacement. + const orcaPluginPath = join(env.OPENCODE_CONFIG_DIR!, 'plugins', 'orca-opencode-status.js') + expect(existsSync(orcaPluginPath)).toBe(true) + expect(readFileSync(orcaPluginPath, 'utf8')).toContain('OrcaOpenCodeStatusPlugin') + + expectUserConfigIntact() + }) + + it.skipIf(process.platform === 'win32')( + 'mirrors top-level entries via symlinks so plugins/ is a real directory', + () => { + // Why: only the plugins/ subtree needs entry-by-entry mirroring so Orca + // can drop a sibling file alongside the user's plugins. Other top-level + // entries (auth.json, opencode.json) are mirrored as a single symlink so + // user edits propagate live on POSIX. + const service = new OpenCodeHookService() + const env = service.buildPtyEnv(ptyId, userConfigDir) + + const overlay = env.OPENCODE_CONFIG_DIR! + expect(lstatSync(join(overlay, 'opencode.json')).isSymbolicLink()).toBe(true) + expect(lstatSync(join(overlay, 'auth.json')).isSymbolicLink()).toBe(true) + // plugins/ must be a real directory in the overlay so Orca can write + // its sibling status plugin into it. + expect(lstatSync(join(overlay, 'plugins')).isDirectory()).toBe(true) + expect(lstatSync(join(overlay, 'plugins')).isSymbolicLink()).toBe(false) + // user-plugin.js inside plugins/ is mirrored entry-by-entry. + expect(lstatSync(join(overlay, 'plugins', 'user-plugin.js')).isSymbolicLink()).toBe(true) + } + ) + + it("does not overwrite a user plugin file with the same filename as Orca's plugin", () => { + // Why: the failure mode this guards against — a user-owned plugin file + // happens to be named orca-opencode-status.js. Without the per-entry + // skip in mirrorUserConfig, the file would be linked into the overlay + // and Orca's writeFileSync would write through the symlink, destroying + // the user's content on their real filesystem. + const userOrcaSentinel = 'USER OWNED ORCA-NAMED PLUGIN — DO NOT CLOBBER' + writeFileSync(join(userConfigDir, 'plugins', 'orca-opencode-status.js'), userOrcaSentinel) + + const service = new OpenCodeHookService() + const env = service.buildPtyEnv(ptyId, userConfigDir) + + // User's source file must be untouched. + expect(readFileSync(join(userConfigDir, 'plugins', 'orca-opencode-status.js'), 'utf8')).toBe( + userOrcaSentinel + ) + + // Overlay copy is Orca's real plugin source, not the user's file. + const overlayPlugin = readFileSync( + join(env.OPENCODE_CONFIG_DIR!, 'plugins', 'orca-opencode-status.js'), + 'utf8' + ) + expect(overlayPlugin).toContain('OrcaOpenCodeStatusPlugin') + expect(overlayPlugin).not.toBe(userOrcaSentinel) + expectUserConfigIntact() + }) + + it.skipIf(process.platform === 'win32')( + 'does not write through a symlinked plugins/ directory into the user filesystem', + () => { + // Why: if plugins/ is a symlink (common dotfiles pattern), writing Orca's + // status plugin through it would land in the user's real filesystem — + // exactly the failure mode docs/opencode-config-dir-collision.md rejects. + const realPluginsDir = mkdtempSync(join(tmpdir(), 'orca-real-plugins-')) + try { + writeFileSync(join(realPluginsDir, 'real-plugin.js'), 'REAL USER PLUGIN') + + // Replace the userConfigDir/plugins dir created by beforeEach with a + // symlink pointing at the external "real" plugins dir. + rmSync(join(userConfigDir, 'plugins'), { recursive: true, force: true }) + symlinkSync(realPluginsDir, join(userConfigDir, 'plugins'), 'dir') + + const service = new OpenCodeHookService() + const env = service.buildPtyEnv(ptyId, userConfigDir) + + // The user's real filesystem must NOT receive Orca's status plugin. + expect(existsSync(join(realPluginsDir, 'orca-opencode-status.js'))).toBe(false) + // Overlay's plugins/ must be a real directory, not a symlink that + // would write through to the user's filesystem. + expect(lstatSync(join(env.OPENCODE_CONFIG_DIR!, 'plugins')).isSymbolicLink()).toBe(false) + // Orca's status plugin lands in the overlay only. + expect( + existsSync(join(env.OPENCODE_CONFIG_DIR!, 'plugins', 'orca-opencode-status.js')) + ).toBe(true) + // The user's sentinel plugin is reachable through the overlay (mirrored + // entry-by-entry after resolving the symlink target). + expect( + readFileSync(join(env.OPENCODE_CONFIG_DIR!, 'plugins', 'real-plugin.js'), 'utf8') + ).toBe('REAL USER PLUGIN') + } finally { + rmSync(realPluginsDir, { recursive: true, force: true }) + } + } + ) + + it("preserves the user's OPENCODE_CONFIG_DIR when the path does not exist", () => { + // Why: typoed user path — overriding it with an Orca-owned dir would let + // Orca's status plugin "succeed" while silently hiding the user's typo. + // The design rejects that: leave the user's value alone and let OpenCode + // surface the typo on its own. + const service = new OpenCodeHookService() + const missingPath = join(tmpdir(), `orca-opencode-nope-${Date.now()}`) + expect(existsSync(missingPath)).toBe(false) + + const env = service.buildPtyEnv(ptyId, missingPath) + expect(env).toEqual({ OPENCODE_CONFIG_DIR: missingPath }) + // No overlay was created at the typo path. + expect(existsSync(missingPath)).toBe(false) + // No overlay dir under userData either. + expect(existsSync(join(userDataDir, 'opencode-config-overlays', toSafeDirName(ptyId)))).toBe( + false + ) + }) + + it("preserves the user's OPENCODE_CONFIG_DIR when the mirror step fails", async () => { + // Why: mock the shared mirrorEntry helper to throw on the first symlink + // (e.g. Windows without developer mode → EPERM). The hook service must + // catch and fall back to { OPENCODE_CONFIG_DIR: existingConfigDir } — + // the user's plugins/auth/models keep loading; only Orca's status plugin + // is forfeited. + const overlayMirror = await import('../pty/overlay-mirror') + const mirrorSpy = vi.spyOn(overlayMirror, 'mirrorEntry').mockImplementation(() => { + throw new Error('simulated EPERM on symlink') + }) + try { + const service = new OpenCodeHookService() + const env = service.buildPtyEnv(ptyId, userConfigDir) + expect(env).toEqual({ OPENCODE_CONFIG_DIR: userConfigDir }) + // Overlay dir was rolled back so a half-built tree does not leak. + const overlayDir = join(userDataDir, 'opencode-config-overlays', toSafeDirName(ptyId)) + expect(existsSync(overlayDir)).toBe(false) + expectUserConfigIntact() + } finally { + mirrorSpy.mockRestore() + } + }) + + it.skipIf(process.platform === 'win32')( + 'clearPty removes the overlay without following symlinks into the user dir', + () => { + // Critical regression guard from issue #1083: the overlay's mirrored + // entries are symlinks pointing at the user's real config. Recursive + // teardown must NEVER walk through them. + const service = new OpenCodeHookService() + service.buildPtyEnv(ptyId, userConfigDir) + + const overlayDir = join(userDataDir, 'opencode-config-overlays', toSafeDirName(ptyId)) + expect(existsSync(overlayDir)).toBe(true) + + service.clearPty(ptyId) + + expect(existsSync(overlayDir)).toBe(false) + // User config must still exist with all original contents. + expectUserConfigIntact() + // The plugins/ dir under the user config is also intact. + expect(readdirSync(join(userConfigDir, 'plugins'))).toEqual(['user-plugin.js']) + } + ) + + it('rebuilding the overlay for the same ptyId does not corrupt the user dir', () => { + // Mirrors the daemon cold-restore code path that calls buildPtyEnv with + // the same sessionId across restarts. Each rebuild must clear the prior + // overlay safely (no symlink-walk into user data) and produce a fresh + // overlay with both user files and Orca's plugin. + const service = new OpenCodeHookService() + service.buildPtyEnv(ptyId, userConfigDir) + service.buildPtyEnv(ptyId, userConfigDir) + const env = service.buildPtyEnv(ptyId, userConfigDir) + + expect( + readFileSync(join(env.OPENCODE_CONFIG_DIR!, 'plugins', 'orca-opencode-status.js'), 'utf8') + ).toContain('OrcaOpenCodeStatusPlugin') + expectUserConfigIntact() + }) +}) diff --git a/src/main/opencode/hook-service.ts b/src/main/opencode/hook-service.ts index b789b2146..074ed6720 100644 --- a/src/main/opencode/hook-service.ts +++ b/src/main/opencode/hook-service.ts @@ -4,10 +4,22 @@ runtime artifact and scatter tightly coupled string-template logic. */ import { app } from 'electron' import { join } from 'path' -import { mkdirSync, writeFileSync, rmSync } from 'fs' +import { + existsSync, + mkdirSync, + readdirSync, + realpathSync, + rmSync, + statSync, + unlinkSync, + writeFileSync +} from 'fs' import { createHash } from 'crypto' +import { mirrorEntry, safeRemoveOverlay } from '../pty/overlay-mirror' const ORCA_OPENCODE_PLUGIN_FILE = 'orca-opencode-status.js' +const OPENCODE_LEGACY_HOOKS_DIR = 'opencode-hooks' +const OPENCODE_OVERLAY_DIR = 'opencode-config-overlays' // Why: the id passed in by pty.ts's daemon path is a sessionId shaped like // "@@" where worktreeId itself contains "::" and a @@ -331,44 +343,169 @@ export class OpenCodeHookService { if (!isUsableId(ptyId)) { return } - // Why: writePluginConfig creates a directory per PTY under userData. - // Without cleanup these accumulate across sessions. Using getConfigDir - // keeps cleanup aligned with the path writePluginConfig created. - const configDir = this.getConfigDir(ptyId) + // Why: a PTY with this id may have been spawned under either regime — + // overlay (user had OPENCODE_CONFIG_DIR set) or legacy per-PTY (no user + // value). Both roots use the same hashed name, so try the overlay first + // via the safe-descend teardown that never follows symlinks/junctions + // into the user's source dir, then sweep any legacy directory left over + // from a prior code path. + safeRemoveOverlay(this.getOverlayDir(ptyId), this.getOverlayRoot()) try { - rmSync(configDir, { recursive: true, force: true }) + rmSync(this.getLegacyConfigDir(ptyId), { recursive: true, force: true }) } catch { - // Why: best-effort cleanup. The directory may already be gone if the user - // manually purged userData, or the OS may hold a lock briefly. + // Why: best-effort cleanup of the no-OPENCODE_CONFIG_DIR-set regime; + // there are no symlinks/junctions in this tree (Orca-only files), so + // rmSync recursive is safe here. } } - buildPtyEnv(ptyId: string): Record { - const configDir = this.writePluginConfig(ptyId) - if (!configDir) { - // Why: plugin config is best-effort. Returning an empty object lets the - // PTY spawn without the OpenCode plugin when the filesystem is locked; - // the agent-hooks env (ORCA_AGENT_HOOK_PORT/TOKEN/ORCA_PANE_KEY) is - // still injected separately by ipc/pty.ts so other agents keep working. - return {} - } - - // Why: OPENCODE_CONFIG_DIR points OpenCode at a plugin directory we own. - // Injecting it into every Orca PTY means manually launched `opencode` - // sessions automatically pick up the status plugin too, not just sessions - // started from a hardcoded command template. - return { OPENCODE_CONFIG_DIR: configDir } - } - - private getConfigDir(ptyId: string): string { - return join(app.getPath('userData'), 'opencode-hooks', toSafeDirName(ptyId)) - } - - private writePluginConfig(ptyId: string): string | null { + buildPtyEnv(ptyId: string, existingConfigDir?: string | undefined): Record { if (!isUsableId(ptyId)) { - return null + // Why: defense-in-depth. If the id fails the bounds guard, a user-set + // OPENCODE_CONFIG_DIR should still be preserved so OpenCode loads the + // user's own config — only the Orca status plugin is forfeited. + return existingConfigDir ? { OPENCODE_CONFIG_DIR: existingConfigDir } : {} } - const configDir = this.getConfigDir(ptyId) + + if (!existingConfigDir) { + // Why: no user value to mirror — keep the original per-PTY behavior so + // a manually launched `opencode` outside Orca's command templates picks + // up the status plugin via OPENCODE_CONFIG_DIR injection alone. + const configDir = this.writeLegacyPluginConfig(ptyId) + if (!configDir) { + return {} + } + return { OPENCODE_CONFIG_DIR: configDir } + } + + // Why: do NOT `mkdir -p` the user's typoed path — overriding it with an + // Orca-owned dir is the exact failure mode we reject Superset's wrapper + // for in docs/opencode-config-dir-collision.md. Let OpenCode surface the + // typo on its own; we only forfeit our status plugin for this pane. + if (!existsSync(existingConfigDir)) { + return { OPENCODE_CONFIG_DIR: existingConfigDir } + } + + const overlayDir = this.getOverlayDir(ptyId) + safeRemoveOverlay(overlayDir, this.getOverlayRoot()) + + try { + mkdirSync(overlayDir, { recursive: true }) + this.mirrorUserConfig(existingConfigDir, overlayDir) + this.writePluginIntoOverlay(overlayDir) + } catch { + // Why: overlay creation is best-effort. Symlink-creation can fail on + // Windows without developer mode (EPERM), userData can be read-only on + // locked-down corporate machines, etc. In every case, preserve the + // user's OPENCODE_CONFIG_DIR — a missing status plugin is a vastly + // smaller harm than silently dropping the user's auth/models/keymap. + this.clearPty(ptyId) + return { OPENCODE_CONFIG_DIR: existingConfigDir } + } + + return { OPENCODE_CONFIG_DIR: overlayDir } + } + + private getOverlayRoot(): string { + return join(app.getPath('userData'), OPENCODE_OVERLAY_DIR) + } + + private getOverlayDir(ptyId: string): string { + // Why: the overlay root is distinct from the legacy hooks root so the + // two regimes are easy to tell apart on disk during debugging. + return join(this.getOverlayRoot(), toSafeDirName(ptyId)) + } + + private getLegacyConfigDir(ptyId: string): string { + return join(app.getPath('userData'), OPENCODE_LEGACY_HOOKS_DIR, toSafeDirName(ptyId)) + } + + // Why: walks the user's OPENCODE_CONFIG_DIR top-level entries. The + // `plugins/` subdirectory gets created as a real directory in the overlay + // so Orca can drop a sibling file alongside the user's plugins; everything + // else (opencode.json, auth.json, themes/, etc.) is mirrored as a single + // top-level entry via symlink/junction so user edits propagate live on + // POSIX (and on Windows-with-developer-mode) without copying files. + private mirrorUserConfig(sourceDir: string, overlayDir: string): void { + for (const entry of readdirSync(sourceDir, { withFileTypes: true })) { + const sourcePath = join(sourceDir, entry.name) + + if (entry.name === 'plugins') { + // Why: check isSymbolicLink BEFORE isDirectory — a Windows junction + // can report both as true on a Dirent, and we must take the symlink + // branch so the per-entry mirroring (not a single mirrorEntry call + // that would create a symlink at /plugins) handles it. + const isSymlink = entry.isSymbolicLink() + let isLinkPointingToDir = false + if (isSymlink) { + try { + isLinkPointingToDir = statSync(sourcePath).isDirectory() + } catch { + // Why: broken symlink (target missing) or permission error — fall + // through to the default mirrorEntry path so the dangling link is + // mirrored verbatim rather than write-through-resolved. + isLinkPointingToDir = false + } + } + + if ((!isSymlink && entry.isDirectory()) || isLinkPointingToDir) { + // Why: when the user's plugins/ is a symlink-to-dir, resolve to the + // real target so readdir returns the actual entries and child paths + // join against the resolved root. mirrorEntry then creates symlinks + // pointing into the resolved real plugins (not back through the + // user's link), and /plugins itself stays a real dir so + // writePluginIntoOverlay can never write through to the user's FS. + const resolvedSource = isLinkPointingToDir ? realpathSync(sourcePath) : sourcePath + const overlayPluginsDir = join(overlayDir, 'plugins') + mkdirSync(overlayPluginsDir, { recursive: true }) + for (const pluginEntry of readdirSync(resolvedSource, { withFileTypes: true })) { + // Why: skip a user file with the same filename as Orca's plugin — + // mirroring it here would either resolve a same-named target via + // symlink (writePluginIntoOverlay then clobbers the user's file + // through the link) or collide on Windows with the directory entry + // about to be created by writePluginIntoOverlay. Either way the + // user's plugin would be lost. Skipping yields the desired + // semantics: Orca's status plugin runs and the user's same-named + // plugin is shadowed for this PTY only — their source file on disk + // is untouched. + if (pluginEntry.name === ORCA_OPENCODE_PLUGIN_FILE) { + continue + } + mirrorEntry( + join(resolvedSource, pluginEntry.name), + join(overlayPluginsDir, pluginEntry.name) + ) + } + continue + } + } + + mirrorEntry(sourcePath, join(overlayDir, entry.name)) + } + } + + // Why: write Orca's status plugin into the overlay's plugins/ dir. The + // pre-write unlink is the load-bearing part — POSIX writeFileSync over a + // symlink writes through to the link target, so without it a user-owned + // plugin with this filename would be clobbered through a mirrored link. + // Skipping the same-named user file in mirrorUserConfig already prevents + // the link from being created, but the unlink keeps this function safe + // even if a stale overlay slips through with the link still in place. + private writePluginIntoOverlay(overlayDir: string): void { + const pluginsDir = join(overlayDir, 'plugins') + mkdirSync(pluginsDir, { recursive: true }) + const pluginPath = join(pluginsDir, ORCA_OPENCODE_PLUGIN_FILE) + try { + unlinkSync(pluginPath) + } catch { + // No-op: file may not exist on a fresh overlay. Any persistent failure + // (e.g. permissions) will surface on the writeFileSync below. + } + writeFileSync(pluginPath, getOpenCodePluginSource()) + } + + private writeLegacyPluginConfig(ptyId: string): string | null { + const configDir = this.getLegacyConfigDir(ptyId) const pluginsDir = join(configDir, 'plugins') try { mkdirSync(pluginsDir, { recursive: true }) diff --git a/src/main/pi/titlebar-extension-service.ts b/src/main/pi/titlebar-extension-service.ts index 36d508778..32fb00fb2 100644 --- a/src/main/pi/titlebar-extension-service.ts +++ b/src/main/pi/titlebar-extension-service.ts @@ -1,22 +1,22 @@ -import { - cpSync, - existsSync, - lstatSync, - linkSync, - mkdirSync, - readdirSync, - rmdirSync, - symlinkSync, - unlinkSync, - writeFileSync -} from 'fs' +import { existsSync, mkdirSync, readdirSync, writeFileSync } from 'fs' import { homedir } from 'os' -import { basename, join, relative, resolve, sep } from 'path' +import { basename, join } from 'path' import { app } from 'electron' import { ORCA_PI_AGENT_STATUS_EXTENSION_FILE, getPiAgentStatusExtensionSource } from './agent-status-extension-source' +import { + isSafeDescendCandidate as sharedIsSafeDescendCandidate, + mirrorEntry, + safeRemoveOverlay +} from '../pty/overlay-mirror' + +// Why: the Pi test suite imports `isSafeDescendCandidate` from this module's +// public surface to lock in the Windows-junction ordering invariant against +// future refactors. Re-export the shared implementation so the test contract +// keeps holding after the helper moved to src/main/pty/overlay-mirror.ts. +export const isSafeDescendCandidate = sharedIsSafeDescendCandidate const ORCA_PI_EXTENSION_FILE = 'orca-titlebar-spinner.ts' const ORCA_PI_PREFILL_EXTENSION_FILE = 'orca-prefill.ts' @@ -123,49 +123,6 @@ function getDefaultPiAgentDir(): string { return join(homedir(), PI_AGENT_DIR_NAME, PI_AGENT_SUBDIR) } -function mirrorEntry(sourcePath: string, targetPath: string): void { - // Why: lstatSync (not statSync) so that if the user's Pi dir contains its - // OWN symlinks (e.g. skills symlinked from ~/.agents/skills), we mirror the - // link itself rather than resolving it to a type and then creating a junction - // at an unrelated path. isSymbolicLink() MUST be checked before isDirectory() - // on Windows because directory junctions/reparse points report both true. - const sourceStats = lstatSync(sourcePath) - const isSymlink = sourceStats.isSymbolicLink() - const isDirectoryLike = !isSymlink && sourceStats.isDirectory() - - if (process.platform === 'win32') { - if (isDirectoryLike) { - symlinkSync(sourcePath, targetPath, 'junction') - return - } - - try { - linkSync(sourcePath, targetPath) - return - } catch { - cpSync(sourcePath, targetPath) - return - } - } - - symlinkSync(sourcePath, targetPath, isDirectoryLike ? 'dir' : 'file') -} - -// Exported for tests. A "descend candidate" is an entry whose children we -// should recurse into when tearing down the overlay. Anything that is a -// symlink (including a Windows directory junction) must NOT be a candidate -// even if it also reports isDirectory() — following it would walk into the -// link target and delete user data, which is the bug in #1083. -export function isSafeDescendCandidate(stats: { - isSymbolicLink(): boolean - isDirectory(): boolean -}): boolean { - if (stats.isSymbolicLink()) { - return false - } - return stats.isDirectory() -} - export class PiTitlebarExtensionService { private getOverlayRoot(): string { return join(app.getPath('userData'), PI_OVERLAY_DIR_NAME) @@ -175,85 +132,11 @@ export class PiTitlebarExtensionService { return join(this.getOverlayRoot(), ptyId) } - // Why: the overlay tree contains symlinks/junctions that point back into the - // user's real Pi state (~/.pi/agent or $PI_CODING_AGENT_DIR). fs.rmSync with - // { recursive: true } has repeatedly regressed on Windows when walking - // NTFS junctions — it can follow them and delete the *target*, destroying - // the user's skills, extensions, sessions, and auth.json. See issue #1083. - // - // Never descend into a symlink/junction here: for any non-real-directory - // entry we unlink the link itself; only entries that are truly directories - // on disk (our own extensions/ dir and the overlay root) are recursed into. - // We also refuse to operate on any path outside the overlay root as a - // last-line guard against PI_OVERLAY_DIR_NAME ever being mis-resolved. + // Why: overlay teardown must use the shared safeRemoveOverlay so the + // Windows-junction guard from issue #1083 stays in lock-step across all + // overlay consumers (Pi here, OpenCode in src/main/opencode/hook-service.ts). private safeRemoveOverlay(overlayDir: string): void { - const overlayRoot = this.getOverlayRoot() - const resolvedRoot = resolve(overlayRoot) - const resolvedTarget = resolve(overlayDir) - const rel = relative(resolvedRoot, resolvedTarget) - if (rel === '' || rel.startsWith('..') || rel.includes(`..${sep}`)) { - // Target is not strictly inside the overlay root — refuse to touch it. - // Log so a misconfigured caller does not silently leak overlays forever - // with no signal that this guard is firing. - console.warn( - `[pi-titlebar] refusing to remove overlay outside root: target=${resolvedTarget} root=${resolvedRoot}` - ) - return - } - this.safeRemoveTree(resolvedTarget) - } - - private safeRemoveTree(path: string): void { - let stat - try { - stat = lstatSync(path) - } catch { - return - } - - // Any symlink or Windows junction is unlinked in place, NEVER descended. - // statSync would follow the link and report the target's stats, which is - // exactly the bug we are guarding against, so the check uses lstat. - // - // On Windows, lstat on a directory junction can report BOTH - // isSymbolicLink() === true AND isDirectory() === true, so we MUST check - // isSymbolicLink first — otherwise a junction enters the recursive branch - // and readdirSync enumerates the link's target, the exact bug in #1083. - if (!isSafeDescendCandidate(stat)) { - try { - unlinkSync(path) - } catch { - // Best-effort: antivirus/indexers can hold handles briefly on Windows. - // A leftover link is harmless; the next spawn rebuilds the overlay. - } - return - } - - let entries - try { - entries = readdirSync(path, { withFileTypes: true }) - } catch { - return - } - - for (const entry of entries) { - const child = join(path, entry.name) - if (isSafeDescendCandidate(entry)) { - this.safeRemoveTree(child) - continue - } - try { - unlinkSync(child) - } catch { - // best-effort, see above - } - } - - try { - rmdirSync(path) - } catch { - // Directory may be non-empty if an unlink above failed; harmless. - } + safeRemoveOverlay(overlayDir, this.getOverlayRoot()) } private mirrorAgentDir(sourceAgentDir: string, overlayDir: string): void { diff --git a/src/main/pty/overlay-mirror.ts b/src/main/pty/overlay-mirror.ts new file mode 100644 index 000000000..d529ac139 --- /dev/null +++ b/src/main/pty/overlay-mirror.ts @@ -0,0 +1,128 @@ +// Why: Pi (PI_CODING_AGENT_DIR) and OpenCode (OPENCODE_CONFIG_DIR) both inject +// Orca-owned files into per-PTY overlay directories that mirror a user-owned +// source dir via symlinks/junctions. The safety guarantees here — never +// descend into a symlink/junction during teardown, refuse to operate outside +// the overlay root, lstat-not-stat to avoid following links — are the result +// of debugging issue #1083 (Windows directory junctions causing fs.rmSync to +// delete the user's real Pi state). Shared in one module so a new overlay +// consumer cannot accidentally diverge from the audited cleanup behavior. + +import { cpSync, linkSync, lstatSync, readdirSync, rmdirSync, symlinkSync, unlinkSync } from 'fs' +import { join, relative, resolve, sep } from 'path' + +export function mirrorEntry(sourcePath: string, targetPath: string): void { + // Why: lstatSync (not statSync) so that if the user's source dir contains + // its OWN symlinks (e.g. skills symlinked from ~/.agents/skills), we mirror + // the link itself rather than resolving it to a type and then creating a + // junction at an unrelated path. isSymbolicLink() MUST be checked before + // isDirectory() on Windows because directory junctions/reparse points + // report both true. + const sourceStats = lstatSync(sourcePath) + const isSymlink = sourceStats.isSymbolicLink() + const isDirectoryLike = !isSymlink && sourceStats.isDirectory() + + if (process.platform === 'win32') { + if (isDirectoryLike) { + symlinkSync(sourcePath, targetPath, 'junction') + return + } + + try { + linkSync(sourcePath, targetPath) + return + } catch { + cpSync(sourcePath, targetPath) + return + } + } + + symlinkSync(sourcePath, targetPath, isDirectoryLike ? 'dir' : 'file') +} + +// Exported for tests. A "descend candidate" is an entry whose children we +// should recurse into when tearing down the overlay. Anything that is a +// symlink (including a Windows directory junction) must NOT be a candidate +// even if it also reports isDirectory() — following it would walk into the +// link target and delete user data, which is the bug in #1083. +export function isSafeDescendCandidate(stats: { + isSymbolicLink(): boolean + isDirectory(): boolean +}): boolean { + if (stats.isSymbolicLink()) { + return false + } + return stats.isDirectory() +} + +// Why: the overlay tree contains symlinks/junctions that point back into the +// user's real state dir. fs.rmSync with { recursive: true } has repeatedly +// regressed on Windows when walking NTFS junctions — it can follow them and +// delete the *target*, destroying the user's data. Never descend into a +// symlink/junction here: for any non-real-directory entry we unlink the link +// itself; only entries that are truly directories on disk are recursed into. +export function safeRemoveTree(path: string): void { + let stat + try { + stat = lstatSync(path) + } catch { + return + } + + // On Windows, lstat on a directory junction can report BOTH + // isSymbolicLink() === true AND isDirectory() === true, so we MUST check + // isSymbolicLink first — otherwise a junction enters the recursive branch + // and readdirSync enumerates the link's target, the exact bug in #1083. + if (!isSafeDescendCandidate(stat)) { + try { + unlinkSync(path) + } catch { + // Best-effort: antivirus/indexers can hold handles briefly on Windows. + // A leftover link is harmless; the next spawn rebuilds the overlay. + } + return + } + + let entries + try { + entries = readdirSync(path, { withFileTypes: true }) + } catch { + return + } + + for (const entry of entries) { + const child = join(path, entry.name) + if (isSafeDescendCandidate(entry)) { + safeRemoveTree(child) + continue + } + try { + unlinkSync(child) + } catch { + // best-effort, see above + } + } + + try { + rmdirSync(path) + } catch { + // Directory may be non-empty if an unlink above failed; harmless. + } +} + +// Why: last-line guard against an overlay-root constant ever being +// mis-resolved. Any caller that points safeRemoveTree at a path outside its +// designated overlay root is refused so a misconfiguration cannot turn into +// an `rm -rf` of arbitrary user data. Logs (rather than throws) so a buggy +// caller stays visible without crashing the PTY spawn. +export function safeRemoveOverlay(overlayDir: string, overlayRoot: string): void { + const resolvedRoot = resolve(overlayRoot) + const resolvedTarget = resolve(overlayDir) + const rel = relative(resolvedRoot, resolvedTarget) + if (rel === '' || rel.startsWith('..') || rel.includes(`..${sep}`)) { + console.warn( + `[overlay-mirror] refusing to remove overlay outside root: target=${resolvedTarget} root=${resolvedRoot}` + ) + return + } + safeRemoveTree(resolvedTarget) +}