jason.woltje
4ff4f44c98
Unclaimed files left in the canonical checkout. Inventory: canonical-cleanup-inventory.md (commit-as-evidence groups). detect-secrets scan clean on 2026-11-02: hits are sha256 pins, git object ids, test sentinels already in tracked tests and unit-file lines.
115 lines
9.3 KiB
Diff
115 lines
9.3 KiB
Diff
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"] });
|