review: post-merge fixes for phases 1, 6, 2, 5

docs/follow-ups.md

@@ -0,0 +1,72 @@
+# Post-merge follow-ups
+
+Tracking nice-to-have items surfaced by the post-merge review pass on
+`review/post-merge-fixes`. None block production; each can be picked up by
+the next phase or a polish PR. Citations point at file:line in the merged
+commit; line numbers may drift after the review PR lands.
+
+## Phase 1 — Midnight skin + mobile-first
+
+- **Tasks slot missing from primary nav** — `useAppNavItems.ts:18-24` defines
+  five slots (Chat / Sessions / Preview / Browser / More). `AppContent.tsx:202`
+  honors `activeTab === 'tasks'` for accent, but no rail/tab-bar entry surfaces
+  the kanban view. The floating "Tasks" button in `MainContent.tsx` is the
+  only entry point. Decide: surface as a sixth slot or accept floating-only.
+- **Sheet initial focus** — `MobileSidebarSheet.tsx:65` opens with no initial
+  focus target, so screen readers land on the trigger. Move focus to the sheet
+  container or first interactive child on open.
+- **Drag-handle hit area** — `MobileSidebarSheet.tsx:84-92` is `pt-3 pb-2`
+  (~28px). Below the 44×44 minimum the rest of the app respects.
+- **`visualViewport` listener cleanup** — `AppContent.tsx:128-137` registers
+  once; consider also listening to `scroll` (iOS sometimes only fires that on
+  keyboard show) and clearing `--keyboard-height` if `vv` becomes undefined.
+
+## Phase 2 — Sidebar tree + repo grouping
+
+- **Native `prompt`/`confirm` for topic rename/delete** —
+  `SidebarTopicGroup.tsx:138`. Works but jarring vs. Midnight; replace with
+  inline editable chip + custom modal.
+- **Mobile search-scope segment uses ad-hoc styles** — `SidebarHeader.tsx`
+  mobile branch (~line 240). Desktop got the `.ds-segment` polish; apply the
+  same to mobile.
+- **Aria mismatch for single-project repos** — `SidebarProjectTree.tsx:189`:
+  when no group header renders, `aria-labelledby={groupId}` points at a
+  nonexistent button. Drop the attr in that branch.
+- **`crypto.randomUUID()` fallback** — `useTopicStorage.ts:106`. Add a
+  `Math.random()`-based fallback for insecure-context resilience.
+- **Repo-grouper cache invalidation** — `repo-grouper.js:333` keys by
+  `fullPath` only. Add a TTL or git-mtime check so origin URL changes are
+  picked up without manual cache deletion.
+
+## Phase 5 — Preview + Chrome + Worktrees + Tasks
+
+- **Worktree → session map is partial** — only the *currently selected*
+  worktree's dot can light up. Cross-worktree resolution requires the
+  server-side activity stream to know which sessionId is running in each
+  `<repo>/.claude/worktrees/<slug>` cwd. Add `/api/worktrees/active-sessions`
+  (or include `__cwd` on every session payload) and feed it into
+  `SessionActivityProvider` in `AppContent.tsx`.
+- **Tasks aside is fixed-width on desktop** — `MainContent.tsx:194-208` uses
+  `w-[380px]`. The phase-5 brief asks for a resizable right panel.
+- **Preview proxy CSP/X-Frame strip is unconditional** —
+  `preview-proxy.js:79-81` strips CSP and X-Frame-Options on every response.
+  In production deployments that's broader than needed — consider gating to
+  `process.env.NODE_ENV !== 'production'` or to the configured port allowlist.
+- **`SpawnSubAgentButton` SSE event narrowing** — `SpawnSubAgentButton.tsx:127`
+  casts `evt.event` straight to `StreamEvent['type']`, so a malformed server
+  event would silently produce a garbage payload. Narrow with a `switch`.
+- **`SessionFilesTouchedChips` layout shift** — `SessionFilesTouchedChips.tsx:88`
+  reserves no height before chips arrive; once the IntersectionObserver
+  resolves, the row jumps. Reserve a min-height.
+- **`spawn-sub-agent` has no time/output cap** — `mcp-bootstrap.js:259`. A
+  runaway sub-agent could stream forever. Add a hard timeout + byte cap.
+
+## Cross-cutting
+
+- **Bundle size warning** — main client chunk is 2.5 MB minified (~760 KB
+  gzipped). Vite warns above 1 MB. Worth a code-split pass: lazy-load
+  Preview/Browser/Tasks panes, split CodeMirror/xterm vendor chunks further.
+- **No `npm test` script** — Phase briefs reference test runs but
+  `package.json` has none. Add at minimum a Vitest harness with smoke tests
+  for the new server routes (preview-proxy port allowlist, mcp-bootstrap
+  workingDir validation, tasks path-traversal guard).

