* test(#3034): failing-first coverage for opt-in parallel reviewer lanes Executes the real invoke_reviewers dispatch block from review.md against a stubbed gsd_run seam rather than pattern-matching the workflow text, so the two properties that actually carry risk are observable: that every lane is joined before aggregation, and that concurrent lanes cannot tear a line in gsd-review-lane-results.jsonl. Concurrency is proven by a barrier fixture, not by elapsed time -- each stub lane blocks until all lanes have checked in, which can only complete if they overlap. Red against the current sequential dispatch, by design. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3034): add opt-in parallel reviewer lanes Reviewer lanes within one review pass inspect the same immutable plan snapshot and have no dependency on one another, but were dispatched strictly one at a time, so a multi-reviewer pass cost roughly the sum of its lanes. The serialization is a deliberate protection against provider rate limits, so it stays the default; review.parallel_lanes opts a project out of it. The loop body is hoisted into run_review_lane so the sequential and concurrent paths share one body -- two hand-synced dispatch bodies is the divergence class ADR-2782 spent a phase deleting. Each lane writes a slug-scoped result file, concatenated in selection order after the join: concurrent O_APPEND is atomic only below PIPE_BUF, and write_reviews parses that JSONL to render the models:/model_sources: frontmatter, so a torn line is a broken REVIEWS.md rather than a cosmetic log defect. Aggregating in selection order also keeps the artifact byte-identical between the two paths. The guard is strict equality on "true" and falls back to sequential when config-get fails -- the opposite polarity from the commit_docs guard, because failing open here fires the very requests the default prevents. Also corrects docs/COMMANDS.md and its four locale mirrors, which described --all as running every configured reviewer in parallel when dispatch was in fact sequential. Closes #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3034): de-duplicate dispatch slugs and scope lane locals Review finding (Standards axis): a slug repeated in SELECTED_REVIEWERS would put two concurrent background jobs on the same > -truncated per-lane result file. The shared-append form this replaced could not corrupt itself that way, so de-duplicating is what keeps the concurrent path no worse than the sequential one. Selection de-dupes today -- the roster is a Set and review.default_reviewers normalizes lowercase-unique -- but reachability analysis is not a contract, which is the same reason the roster derivation itself is guarded. Splitting once into DISPATCH_SLUGS also removes the duplicated tr-split the same review flagged: the dispatch and aggregation loops now share one list, which is what guarantees they walk the same slugs in the same order. A plain string accumulator rather than an array, because zsh and bash disagree on array indexing and this block runs under both. Also scopes run_review_lane's locals. Not a live fix -- each dispatched call already forks its own subshell -- but it makes the isolation a property of the function rather than of the dispatch mechanism happening to fork. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3034): acknowledge review.md growth, drop spent 2295 ack The differential attribution gate reported review.md growing 4173 bytes (30712 -> 34885) with no live acknowledgment. Adds the per-PR fragment it asks for, naming only the one path it reported. Deleting tests/emitted-drift-acks/2295-resolved-model.json is required, not opportunistic. That fragment declared review.md and nothing else, and its ripple is already absorbed into the base, so it is spent -- it can no longer clear anything, which is why the gate still reported review.md as unacknowledged. It could not simply be left alone either: two ack sources may never name the same path, so it blocked this PR's fragment outright. CONTRIBUTING is explicit that a fragment whose last entry is removed gets deleted with it, because an empty fragment signals nothing while its presence reads as a live alarm. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3034): backfill changeset PR number Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
131
docs/how-to/enable-parallel-reviewer-lanes.md
Normal file
131
docs/how-to/enable-parallel-reviewer-lanes.md
Normal file
@@ -0,0 +1,131 @@
|
||||
# How to enable parallel reviewer lanes
|
||||
|
||||
Cut the wall-clock cost of a multi-reviewer `/gsd-review` pass from the sum of its lanes toward
|
||||
its slowest lane — without losing the rate-limit protection the sequential default exists to
|
||||
provide.
|
||||
|
||||
> **Default-off, and deliberately so.** Reviewer lanes are dispatched one at a time because
|
||||
> concurrent invocation can trip provider rate limits, and a lane lost to a rate limit is a
|
||||
> cross-AI review that quietly went blind in one eye. Turning this on is you asserting that your
|
||||
> providers can take the concurrency. Nothing detects that for you.
|
||||
|
||||
**What you need:**
|
||||
|
||||
- Two or more reviewer lanes that actually run on this host. With one lane there is nothing to
|
||||
overlap and the setting changes nothing.
|
||||
- Provider capacity for concurrent requests — separate accounts, generous quota, or local model
|
||||
servers (`ollama`, `lm-studio`, `llama-cpp`) that have no external limit at all.
|
||||
|
||||
---
|
||||
|
||||
## Step 1 — Check what your review pass actually runs
|
||||
|
||||
Parallelism only helps if several lanes are selected. Confirm the set first:
|
||||
|
||||
```bash
|
||||
gsd config-get review.default_reviewers
|
||||
```
|
||||
|
||||
If that returns `Key not found`, no-flag runs use every detected reviewer and `--all` is
|
||||
redundant. If it names a single reviewer, stop here — enabling the key would change nothing.
|
||||
|
||||
---
|
||||
|
||||
## Step 2 — Turn the key on
|
||||
|
||||
```bash
|
||||
gsd config-set review.parallel_lanes true
|
||||
```
|
||||
|
||||
Verify it took:
|
||||
|
||||
```bash
|
||||
gsd config-get review.parallel_lanes --raw
|
||||
# → true
|
||||
```
|
||||
|
||||
**The guard is strict equality.** Only the exact value `true` opts in. `"1"`, `"yes"`, `"on"` and
|
||||
`"TRUE"` are all read as *not enabled* and leave dispatch sequential. This is intentional: a
|
||||
mistyped config gets the conservative behavior rather than firing concurrent requests at a
|
||||
rate-limited provider. If `config-get` shows anything other than `true`, the setting is off.
|
||||
|
||||
---
|
||||
|
||||
## Step 3 — Run a review and read the result
|
||||
|
||||
```bash
|
||||
/gsd-review --phase 3 --all
|
||||
```
|
||||
|
||||
or, for the convergence loop:
|
||||
|
||||
```bash
|
||||
/gsd-plan-review-convergence 3 --all
|
||||
```
|
||||
|
||||
Open `{phase_dir}/{padded_phase}-REVIEWS.md` and check the `reviewers:` frontmatter list. Every
|
||||
lane you selected must appear there. That list is the contract: lanes are joined before the file
|
||||
is rendered, so a missing reviewer means that lane did not produce a review — never that
|
||||
aggregation ran early.
|
||||
|
||||
Section order in `REVIEWS.md`, and line order in the run's `gsd-review-lane-results.jsonl`, are
|
||||
unchanged from sequential dispatch. They follow reviewer-selection order, not completion order,
|
||||
so a diff of two runs shows no reordering churn.
|
||||
|
||||
---
|
||||
|
||||
## Telling the outcomes apart
|
||||
|
||||
The single most useful habit: **an empty or stub review is a dropped lane, not a clean review.**
|
||||
That is true sequentially too, but concurrency gives you more ways to drop one at once.
|
||||
|
||||
| What you see | What it means | What to do |
|
||||
|---|---|---|
|
||||
| Every selected lane in `reviewers:`, all sections populated | Working as intended | Nothing |
|
||||
| A lane's section carries the "failed or returned empty output" header | The lane ran and produced nothing usable. Read the captured stderr in the stub | If it names a rate limit or quota, your provider cannot take this concurrency — see below |
|
||||
| A lane reports `probe_timeout` or `host_unreachable` | The lane could not be reached at all — a local server that is down, not a concurrency effect | Start the server; unrelated to this setting |
|
||||
| A lane reports `budget_too_small` | Its prompt budget cannot fit the minimum review set | Raise `review.max_prompt_tokens_per_reviewer.<slug>`; unrelated to this setting |
|
||||
| A lane reports `egress_host_changed` | The lane was consented to one destination and the config now names another. It is blocked, not redirected | Re-consent deliberately; unrelated to this setting |
|
||||
| A lane is missing from `reviewers:` entirely | It was never selected | Check your flags and `review.default_reviewers` |
|
||||
| Several lanes stub out at once, with provider errors | The concurrency is more than your account can take | Turn the key back off, or narrow `review.default_reviewers` |
|
||||
|
||||
**Rate-limited lanes fail loudly.** A lane that gets throttled goes down the same path as any
|
||||
other failing lane — a diagnostic stub carrying its stderr, kept distinguishable from a real
|
||||
review by its header. It is not silently dropped and it does not abort its sibling lanes.
|
||||
|
||||
---
|
||||
|
||||
## Turning it back off
|
||||
|
||||
```bash
|
||||
gsd config-set review.parallel_lanes false
|
||||
```
|
||||
|
||||
The next pass dispatches sequentially again. Nothing else changes: no artifact written under the
|
||||
parallel setting needs migrating, because the output layout is identical in both modes.
|
||||
|
||||
---
|
||||
|
||||
## What this does not speed up
|
||||
|
||||
**Convergence cycles stay sequential.** `/gsd-plan-review-convergence` runs
|
||||
`plan-phase → review → replan → re-review`, and each cycle genuinely depends on the previous
|
||||
one's output. Enabling this key makes each *review pass* inside a cycle faster; it does not
|
||||
reduce the number of cycles, and it does not overlap planning with reviewing. A three-cycle
|
||||
convergence run is still three sequential rounds.
|
||||
|
||||
**There is no concurrency bound.** Every selected lane dispatches at once. Eleven selected lanes
|
||||
means eleven concurrent requests. If that is more than you want, the control is the size of your
|
||||
selected set — `review.default_reviewers` or explicit flags — not a throttle on this setting.
|
||||
|
||||
**Reviewer instances sharing an adapter are not grouped.** Two
|
||||
[`review.reviewer_instances`](../CONFIGURATION.md#reviewer-instances-for-gsd-review-1517) entries
|
||||
backed by the same CLI dispatch concurrently against that one provider. If you run several
|
||||
same-provider instances, you are the most likely configuration to hit a limit, and this setting
|
||||
gives you no way to serialize just those.
|
||||
|
||||
---
|
||||
|
||||
**See also:** [Configuration reference](../CONFIGURATION.md#parallel-reviewer-lanes-for-gsd-review-3034)
|
||||
· [Reviewer instances](../CONFIGURATION.md#reviewer-instances-for-gsd-review-1517)
|
||||
· [`/gsd-review` command reference](../COMMANDS.md)
|
||||
Reference in New Issue
Block a user