From fefda577c1425af4fe13bf33d6ee85b63ba53289 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Sun, 6 Sep 2026 00:21:20 +0100 Subject: [PATCH] fix: the audit's remaining findings, and doctor learns whether jobs detach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings from the six-persona sweep and the acceptance passes, plus a commitment I made and dropped. doctor now reports whether dist/job-runner.js is present. resolveRunnerPath() returning undefined is not an error — it is the signal to run jobs IN-PROCESS, which is right for an unbuilt checkout and wrong everywhere else, because the concurrency cap is enforced by the supervisor pool. So the setting that exists because of a measured OOM silently stops applying, with one stderr line at dispatch time and nothing checking it. I said I would add this after the first auditor raised it and did not; an audit noticing twice is what it took. Dead workspace locks are swept. A lock whose holder is gone IS reclaimed — but only when something contends for that exact directory, and nothing contends for a path you dispatched against once. A resource audit measured a seven-day-old lock still sitting there. The sweep reuses isDead(), so a file is removed under exactly the rule that would let a waiter steal it; a looser second rule here could delete a lock the acquire path still considers live. `usage` with no routes printed a bare header. That reads as a broken command, which is the one thing it does not mean. It now says there are no routes and names both ways to get one. `--json` is a promise about the shape of the output and was kept only on the success path: a bad --config made `doctor --json` print a sentence, so anything parsing it got a parse error instead of the reason. Same message, correct envelope; without --json the plain line is unchanged, and a test pins both directions. README: the configure paragraph was one 180-word block covering four separate concerns, which a new-user audit could not follow. Split under labelled headings, with the job-runner check named and `--live` marked as spending quota. Carried and NOT fixed, with reasons: stream-subprocess has no backpressure and kills a healthy child on queue overflow — a real design change to the path every dispatch takes, and the module already documents the behaviour honestly; MCP JSON-RPC error frames bypass the redaction sinks on the HTTP transport, structural and never demonstrated carrying a credential; two weak test assertions that are redundant rather than wrong. npm run check: 1258 passed, 8 skipped, exit 0. --- README.md | 44 ++++++++++++++++++++++-------------- src/bin.ts | 34 ++++++++++++++++++++++++++-- src/jobs.ts | 2 +- src/jobs/start.ts | 7 ++++++ src/status.ts | 15 ++++++++++++ src/workspace-lock.ts | 42 ++++++++++++++++++++++++++++++++++ tests/bin-entrypoint.test.ts | 34 ++++++++++++++++++++++++++++ tests/workspace-lock.test.ts | 42 ++++++++++++++++++++++++++++++++++ 8 files changed, 200 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 913c24f..cdb482a 100644 --- a/README.md +++ b/README.md @@ -77,23 +77,33 @@ pin routes, add an endpoint, or change a default. `configure --yes` detects installed harnesses, writes `config.yaml` into the tool's own state directory (`~/.harness-dispatch/`, or `HARNESS_DISPATCH_STATE_DIR`) — unless a `config.yaml` already exists in the current directory or -`HARNESS_DISPATCH_CONFIG` is set, in which case that file is the target — and then -offers to register this server with each MCP client it finds (Claude Code, -Cursor) — showing you what it would write, and what is already there, before -changing anything. `--no-clients` skips the offer and prints a snippet to paste -instead; `harness-dispatch connect` does the same registration later on its own, -and `connect --remove` undoes it. Without -`--yes` configure previews and writes nothing. Re-running it regenerates a file it -wrote and you have not edited, so installing a harness later is just `configure --yes` -again; a file you have changed is refused without `--force`, and because such a file -lists its own routes, even `--force` regenerates it from the file rather than from a -fresh detection (it says so; add `detect: true` to the file to merge new harnesses). `doctor` then checks the whole chain: -binary, config load, harness detection, auth and billing classification, route -readiness, and for a Codex route asks `codex login status` whether the CLI is -logged in (the other harnesses have no equivalent this tool has verified, so -their login state is not checked). `--live` goes further and routes one tiny real prompt, so you see a -completion before wiring anything into your agent. The live probe never touches paid or -unknown-billing routes unless you pass `--allow-paid`. +`HARNESS_DISPATCH_CONFIG` is set, in which case that file is the target. + +Without `--yes` it previews and writes nothing. + +**Registering with your MCP clients.** After writing, `configure` offers to +register this server with each client it finds (Claude Code, Cursor), showing +what it would write and what is already there before changing anything. +`--no-clients` skips the offer and prints a snippet to paste instead. +`harness-dispatch connect` does the same registration later on its own, and +`connect --remove` undoes it. + +**Re-running it.** A file `configure` wrote and you have not edited is +regenerated, so installing a harness later is just `configure --yes` again. A +file you have changed is refused without `--force` — and because such a file +lists its own routes, even `--force` regenerates it from the file rather than +from a fresh detection. It says so when that happens; add `detect: true` to the +file to merge newly installed harnesses in. + +**What `doctor` checks.** The whole chain: binary, config load, harness +detection, auth and billing classification, route readiness, whether +`dist/job-runner.js` is present (without it jobs run in-process and the +concurrency cap does not apply), and for a Codex route it asks `codex login +status` whether the CLI is logged in. The other harnesses have no equivalent +this tool has verified, so their login state is not checked. `--live` goes +further and routes one tiny real prompt through an eligible route, so you see a +completion before wiring anything into your agent — that one spends quota, and +it never touches paid or unknown-billing routes unless you pass `--allow-paid`. Your Claude Code / Codex / Cursor subscriptions run by default with no opt-in; `configure` tells you if anything is blocked and why. diff --git a/src/bin.ts b/src/bin.ts index 3ab5430..7ff0e78 100644 --- a/src/bin.ts +++ b/src/bin.ts @@ -20,6 +20,7 @@ import { codexLoginState } from "./dispatchers/shared/harness-login.js"; import { clientConfigLocations, inspectClientEntries } from "./mcp-clients.js"; import { buildDispatchers } from "./mcp/dispatcher-factory.js"; import { startMcpServer } from "./mcp/server.js"; +import { resolveRunnerPath } from "./jobs.js"; import { initObservability } from "./observability/index.js"; import { QuotaCache } from "./quota.js"; import { Router } from "./router.js"; @@ -838,6 +839,25 @@ async function cmdDoctor( // asked the same question twice to fill two fields. ...stateDirWritable(), }, + { + // Whether dispatches will actually be detached. + // + // `resolveRunnerPath()` returning undefined is not an error — it is the + // signal to run the job IN-PROCESS, which is right for an unbuilt + // checkout and wrong everywhere else: the concurrency cap is enforced by + // the supervisor pool, so in-process mode silently removes the bound + // that exists because of a measured OOM. It prints one line on stderr at + // dispatch time and nothing checked it, so "am I actually capped?" had + // no answer. An audit noticed; this is that answer. + name: "job-runner", + ok: resolveRunnerPath() !== undefined, + detail: + resolveRunnerPath() !== undefined + ? "found; jobs run detached and the concurrency cap applies" + : "dist/job-runner.js not found — jobs will run IN-PROCESS, which " + + "removes the max_concurrent_runs cap and does not survive a server " + + "restart. Run `npm run build`, or reinstall the package.", + }, { name: "http-auth", ok: true, @@ -1391,8 +1411,18 @@ if (isThisFile(entrypoint)) { // reliable way to tell "bug" from "bad input" by class here. Only a // non-Error throw (a genuine programming error) keeps its stack. if (err instanceof UsageError || err instanceof Error) { - process.stderr.write(`harness-dispatch: ${err.message} -`); + // `--json` is a promise about the SHAPE of this command's output, and + // it was kept only on the success path: a bad --config made + // `doctor --json` print a sentence, so anything parsing the output got + // a parse error instead of the reason. The message is the same; only + // the envelope follows what was asked for. Errors still go to stderr, + // so a caller reading stdout for results is unaffected either way. + const wantsJson = process.argv.slice(2).includes("--json"); + process.stderr.write( + wantsJson + ? `${JSON.stringify({ ok: false, error: err.message }, null, 2)}\n` + : `harness-dispatch: ${err.message}\n`, + ); process.exit(1); } throw err; diff --git a/src/jobs.ts b/src/jobs.ts index c03ff23..409f9ae 100644 --- a/src/jobs.ts +++ b/src/jobs.ts @@ -7,7 +7,7 @@ * consumer imports from here, so the split is invisible outside src/jobs/. */ -export { executeJobDir, runJob } from "./jobs/run.js"; +export { executeJobDir, resolveRunnerPath, runJob } from "./jobs/run.js"; export { activeCapacity, claimJobDir, diff --git a/src/jobs/start.ts b/src/jobs/start.ts index 92d0d24..41286e9 100644 --- a/src/jobs/start.ts +++ b/src/jobs/start.ts @@ -5,6 +5,7 @@ */ import { mkdir, writeFile } from "node:fs/promises"; +import { pruneDeadWorkspaceLocks } from "../workspace-lock.js"; import path from "node:path"; import { resolveWorkingDir, validateWorkingDir, workingDirWarning } from "../working-dir.js"; import { buildContextPreamble } from "./context.js"; @@ -54,6 +55,12 @@ export async function startAsyncJobTracked(deps: JobDeps, input: StartJobInput): if (configError !== undefined) throw new Error(configError); await pruneStaleJobs(); + // Same maintenance moment, same reason: a lock whose holder is gone is only + // reclaimed when something contends for that exact directory, so a + // dispatched-once workspace leaves its file behind forever. Best effort, and + // deliberately not awaited-into-failure — a sweep must never block the job + // that was actually asked for. + await pruneDeadWorkspaceLocks().catch(() => undefined); const jobId = newJobId(); const root = jobsRoot(); const jobDir = path.join(root, jobId); diff --git a/src/status.ts b/src/status.ts index 1cfe9fc..a112a85 100644 --- a/src/status.ts +++ b/src/status.ts @@ -505,6 +505,21 @@ export function buildUsage(status: HarnessDispatchStatus): HarnessDispatchUsage export function renderUsageText(usage: HarnessDispatchUsage): string { const lines: string[] = ["harness-dispatch usage", ""]; + if (usage.routes.length === 0) { + // A bare header and nothing else reads as "this command is broken", which + // is the one thing it does not mean. Carried as an open item across three + // releases because it looked cosmetic; it is the first thing a user with + // no routes sees, and it told them nothing about why or what to do. + lines.push( + "No routes configured.", + "", + "Install a harness CLI (claude, codex, cursor-agent, agy) and it is picked", + "up automatically, or add an `endpoints:` entry to config.yaml — those need", + "no CLI. `harness-dispatch doctor` says which of the two applies here.", + "", + ); + return lines.join("\n"); + } for (const route of usage.routes) { const mark = route.available && route.enabled ? "ok" : "off"; const quota = diff --git a/src/workspace-lock.ts b/src/workspace-lock.ts index 24ed10c..e3188a4 100644 --- a/src/workspace-lock.ts +++ b/src/workspace-lock.ts @@ -30,6 +30,7 @@ import { createHash } from "node:crypto"; import { mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; +import { readdir, rm } from "node:fs/promises"; import path from "node:path"; import { stateRoot } from "./state-dir.js"; @@ -277,3 +278,44 @@ export async function acquireWorkspaceLock( if (inProcessLocks.get(key) === current) inProcessLocks.delete(key); }; } + +/** + * Delete lock files whose holder is definitely gone. + * + * A dead lock IS reclaimed correctly — but only when something contends for + * that same working directory. Nothing contends for a path you dispatched + * against once and moved on from, so its file stays forever: one per distinct + * workspace, indefinitely. Found by a resource audit, which measured a + * seven-day-old lock still sitting there. + * + * Tiny individually (a couple of hundred bytes), and the point is the count + * rather than the size — this directory is walked when locks are examined, so + * an unbounded file count is a cost paid later. + * + * Deliberately reuses `isDead`, so a lock is swept under exactly the same rule + * that lets a waiter steal it. A second, looser rule here would be a way for + * this to delete a lock the acquire path still considers live. + */ +export async function pruneDeadWorkspaceLocks(): Promise { + const dir = path.join(stateRoot(), "workspace-locks"); + let entries: string[]; + try { + entries = await readdir(dir); + } catch { + return; // No lock directory yet: nothing to sweep. + } + for (const entry of entries) { + if (!entry.endsWith(".json")) continue; + const file = path.join(dir, entry); + try { + const record = readRecord(file); + // An unreadable record is not evidence of a live holder — `readRecord` + // returns undefined for a corrupt or half-written file, and the acquire + // path already treats that as absent. + if (record !== undefined && !isDead(record)) continue; + await rm(file, { force: true }); + } catch { + // Vanished mid-sweep, or another process got there first. + } + } +} diff --git a/tests/bin-entrypoint.test.ts b/tests/bin-entrypoint.test.ts index 22ecb17..89cb3e1 100644 --- a/tests/bin-entrypoint.test.ts +++ b/tests/bin-entrypoint.test.ts @@ -52,3 +52,37 @@ describe("the built command runs when invoked through a link (as npm installs it }, ); }); + +describe("--json is a promise about the shape of the output, including on failure", () => { + /** + * `--json` was honoured only on the success path. A bad `--config` made + * `doctor --json` print a sentence, so anything parsing the output got a + * parse error rather than the reason — carried as an open acceptance item + * across three releases because it reads as cosmetic. It is not: the whole + * point of the flag is that a program, not a person, is reading. + */ + it.skipIf(!existsSync(bin))("reports a bad --config as JSON when --json is given", async () => { + const dir = mkdtempSync(path.join(tmpdir(), "hd-json-err-")); + cleanup.push(dir); + // A directory, which is a real mistake people make and cannot be a config. + const err = await run(process.execPath, [bin, "doctor", "--json", "--config", dir]).catch( + (e: { stderr?: string; stdout?: string }) => e, + ); + const text = String(err.stderr ?? ""); + const parsed = JSON.parse(text) as { ok: boolean; error: string }; + expect(parsed.ok).toBe(false); + expect(parsed.error).toContain("is a directory"); + }); + + it.skipIf(!existsSync(bin))("still reports it as a plain line without --json", async () => { + // The default must not become JSON for a person reading a terminal. + const dir = mkdtempSync(path.join(tmpdir(), "hd-txt-err-")); + cleanup.push(dir); + const err = await run(process.execPath, [bin, "doctor", "--config", dir]).catch( + (e: { stderr?: string }) => e, + ); + const text = String(err.stderr ?? ""); + expect(text).toContain("harness-dispatch: "); + expect(() => JSON.parse(text)).toThrow(); + }); +}); diff --git a/tests/workspace-lock.test.ts b/tests/workspace-lock.test.ts index 8bc8d03..56efd18 100644 --- a/tests/workspace-lock.test.ts +++ b/tests/workspace-lock.test.ts @@ -259,3 +259,45 @@ function readdirSyncSafeName(workingDir: string): string { const { createHash } = require("node:crypto") as typeof import("node:crypto"); return `${createHash("sha256").update(key).digest("hex").slice(0, 16)}.json`; } + +describe("pruneDeadWorkspaceLocks", () => { + /** + * A dead lock IS reclaimed — but only when something contends for that same + * working directory. Nothing contends for a path dispatched against once and + * left alone, so its file stayed forever: one per distinct workspace. A + * resource audit measured a seven-day-old lock still sitting there. + */ + it("removes a lock whose holder is gone and keeps a live one", async () => { + const { pruneDeadWorkspaceLocks } = await import("../src/workspace-lock.js"); + await fs.mkdir(lockDir(), { recursive: true }); + + const dead = path.join(lockDir(), "dead000000000000.json"); + await fs.writeFile( + dead, + JSON.stringify({ pid: 0, key: "/gone", beatMs: Date.now() - 10 * 60_000 }), + "utf8", + ); + // This process is alive and its beat is now, so it must survive. + const live = path.join(lockDir(), "live000000000000.json"); + await fs.writeFile( + live, + JSON.stringify({ pid: process.pid, key: "/here", beatMs: Date.now() }), + "utf8", + ); + // Unreadable: the acquire path already treats this as absent, so the + // sweep must not be more cautious than the rule that steals it. + const corrupt = path.join(lockDir(), "corrupt00000000.json"); + await fs.writeFile(corrupt, "{not json", "utf8"); + + await pruneDeadWorkspaceLocks(); + + expect(existsSync(dead), "a dead lock was kept").toBe(false); + expect(existsSync(corrupt), "an unreadable lock was kept").toBe(false); + expect(existsSync(live), "a LIVE lock was deleted").toBe(true); + }); + + it("does nothing when there is no lock directory yet", async () => { + const { pruneDeadWorkspaceLocks } = await import("../src/workspace-lock.js"); + await expect(pruneDeadWorkspaceLocks()).resolves.toBeUndefined(); + }); +});