diff --git a/.changeset/lively-yaks-frolic.md b/.changeset/lively-yaks-frolic.md new file mode 100644 index 000000000..876c837fd --- /dev/null +++ b/.changeset/lively-yaks-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2820 +--- +**`/gsd:review` no longer silently drops a reviewer you asked for** — naming a reviewer with an explicit flag (`--gemini --qwen`) on a host where that lane could not run reported an info note and reviewed with a thinner set, while the run reported success; a cross-AI review that quietly loses a lane is blind in one eye. An explicitly-named lane that cannot run — CLI absent, `jq` missing, or local server unreachable — is now an error. `--all` and `review.default_reviewers` are unchanged and still skip undetected lanes with an info note. The Qwen lane also now captures stderr to a sidecar and includes it in its failure stub, matching every other lane, so a missing binary and an auth prompt are no longer indistinguishable from an empty review. (#2794) diff --git a/.gitignore b/.gitignore index 08ad71c76..9b7cf6a0e 100644 --- a/.gitignore +++ b/.gitignore @@ -114,6 +114,7 @@ build/ /gsd-core/bin/lib/clock.cjs /gsd-core/bin/lib/ui-safety-gate.cjs /gsd-core/bin/lib/review-reviewer-selection.cjs +/gsd-core/bin/lib/review-lane-descriptor.cjs /gsd-core/bin/lib/clusters.cjs /gsd-core/bin/lib/installer-migrations/001-legacy-orphan-files.cjs /gsd-core/bin/lib/observability/redaction.cjs diff --git a/CONTEXT.md b/CONTEXT.md index a93ff4528..e82fd720b 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -109,6 +109,9 @@ Module owning project-root resolution from any starting directory. Walks the anc ### Planning Path Projection Module Module owning projection from project/workstream context to concrete `.planning` paths. Policy precedence is `explicit workstream > env workstream > env project > root`. Invalid workspace context is a validation error at this seam rather than a silent fallback. +### Reviewer Lane Descriptor Module +Module owning the single declared contract for a **reviewer lane** — one external CLI or model endpoint that `/gsd:review` hands a plan to for independent review (ADR-2782 Phase 1, #2794; subsumes #2690). Before it, a lane was declared across three unrelated surfaces — the roster in `src/review-reviewer-selection.cts`, ~640 lines of hand-authored per-CLI bash in the `invoke_reviewers` step of `gsd-core/workflows/review.md`, and the hardcoded section headings in `write_reviews` — so cross-cutting fixes landed per-leg (#2494 and #2605 were the same empty-output defect filed twice; #2475/#2295/#2272 are the same shape). Interface: `REVIEWER_LANES` (frozen table of the 11 shipped lanes, each declaring `slug`, `flags`, `probe`, `invoke`, `timeoutFloorMs`, `emptyOutput`, `reviewsSection`, `evidenceClass`, `requiresBinaries`, `promptBudgetKey`, `handler`), `PARITY_VIOLATION` (frozen reason enum — adding a reason is three coordinated changes: enum + emitting site + the test locking `Object.keys(...).sort()`), and `checkReviewerLaneParity({descriptor, roster, workflowText}) → {ok, violations[]}`. **The module DECLARES; it does not execute** — `invoke_reviewers` still runs hand-authored legs until Phase 5b (#2799) makes it iterate; what Phase 1 guarantees is that a leg cannot be added, removed, or renamed without the table and the `REVIEWS.md` section moving with it. Field names, **nesting**, and enum members track ADR-2782 D1/D2/D6/D7 so Phase 2 (#2795) harvests the shape into the capability manifest with no translation layer — including `transport` at the **lane level** (a sibling of `probe`/`invoke`, as D1's manifest example places it) rather than nested inside `invoke`, which would read more naturally as a TS discriminated union and is exactly the convenience Phase 2 would have to translate away. **Four** vocabulary widenings were forced by surveying the eleven shipped legs, all additive widenings of closed enums, each forced by a lane that exists today — and **ADR-2782 was amended in the same PR** (its Amendments section, 2026-07-29) rather than left diverging, so Phase 2 implements the validator against the amended vocabulary: `promptChannel: 'none'` (CodeRabbit is fed no prompt — it reviews the working-tree diff), `outputChannel: 'file-arg'` (Codex captures the review through its own `-o/--output-last-message` and discards stdout, #1698), `outputArg` (its companion — knowing the review lands in a file is useless without the argument naming the file), and `flags: string[]` where D1 shows a singular `flag` (Antigravity is selected by both `--antigravity` and `--agy`, which one field cannot express; this also widens D8's uniqueness invariant, enforced here over the flattened flag set). `LANE_SLUG_RE` (`^[a-z0-9][a-z0-9_-]*$`) pins the slug grammar because `LEG_MARKER_RE` can only capture `[a-z0-9_-]`: a slug outside that class is **unmatchable** — its marker can be present and correct and the scan still never sees it — so a violating slug is reported `INVALID_SLUG` rather than reported missing forever. A loud named violation beats a silent miss. The descriptor deliberately does **not** promise uniformity — lane divergence is real and frequently correct (three lanes are HTTP endpoints with no binary; timeout floors genuinely differ; Antigravity needs a three-layer fallback for an upstream stdout bug) — so behavior data cannot express is delegated to a named `handler` (closed first-party enum: `null` | `antigravity` | `openai-compatible`), never to conditionals inside the table. `checkReviewerLaneParity` is the `DEFECT.GENERATIVE-FIX` assertion the roster has never had, and it is **bidirectional**: a forward-only check misses the failure it exists to catch (#2718 added a lane leg, #2781 was the drift that followed), so an undeclared leg fails too. Legs are identified by an explicit `` marker rather than inferred from prose shape, because five non-lane bold labels in `invoke_reviewers` share the bold-then-fence shape a heuristic would key on. Section matching is anchored at h2 with an exact ` Review` suffix and no parenthetical, so ADR-1517 reviewer-instance headings (`## OpenCode Review (opencode-deepseek)`) are exempt — ADR-2782 D8: instances are not lanes. Pure and total: no filesystem access, CRLF-insensitive, and **never throws on any input** — every field is validated before use (`MALFORMED_LANE` / `INVALID_SLUG`) rather than trusted, because Phase 2 feeds this same function manifest-derived data from third-party overlays, and a parity gate that crashes on bad input is indistinguishable from one that was never run. Empty input degrades to violations so a read failure is never mistaken for a clean bill of health. Totality, determinism (no leaked regex `lastIndex`), and the invalid-slug contract are `fast-check` property-tested with a pinned seed. Source of truth: `src/review-lane-descriptor.cts`. Test anchor: `tests/review-lane-descriptor.test.cjs`. See `docs/adr/2782-reviewer-lane-capability-surface.md`. + ### Resolution Provenance Cross-seam principle (ADR-1411, epic #1411): context resolution — config loading, project-root anchoring, workstream resolution — must report its provenance, not fall open silently to defaults. A resolver anchors deterministically to the project root (one walk-up module, no dependence on an arbitrary descendant cwd), returns *what* it resolved **and** *where it came from* (`source`/`degraded`), and surfaces a diagnostic when a *configured* input resolves empty (`not configured` and `configured-but-empty` are distinguishable). The resolution-side analog of ADR-227 (input-validation shape). Target seams: Config Loader Module (`loadConfig` → `ConfigResolution { config, source, degraded }`), Project-Root Resolution Module (single nearest-`.planning/` walk-up, retiring ad-hoc resolvers like `resolvePlanningCwd`), I/O Module (`Resolution { value, configured, reason, warnings }` output envelope). A configured input resolving empty without a reason is a CI-guarded regression. **P1 (nearest-.planning/ heuristic) shipped in #1413; P2 (loadConfigResolved + agent-skills diagnostic) shipped in #1415 / closes #1366**: `loadConfigResolved` now implements the Config Loader seam target; `cmdAgentSkills` uses `findProjectRoot` + `loadConfigResolved` and emits `configured`/`reason`/`source`/`degraded` in its `--json` IR. **Corrupt is not absent (ADR-1411 amendment 2026-07-26, epic #1879 Phase 0 / #2674):** the principle above governs a resolution *miss*; input that is present but *not usable* (a `SyntaxError`, an errno such as `EACCES`/`EIO`, or a malformed structure with no exception at all) is a distinct class that must stay distinguishable from genuine absence. The defect in that class is not the fallback — ADR-227 requires malformed input to be coerced rather than propagated, and this ADR already permits a fallback — it is that the fallback is **invisible**. So every current return value is preserved and the cause is made visible by one of two mechanisms: **in-band**, where the result already carries a provenance envelope, name the cause in it (`ConfigResolution` gains a `reason`; `Resolution`'s four documented values all describe a miss, so new unusable-input values are introduced with the first adopter) and also expose it on the surface callers actually use, since a `reason` no caller reads is an unreachable field; **out-of-band**, where the read returns a bare sentinel or a plausible default it cannot extend, keep that value and emit a deduplicated `stderr` diagnostic keyed on resolved-path + errno, reusing the `_warnedUnknownConfigKeys` guard pattern. The diagnostic is unconditional — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since an opt-in nobody sets is the same silence. Throwing is **not** the cluster's answer — it stays confined to ADR-227's genuinely-fatal carve-out, decided per call, never inferred from the return shape. diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index bb36a5e47..00fd4d6eb 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -1470,7 +1470,11 @@ Reviewers are prompted to verify the plan's claims against the actual repository | `--llama-cpp` | Include llama.cpp server review | | `--all` | Include all available reviewers (CLI + local model servers) | -**`jq` prerequisite (some lanes only):** `--ollama`, `--lm-studio`, `--llama-cpp`, `--opencode`, and `--agy` parse JSON that GSD does not produce itself — OpenAI-compatible `/v1/chat/completions` responses, OpenCode's JSONL event stream, and Antigravity's conversation cache — so they require [`jq`](https://jqlang.org/download/) on your `PATH`. If `jq` is missing, `/gsd-review` reports those five as unavailable and tells you to install it, rather than running them into an empty review. The other six lanes (`--gemini`, `--claude`, `--codex`, `--coderabbit`, `--qwen`, `--cursor`) need no `jq`. Reading your configured models, hosts, and token budgets never requires `jq`. +**`jq` prerequisite (some lanes only):** `--ollama`, `--lm-studio`, `--llama-cpp`, `--opencode`, and `--agy` parse JSON that GSD does not produce itself — OpenAI-compatible `/v1/chat/completions` responses, OpenCode's JSONL event stream, and Antigravity's conversation cache — so they require [`jq`](https://jqlang.org/download/) on your `PATH`. If `jq` is missing, `/gsd-review` reports those five as unavailable and tells you to install it, rather than running them into an empty review — an info note when the lane was reached through `--all` or `review.default_reviewers`, an error when you named it with an explicit flag. The other six lanes (`--gemini`, `--claude`, `--codex`, `--coderabbit`, `--qwen`, `--cursor`) need no `jq`. Reading your configured models, hosts, and token budgets never requires `jq`. + +**Unavailable reviewers:** an explicit reviewer flag is an assertion. If you name a reviewer that cannot run on this host — its CLI is not installed, a prerequisite such as `jq` is missing, or its local server is unreachable — `/gsd-review` reports an **error** for that reviewer and does not proceed with a reduced set. This holds even when other named reviewers are available: `--gemini --qwen` on a host without `qwen` fails rather than silently becoming a Gemini-only review. + +Reviewers reached through `--all` or `review.default_reviewers` behave differently: an undetected reviewer there is reported as an info note and skipped. Use `--all` for "whatever is available on this host", and `review.default_reviewers` for a preferred subset that may vary by host. **Default reviewer behavior (no flags):** - If `review.default_reviewers` is **unset**, `/gsd-review` runs all detected reviewers (current default behavior). diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 25d468ac9..7e8a55bf6 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -245,7 +245,7 @@ Use `review.default_reviewers` to scope the no-flag `/gsd-review` run to a subse | Setting | Type | Default | Description | |---------|------|---------|-------------| -| `review.default_reviewers` | string[] \| null | `null` (all detected reviewers) | Optional default subset for no-flag `/gsd-review`, e.g. `["gemini","codex"]`. Entries may be built-in reviewer slugs or configured `review.reviewer_instances` names. Precedence is: explicit reviewer flags > `--all` > `review.default_reviewers` > all detected. Unknown slugs are ignored with a warning when no instances are configured; with `review.reviewer_instances` present, unknown entries are hard errors to catch typoed instance names. Known-but-undetected slugs are ignored with an info note; empty arrays are rejected by `config-set`. | +| `review.default_reviewers` | string[] \| null | `null` (all detected reviewers) | Optional default subset for no-flag `/gsd-review`, e.g. `["gemini","codex"]`. Entries may be built-in reviewer slugs or configured `review.reviewer_instances` names. Precedence is: explicit reviewer flags > `--all` > `review.default_reviewers` > all detected. Unknown slugs are ignored with a warning when no instances are configured; with `review.reviewer_instances` present, unknown entries are hard errors to catch typoed instance names. Known-but-undetected slugs are ignored with an info note; empty arrays are rejected by `config-set`. This leniency is specific to the configured default: a reviewer named by an explicit CLI flag that cannot run is an error, not an info note. | Example: diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index 0b1ea6247..eeba1f918 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -419,6 +419,7 @@ "research-provider.cjs", "research-store.cjs", "resolution.cjs", + "review-lane-descriptor.cjs", "review-reviewer-selection.cjs", "roadmap-command-router.cjs", "roadmap-parser.cjs", diff --git a/docs/adr/2782-reviewer-lane-capability-surface.md b/docs/adr/2782-reviewer-lane-capability-surface.md index ea71eae1d..be5b3c609 100644 --- a/docs/adr/2782-reviewer-lane-capability-surface.md +++ b/docs/adr/2782-reviewer-lane-capability-surface.md @@ -2,6 +2,7 @@ - **Status:** Accepted - **Date:** 2026-07-28 +- **Amended:** 2026-07-29 by Phase 1 ([#2794](https://github.com/open-gsd/gsd-core/issues/2794)) — D1's `flag` becomes `flags[]`; D2's `promptChannel` gains `none`, `outputChannel` gains `file-arg` with a companion `outputArg`; D8's uniqueness invariant restated over the flattened flag set. All four are additive widenings of closed enums, each forced by a shipped lane the original survey did not cover. See **Amendments** at the end. - **Issue:** [#2782](https://github.com/open-gsd/gsd-core/issues/2782) (epic); Phase 0 tracked by [#2793](https://github.com/open-gsd/gsd-core/issues/2793) - **Amends:** [ADR-857](857-capability-system.md) (extension points as data — extends D7/D8 in the same "amend, not reverse" sense ADR-1244 D8 established) · [ADR-894](894-capability-declaration-format.md) (adds a role-typed body and a third role) · [ADR-1016](1016-runtime-capability-descriptor.md) (the runtime body is no longer the *only* body a `role: "runtime"` capability may carry; its closed-vocabulary principle is upheld, not relaxed — see D6) · [ADR-1244](1244-capability-ecosystem.md) (D5 gains a fourth executable-surface disclosure class; D9's matrix gains a lane column) - **Unchanged and explicitly out of scope:** [ADR-0011](0011-review-default-reviewers.md) (reviewer selection precedence) · [ADR-1517](1517-reviewer-instances-config-surface.md) (the `REVIEWS.md` contract and reviewer instances) @@ -96,7 +97,7 @@ A reviewer lane is declared as data in a `reviewer` body: "reviewer": { "slug": "acme", - "flag": "--acme", + "flags": ["--acme"], "transport": "spawn", "probe": { "kind": "command-exists", "binary": "acme" }, "invoke": { @@ -146,8 +147,9 @@ context (b) describes. |---|---|---| | `invoke.binary` | required | **forbidden** | | `invoke.args` | required (array) | forbidden | -| `invoke.promptChannel` | `stdin` \| `argv` \| `argv-file-ref` | forbidden | -| `invoke.outputChannel` | `stdout` | forbidden | +| `invoke.promptChannel` | `stdin` \| `argv` \| `argv-file-ref` \| `none` | forbidden | +| `invoke.outputChannel` | `stdout` \| `file-arg` | forbidden | +| `invoke.outputArg` | required iff `outputChannel: "file-arg"`, else forbidden | forbidden | | `invoke.hostConfigKey` | forbidden | required (dotted config key holding the base URL) | | `invoke.path` | forbidden | required (e.g. `/v1/chat/completions`) | | `invoke.modelDiscovery` | forbidden | closed enum: `none` \| `first-from-models-endpoint` | @@ -166,11 +168,27 @@ the run directory. That instruction must also carry the **absolute repository ro argv-fed CLI does not reliably inherit the review's working directory — the existing `cursor` and `kimi-code` legs already do this by hand (`review.md:447-448`, `:550-552`). -`outputChannel` has exactly one member (`stdout`) today. It is nonetheless a required, named, -closed-enum field rather than an implicit assumption, because the alternative — a lane that writes -its review to a file and prints nothing — is a shape a real CLI can take, and an unnamed assumption -is the thing a later contributor silently violates. A one-member enum that must grow under review is -the ADR-1016 pattern; an undocumented assumption is not. +`outputChannel` is a required, named, closed-enum field rather than an implicit assumption, because +the alternative — a lane that writes its review to a file and prints nothing — is a shape a real CLI +can take, and an unnamed assumption is the thing a later contributor silently violates. + +**Amended 2026-07-29 (#2794):** this ADR originally recorded `outputChannel` as having "exactly one +member (`stdout`) today" and described the file-writing lane as a shape a real CLI *could* take. It +already does. `codex` captures its review through its own `-o/--output-last-message ` and +discards stdout, because on Windows it writes process-teardown output to stdout *after* the final +message, and a stdout redirect would append that noise to a non-empty file — slipping past the +empty-output guard as a silently polluted review (#1698). The enum therefore ships with two members, +and `file-arg` carries a companion `outputArg` naming the argument that takes the path: knowing the +review lands in a file is useless without it. + +`promptChannel` likewise gains `none`. `coderabbit` is fed no prompt at all — it reviews the +working-tree diff and accepts neither a prompt nor a model flag (`review.md:367`). The original +three-member enum had no way to say "this lane receives nothing", which would have forced Phase 2 to +either invent a sentinel or mis-declare the lane. + +Both were found by building Phase 1's descriptor table against all eleven shipped legs. That is the +same evidence path that produced `openai-http` in the first place, and it is the process working: +the vocabulary widens on a lane that exists, never on speculation. Three further declared fields carry per-lane divergence that would otherwise live only in prose: @@ -415,8 +433,14 @@ degrades a lane rather than hanging a command. ### D8 — Uniqueness is a build-time conformance invariant -Across the merged first-party ∪ overlay set, `reviewer.slug`, `reviewer.flag`, and -`reviewer.reviewsSection` are each unique. A collision fails the build gate. `reviewsSection` +Across the merged first-party ∪ overlay set, `reviewer.slug`, `reviewer.flags`, and +`reviewer.reviewsSection` are each unique — for `flags`, over the **flattened** set of every lane's +flags, since one lane may declare several. A collision fails the build gate. + +**Amended 2026-07-29 (#2794):** D1 originally declared a singular `flag`, and this invariant was +stated over it. `antigravity` is selected by **both** `--antigravity` and `--agy` +(`review.md`, `docs/COMMANDS.md`), which a single-valued field cannot express, so the field is +`flags: string[]` and uniqueness flattens across lanes. `reviewsSection` uniqueness is not cosmetic: two lanes sharing a heading would silently merge their output in `REVIEWS.md`, producing a review that appears to have consensus it does not have. @@ -556,3 +580,33 @@ Phase 1 parity assertion green across the entire migration. cases that genuinely are servers. 8. **One `role: "reviewer"` for every lane**, splitting the six dual-purpose runtimes. Cleaner discriminator; rejected for churn (D3). + +## Amendments + +### 2026-07-29 — vocabulary widened by Phase 1 (#2794) + +Phase 1 built the core descriptor table against all eleven shipped legs, which is the first time +every lane's contract was written down in one place. That surfaced four cases the original survey +did not cover. All four are **additive widenings of closed enums**, none reverses a decision, and +each is forced by a lane that exists today rather than by a hypothetical: + +| # | Decision | Was | Is | Forced by | +|---|---|---|---|---| +| 1 | D2 | `promptChannel: stdin \| argv \| argv-file-ref` | adds `none` | `coderabbit` is fed no prompt — it reviews the working-tree diff | +| 2 | D2 | `outputChannel: stdout` ("exactly one member today") | adds `file-arg` | `codex` already writes via `-o/--output-last-message` and discards stdout (#1698) | +| 3 | D2 | — | adds `outputArg`, required iff `file-arg` | knowing the review lands in a file is useless without the argument naming it | +| 4 | D1, D8 | `flag: string` | `flags: string[]`, uniqueness flattened | `antigravity` is selected by both `--antigravity` and `--agy` | + +**Why this is the process working, not a design failure.** D2 already records that the original +draft assumed one lane shape and that reading all twelve legs disproved it — `openai-http` exists +because a survey produced evidence, not because anyone predicted it. These four are the same +mechanism at the next level of detail: the vocabulary widens when a real lane does not fit, under +review, and never on speculation. D6's escalation path ("file an issue naming the primitive the +vocabulary lacks") is for third parties; a first-party phase that finds the gap while implementing +amends the ADR directly, which is what happened here. + +**What this does not change.** No decision is reversed. `transport` remains a closed two-member +discriminator selecting the invoke sub-shape; `probe.kind` and `handler` are untouched; the +absent-safe invariant (D4), the disclosure class (D5), and the config-ownership table (D9) are +unaffected. Phase 2 (#2795) implements the manifest validator against the vocabulary **as amended +here**, which is the point of amending rather than leaving it for Phase 2 to rediscover. diff --git a/eslint.config.mjs b/eslint.config.mjs index 4d4b8fc01..1e589ec7b 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -92,6 +92,7 @@ export default tseslint.config( 'gsd-core/bin/lib/clock.cjs', 'gsd-core/bin/lib/ui-safety-gate.cjs', 'gsd-core/bin/lib/review-reviewer-selection.cjs', + 'gsd-core/bin/lib/review-lane-descriptor.cjs', 'gsd-core/bin/lib/clusters.cjs', 'gsd-core/bin/lib/installer-migrations/001-legacy-orphan-files.cjs', 'gsd-core/bin/lib/observability/redaction.cjs', diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index 22fda7a34..8507c8b98 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -50,9 +50,11 @@ command -v jq >/dev/null 2>&1 && echo "jq:available" || echo "jq:missing" **jq-dependent reviewer lanes.** `jq` is a production prerequisite for the `ollama`, `lm_studio`, `llama_cpp`, `opencode`, and `antigravity` lanes only. If `detect_clis` reports `jq:missing`, treat those five as **undetected** — they -follow the same "known-but-undetected" path as a missing CLI (info note, ignored; -and if they were the only selected reviewers, fail with the actionable message -below rather than producing an empty review). Tell the user to install jq: +follow the same "known-but-undetected" path as a missing CLI. Which path that is +depends on how the lane was selected (see the precedence rules below): reached +through `review.default_reviewers` or `--all` it is an info note and the lane is +ignored; named by an explicit flag it is an **error**, because the user asserted +that lane. Tell the user to install jq: ``` NOTE: jq is not on PATH — the ollama, lm_studio, llama_cpp, opencode, and @@ -85,10 +87,22 @@ Reviewer-selection precedence: 3. `review.default_reviewers` 4. No key + no flags → all detected reviewers +**Explicit reviewer flags are an assertion, not a preference (ADR-2782 D4).** A lane the user +named on the command line and that cannot run is an **error**, surfaced and non-silent — even +when other named lanes did run. Do not proceed with a thinner reviewer set and report success: +`--gemini --qwen` on a host without `qwen` fails, it does not quietly become a Gemini-only +review. This applies however the lane became unavailable — binary missing, prerequisite `jq` +absent, or a local server not reachable. + +The asymmetry is deliberate: *not finding a lane nobody asked for is normal; failing to run a +lane somebody asked for is an error.* A user who wants "whatever is available" has `--all`; a +user who wants a preferred set has `review.default_reviewers`. Both stay lenient below. + `review.default_reviewers` behavior: - Value must be a non-empty array of slug strings (configured via `gsd config-set review.default_reviewers '["gemini","codex"]'`) - Unknown slugs warn and are ignored -- Known-but-undetected slugs emit an info note and are ignored +- Known-but-undetected slugs emit an info note and are ignored — a configured default is a + preference evaluated across many hosts, so a subset being present is expected, not an error - If all configured reviewers are unavailable, fail with an actionable message **Reviewer instances (#1517, optional):** if `review.reviewer_instances` is configured, @@ -303,6 +317,7 @@ For each selected CLI, invoke in sequence (not parallel — avoid rate limits): **Timeout guidance (#2194):** prompt-fed source-grounded reviews are slow — measured ~570s for Codex at `xhigh` effort and ~525s for headless Claude on a large plan set. Each of the Gemini / Claude / Codex blocks below MUST be invoked with a high Bash `timeout:` — at least `900000` (15 min), and `1200000` (20 min) for Codex `xhigh` or headless Claude — so a lane is not killed mid-review. On Claude Code, raise the host cap via `BASH_MAX_TIMEOUT_MS` if a review can exceed it. A silent empty output after a long run is a **timeout kill, not a crash** — the Codex `0xc0000142` misdiagnosis persisted because the empty-output branches below cannot distinguish the two; treat an empty result on a slow lane as a dropped lane and re-run with more time rather than diagnosing a CLI/sandbox failure. A cross-AI review that silently drops a lane is blind in one eye. + **Gemini:** ```bash # #2494: capture stderr to a .err sidecar (not /dev/null) and stub an empty @@ -326,6 +341,7 @@ if [ ! -s {run_dir}/gsd-review-gemini.md ]; then fi ``` + **Claude (separate session):** ```bash # #2494: same guard as the Gemini block above — stderr to a .err sidecar @@ -341,6 +357,7 @@ if [ ! -s {run_dir}/gsd-review-claude.md ]; then fi ``` + **Codex:** ```bash # No hook-trust bypass flag — see the #2479 note above. Capture stderr to a .err @@ -362,6 +379,7 @@ if [ ! -s {run_dir}/gsd-review-codex.md ]; then fi ``` + **CodeRabbit:** Note: CodeRabbit reviews the current git diff/working tree — it does not accept a prompt or model flag. It may take up to 5 minutes. Use `timeout: 360000` on the Bash tool call. The source-grounding requirement in the build_prompt Review Instructions applies only to the prompt-fed reviewers above; CodeRabbit is a diff-only reviewer and never receives it. Treat its output as a diff observation, not a grounded plan-level verdict. @@ -378,6 +396,7 @@ if [ ! -s {run_dir}/gsd-review-coderabbit.md ]; then fi ``` + **OpenCode (via GitHub Copilot):** OpenCode's default `build` agent is an agentic coder, not a prompt→completion API. @@ -426,14 +445,22 @@ else fi ``` + **Qwen Code:** ```bash -cat {run_dir}/gsd-review-prompt.md | qwen - 2>/dev/null > {run_dir}/gsd-review-qwen.md +# #2794: the last leg still sending stderr to /dev/null. Every other lane +# captures it to a .err sidecar and appends it to the stub (#2494/#2605); qwen +# wrote a bare "failed or returned empty output." with no diagnostic at all, so +# a missing binary, an auth prompt, and a rate-limit were indistinguishable from +# each other and from a clean empty review. +cat {run_dir}/gsd-review-prompt.md | qwen - 2>{run_dir}/gsd-review-qwen.err > {run_dir}/gsd-review-qwen.md if [ ! -s {run_dir}/gsd-review-qwen.md ]; then - echo "Qwen review failed or returned empty output." > {run_dir}/gsd-review-qwen.md + echo "Qwen review failed or returned empty output. stderr:" > {run_dir}/gsd-review-qwen.md + cat {run_dir}/gsd-review-qwen.err >> {run_dir}/gsd-review-qwen.md 2>/dev/null fi ``` + **Cursor:** ```bash # cursor-agent is a SEPARATE binary from the `cursor` IDE launcher; print mode (-p) takes the @@ -453,6 +480,7 @@ if [ ! -s {run_dir}/gsd-review-cursor.md ]; then fi ``` + **Antigravity CLI:** **Maintainer note — why this block has three layers (last updated against agy 1.0.16):** @@ -638,6 +666,7 @@ if [ -s {run_dir}/gsd-review-antigravity.md ] && \ fi ``` + **Ollama (local, OpenAI-compatible):** Read host and model from config. All three local backends share the same `/v1/chat/completions` endpoint — only host and model differ. Use `jq --rawfile` to safely encode the multi-line prompt as JSON without shell-escaping issues. @@ -741,6 +770,7 @@ echo "Ollama review skipped: prompt budget (${OLLAMA_REVIEWER_BUDGET} tokens) to fi ``` + **LM Studio (local, OpenAI-compatible):** ```bash # Resolve prompt budget for LM Studio: per-reviewer override > global default > null @@ -826,6 +856,7 @@ echo "LM Studio review skipped: prompt budget (${LM_STUDIO_REVIEWER_BUDGET} toke fi ``` + **llama.cpp (local, OpenAI-compatible):** ```bash # Resolve prompt budget for llama.cpp: per-reviewer override > global default > null diff --git a/src/review-lane-descriptor.cts b/src/review-lane-descriptor.cts new file mode 100644 index 000000000..dd1d06a95 --- /dev/null +++ b/src/review-lane-descriptor.cts @@ -0,0 +1,656 @@ +/** + * Reviewer Lane Descriptor Module (ADR-2782 Phase 1, #2794 — closes #2690). + * + * ONE place where the cross-AI reviewer lane contract is declared as data. + * + * Before this module the contract lived in three unrelated surfaces: the roster + * in `review-reviewer-selection.cts`, ~640 lines of hand-authored per-CLI bash in + * `gsd-core/workflows/review.md` `invoke_reviewers`, and the hardcoded section + * headings in `write_reviews`. A cross-cutting fix therefore landed per-leg — + * #2494 and #2605 were the same empty-output defect filed twice, and #2475 / + * #2295 / #2272 are the same shape. + * + * SCOPE (ADR-2782's phase table). This module DECLARES; it does not execute. + * `invoke_reviewers` still runs hand-authored legs until Phase 5b (#2799) makes + * it iterate. What Phase 1 buys is that a leg can no longer be added, removed, + * or renamed without the table and the REVIEWS.md section moving with it — + * `checkReviewerLaneParity` below is the `DEFECT.GENERATIVE-FIX` assertion + * (`CONTEXT.md:797`) that the roster has never had. + * + * The descriptor deliberately does NOT promise uniformity. Lane divergence is + * real and frequently correct (measured timeout floors differ; three lanes are + * HTTP endpoints with no binary; Antigravity needs a three-layer fallback for an + * upstream stdout bug). The value is one place where divergence is DECLARED. + * Behaviour that data cannot express is delegated to a named `handler` + * (ADR-2782 D6 closed enum) — never to conditionals inside the table. + * + * Field names, nesting, and enum members track ADR-2782 D1/D2/D6/D7 so Phase 2 + * (#2795) harvests this shape into the capability manifest without a translation + * layer — INCLUDING `transport`'s placement at the lane level (see SpawnLane). + * + * Building this table against all eleven shipped legs surfaced four cases the + * ADR's original survey did not cover. Rather than diverge silently — which is + * exactly the translation layer this module exists to avoid — ADR-2782 was + * AMENDED in the same PR (see its Amendments section, 2026-07-29). All four are + * additive widenings of closed enums, each forced by a lane that exists today: + * + * 1. `promptChannel: 'none'` — CodeRabbit reviews the working-tree diff and is + * fed no prompt at all. + * 2. `outputChannel: 'file-arg'` — Codex writes the review via its own + * `-o/--output-last-message ` and discards stdout (#1698), because on + * Windows it emits teardown noise to stdout after the final message. + * 3. `outputArg` — the companion to (2): knowing the review lands in a file is + * useless without the argument that names the file. + * 4. `flags: string[]` — Antigravity is selected by BOTH `--antigravity` and + * `--agy`, which a single-valued field cannot express. This also flattens + * D8's uniqueness invariant across every lane's flags. + * + * Phase 2 (#2795) implements the manifest validator against the amended + * vocabulary, which is the point of amending rather than leaving it to be + * rediscovered. + */ + +/** ADR-2782 D2 — closed transport discriminator; selects the invoke sub-shape. */ +export type LaneTransport = 'spawn' | 'openai-http'; + +/** ADR-2782 D2, widened by the CodeRabbit lane (see module header). */ +export type PromptChannel = 'stdin' | 'argv' | 'argv-file-ref' | 'none'; + +/** ADR-2782 D2, widened by the Codex lane (see module header). */ +export type OutputChannel = 'stdout' | 'file-arg'; + +/** ADR-2782 D2 — how reasoning effort reaches the tool. */ +export type EffortChannel = 'none' | 'argv' | 'env'; + +/** ADR-2782 D2 — CodeRabbit reviews a diff, not the source tree (`review.md:367`). */ +export type EvidenceClass = 'source-grounded' | 'diff-only'; + +/** ADR-2782 D6 — closed enum of first-party imperative modules. Ported in Phase 5b. */ +export type LaneHandler = null | 'antigravity' | 'openai-compatible'; + +/** + * What a lane does when it produces no usable output. + * `stub-with-stderr` is the normalized policy (#2494/#2605): write a diagnostic + * stub carrying the captured stderr, so `write_reviews` can tell a failed lane + * apart from a reviewer that ran cleanly with nothing to report. + * `handler-owned` means the lane's handler writes its own diagnostics. + */ +export type EmptyOutputPolicy = 'stub-with-stderr' | 'handler-owned'; + +/** ADR-2782 D7 — every probe that starts a process or connection MUST be bounded. */ +export type LaneProbe = + | { kind: 'command-exists'; binary: string } + | { kind: 'command-capability'; binary: string; needle: string; timeoutMs: number } + | { kind: 'http-reachable'; hostConfigKey: string; path: string; timeoutMs: number }; + +export interface SpawnInvoke { + binary: string; + args: ReadonlyArray; + promptChannel: PromptChannel; + outputChannel: OutputChannel; + /** Present only when `outputChannel === 'file-arg'`. */ + outputArg?: string; + /** `null` when the lane accepts no model override. */ + modelArg: string | null; + effortChannel: EffortChannel; +} + +export interface HttpInvoke { + /** Dotted config key holding the base URL. */ + hostConfigKey: string; + path: string; + modelDiscovery: 'none' | 'first-from-models-endpoint'; + effortChannel: 'none'; +} + +export type LaneInvoke = SpawnInvoke | HttpInvoke; + +interface ReviewerLaneCommon { + slug: string; + /** + * Every CLI flag that selects this lane. First entry is canonical. + * ADR-2782 D1 shows a singular `flag`; see the module header's widening (3). + */ + flags: ReadonlyArray; + probe: LaneProbe; + /** Outer wall-clock bound. An inner tool-native timeout lives in the handler (D6). */ + timeoutFloorMs: number; + emptyOutput: EmptyOutputPolicy; + /** The `## Review` heading in write_reviews. Unique (D8). */ + reviewsSection: string; + evidenceClass: EvidenceClass; + /** External tools required on PATH — `jq` for five lanes. */ + requiresBinaries: ReadonlyArray; + /** Dotted config key for per-lane prompt trimming, or null. */ + promptBudgetKey: string | null; + handler: LaneHandler; +} + +/** + * `transport` sits at the LANE level, a sibling of `probe` and `invoke`, exactly + * as ADR-2782 D1's manifest example places it — not nested inside `invoke`. + * Nesting it would read more naturally as a TypeScript discriminated union, and + * that is precisely the convenience Phase 2 (#2795) would have to translate away + * when harvesting this shape into the capability manifest. The union is + * discriminated at the lane level instead, which costs nothing and keeps the two + * vocabularies identical. + */ +export interface SpawnLane extends ReviewerLaneCommon { + transport: 'spawn'; + invoke: SpawnInvoke; +} + +export interface HttpLane extends ReviewerLaneCommon { + transport: 'openai-http'; + invoke: HttpInvoke; +} + +export type ReviewerLane = SpawnLane | HttpLane; + +const SPAWN_STDIN_STDOUT = { + promptChannel: 'stdin', + outputChannel: 'stdout', +} as const; + +/** + * The eleven lanes shipped today, in `write_reviews` order. + * + * `kimi-code` is deliberately absent: it is net-new with no leg, and ADR-2782 + * lands it in Phase 5b alongside the iteration that can invoke it. Declaring it + * here would make it selectable but not invocable. + */ +export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ + { + slug: 'gemini', + flags: ['--gemini'], + transport: 'spawn', + probe: { kind: 'command-exists', binary: 'gemini' }, + invoke: { + binary: 'gemini', + args: ['-p', '-'], + ...SPAWN_STDIN_STDOUT, + modelArg: '-m', + effortChannel: 'none', + }, + timeoutFloorMs: 900_000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'Gemini', + evidenceClass: 'source-grounded', + requiresBinaries: [], + promptBudgetKey: null, + handler: null, + }, + { + // 1_200_000 rather than the 900_000 floor: headless Claude measured ~525 s + // on a large plan set (review.md:304). + slug: 'claude', + flags: ['--claude'], + transport: 'spawn', + probe: { kind: 'command-exists', binary: 'claude' }, + invoke: { + binary: 'claude', + args: ['-p', '-'], + ...SPAWN_STDIN_STDOUT, + modelArg: '--model', + effortChannel: 'argv', + }, + timeoutFloorMs: 1_200_000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'Claude', + evidenceClass: 'source-grounded', + requiresBinaries: [], + promptBudgetKey: null, + handler: null, + }, + { + // Codex captures the review through its own `-o/--output-last-message` and + // discards stdout, because on Windows it writes process-teardown noise to + // stdout AFTER the final message, which a stdout redirect would append to a + // non-empty file and slip past the empty-output guard (#1698). + slug: 'codex', + flags: ['--codex'], + transport: 'spawn', + probe: { kind: 'command-exists', binary: 'codex' }, + invoke: { + binary: 'codex', + args: ['exec', '--ephemeral', '--skip-git-repo-check', '-'], + promptChannel: 'stdin', + outputChannel: 'file-arg', + outputArg: '-o', + modelArg: '--model', + effortChannel: 'argv', + }, + timeoutFloorMs: 1_200_000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'Codex', + evidenceClass: 'source-grounded', + requiresBinaries: [], + promptBudgetKey: null, + handler: null, + }, + { + // Fed no prompt: CodeRabbit reviews the working-tree diff and accepts + // neither a prompt nor a model flag (review.md:367). Its findings are + // deliberately down-weighted in the consensus step. + slug: 'coderabbit', + flags: ['--coderabbit'], + transport: 'spawn', + probe: { kind: 'command-exists', binary: 'coderabbit' }, + invoke: { + binary: 'coderabbit', + args: ['review', '--prompt-only'], + promptChannel: 'none', + outputChannel: 'stdout', + modelArg: null, + effortChannel: 'none', + }, + timeoutFloorMs: 360_000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'CodeRabbit', + evidenceClass: 'diff-only', + requiresBinaries: [], + promptBudgetKey: null, + handler: null, + }, + { + // `--format json` is the primary invocation, not a fallback: the review text + // lives in assistant `text` parts, which the default formatter drops when the + // agent stops with no final message (#1936). Reconstruction needs jq. + slug: 'opencode', + flags: ['--opencode'], + transport: 'spawn', + probe: { kind: 'command-exists', binary: 'opencode' }, + invoke: { + binary: 'opencode', + args: ['run', '--format', 'json', '-'], + ...SPAWN_STDIN_STDOUT, + modelArg: '--model', + effortChannel: 'argv', + }, + timeoutFloorMs: 660_000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'OpenCode', + evidenceClass: 'source-grounded', + requiresBinaries: ['jq'], + promptBudgetKey: null, + handler: null, + }, + { + slug: 'qwen', + flags: ['--qwen'], + transport: 'spawn', + probe: { kind: 'command-exists', binary: 'qwen' }, + invoke: { + binary: 'qwen', + args: ['-'], + ...SPAWN_STDIN_STDOUT, + modelArg: null, + effortChannel: 'none', + }, + timeoutFloorMs: 900_000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'Qwen', + evidenceClass: 'source-grounded', + requiresBinaries: [], + promptBudgetKey: null, + handler: null, + }, + { + // `cursor-agent` is a SEPARATE binary from the `cursor` IDE launcher. Print + // mode takes the prompt as an ARGUMENT, so a full plan set is passed by file + // reference to stay clear of the 32,767-char Windows execFileSync ceiling. + slug: 'cursor', + flags: ['--cursor'], + transport: 'spawn', + probe: { kind: 'command-exists', binary: 'cursor-agent' }, + invoke: { + binary: 'cursor-agent', + args: ['-p', '--mode', 'ask', '--trust', '--output-format', 'text'], + promptChannel: 'argv-file-ref', + outputChannel: 'stdout', + modelArg: null, + effortChannel: 'none', + }, + timeoutFloorMs: 900_000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'Cursor', + evidenceClass: 'source-grounded', + requiresBinaries: [], + promptBudgetKey: null, + handler: null, + }, + { + // Handler-owned: a three-layer fallback for an upstream stdout bug, a + // two-level timeout (600 s external cap over a 540 s native --print-timeout), + // and a stale-response watermark guard. `timeoutFloorMs` carries the OUTER + // bound only; the inner one lives in the handler (ADR-2782 D6). + slug: 'antigravity', + flags: ['--antigravity', '--agy'], + transport: 'spawn', + probe: { kind: 'command-exists', binary: 'agy' }, + invoke: { + binary: 'agy', + args: ['--print-timeout', '540s', '-p'], + promptChannel: 'argv-file-ref', + outputChannel: 'stdout', + modelArg: '--model', + effortChannel: 'none', + }, + timeoutFloorMs: 600_000, + emptyOutput: 'handler-owned', + reviewsSection: 'Antigravity', + evidenceClass: 'source-grounded', + requiresBinaries: ['jq'], + promptBudgetKey: null, + handler: 'antigravity', + }, + { + slug: 'ollama', + flags: ['--ollama'], + transport: 'openai-http', + probe: { + kind: 'http-reachable', + hostConfigKey: 'review.ollama_host', + path: '/v1/models', + timeoutMs: 2_000, + }, + invoke: { + hostConfigKey: 'review.ollama_host', + path: '/v1/chat/completions', + modelDiscovery: 'first-from-models-endpoint', + effortChannel: 'none', + }, + timeoutFloorMs: 120_000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'Ollama', + evidenceClass: 'source-grounded', + requiresBinaries: ['jq'], + promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.ollama', + handler: 'openai-compatible', + }, + { + slug: 'lm_studio', + flags: ['--lm-studio'], + transport: 'openai-http', + probe: { + kind: 'http-reachable', + hostConfigKey: 'review.lm_studio_host', + path: '/v1/models', + timeoutMs: 2_000, + }, + invoke: { + hostConfigKey: 'review.lm_studio_host', + path: '/v1/chat/completions', + modelDiscovery: 'first-from-models-endpoint', + effortChannel: 'none', + }, + timeoutFloorMs: 120_000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'LM Studio', + evidenceClass: 'source-grounded', + requiresBinaries: ['jq'], + promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.lm_studio', + handler: 'openai-compatible', + }, + { + slug: 'llama_cpp', + flags: ['--llama-cpp'], + transport: 'openai-http', + probe: { + kind: 'http-reachable', + hostConfigKey: 'review.llama_cpp_host', + path: '/v1/models', + timeoutMs: 2_000, + }, + invoke: { + hostConfigKey: 'review.llama_cpp_host', + path: '/v1/chat/completions', + modelDiscovery: 'first-from-models-endpoint', + effortChannel: 'none', + }, + timeoutFloorMs: 120_000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'llama.cpp', + evidenceClass: 'source-grounded', + requiresBinaries: ['jq'], + promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.llama_cpp', + handler: 'openai-compatible', + }, +].map((lane) => Object.freeze(lane)) as ReviewerLane[]); + +/* ------------------------------------------------------------------ * + * DEFECT.GENERATIVE-FIX parity (CONTEXT.md:797) + * ------------------------------------------------------------------ */ + +/** + * Frozen reason enum. Tests assert on these values, never on rendered prose — + * CONTRIBUTING.md "Tests assert on typed structured values". Adding a reason is + * three coordinated changes: this enum, the emitting site, and the test locking + * `Object.keys(...).sort()`. + */ +export const PARITY_VIOLATION = Object.freeze({ + MALFORMED_LANE: 'malformed_lane', + INVALID_SLUG: 'invalid_slug', + ROSTER_SLUG_UNDECLARED: 'roster_slug_undeclared', + DESCRIPTOR_LANE_NOT_IN_ROSTER: 'descriptor_lane_not_in_roster', + LEG_MARKER_MISSING: 'leg_marker_missing', + LEG_MARKER_DUPLICATED: 'leg_marker_duplicated', + LEG_MARKER_UNDECLARED: 'leg_marker_undeclared', + SECTION_MISSING: 'section_missing', + SECTION_DUPLICATED: 'section_duplicated', + SECTION_UNDECLARED: 'section_undeclared', + DUPLICATE_SLUG: 'duplicate_slug', + DUPLICATE_FLAG: 'duplicate_flag', + DUPLICATE_SECTION: 'duplicate_section', +} as const); + +export type ParityViolationReason = + (typeof PARITY_VIOLATION)[keyof typeof PARITY_VIOLATION]; + +export interface ParityViolation { + reason: ParityViolationReason; + subject: string; +} + +export interface ParityResult { + ok: boolean; + violations: ParityViolation[]; +} + +export interface ParityInput { + descriptor: ReadonlyArray; + roster: ReadonlyArray; + /** Full text of gsd-core/workflows/review.md. */ + workflowText: string; +} + +/** + * The machine-readable marker that makes an `invoke_reviewers` leg identifiable. + * + * The legs are prose-labelled (`**Qwen Code:**`, `**LM Studio (local, + * OpenAI-compatible):**`), and five NON-lane bold labels in the same step have + * the identical bold-then-fence shape (`**Timeout guidance (#2194):**`, + * `**No hook-trust bypass (#2479):**`, `**Maintainer note — …:**`). Inferring + * legs from prose shape would be a heuristic asserting what it cannot prove, so + * each leg carries an explicit marker instead. Phase 5b iterates on these. + */ +const LEG_MARKER_RE = //g; + +/** + * The slug grammar, and the reason it is enforced rather than assumed. + * + * `LEG_MARKER_RE` can only capture `[a-z0-9_-]`. A lane whose slug falls outside + * that class is therefore UNMATCHABLE in the workflow: its marker can be present + * and correct and the scan will still never see it, so the lane reports + * `LEG_MARKER_MISSING` forever with no indication why. All eleven shipped slugs + * sit inside the class (`lm_studio`, `llama_cpp` use the underscore), so this + * never bites today — but Phase 2 (#2795) admits third-party overlay lanes, and + * a slug like `acme.reviewer` would silently vanish from a review. + * + * ADR-2782 does not specify a slug grammar. Rather than widen the marker regex — + * which would make an HTML comment scanned out of prose more ambiguous, not less + * — the grammar is pinned here and a violating slug is reported as + * `INVALID_SLUG`. A loud, named violation beats a silent miss; that is the whole + * design principle of this module. + */ +export const LANE_SLUG_RE = /^[a-z0-9][a-z0-9_-]*$/; + +/** + * A lane section heading in write_reviews. + * + * Anchored at h2 with an exact ` Review` suffix and NO parenthetical, because: + * - `## OpenCode Review (opencode-deepseek)` is an ADR-1517 reviewer INSTANCE, + * and ADR-2782 D8 states instances are not lanes — two such headings are + * already in the file, so a naive matcher fails on day one; + * - `## Consensus Summary` has no ` Review` suffix; + * - `# Cross-AI Plan Review — Phase {N}` is h1, not h2. + * `[^()\n]+` excludes the parenthetical form rather than stripping it, so an + * instance heading never resolves to a lane. + */ +const SECTION_HEADING_RE = /^##[ \t]+([^()\n\r]+?) Review[ \t]*$/gm; + +/** Bounds of the step a marker must appear inside. */ +const INVOKE_STEP_RE = /([\s\S]*?)<\/step>/; +const WRITE_STEP_RE = /([\s\S]*?)<\/step>/; + +function sliceStep(workflowText: string, re: RegExp): string { + const m = workflowText.match(re); + return m ? m[1] : ''; +} + +function countOccurrences(haystack: string, re: RegExp): Map { + const counts = new Map(); + // Fresh lastIndex per call — a module-level /g regex carries state between + // calls and would silently skip matches on the second invocation. + const scanner = new RegExp(re.source, re.flags); + let m: RegExpExecArray | null; + while ((m = scanner.exec(haystack)) !== null) { + const key = m[1].trim(); + counts.set(key, (counts.get(key) ?? 0) + 1); + } + return counts; +} + +/** + * Bidirectional parity across four surfaces: the descriptor, the roster + * (`KNOWN_REVIEWER_SLUGS`), the `invoke_reviewers` legs, and the `write_reviews` + * sections. + * + * Bidirectional is the point. A forward-only check ("does each declared lane + * resolve?") misses the failure this exists to catch: #2718 added a lane leg and + * #2781 was the documentation drift that followed. An undeclared leg must fail. + * + * Pure and total: never reads the filesystem, never throws. Empty or malformed + * `workflowText` degrades to violations, so a caller cannot mistake a read + * failure for a clean bill of health. + * + * CRLF-insensitive: `\r` is stripped before matching, because a Windows + * autocrlf checkout would otherwise leave every marker and heading unmatched + * and report the whole roster missing. + */ +export function checkReviewerLaneParity(input: ParityInput): ParityResult { + const { descriptor, roster } = input; + const workflowText = String(input.workflowText ?? '').replace(/\r\n/g, '\n'); + const violations: ParityViolation[] = []; + + const add = (reason: ParityViolationReason, subject: string): void => { + violations.push({ reason, subject }); + }; + + // --- D8 uniqueness within the descriptor itself --- + // + // Every field is validated before use rather than trusted. In Phase 1 the + // descriptor is a frozen in-source table and none of these guards can fire, + // but Phase 2 (#2795) feeds this same function manifest-derived data from + // third-party overlays — which is precisely where a malformed entry arrives. + // A checker that throws on bad input cannot report on it, and a parity gate + // that crashes is indistinguishable from one that was never run. + const seenSlug = new Set(); + const seenFlag = new Set(); + const seenSection = new Set(); + + /** A lane that survived validation: slug is a string in the declared grammar. */ + interface ValidatedLane { + slug: string; + reviewsSection: string | null; + } + const lanes: ValidatedLane[] = []; + + // The declared parameter type says `ReviewerLane[]`, but this function is a + // trust boundary — narrow from `unknown` rather than believing the annotation. + const rawLanes: unknown[] = Array.isArray(descriptor) ? (descriptor as unknown[]) : []; + for (const raw of rawLanes) { + if (raw === null || typeof raw !== 'object') { + add(PARITY_VIOLATION.MALFORMED_LANE, String(raw)); + continue; + } + const lane = raw as Record; + const slug = lane.slug; + if (typeof slug !== 'string' || !LANE_SLUG_RE.test(slug)) { + add(PARITY_VIOLATION.INVALID_SLUG, String(slug)); + continue; + } + const section = typeof lane.reviewsSection === 'string' ? lane.reviewsSection : null; + lanes.push({ slug, reviewsSection: section }); + + if (seenSlug.has(slug)) add(PARITY_VIOLATION.DUPLICATE_SLUG, slug); + seenSlug.add(slug); + + const flags: unknown[] = Array.isArray(lane.flags) ? (lane.flags as unknown[]) : []; + for (const flag of flags) { + if (typeof flag !== 'string') continue; + if (seenFlag.has(flag)) add(PARITY_VIOLATION.DUPLICATE_FLAG, flag); + seenFlag.add(flag); + } + + // Two lanes sharing a heading would silently MERGE their output in + // REVIEWS.md, producing a review that appears to have consensus it does + // not have (ADR-2782 D8). + if (section !== null) { + if (seenSection.has(section)) add(PARITY_VIOLATION.DUPLICATE_SECTION, section); + seenSection.add(section); + } + } + + // --- descriptor <-> roster --- + const rosterSet = new Set( + (Array.isArray(roster) ? roster : []).filter((x): x is string => typeof x === 'string'), + ); + for (const slug of rosterSet) { + if (!seenSlug.has(slug)) add(PARITY_VIOLATION.ROSTER_SLUG_UNDECLARED, slug); + } + for (const slug of seenSlug) { + if (!rosterSet.has(slug)) add(PARITY_VIOLATION.DESCRIPTOR_LANE_NOT_IN_ROSTER, slug); + } + + // --- descriptor <-> invoke_reviewers legs --- + const markerCounts = countOccurrences( + sliceStep(workflowText, INVOKE_STEP_RE), + LEG_MARKER_RE, + ); + for (const lane of lanes) { + const n = markerCounts.get(lane.slug) ?? 0; + if (n === 0) add(PARITY_VIOLATION.LEG_MARKER_MISSING, lane.slug); + else if (n > 1) add(PARITY_VIOLATION.LEG_MARKER_DUPLICATED, lane.slug); + } + for (const slug of markerCounts.keys()) { + if (!seenSlug.has(slug)) add(PARITY_VIOLATION.LEG_MARKER_UNDECLARED, slug); + } + + // --- descriptor <-> write_reviews sections --- + const sectionCounts = countOccurrences( + sliceStep(workflowText, WRITE_STEP_RE), + SECTION_HEADING_RE, + ); + for (const lane of lanes) { + if (lane.reviewsSection === null) continue; + const n = sectionCounts.get(lane.reviewsSection) ?? 0; + if (n === 0) add(PARITY_VIOLATION.SECTION_MISSING, lane.reviewsSection); + else if (n > 1) add(PARITY_VIOLATION.SECTION_DUPLICATED, lane.reviewsSection); + } + for (const section of sectionCounts.keys()) { + if (!seenSection.has(section)) add(PARITY_VIOLATION.SECTION_UNDECLARED, section); + } + + return { ok: violations.length === 0, violations }; +} diff --git a/src/review-reviewer-selection.cts b/src/review-reviewer-selection.cts index 86f5f2b62..b9b5968a7 100644 --- a/src/review-reviewer-selection.cts +++ b/src/review-reviewer-selection.cts @@ -241,12 +241,27 @@ export function resolveReviewerSelection( if (explicitFlags.size > 0) { source = 'explicit_flags'; + // ADR-2782 D4: absent-safe governs DISCOVERY, never explicit selection. + // Not finding a lane nobody asked for is normal; failing to run a lane + // somebody asked for is an error. Every miss used to be an `info`, so a + // PARTIAL miss (`--gemini --qwen` with qwen absent) ran the review with a + // thinner reviewer set while present_results reported success — "a cross-AI + // review that silently drops a lane is blind in one eye" (review.md). + // A total miss already errored, but only as a side effect of the selected + // set being empty; the partial case had no signal at all. + const priorErrorCount = errors.length; selected = [...explicitFlags].filter((slug) => detected.has(slug)); - const missing = [...explicitFlags].filter((slug) => !detected.has(slug)); - if (missing.length > 0) { - infos.push(`explicit reviewers missing on host: ${missing.join(', ')}`); + // Sorted so the error order does not depend on flag order on the command line. + const missing = [...explicitFlags].filter((slug) => !detected.has(slug)).sort(); + for (const slug of missing) { + errors.push(`explicit reviewer '${slug}' is not available on this host`); } - if (selected.length === 0 && errors.length === 0) { + // Guarded on the error count as it stood BEFORE this branch, so the + // aggregate message fires under exactly the conditions it did previously. + // Guarding on `errors.length` would let the per-slug errors just pushed + // suppress it, silently dropping a message this change did not intend to + // remove. + if (selected.length === 0 && priorErrorCount === 0) { errors.push('no selected reviewers are available for explicit flags'); } } else if (allFlag) { diff --git a/tests/emitted-drift-ack.json b/tests/emitted-drift-ack.json new file mode 100644 index 000000000..cc4d6ecb3 --- /dev/null +++ b/tests/emitted-drift-ack.json @@ -0,0 +1,8 @@ +{ + "version": 1, + "paths": { + "review.md": { + "reason": "#2794 / ADR-2782 Phase 1. Three deliberate additions to gsd-core/workflows/review.md: (1) an explicit `` marker above each of the 11 invoke_reviewers legs, which is what makes a leg machine-identifiable for the DEFECT.GENERATIVE-FIX parity assertion — the legs are prose-labelled and five NON-lane bold labels in the same step share the bold-then-fence shape a heuristic matcher would key on, so inference was not an option; (2) the ADR-2782 D4 explicit-selection contract in the detect_clis prose, without which the selector correction is unobservable (review-reviewer-selection.cts has no production caller — the workflow narrates the policy and the reviewing agent executes it, so code and prose must move together); (3) the qwen leg's stderr sidecar, the last lane still discarding stderr to /dev/null (#2494/#2605 class). Growth is prose and comments only; no lane's observable command shape changed apart from the qwen redirect." + } + } +} diff --git a/tests/fix-2794-review-qwen-empty-guard.test.cjs b/tests/fix-2794-review-qwen-empty-guard.test.cjs new file mode 100644 index 000000000..2e9ac37ed --- /dev/null +++ b/tests/fix-2794-review-qwen-empty-guard.test.cjs @@ -0,0 +1,166 @@ +// allow-test-rule: source-text-is-the-product (see #2794) +// The qwen reviewer dispatch block in gsd-core/workflows/review.md IS the runtime +// contract — the workflow's text is what the reviewing agent executes. This suite +// extracts that shell block verbatim and runs it under a real bash against a stubbed +// `qwen`, so the shipped guard is what gets exercised rather than a reimplementation +// of it. Assertions are on the guard's documented output contract (the stub line the +// consensus step must be able to tell apart from a clean empty review, and the +// presence of the captured stderr), not incidental string matching. + +/** + * Regression tests for #2794 — the qwen reviewer leg was the last one still + * sending stderr to /dev/null. + * + * Every other lane captures stderr to a `.err` sidecar and appends it to the + * empty-output stub (#2494 for claude/gemini, #2605 for the local servers and + * CodeRabbit). qwen alone ran `2>/dev/null` and wrote a bare + * "Qwen review failed or returned empty output." with no diagnostic, so a missing + * binary, an auth prompt, a rate-limit, and a genuinely empty review were all + * indistinguishable — the same cross-cutting defect landing per-leg that ADR-2782 + * exists to end. + * + * These tests fail against pre-fix review.md: the stub carries no stderr, so the + * diagnostic assertion in E2 trips. + */ + +'use strict'; + +const { describe, test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const { spawnSync } = require('node:child_process'); +const fs = require('node:fs'); +const path = require('node:path'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +const ROOT = path.join(__dirname, '..'); +const REVIEW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'review.md'); + +// Normalize CRLF: on a Windows autocrlf checkout every line carries a trailing +// \r, which would mangle the extracted block's redirect tokens and defeat the +// fence regex below. +const WORKFLOW = fs.readFileSync(REVIEW_PATH, 'utf-8').replace(/\r\n/g, '\n'); + +/** + * Extract the qwen dispatch block verbatim. If review.md changes the block's + * shape this throws and the test fails loudly — intended coupling, the same + * contract the #2494 and #2605 suites pin for their legs. + */ +function extractQwenBlock() { + // `\r?\n` throughout: WORKFLOW is CRLF-normalized above, but the anchors stay + // CRLF-tolerant so the regex cannot silently miss on a Windows checkout. + const m = WORKFLOW.match( + /\r?\n[\s\S]*?```bash\r?\n([\s\S]*?)\r?\n```/, + ); + assert.ok(m, 'review.md must define the qwen reviewer dispatch as a bash block (#2794)'); + return m[1]; +} + +const QWEN_BLOCK = extractQwenBlock(); + +// The extracted block is POSIX shell. Git Bash on Windows ignores Node's chmod +// exec bit for PATH-executed extension-less scripts, so the stub would never run +// there; the guard logic is platform-independent and is asserted in full on every +// macOS/Linux leg. +const WIN32_SKIP = + 'extracted block is POSIX shell; guard logic is platform-independent and asserted on macOS/Linux'; + +let sandbox; + +before(() => { sandbox = createTempDir('gsd-2794-'); }); +after(() => { cleanup(sandbox); }); + +/** + * Run the extracted block with `{run_dir}` pointed at a fresh run directory and + * `qwenBody` installed on PATH as `qwen`. Pass `qwenBody: null` to run with no + * qwen on PATH at all. + */ +function runQwenLeg(qwenBody) { + // Single exec site so the Windows guard lives in one place: Git Bash (msys2) + // ignores Node's chmod exec bit for PATH-executed extension-less scripts + // (DEFECT.WINDOWS-TEST-PORTABILITY). Every test below is skipped on win32 — + // this early return keeps the exec unreachable there rather than relying on + // the skip alone. + if (process.platform === 'win32') return null; + + const caseDir = fs.mkdtempSync(path.join(sandbox, 'run-')); + const runDir = path.join(caseDir, 'run'); + const binDir = path.join(caseDir, 'bin'); + fs.mkdirSync(runDir); + fs.mkdirSync(binDir); + + if (qwenBody !== null) { + const stub = path.join(binDir, 'qwen'); + fs.writeFileSync(stub, qwenBody); + fs.chmodSync(stub, 0o755); + } + + fs.writeFileSync(path.join(runDir, 'gsd-review-prompt.md'), '# review prompt\n'); + + const script = QWEN_BLOCK.split('{run_dir}').join(runDir); + const result = spawnSync('bash', ['-c', script], { + encoding: 'utf8', + timeout: 30000, + killSignal: 'SIGKILL', + // An empty PATH would also break `cat`; prepend the stub dir instead so the + // no-qwen case still resolves the shell builtins the block needs. + env: { ...process.env, PATH: `${binDir}${path.delimiter}${process.env.PATH}` }, + }); + + const reviewPath = path.join(runDir, 'gsd-review-qwen.md'); + return { + result, + reviewPath, + errPath: path.join(runDir, 'gsd-review-qwen.err'), + review: fs.existsSync(reviewPath) ? fs.readFileSync(reviewPath, 'utf-8') : null, + }; +} + +const STUB_STDERR = 'gsd-2794-stub: qwen: authentication required'; + +describe('qwen reviewer leg — empty-output guard (#2794)', () => { + test('writes the review on success', { skip: process.platform === 'win32' ? WIN32_SKIP : false }, () => { + const out = runQwenLeg('#!/bin/sh\necho "## Qwen findings"\nexit 0\n'); + assert.ok(out.review !== null, 'review file must exist'); + assert.match(out.review, /Qwen findings/); + // A successful lane must NOT be decorated with the failure stub. + assert.doesNotMatch(out.review, /failed or returned empty output/i); + }); + + test('a failed lane surfaces its stderr in the review stub', { skip: process.platform === 'win32' ? WIN32_SKIP : false }, () => { + // THE regression row. Pre-fix the block ran `2>/dev/null`, so this stderr + // was discarded and the stub carried no diagnostic at all. + const out = runQwenLeg(`#!/bin/sh\necho "${STUB_STDERR}" >&2\nexit 1\n`); + assert.ok(out.review !== null, 'review file must exist after a failed lane'); + assert.notStrictEqual(out.review.trim(), '', 'review file must not be empty'); + assert.match( + out.review, + /Qwen review failed or returned empty output/i, + 'stub must name the lane as failed-or-empty so consensus can tell it apart', + ); + assert.ok( + out.review.includes(STUB_STDERR), + `stub must carry the captured stderr; got: ${JSON.stringify(out.review)}`, + ); + assert.ok(fs.existsSync(out.errPath), 'stderr sidecar must be written'); + }); + + test('a silently empty lane still produces a diagnosable stub', { skip: process.platform === 'win32' ? WIN32_SKIP : false }, () => { + // Boundary: nothing on either stream. The stub line is the only signal, and + // it must still be present so write_reviews does not render the lane as a + // reviewer that ran cleanly with nothing to report. + const out = runQwenLeg('#!/bin/sh\nexit 0\n'); + assert.ok(out.review !== null, 'review file must exist'); + assert.notStrictEqual(out.review.trim(), '', 'review file must not be zero-byte'); + assert.match(out.review, /Qwen review failed or returned empty output/i); + }); + + test('a missing qwen binary produces a diagnosable stub', { skip: process.platform === 'win32' ? WIN32_SKIP : false }, () => { + const out = runQwenLeg(null); + assert.ok(out.review !== null, 'review file must exist when the binary is absent'); + assert.notStrictEqual(out.review.trim(), '', 'review file must not be zero-byte'); + assert.match(out.review, /Qwen review failed or returned empty output/i); + // The shell's own "command not found" lands in the sidecar and is appended, + // which is the whole point of capturing instead of discarding. + assert.ok(fs.existsSync(out.errPath), 'stderr sidecar must be written'); + }); +}); diff --git a/tests/review-lane-descriptor.test.cjs b/tests/review-lane-descriptor.test.cjs new file mode 100644 index 000000000..4e2f4211a --- /dev/null +++ b/tests/review-lane-descriptor.test.cjs @@ -0,0 +1,734 @@ +'use strict'; + +/** + * Reviewer Lane Descriptor + DEFECT.GENERATIVE-FIX parity (#2794, ADR-2782 Phase 1). + * + * `CONTEXT.md:797` requires that a constant shared between two parallel surfaces + * carry a parity assertion failing when they diverge. The reviewer roster has + * never had one: it is declared across four surfaces — the descriptor, the + * roster in `review-reviewer-selection.cts`, the `invoke_reviewers` legs, and the + * `write_reviews` section headings — and only the Cursor lane has ever been + * parity-checked at all. + * + * The assertion is exercised in BOTH directions, because a forward-only check + * ("does every declared lane resolve?") misses the failure this exists to catch: + * #2718 added a lane leg and #2781 was the documentation drift that followed. So + * every negative row below feeds a SYNTHETIC divergence to the pure checker and + * asserts the specific violation — a parity test that has never been seen to + * fail is a green light on drift, not a guarantee. + * + * Assertions are on the frozen `PARITY_VIOLATION` reason enum, never on rendered + * prose (CONTRIBUTING.md — "tests assert on typed structured values"). + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const fc = require('fast-check'); + +const { + REVIEWER_LANES, + PARITY_VIOLATION, + checkReviewerLaneParity, +} = require('../gsd-core/bin/lib/review-lane-descriptor.cjs'); +const { + KNOWN_REVIEWER_SLUGS, +} = require('../gsd-core/bin/lib/review-reviewer-selection.cjs'); + +const ROOT = path.join(__dirname, '..'); +// Normalized to LF on read so the CRLF cases below can construct a Windows +// checkout deterministically from a known-LF baseline. +const WORKFLOW_TEXT = fs + .readFileSync(path.join(ROOT, 'gsd-core', 'workflows', 'review.md'), 'utf-8') + .replace(/\r\n/g, '\n'); + +/** Render the LF baseline as a Windows autocrlf checkout would store it. */ +function asCrlf(text) { + return text.split('\n').join('\r\n'); +} + +/** Run the checker against the shipped inputs, with targeted overrides. */ +function check(overrides = {}) { + return checkReviewerLaneParity({ + descriptor: REVIEWER_LANES, + roster: KNOWN_REVIEWER_SLUGS, + workflowText: WORKFLOW_TEXT, + ...overrides, + }); +} + +/** The violation reasons produced, as a plain sorted array of `reason:subject`. */ +function reasons(result) { + return result.violations.map((v) => `${v.reason}:${v.subject}`).sort(); +} + +/** A lane object that is structurally valid but names nothing real. */ +function fakeLane(slug) { + return { ...REVIEWER_LANES[0], slug, flags: [`--${slug}`], reviewsSection: slug }; +} + +describe('reviewer lane parity — the shipped repo', () => { + test('descriptor, roster, workflow legs and output sections all agree', () => { + const r = check(); + assert.deepStrictEqual( + r.violations, + [], + `shipped repo must satisfy lane parity; got: ${JSON.stringify(r.violations)}`, + ); + assert.strictEqual(r.ok, true); + }); + + test('the descriptor covers every roster slug and vice versa', () => { + assert.deepStrictEqual( + REVIEWER_LANES.map((l) => l.slug).sort(), + [...KNOWN_REVIEWER_SLUGS].sort(), + ); + }); + + test('parity is evaluated over a non-empty lane set', () => { + // Guards the vacuous-truth failure mode: an empty descriptor trivially + // satisfies every forward check. + assert.ok(REVIEWER_LANES.length >= 11, 'expected at least the 11 shipped lanes'); + }); +}); + +describe('reviewer lane parity — descriptor vs roster', () => { + test('a roster slug with no descriptor entry is a violation', () => { + const r = check({ roster: [...KNOWN_REVIEWER_SLUGS, 'kimi_code'] }); + assert.deepStrictEqual(reasons(r), [ + `${PARITY_VIOLATION.ROSTER_SLUG_UNDECLARED}:kimi_code`, + ]); + }); + + test('a descriptor lane absent from the roster is a violation', () => { + const r = check({ descriptor: [...REVIEWER_LANES, fakeLane('acme')] }); + assert.ok( + reasons(r).includes(`${PARITY_VIOLATION.DESCRIPTOR_LANE_NOT_IN_ROSTER}:acme`), + `expected a not-in-roster violation, got: ${JSON.stringify(reasons(r))}`, + ); + }); +}); + +describe('reviewer lane parity — descriptor vs invoke_reviewers legs', () => { + test('a leg added without a descriptor entry is a violation', () => { + // The #2718 shape: a new lane's bash block lands in the workflow and nothing + // else moves. This is the row a forward-only assertion cannot catch. + const r = check({ + workflowText: WORKFLOW_TEXT.replace( + '', + '\n', + ), + }); + assert.deepStrictEqual(reasons(r), [ + `${PARITY_VIOLATION.LEG_MARKER_UNDECLARED}:kimi_code`, + ]); + }); + + test('a declared lane whose workflow leg was removed is a violation', () => { + const r = check({ + workflowText: WORKFLOW_TEXT.replace('', ''), + }); + assert.deepStrictEqual(reasons(r), [ + `${PARITY_VIOLATION.LEG_MARKER_MISSING}:qwen`, + ]); + }); + + test('a duplicated leg marker is a violation', () => { + const r = check({ + workflowText: WORKFLOW_TEXT.replace( + '', + '\n', + ), + }); + assert.deepStrictEqual(reasons(r), [ + `${PARITY_VIOLATION.LEG_MARKER_DUPLICATED}:qwen`, + ]); + }); + + test('marker matching tolerates whitespace variation', () => { + const r = check({ + workflowText: WORKFLOW_TEXT.replace( + '', + '', + ), + }); + assert.deepStrictEqual(r.violations, []); + }); + + test('a marker outside the invoke_reviewers step does not satisfy the leg', () => { + // Scoped, not file-wide: a marker parked in write_reviews must not be + // mistaken for a dispatch leg. + const moved = WORKFLOW_TEXT + .replace('', '') + .replace('## Qwen Review', '\n## Qwen Review'); + const r = check({ workflowText: moved }); + assert.deepStrictEqual(reasons(r), [ + `${PARITY_VIOLATION.LEG_MARKER_MISSING}:qwen`, + ]); + }); +}); + +describe('reviewer lane parity — descriptor vs write_reviews sections', () => { + test('an output section with no declared lane is a violation', () => { + const r = check({ + workflowText: WORKFLOW_TEXT.replace( + '## Qwen Review', + '## Qwen Review\n\n{qwen}\n\n---\n\n## Kimi Review', + ), + }); + assert.deepStrictEqual(reasons(r), [ + `${PARITY_VIOLATION.SECTION_UNDECLARED}:Kimi`, + ]); + }); + + test('a declared lane with no output section is a violation', () => { + const r = check({ + workflowText: WORKFLOW_TEXT.replace('## Qwen Review', '## Renamed Heading'), + }); + assert.ok( + reasons(r).includes(`${PARITY_VIOLATION.SECTION_MISSING}:Qwen`), + `expected a section-missing violation, got: ${JSON.stringify(reasons(r))}`, + ); + }); + + test('a duplicated output section is a violation', () => { + // Two lanes under one heading would silently MERGE in REVIEWS.md, producing + // a review that appears to have consensus it does not have (ADR-2782 D8). + const r = check({ + workflowText: WORKFLOW_TEXT.replace( + '## Qwen Review', + '## Qwen Review\n\n{a}\n\n---\n\n## Qwen Review', + ), + }); + assert.deepStrictEqual(reasons(r), [ + `${PARITY_VIOLATION.SECTION_DUPLICATED}:Qwen`, + ]); + }); +}); + +describe('reviewer lane parity — not-corruption (must NOT fire)', () => { + test('ADR-1517 instance sections are exempt from lane parity', () => { + // `## OpenCode Review (opencode-deepseek)` and `(opencode-mimo)` are already + // in the shipped file. ADR-2782 D8: reviewer instances are not lanes. A + // naive `## … Review` matcher fails against these on day one. + const r = check(); + assert.deepStrictEqual(r.violations, []); + + const withNewInstance = WORKFLOW_TEXT.replace( + '## Qwen Review', + '## Qwen Review (qwen-turbo)\n\n{x}\n\n---\n\n## Qwen Review', + ); + assert.deepStrictEqual( + checkReviewerLaneParity({ + descriptor: REVIEWER_LANES, + roster: KNOWN_REVIEWER_SLUGS, + workflowText: withNewInstance, + }).violations, + [], + 'adding a reviewer instance section must not trip lane parity', + ); + }); + + test('the h1 title and non-lane headings are not read as lane sections', () => { + // `# Cross-AI Plan Review — Phase {N}` contains "Review" but is h1; + // `## Consensus Summary` is h2 but has no ` Review` suffix. + const r = check(); + assert.deepStrictEqual(r.violations, []); + + const withExtras = WORKFLOW_TEXT.replace( + '## Consensus Summary', + '## Another Summary\n\n---\n\n## Consensus Summary', + ); + assert.deepStrictEqual( + checkReviewerLaneParity({ + descriptor: REVIEWER_LANES, + roster: KNOWN_REVIEWER_SLUGS, + workflowText: withExtras, + }).violations, + [], + ); + }); + + test('bold prose in invoke_reviewers is not read as a leg', () => { + // Five non-lane bold labels share the bold-then-fence shape a heuristic + // matcher would key on. Adding another must not register a lane. + const withProse = WORKFLOW_TEXT.replace( + '', + '**Some new maintainer note (#9999):**\n\n```bash\necho hi\n```\n\n', + ); + assert.deepStrictEqual( + checkReviewerLaneParity({ + descriptor: REVIEWER_LANES, + roster: KNOWN_REVIEWER_SLUGS, + workflowText: withProse, + }).violations, + [], + ); + }); +}); + +describe('reviewer lane parity — cross-platform and hostile input', () => { + test('parity is CRLF-insensitive', () => { + // A Windows autocrlf checkout puts \r on every line; without normalization + // every marker and heading would miss and the whole roster would report + // missing. + const r = check({ workflowText: asCrlf(WORKFLOW_TEXT) }); + assert.deepStrictEqual(r.violations, []); + }); + + test('a divergence is still detected under CRLF', () => { + const crlf = asCrlf( + WORKFLOW_TEXT.replace('', ''), + ); + assert.deepStrictEqual(reasons(check({ workflowText: crlf })), [ + `${PARITY_VIOLATION.LEG_MARKER_MISSING}:qwen`, + ]); + }); + + test('empty workflow text degrades to violations rather than throwing', () => { + // A read failure must never be mistaken for a clean bill of health. + const r = check({ workflowText: '' }); + assert.strictEqual(r.ok, false); + assert.strictEqual( + r.violations.filter((v) => v.reason === PARITY_VIOLATION.LEG_MARKER_MISSING) + .length, + REVIEWER_LANES.length, + ); + }); + + test('non-string workflow text is coerced, never thrown on', () => { + for (const bad of [undefined, null]) { + const r = check({ workflowText: bad }); + assert.strictEqual(r.ok, false, `expected violations for ${String(bad)}`); + } + }); + + test('repeated evaluation is stable (no leaked regex lastIndex)', () => { + // A module-level /g regex carries state between calls and would silently + // skip matches on the second invocation. + const first = check(); + const second = check(); + assert.deepStrictEqual(second.violations, first.violations); + assert.deepStrictEqual(second.violations, []); + }); +}); + +describe('reviewer lane parity — descriptor-internal uniqueness (ADR-2782 D8)', () => { + test('duplicate lane slugs are a violation', () => { + const r = check({ descriptor: [...REVIEWER_LANES, REVIEWER_LANES[0]] }); + assert.ok(reasons(r).some((x) => x.startsWith(PARITY_VIOLATION.DUPLICATE_SLUG))); + }); + + test('duplicate lane flags are a violation', () => { + const clash = { ...fakeLane('acme'), flags: ['--gemini'] }; + const r = check({ descriptor: [...REVIEWER_LANES, clash] }); + assert.ok( + reasons(r).includes(`${PARITY_VIOLATION.DUPLICATE_FLAG}:--gemini`), + `expected a duplicate-flag violation, got: ${JSON.stringify(reasons(r))}`, + ); + }); + + test('duplicate reviewsSection is a violation', () => { + const clash = { ...fakeLane('acme'), reviewsSection: 'Gemini' }; + const r = check({ descriptor: [...REVIEWER_LANES, clash] }); + assert.ok( + reasons(r).includes(`${PARITY_VIOLATION.DUPLICATE_SECTION}:Gemini`), + `expected a duplicate-section violation, got: ${JSON.stringify(reasons(r))}`, + ); + }); +}); + +/** + * `checkReviewerLaneParity` parses markdown for lane markers and section + * headings, so CLAUDE.md's TEST RULES ("Parsers... must include at least one + * fast-check property test") applies. + * + * FIXTURE PROVENANCE (CONTRIBUTING.md #2371): the generators below are + * DOCUMENT-shaped, not writer-seeded. They emit arbitrary markdown — arbitrary + * noise lines, arbitrary heading levels, arbitrary bold labels, markers placed + * at arbitrary positions — rather than being built by calling the same regexes + * the checker uses. A generator seeded from the module's own matchers could only + * ever produce documents the matchers already recognize, which makes the + * document shape a constant and the property unfalsifiable. + * + * Deterministic per repo rules: seed pinned, run count bounded. + */ +describe('reviewer lane parity — properties', () => { + const FC = { seed: 20260729, numRuns: 200 }; + + /** Slugs inside the declared grammar — the only ones a marker can carry. */ + const slugArb = fc.stringMatching(/^[a-z][a-z0-9_-]{0,12}$/); + + /** + * Slugs OUTSIDE the grammar. These are unmatchable by LEG_MARKER_RE, so the + * contract is that they are reported as INVALID_SLUG rather than silently + * reported missing. Includes regex metacharacters and prototype-pollution + * shaped keys. + */ + const badSlugArb = fc.constantFrom( + 'a.b', 'a*b', 'a+b', '(a)', '[a]', 'a|b', 'A', '-lead', '_lead', '__proto__', '', + ); + + /** + * Slugs that ARE inside the grammar but collide with Object.prototype keys. + * `constructor` is all-lowercase, so it is a legitimate slug — the risk is + * prototype pollution in the counting maps, not validation. + */ + const prototypeKeyArb = fc.constantFrom('constructor', 'tostring', 'valueof', 'hasownproperty'); + + /** Arbitrary markdown noise that must never be read as a marker or a lane heading. */ + const noiseArb = fc.array( + fc.oneof( + fc.constant(''), + fc.constant('**Timeout guidance (#2194):**'), + fc.constant('```bash'), + fc.constant('```'), + fc.constant('# Cross-AI Plan Review — Phase {N}'), + fc.constant('## Consensus Summary'), + fc.constant('### Agreed Concerns'), + fc.constant(''), + fc.stringMatching(/^[A-Za-z0-9 ,.()#*_-]{0,40}$/), + ), + { maxLength: 12 }, + ); + + /** Build a review.md-shaped document declaring exactly `slugs`. */ + function docFor(slugs, sections, noise, eol) { + const legs = slugs.map((s) => `\n**${s}:**`).join('\n'); + const heads = sections.map((s) => `## ${s} Review`).join('\n\n'); + const body = [ + '', + ...noise, + legs, + '', + '', + ...noise, + heads, + '## Consensus Summary', + '', + ].join('\n'); + return body.split('\n').join(eol); + } + + const laneSetArb = fc + .uniqueArray(slugArb, { minLength: 1, maxLength: 6 }) + .map((slugs) => + slugs.map((slug, i) => ({ + slug, + flags: [`--${slug}`], + transport: 'spawn', + probe: { kind: 'command-exists', binary: slug }, + invoke: { + binary: slug, + args: [], + promptChannel: 'stdin', + outputChannel: 'stdout', + modelArg: null, + effortChannel: 'none', + }, + timeoutFloorMs: 1000, + emptyOutput: 'stub-with-stderr', + // Section names are index-tagged so they stay unique even when two slugs + // differ only by a character the heading grammar would not distinguish. + reviewsSection: `Sec${i}`, + evidenceClass: 'source-grounded', + requiresBinaries: [], + promptBudgetKey: null, + handler: null, + })), + ); + + test('never throws on arbitrary input, and ok always agrees with the violations', () => { + // Totality is a real requirement, not a nicety: Phase 2 (#2795) feeds this + // same function manifest-derived data from third-party overlays. A checker + // that throws on bad input cannot report on it, and a parity gate that + // crashes is indistinguishable from one that was never run. + fc.assert( + fc.property( + fc.anything(), + fc.anything(), + fc.anything(), + (descriptor, roster, workflowText) => { + let r; + try { + r = checkReviewerLaneParity({ descriptor, roster, workflowText }); + } catch { + return false; + } + return ( + Array.isArray(r.violations) && r.ok === (r.violations.length === 0) + ); + }, + ), + FC, + ); + }); + + test('a slug outside the marker grammar is reported, never silently missing', () => { + // The silent-miss this prevents: LEG_MARKER_RE captures only [a-z0-9_-], so + // a slug like `acme.reviewer` can have a present, correct marker that the + // scan can never see — reporting LEG_MARKER_MISSING forever with no clue why. + fc.assert( + fc.property(badSlugArb, (bad) => { + const lane = { ...fakeLane('placeholder'), slug: bad }; + const r = checkReviewerLaneParity({ + descriptor: [lane], + roster: [bad], + workflowText: `\n\n`, + }); + const reasonsOut = r.violations.map((v) => v.reason); + return ( + reasonsOut.includes(PARITY_VIOLATION.INVALID_SLUG) && + !reasonsOut.includes(PARITY_VIOLATION.LEG_MARKER_MISSING) + ); + }), + FC, + ); + }); + + test('a prototype-key slug behaves like any other valid slug', () => { + // The counting layer uses Map/Set, not bare objects, so a slug named + // `constructor` cannot reach Object.prototype. Locking it: a bare-object + // counter would make this lane appear present when it is absent. + fc.assert( + fc.property(prototypeKeyArb, (name) => { + const lane = { ...fakeLane('placeholder'), slug: name, reviewsSection: 'Sec' }; + const declared = checkReviewerLaneParity({ + descriptor: [lane], + roster: [name], + workflowText: + `\n\n\n` + + '\n## Sec Review\n', + }); + const absent = checkReviewerLaneParity({ + descriptor: [lane], + roster: [name], + workflowText: '\n', + }); + return ( + declared.ok && + absent.violations.some( + (v) => v.reason === PARITY_VIOLATION.LEG_MARKER_MISSING && v.subject === name, + ) + ); + }), + FC, + ); + }); + + test('a non-object lane entry is reported as malformed, not thrown on', () => { + fc.assert( + fc.property( + fc.constantFrom(0, 1, '', 'x', null, true, [], NaN), + (junk) => { + const r = checkReviewerLaneParity({ + descriptor: [junk], + roster: [], + workflowText: '', + }); + const reasonsOut = r.violations.map((v) => v.reason); + return ( + reasonsOut.includes(PARITY_VIOLATION.MALFORMED_LANE) || + reasonsOut.includes(PARITY_VIOLATION.INVALID_SLUG) + ); + }, + ), + FC, + ); + }); + + test('a document declaring exactly the descriptor satisfies parity', () => { + fc.assert( + fc.property(laneSetArb, noiseArb, fc.constantFrom('\n', '\r\n'), (lanes, noise, eol) => { + const doc = docFor( + lanes.map((l) => l.slug), + lanes.map((l) => l.reviewsSection), + noise, + eol, + ); + const r = checkReviewerLaneParity({ + descriptor: lanes, + roster: lanes.map((l) => l.slug), + workflowText: doc, + }); + return r.ok; + }), + FC, + ); + }); + + test('removing one lane marker always yields exactly that lane missing', () => { + fc.assert( + fc.property(laneSetArb, noiseArb, fc.nat(), (lanes, noise, pick) => { + const victim = lanes[pick % lanes.length]; + const kept = lanes.filter((l) => l.slug !== victim.slug); + const doc = docFor( + kept.map((l) => l.slug), + lanes.map((l) => l.reviewsSection), + noise, + '\n', + ); + const r = checkReviewerLaneParity({ + descriptor: lanes, + roster: lanes.map((l) => l.slug), + workflowText: doc, + }); + const missing = r.violations.filter( + (v) => v.reason === PARITY_VIOLATION.LEG_MARKER_MISSING, + ); + return missing.length === 1 && missing[0].subject === victim.slug; + }), + FC, + ); + }); + + test('evaluation is deterministic across repeated calls', () => { + // Guards regex lastIndex leaking between invocations of a module-level /g. + fc.assert( + fc.property(laneSetArb, noiseArb, (lanes, noise) => { + const input = { + descriptor: lanes, + roster: lanes.map((l) => l.slug), + workflowText: docFor( + lanes.map((l) => l.slug), + lanes.map((l) => l.reviewsSection), + noise, + '\n', + ), + }; + const a = checkReviewerLaneParity(input); + const b = checkReviewerLaneParity(input); + return JSON.stringify(a) === JSON.stringify(b); + }), + FC, + ); + }); +}); + +describe('reviewer lane descriptor — declared shape (ADR-2782 D1/D2/D6/D7)', () => { + test('every lane declares a closed transport at the lane level', () => { + // ADR-2782 D1 places `transport` as a sibling of `probe` and `invoke`, not + // nested inside `invoke`. Locking the placement keeps Phase 2's manifest + // harvest free of a translation step. + for (const lane of REVIEWER_LANES) { + assert.ok( + ['spawn', 'openai-http'].includes(lane.transport), + `${lane.slug}: unexpected transport ${lane.transport}`, + ); + assert.strictEqual( + lane.invoke.transport, + undefined, + `${lane.slug}: transport must not be duplicated inside invoke`, + ); + } + }); + + test('the transport sub-shape is respected per lane', () => { + // A descriptor carrying fields from both sub-shapes — or neither — has + // undefined meaning, which is what a closed vocabulary exists to prevent. + for (const lane of REVIEWER_LANES) { + const i = lane.invoke; + if (lane.transport === 'spawn') { + assert.ok(i.binary, `${lane.slug}: spawn lane must declare a binary`); + assert.ok(Array.isArray(i.args), `${lane.slug}: spawn lane must declare args`); + assert.strictEqual(i.hostConfigKey, undefined, `${lane.slug}: spawn lane must not declare hostConfigKey`); + } else { + assert.ok(i.hostConfigKey, `${lane.slug}: http lane must declare hostConfigKey`); + assert.ok(i.path, `${lane.slug}: http lane must declare a path`); + assert.strictEqual(i.binary, undefined, `${lane.slug}: http lane must not declare a binary`); + assert.strictEqual(i.effortChannel, 'none', `${lane.slug}: http lanes carry no effort channel`); + } + } + }); + + test('every probe kind is in the closed enum', () => { + for (const lane of REVIEWER_LANES) { + assert.ok( + ['command-exists', 'command-capability', 'http-reachable'].includes(lane.probe.kind), + `${lane.slug}: unexpected probe kind ${lane.probe.kind}`, + ); + } + }); + + test('every probe that opens a connection declares a bound', () => { + // DEFECT.UNBOUNDED-SUBPROCESS: an unbounded probe hangs every future review, + // including reviews that never asked for that lane. + for (const lane of REVIEWER_LANES) { + if (lane.probe.kind === 'command-exists') continue; + assert.ok( + Number.isInteger(lane.probe.timeoutMs) && lane.probe.timeoutMs > 0, + `${lane.slug}: ${lane.probe.kind} probe must declare a positive timeoutMs`, + ); + } + }); + + test('handler is a closed first-party enum', () => { + const allowed = [null, 'antigravity', 'openai-compatible']; + for (const lane of REVIEWER_LANES) { + assert.ok(allowed.includes(lane.handler), `${lane.slug}: unexpected handler ${lane.handler}`); + } + assert.deepStrictEqual( + REVIEWER_LANES.filter((l) => l.handler !== null).map((l) => l.slug).sort(), + ['antigravity', 'llama_cpp', 'lm_studio', 'ollama'], + ); + }); + + test('every lane declares a positive timeout floor', () => { + for (const lane of REVIEWER_LANES) { + assert.ok( + Number.isInteger(lane.timeoutFloorMs) && lane.timeoutFloorMs > 0, + `${lane.slug}: timeoutFloorMs must be a positive integer`, + ); + } + }); + + test('empty-output policy is normalized across lanes', () => { + // Only Antigravity opts out, and it does so by owning its own diagnostics + // through a handler (ADR-2782 D6) — not by discarding stderr. + assert.deepStrictEqual( + REVIEWER_LANES.filter((l) => l.emptyOutput !== 'stub-with-stderr').map((l) => l.slug), + ['antigravity'], + ); + }); + + test('the descriptor table is frozen', () => { + assert.ok(Object.isFrozen(REVIEWER_LANES)); + for (const lane of REVIEWER_LANES) { + assert.ok(Object.isFrozen(lane), `${lane.slug}: lane must be frozen`); + } + }); + + test('flag uniqueness holds across the flattened multi-flag set', () => { + // ADR-2782 D8 states uniqueness over a singular `reviewer.flag`; this module + // widens that field to `flags[]` (Antigravity is --antigravity AND --agy), so + // the invariant is enforced over every lane's flattened flag set. + const all = REVIEWER_LANES.flatMap((l) => l.flags); + assert.deepStrictEqual([...new Set(all)].sort(), [...all].sort()); + assert.ok( + REVIEWER_LANES.some((l) => l.flags.length > 1), + 'expected at least one multi-flag lane, else the widening is untested', + ); + }); + + test('the violation reason enum is locked', () => { + // Adding a reason is three coordinated changes: enum, emitting site, and + // this assertion. + assert.deepStrictEqual(Object.keys(PARITY_VIOLATION).sort(), [ + 'DESCRIPTOR_LANE_NOT_IN_ROSTER', + 'DUPLICATE_FLAG', + 'DUPLICATE_SECTION', + 'DUPLICATE_SLUG', + 'INVALID_SLUG', + 'LEG_MARKER_DUPLICATED', + 'LEG_MARKER_MISSING', + 'LEG_MARKER_UNDECLARED', + 'MALFORMED_LANE', + 'ROSTER_SLUG_UNDECLARED', + 'SECTION_DUPLICATED', + 'SECTION_MISSING', + 'SECTION_UNDECLARED', + ]); + assert.ok(Object.isFrozen(PARITY_VIOLATION)); + }); +}); diff --git a/tests/review-reviewer-selection.test.cjs b/tests/review-reviewer-selection.test.cjs index 6110b2386..11b33cd34 100644 --- a/tests/review-reviewer-selection.test.cjs +++ b/tests/review-reviewer-selection.test.cjs @@ -144,3 +144,166 @@ describe('resolveReviewerSelection', () => { assert.deepStrictEqual(r.errors, []); }); }); + +/** + * ADR-2782 D4 (#2794) — absent-safe governs DISCOVERY, never explicit selection. + * + * "Not finding a lane nobody asked for is normal; failing to run a lane somebody + * asked for is an error." + * + * Before this change every explicit miss was an `info`. A TOTAL miss still + * errored, but only as a side effect of the selected set coming out empty — so + * the PARTIAL miss (`--gemini --qwen` on a host without qwen) had no signal at + * all: the review ran with a thinner reviewer set and present_results reported + * success. The workflow's own guidance names why that is wrong — "a cross-AI + * review that silently drops a lane is blind in one eye". + */ +describe('resolveReviewerSelection — explicit flags are an assertion (ADR-2782 D4)', () => { + const errorsMentioning = (r, slug) => r.errors.filter((e) => e.includes(slug)); + + test('an explicit flag for a detected reviewer selects it with no message', () => { + const r = resolveReviewerSelection({ + detected: ['gemini'], + explicitFlags: ['gemini'], + }); + assert.deepStrictEqual(r.selected, ['gemini']); + assert.deepStrictEqual(r.errors, []); + assert.deepStrictEqual(r.infos, []); + }); + + test('a partial explicit miss errors instead of degrading silently', () => { + // THE regression row. Pre-fix this produced errors: [] and an info note, + // and the run proceeded one-eyed. + const r = resolveReviewerSelection({ + detected: ['gemini'], + explicitFlags: ['gemini', 'qwen'], + }); + assert.deepStrictEqual(r.selected, ['gemini'], 'the detected lane is still selected'); + assert.strictEqual( + errorsMentioning(r, 'qwen').length, + 1, + `expected exactly one error naming qwen, got: ${JSON.stringify(r.errors)}`, + ); + assert.deepStrictEqual(r.infos, [], 'the miss must not be downgraded to an info'); + }); + + test('a sole explicit flag that is undetected errors per-slug and in aggregate', () => { + const r = resolveReviewerSelection({ + detected: ['gemini'], + explicitFlags: ['qwen'], + }); + assert.deepStrictEqual(r.selected, []); + assert.strictEqual(errorsMentioning(r, 'qwen').length, 1); + // The pre-existing aggregate message is preserved, not replaced — the + // per-slug errors must not suppress it. + assert.ok( + r.errors.some((e) => e.includes('no selected reviewers are available')), + `expected the aggregate error to survive, got: ${JSON.stringify(r.errors)}`, + ); + }); + + test('every missing explicit flag produces its own error, in a stable order', () => { + const forward = resolveReviewerSelection({ + detected: ['gemini'], + explicitFlags: ['gemini', 'qwen', 'codex'], + }); + const reversed = resolveReviewerSelection({ + detected: ['gemini'], + explicitFlags: ['gemini', 'codex', 'qwen'], + }); + assert.strictEqual(errorsMentioning(forward, 'qwen').length, 1); + assert.strictEqual(errorsMentioning(forward, 'codex').length, 1); + // Order must not depend on the order flags appeared on the command line. + assert.deepStrictEqual(forward.errors, reversed.errors); + }); + + test('a duplicate explicit flag produces exactly one error', () => { + const r = resolveReviewerSelection({ + detected: ['gemini'], + explicitFlags: ['qwen', 'qwen'], + }); + assert.strictEqual(errorsMentioning(r, 'qwen').length, 1); + }); + + test('explicit flag matching is case-insensitive', () => { + const r = resolveReviewerSelection({ + detected: ['gemini'], + explicitFlags: ['QWEN'], + }); + assert.strictEqual(errorsMentioning(r, 'qwen').length, 1); + }); + + test('an explicit flag with nothing detected at all errors', () => { + const r = resolveReviewerSelection({ + detected: [], + explicitFlags: ['gemini'], + }); + assert.deepStrictEqual(r.selected, []); + assert.strictEqual(errorsMentioning(r, 'gemini').length, 1); + }); + + test('pre-existing config errors do not suppress per-slug explicit errors', () => { + const r = resolveReviewerSelection({ + detected: ['gemini'], + explicitFlags: ['qwen'], + configuredDefaultReviewers: 'not-an-array', + }); + assert.strictEqual(errorsMentioning(r, 'qwen').length, 1); + assert.ok(r.errors.some((e) => e.includes('must be a JSON array'))); + // Guarded on the PRE-branch error count, so the aggregate fires exactly when + // it did before this change — i.e. not here, because a config error already + // existed. + assert.ok(!r.errors.some((e) => e.includes('no selected reviewers are available'))); + }); + + test('non-string explicit flags are coerced, never thrown on', () => { + const r = resolveReviewerSelection({ + detected: ['gemini'], + explicitFlags: [null, 0, { a: 1 }], + }); + assert.strictEqual(r.source, 'explicit_flags'); + assert.strictEqual(r.errors.length, 3 + 1, 'three unknown flags plus the aggregate'); + }); +}); + +/** + * The other half of D4, and the reason the carve-out is scoped to explicit + * flags only: discovery paths stay lenient. `--all` is a quantifier over what + * exists; `review.default_reviewers` is a preference evaluated across many + * hosts. Neither is an assertion about a specific lane, so neither errors. + */ +describe('resolveReviewerSelection — discovery paths stay lenient (ADR-2782 D4)', () => { + test('--all does not error on undetected lanes', () => { + const r = resolveReviewerSelection({ + detected: ['gemini'], + allFlag: true, + }); + assert.equal(r.source, 'all_flag'); + assert.deepStrictEqual(r.selected, ['gemini']); + assert.deepStrictEqual(r.errors, []); + assert.deepStrictEqual(r.infos, []); + }); + + test('a configured default that is undetected stays an info, not an error', () => { + const r = resolveReviewerSelection({ + detected: ['gemini'], + configuredDefaultReviewers: ['gemini', 'codex'], + }); + assert.equal(r.source, 'config_default'); + assert.deepStrictEqual(r.selected, ['gemini']); + assert.deepStrictEqual(r.errors, [], 'a preference miss must not become an error'); + assert.ok( + r.infos.some((i) => i.includes('codex')), + `expected an info naming codex, got: ${JSON.stringify(r.infos)}`, + ); + }); + + test('an unknown configured slug stays a warning', () => { + const r = resolveReviewerSelection({ + detected: ['gemini'], + configuredDefaultReviewers: ['gemini', '__nope__'], + }); + assert.deepStrictEqual(r.errors, []); + assert.ok(r.warnings.some((w) => w.includes('__nope__'))); + }); +});