From 713e3c354f9df6bbb80532f9b43ca1ca0a1df0f0 Mon Sep 17 00:00:00 2001 From: lcxhh Date: Thu, 15 Oct 2026 12:49:08 +0800 Subject: [PATCH 1/2] fix(codex): honor requested restart after config changes --- skills/ocx/references/01_surface_lifecycle.md | 2 +- .../references/01_surface_observe-system.md | 2 +- src/cli/capabilities-base.ts | 4 +- src/cli/dispatch.ts | 8 ++- src/cli/registry.ts | 2 +- src/codex/inject.ts | 10 ++++ src/codex/sync.ts | 4 ++ structure/cli-management.md | 2 + structure/config.md | 2 +- tests/cli/cli-dispatch.test.ts | 37 ++++++++++++ .../codex-inject-integration.test.ts | 11 +++- .../codex-inject-missing-config.test.ts | 21 +++++-- .../codex-inject-v1-reconcile.test.ts | 56 +++++++++++++++++-- .../codex-integration/codex-sync-api.test.ts | 14 +++++ 14 files changed, 153 insertions(+), 22 deletions(-) diff --git a/skills/ocx/references/01_surface_lifecycle.md b/skills/ocx/references/01_surface_lifecycle.md index e8aa0b21a9d..e44ce0995ac 100644 --- a/skills/ocx/references/01_surface_lifecycle.md +++ b/skills/ocx/references/01_surface_lifecycle.md @@ -85,7 +85,7 @@ State-changing: yes. | Flag | Value | Meaning | |---|---|---| -| `--restart-codex` | boolean | Restart the Codex app-servers and fully quit and relaunch the Codex desktop app after a catalog or cache write, on macOS, Linux and Windows. | +| `--restart-codex` | boolean | Restart the Codex app-servers and fully quit and relaunch the Codex desktop app after a catalog, cache, or changed configuration write, on macOS, Linux and Windows. | | `--restart-app-server-only` | boolean | Restart only the Codex app-servers and leave the desktop app running; wins over --restart-codex when both are given. | | `--restart-desktop-app` | boolean | Deprecated alias of --restart-codex. | diff --git a/skills/ocx/references/01_surface_observe-system.md b/skills/ocx/references/01_surface_observe-system.md index 2c6f86aded2..4ffff00184b 100644 --- a/skills/ocx/references/01_surface_observe-system.md +++ b/skills/ocx/references/01_surface_observe-system.md @@ -433,7 +433,7 @@ State-changing: yes. JSON mode: `payload`. -- `sync --restart-codex` is not a substitute: it restarts only as a side effect after a catalog or cache write, so it cannot restart a healthy install on request. +- `sync --restart-codex` is not a substitute: it restarts only as a side effect after a catalog, cache, or changed configuration write, so it cannot restart a healthy install on request. - Restarts the Codex desktop app as well as the app-servers, through the same module the CLI uses. When the proxy itself runs inside the Codex app it refuses instead, because restarting the app would kill the request. - --yes is mandatory because this interrupts a running editor session and may discard unsaved composer drafts, model-picker selections, and pending approval prompts; it must never happen because an agent guessed a subcommand. diff --git a/src/cli/capabilities-base.ts b/src/cli/capabilities-base.ts index 4a374354b43..ca3f46648f9 100644 --- a/src/cli/capabilities-base.ts +++ b/src/cli/capabilities-base.ts @@ -576,7 +576,7 @@ export const CAPABILITIES: readonly Capability[] = [ flags: [{"name":"--yes","value":"boolean","summary":"Required: fully quits and relaunches the operator's Codex desktop app, which may discard unsaved composer drafts, model-picker selections, and pending approval prompts; also restarts its app-servers."},{"name":"--json","value":"boolean","summary":"Emit the restart result as JSON."}], mutates: true, json: "payload", - details: ["`sync --restart-codex` is not a substitute: it restarts only as a side effect after a catalog or cache write, so it cannot restart a healthy install on request.","Restarts the Codex desktop app as well as the app-servers, through the same module the CLI uses. When the proxy itself runs inside the Codex app it refuses instead, because restarting the app would kill the request.","--yes is mandatory because this interrupts a running editor session and may discard unsaved composer drafts, model-picker selections, and pending approval prompts; it must never happen because an agent guessed a subcommand."], + details: ["`sync --restart-codex` is not a substitute: it restarts only as a side effect after a catalog, cache, or changed configuration write, so it cannot restart a healthy install on request.","Restarts the Codex desktop app as well as the app-servers, through the same module the CLI uses. When the proxy itself runs inside the Codex app it refuses instead, because restarting the app would kill the request.","--yes is mandatory because this interrupts a running editor session and may discard unsaved composer drafts, model-picker selections, and pending approval prompts; it must never happen because an agent guessed a subcommand."], usage: "ocx system codex-restart --yes [--json]", }, { @@ -694,7 +694,7 @@ export const CAPABILITIES: readonly Capability[] = [ summary: "Synchronize client catalogs, including Aside profiles through the running server's mutation owner.", routes: [{ method: "POST", path: "/api/client-integrations/aside/sync" }], flags: [ - { name: "--restart-codex", value: "boolean", summary: "Restart the Codex app-servers and fully quit and relaunch the Codex desktop app after a catalog or cache write, on macOS, Linux and Windows." }, + { name: "--restart-codex", value: "boolean", summary: "Restart the Codex app-servers and fully quit and relaunch the Codex desktop app after a catalog, cache, or changed configuration write, on macOS, Linux and Windows." }, { name: "--restart-app-server-only", value: "boolean", summary: "Restart only the Codex app-servers and leave the desktop app running; wins over --restart-codex when both are given." }, { name: "--restart-desktop-app", value: "boolean", summary: "Deprecated alias of --restart-codex." }, ], diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index 59d9350e4be..9ffe4e7494b 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -461,11 +461,15 @@ const commandRunners: Record = { code = 1; console.error("Codex sync did not complete. Fix the reported Codex config issue and retry."); } - // Only warn/restart when a catalog or models_cache write actually happened. This is + // Catalog/model-cache writes retain their warning/restart handling. This is // deliberately not an `else`: refreshCodexModelCatalog runs before injectCodexConfig, // so a sync can fail (`ok: false`) after the catalog was already rewritten — which is // exactly when a long-lived app-server is holding the stale list. - if (synced.catalogWritten || synced.cacheSynced) { + // A configuration-only change also needs a reload, but only when explicitly + // requested. A successful no-op or refused write is never restart authority. + const requestedConfigRestart = (restartScope.appServers || restartScope.desktopApp) + && synced.ok && synced.configChanged === true; + if (synced.catalogWritten || synced.cacheSynced || requestedConfigRestart) { await handleRestartScopeAfterWrite(restartScope, console); } // `ocx sync` is a direct CLI path; it does not call the management diff --git a/src/cli/registry.ts b/src/cli/registry.ts index f31b42cbda5..38a835472eb 100644 --- a/src/cli/registry.ts +++ b/src/cli/registry.ts @@ -178,7 +178,7 @@ export const CLI_COMMANDS: CliCommandEntry[] = [ summary: "Fetch provider models and inject them into Codex config.", details: [ "After writing the catalog, warns if long-lived Codex app-server processes are still running.", - "--restart-codex restarts the app-servers AND fully quits and relaunches the Codex desktop app on macOS, Linux and Windows, so its model picker re-reads the catalog. It ends live conversations.", + "--restart-codex restarts the app-servers AND fully quits and relaunches the Codex desktop app on macOS, Linux and Windows, after catalog/cache writes or a committed configuration change. It ends live conversations.", "--restart-app-server-only keeps the narrow behaviour: SIGTERM to matching app-server / code-mode-host processes, desktop app left running. It wins over --restart-codex when both are given.", "--restart-desktop-app is a deprecated alias of --restart-codex and prints a notice.", ], diff --git a/src/codex/inject.ts b/src/codex/inject.ts index a2c6102e878..d315e1295bd 100644 --- a/src/codex/inject.ts +++ b/src/codex/inject.ts @@ -142,6 +142,8 @@ export interface CodexInjectResult { message: string; /** False when injection intentionally preserves configuration owned by another provider. */ configApplied?: false; + /** True only after a successful configuration transaction changes config.toml bytes. */ + configChanged?: boolean; /** * Structured read-only history preflight refusal; never parsed from display text. * @@ -717,6 +719,11 @@ async function injectCodexConfigImpl( releaseConfigWriteLock(configLock.handle); heldConfigWriteLock = undefined; } + // Both write paths have committed, including the coordinator's post-callback checks. + // Compare against the admitted original, not post-v1-reconcile input: the native + // feature transition is part of this transaction too. Refusals/rollbacks return + // above and must never publish this receipt. No filesystem reread after unlock. + const configChanged = Boolean(missingConfig) || effectivePlan.content !== rawContent; // Legacy mode still forward-tags history so re-tagged threads stay listable. Design B needs // the opposite: a one-time migration of previously re-tagged threads BACK to openai (restore // machinery; cheap no-op when there is nothing to migrate). The client-compaction opt-in keeps @@ -799,6 +806,7 @@ async function injectCodexConfigImpl( if (effectivePlan.keptUserBaseUrl && effectivePlan.keepRootOverrideAlongsideTable) { return { success: true, + configChanged, ...(effectivePlan.nativeSubagentDefaultsWarning ? { nativeSubagentDefaultsWarning: effectivePlan.nativeSubagentDefaultsWarning } : {}), ...(effectivePlan.historyRelabelRefusal ? { historyPreflightFailureReason: effectivePlan.historyRelabelRefusal } : {}), message: @@ -817,6 +825,7 @@ async function injectCodexConfigImpl( if (effectivePlan.keptUserBaseUrl) { return { success: true, + configChanged, ...(effectivePlan.nativeSubagentDefaultsWarning ? { nativeSubagentDefaultsWarning: effectivePlan.nativeSubagentDefaultsWarning } : {}), @@ -839,6 +848,7 @@ async function injectCodexConfigImpl( : `Pointed Codex's built-in openai provider at the opencodex proxy (openai_base_url + realtime sideband override).\n`; return { success: true, + configChanged, ...(effectivePlan.nativeSubagentDefaultsWarning ? { nativeSubagentDefaultsWarning: effectivePlan.nativeSubagentDefaultsWarning } : {}), ...(effectivePlan.historyRelabelRefusal ? { historyPreflightFailureReason: effectivePlan.historyRelabelRefusal } : {}), message: diff --git a/src/codex/sync.ts b/src/codex/sync.ts index 4be25a40aac..1867fd9573c 100644 --- a/src/codex/sync.ts +++ b/src/codex/sync.ts @@ -33,6 +33,8 @@ export interface CodexSyncResult { catalogPath: string | null; catalogExists: boolean; catalogWritten: boolean; + /** Committed configuration-byte change, never inferred from a successful no-op. */ + configChanged?: boolean; cacheSynced: boolean; /** * Whether the catalog owner committed a validated catalog or refused the @@ -214,6 +216,7 @@ export async function syncModelsToCodex( return { status: "applied", ok: result.success, + ...(result.success && result.configChanged === true ? { configChanged: true } : {}), added: 0, catalogPath: null, catalogExists: false, @@ -332,6 +335,7 @@ export async function syncModelsToCodex( return { status: "applied", ok: result.success, + ...(result.success && result.configChanged === true ? { configChanged: true } : {}), added, ...(refreshOutcome ? { refreshOutcome } : {}), catalogPath, diff --git a/structure/cli-management.md b/structure/cli-management.md index ec0a2af8f04..4089a2e263a 100644 --- a/structure/cli-management.md +++ b/structure/cli-management.md @@ -125,3 +125,5 @@ Internal version observations in `src/service/managing-cli.ts` (`probeVersionOnc `src/cli/account-auth.ts` creates a Grok coupon operation UUID before every explicitly confirmed consume request unless the caller supplies one. Uncertain delivery and unknown-outcome API responses, including `operation_state_changed`, print guidance to preserve that same `--operation-id`; the CLI performs no automatic redemption retry. Ledger and reconciliation semantics are owned by [Grok reset coupons](providers/xai-grok.md#grok-reset-coupons-billing-api-parity). Remote Link enrollment is shared with the dashboard through `src/server/management/link-enrollment.ts` and `src/cli/link-enrollment.ts`. `ocx link probe ` returns a temporary SSH fingerprint/key type/version offer. `ocx link add --expect-fingerprint SHA256:…` confirms exact trust and applies under loopback admin authority, with TTY explicit confirmation when the flag is omitted. Non-TTY omission fails before discovery. One Home URL and a 360-second deadline cover all requests and bodies; redirects are refused, credentials are never printed, and post-connect residual failures retain fixed recovery. `tests/cli/cli-link-enrollment.test.ts` verifies these boundaries. Child `link status` fallback uses the shared projector with persisted client connection and `clientLinkTunnelStatus`, plus validated client-owned sidecar/marker initiation identity; ambiguous evidence never invents direction. An unobserved Home-owned tunnel is conservatively idle. Link-mode `status` reads supported Hub projections via the local Child without sending a credential; a standalone Home role-based 404 explains absent Hub state rather than recommending an upgrade. + +An explicit `sync --restart-codex` (or app-server-only scope) also reloads after a committed configuration-byte change, even when no catalog/cache was written. Without an explicit restart scope, configuration-only changes do not restart clients. Existing catalog/cache-write restart behavior remains unchanged; success alone is not proof of changed configuration. diff --git a/structure/config.md b/structure/config.md index f803d408c5c..1d05f6e3124 100644 --- a/structure/config.md +++ b/structure/config.md @@ -126,7 +126,7 @@ The former `showCodexSparkQuota` key is inert passthrough data when loading an o ## Config injection -An explicit desktop restart after injection uses the [runtime process-membership contract](runtime.md#codex-desktop-process-membership); mixed Windows path spelling does not change which installation the restart targets. +Configuration injection exposes `configChanged` only after a successful configuration transaction, comparing admitted original bytes with final committed bytes including native feature reconciliation. Preflight, preserved external ownership, skipped writes and compensated failures never authorize a configuration-only restart. An explicit desktop restart after injection uses the [runtime process-membership contract](runtime.md#codex-desktop-process-membership); mixed Windows path spelling does not change which installation the restart targets. One further root key is conditional rather than part of either routing form. While the web-search sidecar is switched off (`webSearchSidecar.enabled: false`), the injection also owns Codex's own diff --git a/tests/cli/cli-dispatch.test.ts b/tests/cli/cli-dispatch.test.ts index 246238a8cd1..fa89e4b3e0b 100644 --- a/tests/cli/cli-dispatch.test.ts +++ b/tests/cli/cli-dispatch.test.ts @@ -1364,3 +1364,40 @@ describe("CLI usage recovery contracts", () => { }, ); }); + + +describe("explicit sync restart after a committed configuration change", () => { + for (const scenario of [ + { changed: true, ok: true, requested: true, expected: 1 }, + { changed: false, ok: true, requested: true, expected: 0 }, + { changed: true, ok: false, requested: true, expected: 0 }, + { changed: true, ok: true, requested: false, expected: 0 }, + ]) test(JSON.stringify(scenario), async () => { + const state = await import("../../src/client/state"); + const sync = await import("../../src/codex/sync"); + const restart = await import("../../src/cli/restart-scope"); + const aside = await import("../../src/cli/aside-profiles"); + const stateSpy = spyOn(state, "readClientConnectionState").mockReturnValue({ kind: "disconnected" }); + const syncSpy = spyOn(sync, "syncModelsToCodex").mockResolvedValue({ + status: "applied", ok: scenario.ok, configChanged: scenario.changed, + added: 0, catalogPath: null, catalogExists: false, catalogWritten: false, cacheSynced: false, message: "fixture", + }); + const restartSpy = spyOn(restart, "handleRestartScopeAfterWrite").mockResolvedValue({}); + const asideSpy = spyOn(aside, "refreshAsideProfilesThroughServer").mockResolvedValue([]); + const log = spyOn(console, "log").mockImplementation(() => {}); + const error = spyOn(console, "error").mockImplementation(() => {}); + const network = spyOn(globalThis, "fetch").mockRejectedValue(new Error("Unexpected network request")); + try { + const args = scenario.requested ? ["sync", "--restart-codex"] : ["sync"]; + expect(await dispatchCommand({ kind: "command", command: "sync", args }, { + ...fakeDeps, args, findLiveProxy: async () => null, + })).toBe(scenario.ok ? 0 : 1); + expect(restartSpy).toHaveBeenCalledTimes(scenario.expected); + if (scenario.expected) expect(restartSpy.mock.calls[0]![0]).toEqual({ appServers: true, desktopApp: true }); + expect(network).not.toHaveBeenCalled(); + } finally { + stateSpy.mockRestore(); syncSpy.mockRestore(); restartSpy.mockRestore(); asideSpy.mockRestore(); + log.mockRestore(); error.mockRestore(); network.mockRestore(); + } + }); +}); diff --git a/tests/codex-integration/codex-inject-integration.test.ts b/tests/codex-integration/codex-inject-integration.test.ts index 05977c7aad5..528b7d5ed58 100644 --- a/tests/codex-integration/codex-inject-integration.test.ts +++ b/tests/codex-integration/codex-inject-integration.test.ts @@ -900,10 +900,12 @@ describe("injectCodexConfig integration (Design B)", () => { expect(result.status).toBe(0); const value = JSON.parse(result.stdout.trim()); expect(value.preflight.success).toBe(true); + expect(value.preflight.configChanged).not.toBe(true); expect(value.before).toBe(original); expect(value.afterPreflight).toBe(original); expect(value.journalAfterPreflight).toBe(false); expect(value.committed.success).toBe(true); + expect(value.committed.configChanged).toBe(true); expect(value.injected).toContain('base_url = "https://hub.example.test/v1"'); expect(value.injected).toContain('env_key = "OPENCODEX_API_AUTH_TOKEN"'); expect(value.owner).toEqual({ kind: "client", apiKeyId: "client-key-1" }); @@ -971,9 +973,13 @@ describe("injectCodexConfig integration (Design B)", () => { test("re-inject over a Design B config is idempotent", () => { writeFileSync(join(codexHome, "config.toml"), 'model = "gpt-5.5"\n', "utf8"); - expect(runInject(codexHome, ocxHome).status).toBe(0); + const firstResult = runInject(codexHome, ocxHome); + expect(firstResult.status, firstResult.stderr).toBe(0); + expect(JSON.parse(firstResult.stdout)).toMatchObject({ success: true, configChanged: true }); const first = readFileSync(join(codexHome, "config.toml"), "utf8"); - expect(runInject(codexHome, ocxHome).status).toBe(0); + const secondResult = runInject(codexHome, ocxHome); + expect(secondResult.status, secondResult.stderr).toBe(0); + expect(JSON.parse(secondResult.stdout)).toMatchObject({ success: true, configChanged: false }); const second = readFileSync(join(codexHome, "config.toml"), "utf8"); expect(second.match(/openai_base_url/g)?.length).toBe(1); @@ -1514,6 +1520,7 @@ describe("injectCodexConfig integration (Design B)", () => { expect(result.message).toContain("routing NOT injected"); expect(result.message).toContain('external model_provider "custom"'); expect(result.configApplied).toBe(false); + expect(result.configChanged).not.toBe(true); expect(result.message).toContain("http://127.0.0.1:10100/v1"); expect(result.message).toContain("Responses passthrough"); expect(result.nativeSubagentDefaultsWarning).toContain("external model_provider"); diff --git a/tests/codex-integration/codex-inject-missing-config.test.ts b/tests/codex-integration/codex-inject-missing-config.test.ts index 68b236881d3..44316058385 100644 --- a/tests/codex-integration/codex-inject-missing-config.test.ts +++ b/tests/codex-integration/codex-inject-missing-config.test.ts @@ -28,11 +28,11 @@ afterEach(() => { removeTreeWithRetry(root); }); -function runInject(env: Record, validateOnly = false): { result: { success: boolean; message: string }; stderr: string } { +function runInject(env: Record, validateOnly = false): { result: { success: boolean; message: string; configChanged?: boolean }; stderr: string } { const script = ` const { injectCodexConfig } = require("./src/codex/inject"); injectCodexConfig(10100, {}, ${validateOnly ? "{ validateOnly: true }" : "{}"}).then(result => { - console.log(JSON.stringify({ success: result.success, message: result.message })); + console.log(JSON.stringify({ success: result.success, message: result.message, configChanged: result.configChanged })); }); `; const spawned = spawnSync(process.execPath, ["--eval", script], { @@ -50,6 +50,7 @@ test("a Codex home without config.toml is bootstrapped and routed", () => { mkdirSync(codexHome); const { result, stderr } = runInject({ CODEX_HOME: codexHome, OPENCODEX_HOME: join(root, "ocx") }); expect(result.success, stderr).toBe(true); + expect(result.configChanged).toBe(true); const config = readFileSync(join(codexHome, "config.toml"), "utf8"); expect(config).toContain("opencodex"); }); @@ -59,6 +60,7 @@ test("a validate-only preflight passes without creating config.toml", () => { mkdirSync(codexHome); const { result, stderr } = runInject({ CODEX_HOME: codexHome, OPENCODEX_HOME: join(root, "ocx") }, true); expect(result.success, stderr).toBe(true); + expect(result.configChanged).not.toBe(true); expect(existsSync(join(codexHome, "config.toml"))).toBe(false); }); @@ -69,6 +71,7 @@ test("a missing default Codex home is refused with an actionable message and not CODEX_HOME: "", HOME: home, USERPROFILE: home, OPENCODEX_HOME: join(root, "ocx"), }); expect(result.success).toBe(false); + expect(result.configChanged).not.toBe(true); expect(result.message).toContain("does not exist yet"); expect(result.message).toContain("CODEX_HOME"); expect(existsSync(join(home, ".codex"))).toBe(false); @@ -79,10 +82,14 @@ test("a failure inside the write boundary after bootstrap rolls config.toml back mkdirSync(codexHome); const script = ` const inject = require("./src/codex/inject"); - inject.setBeforeHistoryArtifactCommitForTests(() => { throw new Error("fixture failure after bootstrap"); }); + let reachedArtifactCommit = false; + inject.setBeforeHistoryArtifactCommitForTests(() => { + reachedArtifactCommit = true; + throw new Error("fixture failure after bootstrap"); + }); inject.injectCodexConfig(10100, {}, {}).then( - result => console.log(JSON.stringify({ settled: "result", success: result.success })), - error => console.log(JSON.stringify({ settled: "error", message: String(error && error.message) })), + result => console.log(JSON.stringify({ settled: "result", success: result.success, configChanged: result.configChanged, reachedArtifactCommit })), + error => console.log(JSON.stringify({ settled: "error", message: String(error && error.message), reachedArtifactCommit })), ); `; const spawned = spawnSync(process.execPath, ["--eval", script], { @@ -92,7 +99,9 @@ test("a failure inside the write boundary after bootstrap rolls config.toml back timeout: SPAWN_BUDGET_MS - 5_000, }); const lines = (spawned.stdout ?? "").trim().split("\n"); - const outcome = JSON.parse(lines[lines.length - 1] ?? "{}") as { settled?: string; success?: boolean }; + const outcome = JSON.parse(lines[lines.length - 1] ?? "{}") as { settled?: string; success?: boolean; configChanged?: boolean; reachedArtifactCommit?: boolean }; + expect(outcome.reachedArtifactCommit, spawned.stderr).toBe(true); + expect(outcome.configChanged).not.toBe(true); expect(outcome.settled === "error" || outcome.success === false, spawned.stderr).toBe(true); expect(existsSync(join(codexHome, "config.toml"))).toBe(false); }); diff --git a/tests/codex-integration/codex-inject-v1-reconcile.test.ts b/tests/codex-integration/codex-inject-v1-reconcile.test.ts index a90fc213bc0..1f1946b720c 100644 --- a/tests/codex-integration/codex-inject-v1-reconcile.test.ts +++ b/tests/codex-integration/codex-inject-v1-reconcile.test.ts @@ -41,7 +41,7 @@ function runV1CoordinatorFailure( codexHome: string, ocxHome: string, failure: "commit" | "publish", -): { threw: boolean; message: string; toggles: number; sawReconcile: boolean; sawArtifactsAtCommit: boolean } { +): { threw: boolean; message: string; toggles: number; sawReconcile: boolean; sawArtifactsAtCommit: boolean; configChanged?: boolean } { const script = ` const fs = require("node:fs"); const { join } = require("node:path"); @@ -75,9 +75,10 @@ function runV1CoordinatorFailure( } let threw = false; let message = ""; - try { await injectCodexConfig(10100, { multiAgentMode: "v1" }); } + let result; + try { result = await injectCodexConfig(10100, { multiAgentMode: "v1" }); } catch (error) { threw = true; message = String(error); } - console.log(JSON.stringify({ threw, message, toggles, sawReconcile, sawArtifactsAtCommit })); + console.log(JSON.stringify({ threw, message, toggles, sawReconcile, sawArtifactsAtCommit, configChanged: result?.configChanged })); `; const child = spawnSync(process.execPath, ["--eval", script], { cwd: repoRoot, @@ -203,13 +204,51 @@ describe("injectCodexConfig v1-surface reconcile", () => { expect(child.status, child.stderr).toBe(0); const out = JSON.parse(child.stdout); - expect(out.result).toMatchObject({ success: true }); + expect(out.result).toMatchObject({ success: true, configChanged: true }); expect(out.expectedId).toMatch(/^[a-f0-9]{64}$/); expect(out.publishedId).toBe(out.expectedId); expect(out.committedConfig).toBe(out.expectedConfig); expect(out.committedConfig).toContain("multi_agent_v2 = false"); }); + test("configChanged includes a v1-only reconciliation before an otherwise identical artifact plan", () => { + const configPath = join(codexHome, "config.toml"); + writeFileSync(configPath, 'model = "gpt-5.5"\n\n[features]\nmulti_agent_v2 = false\n', "utf8"); + const script = ` + const fs = require("node:fs"); + const { join } = require("node:path"); + const { injectCodexConfig } = require("./src/codex/inject"); + const { setCodexMultiAgentV2ToggleForTests } = require("./src/codex/inject/multi-agent-v2"); + const path = join(process.env.CODEX_HOME, "config.toml"); + const config = { multiAgentMode: "v1" }; + setCodexMultiAgentV2ToggleForTests(enabled => { + const current = fs.readFileSync(path, "utf8"); + fs.writeFileSync(path, current.replace("multi_agent_v2 = true", "multi_agent_v2 = " + enabled)); + }); + const first = await injectCodexConfig(10100, config); + const stable = fs.readFileSync(path, "utf8"); + fs.writeFileSync(path, stable.replace("multi_agent_v2 = false", "multi_agent_v2 = true")); + const before = fs.readFileSync(path, "utf8"); + const reconciled = await injectCodexConfig(10100, config); + const after = fs.readFileSync(path, "utf8"); + const unchanged = await injectCodexConfig(10100, config); + console.log(JSON.stringify({ first, reconciled, unchanged, before, after, stable })); + `; + const child = spawnSync(process.execPath, ["--eval", script], { + cwd: repoRoot, + env: { ...process.env, CODEX_HOME: codexHome, OPENCODEX_HOME: ocxHome }, + encoding: "utf8", + timeout: SPAWN_BUDGET_MS - 5_000, + }); + expect(child.status, child.stderr).toBe(0); + const out = JSON.parse(child.stdout); + expect(out.first).toMatchObject({ success: true, configChanged: true }); + expect(out.before).not.toBe(out.stable); + expect(out.after).toBe(out.stable); + expect(out.reconciled).toMatchObject({ success: true, configChanged: true }); + expect(out.unchanged).toMatchObject({ success: true, configChanged: false }); + }); + test("a skipped v1 injection does not run the v2 reconcile or leave the file changed", () => { const configPath = join(codexHome, "config.toml"); writeFileSync(configPath, 'model = "gpt-5.5"\n\n[features]\nmulti_agent_v2 = true\n', "utf8"); @@ -236,6 +275,7 @@ describe("injectCodexConfig v1-surface reconcile", () => { expect(child.status, child.stderr).toBe(0); const out = JSON.parse(child.stdout); expect(out.result).toMatchObject({ success: true, status: "skipped", skippedReason: "desired_disabled" }); + expect(out.result.configChanged).not.toBe(true); // The gate ran before the reconcile: no transition ran and nothing was written. expect(out.toggles).toBe(0); const config = readFileSync(configPath, "utf8"); @@ -276,7 +316,9 @@ describe("injectCodexConfig v1-surface reconcile", () => { }); expect(child.status, child.stderr).toBe(0); - expect(JSON.parse(child.stdout)).toMatchObject({ threw: true }); + const out = JSON.parse(child.stdout); + expect(out).toMatchObject({ threw: true }); + expect(out.result?.configChanged).not.toBe(true); // Byte-exact restoration: the flag flip was rolled back with everything else. expect(readFileSync(configPath, "utf8")).toBe(original); expect(existsSync(join(codexHome, "opencodex.config.toml"))).toBe(false); @@ -290,6 +332,7 @@ describe("injectCodexConfig v1-surface reconcile", () => { writeFileSync(configPath, originalConfig); const out = runV1CoordinatorFailure(codexHome, ocxHome, "commit"); + expect(out.configChanged).not.toBe(true); expect(out).toMatchObject({ threw: true, toggles: 1, sawReconcile: true, sawArtifactsAtCommit: true }); expect(out.message).toContain("fixture coordinator commit failed"); expect(readFileSync(configPath, "utf8")).toBe(originalConfig); @@ -304,6 +347,7 @@ describe("injectCodexConfig v1-surface reconcile", () => { writeFileSync(configPath, originalConfig); const out = runV1CoordinatorFailure(codexHome, ocxHome, "publish"); + expect(out.configChanged).not.toBe(true); expect(out).toMatchObject({ threw: true, toggles: 1, sawReconcile: true, sawArtifactsAtCommit: false }); expect(out.message).toContain("could not be published: conflict"); expect(readFileSync(configPath, "utf8")).toBe(originalConfig); @@ -359,7 +403,7 @@ describe("injectCodexConfig v1-surface reconcile", () => { expect(child.status, child.stderr).toBe(0); const out = JSON.parse(child.stdout); - expect(out.result).toMatchObject({ success: true }); + expect(out.result).toMatchObject({ success: true, configChanged: true }); expect(out.competitor.status).toBe(0); expect(JSON.parse(out.competitor.stdout).success).toBe(false); const config = readFileSync(configPath, "utf8"); diff --git a/tests/codex-integration/codex-sync-api.test.ts b/tests/codex-integration/codex-sync-api.test.ts index 875ed74d2f4..6b2d7f4c16a 100644 --- a/tests/codex-integration/codex-sync-api.test.ts +++ b/tests/codex-integration/codex-sync-api.test.ts @@ -124,6 +124,20 @@ describe("GUI/CLI Codex sync backend", () => { serviceManagerPreloadPath = undefined; if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); }); + test.each([true, false])("forwards only a committed configuration change: %s", async changed => { + const result = await syncModelsToCodex(12345, config, null, { + admitCodexWrite: admittedSync, + refreshCodexModelCatalog: async () => ({ added: 0, path: "/tmp/fixture-catalog.json", catalogExists: false, + catalogWritten: false, cacheSynced: false, comboOmissions: [] }), + injectCodexConfig: async () => ({ success: true, configChanged: changed, message: "fixture" }), + currentExternalCodexModelProvider: () => null, + collectCodexHomeDiagnostic: () => homeDiagnostic(), + }); + expect(result.ok).toBe(true); + expect(result.configChanged === true).toBe(changed); + expect(result.catalogWritten).toBe(false); + expect(result.cacheSynced).toBe(false); + }); test("returns the structured sync result used by POST /api/sync", async () => { let injectedPort = 0; let injectedCatalogPath: string | null | undefined; From edb93130ed922a6e60ec990469d9a63cd5b1a648 Mon Sep 17 00:00:00 2001 From: GOAT Date: Thu, 15 Oct 2026 16:54:50 +0900 Subject: [PATCH 2/2] refactor(codex): move apply-history failure framing out of inject.ts The configChanged receipt pushed src/codex/inject.ts past its file-size ratchet cap (993 > 987). Move the self-contained formatApplyHistoryFailure byte for byte into src/codex/inject/history-failure.ts; inject.ts imports and re-exports it, so callers are unchanged. --- src/codex/inject.ts | 28 ++-------------------------- src/codex/inject/history-failure.ts | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 26 deletions(-) create mode 100644 src/codex/inject/history-failure.ts diff --git a/src/codex/inject.ts b/src/codex/inject.ts index d315e1295bd..64c30fc61c4 100644 --- a/src/codex/inject.ts +++ b/src/codex/inject.ts @@ -46,8 +46,8 @@ import { writeJournal, } from "./journal"; import { HISTORY_RELABEL_STANDS_DOWN } from "./history-provider"; +import { formatApplyHistoryFailure } from "./inject/history-failure"; import { - describeHistoryJobFailure, deriveCodexHistoryOperation, resolveCodexHistoryJobTarget, runCodexHistoryJob, @@ -870,31 +870,7 @@ export function getCodexConfigPath(): string { return CODEX_CONFIG_PATH; } -/** - * Frame one failed apply history job honestly. - * - * A genuine lock keeps the established deferred/SKIPPED wording; any other - * reason names itself instead of blaming the Codex app/IDE. - */ -export function formatApplyHistoryFailure(outcome: CodexHistoryJobOutcome, legacyMode: boolean): string { - // A busy database is a deferral no matter which half observed it: the lock - // contended (blocked/busy), or the worker acquired the lock and then found - // SQLite busy (failed with a busy history reason). Only those keep the - // deferred headline; every other failure is a real "NOT changed". - const busy = - (outcome.kind === "blocked" && outcome.reason === "busy") || - (outcome.kind === "failed" && outcome.historyFailureReason === "busy"); - const partiallyChanged = outcome.kind === "failed" - && ((outcome.rows ?? 0) > 0 || (outcome.files ?? 0) > 0); - const headline = partiallyChanged - ? "Codex resume history changed but did not converge" - : legacyMode - ? "Codex resume history sync SKIPPED" - : busy - ? "Codex resume history metadata restore deferred" - : "Codex resume history NOT changed"; - return ` ⚠️ ${headline}: ${describeHistoryJobFailure(outcome, "apply", legacyMode)}\n`; -} +export { formatApplyHistoryFailure }; export { providerBaseHost, diff --git a/src/codex/inject/history-failure.ts b/src/codex/inject/history-failure.ts new file mode 100644 index 00000000000..5890d5202f4 --- /dev/null +++ b/src/codex/inject/history-failure.ts @@ -0,0 +1,27 @@ +import { describeHistoryJobFailure, type CodexHistoryJobOutcome } from "../history-job"; + +/** + * Frame one failed apply history job honestly. + * + * A genuine lock keeps the established deferred/SKIPPED wording; any other + * reason names itself instead of blaming the Codex app/IDE. + */ +export function formatApplyHistoryFailure(outcome: CodexHistoryJobOutcome, legacyMode: boolean): string { + // A busy database is a deferral no matter which half observed it: the lock + // contended (blocked/busy), or the worker acquired the lock and then found + // SQLite busy (failed with a busy history reason). Only those keep the + // deferred headline; every other failure is a real "NOT changed". + const busy = + (outcome.kind === "blocked" && outcome.reason === "busy") || + (outcome.kind === "failed" && outcome.historyFailureReason === "busy"); + const partiallyChanged = outcome.kind === "failed" + && ((outcome.rows ?? 0) > 0 || (outcome.files ?? 0) > 0); + const headline = partiallyChanged + ? "Codex resume history changed but did not converge" + : legacyMode + ? "Codex resume history sync SKIPPED" + : busy + ? "Codex resume history metadata restore deferred" + : "Codex resume history NOT changed"; + return ` ⚠️ ${headline}: ${describeHistoryJobFailure(outcome, "apply", legacyMode)}\n`; +}