diff --git a/AGENTS.md b/AGENTS.md index d9d7226..756a45a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -31,6 +31,12 @@ src/ log reader. Not pure — resolving symlinks is the point. launch-log.ts Reader side of the launch logs: the inverse of launchLogName, fenced-block extraction, run timestamps. + run-record.ts Run records — the parser for what BOTH launchers write, + and the merge that turns (record, log stat, queue match) + into one list row. Extracted for the usual reason, and + because the merge is where the decisions are: which + source wins on times, what a missing record means, and + what gets said when nothing is known. launch-policy.ts Reads and validates the shared launch roster, and builds the spawn argv. Same extraction reason. vocabulary.ts The task-queue vocabulary (statuses, task types, workflow @@ -82,6 +88,12 @@ importable. esbuild and `tsc` (`allowImportingTsExtensions`) both accept `.ts`. - **The headless-runs section reads launch logs and never writes them.** Both routes are `GET`, resolve through `resolveAllowedPath`, and never treat a route id as a path: `:id` is validated as `-` and the filename is then *rebuilt* via `launchLogName`, so the caller's string cannot reach the filesystem even before the realpath guard runs. The guard applies to the **list** route as well as the detail route — the list head-reads every file, so without it a symlink planted in the log directory puts the first line of its target into a row. That was verified against a real `~/.secrets/forge.env` symlink, which leaked with the guard removed and was refused with it present. - **The launch-log filename parser is the inverse of `launchLogName`, and is pinned to it by a round-trip test.** `launch-policy.ts` owns the name shape and two producers write it (this plugin and `task-dispatcher.py`). A hardcoded regex here would not error when it drifted — the section would simply list nothing. Unparseable names are **skipped**, never rendered with a guessed or empty agent. - **A run's status comes from the queue, never from the log.** A log proves a session ran; it does not prove its task closed. `steward-f42d3aeb` completed on 2026-08-23 against a task that sat at `approved` for four days. Render that disagreement — do not infer status from the log's prose. +- **A reaped run's `exit_code` is never rendered as success.** `outcomeLabel` maps a null code on an ended run to `ended, exit code unknown`, and the panel colours it as neither pass nor fail. Null is not missing data: a dispatcher tick spawns a detached child and exits, so the child is reparented and its status is reaped by init — there is no `waitpid()` and no surviving `/proc` entry. This plugin is long-lived and *can* observe its own children exit, so `child.on('exit')` records a real code for its own launches; that asymmetry is the design, not an inconsistency to iron out. +- **The run-record filename is the log filename with a `.json` suffix, and both are produced in `launch-policy.ts`.** `launchLogName` and `runRecordFileName` sit next to each other because the union in `listHeadlessRuns` keys on their shared stem — if the two ever disagree, one run renders as two rows, with the metadata on one and the output on the other. A round-trip test pins them. The `.log` name itself must not move: `task-dispatcher` writes it and the launch-log retention job matches on it. +- **The list is the union of logs and records, and a stem with only one of them still renders.** A log with no record is one of the 29 runs that predate them. A record with no readable log is the security-audit launcher, whose output goes to `~/.pm2/logs` — a prefix that stays outside `PREVIEW_ALLOWED_PREFIXES` deliberately, because it covers every PM2 service log on this host. Keying the list on `.log` files alone would omit the commonest kind of headless session here; adding the prefix to make it readable would turn this endpoint into a reader of every service log. The row renders and the detail view says where the log is. +- **`loadRunRecord()` in `run-record.ts` is the only reader of a run record.** It resolves through `resolveAllowedPath` and returns the approved path alongside the record, so a caller writing back uses the path the guard checked. Do not add a second inline read in `server.ts` — that is exactly what happened before, and because that reader's content was not surfaced anywhere, nothing was red and nothing was tested. The extraction exists so the guard has tests at all; `server.ts` calls `listen()` at import time and cannot have them. +- **Values off the wire are validated, not type-asserted.** `JSON.parse(body) as {…}` is a compile-time claim and no runtime check. `mode` on the Start route escaped notice for as long as it only reached safe `===` comparisons; it became a real gap the moment it acquired a consumer that persists it into task history. When adding a consumer of a request field, check what already validates it — the answer may be "the shape of the code that used to read it." +- **A Start records itself in the task's history without changing its status.** The control-API call re-asserts the status the task is already in, which the handler accepts with `allow_override` and a note. Advancing `approved` → `in-progress` here looks tidier and breaks every plugin-started session: the agent's own first action is `update_task(in-progress)`, permitted only *from* `approved`. Recording must also never fail the Start — the session is already running by then. - **`birthtime` is trusted only when it precedes `mtime`.** The historical logs were copy-migrated into the launch-log directory, and a copy resets birthtime while `cp -p` preserves mtime, so for every migrated run birthtime is *later* than mtime. Trusting it reports each as starting "just now" and running for a negative duration. An untrustworthy birthtime yields a `null` duration, rendered as unknown rather than as zero. - **Unterminated fenced blocks are dropped from `commands`.** Those strings are handed to the operator behind a copy button, i.e. built to be pasted into a shell. A fence with no closing delimiter has no known end, and half of a destructive command is worse than none. The full text is still rendered in the log pane. - **The WebSocket upgrade gates on the peer address first, and on `Origin` only if one is present.** The server binds `127.0.0.1` on an ephemeral port, so a non-loopback peer is refused outright; a loopback peer with a *present but wrong* `Origin` is still refused. A loopback peer with **no** `Origin` is accepted, because that is CloudCLI's own plugin WS proxy — it uses the `ws` client library, which sends no `Origin` unless one is passed, and its browser leg is already authenticated by CloudCLI's `verifyClient` before the proxy is invoked. diff --git a/CHANGELOG.md b/CHANGELOG.md index 97e589f..427b711 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,100 @@ All notable changes to this project will be documented in this file. +## [0.9.0] - 2026-08-29 + +Reads the run records both launchers now write, and makes a Start leave a mark on the task +it started. + +Tracker: vikunja#559. Build plan: agent-workflow-interop-2026-08, Phase 3. + +### Added + +- **A run record beside every launch this plugin starts.** + `~/.claude/comms/artifacts/task-launches/-.json`, the same shape + `task-dispatcher` v1.3.0 writes. A **sibling** of the log, never a replacement: the + `.log` name is what this plugin's own reader parses and what the launch-log retention + job matches on. +- **A real exit code for runs this plugin starts.** It is a long-lived process, so the + child handle outlives the spawn and `'exit'` still fires after `unref()`. A + dispatcher-launched run can only ever be reaped as `pid-gone` with a null code; this one + records what the process actually returned, and a signalled child records the signal + rather than a coerced number. +- **`FORGE_RUN_ID` / `FORGE_TASK_ID` in the launched session's environment**, so a trace + can be joined back to the task that paid for it. +- **An outcome column**, with three states kept distinct: `no run record`, `running`, + and the run's actual outcome — `exit 0`, `exit 137`, `ended, exit code unknown`, or + `slot released — still running`. `ended, exit code unknown` is the honest rendering of a + dispatcher launch and is deliberately not collapsed into success. +- **The list is now the union of logs and records.** A record with no co-located log + renders too — that is the security-audit launcher, whose output goes to `~/.pm2/logs`. + That prefix stays outside the preview allowlist because it covers every PM2 service log + on this host, so the detail view says where the log is rather than the row being dropped. + Dropping it would have omitted the commonest kind of headless session here. +- **Open runs sort above finished ones.** A descending compare on `ended` put them at the + bottom, under three months of finished runs. + +### Fixed + +- **A Start made no queue mutation at all.** A plugin-started task stayed at `approved` + until its agent got as far as claiming it, and a session that died before that left + nothing behind anywhere — which is why one completed steward run was invisible for four + days (vikunja#534). + + A Start now appends a history entry through the control API. **The status is + deliberately unchanged** — the call re-asserts the status the task is already in. + Advancing `approved` → `in-progress` is the obvious-looking alternative and it breaks + every plugin-started session: the agent's own first action is + `update_task(in-progress)`, which task-queue-mcp permits only *from* `approved`. +- **Duration for a still-open run is unknown, not a growing number.** It would otherwise + be derived from the log's mtime and tick upward on every poll for a session that in fact + died an hour ago and has not been reaped. + +### Changed + +- **`src/run-record.ts` added** — the parser for what both launchers write, plus the merge + that turns (record, log stat, queue match) into one list row. Extracted from `server.ts` + for the reason every other module here was: `server.ts` listens at import time, so + nothing inside it can be unit-tested, and the merge is where the decisions are. +- `runRecordFileName` sits beside `launchLogName` in `launch-policy.ts`, pinned to it by a + round-trip test. If the two stems disagree, one run renders as two rows. + +### Security + +Both from the audit (`agent-workflow-interop-2026-08-phase34`, 0 Critical/High/Medium, +2 Info). Both fixed rather than accepted. + +- **`mode` on `POST /tasks/:id/start` is validated, not type-asserted.** `JSON.parse(body) + as {mode: StartMode}` is a compile-time claim and no runtime check. Harmless while the + value only reached `=== 'review'` comparisons that fell through safely — but this + release gave it a second consumer that writes it into a task's **persisted history + note**, and a value that lands in a durable record deserves a validator. `toStartMode()` + now sits beside the type in `launch-policy.ts`, with the closed set exported so the two + cannot drift. + + An absent mode still defaults to `review`; a present but unrecognised one is a 400. + Defaulting there would silently downgrade an operator who asked for `auto`, turning a + typo into a session that quietly does nothing. An unparseable body is refused for the + same reason. + +- **`loadRunRecord()` is now the only way this plugin reads a run record**, extracted into + `run-record.ts` so the path guard has tests. `closeRunRecord`'s guard was added during + the pre-audit and had no regression test of its own — `server.ts` calls `listen()` at + import time, so nothing in it can be unit-tested, which is the same property that let + the guard be omitted to begin with. The audit's point was that a fix with no test is one + refactor from being undone. + + It returns the **guard-approved** path alongside the record, so a caller writing the + record back uses the path realpath approved rather than re-deriving one that was never + checked. Seven tests, including a real escaping symlink and a non-escaping one. + +### Unchanged, deliberately + +- **Status still comes from the queue and only from the queue.** A record saying a run + exited 0 does not promote its task's status. A finished run whose task is still + `approved` is the disagreement this panel exists to show. +- **The `-.log` filename.** Two other consumers key on it. + ## [0.8.0] - 2026-08-29 Closes the plugin's task-queue vocabulary drift, then gates it so it cannot silently diff --git a/README.md b/README.md index 6ad6078..2aa83ff 100644 --- a/README.md +++ b/README.md @@ -87,17 +87,56 @@ of final text, and exits. Every such launch already wrote its full stdout to 26 of these had accumulated with nothing able to show them, and one completed steward run stayed invisible for four days. -Each row shows agent, short task id, status, started, duration, and the first line of -output. Click a row to open the full log. +Each row shows agent, short task id, status, started, duration, outcome, and the first line +of output. Click a row to open the full log. **Status comes from the task queue, not from the log.** A log proves a session ran; it does not prove the task closed. The two disagreeing — a finished run whose task is still -`approved` — is the feature working, not a bug. - -**Duration can be unknown**, rendered as an em dash. It's derived from the log file's -timestamps, and birthtime is only trusted when it precedes mtime. Every log migrated into -the shared directory on 2026-08-27 was copied rather than moved, and a copy resets birthtime -while preserving mtime — so those runs show an unknown duration rather than a wrong one. +`approved` — is the feature working, not a bug. This holds for the run record too: a record +saying the run exited 0 never promotes a task's status. + +### Run records + +Both launchers now write `-.json` beside the log. It is a **sibling**, never a +replacement — the `.log` name is what this plugin's own reader parses and what the +launch-log retention job matches on. + +The list is the **union** of the two artefacts, keyed on the shared `-` stem: + +- A **log with no record** is one of the 29 runs that predate them. Times still come from + the file's mtime, and the outcome column reads `no run record`. +- A **record with no readable log** is the security-audit launcher, which writes its output + to `~/.pm2/logs/security-audit-.log`. That prefix stays outside the preview + allowlist deliberately — it covers every PM2 service log on this host, and adding it would + make this endpoint a reader of all of them. The row renders anyway and the detail view + says where the log is, because dropping it would omit the commonest kind of headless + session here. + +**The outcome column has three honest states and does not collapse them.** + +| Rendered | Means | +|---|---| +| `no run record` | Predates run records; nothing is known about how it ended | +| `running` | A record with no `ended` | +| `exit 0`, `exit 137` | A real observed exit code — only this plugin's own launches get one | +| `ended, exit code unknown` | The run ended and the code is unrecoverable | +| `slot released — still running` | Past the dispatcher's max runtime; its concurrency slot was freed and the process was left alone | + +`ended, exit code unknown` is not a gap. A dispatcher tick spawns a detached child and +exits, so the child is reparented and its status is reaped by init — there is no `waitpid()` +and no surviving `/proc` entry. This plugin is a long-lived process and *can* observe its own +children exit, so runs it starts carry a real code. Rendering the unknown case as success +would be a counter reporting success for something nobody observed succeed. + +**Duration can be unknown**, rendered as an em dash. With a record it is `ended - started`, +and it is unknown while the run is still open — deliberately not "now minus started", which +would tick upward forever for a session that died an hour ago and has not been reaped. +Without a record it comes from the log file's timestamps, where birthtime is only trusted +when it precedes mtime: every log migrated into the shared directory on 2026-08-27 was +copied rather than moved, and a copy resets birthtime while preserving mtime. + +Open runs sort above finished ones. A plain descending compare on `ended` puts them at the +bottom, under three months of finished runs. Below the log text, a **Commands** block lists every fenced code block scraped from the output, each with a copy button. The extraction is deliberately dumb — no inference about @@ -161,7 +200,7 @@ The backend exposes a small HTTP API consumed by the UI via `api.rpc()`. | `GET` | `/health` | Liveness check; returns `{status, uptime, version}` | | `GET` | `/tasks` | List tasks; query params `agent`, `status`, `type` | | `GET` | `/tasks/:id` | Task detail plus context-ref previews | -| `POST` | `/tasks/:id/start` | Launch a session; body `{mode: "review"\|"auto"}` (a local spawn, not a queue mutation) | +| `POST` | `/tasks/:id/start` | Launch a session; body `{mode: "review"\|"auto"}`. Spawns locally, writes a run record, and records the launch in the task's history | | `POST` | `/tasks/:id/approve` | Approve — proxied | | `POST` | `/tasks/:id/cancel` | Cancel (terminal); body `{note?}` — proxied | | `POST` | `/tasks/:id/status` | Operator status change; body `{status, note?, allow_override?}` — proxied | @@ -188,6 +227,11 @@ The upgrade handler gates on the **peer address** first: the server binds `127.0 | `review` | `plan` | Read the task, present a summary, wait for approval | | `auto` | `default` | Read the task, claim it (`in-progress`), execute | +`mode` is validated against that set at the parse site. An **omitted** mode defaults to +`review` — the safe leg. A **present but unrecognised** mode is a 400, not a silent +default: defaulting would downgrade an operator who asked for `auto`, turning a typo into +a session that quietly does nothing. + ### The launch policy file A task's `target_agent` is resolved through a **data file**, not a map in the source: @@ -236,10 +280,29 @@ rather than launching. > `--permission-mode plan` is not reachable. The UI says so on launch rather than implying a > tool gate. -### Launch logs +### Launch logs and run records Each launch appends to `~/.claude/comms/artifacts/task-launches/-.log`, the -same shape and directory the reference dispatcher writes, so both are listable together. +same shape and directory the reference dispatcher writes, so both are listable together, and +writes `-.json` beside it. The session also receives `FORGE_RUN_ID` and +`FORGE_TASK_ID` in its environment, which is what makes a trace joinable back to its task. + +### A Start leaves a mark on the task + +Before v0.9.0 a Start made **no queue mutation at all**, so a plugin-started task stayed at +`approved` until its agent got as far as claiming it — and a session that died before that +left nothing behind anywhere. That is why one completed steward run was invisible for four +days. + +A Start now appends a history entry through the control API. **The status is deliberately +unchanged**: the call re-asserts the status the task is already in. Advancing +`approved` → `in-progress` here is the obvious-looking alternative and it breaks every +plugin-started session — the agent's own first action is `update_task(in-progress)`, which +task-queue-mcp permits only *from* `approved`. Doing it for the agent means its claim is +rejected as an invalid transition. + +A failure to record is logged and does not fail the Start: the session is already running by +then, and reporting the launch as failed would be the bigger lie. ## Development diff --git a/manifest.json b/manifest.json index 88f0af7..390af0d 100644 --- a/manifest.json +++ b/manifest.json @@ -1,7 +1,7 @@ { "name": "task-queue", "displayName": "Task Queue", - "version": "0.8.0", + "version": "0.9.0", "description": "Task queue dashboard — view, filter, and launch agent tasks.", "author": "TadMSTR", "icon": "icon.svg", diff --git a/package.json b/package.json index 751bc46..4ad159c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "cloudcli-plugin-task-queue", - "version": "0.8.0", + "version": "0.9.0", "private": true, "type": "module", "scripts": { diff --git a/src/launch-log.ts b/src/launch-log.ts index 91cc551..2479439 100644 --- a/src/launch-log.ts +++ b/src/launch-log.ts @@ -34,6 +34,7 @@ export interface ParsedLaunchLog { * a bare task UUID and no agent, and a row with an empty agent is worse than no row. */ const LAUNCH_LOG_RE = /^([a-z][a-z0-9_-]*)-([0-9a-f]{8})\.log$/; +const RUN_RECORD_RE = /^([a-z][a-z0-9_-]*)-([0-9a-f]{8})\.json$/; export function parseLaunchLogName(filename: string): ParsedLaunchLog | null { const m = LAUNCH_LOG_RE.exec(filename); @@ -41,6 +42,17 @@ export function parseLaunchLogName(filename: string): ParsedLaunchLog | null { return { agent: m[1], taskId8: m[2] }; } +/** + * The same split for a run record. Two regexes rather than one with an alternating + * suffix, so a caller cannot accidentally accept a `.log` where it wanted a `.json` — + * the two are listed from one directory and the union is keyed on the shared stem. + */ +export function parseRunRecordName(filename: string): ParsedLaunchLog | null { + const m = RUN_RECORD_RE.exec(filename); + if (!m) return null; + return { agent: m[1], taskId8: m[2] }; +} + /** The `-` route id for a run, i.e. its filename without `.log`. */ export function runId(agent: string, taskId8: string): string { return `${agent}-${taskId8}`; diff --git a/src/launch-policy.ts b/src/launch-policy.ts index c386f7b..c203738 100644 --- a/src/launch-policy.ts +++ b/src/launch-policy.ts @@ -170,6 +170,28 @@ export function loadLaunchPolicy(filePath: string, home: string): LaunchPolicy { /** What the plugin's Start button sends. */ export type StartMode = 'review' | 'auto'; + +/** The closed set behind StartMode, so the type can be checked at runtime too. */ +export const START_MODES: readonly StartMode[] = ['review', 'auto']; + +/** + * A `mode` off the wire, or null if it is not one. + * + * `POST /tasks/:id/start` used to type-ASSERT its body (`JSON.parse(body) as {mode}`), + * which is a compile-time claim and no runtime check at all. That was harmless while the + * value only ever reached `=== 'review'` comparisons that fell through safely — but the + * run-record build gave it a second consumer that writes it into a task's PERSISTED + * history note, and a value that lands in a durable record deserves a validator. + * + * ABSENT IS NOT INVALID. An omitted mode keeps the long-standing `review` default: the + * safe leg, the one that reads the task and waits. A PRESENT but unrecognised mode is + * null and the caller must refuse it — defaulting there would silently downgrade an + * operator who asked for `auto`, turning a typo into a session that quietly does nothing. + */ +export function toStartMode(raw: unknown): StartMode | null { + if (raw === undefined || raw === null) return 'review'; + return (START_MODES as readonly string[]).includes(raw as string) ? (raw as StartMode) : null; +} /** * What task-queue-mcp and run-steward.sh accept. Sourced from `vocabulary.ts` rather than * spelled again here — this file had its own copy, and copies of this list are vikunja#558. @@ -260,3 +282,12 @@ export function buildLaunchArgv( export function launchLogName(agent: string, taskId: string): string { return `${agent}-${taskId.slice(0, 8)}.log`; } + +/** + * Run-record filename — the log's stem with a `.json` suffix. Also shared with + * task-dispatcher (run_record_name()). Defined here rather than in run-record.ts so the + * two names that must agree sit next to each other; splitting them is how the stem drifts. + */ +export function runRecordFileName(agent: string, taskId: string): string { + return `${agent}-${taskId.slice(0, 8)}.json`; +} diff --git a/src/panels/headless-runs.ts b/src/panels/headless-runs.ts index 6689e2f..e71d6b5 100644 --- a/src/panels/headless-runs.ts +++ b/src/panels/headless-runs.ts @@ -22,6 +22,30 @@ interface HeadlessRunsOptions { onCopy: (text: string) => void; } +/** + * The outcome column. Three distinct states, never collapsed: + * + * '' no run record — this run predates them; nothing is known + * 'running' a record with no `ended` + * run.outcome 'exit 0' | 'exit 137' | 'ended, exit code unknown' | … + * + * The middle phrase of the third is the one that must survive review. Rendering an + * unrecoverable exit code as "ok" is exactly the counter-reporting-success failure the + * run record was added to expose. + */ +function outcomeText(run: Pick): string { + if (!run.has_record) return 'no run record'; + return run.outcome ?? 'running'; +} + +function outcomeColor(run: HeadlessRun, c: ThemeColors): string { + if (!run.has_record) return c.muted; + if (run.outcome === null) return c.accent; // still running + if (run.exit_code === 0) return c.muted; // ordinary success, not shouted + if (run.exit_code !== null) return c.warn; // a real non-zero code + return c.muted; // honestly unknown +} + function formatDuration(seconds: number | null): string { // An unknown duration reads as unknown. Rendering it as 0s would claim the run was // instantaneous, which is a different and false statement. @@ -57,7 +81,9 @@ export function renderHeadlessRuns(parent: HTMLElement, opts: HeadlessRunsOption blurb.textContent = 'Agent sessions launched without an operator watching. Each one wrote its final ' + 'output to a log; this is that output. Status comes from the task queue, not from ' - + 'the log — a finished run whose task is still open is worth noticing.'; + + 'the log — a finished run whose task is still open is worth noticing. Outcome comes ' + + 'from the run record, and "exit code unknown" is a real answer: a dispatcher tick ' + + 'does not outlive the session it starts, so there is no code left to read.'; section.appendChild(blurb); if (opts.selectedRunId && opts.selectedRun) { @@ -139,6 +165,7 @@ function renderRow(run: HeadlessRun, opts: HeadlessRunsOptions): HTMLElement { ${escHtml(run.status)} ${escHtml(ago(run.started))} ${escHtml(formatDuration(run.duration_s))} + ${escHtml(outcomeText(run))} `; // The first line is agent stdout. textContent, never innerHTML. @@ -169,9 +196,24 @@ function renderDetail(section: HTMLElement, opts: HeadlessRunsOptions): void { ${escHtml(run.agent)} ${escHtml(run.task_id ?? run.task_id8)} ${escHtml(run.status)} + ${escHtml(outcomeText(run))} + ${run.launched_by ? `via ${escHtml(run.launched_by)}` : ''} `; section.appendChild(meta); + // A run whose log this UI may not read still renders — it is a real run the operator + // asked about. Saying where the output is beats an empty pane or a 404. + if (!run.log_readable) { + const note = document.createElement('div'); + note.style.cssText = `color:${c.warn};font-size:11px;margin-bottom:10px;padding:10px;` + + `background:${c.surface};border:1px solid ${c.border};border-radius:4px;line-height:1.5;`; + note.textContent = run.log_path + ? `This run's output is not readable from here. It is at: ${run.log_path}` + : 'This run left no readable log in the launch directory.'; + section.appendChild(note); + return; + } + // ── Commands ── // Omitted entirely when there are none, rather than rendered as an empty header. if (run.commands.length > 0) { diff --git a/src/run-record.ts b/src/run-record.ts new file mode 100644 index 0000000..ba3df4d --- /dev/null +++ b/src/run-record.ts @@ -0,0 +1,272 @@ +/** + * Run records — the entity that turns a launch from a side effect into a fact. + * + * Written by BOTH launchers into ~/.claude/comms/artifacts/task-launches/ as + * `-.json`, a SIBLING of the `-.log` each already wrote. + * The log's name is deliberately untouched: this plugin's own reader parses it, and the + * launch-log retention job matches on it, so renaming it to make room for a record would + * have silently stopped both. + * + * This module is both the writer (for runs this plugin starts) and the parser. It has no + * `fs` import beyond what the writer needs and no HTTP, so it is unit-testable without + * booting server.ts — the ws-guard.ts reason. + * + * THE FIELD SET IS A CROSS-REPO CONTRACT with task_dispatcher.cli.write_run_record(). + * There is no gate pinning the two yet; Phase 5 of agent-workflow-interop-2026-08 is + * where that lands. Until then, parseRunRecord() below treats every field except + * `task_id` as optional, so a dispatcher that adds one does not make this side refuse + * the record outright. + */ + +import fs from 'node:fs'; +import path from 'node:path'; +import { resolveAllowedPath } from './path-guard.ts'; +import { runRecordFileName } from './launch-policy.ts'; + +/** What a launcher writes. Nulls are meaningful: see `exit_code`. */ +export interface RunRecord { + run_id: string; + task_id: string; + agent: string; + /** 'dispatcher' | 'dispatcher-audit' | 'plugin' — who started it. */ + launched_by: string; + run_as_user: string | null; + launcher: string | null; + workflow_mode: string; + started: string; + pid: number | null; + /** ISO timestamp, or null while the run is still open. */ + ended: string | null; + /** + * NULL IS NOT A MISSING VALUE. For a dispatcher-launched run it is the honest answer: + * a cron tick spawns a detached child and exits, so the child is reparented and its + * status is reaped by init — there is no exit code left to read anywhere. `reaped` + * says which kind of unknown it is. A zero here would report success for something + * nobody observed succeed. + * + * This plugin is a long-lived process and CAN observe its own children exit, so runs + * it starts do carry a real code. + */ + exit_code: number | null; + /** 'pid-gone' | 'max-runtime' | null — set by whoever closed the record. */ + reaped: string | null; + log_path: string; +} + +function str(v: unknown, fallback = ''): string { + return typeof v === 'string' ? v : fallback; +} + +function strOrNull(v: unknown): string | null { + return typeof v === 'string' && v !== '' ? v : null; +} + +/** + * Parse a record written by either launcher. Returns null for anything unusable. + * + * Tolerant on purpose, and only in one direction: an UNKNOWN field is ignored and a + * MISSING field degrades to null, but a record with no `task_id` is rejected outright + * because every consumer here joins on it. The two writers live in different repositories + * and deploy separately, so a record from a newer dispatcher than this plugin is a + * routine state, not a corruption — refusing it would blank the panel on exactly the + * runs the operator most wants to see. + */ +export function parseRunRecord(raw: unknown): RunRecord | null { + // Array.isArray is intent, not validation: JSON cannot put a `task_id` property on an + // array, so the check below already rejects every array. Stated explicitly so the shape + // this accepts is readable, rather than inferred from what happens to fail later. + if (raw === null || typeof raw !== 'object' || Array.isArray(raw)) return null; + const r = raw as Record; + const taskId = str(r.task_id); + if (!taskId) return null; + return { + run_id: str(r.run_id), + task_id: taskId, + agent: str(r.agent), + launched_by: str(r.launched_by, 'unknown'), + run_as_user: strOrNull(r.run_as_user), + launcher: strOrNull(r.launcher), + workflow_mode: str(r.workflow_mode, 'unknown'), + started: str(r.started), + // A pid of 0 or a non-number is not a pid. Left null rather than coerced, so + // "no pid recorded" and "pid 0" cannot be confused. + pid: typeof r.pid === 'number' && Number.isInteger(r.pid) && r.pid > 0 ? r.pid : null, + ended: strOrNull(r.ended), + exit_code: typeof r.exit_code === 'number' ? r.exit_code : null, + reaped: strOrNull(r.reaped), + log_path: str(r.log_path), + }; +} + +/** A record and the realpath-resolved path it was read from. */ +export interface LoadedRunRecord { + path: string; + record: RunRecord; +} + +/** + * Read a run record through the path guard. THE only way this plugin reads one. + * + * Extracted here rather than left in server.ts because server.ts calls `listen()` at + * import time and cannot be unit-tested — which is exactly how one of its two readers + * came to be missing this guard in the first place. That reader's content is not + * surfaced anywhere, so nothing failed and nothing was red; it was found by a pre-audit + * sweep and its fix then had no regression test of its own. Both readers now call this, + * and this has tests. + * + * Returns the resolved path alongside the record so a caller that writes the record back + * uses the path the guard approved, rather than re-deriving one that was never checked. + */ +export function loadRunRecord( + dir: string, + agent: string, + taskId: string, + allowedPrefixes: string[], +): LoadedRunRecord | null { + const resolved = resolveAllowedPath( + path.join(dir, runRecordFileName(agent, taskId)), + allowedPrefixes, + ); + if (!resolved) return null; + let record: RunRecord | null; + try { + record = parseRunRecord(JSON.parse(fs.readFileSync(resolved, 'utf-8'))); + } catch { + return null; // absent or corrupt: the caller falls back to the mtime derivation + } + return record ? { path: resolved, record } : null; +} + +export interface RunSpan { + started: string; + ended: string; + /** null when unknowable OR when the run has not ended. Never rendered as zero. */ + duration_s: number | null; +} + +/** + * The run's span, from the record rather than from file timestamps. + * + * `ended: null` means the run is still open, and duration is therefore not yet a + * quantity — not zero, and not "now minus started", which would tick upward on every + * poll for a session that in fact died an hour ago and has not been reaped. The list + * shows the started time for those, and the reaper fills the rest in. + * + * Returns null if the record carries no usable start time, so the caller can fall back + * to the mtime derivation the 29 pre-record logs still depend on. + */ +export function recordSpan(record: RunRecord): RunSpan | null { + const startMs = Date.parse(record.started); + if (!Number.isFinite(startMs)) return null; + const endMs = record.ended === null ? NaN : Date.parse(record.ended); + if (!Number.isFinite(endMs)) { + return { started: new Date(startMs).toISOString(), ended: '', duration_s: null }; + } + return { + started: new Date(startMs).toISOString(), + ended: new Date(endMs).toISOString(), + // A negative span means the two timestamps disagree about ordering; report unknown + // rather than a negative duration. The same failure the mtime path already guards. + duration_s: endMs >= startMs ? Math.round((endMs - startMs) / 1000) : null, + }; +} + +/** + * How a finished run ended, in one phrase, or null while it is still open. + * + * The three cases are deliberately distinct in the UI. "exit 0" is a real observation; + * "ended, exit code unknown" is the honest rendering of a dispatcher-launched run; and + * "still running (over max runtime)" is a run whose slot was released while its process + * kept going. Collapsing the middle one into "ok" is the whole failure this build is + * about. + */ +export function outcomeLabel(record: RunRecord): string | null { + if (record.ended === null) return null; + if (record.reaped === 'max-runtime') return 'slot released — still running'; + if (record.exit_code === null) return 'ended, exit code unknown'; + return record.exit_code === 0 ? 'exit 0' : `exit ${record.exit_code}`; +} + +// ── Assembling a list row ───────────────────────────────────────────── +// +// Extracted from server.ts's listHeadlessRuns for the reason every other module here was +// (ws-guard, path-guard, control-api, dead-letters): server.ts opens a listener at import +// time, so nothing that lives in it can be unit-tested. This is the part with the actual +// decisions in it — which source wins on times, what a missing record means, and what +// gets said when nothing is known — so it is the part that must be testable. + +/** What the caller gathered from disk for one `-` stem. */ +export interface RunSources { + agent: string; + taskId8: string; + /** The parsed run record, or null for the runs that predate them. */ + record: RunRecord | null; + /** Times derived from the log's mtime/birthtime, or null when no log is readable. */ + fileTimes: RunSpan | null; + logSize: number; + firstLine: string; + /** The live queue task matching the id prefix, if exactly one does. */ + queue?: { status: string; taskId: string }; +} + +export interface HeadlessRunView { + id: string; + agent: string; + task_id8: string; + task_id: string | null; + status: string; + started: string; + ended: string; + duration_s: number | null; + size: number; + first_line: string; + has_record: boolean; + launched_by: string | null; + outcome: string | null; + exit_code: number | null; + log_readable: boolean; +} + +export function toHeadlessRunView(src: RunSources): HeadlessRunView { + const { record } = src; + // The record wins on times when it has usable ones: it knows when the PROCESS started, + // where the log only knows when its file was touched. For the 26 logs copy-migrated on + // 2026-08-27 those differ by months, which is why runTimes() has to distrust birthtime + // at all — a record has no such problem and does not need the same defence. + const times = (record ? recordSpan(record) : null) + ?? src.fileTimes + ?? { started: '', ended: '', duration_s: null }; + return { + id: `${src.agent}-${src.taskId8}`, + agent: src.agent, + task_id8: src.taskId8, + // The record carries the WHOLE task id even when no live queue task matches the + // prefix — which is what keeps an archived task's run identifiable. + task_id: src.queue?.taskId ?? record?.task_id ?? null, + // Status comes from the QUEUE and only from the queue. A run record proves a session + // ran and how it ended; it does not prove the task closed. steward-f42d3aeb is a + // completed run whose task sat at `approved` for four days — that disagreement is + // signal, and inferring status from the record would erase it. + status: src.queue?.status ?? 'unknown', + size: src.logSize, + first_line: src.firstLine, + has_record: record !== null, + launched_by: record?.launched_by ?? null, + outcome: record ? outcomeLabel(record) : null, + exit_code: record?.exit_code ?? null, + log_readable: src.fileTimes !== null, + ...times, + }; +} + +/** + * List order: open runs first, then most recently ended. + * + * Open runs have no `ended`, which a plain descending string compare sorts to the BOTTOM + * — underneath three months of finished ones, which is precisely where an operator will + * not look for the session that is running right now. + */ +export function compareRuns(a: HeadlessRunView, b: HeadlessRunView): number { + if (!a.ended !== !b.ended) return a.ended ? 1 : -1; + return a.ended ? b.ended.localeCompare(a.ended) : b.started.localeCompare(a.started); +} diff --git a/src/server.ts b/src/server.ts index 33f68f5..2d49dc7 100644 --- a/src/server.ts +++ b/src/server.ts @@ -3,6 +3,7 @@ import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; import { spawn } from 'node:child_process'; +import { randomUUID } from 'node:crypto'; import { WebSocketServer, WebSocket } from 'ws'; import { load as yamlLoad } from 'js-yaml'; import { callControlApi, type ControlAction } from './control-api.ts'; @@ -10,6 +11,14 @@ import { evaluateUpgrade, allowedOrigins } from './ws-guard.ts'; import { resolveAllowedPath } from './path-guard.ts'; import type { DeadLetter, HeadlessRun, HeadlessRunDetail } from './types.ts'; import { toDeadLetter } from './dead-letters.ts'; +import { isTerminal } from './vocabulary.ts'; +import { + loadRunRecord, + outcomeLabel, + toHeadlessRunView, + compareRuns, + type RunRecord, +} from './run-record.ts'; import { parseLaunchLogName, parseRunId, @@ -18,12 +27,16 @@ import { firstLine, extractFencedBlocks, runTimes, + parseRunRecordName, } from './launch-log.ts'; import { loadLaunchPolicy, policyPath, buildLaunchArgv, launchLogName, + runRecordFileName, + toStartMode, + START_MODES, lookupAgent, LaunchPolicyError, type LaunchPolicy, @@ -235,6 +248,102 @@ function resolveBin(name: string): string | null { return null; } +/** Write a run record beside the launch log. Never throws; a launch is already running. */ +function writeRunRecord(record: RunRecord): void { + try { + fs.mkdirSync(LAUNCH_LOG_DIR, { recursive: true }); + const target = path.join(LAUNCH_LOG_DIR, runRecordFileName(record.agent, record.task_id)); + // Write-then-rename, and the tmp name carries this process's pid: two producers write + // this directory (this plugin and the dispatcher), so a bare `.tmp` could collide. + const tmp = `${target}.${process.pid}.tmp`; + fs.writeFileSync(tmp, `${JSON.stringify(record, null, 2)}\n`, { mode: 0o600 }); + fs.renameSync(tmp, target); + } catch (err) { + // A missing record undercounts a concurrency slot; a thrown error here would fail a + // Start whose session is already running, which is strictly worse. + console.error(`[task-queue] could not write run record for ${record.task_id}: ${(err as Error).message}`); + } +} + +/** + * Stamp a run as ended with the code its child actually returned. + * + * Re-read and re-written rather than held in memory: the dispatcher's reaper may have + * closed the same record in between, and the file is the shared state. If it did, its + * `pid-gone` is left alone — a reaper that already gave up on this run has recorded + * something true, and overwriting it with a code observed later would be tidier and less + * accurate about what was known when. + */ +function closeRunRecord( + agent: string, + taskId: string, + code: number | null, + signal: NodeJS.Signals | null, +): void { + // Through loadRunRecord, like the list route — one guarded reader, and it is unit + // tested. This call site had its own inline read until the audit pointed out that a + // fix with no regression test is one refactor away from being undone, and that this + // reader is the one that gets forgotten precisely because its content is not surfaced. + const loaded = loadRunRecord(LAUNCH_LOG_DIR, agent, taskId, PREVIEW_ALLOWED_PREFIXES); + if (!loaded) return; + // The path the GUARD approved, not one re-derived here. + const target = loaded.path; + try { + const existing = loaded.record; + if (existing.ended !== null) return; + existing.ended = new Date().toISOString(); + // A signalled child has no exit code — `code` is null and `signal` names it. Recorded + // as the reason rather than coerced to a number, for the same reason the dispatcher + // refuses to invent a zero: 'killed by SIGKILL' and 'exited 0' are different facts. + existing.exit_code = code; + existing.reaped = signal === null ? 'exited' : `signal:${signal}`; + const tmp = `${target}.${process.pid}.tmp`; + fs.writeFileSync(tmp, `${JSON.stringify(existing, null, 2)}\n`, { mode: 0o600 }); + fs.renameSync(tmp, target); + } catch { /* the record was never written, or is gone — nothing to close */ } +} + +/** + * Leave a trace of a Start in the TASK, not only in the launch directory. + * + * A launch that leaves no mark on the task is why a completed steward run sat invisible + * for four days: the plugin's Start button made no queue mutation at all, so a + * plugin-started task stayed at `approved` until its agent got as far as claiming it — + * and a session that died before that left nothing behind anywhere. + * + * THE STATUS IS DELIBERATELY UNCHANGED — the call re-asserts the status the task is + * already in. It is tempting to advance `approved` → `in-progress` here, and it would + * break every plugin-started session: the agent's own first action is + * update_task(in-progress), which task-queue-mcp permits only FROM `approved`. Doing it + * for the agent means its claim is refused as an invalid transition. + * + * A same-status move needs `allow_override` plus a note, both of which this passes; the + * handler appends the history entry and the assignment is a no-op. Best-effort: a + * failure here is logged and does not fail the Start, because the session is already + * running by this point and reporting the launch as failed would be the bigger lie. + */ +async function recordStartInQueue( + taskId: string, + taskData: Record, + mode: StartMode, +): Promise { + const current = typeof taskData.status === 'string' ? taskData.status : ''; + // Terminal and unknown statuses are refused by the handler anyway; not asking is + // quieter than asking and logging a rejection on every Start of a closed task. + if (!current || isTerminal(current)) return; + const { status, data } = await callControlApi(taskId, 'status', { + status: current, + allow_override: true, + note: `Session launched from CloudCLI in ${mode} mode`, + }, { apiBase: TASK_QUEUE_API, secret: TASK_QUEUE_API_SECRET }); + if (status !== 200) { + console.error( + `[task-queue] launched ${taskId} but could not record it in the task's history: ` + + `${status} ${JSON.stringify(data)}`, + ); + } +} + /** * `queuedMode` is the task's own `workflow_mode`, passed through to the launcher. It is * read from the queue record by the caller and never inferred here — see toWorkflowMode. @@ -297,17 +406,56 @@ function launchSession( return { ok: false, error: `Cannot open launch log: ${(err as Error).message}` }; } + const runId = randomUUID(); try { const child = spawn(argv[0], argv.slice(1), { cwd: entry.projectDir, stdio: ['ignore', logFd, logFd], detached: true, + // The run identity, so a Langfuse trace can be joined back to the task that paid + // for it. Same two names the dispatcher uses; a run-as agent gets them from its + // launcher's flags instead, because sudo scrubs the environment. + env: { ...process.env, FORGE_RUN_ID: runId, FORGE_TASK_ID: taskId }, }); child.on('error', (err) => { try { fs.appendFileSync(logPath, `[launch error] ${err.message}\n`); } catch { /* best-effort */ } }); + // WRITTEN AFTER spawn, because the record's purpose is to carry a pid. A record + // written first would have to carry a null one, which every reader treats as a dead + // run — so the dispatcher's reaper would sweep the task to `failed` moments after + // the session started successfully. + writeRunRecord({ + run_id: runId, + task_id: taskId, + agent: targetAgent, + launched_by: 'plugin', + run_as_user: entry.runAsUser ?? null, + launcher: entry.launcher ?? null, + workflow_mode: queuedMode ?? 'unknown', + started: new Date().toISOString(), + pid: child.pid ?? null, + ended: null, + exit_code: null, + reaped: null, + log_path: logPath, + }); + + // THIS PLUGIN CAN DO WHAT THE DISPATCHER CANNOT. It is a long-lived process, so the + // handle outlives the child and 'exit' still fires after unref() — unref only drops + // the event-loop reference, and the HTTP server keeps the loop alive regardless. So + // runs started here carry a REAL exit code, where a dispatcher-launched run can only + // ever be reaped as `pid-gone`. + // + // If CloudCLI restarts before the child ends, this never fires and the record stays + // open. That is not a leak: the dispatcher's reaper closes it on the next tick, with + // the honest `pid-gone` and a null code. The two mechanisms are complementary and + // neither invents an outcome. + child.on('exit', (code, signal) => { + closeRunRecord(targetAgent, taskId, code, signal); + }); + child.unref(); return note ? { ok: true, note } : { ok: true }; } catch (err) { @@ -378,48 +526,84 @@ function readHead(filePath: string, bytes: number): string { } } -function listHeadlessRuns(agentFilter?: string): HeadlessRun[] { +/** + * Read the run record for a stem, if there is one and it is readable from here. + * + * The path guard runs on records exactly as it does on logs. A record is JSON this + * process parses and copies into an API response, so a symlink planted in the launch + * directory would otherwise put an arbitrary file's contents in front of the operator. + */ +function readRunRecordFor(agent: string, taskId8: string): RunRecord | null { + return loadRunRecord(LAUNCH_LOG_DIR, agent, taskId8, PREVIEW_ALLOWED_PREFIXES)?.record ?? null; +} + +/** + * Every `-` stem in the launch directory, from EITHER artefact. + * + * The union matters in one direction that is not hypothetical: the security-audit + * launcher writes its record here and its log to ~/.pm2/logs, so keying the list on + * `.log` files alone would omit the commonest kind of headless session on this host. + * Listing only `.json` would drop the other 29 the other way — those predate run records. + */ +function launchStems(): Map { + const stems = new Map(); let names: string[]; try { names = fs.readdirSync(LAUNCH_LOG_DIR); } catch { - return []; // directory absent is an empty list, not an error + return stems; // directory absent is an empty list, not an error + } + for (const name of names) { + // Skip anything matching neither shape rather than guessing at an agent. Two + // pre-2026-08 orphans are named with a bare task UUID and no agent at all. + const parsed = parseLaunchLogName(name) ?? parseRunRecordName(name); + if (!parsed) continue; + stems.set(runId(parsed.agent, parsed.taskId8), parsed); } + return stems; +} +function listHeadlessRuns(agentFilter?: string): HeadlessRun[] { const queue = queueIndexByPrefix(); const runs: HeadlessRun[] = []; - for (const name of names) { - // Skip anything that is not -.log rather than guessing at an agent. - // Two pre-2026-08 orphans are named with a bare task UUID and no agent. - const parsed = parseLaunchLogName(name); - if (!parsed) continue; + for (const parsed of launchStems().values()) { if (agentFilter && parsed.agent !== agentFilter) continue; + const record = readRunRecordFor(parsed.agent, parsed.taskId8); + // The guard runs on the LIST route too, not only on the detail route: this loop // head-reads every file, so a symlink planted in the log dir would otherwise put // the first line of its target into a row. - const resolved = resolveAllowedPath(path.join(LAUNCH_LOG_DIR, name), PREVIEW_ALLOWED_PREFIXES); - if (!resolved) continue; - - let st: fs.Stats; - try { st = fs.statSync(resolved); } catch { continue; } - if (!st.isFile()) continue; + const resolved = resolveAllowedPath( + path.join(LAUNCH_LOG_DIR, launchLogName(parsed.agent, parsed.taskId8)), + PREVIEW_ALLOWED_PREFIXES, + ); + let st: fs.Stats | null = null; + if (resolved) { + try { + const stat = fs.statSync(resolved); + if (stat.isFile()) st = stat; + } catch { /* record with no co-located log — the audit launcher's shape */ } + } + // A stem with neither a readable log nor a record is nothing to show. + if (!st && !record) continue; - const match = queue.get(parsed.taskId8); - runs.push({ - id: runId(parsed.agent, parsed.taskId8), + // Everything below the read is toHeadlessRunView's, in run-record.ts, so the merge + // decisions are unit-testable — this function can only be exercised by booting a + // listener. It does the reading; that does the deciding. + runs.push(toHeadlessRunView({ agent: parsed.agent, - task_id8: parsed.taskId8, - task_id: match?.taskId ?? null, - status: match?.status ?? 'unknown', - size: st.size, - first_line: firstLine(readHead(resolved, HEADLESS_HEAD_BYTES)), - ...runTimes(st), - }); + taskId8: parsed.taskId8, + record, + fileTimes: st ? runTimes(st) : null, + logSize: st?.size ?? 0, + firstLine: st && resolved ? firstLine(readHead(resolved, HEADLESS_HEAD_BYTES)) : '', + queue: queue.get(parsed.taskId8), + })); } - runs.sort((a, b) => b.ended.localeCompare(a.ended)); + runs.sort(compareRuns); return runs; } @@ -430,40 +614,59 @@ function readHeadlessRun(id: string): HeadlessRunDetail | null { const parsed = parseRunId(id); if (!parsed) return null; + const record = readRunRecordFor(parsed.agent, parsed.taskId8); + const resolved = resolveAllowedPath( path.join(LAUNCH_LOG_DIR, runIdToFilename(parsed)), PREVIEW_ALLOWED_PREFIXES, ); - if (!resolved) return null; - let text: string; - let truncated: boolean; + let text = ''; + let truncated = false; + let logReadable = false; let fd: number | null = null; - try { - const st = fs.statSync(resolved); - if (!st.isFile()) return null; - fd = fs.openSync(resolved, 'r'); - // Read one byte past the cap so a file exactly at the cap is not called truncated. - const buf = Buffer.alloc(HEADLESS_MAX_BYTES + 1); - const n = fs.readSync(fd, buf, 0, HEADLESS_MAX_BYTES + 1, 0); - truncated = n > HEADLESS_MAX_BYTES; - text = buf.subarray(0, Math.min(n, HEADLESS_MAX_BYTES)).toString('utf-8'); - } catch { - return null; - } finally { - if (fd !== null) { try { fs.closeSync(fd); } catch { /* already gone */ } } + if (resolved) { + try { + const st = fs.statSync(resolved); + if (st.isFile()) { + fd = fs.openSync(resolved, 'r'); + // Read one byte past the cap so a file exactly at the cap is not called truncated. + const buf = Buffer.alloc(HEADLESS_MAX_BYTES + 1); + const n = fs.readSync(fd, buf, 0, HEADLESS_MAX_BYTES + 1, 0); + truncated = n > HEADLESS_MAX_BYTES; + text = buf.subarray(0, Math.min(n, HEADLESS_MAX_BYTES)).toString('utf-8'); + logReadable = true; + } + } catch { /* falls through to the record-only rendering below */ } + finally { + if (fd !== null) { try { fs.closeSync(fd); } catch { /* already gone */ } } + } } + // An unreadable log is only a 404 when there is no record either. With a record, the + // run is a real thing the operator asked about and the honest answer is its metadata + // plus where the output actually is — the security-audit launcher writes to + // ~/.pm2/logs, which stays outside the preview allowlist deliberately: that prefix + // covers every PM2 service log on the host, and adding it would make this endpoint a + // reader of all of them. + if (!logReadable && !record) return null; + const match = queueIndexByPrefix().get(parsed.taskId8); return { id: runId(parsed.agent, parsed.taskId8), agent: parsed.agent, task_id8: parsed.taskId8, - task_id: match?.taskId ?? null, + task_id: match?.taskId ?? record?.task_id ?? null, status: match?.status ?? 'unknown', text, commands: extractFencedBlocks(text), truncated, + has_record: record !== null, + launched_by: record?.launched_by ?? null, + outcome: record ? outcomeLabel(record) : null, + exit_code: record?.exit_code ?? null, + log_path: record?.log_path ?? null, + log_readable: logReadable, }; } @@ -592,15 +795,34 @@ const server = http.createServer(async (req, res) => { const startMatch = pathname.match(/^\/tasks\/([a-zA-Z0-9_-]+)\/start$/); if (startMatch && req.method === 'POST') { const body = await readBody(req); - const { mode } = JSON.parse(body) as { mode: StartMode }; + // Validated, not type-asserted. `as {mode: StartMode}` is a compile-time claim and + // no runtime check, and this value now reaches a task's persisted history note via + // recordStartInQueue. An unparseable body is refused here too rather than falling + // through to the `review` default — the mutation routes below already do this, and + // a Start is the one route where guessing has a visible consequence. + let mode: StartMode | null; + try { + mode = toStartMode((body ? JSON.parse(body) : {})?.mode); + } catch { + mode = null; + } + if (mode === null) { + res.statusCode = 400; + res.end(JSON.stringify({ + ok: false, + error: `invalid start mode — expected one of ${START_MODES.join(', ')}`, + })); + return; + } const taskData = getTask(startMatch[1]); if (!taskData) { res.statusCode = 404; res.end(JSON.stringify({ error: 'task not found' })); return; } const result = launchSession( startMatch[1], taskData.target_agent ?? '', - mode ?? 'review', + mode, taskData.workflow_mode, ); + if (result.ok) await recordStartInQueue(startMatch[1], taskData, mode); res.statusCode = result.ok ? 200 : 400; res.end(JSON.stringify(result)); return; diff --git a/src/tests/run-record.test.ts b/src/tests/run-record.test.ts new file mode 100644 index 0000000..b0ddccf --- /dev/null +++ b/src/tests/run-record.test.ts @@ -0,0 +1,388 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { launchLogName, runRecordFileName } from '../launch-policy.ts'; +import { parseLaunchLogName, parseRunRecordName } from '../launch-log.ts'; +import { parseRunRecord, recordSpan, outcomeLabel, type RunRecord } from '../run-record.ts'; + +const TASK = 'f42d3aeb-1c4e-4a77-9b2f-0d5e6a7b8c9d'; + +function record(over: Partial = {}): RunRecord { + return { + run_id: 'r1', task_id: TASK, agent: 'developer', launched_by: 'dispatcher', + run_as_user: null, launcher: null, workflow_mode: 'auto', + started: '2026-08-29T10:00:00.000Z', pid: 4242, + ended: null, exit_code: null, reaped: null, log_path: '/l.log', + ...over, + }; +} + +// ── The filename contract ───────────────────────────────────────────── +// The record's stem must equal the log's stem. If it ever does not, the union in +// listHeadlessRuns() stops joining them and one run renders as two rows — with the +// metadata on one and the output on the other. + +test('the record filename is the log filename with a .json suffix', () => { + for (const agent of ['sysadmin', 'developer', 'research', 'writer', 'security', 'steward']) { + const log = launchLogName(agent, TASK); + const rec = runRecordFileName(agent, TASK); + assert.equal(rec, log.replace(/\.log$/, '.json'), `stems differ for ${agent}`); + assert.deepEqual(parseRunRecordName(rec), parseLaunchLogName(log)); + } +}); + +test('parseRunRecordName does not accept a log, and vice versa', () => { + // The union keys on a shared stem read from ONE directory listing, so a parser that + // accepted both suffixes would let a `.log` be looked up as a record. + assert.equal(parseRunRecordName(launchLogName('developer', TASK)), null); + assert.equal(parseLaunchLogName(runRecordFileName('developer', TASK)), null); +}); + +test('an orphan named with a bare task UUID matches neither', () => { + // Two such files exist in the live directory. A row with an empty agent is worse than + // no row. + assert.equal(parseRunRecordName('94e1e015-b8dd-4841-a2ae-03be0742b11f.json'), null); + assert.equal(parseLaunchLogName('94e1e015-b8dd-4841-a2ae-03be0742b11f.log'), null); +}); + +// ── Parsing what the other repo writes ──────────────────────────────── + +test('a record written by the dispatcher parses', () => { + const raw = { + run_id: 'abc', task_id: TASK, agent: 'steward', launched_by: 'dispatcher', + run_as_user: 'agent-steward', launcher: '/usr/local/sbin/forge/run-steward.sh', + workflow_mode: 'auto', started: '2026-08-29T10:00:00+00:00', pid: 991, + pid_start_ticks: 12345, ended: null, exit_code: null, + log_path: '/home/ted/.claude/comms/artifacts/task-launches/steward-f42d3aeb.log', + }; + const parsed = parseRunRecord(raw); + assert.equal(parsed?.agent, 'steward'); + assert.equal(parsed?.run_as_user, 'agent-steward'); + assert.equal(parsed?.pid, 991); + assert.equal(parsed?.exit_code, null); +}); + +test('an unknown field is ignored rather than rejected', () => { + // The two writers deploy separately, so a record from a newer dispatcher is a routine + // state. Refusing it would blank the panel on exactly the newest runs. + const parsed = parseRunRecord({ task_id: TASK, agent: 'developer', a_field_from_2027: 1 }); + assert.equal(parsed?.agent, 'developer'); +}); + +test('a record with no task_id is rejected', () => { + // Every consumer joins on it; a record without one cannot be attached to anything. + assert.equal(parseRunRecord({ agent: 'developer', run_id: 'x' }), null); +}); + +test('a non-object is not a record', () => { + for (const raw of [null, undefined, 42, 'x', [1, 2], [{ task_id: TASK }]]) { + assert.equal(parseRunRecord(raw), null, `accepted ${JSON.stringify(raw)}`); + } + // NOTE ON THE TWO ARRAY CASES: they are rejected by the task_id check as much as by + // the Array.isArray guard, because JSON cannot give an array a `task_id` property. + // Deleting that guard would not fail this test. It is kept as an explicit statement of + // intent, not as load-bearing validation — do not read this assertion as proving it. +}); + +test('a pid of 0 or a non-integer is recorded as no pid', () => { + // "no pid recorded" and "pid 0" must not be confusable — one is missing data, the + // other would name the kernel scheduler. + for (const pid of [0, -1, 1.5, '4242', null, true]) { + assert.equal(parseRunRecord({ task_id: TASK, pid })?.pid, null, `accepted pid ${pid}`); + } + assert.equal(parseRunRecord({ task_id: TASK, pid: 4242 })?.pid, 4242); +}); + +test('exit_code 0 survives parsing as 0, not as null', () => { + // A `?? null` or a truthiness test here turns every clean exit into an unknown one. + assert.equal(parseRunRecord({ task_id: TASK, exit_code: 0 })?.exit_code, 0); +}); + +// ── Spans ───────────────────────────────────────────────────────────── + +test('a closed run has a duration', () => { + const span = recordSpan(record({ ended: '2026-08-29T10:02:30.000Z' })); + assert.equal(span?.duration_s, 150); +}); + +test('an open run has no duration, and it is not zero', () => { + // Nor is it "now minus started", which would tick upward on every poll for a session + // that in fact died an hour ago and has not been reaped yet. + const span = recordSpan(record()); + assert.equal(span?.duration_s, null); + assert.equal(span?.ended, ''); +}); + +test('an end before its start is an unknown duration, not a negative one', () => { + const span = recordSpan(record({ started: '2026-08-29T10:05:00.000Z', ended: '2026-08-29T10:00:00.000Z' })); + assert.equal(span?.duration_s, null); +}); + +test('an unparseable start time falls back to the caller', () => { + // null is the signal to use the mtime derivation the 29 pre-record logs depend on. + assert.equal(recordSpan(record({ started: 'not a date' })), null); +}); + +// ── Outcomes: the assertion this build exists for ───────────────────── + +test('an unrecoverable exit code says so and is never rendered as success', () => { + const label = outcomeLabel(record({ ended: '2026-08-29T10:01:00.000Z', reaped: 'pid-gone' })); + assert.equal(label, 'ended, exit code unknown'); + assert.notEqual(label, 'exit 0'); +}); + +test('a real exit code is reported as itself', () => { + assert.equal(outcomeLabel(record({ ended: '2026-08-29T10:01:00.000Z', exit_code: 0 })), 'exit 0'); + assert.equal(outcomeLabel(record({ ended: '2026-08-29T10:01:00.000Z', exit_code: 137 })), 'exit 137'); +}); + +test('a run whose slot was released while it kept running says both things', () => { + // max-runtime frees the concurrency slot and deliberately does not end the session or + // touch the task. Reporting it as finished would be the fabricated failure. + const label = outcomeLabel(record({ ended: '2026-08-29T16:00:00.000Z', reaped: 'max-runtime' })); + assert.equal(label, 'slot released — still running'); +}); + +test('an open run has no outcome at all', () => { + assert.equal(outcomeLabel(record()), null); +}); + +// ── Assembling a list row ───────────────────────────────────────────── + +import { toHeadlessRunView, compareRuns, type RunSources } from '../run-record.ts'; + +function sources(over: Partial = {}): RunSources { + return { + agent: 'developer', taskId8: 'f42d3aeb', + record: null, + fileTimes: { started: '2026-08-01T00:00:00.000Z', ended: '2026-08-01T00:01:00.000Z', duration_s: 60 }, + logSize: 100, firstLine: 'hello', + ...over, + }; +} + +test('a log with no record still renders — the 29 that predate run records', () => { + const view = toHeadlessRunView(sources()); + assert.equal(view.has_record, false); + assert.equal(view.duration_s, 60, 'must fall back to the mtime derivation'); + assert.equal(view.outcome, null); + assert.equal(view.exit_code, null); + assert.equal(view.log_readable, true); +}); + +test('a record with no readable log still renders — the audit launcher', () => { + // Its log goes to ~/.pm2/logs, which stays outside the preview allowlist because that + // prefix covers every PM2 service log on the host. Dropping the row instead would omit + // the commonest kind of headless session here. + const view = toHeadlessRunView(sources({ + agent: 'security', + record: record({ agent: 'security', launched_by: 'dispatcher-audit', ended: '2026-08-29T10:01:00.000Z' }), + fileTimes: null, logSize: 0, firstLine: '', + })); + assert.equal(view.log_readable, false); + assert.equal(view.has_record, true); + assert.equal(view.launched_by, 'dispatcher-audit'); + assert.equal(view.outcome, 'ended, exit code unknown'); +}); + +test('the record wins on times over the log file', () => { + // The 26 logs copy-migrated on 2026-08-27 have a birthtime months after their mtime. + // A record knows when the process started and needs no such defence. + const view = toHeadlessRunView(sources({ + record: record({ started: '2026-08-29T10:00:00.000Z', ended: '2026-08-29T10:00:30.000Z' }), + })); + assert.equal(view.duration_s, 30); + assert.equal(view.started, '2026-08-29T10:00:00.000Z'); +}); + +test('an unusable record start time falls back to the file, not to nothing', () => { + const view = toHeadlessRunView(sources({ record: record({ started: 'garbage' }) })); + assert.equal(view.duration_s, 60); + assert.equal(view.has_record, true, 'the record is still there — only its times were unusable'); +}); + +test('status comes from the queue and never from the record', () => { + // steward-f42d3aeb is a completed run whose task sat at `approved` for four days. + // Rendering that disagreement is the whole point of the panel. + const view = toHeadlessRunView(sources({ + record: record({ ended: '2026-08-29T10:01:00.000Z', exit_code: 0 }), + queue: { status: 'approved', taskId: TASK }, + })); + assert.equal(view.status, 'approved'); + assert.equal(view.outcome, 'exit 0'); +}); + +test('with no queue match the task id still comes from the record', () => { + // An archived task has no live queue entry; its run stays identifiable regardless. + const view = toHeadlessRunView(sources({ record: record() })); + assert.equal(view.task_id, TASK); + assert.equal(view.status, 'unknown'); +}); + +test('a finished run with no queue match is `unknown`, never `completed`', () => { + // The tempting inference — the run ended, so the task must be done — is the exact + // thing this panel exists to refuse. A run ending says nothing about whether its + // agent got as far as closing the task, and the four-day-invisible steward run is + // what happens when the two are conflated. + const view = toHeadlessRunView(sources({ + record: record({ ended: '2026-08-29T10:01:00.000Z', exit_code: 0 }), + })); + assert.equal(view.status, 'unknown'); +}); + +test('with neither a queue match nor a record the task id is null', () => { + assert.equal(toHeadlessRunView(sources()).task_id, null); +}); + +test('a zero exit code survives into the view as 0', () => { + const view = toHeadlessRunView(sources({ + record: record({ ended: '2026-08-29T10:01:00.000Z', exit_code: 0 }), + })); + assert.equal(view.exit_code, 0); +}); + +// ── Order ───────────────────────────────────────────────────────────── + +test('open runs sort above finished ones', () => { + // A descending compare on `ended` puts them at the bottom, under three months of + // finished runs — which is where nobody looks for the session running right now. + const open = toHeadlessRunView(sources({ record: record({ started: '2020-01-01T00:00:00.000Z' }), fileTimes: null })); + const recent = toHeadlessRunView(sources({ + record: record({ started: '2026-08-29T09:00:00.000Z', ended: '2026-08-29T09:30:00.000Z' }), + })); + assert.deepEqual([recent, open].sort(compareRuns).map(r => r.ended === ''), [true, false]); +}); + +test('finished runs sort most-recent first', () => { + const older = toHeadlessRunView(sources({ record: record({ ended: '2026-08-01T00:00:00.000Z' }) })); + const newer = toHeadlessRunView(sources({ record: record({ ended: '2026-08-29T00:00:00.000Z' }) })); + assert.deepEqual([older, newer].sort(compareRuns).map(r => r.ended), [newer.ended, older.ended]); +}); + +test('two open runs sort most-recently-started first', () => { + const older = toHeadlessRunView(sources({ record: record({ started: '2026-08-01T00:00:00.000Z' }), fileTimes: null })); + const newer = toHeadlessRunView(sources({ record: record({ started: '2026-08-29T00:00:00.000Z' }), fileTimes: null })); + assert.deepEqual([older, newer].sort(compareRuns).map(r => r.started), [newer.started, older.started]); +}); + +// ── The guarded reader ──────────────────────────────────────────────── +// +// Added at the audit's request (agent-workflow-interop-2026-08-phase34, INFO 2). The +// path guard on closeRunRecord() was fixed during the pre-audit and had no regression +// test of its own — server.ts calls listen() at import time, so nothing in it can be +// unit-tested. That is the same property that let the guard be omitted in the first +// place, so the read was extracted here rather than the gap being accepted. + +import os from 'node:os'; +import { loadRunRecord } from '../run-record.ts'; + +function tmpdir(): string { + return fs.mkdtempSync(path.join(os.tmpdir(), 'run-record-test-')); +} + +function writeRecord(dir: string, agent: string, taskId: string, over: Partial = {}): string { + const file = path.join(dir, runRecordFileName(agent, taskId)); + fs.writeFileSync(file, JSON.stringify(record({ agent, task_id: taskId, ...over }))); + return file; +} + +test('loadRunRecord reads a record inside an allowed prefix', () => { + const dir = fs.realpathSync(tmpdir()); + writeRecord(dir, 'developer', TASK); + const loaded = loadRunRecord(dir, 'developer', TASK, [dir]); + assert.equal(loaded?.record.agent, 'developer'); + assert.equal(loaded?.record.task_id, TASK); + assert.equal(loaded?.path, path.join(dir, 'developer-f42d3aeb.json')); +}); + +test('loadRunRecord refuses a record outside every allowed prefix', () => { + const dir = fs.realpathSync(tmpdir()); + writeRecord(dir, 'developer', TASK); + assert.equal(loadRunRecord(dir, 'developer', TASK, [fs.realpathSync(tmpdir())]), null); +}); + +test('loadRunRecord refuses a SYMLINK that escapes the allowed prefix', () => { + // THE regression test the audit asked for. A symlink sitting inside an allowed + // directory and pointing outside it passes a path.resolve-only check; only realpath + // catches it. The list route's guard exists because a real ~/.secrets/forge.env + // symlink was planted in the live launch directory and read. + const dir = fs.realpathSync(tmpdir()); + const outside = fs.realpathSync(tmpdir()); + const secret = path.join(outside, 'secret.json'); + fs.writeFileSync(secret, JSON.stringify({ task_id: TASK, agent: 'not-yours' })); + fs.symlinkSync(secret, path.join(dir, runRecordFileName('developer', TASK))); + + // Readable as a plain file — so this is not passing for want of a target. + assert.equal(JSON.parse(fs.readFileSync(path.join(dir, 'developer-f42d3aeb.json'), 'utf-8')).agent, 'not-yours'); + assert.equal(loadRunRecord(dir, 'developer', TASK, [dir]), null); +}); + +test('loadRunRecord accepts a symlink that stays inside the allowed prefix', () => { + // The guard is about the RESOLVED location, not about symlinks being forbidden. + const dir = fs.realpathSync(tmpdir()); + const real = path.join(dir, 'real.json'); + fs.writeFileSync(real, JSON.stringify(record())); + fs.symlinkSync(real, path.join(dir, runRecordFileName('developer', TASK))); + assert.equal(loadRunRecord(dir, 'developer', TASK, [dir])?.path, real); +}); + +test('loadRunRecord returns null for an absent record', () => { + const dir = fs.realpathSync(tmpdir()); + assert.equal(loadRunRecord(dir, 'developer', TASK, [dir]), null); +}); + +test('loadRunRecord returns null for a corrupt record rather than throwing', () => { + const dir = fs.realpathSync(tmpdir()); + fs.writeFileSync(path.join(dir, runRecordFileName('developer', TASK)), '{not json'); + assert.equal(loadRunRecord(dir, 'developer', TASK, [dir]), null); +}); + +test('loadRunRecord returns the guard-approved path, for callers that write back', () => { + // closeRunRecord() stamps the exit code onto this path. Re-deriving one would write to + // a location the guard never approved — through the symlink, in the escaping case. + const dir = fs.realpathSync(tmpdir()); + const real = path.join(dir, 'real.json'); + fs.writeFileSync(real, JSON.stringify(record())); + fs.symlinkSync(real, path.join(dir, runRecordFileName('developer', TASK))); + const loaded = loadRunRecord(dir, 'developer', TASK, [dir]); + assert.equal(loaded?.path, real, 'must be the resolved target, not the link name'); +}); + +test('loadRunRecord takes a full task id or an 8-char prefix', () => { + // The list route holds only the prefix parsed from a filename; closeRunRecord holds + // the whole id. Both must land on the same file. + const dir = fs.realpathSync(tmpdir()); + writeRecord(dir, 'developer', TASK); + assert.equal(loadRunRecord(dir, 'developer', TASK, [dir])?.record.task_id, TASK); + assert.equal(loadRunRecord(dir, 'developer', 'f42d3aeb', [dir])?.record.task_id, TASK); +}); + +// ── Start mode validation ───────────────────────────────────────────── + +import { toStartMode, START_MODES } from '../launch-policy.ts'; + +test('an absent mode keeps the review default', () => { + // The safe leg: read the task and wait. Long-standing behaviour, not a new guess. + assert.equal(toStartMode(undefined), 'review'); + assert.equal(toStartMode(null), 'review'); +}); + +test('both real modes survive', () => { + for (const m of START_MODES) assert.equal(toStartMode(m), m); +}); + +test('a present but unrecognised mode is null, NOT the default', () => { + // Defaulting here would silently downgrade an operator who asked for `auto`, turning a + // typo into a session that quietly does nothing. The route refuses instead. + for (const raw of ['auto ', 'AUTO', 'plan', '', 'semi-auto', 42, true, {}, ['auto']]) { + assert.equal(toStartMode(raw), null, `accepted ${JSON.stringify(raw)}`); + } +}); + +test('an arbitrary string cannot ride into a persisted history note', () => { + // The reason this validator exists: `mode` reaches recordStartInQueue's note, which + // the control API persists into the task's history. + assert.equal(toStartMode('review"; DROP'), null); + assert.equal(toStartMode(''), null); +}); diff --git a/src/types.ts b/src/types.ts index dc3327c..59c14d1 100644 --- a/src/types.ts +++ b/src/types.ts @@ -156,6 +156,28 @@ export interface HeadlessRun { duration_s: number | null; size: number; first_line: string; + /** + * True when a run record was found beside the log. False for the 29 logs that predate + * run records, whose times are still derived from file mtimes — the fields below are + * all null for those, and the UI says "no run record" rather than implying an outcome. + */ + has_record: boolean; + /** 'dispatcher' | 'dispatcher-audit' | 'plugin', or null without a record. */ + launched_by: string | null; + /** + * How the run ended, in one phrase, or null while it is open or unrecorded. NEVER + * derived from the log's prose — see outcomeLabel(). + */ + outcome: string | null; + /** null is an honest unknown for a dispatcher launch, not a missing value. */ + exit_code: number | null; + /** + * False when the record names a log this UI may not read — the security-audit launcher + * writes to ~/.pm2/logs, which is deliberately outside the preview allowlist because + * that prefix covers every PM2 service log on the host. The row still renders; the + * detail view says where the log is instead of pretending it is empty. + */ + log_readable: boolean; } export interface HeadlessRunDetail { @@ -168,4 +190,11 @@ export interface HeadlessRunDetail { /** Fenced code blocks scraped from the log, offered with copy buttons. */ commands: string[]; truncated: boolean; + has_record: boolean; + launched_by: string | null; + outcome: string | null; + exit_code: number | null; + /** Absolute path of the log, from the record. Shown when it is not readable here. */ + log_path: string | null; + log_readable: boolean; }