From 1f3ea6faaab04163562dc3cf02e2361fb91810f2 Mon Sep 17 00:00:00 2001 From: Ben Badejo <188106718+bdjben@users.noreply.github.com> Date: Fri, 10 Jul 2026 12:38:35 +0300 Subject: [PATCH] fix(codex): prevent startup hangs after binding migration (#103281) * fix(codex): archive imported orphan binding sidecars * fix(codex): harden binding sidecar migration Co-authored-by: Benjamin Badejo * chore: keep release notes out of contributor PRs --------- Co-authored-by: Benjamin Badejo Co-authored-by: Peter Steinberger --- extensions/codex/doctor-contract-api.test.ts | 628 +++++++++++++++--- .../codex/src/app-server/session-binding.ts | 14 +- .../src/migration/session-binding-sidecars.ts | 338 +++++++--- 3 files changed, 787 insertions(+), 193 deletions(-) diff --git a/extensions/codex/doctor-contract-api.test.ts b/extensions/codex/doctor-contract-api.test.ts index a09fabfdd97b..ce143472c6f6 100644 --- a/extensions/codex/doctor-contract-api.test.ts +++ b/extensions/codex/doctor-contract-api.test.ts @@ -20,21 +20,104 @@ import { bindingStoreKey, CODEX_APP_SERVER_BINDING_MAX_ENTRIES, CODEX_APP_SERVER_BINDING_NAMESPACE, + createStoredCodexAppServerBinding, type StoredCodexAppServerBinding, } from "./src/app-server/session-binding.js"; import { legacyCodexConversationBindingId } from "./src/conversation-binding-data.js"; -function createDoctorContext(env: NodeJS.ProcessEnv): PluginDoctorStateMigrationContext { +function createDoctorContext( + env: NodeJS.ProcessEnv, + afterRegister?: () => Promise, +): PluginDoctorStateMigrationContext { return { openPluginStateKeyedStore(options: OpenKeyedStoreOptions) { - return createPluginStateKeyedStoreForTests("codex", { + const store = createPluginStateKeyedStoreForTests("codex", { ...options, env: options.env ?? env, }); + return afterRegister + ? { + ...store, + async registerIfAbsent(...args: Parameters) { + const registered = await store.registerIfAbsent(...args); + await afterRegister(); + return registered; + }, + } + : store; }, }; } +function openBindingStore(env: NodeJS.ProcessEnv) { + return createDoctorContext(env).openPluginStateKeyedStore({ + namespace: CODEX_APP_SERVER_BINDING_NAMESPACE, + maxEntries: CODEX_APP_SERVER_BINDING_MAX_ENTRIES, + overflowPolicy: "reject-new", + }); +} + +async function createBindingMigrationFixture(options: { + binding?: Record; + name: string; + sessionIndex?: Record; + threadId: string; +}) { + const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-doctor-")); + const env = { ...process.env, OPENCLAW_STATE_DIR: stateDir }; + const sessionsDir = path.join(stateDir, "agents", "main", "sessions"); + const transcriptPath = path.join(sessionsDir, `${options.name}.jsonl`); + const sidecarPath = `${transcriptPath}.codex-app-server.json`; + await fs.mkdir(sessionsDir, { recursive: true }); + await fs.writeFile( + transcriptPath, + `${JSON.stringify({ type: "session", id: options.name })}\n`, + "utf8", + ); + if (options.sessionIndex !== undefined) { + await fs.writeFile( + path.join(sessionsDir, "sessions.json"), + JSON.stringify(options.sessionIndex), + "utf8", + ); + } + await fs.writeFile( + sidecarPath, + JSON.stringify({ + schemaVersion: 2, + threadId: options.threadId, + sessionFile: transcriptPath, + updatedAt: "2026-01-01T00:00:00.000Z", + pluginAppPolicyContext: { + fingerprint: "policy-1", + apps: {}, + pluginAppIds: {}, + }, + ...options.binding, + }), + "utf8", + ); + const migration = stateMigrations[0]; + if (!migration) { + throw new Error("missing Codex binding migration"); + } + return { + env, + migration, + params: { + config: {}, + env, + stateDir, + oauthDir: path.join(stateDir, "oauth"), + context: createDoctorContext(env), + }, + sessionsDir, + sidecarPath, + stateDir, + transcriptPath, + }; +} + afterEach(() => { resetPluginStateStoreForTests(); }); @@ -127,31 +210,17 @@ describe("codex doctor contract", () => { }); it("imports and archives shipped binding sidecars", async () => { - const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-doctor-")); - const env = { ...process.env, OPENCLAW_STATE_DIR: stateDir }; - const sessionsDir = path.join(stateDir, "agents", "main", "sessions"); - const transcriptPath = path.join(sessionsDir, "session-current.jsonl"); - const sidecarPath = `${transcriptPath}.codex-app-server.json`; - await fs.mkdir(sessionsDir, { recursive: true }); - await fs.writeFile(transcriptPath, '{"type":"session","id":"session-current"}\n', "utf8"); - await fs.writeFile( - path.join(sessionsDir, "sessions.json"), - JSON.stringify({ + const fixture = await createBindingMigrationFixture({ + name: "session-current", + sessionIndex: { "agent:main:session-1": { sessionId: "session-current", sessionFile: "session-current.jsonl", - updatedAt: Date.now(), + updatedAt: 1, }, - }), - "utf8", - ); - await fs.writeFile( - sidecarPath, - JSON.stringify({ - schemaVersion: 2, - threadId: "thread-1", - sessionFile: transcriptPath, - updatedAt: "2026-01-01T00:00:00.000Z", + }, + threadId: "thread-1", + binding: { pluginAppPolicyContext: { fingerprint: "policy-1", apps: { @@ -166,34 +235,18 @@ describe("codex doctor contract", () => { }, pluginAppIds: {}, }, - }), - "utf8", - ); - const params = { - config: {}, - env, - stateDir, - oauthDir: path.join(stateDir, "oauth"), - context: createDoctorContext(env), - }; - const migration = stateMigrations[0]; - if (!migration) { - throw new Error("missing Codex binding migration"); - } + }, + }); - await expect(migration.detectLegacyState(params)).resolves.toMatchObject({ + await expect(fixture.migration.detectLegacyState(fixture.params)).resolves.toMatchObject({ preview: [expect.stringContaining("legacy sidecar")], }); - await expect(migration.migrateLegacyState(params)).resolves.toMatchObject({ + await expect(fixture.migration.migrateLegacyState(fixture.params)).resolves.toMatchObject({ changes: [expect.stringContaining("Migrated 1")], warnings: [], }); - const store = createDoctorContext(env).openPluginStateKeyedStore({ - namespace: CODEX_APP_SERVER_BINDING_NAMESPACE, - maxEntries: CODEX_APP_SERVER_BINDING_MAX_ENTRIES, - overflowPolicy: "reject-new", - }); + const store = openBindingStore(fixture.env); await expect( store.lookup( bindingStoreKey({ @@ -217,98 +270,472 @@ describe("codex doctor contract", () => { store.lookup( bindingStoreKey({ kind: "conversation", - bindingId: legacyCodexConversationBindingId(transcriptPath), + bindingId: legacyCodexConversationBindingId(fixture.transcriptPath), }), ), ).resolves.toMatchObject({ state: "active", binding: { threadId: "thread-1" } }); - await expect(fs.access(`${sidecarPath}.migrated`)).resolves.toBeUndefined(); + await expect(fs.access(`${fixture.sidecarPath}.migrated`)).resolves.toBeUndefined(); await expect( - fs.readFile(path.join(sessionsDir, "sessions.json"), "utf8").then(JSON.parse), + fs.readFile(path.join(fixture.sessionsDir, "sessions.json"), "utf8").then(JSON.parse), ).resolves.toMatchObject({ "agent:main:session-1": { sessionId: "session-current", agentHarnessId: "codex" }, }); - await fs.rm(stateDir, { recursive: true, force: true }); + await fs.rm(fixture.stateDir, { recursive: true, force: true }); }); - it("reports unresolved-owner binding sidecars as notices after importing conversation binding", async () => { - const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-doctor-")); - const env = { ...process.env, OPENCLAW_STATE_DIR: stateDir }; - const sessionsDir = path.join(stateDir, "agents", "main", "sessions"); - const transcriptPath = path.join(sessionsDir, "orphan.jsonl"); - const sidecarPath = `${transcriptPath}.codex-app-server.json`; - await fs.mkdir(sessionsDir, { recursive: true }); + it("matches an owner through the contained fallback for a stale session file locator", async () => { + const sessionKey = "agent:main:stale-locator"; + const fixture = await createBindingMigrationFixture({ + name: "stale-locator", + sessionIndex: { + [sessionKey]: { + sessionId: "stale-locator", + sessionFile: "../outside.jsonl", + }, + }, + threadId: "thread-stale-locator", + }); + + const result = await fixture.migration.migrateLegacyState(fixture.params); + + expect(result.warnings).toEqual([]); + await expect(fs.access(`${fixture.sidecarPath}.migrated`)).resolves.toBeUndefined(); + await expect( + fs.readFile(path.join(fixture.sessionsDir, "sessions.json"), "utf8").then(JSON.parse), + ).resolves.toMatchObject({ [sessionKey]: { agentHarnessId: "codex" } }); + await expect( + openBindingStore(fixture.env).lookup( + bindingStoreKey({ + kind: "session", + agentId: "main", + sessionId: "stale-locator", + sessionKey, + }), + ), + ).resolves.toMatchObject({ state: "active", binding: { threadId: "thread-stale-locator" } }); + + await fs.rm(fixture.stateDir, { recursive: true, force: true }); + }); + + it("deduplicates session-store aliases before classifying binding ownership", async () => { + const fixture = await createBindingMigrationFixture({ + name: "aliased-store", + sessionIndex: { + "agent:main:aliased-store": { + sessionId: "aliased-store", + sessionFile: "aliased-store.jsonl", + }, + }, + threadId: "thread-aliased-store", + }); await fs.writeFile( - sidecarPath, + path.join(fixture.sessionsDir, "sessions.json"), JSON.stringify({ - schemaVersion: 2, - threadId: "thread-orphan", - sessionFile: transcriptPath, - updatedAt: "2026-01-01T00:00:00.000Z", - pluginAppPolicyContext: { - fingerprint: "policy-1", - apps: {}, - pluginAppIds: {}, + "agent:main:aliased-store": { + sessionId: "aliased-store", + sessionFile: fixture.transcriptPath, }, }), "utf8", ); - const params = { - config: {}, - env, - stateDir, - oauthDir: path.join(stateDir, "oauth"), - context: createDoctorContext(env), - }; - const migration = stateMigrations[0]; - if (!migration) { - throw new Error("missing Codex binding migration"); - } + const storeAlias = path.join(fixture.stateDir, "sessions-alias.json"); + await fs.symlink(path.join(fixture.sessionsDir, "sessions.json"), storeAlias); - const result = await migration.migrateLegacyState(params); - const canonicalSidecarPath = await fs.realpath(sidecarPath); + const result = await fixture.migration.migrateLegacyState({ + ...fixture.params, + config: { session: { store: storeAlias } }, + }); - expect(result.warnings).toStrictEqual([]); - expect(result.notices).toStrictEqual([ - `Left Codex binding sidecar in place after importing its conversation binding because its session owner could not be resolved: ${canonicalSidecarPath}`, - ]); - expect(result.changes).toContain( - "Migrated 1 safe Codex app-server binding row(s) to plugin state; retained legacy sidecars needing review", + expect(result.warnings).toEqual([]); + await expect(fs.access(`${fixture.sidecarPath}.migrated`)).resolves.toBeUndefined(); + const configuredIndex = JSON.parse(await fs.readFile(storeAlias, "utf8")) as Record< + string, + Record + >; + const targetIndex = JSON.parse( + await fs.readFile(path.join(fixture.sessionsDir, "sessions.json"), "utf8"), + ) as Record>; + expect(configuredIndex["agent:main:aliased-store"]).toMatchObject({ + agentHarnessId: "codex", + }); + expect(targetIndex["agent:main:aliased-store"]).not.toHaveProperty("agentHarnessId"); + + await fs.rm(fixture.stateDir, { recursive: true, force: true }); + }); + + it("resolves relative session files from a symlinked store path", async () => { + const sessionKey = "agent:main:symlinked-store"; + const fixture = await createBindingMigrationFixture({ + name: "symlinked-store", + sessionIndex: { + [sessionKey]: { + sessionId: "symlinked-store", + sessionFile: "symlinked-store.jsonl", + }, + }, + threadId: "thread-symlinked-store", + }); + const configuredDir = path.join(fixture.stateDir, "configured-sessions"); + const configuredStore = path.join(configuredDir, "sessions.json"); + const configuredTranscript = path.join(configuredDir, "symlinked-store.jsonl"); + const configuredSidecar = `${configuredTranscript}.codex-app-server.json`; + await fs.mkdir(configuredDir, { recursive: true }); + await fs.rename(fixture.transcriptPath, configuredTranscript); + await fs.rename(fixture.sidecarPath, configuredSidecar); + const sidecar = JSON.parse(await fs.readFile(configuredSidecar, "utf8")) as Record< + string, + unknown + >; + await fs.writeFile( + configuredSidecar, + JSON.stringify({ ...sidecar, sessionFile: configuredTranscript }), + "utf8", ); - await expect(fs.access(sidecarPath)).resolves.toBeUndefined(); - const store = createDoctorContext(env).openPluginStateKeyedStore({ + await fs.symlink(path.join(fixture.sessionsDir, "sessions.json"), configuredStore); + + const result = await fixture.migration.migrateLegacyState({ + ...fixture.params, + config: { session: { store: configuredStore } }, + }); + + expect(result.warnings).toEqual([]); + await expect(fs.access(`${configuredSidecar}.migrated`)).resolves.toBeUndefined(); + await expect(fs.readFile(configuredStore, "utf8").then(JSON.parse)).resolves.toMatchObject({ + [sessionKey]: { agentHarnessId: "codex" }, + }); + + await fs.rm(fixture.stateDir, { recursive: true, force: true }); + }); + + it.each([ + { label: "new", preexisting: false }, + { label: "pre-existing", preexisting: true }, + ])( + "retires a $label session row when its owner rebinds during migration", + async ({ preexisting }) => { + const sessionKey = "agent:main:session-1"; + const fixture = await createBindingMigrationFixture({ + name: "session-current", + sessionIndex: { + [sessionKey]: { + sessionId: "session-current", + sessionFile: "session-current.jsonl", + lifecycleRevision: "rev-1", + }, + }, + threadId: "thread-1", + }); + const sessionBindingKey = bindingStoreKey({ + kind: "session", + agentId: "main", + sessionId: "session-current", + sessionKey, + }); + const imported = createStoredCodexAppServerBinding( + JSON.parse(await fs.readFile(fixture.sidecarPath, "utf8")), + ); + if (!imported) { + throw new Error("missing imported Codex binding"); + } + const store = openBindingStore(fixture.env); + if (preexisting) { + await store.register(sessionBindingKey, { ...imported, sessionId: "session-current" }); + } + let rebound = false; + const context = createDoctorContext(fixture.env, async () => { + if (rebound) { + return; + } + rebound = true; + await fs.writeFile( + path.join(fixture.sessionsDir, "sessions.json"), + JSON.stringify({ + [sessionKey]: { + sessionId: "session-current", + sessionFile: "replacement.jsonl", + lifecycleRevision: "rev-2", + }, + }), + ); + }); + + const result = await fixture.migration.migrateLegacyState({ ...fixture.params, context }); + + expect(result.warnings).toEqual([ + expect.stringContaining("session owner changed before Codex ownership could be recorded"), + ]); + await expect(fs.access(fixture.sidecarPath)).resolves.toBeUndefined(); + await expect(fs.access(`${fixture.sidecarPath}.migrated`)).rejects.toThrow(); + await expect( + fs.readFile(path.join(fixture.sessionsDir, "sessions.json"), "utf8").then(JSON.parse), + ).resolves.not.toHaveProperty(`${sessionKey}.agentHarnessId`); + await expect(store.lookup(sessionBindingKey)).resolves.toMatchObject({ + version: 1, + state: "cleared", + sessionId: "session-current", + retired: true, + }); + + await fs.rm(fixture.stateDir, { recursive: true, force: true }); + }, + ); + + it("does not resurrect a retired session generation from its legacy sidecar", async () => { + const sessionKey = "agent:main:retired"; + const fixture = await createBindingMigrationFixture({ + name: "retired", + sessionIndex: { + [sessionKey]: { + sessionId: "retired", + sessionFile: "retired.jsonl", + }, + }, + threadId: "thread-retired", + }); + const store = openBindingStore(fixture.env); + const active = createStoredCodexAppServerBinding( + JSON.parse(await fs.readFile(fixture.sidecarPath, "utf8")), + ); + if (!active) { + throw new Error("missing imported Codex binding"); + } + await store.register( + bindingStoreKey({ + kind: "conversation", + bindingId: legacyCodexConversationBindingId(fixture.transcriptPath), + }), + active, + ); + const sessionBindingKey = bindingStoreKey({ + kind: "session", + agentId: "main", + sessionId: "retired", + sessionKey, + }); + const retired: StoredCodexAppServerBinding = { + version: 1, + state: "cleared", + sessionId: "retired", + retired: true, + }; + await store.register(sessionBindingKey, retired); + + const result = await fixture.migration.migrateLegacyState(fixture.params); + + expect(result.changes).toEqual([]); + expect(result.warnings).toEqual([ + expect.stringContaining(`canonical plugin state changed at ${sessionBindingKey}`), + ]); + await expect(fs.access(fixture.sidecarPath)).resolves.toBeUndefined(); + await expect(store.lookup(sessionBindingKey)).resolves.toEqual(retired); + await expect( + fs.readFile(path.join(fixture.sessionsDir, "sessions.json"), "utf8").then(JSON.parse), + ).resolves.not.toHaveProperty(`${sessionKey}.agentHarnessId`); + + await fs.rm(fixture.stateDir, { recursive: true, force: true }); + }); + + it.each(["active", "cleared"] as const)( + "archives zero-owner sidecars without changing imported $state conversation state", + async (state) => { + const fixture = await createBindingMigrationFixture({ + name: `orphan-${state}`, + threadId: "thread-orphan", + }); + const bindingKey = bindingStoreKey({ + kind: "conversation", + bindingId: legacyCodexConversationBindingId(fixture.transcriptPath), + }); + const active = createStoredCodexAppServerBinding( + JSON.parse(await fs.readFile(fixture.sidecarPath, "utf8")), + ); + if (!active) { + throw new Error("missing imported Codex binding"); + } + const existing: StoredCodexAppServerBinding = + state === "active" ? active : { version: 1, state: "cleared", retired: true }; + const store = openBindingStore(fixture.env); + await store.register(bindingKey, existing); + + await expect(fixture.migration.migrateLegacyState(fixture.params)).resolves.toEqual({ + changes: [ + "Migrated 1 Codex app-server binding sidecar(s) to plugin state and archived the legacy sources", + ], + warnings: [], + }); + await expect(fs.access(fixture.sidecarPath)).rejects.toThrow(); + await expect(fs.access(`${fixture.sidecarPath}.migrated`)).resolves.toBeUndefined(); + await expect(store.lookup(bindingKey)).resolves.toEqual(existing); + await expect(fixture.migration.detectLegacyState(fixture.params)).resolves.toBeNull(); + await expect(fixture.migration.migrateLegacyState(fixture.params)).resolves.toEqual({ + changes: [], + warnings: [], + }); + + await fs.rm(fixture.stateDir, { recursive: true, force: true }); + }, + ); + + it("retains a zero-owner sidecar when canonical plugin state is malformed", async () => { + const fixture = await createBindingMigrationFixture({ + name: "orphan-invalid-state", + threadId: "thread-orphan", + }); + const bindingKey = bindingStoreKey({ + kind: "conversation", + bindingId: legacyCodexConversationBindingId(fixture.transcriptPath), + }); + const store = createDoctorContext(fixture.env).openPluginStateKeyedStore({ namespace: CODEX_APP_SERVER_BINDING_NAMESPACE, maxEntries: CODEX_APP_SERVER_BINDING_MAX_ENTRIES, overflowPolicy: "reject-new", }); - await expect( - store.lookup( - bindingStoreKey({ - kind: "conversation", - bindingId: legacyCodexConversationBindingId(transcriptPath), - }), - ), - ).resolves.toMatchObject({ state: "active", binding: { threadId: "thread-orphan" } }); + const malformed = { version: 1, state: "active" }; + await store.register(bindingKey, malformed); - await fs.rm(stateDir, { recursive: true, force: true }); + const result = await fixture.migration.migrateLegacyState(fixture.params); + + expect(result.changes).toEqual([]); + expect(result.warnings).toEqual([ + expect.stringContaining(`canonical plugin state is invalid at ${bindingKey}`), + ]); + await expect(fs.access(fixture.sidecarPath)).resolves.toBeUndefined(); + await expect(store.lookup(bindingKey)).resolves.toEqual(malformed); + + await fs.rm(fixture.stateDir, { recursive: true, force: true }); }); - it("does not scan above stateDir when a session store sits at its parent", async () => { + it("retains mixed Codex and foreign ambiguous binding owners", async () => { + const fixture = await createBindingMigrationFixture({ + name: "shared", + sessionIndex: { + "agent:main:first": { + sessionId: "first", + sessionFile: "shared.jsonl", + agentHarnessId: "codex", + }, + "agent:main:second": { + sessionId: "second", + sessionFile: "shared.jsonl", + agentHarnessId: "pi", + }, + }, + threadId: "thread-shared", + }); + + const result = await fixture.migration.migrateLegacyState(fixture.params); + + expect(result.changes).toEqual([]); + expect(result.warnings).toHaveLength(1); + expect(result.warnings[0]).toContain("2 matching session owners make ownership ambiguous"); + await expect(fs.access(fixture.sidecarPath)).resolves.toBeUndefined(); + await expect(openBindingStore(fixture.env).entries()).resolves.toEqual([]); + + await fs.rm(fixture.stateDir, { recursive: true, force: true }); + }); + + it("retains a sidecar owned by a foreign harness without importing plugin state", async () => { + const fixture = await createBindingMigrationFixture({ + name: "foreign", + sessionIndex: { + "agent:main:foreign": { + sessionId: "foreign", + sessionFile: "foreign.jsonl", + agentHarnessId: "pi", + }, + }, + threadId: "thread-foreign", + }); + + const result = await fixture.migration.migrateLegacyState(fixture.params); + + expect(result.changes).toEqual([]); + expect(result.warnings).toEqual([expect.stringContaining("owned by agent harness pi")]); + await expect(fs.access(fixture.sidecarPath)).resolves.toBeUndefined(); + await expect(openBindingStore(fixture.env).entries()).resolves.toEqual([]); + + await fs.rm(fixture.stateDir, { recursive: true, force: true }); + }); + + it.each([ + { contents: "{", detail: "invalid JSON", label: "invalid JSON" }, + { + contents: JSON.stringify({ + "agent:main:invalid": { sessionId: "invalid", agentHarnessId: 42 }, + }), + detail: "invalid entries", + label: "malformed harness metadata", + }, + { + contents: JSON.stringify({ + "agent:main:unsafe": { sessionId: "../unsafe", sessionFile: "unsafe.jsonl" }, + }), + detail: "invalid entries", + label: "unsafe session id", + }, + ])("retains binding sidecars for an indeterminate $label index", async ({ contents, detail }) => { + const fixture = await createBindingMigrationFixture({ + name: "unknown-owner", + threadId: "thread-unknown-owner", + }); + const externalDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-store-")); + const externalStore = path.join(externalDir, "sessions.json"); + await fs.writeFile(externalStore, contents, "utf8"); + const params = { + ...fixture.params, + config: { session: { store: externalStore } }, + }; + + const result = await fixture.migration.migrateLegacyState(params); + + expect(result.changes).toEqual([]); + expect(result.warnings).toHaveLength(1); + expect(result.warnings[0]).toContain("session index"); + expect(result.warnings[0]).toContain(detail); + await expect(fs.access(fixture.sidecarPath)).resolves.toBeUndefined(); + await expect(fs.access(`${fixture.sidecarPath}.migrated`)).rejects.toThrow(); + await expect(openBindingStore(fixture.env).entries()).resolves.toEqual([]); + + await Promise.all([ + fs.rm(fixture.stateDir, { recursive: true, force: true }), + fs.rm(externalDir, { recursive: true, force: true }), + ]); + }); + + it("does not scan above stateDir or follow escaped external store locators", async () => { const outerDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-doctor-outer-")); + const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-doctor-outside-")); const stateDir = path.join(outerDir, "state"); await fs.mkdir(stateDir, { recursive: true }); const strayDir = path.join(outerDir, "unrelated"); await fs.mkdir(strayDir, { recursive: true }); + const externalStore = path.join(outerDir, "sessions.json"); await fs.writeFile( path.join(strayDir, "foreign.jsonl.codex-app-server.json"), JSON.stringify({ schemaVersion: 2, threadId: "thread-foreign" }), "utf8", ); + await fs.writeFile( + path.join(outsideDir, "foreign.jsonl.codex-app-server.json"), + JSON.stringify({ schemaVersion: 2, threadId: "thread-escaped" }), + "utf8", + ); + await fs.writeFile( + externalStore, + JSON.stringify({ + "agent:main:foreign": { + sessionId: "foreign", + sessionFile: path.join(outsideDir, "foreign.jsonl"), + }, + }), + "utf8", + ); const env = { ...process.env, OPENCLAW_STATE_DIR: stateDir }; const params = { - // The store dir is exactly the parent of stateDir; doctor must treat it - // as an external store (indexed reads only), not a scannable state root. - config: { session: { store: path.join(outerDir, "sessions.json") } }, + // The store directory is exactly stateDir's parent. It stays indexed-only, + // and its explicit locator cannot escape that directory. + config: { session: { store: externalStore } }, env, stateDir, oauthDir: path.join(stateDir, "oauth"), @@ -321,7 +748,10 @@ describe("codex doctor contract", () => { await expect(migration.detectLegacyState(params)).resolves.toBeNull(); - await fs.rm(outerDir, { recursive: true, force: true }); + await Promise.all([ + fs.rm(outerDir, { recursive: true, force: true }), + fs.rm(outsideDir, { recursive: true, force: true }), + ]); }); it("renames old approval-routed destructive plugin policy values", () => { diff --git a/extensions/codex/src/app-server/session-binding.ts b/extensions/codex/src/app-server/session-binding.ts index 5da18ee96716..3187a0f76a7d 100644 --- a/extensions/codex/src/app-server/session-binding.ts +++ b/extensions/codex/src/app-server/session-binding.ts @@ -379,7 +379,7 @@ export function createCodexAppServerBindingStore( try { let renewed = false; const stored = update(key, (raw) => { - const current = readStoredBinding(raw); + const current = readStoredCodexAppServerBinding(raw); if (raw !== undefined && !current) { throw new Error(`Invalid Codex app-server binding row: ${key}`); } @@ -426,7 +426,7 @@ export function createCodexAppServerBindingStore( update( key, (raw) => { - const current = readStoredBinding(raw); + const current = readStoredCodexAppServerBinding(raw); if (raw !== undefined && !current) { throw new Error(`Invalid Codex app-server binding row: ${key}`); } @@ -470,7 +470,7 @@ export function createCodexAppServerBindingStore( async read(identity) { const key = bindingStoreKey(identity); const raw = state.lookup(key); - const stored = readStoredBinding(raw); + const stored = readStoredCodexAppServerBinding(raw); if (raw !== undefined && !stored) { throw new Error(`Invalid Codex app-server binding row: ${key}`); } @@ -482,7 +482,7 @@ export function createCodexAppServerBindingStore( async prepareSessionGenerationReclaim(identity) { const key = bindingStoreKey(identity); const raw = state.lookup(key); - const current = readStoredBinding(raw); + const current = readStoredCodexAppServerBinding(raw); if (raw !== undefined && !current) { throw new Error(`Invalid Codex app-server binding row: ${key}`); } @@ -723,7 +723,7 @@ export function createCodexAppServerBindingStore( raw: unknown, matches: (current: StoredCodexAppServerBinding) => boolean, ) => { - const current = readStoredBinding(raw); + const current = readStoredCodexAppServerBinding(raw); if (!current || !matches(current) || current.lease?.token !== token) { return undefined; } @@ -789,7 +789,9 @@ export function bindingStoreKey(identity: CodexAppServerBindingIdentity): string return `conversation:${bindingId}`; } -function readStoredBinding(value: unknown): StoredCodexAppServerBinding | undefined { +export function readStoredCodexAppServerBinding( + value: unknown, +): StoredCodexAppServerBinding | undefined { const result = storedBindingSchema.safeParse(value); return result.success ? (stripUndefinedValue(result.data) as StoredCodexAppServerBinding) diff --git a/extensions/codex/src/migration/session-binding-sidecars.ts b/extensions/codex/src/migration/session-binding-sidecars.ts index f241d5a5d747..4c4019eea230 100644 --- a/extensions/codex/src/migration/session-binding-sidecars.ts +++ b/extensions/codex/src/migration/session-binding-sidecars.ts @@ -50,14 +50,26 @@ type LegacyBindingOwner = { sessionKey: string; storePath: string; transcriptPath: string; + lifecycleRevision?: string; agentHarnessId?: string; }; +type LegacySessionIndexEntry = { + sessionId: string; + sessionFile?: string; + lifecycleRevision?: string; + agentHarnessId?: string; +}; + +type BindingOwnerCollection = { + owners: Map; + failures: string[]; +}; + type SourceMigrationResult = { archived: boolean; importedKeys: number; warning?: string; - notice?: string; }; // Keep the doctor contract graph independent from the full Codex runtime. @@ -88,6 +100,8 @@ async function collectSessionSurfaces(params: MigrationEnvironment): Promise(), }; surface.scan ||= scan; + // A store's configured path defines how relative sessionFile locators are + // resolved. Keep it intact; canonicalize only when deduplicating aliases. surface.storePaths.add(path.resolve(storePath)); surface.agentIds.add(agentId); surfaces.set(canonicalRoot, surface); @@ -159,22 +173,80 @@ async function collectLegacyBindingSources( }; } +async function readLegacySessionIndex( + storePath: string, +): Promise< + { entries: Array<{ sessionKey: string; entry: LegacySessionIndexEntry }> } | { failure: string } +> { + let contents: string; + try { + contents = await fs.readFile(storePath, "utf8"); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + return code === "ENOENT" + ? { entries: [] } + : { failure: `session index ${storePath} could not be read${code ? ` (${code})` : ""}` }; + } + let raw: unknown; + try { + raw = JSON.parse(contents); + } catch { + return { failure: `session index ${storePath} could not be read (invalid JSON)` }; + } + if (!isRecord(raw)) { + return { failure: `session index ${storePath} has invalid entries` }; + } + let normalizedEntries: ReturnType; + try { + normalizedEntries = listSessionEntries({ storePath, hydrateSkillPromptRefs: false }); + } catch { + return { failure: `session index ${storePath} could not be normalized` }; + } + const normalizedByKey = new Map( + normalizedEntries.map(({ sessionKey, entry }) => [sessionKey, entry] as const), + ); + const entries: Array<{ sessionKey: string; entry: LegacySessionIndexEntry }> = []; + for (const [sessionKey, value] of Object.entries(raw)) { + if (!isRecord(value)) { + return { failure: `session index ${storePath} has invalid entries` }; + } + const rawSessionId = typeof value.sessionId === "string" ? value.sessionId.trim() : ""; + const sessionId = normalizedByKey.get(sessionKey)?.sessionId?.trim() ?? ""; + const sessionFile = value.sessionFile; + const lifecycleRevision = value.lifecycleRevision; + const agentHarnessId = value.agentHarnessId; + if ( + !sessionId || + sessionId !== rawSessionId || + (sessionFile !== undefined && typeof sessionFile !== "string") || + (lifecycleRevision !== undefined && typeof lifecycleRevision !== "string") || + (agentHarnessId !== undefined && typeof agentHarnessId !== "string") + ) { + return { failure: `session index ${storePath} has invalid entries` }; + } + entries.push({ + sessionKey, + entry: { + sessionId, + ...(typeof sessionFile === "string" ? { sessionFile } : {}), + ...(typeof lifecycleRevision === "string" ? { lifecycleRevision } : {}), + ...(typeof agentHarnessId === "string" ? { agentHarnessId } : {}), + }, + }); + } + return { entries }; +} + async function* iterateIndexedSidecars( surface: SessionSurface, params: MigrationEnvironment, ): AsyncGenerator { for (const storePath of surface.storePaths) { - let entries: ReturnType; - try { - entries = listSessionEntries({ storePath, hydrateSkillPromptRefs: false }); - } catch { + const index = await readLegacySessionIndex(storePath); + if ("failure" in index) { continue; } - for (const { sessionKey, entry } of entries) { - const sessionId = entry.sessionId?.trim(); - if (!sessionId) { - continue; - } + for (const { sessionKey, entry } of index.entries) { const agentId = resolveLegacyBindingOwnerAgentId({ sessionKey, config: params.config, @@ -182,7 +254,7 @@ async function* iterateIndexedSidecars( }); let transcriptPath: string; try { - transcriptPath = resolveSessionFilePath(sessionId, entry, { + transcriptPath = resolveSessionFilePath(entry.sessionId, entry, { sessionsDir: path.dirname(storePath), agentId, }); @@ -221,7 +293,7 @@ async function collectBindingOwners( sources: LegacyBindingSource[], surfaces: SessionSurface[], params: MigrationEnvironment, -): Promise> { +): Promise { const sourcePaths = new Set( await Promise.all(sources.map((source) => canonicalizePath(source.transcriptPath))), ); @@ -237,34 +309,33 @@ async function collectBindingOwners( storeAgentIds.set(storePath, agents); } } + const failures: string[] = []; for (const storePath of storePaths) { - let entries: ReturnType; - try { - entries = listSessionEntries({ storePath, hydrateSkillPromptRefs: false }); - } catch { + const canonicalStorePath = await canonicalizePath(storePath); + const index = await readLegacySessionIndex(storePath); + if ("failure" in index) { + failures.push(index.failure); continue; } const sessionsDir = path.dirname(storePath); - for (const { sessionKey, entry } of entries) { - const sessionId = entry.sessionId?.trim(); - if (!sessionId) { - continue; - } + for (const { sessionKey, entry } of index.entries) { + const sessionId = entry.sessionId; const agentId = resolveLegacyBindingOwnerAgentId({ sessionKey, config: params.config, storeAgentIds: storeAgentIds.get(storePath), }); - let transcriptPath: string; - let legacyTranscriptPath: string; + let effectiveTranscriptPath: string; try { - legacyTranscriptPath = resolveLegacySessionFileLocator(sessionsDir, entry, sessionId); - transcriptPath = await canonicalizePath( - resolveSessionFilePath(sessionId, entry, { sessionsDir, agentId }), - ); + effectiveTranscriptPath = resolveSessionFilePath(sessionId, entry, { + sessionsDir, + agentId, + }); } catch { + failures.push(`session index ${storePath} has an invalid locator for ${sessionKey}`); continue; } + const transcriptPath = await canonicalizePath(effectiveTranscriptPath); if (!sourcePaths.has(transcriptPath)) { continue; } @@ -273,24 +344,28 @@ async function collectBindingOwners( sessionId, sessionKey, storePath, - transcriptPath: legacyTranscriptPath, + transcriptPath: effectiveTranscriptPath, + ...(entry.lifecycleRevision ? { lifecycleRevision: entry.lifecycleRevision } : {}), ...(entry.agentHarnessId?.trim() ? { agentHarnessId: entry.agentHarnessId.trim() } : {}), }; const candidates = owners.get(transcriptPath) ?? new Map(); - candidates.set(`${agentId}\0${sessionId}\0${sessionKey}\0${storePath}`, owner); + const ownerKey = `${agentId}\0${sessionId}\0${sessionKey}\0${canonicalStorePath}`; + const configuredStorePath = resolveStorePath(params.config.session?.store, { + agentId, + env: params.env, + }); + // The same physical store can appear through a configured symlink and a + // discovered real path. Mutate through the path the runtime itself owns. + if (!candidates.has(ownerKey) || storePath === configuredStorePath) { + candidates.set(ownerKey, owner); + } owners.set(transcriptPath, candidates); } } - return new Map([...owners].map(([key, values]) => [key, [...values.values()]])); -} - -function resolveLegacySessionFileLocator( - sessionsDir: string, - entry: { sessionFile?: string }, - sessionId: string, -): string { - const sessionFile = entry.sessionFile?.trim(); - return path.resolve(sessionsDir, sessionFile || `${sessionId}.jsonl`); + return { + owners: new Map([...owners].map(([key, values]) => [key, [...values.values()]])), + failures, + }; } function resolveLegacyBindingOwnerAgentId(params: { @@ -325,7 +400,7 @@ function copyBindingForSession(stored: MigratedBindingRow, sessionId: string): M async function migrateSource( source: LegacyBindingSource, - owner: LegacyBindingOwner | undefined, + candidates: LegacyBindingOwner[], params: MigrationParams, store: PluginStateKeyedStore, ): Promise { @@ -335,6 +410,7 @@ async function migrateSource( importedKeys, warning: `Left Codex binding sidecar in place because ${reason}: ${source.sidecarPath}`, }); + const owner = candidates.length === 1 ? candidates[0] : undefined; try { return await withFileLock(source.sidecarPath, LEGACY_BINDING_LOCK_OPTIONS, async () => { const [contents, stat] = await Promise.all([ @@ -343,7 +419,7 @@ async function migrateSource( ]); const raw = JSON.parse(contents) as Record; const [ - { bindingStoreKey, createStoredCodexAppServerBinding, readCodexAppServerThreadBinding }, + { bindingStoreKey, createStoredCodexAppServerBinding, readStoredCodexAppServerBinding }, { legacyCodexConversationBindingId }, ] = await Promise.all([ import("../app-server/session-binding.js"), @@ -361,10 +437,14 @@ async function migrateSource( if (!baseStored) { return retain("its binding is invalid"); } + if (candidates.length > 1) { + // The legacy writer keyed one sidecar to one active session file. Multiple + // current owners are indeterminate, so preserve the source without writes. + return retain(`${candidates.length} matching session owners make ownership ambiguous`); + } if (owner?.agentHarnessId && owner.agentHarnessId !== CODEX_AGENT_HARNESS_ID) { return retain(`its session is owned by agent harness ${owner.agentHarnessId}`); } - const sourceSessionFile = typeof raw.sessionFile === "string" && raw.sessionFile.trim() ? raw.sessionFile @@ -384,12 +464,17 @@ async function migrateSource( ); let currentConversation: MigratedBindingRow | undefined; for (const key of conversationKeys) { - currentConversation ??= await store.lookup(key); + const current = await store.lookup(key); + if (current === undefined) { + continue; + } + const parsed = readStoredCodexAppServerBinding(current); + if (!parsed) { + return retain(`canonical plugin state is invalid at ${key}`); + } + currentConversation ??= parsed; } const stored = currentConversation ?? baseStored; - if (stored.state !== "active" && stored.state !== "cleared") { - return retain(`canonical plugin state changed at ${conversationKeys[0]}`); - } const sessionKey = owner ? bindingStoreKey({ kind: "session", @@ -398,21 +483,25 @@ async function migrateSource( sessionKey: owner.sessionKey, }) : undefined; - const entries = [ - ...conversationKeys.map((key) => ({ key, value: stored })), - ...(owner && sessionKey - ? [{ key: sessionKey, value: copyBindingForSession(stored, owner.sessionId) }] - : []), - ]; - const hasExpected = (value: MigratedBindingRow | undefined, expected: MigratedBindingRow) => - expected.state === "cleared" - ? value?.state === "cleared" && - value.sessionId === expected.sessionId && - value.retired === expected.retired - : value?.state === "active" && - value.sessionId === expected.sessionId && - isDeepStrictEqual(readCodexAppServerThreadBinding(value.binding), expected.binding); - + const conversationEntries = conversationKeys.map((key) => ({ key, value: stored })); + const sessionEntry = + owner && sessionKey + ? { key: sessionKey, value: copyBindingForSession(stored, owner.sessionId) } + : undefined; + const entries = [...conversationEntries, ...(sessionEntry ? [sessionEntry] : [])]; + const hasExpected = (value: MigratedBindingRow | undefined, target: MigratedBindingRow) => { + const parsed = readStoredCodexAppServerBinding(value); + if (!parsed) { + return false; + } + return target.state === "cleared" + ? parsed.state === "cleared" && + parsed.sessionId === target.sessionId && + parsed.retired === target.retired + : parsed.state === "active" && + parsed.sessionId === target.sessionId && + isDeepStrictEqual(parsed.binding, target.binding); + }; for (const entry of entries) { const current = await store.lookup(entry.key); if (current !== undefined && !hasExpected(current, entry.value)) { @@ -427,45 +516,98 @@ async function migrateSource( return retain(`canonical plugin state changed at ${entry.key}`); } } - if (!owner) { - return { - archived: false, - importedKeys, - notice: `Left Codex binding sidecar in place after importing its conversation binding because its session owner could not be resolved: ${source.sidecarPath}`, - }; - } - const ownershipWarning = await recordSessionOwner(owner); - if (ownershipWarning) { - return retain(ownershipWarning); - } - for (const entry of entries) { - if (!hasExpected(await store.lookup(entry.key), entry.value)) { - return retain(`canonical plugin state changed at ${entry.key}`); + if (owner) { + const ownershipWarning = await recordSessionOwner(owner); + if (ownershipWarning) { + if (sessionEntry?.value.state === "active") { + const update = store.update; + if (!update) { + return retain(`${ownershipWarning}; its stale session binding could not be retired`); + } + await update(sessionEntry.key, (current) => { + const parsed = readStoredCodexAppServerBinding(current); + if (parsed?.lease && parsed.lease.expiresAt > Date.now()) { + return undefined; + } + if (!hasExpected(current, sessionEntry.value)) { + // Atomic no-op: a concurrent runtime owner replaced or removed this row. + return undefined; + } + return { + version: 1, + state: "cleared", + sessionId: owner.sessionId, + retired: true, + }; + }); + if (hasExpected(await store.lookup(sessionEntry.key), sessionEntry.value)) { + return retain(`${ownershipWarning}; its stale session binding could not be retired`); + } + } + return retain(ownershipWarning); + } + for (const entry of entries) { + if (!hasExpected(await store.lookup(entry.key), entry.value)) { + return retain(`canonical plugin state changed at ${entry.key}`); + } } } + // Legacy writers only created sidecars for an existing session file. Once + // unique ownership is recorded, or zero ownership is proven, it is safe to archive. await archiveBindingSidecar(source.sidecarPath); return { archived: true, importedKeys }; }); } catch (error) { + // Parallel doctor runs can both discover a source before the first archives it. + if ( + (error as NodeJS.ErrnoException).code === "ENOENT" && + !(await pathExists(source.sidecarPath)) + ) { + return { archived: true, importedKeys }; + } return retain(`migration or archiving failed: ${String(error)}`); } } async function recordSessionOwner(owner: LegacyBindingOwner): Promise { + let observedForeignHarness: string | undefined; const updated = await updateSessionStoreEntry({ storePath: owner.storePath, sessionKey: owner.sessionKey, skipMaintenance: true, requireWriteSuccess: true, update: (entry) => { - if (entry.sessionId.trim() !== owner.sessionId) { + const transcriptPath = resolveOwnerTranscriptPath(owner, entry); + if ( + entry.sessionId.trim() !== owner.sessionId || + transcriptPath !== owner.transcriptPath || + entry.lifecycleRevision !== owner.lifecycleRevision + ) { return null; } - const harnessId = entry.agentHarnessId?.trim(); - return harnessId ? null : { agentHarnessId: CODEX_AGENT_HARNESS_ID }; + const harnessId = + typeof entry.agentHarnessId === "string" ? entry.agentHarnessId.trim() : undefined; + if (entry.agentHarnessId !== undefined && harnessId === undefined) { + return null; + } + if (harnessId && harnessId !== CODEX_AGENT_HARNESS_ID) { + observedForeignHarness = harnessId; + return null; + } + return { agentHarnessId: CODEX_AGENT_HARNESS_ID }; }, }); - if (!updated || updated.sessionId.trim() !== owner.sessionId) { + if (!updated) { + return observedForeignHarness + ? `its session is owned by agent harness ${observedForeignHarness}` + : "its session owner changed before Codex ownership could be recorded"; + } + const transcriptPath = resolveOwnerTranscriptPath(owner, updated); + if ( + updated.sessionId.trim() !== owner.sessionId || + transcriptPath !== owner.transcriptPath || + updated.lifecycleRevision !== owner.lifecycleRevision + ) { return "its session owner changed before Codex ownership could be recorded"; } const harnessId = updated.agentHarnessId?.trim(); @@ -476,6 +618,20 @@ async function recordSessionOwner(owner: LegacyBindingOwner): Promise { } } +function isRecord(value: unknown): value is Record { + return value !== null && typeof value === "object" && !Array.isArray(value); +} + function isPathWithin(root: string, candidate: string): boolean { const relative = path.relative(root, candidate); // Bare ".." (candidate is root's parent) must stay outside; treating it as @@ -567,12 +727,17 @@ export const stateMigrations: PluginDoctorStateMigration[] = [ async migrateLegacyState(params) { const changes: string[] = []; const warnings: string[] = []; - const notices: string[] = []; const { sources, surfaces } = await collectLegacyBindingSources(params); if (sources.length === 0) { return { changes, warnings }; } - const owners = await collectBindingOwners(sources, surfaces, params); + const ownerCollection = await collectBindingOwners(sources, surfaces, params); + if (ownerCollection.failures.length > 0) { + warnings.push( + `Left ${sources.length} Codex binding sidecar(s) in place because session ownership is indeterminate: ${ownerCollection.failures.join("; ")}`, + ); + return { changes, warnings }; + } const store = params.context.openPluginStateKeyedStore({ namespace: CODEX_APP_SERVER_BINDING_NAMESPACE, maxEntries: CODEX_APP_SERVER_BINDING_MAX_ENTRIES, @@ -581,15 +746,12 @@ export const stateMigrations: PluginDoctorStateMigration[] = [ let migrated = 0; let partialImports = 0; for (const source of sources) { - const candidates = owners.get(await canonicalizePath(source.transcriptPath)) ?? []; - const owner = candidates.length === 1 ? candidates[0] : undefined; - const result = await migrateSource(source, owner, params, store); + const candidates = + ownerCollection.owners.get(await canonicalizePath(source.transcriptPath)) ?? []; + const result = await migrateSource(source, candidates, params, store); if (result.warning) { warnings.push(result.warning); } - if (result.notice) { - notices.push(result.notice); - } if (result.archived) { migrated++; } else { @@ -606,7 +768,7 @@ export const stateMigrations: PluginDoctorStateMigration[] = [ `Migrated ${partialImports} safe Codex app-server binding row(s) to plugin state; retained legacy sidecars needing review`, ); } - return notices.length > 0 ? { changes, warnings, notices } : { changes, warnings }; + return { changes, warnings }; }, }, ];