From a6ed7ff103eb3f026aab7bd93164039ca111ec6e Mon Sep 17 00:00:00 2001 From: Kevin Alwell Date: Tue, 29 Apr 2025 13:10:16 -0400 Subject: [PATCH] Fixes issue #726 by adding config to configToSave object (#728) The saveConfig() function only includes a hardcoded subset of properties when writing the config file. Any property not explicitly listed (like disableResponseStorage) will be dropped. I have added `disableResponseStorage` to the `configToSave` object as the immediate fix. [Linking Issue this fixes.](https://github.com/openai/codex/issues/726) --- codex-cli/src/cli.tsx | 11 ++- codex-cli/src/utils/config.ts | 19 +++- .../disableResponseStorage.agentLoop.test.ts | 93 +++++++++++++++++++ .../tests/disableResponseStorage.test.ts | 43 +++++++++ 4 files changed, 161 insertions(+), 5 deletions(-) create mode 100644 codex-cli/tests/disableResponseStorage.agentLoop.test.ts create mode 100644 codex-cli/tests/disableResponseStorage.test.ts diff --git a/codex-cli/src/cli.tsx b/codex-cli/src/cli.tsx index ce87fb00..059136e8 100644 --- a/codex-cli/src/cli.tsx +++ b/codex-cli/src/cli.tsx @@ -294,6 +294,12 @@ if (!apiKey && !NO_API_KEY_REQUIRED.has(provider.toLowerCase())) { process.exit(1); } +const flagPresent = Object.hasOwn(cli.flags, "disableResponseStorage"); + +const disableResponseStorage = flagPresent + ? Boolean(cli.flags.disableResponseStorage) // value user actually passed + : (config.disableResponseStorage ?? false); // fall back to YAML, default to false + config = { apiKey, ...config, @@ -303,10 +309,7 @@ config = { (cli.flags.reasoning as ReasoningEffort | undefined) ?? "high", flexMode: Boolean(cli.flags.flexMode), provider, - disableResponseStorage: - cli.flags.disableResponseStorage !== undefined - ? Boolean(cli.flags.disableResponseStorage) - : config.disableResponseStorage, + disableResponseStorage, }; // Check for updates after loading config. This is important because we write state file in diff --git a/codex-cli/src/utils/config.ts b/codex-cli/src/utils/config.ts index fb2d19c5..87274e12 100644 --- a/codex-cli/src/utils/config.ts +++ b/codex-cli/src/utils/config.ts @@ -323,6 +323,22 @@ export const loadConfig = ( } } + if ( + storedConfig.disableResponseStorage !== undefined && + typeof storedConfig.disableResponseStorage !== "boolean" + ) { + if (storedConfig.disableResponseStorage === "true") { + storedConfig.disableResponseStorage = true; + } else if (storedConfig.disableResponseStorage === "false") { + storedConfig.disableResponseStorage = false; + } else { + log( + `[codex] Warning: 'disableResponseStorage' in config is not a boolean (got '${storedConfig.disableResponseStorage}'). Ignoring this value.`, + ); + delete storedConfig.disableResponseStorage; + } + } + const instructionsFilePathResolved = instructionsPath ?? INSTRUCTIONS_FILEPATH; const userInstructions = existsSync(instructionsFilePathResolved) @@ -372,7 +388,7 @@ export const loadConfig = ( instructions: combinedInstructions, notify: storedConfig.notify === true, approvalMode: storedConfig.approvalMode, - disableResponseStorage: storedConfig.disableResponseStorage ?? false, + disableResponseStorage: storedConfig.disableResponseStorage === true, reasoningEffort: storedConfig.reasoningEffort, }; @@ -488,6 +504,7 @@ export const saveConfig = ( provider: config.provider, providers: config.providers, approvalMode: config.approvalMode, + disableResponseStorage: config.disableResponseStorage, reasoningEffort: config.reasoningEffort, }; diff --git a/codex-cli/tests/disableResponseStorage.agentLoop.test.ts b/codex-cli/tests/disableResponseStorage.agentLoop.test.ts new file mode 100644 index 00000000..b891e89a --- /dev/null +++ b/codex-cli/tests/disableResponseStorage.agentLoop.test.ts @@ -0,0 +1,93 @@ +/** + * codex-cli/tests/disableResponseStorage.agentLoop.test.ts + * + * Verifies AgentLoop's request-building logic for both values of + * disableResponseStorage. + */ + +import { describe, it, expect, vi } from "vitest"; +import { AgentLoop } from "../src/utils/agent/agent-loop"; +import type { AppConfig } from "../src/utils/config"; +import { ReviewDecision } from "../src/utils/agent/review"; + +/* ─────────── 1. Spy + module mock ─────────────────────────────── */ +const createSpy = vi.fn().mockResolvedValue({ + data: { id: "resp_123", status: "completed", output: [] }, +}); + +vi.mock("openai", () => ({ + default: class { + public responses = { create: createSpy }; + }, + APIConnectionTimeoutError: class extends Error {}, +})); + +/* ─────────── 2. Parametrised tests ─────────────────────────────── */ +describe.each([ + { flag: true, title: "omits previous_response_id & sets store:false" }, + { flag: false, title: "sends previous_response_id & allows store:true" }, +])("AgentLoop with disableResponseStorage=%s", ({ flag, title }) => { + /* build a fresh config for each case */ + const cfg: AppConfig = { + model: "o4-mini", + provider: "openai", + instructions: "", + disableResponseStorage: flag, + notify: false, + }; + + it(title, async () => { + /* reset spy per iteration */ + createSpy.mockClear(); + + const loop = new AgentLoop({ + model: cfg.model, + provider: cfg.provider, + config: cfg, + instructions: "", + approvalPolicy: "suggest", + disableResponseStorage: flag, + additionalWritableRoots: [], + onItem() {}, + onLoading() {}, + getCommandConfirmation: async () => ({ review: ReviewDecision.YES }), + onLastResponseId() {}, + }); + + await loop.run([ + { + type: "message", + role: "user", + content: [{ type: "input_text", text: "hello" }], + }, + ]); + + expect(createSpy).toHaveBeenCalledTimes(1); + + const call = createSpy.mock.calls[0]; + if (!call) { + throw new Error("Expected createSpy to have been called at least once"); + } + const payload: any = call[0]; + + if (flag) { + /* behaviour when ZDR is *on* */ + expect(payload).not.toHaveProperty("previous_response_id"); + if (payload.input) { + payload.input.forEach((m: any) => { + expect(m.store === undefined ? false : m.store).toBe(false); + }); + } + } else { + /* behaviour when ZDR is *off* */ + expect(payload).toHaveProperty("previous_response_id"); + if (payload.input) { + payload.input.forEach((m: any) => { + if ("store" in m) { + expect(m.store).not.toBe(false); + } + }); + } + } + }); +}); diff --git a/codex-cli/tests/disableResponseStorage.test.ts b/codex-cli/tests/disableResponseStorage.test.ts new file mode 100644 index 00000000..83c22450 --- /dev/null +++ b/codex-cli/tests/disableResponseStorage.test.ts @@ -0,0 +1,43 @@ +/** + * codex/codex-cli/tests/disableResponseStorage.test.ts + */ + +import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { mkdtempSync, rmSync, writeFileSync, mkdirSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +import { loadConfig, saveConfig } from "../src/utils/config"; +import type { AppConfig } from "../src/utils/config"; + +const sandboxHome: string = mkdtempSync(join(tmpdir(), "codex-home-")); +const codexDir: string = join(sandboxHome, ".codex"); +const yamlPath: string = join(codexDir, "config.yaml"); + +describe("disableResponseStorage persistence", () => { + beforeAll((): void => { + // mkdir -p ~/.codex inside the sandbox + rmSync(codexDir, { recursive: true, force: true }); + mkdirSync(codexDir, { recursive: true }); + + // seed YAML with ZDR enabled + writeFileSync(yamlPath, "model: o4-mini\ndisableResponseStorage: true\n"); + }); + + afterAll((): void => { + rmSync(sandboxHome, { recursive: true, force: true }); + }); + + it("keeps disableResponseStorage=true across load/save cycle", async (): Promise => { + // 1️⃣ explicitly load the sandbox file + const cfg1: AppConfig = loadConfig(yamlPath); + expect(cfg1.disableResponseStorage).toBe(true); + + // 2️⃣ save right back to the same file + await saveConfig(cfg1, yamlPath); + + // 3️⃣ reload and re-assert + const cfg2: AppConfig = loadConfig(yamlPath); + expect(cfg2.disableResponseStorage).toBe(true); + }); +});