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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

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<string, ProjectConfig>,
+): 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);

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("<!-- DESCRIPTION START -->") || body.includes("### ");
 
-  // Extract description between DESCRIPTION START/END comments
-  const descMatch = body.match(/<!-- DESCRIPTION START -->\s*([\s\S]*?)\s*<!-- DESCRIPTION END -->/);
-  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(/<!-- DESCRIPTION START -->\s*([\s\S]*?)\s*<!-- DESCRIPTION END -->/);
+    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}
                 </a>
-                <span className="text-[var(--color-text-muted)]">&middot;</span>
+                {(session.branch || session.issueUrl) && (
+                  <span className="text-[var(--color-text-muted)]">&middot;</span>
+                )}
               </>
             )}
 
@@ -237,7 +247,9 @@ export function SessionDetail({ session }: SessionDetailProps) {
                     {session.branch}
                   </span>
                 )}
-                <span className="text-[var(--color-text-muted)]">&middot;</span>
+                {session.issueUrl && (
+                  <span className="text-[var(--color-text-muted)]">&middot;</span>
+                )}
               </>
             )}
 

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;
   }