From 10dde452f90fc3d4d1aea0add67d13cdfb2567ab Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 5 Mar 2026 22:29:26 -0800 Subject: [PATCH] fix: address core review findings Co-authored-by: codex --- .../OrchestrationEngineHarness.integration.ts | 4 +- .../orchestrationEngine.integration.test.ts | 18 ++--- apps/server/src/codexAppServerManager.test.ts | 78 ++++++++++++++++++- apps/server/src/codexAppServerManager.ts | 17 ++-- apps/server/src/main.ts | 2 - .../src/provider/Layers/CodexAdapter.test.ts | 72 +++++++++++++++++ .../src/provider/Layers/CodexAdapter.ts | 5 +- .../Layers/ProviderSessionDirectory.test.ts | 30 +++++++ .../Layers/ProviderSessionDirectory.ts | 6 +- packages/contracts/src/orchestration.test.ts | 20 +++++ packages/contracts/src/orchestration.ts | 2 +- packages/shared/src/model.test.ts | 5 ++ packages/shared/src/model.ts | 3 +- 13 files changed, 232 insertions(+), 30 deletions(-) diff --git a/apps/server/integration/OrchestrationEngineHarness.integration.ts b/apps/server/integration/OrchestrationEngineHarness.integration.ts index 33e29dfec2341f91ce6b2ef4bae7dffe06d13a4d..66057407c62637514e7485707354cb4d7ec12d30 100644 --- a/apps/server/integration/OrchestrationEngineHarness.integration.ts +++ b/apps/server/integration/OrchestrationEngineHarness.integration.ts @@ -152,7 +152,7 @@ export interface OrchestrationIntegrationHarness { readonly rootDir: string; readonly workspaceDir: string; readonly dbPath: string; - readonly adapterHarness: TestProviderAdapterHarness; + readonly adapterHarness: TestProviderAdapterHarness | null; readonly engine: OrchestrationEngineShape; readonly snapshotQuery: ProjectionSnapshotQuery["Service"]; readonly providerService: ProviderService["Service"]; @@ -435,7 +435,7 @@ export const makeOrchestrationIntegrationHarness = ( rootDir, workspaceDir, dbPath, - adapterHarness: adapterHarness as TestProviderAdapterHarness, + adapterHarness, engine, snapshotQuery, providerService, diff --git a/apps/server/integration/orchestrationEngine.integration.test.ts b/apps/server/integration/orchestrationEngine.integration.test.ts index 2ad353188bcaa5c9f06a6bb2377bc3fa601b0dce..3b0a3a4002f819703db8b0cd663b3fbdb1509cbf 100644 --- a/apps/server/integration/orchestrationEngine.integration.test.ts +++ b/apps/server/integration/orchestrationEngine.integration.test.ts @@ -180,7 +180,7 @@ it.live("runs a single turn end-to-end and persists checkpoint state in sqlite + ], }; - yield* harness.adapterHarness.queueTurnResponseForNextSession(turnResponse); + yield* harness.adapterHarness!.queueTurnResponseForNextSession(turnResponse); yield* startTurn({ harness, commandId: "cmd-turn-start-single", @@ -314,7 +314,7 @@ it.live("runs multi-turn file edits and persists checkpoint diffs", () => Effect.gen(function* () { yield* seedProjectAndThread(harness); - yield* harness.adapterHarness.queueTurnResponseForNextSession({ + yield* harness.adapterHarness!.queueTurnResponseForNextSession({ events: [ { type: "turn.started", @@ -373,7 +373,7 @@ it.live("runs multi-turn file edits and persists checkpoint diffs", () => (entry) => entry.checkpoints.length === 1 && entry.session?.threadId === "thread-1", ); - yield* harness.adapterHarness.queueTurnResponse(THREAD_ID, { + yield* harness.adapterHarness!.queueTurnResponse(THREAD_ID, { events: [ { type: "turn.started", @@ -473,7 +473,7 @@ it.live("tracks approval requests and resolves pending approvals on user respons Effect.gen(function* () { yield* seedProjectAndThread(harness); - yield* harness.adapterHarness.queueTurnResponseForNextSession({ + yield* harness.adapterHarness!.queueTurnResponseForNextSession({ events: [ { type: "turn.started", @@ -538,7 +538,7 @@ it.live("tracks approval requests and resolves pending approvals on user respons assert.equal(resolvedRow.decision, "accept"); const approvalResponses = yield* waitForSync( - () => harness.adapterHarness.getApprovalResponses(THREAD_ID), + () => harness.adapterHarness!.getApprovalResponses(THREAD_ID), (responses) => responses.length === 1, "provider approval response", ); @@ -554,7 +554,7 @@ it.live("records failed turn runtime state and checkpoint status as error", () = Effect.gen(function* () { yield* seedProjectAndThread(harness); - yield* harness.adapterHarness.queueTurnResponseForNextSession({ + yield* harness.adapterHarness!.queueTurnResponseForNextSession({ events: [ { type: "turn.started", @@ -633,7 +633,7 @@ it.live("reverts to an earlier checkpoint and trims checkpoint projections + git Effect.gen(function* () { yield* seedProjectAndThread(harness); - yield* harness.adapterHarness.queueTurnResponseForNextSession({ + yield* harness.adapterHarness!.queueTurnResponseForNextSession({ events: [ { type: "turn.started", @@ -691,7 +691,7 @@ it.live("reverts to an earlier checkpoint and trims checkpoint projections + git (entry) => entry.session?.threadId === "thread-1" && entry.checkpoints.length === 1, ); - yield* harness.adapterHarness.queueTurnResponse(THREAD_ID, { + yield* harness.adapterHarness!.queueTurnResponse(THREAD_ID, { events: [ { type: "turn.started", @@ -796,7 +796,7 @@ it.live("reverts to an earlier checkpoint and trims checkpoint projections + git gitRefExists(harness.workspaceDir, checkpointRefForThreadTurn(THREAD_ID, 2)), false, ); - assert.deepEqual(harness.adapterHarness.getRollbackCalls(THREAD_ID), [1]); + assert.deepEqual(harness.adapterHarness!.getRollbackCalls(THREAD_ID), [1]); const checkpointRows = yield* harness.checkpointRepository.listByThreadId({ threadId: THREAD_ID, diff --git a/apps/server/src/codexAppServerManager.test.ts b/apps/server/src/codexAppServerManager.test.ts index 6032a1e3cf38c8c87e1d6f35de0e2962ac99e99d..58c34020294c8552e62d609679b19e1e3b7847ea 100644 --- a/apps/server/src/codexAppServerManager.test.ts +++ b/apps/server/src/codexAppServerManager.test.ts @@ -454,8 +454,8 @@ describe("respondToUserInput", () => { asThreadId("thread_1"), ApprovalRequestId.makeUnsafe("req-user-input-1"), { - scope: "All request methods", - compat: "Keep current envelope", + scope: "All request methods", + compat: "Keep current envelope", }, ); @@ -482,6 +482,80 @@ describe("respondToUserInput", () => { }), ); }); + + it("preserves explicit empty multi-select answers", async () => { + const { manager, context, requireSession, writeMessage, emitEvent } = + createPendingUserInputHarness(); + + await manager.respondToUserInput( + asThreadId("thread_1"), + ApprovalRequestId.makeUnsafe("req-user-input-1"), + { + scope: [], + }, + ); + + expect(requireSession).toHaveBeenCalledWith("thread_1"); + expect(writeMessage).toHaveBeenCalledWith(context, { + id: 42, + result: { + answers: { + scope: { answers: [] }, + }, + }, + }); + expect(emitEvent).toHaveBeenCalledWith( + expect.objectContaining({ + method: "item/tool/requestUserInput/answered", + payload: { + requestId: "req-user-input-1", + answers: { + scope: { answers: [] }, + }, + }, + }), + ); + }); + + it("tracks file-read approval requests with the correct method", () => { + const manager = new CodexAppServerManager(); + const context = { + session: { + sessionId: "sess_1", + provider: "codex", + status: "ready", + threadId: asThreadId("thread_1"), + resumeCursor: { threadId: "thread_1" }, + createdAt: "2026-02-10T00:00:00.000Z", + updatedAt: "2026-02-10T00:00:00.000Z", + }, + pendingApprovals: new Map(), + pendingUserInputs: new Map(), + }; + type ApprovalRequestContext = { + session: typeof context.session; + pendingApprovals: typeof context.pendingApprovals; + pendingUserInputs: typeof context.pendingUserInputs; + }; + + ( + manager as unknown as { + handleServerRequest: ( + context: ApprovalRequestContext, + request: Record, + ) => void; + } + ).handleServerRequest(context, { + jsonrpc: "2.0", + id: 42, + method: "item/fileRead/requestApproval", + params: {}, + }); + + const request = Array.from(context.pendingApprovals.values())[0]; + expect(request?.requestKind).toBe("file-read"); + expect(request?.method).toBe("item/fileRead/requestApproval"); + }); }); describe.skipIf(!process.env.CODEX_BINARY_PATH)("startSession live Codex resume", () => { diff --git a/apps/server/src/codexAppServerManager.ts b/apps/server/src/codexAppServerManager.ts index b8447237f2a37900e9fec0718a522b2dc4b4196a..185851ae16cb1c6ffc0e053459f5047383269963 100644 --- a/apps/server/src/codexAppServerManager.ts +++ b/apps/server/src/codexAppServerManager.ts @@ -34,7 +34,10 @@ interface PendingRequest { interface PendingApprovalRequest { requestId: ApprovalRequestId; jsonRpcId: string | number; - method: "item/commandExecution/requestApproval" | "item/fileChange/requestApproval"; + method: + | "item/commandExecution/requestApproval" + | "item/fileChange/requestApproval" + | "item/fileRead/requestApproval"; requestKind: ProviderRequestKind; threadId: ThreadId; turnId?: TurnId; @@ -352,18 +355,14 @@ function toCodexUserInputAnswer(value: unknown): CodexUserInputAnswer { if (Array.isArray(value)) { const answers = value.filter((entry): entry is string => typeof entry === "string"); - if (answers.length > 0) { - return { answers }; - } + return { answers }; } if (value && typeof value === "object") { const maybeAnswers = (value as { answers?: unknown }).answers; if (Array.isArray(maybeAnswers)) { const answers = maybeAnswers.filter((entry): entry is string => typeof entry === "string"); - if (answers.length > 0) { - return { answers }; - } + return { answers }; } } @@ -1064,7 +1063,9 @@ export class CodexAppServerManager extends EventEmitter { }), ); + it.effect("preserves file-read request type when mapping serverRequest/resolved", () => + Effect.gen(function* () { + const adapter = yield* CodexAdapter; + const firstEventFiber = yield* Stream.runHead(adapter.streamEvents).pipe(Effect.forkChild); + + const event: ProviderEvent = { + id: asEventId("evt-file-read-request-resolved"), + kind: "notification", + provider: "codex", + threadId: asThreadId("thread-1"), + createdAt: new Date().toISOString(), + method: "serverRequest/resolved", + requestId: ApprovalRequestId.makeUnsafe("req-file-read-1"), + payload: { + request: { + method: "item/fileRead/requestApproval", + }, + decision: "accept", + }, + }; + + lifecycleManager.emit("event", event); + const firstEvent = yield* Fiber.join(firstEventFiber); + + assert.equal(firstEvent._tag, "Some"); + if (firstEvent._tag !== "Some") { + return; + } + assert.equal(firstEvent.value.type, "request.resolved"); + if (firstEvent.value.type !== "request.resolved") { + return; + } + assert.equal(firstEvent.value.payload.requestType, "file_read_approval"); + }), + ); + + it.effect("preserves explicit empty multi-select user-input answers", () => + Effect.gen(function* () { + const adapter = yield* CodexAdapter; + const firstEventFiber = yield* Stream.runHead(adapter.streamEvents).pipe(Effect.forkChild); + + const event: ProviderEvent = { + id: asEventId("evt-user-input-empty"), + kind: "notification", + provider: "codex", + threadId: asThreadId("thread-1"), + createdAt: new Date().toISOString(), + method: "item/tool/requestUserInput/answered", + payload: { + answers: { + scope: [], + }, + }, + }; + + lifecycleManager.emit("event", event); + const firstEvent = yield* Fiber.join(firstEventFiber); + + assert.equal(firstEvent._tag, "Some"); + if (firstEvent._tag !== "Some") { + return; + } + assert.equal(firstEvent.value.type, "user-input.resolved"); + if (firstEvent.value.type !== "user-input.resolved") { + return; + } + assert.deepEqual(firstEvent.value.payload.answers, { + scope: [], + }); + }), + ); + it.effect("maps windowsSandbox/setupCompleted to session state and warning on failure", () => Effect.gen(function* () { const adapter = yield* CodexAdapter; diff --git a/apps/server/src/provider/Layers/CodexAdapter.ts b/apps/server/src/provider/Layers/CodexAdapter.ts index d26fbe35b6ee2788eef11c4c4b4a464051803be0..a31ba2907e0946b836627215aa7c15350398eb22 100644 --- a/apps/server/src/provider/Layers/CodexAdapter.ts +++ b/apps/server/src/provider/Layers/CodexAdapter.ts @@ -274,9 +274,6 @@ function toCanonicalUserInputAnswers( if (Array.isArray(value)) { const normalized = value.filter((entry): entry is string => typeof entry === "string"); - if (normalized.length === 0) { - return []; - } return [[questionId, normalized.length === 1 ? normalized[0] : normalized] as const]; } @@ -284,7 +281,7 @@ function toCanonicalUserInputAnswers( const answerList = asArray(answerObject?.answers)?.filter( (entry): entry is string => typeof entry === "string", ); - if (!answerList || answerList.length === 0) { + if (!answerList) { return []; } return [[questionId, answerList.length === 1 ? answerList[0] : answerList] as const]; diff --git a/apps/server/src/provider/Layers/ProviderSessionDirectory.test.ts b/apps/server/src/provider/Layers/ProviderSessionDirectory.test.ts index 6efde41e7c192017d2f2562214ecd795875608f3..1882c1cc0e4cb96fc46eaa044d0f2b82e39f89cf 100644 --- a/apps/server/src/provider/Layers/ProviderSessionDirectory.test.ts +++ b/apps/server/src/provider/Layers/ProviderSessionDirectory.test.ts @@ -133,6 +133,36 @@ it.layer(makeDirectoryLayer(SqlitePersistenceMemory))("ProviderSessionDirectoryL } })); + it("resets adapterKey to the new provider when provider changes without an explicit adapter key", () => + Effect.gen(function* () { + const directory = yield* ProviderSessionDirectory; + const runtimeRepository = yield* ProviderSessionRuntimeRepository; + const threadId = ThreadId.makeUnsafe("thread-provider-change"); + + yield* runtimeRepository.upsert({ + threadId, + providerName: "cursor", + adapterKey: "cursor", + runtimeMode: "full-access", + status: "running", + lastSeenAt: new Date().toISOString(), + resumeCursor: null, + runtimePayload: null, + }); + + yield* directory.upsert({ + provider: "codex", + threadId, + }); + + const runtime = yield* runtimeRepository.getByThreadId({ threadId }); + assert.equal(Option.isSome(runtime), true); + if (Option.isSome(runtime)) { + assert.equal(runtime.value.providerName, "codex"); + assert.equal(runtime.value.adapterKey, "codex"); + } + })); + it("rehydrates persisted mappings across layer restart", () => Effect.gen(function* () { const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "t3-provider-directory-")); diff --git a/apps/server/src/provider/Layers/ProviderSessionDirectory.ts b/apps/server/src/provider/Layers/ProviderSessionDirectory.ts index 5615b96c6cbfb2bd088c850d801e89cf2b80f2d8..69e1e439bfdf9a9f34026ac645fa60a6f4b04c6a 100644 --- a/apps/server/src/provider/Layers/ProviderSessionDirectory.ts +++ b/apps/server/src/provider/Layers/ProviderSessionDirectory.ts @@ -95,11 +95,15 @@ const makeProviderSessionDirectory = Effect.gen(function* () { } const now = new Date().toISOString(); + const providerChanged = + existingRuntime !== undefined && existingRuntime.providerName !== binding.provider; yield* repository .upsert({ threadId: resolvedThreadId, providerName: binding.provider, - adapterKey: binding.adapterKey ?? existingRuntime?.adapterKey ?? binding.provider, + adapterKey: + binding.adapterKey ?? + (providerChanged ? binding.provider : (existingRuntime?.adapterKey ?? binding.provider)), runtimeMode: binding.runtimeMode ?? existingRuntime?.runtimeMode ?? "full-access", status: binding.status ?? existingRuntime?.status ?? "running", lastSeenAt: now, diff --git a/packages/contracts/src/orchestration.test.ts b/packages/contracts/src/orchestration.test.ts index 16c6036cf0fa8812161f8d9c2c54f65e7b3e3cf5..25a641edbb9cbda6882e7dd76796094fe7023ea7 100644 --- a/packages/contracts/src/orchestration.test.ts +++ b/packages/contracts/src/orchestration.test.ts @@ -9,6 +9,7 @@ import { OrchestrationSession, ProjectCreateCommand, ThreadTurnStartCommand, + ThreadCreatedPayload, ThreadTurnDiff, ThreadTurnStartRequestedPayload, } from "./orchestration"; @@ -21,6 +22,7 @@ const decodeThreadTurnStartRequestedPayload = Schema.decodeUnknownEffect( ThreadTurnStartRequestedPayload, ); const decodeOrchestrationSession = Schema.decodeUnknownEffect(OrchestrationSession); +const decodeThreadCreatedPayload = Schema.decodeUnknownEffect(ThreadCreatedPayload); it.effect("parses turn diff input when fromTurnCount <= toTurnCount", () => Effect.gen(function* () { @@ -138,6 +140,24 @@ it.effect("preserves explicit provider and runtime mode in thread.turn.start", ( }), ); +it.effect("decodes thread.created runtime mode for historical events", () => + Effect.gen(function* () { + const parsed = yield* decodeThreadCreatedPayload({ + threadId: "thread-1", + projectId: "project-1", + title: "Thread title", + model: "gpt-5.4", + interactionMode: "default", + branch: null, + worktreePath: null, + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", + }); + + assert.strictEqual(parsed.runtimeMode, DEFAULT_RUNTIME_MODE); + }), +); + it.effect("accepts provider-scoped model options in thread.turn.start", () => Effect.gen(function* () { const parsed = yield* decodeThreadTurnStartCommand({ diff --git a/packages/contracts/src/orchestration.ts b/packages/contracts/src/orchestration.ts index 212f2ae05eaa81c085a0b23e8f57fec5251ffe81..59c4bdf82a7b744121ccb45105fa580064ea75ac 100644 --- a/packages/contracts/src/orchestration.ts +++ b/packages/contracts/src/orchestration.ts @@ -594,7 +594,7 @@ export const ThreadCreatedPayload = Schema.Struct({ projectId: ProjectId, title: TrimmedNonEmptyString, model: TrimmedNonEmptyString, - runtimeMode: RuntimeMode, + runtimeMode: RuntimeMode.pipe(Schema.withDecodingDefault(() => DEFAULT_RUNTIME_MODE)), interactionMode: ProviderInteractionMode.pipe( Schema.withDecodingDefault(() => DEFAULT_PROVIDER_INTERACTION_MODE), ), diff --git a/packages/shared/src/model.test.ts b/packages/shared/src/model.test.ts index 00fb0a77500587984f4af0e4760916b7d8fe6cf1..8771a24c1cfb5a95587d9b70f070cc0073ec0009 100644 --- a/packages/shared/src/model.test.ts +++ b/packages/shared/src/model.test.ts @@ -27,6 +27,11 @@ describe("normalizeModelSlug", () => { expect(normalizeModelSlug("gpt-5.2")).toBe("gpt-5.2"); expect(normalizeModelSlug("gpt-5.2-codex")).toBe("gpt-5.2-codex"); }); + + it("does not leak prototype properties as aliases", () => { + expect(normalizeModelSlug("toString")).toBe("toString"); + expect(normalizeModelSlug("constructor")).toBe("constructor"); + }); }); describe("resolveModelSlug", () => { diff --git a/packages/shared/src/model.ts b/packages/shared/src/model.ts index 6f3c1a75ec21346dc04c258b908be4765f5bb12b..592e2dfb9f357830cfe54082b1155b4a68d61201 100644 --- a/packages/shared/src/model.ts +++ b/packages/shared/src/model.ts @@ -36,7 +36,8 @@ export function normalizeModelSlug( } const aliases = MODEL_SLUG_ALIASES_BY_PROVIDER[provider] as Record; - return aliases[trimmed] ?? (trimmed as ModelSlug); + const aliased = aliases[trimmed]; + return typeof aliased === "string" ? aliased : (trimmed as ModelSlug); } export function resolveModelSlug( -- 2.54.0