fix: handle Windows drive-letter paths across the codebase (#148)

* Add metrics collection and terminal performance tests * fix: handle Windows drive-letter paths across the codebase Windows paths like C:\Users\foo\project were broken in multiple places: - agent-storage slugified D:\MyProject as D:-MyProject (illegal colon) - terminal-manager rejected all non-/ paths as relative - bootstrap parser misparsed drive colons as TCP host:port - daemon/client connection helpers misclassified Windows paths - CLI cwd filtering used hardcoded / separators - checkout-git worktree detection used hardcoded / in path checks - worktree archive used split("/").pop() instead of path.basename() All path helpers now normalize separators and handle Windows drive letters with case-insensitive comparison where needed.
2026-03-27 18:17:48 +07:00
parent c667aca3e0
commit ca443b3ecf
25 changed files with 980 additions and 49 deletions
@@ -231,6 +231,16 @@ export type ManagedAgent =
  | ManagedAgentError
  | ManagedAgentClosed;

+export interface AgentMetricsSnapshot {
+  total: number;
+  byLifecycle: Record<string, number>;
+  withActiveForegroundTurn: number;
+  timelineStats: {
+    totalItems: number;
+    maxItemsPerAgent: number;
+  };
+}
+
 type ActiveManagedAgent =
  | ManagedAgentInitializing
  | ManagedAgentIdle
@@ -345,6 +355,37 @@ export class AgentManager {
    this.onAgentAttention = callback;
  }

+  public getMetricsSnapshot(): AgentMetricsSnapshot {
+    const byLifecycle: Record<string, number> = {};
+    let withActiveForegroundTurn = 0;
+    let totalItems = 0;
+    let maxItemsPerAgent = 0;
+
+    for (const agent of this.agents.values()) {
+      byLifecycle[agent.lifecycle] = (byLifecycle[agent.lifecycle] ?? 0) + 1;
+
+      if (agent.activeForegroundTurnId !== null) {
+        withActiveForegroundTurn++;
+      }
+
+      const len = agent.timeline.length;
+      totalItems += len;
+      if (len > maxItemsPerAgent) {
+        maxItemsPerAgent = len;
+      }
+    }
+
+    return {
+      total: this.agents.size,
+      byLifecycle,
+      withActiveForegroundTurn,
+      timelineStats: {
+        totalItems,
+        maxItemsPerAgent,
+      },
+    };
+  }
+
  private touchUpdatedAt(agent: ManagedAgent): Date {
    const nowMs = Date.now();
    const previousMs = agent.updatedAt.getTime();
@@ -1,7 +1,7 @@
 import { describe, expect, test, beforeEach, afterEach } from "vitest";
 import os from "node:os";
 import path from "node:path";
-import { mkdtempSync, rmSync } from "node:fs";
+import { mkdtempSync, readdirSync, rmSync } from "node:fs";
 import { promises as fs } from "node:fs";

 import { createTestLogger } from "../../test-utils/test-logger.js";
@@ -352,6 +352,24 @@ describe("AgentStorage", () => {
    expect(records[0]?.internal).toBe(true);
  });

+  test("Windows drive-letter paths produce valid directory names", async () => {
+    await storage.applySnapshot(
+      createManagedAgent({
+        id: "win-agent",
+        cwd: "D:\\Users\\dev\\MyProject",
+      }),
+    );
+
+    const record = await storage.get("win-agent");
+    expect(record).not.toBeNull();
+
+    // The persisted directory must not contain a colon (invalid on Windows)
+    const dirs = readdirSync(storagePath);
+    expect(dirs).toHaveLength(1);
+    expect(dirs[0]).not.toContain(":");
+    expect(dirs[0]).toBe("D-Users-dev-MyProject");
+  });
+
  test("remove deletes all duplicate record files across project directories", async () => {
    const agentId = "agent-duplicate";

@@ -335,11 +335,16 @@ export class AgentStorage {
 }

 function projectDirNameFromCwd(cwd: string): string {
-  const trimmed = cwd.replace(/^[\\/]+/, "").replace(/[\\/]+$/, "");
-  if (!trimmed) {
-    return "root";
+  // path.win32.parse handles drive letters, UNC roots, and Unix roots on all platforms
+  const { root } = path.win32.parse(cwd);
+  const withoutRoot = cwd.slice(root.length).replace(/[\\/]+$/, "");
+  // Sanitize root: strip colons and separators, keep letters (e.g. "C:\" → "C", "\\server\share\" → "server-share")
+  const sanitizedRoot = root.replace(/[:\\/]+/g, "-").replace(/^-+|-+$/g, "");
+  const prefix = sanitizedRoot ? sanitizedRoot + "-" : "";
+  if (!withoutRoot) {
+    return sanitizedRoot || "root";
  }
-  return trimmed.replace(/[\\/]+/g, "-");
+  return prefix + withoutRoot.replace(/[\\/]+/g, "-");
 }

 async function writeFileAtomically(targetPath: string, payload: string) {
@@ -122,6 +122,23 @@ describe("paseo daemon bootstrap", () => {
    }
  });

+  test("parses whitespace-padded numeric port strings", () => {
+    expect(parseListenString(" 6767 ")).toEqual({
+      type: "tcp",
+      host: "127.0.0.1",
+      port: 6767,
+    });
+  });
+
+  test("rejects Windows absolute paths that are not named pipes", () => {
+    // A Windows drive path like C:\daemon must NOT be silently parsed as TCP
+    // (split(":") would yield host="C" and port="\\daemon" which is nonsensical).
+    expect(() => parseListenString(String.raw`C:\daemon`)).toThrow();
+    expect(() => parseListenString(String.raw`D:\Users\foo\.paseo\daemon.sock`)).toThrow();
+    // Single-letter "host" with no valid port is not a valid listen string
+    expect(() => parseListenString(String.raw`C:\some\path`)).toThrow();
+  });
+
  test("parses Windows named pipes as managed IPC listen targets", () => {
    expect(parseListenString(String.raw`\\.\pipe\paseo-managed-test`)).toEqual({
      type: "pipe",
@@ -35,34 +35,43 @@ function resolveBoundListenTarget(
  };
 }

+// Matches a Windows drive-letter path like C:\ or D:\
+const WINDOWS_DRIVE_RE = /^[A-Za-z]:\\/;
+
 export function parseListenString(listen: string): ListenTarget {
+  // 1. Windows named pipes: \\.\pipe\... or pipe://...
  if (listen.startsWith("\\\\.\\pipe\\") || listen.startsWith("pipe://")) {
    return {
      type: "pipe",
      path: listen.startsWith("pipe://") ? listen.slice("pipe://".length) : listen,
    };
  }
-  // Unix socket: starts with / or ~ or contains .sock
-  if (listen.startsWith("/") || listen.startsWith("~") || listen.includes(".sock")) {
-    return { type: "socket", path: listen };
-  }
-  // Explicit unix:// prefix
+  // 2. Explicit unix:// prefix
  if (listen.startsWith("unix://")) {
    return { type: "socket", path: listen.slice(7) };
  }
-  // TCP: host:port or just port
+  // 3. Reject Windows absolute drive paths — they are not Unix sockets
+  if (WINDOWS_DRIVE_RE.test(listen)) {
+    throw new Error(`Invalid listen string (Windows path is not a valid listen target): ${listen}`);
+  }
+  // 4. POSIX absolute path (/ or ~) — Unix socket
+  if (listen.startsWith("/") || listen.startsWith("~")) {
+    return { type: "socket", path: listen };
+  }
+  // 5. Pure numeric — TCP port on 127.0.0.1
+  const trimmed = listen.trim();
+  if (/^\d+$/.test(trimmed)) {
+    const port = parseInt(trimmed, 10);
+    return { type: "tcp", host: "127.0.0.1", port };
+  }
+  // 6. host:port — TCP
  if (listen.includes(":")) {
    const [host, portStr] = listen.split(":");
-    const port = parseInt(portStr, 10);
-    if (!Number.isFinite(port)) {
+    const parsedPort = parseInt(portStr, 10);
+    if (!Number.isFinite(parsedPort)) {
      throw new Error(`Invalid port in listen string: ${listen}`);
    }
-    return { type: "tcp", host: host || "127.0.0.1", port };
-  }
-  // Just a port number
-  const port = parseInt(listen, 10);
-  if (Number.isFinite(port)) {
-    return { type: "tcp", host: "127.0.0.1", port };
+    return { type: "tcp", host: host || "127.0.0.1", port: parsedPort };
  }
  throw new Error(`Invalid listen string: ${listen}`);
 }
@@ -291,6 +291,8 @@ export type SessionRuntimeMetrics = {
  checkoutDiffFallbackRefreshTargetCount: number;
  terminalDirectorySubscriptionCount: number;
  terminalSubscriptionCount: number;
+  inflightRequests: number;
+  peakInflightRequests: number;
 };

 type FetchAgentsRequestMessage = Extract<SessionInboundMessage, { type: "fetch_agents_request" }>;
@@ -610,6 +612,8 @@ export class Session {
  private readonly activeTerminalStreams = new Map<number, ActiveTerminalStream>();
  private readonly terminalIdToSlot = new Map<string, number>();
  private nextTerminalSlot = 0;
+  private inflightRequests = 0;
+  private peakInflightRequests = 0;
  private readonly checkoutDiffSubscriptions = new Map<string, { targetKey: string }>();
  private readonly checkoutDiffTargets = new Map<string, CheckoutDiffWatchTarget>();
  private readonly workspaceGitWatchTargets = new Map<string, WorkspaceGitWatchTarget>();
@@ -754,6 +758,8 @@ export class Session {
      checkoutDiffFallbackRefreshTargetCount,
      terminalDirectorySubscriptionCount: this.subscribedTerminalDirectories.size,
      terminalSubscriptionCount: this.activeTerminalStreams.size,
+      inflightRequests: this.inflightRequests,
+      peakInflightRequests: this.peakInflightRequests,
    };
  }

@@ -1454,6 +1460,11 @@ export class Session {
   * Main entry point for processing session messages
   */
  public async handleMessage(msg: SessionInboundMessage): Promise<void> {
+    this.inflightRequests++;
+    if (this.inflightRequests > this.peakInflightRequests) {
+      this.peakInflightRequests = this.inflightRequests;
+    }
+    try {
    this.sessionLogger.trace({ inbound: msg }, "inbound message");
    try {
      switch (msg.type) {
@@ -1779,6 +1790,13 @@ export class Session {
        },
      });
    }
+    } finally {
+      this.inflightRequests--;
+    }
+  }
+
+  public resetPeakInflight(): void {
+    this.peakInflightRequests = this.inflightRequests;
  }

  public handleBinaryFrame(frame: TerminalStreamFrame): void {
@@ -197,6 +197,7 @@ type WebSocketRuntimeCounters = {
  hostRejected: number;
 };

+const SLOW_REQUEST_THRESHOLD_MS = 500;
 const EXTERNAL_SESSION_DISCONNECT_GRACE_MS = 90_000;
 const HELLO_TIMEOUT_MS = 15_000;
 const WS_CLOSE_HELLO_TIMEOUT = 4001;
@@ -275,6 +276,7 @@ export class VoiceAssistantWebSocketServer {
  };
  private readonly inboundMessageCounts = new Map<string, number>();
  private readonly inboundSessionRequestCounts = new Map<string, number>();
+  private readonly requestLatencies = new Map<string, number[]>();
  private runtimeMetricsInterval: ReturnType<typeof setInterval> | null = null;

  constructor(
@@ -1056,7 +1058,21 @@ export class VoiceAssistantWebSocketServer {

      if (message.type === "session") {
        this.recordInboundSessionRequestType(message.message.type);
+        const startMs = performance.now();
        await activeConnection.session.handleMessage(message.message);
+        const durationMs = performance.now() - startMs;
+        this.recordRequestLatency(message.message.type, durationMs);
+
+        if (durationMs >= SLOW_REQUEST_THRESHOLD_MS) {
+          activeConnection.connectionLogger.warn(
+            {
+              requestType: message.message.type,
+              durationMs: Math.round(durationMs),
+              inflightRequests: activeConnection.session.getRuntimeMetrics().inflightRequests,
+            },
+            "ws_slow_request",
+          );
+        }
      }
    } catch (error) {
      const err = error instanceof Error ? error : new Error(String(error));
@@ -1147,10 +1163,49 @@ export class VoiceAssistantWebSocketServer {
    this.incrementCount(this.inboundSessionRequestCounts, type);
  }

+  private recordRequestLatency(type: string, durationMs: number): void {
+    let latencies = this.requestLatencies.get(type);
+    if (!latencies) {
+      latencies = [];
+      this.requestLatencies.set(type, latencies);
+    }
+    latencies.push(durationMs);
+  }
+
  private getTopCounts(map: Map<string, number>, limit: number): Array<[string, number]> {
    return [...map.entries()].sort((a, b) => b[1] - a[1]).slice(0, limit);
  }

+  private computeLatencyStats(): Array<{
+    type: string;
+    count: number;
+    minMs: number;
+    maxMs: number;
+    p50Ms: number;
+    totalMs: number;
+  }> {
+    const stats: Array<{
+      type: string;
+      count: number;
+      minMs: number;
+      maxMs: number;
+      p50Ms: number;
+      totalMs: number;
+    }> = [];
+    for (const [type, latencies] of this.requestLatencies) {
+      if (latencies.length === 0) continue;
+      latencies.sort((a, b) => a - b);
+      const count = latencies.length;
+      const minMs = Math.round(latencies[0]!);
+      const maxMs = Math.round(latencies[count - 1]!);
+      const p50Ms = Math.round(latencies[Math.floor(count / 2)]!);
+      const totalMs = Math.round(latencies.reduce((sum, v) => sum + v, 0));
+      stats.push({ type, count, minMs, maxMs, p50Ms, totalMs });
+    }
+    stats.sort((a, b) => b.totalMs - a.totalMs);
+    return stats.slice(0, 15);
+  }
+
  private collectSessionRuntimeMetrics(): SessionRuntimeMetrics {
    const uniqueConnections = new Set<SessionConnection>(this.externalSessionsByKey.values());
    let checkoutDiffTargetCount = 0;
@@ -1159,6 +1214,8 @@ export class VoiceAssistantWebSocketServer {
    let checkoutDiffFallbackRefreshTargetCount = 0;
    let terminalDirectorySubscriptionCount = 0;
    let terminalSubscriptionCount = 0;
+    let inflightRequests = 0;
+    let peakInflightRequests = 0;

    for (const connection of uniqueConnections) {
      const sessionMetrics = connection.session.getRuntimeMetrics();
@@ -1169,6 +1226,9 @@ export class VoiceAssistantWebSocketServer {
        sessionMetrics.checkoutDiffFallbackRefreshTargetCount;
      terminalDirectorySubscriptionCount += sessionMetrics.terminalDirectorySubscriptionCount;
      terminalSubscriptionCount += sessionMetrics.terminalSubscriptionCount;
+      inflightRequests += sessionMetrics.inflightRequests;
+      peakInflightRequests = Math.max(peakInflightRequests, sessionMetrics.peakInflightRequests);
+      connection.session.resetPeakInflight();
    }

    return {
@@ -1178,6 +1238,8 @@ export class VoiceAssistantWebSocketServer {
      checkoutDiffFallbackRefreshTargetCount,
      terminalDirectorySubscriptionCount,
      terminalSubscriptionCount,
+      inflightRequests,
+      peakInflightRequests,
    };
  }

@@ -1192,6 +1254,8 @@ export class VoiceAssistantWebSocketServer {
        connection.sockets.size === 0 && connection.externalDisconnectCleanupTimeout !== null,
    ).length;
    const sessionMetrics = this.collectSessionRuntimeMetrics();
+    const latencyStats = this.computeLatencyStats();
+    const agentSnapshot = this.agentManager.getMetricsSnapshot();

    this.logger.info(
      {
@@ -1210,6 +1274,8 @@ export class VoiceAssistantWebSocketServer {
        inboundMessageTypesTop: this.getTopCounts(this.inboundMessageCounts, 12),
        inboundSessionRequestTypesTop: this.getTopCounts(this.inboundSessionRequestCounts, 20),
        runtime: sessionMetrics,
+        latency: latencyStats,
+        agents: agentSnapshot,
      },
      "ws_runtime_metrics",
    );
@@ -1221,6 +1287,7 @@ export class VoiceAssistantWebSocketServer {
    }
    this.inboundMessageCounts.clear();
    this.inboundSessionRequestCounts.clear();
+    this.requestLatencies.clear();
    this.runtimeWindowStartedAt = now;
  }

@@ -73,6 +73,12 @@ describe("TerminalManager", () => {
      await expect(manager.getTerminals("tmp")).rejects.toThrow("cwd must be absolute path");
    });

+    it("accepts Windows absolute paths", async () => {
+      manager = createTerminalManager();
+      await expect(manager.getTerminals("C:\\Users\\foo\\project")).resolves.not.toThrow();
+      await expect(manager.getTerminals("D:\\MyProject")).resolves.not.toThrow();
+    });
+
    it("creates separate terminals for different cwds", async () => {
      manager = createTerminalManager();
      const tmpTerminals = [await manager.createTerminal({ cwd: "/tmp" })];
@@ -121,6 +127,18 @@ describe("TerminalManager", () => {
      );
    });

+    it("does not reject Windows absolute paths as relative", async () => {
+      manager = createTerminalManager();
+      // Should pass path validation (not throw "cwd must be absolute path").
+      // The terminal may or may not spawn successfully on non-Windows hosts,
+      // so we only assert the validation error is absent.
+      try {
+        await manager.createTerminal({ cwd: "C:\\Users\\foo\\project" });
+      } catch (error) {
+        expect((error as Error).message).not.toBe("cwd must be absolute path");
+      }
+    });
+
    it("inherits registered env for the worktree root cwd", async () => {
      await withShell("/bin/sh", async () => {
        manager = createTerminalManager();
@@ -1,5 +1,5 @@
 import { createTerminal, type TerminalSession } from "./terminal.js";
-import { resolve, sep } from "node:path";
+import { resolve, sep, win32, posix } from "node:path";

 export interface TerminalListItem {
  id: string;
@@ -37,7 +37,7 @@ export function createTerminalManager(): TerminalManager {
  const defaultEnvByRootCwd = new Map<string, Record<string, string>>();

  function assertAbsolutePath(cwd: string): void {
-    if (!cwd.startsWith("/")) {
+    if (!posix.isAbsolute(cwd) && !win32.isAbsolute(cwd)) {
      throw new Error("cwd must be absolute path");
    }
  }
@@ -18,6 +18,9 @@ import {
  NotGitRepoError,
  pushCurrentBranch,
  resolveRepositoryDefaultBranch,
+  parseWorktreeList,
+  isPaseoWorktreePath,
+  isDescendantPath,
 } from "./checkout-git.js";
 import { createWorktree } from "./worktree.js";
 import { getPaseoWorktreeMetadataPath } from "./worktree-metadata.js";
@@ -812,4 +815,73 @@ const x = 1;
    ).rejects.toThrow(/base/i);
    await expect(mergeToBase(worktree.worktreePath, {}, { paseoHome })).rejects.toThrow(/base/i);
  });
+
+  describe("parseWorktreeList", () => {
+    it("parses porcelain worktree output", () => {
+      const output = [
+        "worktree /home/user/repo",
+        "branch refs/heads/main",
+        "",
+        "worktree /home/user/.paseo/worktrees/feature",
+        "branch refs/heads/feature",
+        "",
+      ].join("\n");
+
+      const entries = parseWorktreeList(output);
+      expect(entries).toHaveLength(2);
+      expect(entries[0]).toEqual({ path: "/home/user/repo", branchRef: "refs/heads/main" });
+      expect(entries[1]).toEqual({
+        path: "/home/user/.paseo/worktrees/feature",
+        branchRef: "refs/heads/feature",
+      });
+    });
+
+    it("detects bare repos", () => {
+      const output = ["worktree /home/user/repo.git", "bare", ""].join("\n");
+      const entries = parseWorktreeList(output);
+      expect(entries).toHaveLength(1);
+      expect(entries[0]?.isBare).toBe(true);
+    });
+  });
+
+  describe("isPaseoWorktreePath", () => {
+    it("matches Unix .paseo/worktrees/ paths", () => {
+      expect(isPaseoWorktreePath("/home/user/.paseo/worktrees/feature")).toBe(true);
+    });
+
+    it("matches Windows .paseo\\worktrees\\ paths", () => {
+      expect(isPaseoWorktreePath("C:\\Users\\dev\\.paseo\\worktrees\\feature")).toBe(true);
+    });
+
+    it("rejects paths without .paseo/worktrees segment", () => {
+      expect(isPaseoWorktreePath("/home/user/repo")).toBe(false);
+      expect(isPaseoWorktreePath("C:\\Users\\dev\\repo")).toBe(false);
+    });
+  });
+
+  describe("isDescendantPath", () => {
+    it("detects children with Unix separators", () => {
+      expect(isDescendantPath("/home/user/repo/child", "/home/user/repo")).toBe(true);
+    });
+
+    it("detects children with Windows separators", () => {
+      expect(isDescendantPath("C:\\repos\\child", "C:\\repos")).toBe(true);
+    });
+
+    it("rejects the parent itself", () => {
+      expect(isDescendantPath("/home/user/repo", "/home/user/repo")).toBe(false);
+    });
+
+    it("rejects siblings that share a prefix", () => {
+      expect(isDescendantPath("/home/user/repo-extra", "/home/user/repo")).toBe(false);
+    });
+
+    it("handles mixed separators", () => {
+      expect(isDescendantPath("C:/repo/child", "C:\\repo")).toBe(true);
+    });
+
+    it("is case insensitive on Windows paths", () => {
+      expect(isDescendantPath("c:\\repo\\child", "C:\\repo")).toBe(true);
+    });
+  });
 });
@@ -603,20 +603,39 @@ async function getMainRepoRoot(cwd: string): Promise<string> {
  });
  const worktrees = parseWorktreeList(worktreeOut);
  const nonBareNonPaseo = worktrees.filter(
-    (wt) => !wt.isBare && !wt.path.includes("/.paseo/worktrees/"),
+    (wt) => !wt.isBare && !isPaseoWorktreePath(wt.path),
  );
-  const childrenOfBareRepo = nonBareNonPaseo.filter((wt) => wt.path.startsWith(normalized + "/"));
+  const childrenOfBareRepo = nonBareNonPaseo.filter((wt) => isDescendantPath(wt.path, normalized));
  const mainChild = childrenOfBareRepo.find((wt) => basename(wt.path) === "main");
  return mainChild?.path ?? childrenOfBareRepo[0]?.path ?? nonBareNonPaseo[0]?.path ?? normalized;
 }

-type GitWorktreeEntry = {
+export type GitWorktreeEntry = {
  path: string;
  branchRef?: string;
  isBare?: boolean;
 };

-function parseWorktreeList(output: string): GitWorktreeEntry[] {
+/** Check whether a path contains a `.paseo/worktrees/` segment (both `/` and `\`). */
+export function isPaseoWorktreePath(p: string): boolean {
+  return /[/\\]\.paseo[/\\]worktrees[/\\]/.test(p);
+}
+
+/** True when `child` is strictly inside `parent` (handles both `/` and `\`). */
+export function isDescendantPath(child: string, parent: string): boolean {
+  let c = child.replace(/\\/g, "/").replace(/\/+$/, "");
+  let p = parent.replace(/\\/g, "/").replace(/\/+$/, "");
+  // Case-insensitive on Windows (drive letter like C: or D:)
+  if (/^[A-Za-z]:/.test(c) || /^[A-Za-z]:/.test(p)) {
+    c = c.toLowerCase();
+    p = p.toLowerCase();
+  }
+  if (!c.startsWith(p)) return false;
+  if (c.length === p.length) return false;
+  return c[p.length] === "/";
+}
+
+export function parseWorktreeList(output: string): GitWorktreeEntry[] {
  const entries: GitWorktreeEntry[] = [];
  let current: GitWorktreeEntry | null = null;
  for (const line of output.split("\n")) {