Merge pull request #20 from aaditagrawal/code-quality

chore: code quality cleanup (Snow Leopard pass)

Author: aaditagrawal

AGENTS.md

@@ -30,7 +30,7 @@ If a tradeoff is required, choose correctness and robustness over short-term con
 
 ## Maintainability
 
-Long term maintainability is a core priority. If you add new functionality, first check if there is shared logic that can be extracted to a separate module. Duplicate logic across multiple files is a code smell and should be avoided. Don't be afraid to change existing code. Don't take shortcuts by just adding local logic to solve a problem.
+Long-term maintainability is a core priority. If you add new functionality, first check if there is shared logic that can be extracted to a separate module. Duplicate logic across multiple files is a code smell and should be avoided. Don't be afraid to change existing code. Don't take shortcuts by just adding local logic to solve a problem.
 
 ## Package Roles
 

apps/desktop/src/main.ts

@@ -1230,7 +1230,7 @@ function registerIpcHandlers(): void {
   ipcMain.handle(LOG_LIST_CHANNEL, async () => {
     try {
       const entries = await FS.promises.readdir(LOG_DIR);
-      return entries.filter((f) => f.endsWith(".log")).sort();
+      return entries.filter((f) => f.endsWith(".log")).toSorted();
     } catch {
       return [];
     }

apps/server/src/ampServerManager.ts

@@ -355,6 +355,7 @@ export class AmpServerManager extends EventEmitter<{
     } catch (err) {
       throw new Error(
         `Failed to write to AMP stdin for session ${input.threadId}: ${err instanceof Error ? err.message : String(err)}`,
+        { cause: err },
       );
     }
 

apps/server/src/codexAppServerManager.ts

@@ -27,6 +27,7 @@ import {
   isCodexCliVersionSupported,
   parseCodexCliVersion,
 } from "./provider/codexCliVersion";
+import { createLogger } from "./logger";
 
 type PendingRequestKey = string;
 
@@ -514,6 +515,7 @@ export interface CodexAppServerManagerEvents {
 
 export class CodexAppServerManager extends EventEmitter<CodexAppServerManagerEvents> {
   private readonly sessions = new Map<ThreadId, CodexSessionContext>();
+  private readonly logger = createLogger("codex");
 
   private runPromise: (effect: Effect.Effect<unknown, never>) => Promise<unknown>;
   constructor(services?: ServiceMap.ServiceMap<never>) {
@@ -585,21 +587,21 @@ export class CodexAppServerManager extends EventEmitter<CodexAppServerManagerEve
       this.writeMessage(context, { method: "initialized" });
       try {
         const modelListResponse = await this.sendRequest(context, "model/list", {});
-        console.log("codex model/list response", modelListResponse);
+        this.logger.info("model/list response", { response: modelListResponse });
       } catch (error) {
-        console.log("codex model/list failed", error);
+        this.logger.warn("model/list failed", { error });
       }
       try {
         const accountReadResponse = await this.sendRequest(context, "account/read", {});
-        console.log("codex account/read response", accountReadResponse);
+        this.logger.info("account/read response", { response: accountReadResponse });
         context.account = readCodexAccountSnapshot(accountReadResponse);
-        console.log("codex subscription status", {
+        this.logger.info("subscription status", {
           type: context.account.type,
           planType: context.account.planType,
           sparkEnabled: context.account.sparkEnabled,
         });
       } catch (error) {
-        console.log("codex account/read failed", error);
+        this.logger.warn("account/read failed", { error });
       }
 
       const normalizedModel = resolveCodexModelForAccount(

apps/server/src/geminiCliServerManager.test.ts

@@ -1,5 +1,4 @@
 import { describe, expect, it, vi, beforeEach } from "vitest";
-import { EventEmitter } from "node:events";
 import { ThreadId, TurnId, type ProviderRuntimeEvent } from "@t3tools/contracts";
 
 import { GeminiCliServerManager } from "./geminiCliServerManager";
@@ -10,42 +9,6 @@ const asThreadId = (value: string): ThreadId => ThreadId.makeUnsafe(value);
 // Helpers to inspect spawned processes
 // ---------------------------------------------------------------------------
 
-/** Minimal mock for ChildProcess returned by `spawn`. */
-function createMockChildProcess() {
-  const stdout = new EventEmitter();
-  const stderr = new EventEmitter();
-  const child = Object.assign(new EventEmitter(), {
-    stdout,
-    stderr,
-    stdin: { write: vi.fn() },
-    kill: vi.fn(),
-    pid: 12345,
-  });
-  return child;
-}
-
-/** Capture spawn calls so we can inspect args & feed fake output. */
-function mockSpawn() {
-  const children: ReturnType<typeof createMockChildProcess>[] = [];
-  const spawnMock = vi.fn((_cmd: string, _args: string[]) => {
-    const child = createMockChildProcess();
-    children.push(child);
-    return child;
-  });
-  vi.doMock("node:child_process", () => ({ spawn: spawnMock }));
-  return { spawnMock, children };
-}
-
-/** Feed a line of JSON to the child's stdout as if it were a readline event. */
-function feedStdoutLine(
-  child: ReturnType<typeof createMockChildProcess>,
-  json: Record<string, unknown>,
-): void {
-  // Simulate readline "line" event by emitting data that includes a newline.
-  // Since we use readline.createInterface on stdout, we emit raw data.
-  child.stdout.emit("data", Buffer.from(JSON.stringify(json) + "\n"));
-}
-
 // ---------------------------------------------------------------------------
 // Unit tests — no real gemini process
 // ---------------------------------------------------------------------------
@@ -234,7 +197,7 @@ describe("GeminiCliServerManager", () => {
 
         const sessions = manager.listSessions();
         expect(sessions).toHaveLength(2);
-        expect(sessions.map((s) => s.threadId).sort()).toEqual(["thread-1", "thread-2"]);
+        expect(sessions.map((s) => s.threadId).toSorted()).toEqual(["thread-1", "thread-2"]);
       } finally {
         manager.stopAll();
       }
@@ -352,8 +315,8 @@ describe("GeminiCliServerManager JSON event mapping", () => {
     expect(events).toHaveLength(2);
     expect(events[0]?.type).toBe("content.delta");
     expect(events[0]?.provider).toBe("geminiCli");
-    expect((events[0]?.payload as { delta: string }).delta).toBe("Hello, ");
-    expect((events[0]?.payload as { streamKind: string }).streamKind).toBe("assistant_text");
+    expect((events[0]!.payload as { delta: string }).delta).toBe("Hello, ");
+    expect((events[0]!.payload as { streamKind: string }).streamKind).toBe("assistant_text");
 
     // Both deltas must share the same itemId for proper message aggregation.
     expect(events[1]?.type).toBe("content.delta");
@@ -660,7 +623,7 @@ describe.skipIf(!hasGemini || process.env.RUN_GEMINI_LIVE_TESTS !== "1")(
 
         // Turn should be completed successfully.
         const completed = events.find((e) => e.type === "turn.completed");
-        expect((completed?.payload as { state: string }).state).toBe("completed");
+        expect((completed!.payload as { state: string }).state).toBe("completed");
       } finally {
         manager.stopAll();
       }

apps/server/src/kilo/serverLifecycle.ts

@@ -58,14 +58,8 @@ export async function ensureServer(
   }
 
   const serverPromise = spawnOrConnect(options);
-  try {
-    const state = await serverPromise;
-    return { state, serverPromise };
-  } catch (error) {
-    // Propagate the error — the caller's finally block clears serverPromise
-    // when this.server is still undefined, enabling retry on next call.
-    throw error;
-  }
+  const state = await serverPromise;
+  return { state, serverPromise };
 }
 
 async function spawnOrConnect(options?: KiloProviderOptions): Promise<SharedServerState> {

apps/server/src/kilo/types.ts

@@ -1,5 +1,4 @@
 import type {
-  ProviderApprovalDecision,
   ProviderRuntimeEvent,
   ProviderSendTurnInput,
   ProviderSession,

apps/server/src/opencode/serverLifecycle.ts

@@ -57,14 +57,8 @@ export async function ensureServer(
   }
 
   const serverPromise = spawnOrConnect(options);
-  try {
-    const state = await serverPromise;
-    return { state, serverPromise };
-  } catch (error) {
-    // Propagate the error — the caller's finally block clears serverPromise
-    // when this.server is still undefined, enabling retry on next call.
-    throw error;
-  }
+  const state = await serverPromise;
+  return { state, serverPromise };
 }
 
 async function spawnOrConnect(options?: OpenCodeProviderOptions): Promise<SharedServerState> {

apps/server/src/opencode/types.ts

@@ -1,5 +1,4 @@
 import type {
-  ProviderApprovalDecision,
   ProviderRuntimeEvent,
   ProviderSendTurnInput,
   ProviderSession,

apps/server/src/orchestration/Layers/ProviderCommandReactor.ts

@@ -17,7 +17,6 @@ import { makeDrainableWorker } from "@t3tools/shared/DrainableWorker";
 
 import { resolveThreadWorkspaceCwd } from "../../checkpointing/Utils.ts";
 import { GitCore } from "../../git/Services/GitCore.ts";
-import { ProviderAdapterRequestError, ProviderServiceError } from "../../provider/Errors.ts";
 import { TextGeneration } from "../../git/Services/TextGeneration.ts";
 import { ProviderService } from "../../provider/Services/ProviderService.ts";
 import { OrchestrationEngineService } from "../Services/OrchestrationEngine.ts";
@@ -75,22 +74,6 @@ const DEFAULT_RUNTIME_MODE: RuntimeMode = "full-access";
 const WORKTREE_BRANCH_PREFIX = "t3code";
 const TEMP_WORKTREE_BRANCH_PATTERN = new RegExp(`^${WORKTREE_BRANCH_PREFIX}\\/[0-9a-f]{8}$`);
 
-function isUnknownPendingApprovalRequestError(cause: Cause.Cause<ProviderServiceError>): boolean {
-  const error = Cause.squash(cause);
-  if (Schema.is(ProviderAdapterRequestError)(error)) {
-    const detail = error.detail.toLowerCase();
-    return (
-      detail.includes("unknown pending approval request") ||
-      detail.includes("unknown pending permission request")
-    );
-  }
-  const message = Cause.pretty(cause);
-  return (
-    message.includes("unknown pending approval request") ||
-    message.includes("unknown pending permission request")
-  );
-}
-
 function isTemporaryWorktreeBranch(branch: string): boolean {
   return TEMP_WORKTREE_BRANCH_PATTERN.test(branch.trim().toLowerCase());
 }