History

Address Codex review feedback

Julius Marminge committed 6 months ago

Commit fa4a29f1 parent 3a38f065 files · +75 −8Browse files

Unified · Split

5 files
  1. apps/server/src/provider/Layers/CodexAdapter.ts +2 −1
  2. apps/server/src/provider/codex/CodexSessionRuntime.test.ts +21 −0
  3. apps/server/src/provider/codex/CodexSessionRuntime.ts +15 −2
  4. packages/effect-codex-app-server/src/client.test.ts +30 −0
  5. packages/effect-codex-app-server/src/client.ts +7 −5
apps/server/src/provider/Layers/CodexAdapter.ts+2−1
@@ -365,11 +365,12 @@ function completeCodexTurnTokenUsage( · 364 unmodified lines
365365 }
366366
367367 function toTurnStatus(
368− value: EffectCodexSchema.V2TurnCompletedNotification["turn"]["status"],
368+ value: EffectCodexSchema.V2TurnCompletedNotification["turn"]["status"] | "cancelled",
369369 ): "completed" | "failed" | "cancelled" | "interrupted" {
370370 switch (value) {
371371 case "completed":
372372 case "failed":
373+ case "cancelled":
373374 case "interrupted":
374375 return value;
375376 default:
apps/server/src/provider/codex/CodexSessionRuntime.test.ts+21−0
@@ -171,6 +171,27 @@ describe("isRecoverableThreadResumeError", () => { · 170 unmodified lines
171171 false,
172172 );
173173 });
174+
175+ it("ignores unrelated missing-resource errors that do not mention threads", () => {
176+ assert.equal(
177+ isRecoverableThreadResumeError(
178+ new CodexErrors.CodexAppServerRequestError({
179+ code: -32603,
180+ errorMessage: "Config file not found",
181+ }),
182+ ),
183+ false,
184+ );
185+ assert.equal(
186+ isRecoverableThreadResumeError(
187+ new CodexErrors.CodexAppServerRequestError({
188+ code: -32603,
189+ errorMessage: "Model does not exist",
190+ }),
191+ ),
192+ false,
193+ );
194+ });
174195 });
175196
176197 describe("openCodexThread", () => {
apps/server/src/provider/codex/CodexSessionRuntime.ts+15−2
@@ -383,6 +383,9 @@ function classifyCodexStderrLine(rawLine: string): { readonly message: string } · 382 unmodified lines
383383
384384 export function isRecoverableThreadResumeError(error: unknown): boolean {
385385 const message = (error instanceof Error ? error.message : String(error)).toLowerCase();
386+ if (!message.includes("thread")) {
387+ return false;
388+ }
386389 return RECOVERABLE_THREAD_RESUME_ERROR_SNIPPETS.some((snippet) => message.includes(snippet));
387390 }
388391
@@ -390,8 +393,17 @@ type CodexThreadOpenResponse = · 1 unmodified line
390393 | CodexRpc.ClientRequestResponsesByMethod["thread/start"]
391394 | CodexRpc.ClientRequestResponsesByMethod["thread/resume"];
392395
396+type CodexThreadOpenMethod = "thread/start" | "thread/resume";
397+
398+interface CodexThreadOpenClient {
399+ readonly request: <M extends CodexThreadOpenMethod>(
400+ method: M,
401+ payload: CodexRpc.ClientRequestParamsByMethod[M],
402+ ) => Effect.Effect<CodexRpc.ClientRequestResponsesByMethod[M], CodexErrors.CodexAppServerError>;
403+}
404+
393405 export const openCodexThread = (input: {
394− readonly client: CodexClient.CodexAppServerClientShape;
406+ readonly client: CodexThreadOpenClient;
395407 readonly threadId: ThreadId;
396408 readonly runtimeMode: RuntimeMode;
397409 readonly cwd: string;
@@ -1288,6 +1300,7 @@ export const makeCodexSessionRuntime = ( · 890 unmodified lines
12881300 requestId,
12891301 });
12901302 }
1303+ const codexAnswers = yield* toCodexUserInputAnswers(answers);
12911304 yield* Ref.update(pendingUserInputsRef, (current) => {
12921305 const next = new Map(current);
12931306 next.delete(requestId);
@@ -1302,7 +1315,7 @@ export const makeCodexSessionRuntime = ( · 8 unmodified lines
13021315 ...(pending.turnId ? { turnId: pending.turnId } : {}),
13031316 ...(pending.itemId ? { itemId: pending.itemId } : {}),
13041317 payload: {
1305− answers: yield* toCodexUserInputAnswers(answers),
1318+ answers: codexAnswers,
13061319 },
13071320 });
13081321 }),
packages/effect-codex-app-server/src/client.test.ts+30−0
@@ -121,4 +121,34 @@ it.layer(NodeServices.layer)("effect-codex-app-server client", (it) => { · 120 unmodified lines
121121 ]);
122122 }),
123123 );
124+
125+ it.effect("initializes a command-backed app-server client", () =>
126+ Effect.gen(function* () {
127+ const path = yield* Path.Path;
128+ const scope = yield* Scope.make();
129+ const clientLayer = CodexClient.layerCommand({
130+ command: "bun",
131+ args: ["run", yield* mockPeerPath],
132+ cwd: path.join(import.meta.dirname, ".."),
133+ });
134+ const context = yield* Layer.buildWithScope(clientLayer, scope);
135+
136+ const initialized = yield* Effect.gen(function* () {
137+ const client = yield* CodexClient.CodexAppServerClient;
138+ return yield* client.request("initialize", {
139+ clientInfo: {
140+ name: "effect-codex-app-server-test",
141+ title: "Effect Codex App Server Test",
142+ version: "0.0.0",
143+ },
144+ capabilities: {
145+ experimentalApi: true,
146+ optOutNotificationMethods: null,
147+ },
148+ });
149+ }).pipe(Effect.provide(context), Effect.ensuring(Scope.close(scope, Exit.void)));
150+
151+ assert.equal(initialized.userAgent, "mock-codex-app-server");
152+ }),
153+ );
124154 });
packages/effect-codex-app-server/src/client.ts+7−5
@@ -281,7 +281,7 @@ export const layerCommand = ( · 280 unmodified lines
281281 ...(options.env ? { env: { ...process.env, ...options.env } } : {}),
282282 shell: process.platform === "win32",
283283 });
284− const handle = yield* spawner.spawn(command).pipe(
284+ return yield* spawner.spawn(command).pipe(
285285 Effect.mapError(
286286 (cause) =>
287287 new CodexError.CodexAppServerSpawnError({
@@ -290,9 +290,11 @@ export const layerCommand = ( · 2 unmodified lines
290290 }),
291291 ),
292292 );
293− const client = yield* make(makeChildStdio(handle), options, makeTerminationError(handle));
294− return { client, handle };
295293 }),
296− ({ handle }) => handle.kill().pipe(Effect.orDie),
297− ).pipe(Effect.map(({ client }) => client)),
294+ (handle) => handle.kill().pipe(Effect.orDie),
295+ ).pipe(
296+ Effect.flatMap((handle) =>
297+ make(makeChildStdio(handle), options, makeTerminationError(handle)),
298+ ),
299+ ),
298300 );