From fee8143d619bfd664c0c291abf8354fe119c90ad Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:41:21 -0700 Subject: [PATCH] fix(opencode): atomically install status plugin entrypoints Retain complete status plugin files during replacement, symlinks and existing permissions, including legacy Windows directory permission recovery. Fixes #24121. Continues #24131; permission-recovery review credited to pullfrog. Co-authored-by: Ahmed Nagy --- src/main/codex-accounts/fs-utils.ts | 77 +---- .../opencode/hook-service-legacy-acl.test.ts | 318 ++++++++++++++++++ src/main/opencode/hook-service.ts | 38 ++- src/main/opencode/legacy-plugin-acl-retry.ts | 30 ++ src/relay/opencode-canonical-config.ts | 10 +- src/relay/plugin-overlay.test.ts | 4 +- src/relay/plugin-overlay.ts | 11 +- .../opencode-plugin-atomic-write.test.ts | 279 +++++++++++++++ src/shared/opencode-plugin-atomic-write.ts | 106 ++++++ .../opencode-plugin-permissions.test.ts | 51 +++ .../opencode-tui-plugin-install.test.ts | 59 ++++ src/shared/opencode-tui-plugin-install.ts | 29 +- src/shared/windows-retry-file-operations.ts | 69 ++++ 13 files changed, 970 insertions(+), 111 deletions(-) create mode 100644 src/main/opencode/hook-service-legacy-acl.test.ts create mode 100644 src/main/opencode/legacy-plugin-acl-retry.ts create mode 100644 src/shared/opencode-plugin-atomic-write.test.ts create mode 100644 src/shared/opencode-plugin-atomic-write.ts create mode 100644 src/shared/opencode-plugin-permissions.test.ts create mode 100644 src/shared/opencode-tui-plugin-install.test.ts create mode 100644 src/shared/windows-retry-file-operations.ts diff --git a/src/main/codex-accounts/fs-utils.ts b/src/main/codex-accounts/fs-utils.ts index 295dfac9b66..0f96932ecb8 100644 --- a/src/main/codex-accounts/fs-utils.ts +++ b/src/main/codex-accounts/fs-utils.ts @@ -1,10 +1,15 @@ import { randomUUID } from 'node:crypto' -import { copyFileSync, existsSync, linkSync, renameSync, rmSync, writeFileSync } from 'node:fs' -import { rename } from 'node:fs/promises' +import { existsSync, linkSync, rmSync, writeFileSync } from 'node:fs' import { dirname } from 'node:path' -import { setTimeout } from 'node:timers/promises' import { grantDirAcl, isPermissionError } from '../win32-utils' import { nodeFileContentsEqualSync } from '../../shared/node-file-content-equality' +import { + copyFileWithWindowsRetry, + renameFileWithWindowsRetry, + renameFileWithWindowsRetryAsync +} from '../../shared/windows-retry-file-operations' + +export { copyFileWithWindowsRetry, renameFileWithWindowsRetry, renameFileWithWindowsRetryAsync } export function writeFileAtomically( targetPath: string, @@ -193,69 +198,3 @@ export function publishFileWithoutOverwrite(sourcePath: string, targetPath: stri throw error } } - -// Why: on Windows, file replacement and backup-copy operations can fail with -// EPERM/EACCES/EBUSY if another process (antivirus, Claude CLI, Codex CLI) -// holds the target file open. A short retry avoids transient failures without -// masking real permission errors. Total backoff (~750ms) covers typical AV -// scan windows seen in issue #1507. -export function renameFileWithWindowsRetry(source: string, target: string): void { - runFileOperationWithWindowsRetry(() => renameSync(source, target)) -} - -export async function renameFileWithWindowsRetryAsync( - source: string, - target: string, - isCurrent: () => boolean = () => true -): Promise { - for (let attempt = 1; ; attempt++) { - if (!isCurrent()) { - return false - } - try { - await rename(source, target) - return true - } catch (error) { - if (!shouldRetryFileOperation(error, attempt)) { - throw error - } - await setTimeout(attempt * 50) - } - } -} - -export function copyFileWithWindowsRetry(source: string, target: string): void { - runFileOperationWithWindowsRetry(() => copyFileSync(source, target)) -} - -function runFileOperationWithWindowsRetry(operation: () => void): void { - for (let attempt = 1; ; attempt++) { - try { - operation() - return - } catch (error) { - if (shouldRetryFileOperation(error, attempt)) { - sleepSync(attempt * 50) - continue - } - throw error - } - } -} - -function shouldRetryFileOperation(error: unknown, attempt: number): boolean { - return ( - process.platform === 'win32' && - attempt < 6 && - error instanceof Error && - 'code' in error && - (error.code === 'EPERM' || error.code === 'EACCES' || error.code === 'EBUSY') - ) -} - -// Why: writeFileAtomically is a sync API called from sync paths, so the retry -// backoff must park the thread instead of burning CPU in a Date.now() loop. -const sleepBuffer = new Int32Array(new SharedArrayBuffer(4)) -function sleepSync(ms: number): void { - Atomics.wait(sleepBuffer, 0, 0, ms) -} diff --git a/src/main/opencode/hook-service-legacy-acl.test.ts b/src/main/opencode/hook-service-legacy-acl.test.ts new file mode 100644 index 00000000000..fa8fe75b7d6 --- /dev/null +++ b/src/main/opencode/hook-service-legacy-acl.test.ts @@ -0,0 +1,318 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync +} from 'node:fs' +import { tmpdir } from 'node:os' +import { basename, dirname, join } from 'node:path' +import type * as NodeFs from 'node:fs' +import type * as Win32Utils from '../win32-utils' +import { setAppEnvironment } from '../../shared/app-environment' +import { openCodeTuiPluginDirName } from '../../shared/opencode-tui-plugin-install' +import { + OpenCodeHookService, + openCode2HookService, + getOpenCodePluginSource, + getOpenCode2PluginSource +} from './hook-service' + +const fsMock = vi.hoisted(() => ({ + writeFileSync: vi.fn(), + mkdirSync: vi.fn(), + grantDirAcl: vi.fn<(directory: string) => void>() +})) + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal() + fsMock.writeFileSync.mockImplementation(actual.writeFileSync) + fsMock.mkdirSync.mockImplementation(actual.mkdirSync) + return { ...actual, writeFileSync: fsMock.writeFileSync, mkdirSync: fsMock.mkdirSync } +}) + +vi.mock('../win32-utils', async (importOriginal) => ({ + ...(await importOriginal()), + grantDirAcl: fsMock.grantDirAcl +})) + +const hostPlatform = process.platform +const variants = [ + { + hooks: 'opencode-hooks', + file: 'orca-opencode-status.js', + service: new OpenCodeHookService(), + source: getOpenCodePluginSource + }, + { + hooks: 'opencode2-hooks', + file: 'orca-opencode2-status.js', + service: openCode2HookService, + source: getOpenCode2PluginSource + } +] + +let root: string + +beforeEach(async () => { + const actual = await vi.importActual('node:fs') + fsMock.writeFileSync.mockReset().mockImplementation(actual.writeFileSync) + fsMock.mkdirSync.mockReset().mockImplementation(actual.mkdirSync) + fsMock.grantDirAcl.mockReset() + root = realpathSync(mkdtempSync(join(tmpdir(), 'orca-legacy-plugin-acl-'))) + setAppEnvironment({ + getPath: () => root, + getAppPath: () => process.cwd(), + getVersion: () => '0.0.0-test', + isPackaged: () => false, + onWillQuit: () => {}, + exit: () => {}, + getAppMetrics: () => [] + }) + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + vi.spyOn(console, 'warn').mockImplementation(() => {}) +}) + +afterEach(() => { + vi.restoreAllMocks() + rmSync(root, { recursive: true, force: true }) +}) + +function installedPlugin(variant: (typeof variants)[number]): { + server: string + tui: string + source: string +} { + const plugins = join(root, variant.hooks, 'shared', 'plugins') + const server = join(plugins, variant.file) + const tui = join(plugins, openCodeTuiPluginDirName(variant.file), 'tui.js') + const source = variant.source() + mkdirSync(dirname(tui), { recursive: true }) + writeFileSync(server, '// stale server') + writeFileSync(tui, source) + fsMock.writeFileSync.mockClear() + fsMock.mkdirSync.mockClear() + return { server, tui, source } +} + +function tempWritesFor(target: string): string[] { + return fsMock.writeFileSync.mock.calls + .map(([file]) => String(file)) + .filter((file) => dirname(file) === dirname(target) && basename(file).startsWith('.')) +} + +describe.each(variants)('$hooks legacy plugin ACL recovery', (variant) => { + it.each(['EPERM', 'EACCES'])('retries one denied server generation after %s', async (code) => { + const { server, tui, source } = installedPlugin(variant) + const actual = await vi.importActual('node:fs') + const denial = Object.assign(new Error('protected directory DACL'), { code }) + let attempts = 0 + fsMock.writeFileSync.mockImplementation((...args) => { + expect(readFileSync(server, 'utf8')).toBe('// stale server') + expect(readFileSync(tui, 'utf8')).toBe(source) + actual.writeFileSync(...args) + if (++attempts === 1) { + throw denial + } + }) + + variant.service.refreshLegacySharedPlugin() + + expect(readFileSync(server, 'utf8')).toBe(source) + expect(fsMock.grantDirAcl).toHaveBeenCalledExactlyOnceWith(dirname(server)) + const generations = tempWritesFor(server) + expect(generations).toHaveLength(2) + expect(new Set(generations).size).toBe(2) + expect(generations.every((path) => !existsSync(path))).toBe(true) + expect(fsMock.grantDirAcl.mock.invocationCallOrder[0]).toBeGreaterThan( + fsMock.writeFileSync.mock.invocationCallOrder[0] + ) + expect(fsMock.grantDirAcl.mock.invocationCallOrder[0]).toBeLessThan( + fsMock.writeFileSync.mock.invocationCallOrder[1] + ) + expect(console.warn).not.toHaveBeenCalled() + }) + + it('recovers a denied mkdir in the shared atomic server writer', async () => { + const { server, source } = installedPlugin(variant) + const actual = await vi.importActual('node:fs') + fsMock.mkdirSync + .mockImplementationOnce(() => { + throw Object.assign(new Error('mkdir denied'), { code: 'EPERM' }) + }) + .mockImplementation(actual.mkdirSync) + + variant.service.refreshLegacySharedPlugin() + + expect(fsMock.grantDirAcl).toHaveBeenCalledExactlyOnceWith(dirname(server)) + expect(fsMock.mkdirSync).toHaveBeenCalledTimes(2) + expect(readFileSync(server, 'utf8')).toBe(source) + }) + + it('grants the existing parent when creation of the TUI directory is denied', async () => { + const { server, tui, source } = installedPlugin(variant) + rmSync(dirname(tui), { recursive: true }) + const actual = await vi.importActual('node:fs') + fsMock.mkdirSync + .mockImplementationOnce(() => { + throw Object.assign(new Error('TUI mkdir denied'), { code: 'EACCES' }) + }) + .mockImplementation(actual.mkdirSync) + fsMock.writeFileSync.mockImplementation((...args) => { + if (dirname(String(args[0])) === dirname(server)) { + expect(readFileSync(tui, 'utf8')).toBe(source) + } + return actual.writeFileSync(...args) + }) + + variant.service.refreshLegacySharedPlugin() + + expect(fsMock.grantDirAcl).toHaveBeenCalledExactlyOnceWith(dirname(server)) + expect(readFileSync(tui, 'utf8')).toBe(source) + expect(readFileSync(server, 'utf8')).toBe(source) + }) + + it('recovers a denied TUI write before replacing the server plugin', async () => { + const { server, tui, source } = installedPlugin(variant) + writeFileSync(tui, '// stale TUI') + fsMock.writeFileSync.mockClear() + const actual = await vi.importActual('node:fs') + let denied = false + fsMock.writeFileSync.mockImplementation((...args) => { + if (dirname(String(args[0])) === dirname(tui) && !denied) { + denied = true + throw Object.assign(new Error('TUI write denied'), { code: 'EPERM' }) + } + if (dirname(String(args[0])) === dirname(server)) { + expect(readFileSync(tui, 'utf8')).toBe(source) + } + return actual.writeFileSync(...args) + }) + + variant.service.refreshLegacySharedPlugin() + + expect(fsMock.grantDirAcl).toHaveBeenCalledExactlyOnceWith(dirname(tui)) + expect(tempWritesFor(tui)).toHaveLength(2) + expect(readFileSync(server, 'utf8')).toBe(source) + }) + + it('keeps the old server when TUI recovery still fails', () => { + const { server, tui } = installedPlugin(variant) + writeFileSync(tui, '// stale TUI') + fsMock.writeFileSync.mockClear() + const denial = Object.assign(new Error('TUI generation denied'), { code: 'EPERM' }) + fsMock.writeFileSync.mockImplementation(() => { + throw denial + }) + + variant.service.refreshLegacySharedPlugin() + + expect(fsMock.grantDirAcl).toHaveBeenCalledExactlyOnceWith(dirname(tui)) + expect(tempWritesFor(tui)).toHaveLength(2) + expect(tempWritesFor(server)).toHaveLength(0) + expect(readFileSync(tui, 'utf8')).toBe('// stale TUI') + expect(readFileSync(server, 'utf8')).toBe('// stale server') + expect(console.warn).toHaveBeenCalledWith(expect.any(String), server, denial) + }) + + it.each(['darwin', 'linux'] as const)('does not grant or retry on %s', (platform) => { + const { server } = installedPlugin(variant) + vi.spyOn(process, 'platform', 'get').mockReturnValue(platform) + const denial = Object.assign(new Error('write denied'), { code: 'EPERM' }) + fsMock.writeFileSync.mockImplementation(() => { + throw denial + }) + + variant.service.refreshLegacySharedPlugin() + + expect(tempWritesFor(server)).toHaveLength(1) + expect(fsMock.grantDirAcl).not.toHaveBeenCalled() + expect(readFileSync(server, 'utf8')).toBe('// stale server') + expect(console.warn).toHaveBeenCalledWith(expect.any(String), server, denial) + }) + + it.each(['EIO', 'ENOSPC', 'EBUSY', 'ENOENT'])('does not retry an unrelated %s error', (code) => { + const { server } = installedPlugin(variant) + fsMock.writeFileSync.mockImplementation(() => { + throw Object.assign(new Error('unrelated failure'), { code }) + }) + + variant.service.refreshLegacySharedPlugin() + + expect(tempWritesFor(server)).toHaveLength(1) + expect(fsMock.grantDirAcl).not.toHaveBeenCalled() + expect(readFileSync(server, 'utf8')).toBe('// stale server') + }) + + it.each(['grant', 'retry'])('keeps the original denial when the %s fails', (failure) => { + const { server } = installedPlugin(variant) + const original = Object.assign(new Error('original denial'), { code: 'EPERM' }) + const later = Object.assign(new Error('later failure'), { code: 'EACCES' }) + fsMock.writeFileSync + .mockImplementationOnce(() => { + throw original + }) + .mockImplementation(() => { + throw later + }) + if (failure === 'grant') { + fsMock.grantDirAcl.mockImplementation(() => { + throw later + }) + } + + variant.service.refreshLegacySharedPlugin() + + expect(fsMock.grantDirAcl).toHaveBeenCalledExactlyOnceWith(dirname(server)) + expect(tempWritesFor(server)).toHaveLength(failure === 'grant' ? 1 : 2) + expect(console.warn).toHaveBeenCalledWith(expect.any(String), server, original) + expect(readFileSync(server, 'utf8')).toBe('// stale server') + expect(readdirSync(dirname(server)).some((name) => name.endsWith('.tmp'))).toBe(false) + }) + + it.skipIf(hostPlatform === 'win32')( + 'repairs the canonical target directory and preserves its link', + async () => { + const { server, source } = installedPlugin(variant) + const target = join(root, 'dotfiles', 'status.js') + mkdirSync(dirname(target), { recursive: true }) + writeFileSync(target, '// stale target') + rmSync(server) + symlinkSync(target, server) + fsMock.writeFileSync.mockClear() + const actual = await vi.importActual('node:fs') + fsMock.writeFileSync + .mockImplementationOnce(() => { + throw Object.assign(new Error('target denied'), { code: 'EPERM' }) + }) + .mockImplementation(actual.writeFileSync) + + variant.service.refreshLegacySharedPlugin() + + expect(fsMock.grantDirAcl).toHaveBeenCalledExactlyOnceWith(dirname(target)) + expect(lstatSync(server).isSymbolicLink()).toBe(true) + expect(readFileSync(target, 'utf8')).toBe(source) + } + ) + + it('leaves current and absent installs alone', () => { + variant.service.refreshLegacySharedPlugin() + expect(existsSync(join(root, variant.hooks))).toBe(false) + const { server, source } = installedPlugin(variant) + writeFileSync(server, source) + fsMock.writeFileSync.mockClear() + fsMock.mkdirSync.mockClear() + + variant.service.refreshLegacySharedPlugin() + + expect(fsMock.grantDirAcl).not.toHaveBeenCalled() + expect(fsMock.writeFileSync).not.toHaveBeenCalled() + expect(fsMock.mkdirSync).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/opencode/hook-service.ts b/src/main/opencode/hook-service.ts index 88007d05c88..41f65243ab9 100644 --- a/src/main/opencode/hook-service.ts +++ b/src/main/opencode/hook-service.ts @@ -1,4 +1,3 @@ -import { writeFileAtomically } from '../codex-accounts/fs-utils' import { getAppEnvironment } from '../../shared/app-environment' import { join } from 'node:path' import { @@ -8,7 +7,6 @@ import { readdirSync, realpathSync, statSync, - unlinkSync, writeFileSync } from 'node:fs' import { createHash } from 'node:crypto' @@ -32,9 +30,15 @@ import { openCodeTuiPluginDirName, writeOpenCodeTuiPlugin } from '../../shared/opencode-tui-plugin-install' +import { writeLegacyOpenCodePluginWithAclRetry } from './legacy-plugin-acl-retry' export { getOpenCode2PluginSource, getOpenCodeFamilyPluginSource, getOpenCodePluginSource } +import { + writeCanonicalOpenCodePluginAtomically, + writeOverlayOpenCodePluginAtomically +} from '../../shared/opencode-plugin-atomic-write' + const ORCA_OPENCODE_PLUGIN_FILE = 'orca-opencode-status.js' const OPENCODE_OVERLAY_DIR = 'opencode-config-overlays' const OPENCODE_OVERLAY_MANIFEST_FILE = '.orca-opencode-overlay-manifest.json' @@ -137,9 +141,14 @@ export class OpenCodeHookService { const source = this.pluginSource() const installed = readFileSync(pluginPath, 'utf8') // Why: a TUI or service still loading this dir needs the TUI copy too, or the service keeps reporting under its starter pane. - this.writeTuiPlugin(pluginsDir, source) + writeLegacyOpenCodePluginWithAclRetry( + join(pluginsDir, openCodeTuiPluginDirName(this.pluginFileName), 'tui.js'), + () => this.writeTuiPlugin(pluginsDir, source) + ) if (installed !== source) { - writeFileAtomically(pluginPath, source) + writeLegacyOpenCodePluginWithAclRetry(pluginPath, () => + writeCanonicalOpenCodePluginAtomically(pluginPath, source) + ) } } catch (error) { if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { @@ -279,20 +288,15 @@ export class OpenCodeHookService { this.writeOverlayManifest(overlayDir, nextManifest) } - // Why: pre-write unlink guards against POSIX writeFileSync writing through a mirrored symlink and clobbering a same-named user plugin. + // Atomic replacement detaches mirrored links without touching user plugins. private writePluginIntoOverlay(overlayDir: string): void { const pluginsDir = join(overlayDir, 'plugins') mkdirSync(pluginsDir, { recursive: true }) const pluginPath = join(pluginsDir, this.pluginFileName) const source = this.pluginSource() - this.writeTuiPlugin(pluginsDir, source) + this.writeTuiPlugin(pluginsDir, source, 'overlay') if (!isOverlayOpenCodePluginCurrent(pluginPath, source)) { - try { - unlinkSync(pluginPath) - } catch { - // File may not exist on a fresh overlay; a real failure surfaces on writeFileSync below. - } - writeFileSync(pluginPath, source) + writeOverlayOpenCodePluginAtomically(pluginPath, source) } } @@ -303,13 +307,17 @@ export class OpenCodeHookService { const source = this.pluginSource() this.writeTuiPlugin(pluginsDir, source) if (!isInstalledOpenCodePluginCurrent(pluginPath, source)) { - writeFileSync(pluginPath, source) + writeCanonicalOpenCodePluginAtomically(pluginPath, source) } } - private writeTuiPlugin(pluginsDir: string, source: string): void { + private writeTuiPlugin( + pluginsDir: string, + source: string, + ownership: 'canonical' | 'overlay' = 'canonical' + ): void { if (this.installsTuiPlugin) { - writeOpenCodeTuiPlugin(pluginsDir, this.pluginFileName, source) + writeOpenCodeTuiPlugin(pluginsDir, this.pluginFileName, source, ownership) } } } diff --git a/src/main/opencode/legacy-plugin-acl-retry.ts b/src/main/opencode/legacy-plugin-acl-retry.ts new file mode 100644 index 00000000000..6f6a9580c59 --- /dev/null +++ b/src/main/opencode/legacy-plugin-acl-retry.ts @@ -0,0 +1,30 @@ +import { existsSync } from 'node:fs' +import { dirname } from 'node:path' +import { resolveCanonicalPluginWritePath } from '../../shared/opencode-plugin-atomic-write' +import { grantDirAcl, isPermissionError } from '../win32-utils' + +// Chromium can reset userData's DACL after the startup grant; keep a per-write backstop. +export function writeLegacyOpenCodePluginWithAclRetry( + pluginPath: string, + writePlugin: () => void +): void { + try { + writePlugin() + } catch (error) { + if (process.platform === 'win32' && isPermissionError(error)) { + try { + let directory = dirname(resolveCanonicalPluginWritePath(pluginPath)) + // A denied mkdir needs a grant on its existing parent before it can inherit an ACL. + while (!existsSync(directory) && dirname(directory) !== directory) { + directory = dirname(directory) + } + grantDirAcl(directory) + writePlugin() + return + } catch { + // Preserve the original permission error if the grant or retry fails. + } + } + throw error + } +} diff --git a/src/relay/opencode-canonical-config.ts b/src/relay/opencode-canonical-config.ts index 8cdfa7d5536..21ed93a3d76 100644 --- a/src/relay/opencode-canonical-config.ts +++ b/src/relay/opencode-canonical-config.ts @@ -1,7 +1,8 @@ -import { existsSync, mkdirSync, unlinkSync, writeFileSync } from 'node:fs' +import { existsSync, mkdirSync } from 'node:fs' import { isAbsolute, join, relative, resolve } from 'node:path' import { resolveOpenCodeConfigDirectory } from '../shared/opencode-config-directory' import { isInstalledOpenCodePluginCurrent } from '../shared/opencode-installed-plugin' +import { writeCanonicalOpenCodePluginAtomically } from '../shared/opencode-plugin-atomic-write' import { writeOpenCodeTuiPlugin } from '../shared/opencode-tui-plugin-install' const RELAY_HOOKS_DIR = '.orca-relay' @@ -26,12 +27,7 @@ export function installOpenCodePluginInCanonicalConfig( mkdirSync(join(configDir, 'plugins'), { recursive: true }) writeOpenCodeTuiPlugin(join(configDir, 'plugins'), pluginFileName, source) if (!isInstalledOpenCodePluginCurrent(pluginPath, source)) { - try { - unlinkSync(pluginPath) - } catch { - // The file may not exist on the first install. - } - writeFileSync(pluginPath, source) + writeCanonicalOpenCodePluginAtomically(pluginPath, source) } return true } catch (err) { diff --git a/src/relay/plugin-overlay.test.ts b/src/relay/plugin-overlay.test.ts index 9129e202287..6694627e9ae 100644 --- a/src/relay/plugin-overlay.test.ts +++ b/src/relay/plugin-overlay.test.ts @@ -137,9 +137,9 @@ describe('PluginOverlayManager', () => { manager.setSources({ opencode2PluginSource: 'v2 plugin, next release' }) expect(manager.installOpenCodePlugin('opencode2', env)).toBe(true) - expect(lstatSync(pluginPath).isFile()).toBe(true) + expect(lstatSync(pluginPath).isSymbolicLink()).toBe(true) expect(readFileSync(pluginPath, 'utf8')).toBe('v2 plugin, next release') - expect(readFileSync(targetPath, 'utf8')).toBe('v2 plugin') + expect(readFileSync(targetPath, 'utf8')).toBe('v2 plugin, next release') } ) diff --git a/src/relay/plugin-overlay.ts b/src/relay/plugin-overlay.ts index 35455d06859..6d78918c46e 100644 --- a/src/relay/plugin-overlay.ts +++ b/src/relay/plugin-overlay.ts @@ -24,9 +24,9 @@ import { readdirSync, realpathSync, statSync, - unlinkSync, writeFileSync } from 'node:fs' +import { writeOverlayOpenCodePluginAtomically } from '../shared/opencode-plugin-atomic-write' import { homedir } from 'node:os' import { join } from 'node:path' import { mirrorEntry, safeRemoveOverlay } from '../main/pty/overlay-mirror' @@ -214,13 +214,8 @@ export class PluginOverlayManager { const pluginsDir = join(overlayDir, 'plugins') mkdirSync(pluginsDir, { recursive: true }) const pluginPath = join(pluginsDir, pluginFileName) - writeOpenCodeTuiPlugin(pluginsDir, pluginFileName, source) - try { - unlinkSync(pluginPath) - } catch { - // Fresh overlay or no same-named stale symlink. - } - writeFileSync(pluginPath, source) + writeOpenCodeTuiPlugin(pluginsDir, pluginFileName, source, 'overlay') + writeOverlayOpenCodePluginAtomically(pluginPath, source) } /** Materialize the OpenCode plugin overlay for `id` (typically the diff --git a/src/shared/opencode-plugin-atomic-write.test.ts b/src/shared/opencode-plugin-atomic-write.test.ts new file mode 100644 index 00000000000..597304a3ff8 --- /dev/null +++ b/src/shared/opencode-plugin-atomic-write.test.ts @@ -0,0 +1,279 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + realpathSync, + rmSync, + statSync, + symlinkSync +} from 'node:fs' +import type * as NodeFs from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import * as retryOps from './windows-retry-file-operations' +import { + resolveCanonicalPluginWritePath, + writeCanonicalOpenCodePluginAtomically, + writeOverlayOpenCodePluginAtomically +} from './opencode-plugin-atomic-write' + +type WriteFileSyncFn = typeof NodeFs.writeFileSync + +const { fsMock } = vi.hoisted(() => { + let realWrite: WriteFileSyncFn | undefined + return { + fsMock: { + writeFileSync: vi.fn(), + getRealWrite: (): WriteFileSyncFn | undefined => realWrite, + setRealWrite: (fn: WriteFileSyncFn): void => { + realWrite = fn + } + } + } +}) + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal() + fsMock.setRealWrite(actual.writeFileSync) + fsMock.writeFileSync.mockImplementation((...args: Parameters) => + actual.writeFileSync(...args) + ) + return { + ...actual, + writeFileSync: fsMock.writeFileSync + } +}) + +afterEach(() => { + const realWrite = fsMock.getRealWrite() + if (realWrite) { + fsMock.writeFileSync.mockImplementation((...args: Parameters) => + realWrite(...args) + ) + } + vi.restoreAllMocks() +}) + +describe('opencode-plugin-atomic-write', () => { + it('writes atomically via sibling temp file and never writes directly in place', () => { + const testDir = mkdtempSync(join(tmpdir(), 'opencode-atomic-write-')) + const pluginPath = join(testDir, 'plugins', 'orca-opencode-status.js') + const writtenPaths: string[] = [] + + const realWrite = fsMock.getRealWrite() + expect(realWrite).toBeDefined() + if (!realWrite) { + return + } + + fsMock.writeFileSync.mockImplementation( + ( + file: Parameters[0], + data: Parameters[1], + options: Parameters[2] + ) => { + writtenPaths.push(String(file)) + return realWrite(file, data, options) + } + ) + + try { + writeCanonicalOpenCodePluginAtomically(pluginPath, 'console.log("hello")') + expect(readFileSync(pluginPath, 'utf8')).toBe('console.log("hello")') + expect(writtenPaths).toHaveLength(1) + expect(writtenPaths[0]).not.toBe(pluginPath) + expect(writtenPaths[0]).toContain('.orca-opencode-status.js.') + expect(writtenPaths[0]).toContain('.tmp') + } finally { + rmSync(testDir, { recursive: true, force: true }) + } + }) + + it('preserves symlink and updates underlying target in canonical mode', () => { + if (process.platform === 'win32') { + return + } + const realWrite = fsMock.getRealWrite() + expect(realWrite).toBeDefined() + if (!realWrite) { + return + } + + const testDir = mkdtempSync(join(tmpdir(), 'opencode-canonical-symlink-')) + const pluginsDir = join(testDir, 'plugins') + mkdirSync(pluginsDir, { recursive: true }) + const realFile = join(testDir, 'dotfiles-plugin.js') + const linkFile = join(pluginsDir, 'orca-opencode-status.js') + + realWrite(realFile, 'initial content', 'utf8') + symlinkSync(realFile, linkFile) + + expect(resolveCanonicalPluginWritePath(linkFile)).toBe(realpathSync.native(realFile)) + + writeCanonicalOpenCodePluginAtomically(linkFile, 'updated content') + + expect(lstatSync(linkFile).isSymbolicLink()).toBe(true) + expect(readFileSync(realFile, 'utf8')).toBe('updated content') + expect(readFileSync(linkFile, 'utf8')).toBe('updated content') + + rmSync(testDir, { recursive: true, force: true }) + }) + + it('preserves dangling symlink and creates destination target in canonical mode', () => { + if (process.platform === 'win32') { + return + } + const testDir = mkdtempSync(join(tmpdir(), 'opencode-dangling-symlink-')) + const pluginsDir = join(testDir, 'plugins') + mkdirSync(pluginsDir, { recursive: true }) + const missingTarget = join(testDir, 'dotfiles-plugin.js') + const linkFile = join(pluginsDir, 'orca-opencode-status.js') + + symlinkSync(missingTarget, linkFile) + expect(existsSync(missingTarget)).toBe(false) + expect(lstatSync(linkFile).isSymbolicLink()).toBe(true) + + expect(resolveCanonicalPluginWritePath(linkFile)).toBe(missingTarget) + + writeCanonicalOpenCodePluginAtomically(linkFile, 'dangling resolved content') + + expect(lstatSync(linkFile).isSymbolicLink()).toBe(true) + expect(existsSync(missingTarget)).toBe(true) + expect(readFileSync(missingTarget, 'utf8')).toBe('dangling resolved content') + expect(readFileSync(linkFile, 'utf8')).toBe('dangling resolved content') + + rmSync(testDir, { recursive: true, force: true }) + }) + + it('preserves existing file permissions when updating plugin', () => { + if (process.platform === 'win32') { + return + } + const realWrite = fsMock.getRealWrite() + if (!realWrite) { + return + } + const testDir = mkdtempSync(join(tmpdir(), 'opencode-permissions-')) + const pluginPath = join(testDir, 'status.js') + + realWrite(pluginPath, 'old content', { encoding: 'utf8', mode: 0o600 }) + expect(statSync(pluginPath).mode & 0o777).toBe(0o600) + + writeCanonicalOpenCodePluginAtomically(pluginPath, 'new content') + expect(readFileSync(pluginPath, 'utf8')).toBe('new content') + expect(statSync(pluginPath).mode & 0o777).toBe(0o600) + + rmSync(testDir, { recursive: true, force: true }) + }) + + it('leaves existing target intact and cleans up temp file if rename fails', () => { + const testDir = mkdtempSync(join(tmpdir(), 'opencode-rename-fail-')) + const pluginPath = join(testDir, 'status.js') + const realWrite = fsMock.getRealWrite() + if (!realWrite) { + return + } + realWrite(pluginPath, 'original content', 'utf8') + + vi.spyOn(retryOps, 'renameFileWithWindowsRetry').mockImplementation(() => { + throw new Error('EPERM: file locked') + }) + + expect(() => writeCanonicalOpenCodePluginAtomically(pluginPath, 'new content')).toThrow( + 'EPERM: file locked' + ) + expect(readFileSync(pluginPath, 'utf8')).toBe('original content') + expect(readdirSync(testDir).filter((name) => name.endsWith('.tmp'))).toEqual([]) + + rmSync(testDir, { recursive: true, force: true }) + }) + + it('replaces symlink in overlay mode without mutating user target file', () => { + if (process.platform === 'win32') { + return + } + const realWrite = fsMock.getRealWrite() + expect(realWrite).toBeDefined() + if (!realWrite) { + return + } + + const testDir = mkdtempSync(join(tmpdir(), 'opencode-overlay-symlink-')) + const pluginsDir = join(testDir, 'overlay', 'plugins') + mkdirSync(pluginsDir, { recursive: true }) + const userPlugin = join(testDir, 'user-plugin.js') + const overlayPlugin = join(pluginsDir, 'orca-opencode-status.js') + + realWrite(userPlugin, 'user original source', 'utf8') + symlinkSync(userPlugin, overlayPlugin) + + writeOverlayOpenCodePluginAtomically(overlayPlugin, 'orca status source') + + expect(lstatSync(overlayPlugin).isSymbolicLink()).toBe(false) + expect(lstatSync(overlayPlugin).isFile()).toBe(true) + expect(readFileSync(overlayPlugin, 'utf8')).toBe('orca status source') + expect(readFileSync(userPlugin, 'utf8')).toBe('user original source') + + rmSync(testDir, { recursive: true, force: true }) + }) + + it('creates missing directories automatically when writing plugin', () => { + const testDir = mkdtempSync(join(tmpdir(), 'opencode-nested-dir-')) + const deeplyNestedPlugin = join(testDir, 'nested', 'path', 'plugins', 'status.js') + + writeOverlayOpenCodePluginAtomically(deeplyNestedPlugin, 'content') + expect(existsSync(deeplyNestedPlugin)).toBe(true) + expect(readFileSync(deeplyNestedPlugin, 'utf8')).toBe('content') + + rmSync(testDir, { recursive: true, force: true }) + }) + + it('resolves long dangling symlink chains and creates target without replacing intermediate links', () => { + if (process.platform === 'win32') { + return + } + const testDir = mkdtempSync(join(tmpdir(), 'opencode-long-symlinks-')) + const missingTarget = join(testDir, 'final-target.js') + let current = missingTarget + const links: string[] = [] + for (let i = 0; i < 15; i++) { + const nextLink = join(testDir, `link-${i}.js`) + symlinkSync(current, nextLink) + current = nextLink + links.push(nextLink) + } + + writeCanonicalOpenCodePluginAtomically(current, 'long chain content') + + expect(existsSync(missingTarget)).toBe(true) + expect(readFileSync(missingTarget, 'utf8')).toBe('long chain content') + for (const link of links) { + expect(lstatSync(link).isSymbolicLink()).toBe(true) + } + + rmSync(testDir, { recursive: true, force: true }) + }) + + it('throws on symlink loop without replacing intermediate symlinks', () => { + if (process.platform === 'win32') { + return + } + const testDir = mkdtempSync(join(tmpdir(), 'opencode-loop-symlinks-')) + const linkA = join(testDir, 'link-a.js') + const linkB = join(testDir, 'link-b.js') + symlinkSync(linkB, linkA) + symlinkSync(linkA, linkB) + + expect(() => writeCanonicalOpenCodePluginAtomically(linkA, 'loop content')).toThrow( + /ELOOP|symbolic link/i + ) + expect(lstatSync(linkA).isSymbolicLink()).toBe(true) + expect(lstatSync(linkB).isSymbolicLink()).toBe(true) + + rmSync(testDir, { recursive: true, force: true }) + }) +}) diff --git a/src/shared/opencode-plugin-atomic-write.ts b/src/shared/opencode-plugin-atomic-write.ts new file mode 100644 index 00000000000..1ef27089bd0 --- /dev/null +++ b/src/shared/opencode-plugin-atomic-write.ts @@ -0,0 +1,106 @@ +import { randomUUID } from 'node:crypto' +import { + chmodSync, + existsSync, + lstatSync, + mkdirSync, + readlinkSync, + realpathSync, + statSync, + unlinkSync, + writeFileSync +} from 'node:fs' +import { basename, dirname, join, resolve } from 'node:path' +import { renameFileWithWindowsRetry } from './windows-retry-file-operations' + +function isEnoentError(error: unknown): boolean { + return error instanceof Error && 'code' in error && error.code === 'ENOENT' +} + +// Why: atomic rename on a dotfiles symlink replaces the link itself; resolving canonical target updates the real repo file. +export function resolveCanonicalPluginWritePath(pluginPath: string): string { + try { + const stat = lstatSync(pluginPath) + if (!stat.isSymbolicLink()) { + return pluginPath + } + } catch (error) { + if (isEnoentError(error)) { + return pluginPath + } + throw error + } + + // Why: realpathSync.native resolves canonical target when it exists; + // if target is missing (dangling symlink), follow readlinkSync chain so the + // target file is created at the intended destination and the symlink stays intact. + try { + return realpathSync.native(pluginPath) + } catch (error) { + if (!isEnoentError(error)) { + throw error + } + } + + const visited = new Set([pluginPath]) + let current = pluginPath + for (let depth = 0; depth < 40; depth++) { + try { + const link = readlinkSync(current) + current = resolve(dirname(current), link) + if (visited.has(current)) { + throw new Error(`Symbolic link loop detected resolving "${pluginPath}"`) + } + visited.add(current) + const nextStat = lstatSync(current) + if (!nextStat.isSymbolicLink()) { + return current + } + } catch (error) { + if (isEnoentError(error)) { + return current + } + throw error + } + } + throw new Error(`Too many levels of symbolic links resolving "${pluginPath}"`) +} + +// Why: write to sibling temp file and rename so concurrent reloads never observe a truncated or missing file. +function writeAtomicFile(targetPath: string, content: string): void { + const dir = dirname(targetPath) + mkdirSync(dir, { recursive: true }) + let existingMode: number | undefined + try { + existingMode = statSync(targetPath).mode & 0o777 + } catch { + // Target does not exist yet. + } + const tmpPath = join(dir, `.${basename(targetPath)}.${process.pid}.${randomUUID()}.tmp`) + try { + writeFileSync(tmpPath, content, { encoding: 'utf8', mode: existingMode }) + if (existingMode !== undefined) { + chmodSync(tmpPath, existingMode) + } + renameFileWithWindowsRetry(tmpPath, targetPath) + } finally { + if (existsSync(tmpPath)) { + try { + unlinkSync(tmpPath) + } catch { + // Best effort cleanup. + } + } + } +} + +// Why: preserve dotfile symlinks at canonical paths by atomically replacing the real target file. +export function writeCanonicalOpenCodePluginAtomically(pluginPath: string, source: string): void { + const targetPath = resolveCanonicalPluginWritePath(pluginPath) + writeAtomicFile(targetPath, source) +} + +// Why: replace any mirrored symlink directly in the overlay without following it to the user's config. +export function writeOverlayOpenCodePluginAtomically(pluginPath: string, source: string): void { + writeAtomicFile(pluginPath, source) +} diff --git a/src/shared/opencode-plugin-permissions.test.ts b/src/shared/opencode-plugin-permissions.test.ts new file mode 100644 index 00000000000..3bbf5f4f7e8 --- /dev/null +++ b/src/shared/opencode-plugin-permissions.test.ts @@ -0,0 +1,51 @@ +import { build } from 'esbuild' +import { expect, it } from 'vitest' +import { mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' +import { runProcess } from './child-process/run-process' +it.skipIf(process.platform === 'win32')( + 'retains existing permissions when the process umask is stricter', + async () => { + const root = mkdtempSync(join(tmpdir(), 'orca-plugin-umask-')) + const pluginPath = join(root, 'plugin.js') + const fixturePath = join(root, 'permission-check.cjs') + const modulePath = resolve(process.cwd(), 'src/shared/opencode-plugin-atomic-write.ts') + try { + await build({ + stdin: { + contents: ` + import { chmodSync, statSync, writeFileSync } from 'node:fs'; + import { writeCanonicalOpenCodePluginAtomically } from ${JSON.stringify(modulePath)}; + const target = ${JSON.stringify(pluginPath)}; + writeFileSync(target, 'old'); + chmodSync(target, 0o664); + process.umask(0o027); + writeCanonicalOpenCodePluginAtomically(target, 'new'); + console.log(statSync(target).mode & 0o777); + `, + resolveDir: process.cwd(), + loader: 'ts' + }, + bundle: true, + platform: 'node', + format: 'cjs', + target: 'node22', + outfile: fixturePath, + logLevel: 'silent' + }) + const result = await runProcess({ + program: process.execPath, + cwd: root, + args: [fixturePath], + timeoutMs: 10_000, + maxOutputBytes: 4_096 + }) + expect(result.code, result.stderr).toBe(0) + expect(result.stdout.trim()).toBe(String(0o664)) + expect(readFileSync(pluginPath, 'utf8')).toBe('new') + } finally { + rmSync(root, { recursive: true, force: true }) + } + } +) diff --git a/src/shared/opencode-tui-plugin-install.test.ts b/src/shared/opencode-tui-plugin-install.test.ts new file mode 100644 index 00000000000..18e08a4b5f5 --- /dev/null +++ b/src/shared/opencode-tui-plugin-install.test.ts @@ -0,0 +1,59 @@ +import { afterEach, expect, it } from 'vitest' +import { + lstatSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + statSync, + symlinkSync, + writeFileSync +} from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { openCodeTuiPluginDirName, writeOpenCodeTuiPlugin } from './opencode-tui-plugin-install' + +const roots: string[] = [] +afterEach(() => roots.splice(0).forEach((root) => rmSync(root, { recursive: true, force: true }))) + +function linkedEntry(): { plugins: string; entry: string; target: string } { + const root = mkdtempSync(join(tmpdir(), 'orca-tui-install-')) + roots.push(root) + const plugins = join(root, 'plugins') + const entry = join(plugins, openCodeTuiPluginDirName('orca-status.js'), 'tui.js') + const target = join(root, 'user-plugin.js') + mkdirSync(join(plugins, openCodeTuiPluginDirName('orca-status.js')), { recursive: true }) + writeFileSync(target, 'old') + symlinkSync(target, entry) + return { plugins, entry, target } +} + +it.skipIf(process.platform === 'win32')( + 'updates a canonical TUI symlink target and preserves the link', + () => { + const { plugins, entry, target } = linkedEntry() + writeOpenCodeTuiPlugin(plugins, 'orca-status.js', 'new') + expect(lstatSync(entry).isSymbolicLink()).toBe(true) + expect(readFileSync(target, 'utf8')).toBe('new') + } +) + +it.skipIf(process.platform === 'win32')( + 'detaches an overlay TUI link even when its user bytes match', + () => { + const { plugins, entry, target } = linkedEntry() + writeOpenCodeTuiPlugin(plugins, 'orca-status.js', 'old', 'overlay') + expect(lstatSync(entry).isFile()).toBe(true) + expect(readFileSync(target, 'utf8')).toBe('old') + } +) + +it('does not reload a current TUI plugin by changing its timestamp', () => { + const root = mkdtempSync(join(tmpdir(), 'orca-tui-install-')) + roots.push(root) + writeOpenCodeTuiPlugin(root, 'orca-status.js', 'current') + const entry = join(root, openCodeTuiPluginDirName('orca-status.js'), 'tui.js') + const before = statSync(entry).mtimeMs + writeOpenCodeTuiPlugin(root, 'orca-status.js', 'current') + expect(statSync(entry).mtimeMs).toBe(before) +}) diff --git a/src/shared/opencode-tui-plugin-install.ts b/src/shared/opencode-tui-plugin-install.ts index db05aa8834d..cad825212a8 100644 --- a/src/shared/opencode-tui-plugin-install.ts +++ b/src/shared/opencode-tui-plugin-install.ts @@ -1,6 +1,13 @@ -import { mkdirSync, unlinkSync, writeFileSync } from 'node:fs' +import { mkdirSync } from 'node:fs' +import { + writeCanonicalOpenCodePluginAtomically, + writeOverlayOpenCodePluginAtomically +} from './opencode-plugin-atomic-write' import { join } from 'node:path' -import { isInstalledOpenCodePluginCurrent } from './opencode-installed-plugin' +import { + isInstalledOpenCodePluginCurrent, + isOverlayOpenCodePluginCurrent +} from './opencode-installed-plugin' /** * Directory holding the TUI copy of a status plugin file. OpenCode 2 loads a @@ -19,18 +26,20 @@ export function openCodeTuiPluginDirName(pluginFileName: string): string { export function writeOpenCodeTuiPlugin( pluginsDir: string, pluginFileName: string, - source: string + source: string, + ownership: 'canonical' | 'overlay' = 'canonical' ): void { const dir = join(pluginsDir, openCodeTuiPluginDirName(pluginFileName)) const entry = join(dir, 'tui.js') - if (isInstalledOpenCodePluginCurrent(entry, source)) { + const isCurrent = + ownership === 'canonical' ? isInstalledOpenCodePluginCurrent : isOverlayOpenCodePluginCurrent + if (isCurrent(entry, source)) { return } mkdirSync(dir, { recursive: true }) - try { - unlinkSync(entry) - } catch { - // First install, or nothing to replace. - } - writeFileSync(entry, source) + const write = + ownership === 'canonical' + ? writeCanonicalOpenCodePluginAtomically + : writeOverlayOpenCodePluginAtomically + write(entry, source) } diff --git a/src/shared/windows-retry-file-operations.ts b/src/shared/windows-retry-file-operations.ts new file mode 100644 index 00000000000..759bada16e9 --- /dev/null +++ b/src/shared/windows-retry-file-operations.ts @@ -0,0 +1,69 @@ +import { copyFileSync, renameSync } from 'node:fs' +import { rename } from 'node:fs/promises' +import { setTimeout } from 'node:timers/promises' + +// Why: on Windows, file replacement and backup-copy operations can fail with +// EPERM/EACCES/EBUSY if another process (antivirus, Claude CLI, Codex CLI) +// holds the target file open. A short retry avoids transient failures without +// masking real permission errors. Total backoff (~750ms) covers typical AV +// scan windows seen in issue #1507. +export function renameFileWithWindowsRetry(source: string, target: string): void { + runFileOperationWithWindowsRetry(() => renameSync(source, target)) +} + +export async function renameFileWithWindowsRetryAsync( + source: string, + target: string, + isCurrent: () => boolean = () => true +): Promise { + for (let attempt = 1; ; attempt++) { + if (!isCurrent()) { + return false + } + try { + await rename(source, target) + return true + } catch (error) { + if (!shouldRetryFileOperation(error, attempt)) { + throw error + } + await setTimeout(attempt * 50) + } + } +} + +export function copyFileWithWindowsRetry(source: string, target: string): void { + runFileOperationWithWindowsRetry(() => copyFileSync(source, target)) +} + +function runFileOperationWithWindowsRetry(operation: () => void): void { + for (let attempt = 1; ; attempt++) { + try { + operation() + return + } catch (error) { + if (shouldRetryFileOperation(error, attempt)) { + sleepSync(attempt * 50) + continue + } + throw error + } + } +} + +function shouldRetryFileOperation(error: unknown, attempt: number): boolean { + return ( + process.platform === 'win32' && + attempt < 6 && + error instanceof Error && + 'code' in error && + (error.code === 'EPERM' || error.code === 'EACCES' || error.code === 'EBUSY') + ) +} + +// Why: writeFileAtomically is a sync API called from sync paths, so the retry +// backoff must park the thread instead of burning CPU in a Date.now() loop. +const sleepBuffer = new Int32Array(new SharedArrayBuffer(4)) +function sleepSync(ms: number): void { + Atomics.wait(sleepBuffer, 0, 0, ms) +}