Files
jason.woltjeandClaude Opus 5.5 346347c950 agents: Dewey row 133 round 2 packet (#1627, rebase onto rows 49 and 123)
Base 89fd2d6fe. r1's build.patch on the new base gives the r2 tree byte for
byte, so delta.patch is empty; the interdiff shows only index lines and hunk
offsets. Mutants 5/5 killed with controls passing, targeted gate rc 0.
Candidate manifest sha256 1c0e6f8f260ae51b31c2235193c36ed1c005faefe317e1452a351a3bb5690ef0.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
2026-10-15 12:59:16 -05:00

319 lines
17 KiB
Diff

diff --git a/packages/bus/README.md b/packages/bus/README.md
index 3bfd35a55..36965636c 100644
--- a/packages/bus/README.md
+++ b/packages/bus/README.md
@@ -209,6 +209,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 ecb18f8b8..239af6921 100644
--- a/packages/bus/src/broker.mjs
+++ b/packages/bus/src/broker.mjs
@@ -336,15 +336,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);
@@ -887,6 +892,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');
@@ -988,6 +997,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'
@@ -1038,7 +1051,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 &&
@@ -1143,7 +1156,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.
@@ -1186,7 +1199,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);
});