From a2387a05454f0ff84a7bc5308ce9465a36ec2e01 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 24 Aug 2026 15:25:22 -0400 Subject: [PATCH] feat(#3034): add opt-in parallel reviewer lanes (#3822) * 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 * 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 * 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 * 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 * chore(#3034): backfill changeset PR number Refs #3034 Co-Authored-By: Claude Opus 5 --------- Co-authored-by: sim Co-authored-by: Claude Opus 5 --- .changeset/zesty-sloths-sprint.md | 5 + docs/COMMANDS.md | 2 +- docs/CONFIGURATION.md | 43 +- docs/FEATURES.md | 22 + docs/README.md | 1 + docs/how-to/enable-parallel-reviewer-lanes.md | 131 ++++ docs/ja-JP/COMMANDS.md | 2 +- docs/ko-KR/COMMANDS.md | 2 +- docs/pt-BR/COMMANDS.md | 2 +- docs/zh-CN/COMMANDS.md | 2 +- .../bin/shared/config-schema.manifest.json | 1 + gsd-core/workflows/review.md | 77 ++- .../3034-parallel-reviewer-lanes.json | 8 + tests/review-parallel-lanes.test.cjs | 570 ++++++++++++++++++ 14 files changed, 858 insertions(+), 10 deletions(-) create mode 100644 .changeset/zesty-sloths-sprint.md create mode 100644 docs/how-to/enable-parallel-reviewer-lanes.md create mode 100644 tests/emitted-drift-acks/3034-parallel-reviewer-lanes.json create mode 100644 tests/review-parallel-lanes.test.cjs diff --git a/.changeset/zesty-sloths-sprint.md b/.changeset/zesty-sloths-sprint.md new file mode 100644 index 000000000..3476c8b5f --- /dev/null +++ b/.changeset/zesty-sloths-sprint.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3822 +--- +**`/gsd-review` can now dispatch reviewer lanes concurrently** — a multi-reviewer pass cost roughly the sum of its lanes even though every lane inspects the same immutable plan snapshot and none depends on another. Set `review.parallel_lanes` to `true` to overlap them within a single pass; the default stays sequential and keeps the provider-rate-limit protection, and convergence cycles stay sequential either way. This also corrects `docs/COMMANDS.md`, which described `--all` as running every configured reviewer in parallel when dispatch was in fact sequential. (#3034) diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 262a3ef1a..b0d2719cd 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -267,7 +267,7 @@ Cross-AI plan convergence loop — replan with review feedback until no HIGH con |-----------------|----------|-------------| | `N` | **Yes** | Phase number to plan and review | | Reviewer flags | No | Pass through every reviewer lane flag: `--gemini`, `--claude`, `--codex`, `--coderabbit`, `--opencode`, `--qwen`, `--cursor`, `--agy` / `--antigravity`, `--ollama`, `--lm-studio`, `--llama-cpp`, `--kimi-code` | -| `--all` | No | Run every configured reviewer in parallel | +| `--all` | No | Run every configured reviewer. Lanes are dispatched **sequentially by default**; set `review.parallel_lanes` to `true` to dispatch them concurrently within a single review pass | | `--max-cycles N` | No | Override cycle cap (default 3) | **Exit behavior:** Loop exits when both `current_high` and `current_actionable` hit zero. Stall detection warns when the total unresolved review count is not decreasing across cycles. Escalation gate asks the user to proceed or review manually when `--max-cycles` is hit with HIGH or actionable non-HIGH concerns still open. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 9127e425a..9228b9394 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -85,7 +85,8 @@ GSD stores project settings in `.planning/config.json`. Created during `/gsd-new "review": { "default_reviewers": null, "reviewer_instances": {}, - "models": {} + "models": {}, + "parallel_lanes": false }, "parallelization": { "enabled": true, @@ -293,6 +294,45 @@ Example: } ``` +### Parallel reviewer lanes for `/gsd-review` (#3034) + +By default `/gsd-review` invokes reviewer lanes one at a time. That is deliberate: concurrent +invocation can trip provider rate limits, and a lane dropped to a rate limit is a review that +silently lost an opinion. A pass with several reviewers therefore costs roughly the sum of their +runtimes. + +The lanes within one review pass have no data dependency on one another — they all inspect the +same immutable plan snapshot. If your providers can accept concurrent requests (independent +accounts, generous quota, or local model servers), you can opt in: + +| Setting | Type | Default | Description | +|---------|------|---------|-------------| +| `review.parallel_lanes` | boolean | `false` | When `true`, dispatch independent selected reviewer lanes concurrently within a single `/gsd-review` pass. All lanes are joined before `REVIEWS.md` and consensus are rendered. | + +```bash +gsd config-set review.parallel_lanes true +/gsd-plan-review-convergence 3 --all +``` + +```json +{ + "review": { + "parallel_lanes": true + } +} +``` + +**What this does not change.** Convergence cycles stay sequential — `review → replan → re-review` +has a real data dependency, so enabling this speeds up each pass, not the number of passes. +Per-lane timeouts, prompt budgets, diagnostic stubs, explicit-lane failure, trust/egress checks and +result-file layout are all unchanged. + +**Before you enable it.** Every selected lane dispatches at once — there is no concurrency bound. +Selecting eleven lanes issues eleven concurrent requests. Two reviewer instances backed by the same +adapter (see below) also dispatch concurrently against that one provider, which is the most likely +way to hit a limit. If a lane does get rate-limited it fails the way any other failing lane does: +a diagnostic stub with captured stderr, never a silently dropped review. + ### Reviewer instances for `/gsd-review` (#1517) Use `review.reviewer_instances` to run one model-capable adapter as several independent @@ -1204,6 +1244,7 @@ Configure per-CLI model selection for `/gsd-review`. When set, overrides the CLI | `review.default_reviewers` | string[] \| null | (all detected reviewers) | Default reviewer subset for no-flag `/gsd-review`. Example: `["gemini","codex"]`. May include configured `review.reviewer_instances` names. Explicit flags and `--all` override this setting. | | `review.max_prompt_tokens` | number\|null | null | Default maximum estimated tokens for the assembled review prompt. When set, the prompt is deterministically trimmed before being sent to each reviewer. Per-reviewer overrides via `review.max_prompt_tokens_per_reviewer` take precedence. null = no trim (current behavior). | | `review.max_prompt_tokens_per_reviewer` | object | {} | Per-reviewer token budget overrides. Keys are reviewer slugs. Only lanes that declare a budget key accept one — today `ollama`, `lm_studio` and `llama_cpp`, the local model servers this exists for. Values override `review.max_prompt_tokens` for that reviewer. A per-lane value of `0` disables trimming for that lane specifically. | +| `review.parallel_lanes` | boolean | `false` | Dispatch independent reviewer lanes concurrently within a single `/gsd-review` pass. Default `false` keeps the sequential dispatch that protects against provider rate limits. Opt in only when your providers can accept concurrent requests. Convergence cycles stay sequential either way. | | `review.ollama_host` | string | `http://localhost:11434` | Base URL of the Ollama server. Override when running Ollama on a non-default port or remote host: `gsd config-set review.ollama_host http://192.168.1.10:11434` | | `review.lm_studio_host` | string | `http://localhost:1234` | Base URL of the LM Studio local server. Override when using a non-default port. | | `review.llama_cpp_host` | string | `http://localhost:8080` | Base URL of the llama.cpp server (`llama-server`). Override when using a non-default port. | diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 85eee5680..cc29b1e16 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -3529,3 +3529,25 @@ See [Resolve verify-command path findings](how-to/resolve-verify-command-path-fi **Known limits:** no sandbox — once enabled, nothing constrains which origins are reached ([ADR-1244](adr/1244-capability-ecosystem.md) D5); DOM observation only, no screenshot diffing, accessibility audit, or performance tracing. **Reference:** [Configuration](CONFIGURATION.md) · [Enable live-DOM verification](how-to/enable-live-dom-verification.md) · [Explanation](explanation/live-dom-uat-capability.md) · [Agents](AGENTS.md) + +--- + +### 165. Opt-In Parallel Reviewer Lanes + +**Command:** `/gsd-review`, `/gsd-plan-review-convergence` + +**Config key:** `review.parallel_lanes` (default `false`) + +**Purpose:** Reviewer lanes within one review pass have no data dependency on each other — they all inspect the same immutable plan snapshot — but were dispatched strictly one at a time, so a pass with Codex, Gemini and Claude cost roughly the sum of three long reviewer calls. The serialization was a deliberate, unconditional protection against provider rate limits, which made it a global policy imposed on users whose providers could comfortably take concurrent requests, or who run local model servers with no limits at all (#3034). + +**Behavior:** With the key enabled, the `invoke_reviewers` step dispatches each selected lane as a background job and joins all of them before `REVIEWS.md` and consensus are rendered. Wall-clock cost falls toward the slowest lane rather than the sum. Default remains `false`, preserving the existing sequential dispatch and its rate-limit protection. + +**The guard is strict equality, and it fails safe.** Only the exact value `true` opts in — `"1"`, `"yes"` and `"TRUE"` all stay sequential, so a mistyped config gets the conservative behavior rather than concurrent requests at a rate-limited provider. A failure to read the config falls back to sequential too. This polarity is deliberately the opposite of the `commit_docs` guard, which fails open: there, failing open preserves user intent; here it would fire the very requests the default exists to prevent. + +**Result ordering is unchanged in both modes.** Per-lane results are written to slug-scoped files and concatenated in reviewer-selection order after the join, so `gsd-review-lane-results.jsonl` reads identically whether lanes ran sequentially or concurrently. Completion order never reaches the artifact. This also means concurrent lanes never share an append handle — a lane result larger than the pipe-atomicity bound cannot interleave and corrupt the `models:` / `model_sources:` frontmatter that `write_reviews` renders from that file. + +**Per-lane semantics are untouched.** Timeouts, prompt budgets, the diagnostic stub for an empty or failed lane, explicit-lane failure ([ADR-2782](adr/2782-reviewer-lane-capability-surface.md) D4), trust/egress checks and result-file layout all behave exactly as they do sequentially. A failing lane does not abort its siblings. + +**Known limits:** convergence cycles stay sequential by design (`review → replan → re-review` has a genuine data dependency), so this speeds up each pass rather than reducing the number of passes; there is no concurrency bound, so every selected lane dispatches at once; and reviewer instances sharing one adapter dispatch concurrently against that single provider, which is the most likely way to hit a limit. + +**Reference:** [Configuration](CONFIGURATION.md#parallel-reviewer-lanes-for-gsd-review-3034) · [Enable parallel reviewer lanes](how-to/enable-parallel-reviewer-lanes.md) · [Commands](COMMANDS.md) diff --git a/docs/README.md b/docs/README.md index 9b8540482..736603c67 100644 --- a/docs/README.md +++ b/docs/README.md @@ -36,6 +36,7 @@ Language versions: [English](README.md) · [Português (pt-BR)](pt-BR/README.md) - [Plan a phase](how-to/plan-a-phase.md) — run research, decompose work, and verify plan quality - [Verify a dependency-compatibility claim](how-to/verify-a-dependency-compatibility-claim.md) — act on a compatibility claim the researcher left `[ASSUMED]`, and tell "nothing declared" apart from "a constraint is declared" and "the lookup failed" - [Execute a phase](how-to/execute-a-phase.md) — run plans in parallel waves with fresh-context subagents +- [Enable parallel reviewer lanes](how-to/enable-parallel-reviewer-lanes.md) — cut a multi-reviewer `/gsd-review` pass toward its slowest lane, and tell a rate-limited lane apart from one that was never selected - [Verify and ship](how-to/verify-and-ship.md) — walk through completed work, diagnose failures, and create the PR - [Catch complexity before it compounds](how-to/act-on-a-refactor-proposal.md) — enable the post-execute refactor hook, read a proposal's score vs. anchor delta, and accept or decline it - [Run phases autonomously](how-to/run-phases-autonomously.md) — use autonomous mode for unattended phase execution diff --git a/docs/how-to/enable-parallel-reviewer-lanes.md b/docs/how-to/enable-parallel-reviewer-lanes.md new file mode 100644 index 000000000..bad873202 --- /dev/null +++ b/docs/how-to/enable-parallel-reviewer-lanes.md @@ -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.`; 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) diff --git a/docs/ja-JP/COMMANDS.md b/docs/ja-JP/COMMANDS.md index 056c7ed42..138664876 100644 --- a/docs/ja-JP/COMMANDS.md +++ b/docs/ja-JP/COMMANDS.md @@ -219,7 +219,7 @@ WebSearch から取得したパッケージは `[ASSUMED]`（`[VERIFIED]` では |-----------------|----------|-------------| | `N` | **Yes** | 計画およびレビューするフェーズ番号 | | レビュアーフラグ | No | すべてのレビュアーレーンフラグをそのまま渡す: `--gemini`、`--claude`、`--codex`、`--coderabbit`、`--opencode`、`--qwen`、`--cursor`、`--agy` / `--antigravity`、`--ollama`、`--lm-studio`、`--llama-cpp`、`--kimi-code` | -| `--all` | No | 設定済みのすべてのレビュアーを並列で実行 | +| `--all` | No | 設定済みのすべてのレビュアーを実行。レーンはデフォルトでは**順次**ディスパッチされます。`review.parallel_lanes` を `true` にすると、1 回のレビューパス内で並行してディスパッチされます | | `--max-cycles N` | No | サイクル上限を上書き（デフォルト3） | **終了動作:** HIGH カウントがゼロになるとループが終了します。HIGH カウントがサイクル間で減少しない場合はストール検出が警告します。`--max-cycles` に達しても HIGH 懸念が残っている場合、エスカレーションゲートがユーザーに続行するか手動でレビューするかを確認します。 diff --git a/docs/ko-KR/COMMANDS.md b/docs/ko-KR/COMMANDS.md index 493594b5b..3274156a5 100644 --- a/docs/ko-KR/COMMANDS.md +++ b/docs/ko-KR/COMMANDS.md @@ -219,7 +219,7 @@ WebSearch에서 가져온 패키지는 `[ASSUMED]`(`[VERIFIED]`가 아님)로 |-----------------|----------|-------------| | `N` | **예** | 계획 및 리뷰할 단계 번호 | | 리뷰어 플래그 | 아니요 | 모든 리뷰어 레인 플래그를 그대로 전달: `--gemini`, `--claude`, `--codex`, `--coderabbit`, `--opencode`, `--qwen`, `--cursor`, `--agy` / `--antigravity`, `--ollama`, `--lm-studio`, `--llama-cpp`, `--kimi-code` | -| `--all` | 아니요 | 구성된 모든 리뷰어를 병렬로 실행 | +| `--all` | 아니요 | 구성된 모든 리뷰어를 실행합니다. 레인은 기본적으로 **순차적으로** 디스패치되며, `review.parallel_lanes`를 `true`로 설정하면 단일 리뷰 패스 내에서 동시에 디스패치됩니다 | | `--max-cycles N` | 아니요 | 사이클 상한 재정의 (기본값 3) | **종료 동작:** HIGH 카운트가 0이 되면 루프 종료. 사이클 간 HIGH 카운트가 감소하지 않을 때 정체 감지 경고. `--max-cycles`에 도달해도 HIGH 우려사항이 남아 있으면 에스컬레이션 게이트가 계속 진행하거나 수동 리뷰를 요청합니다. diff --git a/docs/pt-BR/COMMANDS.md b/docs/pt-BR/COMMANDS.md index 6ccafc38d..40a56a238 100644 --- a/docs/pt-BR/COMMANDS.md +++ b/docs/pt-BR/COMMANDS.md @@ -219,7 +219,7 @@ Loop de convergência de planos cross-AI — replaneja com feedback de revisão |------------------|-------------|-----------| | `N` | **Sim** | Número da fase a planejar e revisar | | Flags de revisor | Não | Repassa todas as flags de lane de revisor: `--gemini`, `--claude`, `--codex`, `--coderabbit`, `--opencode`, `--qwen`, `--cursor`, `--agy` / `--antigravity`, `--ollama`, `--lm-studio`, `--llama-cpp`, `--kimi-code` | -| `--all` | Não | Executa todos os revisores configurados em paralelo | +| `--all` | Não | Executa todos os revisores configurados. As lanes são despachadas **sequencialmente** por padrão; defina `review.parallel_lanes` como `true` para despachá-las simultaneamente em uma única passagem de revisão | | `--max-cycles N` | Não | Substitui o limite de ciclos (padrão 3) | **Comportamento de saída:** O loop termina quando a contagem HIGH chega a zero. A detecção de estagnação avisa quando a contagem HIGH não diminui entre ciclos. O portão de escalação solicita ao usuário que prossiga ou revise manualmente quando `--max-cycles` é atingido com preocupações HIGH ainda em aberto. diff --git a/docs/zh-CN/COMMANDS.md b/docs/zh-CN/COMMANDS.md index 4ca3b4e9b..94361d277 100644 --- a/docs/zh-CN/COMMANDS.md +++ b/docs/zh-CN/COMMANDS.md @@ -219,7 +219,7 @@ v1.40 中，六个命名空间路由器作为第一阶段入口点随附发布 |-----------------|----------|-------------| | `N` | **是** | 要规划和审查的阶段编号 | | 审查者标志 | 否 | 原样传递所有审查者通道标志：`--gemini`、`--claude`、`--codex`、`--coderabbit`、`--opencode`、`--qwen`、`--cursor`、`--agy` / `--antigravity`、`--ollama`、`--lm-studio`、`--llama-cpp`、`--kimi-code` | -| `--all` | 否 | 并行运行所有已配置的审查者 | +| `--all` | 否 | 运行所有已配置的审查者。审查通道默认**顺序**分发；将 `review.parallel_lanes` 设为 `true` 可在单次审查中并发分发 | | `--max-cycles N` | 否 | 覆盖循环上限（默认 3） | **退出行为：** HIGH 计数归零时循环退出。停滞检测在 HIGH 计数在各循环间未减少时发出警告。当达到 `--max-cycles` 且仍有 HIGH 问题未解决时，升级门询问用户是继续还是手动审查。 diff --git a/gsd-core/bin/shared/config-schema.manifest.json b/gsd-core/bin/shared/config-schema.manifest.json index bc4588411..6a055de41 100644 --- a/gsd-core/bin/shared/config-schema.manifest.json +++ b/gsd-core/bin/shared/config-schema.manifest.json @@ -55,6 +55,7 @@ "review.default_reviewers", "review.max_prompt_tokens", "review.max_prompt_tokens_per_reviewer", + "review.parallel_lanes", "workflow.cross_ai_execution", "workflow.cross_ai_command", "workflow.cross_ai_timeout", diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index 19ed9e458..b6b1b1429 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -316,7 +316,11 @@ reintroduce the flag (even spelled out in prose — a regression test bans the l If `section_manifest` is `null` or `"reviewer-instances-note-2"` is in its `included` list: read and execute `gsd-core/workflows/review/steps/reviewer-instances-note-2.md`. Otherwise skip — do not read the file. -Lanes run **sequentially, not in parallel** — concurrent invocation trips provider rate limits. +Lanes run **sequentially by default** — concurrent invocation trips provider rate limits, and a lane +lost to one is a cross-AI review that quietly went blind in one eye. A project whose providers can +accept the concurrency opts in with `review.parallel_lanes: true` (#3034): the selected lanes are +dispatched together and **all** joined before aggregation. The default is unchanged, and convergence +cycles stay sequential either way — only the lanes *within* one pass overlap. ```bash # #2962: zsh aborts the block on an unmatched for-list glob (nomatch); bash passes it through. nullglob both. @@ -327,6 +331,13 @@ REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" # SELECTED_REVIEWERS is the comma-separated result of reviewer selection (ADR-0011 precedence: # explicit flags > --all > review.default_reviewers > all detected). Unchanged by this phase. +# #3034: opt-in concurrent lane dispatch. STRICT equality on "true" is deliberate — "1", "yes" and +# "TRUE" must NOT opt in, so a mistyped config gets the conservative behaviour rather than firing +# concurrent requests at a rate-limited provider. Note the `|| echo "false"` fallback is the +# OPPOSITE polarity from the commit_docs guard, which fails OPEN: there, failing open preserves the +# user's intent; here it would fire exactly the requests the default exists to prevent. +PARALLEL_LANES=$(gsd_run query config-get review.parallel_lanes --raw 2>/dev/null || echo "false") + # Shared budget-trim helper. Was defined inside the Ollama leg; it is lane-agnostic, so it is # hoisted here now that any lane may declare a promptBudgetKey. Returns non-zero when the budget # is too small for the minimum review set (prompt-budget exit 2 / 11). @@ -360,7 +371,20 @@ gsd_run query review-lane plan \ --selected "$SELECTED_REVIEWERS" --run-dir "$RUN_DIR" --repo-root "$REPO_ROOT" --json \ > "$RUN_DIR/gsd-review-lanes.json" -for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do +# One lane, start to finish. Hoisted into a function so the sequential and concurrent paths share +# ONE body: two dispatch bodies kept in sync by hand is the generative-fix divergence ADR-2782 spent +# a phase deleting, and it is what let #2494/#2605 be filed twice as the same defect. +# +# The result goes to a SLUG-SCOPED file, never a shared append. Concurrent O_APPEND is atomic only +# below PIPE_BUF (4096 on Linux, 512 on some platforms), so a lane result above that bound could +# interleave — and write_reviews parses this JSONL to render the models:/model_sources: frontmatter, +# so a torn line is a broken REVIEWS.md, not a cosmetic log defect. +run_review_lane() { + # `local` is hygiene, not a live fix: each `&`-dispatched call already forks its own subshell, so + # concurrent lanes cannot share these today. Scoped anyway so the isolation is a property of this + # function rather than of the dispatch mechanism happening to fork. + local SLUG LANE_BUDGET PROMPT_ARG TRIMMED + SLUG="$1" # Per-lane prompt budget. The lane declares its own `promptBudgetKey`; `plan` resolved it, # applying #2797's sentinel rule (-1 = unset → fall back to the global budget; 0 legitimately # means "do not trim this lane"). Trimming itself stays in prompt-budget, which owns it. @@ -378,7 +402,9 @@ for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do # response used to (#2605), so leave the skip visible in the review output, not only on stderr. echo "$SLUG review skipped: prompt budget (${LANE_BUDGET} tokens) too small for the minimum review set." \ > "$RUN_DIR/gsd-review-$SLUG.md" - continue + # Was `continue` when this was a loop body. Inside a function that keyword is not the loop + # control it looks like — `return 0` is what skips this lane and leaves it with no result line. + return 0 fi fi @@ -387,7 +413,50 @@ for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do # normal, failing to run one somebody asked for is an error. gsd_run query review-lane invoke --slug "$SLUG" \ --run-dir "$RUN_DIR" --repo-root "$REPO_ROOT" $PROMPT_ARG $EXPLICIT_FLAG --json \ - >> "$RUN_DIR/gsd-review-lane-results.jsonl" + > "$RUN_DIR/gsd-review-lane-result-$SLUG.json" +} + +# Split ONCE, de-duplicated, and reuse for both loops below. Two reasons, and the second is +# load-bearing: a slug repeated in SELECTED_REVIEWERS would put TWO concurrent background jobs on +# `> "$RUN_DIR/gsd-review-lane-result-$SLUG.json"` — the same file, both truncating. The shared-append +# form this replaced could not corrupt itself that way, so de-duping is what keeps the concurrent +# path no worse than the sequential one. Selection de-dupes today (the roster is a Set; +# review.default_reviewers normalizes lowercase-unique), but reachability analysis is not a contract +# and the next caller should not have to redo it. +# +# A plain string accumulator, not an array: zsh and bash disagree on array indexing and this block +# runs under both (see the nullglob/NULL_GLOB pairing above). +DISPATCH_SLUGS="" +for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do + case " $DISPATCH_SLUGS " in + *" $SLUG "*) continue ;; + esac + DISPATCH_SLUGS="$DISPATCH_SLUGS $SLUG" +done + +for SLUG in $DISPATCH_SLUGS; do + if [ "$PARALLEL_LANES" = "true" ]; then + run_review_lane "$SLUG" & + else + run_review_lane "$SLUG" + fi +done + +# Join every dispatched lane. A bare `wait` with no background jobs returns 0, so the sequential +# path needs no guard around it. NOTHING below this line may run before every lane has finished — +# write_reviews renders REVIEWS.md and the consensus summary from the aggregate below, and a review +# assembled from a partial set looks complete while silently missing a reviewer. +wait + +# Aggregate in SELECTED_REVIEWERS order, NOT completion order, so the JSONL a concurrent run +# produces is byte-identical to the one a sequential run produces. This is post-join and therefore +# single-threaded, so `>>` here is safe. A lane that was budget-skipped, or that never started, +# leaves no result file and correctly contributes no line. +for SLUG in $DISPATCH_SLUGS; do + LANE_RESULT="$RUN_DIR/gsd-review-lane-result-$SLUG.json" + if [ -f "$LANE_RESULT" ]; then + cat "$LANE_RESULT" >> "$RUN_DIR/gsd-review-lane-results.jsonl" + fi done ``` diff --git a/tests/emitted-drift-acks/3034-parallel-reviewer-lanes.json b/tests/emitted-drift-acks/3034-parallel-reviewer-lanes.json new file mode 100644 index 000000000..10718af91 --- /dev/null +++ b/tests/emitted-drift-acks/3034-parallel-reviewer-lanes.json @@ -0,0 +1,8 @@ +{ + "version": 1, + "paths": { + "review.md": { + "reason": "#3034 adds opt-in concurrent reviewer-lane dispatch to the invoke_reviewers step. The growth is the feature plus the reasoning that has to travel with it, because this block is shipped shell that is read by an agent at runtime rather than by a compiler: the strict-equality guard and its fail-safe polarity (deliberately inverted relative to the commit_docs guard, so it does not read as an inconsistency to be tidied away), why per-lane result files replaced a shared append (concurrent O_APPEND is atomic only below PIPE_BUF, and write_reviews parses that JSONL to render the models:/model_sources: frontmatter), why aggregation walks selection order rather than completion order, why the slug list is de-duplicated before dispatch, and why the loop body was hoisted into one shared function instead of forking into two dispatch bodies. No prose was moved into an eagerly @-imported reference to shrink the measured file -- total loaded context grew by exactly this diff." + } + } +} diff --git a/tests/review-parallel-lanes.test.cjs b/tests/review-parallel-lanes.test.cjs new file mode 100644 index 000000000..ac166a924 --- /dev/null +++ b/tests/review-parallel-lanes.test.cjs @@ -0,0 +1,570 @@ +'use strict'; + +/** + * Failing-first tests for #3034 (opt-in parallel reviewer lanes). + * + * Design: .gsd/phase/feat-3034-parallel-reviewer-lanes/40-design.md + * Test matrix: .gsd/phase/feat-3034-parallel-reviewer-lanes/50-test-matrix.md + * + * The unit under test is the real, shipped `` + * fenced bash block in gsd-core/workflows/review.md — extracted and EXECUTED + * (never re-typed), with the single I/O seam `gsd_run` replaced by a shell + * function stub. The feature this file exercises (an opt-in + * `review.parallel_lanes` config key that backgrounds lane dispatch and + * joins before aggregation) does not exist yet, so several tests below are + * expected to be RED against the current shipped block. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { + createTempDir, + createTempProject, + cleanup, + readFileNormalized, + runGsdTools, +} = require('./helpers.cjs'); +const { runHook } = require('./helpers/process-seam.cjs'); +const { HOOK_FANOUT_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); + +const REPO_ROOT = path.join(__dirname, '..'); +const REVIEW_MD_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'review.md'); + +const SELECTED_3 = 'codex,gemini,claude'; + +// ─── extraction (source-text-is-the-product) ────────────────────────────── + +/** + * Reads review.md and extracts the fenced bash block inside + * ``. Mirrors extractCwdGuardBash() in + * tests/worktree-cleanup.test.cjs: readFileNormalized() strips CRLF at the + * read boundary (what actually makes the captured body safe to hand to + * bash); the `\r?\n` in the fence regex below is redundant on that + * already-normalized input but kept anyway because a bare `\n` in a + * markdown-fence-shaped regex trips the local/no-crlf-fragile-split rule. + */ +function extractInvokeReviewersBash() { + const content = readFileNormalized(REVIEW_MD_PATH); + + const stepMarker = ''; + const stepIdx = content.indexOf(stepMarker); + if (stepIdx === -1) { + throw new Error(`extractInvokeReviewersBash: could not find "${stepMarker}" in ${REVIEW_MD_PATH}`); + } + const afterStep = content.slice(stepIdx + stepMarker.length); + + const endMarker = ''; + const endIdx = afterStep.indexOf(endMarker); + if (endIdx === -1) { + throw new Error(`extractInvokeReviewersBash: could not find closing "${endMarker}" after invoke_reviewers in ${REVIEW_MD_PATH}`); + } + const stepBody = afterStep.slice(0, endIdx); + + const fenceRe = /```(?:bash|sh)\r?\n([\s\S]*?)```/; + const fenceMatch = fenceRe.exec(stepBody); + if (!fenceMatch) { + throw new Error(`extractInvokeReviewersBash: no \`\`\`bash fence found inside invoke_reviewers step in ${REVIEW_MD_PATH}`); + } + const block = fenceMatch[1]; + + if (!block.trim()) { + throw new Error('extractInvokeReviewersBash: extracted bash block is empty'); + } + if (!block.includes('review-lane invoke')) { + throw new Error('extractInvokeReviewersBash: extracted block does not contain "review-lane invoke" — anchor may have drifted'); + } + if (!block.includes('SELECTED_REVIEWERS')) { + throw new Error('extractInvokeReviewersBash: extracted block does not contain "SELECTED_REVIEWERS" — anchor may have drifted'); + } + + return block; +} + +// ─── stub preamble ───────────────────────────────────────────────────────── +// +// The ONLY I/O seam the extracted block calls is `gsd_run`. Everything else +// executed by runDispatch() is the real shipped shell text. `barrier` is the +// positive, deterministic concurrency proof this suite uses in place of any +// elapsed-time assertion (CLAUDE.md bans wall-clock assertions). + +const STUB_PREAMBLE = [ + 'arg_after() {', + ' local flag="$1"; shift', + ' while [ $# -gt 0 ]; do', + ' if [ "$1" = "$flag" ]; then', + ' printf %s "$2"', + ' return 0', + ' fi', + ' shift', + ' done', + '}', + '', + 'in_list() {', + ' local needle="$1" list="$2" item old_ifs="$IFS"', + ' IFS=","', + ' for item in $list; do', + ' if [ "$item" = "$needle" ]; then', + ' IFS="$old_ifs"', + ' return 0', + ' fi', + ' done', + ' IFS="$old_ifs"', + ' return 1', + '}', + '', + 'dep_of() {', + ' local slug="$1" pair old_ifs="$IFS"', + ' IFS=","', + ' for pair in $STUB_DEPS; do', + ' case "$pair" in', + ' "$slug="*)', + ' printf %s "${pair#*=}"', + ' IFS="$old_ifs"', + ' return 0', + ' ;;', + ' esac', + ' done', + ' IFS="$old_ifs"', + '}', + '', + 'wait_for_file() {', + ' local target="$1" i=0', + ' while [ ! -f "$target" ] && [ "$i" -lt 200 ]; do', + ' sleep 0.05', + ' i=$((i + 1))', + ' done', + '}', + '', + 'barrier() {', + ' local slug="$1" i=0 count', + ' touch "$BARRIER_DIR/$slug"', + ' count=$(ls -1 "$BARRIER_DIR" | wc -l)', + ' while [ "$count" -lt "$LANE_COUNT" ] && [ "$i" -lt 200 ]; do', + ' sleep 0.05', + ' i=$((i + 1))', + ' count=$(ls -1 "$BARRIER_DIR" | wc -l)', + ' done', + ' if [ "$count" -lt "$LANE_COUNT" ]; then', + ' echo "barrier-timeout:$slug" >> "$TRACE"', + ' return 1', + ' fi', + ' return 0', + '}', + '', + 'gsd_run() {', + ' if [ "$1" = "query" ] && [ "$2" = "config-get" ] && [ "$3" = "review.parallel_lanes" ]; then', + ' if [ "$STUB_CONFIG_GET_FAILS" = "1" ]; then', + ' return 1', + ' fi', + ' printf %s "$STUB_PARALLEL"', + ' return 0', + ' fi', + '', + ' if [ "$1" = "query" ] && [ "$2" = "review-lane" ] && [ "$3" = "plan" ]; then', + ' shift 3', + ' local sel', + ' sel="$(arg_after --selected "$@")"', + ' if in_list "$sel" "$STUB_BUDGET_FAIL"; then', + ' printf %s \'{"promptBudget": 10}\'', + ' else', + ' printf %s \'{"promptBudget": -1}\'', + ' fi', + ' return 0', + ' fi', + '', + ' if [ "$1" = "query" ] && [ "$2" = "prompt-budget" ]; then', + ' shift 2', + ' local out base slug', + ' out="$(arg_after --output-prompt "$@")"', + ' base="$(basename "$out")"', + ' slug="${base#gsd-review-prompt-}"', + ' slug="${slug%.md}"', + ' if in_list "$slug" "$STUB_BUDGET_FAIL"; then', + ' return 2', + ' fi', + ' : > "$out"', + ' return 0', + ' fi', + '', + ' if [ "$1" = "query" ] && [ "$2" = "review-lane" ] && [ "$3" = "invoke" ]; then', + ' shift 3', + ' local slug dep pad', + ' slug="$(arg_after --slug "$@")"', + ' echo "start:$slug" >> "$TRACE"', + ' if [ "$STUB_BARRIER" = "1" ]; then', + ' barrier "$slug" || true', + ' fi', + ' dep="$(dep_of "$slug")"', + ' if [ -n "$dep" ]; then', + ' wait_for_file "$RUN_DIR/done-$dep"', + ' fi', + ' echo "stub review body for $slug" > "$RUN_DIR/gsd-review-$slug.md"', + ' pad=""', + ' if [ "$STUB_PAD_BYTES" -gt 0 ] 2>/dev/null; then', + ' pad="$(head -c "$STUB_PAD_BYTES" /dev/zero | tr "\\0" "x")"', + ' fi', + ' if ! in_list "$slug" "$STUB_SILENT"; then', + ' printf \'{"slug":"%s","pad":"%s"}\\n\' "$slug" "$pad"', + ' fi', + ' touch "$RUN_DIR/done-$slug"', + ' echo "end:$slug" >> "$TRACE"', + ' if in_list "$slug" "$STUB_FAIL"; then', + ' return 1', + ' fi', + ' return 0', + ' fi', + '', + ' return 0', + '}', +].join('\n'); + +// ─── dispatch runner ─────────────────────────────────────────────────────── + +/** + * Build the stub env for a runDispatch() call from `opts`. Every key is + * always present (never omitted) so the generated `set -u` script never + * dereferences an unset variable — `opts.parallel === null` deliberately + * maps to the empty string, which is exactly how an unset config key reads + * back through `config-get --raw`. + */ +function buildEnv(opts, runDir, tracePath, barrierDir) { + const selected = opts.selected; + const laneCount = selected.split(',').filter((s) => s.length > 0).length; + const deps = opts.deps || {}; + return { + ...process.env, + SELECTED_REVIEWERS: selected, + EXPLICIT_FLAG: '', + STUB_PARALLEL: opts.parallel === null || opts.parallel === undefined ? '' : opts.parallel, + STUB_CONFIG_GET_FAILS: opts.configGetFails ? '1' : '0', + STUB_FAIL: (opts.failSlugs || []).join(','), + STUB_BUDGET_FAIL: (opts.budgetFailSlugs || []).join(','), + STUB_SILENT: (opts.silentSlugs || []).join(','), + STUB_BARRIER: opts.barrier ? '1' : '0', + STUB_DEPS: Object.entries(deps).map(([k, v]) => `${k}=${v}`).join(','), + STUB_PAD_BYTES: String(opts.padBytes || 0), + LANE_COUNT: String(laneCount), + TRACE: tracePath, + BARRIER_DIR: barrierDir, + }; +} + +/** + * Runs the real extracted invoke_reviewers bash block with the gsd_run stub + * spliced in front of it. `{run_dir}` is replaced globally with a real temp + * directory. Returns the dispatch outcome plus the artifacts it produced. + */ +function runDispatch(t, opts) { + const scriptDir = createTempDir('gsd-3034-script-'); + const runDir = createTempDir('gsd-3034-rundir-'); + const barrierDir = createTempDir('gsd-3034-barrier-'); + t.after(() => { + cleanup(scriptDir); + cleanup(runDir); + cleanup(barrierDir); + }); + + const block = extractInvokeReviewersBash(); + const tracePath = path.join(scriptDir, 'trace.log'); + const env = buildEnv(opts, runDir, tracePath, barrierDir); + + const scriptPath = path.join(scriptDir, 'dispatch.sh'); + const script = [ + '#!/usr/bin/env bash', + 'set -u', + STUB_PREAMBLE, + block.split('{run_dir}').join(runDir), + ].join('\n'); + fs.writeFileSync(scriptPath, script, { mode: 0o755 }); + + const result = runHook(scriptPath, [], { + interpreter: 'bash', + cwd: runDir, + env, + timeoutMs: HOOK_FANOUT_TIMEOUT_MS, + }); + + const jsonlPath = path.join(runDir, 'gsd-review-lane-results.jsonl'); + const jsonl = fs.existsSync(jsonlPath) ? fs.readFileSync(jsonlPath, 'utf-8') : ''; + const lines = jsonl.split('\n').filter((l) => l.trim() !== ''); + const trace = fs.existsSync(tracePath) + ? readFileNormalized(tracePath).split('\n').filter((l) => l.trim() !== '') + : []; + + return { + outcome: result.outcome, + exitCode: result.exitCode, + stderr: result.stderr, + jsonl, + lines, + trace, + runDir, + }; +} + +/** Parsed slug order from JSONL lines — never assert on raw JSONL text. */ +function slugOrder(lines) { + return lines.map((l) => JSON.parse(l).slug); +} + +function serialTrace(slugs) { + return slugs.flatMap((s) => [`start:${s}`, `end:${s}`]); +} + +// ─── #1/#2 — default and explicit-disabled serial dispatch ─────────────── + +describe('#3034 default and explicit-disabled dispatch stays serial', () => { + test('defaultsToSerialDispatchWhenKeyUnset', (t) => { + const result = runDispatch(t, { selected: SELECTED_3, parallel: null }); + assert.deepEqual(result.trace, serialTrace(['codex', 'gemini', 'claude'])); + }); + + test('staysSerialWhenExplicitlyDisabled', (t) => { + const result = runDispatch(t, { selected: SELECTED_3, parallel: 'false' }); + assert.deepEqual(result.trace, serialTrace(['codex', 'gemini', 'claude'])); + }); +}); + +// ─── #3/#4 — opt-in concurrency and join-before-aggregate ───────────────── + +describe('#3034 opt-in concurrency', () => { + test('dispatchesLanesConcurrentlyWhenEnabled', (t) => { + const result = runDispatch(t, { selected: SELECTED_3, parallel: 'true', barrier: true }); + const timeouts = result.trace.filter((l) => l.startsWith('barrier-timeout:')); + assert.deepEqual(timeouts, [], 'no lane should hit the barrier timeout when lanes run concurrently'); + }); + + test('joinsAllLanesBeforeAggregation', (t) => { + // barrier:true is load-bearing, not decoration. Without it the stub lanes + // finish instantly and a missing `wait` could still race to 3 lines, + // making this pass intermittently. Held at the barrier, a missing join + // deterministically aggregates ZERO lines. + const result = runDispatch(t, { selected: SELECTED_3, parallel: 'true', barrier: true }); + assert.equal(result.lines.length, 3, 'dispatch must return only after every lane wrote its result'); + }); +}); + +// ─── #5/#6 — selection-order preservation ───────────────────────────────── + +describe('#3034 JSONL preserves selection order, not completion order', () => { + test('preservesSelectionOrderSerial', (t) => { + const result = runDispatch(t, { selected: SELECTED_3, parallel: null }); + assert.deepEqual(slugOrder(result.lines), ['codex', 'gemini', 'claude']); + }); + + test('preservesSelectionOrderParallelDespiteCompletionOrder', (t) => { + // codex waits on gemini; gemini waits on claude -> forces reverse + // completion order (claude, gemini, codex) while selection order stays + // codex, gemini, claude. + const result = runDispatch(t, { + selected: SELECTED_3, + parallel: 'true', + deps: { codex: 'gemini', gemini: 'claude' }, + }); + + const endMarkers = result.trace.filter((l) => l.startsWith('end:')); + // Pin the fixture actually forced reverse completion first — otherwise + // the selection-order assertion below would pass vacuously. + assert.deepEqual(endMarkers, ['end:claude', 'end:gemini', 'end:codex']); + + assert.deepEqual(slugOrder(result.lines), ['codex', 'gemini', 'claude']); + }); +}); + +// ─── #7/#8 — PIPE_BUF boundary triple (plus one clearly-oversized case) ─── + +describe('#3034 oversized lane results stay intact under concurrency', () => { + test('keepsOversizedLaneResultsIntactUnderConcurrency', async (t) => { + for (const padBytes of [4095, 4096, 4097, 8192]) { + await t.test(`padBytes=${padBytes}`, (t2) => { + const result = runDispatch(t2, { selected: SELECTED_3, parallel: 'true', padBytes }); + assert.equal(result.lines.length, 3); + for (const line of result.lines) { + assert.doesNotThrow(() => JSON.parse(line), `line failed to parse at padBytes=${padBytes}: ${line.slice(0, 80)}...`); + } + assert.deepEqual(slugOrder(result.lines), ['codex', 'gemini', 'claude']); + }); + } + }); +}); + +// ─── #9 — single-lane boundary ───────────────────────────────────────────── + +describe('#3034 single-lane boundary', () => { + test('singleLaneParallelMatchesSerial', (t) => { + const serial = runDispatch(t, { selected: 'codex', parallel: null }); + const parallel = runDispatch(t, { selected: 'codex', parallel: 'true' }); + + assert.equal(serial.lines.length, 1); + assert.equal(parallel.lines.length, 1); + assert.deepEqual(slugOrder(serial.lines), slugOrder(parallel.lines)); + + const serialMd = fs.readFileSync(path.join(serial.runDir, 'gsd-review-codex.md'), 'utf-8'); + const parallelMd = fs.readFileSync(path.join(parallel.runDir, 'gsd-review-codex.md'), 'utf-8'); + assert.equal(serialMd, parallelMd); + }); +}); + +// ─── #10 — empty selection is a no-op ────────────────────────────────────── + +describe('#3034 empty selection', () => { + test('emptySelectionIsANoOp', (t) => { + const result = runDispatch(t, { selected: '', parallel: 'true' }); + assert.equal(result.outcome, 'exited'); + assert.equal(result.exitCode, 0); + assert.deepEqual(result.lines, []); + assert.deepEqual(result.trace, []); + }); +}); + +describe('#3034 duplicate slug in the selection', () => { + test('duplicateSlugDispatchesOnceAndWritesOneLine', (t) => { + // A slug repeated in SELECTED_REVIEWERS would otherwise put two + // concurrent background jobs on the same `>`-truncated per-slug result + // file, corrupting whichever one finishes last. DISPATCH_SLUGS + // de-duplicates before dispatch, so codex must run exactly once. + const result = runDispatch(t, { selected: 'codex,gemini,codex', parallel: 'true' }); + assert.equal(result.outcome, 'exited'); + assert.equal(result.trace.filter((l) => l === 'start:codex').length, 1); + assert.equal(result.trace.filter((l) => l === 'end:codex').length, 1); + assert.deepEqual(slugOrder(result.lines), ['codex', 'gemini']); + }); +}); + +// ─── #11/#12 — lane failure does not abort siblings ──────────────────────── + +describe('#3034 lane failure does not abort sibling lanes', () => { + test('laneFailureDoesNotAbortSiblingLanes', (t) => { + const result = runDispatch(t, { selected: SELECTED_3, parallel: 'true', failSlugs: ['gemini'] }); + assert.equal(result.lines.length, 3, 'a failing lane still contributes its stub result line'); + assert.ok( + fs.existsSync(path.join(result.runDir, 'gsd-review-gemini.md')), + 'the failing lane\'s diagnostic stub .md must be preserved', + ); + assert.deepEqual(slugOrder(result.lines), ['codex', 'gemini', 'claude']); + }); + + test('laneFailureSerialUnchanged', (t) => { + const result = runDispatch(t, { selected: SELECTED_3, parallel: null, failSlugs: ['gemini'] }); + assert.deepEqual(result.trace, serialTrace(['codex', 'gemini', 'claude'])); + assert.equal(result.lines.length, 3); + }); +}); + +// ─── #13 — budget-too-small skip emits no result line ───────────────────── + +describe('#3034 budget-too-small skip', () => { + test('budgetSkipEmitsNoResultLine', (t) => { + const result = runDispatch(t, { selected: SELECTED_3, parallel: 'true', budgetFailSlugs: ['gemini'] }); + assert.deepEqual(slugOrder(result.lines), ['codex', 'claude'], 'the budget-skipped lane contributes no result line'); + const stub = fs.readFileSync(path.join(result.runDir, 'gsd-review-gemini.md'), 'utf-8'); + assert.match(stub, /skipped/); + assert.match(stub, /prompt budget/); + }); +}); + +// ─── #14/#15 — silent / absent lane contributes no line ─────────────────── + +describe('#3034 silent lane contributes nothing, not a blank line', () => { + test('silentLaneContributesNoLine', (t) => { + const result = runDispatch(t, { selected: SELECTED_3, parallel: 'true', silentSlugs: ['gemini'] }); + assert.deepEqual(slugOrder(result.lines), ['codex', 'claude']); + // Folded row #15 (absent result file): budgetSkipEmitsNoResultLine above + // already exercises the "invoke never started, no file at all" shape via + // its `continue`. This assertion pins the sibling shape: a lane that DID + // run and wrote nothing must not leave a stray blank JSONL line. + assert.ok(!result.jsonl.includes('\n\n'), 'an empty lane result must not leave a blank JSONL line'); + }); +}); + +// ─── #16/#17 — non-canonical truthy values stay serial ──────────────────── + +describe('#3034 non-canonical truthy config values stay serial', () => { + test('nonCanonicalTruthyValuesStaySerial', async (t) => { + const nearMisses = ['TRUE', 'True', '1', 'yes', 'on', ' true', 'true ']; + for (const value of nearMisses) { + await t.test(`parallel_lanes="${value}"`, (t2) => { + const result = runDispatch(t2, { selected: SELECTED_3, parallel: value }); + assert.deepEqual(result.trace, serialTrace(['codex', 'gemini', 'claude']), `value "${value}" must not opt into parallel dispatch`); + }); + } + }); +}); + +// ─── #18 — config-get failure fails safe to serial ───────────────────────── + +describe('#3034 broken config tooling fails safe to serial', () => { + test('configGetFailureFallsBackToSerial', (t) => { + const result = runDispatch(t, { selected: SELECTED_3, parallel: 'true', configGetFails: true }); + assert.deepEqual(result.trace, serialTrace(['codex', 'gemini', 'claude'])); + }); +}); + +// ─── #19/#20/#21 — config-set registers review.parallel_lanes ───────────── + +describe('#3034 review.parallel_lanes config key', () => { + test('configSetAcceptsAndPersistsParallelLanes', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const setResult = runGsdTools('config-set review.parallel_lanes true', tmpDir); + assert.ok(setResult.success, `config-set failed: ${setResult.error}`); + + const configPath = path.join(tmpDir, '.planning', 'config.json'); + const config = JSON.parse(fs.readFileSync(configPath, 'utf-8')); + assert.equal(config.review?.parallel_lanes, true); + assert.equal(typeof config.review?.parallel_lanes, 'boolean'); + + const getResult = runGsdTools('config-get review.parallel_lanes --raw', tmpDir); + assert.ok(getResult.success, `config-get failed: ${getResult.error}`); + assert.equal((getResult.output || '').trim(), 'true'); + }); + + test('configSetPersistsBooleanFalse', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const setResult = runGsdTools('config-set review.parallel_lanes false', tmpDir); + assert.ok(setResult.success, `config-set failed: ${setResult.error}`); + + const configPath = path.join(tmpDir, '.planning', 'config.json'); + const config = JSON.parse(fs.readFileSync(configPath, 'utf-8')); + assert.equal(config.review?.parallel_lanes, false); + assert.equal(typeof config.review?.parallel_lanes, 'boolean'); + }); + + test('rejectsUnregisteredNeighbouringKey', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + // Missing trailing "s" — proves the whitelist is load-bearing and the + // two tests above are not vacuous (they'd pass even for an unregistered + // key if config-set accepted anything). + const result = runGsdTools('config-set review.parallel_lane true', tmpDir); + assert.equal(result.success, false, 'an unregistered near-miss key must be rejected'); + }); +}); + +// ─── #22 — serial/parallel artifact equivalence ──────────────────────────── + +describe('#3034 serial and parallel dispatch produce equivalent artifacts', () => { + test('serialAndParallelProduceEquivalentArtifacts', (t) => { + const serial = runDispatch(t, { selected: SELECTED_3, parallel: null }); + const parallel = runDispatch(t, { selected: SELECTED_3, parallel: 'true' }); + + assert.deepEqual(slugOrder(serial.lines), slugOrder(parallel.lines)); + assert.deepEqual( + serial.lines.map((l) => JSON.parse(l)), + parallel.lines.map((l) => JSON.parse(l)), + ); + + for (const slug of ['codex', 'gemini', 'claude']) { + const serialMd = fs.readFileSync(path.join(serial.runDir, `gsd-review-${slug}.md`), 'utf-8'); + const parallelMd = fs.readFileSync(path.join(parallel.runDir, `gsd-review-${slug}.md`), 'utf-8'); + assert.equal(serialMd, parallelMd, `gsd-review-${slug}.md must be byte-identical between the two paths`); + } + }); +});