From 6845f4520ee38bace4c99d4082893b4bd5004502 Mon Sep 17 00:00:00 2001 From: prateek Date: Sun, 15 Feb 2026 04:22:34 +0530 Subject: [PATCH] fix: address bugbot comments - deduplicate resolveProject, generic error messages (#39) * fix: cherry-pick unique improvements from PR #35 - Filter orchestrator session from worker session list - Enrich issue labels via tracker plugin in sessions API - Handle non-Bugbot review comments in SessionDetail - Conditional separator dots (no trailing dot when no next element) - Race condition fix in ttyd cleanup handlers - Input validation for session IDs in terminal server - Better error messages in terminal endpoint Co-Authored-By: Claude Opus 4.6 * fix: address bugbot comments on PR #39 - Extract duplicated project resolution logic into reusable resolveProject() helper - Return generic error message to API clients instead of exposing internal error details Co-Authored-By: Claude Sonnet 4.5 --------- Co-authored-by: Claude Opus 4.6 --- packages/web/src/app/api/sessions/route.ts | 53 +++++++++++++------ packages/web/src/components/SessionDetail.tsx | 34 ++++++++---- packages/web/src/server/terminal-websocket.ts | 38 ++++++++----- 3 files changed, 86 insertions(+), 39 deletions(-) diff --git a/packages/web/src/app/api/sessions/route.ts b/packages/web/src/app/api/sessions/route.ts index 8f32d86e5..b717cdef2 100644 --- a/packages/web/src/app/api/sessions/route.ts +++ b/packages/web/src/app/api/sessions/route.ts @@ -1,6 +1,27 @@ +import type { Session, ProjectConfig } from "@agent-orchestrator/core"; import { NextResponse } from "next/server"; -import { getServices, getSCM } from "@/lib/services"; -import { sessionToDashboard, enrichSessionPR, computeStats } from "@/lib/serialize"; +import { getServices, getSCM, getTracker } from "@/lib/services"; +import { sessionToDashboard, enrichSessionPR, enrichSessionIssue, computeStats } from "@/lib/serialize"; + +/** Resolve which project a session belongs to. */ +function resolveProject( + core: Session, + projects: Record, +): ProjectConfig | undefined { + // Try explicit projectId first + const direct = projects[core.projectId]; + if (direct) return direct; + + // Match by session prefix + const entry = Object.entries(projects).find(([, p]) => + core.id.startsWith(p.sessionPrefix), + ); + if (entry) return entry[1]; + + // Fall back to first project + const firstKey = Object.keys(projects)[0]; + return firstKey ? projects[firstKey] : undefined; +} /** GET /api/sessions — List all sessions with full state */ export async function GET() { @@ -8,23 +29,23 @@ export async function GET() { const { config, registry, sessionManager } = await getServices(); const coreSessions = await sessionManager.list(); - const dashboardSessions = coreSessions.map(sessionToDashboard); + // Filter out special orchestrator session - it's not a worker session + const workerSessions = coreSessions.filter((s) => s.id !== "orchestrator"); + const dashboardSessions = workerSessions.map(sessionToDashboard); + + // Enrich issue labels using tracker plugin (synchronous) + workerSessions.forEach((core, i) => { + if (!dashboardSessions[i].issueUrl) return; + const project = resolveProject(core, config.projects); + const tracker = getTracker(registry, project); + if (!tracker || !project) return; + enrichSessionIssue(dashboardSessions[i], tracker, project); + }); // Enrich sessions that have PRs with live SCM data (CI, reviews, mergeability) - const enrichPromises = coreSessions.map((core, i) => { + const enrichPromises = workerSessions.map((core, i) => { if (!core.pr) return Promise.resolve(); - // Try explicit projectId, then match by session prefix, then first project - let project = config.projects[core.projectId]; - if (!project) { - const projectEntry = Object.entries(config.projects).find(([, p]) => - core.id.startsWith(p.sessionPrefix), - ); - if (projectEntry) project = projectEntry[1]; - } - if (!project) { - const firstKey = Object.keys(config.projects)[0]; - if (firstKey) project = config.projects[firstKey]; - } + const project = resolveProject(core, config.projects); const scm = getSCM(registry, project); if (!scm) return Promise.resolve(); return enrichSessionPR(dashboardSessions[i], scm, core.pr); diff --git a/packages/web/src/components/SessionDetail.tsx b/packages/web/src/components/SessionDetail.tsx index 4da8b1df0..9fb568152 100644 --- a/packages/web/src/components/SessionDetail.tsx +++ b/packages/web/src/components/SessionDetail.tsx @@ -57,7 +57,7 @@ function humanizeLevel(level: string): string { case "respond": return "Needs Response"; case "review": - return "Pending Review"; + return "Needs Investigation"; case "pending": return "Pending"; case "working": @@ -80,17 +80,25 @@ function relativeTime(iso: string): string { return `${days}d ago`; } -/** Clean up Bugbot comment body - extract title and description, remove HTML junk */ +/** Clean up review comment body - extract title and description, remove HTML junk */ function cleanBugbotComment(body: string): { title: string; description: string } { - // Extract title (first ### heading) - const titleMatch = body.match(/###\s+(.+?)(?:\n|$)/); - const title = titleMatch ? titleMatch[1].replace(/\*\*/g, "").trim() : "Comment"; + // Check if this is a Bugbot comment (has structured markers) + const isBugbot = body.includes("") || body.includes("### "); - // Extract description between DESCRIPTION START/END comments - const descMatch = body.match(/\s*([\s\S]*?)\s*/); - const description = descMatch ? descMatch[1].trim() : body.split("\n")[0] || "No description"; + if (isBugbot) { + // Extract title (first ### heading) + const titleMatch = body.match(/###\s+(.+?)(?:\n|$)/); + const title = titleMatch ? titleMatch[1].replace(/\*\*/g, "").trim() : "Comment"; - return { title, description }; + // Extract description between DESCRIPTION START/END comments + const descMatch = body.match(/\s*([\s\S]*?)\s*/); + const description = descMatch ? descMatch[1].trim() : body.split("\n")[0] || "No description"; + + return { title, description }; + } else { + // For non-Bugbot comments, use full body as description + return { title: "Comment", description: body.trim() }; + } } /** Builds a GitHub branch URL from PR owner/repo/branch. */ @@ -217,7 +225,9 @@ export function SessionDetail({ session }: SessionDetailProps) { > #{pr.number} - · + {(session.branch || session.issueUrl) && ( + · + )} )} @@ -237,7 +247,9 @@ export function SessionDetail({ session }: SessionDetailProps) { {session.branch} )} - · + {session.issueUrl && ( + · + )} )} diff --git a/packages/web/src/server/terminal-websocket.ts b/packages/web/src/server/terminal-websocket.ts index ad1f463a9..8df2a2d4f 100644 --- a/packages/web/src/server/terminal-websocket.ts +++ b/packages/web/src/server/terminal-websocket.ts @@ -154,17 +154,24 @@ function getOrSpawnTtyd(sessionId: string): TtydInstance { // Use once() for cleanup handlers to prevent race condition when both exit and error fire proc.once("exit", (code) => { console.log(`[Terminal] ttyd ${sessionId} exited with code ${code}`); - instances.delete(sessionId); - // Recycle port for reuse - availablePorts.add(port); + // Only delete if this is still the current instance (prevents race with error handler) + const current = instances.get(sessionId); + if (current?.process === proc) { + instances.delete(sessionId); + // Recycle port for reuse + availablePorts.add(port); + } }); proc.once("error", (err) => { console.error(`[Terminal] ttyd ${sessionId} error:`, err.message); - // Clean up instance on spawn error to prevent leak - instances.delete(sessionId); - // Recycle port for reuse - availablePorts.add(port); + // Only delete if this is still the current instance (prevents race with exit handler) + const current = instances.get(sessionId); + if (current?.process === proc) { + instances.delete(sessionId); + // Recycle port for reuse + availablePorts.add(port); + } // Kill any running process try { proc.kill(); @@ -216,6 +223,13 @@ const server = createServer(async (req, res) => { return; } + // Validate session ID to prevent path traversal and injection + if (!/^[a-zA-Z0-9_-]+$/.test(sessionId)) { + res.writeHead(400, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ error: "Invalid session ID" })); + return; + } + // Validate session exists before spawning ttyd using configured dataDir const sessionPath = join(config.dataDir, sessionId); if (!existsSync(sessionPath)) { @@ -224,10 +238,9 @@ const server = createServer(async (req, res) => { return; } - const instance = getOrSpawnTtyd(sessionId); - - // Wait for ttyd to be ready before returning the URL + // Spawn ttyd and wait for it to be ready (catch port exhaustion and startup failures) try { + const instance = getOrSpawnTtyd(sessionId); await waitForTtyd(instance.port, sessionId); // Use the request host to construct the terminal URL (supports remote access) @@ -241,9 +254,10 @@ const server = createServer(async (req, res) => { sessionId, })); } catch (err) { - console.error(`[Terminal] ttyd ${sessionId} failed to become ready:`, err); + const errorMsg = err instanceof Error ? err.message : String(err); + console.error(`[Terminal] Failed to start terminal for ${sessionId}:`, errorMsg); res.writeHead(503, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Terminal server not ready" })); + res.end(JSON.stringify({ error: "Failed to start terminal" })); } return; }