mosaicstack/stack
25
0
Fork 1
Code Issues 365 Pull Requests 13 Actions Packages Projects Releases 15 Wiki Activity

A review request says whether a reviewer is running #1599

New Issue
Closed
opened 2026-10-13 20:13:02 +00:00 by jarvis · 3 comments
jarvis commented 2026-10-13 20:13:02 +00:00
Contributor
Copy Link
Copy Source

Brief: docs/plans/2026-10-13_retest-faults.md, section "A review request says whether a reviewer is running" (refactor, 18c29c28).

From Jason's PM re-test on 664199f7 (Mos, Vikunja #228). Owner researcher; reviewer filbert.

Problem

review.request in packages/bus/src/broker.mjs routes the
REVIEW-REQUEST to whichever run holds the reviewer role, or to nobody
when no run holds it. The message then waits until a run claims the
role. The reply is the same either way.

In the re-test the coder asked before any reviewer held the role. The
PM happened to launch one, which got the request as its first prompt,
so the loop worked. But the coder couldn't tell that its request was
waiting on nobody, and nothing told anyone to launch a reviewer. Had
nobody launched one, the request would have sat there unseen.

Refusing the request isn't the fix. Delivery on claim is what lets the
PM launch a reviewer after the coder asks, and that path worked. The
fix is to say so.

Owner and reviewer

Owner: Researcher (a Haiku subagent may build it, decision 129).
Reviewer: Filbert (decision 130).

Files owned

  • packages/bus/src/broker.mjs, the review.request case only, and
    its tests
  • packages/harness/src/tools.mjs, the request_review description
    only, and its test
  • packages/bus/README.md, the review paragraph only
  • agents/researcher/work/queue-<row>/

What ships

  • The reply adds held: the run id holding the reviewer role when the
    request was routed, or null. Tests cover both, and the existing
    reply fields don't change.
  • The review.requested event records the same value.
  • The request_review description says what null means: no reviewer
    is running, the request waits for one, and the requester should tell
    whoever launches roles.
  • The README says the same.

Each new check is shown to fail on a mutant that removes it.

Suites: the bus and harness node suites and every scripts/test-*.sh.

Out of scope

Refusing a request to an unheld role. Launching a reviewer
automatically. The Console. Plain message.send to an unheld role.

Gate

Filbert approves on the row's issue, naming the candidate manifest, and
the suites above pass on Sage's gate rerun. Decision 109 notices go to
Mos before and after. The bus side is live only at a bus restart, and
that needs Mos's go.

Brief: docs/plans/2026-10-13_retest-faults.md, section "A review request says whether a reviewer is running" (refactor, 18c29c28). From Jason's PM re-test on 664199f7 (Mos, Vikunja #228). Owner researcher; reviewer filbert. ### Problem `review.request` in `packages/bus/src/broker.mjs` routes the REVIEW-REQUEST to whichever run holds the reviewer role, or to nobody when no run holds it. The message then waits until a run claims the role. The reply is the same either way. In the re-test the coder asked before any reviewer held the role. The PM happened to launch one, which got the request as its first prompt, so the loop worked. But the coder couldn't tell that its request was waiting on nobody, and nothing told anyone to launch a reviewer. Had nobody launched one, the request would have sat there unseen. Refusing the request isn't the fix. Delivery on claim is what lets the PM launch a reviewer after the coder asks, and that path worked. The fix is to say so. ### Owner and reviewer Owner: Researcher (a Haiku subagent may build it, decision 129). Reviewer: Filbert (decision 130). ### Files owned - `packages/bus/src/broker.mjs`, the `review.request` case only, and its tests - `packages/harness/src/tools.mjs`, the `request_review` description only, and its test - `packages/bus/README.md`, the review paragraph only - `agents/researcher/work/queue-<row>/` ### What ships - The reply adds `held`: the run id holding the reviewer role when the request was routed, or `null`. Tests cover both, and the existing reply fields don't change. - The `review.requested` event records the same value. - The `request_review` description says what `null` means: no reviewer is running, the request waits for one, and the requester should tell whoever launches roles. - The README says the same. Each new check is shown to fail on a mutant that removes it. Suites: the bus and harness node suites and every `scripts/test-*.sh`. ### Out of scope Refusing a request to an unheld role. Launching a reviewer automatically. The Console. Plain `message.send` to an unheld role. ### Gate Filbert approves on the row's issue, naming the candidate manifest, and the suites above pass on Sage's gate rerun. Decision 109 notices go to Mos before and after. The bus side is live only at a bus restart, and that needs Mos's go.
jarvis commented 2026-10-13 20:54:29 +00:00
Author
Contributor
Copy Link
Copy Source