server/routes/chrome-screencast.js

@@ -8,6 +8,15 @@ import { authenticateWebSocket } from '../middleware/auth.js';
 const CDP_HOST = process.env.CHROME_CDP_HOST || '127.0.0.1';
 const CDP_PORT = Number(process.env.CHROME_CDP_PORT || 9222);
 
+// Opt-in flags. The screencast attaches to the operator's REAL Chrome over
+// CDP — any authenticated Dispatch user could otherwise read cookies via
+// Runtime.evaluate or drive the host browser (steal sessions, navigate to
+// attacker URLs, etc.). Default is fully off; operator must explicitly enable.
+//   DISPATCH_CHROME_VIEW_ENABLED=true        — turn the feature on (view-only)
+//   DISPATCH_CHROME_VIEW_ALLOW_INPUT=true    — additionally accept Input.* events
+const CHROME_VIEW_ENABLED = process.env.DISPATCH_CHROME_VIEW_ENABLED === 'true';
+const CHROME_VIEW_ALLOW_INPUT = process.env.DISPATCH_CHROME_VIEW_ALLOW_INPUT === 'true';
+
 function fetchChromeVersion() {
   return new Promise((resolve, reject) => {
     const req = http.get(
@@ -212,6 +221,16 @@ async function handleChromeScreencastConnection(clientWs) {
 
   clientWs.on('message', async (raw) => {
     if (!cdp) return;
+    if (!CHROME_VIEW_ALLOW_INPUT) {
+      // View-only mode (default). Drop any Input.* dispatch attempts so a
+      // logged-in user cannot drive the operator's real browser. The first
+      // such message gets an explicit notice; subsequent ones are silent.
+      safeSend(clientWs, {
+        type: 'input-disabled',
+        error: 'Input forwarding disabled. Set DISPATCH_CHROME_VIEW_ALLOW_INPUT=true to enable.',
+      });
+      return;
+    }
     let msg;
     try {
       msg = JSON.parse(raw.toString());
@@ -296,6 +315,12 @@ export function attachChromeScreencast(httpServer, rootWss) {
     const rawUrl = req.url || '';
     if (!rawUrl.startsWith('/ws/chrome-view')) return;
 
+    if (!CHROME_VIEW_ENABLED) {
+      socket.write('HTTP/1.1 403 Forbidden\r\n\r\n');
+      socket.destroy();
+      return;
+    }
+
     const parsed = new URL(rawUrl, 'http://localhost');
     const token =
       parsed.searchParams.get('token') ||
@@ -319,23 +344,39 @@ export function attachChromeScreencast(httpServer, rootWss) {
 const router = express.Router();
 
 router.get('/status', async (_req, res) => {
+  if (!CHROME_VIEW_ENABLED) {
+    return res.status(503).json({
+      ok: false,
+      enabled: false,
+      hint: 'Chrome viewer disabled. Set DISPATCH_CHROME_VIEW_ENABLED=true to opt in.',
+    });
+  }
   try {
     const version = await fetchChromeVersion();
     res.json({
       ok: true,
+      enabled: true,
+      inputEnabled: CHROME_VIEW_ALLOW_INPUT,
       browser: version.Browser,
       protocolVersion: version['Protocol-Version'],
     });
   } catch (err) {
     res.status(503).json({
       ok: false,
+      enabled: true,
       error: err && err.message ? err.message : 'CDP unreachable',
       hint: 'Launch Chrome with --remote-debugging-port=9222',
     });
   }
 });
 
 router.get('/tabs', async (_req, res) => {
+  if (!CHROME_VIEW_ENABLED) {
+    return res.status(503).json({
+      enabled: false,
+      hint: 'Chrome viewer disabled. Set DISPATCH_CHROME_VIEW_ENABLED=true to opt in.',
+    });
+  }
   try {
     const tabs = await fetchChromeTabs();
     res.json(

server/routes/mcp-bootstrap.js

@@ -80,6 +80,68 @@ async function readClaudeConfig() {
     return (await readJsonIfExists(CLAUDE_CONFIG_PATH)) || {};
 }
 
+// Returns true only when ~/.claude.json already exists. Bootstrap refuses to
+// materialize a brand-new config for users who have never run Claude — the
+// recommended-MCPs entries would be the *only* keys in that file, which the
+// user did not opt into. Once Claude itself is installed, the file appears
+// and the bootstrap may run on the next boot.
+async function claudeConfigExists() {
+    try {
+        await fs.access(CLAUDE_CONFIG_PATH);
+        return true;
+    } catch (err) {
+        if (err && err.code === 'ENOENT') return false;
+        throw err;
+    }
+}
+
+const ALLOWED_WORKING_DIR_ROOTS = (() => {
+    const roots = [os.homedir()];
+    if (process.env.DISPATCH_PROJECT_ROOTS) {
+        for (const candidate of process.env.DISPATCH_PROJECT_ROOTS.split(path.delimiter)) {
+            const trimmed = candidate.trim();
+            if (trimmed) roots.push(path.resolve(trimmed));
+        }
+    }
+    return roots.map((p) => path.resolve(p));
+})();
+
+const SENSITIVE_HOME_SUBDIRS = ['.ssh', '.aws', '.gnupg', '.config/gh', 'Library/Keychains'];
+
+// Returns the resolved absolute path if `input` is a usable working dir for a
+// sub-agent spawn, or { error } describing why it isn't. Conservative gate:
+// must be absolute, must resolve under HOME (or DISPATCH_PROJECT_ROOTS), and
+// must not point at well-known credential stores.
+async function resolveSafeWorkingDir(input) {
+    if (typeof input !== 'string' || !input.trim()) {
+        return { error: 'workingDir must be a non-empty string' };
+    }
+    if (input.includes('\0')) {
+        return { error: 'workingDir contains a null byte' };
+    }
+    const resolved = path.resolve(input);
+    const homeDir = path.resolve(os.homedir());
+    const underAllowedRoot = ALLOWED_WORKING_DIR_ROOTS.some((root) => {
+        return resolved === root || resolved.startsWith(root + path.sep);
+    });
+    if (!underAllowedRoot) {
+        return { error: 'workingDir must be under $HOME or DISPATCH_PROJECT_ROOTS' };
+    }
+    for (const subdir of SENSITIVE_HOME_SUBDIRS) {
+        const sensitive = path.resolve(homeDir, subdir);
+        if (resolved === sensitive || resolved.startsWith(sensitive + path.sep)) {
+            return { error: `workingDir resolves to a sensitive directory (${subdir})` };
+        }
+    }
+    try {
+        const stat = await fs.stat(resolved);
+        if (!stat.isDirectory()) return { error: 'workingDir is not a directory' };
+    } catch {
+        return { error: 'workingDir does not exist' };
+    }
+    return { path: resolved };
+}
+
 async function writeClaudeConfig(config) {
     await writeJsonAtomic(CLAUDE_CONFIG_PATH, config);
 }
@@ -98,6 +160,12 @@ function installedInConfig(config, name) {
  */
 export async function ensureRecommendedMCPs() {
     const state = await readDispatchState();
+    if (!(await claudeConfigExists())) {
+        // First-run users without Claude installed — do not materialize a
+        // ~/.claude.json containing only Dispatch's two MCPs. They get added
+        // on a later boot once the user has actually run Claude.
+        return { added: [], state, skipped: 'no-claude-config' };
+    }
     let config;
     try {
         config = await readClaudeConfig();
@@ -226,10 +294,16 @@ router.post('/recommended/:name/toggle', async (req, res) => {
 router.post('/spawn-sub-agent', async (req, res) => {
     const subAgentType = typeof req.body?.subAgentType === 'string' ? req.body.subAgentType : 'general-purpose';
     const userPrompt = typeof req.body?.prompt === 'string' ? req.body.prompt : '';
-    const workingDir = typeof req.body?.workingDir === 'string' && req.body.workingDir
+    const requestedWorkingDir = typeof req.body?.workingDir === 'string' && req.body.workingDir
         ? req.body.workingDir
         : process.cwd();
 
+    const validated = await resolveSafeWorkingDir(requestedWorkingDir);
+    if (validated.error) {
+        return res.status(400).json({ error: `Invalid workingDir: ${validated.error}` });
+    }
+    const workingDir = validated.path;
+
     res.setHeader('Content-Type', 'text/event-stream');
     res.setHeader('Cache-Control', 'no-cache');
     res.setHeader('Connection', 'keep-alive');
@@ -352,9 +426,13 @@ router.get('/session-files-touched/:projectName/:sessionId', async (req, res) =>
 });
 
 // Kick off bootstrap as a module-level side-effect so `import` in server/index.js
-// registers the MCPs without needing a second wiring line.
-ensureRecommendedMCPs().catch((err) => {
-    console.error('[mcp-bootstrap] ensureRecommendedMCPs failed:', err.message);
+// registers the MCPs without needing a second wiring line. Deferred via
+// setImmediate so the import chain is not blocked by file IO, and skipped when
+// ~/.claude.json doesn't yet exist (see ensureRecommendedMCPs).
+setImmediate(() => {
+    ensureRecommendedMCPs().catch((err) => {
+        console.error('[mcp-bootstrap] ensureRecommendedMCPs failed:', err.message);
+    });
 });
 
 export default router;

server/routes/preview-proxy.js

@@ -19,10 +19,47 @@ const HOP_BY_HOP_HEADERS = new Set([
   'upgrade',
 ]);
 
+// Default-deny common service ports even within the dev range. Postgres,
+// MySQL, Redis, MongoDB, Memcached, Elasticsearch, Kafka, the CDP itself,
+// and a few other infra defaults — proxying to them via an authenticated
+// browser session is an SSRF foothold, not a developer convenience.
+const DEFAULT_BLOCKED_PORTS = new Set([
+  3306, 5432, 6379, 9092, 9200, 9300, 11211, 27017, 28017, 9222, 9223, 5984, 8086,
+]);
+
+const DEFAULT_PORT_ALLOWLIST = new Set([
+  3000, 3001, 3002, 3003, 3030, 3333,
+  4000, 4173, 4200,
+  5000, 5001, 5173, 5174, 5500, 5555,
+  6000, 6006,
+  7000, 7777,
+  8000, 8001, 8080, 8081, 8888,
+  9000, 9001, 9090, 9876,
+]);
+
+function parsePortListEnv(value) {
+  if (!value) return null;
+  const out = new Set();
+  for (const piece of value.split(',')) {
+    const n = Number(piece.trim());
+    if (Number.isInteger(n) && n >= 1 && n <= 65535) out.add(n);
+  }
+  return out.size > 0 ? out : null;
+}
+
+const ENV_ALLOWLIST = parsePortListEnv(process.env.DISPATCH_PREVIEW_PORTS);
+const ALLOW_ANY_HIGH_PORT = process.env.DISPATCH_PREVIEW_ALLOW_ANY_HIGH_PORT === 'true';
+
 function isValidPort(portStr) {
   if (!PORT_PATTERN.test(portStr)) return false;
   const port = Number(portStr);
-  return port >= 1 && port <= 65535;
+  if (port < 1 || port > 65535) return false;
+  if (DEFAULT_BLOCKED_PORTS.has(port)) return false;
+  if (ENV_ALLOWLIST) return ENV_ALLOWLIST.has(port);
+  if (DEFAULT_PORT_ALLOWLIST.has(port)) return true;
+  // Opt-in escape hatch for users running dev servers on uncommon ports.
+  if (ALLOW_ANY_HIGH_PORT && port >= 1024) return true;
+  return false;
 }
 
 function stripHopByHop(headers) {
@@ -49,7 +86,10 @@ router.use('/:port', (req, res) => {
   const { port } = req.params;
 
   if (!isValidPort(port)) {
-    res.status(400).json({ error: 'Invalid port' });
+    res.status(400).json({
+      error: 'Port not allowed for preview proxy',
+      hint: 'Set DISPATCH_PREVIEW_PORTS or DISPATCH_PREVIEW_ALLOW_ANY_HIGH_PORT=true to allow custom ports.',
+    });
     return;
   }
 

server/routes/tasks.js

@@ -6,7 +6,12 @@ import express from 'express';
 
 const router = express.Router();
 
-const CLAUDE_PROJECTS_DIR = path.join(os.homedir(), '.claude', 'projects');
+const CLAUDE_PROJECTS_DIR = path.resolve(os.homedir(), '.claude', 'projects');
+
+// Reject anything that looks like path traversal or a separator before we
+// try to open a file. Defense-in-depth: the resolved path is also prefix-
+// checked against CLAUDE_PROJECTS_DIR below.
+const UNSAFE_PATH_SEGMENT = /[\\/]|\0|\.\./;
 
 function slugFromProjectName(projectName) {
   // Claude stores JSONLs in `~/.claude/projects/<slug>/`. The slug is the
@@ -16,7 +21,13 @@ function slugFromProjectName(projectName) {
 
 async function readSessionFile(projectSlug, sessionId) {
   if (!projectSlug || !sessionId) return null;
-  const jsonlPath = path.join(CLAUDE_PROJECTS_DIR, projectSlug, `${sessionId}.jsonl`);
+  if (UNSAFE_PATH_SEGMENT.test(projectSlug) || UNSAFE_PATH_SEGMENT.test(sessionId)) {
+    return null;
+  }
+  const jsonlPath = path.resolve(CLAUDE_PROJECTS_DIR, projectSlug, `${sessionId}.jsonl`);
+  if (!jsonlPath.startsWith(CLAUDE_PROJECTS_DIR + path.sep)) {
+    return null;
+  }
   try {
     const raw = await fs.readFile(jsonlPath, 'utf8');
     return raw.split('\n').filter(Boolean);