Feat/terminal pane configs loops

[pi0neerpat]

No description provided.

[pi0neerpat]

@claude review

[pi0neerpat]

@cursor review

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @pi0neerpat's task in 2m 18s —— View job

---

PR Review: Feat/terminal pane configs loops

• Gather context and understand the request
• Read key changed files
• Review server.js changes (API key auth + catalog endpoint)
• Review DispatchView.jsx changes
• Review LoopsView.jsx and LoopDetailView.jsx changes
• Review LoopReviewPanel.jsx (new file)
• Review parsers.js changes
• Review supporting changes (vite.config.js, apiFetchSetup.js, AgentModelPicker.jsx, etc.)
• Compile and post final review

---

Summary

This is a solid feature PR adding Cursor agent support, API key authentication for LAN deployment, a new loop artifacts viewer, and a catalog endpoint. The refactoring (moving AgentModelPicker to its own module, moving the loop launch form into DispatchView) improves reuse. A few issues worth addressing before merge:

---

Security

Path traversal in artifacts endpoint — The timestamp param in /api/loops/:loopType/:timestamp/artifacts is used directly in path.join without any sanitization (server.js):

const runDir = path.join(repo.resolvedPath, '.dispatch', 'loops', loopType, timestamp)

A crafted timestamp like ../../../etc/passwd escapes the loop directory. The fix is to validate timestamp against a safe pattern (e.g., VALID_TIMESTAMP_RE = /^[\w-]+$/) before using it, similar to how loopType is validated against VALID_LOOP_TYPES.

Fix this →

Timing-safe API key comparison — The middleware does key !== DISPATCH_API_KEY (string equality). For a localhost-only tool this is acceptable, but if DISPATCH_BIND=0.0.0.0 is used (the new LAN mode), timing attacks become theoretically possible. Consider using crypto.timingSafeEqual for the comparison when the key is set.

apiFetchSetup.js monkey-patches window.fetch globally — This will inject X-API-Key on all fetch calls from the SPA, including any future third-party calls. It works fine for the current use case but is worth documenting as a constraint.

---

Correctness

