Stop could not stop a commander that had re-armed

liveLoopPid read the pid file as Number(text.trim()), which is NaN as soon
as there are two lines. the voice-memos commander wrote its pid with `>>`,
so after one re-arm the file had two — and NaN read as "no loop". the Stop
button reported zero processes signalled and left both loops running,
silently. exactly backwards: the more a commander was re-armed, the less
stoppable it became. verified on alpha, where both pids are alive right
now.

so the reader is lenient — every live pid in the file, in order — and
stopCommanderProcesses signals all of them rather than the newest. killing
one of two leaves the folder still watched, which from outside is
indistinguishable from Stop doing nothing.

parseLivePids is split out and tested. the caller resolves a path under
DATA_PATH, so a test of the whole thing would have been a test of this
machine.

three lessons from what the first commander actually wrote, into the
template:

- the DATABASE is a thing behind the platform, and it is the one that gets
  missed. that loop writes nudges with `psql $POSTGRES_URL` when
  PUT /api/chat/sessions/:id/nudge exists. worth knowing: a MEMBER's agent
  could not do this — spawn-as-member.ts builds an env allowlist and throws
  on anything unvetted. the owner's runs with the platform's environment.
- refuse to arm without a credential or a target. that watcher already does
  it, and it is the difference between failing and running all night doing
  nothing while reporting armed.
- overwrite the pid file, append the spawned list. which is the bug above,
  stated where the next commander will read it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Officer-Session: 656f5c85-e49a-4f1f-b9f3-042a04b4d043

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/<id>/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/<dirName>/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',

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([]);
+  });
+});

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 {