From 8e744269a2edb2e51d3fa81e576fc29e64d5b186 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 18 Feb 2026 11:03:09 -0800 Subject: [PATCH] Address latest PR review comments Co-authored-by: codex --- apps/server/src/codexAppServerManager.test.ts | 34 +++++++ apps/server/src/codexAppServerManager.ts | 92 +++++++++++-------- .../src/filesystemCheckpointStore.test.ts | 7 ++ apps/server/src/filesystemCheckpointStore.ts | 12 ++- apps/web/src/store.test.ts | 24 +++-- apps/web/src/store.ts | 30 ++++++ 6 files changed, 148 insertions(+), 51 deletions(-) diff --git a/apps/server/src/codexAppServerManager.test.ts b/apps/server/src/codexAppServerManager.test.ts index 59077ce7870f8c308c8773947cf4b1594b5842f1..afb23d48a768f7c1a52b386659e1e919625682f1 100644 --- a/apps/server/src/codexAppServerManager.test.ts +++ b/apps/server/src/codexAppServerManager.test.ts @@ -143,6 +143,40 @@ describe("isRecoverableThreadResumeError", () => { }); }); +describe("startSession", () => { + it("emits session/startFailed when resolving cwd throws before process launch", async () => { + const manager = new CodexAppServerManager(); + const events: Array<{ method: string; kind: string; message?: string }> = []; + manager.on("event", (event) => { + events.push({ + method: event.method, + kind: event.kind, + ...(event.message ? { message: event.message } : {}), + }); + }); + + const processCwd = vi.spyOn(process, "cwd").mockImplementation(() => { + throw new Error("cwd missing"); + }); + try { + await expect( + manager.startSession({ + provider: "codex", + }), + ).rejects.toThrow("cwd missing"); + expect(events).toHaveLength(1); + expect(events[0]).toEqual({ + method: "session/startFailed", + kind: "error", + message: "cwd missing", + }); + } finally { + processCwd.mockRestore(); + manager.stopAll(); + } + }); +}); + describe("sendTurn", () => { it("sends text and image user input items to turn/start", async () => { const { manager, context, requireSession, sendRequest, updateSession } = diff --git a/apps/server/src/codexAppServerManager.ts b/apps/server/src/codexAppServerManager.ts index c3352ccb249c6c54c52389bcb0bf3b0b47c0bb1c..406a41a082b7a2ba28aef99ea97be1ffe3c248ad 100644 --- a/apps/server/src/codexAppServerManager.ts +++ b/apps/server/src/codexAppServerManager.ts @@ -148,41 +148,43 @@ export class CodexAppServerManager extends EventEmitter { const sessionId = randomUUID(); const now = new Date().toISOString(); - const resolvedCwd = input.cwd ?? process.cwd(); - - const session: ProviderSession = { - sessionId, - provider: "codex", - status: "connecting", - model: normalizeCodexModelSlug(input.model), - cwd: resolvedCwd, - createdAt: now, - updatedAt: now, - }; - - const child = spawn("codex", ["app-server"], { - cwd: resolvedCwd, - env: process.env, - stdio: ["pipe", "pipe", "pipe"], - }); - const output = readline.createInterface({ input: child.stdout }); - - const context: CodexSessionContext = { - session, - child, - output, - pending: new Map(), - pendingApprovals: new Map(), - nextRequestId: 1, - stopping: false, - }; - - this.sessions.set(sessionId, context); - this.attachProcessListeners(context); - - this.emitLifecycleEvent(context, "session/connecting", "Starting codex app-server"); + let context: CodexSessionContext | undefined; try { + const resolvedCwd = input.cwd ?? process.cwd(); + + const session: ProviderSession = { + sessionId, + provider: "codex", + status: "connecting", + model: normalizeCodexModelSlug(input.model), + cwd: resolvedCwd, + createdAt: now, + updatedAt: now, + }; + + const child = spawn("codex", ["app-server"], { + cwd: resolvedCwd, + env: process.env, + stdio: ["pipe", "pipe", "pipe"], + }); + const output = readline.createInterface({ input: child.stdout }); + + context = { + session, + child, + output, + pending: new Map(), + pendingApprovals: new Map(), + nextRequestId: 1, + stopping: false, + }; + + this.sessions.set(sessionId, context); + this.attachProcessListeners(context); + + this.emitLifecycleEvent(context, "session/connecting", "Starting codex app-server"); + await this.sendRequest(context, "initialize", { clientInfo: { name: "t3code_desktop", @@ -252,12 +254,24 @@ export class CodexAppServerManager extends EventEmitter { await rm(cwd, { recursive: true, force: true }); } }); + + it("returns false when repository path does not exist", async () => { + const store = new FilesystemCheckpointStore(); + const cwd = path.join(os.tmpdir(), `t3-fs-checkpoints-missing-${Date.now()}`); + + expect(await store.isGitRepository(cwd)).toBe(false); + }); }); diff --git a/apps/server/src/filesystemCheckpointStore.ts b/apps/server/src/filesystemCheckpointStore.ts index 2de2a65b27e39d02f7ea99ceeaa097c8459c2c34..7b517f4fe93d08d40f8e5882e189f0cafaf608ab 100644 --- a/apps/server/src/filesystemCheckpointStore.ts +++ b/apps/server/src/filesystemCheckpointStore.ts @@ -32,10 +32,14 @@ interface GitOptions { export class FilesystemCheckpointStore { async isGitRepository(cwd: string): Promise { - const result = await this.runGit(cwd, ["rev-parse", "--is-inside-work-tree"], { - allowNonZeroExit: true, - }); - return result.code === 0 && result.stdout.trim() === "true"; + try { + const result = await this.runGit(cwd, ["rev-parse", "--is-inside-work-tree"], { + allowNonZeroExit: true, + }); + return result.code === 0 && result.stdout.trim() === "true"; + } catch { + return false; + } } async captureCheckpoint(input: { diff --git a/apps/web/src/store.test.ts b/apps/web/src/store.test.ts index 625934f04e2fce8d9e9897f60f86ed27364a3ca7..cbdd5617bb3ae82b0483a7c532042aafe74b094d 100644 --- a/apps/web/src/store.test.ts +++ b/apps/web/src/store.test.ts @@ -618,10 +618,10 @@ describe("store reducer thread continuity", () => { ], activeThreadId: "thread-b", runtimeMode: "full-access", - diffOpen: false, - diffThreadId: null, - diffTurnId: null, - diffFilePath: null, + diffOpen: true, + diffThreadId: "thread-b", + diffTurnId: "turn-b", + diffFilePath: "src/removed.ts", }; const next = reducer(state, { @@ -646,6 +646,10 @@ describe("store reducer thread continuity", () => { expect(next.threads[0]?.id).toBe("thread-a"); expect(next.threads[0]?.projectId).toBe("project-new-a"); expect(next.activeThreadId).toBe("thread-a"); + expect(next.diffOpen).toBe(false); + expect(next.diffThreadId).toBeNull(); + expect(next.diffTurnId).toBeNull(); + expect(next.diffFilePath).toBeNull(); }); it("treats empty scripts from sync as authoritative", () => { @@ -753,10 +757,10 @@ describe("store reducer thread continuity", () => { ], activeThreadId: "thread-a", runtimeMode: "full-access", - diffOpen: false, - diffThreadId: null, - diffTurnId: null, - diffFilePath: null, + diffOpen: true, + diffThreadId: "thread-a", + diffTurnId: "turn-a", + diffFilePath: "src/a.ts", }; const next = reducer(state, { @@ -769,6 +773,10 @@ describe("store reducer thread continuity", () => { expect(next.threads).toHaveLength(1); expect(next.threads[0]?.id).toBe("thread-b"); expect(next.activeThreadId).toBe("thread-b"); + expect(next.diffOpen).toBe(false); + expect(next.diffThreadId).toBeNull(); + expect(next.diffTurnId).toBeNull(); + expect(next.diffFilePath).toBeNull(); }); it("marks the active thread as visited when selected", () => { diff --git a/apps/web/src/store.ts b/apps/web/src/store.ts index 06f3e10523a2621d082701d6dcc7fcacd2f5b99e..439368cf91565b7657b3f87d7dc6e1cef5dcc800 100644 --- a/apps/web/src/store.ts +++ b/apps/web/src/store.ts @@ -198,6 +198,32 @@ function updateThread( return threads.map((t) => (t.id === threadId ? updater(t) : t)); } +function resetDiffTargetIfMissing(state: AppState, threads: Thread[]) { + if (!state.diffThreadId) { + return { + diffOpen: state.diffOpen, + diffThreadId: state.diffThreadId, + diffTurnId: state.diffTurnId, + diffFilePath: state.diffFilePath, + }; + } + const hasThread = threads.some((thread) => thread.id === state.diffThreadId); + if (hasThread) { + return { + diffOpen: state.diffOpen, + diffThreadId: state.diffThreadId, + diffTurnId: state.diffTurnId, + diffFilePath: state.diffFilePath, + }; + } + return { + diffOpen: false, + diffThreadId: null, + diffTurnId: null, + diffFilePath: null, + }; +} + function mergeTurnDiffSummaries( existing: Thread["turnDiffSummaries"], next: Thread["turnDiffSummaries"], @@ -602,12 +628,14 @@ export function reducer(state: AppState, action: Action): AppState { const activeThreadId = nextThreads.some((thread) => thread.id === state.activeThreadId) ? state.activeThreadId : (nextThreads[0]?.id ?? null); + const diffState = resetDiffTargetIfMissing(state, nextThreads); return { ...state, projects: nextProjects, threads: nextThreads, activeThreadId, + ...diffState, }; } @@ -629,12 +657,14 @@ export function reducer(state: AppState, action: Action): AppState { const activeThreadId = threads.some((thread) => thread.id === state.activeThreadId) ? state.activeThreadId : (threads[0]?.id ?? null); + const diffState = resetDiffTargetIfMissing(state, threads); return { ...state, projects, threads, activeThreadId, + ...diffState, }; } -- 2.54.0