Breaking change: Loop ID format — The loop ID changed from ${loopType}/${dirName} to ${repo.name}/${loopType}/${dirName} (server.js line ~910). Any stored IDs (localStorage, bookmarks, external references) will silently fail to resolve. The LoopDetailView does handle suffix matching (all.find(j => j.id.endsWith(\/${loopId}`))`) as a fallback, but this won't help for IDs stored in the old format.

LoopDetailView auto-resets user tab selection — The useEffect at the top of the found-loop render path:

useEffect(() => {
  if (isSessionLookup && !loop) { setView('terminal') }
  else if (loop) { setView('review') }
}, [isSessionLookup, loop])

This resets the view to 'review' every time loop becomes non-null — so if a user manually switches to the Terminal tab while the loop is running, and then the polling updates loops, the view snaps back to 'review'. Consider only setting the initial default (e.g., track a hasSetInitialView ref).

Fragile prompt path in parseLoopRunDetailed — path.join(runDir, '..', 'prompt.md') relies on runDir being one level deep inside the loop type directory. If the loop directory structure changes, this silently returns an empty prompt. A comment or assertion about the expected structure would help.

Generic agent binary in resume command regex — The new regex /((?:claude(?:\s+code)?|cursor-agent|agent)\s+(?:resume|--resume)\s+...)/i matches any process named agent. This could produce false positives if the user runs other tools with that name in their terminal (e.g. various container/orchestration CLIs). If cursor always uses cursor-agent, narrowing the pattern would be safer.

---

Performance

Sequential model lookups in /api/catalog — The catalog endpoint calls getModelsForAgent three times in sequence:

for (const agent of DISPATCH_AGENT_KINDS) {
  const result = await getModelsForAgent(agent)
  ...
}

getModelsForAgent('claude') makes an external HTTP call to api.anthropic.com. Running all three in parallel with Promise.all would cut the response time significantly.

Fix this →

detectCursorBinary() called on every command build — buildTrackedCursorCommand calls detectCursorBinary() on every invocation, which runs execFileSync. Memoizing the result at startup (or lazily caching it) would avoid repeated process spawns.

---

Minor / Nitpicks

• buildResumeCursorCommand and buildCursorResumeCommand are confusingly named — one is an internal helper, the other is the public entry point. Consider renaming for clarity.
• AgentModelPicker's useEffect dependency array includes onChange — callers that don't wrap their handler in useCallback will cause the effect to fire on every render. This is currently fine because callers in DispatchView do use useCallback, but it's a subtle contract to maintain.
• LoopReviewPanel duplicates LOOP_TYPE_META that also exists in LoopDetailView. A shared constant (e.g., in AgentModelPicker.jsx alongside LOOP_TYPES) would avoid drift.
• In mdComponents.jsx, the inline prop approach is cleaner than the previous node?.properties?.className check — good fix.

---

Positives

• The AgentModelPicker extraction is clean and the reuse across DispatchView and (formerly) LoopsView works well.
• Rate-limit detection via lastErrorSubKind === 'rate_limit' and the resulting job state transition + error banner in ResultsPanel is a great UX improvement.
• The loop ID disambiguation logic (409 with repos list when ambiguous, 404 when not found) in the artifacts endpoint is well thought out.
• formatArtifactContent in LoopReviewPanel doing diff detection and transcript metadata stripping is impressive parsing work.
• Vite proxy injecting the API key header in dev mode is the right approach — keeps secrets out of the built bundle.

[pi0neerpat]

@claude review the fixes and new features

[pi0neerpat]

@cursor bug-bot review

[claude]

Claude finished @pi0neerpat's task in 3m 24s —— View job

---

PR Review: Fixes and New Features

• Gather context and review git diff
• Review security fixes (path traversal, timing-safe comparison)
• Review correctness fixes (loop ID, tab selection, prompt path)
• Review performance fixes (catalog endpoint, cursor binary detection)
• Review new features (job linking, read state, orphan stop, Pi agent)
• Compile final review

---

What Was Fixed

All the issues flagged in the previous review have been addressed:

Issue	Status
Path traversal in artifacts endpoint	✅ Fixed — VALID_LOOP_TIMESTAMP_RE = /^[A-Za-z0-9._:-]{1,120}$/ added at line 265 and validated before path.join
Timing-safe API key comparison	✅ Fixed — crypto.timingSafeEqual with SHA256 hashing (server.js:671-673)
Tab selection auto-reset in LoopDetailView	✅ Fixed — hasSetInitialViewRef prevents the effect from re-firing (LoopDetailView.jsx:34,56)
Fragile prompt path in parseLoopRunDetailed	✅ Fixed — tries 3 fallback paths (server.js:429-431)
Sequential model lookups in /api/catalog	✅ Fixed — Promise.all now used (server.js:1571)
detectCursorBinary() called on every invocation	✅ Fixed — function-property cache (server.js:288)
Generic agent in resume command regex	✅ Fixed — narrowed to cursor-agent only (server.js:3064)

---

New Features

Job Linking (Previous/Next navigation)

JobDetailView.jsx now renders a breadcrumb-style nav bar between related jobs using PreviousJob/NextJob header fields. The server validates both IDs and prevents double-linking a job's NextJob (409 conflict). This is clean.

Minor: buildResumeCursorCommand (line 280) still hardcodes cursor-agent directly, bypassing detectCursorBinary(). Only buildCursorResumeCommand goes through the cache. If the fallback binary is agent, resume commands written via this path will be wrong.

Fix this →

Read/Unread State for Jobs

Nice feature. The two-layer approach (localStorage overrides + server persistence via Read: header) is practical. The server endpoint (/api/jobs/:id/read) is well-validated.

One concern: readOverrides in JobsView are merged with item.read from the server via readOverrides[jobId] ?? item.read. But once a local override exists, it permanently shadows the server value until the component unmounts — even if the server and client agree. The saveReadOverrides useEffect (line 184) persists the entire override map without pruning entries that now match the server state. Over time localStorage fills with stale entries. Consider pruning: after a successful /read response, remove the entry from readOverrides and let item.read (from the next poll) take over.

Stop Button for Orphaned Jobs

The two-click confirmation with a 3-second auto-cancel is a solid pattern. The condition worker.filterStatus === 'active' && !worker.alive correctly targets jobs the server thinks are running but have no live PTY session.

Nitpick: killed jobs now map to 'review' status (line 55) — this is a behavior change from the previous 'failed' classification. Intentional? It means a job killed by the user appears in the review queue, which might confuse users expecting to validate it.

Pi Agent Icon

The icon is inlined as an SVG path in AgentIcon.jsx (clean). However dashboard/src/pi-logo.svg was added but is never imported anywhere — it appears to be an unused asset file.

Fix this →

AGENTS.md

Good addition — clearly explains the config.local.json override behavior, which is non-obvious from config.json alone.

---

Remaining Issues

Still Not Fixed

LOOP_TYPE_META duplicated — Still defined identically in both LoopDetailView.jsx:9 and LoopReviewPanel.jsx:11. As flagged in the original review, these will drift. Extract to a shared constant (e.g., dashboard/src/lib/constants.js or a new loopConstants.js).

Fix this →

New Issue

upsertHeaderLine read-then-write is not atomic — Two concurrent requests modifying the same job file (e.g., a /read and a /kill arriving at the same time) will race: both read the same file, write different lines, and one overwrites the other's change. This is a pre-existing structural issue, but the new /read and job-linking endpoints make concurrent writes more likely. Worth noting for future work, even if not blocking this PR.

---

Summary

All seven issues from the original review were addressed. The new features (job linking, read state, orphan stop, Pi icon) are well-implemented. The main remaining items are: LOOP_TYPE_META duplication (minor but unresolved from before), the unused pi-logo.svg, the buildResumeCursorCommand cursor-agent hardcoding, and the read override pruning question. None of these are blockers, but the first two are easy wins.
| Branch

[cursor]

Findings (bug-bot review)

1. Medium – API key header leaked to cross-origin fetches
   • File: dashboard/src/lib/apiFetchSetup.js
   • Issue: The global window.fetch patch always injected X-API-Key, even for absolute cross-origin URLs. If the SPA ever fetches external resources, this can leak the dashboard API key.
   • Action taken: Fixed. Header is now added only for same-origin requests.

What I verified from the prior review context

• timestamp validation on /api/loops/:loopType/:timestamp/artifacts is present (VALID_LOOP_TIMESTAMP_RE).
• API key comparisons are timing-safe (crypto.timingSafeEqual with SHA-256 digests).
• Catalog model loading now runs in parallel (Promise.all).
• Resume command capture regex no longer includes generic agent fallback (now specific to claude and cursor-agent in capture path).
• Loop detail tab auto-reset issue appears fixed via hasSetInitialViewRef.

Validation run

• node --test parsers.test.js parsers.integration.test.js cli.integration.test.js ✅
• node --test server.integration.test.mjs eventPipeline.test.mjs ✅

Git

• Commit: fea6022
• Branch pushed: feat/terminal-pane-configs-loops