authorgravatar for julius0216@outlook.comJulius Marminge <julius0216@outlook.com> 2026-03-05 22:29:26-08:00
committergravatar for jmarminge@gmail.comJulius Marminge <jmarminge@gmail.com> 2026-03-05 23:00:41-08:00
log10dde452f90fc3d4d1aea0add67d13cdfb2567ab
treee60e5245a8b5dfeaa492733675be6eff36dcd847
parent116c8c5e8e0db2fe7fad070c2f67738a62f93546

fix: address core review findings

Co-authored-by: codex <codex@users.noreply.github.com>

13 files changed, 232 insertions(+), 30 deletions(-)

apps/server/integration/OrchestrationEngineHarness.integration.ts+2-2
......@@ -152,7 +152,7 @@ export interface OrchestrationIntegrationHarness {
152152 readonly rootDir: string;
153153 readonly workspaceDir: string;
154154 readonly dbPath: string;
155 readonly adapterHarness: TestProviderAdapterHarness;
155 readonly adapterHarness: TestProviderAdapterHarness | null;
156156 readonly engine: OrchestrationEngineShape;
157157 readonly snapshotQuery: ProjectionSnapshotQuery["Service"];
158158 readonly providerService: ProviderService["Service"];
......@@ -435,7 +435,7 @@ export const makeOrchestrationIntegrationHarness = (
435435 rootDir,
436436 workspaceDir,
437437 dbPath,
438 adapterHarness: adapterHarness as TestProviderAdapterHarness,
438 adapterHarness,
439439 engine,
440440 snapshotQuery,
441441 providerService,
apps/server/integration/orchestrationEngine.integration.test.ts+9-9
......@@ -180,7 +180,7 @@ it.live("runs a single turn end-to-end and persists checkpoint state in sqlite +
180180 ],
181181 };
182182
183 yield* harness.adapterHarness.queueTurnResponseForNextSession(turnResponse);
183 yield* harness.adapterHarness!.queueTurnResponseForNextSession(turnResponse);
184184 yield* startTurn({
185185 harness,
186186 commandId: "cmd-turn-start-single",
......@@ -314,7 +314,7 @@ it.live("runs multi-turn file edits and persists checkpoint diffs", () =>
314314 Effect.gen(function* () {
315315 yield* seedProjectAndThread(harness);
316316
317 yield* harness.adapterHarness.queueTurnResponseForNextSession({
317 yield* harness.adapterHarness!.queueTurnResponseForNextSession({
318318 events: [
319319 {
320320 type: "turn.started",
......@@ -373,7 +373,7 @@ it.live("runs multi-turn file edits and persists checkpoint diffs", () =>
373373 (entry) => entry.checkpoints.length === 1 && entry.session?.threadId === "thread-1",
374374 );
375375
376 yield* harness.adapterHarness.queueTurnResponse(THREAD_ID, {
376 yield* harness.adapterHarness!.queueTurnResponse(THREAD_ID, {
377377 events: [
378378 {
379379 type: "turn.started",
......@@ -473,7 +473,7 @@ it.live("tracks approval requests and resolves pending approvals on user respons
473473 Effect.gen(function* () {
474474 yield* seedProjectAndThread(harness);
475475
476 yield* harness.adapterHarness.queueTurnResponseForNextSession({
476 yield* harness.adapterHarness!.queueTurnResponseForNextSession({
477477 events: [
478478 {
479479 type: "turn.started",
......@@ -538,7 +538,7 @@ it.live("tracks approval requests and resolves pending approvals on user respons
538538 assert.equal(resolvedRow.decision, "accept");
539539
540540 const approvalResponses = yield* waitForSync(
541 () => harness.adapterHarness.getApprovalResponses(THREAD_ID),
541 () => harness.adapterHarness!.getApprovalResponses(THREAD_ID),
542542 (responses) => responses.length === 1,
543543 "provider approval response",
544544 );
......@@ -554,7 +554,7 @@ it.live("records failed turn runtime state and checkpoint status as error", () =
554554 Effect.gen(function* () {
555555 yield* seedProjectAndThread(harness);
556556
557 yield* harness.adapterHarness.queueTurnResponseForNextSession({
557 yield* harness.adapterHarness!.queueTurnResponseForNextSession({
558558 events: [
559559 {
560560 type: "turn.started",
......@@ -633,7 +633,7 @@ it.live("reverts to an earlier checkpoint and trims checkpoint projections + git
633633 Effect.gen(function* () {
634634 yield* seedProjectAndThread(harness);
635635
636 yield* harness.adapterHarness.queueTurnResponseForNextSession({
636 yield* harness.adapterHarness!.queueTurnResponseForNextSession({
637637 events: [
638638 {
639639 type: "turn.started",
......@@ -691,7 +691,7 @@ it.live("reverts to an earlier checkpoint and trims checkpoint projections + git
691691 (entry) => entry.session?.threadId === "thread-1" && entry.checkpoints.length === 1,
692692 );
693693
694 yield* harness.adapterHarness.queueTurnResponse(THREAD_ID, {
694 yield* harness.adapterHarness!.queueTurnResponse(THREAD_ID, {
695695 events: [
696696 {
697697 type: "turn.started",
......@@ -796,7 +796,7 @@ it.live("reverts to an earlier checkpoint and trims checkpoint projections + git
796796 gitRefExists(harness.workspaceDir, checkpointRefForThreadTurn(THREAD_ID, 2)),
797797 false,
798798 );
799 assert.deepEqual(harness.adapterHarness.getRollbackCalls(THREAD_ID), [1]);
799 assert.deepEqual(harness.adapterHarness!.getRollbackCalls(THREAD_ID), [1]);
800800
801801 const checkpointRows = yield* harness.checkpointRepository.listByThreadId({
802802 threadId: THREAD_ID,
apps/server/src/codexAppServerManager.test.ts+76-2
......@@ -454,8 +454,8 @@ describe("respondToUserInput", () => {
454454 asThreadId("thread_1"),
455455 ApprovalRequestId.makeUnsafe("req-user-input-1"),
456456 {
457 scope: "All request methods",
458 compat: "Keep current envelope",
457 scope: "All request methods",
458 compat: "Keep current envelope",
459459 },
460460 );
461461
......@@ -482,6 +482,80 @@ describe("respondToUserInput", () => {
482482 }),
483483 );
484484 });
485
486 it("preserves explicit empty multi-select answers", async () => {
487 const { manager, context, requireSession, writeMessage, emitEvent } =
488 createPendingUserInputHarness();
489
490 await manager.respondToUserInput(
491 asThreadId("thread_1"),
492 ApprovalRequestId.makeUnsafe("req-user-input-1"),
493 {
494 scope: [],
495 },
496 );
497
498 expect(requireSession).toHaveBeenCalledWith("thread_1");
499 expect(writeMessage).toHaveBeenCalledWith(context, {
500 id: 42,
501 result: {
502 answers: {
503 scope: { answers: [] },
504 },
505 },
506 });
507 expect(emitEvent).toHaveBeenCalledWith(
508 expect.objectContaining({
509 method: "item/tool/requestUserInput/answered",
510 payload: {
511 requestId: "req-user-input-1",
512 answers: {
513 scope: { answers: [] },
514 },
515 },
516 }),
517 );
518 });
519
520 it("tracks file-read approval requests with the correct method", () => {
521 const manager = new CodexAppServerManager();
522 const context = {
523 session: {
524 sessionId: "sess_1",
525 provider: "codex",
526 status: "ready",
527 threadId: asThreadId("thread_1"),
528 resumeCursor: { threadId: "thread_1" },
529 createdAt: "2026-02-10T00:00:00.000Z",
530 updatedAt: "2026-02-10T00:00:00.000Z",
531 },
532 pendingApprovals: new Map(),
533 pendingUserInputs: new Map(),
534 };
535 type ApprovalRequestContext = {
536 session: typeof context.session;
537 pendingApprovals: typeof context.pendingApprovals;
538 pendingUserInputs: typeof context.pendingUserInputs;
539 };
540
541 (
542 manager as unknown as {
543 handleServerRequest: (
544 context: ApprovalRequestContext,
545 request: Record<string, unknown>,
546 ) => void;
547 }
548 ).handleServerRequest(context, {
549 jsonrpc: "2.0",
550 id: 42,
551 method: "item/fileRead/requestApproval",
552 params: {},
553 });
554
555 const request = Array.from(context.pendingApprovals.values())[0];
556 expect(request?.requestKind).toBe("file-read");
557 expect(request?.method).toBe("item/fileRead/requestApproval");
558 });
485559});
486560
487561describe.skipIf(!process.env.CODEX_BINARY_PATH)("startSession live Codex resume", () => {
apps/server/src/codexAppServerManager.ts+9-8
......@@ -34,7 +34,10 @@ interface PendingRequest {
3434interface PendingApprovalRequest {
3535 requestId: ApprovalRequestId;
3636 jsonRpcId: string | number;
37 method: "item/commandExecution/requestApproval" | "item/fileChange/requestApproval";
37 method:
38 | "item/commandExecution/requestApproval"
39 | "item/fileChange/requestApproval"
40 | "item/fileRead/requestApproval";
3841 requestKind: ProviderRequestKind;
3942 threadId: ThreadId;
4043 turnId?: TurnId;
......@@ -352,18 +355,14 @@ function toCodexUserInputAnswer(value: unknown): CodexUserInputAnswer {
352355
353356 if (Array.isArray(value)) {
354357 const answers = value.filter((entry): entry is string => typeof entry === "string");
355 if (answers.length > 0) {
356 return { answers };
357 }
358 return { answers };
358359 }
359360
360361 if (value && typeof value === "object") {
361362 const maybeAnswers = (value as { answers?: unknown }).answers;
362363 if (Array.isArray(maybeAnswers)) {
363364 const answers = maybeAnswers.filter((entry): entry is string => typeof entry === "string");
364 if (answers.length > 0) {
365 return { answers };
366 }
365 return { answers };
367366 }
368367 }
369368
......@@ -1064,7 +1063,9 @@ export class CodexAppServerManager extends EventEmitter<CodexAppServerManagerEve
10641063 method:
10651064 requestKind === "command"
10661065 ? "item/commandExecution/requestApproval"
1067 : "item/fileChange/requestApproval",
1066 : requestKind === "file-read"
1067 ? "item/fileRead/requestApproval"
1068 : "item/fileChange/requestApproval",
10681069 requestKind,
10691070 threadId: context.session.threadId,
10701071 ...(route.turnId ? { turnId: route.turnId } : {}),
apps/server/src/main.ts-2
......@@ -9,8 +9,6 @@
99import { Config, Data, Effect, FileSystem, Layer, Option, Path, Schema, ServiceMap } from "effect";
1010import { Command, Flag } from "effect/unstable/cli";
1111import { NetService } from "@t3tools/shared/Net";
12
13// Dummy comment.
1412import {
1513 DEFAULT_PORT,
1614 resolveStaticDir,
apps/server/src/provider/Layers/CodexAdapter.test.ts+72
......@@ -444,6 +444,78 @@ lifecycleLayer("CodexAdapterLive lifecycle", (it) => {
444444 }),
445445 );
446446
447 it.effect("preserves file-read request type when mapping serverRequest/resolved", () =>
448 Effect.gen(function* () {
449 const adapter = yield* CodexAdapter;
450 const firstEventFiber = yield* Stream.runHead(adapter.streamEvents).pipe(Effect.forkChild);
451
452 const event: ProviderEvent = {
453 id: asEventId("evt-file-read-request-resolved"),
454 kind: "notification",
455 provider: "codex",
456 threadId: asThreadId("thread-1"),
457 createdAt: new Date().toISOString(),
458 method: "serverRequest/resolved",
459 requestId: ApprovalRequestId.makeUnsafe("req-file-read-1"),
460 payload: {
461 request: {
462 method: "item/fileRead/requestApproval",
463 },
464 decision: "accept",
465 },
466 };
467
468 lifecycleManager.emit("event", event);
469 const firstEvent = yield* Fiber.join(firstEventFiber);
470
471 assert.equal(firstEvent._tag, "Some");
472 if (firstEvent._tag !== "Some") {
473 return;
474 }
475 assert.equal(firstEvent.value.type, "request.resolved");
476 if (firstEvent.value.type !== "request.resolved") {
477 return;
478 }
479 assert.equal(firstEvent.value.payload.requestType, "file_read_approval");
480 }),
481 );
482
483 it.effect("preserves explicit empty multi-select user-input answers", () =>
484 Effect.gen(function* () {
485 const adapter = yield* CodexAdapter;
486 const firstEventFiber = yield* Stream.runHead(adapter.streamEvents).pipe(Effect.forkChild);
487
488 const event: ProviderEvent = {
489 id: asEventId("evt-user-input-empty"),
490 kind: "notification",
491 provider: "codex",
492 threadId: asThreadId("thread-1"),
493 createdAt: new Date().toISOString(),
494 method: "item/tool/requestUserInput/answered",
495 payload: {
496 answers: {
497 scope: [],
498 },
499 },
500 };
501
502 lifecycleManager.emit("event", event);
503 const firstEvent = yield* Fiber.join(firstEventFiber);
504
505 assert.equal(firstEvent._tag, "Some");
506 if (firstEvent._tag !== "Some") {
507 return;
508 }
509 assert.equal(firstEvent.value.type, "user-input.resolved");
510 if (firstEvent.value.type !== "user-input.resolved") {
511 return;
512 }
513 assert.deepEqual(firstEvent.value.payload.answers, {
514 scope: [],
515 });
516 }),
517 );
518
447519 it.effect("maps windowsSandbox/setupCompleted to session state and warning on failure", () =>
448520 Effect.gen(function* () {
449521 const adapter = yield* CodexAdapter;
apps/server/src/provider/Layers/CodexAdapter.ts+1-4
......@@ -274,9 +274,6 @@ function toCanonicalUserInputAnswers(
274274
275275 if (Array.isArray(value)) {
276276 const normalized = value.filter((entry): entry is string => typeof entry === "string");
277 if (normalized.length === 0) {
278 return [];
279 }
280277 return [[questionId, normalized.length === 1 ? normalized[0] : normalized] as const];
281278 }
282279
......@@ -284,7 +281,7 @@ function toCanonicalUserInputAnswers(
284281 const answerList = asArray(answerObject?.answers)?.filter(
285282 (entry): entry is string => typeof entry === "string",
286283 );
287 if (!answerList || answerList.length === 0) {
284 if (!answerList) {
288285 return [];
289286 }
290287 return [[questionId, answerList.length === 1 ? answerList[0] : answerList] as const];
apps/server/src/provider/Layers/ProviderSessionDirectory.test.ts+30
......@@ -133,6 +133,36 @@ it.layer(makeDirectoryLayer(SqlitePersistenceMemory))("ProviderSessionDirectoryL
133133 }
134134 }));
135135
136 it("resets adapterKey to the new provider when provider changes without an explicit adapter key", () =>
137 Effect.gen(function* () {
138 const directory = yield* ProviderSessionDirectory;
139 const runtimeRepository = yield* ProviderSessionRuntimeRepository;
140 const threadId = ThreadId.makeUnsafe("thread-provider-change");
141
142 yield* runtimeRepository.upsert({
143 threadId,
144 providerName: "cursor",
145 adapterKey: "cursor",
146 runtimeMode: "full-access",
147 status: "running",
148 lastSeenAt: new Date().toISOString(),
149 resumeCursor: null,
150 runtimePayload: null,
151 });
152
153 yield* directory.upsert({
154 provider: "codex",
155 threadId,
156 });
157
158 const runtime = yield* runtimeRepository.getByThreadId({ threadId });
159 assert.equal(Option.isSome(runtime), true);
160 if (Option.isSome(runtime)) {
161 assert.equal(runtime.value.providerName, "codex");
162 assert.equal(runtime.value.adapterKey, "codex");
163 }
164 }));
165
136166 it("rehydrates persisted mappings across layer restart", () =>
137167 Effect.gen(function* () {
138168 const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "t3-provider-directory-"));
apps/server/src/provider/Layers/ProviderSessionDirectory.ts+5-1
......@@ -95,11 +95,15 @@ const makeProviderSessionDirectory = Effect.gen(function* () {
9595 }
9696
9797 const now = new Date().toISOString();
98 const providerChanged =
99 existingRuntime !== undefined && existingRuntime.providerName !== binding.provider;
98100 yield* repository
99101 .upsert({
100102 threadId: resolvedThreadId,
101103 providerName: binding.provider,
102 adapterKey: binding.adapterKey ?? existingRuntime?.adapterKey ?? binding.provider,
104 adapterKey:
105 binding.adapterKey ??
106 (providerChanged ? binding.provider : (existingRuntime?.adapterKey ?? binding.provider)),
103107 runtimeMode: binding.runtimeMode ?? existingRuntime?.runtimeMode ?? "full-access",
104108 status: binding.status ?? existingRuntime?.status ?? "running",
105109 lastSeenAt: now,
packages/contracts/src/orchestration.test.ts+20
......@@ -9,6 +9,7 @@ import {
99 OrchestrationSession,
1010 ProjectCreateCommand,
1111 ThreadTurnStartCommand,
12 ThreadCreatedPayload,
1213 ThreadTurnDiff,
1314 ThreadTurnStartRequestedPayload,
1415} from "./orchestration";
......@@ -21,6 +22,7 @@ const decodeThreadTurnStartRequestedPayload = Schema.decodeUnknownEffect(
2122 ThreadTurnStartRequestedPayload,
2223);
2324const decodeOrchestrationSession = Schema.decodeUnknownEffect(OrchestrationSession);
25const decodeThreadCreatedPayload = Schema.decodeUnknownEffect(ThreadCreatedPayload);
2426
2527it.effect("parses turn diff input when fromTurnCount <= toTurnCount", () =>
2628 Effect.gen(function* () {
......@@ -138,6 +140,24 @@ it.effect("preserves explicit provider and runtime mode in thread.turn.start", (
138140 }),
139141);
140142
143it.effect("decodes thread.created runtime mode for historical events", () =>
144 Effect.gen(function* () {
145 const parsed = yield* decodeThreadCreatedPayload({
146 threadId: "thread-1",
147 projectId: "project-1",
148 title: "Thread title",
149 model: "gpt-5.4",
150 interactionMode: "default",
151 branch: null,
152 worktreePath: null,
153 createdAt: "2026-01-01T00:00:00.000Z",
154 updatedAt: "2026-01-01T00:00:00.000Z",
155 });
156
157 assert.strictEqual(parsed.runtimeMode, DEFAULT_RUNTIME_MODE);
158 }),
159);
160
141161it.effect("accepts provider-scoped model options in thread.turn.start", () =>
142162 Effect.gen(function* () {
143163 const parsed = yield* decodeThreadTurnStartCommand({
packages/contracts/src/orchestration.ts+1-1
......@@ -594,7 +594,7 @@ export const ThreadCreatedPayload = Schema.Struct({
594594 projectId: ProjectId,
595595 title: TrimmedNonEmptyString,
596596 model: TrimmedNonEmptyString,
597 runtimeMode: RuntimeMode,
597 runtimeMode: RuntimeMode.pipe(Schema.withDecodingDefault(() => DEFAULT_RUNTIME_MODE)),
598598 interactionMode: ProviderInteractionMode.pipe(
599599 Schema.withDecodingDefault(() => DEFAULT_PROVIDER_INTERACTION_MODE),
600600 ),
packages/shared/src/model.test.ts+5
......@@ -27,6 +27,11 @@ describe("normalizeModelSlug", () => {
2727 expect(normalizeModelSlug("gpt-5.2")).toBe("gpt-5.2");
2828 expect(normalizeModelSlug("gpt-5.2-codex")).toBe("gpt-5.2-codex");
2929 });
30
31 it("does not leak prototype properties as aliases", () => {
32 expect(normalizeModelSlug("toString")).toBe("toString");
33 expect(normalizeModelSlug("constructor")).toBe("constructor");
34 });
3035});
3136
3237describe("resolveModelSlug", () => {
packages/shared/src/model.ts+2-1
......@@ -36,7 +36,8 @@ export function normalizeModelSlug(
3636 }
3737
3838 const aliases = MODEL_SLUG_ALIASES_BY_PROVIDER[provider] as Record<string, ModelSlug>;
39 return aliases[trimmed] ?? (trimmed as ModelSlug);
39 const aliased = aliases[trimmed];
40 return typeof aliased === "string" ? aliased : (trimmed as ModelSlug);
4041}
4142
4243export function resolveModelSlug(