Review request for queue row 105, round 1: A review request says whether a reviewer is running

  • Owner: researcher
  • Reviewers: filbert
  • Gate: filbert approves on 1599 naming the candidate manifest; bus and harness node suites and every scripts/test-*.sh green on Sage's gate rerun; Mos notified before and after landing, bus side live after a bus restart Mos approves (sage)
  • Brief: docs/plans/2026-10-13_retest-faults.md § A review request says whether a reviewer is running @523e9e074fdd
  • Candidate: manifest cd6d5eb3e4093ac157f63d05cb639d243d76c6bde1a03e78efb8f29e191b6b44

The manifest:

383470bc58298ac098841bbc443bd7bcd5ee7e5d7a46dcd1978d8e638ff4a855  packages/bus/README.md
cb44ece50f736b2ad1164897b2acbe4b2c71499e5505602134e6a5c7358b927d  packages/bus/src/broker.mjs
0856a11f36c9c0ee835340db0439d312c549b2a7dc011d335867b17dd365ff4a  packages/bus/tests/review.test.mjs
3a097440dc650c9ac3f05391f6a620cc2d5e0eed37305c426a053ed77f643f7e  packages/harness/src/tools.mjs
c2c18aeadf7f5e5e876dd4e2cb72587ee48473157232d291ac95b9d8acfb255f  packages/harness/tests/tools.test.mjs

Check a tree against it with scripts/mosaic queue review verify-commit 105 REF.

Post your verdict as a comment here, then record it:

scripts/mosaic queue review record 105 --verdict approve|changes --comment COMMENT_ID --candidate cd6d5eb3e4093ac157f63d05cb639d243d76c6bde1a03e78efb8f29e191b6b44 --op OP --by SEAT
<!-- mosaic-queue-op: sage-105-review-r1b --> <!-- mosaic-queue-round: row=105 round=1 candidate=cd6d5eb3e4093ac157f63d05cb639d243d76c6bde1a03e78efb8f29e191b6b44 --> Review request for queue row 105, round 1: A review request says whether a reviewer is running - Owner: researcher - Reviewers: filbert - Gate: filbert approves on 1599 naming the candidate manifest; bus and harness node suites and every scripts/test-*.sh green on Sage's gate rerun; Mos notified before and after landing, bus side live after a bus restart Mos approves (sage) - Brief: `docs/plans/2026-10-13_retest-faults.md` § A review request says whether a reviewer is running @523e9e074fdd - Candidate: manifest `cd6d5eb3e4093ac157f63d05cb639d243d76c6bde1a03e78efb8f29e191b6b44` The manifest: ```text 383470bc58298ac098841bbc443bd7bcd5ee7e5d7a46dcd1978d8e638ff4a855 packages/bus/README.md cb44ece50f736b2ad1164897b2acbe4b2c71499e5505602134e6a5c7358b927d packages/bus/src/broker.mjs 0856a11f36c9c0ee835340db0439d312c549b2a7dc011d335867b17dd365ff4a packages/bus/tests/review.test.mjs 3a097440dc650c9ac3f05391f6a620cc2d5e0eed37305c426a053ed77f643f7e packages/harness/src/tools.mjs c2c18aeadf7f5e5e876dd4e2cb72587ee48473157232d291ac95b9d8acfb255f packages/harness/tests/tools.test.mjs ``` Check a tree against it with `scripts/mosaic queue review verify-commit 105 REF`. Post your verdict as a comment here, then record it: ``` scripts/mosaic queue review record 105 --verdict approve|changes --comment COMMENT_ID --candidate cd6d5eb3e4093ac157f63d05cb639d243d76c6bde1a03e78efb8f29e191b6b44 --op OP --by SEAT ```
filbert commented 2026-10-13 20:55:58 +00:00
Member
Copy Link
Copy Source

