diff --git a/apps/server/src/git/Layers/CodexTextGeneration.test.ts b/apps/server/src/git/Layers/CodexTextGeneration.test.ts index 32a37b1a7c8c724d3f91fa2be9d53f16124b781d..e8ca0303fa26e5bfa12704a16bfe0300dcf971a5 100644 --- a/apps/server/src/git/Layers/CodexTextGeneration.test.ts +++ b/apps/server/src/git/Layers/CodexTextGeneration.test.ts @@ -374,7 +374,7 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGenerationLive", (it) => { ), ); - it.effect("returns null branch when codex returns the wrong object shape", () => + it.effect("fails with typed TextGenerationError when codex returns wrong branch payload shape", () => withFakeCodexEnv( { output: JSON.stringify({ @@ -384,12 +384,23 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGenerationLive", (it) => { Effect.gen(function* () { const textGeneration = yield* TextGeneration; - const generated = yield* textGeneration.generateBranchName({ - cwd: process.cwd(), - message: "Fix websocket reconnect flake", - }); + const result = yield* textGeneration + .generateBranchName({ + cwd: process.cwd(), + message: "Fix websocket reconnect flake", + }) + .pipe( + Effect.match({ + onFailure: (error) => ({ _tag: "Left" as const, left: error }), + onSuccess: (value) => ({ _tag: "Right" as const, right: value }), + }), + ); - expect(generated.branch).toBeNull(); + expect(result._tag).toBe("Left"); + if (result._tag === "Left") { + expect(result.left).toBeInstanceOf(TextGenerationError); + expect(result.left.message).toContain("Codex returned invalid structured output"); + } }), ), ); diff --git a/apps/server/src/git/Layers/CodexTextGeneration.ts b/apps/server/src/git/Layers/CodexTextGeneration.ts index df2afc1aa0af691e056aa37e7fb4122f15a52e5b..376e1ed5bb0632b49e9fdfc7368a34d1427aab7a 100644 --- a/apps/server/src/git/Layers/CodexTextGeneration.ts +++ b/apps/server/src/git/Layers/CodexTextGeneration.ts @@ -552,20 +552,10 @@ const makeCodexTextGeneration = Effect.gen(function* () { outputSchemaJson: BRANCH_NAME_OUTPUT_SCHEMA_JSON, imagePaths, parse: (raw) => parseBranchNameOutput(raw), - }).pipe( - Effect.catch((error) => - Effect.gen(function* () { - yield* Effect.logWarning("branch-name generation failed, skipping rename", { - operation: "generateBranchName", - reason: error instanceof Error ? error.message : String(error), - }); - return { branch: null }; - }), - ), - ); + }); return { - branch: generated.branch ? sanitizeBranchName(generated.branch) : null, + branch: sanitizeBranchName(generated.branch), } satisfies BranchNameGenerationResult; }); }; diff --git a/apps/server/src/git/Services/TextGeneration.ts b/apps/server/src/git/Services/TextGeneration.ts index 155349f174ddc12577269022b67f971b565a3d5c..682cb6667e92549c7b6091cce7bead6b41d9aa1e 100644 --- a/apps/server/src/git/Services/TextGeneration.ts +++ b/apps/server/src/git/Services/TextGeneration.ts @@ -45,7 +45,7 @@ export interface BranchNameGenerationInput { } export interface BranchNameGenerationResult { - branch: string | null; + branch: string; } export interface TextGenerationService { diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts index 992ec314a69096695383044c77a58f65503fbc93..0a76ab8655999a3c626f135dd366e3c487ab802e 100644 --- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts +++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts @@ -18,6 +18,7 @@ import { Effect, Exit, Layer, ManagedRuntime, PubSub, Scope, Stream } from "effe import { afterEach, describe, expect, it, vi } from "vitest"; import { type ServerConfigShape, ServerConfig } from "../../config.ts"; +import { TextGenerationError } from "../../git/Errors.ts"; import { OrchestrationEventStoreLive } from "../../persistence/Layers/OrchestrationEventStore.ts"; import { OrchestrationCommandReceiptRepositoryLive } from "../../persistence/Layers/OrchestrationCommandReceipts.ts"; import { SqlitePersistenceMemory } from "../../persistence/Layers/Sqlite.ts"; @@ -123,7 +124,7 @@ describe("ProviderCommandReactor", () => { ); const generateBranchName = vi.fn((_: unknown) => Effect.succeed({ - branch: "generated-name" as string | null, + branch: "generated-name", }), ); @@ -447,13 +448,16 @@ describe("ProviderCommandReactor", () => { expect(harness.renameBranch.mock.calls.length).toBe(1); }); - it("skips worktree rename when generated branch is null", async () => { + it("skips worktree rename when branch-name generation fails", async () => { const harness = await createHarness(); const now = new Date().toISOString(); - harness.generateBranchName.mockImplementationOnce((_: unknown) => - Effect.succeed({ - branch: null, - }), + harness.generateBranchName.mockImplementationOnce(() => + Effect.fail( + new TextGenerationError({ + operation: "generateBranchName", + detail: "model returned invalid payload", + }), + ), ); await Effect.runPromise( diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts index 77a5436d42910d20a5f156115d6e5c85941bd9fa..921f45b1631cbe03fcd6627691f9ecf9542dd76f 100644 --- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts +++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts @@ -17,7 +17,10 @@ import { import { Cache, Cause, Duration, Effect, Layer, Option, Queue, Stream } from "effect"; import { resolveThreadWorkspaceCwd } from "../../checkpointing/Utils.ts"; -import { resolveAttachmentRelativePath, resolveAttachmentRoutePath } from "../../attachmentPaths.ts"; +import { + resolveAttachmentRelativePath, + resolveAttachmentRoutePath, +} from "../../attachmentPaths.ts"; import { ServerConfig } from "../../config.ts"; import { GitCore } from "../../git/Services/GitCore.ts"; import { TextGeneration } from "../../git/Services/TextGeneration.ts"; @@ -229,7 +232,8 @@ const make = Effect.gen(function* () { const existingSessionId = thread.session?.providerSessionId; if (existingSessionId) { const approvalPolicyChanged = - options?.approvalPolicy !== undefined && options.approvalPolicy !== thread.session?.approvalPolicy; + options?.approvalPolicy !== undefined && + options.approvalPolicy !== thread.session?.approvalPolicy; const sandboxModeChanged = options?.sandboxMode !== undefined && options.sandboxMode !== thread.session?.sandboxMode; @@ -237,7 +241,9 @@ const make = Effect.gen(function* () { return existingSessionId; } - const restartedSession = yield* startProviderSession(thread.session?.providerThreadId ?? null); + const restartedSession = yield* startProviderSession( + thread.session?.providerThreadId ?? null, + ); yield* bindSessionToThread(restartedSession); yield* providerService.stopSession({ sessionId: existingSessionId }).pipe( Effect.catchCause((cause) => @@ -286,13 +292,11 @@ const make = Effect.gen(function* () { }); }); - const resolvePersistedAttachments = Effect.fnUntraced(function* ( - input: { - readonly threadId: ThreadId; - readonly messageId: string; - readonly attachments: ReadonlyArray | undefined; - }, - ) { + const resolvePersistedAttachments = Effect.fnUntraced(function* (input: { + readonly threadId: ThreadId; + readonly messageId: string; + readonly attachments: ReadonlyArray | undefined; + }) { if (!input.attachments || input.attachments.length === 0) { return input.attachments ?? []; } @@ -303,54 +307,52 @@ const make = Effect.gen(function* () { const threadSegment = encodeURIComponent(input.threadId); const messageSegment = encodeURIComponent(input.messageId); - return yield* Effect.forEach( - Array.from(input.attachments.entries()), - ([index, attachment]) => - Effect.gen(function* () { - if (attachment.type !== "image") { - return attachment; - } + return yield* Effect.forEach(Array.from(input.attachments.entries()), ([index, attachment]) => + Effect.gen(function* () { + if (attachment.type !== "image") { + return attachment; + } - const resolvedRoutePath = resolveAttachmentRoutePath({ - stateDir: serverConfig.value.stateDir, - dataUrl: attachment.dataUrl, - }); - const resolvedMaterializedPath = - resolvedRoutePath ?? - (() => { - const parsed = parseBase64DataUrl(attachment.dataUrl); - if (!parsed || !parsed.mimeType.startsWith("image/")) { - return null; - } - const extension = inferImageExtension({ - mimeType: parsed.mimeType, - fileName: attachment.name, - }); - return resolveAttachmentRelativePath({ - stateDir: serverConfig.value.stateDir, - relativePath: `${threadSegment}/${messageSegment}-${index}${extension}`, - }); - })(); - - if (!resolvedMaterializedPath) { - return attachment; - } - - const isFile = yield* Effect.sync(() => { - try { - return fs.statSync(resolvedMaterializedPath).isFile(); - } catch { - return false; + const resolvedRoutePath = resolveAttachmentRoutePath({ + stateDir: serverConfig.value.stateDir, + dataUrl: attachment.dataUrl, + }); + const resolvedMaterializedPath = + resolvedRoutePath ?? + (() => { + const parsed = parseBase64DataUrl(attachment.dataUrl); + if (!parsed || !parsed.mimeType.startsWith("image/")) { + return null; } - }); - if (!isFile) { - return attachment; + const extension = inferImageExtension({ + mimeType: parsed.mimeType, + fileName: attachment.name, + }); + return resolveAttachmentRelativePath({ + stateDir: serverConfig.value.stateDir, + relativePath: `${threadSegment}/${messageSegment}-${index}${extension}`, + }); + })(); + + if (!resolvedMaterializedPath) { + return attachment; + } + + const isFile = yield* Effect.sync(() => { + try { + return fs.statSync(resolvedMaterializedPath).isFile(); + } catch { + return false; } - return { - ...attachment, - dataUrl: resolvedMaterializedPath, - } satisfies ChatAttachment; - }), + }); + if (!isFile) { + return attachment; + } + return { + ...attachment, + dataUrl: resolvedMaterializedPath, + } satisfies ChatAttachment; + }), ); }); @@ -382,51 +384,45 @@ const make = Effect.gen(function* () { const oldBranch = input.branch; const cwd = input.worktreePath; const attachments = input.attachments ?? []; - const renameEffect = textGeneration + yield* textGeneration .generateBranchName({ cwd, message: input.messageText, ...(attachments.length > 0 ? { attachments } : {}), }) .pipe( + Effect.catch((error) => + Effect.logWarning( + "provider command reactor failed to generate worktree branch name; skipping rename", + { threadId: input.threadId, cwd, oldBranch, reason: error.message }, + ), + ), Effect.flatMap((generated) => { - if (!generated.branch) { - return Effect.void; - } + if (!generated) return Effect.void; + const targetBranch = buildGeneratedWorktreeBranchName(generated.branch); - if (targetBranch === oldBranch) { - return Effect.void; - } - return git - .renameBranch({ - cwd, - oldBranch, - newBranch: targetBranch, - }) - .pipe( - Effect.flatMap((renamed) => - orchestrationEngine.dispatch({ - type: "thread.meta.update", - commandId: serverCommandId("worktree-branch-rename"), - threadId: input.threadId, - branch: renamed.branch, - worktreePath: cwd, - }), - ), - Effect.asVoid, - ); + if (targetBranch === oldBranch) return Effect.void; + + return Effect.flatMap( + git.renameBranch({ cwd, oldBranch, newBranch: targetBranch }), + (renamed) => + orchestrationEngine.dispatch({ + type: "thread.meta.update", + commandId: serverCommandId("worktree-branch-rename"), + threadId: input.threadId, + branch: renamed.branch, + worktreePath: cwd, + }), + ); }), Effect.catchCause((cause) => - Effect.logWarning("provider command reactor failed to generate or rename worktree branch", { - threadId: input.threadId, - cwd, - oldBranch, - cause: Cause.pretty(cause), - }), + Effect.logWarning( + "provider command reactor failed to generate or rename worktree branch", + { threadId: input.threadId, cwd, oldBranch, cause: Cause.pretty(cause) }, + ), ), + Effect.forkScoped, ); - - yield* Effect.forkScoped(renameEffect); }); const processTurnStartRequested = Effect.fnUntraced(function* (