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 <[email protected]>

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. |

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': {

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/);

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"),

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"] });