fire(trigger) step 4 is loop-shaped, so reviewers get dispatched serially #88

New issue

Open

opened 2026-07-28 22:08:27 +00:00 by crenshawdev · 0 comments

crenshawdev commented

2026-07-28 22:08:27 +00:00

(Migrated from github.com)

But references/review-triggers.md:55 - the file a coordinator actually executes fire(trigger) from - restates it as:

For each reviewer in the set, in parallel where the host allows:

followed by one long bullet per backend. Two problems:

For each is a serial construct. The parallelism is a subordinate clause inside a loop, and where the host allows reads as permission to decide the host might not allow it.
Neither the mandated in ONE message phrasing nor a pointer to seams.md Concurrent dispatch appears.

Every sibling surface states it mechanically - workflows/execute.md:186 ("CONCURRENTLY in one message"), workflows/decision-review.md:70 ("dispatch them CONCURRENTLY in one message"), workflows/phase.md:49 ("batch them in one message; conventions.md Parallel work"), workflows/docs-verify.md:26. review-triggers.md is the only one phrased as permission.

Observed: during /cad-execute 1 the diff trigger dispatched cad-reviewer (~470s), waited for its full return, then started the cross-model provider call. Doubled wall-clock and spend for no added signal, on an advisory gate that could not change the outcome.

Acceptance criteria

references/review-triggers.md step 4 is batch-shaped, not loop-shaped: the unit issued is the whole reviewer set in ONE message, with the per-backend notes describing how to READ each result rather than when to issue it. It cites seams.md Concurrent dispatch, and the where the host allows hedge is gone.
Step 3 resolves the route ONCE for the set rather than per reviewer (seams.md:122 already requires this and step 4 does not say it).
self-verify.mjs gains a check over cadence-core/workflows/*.md and cadence-core/references/*.md that fails when a multi-dispatch instruction lacks the mandated one-message phrasing, so the weak restatement cannot reappear. Same species as the phase-1 checks 7 and 8.
Consider whether fire() should hand back a resolved dispatch manifest (every reviewer, route resolved once, payload attached) instead of a list the coordinator walks - holding one object makes the batch the obvious next action. Design call, not required by 1-3.

Also worth deciding

fire() spends the same on an advisory gate as on a blocking one. Nothing in the procedure tells the coordinator to weigh cost against a gate that cannot halt anything. Possibly a note in step 1, possibly out of scope here.

seams.md:119 "Concurrent dispatch" already states the rule and names this exact case as an example: "Independent dispatches over disjoint payloads (the per-plan executors of a parallel phase, per-doc verifiers, **the two reviewers of one artifact**) fire concurrently in ONE message." But `references/review-triggers.md:55` - the file a coordinator actually executes `fire(trigger)` from - restates it as: > For each reviewer in the set, in parallel where the host allows: followed by one long bullet per backend. Two problems: 1. `For each` is a serial construct. The parallelism is a subordinate clause inside a loop, and `where the host allows` reads as permission to decide the host might not allow it. 2. Neither the mandated `in ONE message` phrasing nor a pointer to seams.md Concurrent dispatch appears. Every sibling surface states it mechanically - `workflows/execute.md:186` ("CONCURRENTLY in one message"), `workflows/decision-review.md:70` ("dispatch them CONCURRENTLY in one message"), `workflows/phase.md:49` ("batch them in one message; conventions.md Parallel work"), `workflows/docs-verify.md:26`. review-triggers.md is the only one phrased as permission. Observed: during `/cad-execute 1` the `diff` trigger dispatched `cad-reviewer` (~470s), waited for its full return, then started the cross-model provider call. Doubled wall-clock and spend for no added signal, on an `advisory` gate that could not change the outcome. ## Acceptance criteria 1. `references/review-triggers.md` step 4 is batch-shaped, not loop-shaped: the unit issued is the whole reviewer set in ONE message, with the per-backend notes describing how to READ each result rather than when to issue it. It cites seams.md Concurrent dispatch, and the `where the host allows` hedge is gone. 2. Step 3 resolves the route ONCE for the set rather than per reviewer (seams.md:122 already requires this and step 4 does not say it). 3. `self-verify.mjs` gains a check over `cadence-core/workflows/*.md` and `cadence-core/references/*.md` that fails when a multi-dispatch instruction lacks the mandated one-message phrasing, so the weak restatement cannot reappear. Same species as the phase-1 checks 7 and 8. 4. Consider whether `fire()` should hand back a resolved dispatch manifest (every reviewer, route resolved once, payload attached) instead of a list the coordinator walks - holding one object makes the batch the obvious next action. Design call, not required by 1-3. ## Also worth deciding `fire()` spends the same on an `advisory` gate as on a `blocking` one. Nothing in the procedure tells the coordinator to weigh cost against a gate that cannot halt anything. Possibly a note in step 1, possibly out of scope here.