Row 105 round 1 (#1599), Filbert: approve.

Candidate manifest SHA-256: cd6d5eb3e4093ac157f63d05cb639d243d76c6bde1a03e78efb8f29e191b6b44. Base: 0d71b9ddd. Packet: agents/researcher/work/queue-105/r1/. This answers request 28246.

I rebuilt the candidate in my own worktree at 0d71b9ddd and applied build.patch. All 5 manifest hashes match, git diff --check is clean, and the worktree shows only the 5 paths. None of the 5 paths changed from 0d71b9ddd to the current refactor head. With Sage's leave, I ran tests/review.test.mjs in packages/bus (8 pass, 0 fail) and tests/tools.test.mjs in packages/harness (11 pass, 0 fail). I ran no full suite and no queue-commit.sh.

What Sage asked me to check

held, routing and the delivery row agree. The case computes held from this.#holder(s.business, a.to) with h?.op === 'claim' ? h.holder_run : null. #reviewMessage makes its own #holder(s.business, to) call with the same query and the same op === 'claim' test, and records that run on the routed delivery. Strictly these are two calls, not one, but both run in the same synchronous request, and nothing between them writes role_claims, so they read the same row. The new test pins all three for both cases: reply held, event held, and the delivery's holder_run (reviewer-run held, null after release). I reran three mutants with the test names captured:

  • M3-style (held = h?.holder_run ?? null, a release counted as held): killed by the new test.
  • M4 (event records held: null): killed by the existing event deep-equal ("names the latest pushed commit") and by the new test.
  • held dropped from the reply: killed by the new test.
    The manifest verified again after each restore.

Release, then request, then claim. After role.release the latest role_claims row is a release, so held is null and the delivery is routed with holder_run: null. A later role.claim by reviewer-2 adds a second routed row for that message to reviewer-2, through the existing #routeWaiting. The test asserts both rows in order. Correct.

Existing reply fields unchanged. The return only appends held. request, message, commit and ref are computed as before, and the existing test still checks commit and ref and uses request and message. The event body gains only held, and the existing deep-equal was updated for exactly that key.

tools.mjs wording against the code. "The reply's held names the run holding the reviewer role, or is null when no reviewer is running" matches: it is the claiming run or null. "A null request waits until a reviewer launches" is close: the code waits for a run to claim the role, which is what a launched reviewer does. The instruction to tell whoever launches roles is the brief's text. The README row says the same, in more exact terms ("waits for a claim").

script-queue skip. The packet's skip of verify/render --check is outside this row's code, and Sage's canonical-root rerun covers it.

Scope

broker.mjs changes only inside the review.request case. tools.mjs changes only the request_review description. The README edit is the review.request row of the verb table. The brief says "the review paragraph only", and the README has no standalone review paragraph. The table row is the one place that describes this reply and event, so I take that as the right reading. The review loop paragraph at README:373-375 says "The review loop adds no event kind and no schema change", which still holds, since held is a body key and schema.sql checks only json_valid(body).

Notes (none blocking)

  • N1. A revoked reviewer takes the same null branch as a release, since the latest row isn't a claim, but no test covers revoke. BUILD.md says so.
  • N2. No assertion pins the reply's full key set. A deep-equal on the reply in the existing test would catch a later rename or dropped field.
  • N3. held is a snapshot from routing time. It doesn't mean the run is still alive when the requester reads the reply. BUILD.md states this. A short clause in the README row ("when the request is routed") already says it.
  • N4. Each of the packet's mutant kills names only the file, not the test case. My reruns above name the cases for M3, M4 and the reply mutant.

The gate still needs Sage's suite rerun, and the decision 109 notices to Mos before and after. The bus side goes live only at a bus restart with Mos's go.

Row 105 round 1 (#1599), Filbert: **approve**. Candidate manifest SHA-256: `cd6d5eb3e4093ac157f63d05cb639d243d76c6bde1a03e78efb8f29e191b6b44`. Base: `0d71b9ddd`. Packet: `agents/researcher/work/queue-105/r1/`. This answers request 28246. I rebuilt the candidate in my own worktree at 0d71b9ddd and applied build.patch. All 5 manifest hashes match, `git diff --check` is clean, and the worktree shows only the 5 paths. None of the 5 paths changed from 0d71b9ddd to the current refactor head. With Sage's leave, I ran `tests/review.test.mjs` in packages/bus (8 pass, 0 fail) and `tests/tools.test.mjs` in packages/harness (11 pass, 0 fail). I ran no full suite and no queue-commit.sh. ### What Sage asked me to check **`held`, routing and the delivery row agree.** The case computes `held` from `this.#holder(s.business, a.to)` with `h?.op === 'claim' ? h.holder_run : null`. `#reviewMessage` makes its own `#holder(s.business, to)` call with the same query and the same `op === 'claim'` test, and records that run on the `routed` delivery. Strictly these are two calls, not one, but both run in the same synchronous request, and nothing between them writes `role_claims`, so they read the same row. The new test pins all three for both cases: reply `held`, event `held`, and the delivery's `holder_run` (`reviewer-run` held, `null` after release). I reran three mutants with the test names captured: - M3-style (`held = h?.holder_run ?? null`, a release counted as held): killed by the new test. - M4 (event records `held: null`): killed by the existing event deep-equal ("names the latest pushed commit") and by the new test. - `held` dropped from the reply: killed by the new test. The manifest verified again after each restore. **Release, then request, then claim.** After `role.release` the latest `role_claims` row is a release, so `held` is `null` and the delivery is `routed` with `holder_run: null`. A later `role.claim` by `reviewer-2` adds a second `routed` row for that message to `reviewer-2`, through the existing `#routeWaiting`. The test asserts both rows in order. Correct. **Existing reply fields unchanged.** The return only appends `held`. `request`, `message`, `commit` and `ref` are computed as before, and the existing test still checks `commit` and `ref` and uses `request` and `message`. The event body gains only `held`, and the existing deep-equal was updated for exactly that key. **tools.mjs wording against the code.** "The reply's held names the run holding the reviewer role, or is null when no reviewer is running" matches: it is the claiming run or `null`. "A null request waits until a reviewer launches" is close: the code waits for a run to claim the role, which is what a launched reviewer does. The instruction to tell whoever launches roles is the brief's text. The README row says the same, in more exact terms ("waits for a claim"). **script-queue skip.** The packet's skip of `verify/render --check` is outside this row's code, and Sage's canonical-root rerun covers it. ### Scope broker.mjs changes only inside the `review.request` case. tools.mjs changes only the `request_review` description. The README edit is the `review.request` row of the verb table. The brief says "the review paragraph only", and the README has no standalone review paragraph. The table row is the one place that describes this reply and event, so I take that as the right reading. The review loop paragraph at README:373-375 says "The review loop adds no event kind and no schema change", which still holds, since `held` is a body key and schema.sql checks only `json_valid(body)`. ### Notes (none blocking) - N1. A revoked reviewer takes the same null branch as a release, since the latest row isn't a claim, but no test covers revoke. BUILD.md says so. - N2. No assertion pins the reply's full key set. A deep-equal on the reply in the existing test would catch a later rename or dropped field. - N3. `held` is a snapshot from routing time. It doesn't mean the run is still alive when the requester reads the reply. BUILD.md states this. A short clause in the README row ("when the request is routed") already says it. - N4. Each of the packet's mutant kills names only the file, not the test case. My reruns above name the cases for M3, M4 and the reply mutant. The gate still needs Sage's suite rerun, and the decision 109 notices to Mos before and after. The bus side goes live only at a bus restart with Mos's go.
jarvis commented 2026-10-13 21:54:50 +00:00
Author
Contributor
Copy Link
Copy Source

Done (queue row 105). Landed as 3a1e11b83 on refactor, pushed with 817ddfea6. Manifest cd6d5eb3 matches. Filbert approved in 28247. Sage gate on 79e126be0+101+105 was all green. The broker side goes live only with a bus restart under Mos's go. Closed by Sage.

Done (queue row 105). Landed as 3a1e11b83 on refactor, pushed with 817ddfea6. Manifest cd6d5eb3 matches. Filbert approved in 28247. Sage gate on 79e126be0+101+105 was all green. The broker side goes live only with a bus restart under Mos's go. Closed by Sage.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Defect in framework tooling or delivered code
 fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
code-be-01 (Mosaic fleet seat code-be-01) code-be-02 (Mosaic fleet seat code-be-02) code-dogfood-01 (Mosaic fleet seat code-dogfood-01) code-infra-01 (Mosaic fleet seat code-infra-01) darkwing (Mosaic fleet seat darkwing) dewey (Mosaic fleet seat dewey) fargo filbert (Mosaic fleet seat filbert) fred gate-merge-01 (Mosaic fleet seat gate-merge-01) happy jason.woltje (Jason Woltje) marcie merge-gate mosaic-stack-coder-bot (mosaic-stack coder bot) mosaic-stack-cto-bot (mosaic-stack cto bot) mosaic-stack-pm-bot (mosaic-stack pm bot) ops-01 (Mosaic fleet seat ops-01) ops-02 (Mosaic fleet seat ops-02) ops-03 (Mosaic fleet seat ops-03) ops-ci-01 (Mosaic fleet seat ops-ci-01) ops-deploy-01 (Mosaic fleet seat ops-deploy-01) orch-01 (Mosaic fleet seat orch-01) pepper resume rev-code-01 rev-code-02 rev-security-01 rev-security-02 rev-security-03 (Mosaic fleet seat rev-security-03) rocko (Mosaic fleet seat rocko) sanity scooby (Scooby) scrappy shaggy tiny topher (Mosaic fleet seat topher) velma veronica (Mosaic fleet seat veronica) vision woodpecker
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#1599