diff --git a/packages/bus/README.md b/packages/bus/README.md index f31c699a4..1b1d0dd0e 100644 --- a/packages/bus/README.md +++ b/packages/bus/README.md @@ -298,7 +298,16 @@ The options come first, so a bad option list refuses `invalid-options` even when An option that isn't an object with exactly `key` and `text` refuses `invalid-request`, like any other malformed argument. Authorization requires the resolved option identified by `approvalChoice` -(default `yes`, which must exist), the same raiser role/run, action and target. +(default `yes`, which must exist), the same raiser role instance, action and +target. Row 136 (#1630): the approval binds to the decision, the business and +the role instance (`raised_by_role`, the key in the business's `roles` map), +not to the run that raised it, so it outlives a restart and any run of that +instance holding its claim may consume it. A decision raised by a thread run +binds to that run's thread too: only a run holding the same thread consumes +it, and a decision a thread instance raised before row 121 matches no thread +run. Another instance, or the same instance in another thread, refuses +`decision-mismatch`. `role.revoke` still requires the target's holder run +recorded at the raise. Inbox projections expose `{action,target,approvalChoice}` as `authorization`: **S4 must display this context when offering resolution**, not infer approval from recommendation or option position. Any other option is a refusal, even if @@ -307,8 +316,8 @@ including cross-role approval (lead decision 64). `authorize`, `message.send` and `role.revoke` share one authority-and-consumption helper. Each caller keeps the consumption check, `action.allowed` event and any broker-owned effect in one transaction. -A later use through any of those paths, including after restart, refuses with -`decision-consumed`. The `decision.raise` context event does not consume approval. +A later use through any of those paths, by any run and including after +restart, refuses with `decision-consumed`. The `decision.raise` context event does not consume approval. A failed transaction rolls consumption back. A mismatch between the decision's recorded class and current policy refuses with `decision-mismatch` before use, in either direction. Within-role `authorize` calls without a decision retain @@ -326,7 +335,10 @@ authority and the full matching, resolution and single-use checks still apply. A consumed approval is not an exactly-once external service operation receipt; an uncertain external outcome must not be retried with that approval. S3/S6 must implement their own external-operation identity and uncertain-outcome -handling. Raising/superseding and resolving are transactional. +handling. Raising/superseding and resolving are transactional. `supersedes` names an +open decision raised by the same role instance, in the same thread when it was +raised in one, by this run or an earlier one; otherwise the raise refuses +`supersede-refused` and withdraws nothing. ### Decision close messages (row 115, #1609) @@ -348,7 +360,7 @@ where `` is the close's `decision_events.by` (the human's name or the closing run). A close with a note adds `Note: ` on its own line. No message goes out for `decision.seen`, for the raising run's own withdraw, for -a supersede (also the raising run's withdraw), or for a within-role decision, +a supersede (a withdraw by the raising role instance), or for a within-role decision, which closes inside `decision.raise`. `message.receive` gives a human close's message the close's `human.input` answer event as its `request`, so the role's turn cites it as it cites a human instruction; a role close's `request` is diff --git a/packages/bus/src/broker.mjs b/packages/bus/src/broker.mjs index 61febf532..a9c0a44d3 100644 --- a/packages/bus/src/broker.mjs +++ b/packages/bus/src/broker.mjs @@ -489,6 +489,18 @@ export class Broker { if (!d) fail('decision-not-found'); return d; } + // Row 136 (#1630): the thread a decision was raised in, the one its raising run claimed, or + // null. A run is bound to one thread for its life and claimed it before it could raise. A + // decision a thread instance raised before row 121 has no claim, so no thread run matches it. + #raisedThread(d) { + return ( + this.#store.get( + "SELECT thread FROM thread_claims WHERE business=? AND holder_run=? AND op='claim' ORDER BY seq DESC LIMIT 1", + d.business, + d.raised_by_run, + )?.thread ?? null + ); + } #decisionView(d) { const row = this.#store.get( "SELECT body FROM events WHERE business=? AND kind='action.allowed' AND json_extract(body,'$.operation')='decision.raise' AND json_extract(body,'$.decision')=? ORDER BY seq LIMIT 1", @@ -602,11 +614,13 @@ export class Broker { d.id, ); const context = evidence ? JSON.parse(evidence.body) : null; + // Row 136 (#1630): bound to the role instance and its thread, not the run, so an approval + // outlives a restart. Single use is #consumeAuthority's. if ( d.class !== cls || d.action !== action || d.raised_by_role !== s.role || - d.raised_by_run !== s.run || + this.#raisedThread(d) !== s.thread || !context || context.target !== target ) @@ -1112,7 +1126,9 @@ export class Broker { 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'); + // Row 136: the same role instance and thread as #checkAuthority's, so a later run supersedes. + if (old.raised_by_role !== s.role || this.#raisedThread(old) !== s.thread || this.#closed(old.id)) + fail('supersede-refused'); this.#store.run( 'INSERT INTO decision_events(decision,at,op,by) VALUES(?,?,?,?)', old.id, diff --git a/packages/bus/tests/approval-binding.test.mjs b/packages/bus/tests/approval-binding.test.mjs new file mode 100644 index 000000000..0d10fb619 --- /dev/null +++ b/packages/bus/tests/approval-binding.test.mjs @@ -0,0 +1,195 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { Store } from '../src/store.mjs'; +import { Broker } from '../src/broker.mjs'; + +// Row 136 (#1630): an approval binds to the decision, the business and the role instance, plus +// the thread when the raising run held one, not to the run that raised it. coder and coder2 are +// two instances of one definition; assistant is a thread instance. +const businesses = { + demo: { + id: 'demo', + human: 'jason', + arbiters: { technical: 'pm', delivery: 'pm' }, + roles: { + coder: { definition: 'coder', authority: { withinRole: [], crossRole: [] } }, + coder2: { definition: 'coder', authority: { withinRole: [], crossRole: [] } }, + pm: { definition: 'pm', authority: { withinRole: [], crossRole: [] } }, + assistant: { definition: 'assistant', authority: { withinRole: ['message.send'], crossRole: [] } }, + }, + definitions: { coder: {}, pm: {}, assistant: {} }, + }, +}; + +function setup(t) { + const root = mkdtempSync(join(tmpdir(), 'bus-binding-')); + let store, b; + const caps = {}; + const ctx = { + get b() { + return b; + }, + get store() { + return store; + }, + caps, + call: (who, verb, args = {}) => b.request(caps[who], { verb, args }), + open(bs = businesses) { + store = new Store(root); + b = new Broker({ store, businesses: bs }); + caps.human = b.bindHuman({ business: 'demo', human: 'jason', via: 'cli', outsideAgent: true }); + }, + // A run of `role` that holds its claim; `thread` for the assistant. + run(name, role, thread) { + caps[name] = b.bindLaunch({ business: 'demo', role, run: name, harness: 'pi', ...(thread ? { thread } : {}) }); + ctx.call(name, 'role.claim'); + return caps[name]; + }, + end: (name) => b.endLaunch({ business: 'demo', run: name, reason: 'exited' }), + // A gated decision, so it routes to the human. + raise: (who, extra = {}) => + ctx.call(who, 'decision.raise', { + action: 'deploy', + target: 'release1', + question: 'Ship release1?', + options: [ + { key: 'yes', text: 'Yes' }, + { key: 'no', text: 'No' }, + ], + recommendation: 'yes', + blocking: false, + ...extra, + }), + approve: (d) => ctx.call('human', 'decision.resolve', { id: d.id, choice: 'yes' }), + use: (who, d) => b.authorize(caps[who], 'deploy', { decision: d.id, target: 'release1' }), + used: (d) => + store.get( + "SELECT count(*) AS n FROM events WHERE kind='action.allowed' AND json_extract(body,'$.decision')=? AND json_extract(body,'$.operation') IS NOT 'decision.raise'", + d.id, + ).n, + ops: (d) => store.all('SELECT op FROM decision_events WHERE decision=? ORDER BY seq', d.id).map((r) => r.op), + }; + ctx.open(); + t.after(() => { + store.close(); + rmSync(root, { recursive: true, force: true }); + }); + return ctx; +} + +// Two assistant threads in one project. +function threads(f) { + const project = f.call('human', 'project.create', { name: 'stack work' }).id; + const A = f.call('human', 'thread.create', { project, name: 'alpha' }).id; + const B = f.call('human', 'thread.create', { project, name: 'beta' }).id; + return { A, B }; +} + +test('REQ-DEC an approval outlives its run and a broker restart; a new run of the instance consumes it once', (t) => { + const f = setup(t); + f.run('run1', 'coder'); + const d = f.raise('run1'); + f.approve(d); + f.end('run1'); + f.store.close(); + f.open(); + f.run('run2', 'coder'); + assert.deepEqual(f.use('run2', d), { class: 'gated', decision: d.id }); + assert.equal(f.used(d), 1); + // Once: neither the consuming run nor a later one uses it again. + assert.throws(() => f.use('run2', d), { code: 'decision-consumed' }); + f.end('run2'); + f.run('run3', 'coder'); + assert.throws(() => f.use('run3', d), { code: 'decision-consumed' }); + assert.equal(f.used(d), 1); +}); + +test('REQ-DEC another instance of the same definition cannot consume an approval, and its refusal consumes nothing', (t) => { + const f = setup(t); + f.run('run1', 'coder'); + const d = f.raise('run1'); + f.approve(d); + f.run('other', 'coder2'); + assert.throws(() => f.use('other', d), { code: 'decision-mismatch' }); + assert.equal(f.used(d), 0); + f.use('run1', d); + assert.equal(f.used(d), 1); +}); + +test('REQ-DEC REQ-THREAD-1 a thread approval is consumed only by a run holding the same thread', (t) => { + const f = setup(t); + const { A, B } = threads(f); + f.run('a1', 'assistant', A); + f.run('b1', 'assistant', B); + const d = f.raise('a1'); + f.approve(d); + // The same instance in another thread. + assert.throws(() => f.use('b1', d), { code: 'decision-mismatch' }); + assert.equal(f.used(d), 0); + f.end('a1'); + f.run('a2', 'assistant', A); + f.use('a2', d); + assert.equal(f.used(d), 1); + assert.throws(() => f.use('a2', d), { code: 'decision-consumed' }); +}); + +test('REQ-DEC a later run of the same instance supersedes an open decision; another instance cannot', (t) => { + const f = setup(t); + f.run('run1', 'coder'); + const old = f.raise('run1'); + f.end('run1'); + f.run('run2', 'coder'); + const replacement = f.raise('run2', { supersedes: old.id, question: 'Ship release1 tonight?' }); + assert.equal(replacement.supersedes, old.id); + assert.deepEqual(f.ops(old), ['withdrawn']); + // The old decision is closed now, so superseding it again refuses. + assert.throws(() => f.raise('run2', { supersedes: old.id }), { code: 'supersede-refused' }); + f.run('other', 'coder2'); + assert.throws(() => f.raise('other', { supersedes: replacement.id }), { code: 'supersede-refused' }); + assert.deepEqual(f.ops(replacement), []); + assert.equal(f.store.get('SELECT count(*) AS n FROM decisions').n, 2); +}); + +test('REQ-DEC REQ-THREAD-1 a thread decision is superseded only from the same thread', (t) => { + const f = setup(t); + const { A, B } = threads(f); + f.run('a1', 'assistant', A); + f.run('b1', 'assistant', B); + const old = f.raise('a1'); + assert.throws(() => f.raise('b1', { supersedes: old.id }), { code: 'supersede-refused' }); + assert.deepEqual(f.ops(old), []); + f.end('a1'); + f.run('a2', 'assistant', A); + f.raise('a2', { supersedes: old.id }); + assert.deepEqual(f.ops(old), ['withdrawn']); +}); + +// Before row 121 a thread instance's run held no thread, so a decision it raised has no thread +// claim. The broker below reads assistant as a plain role to raise one the way such a run did. +test('REQ-DEC REQ-THREAD-1 a thread instance decision raised before row 121 is not consumed or superseded by any thread run', (t) => { + const f = setup(t); + const { A } = threads(f); + f.store.close(); + const plain = structuredClone(businesses); + plain.demo.roles.assistant.definition = 'assistant-before-121'; + plain.demo.definitions['assistant-before-121'] = {}; + f.open(plain); + f.run('old', 'assistant'); + const d = f.raise('old'); + f.approve(d); + // Left open: a resolved decision refuses supersede as closed, whatever its thread. + const open = f.raise('old', { question: 'Ship release2 tonight?' }); + f.end('old'); + assert.equal(f.store.get('SELECT count(*) AS n FROM thread_claims WHERE holder_run=?', 'old').n, 0); + f.store.close(); + f.open(); + f.run('a1', 'assistant', A); + assert.throws(() => f.use('a1', d), { code: 'decision-mismatch' }); + assert.equal(f.used(d), 0); + assert.deepEqual(f.ops(d), ['resolved']); + assert.throws(() => f.raise('a1', { supersedes: open.id }), { code: 'supersede-refused' }); + assert.deepEqual(f.ops(open), []); +}); diff --git a/packages/bus/tests/broker.test.mjs b/packages/bus/tests/broker.test.mjs index cf2e269bd..6e5beea08 100644 --- a/packages/bus/tests/broker.test.mjs +++ b/packages/bus/tests/broker.test.mjs @@ -271,7 +271,7 @@ test('business isolation includes inherited object names and cross-business mess ); assert.deepEqual(call(b, caps[1], 'trail', { subject: d.id }), []); }); -test('authority never transfers between action, run, target, unresolved or replaced role holder', (t) => { +test('authority never transfers between action, target, unresolved or replaced role holder; a later run of the instance keeps it', (t) => { const { b, agent, human } = setup(t), c = agent('coder'), p = agent('pm'); @@ -284,7 +284,8 @@ test('authority never transfers between action, run, target, unresolved or repla call(b, c, 'role.release'); const c2 = agent('coder', 'r2'); call(b, c2, 'role.claim'); - assert.throws(() => b.authorize(c2, 'deploy', { target: 'v1', decision: d.id }), /decision-mismatch/); + // Row 136 (#1630): the approval binds to the instance, not the run (approval-binding.test.mjs). + assert.equal(b.authorize(c2, 'deploy', { target: 'v1', decision: d.id }).decision, d.id); const revoke = raise(b, c2, 'role.revoke', { target: 'pm' }); call(b, human, 'decision.resolve', { id: revoke.id, choice: 'yes' }); call(b, p, 'role.release'); diff --git a/packages/bus/tests/single-use.test.mjs b/packages/bus/tests/single-use.test.mjs index 65e36292d..1ca595a79 100644 --- a/packages/bus/tests/single-use.test.mjs +++ b/packages/bus/tests/single-use.test.mjs @@ -104,21 +104,23 @@ test('gated approval authorizes once, survives store reopen, and fresh approval f.use(fresh); assert.equal(f.count(fresh), 1); }); -test('another run cannot consume an approval; a failed check leaves it usable', (t) => { +// Row 136 (#1630): an approval binds to the role instance, so another run of it consumes it once. +test('another run of the same instance consumes an approval once; a failed check leaves it usable', (t) => { const f = fixture(t), d = f.approve(), other = f.bind('run2'); f.call(f.cap, 'role.release'); f.call(other, 'role.claim'); - assert.throws(() => f.use(d, other), /decision-mismatch/); - assert.equal(f.count(d), 0); - f.call(other, 'role.release'); - f.call(f.cap, 'role.claim'); assert.throws( - () => f.b.authorize(f.cap, 'deploy', { decision: d.id, target: 'other' }), + () => f.b.authorize(other, 'deploy', { decision: d.id, target: 'other' }), /decision-mismatch/, ); - f.use(d); + assert.equal(f.count(d), 0); + f.use(d, other); + assert.equal(f.count(d), 1); + f.call(other, 'role.release'); + f.call(f.cap, 'role.claim'); + assert.throws(() => f.use(d), /decision-consumed/); assert.equal(f.count(d), 1); }); test('two scheduled callers have exactly one grant and one consumed refusal', async (t) => {