From ba42078092fd59ec8f4ea76167e4018e45bbe86e Mon Sep 17 00:00:00 2001 From: faizan-oai <269039902+faizan-oai@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:34:24 -0700 Subject: [PATCH 1/2] fix(windows): honor dedupe authentication and sandbox settings --- .../src/deduplication/codex-review.ts | 12 ++- sdk/typescript/src/scan-comparison.ts | 2 +- sdk/typescript/tests-ts/codex-review.test.ts | 100 +++++++++++++++--- 3 files changed, 93 insertions(+), 21 deletions(-) diff --git a/sdk/typescript/src/deduplication/codex-review.ts b/sdk/typescript/src/deduplication/codex-review.ts index e8e01eddb..2b755bdbd 100644 --- a/sdk/typescript/src/deduplication/codex-review.ts +++ b/sdk/typescript/src/deduplication/codex-review.ts @@ -10,6 +10,7 @@ import { createInterface } from "node:readline"; import { comparisonEnvironment, disabledMcpServers, + environmentEntry, } from "../scan-comparison.js"; import { codexSecurityCredentialHome, @@ -82,7 +83,8 @@ export class CodexReviewRunner { { workingDirectory: this.workingDirectory, signal: this.signal }, ); const apiKey = - environment["OPENAI_API_KEY"] ?? environment["CODEX_API_KEY"]; + environmentEntry(environment, "OPENAI_API_KEY") ?? + environmentEntry(environment, "CODEX_API_KEY"); const args = ["app-server", "--stdio", "--disable", "plugins"]; const stateDatabase = join( codexSecurityStateDirectory(environment), @@ -90,10 +92,12 @@ export class CodexReviewRunner { ); const privatePaths = new Set( [ - environment["CODEX_HOME"] ?? join(homedir(), ".codex"), + environmentEntry(environment, "CODEX_HOME") ?? + join(homedir(), ".codex"), codexSecurityCredentialHome(environment), join(homedir(), ".ssh"), - environment["GH_CONFIG_DIR"] ?? join(homedir(), ".config", "gh"), + environmentEntry(environment, "GH_CONFIG_DIR") ?? + join(homedir(), ".config", "gh"), stateDatabase, `${stateDatabase}-wal`, `${stateDatabase}-shm`, @@ -107,8 +111,6 @@ export class CodexReviewRunner { `permissions.codex_security_review={extends=":read-only",filesystem={${[...privatePaths].map((path) => `${JSON.stringify(path)}="deny"`).join(",")}}}`, "--config", `sqlite_home=${JSON.stringify(directory)}`, - "--config", - 'windows.sandbox="unelevated"', ); if (apiKey) args.push("--config", 'cli_auth_credentials_store="ephemeral"'); diff --git a/sdk/typescript/src/scan-comparison.ts b/sdk/typescript/src/scan-comparison.ts index baa0f7e19..5eb86725c 100644 --- a/sdk/typescript/src/scan-comparison.ts +++ b/sdk/typescript/src/scan-comparison.ts @@ -459,7 +459,7 @@ export async function comparisonEnvironment( return environment; } -function environmentEntry( +export function environmentEntry( environment: Record, requested: string, ): string | undefined { diff --git a/sdk/typescript/tests-ts/codex-review.test.ts b/sdk/typescript/tests-ts/codex-review.test.ts index e56d6bb02..b9d5123ec 100644 --- a/sdk/typescript/tests-ts/codex-review.test.ts +++ b/sdk/typescript/tests-ts/codex-review.test.ts @@ -2,43 +2,79 @@ import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process"; import { existsSync } from "node:fs"; import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import { expect, test } from "bun:test"; import { CodexReviewRunner } from "../src/deduplication/codex-review.js"; +import { environmentEntry } from "../src/scan-comparison.js"; const fixture = fileURLToPath( new URL("fixtures/codex-review.mjs", import.meta.url), ); -for (const scenario of [ - "correction", - "text-only", - "failed-turn", - "exit", - "cancel", -]) { - test(`Codex review transport: ${scenario}`, async () => { +const transportCases: { + scenario: string; + name?: string; + environmentNames?: readonly [string, string, string]; + windowsOnly?: boolean; + windowsSandbox?: "elevated"; +}[] = [ + ...["correction", "text-only", "failed-turn", "exit", "cancel"].map( + (scenario) => ({ scenario }), + ), + { + scenario: "correction", + name: "lowercase Windows environment", + environmentNames: ["codex_home", "openai_api_key", "gh_config_dir"], + windowsOnly: true, + }, + { + scenario: "correction", + name: "mixed-case Windows environment", + environmentNames: ["Codex_Home", "Codex_Api_Key", "Gh_Config_Dir"], + windowsOnly: true, + }, + { + scenario: "correction", + name: "configured Windows sandbox", + windowsSandbox: "elevated", + }, +]; + +for (const { + scenario, + name = scenario, + environmentNames = ["CODEX_HOME", "OPENAI_API_KEY", "GH_CONFIG_DIR"], + windowsOnly = false, + windowsSandbox, +} of transportCases) { + const runCase = test.skipIf(windowsOnly && process.platform !== "win32"); + runCase(`Codex review transport: ${name}`, async () => { const modelHome = await mkdtemp(join(tmpdir(), "codex-review-test-")); const checkout = await mkdtemp(join(tmpdir(), "codex-review-source-")); + const ghConfig = await mkdtemp(join(tmpdir(), "codex-review-gh-")); const transcript = join(modelHome, "messages.jsonl"); let child: ChildProcessWithoutNullStreams | undefined; let directory: string | undefined; let args: readonly string[] = []; const controller = new AbortController(); try { - await writeFile( - join(modelHome, "config.toml"), - '[mcp_servers.synthetic]\ncommand = "synthetic-unused-command"\n', - ); + const configuration = + (windowsSandbox === undefined + ? "" + : `[windows]\nsandbox = "${windowsSandbox}"\n\n`) + + '[mcp_servers.synthetic]\ncommand = "synthetic-unused-command"\n'; + await writeFile(join(modelHome, "config.toml"), configuration); + const [homeName, keyName, ghName] = environmentNames; const runner = new CodexReviewRunner( { PATH: process.env["PATH"], SystemRoot: process.env["SystemRoot"], TEMP: process.env["TEMP"], TMP: process.env["TMP"], - CODEX_HOME: modelHome, - OPENAI_API_KEY: "synthetic-review-key", + [homeName]: modelHome, + [keyName]: "synthetic-review-key", + [ghName]: ghConfig, }, (_command, commandArgs, options) => { args = commandArgs; @@ -95,6 +131,23 @@ for (const scenario of [ } expect(args).toContain('cli_auth_credentials_store="ephemeral"'); expect(args.join(" ")).not.toContain("synthetic-review-key"); + const permissions = args.find((argument) => + argument.startsWith("permissions.codex_security_review="), + ); + expect(permissions).toContain( + `${JSON.stringify(resolve(modelHome))}="deny"`, + ); + expect(permissions).toContain( + `${JSON.stringify(resolve(ghConfig))}="deny"`, + ); + if (windowsSandbox !== undefined) { + expect( + args.some((argument) => /^windows\.sandbox\s*=/u.test(argument)), + ).toBe(false); + expect(await readFile(join(modelHome, "config.toml"), "utf8")).toBe( + configuration, + ); + } if (scenario !== "cancel") expect(await readFile(transcript, "utf8")).toContain( '"method":"account/login/start"', @@ -106,6 +159,23 @@ for (const scenario of [ } finally { await rm(modelHome, { recursive: true, force: true }); await rm(checkout, { recursive: true, force: true }); + await rm(ghConfig, { recursive: true, force: true }); } }); } + +test("environment lookups preserve platform case rules and exact-key precedence", () => { + const aliases = { codex_home: "synthetic-alias" }; + expect(environmentEntry(aliases, "CODEX_HOME")).toBe( + process.platform === "win32" ? "synthetic-alias" : undefined, + ); + expect( + environmentEntry( + { ...aliases, CODEX_HOME: "synthetic-exact" }, + "CODEX_HOME", + ), + ).toBe("synthetic-exact"); + expect(environmentEntry({ ...aliases, CODEX_HOME: "" }, "CODEX_HOME")).toBe( + "", + ); +}); From 37efa95e6af2e3b445f94c6a70e21f2c07a2b84c Mon Sep 17 00:00:00 2001 From: faizan-oai <269039902+faizan-oai@users.noreply.github.com> Date: Thu, 27 Aug 2026 18:19:13 -0700 Subject: [PATCH 2/2] fix(dedupe): handle empty environment values --- .../src/deduplication/codex-review.ts | 13 +- sdk/typescript/tests-ts/codex-review.test.ts | 120 +++++++++++++++--- 2 files changed, 108 insertions(+), 25 deletions(-) diff --git a/sdk/typescript/src/deduplication/codex-review.ts b/sdk/typescript/src/deduplication/codex-review.ts index 2b755bdbd..00cd0ed67 100644 --- a/sdk/typescript/src/deduplication/codex-review.ts +++ b/sdk/typescript/src/deduplication/codex-review.ts @@ -82,9 +82,10 @@ export class CodexReviewRunner { environment, { workingDirectory: this.workingDirectory, signal: this.signal }, ); - const apiKey = - environmentEntry(environment, "OPENAI_API_KEY") ?? - environmentEntry(environment, "CODEX_API_KEY"); + const apiKey = [ + environmentEntry(environment, "OPENAI_API_KEY"), + environmentEntry(environment, "CODEX_API_KEY"), + ].find((value) => value?.trim()); const args = ["app-server", "--stdio", "--disable", "plugins"]; const stateDatabase = join( codexSecurityStateDirectory(environment), @@ -92,11 +93,11 @@ export class CodexReviewRunner { ); const privatePaths = new Set( [ - environmentEntry(environment, "CODEX_HOME") ?? + environmentEntry(environment, "CODEX_HOME") || join(homedir(), ".codex"), codexSecurityCredentialHome(environment), join(homedir(), ".ssh"), - environmentEntry(environment, "GH_CONFIG_DIR") ?? + environmentEntry(environment, "GH_CONFIG_DIR") || join(homedir(), ".config", "gh"), stateDatabase, `${stateDatabase}-wal`, @@ -111,6 +112,8 @@ export class CodexReviewRunner { `permissions.codex_security_review={extends=":read-only",filesystem={${[...privatePaths].map((path) => `${JSON.stringify(path)}="deny"`).join(",")}}}`, "--config", `sqlite_home=${JSON.stringify(directory)}`, + "--config", + 'windows.sandbox="unelevated"', ); if (apiKey) args.push("--config", 'cli_auth_credentials_store="ephemeral"'); diff --git a/sdk/typescript/tests-ts/codex-review.test.ts b/sdk/typescript/tests-ts/codex-review.test.ts index b9d5123ec..4c09ea556 100644 --- a/sdk/typescript/tests-ts/codex-review.test.ts +++ b/sdk/typescript/tests-ts/codex-review.test.ts @@ -1,12 +1,13 @@ import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process"; import { existsSync } from "node:fs"; import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import { homedir, tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; -import { expect, test } from "bun:test"; +import { expect, mock, test } from "bun:test"; import { CodexReviewRunner } from "../src/deduplication/codex-review.js"; import { environmentEntry } from "../src/scan-comparison.js"; +import { runTestInSubprocess } from "./support/test-subprocess.js"; const fixture = fileURLToPath( new URL("fixtures/codex-review.mjs", import.meta.url), @@ -16,8 +17,8 @@ const transportCases: { scenario: string; name?: string; environmentNames?: readonly [string, string, string]; + extraEnvironment?: Record; windowsOnly?: boolean; - windowsSandbox?: "elevated"; }[] = [ ...["correction", "text-only", "failed-turn", "exit", "cancel"].map( (scenario) => ({ scenario }), @@ -34,10 +35,18 @@ const transportCases: { environmentNames: ["Codex_Home", "Codex_Api_Key", "Gh_Config_Dir"], windowsOnly: true, }, + ...["", " \t"].map((value) => ({ + scenario: "correction", + name: `${value === "" ? "empty" : "blank"} OpenAI key uses Codex key`, + environmentNames: ["CODEX_HOME", "CODEX_API_KEY", "GH_CONFIG_DIR"] as const, + extraEnvironment: { OPENAI_API_KEY: value }, + })), { scenario: "correction", - name: "configured Windows sandbox", - windowsSandbox: "elevated", + name: "empty Windows OpenAI alias uses Codex key", + environmentNames: ["Codex_Home", "Codex_Api_Key", "Gh_Config_Dir"], + extraEnvironment: { openai_api_key: "" }, + windowsOnly: true, }, ]; @@ -45,8 +54,8 @@ for (const { scenario, name = scenario, environmentNames = ["CODEX_HOME", "OPENAI_API_KEY", "GH_CONFIG_DIR"], + extraEnvironment, windowsOnly = false, - windowsSandbox, } of transportCases) { const runCase = test.skipIf(windowsOnly && process.platform !== "win32"); runCase(`Codex review transport: ${name}`, async () => { @@ -60,9 +69,6 @@ for (const { const controller = new AbortController(); try { const configuration = - (windowsSandbox === undefined - ? "" - : `[windows]\nsandbox = "${windowsSandbox}"\n\n`) + '[mcp_servers.synthetic]\ncommand = "synthetic-unused-command"\n'; await writeFile(join(modelHome, "config.toml"), configuration); const [homeName, keyName, ghName] = environmentNames; @@ -75,6 +81,7 @@ for (const { [homeName]: modelHome, [keyName]: "synthetic-review-key", [ghName]: ghConfig, + ...extraEnvironment, }, (_command, commandArgs, options) => { args = commandArgs; @@ -140,18 +147,20 @@ for (const { expect(permissions).toContain( `${JSON.stringify(resolve(ghConfig))}="deny"`, ); - if (windowsSandbox !== undefined) { - expect( - args.some((argument) => /^windows\.sandbox\s*=/u.test(argument)), - ).toBe(false); - expect(await readFile(join(modelHome, "config.toml"), "utf8")).toBe( - configuration, - ); + if (scenario !== "cancel") { + const loginRequest = (await readFile(transcript, "utf8")) + .trim() + .split("\n") + .map( + (line) => + JSON.parse(line) as { + method?: string; + params?: { apiKey?: string }; + }, + ) + .find((message) => message.method === "account/login/start"); + expect(loginRequest?.params?.apiKey).toBe("synthetic-review-key"); } - if (scenario !== "cancel") - expect(await readFile(transcript, "utf8")).toContain( - '"method":"account/login/start"', - ); expect(existsSync(join(modelHome, "auth.json"))).toBe(false); expect(child!.exitCode !== null || child!.signalCode !== null).toBe(true); expect(existsSync(directory!)).toBe(false); @@ -164,6 +173,77 @@ for (const { }); } +test("empty credential paths use default directories without denying cwd", async () => { + if ( + runTestInSubprocess( + import.meta.path, + "empty credential paths use default directories without denying cwd", + ) + ) { + return; + } + const comparison = { ...(await import("../src/scan-comparison.js")) }; + const root = await mkdtemp(join(tmpdir(), "codex-review-empty-paths-")); + const checkout = await mkdtemp(join(tmpdir(), "codex-review-source-")); + // An empty CODEX_HOME makes native Codex use the real user profile. + mock.module("../src/scan-comparison.js", () => ({ + ...comparison, + disabledMcpServers: async () => ({}), + })); + try { + const names: [string, string][] = [["CODEX_HOME", "GH_CONFIG_DIR"]]; + if (process.platform === "win32") + names.push(["codex_home", "Gh_Config_Dir"]); + for (const [homeName, ghName] of names) { + let args: readonly string[] = []; + const runner = new CodexReviewRunner( + { + CODEX_CLI_PATH: process.execPath, + CODEX_SECURITY_STATE_DIR: join(root, "state"), + OPENAI_API_KEY: "synthetic-review-key", + [homeName]: "", + [ghName]: "", + }, + (_command, commandArgs) => { + args = commandArgs; + throw new Error("Synthetic stop after permission configuration"); + }, + undefined, + checkout, + ); + await expect( + runner.run({ + model: "gpt-5.6-sol", + effort: "ultra", + prompt: "Review the supplied synthetic reports.", + schema: {}, + validate: (value) => value, + }), + ).rejects.toThrow( + "Codex did not complete a validated deduplication review", + ); + const permissions = args.find((argument) => + argument.startsWith("permissions.codex_security_review="), + ); + for (const path of [ + join(homedir(), ".codex"), + join(homedir(), ".config", "gh"), + ]) { + expect(permissions).toContain( + `${JSON.stringify(resolve(path))}="deny"`, + ); + } + expect(permissions).not.toContain( + `${JSON.stringify(resolve(""))}="deny"`, + ); + } + } finally { + mock.module("../src/scan-comparison.js", () => comparison); + await rm(root, { recursive: true, force: true }); + await rm(checkout, { recursive: true, force: true }); + } +}); + test("environment lookups preserve platform case rules and exact-key precedence", () => { const aliases = { codex_home: "synthetic-alias" }; expect(environmentEntry(aliases, "CODEX_HOME")).toBe(