diff --git a/sdk/typescript/_bundled_plugin/scripts/finalize_scan_contract.py b/sdk/typescript/_bundled_plugin/scripts/finalize_scan_contract.py index d99436db..9069b1ae 100644 --- a/sdk/typescript/_bundled_plugin/scripts/finalize_scan_contract.py +++ b/sdk/typescript/_bundled_plugin/scripts/finalize_scan_contract.py @@ -326,24 +326,56 @@ def _windows_scan_local_files() -> Any: return _WINDOWS_SCAN_LOCAL_FILES -def _open_verified_scan_directory(scan_dir: Path) -> int: +def _open_verified_scan_directory( + scan_dir: Path, expected_root_identity: tuple[int, int] | None = None +) -> int: scan_dir = scan_dir.absolute() try: expected = scan_dir.lstat() + observed_identity = (expected.st_dev, expected.st_ino) + if ( + expected_root_identity is not None + and observed_identity != expected_root_identity + ): + raise ContractError("scan directory: changed after artifact restoration setup") canonical = _require_scan_directory(scan_dir) descriptor = os.open( canonical, os.O_RDONLY | getattr(os, "O_DIRECTORY", 0) | getattr(os, "O_NOFOLLOW", 0), ) except OSError as exc: - raise ContractError("scan directory: expected an existing non-symlink directory") from exc + raise ContractError( + "scan directory: expected an existing non-symlink directory" + ) from exc opened = os.fstat(descriptor) - if (opened.st_dev, opened.st_ino) != (expected.st_dev, expected.st_ino): + opened_identity = (opened.st_dev, opened.st_ino) + if opened_identity != observed_identity or ( + expected_root_identity is not None + and opened_identity != expected_root_identity + ): os.close(descriptor) raise ContractError("scan directory: changed while it was being opened") return descriptor +def scan_root_identity(scan_dir: Path) -> tuple[Path, tuple[int, int]]: + """Return a canonical scan root and its identity from a held handle.""" + + scan_dir = _require_scan_directory(scan_dir) + if not _descriptor_relative_writes_available(): + if not _is_windows(): + raise ContractError( + "scan-local output requires descriptor-relative file operations" + ) + return _windows_scan_local_files().scan_root_identity(scan_dir) + descriptor = _open_verified_scan_directory(scan_dir) + try: + metadata = os.fstat(descriptor) + return scan_dir, (metadata.st_dev, metadata.st_ino) + finally: + os.close(descriptor) + + def _open_scan_local_directory(root_fd: int, parts: tuple[str, ...], *, create: bool) -> int: descriptor = os.dup(root_fd) try: @@ -492,7 +524,12 @@ def _sha256_scan_local_file(scan_dir: Path, relative_path: str, context: str) -> def write_scan_local_bytes( - scan_dir: Path, relative_path: str, payload: bytes, *, external_name: bool = False + scan_dir: Path, + relative_path: str, + payload: bytes, + *, + external_name: bool = False, + expected_root_identity: tuple[int, int] | None = None, ) -> None: scan_dir = _require_scan_directory(scan_dir) if external_name: @@ -505,7 +542,12 @@ def write_scan_local_bytes( if not _is_windows(): raise ContractError("scan-local output requires descriptor-relative file operations") try: - _windows_scan_local_files().atomic_write(scan_dir, relative_path, payload) + _windows_scan_local_files().atomic_write( + scan_dir, + relative_path, + payload, + expected_root_identity=expected_root_identity, + ) except OSError as exc: raise ContractError(f"{relative_path}: {exc}") from exc return @@ -513,7 +555,7 @@ def write_scan_local_bytes( parent_fd: int | None = None temp_name: str | None = None try: - root_fd = _open_verified_scan_directory(scan_dir) + root_fd = _open_verified_scan_directory(scan_dir, expected_root_identity) parts = PurePosixPath(relative_path).parts try: parent_fd = _open_scan_local_directory(root_fd, parts[:-1], create=True) @@ -521,6 +563,9 @@ def write_scan_local_bytes( raise ContractError( f"{relative_path}: expected a path inside the scan directory" ) from exc + # The held descriptor is the authority for the validated parent. A + # concurrent rename cannot redirect later operations through a + # replacement path or link. try: metadata = os.stat(parts[-1], dir_fd=parent_fd, follow_symlinks=False) except FileNotFoundError: @@ -528,6 +573,41 @@ def write_scan_local_bytes( else: if not stat.S_ISREG(metadata.st_mode): raise ContractError(f"{relative_path}: expected a regular non-symlink file") + try: + existing_fd = os.open( + parts[-1], + os.O_RDONLY + | getattr(os, "O_NOFOLLOW", 0) + | getattr(os, "O_NONBLOCK", 0), + dir_fd=parent_fd, + ) + except OSError as exc: + if exc.errno not in {errno.ENOENT, errno.EACCES, errno.EPERM}: + raise + else: + try: + opened = os.fstat(existing_fd) + if not stat.S_ISREG(opened.st_mode): + raise ContractError( + f"{relative_path}: expected a regular non-symlink file" + ) + if (opened.st_dev, opened.st_ino) != ( + metadata.st_dev, + metadata.st_ino, + ): + raise ContractError( + f"{relative_path}: changed while it was being opened" + ) + try: + with os.fdopen(existing_fd, "rb") as handle: + existing_fd = -1 + if handle.read() == payload: + return + except OSError: + pass + finally: + if existing_fd >= 0: + os.close(existing_fd) temp_name = f".{path.name}.{secrets.token_hex(8)}.tmp" temp_fd = os.open(temp_name, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600, dir_fd=parent_fd) with os.fdopen(temp_fd, "wb") as handle: diff --git a/sdk/typescript/_bundled_plugin/scripts/windows_scan_local_files.py b/sdk/typescript/_bundled_plugin/scripts/windows_scan_local_files.py index baef9508..d1a9b7b6 100644 --- a/sdk/typescript/_bundled_plugin/scripts/windows_scan_local_files.py +++ b/sdk/typescript/_bundled_plugin/scripts/windows_scan_local_files.py @@ -63,6 +63,9 @@ class WindowsScanLocalFileError(OSError): _FILE_NAME_OPENED = 0x00000008 _ERROR_FILE_NOT_FOUND = 2 _ERROR_PATH_NOT_FOUND = 3 +_ERROR_ACCESS_DENIED = 5 +_ERROR_SHARING_VIOLATION = 32 +_ERROR_LOCK_VIOLATION = 33 _ERROR_FILE_EXISTS = 80 _ERROR_ALREADY_EXISTS = 183 _MISSING_ERRORS = {_ERROR_FILE_NOT_FOUND, _ERROR_PATH_NOT_FOUND} @@ -398,12 +401,20 @@ def _locked_parent( relative_path: str, *, create: bool, + expected_root_identity: tuple[int, int] | None = None, ) -> Iterator[tuple[Path, str]]: """Hold non-deletable handles for every directory in the absolute target path.""" _require_windows() parts = _validated_parts(relative_path) - root_path, expected_root_identity = _canonical_scan_directory(scan_dir) + root_path, observed_root_identity = _canonical_scan_directory(scan_dir) + if ( + expected_root_identity is not None + and observed_root_identity != expected_root_identity + ): + raise _invalid_path( + scan_dir, "scan directory changed after artifact restoration setup" + ) handles: list[_OwnedHandle] = [] try: # Absolute-path Win32 calls remain safe only while every ancestor is @@ -414,8 +425,14 @@ def _locked_parent( assert directory_handle is not None handles.append(directory_handle) current_root = root_path.lstat() - if (current_root.st_dev, current_root.st_ino) != expected_root_identity: - raise _invalid_path(scan_dir, "scan directory changed while it was being opened") + current_root_identity = (current_root.st_dev, current_root.st_ino) + if current_root_identity != observed_root_identity or ( + expected_root_identity is not None + and current_root_identity != expected_root_identity + ): + raise _invalid_path( + scan_dir, "scan directory changed while it was being opened" + ) current_path = root_path for component in parts[:-1]: current_path /= component @@ -431,6 +448,14 @@ def _locked_parent( handle.close() +def scan_root_identity(scan_dir: Path) -> tuple[Path, tuple[int, int]]: + """Return a canonical scan root and its identity while holding it fixed.""" + + with _locked_parent(scan_dir, ".identity", create=False) as (root_path, _): + metadata = root_path.lstat() + return root_path, (metadata.st_dev, metadata.st_ino) + + def open_read_fd(scan_dir: Path, relative_path: str, context: str) -> int: """Open a verified regular file and return an owned binary read descriptor.""" @@ -532,12 +557,65 @@ def _validate_existing_output(path: Path) -> None: _verify_regular_file(handle.value, path) -def atomic_write(scan_dir: Path, relative_path: str, payload: bytes) -> None: +def _existing_output_matches(path: Path, payload: bytes) -> bool: + try: + handle = _create_file( + path, + access=_GENERIC_READ | _FILE_READ_ATTRIBUTES, + # Deny write and delete sharing while comparing the opened contents. + share=_FILE_SHARE_READ, + disposition=_OPEN_EXISTING, + flags=_FILE_FLAG_OPEN_REPARSE_POINT | _FILE_FLAG_BACKUP_SEMANTICS, + missing_ok=True, + ) + except WindowsScanLocalFileError as exc: + if exc.errno in { + _ERROR_ACCESS_DENIED, + _ERROR_SHARING_VIOLATION, + _ERROR_LOCK_VIOLATION, + }: + return False + raise + if handle is None: + return False + with handle: + assert handle.value is not None + _verify_regular_file(handle.value, path) + raw_handle = handle.detach() + try: + assert _msvcrt is not None + descriptor = _msvcrt.open_osfhandle( + raw_handle, os.O_RDONLY | os.O_BINARY + ) + except BaseException: + _close_handle(raw_handle) + raise + try: + with os.fdopen(descriptor, "rb") as stream: + return stream.read() == payload + except OSError: + return False + + +def atomic_write( + scan_dir: Path, + relative_path: str, + payload: bytes, + *, + expected_root_identity: tuple[int, int] | None = None, +) -> None: """Atomically replace a scan-local regular file with ``payload``.""" - with _locked_parent(scan_dir, relative_path, create=True) as (parent_path, leaf_name): + with _locked_parent( + scan_dir, + relative_path, + create=True, + expected_root_identity=expected_root_identity, + ) as (parent_path, leaf_name): destination_path = parent_path / leaf_name _validate_existing_output(destination_path) + if _existing_output_matches(destination_path, payload): + return temp_handle: _OwnedHandle | None = None temp_path: Path | None = None diff --git a/sdk/typescript/src/api.ts b/sdk/typescript/src/api.ts index dc58bb92..0b5f8d8e 100644 --- a/sdk/typescript/src/api.ts +++ b/sdk/typescript/src/api.ts @@ -6,7 +6,6 @@ import { mkdir, readFile, realpath, - rename, rm, writeFile, } from "node:fs/promises"; @@ -56,6 +55,7 @@ import { } from "./cost.js"; import { loadContract, + readScanFile, requireScanFile, type ScanExpectation, } from "./contract.js"; @@ -115,6 +115,7 @@ import { preserveCodexSecurityPluginRegistration, pluginExecutionEnvironment, planOutputArchive, + prepareScanArtifactRestorer, prepareOutputDir, preparePersistentOutputRoot, requireModelSafeOutputDir, @@ -127,6 +128,7 @@ import { type CodexCommand, type PluginInstall, type ProcessEnvironment, + type ScanArtifactRestorer, type WorkbenchCommandOptions, validateOutputDir, } from "./runtime.js"; @@ -334,6 +336,7 @@ interface ClientDependencies { ) => Promise; resolvePluginPython?: typeof resolvePluginPython; prepareOutputDir?: typeof prepareOutputDir; + prepareScanArtifactRestorer?: typeof prepareScanArtifactRestorer; repositoryRevision?: typeof repositoryRevision; resolveCodexCommand?: () => CodexCommand; runWorkbench?: typeof runWorkbench; @@ -491,6 +494,9 @@ export class CodexSecurity { id: string; options: WorkbenchCommandOptions; } | null = null; + const prepareArtifactRestorer = + this.#dependencies.prepareScanArtifactRestorer ?? + prepareScanArtifactRestorer; const workbench = this.#dependencies.runWorkbench ?? runWorkbench; try { const checkOpen = (): void => { @@ -1181,13 +1187,15 @@ export class CodexSecurity { ]), ].map(async (name) => ({ name, - contents: await readFile( - await requireScanFile(scanDir, name, name, signal), - { signal }, - ), + contents: await readScanFile(scanDir, name, name, signal), })), ); + let artifactRestorer: ScanArtifactRestorer | null = null; try { + artifactRestorer = await prepareArtifactRestorer( + workbenchOptions, + scanDir, + ); await runScanEvents({ thread, events: (await followUp()).events, @@ -1203,28 +1211,20 @@ export class CodexSecurity { checkOpen(); } catch (error) { if (signal.aborted || this.#closed) throw error; - for (const artifact of completedArtifacts) { - const path = join(scanDir, artifact.name); - const current = await readFile(path, { signal }).catch( - (readError: NodeJS.ErrnoException) => { - if (readError.code !== "ENOENT") throw readError; - return null; - }, - ); - if (current?.equals(artifact.contents)) continue; - const temporary = join( - dirname(path), - `.${randomUUID()}.${basename(path)}.restore`, - ); - try { - await writeFile(temporary, artifact.contents, { - flag: "wx", - mode: 0o600, - signal, - }); - await rename(temporary, path); - } finally { - await rm(temporary, { force: true }); + if (artifactRestorer !== null) { + for (const artifact of completedArtifacts) { + try { + await artifactRestorer.restore( + artifact.name, + artifact.contents, + ); + } catch (cause) { + if (signal.aborted || this.#closed) throw cause; + throw new OutputDirectoryError( + "Cannot restore an artifact outside the scan directory.", + { cause }, + ); + } } } await collectResult( diff --git a/sdk/typescript/src/contract.ts b/sdk/typescript/src/contract.ts index 655b2ba6..1ac43e9e 100644 --- a/sdk/typescript/src/contract.ts +++ b/sdk/typescript/src/contract.ts @@ -528,6 +528,25 @@ export async function requireScanFile( ).path; } +export async function readScanFile( + scanDirectory: string, + relativePath: string, + context: string, + signal?: AbortSignal, +): Promise { + const file = await openCheckedScanFile( + scanDirectory, + relativePath, + context, + signal, + ); + try { + return await file.readFile({ signal }); + } finally { + await file.close(); + } +} + async function requireCheckedScanFile( scanDirectory: string, relativePath: string, diff --git a/sdk/typescript/src/runtime.ts b/sdk/typescript/src/runtime.ts index 15063adf..c07a3095 100644 --- a/sdk/typescript/src/runtime.ts +++ b/sdk/typescript/src/runtime.ts @@ -78,6 +78,42 @@ const CREDENTIAL_LOCK_POLL_MILLISECONDS = 25; const INCOMPLETE_CREDENTIAL_LOCK_MILLISECONDS = 30_000; const MAX_PROCESS_ID = 2_147_483_647; const MAX_WINDOWS_CREDENTIAL_ACL_STDERR = 64 * 1024; +const PLUGIN_HELPER_SECRET_ENVIRONMENT_VARIABLES = new Set([ + "OPENAI_API_KEY", + "CODEX_API_KEY", + "OPENROUTER_API_KEY", + "FIREWORKS_API_KEY", +]); +const PREPARE_SCAN_ARTIFACT_RESTORER_PROGRAM = ` +from pathlib import Path +from runpy import run_path +import json +import sys + +module = run_path(sys.argv[1]) +canonical_path, root_identity = module["scan_root_identity"](Path(sys.argv[2])) +print(json.dumps({ + "canonicalPath": str(canonical_path), + "dev": str(root_identity[0]), + "ino": str(root_identity[1]), +}, ensure_ascii=False)) +`.trim(); +const RESTORE_SCAN_ARTIFACT_PROGRAM = ` +from pathlib import Path +from runpy import run_path +import sys + +module = run_path(sys.argv[1]) +try: + module["write_scan_local_bytes"]( + Path(sys.argv[2]), + sys.argv[3], + sys.stdin.buffer.read(), + expected_root_identity=(int(sys.argv[4]), int(sys.argv[5])), + ) +except (module["ContractError"], OSError) as error: + raise SystemExit(str(error)) +`.trim(); export interface PluginInstall { pluginRoot: string; @@ -118,6 +154,10 @@ export interface WorkbenchCommandOptions { failureMessage?: string; } +export interface ScanArtifactRestorer { + restore(relativePath: string, contents: Uint8Array): Promise; +} + function environmentValue( environment: ProcessEnvironment, requested: string, @@ -1375,15 +1415,6 @@ export async function runWorkbench( ): Promise { let stdout: string; try { - const environment = Object.fromEntries( - Object.entries(options.environment).filter( - ([name]) => - name.toUpperCase() !== "OPENAI_API_KEY" && - name.toUpperCase() !== "CODEX_API_KEY" && - name.toUpperCase() !== "OPENROUTER_API_KEY" && - name.toUpperCase() !== "FIREWORKS_API_KEY", - ), - ); const result = await runCodexCommand( { command: options.python }, [ @@ -1394,7 +1425,7 @@ export async function runWorkbench( join(options.pluginRoot, "scripts", "workbench_db.py"), ...args, ], - pythonUtf8Environment(environment), + pluginHelperEnvironment(options.environment), input, options.signal, ); @@ -1529,6 +1560,100 @@ export async function validateOutputDir( } } +export async function prepareScanArtifactRestorer( + options: WorkbenchCommandOptions, + scanDirectory: string, +): Promise { + let canonicalPath: string; + let dev: string; + let ino: string; + try { + const result = await runCodexCommand( + { command: options.python }, + [ + "-I", + "-X", + "utf8", + "-B", + "-c", + PREPARE_SCAN_ARTIFACT_RESTORER_PROGRAM, + join(options.pluginRoot, "scripts", "finalize_scan_contract.py"), + scanDirectory, + ], + pluginHelperEnvironment(options.environment), + undefined, + options.signal, + ); + if (!result.success) { + throw new Error( + result.stderr.trim() || + result.stdout.trim() || + `Artifact restoration setup exited with status ${result.exitCode}.`, + ); + } + const prepared: unknown = JSON.parse(result.stdout); + if ( + !isRecord(prepared) || + typeof prepared["canonicalPath"] !== "string" || + prepared["canonicalPath"].length === 0 || + typeof prepared["dev"] !== "string" || + !/^(?:0|[1-9]\d*)$/u.test(prepared["dev"]) || + typeof prepared["ino"] !== "string" || + !/^(?:0|[1-9]\d*)$/u.test(prepared["ino"]) + ) { + throw new Error("Artifact restoration setup returned invalid output."); + } + canonicalPath = prepared["canonicalPath"]; + dev = prepared["dev"]; + ino = prepared["ino"]; + } catch (error) { + if (options.signal?.aborted) throw error; + throw new OutputDirectoryError( + "Could not securely prepare completed scan artifact restoration.", + { cause: error }, + ); + } + + return { + async restore(relativePath, contents) { + try { + const result = await runCodexCommand( + { command: options.python }, + [ + "-I", + "-X", + "utf8", + "-B", + "-c", + RESTORE_SCAN_ARTIFACT_PROGRAM, + join(options.pluginRoot, "scripts", "finalize_scan_contract.py"), + canonicalPath, + relativePath, + dev, + ino, + ], + pluginHelperEnvironment(options.environment), + contents, + options.signal, + ); + if (!result.success) { + throw new Error( + result.stderr.trim() || + result.stdout.trim() || + `Artifact restoration exited with status ${result.exitCode}.`, + ); + } + } catch (error) { + if (options.signal?.aborted) throw error; + throw new OutputDirectoryError( + "Could not safely restore a completed scan artifact.", + { cause: error }, + ); + } + }, + }; +} + export async function planOutputArchive( outputDirectory: string | null, ): Promise { @@ -2360,6 +2485,19 @@ export function pythonUtf8Environment( return normalized; } +function pluginHelperEnvironment( + environment: ProcessEnvironment, +): ProcessEnvironment { + return pythonUtf8Environment( + Object.fromEntries( + Object.entries(environment).filter( + ([name]) => + !PLUGIN_HELPER_SECRET_ENVIRONMENT_VARIABLES.has(name.toUpperCase()), + ), + ), + ); +} + export async function cleanupSdkDirectory(path: string): Promise { await rm(path, { recursive: true, force: true }); } @@ -2368,7 +2506,7 @@ export async function runCodexCommand( command: CodexCommand, args: readonly string[], environment: ProcessEnvironment, - input?: string, + input?: string | Uint8Array, signal?: AbortSignal, ): Promise { const child = spawn(command.command, [...args], { diff --git a/sdk/typescript/tests-ts/api-post-scan.test.ts b/sdk/typescript/tests-ts/api-post-scan.test.ts index 1c17fe13..ae6bbff3 100644 --- a/sdk/typescript/tests-ts/api-post-scan.test.ts +++ b/sdk/typescript/tests-ts/api-post-scan.test.ts @@ -1,8 +1,22 @@ import { createHash } from "node:crypto"; -import { mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { + chmod, + mkdir, + readFile, + rename, + rm, + stat, + symlink, + writeFile, +} from "node:fs/promises"; import { dirname, join } from "node:path"; import type { ThreadEvent } from "@openai/codex-sdk"; import { afterEach, describe, expect, test } from "bun:test"; +import { + prepareScanArtifactRestorer, + type ScanArtifactRestorer, +} from "../src/runtime.js"; +import { PLUGIN_ROOT } from "./plugin-root.js"; import { TestClient } from "./support/api-client.js"; import { completedEvents, @@ -15,81 +29,363 @@ const { cleanup, copyCompletedScan, temporaryDirectory } = afterEach(cleanup); +interface FailedPostScanContext { + artifactPath: string; + outside: string; + scanDir: string; +} + +interface FailedPostScanScenario { + artifact: string; + initialContents?: string | Uint8Array; + mutate(context: FailedPostScanContext): Promise; + wrapRestorer?( + restorer: ScanArtifactRestorer, + context: FailedPostScanContext, + ): ScanArtifactRestorer; +} + +async function* failedEvents(): AsyncGenerator { + yield { + type: "turn.failed", + error: { message: "Could not draft fixes." }, + }; +} + +async function startFailedPostScan(scenario: FailedPostScanScenario) { + const root = await temporaryDirectory(); + const repository = join(root, "repository"); + const codexHome = join(root, "codex-home"); + const scanDir = join(root, "scan"); + const outside = join(root, "outside"); + const artifactPath = join(scanDir, scenario.artifact); + const context = { artifactPath, outside, scanDir }; + const python = Bun.which("python3") ?? Bun.which("python"); + expect(python).not.toBeNull(); + await mkdir(repository); + await mkdir(codexHome); + await mkdir(scanDir, { mode: 0o700 }); + let turns = 0; + let original = Buffer.alloc(0); + const client = new TestClient( + {}, + { + environment: {}, + prepareRuntime: async () => preparedRuntime(codexHome), + resolvePluginPython: async () => python!, + prepareOutputDir: async () => scanDir, + repositoryRevision: async () => "deadbeef", + prepareScanArtifactRestorer: async (...args) => { + const restorer = await prepareScanArtifactRestorer(...args); + return scenario.wrapRestorer?.(restorer, context) ?? restorer; + }, + createCodex: () => ({ + startThread: () => ({ + id: "thread-1", + async runStreamed() { + turns += 1; + if (turns === 1) { + await copyCompletedScan(root); + if (scenario.initialContents !== undefined) { + await mkdir(dirname(artifactPath), { recursive: true }); + await writeFile(artifactPath, scenario.initialContents); + const manifestPath = join(scanDir, "scan-manifest.json"); + const manifest = JSON.parse( + await readFile(manifestPath, "utf8"), + ); + manifest.scan.artifacts.push({ + path: scenario.artifact, + sha256: createHash("sha256") + .update(await readFile(artifactPath)) + .digest("hex"), + mediaType: scenario.artifact.endsWith(".bin") + ? "application/octet-stream" + : "application/json", + }); + await writeFile(manifestPath, JSON.stringify(manifest)); + } + original = await readFile(artifactPath); + return { events: completedEvents() }; + } + await scenario.mutate(context); + return { events: failedEvents() }; + }, + }), + }), + }, + ); + const scan = client.run(repository, { + postScanPrompt: "Draft confirmed fixes.", + }); + return { + client, + scan, + scanDir, + artifactPath, + outside, + get original() { + return original; + }, + }; +} + +const ordinaryRestorationCases: ReadonlyArray< + readonly [string, FailedPostScanScenario] +> = [ + [ + "missing report", + { artifact: "report.md", mutate: ({ artifactPath }) => rm(artifactPath) }, + ], + [ + "partial report", + { + artifact: "report.md", + mutate: async ({ artifactPath }) => { + await writeFile(artifactPath, "# Incomplete draft\n"); + }, + }, + ], + [ + "replaced report", + { + artifact: "report.md", + mutate: async ({ artifactPath }) => { + await rm(artifactPath); + await writeFile(artifactPath, "# Replacement\n"); + }, + }, + ], + [ + "invalid findings", + { + artifact: "findings.json", + mutate: async ({ artifactPath }) => { + await writeFile(artifactPath, "{invalid"); + }, + }, + ], + [ + "sealed nested artifact", + { + artifact: "artifacts/worker.json", + initialContents: '{"complete":true}\n', + mutate: async ({ artifactPath }) => { + await writeFile(artifactPath, '{"partial":true}'); + }, + }, + ], + [ + "binary artifact", + { + artifact: "artifacts/worker.bin", + initialContents: Buffer.from([0, 255, 10, 1]), + mutate: async ({ artifactPath }) => { + await writeFile(artifactPath, Buffer.from([9, 0, 8])); + }, + }, + ], +]; + describe("completed scan follow-up instructions", () => { - test.each([ - ["missing report", "report.md", undefined], - ["partial report", "report.md", "# Incomplete draft\n"], - ["invalid findings", "findings.json", "{invalid"], - ["sealed nested artifact", "artifacts/worker.json", '{"partial":true}'], - ] as const)( - "restores completed scan artifacts damaged by post-scan instructions: %s", - async (_scenario, artifact, replacement) => { + test.each(ordinaryRestorationCases)( + "restores completed scan artifacts after failed post-scan instructions: %s", + async (_name, scenario) => { + const fixture = await startFailedPostScan(scenario); + expect(await fixture.scan).toMatchObject({ scanDir: fixture.scanDir }); + expect(await readFile(fixture.artifactPath)).toEqual(fixture.original); + await fixture.client.close(); + }, + ); + + test("restores a nested artifact and its missing parent", async () => { + const fixture = await startFailedPostScan({ + artifact: "artifacts/worker.json", + initialContents: '{"complete":true}\n', + mutate: async ({ artifactPath }) => { + await rm(dirname(artifactPath), { recursive: true }); + }, + }); + + expect(await fixture.scan).toMatchObject({ scanDir: fixture.scanDir }); + expect(await readFile(fixture.artifactPath)).toEqual(fixture.original); + await fixture.client.close(); + }); + + test("does not rewrite artifacts unchanged by a failed follow-up", async () => { + let before: { dev: number; ino: number; mtimeMs: number } | null = null; + const fixture = await startFailedPostScan({ + artifact: "report.md", + mutate: async ({ artifactPath }) => { + const metadata = await stat(artifactPath); + before = { + dev: Number(metadata.dev), + ino: Number(metadata.ino), + mtimeMs: Number(metadata.mtimeMs), + }; + }, + }); + + expect(await fixture.scan).toMatchObject({ scanDir: fixture.scanDir }); + const after = await stat(fixture.artifactPath); + expect(before).not.toBeNull(); + expect(Number(after.dev)).toBe(before!.dev); + expect(Number(after.ino)).toBe(before!.ino); + expect(Number(after.mtimeMs)).toBe(before!.mtimeMs); + await fixture.client.close(); + }); + + test.skipIf(process.platform === "win32")( + "restores a changed artifact that cannot be read for comparison", + async () => { + const fixture = await startFailedPostScan({ + artifact: "report.md", + mutate: async ({ artifactPath }) => { + await writeFile(artifactPath, "# Incomplete draft\n"); + await chmod(artifactPath, 0); + }, + }); + + expect(await fixture.scan).toMatchObject({ scanDir: fixture.scanDir }); + expect(await readFile(fixture.artifactPath)).toEqual(fixture.original); + await fixture.client.close(); + }, + ); + + test("rejects a replaced artifact parent without writing through it", async () => { + const fixture = await startFailedPostScan({ + artifact: "artifacts/worker.json", + initialContents: '{"complete":true}\n', + mutate: async ({ artifactPath, outside }) => { + await mkdir(outside); + await writeFile(join(outside, "worker.json"), "untouched\n"); + await rm(dirname(artifactPath), { recursive: true }); + await symlink( + outside, + dirname(artifactPath), + process.platform === "win32" ? "junction" : "dir", + ); + }, + }); + + await expect(fixture.scan).rejects.toThrow("scan directory"); + expect(await readFile(join(fixture.outside, "worker.json"), "utf8")).toBe( + "untouched\n", + ); + await fixture.client.close(); + }); + + test("rejects an artifact parent swapped immediately before the bound write", async () => { + let swapped = false; + const artifact = "artifacts/worker.json"; + const fixture = await startFailedPostScan({ + artifact, + initialContents: '{"complete":true}\n', + mutate: async ({ artifactPath, outside }) => { + await mkdir(outside); + await writeFile(join(outside, "worker.json"), "untouched\n"); + await writeFile(artifactPath, '{"partial":true}'); + }, + wrapRestorer: (restorer, { outside, scanDir }) => ({ + ...restorer, + async restore(relativePath, contents) { + if (!swapped && relativePath === artifact) { + const parent = dirname(join(scanDir, relativePath)); + await rename(parent, `${parent}.original`); + await symlink( + outside, + parent, + process.platform === "win32" ? "junction" : "dir", + ); + swapped = true; + } + await restorer.restore(relativePath, contents); + }, + }), + }); + + await expect(fixture.scan).rejects.toThrow("scan directory"); + expect(await readFile(join(fixture.outside, "worker.json"), "utf8")).toBe( + "untouched\n", + ); + await fixture.client.close(); + }); + + test("rejects a scan root replaced after restoration setup", async () => { + const fixture = await startFailedPostScan({ + artifact: "report.md", + mutate: async ({ scanDir }) => { + await rename(scanDir, `${scanDir}.original`); + await mkdir(scanDir, { mode: 0o700 }); + await writeFile(join(scanDir, "scan-manifest.json"), "untouched\n"); + }, + }); + + await expect(fixture.scan).rejects.toThrow("scan directory"); + expect( + await readFile(join(fixture.scanDir, "scan-manifest.json"), "utf8"), + ).toBe("untouched\n"); + await fixture.client.close(); + }); + + test.skipIf(process.platform === "win32")( + "keeps the final rename bound to the validated parent when its path is replaced", + async () => { const root = await temporaryDirectory(); - const repository = join(root, "repository"); - const codexHome = join(root, "codex-home"); const scanDir = join(root, "scan"); - await mkdir(repository); - await mkdir(codexHome); + const parent = join(scanDir, "artifacts"); + const movedParent = join(root, "moved-artifacts"); + const outside = join(root, "outside"); + const python = Bun.which("python3") ?? Bun.which("python"); + expect(python).not.toBeNull(); await mkdir(scanDir, { mode: 0o700 }); - let turns = 0; - let original = Buffer.alloc(0); - const client = new TestClient( - {}, - { - environment: {}, - prepareRuntime: async () => preparedRuntime(codexHome), - resolvePluginPython: async () => "/managed/python", - prepareOutputDir: async () => scanDir, - repositoryRevision: async () => "deadbeef", - createCodex: () => ({ - startThread: () => ({ - id: "thread-1", - async runStreamed() { - turns += 1; - if (turns === 1) { - await copyCompletedScan(root); - if (artifact.startsWith("artifacts/")) { - const artifactPath = join(scanDir, artifact); - await mkdir(dirname(artifactPath), { recursive: true }); - await writeFile(artifactPath, '{"complete":true}\n'); - const manifestPath = join(scanDir, "scan-manifest.json"); - const manifest = JSON.parse( - await readFile(manifestPath, "utf8"), - ); - manifest.scan.artifacts.push({ - path: artifact, - sha256: createHash("sha256") - .update(await readFile(artifactPath)) - .digest("hex"), - mediaType: "application/json", - }); - await writeFile(manifestPath, JSON.stringify(manifest)); - } - original = await readFile(join(scanDir, artifact)); - return { events: completedEvents() }; - } - const artifactPath = join(scanDir, artifact); - if (replacement === undefined) await rm(artifactPath); - else await writeFile(artifactPath, replacement); - async function* failedEvents(): AsyncGenerator { - yield { - type: "turn.failed", - error: { message: "Could not draft fixes." }, - }; - } - return { events: failedEvents() }; - }, - }), - }), - }, - ); + await mkdir(parent); + await mkdir(outside); + await writeFile(join(parent, "worker.bin"), Buffer.from([1])); + await writeFile(join(outside, "worker.bin"), "untouched\n"); + const script = [ + "from pathlib import Path", + "from runpy import run_path", + "import sys", + "module = run_path(sys.argv[1])", + "scan_dir = Path(sys.argv[2])", + "parent = scan_dir / 'artifacts'", + "moved_parent = Path(sys.argv[4])", + "outside = Path(sys.argv[3])", + "canonical, identity = module['scan_root_identity'](scan_dir)", + "original_replace = module['os'].replace", + "swapped = False", + "def replace(source, destination, *, src_dir_fd=None, dst_dir_fd=None):", + " global swapped", + " if not swapped:", + " parent.rename(moved_parent)", + " parent.symlink_to(outside, target_is_directory=True)", + " swapped = True", + " return original_replace(source, destination, src_dir_fd=src_dir_fd, dst_dir_fd=dst_dir_fd)", + "module['os'].replace = replace", + "module['write_scan_local_bytes'](canonical, 'artifacts/worker.bin', bytes([0, 255, 10, 1]), expected_root_identity=identity)", + ].join("\n"); + const execution = Bun.spawnSync([ + python!, + "-I", + "-B", + "-c", + script, + join(PLUGIN_ROOT, "scripts", "finalize_scan_contract.py"), + scanDir, + outside, + movedParent, + ]); - const result = await client.run(repository, { - postScanPrompt: "Draft confirmed fixes.", - }); - expect(result).toMatchObject({ scanDir }); - expect(await readFile(join(scanDir, artifact))).toEqual(original); - await client.close(); + expect( + execution.exitCode, + new TextDecoder().decode(execution.stderr), + ).toBe(0); + expect(await readFile(join(outside, "worker.bin"), "utf8")).toBe( + "untouched\n", + ); + expect(await readFile(join(movedParent, "worker.bin"))).toEqual( + Buffer.from([0, 255, 10, 1]), + ); }, ); }); diff --git a/sdk/typescript/tests-ts/api.test.ts b/sdk/typescript/tests-ts/api.test.ts index 35e85126..c288bbd0 100644 --- a/sdk/typescript/tests-ts/api.test.ts +++ b/sdk/typescript/tests-ts/api.test.ts @@ -3330,75 +3330,92 @@ describe("CodexSecurity orchestration", () => { await client.close(); }); - test("warns about post-scan failures without failing a completed scan", async () => { - const root = await temporaryDirectory(); - const repository = join(root, "repository"); - const codexHome = join(root, "codex-home"); - const scanDir = join(root, "scan"); - await mkdir(repository); - await mkdir(codexHome); - await mkdir(scanDir, { mode: 0o700 }); - const commands: Array = []; - const warnings: string[] = []; - let turns = 0; + test.each([ + ["the follow-up turn", false, "Could not draft fixes."], + ["artifact restoration setup", true, "restoration setup failed"], + ] as const)( + "warns when %s fails without failing a completed scan", + async (_scenario, setupFails, failureMessage) => { + const root = await temporaryDirectory(); + const repository = join(root, "repository"); + const codexHome = join(root, "codex-home"); + const scanDir = join(root, "scan"); + await mkdir(repository); + await mkdir(codexHome); + await mkdir(scanDir, { mode: 0o700 }); + const commands: Array = []; + const warnings: string[] = []; + let turns = 0; - const client = new TestClient( - {}, - { - environment: {}, - prepareRuntime: async () => preparedRuntime(codexHome), - resolvePluginPython: async () => "/managed/python", - prepareOutputDir: async () => scanDir, - repositoryRevision: async () => "deadbeef", - runWorkbench: async ( - _options: unknown, - args: readonly string[], - input?: string, - ): Promise => { - commands.push(args); - return mockWorkbench(args, input); - }, - createCodex: () => ({ - startThread: () => ({ - id: "thread-1", - async runStreamed() { - turns += 1; - if (turns === 1) { - await copyCompletedScan(root); - return { events: completedEvents() }; - } - async function* failedEvents(): AsyncGenerator { - yield { - type: "turn.failed", - error: { message: "Could not draft fixes." }, - }; + const client = new TestClient( + {}, + { + environment: {}, + prepareRuntime: async () => preparedRuntime(codexHome), + resolvePluginPython: async () => "/managed/python", + prepareOutputDir: async () => scanDir, + repositoryRevision: async () => "deadbeef", + ...(setupFails + ? { + prepareScanArtifactRestorer: async () => { + throw new Error(failureMessage); + }, } - return { events: failedEvents() }; - }, + : {}), + runWorkbench: async ( + _options: unknown, + args: readonly string[], + input?: string, + ): Promise => { + commands.push(args); + return mockWorkbench(args, input); + }, + createCodex: () => ({ + startThread: () => ({ + id: "thread-1", + async runStreamed() { + turns += 1; + if (turns === 1) { + await copyCompletedScan(root); + return { events: completedEvents() }; + } + if (setupFails) { + throw new Error("post-scan turn started after setup failed"); + } + async function* failedEvents(): AsyncGenerator { + yield { + type: "turn.failed", + error: { message: "Could not draft fixes." }, + }; + } + return { events: failedEvents() }; + }, + }), }), - }), - }, - ); + }, + ); - await expect( - client.run(repository, { - postScanPrompt: "Draft confirmed fixes.", - onWarning: (warning) => warnings.push(warning), - }), - ).resolves.toMatchObject({ scanDir }); - expect(warnings).toEqual([ - "Could not run post-scan instructions: Could not draft fixes.", - ]); - expect(commands.map((command) => command[0])).toEqual([ - "register-cli-scan", - "get-scan-feedback", - "set-scan-thread", - "prepare-scan-completion", - "complete-scan", - "list-global-findings", - ]); - await client.close(); - }); + await expect( + client.run(repository, { + postScanPrompt: "Draft confirmed fixes.", + onWarning: (warning) => warnings.push(warning), + }), + ).resolves.toMatchObject({ scanDir }); + expect(warnings).toEqual([ + `Could not run post-scan instructions: ${failureMessage}`, + ]); + expect(turns).toBe(setupFails ? 1 : 2); + expect(commands.map((command) => command[0])).toEqual([ + "register-cli-scan", + "get-scan-feedback", + "set-scan-thread", + "prepare-scan-completion", + "complete-scan", + "list-global-findings", + ]); + await client.close(); + }, + ); test.each([ ["partial coverage", "partial", false], diff --git a/sdk/typescript/tests-ts/support/api-client.ts b/sdk/typescript/tests-ts/support/api-client.ts index 851a440c..2195e73a 100644 --- a/sdk/typescript/tests-ts/support/api-client.ts +++ b/sdk/typescript/tests-ts/support/api-client.ts @@ -68,6 +68,9 @@ export class TestClient extends CodexSecurity { throw new Error("Unexpected Codex invocation in test"); }, environment: {}, + prepareScanArtifactRestorer: async () => ({ + restore: async () => {}, + }), runWorkbench: async (_options, args, input) => mockWorkbench(args, input), ...dependencies,