From e7791c18fa085c2a992960f91d789671e4087a36 Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Tue, 7 Jul 2026 17:48:47 +0200 Subject: [PATCH 1/2] feat(cockpit): worker inspector drawer + 3a-followup fixes (issue #70) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds GET /api/worker// to cockpit-serve.sh: an event timeline (newest first) from events.jsonl, the latest --detail breadcrumbs surfaced separately, and live worktree forensics (branch, status --short, last 5 commits, diffstat vs main's merge-base) computed by shelling out to git — zero agent tokens. The worker's worktree is located by conventional directory name (.claude/worktrees/issue-) or, failing that, by scanning `git worktree list` for a branch matching feat/issue--*; no match degrades to a clear found:false marker rather than erroring. Wires a click-to-open inspector drawer into the served dashboard's live- progress table (static one-shot cockpit.sh output is unchanged), rendering the three sections above with every dynamic value via textContent only. Also lands the two PR #73-flagged 3a followups: - cockpit-serve.sh's bash EXIT trap for its mktemp'd HTML file never fired (exec node replaces the shell), leaking one temp file per invocation; cleanup now happens in node's own 'exit' handler instead. - /events' SSE tailer went stale forever once log-event.sh's rotation shrank events.jsonl below the subscriber's read offset; pump() now resets the offset to 0 whenever the file has shrunk. Extends the log-event.sh paragraphs in orchestrator/implementer/reviewer.md so every phase-transition call also passes a terse --detail breadcrumb, and extends cockpit.test.sh with an offline worker-inspector smoke case (synthetic temp git repo/worktree under $TMPDIR, mirroring .claude/self/smoke-fanout.sh's own git-init/worktree-add pattern) plus regression checks for both fixes. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NTZEsfFT4mrZpv8CY67Frd --- .claude/agents/implementer.md | 6 +- .claude/agents/orchestrator.md | 6 +- .claude/agents/reviewer.md | 6 +- .claude/scripts/cockpit-serve.sh | 350 ++++++++++++++++++++++++++++++- .claude/scripts/cockpit.test.sh | 135 ++++++++++++ 5 files changed, 492 insertions(+), 11 deletions(-) diff --git a/.claude/agents/implementer.md b/.claude/agents/implementer.md index 98c2877..d99f0bb 100644 --- a/.claude/agents/implementer.md +++ b/.claude/agents/implementer.md @@ -41,6 +41,6 @@ Never call bare `gh`. EVERY `gh` invocation (PR create/update, comments, `gh api If a reviewer rejects your work, address every reason, re-run the gates, and report again. Iterate until approved. ## Progress events (observability) -Best-effort, additive only — never changes gate enforcement or control flow. Log a progress event at each phase transition: -`bash ${CLAUDE_PLUGIN_ROOT:-.claude}/scripts/log-event.sh --role implementer --task --phase --model ` -Call it with `--phase implementing` when you start work (step 3), `--phase gate-running` before step 5's gate run, and `--phase done` when you file your report (step 7). If `log-event.sh` fails, ignore it and continue — it must never block or alter your work. +Best-effort, additive only — never changes gate enforcement or control flow. Log a progress event at each phase transition, ALSO passing a one-line `--detail ""` breadcrumb (one short terse sentence — it costs a few tokens, so keep it terse): +`bash ${CLAUDE_PLUGIN_ROOT:-.claude}/scripts/log-event.sh --role implementer --task --phase --model --detail ""` +Call it with `--phase implementing` when you start work (step 3), `--phase gate-running` before step 5's gate run, and `--phase done` when you file your report (step 7). If `log-event.sh` fails, ignore it and continue — it must never block or alter your work (the `--detail` breadcrumb is the same best-effort deal: never let it block you either). diff --git a/.claude/agents/orchestrator.md b/.claude/agents/orchestrator.md index 570757e..b1e9549 100644 --- a/.claude/agents/orchestrator.md +++ b/.claude/agents/orchestrator.md @@ -39,9 +39,9 @@ Every subagent you spawn starts a fresh context that loads CLAUDE.md and its age - Git hygiene: tell workers to **stage explicit paths, never `git add -A`/`git commit -a`**. A sandboxed session masks config paths (shell rc, `.gitconfig`, `.mcp.json`, `.claude/{hooks,skills,routines}`, editor dirs) as `/dev/null` device nodes that show up in `git status`; a blanket add can abort the commit. They're expected artifacts, not the worker's changes (see `docs/HARDENING.md` → Caveats). ## Progress events (observability) -Best-effort, additive only — never changes gate enforcement, review consensus, or control flow. After you present the plan (step 2), run: -`bash ${CLAUDE_PLUGIN_ROOT:-.claude}/scripts/log-event.sh --role orchestrator --task --phase scoped --model ` -When the run wraps (step 6), you may also log `--phase done`. If `log-event.sh` fails for any reason, ignore it and continue — never let it block or alter your loop. +Best-effort, additive only — never changes gate enforcement, review consensus, or control flow. After you present the plan (step 2), run, ALSO passing a one-line `--detail ""` breadcrumb (one short terse sentence — it costs a few tokens, so keep it terse): +`bash ${CLAUDE_PLUGIN_ROOT:-.claude}/scripts/log-event.sh --role orchestrator --task --phase scoped --model --detail ""` +When the run wraps (step 6), you may also log `--phase done`. If `log-event.sh` fails for any reason, ignore it and continue — never let it block or alter your loop (the `--detail` breadcrumb is the same best-effort deal: never let it block you either). ## Status report format (your "standup") ``` diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md index 743dcd9..5bb09b5 100644 --- a/.claude/agents/reviewer.md +++ b/.claude/agents/reviewer.md @@ -43,6 +43,6 @@ If you touch GitHub at all (e.g. `gh pr diff`, `gh pr view`, `gh api`), route it Reject if you find anything that would block merge under your lens. Be specific and actionable so the implementer can fix without guessing. ## Progress events (observability) -Best-effort, additive only — never changes review consensus or control flow. Log a progress event at the start of your review and when you emit your verdict: -`bash ${CLAUDE_PLUGIN_ROOT:-.claude}/scripts/log-event.sh --role reviewer --task --phase reviewing --model --lens ` -then again with `--phase done` once you've emitted your verdict. If `log-event.sh` fails, ignore it and continue — it must never block or alter your review. +Best-effort, additive only — never changes review consensus or control flow. Log a progress event at the start of your review and when you emit your verdict, ALSO passing a one-line `--detail ""` breadcrumb (one short terse sentence — it costs a few tokens, so keep it terse): +`bash ${CLAUDE_PLUGIN_ROOT:-.claude}/scripts/log-event.sh --role reviewer --task --phase reviewing --model --lens --detail ""` +then again with `--phase done` once you've emitted your verdict. If `log-event.sh` fails, ignore it and continue — it must never block or alter your review (the `--detail` breadcrumb is the same best-effort deal: never let it block you either). diff --git a/.claude/scripts/cockpit-serve.sh b/.claude/scripts/cockpit-serve.sh index 89b22af..a622c98 100755 --- a/.claude/scripts/cockpit-serve.sh +++ b/.claude/scripts/cockpit-serve.sh @@ -33,6 +33,24 @@ # and returns 200 {"ok":true} (or {"ok":false,"error"} # on failure). The injected client calls this from its # refresh timer and its manual "Refresh now" button. +# GET /api/worker// +# Worker inspector (issue #70), backing the drawer that +# opens when a live-progress row is clicked. Returns +# JSON: { role, task, timeline, breadcrumbs, worktree }. +# - timeline: every events.jsonl record matching +# (role,task), NEWEST FIRST. +# - breadcrumbs: the most recent non-empty --detail +# values (subset of timeline), surfaced separately +# so the drawer can show them prominently. +# - worktree: forensics computed LIVE by shelling out +# to git (zero agent tokens) against the worker's +# worktree — located by directory name +# `.claude/worktrees/issue-` or, failing that, +# a registered worktree whose branch matches +# `feat/issue--*`. { found, path, branch, +# status, commits, diffstat, mergeBase, error }; if +# no worktree matches, found:false with a plain +# "no worktree found" error string (never a crash). # # Foreground process — SIGTERM/SIGINT close the server cleanly (via `exec`, # below, node receives signals directly; no bash wrapper indirection). @@ -67,9 +85,24 @@ fi gh_refresh="${COCKPIT_GH_REFRESH:-60}" -tmp_out="$(mktemp "${TMPDIR:-/tmp}/cockpit-serve.XXXXXX.html")" -trap 'rm -f "$tmp_out"' EXIT +# Worker-inspector forensics root (issue #70): the directory that CONTAINS +# .claude/worktrees/ and the git repo itself, so /api/worker// +# can locate a worker's worktree by name or by branch. Defaults to $root +# (same consumer-project root everything else here uses); overridable so +# cockpit.test.sh can point it at a synthetic temp repo/worktree instead of +# the real one — no network/gh either way, just local git plumbing. +worktrees_root="${COCKPIT_SERVE_WORKTREES_ROOT:-$root}" +tmp_out="$(mktemp "${TMPDIR:-/tmp}/cockpit-serve.XXXXXX.html")" +# NOTE: deliberately NO bash `trap ... EXIT` here. `exec` below REPLACES this +# shell process image with node (same PID) — a bash-level EXIT trap +# registered before `exec` would sit there registered but never fire once +# node takes over, which used to leak one temp HTML file per invocation +# (3a-followup fix, issue #70). Node now owns TMP_OUT's entire lifecycle and +# removes it itself via its own 'exit' handler, below, which fires for every +# exit path (normal return, process.exit() from shutdown(), and uncaught +# exceptions alike). +# # `exec` replaces this shell with node (same PID) so SIGTERM/SIGINT go # straight to node's own handlers below — no bash signal-forwarding needed. COCKPIT_SERVE_SELF="$cockpit" \ @@ -78,9 +111,11 @@ COCKPIT_SERVE_PORT="$port" \ COCKPIT_SERVE_EVENTS_FILE="$events_file" \ COCKPIT_SERVE_GH_REFRESH="$gh_refresh" \ COCKPIT_SERVE_TMP_OUT="$tmp_out" \ +COCKPIT_SERVE_WORKTREES_ROOT="$worktrees_root" \ exec node - <<'NODE_SERVE' const http = require("http"); const fs = require("fs"); +const path = require("path"); const { execFileSync } = require("child_process"); const SELF = process.env.COCKPIT_SERVE_SELF; @@ -90,6 +125,15 @@ const EVENTS_FILE = process.env.COCKPIT_SERVE_EVENTS_FILE; const GH_REFRESH_SECONDS = parseInt(process.env.COCKPIT_SERVE_GH_REFRESH, 10) || 60; const GH_REFRESH_MS = GH_REFRESH_SECONDS * 1000; const TMP_OUT = process.env.COCKPIT_SERVE_TMP_OUT; +const WORKTREES_ROOT = process.env.COCKPIT_SERVE_WORKTREES_ROOT || process.cwd(); + +// Fix (issue #70, 3a-followup): TMP_OUT cleanup moved here from the now-dead +// bash EXIT trap (see the shell comment above `exec node`, above) — this +// fires on every node exit path, so the temp HTML file cockpit.sh renders +// into no longer leaks one file per invocation. +process.on("exit", () => { + try { fs.unlinkSync(TMP_OUT); } catch (e) { /* already gone, or never created */ } +}); // --------------------------------------------------------------------------- // Render cache: re-run cockpit.sh (the ONE renderer) at most once per @@ -231,6 +275,144 @@ function clientScript() { refreshBtn.addEventListener("click", doRefresh); setInterval(doRefresh, ${GH_REFRESH_MS}); + // Worker inspector drawer (issue #70): clicking a live-worker row fetches + // GET /api/worker// and shows its event timeline (newest + // first), latest breadcrumbs, and worktree forensics in a side panel. + // Every dynamic value lands via textContent only (never innerHTML), + // mirroring upsertRow()'s XSS-safety contract above. + var drawerStyle = document.createElement("style"); + drawerStyle.textContent = "#live tbody tr { cursor: pointer; } #live tbody tr:hover { outline: 1px solid currentColor; }"; + document.head.appendChild(drawerStyle); + + var drawer = null; + function ensureDrawer() { + if (drawer) return drawer; + drawer = document.createElement("div"); + drawer.id = "worker-drawer"; + drawer.setAttribute("style", "position:fixed;top:0;right:0;bottom:0;width:min(480px,90vw);overflow-y:auto;" + + "background:#161b22;color:#e6edf3;border-left:1px solid #30363d;padding:1rem;" + + "box-shadow:-2px 0 8px rgba(0,0,0,0.4);display:none;z-index:1000;"); + + var closeBtn = document.createElement("button"); + closeBtn.type = "button"; + closeBtn.textContent = "Close"; + closeBtn.addEventListener("click", function () { drawer.style.display = "none"; }); + + var title = document.createElement("h2"); + title.id = "drawer-title"; + + var breadcrumbsHeading = document.createElement("h3"); + breadcrumbsHeading.textContent = "Latest breadcrumbs"; + var breadcrumbsList = document.createElement("ul"); + breadcrumbsList.id = "drawer-breadcrumbs"; + + var forensicsHeading = document.createElement("h3"); + forensicsHeading.textContent = "Worktree forensics"; + var forensicsBody = document.createElement("pre"); + forensicsBody.id = "drawer-forensics"; + forensicsBody.style.whiteSpace = "pre-wrap"; + forensicsBody.style.fontSize = "0.8rem"; + + var timelineHeading = document.createElement("h3"); + timelineHeading.textContent = "Event timeline (newest first)"; + var timelineList = document.createElement("ul"); + timelineList.id = "drawer-timeline"; + + drawer.appendChild(closeBtn); + drawer.appendChild(title); + drawer.appendChild(breadcrumbsHeading); + drawer.appendChild(breadcrumbsList); + drawer.appendChild(forensicsHeading); + drawer.appendChild(forensicsBody); + drawer.appendChild(timelineHeading); + drawer.appendChild(timelineList); + document.body.appendChild(drawer); + return drawer; + } + + function renderDrawer(role, task, data) { + var d = ensureDrawer(); + d.querySelector("#drawer-title").textContent = "Worker: " + role + " / " + task; + + var breadcrumbsList = d.querySelector("#drawer-breadcrumbs"); + breadcrumbsList.textContent = ""; + var crumbs = (data && data.breadcrumbs) || []; + if (crumbs.length === 0) { + var noCrumb = document.createElement("li"); + noCrumb.textContent = "(no breadcrumbs yet)"; + breadcrumbsList.appendChild(noCrumb); + } else { + crumbs.forEach(function (c) { + var li = document.createElement("li"); + li.textContent = "[" + (c.phase || "") + "] " + (c.detail || "") + " (" + (c.ts || "") + ")"; + breadcrumbsList.appendChild(li); + }); + } + + var forensicsBody = d.querySelector("#drawer-forensics"); + var wt = (data && data.worktree) || { found: false }; + if (!wt.found) { + forensicsBody.textContent = "no worktree found" + (wt.error ? " (" + wt.error + ")" : ""); + } else { + var lines = []; + lines.push("path: " + (wt.path || "")); + lines.push("branch: " + (wt.branch || "")); + lines.push(""); + lines.push("status --short:"); + lines.push(wt.status && wt.status.length ? wt.status : "(clean)"); + lines.push("last 5 commits:"); + (wt.commits && wt.commits.length ? wt.commits : ["(none)"]).forEach(function (c) { lines.push(" " + c); }); + lines.push(""); + lines.push("diffstat vs main:"); + lines.push(wt.diffstat && wt.diffstat.length ? wt.diffstat : "(no diff)"); + if (wt.error) lines.push("\n(note: " + wt.error + ")"); + forensicsBody.textContent = lines.join("\n"); + } + + var timelineList = d.querySelector("#drawer-timeline"); + timelineList.textContent = ""; + var tl = (data && data.timeline) || []; + if (tl.length === 0) { + var noEv = document.createElement("li"); + noEv.textContent = "(no events)"; + timelineList.appendChild(noEv); + } else { + tl.forEach(function (ev) { + var li = document.createElement("li"); + var lensPart = ev.lens ? " (" + ev.lens + ")" : ""; + var detailPart = ev.detail ? ": " + ev.detail : ""; + li.textContent = (ev.ts || "") + " — " + (ev.phase || "") + lensPart + detailPart; + timelineList.appendChild(li); + }); + } + + d.style.display = "block"; + } + + // Server-rendered rows (cockpit.sh) carry role/task on a hidden trailing + // ; SSE-created rows (upsertRow(), above) carry + // them directly as data-role/data-task on the itself. Support both. + function rowRoleTask(tr) { + if (!tr) return null; + var role = tr.getAttribute("data-role"); + var task = tr.getAttribute("data-task"); + if (role != null && task != null) return { role: role, task: task }; + var meta = tr.querySelector("td.wrow-meta"); + if (meta) return { role: meta.getAttribute("data-role") || "", task: meta.getAttribute("data-task") || "" }; + return null; + } + + document.addEventListener("click", function (ev) { + var tr = ev.target && ev.target.closest ? ev.target.closest("#live tbody tr") : null; + if (!tr) return; + var rt = rowRoleTask(tr); + if (!rt) return; + fetch("/api/worker/" + encodeURIComponent(rt.role) + "/" + encodeURIComponent(rt.task)) + .then(function (r) { return r.json(); }) + .then(function (data) { renderDrawer(rt.role, rt.task, data); }) + .catch(function () { /* inert on fetch failure -- drawer just doesn't open */ }); + }); + connect(); } catch (e) { /* inert on any DOM/EventSource-less environment */ } })(); @@ -266,6 +448,15 @@ function handleEvents(req, res) { fs.stat(EVENTS_FILE, (err, stat) => { if (closed) return; if (err) return; // file missing yet -- nothing to send + // Fix (issue #70, 3a-followup): log-event.sh rotates events.jsonl (caps + // it to the last EVENTS_MAX_LINES lines via a temp-file + atomic mv — + // see log-event.sh), which can shrink the file below our current read + // offset. Left unhandled, every future stat.size <= offset check below + // would stay permanently true and this subscriber would silently stop + // tailing forever. If the file is now SMALLER than where we'd read to, + // it rotated (or was truncated/replaced) -- reset to 0 and resume + // tailing from the top of the new file. + if (stat.size < offset) offset = 0; if (stat.size <= offset) return; // no new bytes fs.open(EVENTS_FILE, "r", (openErr, fd) => { if (closed) return; @@ -315,6 +506,157 @@ function handleEvents(req, res) { }); } +// --------------------------------------------------------------------------- +// /api/worker// — worker inspector (issue #70). Zero agent +// tokens: everything here is either read from the local events.jsonl or +// computed live by shelling out to git. See the route-table comment near the +// top of this file for the exact JSON shape. +// --------------------------------------------------------------------------- +function readEventsAll() { + let text = ""; + try { text = fs.readFileSync(EVENTS_FILE, "utf8"); } catch (e) { return []; } + const out = []; + for (const line of text.split("\n")) { + const trimmed = line.trim(); + if (!trimmed) continue; + try { + const obj = JSON.parse(trimmed); + // typeof [] === "object" too -- exclude arrays, mirroring cockpit.sh's + // own readEvents() malformed/array-line tolerance. + if (obj && typeof obj === "object" && !Array.isArray(obj)) out.push(obj); + } catch (e) { /* skip malformed line */ } + } + return out; +} + +function escapeRegExp(s) { + return String(s).replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +// Locate the worker's worktree: prefer the conventional directory name, else +// fall back to scanning `git worktree list` for a branch matching +// feat/issue--*. Returns an absolute path, or null if neither matches +// (degrade path -- the caller renders a "no worktree found" marker instead +// of erroring). +function findWorktree(taskId) { + const byName = path.join(WORKTREES_ROOT, ".claude", "worktrees", `issue-${taskId}`); + try { + if (fs.statSync(byName).isDirectory()) return byName; + } catch (e) { /* not found by name -- fall through to the branch scan */ } + + try { + const out = execFileSync("git", ["-C", WORKTREES_ROOT, "worktree", "list", "--porcelain"], { + encoding: "utf8", + }); + const branchRe = new RegExp("^refs/heads/feat/issue-" + escapeRegExp(taskId) + "-.*$"); + let candidatePath = null; + let candidateBranch = null; + for (const rawLine of out.split("\n")) { + const line = rawLine.trim(); + if (line.startsWith("worktree ")) { + candidatePath = line.slice("worktree ".length); + candidateBranch = null; + } else if (line.startsWith("branch ")) { + candidateBranch = line.slice("branch ".length); + if (candidatePath && candidateBranch && branchRe.test(candidateBranch)) return candidatePath; + } else if (line === "") { + candidatePath = null; + candidateBranch = null; + } + } + } catch (e) { /* WORKTREES_ROOT isn't a git repo, or git is unavailable */ } + + return null; +} + +// Worktree forensics (issue #70): current branch, short status, last 5 +// commits, and a diffstat vs main's merge-base. Every git call is wrapped +// individually so one missing ref (e.g. no local "main") degrades that ONE +// field rather than failing the whole endpoint. +function gatherForensics(wtPath) { + const result = { + found: true, + path: wtPath, + branch: null, + status: "", + commits: [], + diffstat: "", + mergeBase: null, + error: null, + }; + const errors = []; + + try { + result.branch = execFileSync("git", ["-C", wtPath, "rev-parse", "--abbrev-ref", "HEAD"], { + encoding: "utf8", + }).trim(); + } catch (e) { errors.push("branch: " + String((e && e.message) || e)); } + + try { + result.status = execFileSync("git", ["-C", wtPath, "status", "--short"], { encoding: "utf8" }); + } catch (e) { errors.push("status: " + String((e && e.message) || e)); } + + try { + const log = execFileSync("git", ["-C", wtPath, "log", "--oneline", "-5"], { encoding: "utf8" }); + result.commits = log.split("\n").filter((l) => l.length > 0); + } catch (e) { errors.push("log: " + String((e && e.message) || e)); } + + let mergeBaseOk = false; + for (const base of ["main", "origin/main"]) { + try { + const mb = execFileSync("git", ["-C", wtPath, "merge-base", base, "HEAD"], { encoding: "utf8" }).trim(); + result.mergeBase = mb; + result.diffstat = execFileSync("git", ["-C", wtPath, "diff", "--stat", `${mb}...HEAD`], { + encoding: "utf8", + maxBuffer: 10 * 1024 * 1024, + }); + mergeBaseOk = true; + break; + } catch (e) { /* try the next base candidate */ } + } + if (!mergeBaseOk) errors.push("diffstat: no merge-base found against main/origin main"); + + if (errors.length) result.error = errors.join("; "); + return result; +} + +function handleWorkerInspector(req, res, role, task) { + try { + const allEvents = readEventsAll(); + const matching = allEvents.filter((ev) => { + const evRole = ev.role != null ? String(ev.role) : ""; + const evTask = ev.task != null ? String(ev.task) : ""; + return evRole === role && evTask === task; + }); + const timeline = matching.slice().reverse(); // NEWEST FIRST (file/append order reversed) + const breadcrumbs = timeline + .filter((ev) => ev.detail != null && String(ev.detail).trim() !== "") + .slice(0, 10) + .map((ev) => ({ ts: ev.ts || "", phase: ev.phase || "", detail: String(ev.detail) })); + + const wtPath = findWorktree(task); + const worktree = wtPath + ? gatherForensics(wtPath) + : { + found: false, + path: null, + branch: null, + status: "", + commits: [], + diffstat: "", + mergeBase: null, + error: "no worktree found for task " + task, + }; + + const body = JSON.stringify({ role, task, timeline, breadcrumbs, worktree }); + res.writeHead(200, { "Content-Type": "application/json; charset=utf-8" }); + res.end(body); + } catch (e) { + res.writeHead(500, { "Content-Type": "application/json; charset=utf-8" }); + res.end(JSON.stringify({ error: String((e && e.message) || e) })); + } +} + function handleRefresh(req, res) { try { getHtml(true); @@ -347,6 +689,10 @@ const server = http.createServer((req, res) => { if (url === "/" || url === "/index.html") return handleIndex(req, res); if (url === "/events") return handleEvents(req, res); if (url === "/api/refresh") return handleRefresh(req, res); + const workerMatch = url.match(/^\/api\/worker\/([^/]+)\/([^/]+)$/); + if (workerMatch) { + return handleWorkerInspector(req, res, decodeURIComponent(workerMatch[1]), decodeURIComponent(workerMatch[2])); + } res.writeHead(404, { "Content-Type": "text/plain" }); res.end("not found"); }); diff --git a/.claude/scripts/cockpit.test.sh b/.claude/scripts/cockpit.test.sh index 3839beb..21e1793 100755 --- a/.claude/scripts/cockpit.test.sh +++ b/.claude/scripts/cockpit.test.sh @@ -250,6 +250,13 @@ port="$(node -e ' s.listen(0, "127.0.0.1", () => { console.log(s.address().port); s.close(); }); ')" +# 3a-followup fix (a) regression baseline (issue #70): count any leftover +# cockpit-serve temp HTML files BEFORE this server ever runs, so the +# after-shutdown check below only flags a NEW leak, not pre-existing debris. +tmp_glob="${TMPDIR:-/tmp}"/cockpit-serve.*.html +tmp_before=0 +for f in $tmp_glob; do [ -e "$f" ] && tmp_before=$((tmp_before + 1)); done + serve_log="$work/serve.log" : > "$serve_log" bash "$cockpit_serve" "$port" --fixtures "$work/fixtures" >"$serve_log" 2>&1 & @@ -289,9 +296,137 @@ kill "$sse_pid" >/dev/null 2>&1 || true wait "$sse_pid" 2>/dev/null || true check "SSE /events delivers the newly appended events.jsonl line within the timeout" [ "$sse_seen" -eq 1 ] +# 3a-followup fix (b) regression (issue #70): log-event.sh's rotation caps +# events.jsonl to its last N lines via a temp-file + atomic mv, which can +# shrink the file below a subscriber's current read offset. Start a fresh +# subscriber, let it read the CURRENT (larger) file at least once so its +# offset advances past 0, then simulate rotation by replacing the file with a +# much SMALLER one containing a brand-new marker line -- a subscriber whose +# offset never resets would sit past EOF forever and never see it. +rot_out="$work/sse-rotation.out" +: > "$rot_out" +timeout 6 curl -sN "http://127.0.0.1:$port/events" >"$rot_out" 2>/dev/null & +rot_pid=$! +sleep 1.5 +printf '{"ts":"2026-01-01T00:20:00Z","role":"implementer","model":"sonnet","task":"rot70","phase":"implementing","lens":"","detail":"post-rotation line"}\n' >"$work/fixtures/events.jsonl" +deadline=$((SECONDS + 5)) +rot_seen=0 +while [ "$SECONDS" -lt "$deadline" ]; do + grep -q '"task":"rot70"' "$rot_out" 2>/dev/null && { rot_seen=1; break; } + sleep 0.2 +done +kill "$rot_pid" >/dev/null 2>&1 || true +wait "$rot_pid" 2>/dev/null || true +check "SSE /events resumes tailing after events.jsonl shrinks/rotates (3a-followup fix)" [ "$rot_seen" -eq 1 ] + +kill "$server_pid" >/dev/null 2>&1 || true +wait "$server_pid" 2>/dev/null || true +server_pid="" + +# 3a-followup fix (a) regression (issue #70): after the server above exits, +# its mktemp'd HTML temp file must be gone -- proves cleanup now happens in +# node's own 'exit' handler rather than the dead bash EXIT trap that used to +# sit after `exec node` (never fired, since exec replaces the shell). +tmp_after=0 +for f in $tmp_glob; do [ -e "$f" ] && tmp_after=$((tmp_after + 1)); done +check "cockpit-serve.sh does not leak its temp HTML file after exit (3a-followup fix)" [ "$tmp_after" -eq "$tmp_before" ] + +# --------------------------------------------------------------------------- +# 6. Worker inspector endpoint (GET /api/worker//, issue #70): +# event timeline + latest breadcrumbs + live worktree forensics, entirely +# offline. Forensics are computed by cockpit-serve.sh shelling out to git +# against a SYNTHETIC temp git repo/worktree built here under $TMPDIR +# (mirrors .claude/self/smoke-fanout.sh's own git-init/worktree-add +# pattern) -- never against this checkout, so this stays deterministic and +# isolated. Covers BOTH worktree-lookup strategies documented in +# cockpit-serve.sh: by conventional directory name +# (.claude/worktrees/issue-) and by branch-name fallback +# (feat/issue--*, worktree living anywhere else on disk), plus the +# graceful "no worktree found" degrade path. +# --------------------------------------------------------------------------- +insp_root="$work/inspector-repo" +IG() { git -C "$insp_root" -c user.name=insp -c user.email=insp@local -c commit.gpgsign=false "$@"; } +mkdir -p "$insp_root" +IG init -q -b main . +IG commit -q --allow-empty -m "inspector fixture: initial" + +# Worker "implementer"/"70a": worktree found by CONVENTIONAL DIRECTORY NAME. +mkdir -p "$insp_root/.claude/worktrees" +IG worktree add -q -b feat/issue-70a-inspector "$insp_root/.claude/worktrees/issue-70a" main +git -C "$insp_root/.claude/worktrees/issue-70a" -c user.name=insp -c user.email=insp@local -c commit.gpgsign=false \ + commit -q --allow-empty -m "feat: inspector work for 70a" + +# Worker "reviewer"/"70b": worktree lives OUTSIDE .claude/worktrees/ entirely +# -- only discoverable via the BRANCH-NAME FALLBACK (feat/issue-70b-*). +insp_wt_70b="$work/inspector-wt-elsewhere" +IG worktree add -q -b feat/issue-70b-other-branch "$insp_wt_70b" main +git -C "$insp_wt_70b" -c user.name=insp -c user.email=insp@local -c commit.gpgsign=false \ + commit -q --allow-empty -m "feat: other work for 70b" + +mkdir -p "$work/fixtures-inspector" +echo "[]" >"$work/fixtures-inspector/issues.json" +echo "[]" >"$work/fixtures-inspector/prs.json" +cat >"$work/fixtures-inspector/events.jsonl" <<'EOF' +{"ts":"2026-01-01T01:00:00Z","role":"implementer","model":"sonnet","task":"70a","phase":"implementing","lens":"","detail":"scoped the inspector work"} +{"ts":"2026-01-01T01:05:00Z","role":"implementer","model":"sonnet","task":"70a","phase":"gate-running","lens":"","detail":"running gates"} +{"ts":"2026-01-01T01:02:00Z","role":"reviewer","model":"opus","task":"70b","phase":"reviewing","lens":"tests","detail":""} +EOF + +insp_port="$(node -e ' + const s = require("net").createServer(); + s.listen(0, "127.0.0.1", () => { console.log(s.address().port); s.close(); }); +')" +insp_serve_log="$work/serve-inspector.log" +: > "$insp_serve_log" +COCKPIT_SERVE_WORKTREES_ROOT="$insp_root" bash "$cockpit_serve" "$insp_port" --fixtures "$work/fixtures-inspector" >"$insp_serve_log" 2>&1 & +server_pid=$! + +insp_ready=0 +for _ in $(seq 1 50); do + grep -q "cockpit serving" "$insp_serve_log" 2>/dev/null && { insp_ready=1; break; } + kill -0 "$server_pid" 2>/dev/null || break + sleep 0.2 +done +check "worker-inspector server (custom WORKTREES_ROOT) starts within the readiness timeout" [ "$insp_ready" -eq 1 ] + +resp_70a="$(curl -s "http://127.0.0.1:$insp_port/api/worker/implementer/70a" 2>/dev/null)" +check "worker-inspector 70a: timeline has both events, NEWEST FIRST, plus breadcrumbs + forensics fields" node -e ' + const got = JSON.parse(process.argv[1]); + if (!Array.isArray(got.timeline) || got.timeline.length !== 2) throw new Error("expected 2 timeline entries, got " + JSON.stringify(got.timeline)); + if (got.timeline[0].phase !== "gate-running") throw new Error("expected newest-first (gate-running first), got " + got.timeline[0].phase); + if (got.timeline[1].phase !== "implementing") throw new Error("expected implementing second, got " + got.timeline[1].phase); + if (!Array.isArray(got.breadcrumbs) || got.breadcrumbs.length !== 2) throw new Error("expected 2 breadcrumbs, got " + JSON.stringify(got.breadcrumbs)); + if (got.breadcrumbs[0].detail !== "running gates") throw new Error("expected latest breadcrumb first, got " + JSON.stringify(got.breadcrumbs[0])); + const wt = got.worktree; + if (!wt || wt.found !== true) throw new Error("expected worktree found via directory-name lookup, got " + JSON.stringify(wt)); + if (wt.branch !== "feat/issue-70a-inspector") throw new Error("unexpected branch " + wt.branch); + if (typeof wt.status !== "string") throw new Error("status field missing/wrong type"); + if (!Array.isArray(wt.commits) || wt.commits.length === 0) throw new Error("commits field missing/empty"); + if (typeof wt.diffstat !== "string") throw new Error("diffstat field missing/wrong type"); +' "$resp_70a" + +resp_70b="$(curl -s "http://127.0.0.1:$insp_port/api/worker/reviewer/70b" 2>/dev/null)" +check "worker-inspector 70b: worktree found via BRANCH-NAME fallback (not conventional dir name)" node -e ' + const got = JSON.parse(process.argv[1]); + const wt = got.worktree; + if (!wt || wt.found !== true) throw new Error("expected worktree found via branch fallback, got " + JSON.stringify(wt)); + if (wt.branch !== "feat/issue-70b-other-branch") throw new Error("unexpected branch " + wt.branch); + if (!Array.isArray(wt.commits) || wt.commits.length === 0) throw new Error("commits field missing/empty"); + if (typeof wt.diffstat !== "string") throw new Error("diffstat field missing/wrong type"); +' "$resp_70b" + +resp_missing="$(curl -s "http://127.0.0.1:$insp_port/api/worker/nobody/999999" 2>/dev/null)" +check "worker-inspector degrades gracefully: no matching worktree returns found:false, not an error" node -e ' + const got = JSON.parse(process.argv[1]); + if (got.worktree.found !== false) throw new Error("expected found:false, got " + JSON.stringify(got.worktree)); + if (Array.isArray(got.timeline) && got.timeline.length !== 0) throw new Error("expected empty timeline for an unknown worker"); +' "$resp_missing" + kill "$server_pid" >/dev/null 2>&1 || true wait "$server_pid" 2>/dev/null || true server_pid="" +IG worktree remove --force "$insp_root/.claude/worktrees/issue-70a" >/dev/null 2>&1 || true +IG worktree remove --force "$insp_wt_70b" >/dev/null 2>&1 || true echo "" if [ "$fail" -eq 0 ]; then From 0c5ed4cc317091d339077a650cc6c755dba9c13b Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Tue, 7 Jul 2026 17:57:33 +0200 Subject: [PATCH 2/2] fix(cockpit): stop worker-inspector URIError crash, reject traversal in role/task Correctness review (BLOCKING): a malformed percent-escape in /api/worker// reached decodeURIComponent() outside handleWorkerInspector's try/catch, throwing an uncaught URIError that killed the whole node process for every client (no process.on("uncaughtException") handler exists). The route now passes the raw matched segments through, and handleWorkerInspector decodes them itself inside a try/catch, responding 400 JSON on failure instead of crashing. Hardening (both reviewers flagged): role/task are interpolated into path.join(WORKTREES_ROOT, ...) and a RegExp branch scan. Reject any decoded value containing a path separator, "..", or NUL up front with 400 JSON, closing the traversal note (git calls already use execFileSync with arg arrays, so this is defense-in-depth). Adds an offline regression test hitting the endpoint with a malformed percent-escape and a traversal-shaped task, asserting both return 400 and that the server is still serving valid requests afterward -- proving the crash is fixed rather than just changing a status code. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NTZEsfFT4mrZpv8CY67Frd --- .claude/scripts/cockpit-serve.sh | 27 +++++++++++++++++++++++++-- .claude/scripts/cockpit.test.sh | 24 ++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/.claude/scripts/cockpit-serve.sh b/.claude/scripts/cockpit-serve.sh index a622c98..014155e 100755 --- a/.claude/scripts/cockpit-serve.sh +++ b/.claude/scripts/cockpit-serve.sh @@ -620,7 +620,30 @@ function gatherForensics(wtPath) { return result; } -function handleWorkerInspector(req, res, role, task) { +// Rejects role/task values that could escape their intended slot: a path +// separator or ".." would let `task` reach outside WORKTREES_ROOT/.claude/worktrees/ +// in findWorktree()'s path.join(), and a NUL would truncate a C-string arg. +// All git calls already use execFileSync with arg arrays (no shell), so this +// is defense-in-depth, not the only guard -- but it closes the traversal note. +function isUnsafeIdentifier(s) { + return /[/\\]|\.\.|\x00/.test(s); +} + +function handleWorkerInspector(req, res, rawRole, rawTask) { + let role, task; + try { + role = decodeURIComponent(rawRole); + task = decodeURIComponent(rawTask); + } catch (e) { + res.writeHead(400, { "Content-Type": "application/json; charset=utf-8" }); + res.end(JSON.stringify({ error: "malformed URI component in worker path" })); + return; + } + if (isUnsafeIdentifier(role) || isUnsafeIdentifier(task)) { + res.writeHead(400, { "Content-Type": "application/json; charset=utf-8" }); + res.end(JSON.stringify({ error: "role/task must not contain path separators, \"..\", or NUL" })); + return; + } try { const allEvents = readEventsAll(); const matching = allEvents.filter((ev) => { @@ -691,7 +714,7 @@ const server = http.createServer((req, res) => { if (url === "/api/refresh") return handleRefresh(req, res); const workerMatch = url.match(/^\/api\/worker\/([^/]+)\/([^/]+)$/); if (workerMatch) { - return handleWorkerInspector(req, res, decodeURIComponent(workerMatch[1]), decodeURIComponent(workerMatch[2])); + return handleWorkerInspector(req, res, workerMatch[1], workerMatch[2]); } res.writeHead(404, { "Content-Type": "text/plain" }); res.end("not found"); diff --git a/.claude/scripts/cockpit.test.sh b/.claude/scripts/cockpit.test.sh index 21e1793..7df7705 100755 --- a/.claude/scripts/cockpit.test.sh +++ b/.claude/scripts/cockpit.test.sh @@ -422,6 +422,30 @@ check "worker-inspector degrades gracefully: no matching worktree returns found: if (Array.isArray(got.timeline) && got.timeline.length !== 0) throw new Error("expected empty timeline for an unknown worker"); ' "$resp_missing" +# Crash-fix regression (issue #70 correctness review, BLOCKING): a malformed +# percent-escape used to reach decodeURIComponent() OUTSIDE +# handleWorkerInspector's try/catch and throw an uncaught URIError, killing +# the whole node process (no process.on("uncaughtException") handler +# existed, or was meant to). Assert the endpoint now responds 400 instead of +# dropping the connection, AND that the server is still alive/serving +# afterward -- that second assertion is what actually proves the crash is +# fixed, since a dead server would also fail every check after it. +resp_malformed="$(curl -s -o - -w '%{http_code}' "http://127.0.0.1:$insp_port/api/worker/%/1" 2>/dev/null)" +malformed_code="${resp_malformed: -3}" +check "worker-inspector: malformed percent-escape (%2F.../1) returns 400, not a dropped connection" [ "$malformed_code" = "400" ] + +# Path-traversal / separator-injection hardening (both reviewers flagged): +# `task` is interpolated into path.join(...) and a RegExp scan, so a decoded +# value containing "/" or ".." must be rejected up front rather than reaching +# findWorktree(). +resp_traversal="$(curl -s -o - -w '%{http_code}' "http://127.0.0.1:$insp_port/api/worker/implementer/..%2f.." 2>/dev/null)" +traversal_code="${resp_traversal: -3}" +check "worker-inspector: task containing '..' + encoded separator (traversal attempt) returns 400" [ "$traversal_code" = "400" ] + +resp_after_attack="$(curl -s -o - -w '%{http_code}' "http://127.0.0.1:$insp_port/api/worker/implementer/70a" 2>/dev/null)" +after_attack_code="${resp_after_attack: -3}" +check "worker-inspector: server still serves a valid request after malformed/traversal attempts (proves no crash)" [ "$after_attack_code" = "200" ] + kill "$server_pid" >/dev/null 2>&1 || true wait "$server_pid" 2>/dev/null || true server_pid=""