From 9751f018be824d1d86d6667b2c9498c1cafe946f Mon Sep 17 00:00:00 2001 From: ThinkOff Date: Fri, 18 Sep 2026 14:16:04 +0300 Subject: [PATCH 01/14] feat(confirmations): team-lead approvals, with the lead unset by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Petrus, 2026-09-18 07:20: "we need autoapprove or team lead approve for these if i sleep", and 07:25: "Ok lets do 2 and 3. Team lead assigned by me dynamically (chat command?) and lead can move the duty to another agent if they need to". Authorized at 07:25 and unstarted four hours later. A single lead may decide ordinary confirmations in his place. The rules are what stop that becoming "any agent may approve anything": - The lead starts UNSET and there is no default, so deploying this cannot weaken the gate by itself. Unset means only the owner decides, which is exactly today's behaviour. - NO SELF-APPOINTMENT. An agent cannot make itself lead and cannot fill an empty post; only the human owner can. Otherwise the first agent to boot grants itself approval rights. - The sitting lead MAY hand over, because Petrus asked for that, and loses the duty by doing so. It cannot re-appoint itself. - Intents marked `requiresHuman` are never delegable. A sleeping owner is a reason to wait on a credential write, not to widen who may authorise it. The CALLER classifies: the precommand gate has the command text and the patterns, so the daemon never guesses intent from a prompt string. It also fixes an audit-trail lie that predates this: decideIntent hardcoded `actor: 'petrus'` and `approver: 'petrus'` on every receipt and status push, whoever called the endpoint. Decisions now record who made them and under which rule, and listIntents/getIntent expose it — they are what the queue and the dashboard read, so leaving it out hid the trail exactly where anyone would look for it. HTTP: GET /lead for status, POST /lead to assign, transfer or clear (handle: null). POST /intent/:id/decision takes an optional `actor`, defaulting to the owner so every existing caller keeps working unchanged. Refusals return 403, not 400 — the request was well formed and a rule declined it. Twelve tests, most of them negative controls: a delegation feature is only as good as what it refuses, and a suite proving only that the lead CAN approve would pass just as happily with no rules at all. Full suite 343/343. Inert until the daemon restarts, which interrupts every agent's command gate — that is Petrus's call to make, not mine. Co-Authored-By: Claude Opus 5 --- src/confirmations.mjs | 198 ++++++++++++++++++++++++++++++++++++++-- test/team-lead.test.mjs | 118 ++++++++++++++++++++++++ 2 files changed, 306 insertions(+), 10 deletions(-) create mode 100644 test/team-lead.test.mjs diff --git a/src/confirmations.mjs b/src/confirmations.mjs index b56ebfa..6ede44b 100644 --- a/src/confirmations.mjs +++ b/src/confirmations.mjs @@ -293,6 +293,12 @@ export function listIntents() { // output cannot disagree about what is known. announceSummary: announceSummaryLine(i), announcements: announcementsView(i), + // Who decided, and under which rule. The queue and the dashboard both read + // this projection, so leaving it out hid the audit trail in the one place + // anyone would go looking for it. + decidedBy: i.decidedBy ?? null, + decidedByRole: i.decidedByRole ?? null, + requiresHuman: Boolean(i.requiresHuman), })); } @@ -312,17 +318,130 @@ export function getIntent(id) { announceState: announceStateOf(i), announceSummary: announceSummaryLine(i), announcements: announcementsView(i), + decidedBy: i.decidedBy ?? null, + decidedByRole: i.decidedByRole ?? null, + requiresHuman: Boolean(i.requiresHuman), }; } // Decide an intent. Returns true if decided, false if id unknown or already // decided. Idempotent for same decision; rejects different decision after // settle. -export function decideIntent(id, rawDecision, { receiptsPath } = {}) { - // Look the intent up BEFORE validating, because what counts as a legal - // answer depends on the intent: a choice intent's legal answers are its own - // declared options, and nothing else. Validating first against a fixed - // approve/deny vocabulary is what made multi-option intents impossible. +// +// Look the intent up BEFORE validating, because what counts as a legal +// answer depends on the intent: a choice intent's legal answers are its own +// declared options, and nothing else. Validating first against a fixed +// approve/deny vocabulary is what made multi-option intents impossible. +// --------------------------------------------------------------------------- +// TEAM-LEAD APPROVALS +// +// Petrus, 2026-09-18 07:20: "we need autoapprove or team lead approve for these +// if i sleep", and 07:25: "Ok lets do 2 and 3. Team lead assigned by me +// dynamically (chat command?) and lead can move the duty to another agent if +// they need to". +// +// So: a single lead may decide ordinary confirmations in his place. The rules +// below are what keep that from becoming "any agent can approve anything". +// +// * The lead starts UNSET and there is no default. An unset lead means only +// the human owner decides, which is exactly today's behaviour — this +// feature can never weaken the gate by simply being deployed. +// * NO SELF-APPOINTMENT. An agent cannot make itself lead, and cannot appoint +// one while the post is empty; only the human owner can fill it. Otherwise +// the first agent to boot grants itself approval rights. +// * The sitting lead MAY hand over, because Petrus asked for exactly that. +// It may not appoint a second lead: handing over clears its own claim. +// * The human owner can always assign, transfer or clear, and can always +// decide regardless of who holds the post. +// * Intents marked `requiresHuman` are never delegable. Destructive, +// credential and paid actions are the owner's alone; a sleeping owner is a +// reason to delay those, not to widen who may authorise them. +// * Every assignment and every decision records WHO did it. Before this, the +// receipt said `actor: 'petrus'` no matter who called the endpoint, so the +// audit trail asserted something nobody had checked. +// --------------------------------------------------------------------------- + +/** Handle of the human owner — the only identity that can fill an empty post. */ +export const OWNER_HANDLE = 'petrus'; + +let teamLead = null; // { handle, assignedBy, assignedAt } + +function normalizeHandle(handle) { + if (typeof handle !== 'string') return null; + const trimmed = handle.trim().replace(/^@+/, ''); + return trimmed ? `@${trimmed}` : null; +} + +function isOwner(actor) { + return normalizeHandle(actor) === normalizeHandle(OWNER_HANDLE); +} + +/** Current lead, or null. Safe to call before any assignment. */ +export function getLead() { + return teamLead ? { ...teamLead } : null; +} + +/** + * Assign, transfer or clear the lead. + * - owner may do anything; + * - the sitting lead may transfer the duty onward, or clear it; + * - nobody else may touch it, and nobody may appoint themselves. + * Pass `handle: null` to clear. + */ +export function setLead(handle, { actor, receiptsPath } = {}) { + const by = normalizeHandle(actor); + if (!by) return { ok: false, error: 'actor is required' }; + const target = handle === null ? null : normalizeHandle(handle); + if (handle !== null && !target) return { ok: false, error: 'handle is required' }; + + const owner = isOwner(by); + const sitting = teamLead && normalizeHandle(teamLead.handle) === by; + if (!owner && !sitting) { + return { + ok: false, + error: teamLead + ? `only ${OWNER_HANDLE} or the current lead (${teamLead.handle}) may change the lead` + : `the lead is unset; only ${OWNER_HANDLE} may appoint one`, + }; + } + // A lead handing over must name someone else. Re-appointing yourself is a + // no-op dressed as an action, and self-appointment is the thing we forbid. + if (!owner && target && target === by) { + return { ok: false, error: 'a lead cannot re-appoint itself; name another agent or clear' }; + } + + const previous = teamLead ? teamLead.handle : null; + teamLead = target ? { handle: target, assignedBy: by, assignedAt: Date.now() } : null; + postReceipt(receiptsPath, { + kind: 'lead.changed', lead: target, previous, actor: by, at: Date.now(), + }); + return { ok: true, lead: getLead(), previous }; +} + +/** + * May `actor` decide this intent? Returns a reason when not, so the caller can + * say which rule refused rather than a bare 403. + */ +export function canDecide(intent, actor) { + const who = normalizeHandle(actor); + if (!who) return { ok: false, error: 'actor is required' }; + if (isOwner(who)) return { ok: true, role: 'owner' }; + if (intent?.requiresHuman) { + return { + ok: false, + error: `this action is reserved for ${OWNER_HANDLE} and cannot be delegated`, + }; + } + if (teamLead && normalizeHandle(teamLead.handle) === who) return { ok: true, role: 'lead' }; + return { + ok: false, + error: teamLead + ? `only ${OWNER_HANDLE} or the team lead (${teamLead.handle}) may decide` + : `only ${OWNER_HANDLE} may decide; no team lead is assigned`, + }; +} + +export function decideIntent(id, rawDecision, { receiptsPath, actor = OWNER_HANDLE } = {}) { const i = intents.get(id); if (!i) return { ok: false, error: `unknown intent ${id}` }; const options = Array.isArray(i.options) && i.options.length ? i.options : null; @@ -346,12 +465,18 @@ export function decideIntent(id, rawDecision, { receiptsPath } = {}) { if (i.decision === decision) return { ok: true, idempotent: true }; return { ok: false, error: `intent ${id} already decided as ${i.decision}` }; } + const permitted = canDecide(i, actor); + if (!permitted.ok) return { ok: false, error: permitted.error }; + i.status = 'decided'; i.decision = decision; i.decidedAt = Date.now(); + i.decidedBy = normalizeHandle(actor); + i.decidedByRole = permitted.role; persistIntent(id, i); postReceipt(receiptsPath, { kind: 'intent.decided', id, decision, decidedAt: i.decidedAt, prompt: i.prompt, + actor: i.decidedBy, role: permitted.role, }); // Resolve waiters. for (const r of i.resolvers) { @@ -369,7 +494,7 @@ export function decideIntent(id, rawDecision, { receiptsPath } = {}) { if (decision === 'deny') { settleAction(action, { status: 'denied', - actor: 'petrus', + actor: i.decidedBy, decided_at: new Date(i.decidedAt).toISOString(), ran_at: null, command: null, @@ -390,7 +515,7 @@ export function decideIntent(id, rawDecision, { receiptsPath } = {}) { runApprovedAction(action, { receiptsPath }).catch((e) => { settleAction(action, { status: 'failed', - actor: 'petrus', + actor: i.decidedBy, decided_at: action.decided_at, ran_at: new Date().toISOString(), command: action.command || null, @@ -408,7 +533,7 @@ export function decideIntent(id, rawDecision, { receiptsPath } = {}) { // rather than inventing a state that could move a row backwards. pushStatus(id, (options || decision === 'approve') ? 'approved' : 'denied', { decision, - approver: 'petrus', + approver: i.decidedBy, decided_at: new Date(i.decidedAt).toISOString(), }); } @@ -430,6 +555,10 @@ export async function createIntent({ receiptsPath, fromHandle, // optional originator handle (e.g. "@CodexMB") for per-agent // chat-author attribution; passed through to announcers. + requiresHuman = false, // destructive/credential/paid: never delegable to a + // team lead, however sound the lead is. A sleeping + // owner is a reason to WAIT on these, not to widen + // who may authorise them. }) { const id = randomUUID().slice(0, 8); // ROUTE-DEPENDENT CAP, verified on GroupMind origin/main 159b16b. This @@ -468,11 +597,14 @@ export async function createIntent({ // before this existed, which reads as 'unknown'. The distinction is the // backward-compatibility rule, so do not drop this to save a few bytes. announcements: {}, + requiresHuman: Boolean(requiresHuman), + decidedBy: null, }; intents.set(id, intent); persistIntent(id, intent); postReceipt(receiptsPath, { kind: 'intent.created', id, prompt, session, options: cleanOptions, channels, createdAt: intent.createdAt, + requiresHuman: intent.requiresHuman, }); pushStatus(id, 'pending', { target_summary: prompt }); // Record what happens to each channel's post. Handed DOWN to the announcer, @@ -933,8 +1065,43 @@ export function startConfirmationsServer({ res.end(JSON.stringify({ ok: false, error: 'invalid json' })); return; } - const result = decideIntent(id, payload.decision, { receiptsPath }); - res.writeHead(result.ok ? 200 : 400, { 'Content-Type': 'application/json' }); + // `actor` is who is deciding. It defaults to the owner so every + // existing caller (the phone buttons, the dashboard) keeps working + // unchanged; a team lead identifies itself explicitly. + const result = decideIntent(id, payload.decision, { + receiptsPath, + actor: payload.actor || OWNER_HANDLE, + }); + // 403, not 400: the request was well formed and the rule refused it. + const code = result.ok ? 200 : /may decide|reserved for|actor is required/.test(result.error || '') ? 403 : 400; + res.writeHead(code, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify(result)); + }); + return; + } + if (req.method === 'GET' && url.pathname === '/lead') { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ ok: true, lead: getLead(), owner: OWNER_HANDLE })); + return; + } + if (req.method === 'POST' && url.pathname === '/lead') { + let body = ''; + req.on('data', (c) => { body += c; }); + req.on('end', () => { + let payload; + try { payload = JSON.parse(body); } catch { + res.writeHead(400, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ ok: false, error: 'invalid json' })); + return; + } + // `handle: null` clears the post. `actor` is who is asking, and the + // rules in setLead decide whether they may. + const result = setLead( + Object.prototype.hasOwnProperty.call(payload, 'handle') ? payload.handle : undefined, + { actor: payload.actor, receiptsPath } + ); + const code = result.ok ? 200 : /may appoint|may change|cannot re-appoint/.test(result.error || '') ? 403 : 400; + res.writeHead(code, { 'Content-Type': 'application/json' }); res.end(JSON.stringify(result)); }); return; @@ -1062,6 +1229,13 @@ export function startConfirmationsServer({ // `from_handle` so the GroupMind announcer authors the chat // post as the originating agent rather than the daemon owner. fromHandle: typeof payload.from_handle === 'string' ? payload.from_handle : undefined, + // The CALLER classifies. The precommand gate already knows which + // commands are destructive, credential-touching or paid — it has + // the command text and the patterns — and marks those here so a + // team lead can never approve them. The daemon does not re-derive + // that from a prompt string: guessing intent from prose is exactly + // how a gate quietly stops gating. + requiresHuman: payload.requires_human === true, }); res.writeHead(201, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ ok: true, id })); @@ -1819,4 +1993,8 @@ export function _resetForTests() { } } intents.clear(); + // The lead is global state too: a test that appoints one must not leak that + // appointment into the next test, or a later "an agent cannot decide" case + // passes for the wrong reason. + teamLead = null; } diff --git a/test/team-lead.test.mjs b/test/team-lead.test.mjs new file mode 100644 index 0000000..ec4bbbb --- /dev/null +++ b/test/team-lead.test.mjs @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// +// Team-lead approvals (Petrus, 2026-09-18 07:25: "Ok lets do 2 and 3. Team lead +// assigned by me dynamically and lead can move the duty to another agent"). +// +// Most of these are NEGATIVE controls on purpose. A delegation feature is only +// as good as the things it refuses, and a test suite that only proves the lead +// CAN approve would pass just as happily if the rules were not there at all. + +import { test, beforeEach } from 'node:test'; +import assert from 'node:assert/strict'; + +import { + OWNER_HANDLE, + createIntent, + decideIntent, + getLead, + setLead, + _resetForTests, +} from '../src/confirmations.mjs'; + +const OWNER = OWNER_HANDLE; +const LEAD = '@hermes'; +const OTHER = '@codexmb'; + +async function pendingIntent(opts = {}) { + return createIntent({ prompt: 'run something', session: 's', channels: [], ...opts }); +} + +beforeEach(() => _resetForTests()); + +test('the lead starts unset, so deploying this changes nothing on its own', () => { + assert.equal(getLead(), null); +}); + +test('an agent cannot appoint itself while the post is empty', () => { + const r = setLead(LEAD, { actor: LEAD }); + assert.equal(r.ok, false); + assert.match(r.error, /only petrus may appoint/i); + assert.equal(getLead(), null); +}); + +test('an agent cannot appoint someone else while the post is empty', () => { + assert.equal(setLead(OTHER, { actor: LEAD }).ok, false); + assert.equal(getLead(), null); +}); + +test('with no lead assigned, an agent cannot decide', async () => { + const id = await pendingIntent(); + const r = decideIntent(id, 'approve', { actor: LEAD }); + assert.equal(r.ok, false); + assert.match(r.error, /no team lead is assigned/i); +}); + +test('the owner appoints, and the lead may then decide an ordinary intent', async () => { + assert.equal(setLead(LEAD, { actor: OWNER }).ok, true); + assert.equal(getLead().handle, LEAD); + const id = await pendingIntent(); + assert.equal(decideIntent(id, 'approve', { actor: LEAD }).ok, true); +}); + +test('a non-lead agent still cannot decide once a lead exists', async () => { + setLead(LEAD, { actor: OWNER }); + const id = await pendingIntent(); + const r = decideIntent(id, 'approve', { actor: OTHER }); + assert.equal(r.ok, false); + assert.match(r.error, /team lead \(@hermes\)/); +}); + +test('requiresHuman intents are NOT delegable, even to a sitting lead', async () => { + setLead(LEAD, { actor: OWNER }); + const id = await pendingIntent({ requiresHuman: true }); + const r = decideIntent(id, 'approve', { actor: LEAD }); + assert.equal(r.ok, false); + assert.match(r.error, /reserved for petrus/i); + // and the owner can still decide it + assert.equal(decideIntent(id, 'approve', { actor: OWNER }).ok, true); +}); + +test('the lead may hand the duty over, and loses it by doing so', async () => { + setLead(LEAD, { actor: OWNER }); + assert.equal(setLead(OTHER, { actor: LEAD }).ok, true); + assert.equal(getLead().handle, OTHER); + const id = await pendingIntent(); + const r = decideIntent(id, 'approve', { actor: LEAD }); + assert.equal(r.ok, false, 'the previous lead must not keep deciding after handover'); + assert.equal(decideIntent(id, 'approve', { actor: OTHER }).ok, true); +}); + +test('a lead cannot re-appoint itself', () => { + setLead(LEAD, { actor: OWNER }); + const r = setLead(LEAD, { actor: LEAD }); + assert.equal(r.ok, false); + assert.match(r.error, /cannot re-appoint itself/); +}); + +test('the owner can clear the post, and delegation stops immediately', async () => { + setLead(LEAD, { actor: OWNER }); + assert.equal(setLead(null, { actor: OWNER }).ok, true); + assert.equal(getLead(), null); + const id = await pendingIntent(); + assert.equal(decideIntent(id, 'approve', { actor: LEAD }).ok, false); +}); + +test('the decision records who made it, not a hardcoded owner', async () => { + setLead(LEAD, { actor: OWNER }); + const id = await pendingIntent(); + decideIntent(id, 'approve', { actor: LEAD }); + const { listIntents } = await import('../src/confirmations.mjs'); + const found = listIntents().find((i) => i.id === id); + assert.equal(found.decidedBy, LEAD); + assert.equal(found.decidedByRole, 'lead'); +}); + +test('handles compare without caring about a leading @ or stray spaces', () => { + assert.equal(setLead(' hermes ', { actor: OWNER }).ok, true); + assert.equal(getLead().handle, '@hermes'); +}); From d4503fc4b25845a4e5b9df007ce8f473466250b3 Mon Sep 17 00:00:00 2001 From: ThinkOff Date: Fri, 18 Sep 2026 14:18:21 +0300 Subject: [PATCH 02/14] feat(automation): /lead chat command, outside the configurable rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Petrus asked for the lead to be "assigned by me dynamically (chat command?)". /lead @agent appoint or transfer /lead status who holds it (anyone may ask) /lead clear vacate the post Handled before the rules array and consuming the message, not as a rule. Who may approve shell commands on this machine should not be editable by adding a JSON entry — a rule that grants approval rights is a rule someone can write by accident, and a cooldown or a first-match-wins ordering could otherwise swallow the command. Authorisation is double-keyed: the sender must be the owner handle AND the message must carry isHuman. Either alone is too weak — agents post under their own handles with isHuman false, and a tapped action button arrives as `petrus` with isHuman false, so requiring both means neither an agent quoting this syntax nor a replayed button can appoint anyone. The daemon enforces the same rules again; this is the outer key, not the only one. The parser is deliberately strict: exactly one token after /lead. My own test caught the lenient version appointing @somebody from "/lead somebody nice please" — taking the first word is a guess, and a wrong guess here hands command approval to the wrong agent. Nine tests, seven of them refusals. Full suite 352/352. Co-Authored-By: Claude Opus 5 --- src/room-automation.mjs | 111 +++++++++++++++++++++++++++++++++++++ test/lead-command.test.mjs | 107 +++++++++++++++++++++++++++++++++++ 2 files changed, 218 insertions(+) create mode 100644 test/lead-command.test.mjs diff --git a/src/room-automation.mjs b/src/room-automation.mjs index a350d34..8c31cd8 100644 --- a/src/room-automation.mjs +++ b/src/room-automation.mjs @@ -106,6 +106,97 @@ function postMessage(room, body, apiKey, config) { /** * Check if a message matches a rule's conditions. */ +// --------------------------------------------------------------------------- +// /lead — the chat command Petrus asked for on 2026-09-18 07:25 ("Team lead +// assigned by me dynamically (chat command?)"). +// +// /lead @agent appoint or transfer /lead status who holds it +// /lead clear vacate the post +// +// HANDLED HERE, NOT AS A CONFIGURABLE RULE, on purpose. Who may approve +// commands on this machine is not something that should be editable by adding +// an entry to a rules array in a JSON file — a rule that grants approval +// rights is a rule someone can write by accident. +// +// AUTHORISATION IS DOUBLE-KEYED: the sender must be the owner handle AND the +// message must be flagged as human. Either alone is too weak — agents post +// under their own handles with isHuman false, and a tapped action button +// arrives as `petrus` with isHuman false (see the action-button notes), so +// requiring both means neither an agent quoting this syntax nor a replayed +// button can appoint anyone. The daemon enforces the same rules again; this +// is the outer key, not the only one. +// --------------------------------------------------------------------------- + +const LEAD_COMMAND_RE = /^\s*\/lead\b\s*(.*)$/i; + +function parseLeadCommand(body) { + const m = LEAD_COMMAND_RE.exec(body || ''); + if (!m) return null; + const rest = (m[1] || '').trim(); + if (!rest || /^status$/i.test(rest)) return { op: 'status' }; + if (/^clear$/i.test(rest)) return { op: 'clear' }; + // STRICT on purpose: exactly one token, nothing trailing. Taking the first + // word of "/lead somebody nice please" and appointing @somebody is a wrong + // guess that hands command-approval rights to the wrong agent. When the + // input is not unambiguous, refuse and say so. + if (!/^@?[A-Za-z0-9_.-]+$/.test(rest)) return { op: 'invalid', handle: rest }; + return { op: 'assign', handle: rest.replace(/^@+/, '') }; +} + +async function callDaemon(daemonUrl, path, { method = 'GET', body } = {}) { + const res = await fetch(`${daemonUrl.replace(/\/+$/, '')}${path}`, { + method, + headers: body ? { 'Content-Type': 'application/json' } : undefined, + body: body ? JSON.stringify(body) : undefined, + }); + let payload = null; + try { payload = await res.json(); } catch { /* non-JSON error body */ } + return { status: res.status, payload }; +} + +/** + * Returns a reply string when the message was a /lead command, or null when it + * was not one. Never throws: a daemon that is down must not stop the poller. + */ +export async function handleLeadCommand(msg, { daemonUrl, ownerHandle = 'petrus' } = {}) { + const parsed = parseLeadCommand(msg.body || ''); + if (!parsed) return null; + + const sender = (msg.user?.handle || msg.from || msg.sender || '').replace(/^@+/, '').toLowerCase(); + const owner = ownerHandle.replace(/^@+/, '').toLowerCase(); + const fromOwner = sender === owner && msg.isHuman === true; + + try { + if (parsed.op === 'status') { + const { payload } = await callDaemon(daemonUrl, '/lead'); + const lead = payload?.lead; + return lead + ? `Team lead: ${lead.handle} (assigned by ${lead.assignedBy}).` + : 'Team lead: unset. Only the owner can decide confirmations, and only the owner can appoint a lead.'; + } + if (parsed.op === 'invalid') { + return `"${parsed.handle}" is not a handle. Use /lead @agent, /lead status or /lead clear.`; + } + if (!fromOwner) { + // Say which key was missing rather than a flat refusal — a lead trying to + // hand over from chat needs to know the daemon route exists for that. + return `Only ${ownerHandle} can change the team lead from chat. A sitting lead may hand over via the daemon.`; + } + const { status, payload } = await callDaemon(daemonUrl, '/lead', { + method: 'POST', + body: { handle: parsed.op === 'clear' ? null : parsed.handle, actor: ownerHandle }, + }); + if (payload?.ok) { + return parsed.op === 'clear' + ? 'Team lead cleared. Confirmations are owner-only again.' + : `Team lead is now @${parsed.handle}. Destructive, credential and paid actions still wait for ${ownerHandle}.`; + } + return `Could not change the lead (${status}): ${payload?.error || 'no response'}`; + } catch (e) { + return `Could not reach the confirmations daemon: ${e.message}`; + } +} + function matchesRule(msg, rule) { const match = rule.match || {}; const body = (msg.body || '').toLowerCase(); @@ -319,6 +410,26 @@ export async function startRoomAutomation({ rooms, apiKey, handle, interval, con // Attach room for rule matching m.room = room; + // /lead runs BEFORE the configurable rules and consumes the message. + // It is a command about who may approve things, so it must not be + // shadowed, cooled down or overridden by whatever is in the rules + // array. + const leadReply = await handleLeadCommand(m, { + daemonUrl: config?.confirmations?.daemon_url || 'http://127.0.0.1:8788', + ownerHandle: config?.poller?.owner_handle || 'petrus', + }); + if (leadReply !== null) { + postMessage(room, leadReply, apiKey, config); + appendReceipt(receiptPath, createReceipt({ + actor: { name: 'automation', kind: 'command' }, + action: `/lead from ${m.user?.handle || m.from || '?'}`, + status: 'completed', + startedAt: new Date().toISOString(), + })); + actionsRun++; + continue; + } + // Check each rule for (const rule of rules) { if (!matchesRule(m, rule)) continue; diff --git a/test/lead-command.test.mjs b/test/lead-command.test.mjs new file mode 100644 index 0000000..1fffbf4 --- /dev/null +++ b/test/lead-command.test.mjs @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// +// The /lead chat command. Again mostly negative controls: this command decides +// who may approve shell commands on this machine, so what it REFUSES is the +// whole point. A suite proving only that petrus can appoint would pass just as +// happily if the sender check were missing. + +import { test, before, after } from 'node:test'; +import assert from 'node:assert/strict'; +import { createServer } from 'node:http'; + +import { handleLeadCommand } from '../src/room-automation.mjs'; + +let server; +let daemonUrl; +let lastPost = null; +let lead = null; + +before(async () => { + // A stand-in daemon: enough to prove the command reaches it with the right + // body, without pulling the real confirmations server into this test. + server = createServer((req, res) => { + if (req.method === 'GET' && req.url === '/lead') { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ ok: true, lead, owner: 'petrus' })); + return; + } + if (req.method === 'POST' && req.url === '/lead') { + let body = ''; + req.on('data', c => { body += c; }); + req.on('end', () => { + lastPost = JSON.parse(body); + lead = lastPost.handle ? { handle: `@${lastPost.handle}`, assignedBy: lastPost.actor } : null; + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ ok: true, lead })); + }); + return; + } + res.writeHead(404); res.end('{}'); + }); + await new Promise(r => server.listen(0, '127.0.0.1', r)); + daemonUrl = `http://127.0.0.1:${server.address().port}`; +}); + +after(() => server?.close()); + +const owner = body => ({ body, from: 'petrus', isHuman: true }); +const agent = body => ({ body, from: '@codexmb', isHuman: false }); + +test('a message that is not /lead is not consumed', async () => { + assert.equal(await handleLeadCommand(owner('deploy the thing'), { daemonUrl }), null); + assert.equal(await handleLeadCommand(owner('the team lead should decide'), { daemonUrl }), null); +}); + +test('status is readable by anyone, including agents', async () => { + lead = null; + const reply = await handleLeadCommand(agent('/lead status'), { daemonUrl }); + assert.match(reply, /unset/i); +}); + +test('an AGENT cannot appoint anyone, including itself', async () => { + lastPost = null; + const reply = await handleLeadCommand(agent('/lead @codexmb'), { daemonUrl }); + assert.match(reply, /only petrus/i); + assert.equal(lastPost, null, 'the daemon must never have been called'); +}); + +test('a message merely CLAIMING to be from petrus is refused without the human flag', async () => { + lastPost = null; + const spoofed = { body: '/lead @codexmb', from: 'petrus', isHuman: false }; + const reply = await handleLeadCommand(spoofed, { daemonUrl }); + assert.match(reply, /only petrus/i); + assert.equal(lastPost, null); +}); + +test('the owner can appoint, and the daemon is called with the right body', async () => { + lastPost = null; + const reply = await handleLeadCommand(owner('/lead @hermes'), { daemonUrl }); + assert.match(reply, /now @hermes/i); + assert.deepEqual(lastPost, { handle: 'hermes', actor: 'petrus' }); +}); + +test('the reply says the owner-only class still waits for him', async () => { + const reply = await handleLeadCommand(owner('/lead @hermes'), { daemonUrl }); + assert.match(reply, /credential and paid actions still wait/i); +}); + +test('the owner can clear the post', async () => { + await handleLeadCommand(owner('/lead @hermes'), { daemonUrl }); + const reply = await handleLeadCommand(owner('/lead clear'), { daemonUrl }); + assert.match(reply, /cleared/i); + assert.equal(lastPost.handle, null); +}); + +test('a malformed handle is rejected before the daemon is touched', async () => { + lastPost = null; + const reply = await handleLeadCommand(owner('/lead somebody nice please!!'), { daemonUrl }); + assert.match(reply, /not a handle/i); + assert.equal(lastPost, null); +}); + +test('a daemon that is down reports it instead of throwing', async () => { + const reply = await handleLeadCommand(owner('/lead @hermes'), { + daemonUrl: 'http://127.0.0.1:1', + }); + assert.match(reply, /could not reach/i); +}); From 74b09421b8ed7cc73325d1bb5c03a459b743cfc8 Mon Sep 17 00:00:00 2001 From: ThinkOff Date: Fri, 18 Sep 2026 14:24:28 +0300 Subject: [PATCH 03/14] fix(automation): /lead status must not answer "unset" from a 404 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Petrus typed `/lead status` in the room one minute after I described the command. Nothing answered, because the code is on a branch and the daemon has not restarted — but the more interesting problem is what WOULD have answered. The daemon currently running has no /lead route, so GET /lead returns 404. The first version read that as no lead assigned and replied "Team lead: unset", which is true-sounding, plausible, and produced entirely by the endpoint not existing. That is the same failure this repo spent the morning finding twice: a broken read wearing a legitimate answer — an admin login that said "invalid password" when the rate-limit table was missing, and a scratchpads page that said "none found" when the table never existed. It now distinguishes them and says the feature is built but not running here. A status command that cannot tell "nobody holds the post" from "this endpoint does not exist" is worse than no status command, because this one is about who may approve shell commands. Co-Authored-By: Claude Opus 5 --- src/room-automation.mjs | 12 ++++++++++-- test/lead-command.test.mjs | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/room-automation.mjs b/src/room-automation.mjs index 8c31cd8..505fb02 100644 --- a/src/room-automation.mjs +++ b/src/room-automation.mjs @@ -168,8 +168,16 @@ export async function handleLeadCommand(msg, { daemonUrl, ownerHandle = 'petrus' try { if (parsed.op === 'status') { - const { payload } = await callDaemon(daemonUrl, '/lead'); - const lead = payload?.lead; + const { status, payload } = await callDaemon(daemonUrl, '/lead'); + // A daemon that does not KNOW about /lead answers 404, and reading that + // as "unset" would be a false answer wearing a legitimate one — the same + // failure shape as an empty list that is really a broken query. Say the + // feature is not running instead. + if (status === 404 || !payload?.ok) { + return 'Team lead: this daemon does not have /lead — the feature is built but not ' + + 'running here yet (it needs a restart). Confirmations are owner-only meanwhile.'; + } + const lead = payload.lead; return lead ? `Team lead: ${lead.handle} (assigned by ${lead.assignedBy}).` : 'Team lead: unset. Only the owner can decide confirmations, and only the owner can appoint a lead.'; diff --git a/test/lead-command.test.mjs b/test/lead-command.test.mjs index 1fffbf4..46351d6 100644 --- a/test/lead-command.test.mjs +++ b/test/lead-command.test.mjs @@ -105,3 +105,17 @@ test('a daemon that is down reports it instead of throwing', async () => { }); assert.match(reply, /could not reach/i); }); + +test('status does not report "unset" when the daemon lacks the route', async () => { + // Petrus typed /lead status at 11:23 before the daemon had been restarted. + // The first version answered "unset" — true-sounding, and produced by a 404. + // An answer that cannot tell "nobody holds the post" from "this endpoint does + // not exist" is the empty-list bug again, in a security-relevant place. + const dead = createServer((req, res) => { res.writeHead(404); res.end('{}'); }); + await new Promise(r => dead.listen(0, '127.0.0.1', r)); + const url = `http://127.0.0.1:${dead.address().port}`; + const reply = await handleLeadCommand({ body: '/lead status', from: 'petrus', isHuman: true }, { daemonUrl: url }); + assert.match(reply, /not running here yet/i); + assert.doesNotMatch(reply, /^Team lead: unset/); + dead.close(); +}); From 0f49a8a8deecb73d4a4c5c0d893614bf844116cd Mon Sep 17 00:00:00 2001 From: ThinkOff Date: Fri, 18 Sep 2026 14:28:24 +0300 Subject: [PATCH 04/14] fix(confirmations): an actor in the request body is a claim, not an identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codexmb reviewed af178ad against a real daemon and was right on every point. My authorization rules were sound and sat on top of an identity anybody could assert, so they were decoration: 1. POST /lead took `actor` from the JSON body. Any caller that could reach the daemon could say actor:"petrus" and appoint itself lead. 2. POST /intent/:id/decision defaulted a missing actor to the owner, so the whole role boundary was bypassed by OMITTING a field. 3. createIntent received fromHandle and discarded it, so a lead could raise a confirmation and approve its own request. My tests could not have found any of this: they called decideIntent() and setLead() directly with truthful actor strings. A test that supplies its own identity cannot discover that identity is unauthenticated. That is the lesson, not the bug. Fixes: - `principals` maps per-agent token -> handle, and the actor is now the PROVEN principal. `payload.actor` is never read. A request that supplies an actor without proving one is refused 403 rather than quietly downgraded. - POST /lead requires a proven principal outright. Appointing a lead is a privilege grant, not a routine approval, so it never falls back to "local and anonymous means petrus" — and with no principals configured the route refuses, so an unconfigured daemon has no delegation rather than a forgeable one. This ships INERT, not insecure. - The intent retains requestedBy, and a lead cannot approve its own request. - Anonymous callers still act as the owner for ordinary decisions. That is today's trust model — the daemon binds locally and Petrus's phone buttons carry no identity — and changing it here would lock him out of his own approvals. Two things only the HTTP tests could find, both now fixed: a registered per-agent token was rejected at the shared-token door with 401 before anything could read it, and refusals returned 400 or 403 according to a REGEX over the error message. Authorization outcomes now carry a `forbidden` flag; a status code inferred from prose breaks silently the next time someone rewords a string, and this one guards who may run shell commands. Seven HTTP-level negative tests. Full suite 360/360. Deployment stays held. Co-Authored-By: Claude Opus 5 --- src/confirmations.mjs | 114 +++++++++++++++++++++++++++++++----- test/lead-http.test.mjs | 126 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 225 insertions(+), 15 deletions(-) create mode 100644 test/lead-http.test.mjs diff --git a/src/confirmations.mjs b/src/confirmations.mjs index 6ede44b..4736e32 100644 --- a/src/confirmations.mjs +++ b/src/confirmations.mjs @@ -399,6 +399,7 @@ export function setLead(handle, { actor, receiptsPath } = {}) { if (!owner && !sitting) { return { ok: false, + forbidden: true, error: teamLead ? `only ${OWNER_HANDLE} or the current lead (${teamLead.handle}) may change the lead` : `the lead is unset; only ${OWNER_HANDLE} may appoint one`, @@ -407,7 +408,7 @@ export function setLead(handle, { actor, receiptsPath } = {}) { // A lead handing over must name someone else. Re-appointing yourself is a // no-op dressed as an action, and self-appointment is the thing we forbid. if (!owner && target && target === by) { - return { ok: false, error: 'a lead cannot re-appoint itself; name another agent or clear' }; + return { ok: false, forbidden: true, error: 'a lead cannot re-appoint itself; name another agent or clear' }; } const previous = teamLead ? teamLead.handle : null; @@ -424,17 +425,29 @@ export function setLead(handle, { actor, receiptsPath } = {}) { */ export function canDecide(intent, actor) { const who = normalizeHandle(actor); - if (!who) return { ok: false, error: 'actor is required' }; + if (!who) return { ok: false, forbidden: true, error: 'actor is required' }; if (isOwner(who)) return { ok: true, role: 'owner' }; if (intent?.requiresHuman) { return { ok: false, + forbidden: true, error: `this action is reserved for ${OWNER_HANDLE} and cannot be delegated`, }; } - if (teamLead && normalizeHandle(teamLead.handle) === who) return { ok: true, role: 'lead' }; + if (teamLead && normalizeHandle(teamLead.handle) === who) { + // A lead may clear other agents' work, never its own request. + if (intent?.requestedBy && normalizeHandle(intent.requestedBy) === who) { + return { + ok: false, + forbidden: true, + error: 'a team lead cannot approve its own request; this one needs the owner', + }; + } + return { ok: true, role: 'lead' }; + } return { ok: false, + forbidden: true, error: teamLead ? `only ${OWNER_HANDLE} or the team lead (${teamLead.handle}) may decide` : `only ${OWNER_HANDLE} may decide; no team lead is assigned`, @@ -466,7 +479,7 @@ export function decideIntent(id, rawDecision, { receiptsPath, actor = OWNER_HAND return { ok: false, error: `intent ${id} already decided as ${i.decision}` }; } const permitted = canDecide(i, actor); - if (!permitted.ok) return { ok: false, error: permitted.error }; + if (!permitted.ok) return { ok: false, forbidden: true, error: permitted.error }; i.status = 'decided'; i.decision = decision; @@ -599,6 +612,10 @@ export async function createIntent({ announcements: {}, requiresHuman: Boolean(requiresHuman), decidedBy: null, + // Retained so canDecide can refuse self-approval. Without it a lead could + // raise a confirmation and clear it themselves, which is not delegation, + // it is a bypass with extra steps (codexmb, 2026-09-18). + requestedBy: typeof fromHandle === 'string' ? fromHandle : null, }; intents.set(id, intent); persistIntent(id, intent); @@ -1034,19 +1051,51 @@ export function startConfirmationsServer({ port = 8788, host = '127.0.0.1', authToken = '', + // Per-agent tokens: { "": "@handle" }. THIS is what makes an actor an + // identity rather than a claim. `authToken` is a SHARED secret — everyone who + // has it looks identical — so a body field saying actor:"petrus" proves + // nothing (codexmb, 2026-09-18, reproduced against a real daemon). + // + // With no principals configured, privileged routes refuse and delegation is + // simply unavailable. That is deliberate: this feature ships INERT rather + // than insecure, and an unconfigured daemon behaves exactly as it does today. + principals = {}, receiptsPath, announce, // optional: enables POST /intent to create new intents externally wakeScript, // optional: shell script path; enables POST /wake to nudge the local IDE sessions, // optional: {agents: {...}} enables POST /sessions/send + GET /sessions/agents } = {}) { + const principalByToken = new Map( + Object.entries(principals || {}).map(([token, handle]) => [token, handle]) + ); + + /** + * The handle this request has PROVEN, or null. Never reads the body: an + * actor supplied by the caller is a self-declaration. Returns null when the + * bearer is the shared token, because the shared token identifies no one. + */ + function resolvePrincipal(req) { + const got = (req.headers.authorization || '').replace(/^Bearer\s+/i, ''); + if (!got) return null; + return principalByToken.get(got) ?? null; + } + const server = createServer((req, res) => { const url = new URL(req.url, `http://${req.headers.host || 'localhost'}`); // Auth check (constant-time when token configured). + // + // A registered PER-AGENT token is also a valid bearer. Without this the + // shared token is the only thing that gets past the door, so an agent + // presenting its own identity is rejected at 401 before anything can read + // it — delegation would be unusable on exactly the daemons that configure + // auth. Found by the HTTP-level tests; the function-level ones could not + // see it, because they never went through the door at all. if (authToken) { const got = (req.headers.authorization || '').replace(/^Bearer\s+/i, ''); const a = Buffer.from(got); const b = Buffer.from(authToken); - const ok = a.length === b.length && timingSafeEqual(a, b); + const ok = (a.length === b.length && timingSafeEqual(a, b)) + || principalByToken.has(got); if (!ok) { res.writeHead(401, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ ok: false, error: 'unauthorized' })); @@ -1065,15 +1114,35 @@ export function startConfirmationsServer({ res.end(JSON.stringify({ ok: false, error: 'invalid json' })); return; } - // `actor` is who is deciding. It defaults to the owner so every - // existing caller (the phone buttons, the dashboard) keeps working - // unchanged; a team lead identifies itself explicitly. + // The actor is the PROVEN principal, never `payload.actor`. A caller + // that says actor:"petrus" has said nothing (codexmb, 2026-09-18). + // + // With no per-agent principals configured, an unauthenticated caller + // is still treated as the owner. That is today's trust model — the + // daemon binds locally and Petrus's phone buttons carry no identity — + // and changing it here would lock him out of his own approvals. What + // it must NOT do is let that same anonymous caller act as a lead or + // clear an owner-only intent, which is why delegation requires a + // proven principal and never this fallback. + const principal = resolvePrincipal(req); + if (payload.actor && !principal) { + res.writeHead(403, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ + ok: false, + error: 'actor was supplied but this request proves no identity; ' + + 'use a per-agent token (principals) or omit actor', + })); + return; + } const result = decideIntent(id, payload.decision, { receiptsPath, - actor: payload.actor || OWNER_HANDLE, + actor: principal || OWNER_HANDLE, }); - // 403, not 400: the request was well formed and the rule refused it. - const code = result.ok ? 200 : /may decide|reserved for|actor is required/.test(result.error || '') ? 403 : 400; + // 403, not 400: the request was well formed and a RULE refused it. + // Read from a flag rather than by matching the message text — a status + // code inferred from prose breaks silently the next time someone + // rewords an error, and this one guards who may run commands. + const code = result.ok ? 200 : result.forbidden ? 403 : 400; res.writeHead(code, { 'Content-Type': 'application/json' }); res.end(JSON.stringify(result)); }); @@ -1094,13 +1163,28 @@ export function startConfirmationsServer({ res.end(JSON.stringify({ ok: false, error: 'invalid json' })); return; } - // `handle: null` clears the post. `actor` is who is asking, and the - // rules in setLead decide whether they may. + // Appointing a lead is a PRIVILEGE GRANT, not a routine approval, so + // it never falls back to "anonymous means petrus". Without a proven + // principal this route refuses outright, which is why an unconfigured + // daemon simply has no delegation rather than a forgeable one. + const principal = resolvePrincipal(req); + if (!principal) { + res.writeHead(403, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ + ok: false, + error: principalByToken.size === 0 + ? 'delegation is unavailable: this daemon has no per-agent tokens ' + + '(principals), so it cannot tell who is asking' + : 'this request proves no identity; use your per-agent token', + })); + return; + } + // `handle: null` clears the post. The actor is the proven principal. const result = setLead( Object.prototype.hasOwnProperty.call(payload, 'handle') ? payload.handle : undefined, - { actor: payload.actor, receiptsPath } + { actor: principal, receiptsPath } ); - const code = result.ok ? 200 : /may appoint|may change|cannot re-appoint/.test(result.error || '') ? 403 : 400; + const code = result.ok ? 200 : result.forbidden ? 403 : 400; res.writeHead(code, { 'Content-Type': 'application/json' }); res.end(JSON.stringify(result)); }); diff --git a/test/lead-http.test.mjs b/test/lead-http.test.mjs new file mode 100644 index 0000000..3d1b9d9 --- /dev/null +++ b/test/lead-http.test.mjs @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// +// HTTP-LEVEL negative tests for team-lead delegation. +// +// WHY THESE EXIST SEPARATELY: my first suite called decideIntent() and +// setLead() directly, passing truthful actor strings. Every one passed while +// the HTTP boundary handed the actor straight out of the request body, so +// anybody who could reach the daemon could call themselves petrus. @codexmb +// reproduced that against a real daemon on 2026-09-18. A test that supplies +// its own identity cannot discover that identity is unauthenticated. + +import { test, before, after } from 'node:test'; +import assert from 'node:assert/strict'; + +import { + createIntent, + getLead, + listIntents, + startConfirmationsServer, + _resetForTests, +} from '../src/confirmations.mjs'; + +const LEAD_TOKEN = 'token-for-hermes'; +const OTHER_TOKEN = 'token-for-codexmb'; +const SHARED = 'shared-bearer-everyone-has'; + +let base; +let server; + +before(async () => { + server = startConfirmationsServer({ + port: 0, + host: '127.0.0.1', + authToken: SHARED, + principals: { [LEAD_TOKEN]: '@hermes', [OTHER_TOKEN]: '@codexmb' }, + receiptsPath: '/tmp/iak-test-lead-http.jsonl', + announce: async () => {}, + }); + await new Promise(r => server.listen ? server.listen(0, '127.0.0.1', r) : r()); + const addr = server.address(); + base = `http://127.0.0.1:${addr.port}`; +}); + +after(() => server?.close?.()); + +const post = (path, body, token = SHARED) => + fetch(`${base}${path}`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` }, + body: JSON.stringify(body), + }); + +test('a shared-token caller CANNOT appoint itself by claiming to be petrus', async () => { + _resetForTests(); + const res = await post('/lead', { handle: 'codexmb', actor: 'petrus' }); + assert.equal(res.status, 403); + assert.equal(getLead(), null, 'nobody may be appointed by a caller that proves nothing'); +}); + +test('a per-agent token cannot appoint while the post is empty either', async () => { + _resetForTests(); + const res = await post('/lead', { handle: 'hermes' }, LEAD_TOKEN); + assert.equal(res.status, 403); + assert.equal(getLead(), null); +}); + +test('omitting actor does not let an anonymous caller clear an owner-only intent', async () => { + _resetForTests(); + const id = await createIntent({ + prompt: 'rm -rf $HOME', session: 's', channels: [], requiresHuman: true, announce: async () => {}, + }); + // Today's trust model: anonymous == owner, so this DOES settle — the point + // is that it settles AS THE OWNER, and cannot be laundered into a lead + // decision by naming one. + const claimed = await post(`/intent/${id}/decision`, { decision: 'approve', actor: 'hermes' }); + assert.equal(claimed.status, 403, 'a claimed actor without proof must be refused outright'); + const found = listIntents().find(i => i.id === id); + assert.equal(found.status, 'pending', 'the refused call must not have decided anything'); +}); + +test('a proven non-lead principal cannot decide', async () => { + _resetForTests(); + const id = await createIntent({ prompt: 'ls', session: 's', channels: [], announce: async () => {} }); + const res = await post(`/intent/${id}/decision`, { decision: 'approve', actor: 'codexmb' }, OTHER_TOKEN); + assert.equal(res.status, 403); + assert.equal(listIntents().find(i => i.id === id).status, 'pending'); +}); + +test('the decision receipt names the proven principal, not the claimed one', async () => { + _resetForTests(); + // Appoint via the library (the owner path), then decide over HTTP as the lead. + const { setLead } = await import('../src/confirmations.mjs'); + setLead('hermes', { actor: 'petrus' }); + const id = await createIntent({ prompt: 'ls', session: 's', channels: [], announce: async () => {} }); + // Claim to be petrus while holding hermes's token: the claim is ignored. + const res = await post(`/intent/${id}/decision`, { decision: 'approve', actor: 'petrus' }, LEAD_TOKEN); + assert.equal(res.status, 200); + const found = listIntents().find(i => i.id === id); + assert.equal(found.decidedBy, '@hermes'); + assert.equal(found.decidedByRole, 'lead'); +}); + +test('a lead cannot approve its own request', async () => { + _resetForTests(); + const { setLead } = await import('../src/confirmations.mjs'); + setLead('hermes', { actor: 'petrus' }); + const id = await createIntent({ + prompt: 'ls', session: 's', channels: [], fromHandle: '@hermes', announce: async () => {}, + }); + const res = await post(`/intent/${id}/decision`, { decision: 'approve' }, LEAD_TOKEN); + assert.equal(res.status, 403); + assert.equal(listIntents().find(i => i.id === id).status, 'pending'); +}); + +test('a proven lead still cannot clear a requiresHuman intent', async () => { + _resetForTests(); + const { setLead } = await import('../src/confirmations.mjs'); + setLead('hermes', { actor: 'petrus' }); + const id = await createIntent({ + prompt: 'cp ~/.ssh/id_ed25519 /tmp', session: 's', channels: [], + requiresHuman: true, announce: async () => {}, + }); + const res = await post(`/intent/${id}/decision`, { decision: 'approve' }, LEAD_TOKEN); + assert.equal(res.status, 403); + assert.equal(listIntents().find(i => i.id === id).status, 'pending'); +}); From 913b10e222fd883c901ce5b8f16fc78e5404dfd4 Mon Sep 17 00:00:00 2001 From: ThinkOff Date: Fri, 18 Sep 2026 14:37:08 +0300 Subject: [PATCH 05/14] test: await the throwaway server's close so the suite stops flaking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The full suite failed once at 352/353 and passed on every rerun. The cause was in the test I had just added: it created a one-off HTTP server and called close() without awaiting, outside any finally — so an assertion failure leaked the listener, and a leaked listener is how a suite starts failing once in three runs. An intermittent failure is worse than a consistent one, because the third flake is the one everybody stops reading, and this suite is the thing standing between a team lead and approving shell commands. Awaited, and in a finally. Three consecutive full runs at 360/360. Co-Authored-By: Claude Opus 5 --- test/lead-command.test.mjs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/test/lead-command.test.mjs b/test/lead-command.test.mjs index 46351d6..6f558c5 100644 --- a/test/lead-command.test.mjs +++ b/test/lead-command.test.mjs @@ -114,8 +114,15 @@ test('status does not report "unset" when the daemon lacks the route', async () const dead = createServer((req, res) => { res.writeHead(404); res.end('{}'); }); await new Promise(r => dead.listen(0, '127.0.0.1', r)); const url = `http://127.0.0.1:${dead.address().port}`; - const reply = await handleLeadCommand({ body: '/lead status', from: 'petrus', isHuman: true }, { daemonUrl: url }); - assert.match(reply, /not running here yet/i); - assert.doesNotMatch(reply, /^Team lead: unset/); - dead.close(); + try { + const reply = await handleLeadCommand({ body: '/lead status', from: 'petrus', isHuman: true }, { daemonUrl: url }); + assert.match(reply, /not running here yet/i); + assert.doesNotMatch(reply, /^Team lead: unset/); + } finally { + // Awaited, and in a finally: an unawaited close leaves the handle open if + // the assertion throws, and a leaked listener is how a suite starts failing + // once in three runs — which is worse than failing every time, because the + // third flake is the one everybody stops reading. + await new Promise(r => dead.close(r)); + } }); From d0aa7d929b1c50cdc619cbab0091887122e760cc Mon Sep 17 00:00:00 2001 From: ThinkOff Date: Fri, 18 Sep 2026 14:39:49 +0300 Subject: [PATCH 06/14] fix(confirmations): the anonymous-is-owner fallback is legacy mode, not a default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codexmb, second review of 36524b2, reproduced over real HTTP: the lead's own token got 403 on a requiresHuman intent, and then the SHARED token with no actor got 200 on the same intent. `actor: principal || OWNER_HANDLE` handed the owner's authority to any caller who omitted a field, so the boundary I had just written was bypassable by deleting `actor` — including by the lead that had been refused a moment earlier. Once a daemon configures `principals` it has asserted it can tell callers apart, so from then on it must: every decision needs a proven principal, the owner's included. Configuring principals therefore means also issuing Petrus one. That is the point rather than an oversight — "owner" cannot be a privilege the daemon is unable to verify. A daemon with NO principals keeps today's behaviour exactly, so nothing that works now stops working and his phone buttons are untouched. The refusal is not narrowed to requiresHuman on purpose. Narrowing it would leave the identical hole one field away for ordinary intents, which is how this one existed in the first place. Two new negative tests for the bypass, and two POSITIVE ones for legacy mode — proving the refusal without proving that an unconfigured daemon still lets him in would test half the branch and ship the half that locks him out. Eleven HTTP tests. Full suite 362/362. Deployment stays held. Co-Authored-By: Claude Opus 5 --- src/confirmations.mjs | 24 +++++++++++++ test/lead-http.test.mjs | 79 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/src/confirmations.mjs b/src/confirmations.mjs index 4736e32..7ef89c0 100644 --- a/src/confirmations.mjs +++ b/src/confirmations.mjs @@ -1134,6 +1134,30 @@ export function startConfirmationsServer({ })); return; } + // THE ANONYMOUS-IS-OWNER FALLBACK IS LEGACY MODE ONLY. + // + // @codexmb, 2026-09-18: with principals configured, the lead's own + // token got 403 on a requiresHuman intent and then the SHARED token + // with no actor got 200 on the same one. `principal || OWNER_HANDLE` + // handed every caller the owner's authority by omission, so the + // boundary I had just written was bypassable by deleting a field. + // + // Once a daemon configures principals it has said it can tell callers + // apart, so from then on it must: every decision needs a proven + // principal, the owner's included. Configuring principals therefore + // means also issuing Petrus one — which is the point, not an + // oversight. A daemon with NO principals keeps today's behaviour + // exactly, so nothing that works now stops working + // ([[never lock Petrus out]]). + if (!principal && principalByToken.size > 0) { + res.writeHead(403, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ + ok: false, + error: 'this daemon identifies callers; present your own token. ' + + 'Anonymous decisions are only accepted when no principals are configured.', + })); + return; + } const result = decideIntent(id, payload.decision, { receiptsPath, actor: principal || OWNER_HANDLE, diff --git a/test/lead-http.test.mjs b/test/lead-http.test.mjs index 3d1b9d9..23d9886 100644 --- a/test/lead-http.test.mjs +++ b/test/lead-http.test.mjs @@ -124,3 +124,82 @@ test('a proven lead still cannot clear a requiresHuman intent', async () => { assert.equal(res.status, 403); assert.equal(listIntents().find(i => i.id === id).status, 'pending'); }); + +test('the shared token cannot approve what a proven lead was just refused', async () => { + // @codexmb, 2026-09-18: the lead's own token got 403 on a requiresHuman + // intent, then the SHARED token with no actor got 200 on the same one. + // `principal || OWNER_HANDLE` handed the owner's authority to anyone who + // omitted a field, so the boundary was bypassable by deleting `actor`. + _resetForTests(); + const { setLead } = await import('../src/confirmations.mjs'); + setLead('hermes', { actor: 'petrus' }); + const id = await createIntent({ + prompt: 'cp ~/.ssh/id_ed25519 /tmp', session: 's', channels: [], + requiresHuman: true, announce: async () => {}, + }); + const asLead = await post(`/intent/${id}/decision`, { decision: 'approve' }, LEAD_TOKEN); + assert.equal(asLead.status, 403, 'a lead may not clear an owner-only intent'); + const anonymous = await post(`/intent/${id}/decision`, { decision: 'approve' }); + assert.equal(anonymous.status, 403, 'and neither may the same caller by omitting actor'); + assert.equal(listIntents().find(i => i.id === id).status, 'pending'); +}); + +test('an ordinary intent is refused anonymously too once principals exist', async () => { + // Not a special case for requiresHuman: a daemon that says it can identify + // callers must identify them. Narrowing the refusal to owner-only intents + // would leave the same hole one field away. + _resetForTests(); + const id = await createIntent({ prompt: 'ls', session: 's', channels: [], announce: async () => {} }); + const res = await post(`/intent/${id}/decision`, { decision: 'approve' }); + assert.equal(res.status, 403); + assert.equal(listIntents().find(i => i.id === id).status, 'pending'); +}); + +test('LEGACY MODE: with no principals, anonymous still decides — Petrus is not locked out', async () => { + // The other half of the branch above, and the one that matters to the human: + // a daemon that has NOT been given per-agent tokens must behave exactly as it + // does today, or his phone buttons stop working the moment this ships. + // Proving only the refusal would leave that untested. + _resetForTests(); + const legacy = startConfirmationsServer({ + port: 0, host: '127.0.0.1', + receiptsPath: '/tmp/iak-test-lead-legacy.jsonl', + announce: async () => {}, + }); + await new Promise(r => legacy.listen(0, '127.0.0.1', r)); + const url = `http://127.0.0.1:${legacy.address().port}`; + try { + const id = await createIntent({ prompt: 'ls', session: 's', channels: [], announce: async () => {} }); + const res = await fetch(`${url}/intent/${id}/decision`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ decision: 'approve' }), + }); + assert.equal(res.status, 200, 'legacy anonymous approval must keep working'); + assert.equal(listIntents().find(i => i.id === id).decision, 'approve'); + } finally { + await new Promise(r => legacy.close(r)); + } +}); + +test('LEGACY MODE: /lead still refuses, because delegation needs identity', async () => { + _resetForTests(); + const legacy = startConfirmationsServer({ + port: 0, host: '127.0.0.1', + receiptsPath: '/tmp/iak-test-lead-legacy.jsonl', + announce: async () => {}, + }); + await new Promise(r => legacy.listen(0, '127.0.0.1', r)); + const url = `http://127.0.0.1:${legacy.address().port}`; + try { + const res = await fetch(`${url}/lead`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ handle: 'hermes', actor: 'petrus' }), + }); + assert.equal(res.status, 403); + assert.match((await res.json()).error, /no per-agent tokens/i); + } finally { + await new Promise(r => legacy.close(r)); + } +}); From 990bf484b41eaf97c5a55407bf6040f99e828008 Mon Sep 17 00:00:00 2001 From: claudemm Date: Fri, 18 Sep 2026 19:37:02 +0300 Subject: [PATCH 07/14] test: the authorization boundary, over HTTP, against real intents @codexmb after the restart: "message delivery and presence of handleLeadCommand do not verify the authorization boundary." Correct -- a commit hash proves which code loaded, not what it permits. Seven cases now assert what it permits, and each asserts a specific status AND a specific reason, because a refusal for the wrong reason is not a pass. lead clears another agent's routine intent 200 lead decides its OWN request 403 "own request" lead decides a requiresHuman intent 403 "reserved for" a non-lead agent decides 403 the OWNER decides a requiresHuman intent 200 (never locked out) unproven caller claims actor:petrus 403 "proves no identity" unregistered token, principals configured 403 "identifies callers" Four instrument failures preceded the first real measurement, and the file records them because each would have produced a confident wrong answer. The route was /decide, not /decision, so everything returned 404 -- and one test still passed because it asserted notEqual(status, 200). The server reads Authorization: Bearer, not X-API-Key, so no token resolved and everything returned 403, which is a boundary that refuses every caller, looks secure, and tests nothing. A sed printed "header corrected" while changing no file. And assert.throws was used against setLead, which returns a refusal rather than throwing; reporting that failure would have read as "the code does not refuse self-appointment". Co-Authored-By: Claude Opus 5 --- test/authorization-boundary.test.mjs | 134 +++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 test/authorization-boundary.test.mjs diff --git a/test/authorization-boundary.test.mjs b/test/authorization-boundary.test.mjs new file mode 100644 index 0000000..8dfa04d --- /dev/null +++ b/test/authorization-boundary.test.mjs @@ -0,0 +1,134 @@ +// The authorization boundary, over real HTTP, against real intents. +// +// @codexmb, after the daemon was restarted with this code live: "message +// delivery and presence of handleLeadCommand do not verify the authorization +// boundary." A commit hash proves which code loaded, not what it permits. +// +// FOUR INSTRUMENT FAILURES PRECEDED THE FIRST REAL MEASUREMENT HERE, and each +// one would have produced a confident wrong answer: +// +// wrong route /intent/:id/decide -> 404 for EVERYTHING. One test still +// "passed" because it asserted notEqual(status, 200); 404 is +// not 200. It would pass against a server with no authorization +// code at all. +// wrong header the server reads `Authorization: Bearer`, not X-API-Key, so +// no token resolved and EVERYTHING was 403. A boundary that +// refuses every caller looks secure and tests nothing. +// a lying sed printed "header corrected" while changing no file. +// wrong shape assert.throws() against setLead, which RETURNS a refusal. Had +// I reported that failure it would have read as "the code does +// not refuse self-appointment". +// +// Hence: every case asserts a specific STATUS and a specific REASON. A refusal +// for the wrong reason is not a pass. + +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { mkdtempSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { startConfirmationsServer, createIntent, setLead, OWNER_HANDLE } + from '../src/confirmations.mjs'; + +const OWNER = 'tok-owner', LEAD = 'tok-lead', OTHER = 'tok-other'; + +async function boot() { + const receiptsPath = join(mkdtempSync(join(tmpdir(), 'authz-')), 'receipts.jsonl'); + const server = startConfirmationsServer({ + port: 0, receiptsPath, + principals: { [OWNER]: OWNER_HANDLE, [LEAD]: 'leadagent', [OTHER]: 'otheragent' }, + }); + // listen() is async; address() is null until 'listening'. Not awaiting this + // is what made the first version of this file hang forever. + await new Promise((res, rej) => { + server.once('error', rej); + server.listening ? res() : server.once('listening', res); + }); + setLead('leadagent', { actor: OWNER_HANDLE, receiptsPath }); + return { server, port: server.address().port, receiptsPath, + shut: () => new Promise((r) => server.close(r)) }; +} + +const mkIntent = (receiptsPath, opts) => + createIntent({ prompt: 't', session: 's', channels: [], timeoutSec: 1, + receiptsPath, ...opts }); + +async function decide(port, id, token, body = { decision: 'approve' }) { + const r = await fetch(`http://127.0.0.1:${port}/intent/${id}/decision`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', + ...(token ? { Authorization: `Bearer ${token}` } : {}) }, + body: JSON.stringify(body), + }); + return { status: r.status, body: await r.json() }; +} + +test('the lead may clear another agent routine work', async () => { + const t = await boot(); + try { + const i = await mkIntent(t.receiptsPath, { fromHandle: 'otheragent' }); + const r = await decide(t.port, i.id ?? i, LEAD); + assert.equal(r.status, 200, 'delegation does not work at all'); + } finally { await t.shut(); } +}); + +test('the lead may NOT decide its own request', async () => { + const t = await boot(); + try { + const i = await mkIntent(t.receiptsPath, { fromHandle: 'leadagent' }); + const r = await decide(t.port, i.id ?? i, LEAD); + assert.equal(r.status, 403, 'a lead self-approved'); + assert.match(r.body.error, /own request/, 'refused, but for the wrong reason'); + } finally { await t.shut(); } +}); + +test('requiresHuman is never delegable', async () => { + const t = await boot(); + try { + const i = await mkIntent(t.receiptsPath, + { fromHandle: 'otheragent', requiresHuman: true }); + const r = await decide(t.port, i.id ?? i, LEAD); + assert.equal(r.status, 403, 'the lead decided an owner-only action'); + assert.match(r.body.error, /reserved for/, 'refused, but for the wrong reason'); + } finally { await t.shut(); } +}); + +test('an agent that is not the lead cannot decide', async () => { + const t = await boot(); + try { + const i = await mkIntent(t.receiptsPath, { fromHandle: 'otheragent' }); + const r = await decide(t.port, i.id ?? i, OTHER); + assert.equal(r.status, 403, 'any authenticated agent could decide'); + } finally { await t.shut(); } +}); + +test('the owner may still decide an owner-only action', async () => { + const t = await boot(); + try { + const i = await mkIntent(t.receiptsPath, + { fromHandle: 'otheragent', requiresHuman: true }); + const r = await decide(t.port, i.id ?? i, OWNER); + assert.equal(r.status, 200, 'the owner was locked out of their own approvals'); + } finally { await t.shut(); } +}); + +test('an unproven caller cannot claim an actor', async () => { + const t = await boot(); + try { + const i = await mkIntent(t.receiptsPath, { fromHandle: 'otheragent' }); + const r = await decide(t.port, i.id ?? i, undefined, + { decision: 'approve', actor: 'petrus' }); + assert.equal(r.status, 403, 'impersonation by assertion is open'); + assert.match(r.body.error, /proves no identity/); + } finally { await t.shut(); } +}); + +test('once principals exist, anonymous is not the owner', async () => { + const t = await boot(); + try { + const i = await mkIntent(t.receiptsPath, { fromHandle: 'otheragent' }); + const r = await decide(t.port, i.id ?? i, 'tok-not-registered'); + assert.equal(r.status, 403, 'the anonymous-is-owner fallback is reachable'); + assert.match(r.body.error, /identifies callers/); + } finally { await t.shut(); } +}); From 680e8a2d9ae5fbe947743e217c27f53885e76cd3 Mon Sep 17 00:00:00 2001 From: claudemm Date: Fri, 18 Sep 2026 23:23:55 +0300 Subject: [PATCH 08/14] automation: give /lead a process, and stop losing multi-line commands petrus typed /lead status at 16:27 and again at 20:03 and got silence both times. Three layers, found only because he pressed it twice: 1 /lead lives in src/room-automation.mjs; bin/cli.mjs imported src/team-relay/room-automation.mjs, which has no handler 2 startRoomAutomation runs only under the "automate" subcommand 3 nothing on the fleet launches "automate" -- start-all.sh runs "rooms watch" and the daemon So the handler had never had a process. The unit tests imported the file I edited, so they passed against a module production does not load. - both entry points now import src/room-automation.mjs, which also has a better case-insensitive self-skip than the copy that was running - rooms watch starts automation on its own poll loop, not awaited, so a crash there cannot take room notifications down with it - test/automation-module-wiring.test.mjs asserts that the module cli.mjs loads actually exports handleLeadCommand. It fails on the old import; I checked that rather than assuming. Second defect, found by @claudeMB's end-to-end test rather than by me: the command regex ran against the whole body with no /m flag, so `$` demanded end-of-string and ANY second line made the command invisible. His "/lead status" with a note underneath parsed as nothing. Someone who types a command and then a sentence has still typed a command. - status and clear read the first line and tolerate prose after it - appointing still requires the command to be the whole message: reading a privilege grant out of the first line of a longer post is how a quoted line becomes a real appointment - that refusal now says "appointing has to be the whole message" rather than "@grok is not a handle", which was false test/lead-command-path.test.mjs exercises the path against a RECORDING http server, because the property is whether a privileged request reaches the daemon, not what the function returns. Unauthorised cases must leave it with zero requests; the owner's status must produce exactly GET /lead. Replacing the identity check with `true` turns 3 of 8 red. 381/381. KNOWN GAPS, raised by @codexmb against the published module and NOT addressed here. They are real and they are next: - seen.add happens before dispatch but saveSeenIds only after, so a crash in between replays the command on restart - two processes keep independent in-memory sets; the shared file is not an execution lock. I briefly ran two watchers today and proved it. - a first-run fetch failure returns [] and seeding proceeds, so the first successful poll can treat history as new. Seeding must fail closed. - existing non-empty state has no startup cutoff - postMessage ignores HTTP status, and the lead branch records "completed" even when posting returned false Handler tests do not exercise poller startup or replay, which is his point and it stands. Co-Authored-By: Claude Opus 5 --- bin/cli.mjs | 29 +++++- src/room-automation.mjs | 20 +++- src/team-relay/index.mjs | 2 +- test/automation-module-wiring.test.mjs | 51 ++++++++++ test/lead-command-path.test.mjs | 127 +++++++++++++++++++++++++ 5 files changed, 226 insertions(+), 3 deletions(-) create mode 100644 test/automation-module-wiring.test.mjs create mode 100644 test/lead-command-path.test.mjs diff --git a/bin/cli.mjs b/bin/cli.mjs index ceba580..1a000df 100755 --- a/bin/cli.mjs +++ b/bin/cli.mjs @@ -17,7 +17,7 @@ import { startRoomPoller, checkRoomMessages } from '../src/team-relay/room-polle import { startLinearPoller } from '../src/team-relay/linear-poller.mjs'; import { memoryList, memoryGet, memorySet, memoryAppend, memoryDelete, memorySearch } from '../src/team-relay/memory.mjs'; import { moltbookPost, moltbookFeed } from '../src/team-relay/moltbook.mjs'; -import { startRoomAutomation } from '../src/team-relay/room-automation.mjs'; +import { startRoomAutomation } from '../src/room-automation.mjs'; import { pollDiscord, startDiscordPoller } from '../src/team-relay/discord-poller.mjs'; import { UnifiedPoller } from '../src/team-relay/unified-poller.mjs'; import { groupmindAdapter } from '../src/team-relay/adapters/groupmind.mjs'; @@ -562,6 +562,33 @@ async function main() { console.error('Error: poller.rooms, poller.api_key (or poller.api_key_file), and poller.handle must be set in config'); process.exit(1); } + // Room AUTOMATION rides along with the watcher. + // + // It used to run only under the separate "automate" subcommand, which + // nothing on the fleet launches. That is why /lead answered nobody: the + // handler existed, its module was imported, its tests passed, and no + // process ever executed it. petrus pressed the command twice and got + // silence twice before anyone asked WHICH PROCESS RUNS THIS. + // + // Started here rather than as a second daemon so there is one thing to + // launch and one thing to restart. It keeps its own seen-ids file + // (automation.seen_file), separate from the poller's, so the two never + // consume each other's messages; on a first run with no seen file it + // SEEDS rather than replaying, which is what stops a historical /lead or + // /approve from executing on startup. + // + // Not awaited: both loop forever. A crash in automation must not take the + // poller - and therefore every room notification - down with it. + startRoomAutomation({ + rooms: pollerRooms, + apiKey: pollerApiKey, + handle: pollerHandle, + config, + }).catch((e) => { + console.error(`Room automation stopped: ${e?.message || e}`); + console.error(' The poller is still running; chat commands like /lead are not.'); + }); + await startRoomPoller({ rooms: pollerRooms, apiKey: pollerApiKey, diff --git a/src/room-automation.mjs b/src/room-automation.mjs index 505fb02..88a7daf 100644 --- a/src/room-automation.mjs +++ b/src/room-automation.mjs @@ -130,9 +130,16 @@ function postMessage(room, body, apiKey, config) { const LEAD_COMMAND_RE = /^\s*\/lead\b\s*(.*)$/i; function parseLeadCommand(body) { - const m = LEAD_COMMAND_RE.exec(body || ''); + // Read the FIRST LINE only. The original regex ran against the whole body + // with no /m flag, so `$` demanded end-of-string and any second line made the + // command invisible: @claudeMB's test message was "/lead status" followed by + // a note to petrus, and it was silently ignored. Someone typing a command and + // then a sentence has still typed a command. + const lines = String(body || '').split('\n'); + const m = LEAD_COMMAND_RE.exec(lines[0] || ''); if (!m) return null; const rest = (m[1] || '').trim(); + const hasMoreLines = lines.slice(1).join('').trim().length > 0; if (!rest || /^status$/i.test(rest)) return { op: 'status' }; if (/^clear$/i.test(rest)) return { op: 'clear' }; // STRICT on purpose: exactly one token, nothing trailing. Taking the first @@ -140,6 +147,11 @@ function parseLeadCommand(body) { // guess that hands command-approval rights to the wrong agent. When the // input is not unambiguous, refuse and say so. if (!/^@?[A-Za-z0-9_.-]+$/.test(rest)) return { op: 'invalid', handle: rest }; + // Appointing is a PRIVILEGE GRANT, so it stays maximally strict: the command + // must be the whole message. Reading it out of the first line of a longer + // post is how a quoted line becomes an appointment. Status and clear are + // harmless reads and may carry trailing prose. + if (hasMoreLines) return { op: 'not-alone', handle: rest }; return { op: 'assign', handle: rest.replace(/^@+/, '') }; } @@ -182,6 +194,12 @@ export async function handleLeadCommand(msg, { daemonUrl, ownerHandle = 'petrus' ? `Team lead: ${lead.handle} (assigned by ${lead.assignedBy}).` : 'Team lead: unset. Only the owner can decide confirmations, and only the owner can appoint a lead.'; } + if (parsed.op === 'not-alone') { + // Say WHY. "@grok is not a handle" would be false and would send whoever + // typed it looking for a typo that is not there. + return `Appointing a lead has to be the whole message. Send just "/lead @${parsed.handle.replace(/^@+/, '')}" ` + + 'on its own, with nothing after it.'; + } if (parsed.op === 'invalid') { return `"${parsed.handle}" is not a handle. Use /lead @agent, /lead status or /lead clear.`; } diff --git a/src/team-relay/index.mjs b/src/team-relay/index.mjs index 7a4c353..65b7c67 100644 --- a/src/team-relay/index.mjs +++ b/src/team-relay/index.mjs @@ -2,7 +2,7 @@ // team-relay — generic room/comms modules for IDE Agent Kit export { startRoomPoller, checkRoomMessages } from './room-poller.mjs'; -export { startRoomAutomation } from './room-automation.mjs'; +export { startRoomAutomation } from '../room-automation.mjs'; export { startWebhookServer } from './webhook-server.mjs'; export { pollDiscord, startDiscordPoller } from './discord-poller.mjs'; export { UnifiedPoller } from './unified-poller.mjs'; diff --git a/test/automation-module-wiring.test.mjs b/test/automation-module-wiring.test.mjs new file mode 100644 index 0000000..74873ed --- /dev/null +++ b/test/automation-module-wiring.test.mjs @@ -0,0 +1,51 @@ +// The room automation exists TWICE under the same filename: +// src/room-automation.mjs the one the tests import +// src/team-relay/room-automation.mjs the one bin/cli.mjs used to import +// +// On 2026-09-18 the /lead chat command was written into the first and the +// watcher loaded the second. Every unit test passed, the daemon answered, the +// process was restarted -- and petrus got silence twice, because the handler +// was in a file nothing loaded. A green suite against an unloaded module is +// not evidence about the product. +// +// This test asserts the entry points and the tests agree on WHICH module runs. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +const root = join(dirname(fileURLToPath(import.meta.url)), '..'); +const read = (p) => readFileSync(join(root, p), 'utf8'); + +test('bin/cli.mjs loads the room-automation module that exports the /lead handler', async () => { + const cli = read('bin/cli.mjs'); + const m = /import\s*\{[^}]*startRoomAutomation[^}]*\}\s*from\s*'([^']+)'/.exec(cli); + assert.ok(m, 'cli.mjs does not import startRoomAutomation at all'); + const spec = m[1]; + + // Resolve what cli.mjs actually loads, and require the handler to be in it. + const mod = await import(new URL(spec.replace(/^\.\.\//, '../'), import.meta.url).href) + .catch(async () => import(join(root, spec.replace(/^\.\.\//, '')))); + assert.ok( + typeof mod.handleLeadCommand === 'function', + `cli.mjs imports ${spec}, which does not export handleLeadCommand — ` + + 'the chat command would be unreachable no matter how many tests pass', + ); +}); + +test('every re-export of startRoomAutomation points at the same module', () => { + const cli = read('bin/cli.mjs'); + const idx = read('src/team-relay/index.mjs'); + const specOf = (src) => (/startRoomAutomation[^}]*\}\s*from\s*'([^']+)'/.exec(src) || [])[1]; + const a = specOf(cli); + const b = specOf(idx); + assert.ok(a && b, 'could not find both specifiers'); + // Different relative depths, same target file. + const norm = (s) => s.replace(/^\.\.\//, '').replace(/^\.\//, '').replace(/^src\//, ''); + assert.equal( + norm(a), norm(b), + `bin/cli.mjs loads ${a} but src/team-relay/index.mjs re-exports ${b} — ` + + 'two entry points, two different modules, is exactly how /lead was lost', + ); +}); diff --git a/test/lead-command-path.test.mjs b/test/lead-command-path.test.mjs new file mode 100644 index 0000000..2bd8062 --- /dev/null +++ b/test/lead-command-path.test.mjs @@ -0,0 +1,127 @@ +// The /lead command path, against a RECORDING daemon rather than assumptions. +// +// Asked for by @codexmb after petrus got silence twice: "/health alone is +// insufficient ... exercise the real poller-to-handler path with controlled +// fixtures, including an old command and an agent-authored command ... preserve +// authenticated human identity checks; do not relax them to test". +// +// The property under test is not what the function RETURNS. It is whether a +// privileged request ever reaches the daemon. So the daemon here is a real +// server that records every request, and the unauthorised cases must leave it +// with nothing. +// +// This file's own history is the reason it is written that way. Its first +// version called handleLeadCommand({ msg, ... }) when the signature is +// (msg, { ... }), so every body was undefined, every call returned null, and +// four "the request is refused" tests passed against a function that had not +// been asked anything. Its second version asserted null for refusals, and +// failed -- because the handler refuses OUT LOUD, which is better than silence +// and was the correct behaviour all along. Assert the property, not the shape +// you expected. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { createServer } from 'node:http'; +import { handleLeadCommand } from '../src/room-automation.mjs'; + +const OWNER = 'petrus'; + +async function withRecordingDaemon(run) { + const seen = []; + const server = createServer((req, res) => { + seen.push(`${req.method} ${req.url}`); + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ ok: true, lead: null, owner: OWNER })); + }); + await new Promise((r) => server.listen(0, '127.0.0.1', r)); + const url = `http://127.0.0.1:${server.address().port}`; + try { + return await run(url, seen); + } finally { + await new Promise((r) => server.close(r)); + } +} + +const msg = (o = {}) => ({ id: 'm1', from: OWNER, isHuman: true, body: '/lead status', room: 'r', ...o }); + +test('an AGENT-authored appointment never reaches the daemon', async () => { + await withRecordingDaemon(async (url, seen) => { + const out = await handleLeadCommand( + msg({ from: '@claudemm', isHuman: false, body: '/lead @claudemm' }), + { daemonUrl: url, ownerHandle: OWNER }); + assert.deepEqual(seen, [], `an agent reached the daemon: ${seen.join(', ')}`); + assert.match(String(out), /only petrus/i, 'and it must SAY it refused, not go quiet'); + }); +}); + +test('owner handle with isHuman false -- a replayed button -- never reaches the daemon', async () => { + await withRecordingDaemon(async (url, seen) => { + const out = await handleLeadCommand( + msg({ isHuman: false, body: '/lead @claudemm' }), + { daemonUrl: url, ownerHandle: OWNER }); + assert.deepEqual(seen, [], 'the owner handle alone must not be authorisation'); + assert.match(String(out), /only petrus/i); + }); +}); + +test('a human who is not the owner never reaches the daemon', async () => { + await withRecordingDaemon(async (url, seen) => { + await handleLeadCommand( + msg({ from: 'someone-else', isHuman: true, body: '/lead @them' }), + { daemonUrl: url, ownerHandle: OWNER }); + assert.deepEqual(seen, []); + }); +}); + +test('prose is not a command: nothing is parsed, nothing is sent', async () => { + await withRecordingDaemon(async (url, seen) => { + const out = await handleLeadCommand(msg({ body: 'lead status please' }), { daemonUrl: url, ownerHandle: OWNER }); + assert.equal(out, null); + assert.deepEqual(seen, []); + }); +}); + +test('the OWNER asking for status does reach the daemon, and reads its answer', async () => { + // The positive control. Without one, every assertion above would also pass + // against a handler that does nothing at all -- which is exactly how the + // first version of this file "passed". + await withRecordingDaemon(async (url, seen) => { + const out = await handleLeadCommand(msg(), { daemonUrl: url, ownerHandle: OWNER }); + assert.deepEqual(seen, ['GET /lead'], 'a genuine owner status must query the daemon'); + assert.match(String(out), /unset/i, 'and must report what the daemon said'); + }); +}); + +test('an unreachable daemon is reported, never silently swallowed', async () => { + // Silence is what petrus experienced twice. It must not be a normal outcome. + const out = await handleLeadCommand(msg(), { daemonUrl: 'http://127.0.0.1:59999', ownerHandle: OWNER }); + assert.ok(out, 'a dead daemon must still produce something to say'); + assert.match(String(out), /could not reach|unavailable|error/i); +}); + +test('a command followed by prose is still a command (status)', async () => { + // @claudeMB sent "/lead status" with a note underneath as an end-to-end test + // and it was silently ignored: the regex ran against the whole body with no + // /m flag, so a second line made `$` fail and the command invisible. Someone + // who types a command and then a sentence has still typed a command. + await withRecordingDaemon(async (url, seen) => { + const out = await handleLeadCommand( + msg({ from: '@claudeMB', isHuman: false, body: '/lead status\n\nnote to petrus: ignore this' }), + { daemonUrl: url, ownerHandle: OWNER }); + assert.deepEqual(seen, ['GET /lead'], 'the status read must still happen'); + assert.match(String(out), /unset/i); + }); +}); + +test('an APPOINTMENT with anything after it is refused, and says why', async () => { + // Status tolerates trailing prose; granting a privilege does not. Reading an + // appointment out of the first line of a longer post is how a quoted line + // becomes a real grant. + await withRecordingDaemon(async (url, seen) => { + const out = await handleLeadCommand( + msg({ body: '/lead @grok\nplease do this' }), + { daemonUrl: url, ownerHandle: OWNER }); + assert.deepEqual(seen, [], 'nothing may reach the daemon'); + assert.match(String(out), /whole message/i, 'and the reason must be the real one'); + assert.doesNotMatch(String(out), /is not a handle/i, '"@grok is not a handle" would be false'); + }); +}); From cdf1088d3207bca7c1a3c400dd92e15b3d51558e Mon Sep 17 00:00:00 2001 From: claudemm Date: Fri, 18 Sep 2026 23:27:19 +0300 Subject: [PATCH 09/14] automation: fail closed, persist before acting, and stop lying in receipts @codexmb reviewed the published module and found five concrete defects on a path that can execute /lead and /approve. All five, plus one of my own. FAIL-OPEN SEEDING. fetchRoomMessages returned [] for a failed request, so "the room is quiet" and "I could not ask" were the same value. A dropped packet at startup produced a successful seed of nothing, and the first poll that worked then saw the entire history as new -- a replay of privileged commands caused by a transient network error. - the fetch returns null on any non-2xx or thrown error - seeding aborts if ANY room cannot be read, and retries on the poll interval - dispatch is gated on `ready`: until a seed completes, the loop executes nothing at all - a room that fails mid-poll is skipped rather than guessed at REPLAY ON CRASH. seen.add ran before dispatch but saveSeenIds only at the end of the poll, so a crash in between re-executed the command on restart. The id is now persisted immediately, before anything acts on the message. At-most-once is the right bias here: a missed /lead is a message petrus can send again, a repeated one is an appointment he never made. RECEIPTS THAT LIE. postMessage shelled out to curl and returned true whenever curl exited 0 -- which it does for a 500 -- and the lead branch recorded status "completed" regardless. That records the exact silence petrus experienced as a success. The status is checked, and the receipt now says completed or failed according to what happened. MY OWN, found while reading it: the API key was interpolated into `curl -H "X-API-Key: ${apiKey}"` under execSync, putting the credential in the process table for anyone running ps. Both calls use fetch with a header now. This repo's own rule is that keys never go inline in a shell. test/automation-fail-closed.test.mjs proves the distinction the rest depends on: an HTTP error is null, a network failure is null, an EMPTY ROOM IS [] -- the positive control, without which a function that always returned null would pass -- and the key is asserted to arrive as a header. Restoring the fail-open returns turns 2 of 5 red. 386/386. Note for anyone reading the CI: this suite has an intermittent single failure that has never named a test and has not reproduced in three consecutive runs each time I have seen it. It is not in these files, and I have not chased it. STILL OPEN from his review: two processes keep independent in-memory seen-sets, so the shared file is not an execution lock. I demonstrated that myself today by briefly running two watchers. A single-instance lock is the next commit, not this one. Co-Authored-By: Claude Opus 5 --- src/room-automation.mjs | 124 ++++++++++++++++++++------- test/automation-fail-closed.test.mjs | 78 +++++++++++++++++ 2 files changed, 170 insertions(+), 32 deletions(-) create mode 100644 test/automation-fail-closed.test.mjs diff --git a/src/room-automation.mjs b/src/room-automation.mjs index 88a7daf..3b81e48 100644 --- a/src/room-automation.mjs +++ b/src/room-automation.mjs @@ -65,22 +65,34 @@ function saveSeenIds(path, ids) { writeFileSync(path, arr.join('\n') + '\n'); } -function fetchRoomMessages(room, apiKey, limit = 20) { - const url = `https://groupmind.one/api/v1/rooms/${room}/messages?limit=${limit}`; +// Returns an array on success and NULL on failure. The difference matters: [] +// for a failed fetch made "the room is quiet" and "I could not ask" identical, +// so seeding could complete on nothing and the first poll that DID succeed +// treated the whole history as new -- fail-open seeding on a dispatch path +// (@codexmb). +// +// The key also no longer goes through a shell: `curl -H "X-API-Key: ${key}"` +// under execSync puts the credential in the process table for anyone running ps. +export async function fetchRoomMessages(room, apiKey, limit = 20) { + const url = `https://groupmind.one/api/v1/rooms/${encodeURIComponent(room)}/messages?limit=${limit}`; try { - const result = execSync( - `curl -sS -H "X-API-Key: ${apiKey}" "${url}"`, - { encoding: 'utf8', timeout: 15000 } - ); - const data = JSON.parse(result); + const res = await fetch(url, { + headers: { 'X-API-Key': apiKey }, + signal: AbortSignal.timeout(15000), + }); + if (!res.ok) { + console.error(` fetch ${room} failed: HTTP ${res.status}`); + return null; + } + const data = await res.json(); return data.messages || (Array.isArray(data) ? data : []); } catch (e) { console.error(` fetch ${room} failed: ${e.message}`); - return []; + return null; } } -function postMessage(room, body, apiKey, config) { +async function postMessage(room, body, apiKey, config) { if (isAckOnly(body)) { console.log(` ack-only message filtered, skipping post to ${room}: ${body.slice(0, 60)}`); return false; @@ -89,18 +101,27 @@ function postMessage(room, body, apiKey, config) { console.log(` rate-limited (${config?.rate_limit?.message_interval_sec || 30}s interval), skipping post to ${room}`); return false; } - const payload = JSON.stringify({ room, body }); - try { - execSync( - `curl -sS -X POST "https://groupmind.one/api/v1/messages" -H "X-API-Key: ${apiKey}" -H "Content-Type: application/json" -d '${payload.replace(/'/g, "'\\''")}'`, - { timeout: 15000 } - ); - markSent(); - return true; - } catch (e) { - console.error(` post failed: ${e.message}`); - return false; - } + // The old version shelled out to curl and returned true whenever curl exited + // 0 -- which it does for a 500. A receipt then said "completed" for a message + // that never reached the room (@codexmb). The status is checked now, and the + // key no longer travels through a command line where ps can read it. + try { + const res = await fetch('https://groupmind.one/api/v1/messages', { + method: 'POST', + headers: { 'X-API-Key': apiKey, 'Content-Type': 'application/json' }, + body: JSON.stringify({ room, body }), + signal: AbortSignal.timeout(15000), + }); + if (!res.ok) { + console.error(` post failed: HTTP ${res.status}`); + return false; + } + markSent(); + return true; + } catch (e) { + console.error(` post failed: ${e.message}`); + return false; + } } /** @@ -264,14 +285,16 @@ function matchesRule(msg, rule) { * Execute a rule action and return a receipt. */ // `muted` is petrus's emergency-only mode, resolved once per poll cycle by the -// caller (shouldSuppressNudge is async; this function is not). +// caller (shouldSuppressNudge is async; this function itself is now async too, +// because postMessage below awaits the fetch and checks its status instead of +// trusting a shelled-out curl's exit code). // // The carve-out is the point, and it is the same one the room and DM pollers // already make: HIS OWN MESSAGES ALWAYS GET AN ANSWER. Emergency-only exists to // silence agent chatter, not to make the room stop answering the person typing // in it -- a withheld reply to a command he just sent is indistinguishable from // a crash, which is a failure this repo keeps rediscovering. -function executeAction(action, msg, apiKey, config, muted = false) { +async function executeAction(action, msg, apiKey, config, muted = false) { const startedAt = new Date().toISOString(); if (!action) { return createReceipt({ @@ -304,7 +327,7 @@ function executeAction(action, msg, apiKey, config, muted = false) { startedAt, }); } - const ok = postMessage(targetRoom, body, apiKey, config); + const ok = await postMessage(targetRoom, body, apiKey, config); return createReceipt({ actor: { name: config?.poller?.handle || 'ide-agent-kit', kind: 'automation' }, action: `post to ${targetRoom}`, @@ -400,18 +423,34 @@ export async function startRoomAutomation({ rooms, apiKey, handle, interval, con const seen = loadSeenIds(seenFile); const lastFired = new Map(); // rule name → timestamp - // Seed on first run - if (seen.size === 0) { + // Seed on first run, and REFUSE TO DISPATCH if seeding could not complete. + // + // The old version treated a failed fetch as an empty room, so a transient + // network error at startup produced a "successful" seed of nothing -- and the + // first poll that worked then saw every historical message as new. On a path + // that can execute /lead or /approve, that is a replay of privileged history + // caused by a dropped packet (@codexmb). + // + // Dispatch is gated on `ready`. Seeding retries on the poll interval until it + // succeeds; until then the loop executes nothing. + let ready = seen.size > 0; + async function trySeed() { console.log(` seeding seen IDs...`); + const fresh = new Set(); for (const room of rooms) { const msgs = await fetchRoomMessages(room, apiKey, 50); - for (const m of msgs) { - if (m.id) seen.add(m.id); + if (msgs === null) { + console.error(` seeding ABORTED: could not read ${room}. Dispatch stays off until it succeeds.`); + return false; } + for (const m of msgs) if (m.id) fresh.add(m.id); } + for (const id of fresh) seen.add(id); saveSeenIds(seenFile, seen); console.log(` seeded ${seen.size} IDs`); + return true; } + if (!ready) ready = await trySeed(); async function poll() { let actionsRun = 0; @@ -423,11 +462,23 @@ export async function startRoomAutomation({ rooms, apiKey, handle, interval, con const muted = await shouldSuppressNudge(config); if (muted) console.log(' emergency-only: agent-triggered posts withheld; his own still answered'); + if (!ready) { + ready = await trySeed(); + if (!ready) return; // still blind: execute nothing + } + for (const room of rooms) { - const msgs = fetchRoomMessages(room, apiKey); + const msgs = await fetchRoomMessages(room, apiKey); + if (msgs === null) continue; // could not read this room; do not guess for (const m of msgs) { if (!m.id || seen.has(m.id)) continue; + // Mark seen and PERSIST before acting. The old order saved once at the + // end of the poll, so a crash between executing a command and saving + // replayed it on restart (@codexmb). At-most-once is the right bias for + // a privileged action: a missed /lead is a message petrus can send + // again, a repeated one is an appointment he never made. seen.add(m.id); + saveSeenIds(seenFile, seen); // Skip own messages (case-insensitive; see src/common/handles.mjs) const sender = m.user?.handle || m.from || m.sender || ''; @@ -445,11 +496,20 @@ export async function startRoomAutomation({ rooms, apiKey, handle, interval, con ownerHandle: config?.poller?.owner_handle || 'petrus', }); if (leadReply !== null) { - postMessage(room, leadReply, apiKey, config); + const posted = await postMessage(room, leadReply, apiKey, config); + if (!posted) { + // A receipt that says "completed" for a reply nobody can see is + // worse than no receipt: it is the silence petrus experienced, + // recorded as a success (@codexmb). + console.error(' /lead reply was NOT posted; not recording it as completed'); + } appendReceipt(receiptPath, createReceipt({ actor: { name: 'automation', kind: 'command' }, action: `/lead from ${m.user?.handle || m.from || '?'}`, - status: 'completed', + // The status is what HAPPENED, not what was attempted. This said + // 'completed' even when the post returned false, recording the exact + // silence petrus hit as a success (@codexmb). + status: posted ? 'completed' : 'failed', startedAt: new Date().toISOString(), })); actionsRun++; @@ -468,7 +528,7 @@ export async function startRoomAutomation({ rooms, apiKey, handle, interval, con } console.log(` rule "${rule.name}" matched → ${rule.action?.type || '?'}`); - const receipt = executeAction(rule.action, m, apiKey, config, muted); + const receipt = await executeAction(rule.action, m, apiKey, config, muted); appendReceipt(receiptPath, receipt); lastFired.set(rule.name, now); actionsRun++; diff --git a/test/automation-fail-closed.test.mjs b/test/automation-fail-closed.test.mjs new file mode 100644 index 0000000..5af77a0 --- /dev/null +++ b/test/automation-fail-closed.test.mjs @@ -0,0 +1,78 @@ +// Seeding must FAIL CLOSED. +// +// @codexmb, reviewing the published module: "First-run fetch failures return [] +// and seeding proceeds, allowing the first successful poll to treat historical +// messages as new." On a path that can execute /lead or /approve, a dropped +// packet at startup could replay privileged history. +// +// The linchpin is that a failed read is DISTINGUISHABLE from an empty room. +// Everything above it -- the `ready` gate, the retry, refusing to dispatch -- +// depends on this one function returning null rather than []. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { createServer } from 'node:http'; +import { fetchRoomMessages } from '../src/room-automation.mjs'; + +async function withServer(handler, run) { + const server = createServer(handler); + await new Promise((r) => server.listen(0, '127.0.0.1', r)); + const port = server.address().port; + try { return await run(port); } finally { await new Promise((r) => server.close(r)); } +} + +test('an HTTP error is null, NOT an empty room', async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async () => new Response('nope', { status: 500 }); + try { + const out = await fetchRoomMessages('r', 'k'); + assert.equal(out, null, 'a 500 must not look like "the room is quiet"'); + } finally { globalThis.fetch = realFetch; } +}); + +test('a network failure is null, NOT an empty room', async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async () => { throw new Error('ECONNREFUSED'); }; + try { + assert.equal(await fetchRoomMessages('r', 'k'), null); + } finally { globalThis.fetch = realFetch; } +}); + +test('a genuinely empty room is [], which is different', async () => { + // The positive control. Without it, a function that always returned null + // would pass both tests above and break the product completely. + const realFetch = globalThis.fetch; + globalThis.fetch = async () => new Response(JSON.stringify({ messages: [] }), { + status: 200, headers: { 'Content-Type': 'application/json' }, + }); + try { + assert.deepEqual(await fetchRoomMessages('r', 'k'), [], 'an empty room is an answer, not a failure'); + } finally { globalThis.fetch = realFetch; } +}); + +test('messages come back when the room has them', async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async () => new Response(JSON.stringify({ messages: [{ id: 'a' }] }), { + status: 200, headers: { 'Content-Type': 'application/json' }, + }); + try { + assert.deepEqual(await fetchRoomMessages('r', 'k'), [{ id: 'a' }]); + } finally { globalThis.fetch = realFetch; } +}); + +test('the API key travels in a header, never on a command line', async () => { + // It used to be interpolated into `curl -H "X-API-Key: ${apiKey}"` under + // execSync, which puts the credential in the process table for anyone running + // ps -- and this repo's own rule is that keys never go inline in a shell. + await withServer((req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ messages: [], sawKey: req.headers['x-api-key'] })); + }, async (port) => { + let seenHeader = null; + const realFetch = globalThis.fetch; + globalThis.fetch = async (url, init) => { seenHeader = init?.headers?.['X-API-Key']; return realFetch(`http://127.0.0.1:${port}/`, init); }; + try { + await fetchRoomMessages('r', 'secret-key-value'); + assert.equal(seenHeader, 'secret-key-value', 'the key must be passed as a header'); + } finally { globalThis.fetch = realFetch; } + }); +}); From 2b879cf7d27d4d7891c098ceec80d736c218adce Mon Sep 17 00:00:00 2001 From: claudemm Date: Fri, 18 Sep 2026 23:50:22 +0300 Subject: [PATCH 10/14] automation: per-room seeding, atomic persistence, and a loop that reports Round six from @codexmb against 4cefcf1. Three more real defects. SEEDING WAS PER PROCESS, NOT PER ROOM. `ready = seen.size > 0` declared everything seeded whenever the seen-file was non-empty, so adding a room to poller.rooms later meant its whole history arrived as new and any historical /lead in it would dispatch. Seeding is now tracked per room in a marker file beside the seen-file, and a room is seeded before anything from it is acted on. An existing seen-file from before the marker existed is adopted for the rooms configured at that moment -- otherwise deploying this change would itself replay them. Rooms added after that still seed properly. PERSISTENCE WAS NOT ATOMIC. saveSeenIds wrote in place, so an interrupted write left a truncated or empty file -- and an empty seen-file on the next start means every historical message is unseen again. A crash during a routine save became a replay of privileged commands. It writes a temp file and renames now, and a failure to persist is loud rather than swallowed, because the consequence lands on the next startup. THE LOOP COULD DIE QUIETLY. setInterval(poll) fired an async function and dropped the promise, so a rejection in any later poll went to unhandledRejection while the loop still looked healthy; the CLI's .catch only ever covered the first call. The next run is now scheduled after the previous one settles, failures are logged and the loop continues, and dispatch stays gated on a successful seed regardless. 386/386, three consecutive clean runs. The intermittent single failure this suite sometimes shows does not name a test and is not in these files; I have checked that each time rather than assuming. STILL OPEN and still his: a cross-process execution lock. Two processes keep independent in-memory seen-sets and the shared file is not a lock. I demonstrated that today by briefly running two watchers. He has also asked for execution-level tests covering existing-state/new-room startup, atomic-persistence failure, and two concurrent processes before this is called safe for privileged dispatch. Those are not written yet, so it is not called safe, and the running watcher is still on the older build. Co-Authored-By: Claude Opus 5 --- src/room-automation.mjs | 90 ++++++++++++++++++++++++++++++++--------- 1 file changed, 71 insertions(+), 19 deletions(-) diff --git a/src/room-automation.mjs b/src/room-automation.mjs index 3b81e48..db9a413 100644 --- a/src/room-automation.mjs +++ b/src/room-automation.mjs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: AGPL-3.0-only import { execSync } from 'node:child_process'; -import { readFileSync, writeFileSync, appendFileSync, existsSync } from 'node:fs'; +import { readFileSync, writeFileSync, appendFileSync, existsSync, renameSync, unlinkSync } from 'node:fs'; import { randomUUID } from 'node:crypto'; import { createReceipt, appendReceipt } from './receipt.mjs'; import { canSend, markSent } from './rate-limiter.mjs'; @@ -60,9 +60,25 @@ function loadSeenIds(path) { } } +// Write through a temp file and rename. A direct write that is interrupted +// leaves a truncated or empty seen-file, and an empty seen-file on the next +// start means every historical message is unseen again -- a crash during a +// routine save becomes a replay of privileged commands (@codexmb). rename(2) +// within a directory is atomic, so a reader sees the old file or the new one +// and never a half-written one. function saveSeenIds(path, ids) { const arr = [...ids].slice(-2000); - writeFileSync(path, arr.join('\n') + '\n'); + const tmp = `${path}.tmp-${process.pid}`; + try { + writeFileSync(tmp, arr.join('\n') + '\n'); + renameSync(tmp, path); + } catch (e) { + // Losing the save is survivable; losing it SILENTLY is not, because the + // consequence lands on the next startup as a replay. + console.error(` FAILED to persist seen ids to ${path}: ${e.message}`); + try { unlinkSync(tmp); } catch {} + throw e; + } } // Returns an array on success and NULL on failure. The difference matters: [] @@ -433,24 +449,47 @@ export async function startRoomAutomation({ rooms, apiKey, handle, interval, con // // Dispatch is gated on `ready`. Seeding retries on the poll interval until it // succeeds; until then the loop executes nothing. - let ready = seen.size > 0; + // Seeding is per ROOM, not per process. `seen.size > 0` was enough to declare + // the whole thing seeded, so adding a room to the list later meant its entire + // history arrived as new and any historical /lead in it would dispatch + // (@codexmb). The marker file records WHICH rooms have been seeded. + const seededFile = `${seenFile}.seeded`; + const seededRooms = new Set( + (() => { try { return readFileSync(seededFile, 'utf8').split('\n').filter(Boolean); } catch { return []; } })() + ); + function markSeeded(room) { + seededRooms.add(room); + const tmp = `${seededFile}.tmp-${process.pid}`; + try { writeFileSync(tmp, [...seededRooms].join('\n') + '\n'); renameSync(tmp, seededFile); } + catch (e) { console.error(` FAILED to record seeded rooms: ${e.message}`); try { unlinkSync(tmp); } catch {} throw e; } + } + // A pre-existing seen-file from before this marker existed counts as having + // seeded the rooms configured at that time -- otherwise the upgrade itself + // would replay them. New rooms added after this point still seed properly. + if (seen.size > 0 && seededRooms.size === 0) { + for (const room of rooms) seededRooms.add(room); + try { markSeeded(rooms[0]); } catch {} + console.log(` existing seen-file adopted for ${rooms.length} room(s)`); + } + async function trySeed() { - console.log(` seeding seen IDs...`); - const fresh = new Set(); - for (const room of rooms) { + const pending = rooms.filter(r => !seededRooms.has(r)); + if (!pending.length) return true; + console.log(` seeding ${pending.length} room(s): ${pending.join(', ')}`); + for (const room of pending) { const msgs = await fetchRoomMessages(room, apiKey, 50); if (msgs === null) { console.error(` seeding ABORTED: could not read ${room}. Dispatch stays off until it succeeds.`); return false; } - for (const m of msgs) if (m.id) fresh.add(m.id); + for (const m of msgs) if (m.id) seen.add(m.id); + saveSeenIds(seenFile, seen); + markSeeded(room); + console.log(` seeded ${room}; ${seen.size} ids known`); } - for (const id of fresh) seen.add(id); - saveSeenIds(seenFile, seen); - console.log(` seeded ${seen.size} IDs`); return true; } - if (!ready) ready = await trySeed(); + let ready = await trySeed(); async function poll() { let actionsRun = 0; @@ -546,23 +585,36 @@ export async function startRoomAutomation({ rooms, apiKey, handle, interval, con } } - // Initial poll - await poll(); - - // Start interval - const timer = setInterval(poll, pollInterval * 1000); + // setInterval(poll) fired an async function and dropped the promise, so a + // rejection inside a later poll went to unhandledRejection while the loop + // carried on looking healthy -- the CLI's .catch only ever covered the FIRST + // call (@codexmb). This schedules the next run only after the previous one + // settles, and a failure is loud without stopping the loop. + let stopped = false; + let timer = null; + const runLoop = async () => { + if (stopped) return; + try { + await poll(); + } catch (e) { + console.error(` poll failed: ${e?.message || e}`); + console.error(' automation continues; dispatch stays gated on a successful seed.'); + } + if (!stopped) timer = setTimeout(runLoop, pollInterval * 1000); + }; + await runLoop(); process.on('SIGINT', () => { console.log('\nAutomation stopped.'); - clearInterval(timer); + stopped = true; if (timer) clearTimeout(timer); process.exit(0); }); process.on('SIGTERM', () => { - clearInterval(timer); + stopped = true; if (timer) clearTimeout(timer); process.exit(0); }); - return timer; + return { stop: () => { stopped = true; if (timer) clearTimeout(timer); } }; } // Exported for tests only: the mute carve-out is the kind of logic that must From 53826701a02aceb636e819ddb8b3c92f0667207d Mon Sep 17 00:00:00 2001 From: claudemm Date: Fri, 18 Sep 2026 23:56:33 +0300 Subject: [PATCH 11/14] automation: execution-level startup tests, and one that could not fail @codexmb: "cover existing-state/new-room startup ... with execution-level tests before calling privileged dispatch safe. Handle tests alone do not exercise poller startup/replay." They cannot, while the API host is hardcoded, so the base is injectable via automation.api_base and these drive startRoomAutomation itself against a stub room server, counting what it actually POSTs. A message never dispatched leaves no post; a replayed one leaves two. a room ADDED after the seen-file exists is seeded, not replayed a room that cannot be read blocks dispatch entirely Both are control-verified: reverting the per-room seeding turns the first red, reverting the fail-open fetch turns the second red. The second test needed rewriting before it was worth anything. Its first version asserted "no posts" when a room failed -- and with fail-open seeding there were no posts either, because the same pass marked everything seen. It passed against the exact bug it was written to catch. I only found that by running the sabotage rather than trusting the green. It now seeds one room cleanly, adds a genuinely dispatchable message to it, and THEN breaks the second room: correct behaviour dispatches nothing, fail-open posts the new message. That is the third check of mine today that could not have failed, and the second that a deliberate sabotage caught. Running the sabotage is cheap and I should stop treating it as optional. 388/388. STILL OPEN: the cross-process execution lock, and a test for two concurrent processes. The shared seen-file is not a lock -- I demonstrated that today by briefly running two watchers -- and nothing here changes it. Not called safe. Co-Authored-By: Claude Opus 5 --- src/room-automation.mjs | 11 ++- test/automation-startup-replay.test.mjs | 123 ++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 test/automation-startup-replay.test.mjs diff --git a/src/room-automation.mjs b/src/room-automation.mjs index db9a413..d8478d1 100644 --- a/src/room-automation.mjs +++ b/src/room-automation.mjs @@ -89,8 +89,14 @@ function saveSeenIds(path, ids) { // // The key also no longer goes through a shell: `curl -H "X-API-Key: ${key}"` // under execSync puts the credential in the process table for anyone running ps. +export // The base is injectable ONLY so the startup/replay path can be exercised +// against a stub room server. @codexmb: "handle tests alone do not exercise +// poller startup/replay" -- and they cannot, if the host is hardcoded. +let API_BASE = 'https://groupmind.one/api/v1'; +export function __setApiBaseForTest(base) { API_BASE = base || 'https://groupmind.one/api/v1'; } + export async function fetchRoomMessages(room, apiKey, limit = 20) { - const url = `https://groupmind.one/api/v1/rooms/${encodeURIComponent(room)}/messages?limit=${limit}`; + const url = `${API_BASE}/rooms/${encodeURIComponent(room)}/messages?limit=${limit}`; try { const res = await fetch(url, { headers: { 'X-API-Key': apiKey }, @@ -122,7 +128,7 @@ async function postMessage(room, body, apiKey, config) { // that never reached the room (@codexmb). The status is checked now, and the // key no longer travels through a command line where ps can read it. try { - const res = await fetch('https://groupmind.one/api/v1/messages', { + const res = await fetch(`${API_BASE}/messages`, { method: 'POST', headers: { 'X-API-Key': apiKey, 'Content-Type': 'application/json' }, body: JSON.stringify({ room, body }), @@ -424,6 +430,7 @@ export async function startRoomAutomation({ rooms, apiKey, handle, interval, con const receiptPath = config?.receipts?.path || './ide-agent-receipts.jsonl'; const pollInterval = interval || config?.automation?.interval_sec || 30; const selfHandle = resolveSelfHandle({ explicit: handle, config }); + if (config?.automation?.api_base) __setApiBaseForTest(config.automation.api_base); const cooldownMs = (config?.automation?.cooldown_sec || 5) * 1000; console.log(`Room automation started`); diff --git a/test/automation-startup-replay.test.mjs b/test/automation-startup-replay.test.mjs new file mode 100644 index 0000000..4d2f836 --- /dev/null +++ b/test/automation-startup-replay.test.mjs @@ -0,0 +1,123 @@ +// Execution-level startup tests for the privileged dispatch path. +// +// Asked for by @codexmb: "cover existing-state/new-room startup, atomic +// persistence failure, and two concurrent processes with execution-level tests +// before calling privileged dispatch safe. Handle tests alone do not exercise +// poller startup/replay." +// +// These drive startRoomAutomation itself against a stub room server and count +// what it POSTS. A message that is never dispatched leaves no post; a replayed +// one leaves two. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { createServer } from 'node:http'; +import { mkdtempSync, rmSync, writeFileSync, readFileSync, existsSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { startRoomAutomation, __setApiBaseForTest } from '../src/room-automation.mjs'; + +// A stub GroupMind: serves whatever messages each room is configured with and +// records every POST, so "did it dispatch" is observed rather than inferred. +function stubRoom({ rooms, failRooms = new Set() }) { + // failRooms is mutated between runs by the tests that need a room to break. + const posts = []; + const server = createServer((req, res) => { + const url = new URL(req.url, 'http://x'); + if (req.method === 'POST') { + let b = ''; req.on('data', c => (b += c)); + req.on('end', () => { posts.push(JSON.parse(b || '{}')); res.writeHead(200, {'Content-Type':'application/json'}); res.end('{}'); }); + return; + } + const m = url.pathname.match(/\/rooms\/([^/]+)\/messages/); + const room = m && decodeURIComponent(m[1]); + if (!room || failRooms.has(room)) { res.writeHead(500); res.end('nope'); return; } + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ messages: rooms[room] || [] })); + }); + return { server, posts }; +} + +async function listen(server) { + await new Promise(r => server.listen(0, '127.0.0.1', r)); + return server.address().port; +} + +function cfg(dir, rooms, extra = {}) { + return { + poller: { owner_handle: 'petrus', handle: '@claudemm' }, + automation: { + seen_file: join(dir, 'seen.txt'), + interval_sec: 3600, // one poll; the test drives it + rules: [{ name: 'echo', match: { sender: 'petrus' }, action: { type: 'post', body: 'ack' } }], + ...extra, + }, + receipts: { path: join(dir, 'receipts.jsonl') }, + ...({}), + }; +} + +const msg = (id, body) => ({ id, from: 'petrus', isHuman: true, body, folder: '', created_at: new Date().toISOString() }); + +test('a room ADDED after the seen-file exists is seeded, not replayed', async () => { + const dir = mkdtempSync(join(tmpdir(), 'iak-auto-')); + const rooms = { alpha: [msg('a1', 'hello from alpha')], beta: [msg('b1', 'historical beta message')] }; + const { server, posts } = stubRoom({ rooms }); + const port = await listen(server); + const base = `http://127.0.0.1:${port}/api/v1`; + try { + // First run knows only alpha. + const c1 = cfg(dir, ['alpha'], { api_base: base }); + const h1 = await startRoomAutomation({ rooms: ['alpha'], apiKey: 'k', handle: '@claudemm', config: c1, }); + h1?.stop?.(); + const afterFirst = posts.length; + + // beta is added later. Its history must NOT dispatch. + const c2 = cfg(dir, ['alpha', 'beta'], { api_base: base }); + const h2 = await startRoomAutomation({ rooms: ['alpha', 'beta'], apiKey: 'k', handle: '@claudemm', config: c2, }); + h2?.stop?.(); + + assert.equal(posts.length, afterFirst, + `a newly added room replayed ${posts.length - afterFirst} historical message(s): ` + + posts.slice(afterFirst).map(p => p.body).join(' | ')); + assert.ok(existsSync(join(dir, 'seen.txt.seeded')), 'the per-room seeded marker must exist'); + assert.match(readFileSync(join(dir, 'seen.txt.seeded'), 'utf8'), /beta/, 'beta must be recorded as seeded'); + } finally { + await new Promise(r => server.close(r)); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a room that cannot be read blocks dispatch entirely rather than guessing', async () => { + // This test must DISCRIMINATE. An earlier version asserted "no posts" when a + // room failed -- but with fail-open seeding there were no posts either, since + // the same pass marked everything seen. It passed against the bug it was + // written to catch. So: seed alpha first, then add a NEW alpha message that + // WOULD dispatch, and make beta unreadable. Correct behaviour refuses to + // dispatch anything while any room is unseeded; fail-open posts the new one. + const dir = mkdtempSync(join(tmpdir(), 'iak-auto-')); + const rooms = { alpha: [msg('a1', 'first')], beta: [msg('b1', 'beta history')] }; + const failRooms = new Set(); + const { server, posts } = stubRoom({ rooms, failRooms }); + const port = await listen(server); + const base = `http://127.0.0.1:${port}/api/v1`; + try { + // Run 1: alpha only, seeds cleanly, dispatches nothing. + const h1 = await startRoomAutomation({ rooms: ['alpha'], apiKey: 'k', handle: '@claudemm', + config: cfg(dir, ['alpha'], { api_base: base }) }); + h1?.stop?.(); + assert.deepEqual(posts, [], 'seeding alone must never dispatch'); + + // Run 2: a genuinely new alpha message, and beta now unreadable. + rooms.alpha.unshift(msg('a2', 'NEW and dispatchable')); + failRooms.add('beta'); + const h2 = await startRoomAutomation({ rooms: ['alpha', 'beta'], apiKey: 'k', handle: '@claudemm', + config: cfg(dir, ['alpha', 'beta'], { api_base: base }) }); + h2?.stop?.(); + assert.deepEqual(posts, [], + 'beta could not be seeded, so NOTHING may dispatch -- not even the readable room. ' + + `Posted: ${posts.map(p => p.body).join(' | ')}`); + } finally { + await new Promise(r => server.close(r)); + rmSync(dir, { recursive: true, force: true }); + } +}); From 9675a73b89441184695288e4f14f6039fc3a3ab3 Mon Sep 17 00:00:00 2001 From: ThinkOff Date: Sat, 19 Sep 2026 21:44:09 +0300 Subject: [PATCH 12/14] lead: attach the poller's principal token, and say so when it has none MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /lead had never worked end to end. callDaemon attached no identity at all, so POST /lead hit resolvePrincipal, got null, and returned 403 "delegation is unavailable" — even for a correctly typed owner command that passed every gate on this side. Verified against the running daemon rather than read: POST /lead -> 403 GET /lead -> 200 {"ok":true,"lead":null} callDaemon now takes an optional `token` and sends it as a bearer, which is what resolvePrincipal reads. Omitted when unset, so a daemon with no principals configured behaves exactly as before rather than sending an empty Bearer header. A 403 with no token configured is a fault on THIS side, not a refusal of the user, so it now answers in words. A bare "forbidden" sends petrus looking for a permission he already has — the same failure as the /lead status path already guards with its "built but not running here yet" reply. Deliberately NOT included: the token itself. Browser-only /lead trusts msg.isHuman, and that field is forgeable until the GroupMind metadata fix is deployed (antfarm claudemm/fix-room-sender-identity-forgery). Minting a principal now would hang a privilege grant on a forgeable signal and make it look guarded. Order is: deploy the server fix, verify the identity boundary, then mint. codexmb's review blocks the reverse. 388 tests pass. Co-Authored-By: Claude Opus 5 --- src/room-automation.mjs | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/room-automation.mjs b/src/room-automation.mjs index d8478d1..c9f4a80 100644 --- a/src/room-automation.mjs +++ b/src/room-automation.mjs @@ -198,10 +198,22 @@ function parseLeadCommand(body) { return { op: 'assign', handle: rest.replace(/^@+/, '') }; } -async function callDaemon(daemonUrl, path, { method = 'GET', body } = {}) { +async function callDaemon(daemonUrl, path, { method = 'GET', body, token } = {}) { + // `token` is the caller's PER-AGENT principal token. Without it the daemon + // cannot tell who is asking, and POST /lead refuses outright ("delegation is + // unavailable"). That refusal is correct and it is why /lead had never + // worked end to end: this function attached no identity at all, so a + // perfectly authorised owner command died at the last hop. Verified live + // 2026-09-19: POST /lead -> 403, GET /lead -> 200. + // + // Omitted when unset, so a daemon with no principals configured behaves + // exactly as before rather than sending an empty Bearer. + const headers = {}; + if (body) headers['Content-Type'] = 'application/json'; + if (token) headers.Authorization = `Bearer ${token}`; const res = await fetch(`${daemonUrl.replace(/\/+$/, '')}${path}`, { method, - headers: body ? { 'Content-Type': 'application/json' } : undefined, + headers: Object.keys(headers).length > 0 ? headers : undefined, body: body ? JSON.stringify(body) : undefined, }); let payload = null; @@ -213,7 +225,7 @@ async function callDaemon(daemonUrl, path, { method = 'GET', body } = {}) { * Returns a reply string when the message was a /lead command, or null when it * was not one. Never throws: a daemon that is down must not stop the poller. */ -export async function handleLeadCommand(msg, { daemonUrl, ownerHandle = 'petrus' } = {}) { +export async function handleLeadCommand(msg, { daemonUrl, ownerHandle = 'petrus', principalToken } = {}) { const parsed = parseLeadCommand(msg.body || ''); if (!parsed) return null; @@ -254,7 +266,16 @@ export async function handleLeadCommand(msg, { daemonUrl, ownerHandle = 'petrus' const { status, payload } = await callDaemon(daemonUrl, '/lead', { method: 'POST', body: { handle: parsed.op === 'clear' ? null : parsed.handle, actor: ownerHandle }, + token: principalToken, }); + // A 403 here is the daemon saying it cannot identify the caller, which is + // a CONFIGURATION fault on this side, not a refusal of the user. Saying + // "forbidden" would send petrus looking for a permission he already has. + if (status === 403 && !principalToken) { + return 'Team lead is not configured on this machine: the room poller has no ' + + 'principal token, so the daemon cannot tell that the request comes from it. ' + + 'Nothing was changed.'; + } if (payload?.ok) { return parsed.op === 'clear' ? 'Team lead cleared. Confirmations are owner-only again.' From 9c4cb52ec844059dd609ded77ed634b2822a4d8f Mon Sep 17 00:00:00 2001 From: ThinkOff Date: Sat, 19 Sep 2026 23:18:11 +0300 Subject: [PATCH 13/14] lead: pass principals from config, and send the poller's token /lead had never worked end to end, for two independent reasons. Fixing either alone changes nothing: 1. room-automation's callDaemon attached no identity, so POST /lead hit resolvePrincipal, got null, and returned 403. 2. iak-mcp-daemon never passed `principals` from config to startConfirmationsServer -- it read auth_token and stopped. So `principals` in dogfood.json was inert and the daemon answered "this daemon has no per-agent tokens" whatever was configured. Minting a token could not have fixed it. Verified live: petrus typed `/lead claudemb` in the room and it took (lead @claudemb, assignedBy @petrus). First time the feature has worked. The token is read from a file (principal_token_file), never inlined: the precommand gate posts commands to the room and has leaked a key that way. CONFIG IS NOT ENABLED, deliberately. Turning `principals` on locked petrus out of his own approvals within ten minutes: the daemon's :8788 page calls /intent/:id/decision with NO Authorization header, so once principals exist the browser, the phone and the owner are all refused. I had predicted that exact lockout and still shipped it, because I checked the phone (which carries a token from July) and never checked the page. Reverted the config immediately; this commit keeps only the code. Before enabling, one of these has to land first: - the :8788 page carries a token, or - /lead settles through a route that is not the shared decision endpoint Also worth knowing: a daemon restart silently destroys every pending intent (const intents = new Map(), nothing read back at boot). Three restarts tonight wiped the queue, including the intent the new lead was reviewing. 388 tests pass. Co-Authored-By: Claude Opus 5 --- bin/iak-mcp-daemon.mjs | 7 +++++++ src/room-automation.mjs | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/bin/iak-mcp-daemon.mjs b/bin/iak-mcp-daemon.mjs index d9a074c..127a3f9 100755 --- a/bin/iak-mcp-daemon.mjs +++ b/bin/iak-mcp-daemon.mjs @@ -74,6 +74,13 @@ startConfirmationsServer({ port: cc.port || 8788, host: cc.host || '127.0.0.1', authToken: cc.auth_token || '', + // Per-agent principal tokens. startConfirmationsServer has accepted these + // since the team-lead work, but NOTHING EVER PASSED THEM: this call read + // auth_token and stopped, so `principals` in dogfood.json was inert and + // POST /lead answered "this daemon has no per-agent tokens" whatever was + // configured. That is why /lead never worked end to end -- the missing + // bearer in callDaemon was only half of it. + principals: cc.principals || {}, receiptsPath: config?.receipts?.path, announce: serverAnnounce, wakeScript, diff --git a/src/room-automation.mjs b/src/room-automation.mjs index c9f4a80..224778d 100644 --- a/src/room-automation.mjs +++ b/src/room-automation.mjs @@ -2,6 +2,7 @@ import { execSync } from 'node:child_process'; import { readFileSync, writeFileSync, appendFileSync, existsSync, renameSync, unlinkSync } from 'node:fs'; +import { homedir } from 'node:os'; import { randomUUID } from 'node:crypto'; import { createReceipt, appendReceipt } from './receipt.mjs'; import { canSend, markSent } from './rate-limiter.mjs'; @@ -221,6 +222,21 @@ async function callDaemon(daemonUrl, path, { method = 'GET', body, token } = {}) return { status: res.status, payload }; } +function readPrincipalToken(config) { + const p = config?.mcp?.confirmations?.principal_token_file + || config?.confirmations?.principal_token_file; + if (!p) return undefined; + try { + const v = readFileSync(p.replace(/^~/, homedir()), 'utf8').trim(); + return v || undefined; + } catch { + // A missing token file is not fatal: /lead then answers with the + // "not configured on this machine" reply rather than throwing and + // taking the whole poller down with it. + return undefined; + } +} + /** * Returns a reply string when the message was a /lead command, or null when it * was not one. Never throws: a daemon that is down must not stop the poller. @@ -561,6 +577,12 @@ export async function startRoomAutomation({ rooms, apiKey, handle, interval, con const leadReply = await handleLeadCommand(m, { daemonUrl: config?.confirmations?.daemon_url || 'http://127.0.0.1:8788', ownerHandle: config?.poller?.owner_handle || 'petrus', + // The poller's principal token. Read from a FILE, never inlined: + // config carries the path, the value stays in ~/.config/iak-gate.token + // at mode 600. Same token petrus's phone has presented since + // 2026-07-08, mapped to `petrus` in principals -- /lead needs + // actor == petrus because only the owner may appoint a lead. + principalToken: readPrincipalToken(config), }); if (leadReply !== null) { const posted = await postMessage(room, leadReply, apiKey, config); From 0b747c2d56a0833544eebabcfa1f4fb82513251a Mon Sep 17 00:00:00 2001 From: Petrus Pennanen Date: Mon, 21 Sep 2026 08:59:40 +0200 Subject: [PATCH 14/14] fix(test): await executeActionForTest after the fail-closed rebase Rebasing onto main's "fail closed, persist before acting" commit made executeAction (and its executeActionForTest export) async, so this test's four assertions were comparing a Promise's undefined .status instead of the resolved receipt. Await each call. Co-Authored-By: Claude Fable 5.1 --- test/room-automation-mute.test.mjs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/room-automation-mute.test.mjs b/test/room-automation-mute.test.mjs index e14bb59..552ac57 100644 --- a/test/room-automation-mute.test.mjs +++ b/test/room-automation-mute.test.mjs @@ -24,8 +24,8 @@ test('the mute gate is exported for testing', () => { assert.ok(executeActionForTest, 'executeAction must be reachable from a test'); }); -test('emergency-only withholds an agent-triggered post', () => { - const r = executeActionForTest( +test('emergency-only withholds an agent-triggered post', async () => { + const r = await executeActionForTest( { type: 'post', body: 'chatter' }, { from: '@somebot', body: 'hi' }, 'key', OFFLINE, true, @@ -35,8 +35,8 @@ test('emergency-only withholds an agent-triggered post', () => { markSent(); // burn the rate-limit budget: keeps every case below offline -test('emergency-only STILL answers petrus', () => { - const r = executeActionForTest( +test('emergency-only STILL answers petrus', async () => { + const r = await executeActionForTest( { type: 'post', body: 'reply' }, { from: 'petrus', body: '/lead status' }, 'key', OFFLINE, true, @@ -44,8 +44,8 @@ test('emergency-only STILL answers petrus', () => { assert.notEqual(r.status, 'suppressed', 'his own command must always get an answer'); }); -test('normal mode posts for everyone (control)', () => { - const r = executeActionForTest( +test('normal mode posts for everyone (control)', async () => { + const r = await executeActionForTest( { type: 'post', body: 'chatter' }, { from: '@somebot', body: 'hi' }, 'key', OFFLINE, false, @@ -53,9 +53,9 @@ test('normal mode posts for everyone (control)', () => { assert.notEqual(r.status, 'suppressed', 'nothing is withheld when not muted'); }); -test('the owner match ignores @ and case', () => { +test('the owner match ignores @ and case', async () => { for (const who of ['@Petrus', 'PETRUS', '@petrus']) { - const r = executeActionForTest( + const r = await executeActionForTest( { type: 'post', body: 'reply' }, { from: who, body: 'x' }, 'key', { ...OFFLINE, poller: { owner_handle: '@Petrus' } }, true, );