From 3a1e11b83ecb471cecc0688b11395329bef6cbc3 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 13 Oct 2026 16:26:12 -0500 Subject: [PATCH] bus: review.request reports whether a reviewer holds the role (#1599) Row 105. Built by a Haiku subagent under Researcher (decision 129), packet agents/researcher/work/queue-105/r1/, candidate manifest cd6d5eb3e4093ac157f63d05cb639d243d76c6bde1a03e78efb8f29e191b6b44. Filbert approved on #1599, comment 28247. The review.request reply and the review.requested event carry held, the run id holding the reviewer role or null. The request_review tool description says what null means. The broker change is live only at a bus restart. Sage's gate rerun on 79e126be0 with rows 101 and 105 applied, Docker up: node suites bus 128, business 67, cli 146, control-board 124, conversation 182, discord 178, harness 70, ledger 78, mosaic 69, queue 155, runs 43, seat 109, tasks 92, webui 108; scripts auth 15, conductor 17, config 24, discord 66, extension-package 18, foundation 44, queue 28, release 14, task 98. All pass, 0 fail. Co-Authored-By: Claude Opus 5.5 --- packages/bus/README.md | 2 +- packages/bus/src/broker.mjs | 8 ++++++-- packages/bus/tests/review.test.mjs | 27 ++++++++++++++++++++++++++- packages/harness/src/tools.mjs | 2 +- packages/harness/tests/tools.test.mjs | 3 +++ 5 files changed, 37 insertions(+), 5 deletions(-) diff --git a/packages/bus/README.md b/packages/bus/README.md index 8f5eec4aa..0cbbc8192 100644 --- a/packages/bus/README.md +++ b/packages/bus/README.md @@ -146,7 +146,7 @@ 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.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,held}` and sends `to` a REVIEW-REQUEST message naming it. `held` is the run id holding the `to` role when the request is routed (row 105, #1599), or `null` when no run holds it: the message then waits for a claim, and the requester should tell whoever launches roles. Returns `{request,message,commit,ref,held}`. | | `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. | diff --git a/packages/bus/src/broker.mjs b/packages/bus/src/broker.mjs index 4ae20e103..732c7310e 100644 --- a/packages/bus/src/broker.mjs +++ b/packages/bus/src/broker.mjs @@ -997,10 +997,14 @@ export class Broker { fail(older ? 'stale-commit' : 'unknown-commit'); } 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. + const h = this.#holder(s.business, a.to); + const held = h?.op === 'claim' ? h.holder_run : null; 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 }, + { task_ref: a.task_ref, project: push.project, ref: push.ref, commit: a.commit, to: a.to, message: mid, held }, a.task_ref, ); this.#reviewMessage(s, mid, { @@ -1009,7 +1013,7 @@ export class Broker { 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 }; + return { request: eid, message: mid, commit: a.commit, ref: push.ref, held }; } // Row 93: the reviewer's answer to one review.request, sent back to the role that asked. case 'review.verdict': { diff --git a/packages/bus/tests/review.test.mjs b/packages/bus/tests/review.test.mjs index 9a11b8034..94074ce95 100644 --- a/packages/bus/tests/review.test.mjs +++ b/packages/bus/tests/review.test.mjs @@ -106,7 +106,7 @@ test('review.request names the latest pushed commit and sends the reviewer a REV 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 }); + assert.deepEqual(JSON.parse(e.body), { task_ref: TASK, project: 'stack', ref: 'work/vikunja-1-2', commit: A, to: 'reviewer', message: r.message, held: 'reviewer-run' }); const m = store.get('SELECT * FROM messages WHERE id=?', r.message); assert.equal(m.to_role, 'reviewer'); assert.equal(m.from_role, 'coder'); @@ -115,6 +115,31 @@ test('review.request names the latest pushed commit and sends the reviewer a REV assert.match(m.body, /please$/); }); +// Row 105 (#1599): the reply and the event say which run holds the reviewer role when the request is +// routed, or null when nobody holds it. A null request waits for the next claim, as before. +test('review.request says which run holds the reviewer role, or null, and a null request waits for a claim', (t) => { + const { b, store, call, push, events } = setup(t); + const routed = (mid) => store.all("SELECT op, holder_run FROM deliveries WHERE message=? AND op='routed' ORDER BY seq", mid).map((r) => ({ ...r })); + push(A); + const held = call('coder', 'review.request', { task_ref: TASK, commit: A, to: 'reviewer' }); + assert.equal(held.held, 'reviewer-run'); + assert.equal(JSON.parse(events('review.requested')[0].body).held, 'reviewer-run'); + assert.deepEqual(routed(held.message), [{ op: 'routed', holder_run: 'reviewer-run' }]); + // The reviewer releases the role. The latest role_claims row is a release, so nobody holds it. + call('reviewer', 'role.release'); + const waiting = call('coder', 'review.request', { task_ref: TASK, commit: A, to: 'reviewer' }); + assert.equal(waiting.held, null); + assert.equal(JSON.parse(events('review.requested')[1].body).held, null); + assert.deepEqual(routed(waiting.message), [{ op: 'routed', holder_run: null }]); + // A run that claims the role is routed the waiting request, as #routeWaiting did before row 105. + const next = b.bindLaunch({ business: 'demo', role: 'reviewer', run: 'reviewer-2', harness: 'pi' }); + b.request(next, { verb: 'role.claim', args: {} }); + assert.deepEqual(routed(waiting.message), [ + { op: 'routed', holder_run: null }, + { op: 'routed', holder_run: 'reviewer-2' }, + ]); +}); + 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/); diff --git a/packages/harness/src/tools.mjs b/packages/harness/src/tools.mjs index 5f27a6bf9..6435140ad 100644 --- a/packages/harness/src/tools.mjs +++ b/packages/harness/src/tools.mjs @@ -110,7 +110,7 @@ export function typedTools({ resolved, arbiter = false, tracker = false, project }, ["task_ref", "project", "branch"])); } if (held.has("review.request")) { - add("request_review", "review.request", "review.request", "Ask a reviewer role to review the task's latest pushed commit. The reviewer gets a REVIEW-REQUEST message, and its verdict comes back to you as a REVIEW-RESULT.", obj({ + add("request_review", "review.request", "review.request", "Ask a reviewer role to review the task's latest pushed commit. The reviewer gets a REVIEW-REQUEST message, and its verdict comes back to you as a REVIEW-RESULT. The reply's held names the run holding the reviewer role, or is null when no reviewer is running. A null request waits until a reviewer launches, so tell whoever launches roles that a review is waiting.", obj({ task_ref: TASK_REF, commit: str("The commit push_working returned", { pattern: "^[0-9a-f]{40}([0-9a-f]{24})?$" }), to: str("Reviewer role instance"), diff --git a/packages/harness/tests/tools.test.mjs b/packages/harness/tests/tools.test.mjs index 232c3db4b..a2bb20556 100644 --- a/packages/harness/tests/tools.test.mjs +++ b/packages/harness/tests/tools.test.mjs @@ -97,6 +97,9 @@ test("the review loop: push_working and request_review for the coder, fetch_revi assert.deepEqual([ask.verb, ask.action], ["review.request", "review.request"]); assert.deepEqual(ask.parameters.required, ["task_ref", "commit", "to"]); assert.equal(ask.parameters.properties.commit.pattern, "^[0-9a-f]{40}([0-9a-f]{24})?$"); + // Row 105 (#1599): the description says what a null held means, and what the requester should do. + assert.match(ask.description, /The reply's held names the run holding the reviewer role, or is null when no reviewer is running\./); + assert.match(ask.description, /A null request waits until a reviewer launches, so tell whoever launches roles that a review is waiting\./); const reviewer = resolvedFor(t, "reviewer"); const rtools = typedTools({ resolved: reviewer, tracker: true, projects: ["stack"] });