diff --git a/sdk/typescript/scripts/build-plugin.mjs b/sdk/typescript/scripts/build-plugin.mjs index 054e67e66a..9adde02a97 100644 --- a/sdk/typescript/scripts/build-plugin.mjs +++ b/sdk/typescript/scripts/build-plugin.mjs @@ -12,32 +12,13 @@ import { rm, } from "node:fs/promises"; import { dirname, join, posix, resolve } from "node:path"; -import { setTimeout as delay } from "node:timers/promises"; +import { retryWindowsRename } from "../src/retry-windows-rename.js"; import { isMain } from "./is-main.mjs"; import { pluginContractFiles } from "./plugin-contract.mjs"; const packageRoot = resolve(import.meta.dirname, ".."); const repositoryRoot = resolve(packageRoot, "../.."); -async function retryWindowsRename(operation, rollback) { - // Windows can temporarily deny a directory rename while a file inside is open. - for (let attempt = 0; ; attempt++) { - try { - return await operation(); - } catch (error) { - await rollback?.(); - if ( - process.platform !== "win32" || - !["EPERM", "EACCES", "EBUSY"].includes(error.code) || - attempt === 5 - ) { - throw error; - } - await delay(100 * (attempt + 1)); - } - } -} - function validatePath(path) { const parts = path.split("/"); if ( diff --git a/sdk/typescript/scripts/package-dist-files.mjs b/sdk/typescript/scripts/package-dist-files.mjs index a09a38418a..7e1a295461 100644 --- a/sdk/typescript/scripts/package-dist-files.mjs +++ b/sdk/typescript/scripts/package-dist-files.mjs @@ -129,6 +129,7 @@ const distModules = [ "record", "remediation", "request-metadata", + "retry-windows-rename", "runtime", "scan-activity", "scan-comparison", diff --git a/sdk/typescript/src/retry-windows-rename.d.ts b/sdk/typescript/src/retry-windows-rename.d.ts new file mode 100644 index 0000000000..6628ea4f00 --- /dev/null +++ b/sdk/typescript/src/retry-windows-rename.d.ts @@ -0,0 +1,5 @@ +export function retryWindowsRename( + operation: () => Promise, + rollback?: () => Promise, + signal?: AbortSignal, +): Promise; diff --git a/sdk/typescript/src/retry-windows-rename.js b/sdk/typescript/src/retry-windows-rename.js new file mode 100644 index 0000000000..9c8899375d --- /dev/null +++ b/sdk/typescript/src/retry-windows-rename.js @@ -0,0 +1,25 @@ +import { setTimeout as delay } from "node:timers/promises"; + +/** + * Windows can temporarily deny a directory rename while a descendant is open. + * @param {() => Promise} operation + * @param {() => Promise} [rollback] + * @param {AbortSignal} [signal] + */ +export async function retryWindowsRename(operation, rollback, signal) { + for (let attempt = 0; ; attempt++) { + try { + return await operation(); + } catch (error) { + await rollback?.(); + if ( + process.platform !== "win32" || + !["EPERM", "EACCES", "EBUSY"].includes(error?.code) || + attempt === 5 + ) { + throw error; + } + await delay(100 * (attempt + 1), undefined, { signal }); + } + } +} diff --git a/sdk/typescript/src/runtime.ts b/sdk/typescript/src/runtime.ts index 5b4a8b2f8a..fe8d9fdb48 100644 --- a/sdk/typescript/src/runtime.ts +++ b/sdk/typescript/src/runtime.ts @@ -1,4 +1,5 @@ import { executablePathForSpawn } from "./executable-path.js"; +import { retryWindowsRename } from "./retry-windows-rename.js"; export { executablePathForSpawn } from "./executable-path.js"; import { resolveBundledCodexExecutable } from "./codex-sdk-environment.js"; import { gitProtectionRoots } from "./targets.js"; @@ -2642,6 +2643,7 @@ export async function extractPluginZip( const staging = await realpath( await mkdtemp(join(dirname(destination), ".codex-security-plugin-")), ); + let publishingOnWindows = false; try { throwIfSignalAborted(signal); let expandedSize = 0; @@ -2718,11 +2720,36 @@ export async function extractPluginZip( const pluginRoot = await discoverPluginRoot(staging); throwIfSignalAborted(signal); const relativeRoot = relative(staging, pluginRoot); - await rename(staging, destination); + if (process.platform === "win32") { + publishingOnWindows = true; + await retryWindowsRename( + async () => { + throwIfSignalAborted(signal); + if (await lstat(destination).catch(nullIfMissingFileError)) + throw new Error(`Destination already exists: ${destination}`); + await rename(staging, destination); + }, + undefined, + signal, + ); + publishingOnWindows = false; + } else { + await rename(staging, destination); + } return await validatePluginRoot(join(destination, relativeRoot)); } catch (error) { - await rm(staging, { recursive: true, force: true }).catch(() => undefined); + let cleanupDiagnostic = ""; + await rm(staging, { recursive: true, force: true }).catch( + (cleanupError) => { + cleanupDiagnostic = `\nCould not remove staging directory ${staging}: ${errorMessage(cleanupError)}`; + }, + ); throwIfSignalAborted(signal); + if (publishingOnWindows) + throw new PluginBootstrapError( + `Could not publish extracted plugin ZIP: ${errorMessage(error)}${cleanupDiagnostic}`, + { cause: error }, + ); if (error instanceof PluginBootstrapError) throw error; throw new PluginBootstrapError(`Invalid plugin ZIP: ${archivePath}`, { cause: error, diff --git a/sdk/typescript/tests-ts/build-plugin.test.ts b/sdk/typescript/tests-ts/build-plugin.test.ts index 0b82e255fd..97de0726b5 100644 --- a/sdk/typescript/tests-ts/build-plugin.test.ts +++ b/sdk/typescript/tests-ts/build-plugin.test.ts @@ -38,6 +38,23 @@ async function writeFixture( await writeFile(destination, contents); } +async function copyBuildEntrypoint(root: string): Promise { + for (const path of [ + "package.json", + "scripts/build-plugin.mjs", + "scripts/check-plugin-source.mjs", + "scripts/is-main.mjs", + "scripts/plugin-contract.mjs", + "src/retry-windows-rename.js", + ]) { + await writeFixture( + root, + path, + await readFile(new URL(`../${path}`, import.meta.url), "utf8"), + ); + } +} + async function files(root: string, prefix = ""): Promise { const entries = await readdir(join(root, prefix), { withFileTypes: true }); const paths = await Promise.all( @@ -435,16 +452,7 @@ describe("bundled plugin build", () => { const packageRoot = join(root, "sdk", "typescript"); const source = join(root, "plugins", "codex-security"); const destination = join(packageRoot, "_bundled_plugin"); - for (const script of ["build-plugin", "plugin-contract", "is-main"]) { - await writeFixture( - packageRoot, - `scripts/${script}.mjs`, - await readFile( - new URL(`../scripts/${script}.mjs`, import.meta.url), - "utf8", - ), - ); - } + await copyBuildEntrypoint(packageRoot); await writeFixture(source, ".codex-plugin/plugin.json", "{}\n"); await writeFixture( source, @@ -491,26 +499,7 @@ await writeFile(join(output, "unexpected.txt"), "undeclared output");\n`, const packageRoot = join(root, "sdk", "typescript"); const source = join(root, "plugins", "codex-security"); - await writeFixture( - packageRoot, - "package.json", - await readFile(new URL("../package.json", import.meta.url), "utf8"), - ); - for (const script of [ - "build-plugin", - "check-plugin-source", - "is-main", - "plugin-contract", - ]) { - await writeFixture( - packageRoot, - `scripts/${script}.mjs`, - await readFile( - new URL(`../scripts/${script}.mjs`, import.meta.url), - "utf8", - ), - ); - } + await copyBuildEntrypoint(packageRoot); await writeFixture( source, "plugin-files.json", diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index a996d793b8..31320962fb 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -1528,6 +1528,114 @@ for await (const line of createInterface({ input: process.stdin })) { } }); + test.skipIf(process.platform !== "win32").each(["released", "held"])( + "handles plugin ZIP publication when a Windows directory lock is %s", + async (lock) => { + const root = await temporaryDirectory(); + const archive = join(root, "plugin.zip"); + const destination = join(root, "extracted"); + const manifest = ".codex-plugin/plugin.json"; + const contents = JSON.stringify({ + name: "codex-security", + version: "1.2.3", + }); + await writeFile(archive, zipSync({ [manifest]: strToU8(contents) })); + const nativeRename = fsPromises.rename; + let blocked: NodeJS.ErrnoException | undefined; + let staging = ""; + let release: (() => Promise) | undefined; + using publication = spyOn(fsPromises, "rename").mockImplementation( + async (from, to) => { + if (to !== destination || staging) return nativeRename(from, to); + staging = String(from); + // Windows keeps a process's current directory open until the process exits. + const holder = childProcess.spawn( + "node", + ["-e", 'process.stdout.write("ready"); process.stdin.resume()'], + { + cwd: join(staging, ".codex-plugin"), + windowsHide: true, + }, + ); + const exited = once(holder, "exit"); + release = async () => { + holder.stdin.end(); + await exited; + }; + await once(holder.stdout, "data"); + try { + await nativeRename(from, to); + } catch (error) { + blocked = error as NodeJS.ErrnoException; + throw error; + } finally { + if (lock === "released") await release(); + } + }, + ); + try { + const result = extractPluginZip(archive, destination); + if (lock === "held") { + const failure = await result.catch((error: unknown) => error); + expect((failure as Error).message).toContain( + `Could not remove staging directory ${staging}:`, + ); + expect(failure).toMatchObject({ + message: expect.stringMatching(/publish.*ZIP.*rename/iu), + cause: { syscall: "rename", dest: destination }, + }); + expect(existsSync(staging)).toBe(true); + expect(existsSync(destination)).toBe(false); + } else { + expect(await result).toBe(destination); + expect(await readFile(join(destination, manifest), "utf8")).toBe( + contents, + ); + expect(existsSync(staging)).toBe(false); + } + expect(["EPERM", "EACCES", "EBUSY"]).toContain(blocked?.code ?? ""); + expect(publication.mock.calls.length).toBeGreaterThan(1); + } finally { + if (lock === "held") await release?.(); + } + }, + ); + + test.each(["empty", "nonempty"])( + "preserves platform behavior for an existing %s plugin ZIP destination", + async (kind) => { + const root = await temporaryDirectory(); + const archive = join(root, "plugin.zip"); + const destination = join(root, "extracted"); + await mkdir(destination); + const sentinel = join(destination, "previous"); + if (kind === "nonempty") await writeFile(sentinel, "keep"); + await writeFile( + archive, + zipSync({ + ".codex-plugin/plugin.json": strToU8( + JSON.stringify({ name: "codex-security", version: "1.2.3" }), + ), + }), + ); + const result = extractPluginZip(archive, destination); + if (process.platform === "win32" || kind === "nonempty") { + await expect(result).rejects.toThrow( + process.platform === "win32" + ? "Destination already exists" + : "Invalid plugin ZIP", + ); + if (kind === "nonempty") + expect(await readFile(sentinel, "utf8")).toBe("keep"); + else expect(await readdir(destination)).toEqual([]); + } else { + expect(await result).toBe(destination); + expect(await readdir(destination)).toEqual([".codex-plugin"]); + } + expect((await readdir(root)).sort()).toEqual(["extracted", "plugin.zip"]); + }, + ); + test("decodes flag-clear ZIP filenames with the legacy CP437 encoding", async () => { const root = await temporaryDirectory(); const archive = Buffer.from( diff --git a/sdk/typescript/tsconfig.build.json b/sdk/typescript/tsconfig.build.json index e36d09dd8d..ccd5ab1710 100644 --- a/sdk/typescript/tsconfig.build.json +++ b/sdk/typescript/tsconfig.build.json @@ -1,6 +1,6 @@ { "extends": "./tsconfig.json", - "include": ["src/**/*.ts", "src/**/*.tsx"], + "include": ["src/**/*.ts", "src/**/*.tsx", "src/retry-windows-rename.js"], "compilerOptions": { "rootDir": "src", "outDir": "dist",