review loop: working repositories, git socket, push and review verdicts, row 93 (#1587)
Commits move as v2 bundles over bus-host/git.sock (decision 135). The bus
host keeps one bare working repository per business and project, seeds main
from the project root under a per-repository lock, fetches a pushed bundle
with fsck into a temp ref, requires history shared with main, and moves the
task's work ref only as a fast-forward. Host and runner git run with no
system or global config, hooks at /dev/null and local transports only. The
socket binds only for roles holding git.push.working or review.verdict.
The broker records a push as the action.allowed for git.push.working with
its commit, takes review.request for the latest push and one review.verdict
per request, and refuses task.close on a pushed task without an approve
verdict for the latest push. recordEvent reserves both shapes, so agents
can't forge either.
Reviewed by Dewey (28196) and Sage (28197), both approve. Gate on 7a729aef:
14 node suites and 9 scripts green.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
+38
-4
@@ -73,9 +73,11 @@ S6 adds launch ops on the same IPC channel, one reply each
|
||||
- `{op:'identity',cap}`: the business, role and run a capability names.
|
||||
- `{op:'authorizeLaunch',cap,instance}`: `authorize(cap,'role.launch',
|
||||
{target:instance})`; a refusal is also recorded as `action.refused`.
|
||||
- `{op:'refuse',cap,code}`: records a refusal the host decided itself
|
||||
(instance list, capacity, a failed spawn) as `action.refused` against the
|
||||
caller, the code only.
|
||||
- `{op:'refuse',cap,code,subject?}`: records a refusal the host decided
|
||||
itself (instance list, capacity, a failed spawn, a git socket check) as
|
||||
`action.refused` against the caller, the code only. A task ref as
|
||||
`subject` files the refusal on that task's trail, as a refused verb's
|
||||
does; any other subject, or none, records null (row 93).
|
||||
- `{op:'endLaunch',record}`: `{business,run,reason,exitCode?}` for a run
|
||||
this broker bound. It writes `session.ended` `{reason,exitCode,released}`,
|
||||
releases the run's claim if it still holds it, and drops every capability
|
||||
@@ -85,6 +87,25 @@ S6 adds launch ops on the same IPC channel, one reply each
|
||||
- `{op:'credentialStatus',business,role}`: the role's `{service,date,state}`
|
||||
rows, never a token.
|
||||
|
||||
Row 93 (#1587) adds three ops for the host's git socket
|
||||
(`packages/cli/src/git-socket.mjs`). A refusal from any of them is recorded
|
||||
as `action.refused` on the task's trail.
|
||||
|
||||
- `{op:'checkPush',cap,task_ref}`: `checkPush(cap,task_ref)` checks the
|
||||
caller's `git.push.working` authority before the host takes a push's
|
||||
bundle. It records nothing unless it refuses.
|
||||
- `{op:'recordPush',cap,push}`: `push` is `{task_ref,project,branch,ref,
|
||||
commit,previous}` once the work ref moved. It checks the authority again
|
||||
and writes the `action.allowed` for `git.push.working`, with the task as
|
||||
subject and `{project,branch,ref,commit,previous}` in its body. That event
|
||||
is the push record: the only `action.allowed` for `git.push.working` that
|
||||
names a commit. The project must be one of the business's. The host moves
|
||||
the ref back when this refuses.
|
||||
- `{op:'reviewFetch',cap,args}`: `args` is `{task_ref,request}`. The caller
|
||||
must hold `review.verdict` and be the role the `review.requested` event was
|
||||
addressed to (`request-not-found` otherwise). Returns the request's
|
||||
`{project,commit,ref}`, which the host bundles for the reviewer.
|
||||
|
||||
`{op:'close'}`, SIGTERM and SIGINT close the socket and DB and remove the owned
|
||||
writer lock. Losing the host IPC channel closes with exit 2. Startup or host
|
||||
protocol failures return a static error code and exit 2, never a raw exception.
|
||||
@@ -123,6 +144,8 @@ an operator inspect content and delivery evidence after an uncertain result.
|
||||
| `message.send` | `{to,body,class?,in_reply_to?,corrects?,decision?}`; returns `{id,request}`. Human instruction's `request` is its `human.input` event ID. |
|
||||
| `message.receive` | No args; up to 100 undelivered messages for current holder, including originating `request` for human instructions. |
|
||||
| `message.read` | `{id}`; recipient holder's acknowledgement; no new holder can acknowledge old holder's delivery. |
|
||||
| `review.request` | `{task_ref,commit,to,note?,decision?}`; action `review.request` (row 93, #1587). `commit` must be the task's latest pushed commit: an older one refuses `stale-commit`, any other `unknown-commit`. `to` must be another role holding `review.verdict` (`unknown-role`, `self-review`, `not-a-reviewer`). Writes a `review.requested` event `{task_ref,project,ref,commit,to,message}` and sends `to` a REVIEW-REQUEST message naming it. Returns `{request,message,commit,ref}`. |
|
||||
| `review.verdict` | `{task_ref,request,verdict,body,decision?}`; action `review.verdict`. `verdict` is `approve` or `changes`. `request` must be a `review.requested` event on the task addressed to the caller's role (`request-not-found`), with no verdict yet (`verdict-exists`). Writes a `review.verdict` event `{task_ref,request,commit,verdict,message}` and sends the requesting role a REVIEW-RESULT in reply to the request's message. Returns `{verdict,message,commit}`. |
|
||||
| `event.emit` | `{kind:'action.allowed',body:{action:'routine',target?},subject?}`; routine observations only. |
|
||||
| `launch.revoke`, `launch.restore` | No args; outside-agent human CLI only. |
|
||||
| `inbox` | No args; plain JSON projection scoped to capability's business. |
|
||||
@@ -337,7 +360,18 @@ it is not itself an external service executor. An adapter must not treat a prior
|
||||
check as authority after an await/holder change. Task/credential/poll registration
|
||||
and serialized external operations are S3's integration work, not a generic
|
||||
user-defined-handler endpoint in S2. `recordEvent` stamps identity, uses a
|
||||
transaction, and cannot emit human input or human launch control events.
|
||||
transaction, and cannot emit human input or human launch control events,
|
||||
or the review loop's `review.requested` and `review.verdict` events, or an
|
||||
`action.allowed` for `git.push.working`. Only the verbs and host ops above
|
||||
write those. The review loop adds no event kind and no schema change.
|
||||
|
||||
`authorize(cap,'task.close',{target,verdict})` checks the verdict on a task
|
||||
with a recorded push, inside the same transaction (row 93): it
|
||||
must be the id of a `review.verdict` event on that task
|
||||
(`verdict-not-found`), with verdict `approve` (`verdict-not-approve`), for
|
||||
the task's latest pushed commit (`verdict-stale`). A refusal rolls the
|
||||
`action.allowed` back. On a task with no push the verdict stays a citation
|
||||
the broker doesn't check.
|
||||
Task refs in action/review bodies require matching `subject`; all task events
|
||||
are also guarded by the pinned schema. `task.created.body.request` must name an
|
||||
existing same-business human input, with a requirement ID.
|
||||
|
||||
+199
-6
@@ -85,6 +85,12 @@ function identifier(x) {
|
||||
return x;
|
||||
}
|
||||
const taskRef = (x) => typeof x === 'string' && /^vikunja:[1-9][0-9]*\/[1-9][0-9]*$/.test(x);
|
||||
// A git commit id, SHA-1 or SHA-256.
|
||||
const oid = (x) => typeof x === 'string' && /^[0-9a-f]{40}([0-9a-f]{24})?$/.test(x);
|
||||
const VERDICTS = ['approve', 'changes'];
|
||||
// Row 93 (#1587): the review loop's evidence, written only by the verbs below. A push is an
|
||||
// action.allowed for git.push.working that names its commit, written only by recordPush.
|
||||
const REVIEW_KINDS = ['review.requested', 'review.verdict'];
|
||||
|
||||
export class Broker {
|
||||
#store;
|
||||
@@ -232,10 +238,11 @@ export class Broker {
|
||||
}
|
||||
// Trusted launcher API: records a refusal the host decided itself (instance list, capacity,
|
||||
// credentials) as action.refused against the caller, the way request() records its own.
|
||||
refuse(cap, code) {
|
||||
// A task ref as subject files the refusal on that task's trail, as request() does.
|
||||
refuse(cap, code, subject = null) {
|
||||
const s = this.#session(cap);
|
||||
if (!/^[a-z-]{1,64}$/.test(code)) fail('invalid-request');
|
||||
return this.#refused(s, new BusError(code), null).code;
|
||||
return this.#refused(s, new BusError(code), { args: { task_ref: subject } }).code;
|
||||
}
|
||||
// Trusted human transport invokes only AFTER checking its CLI process and launch ancestry.
|
||||
bindHuman(record) {
|
||||
@@ -422,7 +429,8 @@ export class Broker {
|
||||
return { class: cls, decision: d.id };
|
||||
}
|
||||
// Caller owns the transaction: authority, consumption and effect commit together.
|
||||
#consumeAuthority(s, action, context = {}) {
|
||||
// detail adds to the action.allowed body: a push's commit (row 93).
|
||||
#consumeAuthority(s, action, context = {}, detail = {}) {
|
||||
const result = this.#checkAuthority(s, action, context);
|
||||
// decision.raise records context, not permission consumed by an executor.
|
||||
if (
|
||||
@@ -437,22 +445,111 @@ export class Broker {
|
||||
this.#event(
|
||||
s,
|
||||
'action.allowed',
|
||||
{ action, ...result, target: context.target ?? null },
|
||||
{ ...detail, action, ...result, target: context.target ?? null },
|
||||
context.target ?? null,
|
||||
);
|
||||
return result;
|
||||
}
|
||||
// Trusted S3 handler calls inside its own operation; not a generic socket action executor.
|
||||
// Row 93 (#1587): task.close on a task with a recorded push also needs
|
||||
// context.verdict, checked by #closeVerdict; a refusal there rolls the action.allowed back.
|
||||
authorize(cap, action, context = {}) {
|
||||
const s = this.#session(cap);
|
||||
return this.#store.transaction(() => this.#consumeAuthority(s, action, context));
|
||||
return this.#store.transaction(() => {
|
||||
const result = this.#consumeAuthority(s, action, context);
|
||||
if (action === 'task.close') this.#closeVerdict(s, context.target, context.verdict);
|
||||
return result;
|
||||
});
|
||||
}
|
||||
// A task with a pushed commit closes only on an approve verdict for the latest push. A task with
|
||||
// no push closes as before, its verdict a citation the broker doesn't check.
|
||||
#closeVerdict(s, task, verdict) {
|
||||
const push = this.#latestPush(s.business, task);
|
||||
if (!push) return;
|
||||
const v = id(verdict)
|
||||
? this.#store.get(
|
||||
"SELECT body FROM events WHERE business=? AND kind='review.verdict' AND id=? AND subject=?",
|
||||
s.business,
|
||||
verdict,
|
||||
task,
|
||||
)
|
||||
: null;
|
||||
if (!v) fail('verdict-not-found');
|
||||
const body = JSON.parse(v.body);
|
||||
if (body.verdict !== 'approve') fail('verdict-not-approve');
|
||||
if (body.commit !== push.commit) fail('verdict-stale');
|
||||
}
|
||||
// A push is the action.allowed recordPush wrote, the only one for git.push.working with a commit.
|
||||
#latestPush(business, task) {
|
||||
const r = this.#store.get(
|
||||
"SELECT body FROM events WHERE business=? AND kind='action.allowed' AND json_extract(body,'$.action')='git.push.working' AND json_type(body,'$.commit')='text' AND subject=? ORDER BY seq DESC LIMIT 1",
|
||||
business,
|
||||
task,
|
||||
);
|
||||
return r ? JSON.parse(r.body) : null;
|
||||
}
|
||||
// Row 93: the bus host's git socket asks before it takes a push's bytes. Nothing is recorded
|
||||
// unless it refuses: the push is recorded, and its authority consumed, once the ref moved.
|
||||
checkPush(cap, task_ref) {
|
||||
const s = this.#session(cap);
|
||||
return this.#store.transaction(() => {
|
||||
this.#agent(s);
|
||||
if (!taskRef(task_ref)) fail('invalid-request');
|
||||
return this.#checkAuthority(s, 'git.push.working', { target: task_ref });
|
||||
});
|
||||
}
|
||||
// Row 93: records a push the git socket made (packages/seat/src/working.mjs) as the action.allowed
|
||||
// for git.push.working, with the commit. A refusal here moves the ref back. Trusted host only.
|
||||
recordPush(cap, push) {
|
||||
const s = this.#session(cap);
|
||||
return this.#store.transaction(() => {
|
||||
this.#agent(s);
|
||||
keys(push, ['task_ref', 'project', 'branch', 'ref', 'commit', 'previous'], ['task_ref', 'project', 'branch', 'ref', 'commit', 'previous']);
|
||||
if (
|
||||
!taskRef(push.task_ref) ||
|
||||
!Object.hasOwn(this.#business(s.business).projects ?? {}, push.project) ||
|
||||
!oid(push.commit) ||
|
||||
(push.previous !== null && !oid(push.previous))
|
||||
)
|
||||
fail('invalid-request');
|
||||
string(push.branch, 200);
|
||||
string(push.ref, 200);
|
||||
const { task_ref, project, branch, ref, commit, previous } = push;
|
||||
this.#consumeAuthority(s, 'git.push.working', { target: task_ref }, { project, branch, ref, commit, previous });
|
||||
return { task_ref, commit };
|
||||
});
|
||||
}
|
||||
// Row 93: what the git socket's fetch for a reviewer may send, from the review.requested event the
|
||||
// reviewer cites. The caller must hold review.verdict and be the request's reviewer. Trusted host only.
|
||||
reviewFetch(cap, { task_ref, request } = {}) {
|
||||
const s = this.#session(cap);
|
||||
return this.#store.transaction(() => {
|
||||
this.#agent(s);
|
||||
if (!taskRef(task_ref) || !id(request)) fail('invalid-request');
|
||||
if (this.#classification(s, 'review.verdict') === 'gated') fail('not-authorized');
|
||||
const r = this.#reviewRequest(s, task_ref, request);
|
||||
return { project: r.project, commit: r.commit, ref: r.ref };
|
||||
});
|
||||
}
|
||||
// A review.requested event on the task, addressed to this session's role.
|
||||
#reviewRequest(s, task, request) {
|
||||
const r = this.#store.get(
|
||||
"SELECT actor_role, body FROM events WHERE business=? AND kind='review.requested' AND id=? AND subject=?",
|
||||
s.business,
|
||||
request,
|
||||
task,
|
||||
);
|
||||
const body = r ? JSON.parse(r.body) : null;
|
||||
if (!body || body.to !== s.role) fail('request-not-found');
|
||||
return { ...body, from: r.actor_role };
|
||||
}
|
||||
// Trusted adapters only. No agent socket route reaches this method.
|
||||
recordEvent(cap, { kind, body, subject = null }) {
|
||||
const s = this.#session(cap);
|
||||
return this.#store.transaction(() => {
|
||||
this.#agent(s);
|
||||
if (['human.input', 'launch.revoked', 'launch.restored'].includes(kind)) fail('reserved-event');
|
||||
if (['human.input', 'launch.revoked', 'launch.restored', ...REVIEW_KINDS].includes(kind)) fail('reserved-event');
|
||||
if (kind === 'action.allowed' && body?.action === 'git.push.working') fail('reserved-event');
|
||||
return this.#event(s, kind, body, subject);
|
||||
});
|
||||
}
|
||||
@@ -873,6 +970,83 @@ export class Broker {
|
||||
this.#delivery(m.id, 'read', s.human ? null : this.#holder(s.business, s.role));
|
||||
return { read: true };
|
||||
}
|
||||
// Row 93 (#1587): the coder asks a reviewer role to review the task's latest pushed commit.
|
||||
case 'review.request': {
|
||||
keys(a, ['task_ref', 'commit', 'to', 'note', 'decision'], ['task_ref', 'commit', 'to']);
|
||||
if (!taskRef(a.task_ref) || !oid(a.commit)) fail('invalid-request');
|
||||
const withinRole = this.#classification(s, 'review.request') === 'within-role';
|
||||
this.#consumeAuthority(s, 'review.request', {
|
||||
decision: withinRole ? null : (a.decision ?? null),
|
||||
target: a.task_ref,
|
||||
});
|
||||
if (a.note !== undefined) string(a.note, 16384);
|
||||
if (!Object.hasOwn(b.roles, a.to)) fail('unknown-role');
|
||||
if (a.to === s.role) fail('self-review');
|
||||
const reviewer = b.roles[a.to].authority;
|
||||
if (![...reviewer.withinRole, ...reviewer.crossRole].includes('review.verdict')) fail('not-a-reviewer');
|
||||
const push = this.#latestPush(s.business, a.task_ref);
|
||||
if (push?.commit !== a.commit) {
|
||||
const older = this.#store.get(
|
||||
"SELECT 1 FROM events WHERE business=? AND kind='action.allowed' AND json_extract(body,'$.action')='git.push.working' AND subject=? AND json_extract(body,'$.commit')=?",
|
||||
s.business,
|
||||
a.task_ref,
|
||||
a.commit,
|
||||
);
|
||||
fail(older ? 'stale-commit' : 'unknown-commit');
|
||||
}
|
||||
const mid = randomUUID();
|
||||
const eid = this.#event(
|
||||
s,
|
||||
'review.requested',
|
||||
{ task_ref: a.task_ref, project: push.project, ref: push.ref, commit: a.commit, to: a.to, message: mid },
|
||||
a.task_ref,
|
||||
);
|
||||
this.#reviewMessage(s, mid, {
|
||||
to: a.to,
|
||||
cls: 'REVIEW-REQUEST',
|
||||
decision: withinRole ? null : (a.decision ?? null),
|
||||
body: `Review request ${eid} for ${a.task_ref}: commit ${a.commit} on ${push.ref} in project ${push.project}. Fetch it with fetch_review, then answer with review_verdict citing this request.${a.note ? '\n\n' + a.note : ''}`,
|
||||
});
|
||||
return { request: eid, message: mid, commit: a.commit, ref: push.ref };
|
||||
}
|
||||
// Row 93: the reviewer's answer to one review.request, sent back to the role that asked.
|
||||
case 'review.verdict': {
|
||||
keys(a, ['task_ref', 'request', 'verdict', 'body', 'decision'], ['task_ref', 'request', 'verdict', 'body']);
|
||||
if (!taskRef(a.task_ref) || !id(a.request) || !VERDICTS.includes(a.verdict)) fail('invalid-request');
|
||||
const withinRole = this.#classification(s, 'review.verdict') === 'within-role';
|
||||
this.#consumeAuthority(s, 'review.verdict', {
|
||||
decision: withinRole ? null : (a.decision ?? null),
|
||||
target: a.task_ref,
|
||||
});
|
||||
string(a.body, 32768);
|
||||
const r = this.#reviewRequest(s, a.task_ref, a.request);
|
||||
// One verdict per request: a changed mind needs a new request, so a close can't cite an
|
||||
// approve that a later verdict on the same request took back.
|
||||
if (
|
||||
this.#store.get(
|
||||
"SELECT 1 FROM events WHERE business=? AND kind='review.verdict' AND subject=? AND json_extract(body,'$.request')=?",
|
||||
s.business,
|
||||
a.task_ref,
|
||||
a.request,
|
||||
)
|
||||
)
|
||||
fail('verdict-exists');
|
||||
const mid = randomUUID();
|
||||
const eid = this.#event(
|
||||
s,
|
||||
'review.verdict',
|
||||
{ task_ref: a.task_ref, request: a.request, commit: r.commit, verdict: a.verdict, message: mid },
|
||||
a.task_ref,
|
||||
);
|
||||
this.#reviewMessage(s, mid, {
|
||||
to: r.from,
|
||||
cls: 'REVIEW-RESULT',
|
||||
in_reply_to: r.message,
|
||||
decision: withinRole ? null : (a.decision ?? null),
|
||||
body: `Review verdict ${eid} on ${a.task_ref}, commit ${r.commit}: ${a.verdict}. A task_close cites this verdict id.\n\n${a.body}`,
|
||||
});
|
||||
return { verdict: eid, message: mid, commit: r.commit };
|
||||
}
|
||||
case 'launch.revoke':
|
||||
case 'launch.restore': {
|
||||
keys(a, []);
|
||||
@@ -1055,6 +1229,25 @@ export class Broker {
|
||||
decision,
|
||||
);
|
||||
}
|
||||
// The message a review verb sends, routed like message.send's.
|
||||
#reviewMessage(s, mid, { to, cls, in_reply_to = null, decision = null, body }) {
|
||||
this.#store.run(
|
||||
'INSERT INTO messages(id,at,business,from_role,from_run,to_role,class,in_reply_to,decision,corrects,body) VALUES(?,?,?,?,?,?,?,?,?,?,?)',
|
||||
mid,
|
||||
this.#now(),
|
||||
s.business,
|
||||
s.role,
|
||||
s.run,
|
||||
to,
|
||||
cls,
|
||||
in_reply_to,
|
||||
decision,
|
||||
null,
|
||||
body,
|
||||
);
|
||||
const h = this.#holder(s.business, to);
|
||||
this.#delivery(mid, 'routed', h?.op === 'claim' ? h : null);
|
||||
}
|
||||
#delivery(mid, op, h, transport = null) {
|
||||
this.#store.run(
|
||||
'INSERT INTO deliveries(message,at,op,holder_run,transport,address) VALUES(?,?,?,?,?,?)',
|
||||
|
||||
@@ -2,12 +2,25 @@
|
||||
// never command arguments, stdout or service-token-bearing environment variables.
|
||||
import { startBroker } from './runtime.mjs';
|
||||
import { BusError } from './broker.mjs';
|
||||
const LAUNCH_OPS = new Set(['identity', 'authorizeLaunch', 'refuse', 'endLaunch', 'credentialStatus']);
|
||||
const LAUNCH_OPS = new Set([
|
||||
'identity',
|
||||
'authorizeLaunch',
|
||||
'refuse',
|
||||
'endLaunch',
|
||||
'credentialStatus',
|
||||
'checkPush',
|
||||
'recordPush',
|
||||
'reviewFetch',
|
||||
]);
|
||||
function launchOp(m) {
|
||||
if (m.op === 'identity') return runtime.identity(m.cap);
|
||||
if (m.op === 'authorizeLaunch') return runtime.authorizeLaunch(m.cap, m.instance);
|
||||
if (m.op === 'refuse') return runtime.refuse(m.cap, m.code);
|
||||
if (m.op === 'refuse') return runtime.refuse(m.cap, m.code, m.subject);
|
||||
if (m.op === 'endLaunch') return runtime.endLaunch(m.record);
|
||||
// Row 93 (#1587): the git socket's ops.
|
||||
if (m.op === 'checkPush') return runtime.checkPush(m.cap, m.task_ref);
|
||||
if (m.op === 'recordPush') return runtime.recordPush(m.cap, m.push);
|
||||
if (m.op === 'reviewFetch') return runtime.reviewFetch(m.cap, m.args);
|
||||
return runtime.credentialStatus(m.business, m.role);
|
||||
}
|
||||
// The host's request id comes back on the reply, so a late reply can't
|
||||
|
||||
@@ -75,15 +75,22 @@ export async function startBroker({
|
||||
});
|
||||
// S6 launcher operations. The host decides capacity and the instance list; the broker
|
||||
// authorizes role.launch and keeps the evidence, refusals included.
|
||||
function authorizeLaunch(cap, instance) {
|
||||
// A refusal is recorded against the caller, as request() records one.
|
||||
function refusing(cap, fn, subject = null) {
|
||||
try {
|
||||
return broker.authorize(cap, 'role.launch', { target: instance });
|
||||
return fn();
|
||||
} catch (e) {
|
||||
if (!(e instanceof BusError)) throw e;
|
||||
broker.refuse(cap, e.code);
|
||||
broker.refuse(cap, e.code, subject);
|
||||
throw e;
|
||||
}
|
||||
}
|
||||
const authorizeLaunch = (cap, instance) =>
|
||||
refusing(cap, () => broker.authorize(cap, 'role.launch', { target: instance }));
|
||||
// Row 93 (#1587): the host's git socket. A push is checked before the host takes the bundle
|
||||
// and recorded once the ref moved; a reviewer's fetch is checked against its review.requested.
|
||||
const checkPush = (cap, task_ref) => refusing(cap, () => broker.checkPush(cap, task_ref), task_ref);
|
||||
const reviewFetch = (cap, args) => refusing(cap, () => broker.reviewFetch(cap, args), args?.task_ref);
|
||||
// Metadata only (service, date, state); the token stays in Credentials.
|
||||
function credentialStatus(business, role) {
|
||||
return credentials
|
||||
@@ -99,7 +106,10 @@ export async function startBroker({
|
||||
endLaunch: (r) => broker.endLaunch(r),
|
||||
identity: (cap) => broker.identity(cap),
|
||||
authorizeLaunch,
|
||||
refuse: (cap, code) => broker.refuse(cap, code),
|
||||
checkPush,
|
||||
recordPush: (cap, push) => refusing(cap, () => broker.recordPush(cap, push), push?.task_ref),
|
||||
reviewFetch,
|
||||
refuse: (cap, code, subject) => broker.refuse(cap, code, subject),
|
||||
credentialStatus,
|
||||
launches: bound,
|
||||
readers: readCaps,
|
||||
|
||||
@@ -197,7 +197,7 @@ test('task action subjects and linked decision trail are complete and ordered',
|
||||
for (let i = 1; i < rows.length; i++)
|
||||
assert.ok(rows[i].at > rows[i - 1].at, 'broker write order has unique timestamps');
|
||||
assert.throws(
|
||||
() => b.recordEvent(c, { kind: 'review.verdict', body: { task_ref: 'vikunja:3/1' } }),
|
||||
() => b.recordEvent(c, { kind: 'action.refused', body: { task_ref: 'vikunja:3/1' } }),
|
||||
/task-subject-required/,
|
||||
);
|
||||
});
|
||||
|
||||
@@ -0,0 +1,213 @@
|
||||
// Row 93 (#1587): the review loop's broker side. The host's git socket
|
||||
// checks a push (checkPush) and records it once the ref moved (recordPush), the
|
||||
// coder asks for a review (review.request, a review.requested event),
|
||||
// the reviewer's fetch is checked against that request (reviewFetch), the
|
||||
// reviewer answers (review.verdict), and task.close on a pushed task needs an
|
||||
// approve verdict for the latest push.
|
||||
|
||||
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';
|
||||
|
||||
const TASK = 'vikunja:1/2';
|
||||
const A = 'a'.repeat(40);
|
||||
const B = 'b'.repeat(40);
|
||||
const businesses = {
|
||||
demo: {
|
||||
id: 'demo',
|
||||
human: 'jason',
|
||||
arbiters: { technical: 'cto', delivery: 'pm' },
|
||||
projects: { stack: { root: '/nonexistent' } },
|
||||
roles: {
|
||||
pm: { authority: { withinRole: ['message.send', 'task.close'], crossRole: [] } },
|
||||
cto: { authority: { withinRole: ['message.send', 'review.request'], crossRole: [] } },
|
||||
coder: {
|
||||
authority: { withinRole: ['task.update.assigned', 'git.push.working', 'review.request', 'message.send'], crossRole: [] },
|
||||
},
|
||||
reviewer: { authority: { withinRole: ['review.verdict', 'message.send', 'task.update.assigned'], crossRole: [] } },
|
||||
second: { authority: { withinRole: ['review.verdict'], crossRole: [] } },
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
function setup(t) {
|
||||
const root = mkdtempSync(join(tmpdir(), 'bus-review-'));
|
||||
const store = new Store(root);
|
||||
const b = new Broker({ store, businesses });
|
||||
t.after(() => {
|
||||
store.close();
|
||||
rmSync(root, { recursive: true, force: true });
|
||||
});
|
||||
const caps = {};
|
||||
for (const role of Object.keys(businesses.demo.roles)) {
|
||||
caps[role] = b.bindLaunch({ business: 'demo', role, run: `${role}-run`, harness: 'pi' });
|
||||
b.request(caps[role], { verb: 'role.claim', args: {} });
|
||||
}
|
||||
const call = (role, verb, args) => b.request(caps[role], { verb, args });
|
||||
// What the git socket does for a push: check, then record once the ref moved.
|
||||
const push = (commit, previous = null, task_ref = TASK) => {
|
||||
b.checkPush(caps.coder, task_ref);
|
||||
return b.recordPush(caps.coder, { task_ref, project: 'stack', branch: 'feat', ref: 'work/vikunja-1-2', commit, previous });
|
||||
};
|
||||
const events = (kind) => store.all('SELECT * FROM events WHERE kind=? ORDER BY seq', kind);
|
||||
const pushes = () => events('action.allowed').filter((e) => JSON.parse(e.body).action === 'git.push.working');
|
||||
return { b, store, caps, call, push, events, pushes };
|
||||
}
|
||||
|
||||
test('a push is recorded as the action.allowed for git.push.working on the task, and only the host records one', (t) => {
|
||||
const { b, caps, push, events, pushes } = setup(t);
|
||||
// The check before the host takes the bundle records nothing unless it refuses.
|
||||
b.checkPush(caps.coder, TASK);
|
||||
assert.equal(events('action.allowed').length, 0);
|
||||
assert.throws(() => b.checkPush(caps.reviewer, TASK), /decision-required/);
|
||||
assert.throws(() => b.checkPush(caps.coder, 'gitea:1'), /invalid-request/);
|
||||
assert.deepEqual(push(A), { task_ref: TASK, commit: A });
|
||||
const [e] = pushes();
|
||||
assert.equal(e.subject, TASK);
|
||||
assert.equal(e.actor_role, 'coder');
|
||||
assert.deepEqual(JSON.parse(e.body), {
|
||||
project: 'stack',
|
||||
branch: 'feat',
|
||||
ref: 'work/vikunja-1-2',
|
||||
commit: A,
|
||||
previous: null,
|
||||
action: 'git.push.working',
|
||||
class: 'within-role',
|
||||
target: TASK,
|
||||
});
|
||||
// A role without the action can't have a push recorded either.
|
||||
assert.throws(
|
||||
() => b.recordPush(caps.reviewer, { task_ref: TASK, project: 'stack', branch: 'feat', ref: 'work/vikunja-1-2', commit: B, previous: A }),
|
||||
/decision-required/,
|
||||
);
|
||||
const good = { task_ref: TASK, project: 'stack', branch: 'feat', ref: 'work/vikunja-1-2', commit: B, previous: A };
|
||||
for (const bad of [{ ...good, project: 'other' }, { ...good, commit: 'x' }, { ...good, previous: 'x' }, { ...good, task_ref: 'gitea:1' }]) assert.throws(() => b.recordPush(caps.coder, bad), /invalid-request/);
|
||||
const { previous, ...short } = good;
|
||||
assert.throws(() => b.recordPush(caps.coder, short), /invalid-request/);
|
||||
for (const kind of ['review.requested', 'review.verdict'])
|
||||
assert.throws(() => b.recordEvent(caps.coder, { kind, body: { task_ref: TASK }, subject: TASK }), /reserved-event/);
|
||||
assert.throws(
|
||||
() => b.recordEvent(caps.coder, { kind: 'action.allowed', body: { action: 'git.push.working', target: TASK, commit: B }, subject: TASK }),
|
||||
/reserved-event/,
|
||||
);
|
||||
assert.equal(pushes().length, 1);
|
||||
});
|
||||
|
||||
test('review.request names the latest pushed commit and sends the reviewer a REVIEW-REQUEST', (t) => {
|
||||
const { store, call, push, events } = setup(t);
|
||||
push(A);
|
||||
const r = call('coder', 'review.request', { task_ref: TASK, commit: A, to: 'reviewer', note: 'please' });
|
||||
assert.equal(r.commit, A);
|
||||
assert.equal(r.ref, 'work/vikunja-1-2');
|
||||
const [e] = events('review.requested');
|
||||
assert.equal(e.id, r.request);
|
||||
assert.equal(e.subject, TASK);
|
||||
assert.deepEqual(JSON.parse(e.body), { task_ref: TASK, project: 'stack', ref: 'work/vikunja-1-2', commit: A, to: 'reviewer', message: r.message });
|
||||
const m = store.get('SELECT * FROM messages WHERE id=?', r.message);
|
||||
assert.equal(m.to_role, 'reviewer');
|
||||
assert.equal(m.from_role, 'coder');
|
||||
assert.equal(m.class, 'REVIEW-REQUEST');
|
||||
assert.match(m.body, new RegExp(`Review request ${r.request} for vikunja:1/2: commit ${A}`));
|
||||
assert.match(m.body, /please$/);
|
||||
});
|
||||
|
||||
test('review.request refuses a commit that isn\'t the latest push, a non-reviewer and a role without the action', (t) => {
|
||||
const { call, push, events } = setup(t);
|
||||
assert.throws(() => call('coder', 'review.request', { task_ref: TASK, commit: A, to: 'reviewer' }), /unknown-commit/);
|
||||
push(A);
|
||||
push(B, A);
|
||||
assert.throws(() => call('coder', 'review.request', { task_ref: TASK, commit: A, to: 'reviewer' }), /stale-commit/);
|
||||
assert.throws(() => call('coder', 'review.request', { task_ref: TASK, commit: 'c'.repeat(40), to: 'reviewer' }), /unknown-commit/);
|
||||
assert.throws(() => call('coder', 'review.request', { task_ref: 'vikunja:1/3', commit: B, to: 'reviewer' }), /unknown-commit/);
|
||||
assert.throws(() => call('coder', 'review.request', { task_ref: TASK, commit: B, to: 'nobody' }), /unknown-role/);
|
||||
assert.throws(() => call('coder', 'review.request', { task_ref: TASK, commit: B, to: 'pm' }), /not-a-reviewer/);
|
||||
assert.throws(() => call('cto', 'review.request', { task_ref: TASK, commit: B, to: 'cto' }), /self-review/);
|
||||
assert.throws(() => call('reviewer', 'review.request', { task_ref: TASK, commit: B, to: 'second' }), /decision-required/);
|
||||
assert.throws(() => call('coder', 'review.request', { task_ref: TASK, commit: 'B', to: 'reviewer' }), /invalid-request/);
|
||||
assert.throws(() => call('coder', 'review.request', { task_ref: TASK, commit: B, to: 'reviewer', extra: 1 }), /invalid-request/);
|
||||
assert.equal(events('review.requested').length, 0);
|
||||
// A refusal rolls its action.allowed back, and is recorded as action.refused.
|
||||
assert.equal(events('action.allowed').filter((e) => JSON.parse(e.body).action === 'review.request').length, 0);
|
||||
assert.ok(events('action.refused').length >= 10);
|
||||
// Another role that holds review.request asks too.
|
||||
assert.equal(call('cto', 'review.request', { task_ref: TASK, commit: B, to: 'reviewer' }).commit, B);
|
||||
});
|
||||
|
||||
test('reviewFetch answers only the addressed reviewer, for the request it cites', (t) => {
|
||||
const { b, caps, call, push } = setup(t);
|
||||
push(A);
|
||||
const { request } = call('coder', 'review.request', { task_ref: TASK, commit: A, to: 'reviewer' });
|
||||
assert.deepEqual(b.reviewFetch(caps.reviewer, { task_ref: TASK, request }), { project: 'stack', commit: A, ref: 'work/vikunja-1-2' });
|
||||
assert.throws(() => b.reviewFetch(caps.coder, { task_ref: TASK, request }), /not-authorized/);
|
||||
assert.throws(() => b.reviewFetch(caps.pm, { task_ref: TASK, request }), /not-authorized/);
|
||||
assert.throws(() => b.reviewFetch(caps.second, { task_ref: TASK, request }), /request-not-found/);
|
||||
assert.throws(() => b.reviewFetch(caps.reviewer, { task_ref: 'vikunja:1/3', request }), /request-not-found/);
|
||||
assert.throws(() => b.reviewFetch(caps.reviewer, { task_ref: TASK, request: 'no-such-request' }), /request-not-found/);
|
||||
assert.throws(() => b.reviewFetch(caps.reviewer, { task_ref: TASK }), /invalid-request/);
|
||||
});
|
||||
|
||||
test('review.verdict answers one request once, to the role that asked', (t) => {
|
||||
const { store, call, push, events } = setup(t);
|
||||
push(A);
|
||||
const req = call('coder', 'review.request', { task_ref: TASK, commit: A, to: 'reviewer' });
|
||||
assert.throws(() => call('coder', 'review.verdict', { task_ref: TASK, request: req.request, verdict: 'approve', body: 'mine' }), /decision-required/);
|
||||
assert.throws(() => call('second', 'review.verdict', { task_ref: TASK, request: req.request, verdict: 'approve', body: 'not mine' }), /request-not-found/);
|
||||
assert.throws(() => call('reviewer', 'review.verdict', { task_ref: TASK, request: req.request, verdict: 'maybe', body: 'x' }), /invalid-request/);
|
||||
const v = call('reviewer', 'review.verdict', { task_ref: TASK, request: req.request, verdict: 'changes', body: 'fix the test' });
|
||||
assert.equal(v.commit, A);
|
||||
const [e] = events('review.verdict');
|
||||
assert.equal(e.id, v.verdict);
|
||||
assert.deepEqual(JSON.parse(e.body), { task_ref: TASK, request: req.request, commit: A, verdict: 'changes', message: v.message });
|
||||
const m = store.get('SELECT * FROM messages WHERE id=?', v.message);
|
||||
assert.equal(m.to_role, 'coder');
|
||||
assert.equal(m.class, 'REVIEW-RESULT');
|
||||
assert.equal(m.in_reply_to, req.message);
|
||||
assert.match(m.body, /: changes\. A task_close cites this verdict id\.\n\nfix the test$/);
|
||||
assert.throws(() => call('reviewer', 'review.verdict', { task_ref: TASK, request: req.request, verdict: 'approve', body: 'changed my mind' }), /verdict-exists/);
|
||||
assert.equal(events('review.verdict').length, 1);
|
||||
});
|
||||
|
||||
test('task.close on a pushed task needs an approve verdict for the latest push', (t) => {
|
||||
const { b, caps, call, push, events } = setup(t);
|
||||
const close = (verdict, target = TASK) => b.authorize(caps.pm, 'task.close', { target, verdict });
|
||||
// No push: the verdict is a citation the broker doesn't check.
|
||||
assert.equal(close('anything').class, 'within-role');
|
||||
push(A);
|
||||
const allowed = () => events('action.allowed').filter((e) => JSON.parse(e.body).action === 'task.close').length;
|
||||
const before = allowed();
|
||||
assert.throws(() => close(undefined), /verdict-not-found/);
|
||||
assert.throws(() => close('no-such-verdict'), /verdict-not-found/);
|
||||
const r1 = call('coder', 'review.request', { task_ref: TASK, commit: A, to: 'reviewer' });
|
||||
const changes = call('reviewer', 'review.verdict', { task_ref: TASK, request: r1.request, verdict: 'changes', body: 'no' });
|
||||
assert.throws(() => close(changes.verdict), /verdict-not-approve/);
|
||||
// A verdict on another task doesn't count for this one.
|
||||
push(A, null, 'vikunja:1/3');
|
||||
const other = call('coder', 'review.request', { task_ref: 'vikunja:1/3', commit: A, to: 'reviewer' });
|
||||
const otherOk = call('reviewer', 'review.verdict', { task_ref: 'vikunja:1/3', request: other.request, verdict: 'approve', body: 'ok' });
|
||||
assert.throws(() => close(otherOk.verdict), /verdict-not-found/);
|
||||
const r2 = call('coder', 'review.request', { task_ref: TASK, commit: A, to: 'reviewer' });
|
||||
const approve = call('reviewer', 'review.verdict', { task_ref: TASK, request: r2.request, verdict: 'approve', body: 'ok' });
|
||||
// A push after the approval makes it stale.
|
||||
push(B, A);
|
||||
assert.throws(() => close(approve.verdict), /verdict-stale/);
|
||||
assert.equal(allowed(), before, 'each refused close rolled its action.allowed back');
|
||||
const r3 = call('coder', 'review.request', { task_ref: TASK, commit: B, to: 'reviewer' });
|
||||
const fresh = call('reviewer', 'review.verdict', { task_ref: TASK, request: r3.request, verdict: 'approve', body: 'ok' });
|
||||
assert.equal(close(fresh.verdict).class, 'within-role');
|
||||
assert.equal(allowed(), before + 1);
|
||||
});
|
||||
|
||||
test('a refusal the host decided is filed on the task it names, and only a task ref counts', (t) => {
|
||||
const { b, caps, events } = setup(t);
|
||||
assert.equal(b.refuse(caps.coder, 'non-fast-forward', TASK), 'non-fast-forward');
|
||||
assert.equal(b.refuse(caps.coder, 'invalid-request', 'gitea:1'), 'invalid-request');
|
||||
assert.equal(b.refuse(caps.coder, 'invalid-request'), 'invalid-request');
|
||||
assert.deepEqual(
|
||||
events('action.refused').map((e) => [e.subject, JSON.parse(e.body).code]),
|
||||
[[TASK, 'non-fast-forward'], [null, 'invalid-request'], [null, 'invalid-request']],
|
||||
);
|
||||
});
|
||||
Reference in New Issue
Block a user