diff --git a/src/servers/api/command-hq/directory.ts b/src/servers/api/command-hq/directory.ts index 7d06a306..0f668a5b 100644 --- a/src/servers/api/command-hq/directory.ts +++ b/src/servers/api/command-hq/directory.ts @@ -62,6 +62,12 @@ export function credentialSection(credentialPath: string | null): string[] { 'looks correct. It is still wrong: it skips every permission, every setting the owner chose and every', 'log, and it breaks silently the day that service moves.', '', + '**The database is one of those things, and it is the one that gets missed.** `psql $POSTGRES_URL`', + 'may work from your shell. Never use it. Every table has routes in front of it that check who is', + 'asking and keep the caches correct — `PUT /api/chat/sessions//nudge` rather than an INSERT into', + '`nudges`, and so on for everything else. A write that goes round the API is a write nothing else on', + 'the machine knows happened.', + '', 'Prefer a TASK over a raw call where one exists. `GET /api/tasks` lists them and', '`POST /api/tasks//run` runs one and returns what it did — one reviewed definition on disk', 'instead of a re-implementation living in a loop nobody reads.', @@ -156,8 +162,12 @@ export function commanderClaudeMd(params: CommanderClaudeMdParams): string { '', 'Two paths only this commander can be told, and it must write to them or it cannot be stopped:', '', - `- its loop's own pid: \`${params.loopPidPath}\``, - `- one session id per line, for every agent its loop spawns: \`${params.spawnedPath}\``, + `- its loop's own pid: \`${params.loopPidPath}\` — OVERWRITE it (\`>\`), one line, no more.`, + `- one session id per line, for every agent its loop spawns: \`${params.spawnedPath}\` — APPEND (\`>>\`).`, + '', + 'The difference matters. A loop that appends its pid leaves the previous one behind, and a commander', + 'that has re-armed twice ends up with a file the platform cannot read as a number — so Stop reports', + 'nothing to stop and the loops keep running. One pid, replaced each time it arms.', '', 'The platform cannot find this process and cannot recover a spawned agent id after it exits, so this', 'record is the only one. Without it, deleting this commander leaves its loop running.', @@ -219,6 +229,10 @@ export function commanderClaudeMd(params: CommanderClaudeMdParams): string { '**Leave the ambiguous alone and name it.** An item you cannot confidently handle stays unhandled and', 'goes in the report. Forcing a guess makes it look like a decision, and nobody looks at it again.', '', + '**Refuse to arm without what you need.** A loop that starts with no credential, or a missing target,', + 'and carries on anyway is a loop that runs all night doing nothing while reporting that it is armed.', + 'Check at the top, exit non-zero, say which thing was missing.', + '', '## Where things go', '', 'Create only what this commander actually uses. A directory that exists and stays empty reads as', diff --git a/src/servers/api/command-hq/spawned.test.ts b/src/servers/api/command-hq/spawned.test.ts new file mode 100644 index 00000000..e2af5314 --- /dev/null +++ b/src/servers/api/command-hq/spawned.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, test } from 'bun:test'; +import { parseLivePids } from './spawned'; + +/* + A commander records its loop's pid so the platform can stop it. This used to be `Number(text.trim())`, + which is NaN the moment the file has two lines — and the first real commander wrote its pid with `>>`, + so after one re-arm it had two. + + NaN read as "no loop": Stop reported zero processes signalled and left both loops running. Silent, and + exactly backwards — the more times a commander was re-armed, the less stoppable it became. +*/ + +const alive = (live: number[]) => (pid: number) => live.includes(pid); + +describe('reading a commander loop pid file', () => { + test('one pid, the ordinary case', () => { + expect(parseLivePids('4242\n', alive([4242]))).toEqual([4242]); + }); + + // The bug, pinned. Two lines used to produce NaN and therefore nothing. + test('an appended file yields every live pid, not NaN', () => { + expect(parseLivePids('3675353\n3675487\n', alive([3675353, 3675487]))).toEqual([3675353, 3675487]); + }); + + test('a pid whose process has gone is dropped', () => { + expect(parseLivePids('111\n222\n', alive([222]))).toEqual([222]); + }); + + // 0 and 1 are never a loop, and a negative would be a process GROUP — signalling one of those from a + // typo'd file would reach far more than this commander. + test('junk, blanks and unsignalable ids are ignored', () => { + expect(parseLivePids('\n\nnot-a-pid\n0\n1\n-9\n77\n', alive([77, 0, 1, -9]))).toEqual([77]); + }); + + test('an empty file is no loop rather than an error', () => { + expect(parseLivePids('', alive([]))).toEqual([]); + }); +}); diff --git a/src/servers/api/command-hq/spawned.ts b/src/servers/api/command-hq/spawned.ts index 6555a682..e39b10e5 100644 --- a/src/servers/api/command-hq/spawned.ts +++ b/src/servers/api/command-hq/spawned.ts @@ -117,30 +117,67 @@ export function loopPidPath(email: string, dirName: string): string { } /** The loop's pid, if it recorded one and the process is still alive. */ -export function liveLoopPid(email: string, dirName: string): number | null { +/** + * Every pid recorded in the file whose process is still alive, in the order they were written. + * + * PLURAL, because the file can legitimately hold more than one. A commander re-arming its loop appends, + * and this is written by an agent improvising a shell script — being strict about its formatting buys + * nothing except a commander nobody can stop. + * + * That is not hypothetical. This used to be `Number(text.trim())`, which is NaN the moment there are two + * lines, and NaN meant "no loop": the Stop button reported zero processes signalled and left both loops + * running. Silent, and exactly backwards — the more times a commander was re-armed, the less stoppable it + * became. + */ +export function liveLoopPids(email: string, dirName: string): number[] { const path = loopPidPath(email, dirName); - if (!existsSync(path)) return null; - const pid = Number(readFileSync(path, 'utf-8').trim()); - if (!Number.isInteger(pid) || pid <= 1) return null; + if (!existsSync(path)) return []; + return parseLivePids(readFileSync(path, 'utf-8')); +} + +/** + * The parsing and liveness rule, split out so it can be tested — the caller resolves a real path under + * DATA_PATH, and a test of that would be a test of this machine. + * + * `isAlive` defaults to signal 0, which tests for existence without touching the process. A recorded pid + * whose process has gone is not an error — loops end — and reusing a dead pid's number would kill + * something unrelated. + */ +export function parseLivePids(text: string, isAlive: (pid: number) => boolean = defaultIsAlive): number[] { + return text + .split('\n') + .map((line) => Number(line.trim())) + .filter((n) => Number.isInteger(n) && n > 1) + .filter(isAlive); +} + +function defaultIsAlive(pid: number): boolean { try { - // Signal 0 tests for existence without touching it. A recorded pid whose process has gone is not an - // error — loops end — and reusing a dead pid's number would kill something unrelated. process.kill(pid, 0); - return pid; + return true; } catch { - return null; + return false; } } +/** Is this commander's loop running? The newest live pid, or null. */ +export function liveLoopPid(email: string, dirName: string): number | null { + const pids = liveLoopPids(email, dirName); + return pids.length ? pids[pids.length - 1]! : null; +} + /** * Stop the loop and everything under it. Returns how many processes were signalled. * * Children first, then the loop, so the loop cannot spawn a replacement between the two. */ export function stopCommanderProcesses(email: string, dirName: string): number { - const pid = liveLoopPid(email, dirName); - if (!pid) return 0; - const tree = [...descendantPids(pid), pid]; + // EVERY live loop, not the newest. A commander that re-armed without stopping the previous loop has two + // running, and stopping one of them leaves the folder still being watched by the other — which reads as + // "Stop did nothing", because from outside it is indistinguishable. + const pids = liveLoopPids(email, dirName); + if (!pids.length) return 0; + const tree = [...new Set(pids.flatMap((pid) => [...descendantPids(pid), pid]))]; let signalled = 0; for (const target of tree) { try {