diff --git a/packages/bus/README.md b/packages/bus/README.md index 4694e08b5..43b9b52a6 100644 --- a/packages/bus/README.md +++ b/packages/bus/README.md @@ -208,6 +208,13 @@ requires a strict task ref. `role.launch` is gated unless the business launch block's `by` names the acting role and refused when launch state is revoked. S6 still owns launch allowlists, capacity and actual spawning. +Whoever closes a decision messages the raising role, so a cross-role or gated raise also needs the +route to be allowed to message the raiser under the role definitions' messaging lists (row 133, +#1627). In practice the raiser's `receiveFrom` must list the arbiter's definition, or `human` for a +gated route. Otherwise the raise refuses `decision-route-refused`, logs `action.refused` and stores +nothing. A within-role raise is exempt. The close itself isn't checked again; the business validator +refuses an arbiter whose role definition declares messaging lists. + `decision.raise` checks its choices in this order, and each of these codes names one field: | Code | Field | Refused when | diff --git a/packages/bus/src/broker.mjs b/packages/bus/src/broker.mjs index f10e11b37..d0da6270c 100644 --- a/packages/bus/src/broker.mjs +++ b/packages/bus/src/broker.mjs @@ -330,15 +330,20 @@ export class Broker { // without the lists is unlimited, as before. The sender's lists are checked first, then the // target's, so a tester or the human sending to a limited role is refused unless listed. Every // path that stores a message from a role checks it: message.send, review.request, review.verdict. - #checkMessaging(s, to) { - const b = this.#business(s.business); - const definition = (role) => b.definitions?.[b.roles[role]?.definition]; - const sender = s.human ? 'human' : s.tester ? 'tester' : b.roles[s.role]?.definition; - const target = to === 'human' || to === 'tester' ? to : b.roles[to]?.definition; - const out = s.human || s.tester ? undefined : definition(s.role)?.messaging; - if (out && !out.sendTo.includes(target)) fail('message-recipient-refused'); - const inbound = to === 'human' || to === 'tester' ? undefined : definition(to)?.messaging; - if (inbound && !inbound.receiveFrom.includes(sender)) fail('message-sender-refused'); + // Row 133 (#1627): from and to are 'human', 'tester' or a role instance, so decision.raise can + // check its route as the sender of the close message; `refused` replaces both codes when given. + #checkMessaging(business, from, to, refused) { + const b = this.#business(business); + const person = (who) => who === 'human' || who === 'tester'; + const name = (who) => (person(who) ? who : b.roles[who]?.definition); + const lists = (who) => (person(who) ? undefined : b.definitions?.[name(who)]?.messaging); + const out = lists(from); + if (out && !out.sendTo.includes(name(to))) fail(refused ?? 'message-recipient-refused'); + const inbound = lists(to); + if (inbound && !inbound.receiveFrom.includes(name(from))) fail(refused ?? 'message-sender-refused'); + } + #sender(s) { + return s.human ? 'human' : s.tester ? 'tester' : s.role; } #decision(s, id) { const d = this.#store.get('SELECT * FROM decisions WHERE id=? AND business=?', id, s.business); @@ -837,6 +842,10 @@ export class Broker { if (!['technical', 'delivery'].includes(domain)) fail('invalid-domain'); const proposedRoute = cls === 'gated' ? 'human' : cls === 'cross-role' ? b.arbiters[domain] : s.role; const route = cls === 'cross-role' && proposedRoute === s.role ? 'human' : proposedRoute; + // Row 133 (#1627): whoever closes the decision messages the raiser, so the route must be + // allowed to message the raising role before anything is stored. A within-role decision is + // routed to the raiser's own role and closes in this call. + if (cls !== 'within-role') this.#checkMessaging(s.business, route, s.role, 'decision-route-refused'); if (a.supersedes) { const old = this.#decision(s, a.supersedes); if (old.raised_by_run !== s.run || this.#closed(old.id)) fail('supersede-refused'); @@ -938,6 +947,10 @@ export class Broker { d.id, ); // Row 115 (#1609): a close by anyone but the raising run tells the raising role, routed like message.send's. + // Row 133 (#1627): no messaging check here. decision.raise refused a route that can't message the + // raiser, definitions change only with a pinned copy and arbiters only through the business + // validator, so the route can still message the raiser when it closes the decision. The human's + // withdrawal of an arbiter-routed decision is unchecked too; no shipped role can reach it. if (op !== 'seen' && (s.human || s.run !== d.raised_by_run)) { const outcome = op === 'resolved' @@ -988,7 +1001,7 @@ export class Broker { if (a.to === 'tester') fail('unknown-role'); } if (a.to !== 'human' && a.to !== 'tester' && !Object.hasOwn(b.roles, a.to)) fail('unknown-role'); - this.#checkMessaging(s, a.to); + this.#checkMessaging(s.business, this.#sender(s), a.to); string(a.body, 32768); if ( a.class !== undefined && @@ -1093,7 +1106,7 @@ export class Broker { fail(older ? 'stale-commit' : 'unknown-commit'); } // REQ-ASSIST-2 has no approval exception: a review request is a message to the reviewer. - this.#checkMessaging(s, a.to); + this.#checkMessaging(s.business, this.#sender(s), a.to); const mid = randomUUID(); // Row 105 (#1599): the run holding the reviewer role, or null when nobody holds it and the // request waits for a claim. #reviewMessage routes on the same holder. @@ -1136,7 +1149,7 @@ export class Broker { ) fail('verdict-exists'); // The verdict is a message to the role that asked, so it meets that role's receive list. - this.#checkMessaging(s, r.from); + this.#checkMessaging(s.business, this.#sender(s), r.from); const mid = randomUUID(); const eid = this.#event( s, diff --git a/packages/bus/tests/decision-route.test.mjs b/packages/bus/tests/decision-route.test.mjs new file mode 100644 index 000000000..633521ff7 --- /dev/null +++ b/packages/bus/tests/decision-route.test.mjs @@ -0,0 +1,145 @@ +// Row 133 (#1627, REQ-ASSIST-2): whoever closes a decision messages the raising role, so +// decision.raise refuses a route that can't message the raiser before it stores anything. The route +// is the sender and the raiser the target, under row 120's messaging rules. A within-role decision, +// routed to the raiser's own role, is exempt. +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { Store } from '../src/store.mjs'; +import { Broker } from '../src/broker.mjs'; + +// The assistant's lists come from the shipped role file, as in messaging.test.mjs. +const assistantLists = JSON.parse( + readFileSync(fileURLToPath(new URL('../../../roles/assistant.json', import.meta.url)), 'utf8'), +).messaging; + +const options = [ + { key: 'yes', text: 'Allow' }, + { key: 'no', text: 'Decline' }, +]; +const pusher = { authority: { withinRole: ['message.send'], crossRole: ['git.push.working'] } }; +const roles = { + pm: { definition: 'pm', authority: { withinRole: ['message.send'], crossRole: [] } }, + chief: { definition: 'chief-of-staff', authority: { withinRole: ['message.send'], crossRole: [] } }, + assistant: { definition: 'assistant', authority: { withinRole: ['message.send'], crossRole: [] } }, + // Hears from pm, the technical arbiter, but not from the chief of staff, the delivery arbiter. + coder: { definition: 'coder', ...pusher }, + // Hears from neither arbiter nor the human, but may send to the human: a reversed check passes it. + warden: { definition: 'warden', ...pusher }, + // Hears from the human and both arbiters but sends only to pm: a reversed check refuses it. + scribe: { definition: 'scribe', ...pusher }, +}; +const definitions = { + pm: {}, + 'chief-of-staff': {}, + assistant: { messaging: assistantLists }, + coder: { messaging: { sendTo: ['human', 'pm', 'chief-of-staff'], receiveFrom: ['human', 'pm'] } }, + warden: { messaging: { sendTo: ['human', 'pm', 'chief-of-staff'], receiveFrom: ['coder'] } }, + scribe: { messaging: { sendTo: ['pm'], receiveFrom: ['human', 'pm', 'chief-of-staff'] } }, +}; +const businesses = { + demo: { id: 'demo', human: 'jason', arbiters: { technical: 'pm', delivery: 'chief' }, roles, definitions }, +}; +const ROUTE = 'decision-route-refused'; + +function setup(t) { + const root = mkdtempSync(join(tmpdir(), 'bus-decision-route-')); + const store = new Store(root); + t.after(() => { + store.close(); + rmSync(root, { recursive: true, force: true }); + }); + const b = new Broker({ store, businesses }); + const caps = {}; + for (const role of Object.keys(roles)) { + caps[role] = b.bindLaunch({ business: 'demo', role, run: `${role}-run`, harness: 'pi' }); + b.request(caps[role], { verb: 'role.claim' }); + } + caps.human = b.bindHuman({ business: 'demo', human: 'jason', via: 'cli', outsideAgent: true }); + const call = (who, verb, args = {}) => b.request(caps[who], { verb, args }); + const raise = (who, action, extra = {}) => + call(who, 'decision.raise', { action, question: `${action}?`, options, recommendation: 'no', blocking: false, ...extra }); + const count = (sql) => store.get(sql).n; + const stored = () => ({ + decisions: count('SELECT count(*) n FROM decisions'), + decisionEvents: count('SELECT count(*) n FROM decision_events'), + messages: count('SELECT count(*) n FROM messages'), + allowed: count("SELECT count(*) n FROM events WHERE kind='action.allowed'"), + }); + const refusals = () => + store + .all("SELECT actor_role, body FROM events WHERE kind='action.refused' ORDER BY seq") + .map((e) => [e.actor_role, JSON.parse(e.body).code]); + return { store, call, raise, stored, refusals }; +} + +test('a cross-role raise whose arbiter the raiser does not hear from is refused and stores nothing', (t) => { + const { raise, stored, refusals } = setup(t); + const before = stored(); + // Delivery goes to chief (chief-of-staff), which the coder's receiveFrom doesn't list. + assert.throws(() => raise('coder', 'git.push.working', { domain: 'delivery' }), { code: ROUTE }); + assert.throws(() => raise('warden', 'git.push.working', { domain: 'technical' }), { code: ROUTE }); + assert.deepEqual(refusals(), [['coder', ROUTE], ['warden', ROUTE]]); + assert.deepEqual(stored(), before, 'no decision, decision event, message or allowed event'); +}); + +test('a gated raise by a role that does not hear from the human is refused and stores nothing', (t) => { + const { raise, stored, refusals } = setup(t); + const before = stored(); + // deploy is gated, so the route is the human; warden's receiveFrom lacks human. + assert.throws(() => raise('warden', 'deploy', { target: 'release-1' }), { code: ROUTE }); + // A cross-role action outside the raiser's crossRole is gated too. + assert.throws(() => raise('warden', 'task.create'), { code: ROUTE }); + assert.deepEqual(refusals(), [['warden', ROUTE], ['warden', ROUTE]]); + assert.deepEqual(stored(), before); +}); + +test('a route the raiser hears from is allowed, checked from the route to the raiser', (t) => { + const { raise, call, store } = setup(t); + const technical = raise('coder', 'git.push.working', { domain: 'technical' }); + const gated = raise('coder', 'deploy', { target: 'release-1' }); + assert.deepEqual([technical.route_to, gated.route_to], ['pm', 'human']); + // scribe sends only to pm, but every route here may message scribe. + const routes = [ + raise('scribe', 'git.push.working', { domain: 'delivery' }), + raise('scribe', 'git.push.working', { domain: 'technical' }), + raise('scribe', 'deploy', { target: 'release-1' }), + ].map((d) => d.route_to); + assert.deepEqual(routes, ['chief', 'pm', 'human']); + // The close message the check stands for reaches the raiser. + call('pm', 'decision.resolve', { id: technical.id, choice: 'yes' }); + const close = store.get("SELECT from_role, to_role FROM messages WHERE class='DECISION' AND decision=?", technical.id); + assert.deepEqual({ ...close }, { from_role: 'pm', to_role: 'coder' }); +}); + +test('a refused supersede withdraws nothing', (t) => { + const { raise, store } = setup(t); + const old = raise('coder', 'git.push.working', { domain: 'technical' }); + assert.throws( + () => raise('coder', 'git.push.working', { domain: 'delivery', supersedes: old.id }), + { code: ROUTE }, + ); + assert.equal(store.get('SELECT count(*) n FROM decision_events WHERE decision=?', old.id).n, 0); + assert.equal(store.get('SELECT count(*) n FROM decisions').n, 1); +}); + +test("the assistant's within-role and gated raises still work", (t) => { + const { raise, call, store, refusals } = setup(t); + // message.send is within role: routed to the assistant itself, which its own lists don't name. + const within = raise('assistant', 'message.send', { choice: 'yes' }); + assert.equal(within.route_to, 'assistant'); + assert.equal( + store.get("SELECT op FROM decision_events WHERE decision=?", within.id).op, + 'resolved', + ); + // Anything else is gated: routed to the human, whom the assistant hears from. + const gated = raise('assistant', 'task.create'); + assert.equal(gated.route_to, 'human'); + call('human', 'decision.resolve', { id: gated.id, choice: 'no' }); + const close = store.get("SELECT from_role, to_role FROM messages WHERE class='DECISION' AND decision=?", gated.id); + assert.deepEqual({ ...close }, { from_role: 'human', to_role: 'assistant' }); + assert.deepEqual(refusals(), []); +}); diff --git a/packages/business/src/business.mjs b/packages/business/src/business.mjs index 49ac1fa45..4fa492d4a 100644 --- a/packages/business/src/business.mjs +++ b/packages/business/src/business.mjs @@ -156,6 +156,12 @@ export function validateBusinessDocument(document, file, { rolesDir }) { const name = document.arbiters[kind]; requireId(name, `${file} arbiters.${kind}`); if (!Object.hasOwn(roles, name)) refuse(`${file} arbiters.${kind} names ${name}, which the business file doesn't declare`); + // Row 133 (#1627): an arbiter messages the raiser when it closes a decision, so it can't be a + // role whose definition limits who it sends to and hears from. + const definition = roles[name].definition; + if (definitions[definition].messaging) { + refuse(`${file} arbiters.${kind} names ${name}, whose role definition ${definition} declares messaging lists`); + } arbiters[kind] = name; } diff --git a/packages/business/tests/assistant.test.mjs b/packages/business/tests/assistant.test.mjs index 9cdbff49a..3b9759e96 100644 --- a/packages/business/tests/assistant.test.mjs +++ b/packages/business/tests/assistant.test.mjs @@ -122,6 +122,31 @@ test("the instance takes no credential references and no tracker", () => { ); }); +// Row 133 (#1627): an arbiter messages the raiser when it closes a decision, so a role limited by +// messaging lists can't be one. The message names the kind, the instance and the definition. +test("an arbiter whose role definition declares messaging lists is refused, for each kind", () => { + for (const kind of ["delivery", "technical"]) { + refuses( + () => loadFixture((doc) => { doc.arbiters[kind] = INSTANCE; }), + new RegExp(`arbiters\\.${kind} names assistant, whose role definition assistant declares messaging lists`), + ); + // An instance name that isn't the definition's: both are named. + refuses( + () => loadFixture((doc) => { + doc.roles.aide = { definition: "assistant" }; + doc.arbiters[kind] = "aide"; + }), + new RegExp(`arbiters\\.${kind} names aide, whose role definition assistant declares messaging lists`), + ); + } +}); + +test("an arbiter without messaging lists is accepted beside the limited assistant", () => { + assert.deepEqual(loadFixture().business.arbiters, { delivery: "pm", technical: "cto" }); + const { business } = loadFixture((doc) => { doc.arbiters.delivery = "chief"; }); + assert.deepEqual(business.arbiters, { delivery: "chief", technical: "cto" }); +}); + test("the contract passes work to the chief of staff and directs no one", () => { const text = readFileSync(join(REPO_ROLES, "assistant.md"), "utf8"); assert.match(text, /chief of staff/); diff --git a/packages/business/tests/business.test.mjs b/packages/business/tests/business.test.mjs index 48e181f26..65f696897 100644 --- a/packages/business/tests/business.test.mjs +++ b/packages/business/tests/business.test.mjs @@ -215,6 +215,8 @@ test("the shipped example refuses as written and validates once filled in", () = .replaceAll("/home/you", root)); const business = validateBusinessDocument(filled, file, { rolesDir: REPO_ROLES }); assert.deepEqual(Object.keys(business.roles), ["pm", "cto", "coder", "reviewer", "chief", "assistant"]); + // Row 133 (#1627): the arbiters declare no messaging lists, so the limited assistant beside them passes. + assert.deepEqual(business.arbiters, { delivery: "pm", technical: "cto" }); assert.equal(business.tracker.sync.credentials.vikunja.file, join(root, ".config/mosaic-dev/secrets/mosaic-stack/sync-vikunja.token")); assert.equal(id, 15); });