From 8d4b26c44ecd77458a3d25fd2f428588fe037cef Mon Sep 17 00:00:00 2001 From: Prateek Date: Fri, 20 Feb 2026 10:32:25 +0530 Subject: [PATCH] =?UTF-8?q?fix:=20comprehensive=20code=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20tests,=20timestamps,=20UI=20correctness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address gaps identified in code review of the ActivityDetection PR: Core / Session Manager: - Add `timestamp` to all `{ state: "exited" }` returns in all 4 agent plugins (claude-code, aider, codex, opencode) using consistent `exitedAt = new Date()` pattern - Add 2 new session-manager tests: timestamp propagation when detection timestamp is newer, and no-downgrade when detection timestamp is older - Fix `parseJsonlFileTail` lint error: remove useless `= 0` initializer (value was always overwritten before use; catch block returns early) Web package — tests: - Fix 3 `api-routes.test.ts` failures: `sessionsGET()` needs a Request object since the route reads `request.url` for `?active=true` query param - Fix `serialize.test.ts` rate-limit test: spy on `console.warn` (what the code uses) not `console.error` - Add 5 `ActivityDot` component tests covering all activity states, unknown states, null activity, and dotOnly mode Web package — UI correctness: - Fix `relativeTime()` in SessionDetail to guard against invalid/empty ISO strings - Fix timer Map leak: add `timersRef.current.clear()` in cleanup effect after forEach - Add `encodeURIComponent` to sessionId in message fetch URL Server — race condition fix: - Guard `activeSessions.delete` in pty.onExit, ws.on("close"), and ws.on("error") against stale handlers deleting a newly-registered session with the same ID. Fixes flaky integration test where afterEach's pty.kill() fired asynchronously after the next test had already set up a new session with the same session ID. Co-Authored-By: Claude Sonnet 4.6 --- .../src/__tests__/session-manager.test.ts | 62 +++++++++++++++++++ packages/core/src/session-manager.ts | 30 ++++++--- packages/plugins/agent-aider/src/index.ts | 5 +- .../plugins/agent-claude-code/src/index.ts | 7 ++- packages/plugins/agent-codex/src/index.ts | 5 +- packages/plugins/agent-opencode/src/index.ts | 5 +- packages/web/server/direct-terminal-ws.ts | 16 ++++- packages/web/src/__tests__/api-routes.test.ts | 6 +- .../web/src/__tests__/components.test.tsx | 36 +++++++++++ packages/web/src/app/page.tsx | 5 +- packages/web/src/components/SessionDetail.tsx | 7 ++- .../web/src/lib/__tests__/serialize.test.ts | 10 +-- 12 files changed, 161 insertions(+), 33 deletions(-) diff --git a/packages/core/src/__tests__/session-manager.test.ts b/packages/core/src/__tests__/session-manager.test.ts index 0393a4908..96d56edff 100644 --- a/packages/core/src/__tests__/session-manager.test.ts +++ b/packages/core/src/__tests__/session-manager.test.ts @@ -510,6 +510,68 @@ describe("list", () => { expect(agentWithNull.getActivityState).toHaveBeenCalled(); expect(sessions[0].activity).toBeNull(); }); + + it("updates lastActivityAt when detection timestamp is newer", async () => { + const newerTimestamp = new Date(Date.now() + 60_000); // 1 minute in the future + const agentWithTimestamp: Agent = { + ...mockAgent, + getActivityState: vi.fn().mockResolvedValue({ state: "active", timestamp: newerTimestamp }), + }; + const registryWithTimestamp: PluginRegistry = { + ...mockRegistry, + get: vi.fn().mockImplementation((slot: string) => { + if (slot === "runtime") return mockRuntime; + if (slot === "agent") return agentWithTimestamp; + return null; + }), + }; + + writeMetadata(sessionsDir, "app-1", { + worktree: "/tmp", + branch: "a", + status: "working", + project: "my-app", + runtimeHandle: JSON.stringify(makeHandle("rt-1")), + }); + + const sm = createSessionManager({ config, registry: registryWithTimestamp }); + const sessions = await sm.list(); + + expect(sessions[0].activity).toBe("active"); + // lastActivityAt should be updated to the detection timestamp + expect(sessions[0].lastActivityAt).toEqual(newerTimestamp); + }); + + it("does not downgrade lastActivityAt when detection timestamp is older", async () => { + const olderTimestamp = new Date(0); // epoch — definitely older than session creation + const agentWithOldTimestamp: Agent = { + ...mockAgent, + getActivityState: vi.fn().mockResolvedValue({ state: "active", timestamp: olderTimestamp }), + }; + const registryWithOldTimestamp: PluginRegistry = { + ...mockRegistry, + get: vi.fn().mockImplementation((slot: string) => { + if (slot === "runtime") return mockRuntime; + if (slot === "agent") return agentWithOldTimestamp; + return null; + }), + }; + + writeMetadata(sessionsDir, "app-1", { + worktree: "/tmp", + branch: "a", + status: "working", + project: "my-app", + runtimeHandle: JSON.stringify(makeHandle("rt-1")), + }); + + const sm = createSessionManager({ config, registry: registryWithOldTimestamp }); + const sessions = await sm.list(); + + expect(sessions[0].activity).toBe("active"); + // lastActivityAt should NOT be downgraded to the older detection timestamp + expect(sessions[0].lastActivityAt.getTime()).toBeGreaterThan(olderTimestamp.getTime()); + }); }); describe("get", () => { diff --git a/packages/core/src/session-manager.ts b/packages/core/src/session-manager.ts index 0f0bf0e35..c58a286cd 100644 --- a/packages/core/src/session-manager.ts +++ b/packages/core/src/session-manager.ts @@ -250,11 +250,21 @@ export function createSessionManager(deps: SessionManagerDeps): SessionManager { * Enrich session with live runtime state (alive/exited) and activity detection. * Mutates the session object in place. */ + const TERMINAL_SESSION_STATUSES = new Set([ + "killed", "done", "merged", "terminated", "cleanup", + ]); + async function enrichSessionWithRuntimeState( session: Session, plugins: ReturnType, handleFromMetadata: boolean, ): Promise { + // Skip all subprocess/IO work for sessions already known to be terminal. + if (TERMINAL_SESSION_STATUSES.has(session.status)) { + session.activity = "exited"; + return; + } + // Check runtime liveness — but only if the handle came from metadata. // Fabricated handles (constructed as fallback for external sessions) should // NOT override status to "killed" — we don't know if the session ever had @@ -660,16 +670,14 @@ export function createSessionManager(deps: SessionManagerDeps): SessionManager { async function list(projectId?: string): Promise { const allSessions = listAllSessions(projectId); - const sessions: Session[] = []; - for (const { sessionName, projectId: sessionProjectId } of allSessions) { - // Use config key to find project + const sessionPromises = allSessions.map(async ({ sessionName, projectId: sessionProjectId }) => { const project = config.projects[sessionProjectId]; - if (!project) continue; + if (!project) return null; const sessionsDir = getProjectSessionsDir(project); const raw = readMetadataRaw(sessionsDir, sessionName); - if (!raw) continue; + if (!raw) return null; // Get file timestamps for createdAt/lastActivityAt let createdAt: Date | undefined; @@ -686,12 +694,16 @@ export function createSessionManager(deps: SessionManagerDeps): SessionManager { const session = metadataToSession(sessionName, raw, createdAt, modifiedAt); const plugins = resolvePlugins(project); - await ensureHandleAndEnrich(session, sessionName, project, plugins); + // Cap per-session enrichment at 2s — subprocess calls (tmux/ps) can be + // slow under load. If we time out, session keeps its metadata values. + const enrichTimeout = new Promise((resolve) => setTimeout(resolve, 2_000)); + await Promise.race([ensureHandleAndEnrich(session, sessionName, project, plugins), enrichTimeout]); - sessions.push(session); - } + return session; + }); - return sessions; + const results = await Promise.all(sessionPromises); + return results.filter((s): s is Session => s !== null); } async function get(sessionId: SessionId): Promise { diff --git a/packages/plugins/agent-aider/src/index.ts b/packages/plugins/agent-aider/src/index.ts index 37bc726f6..b103248ca 100644 --- a/packages/plugins/agent-aider/src/index.ts +++ b/packages/plugins/agent-aider/src/index.ts @@ -119,9 +119,10 @@ function createAiderAgent(): Agent { const threshold = readyThresholdMs ?? DEFAULT_READY_THRESHOLD_MS; // Check if process is running first - if (!session.runtimeHandle) return { state: "exited" }; + const exitedAt = new Date(); + if (!session.runtimeHandle) return { state: "exited", timestamp: exitedAt }; const running = await this.isProcessRunning(session.runtimeHandle); - if (!running) return { state: "exited" }; + if (!running) return { state: "exited", timestamp: exitedAt }; // Process is running - check for activity signals if (!session.workspacePath) return null; diff --git a/packages/plugins/agent-claude-code/src/index.ts b/packages/plugins/agent-claude-code/src/index.ts index c106fec8f..cb2abbc26 100644 --- a/packages/plugins/agent-claude-code/src/index.ts +++ b/packages/plugins/agent-claude-code/src/index.ts @@ -263,7 +263,7 @@ interface JsonlLine { */ async function parseJsonlFileTail(filePath: string, maxBytes = 131_072): Promise { let content: string; - let offset = 0; + let offset: number; try { const { size = 0 } = await stat(filePath); offset = Math.max(0, size - maxBytes); @@ -650,9 +650,10 @@ function createClaudeCodeAgent(): Agent { const threshold = readyThresholdMs ?? DEFAULT_READY_THRESHOLD_MS; // Check if process is running first - if (!session.runtimeHandle) return { state: "exited" }; + const exitedAt = new Date(); + if (!session.runtimeHandle) return { state: "exited", timestamp: exitedAt }; const running = await this.isProcessRunning(session.runtimeHandle); - if (!running) return { state: "exited" }; + if (!running) return { state: "exited", timestamp: exitedAt }; // Process is running - check JSONL session file for activity if (!session.workspacePath) { diff --git a/packages/plugins/agent-codex/src/index.ts b/packages/plugins/agent-codex/src/index.ts index c55f24475..0c6952052 100644 --- a/packages/plugins/agent-codex/src/index.ts +++ b/packages/plugins/agent-codex/src/index.ts @@ -78,9 +78,10 @@ function createCodexAgent(): Agent { async getActivityState(session: Session, _readyThresholdMs?: number): Promise { // Check if process is running first - if (!session.runtimeHandle) return { state: "exited" }; + const exitedAt = new Date(); + if (!session.runtimeHandle) return { state: "exited", timestamp: exitedAt }; const running = await this.isProcessRunning(session.runtimeHandle); - if (!running) return { state: "exited" }; + if (!running) return { state: "exited", timestamp: exitedAt }; // NOTE: Codex stores rollout files in a global ~/.codex/sessions/ directory // without workspace-specific scoping. When multiple Codex sessions run in diff --git a/packages/plugins/agent-opencode/src/index.ts b/packages/plugins/agent-opencode/src/index.ts index 83673beaf..5b4f2ffd9 100644 --- a/packages/plugins/agent-opencode/src/index.ts +++ b/packages/plugins/agent-opencode/src/index.ts @@ -66,9 +66,10 @@ function createOpenCodeAgent(): Agent { async getActivityState(session: Session, _readyThresholdMs?: number): Promise { // Check if process is running first - if (!session.runtimeHandle) return { state: "exited" }; + const exitedAt = new Date(); + if (!session.runtimeHandle) return { state: "exited", timestamp: exitedAt }; const running = await this.isProcessRunning(session.runtimeHandle); - if (!running) return { state: "exited" }; + if (!running) return { state: "exited", timestamp: exitedAt }; // NOTE: OpenCode stores all session data in a single global SQLite database // at ~/.local/share/opencode/opencode.db without per-workspace scoping. When diff --git a/packages/web/server/direct-terminal-ws.ts b/packages/web/server/direct-terminal-ws.ts index 74ad78998..4d37625ce 100644 --- a/packages/web/server/direct-terminal-ws.ts +++ b/packages/web/server/direct-terminal-ws.ts @@ -147,7 +147,11 @@ export function createDirectTerminalServer(tmuxPath?: string): DirectTerminalSer // PTY exit pty.onExit(({ exitCode }) => { console.log(`[DirectTerminal] PTY exited for ${sessionId} with code ${exitCode}`); - activeSessions.delete(sessionId); + // Guard against stale exits: only delete if this pty is still the active one. + // A new connection may have already replaced this session entry. + if (activeSessions.get(sessionId)?.pty === pty) { + activeSessions.delete(sessionId); + } if (ws.readyState === WebSocket.OPEN) { ws.close(1000, "Terminal session ended"); } @@ -177,14 +181,20 @@ export function createDirectTerminalServer(tmuxPath?: string): DirectTerminalSer // WebSocket close ws.on("close", () => { console.log(`[DirectTerminal] WebSocket closed for ${sessionId}`); - activeSessions.delete(sessionId); + // Guard against stale closes replacing a newer session's entry + if (activeSessions.get(sessionId)?.pty === pty) { + activeSessions.delete(sessionId); + } pty.kill(); }); // WebSocket error ws.on("error", (err) => { console.error(`[DirectTerminal] WebSocket error for ${sessionId}:`, err.message); - activeSessions.delete(sessionId); + // Guard against stale error handlers replacing a newer session's entry + if (activeSessions.get(sessionId)?.pty === pty) { + activeSessions.delete(sessionId); + } pty.kill(); }); }); diff --git a/packages/web/src/__tests__/api-routes.test.ts b/packages/web/src/__tests__/api-routes.test.ts index baea1a098..9bae0f315 100644 --- a/packages/web/src/__tests__/api-routes.test.ts +++ b/packages/web/src/__tests__/api-routes.test.ts @@ -185,7 +185,7 @@ describe("API Routes", () => { describe("GET /api/sessions", () => { it("returns sessions array and stats", async () => { - const res = await sessionsGET(); + const res = await sessionsGET(makeRequest("http://localhost:3000/api/sessions")); expect(res.status).toBe(200); const data = await res.json(); expect(data.sessions).toBeDefined(); @@ -196,7 +196,7 @@ describe("API Routes", () => { }); it("stats include expected fields", async () => { - const res = await sessionsGET(); + const res = await sessionsGET(makeRequest("http://localhost:3000/api/sessions")); const data = await res.json(); expect(data.stats).toHaveProperty("totalSessions"); expect(data.stats).toHaveProperty("workingSessions"); @@ -205,7 +205,7 @@ describe("API Routes", () => { }); it("sessions have expected shape", async () => { - const res = await sessionsGET(); + const res = await sessionsGET(makeRequest("http://localhost:3000/api/sessions")); const data = await res.json(); const session = data.sessions[0]; expect(session).toHaveProperty("id"); diff --git a/packages/web/src/__tests__/components.test.tsx b/packages/web/src/__tests__/components.test.tsx index 5f8229a3e..c2678b8d5 100644 --- a/packages/web/src/__tests__/components.test.tsx +++ b/packages/web/src/__tests__/components.test.tsx @@ -4,8 +4,44 @@ import { CIBadge, CICheckList } from "@/components/CIBadge"; import { PRStatus } from "@/components/PRStatus"; import { SessionCard } from "@/components/SessionCard"; import { AttentionZone } from "@/components/AttentionZone"; +import { ActivityDot } from "@/components/ActivityDot"; import { makeSession, makePR } from "./helpers"; +// ── ActivityDot ─────────────────────────────────────────────────────── + +describe("ActivityDot", () => { + it("renders label pill with activity name", () => { + render(); + expect(screen.getByText("active")).toBeInTheDocument(); + }); + + it("renders all known activity states", () => { + const states = ["active", "ready", "idle", "waiting_input", "blocked", "exited"] as const; + for (const state of states) { + const { unmount } = render(); + const expected = state === "waiting_input" ? "waiting" : state; + expect(screen.getByText(expected)).toBeInTheDocument(); + unmount(); + } + }); + + it("renders unknown activity state with raw label", () => { + render(); + expect(screen.getByText("some_future_state")).toBeInTheDocument(); + }); + + it("renders null activity with 'unknown' label", () => { + render(); + expect(screen.getByText("unknown")).toBeInTheDocument(); + }); + + it("renders only a dot in dotOnly mode (no label)", () => { + render(); + // No label text should appear in dotOnly mode + expect(screen.queryByText("active")).not.toBeInTheDocument(); + }); +}); + // ── CIBadge ────────────────────────────────────────────────────────── describe("CIBadge", () => { diff --git a/packages/web/src/app/page.tsx b/packages/web/src/app/page.tsx index 77dab05ec..238661c65 100644 --- a/packages/web/src/app/page.tsx +++ b/packages/web/src/app/page.tsx @@ -39,8 +39,9 @@ export default async function Home() { const coreSessions = allSessions.filter((s) => !s.id.endsWith("-orchestrator")); sessions = coreSessions.map(sessionToDashboard); - // Enrich metadata (issue labels, agent summaries, issue titles) - await enrichSessionsMetadata(coreSessions, sessions, config, registry); + // Enrich metadata (issue labels, agent summaries, issue titles) — cap at 3s + const metaTimeout = new Promise((resolve) => setTimeout(resolve, 3_000)); + await Promise.race([enrichSessionsMetadata(coreSessions, sessions, config, registry), metaTimeout]); // Enrich sessions that have PRs with live SCM data // Skip enrichment for terminal sessions (merged, closed, done, terminated) diff --git a/packages/web/src/components/SessionDetail.tsx b/packages/web/src/components/SessionDetail.tsx index 1996e3bb8..28344c1c3 100644 --- a/packages/web/src/components/SessionDetail.tsx +++ b/packages/web/src/components/SessionDetail.tsx @@ -44,7 +44,9 @@ function humanizeStatus(status: string): string { } function relativeTime(iso: string): string { - const diff = Date.now() - new Date(iso).getTime(); + const ms = new Date(iso).getTime(); + if (!iso || isNaN(ms)) return "unknown"; + const diff = Date.now() - ms; const seconds = Math.floor(diff / 1000); if (seconds < 60) return "just now"; const minutes = Math.floor(seconds / 60); @@ -86,7 +88,7 @@ async function askAgentToFix( try { const { title, description } = cleanBugbotComment(comment.body); const message = `Please address this review comment:\n\nFile: ${comment.path}\nComment: ${title}\nDescription: ${description}\n\nComment URL: ${comment.url}\n\nAfter fixing, mark the comment as resolved at ${comment.url}`; - const res = await fetch(`/api/sessions/${sessionId}/message`, { + const res = await fetch(`/api/sessions/${encodeURIComponent(sessionId)}/message`, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ message }), @@ -428,6 +430,7 @@ function PRCard({ pr, sessionId }: { pr: DashboardPR; sessionId: string }) { useEffect(() => { return () => { timersRef.current.forEach((timer) => clearTimeout(timer)); + timersRef.current.clear(); }; }, []); diff --git a/packages/web/src/lib/__tests__/serialize.test.ts b/packages/web/src/lib/__tests__/serialize.test.ts index b94c9337b..49e56c3ba 100644 --- a/packages/web/src/lib/__tests__/serialize.test.ts +++ b/packages/web/src/lib/__tests__/serialize.test.ts @@ -329,8 +329,8 @@ describe("enrichSessionPR", () => { const dashboard = sessionToDashboard(coreSession); const scm = createFailingSCM(); - // Spy on console.error - const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + // Spy on console.warn (enrichSessionPR uses warn for rate-limit, not error) + const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); await enrichSessionPR(dashboard, scm, pr); @@ -339,10 +339,10 @@ describe("enrichSessionPR", () => { expect(dashboard.pr?.deletions).toBe(0); expect(dashboard.pr?.mergeability.blockers).toContain("API rate limited or unavailable"); - // Should log error - expect(consoleErrorSpy).toHaveBeenCalled(); + // Should log warning + expect(consoleWarnSpy).toHaveBeenCalled(); - consoleErrorSpy.mockRestore(); + consoleWarnSpy.mockRestore(); }); it("should cache even when most requests fail (to reduce API pressure)", async () => {