review(bus): darkwing S2b round 1, changes (row 43, #1525)
Verdict comment 26765, queue rev 140. R1: message.send and role.revoke verbs use gated decisions without consuming them. R2: a cross-role decision authorizes without limit once policy makes the action gated. Two rulings for Sage on cross-role reuse and class drift. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,37 @@
|
||||
Row 43, S2b, round 1: **request changes** (Darkwing)
|
||||
|
||||
Candidate `candidate-manifest.sha256` `1b4b2f20…`, three files under
|
||||
`packages/bus/`. Full record:
|
||||
`agents/darkwing/work/slice1-s2b-review/review-r1.md`.
|
||||
|
||||
The patch applies at `65d78d12` and the manifest checks 3/3. Tests pass
|
||||
48/48 on Node 26 and on Node 24. Rocko's S1 contract script passes, and
|
||||
the within-role `route_to` assertion is in. Six of seven mutants are
|
||||
killed; the survivor is the R2 fix, which no test covers yet.
|
||||
|
||||
The `authorize` change holds. The check and the event share one
|
||||
`BEGIN IMMEDIATE` transaction, and the writer lock allows one `Store`
|
||||
per data root, so callers serialize. The `decision.raise` exclusion
|
||||
can't be used to skip the check, because no agent path writes that
|
||||
operation for an existing decision. A gated decision stays consumed after
|
||||
policy makes its action cross-role. The README's decision 62 note claims
|
||||
nothing the decision doesn't.
|
||||
|
||||
Required:
|
||||
- R1: the `message.send` and `role.revoke` verbs use gated decisions
|
||||
without consuming them. A gated `message.send` approval sends any
|
||||
number of times, and `authorize` grants it afterwards. Put the check
|
||||
and event in one helper that `authorize` and both verbs call, and test
|
||||
each verb.
|
||||
- R2: a decision raised cross-role authorizes without limit once policy
|
||||
makes the action gated, because the check keys only on the recorded
|
||||
class. Check consumption when either class is gated, and test it.
|
||||
|
||||
For Sage, two rulings, either of which settles R2:
|
||||
1. Cross-role reuse matters. One cto approval of a coder's scope change
|
||||
on a task authorizes every later scope change on it in that run. I
|
||||
recommend every decision-backed authorization be single-use.
|
||||
2. Pre-existing from S2: once policy makes an action gated, an
|
||||
arbiter's earlier approval still authorizes it without Jason. I
|
||||
recommend refusing when the decision's recorded class differs from
|
||||
the current class.
|
||||
@@ -0,0 +1,31 @@
|
||||
#!/usr/bin/env python3
|
||||
# S2b round 1: Rocko's two mutants plus Darkwing's, repository layout, each compared to a clean baseline.
|
||||
import pathlib,tempfile,shutil,subprocess,json,re,sys
|
||||
cases=[
|
||||
('rocko-drop-check','broker.mjs',"fail('decision-consumed');",";"),
|
||||
('rocko-current-class','broker.mjs',"this.#decision(s, result.decision).class === 'gated'","result.class === 'gated'"),
|
||||
('dw-is-not-to-ne','broker.mjs',"IS NOT 'decision.raise' LIMIT 1","!= 'decision.raise' LIMIT 1"),
|
||||
('dw-count-raise-event','broker.mjs',"AND json_extract(body,'$.operation') IS NOT 'decision.raise' LIMIT 1","LIMIT 1"),
|
||||
('dw-every-class','broker.mjs',"result.decision && this.#decision(s, result.decision).class === 'gated' &&","result.decision &&"),
|
||||
('dw-event-drops-decision','broker.mjs',"{ action, ...result, target: context.target ?? null }","{ action, class: result.class, target: context.target ?? null }"),
|
||||
('dw-either-class','broker.mjs',"this.#decision(s, result.decision).class === 'gated'","(result.class === 'gated' || this.#decision(s, result.decision).class === 'gated')"),
|
||||
]
|
||||
source=pathlib.Path(sys.argv[1])
|
||||
def run(dest):
|
||||
r=subprocess.run(['node','--test',*[str(p) for p in sorted((dest/'tests').glob('*.test.mjs'))]],capture_output=True,text=True,timeout=300)
|
||||
tests=set(l[2:].rsplit(' (',1)[0] for l in r.stdout.splitlines() if l.startswith('✖ ') and not l.startswith('✖ failing tests'))
|
||||
return r.returncode,tests
|
||||
with tempfile.TemporaryDirectory(prefix='dw-s2b-mut-') as root:
|
||||
dest=pathlib.Path(root)/'packages'/'bus';shutil.copytree(source,dest,ignore=shutil.ignore_patterns('dw'))
|
||||
code,base=run(dest);print(json.dumps({'baseline_exit':code,'baseline_failures':sorted(base)}),flush=True)
|
||||
code,f=run(dest);print(json.dumps({'mutation':'no-op','exit':code,'killed':bool(f-base)}),flush=True)
|
||||
out=[]
|
||||
for name,file,old,new in cases:
|
||||
p=dest/'src'/file;b=p.read_text();pat=r'\s*'.join(re.escape(c) for c in old if not c.isspace());n=len(re.findall(pat,b))
|
||||
if n!=1: print(json.dumps({'mutation':name,'error':f'{n} matches'}),flush=True);continue
|
||||
p.write_text(re.sub(pat,lambda m:new,b,count=1))
|
||||
try: code,f=run(dest)
|
||||
finally: p.write_text(b)
|
||||
x=sorted(f-base);out.append({'mutation':name,'exit':code,'killed':bool(x),'new_failures':x})
|
||||
print(json.dumps(out[-1]),flush=True)
|
||||
print('killed',sum(x['killed'] for x in out),'of',len(out))
|
||||
@@ -0,0 +1,10 @@
|
||||
{"baseline_exit": 0, "baseline_failures": []}
|
||||
{"mutation": "no-op", "exit": 0, "killed": false}
|
||||
{"mutation": "rocko-drop-check", "exit": 1, "killed": true, "new_failures": ["a decision raised gated stays single-use after policy changes its action to cross-role", "gated approval authorizes once, survives store reopen, and fresh approval works", "two scheduled callers have exactly one grant and one consumed refusal"]}
|
||||
{"mutation": "rocko-current-class", "exit": 1, "killed": true, "new_failures": ["a decision raised gated stays single-use after policy changes its action to cross-role"]}
|
||||
{"mutation": "dw-is-not-to-ne", "exit": 1, "killed": true, "new_failures": ["a decision raised gated stays single-use after policy changes its action to cross-role", "gated approval authorizes once, survives store reopen, and fresh approval works", "two scheduled callers have exactly one grant and one consumed refusal"]}
|
||||
{"mutation": "dw-count-raise-event", "exit": 1, "killed": true, "new_failures": ["a decision raised gated stays single-use after policy changes its action to cross-role", "another run cannot consume an approval; a failed check leaves it usable", "failed commit rolls consumption back; within-role and cross-role stay reusable", "gated approval authorizes once, survives store reopen, and fresh approval works", "task action subjects and linked decision trail are complete and ordered", "two scheduled callers have exactly one grant and one consumed refusal"]}
|
||||
{"mutation": "dw-every-class", "exit": 1, "killed": true, "new_failures": ["failed commit rolls consumption back; within-role and cross-role stay reusable"]}
|
||||
{"mutation": "dw-event-drops-decision", "exit": 1, "killed": true, "new_failures": ["a decision raised gated stays single-use after policy changes its action to cross-role", "another run cannot consume an approval; a failed check leaves it usable", "failed commit rolls consumption back; within-role and cross-role stay reusable", "gated approval authorizes once, survives store reopen, and fresh approval works", "two scheduled callers have exactly one grant and one consumed refusal"]}
|
||||
{"mutation": "dw-either-class", "exit": 0, "killed": false, "new_failures": []}
|
||||
killed 6 of 7
|
||||
@@ -0,0 +1,57 @@
|
||||
v24.21.0
|
||||
✔ launch identity is stamped, payload identity is refused and stale holder cannot send (160.989147ms)
|
||||
✔ decision classes route from policy; gated resolution is human-only, choice and target must match (254.968273ms)
|
||||
✔ claim exclusion, holder release, gated revoke and rerouting to a new holder are atomic (251.099433ms)
|
||||
✔ launch events require a human CLI capability; generic emit cannot forge authority events (146.970564ms)
|
||||
✔ within-role decisions close atomically and invalid options or blocking omissions refuse (150.298836ms)
|
||||
✔ observer capabilities read human inbox but cannot mutate or forge launch identity (91.593414ms)
|
||||
✔ task action subjects and linked decision trail are complete and ordered (81.43039ms)
|
||||
✔ launch binding is durable and reconnecting requires the identical trusted record (41.427316ms)
|
||||
✔ business isolation includes inherited object names and cross-business message references (86.10921ms)
|
||||
✔ authority never transfers between action, run, target, unresolved or replaced role holder (120.840149ms)
|
||||
✔ task projection uses schema current view, skipping earlier and equal-start polls (60.520573ms)
|
||||
✔ revocation permanently bars the old run from reclaiming first, including after broker restart (99.416738ms)
|
||||
✔ empty message references refuse before storage; refusal-evidence failure stays a typed error (62.793345ms)
|
||||
✔ both arbiters require human resolution when their cross-role route is themselves (101.64615ms)
|
||||
✔ S1 adapter takes resolved limits and refs, rejects mismatched instance, never mutates input (1.800875ms)
|
||||
✔ only validated broker references load; returned data and exceptions cannot expose a known token (3.75381ms)
|
||||
✔ bad file modes, symlinks, repository/data paths, malformed tokens and missing dates refuse (1.897443ms)
|
||||
✔ expiry refuses use and env references never become client data (0.558257ms)
|
||||
✔ S1 parsed service refs work, service mismatch refuses, Gitea rotation due is a warning state (1.227213ms)
|
||||
✔ opaque tokens shorter than 16 characters refuse before use (0.288504ms)
|
||||
✔ human proof binds CLI entry, process start and nonce; agents and incomplete ancestry refuse (2.473733ms)
|
||||
✔ process reader gets own kernel identity without exposing environment values (0.583835ms)
|
||||
✔ EACCES ancestor environments skip only markers; commands and registered launches still refuse (1.926785ms)
|
||||
✔ real pid 1 remains inspectable when its environment is protected (0.28948ms)
|
||||
✔ broker process binds trusted launches, offers reader capabilities, refuses human mutation, closes cleanly (168.916833ms)
|
||||
✔ startup token refusal returns safe code without value or partial listening broker (33.320604ms)
|
||||
✔ loaded fixture token is absent from socket replies and SQLite, including refusal evidence (174.35308ms)
|
||||
✔ killed broker leaves an explicit stale lock; another process cannot silently reclaim it (156.939263ms)
|
||||
✔ trusted host registers later launches; socket clients never have a registration verb (173.330156ms)
|
||||
✔ runtime excludes declared project roots even when host supplies no repoRoots (34.890355ms)
|
||||
✔ a refused launch binding leaves the broker and existing capabilities alive; bad protocol stops it (146.627101ms)
|
||||
✔ v3b prototype refusals, views and append-only mutations (997.18416ms)
|
||||
✔ gated approval authorizes once, survives store reopen, and fresh approval works (244.016807ms)
|
||||
✔ another run cannot consume an approval; a failed check leaves it usable (208.439405ms)
|
||||
✔ two scheduled callers have exactly one grant and one consumed refusal (152.945877ms)
|
||||
✔ failed commit rolls consumption back; within-role and cross-role stay reusable (236.622595ms)
|
||||
✔ a decision raised gated stays single-use after policy changes its action to cross-role (149.152888ms)
|
||||
✔ creates private WAL store and excludes a second writer until explicit close (113.627745ms)
|
||||
✔ rollback is atomic and schema metadata is checked against trusted DDL, not just itself (187.960529ms)
|
||||
✔ existing empty database and symlink runtime directory refuse, never initialize over damage (182.741706ms)
|
||||
✔ crash during a transaction recovers no partial event after explicit fixture-only lock removal (170.646801ms)
|
||||
✔ writer refuses mixed at/read_at forms atomically, even through trusted SQL helpers (84.442872ms)
|
||||
✔ async transactions refuse before invoking their function (77.644716ms)
|
||||
✔ socket capability stamps launch identity; shared views use wire, no SQL client (148.630522ms)
|
||||
✔ two wire claims serialize; a lost reply never automatically retries (179.578612ms)
|
||||
✔ malformed, oversized and identity-forging envelopes refuse without echoing input (106.952818ms)
|
||||
✔ client preserves UTF-8 when a response divides a multibyte character (11.555363ms)
|
||||
✔ committed mutation followed by dropped reply reports unknown and is never retried (135.25781ms)
|
||||
ℹ tests 48
|
||||
ℹ suites 0
|
||||
ℹ pass 48
|
||||
ℹ fail 0
|
||||
ℹ cancelled 0
|
||||
ℹ skipped 0
|
||||
ℹ todo 0
|
||||
ℹ duration_ms 1767.695056
|
||||
@@ -0,0 +1,57 @@
|
||||
v26.8.1
|
||||
✔ launch identity is stamped, payload identity is refused and stale holder cannot send (143.398688ms)
|
||||
✔ decision classes route from policy; gated resolution is human-only, choice and target must match (253.681428ms)
|
||||
✔ claim exclusion, holder release, gated revoke and rerouting to a new holder are atomic (232.027274ms)
|
||||
✔ launch events require a human CLI capability; generic emit cannot forge authority events (143.975331ms)
|
||||
✔ within-role decisions close atomically and invalid options or blocking omissions refuse (147.480516ms)
|
||||
✔ observer capabilities read human inbox but cannot mutate or forge launch identity (75.03986ms)
|
||||
✔ task action subjects and linked decision trail are complete and ordered (77.211834ms)
|
||||
✔ launch binding is durable and reconnecting requires the identical trusted record (53.022592ms)
|
||||
✔ business isolation includes inherited object names and cross-business message references (83.620942ms)
|
||||
✔ authority never transfers between action, run, target, unresolved or replaced role holder (117.191244ms)
|
||||
✔ task projection uses schema current view, skipping earlier and equal-start polls (56.885453ms)
|
||||
✔ revocation permanently bars the old run from reclaiming first, including after broker restart (98.811218ms)
|
||||
✔ empty message references refuse before storage; refusal-evidence failure stays a typed error (63.230338ms)
|
||||
✔ both arbiters require human resolution when their cross-role route is themselves (103.848028ms)
|
||||
✔ S1 adapter takes resolved limits and refs, rejects mismatched instance, never mutates input (1.819597ms)
|
||||
✔ only validated broker references load; returned data and exceptions cannot expose a known token (3.072533ms)
|
||||
✔ bad file modes, symlinks, repository/data paths, malformed tokens and missing dates refuse (1.386612ms)
|
||||
✔ expiry refuses use and env references never become client data (0.492906ms)
|
||||
✔ S1 parsed service refs work, service mismatch refuses, Gitea rotation due is a warning state (0.990136ms)
|
||||
✔ opaque tokens shorter than 16 characters refuse before use (0.233019ms)
|
||||
✔ human proof binds CLI entry, process start and nonce; agents and incomplete ancestry refuse (1.392617ms)
|
||||
✔ process reader gets own kernel identity without exposing environment values (0.95553ms)
|
||||
✔ EACCES ancestor environments skip only markers; commands and registered launches still refuse (0.60336ms)
|
||||
✔ real pid 1 remains inspectable when its environment is protected (0.239057ms)
|
||||
✔ broker process binds trusted launches, offers reader capabilities, refuses human mutation, closes cleanly (161.848103ms)
|
||||
✔ startup token refusal returns safe code without value or partial listening broker (35.527291ms)
|
||||
✔ loaded fixture token is absent from socket replies and SQLite, including refusal evidence (190.913913ms)
|
||||
✔ killed broker leaves an explicit stale lock; another process cannot silently reclaim it (154.597816ms)
|
||||
✔ trusted host registers later launches; socket clients never have a registration verb (155.819973ms)
|
||||
✔ runtime excludes declared project roots even when host supplies no repoRoots (33.442788ms)
|
||||
✔ a refused launch binding leaves the broker and existing capabilities alive; bad protocol stops it (151.50099ms)
|
||||
✔ v3b prototype refusals, views and append-only mutations (924.334082ms)
|
||||
✔ gated approval authorizes once, survives store reopen, and fresh approval works (221.382225ms)
|
||||
✔ another run cannot consume an approval; a failed check leaves it usable (212.619226ms)
|
||||
✔ two scheduled callers have exactly one grant and one consumed refusal (130.86036ms)
|
||||
✔ failed commit rolls consumption back; within-role and cross-role stay reusable (224.241502ms)
|
||||
✔ a decision raised gated stays single-use after policy changes its action to cross-role (152.768533ms)
|
||||
✔ creates private WAL store and excludes a second writer until explicit close (103.646956ms)
|
||||
✔ rollback is atomic and schema metadata is checked against trusted DDL, not just itself (157.875505ms)
|
||||
✔ existing empty database and symlink runtime directory refuse, never initialize over damage (183.071258ms)
|
||||
✔ crash during a transaction recovers no partial event after explicit fixture-only lock removal (147.445961ms)
|
||||
✔ writer refuses mixed at/read_at forms atomically, even through trusted SQL helpers (86.047058ms)
|
||||
✔ async transactions refuse before invoking their function (74.55339ms)
|
||||
✔ socket capability stamps launch identity; shared views use wire, no SQL client (124.445988ms)
|
||||
✔ two wire claims serialize; a lost reply never automatically retries (161.867187ms)
|
||||
✔ malformed, oversized and identity-forging envelopes refuse without echoing input (115.77704ms)
|
||||
✔ client preserves UTF-8 when a response divides a multibyte character (11.657736ms)
|
||||
✔ committed mutation followed by dropped reply reports unknown and is never retried (114.4083ms)
|
||||
ℹ tests 48
|
||||
ℹ suites 0
|
||||
ℹ pass 48
|
||||
ℹ fail 0
|
||||
ℹ cancelled 0
|
||||
ℹ skipped 0
|
||||
ℹ todo 0
|
||||
ℹ duration_ms 1713.351577
|
||||
@@ -0,0 +1,75 @@
|
||||
// Darkwing S2b probes. Not part of the candidate.
|
||||
import test from 'node:test';
|
||||
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';
|
||||
const options = [{ key: 'yes', text: 'Allow' }, { key: 'no', text: 'Decline' }];
|
||||
const base = () => ({ demo: { id: 'demo', human: 'jason', arbiters: { technical: 'cto', delivery: 'pm' }, roles: {
|
||||
pm: { authority: { withinRole: ['message.send'], crossRole: [] } },
|
||||
cto: { authority: { withinRole: ['message.send'], crossRole: [] } },
|
||||
coder: { authority: { withinRole: [], crossRole: ['task.scope.change'] } } } } });
|
||||
const code = (f) => { try { return JSON.stringify(f()); } catch (e) { return e.code ?? String(e.message); } };
|
||||
function setup(t, policy = base()) {
|
||||
const root = mkdtempSync(join(tmpdir(), 'dw-s2b-'));
|
||||
let store, b, human;
|
||||
const runs = {};
|
||||
const open = () => { store = new Store(root); b = new Broker({ store, businesses: policy });
|
||||
human = b.bindHuman({ business: 'demo', human: 'jason', via: 'cli', outsideAgent: true }); };
|
||||
open();
|
||||
t.after(() => { try { store.close(); } catch {} rmSync(root, { recursive: true, force: true }); });
|
||||
const o = {
|
||||
get b() { return b; }, get store() { return store; }, get human() { return human; }, policy,
|
||||
agent: (role, run = role + '-run') => (runs[run] = b.bindLaunch({ business: 'demo', role, run, harness: 'pi' })),
|
||||
call: (cap, verb, args = {}) => b.request(cap, { verb, args }),
|
||||
raise: (cap, action, extra = {}) => b.request(cap, { verb: 'decision.raise', args: { action, question: 'Allow?', options, recommendation: 'no', blocking: false, ...extra } }),
|
||||
reopen: () => { store.close(); open(); },
|
||||
allowed: (id) => store.get("SELECT count(*) n FROM events WHERE kind='action.allowed' AND json_extract(body,'$.decision')=? AND json_extract(body,'$.operation') IS NOT 'decision.raise'", id).n,
|
||||
};
|
||||
return o;
|
||||
}
|
||||
test('A gated message.send approval: reuse through the verb, then authorize', (t) => {
|
||||
const o = setup(t), c = o.agent('coder'), pm = o.agent('pm');
|
||||
o.call(c, 'role.claim'); o.call(pm, 'role.claim');
|
||||
const d = o.raise(c, 'message.send', { target: 'pm' });
|
||||
console.log('A raise class', d.class, 'route_to', d.route_to);
|
||||
o.call(o.human, 'decision.resolve', { id: d.id, choice: 'yes' });
|
||||
for (let i = 0; i < 3; i++) console.log('A message.send', i, code(() => typeof o.call(c, 'message.send', { to: 'pm', body: 'x' + i, decision: d.id }).id));
|
||||
console.log('A action.allowed uses recorded', o.allowed(d.id));
|
||||
console.log('A authorize after 3 sends', code(() => o.b.authorize(c, 'message.send', { decision: d.id, target: 'pm' })));
|
||||
});
|
||||
test('B role.revoke verb, then authorize with the same decision', (t) => {
|
||||
const o = setup(t), c = o.agent('coder'), pm = o.agent('pm');
|
||||
o.call(c, 'role.claim'); o.call(pm, 'role.claim');
|
||||
const d = o.raise(c, 'role.revoke', { target: 'pm' });
|
||||
o.call(o.human, 'decision.resolve', { id: d.id, choice: 'yes' });
|
||||
console.log('B role.revoke verb', code(() => o.call(c, 'role.revoke', { role: 'pm', decision: d.id })));
|
||||
console.log('B action.allowed uses recorded', o.allowed(d.id));
|
||||
console.log('B authorize role.revoke after the verb', code(() => o.b.authorize(c, 'role.revoke', { decision: d.id, target: 'pm' })));
|
||||
console.log('B authorize again', code(() => o.b.authorize(c, 'role.revoke', { decision: d.id, target: 'pm' })));
|
||||
});
|
||||
test('C cross-role approval after policy makes the action gated', (t) => {
|
||||
const o = setup(t), c = o.agent('coder'), cto = o.agent('cto');
|
||||
o.call(c, 'role.claim'); o.call(cto, 'role.claim');
|
||||
const d = o.raise(c, 'task.scope.change', { domain: 'technical', target: 't1' });
|
||||
console.log('C raise class', d.class, 'route_to', d.route_to);
|
||||
o.call(cto, 'decision.resolve', { id: d.id, choice: 'yes' });
|
||||
o.policy.demo.roles.coder.authority.crossRole = [];
|
||||
o.reopen();
|
||||
const c2 = o.agent('coder', 'coder-run');
|
||||
console.log('C current class is gated; authorize with arbiter approval:');
|
||||
for (let i = 0; i < 3; i++) console.log('C authorize', i, code(() => o.b.authorize(c2, 'task.scope.change', { decision: d.id, target: 't1' })));
|
||||
});
|
||||
test('D cross-role approval reuse under unchanged policy', (t) => {
|
||||
const o = setup(t), c = o.agent('coder'), cto = o.agent('cto');
|
||||
o.call(c, 'role.claim'); o.call(cto, 'role.claim');
|
||||
const d = o.raise(c, 'task.scope.change', { domain: 'technical', target: 't1' });
|
||||
o.call(cto, 'decision.resolve', { id: d.id, choice: 'yes' });
|
||||
for (let i = 0; i < 3; i++) console.log('D authorize', i, code(() => o.b.authorize(c, 'task.scope.change', { decision: d.id, target: 't1' })));
|
||||
});
|
||||
test('E a second Store on the same data root refuses, so writers serialize in one process', (t) => {
|
||||
const o = setup(t);
|
||||
const root = o.store.directory.replace(/\/bus$/, '');
|
||||
console.log('E second Store', code(() => { new Store(root); return 'opened'; }));
|
||||
});
|
||||
@@ -0,0 +1,32 @@
|
||||
A raise class gated route_to human
|
||||
A message.send 0 "string"
|
||||
A message.send 1 "string"
|
||||
A message.send 2 "string"
|
||||
A action.allowed uses recorded 0
|
||||
A authorize after 3 sends {"class":"gated","decision":"88c5eca8-1a18-48f7-a1af-e06ca87009f3"}
|
||||
B role.revoke verb {"revoked":true}
|
||||
B action.allowed uses recorded 0
|
||||
B authorize role.revoke after the verb {"class":"gated","decision":"30853e6f-53e2-4bc6-83c7-a6542bd9dcbf"}
|
||||
B authorize again decision-consumed
|
||||
C raise class cross-role route_to cto
|
||||
C current class is gated; authorize with arbiter approval:
|
||||
C authorize 0 {"class":"gated","decision":"07c035bb-5f82-4781-beee-744d03b9ca41"}
|
||||
C authorize 1 {"class":"gated","decision":"07c035bb-5f82-4781-beee-744d03b9ca41"}
|
||||
C authorize 2 {"class":"gated","decision":"07c035bb-5f82-4781-beee-744d03b9ca41"}
|
||||
D authorize 0 {"class":"cross-role","decision":"5c43af42-f444-44d8-aace-8f1d3729dcc9"}
|
||||
D authorize 1 {"class":"cross-role","decision":"5c43af42-f444-44d8-aace-8f1d3729dcc9"}
|
||||
D authorize 2 {"class":"cross-role","decision":"5c43af42-f444-44d8-aace-8f1d3729dcc9"}
|
||||
E second Store writer-locked
|
||||
✔ A gated message.send approval: reuse through the verb, then authorize (98.251242ms)
|
||||
✔ B role.revoke verb, then authorize with the same decision (82.323116ms)
|
||||
✔ C cross-role approval after policy makes the action gated (97.870786ms)
|
||||
✔ D cross-role approval reuse under unchanged policy (80.465719ms)
|
||||
✔ E a second Store on the same data root refuses, so writers serialize in one process (36.405801ms)
|
||||
ℹ tests 5
|
||||
ℹ suites 0
|
||||
ℹ pass 5
|
||||
ℹ fail 0
|
||||
ℹ cancelled 0
|
||||
ℹ skipped 0
|
||||
ℹ todo 0
|
||||
ℹ duration_ms 449.019454
|
||||
@@ -0,0 +1,169 @@
|
||||
# Row 43, S2b single-use approvals, round 1 review (Darkwing)
|
||||
|
||||
Issue #1525. Candidate: Rocko's `candidate-manifest.sha256` (sha256
|
||||
`1b4b2f20bd495180d8ff170a28d237da64060738667773bb2bf353f0ff1583bc`), three
|
||||
files under `packages/bus/`. Packet `agents/rocko/work/slice1-s2b/`,
|
||||
`BUILD.md` sha256 `40965f3c…`, `build.patch` sha256 `74fdfcd8…`. Brief:
|
||||
`docs/plans/2026-10-05_s2b-single-use-approvals.md`. Sage's request is
|
||||
comment 26763, rev 139.
|
||||
|
||||
Verdict: **request changes.** What Rocko built in `authorize` is correct
|
||||
and well tested. Two paths still let one gated approval authorize more
|
||||
than once, and the README now says gated approval is single-use without
|
||||
qualification. Both fixes are small. Two questions go to Sage, and the
|
||||
answer to the first changes how R2 is fixed.
|
||||
|
||||
## What I checked
|
||||
|
||||
- The three packet hashes match Rocko's message. `build.patch` applies at
|
||||
`65d78d12`; the manifest checks 3/3.
|
||||
- I read the whole broker hunk, the new test file and the README diff,
|
||||
plus every broker path that reads a decision: `authorize`,
|
||||
`#checkAuthority`, `decision.raise`, the `role.revoke` and
|
||||
`message.send` verbs, `event.emit` and `recordEvent`.
|
||||
- Tests: 48/48 on Node 26.8.1 (`node26.txt`). In `node:24` (24.21.0),
|
||||
with no network, a non-root UID and the package mounted read-only, also
|
||||
48/48 (`node24.txt`).
|
||||
- Rocko's S1 contract script passes its three assertions against the
|
||||
scratch tree (`s1-contract.txt`). That closes my S2 round 2 note 1. The
|
||||
within-role `route_to` assertion is at `single-use.test.mjs:160`, which
|
||||
closes note 2.
|
||||
- My probes are in `probes-s2b.test.mjs`, output in `probes-s2b.txt`.
|
||||
|
||||
## Sage's five points
|
||||
|
||||
**One transaction.** The consumption check, `#checkAuthority` and the new
|
||||
`action.allowed` insert all run inside `store.transaction`, which opens
|
||||
with `BEGIN IMMEDIATE`. In practice there is no second writer to race.
|
||||
`Store` takes `bus/writer.lock` with `O_EXCL`, so a second `Store` on the
|
||||
same data root refuses with `writer-locked` (probe E), and `node:sqlite`
|
||||
is synchronous, so two callers in one process run one after the other. I
|
||||
tried a cross-thread race and it can't be set up for that reason. Rocko's
|
||||
two-caller test gets one grant and one `decision-consumed`, and the
|
||||
mutant that drops the check fails it.
|
||||
|
||||
**The `decision.raise` exclusion.** I found no way to use it to skip the
|
||||
check. `authorize` writes `{action, class, decision, target}`. The class
|
||||
and decision come from `#checkAuthority`, and the caller's context only
|
||||
supplies `target`, so the event never carries `operation`. Only
|
||||
`decision.raise` writes `operation: 'decision.raise'`, and it names the
|
||||
id it has just inserted. `event.emit` accepts only
|
||||
`{action: 'routine', target}`. `recordEvent` is the trusted adapter path
|
||||
with no socket route. It could write a raise-shaped event, but that can't
|
||||
undo a consumption because the check asks whether any non-raise event
|
||||
exists. `#checkAuthority` reads the earliest raise event, so a later one
|
||||
can't change the context either. The query uses `IS NOT`, which is right
|
||||
for an absent `operation`. My mutant that swaps it for `!=` never
|
||||
consumes, and three tests fail.
|
||||
|
||||
**Recorded class.** A decision raised gated stays consumed after a
|
||||
restart where policy makes its action cross-role. Rocko's test covers
|
||||
it, and the mutant that uses the current class instead fails it. The
|
||||
opposite direction is R2 below.
|
||||
|
||||
**Non-gated reuse.** Unchanged, as the brief asks, and it matters. See
|
||||
"For Sage", question 1.
|
||||
|
||||
**The README note.** It claims only what decision 62 says. The proof
|
||||
stops a direct invocation, a deliberate escape gets human authority, S6
|
||||
closes the gap with its own PID namespace or user and no human socket
|
||||
mounted, and T3 seats must not run the human CLI. It leaves out "on one
|
||||
OS user", but the sentence before it already says same-UID. It also
|
||||
leaves out "the trail shows it", which costs nothing. "That isolation is
|
||||
not supplied by this S2 package" is accurate.
|
||||
|
||||
## Required
|
||||
|
||||
**R1. Gated approvals used outside `authorize` are not consumed.** Two
|
||||
verbs call `#checkAuthority` directly and write no consumption event.
|
||||
|
||||
- `message.send`. A role that holds neither within-role nor cross-role
|
||||
`message.send` raises a gated decision, and Jason approves it. The
|
||||
verb then sends with that decision any number of times. Probe A sends
|
||||
three messages, records zero consumption events, and `authorize` still
|
||||
grants the same decision afterwards. Every shipped role holds
|
||||
`message.send` within-role, so this needs a business file that omits
|
||||
it. The README now says "Gated approval is single-use", and for this
|
||||
verb it isn't.
|
||||
- `role.revoke`. The verb revokes and records nothing. In probe B,
|
||||
`authorize('role.revoke')` with the same decision then grants once
|
||||
more. The holder binding stops the verb itself from running twice, so
|
||||
this is one extra use, not unlimited.
|
||||
|
||||
Fix: move the consumption check and its event into one helper that
|
||||
`authorize` and both verbs call inside their transactions. Add a test
|
||||
for each verb: a second send with the decision refuses, and `authorize`
|
||||
after a verb use refuses.
|
||||
|
||||
**R2. A cross-role approval authorizes a gated action without limit.** A
|
||||
coder raises `task.scope.change` while it is cross-role, and cto
|
||||
approves. Policy then drops it from the coder's `crossRole`, which makes
|
||||
it gated. `#checkAuthority` returns `class: 'gated'` on cto's approval,
|
||||
and the consumption check skips it because the recorded class is
|
||||
`cross-role`. Probe C authorizes it three times. Fix: check consumption
|
||||
when either the recorded class or the current class is gated, and test
|
||||
it. That mutant (`dw-either-class`) passes all 48 tests today. If Sage
|
||||
rules yes on question 1 or 2, that ruling covers this instead.
|
||||
|
||||
## For Sage
|
||||
|
||||
**1. Cross-role reuse matters.** Probe D: one cto approval of a coder's
|
||||
`task.scope.change` on `t1` authorizes it three times, and it would keep
|
||||
going for the rest of that run. The decision binds the action, run and
|
||||
target but not what the change is, so cto's yes to one scope change is a
|
||||
yes to every scope change on that task. That is the hole decision 63
|
||||
closes for Jason's approvals. I recommend every decision-backed
|
||||
authorization be single-use. Within-role actions pass no decision to
|
||||
`authorize`, so nothing changes for them. The code change is dropping
|
||||
the class condition; Rocko's "cross-role stays reusable" assertion flips,
|
||||
and my `dw-every-class` mutant shows that test would catch it. Ruling now
|
||||
lets Rocko do it in round 2, and it also settles R2.
|
||||
|
||||
**2. A pre-existing S2 gap, found through R2.** When policy makes an
|
||||
action gated, `#checkAuthority` accepts an approval the arbiter gave
|
||||
while it was cross-role (probe C reports `class: 'gated'` on cto's
|
||||
approval). Tightening policy doesn't require Jason's yes for approvals
|
||||
already given. I recommend `#checkAuthority` refuse with
|
||||
`decision-mismatch` when the decision's recorded class differs from the
|
||||
current class. That also settles R2, and it covers the direction Rocko
|
||||
tested: a gated decision stays unusable after the action becomes
|
||||
cross-role. It touches the same lines, so I'd take it in S2b round 2,
|
||||
but it changes S2 behavior beyond the brief, so it's your call.
|
||||
|
||||
## Mutation evidence
|
||||
|
||||
`mutations-s2b.py` runs Rocko's two mutants and five of mine against the
|
||||
full test glob, in the repository layout, against a clean baseline
|
||||
(`mutations-s2b.txt`). The no-op isn't killed.
|
||||
|
||||
| Mutant | Result |
|
||||
|---|---|
|
||||
| Rocko: drop `fail('decision-consumed')` | killed, 3 tests |
|
||||
| Rocko: current class instead of recorded class | killed, 1 test |
|
||||
| `IS NOT` becomes `!=` | killed, 3 tests |
|
||||
| the query also counts the raise event | killed, 6 tests |
|
||||
| consume every class, not only gated | killed, 1 test |
|
||||
| the `authorize` event drops `decision` | killed, 5 tests |
|
||||
| consume when either class is gated (the R2 fix) | survives |
|
||||
|
||||
The survivor is R2's fix. No test covers that case yet.
|
||||
|
||||
## Notes, not blocking
|
||||
|
||||
1. The consumption query is a `json_extract` scan over `events`, and the
|
||||
table has no index. `#checkAuthority`'s raise-event lookup already
|
||||
scans the same way, so this doubles an existing cost. If volume
|
||||
makes it matter, an index is a schema change, which would be my v3c.
|
||||
2. `BUILD.md` reports `test-task.sh` at 26 passed, 2 failed, from the
|
||||
live recall cases with Docker unavailable. `SKIPS.md` lists the
|
||||
guarded exclusions. Sage's integration gate covers those.
|
||||
|
||||
## Files
|
||||
|
||||
- `review-r1.md`, `comment.md`
|
||||
- `node26.txt`, `node24.txt`, `s1-contract.txt`
|
||||
- `probes-s2b.test.mjs`, `probes-s2b.txt`: probes A to E
|
||||
- `mutations-s2b.py`, `mutations-s2b.txt`
|
||||
|
||||
The probes run from a `dw/` directory beside `packages/bus/src`. The
|
||||
mutation runner takes the package directory as its argument.
|
||||
@@ -0,0 +1,3 @@
|
||||
PASS R2 loadBusiness refuses launch.by without within-role role.launch (exit 2)
|
||||
PASS R2 narrowed launch is null and gated; S2 adapter/broker refuses launch without decision
|
||||
PASS actual S1 validate/resolve -> S2 normalize/start -> socket claim/message; scoped fixture token callback; launch classification
|
||||
Reference in New Issue
Block a user