From 8b44a0da43fbb2e80e41411f6c56b5cd441d22f1 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 29 Jul 2026 07:31:32 -0400 Subject: [PATCH] chore(#2794): single-source the reviewer invocation contract + parity assertion (#2820) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(#2794): single-source the reviewer invocation contract Phase 1 of epic #2782 (ADR-2782). Introduces one core descriptor table as the declared contract for all 11 cross-AI reviewer lanes, and the DEFECT.GENERATIVE-FIX parity assertion the roster has never had. The lane contract lived in three unrelated surfaces — the roster, ~640 lines of hand-authored per-CLI bash in invoke_reviewers, and the write_reviews section headings — so cross-cutting fixes landed per-leg (#2494 and #2605 were the same empty-output defect filed twice). - src/review-lane-descriptor.cts: frozen table declaring per lane the slug, flags, probe, invoke shape, timeout floor, empty-output policy, REVIEWS.md section, evidence class, required binaries, prompt-budget key and handler. Field names track ADR-2782 D1/D2/D6/D7 verbatim so Phase 2 harvests the shape with no translation layer. It declares; it does not execute — invoke_reviewers iterates in Phase 5b. - checkReviewerLaneParity: bidirectional parity across descriptor, roster, invoke_reviewers legs and write_reviews sections. Forward-only would miss the failure it exists to catch (#2718 added a leg, #2781 was the drift). ADR-1517 instance headings are exempt per D8. - Legs carry an explicit marker; five non-lane bold labels share the bold-then-fence shape a heuristic matcher would key on. - ADR-2782 D4: an explicitly-flagged reviewer that cannot run is now an error in both the core module and the workflow prose that mirrors it. A code-only change would be unobservable — the module has no production caller; the workflow narrates the policy. Discovery paths (--all, review.default_reviewers) stay lenient. - Fixes the qwen leg, the last one discarding stderr to /dev/null. Two ADR-2782 D2 vocabulary widenings were forced by surveying the shipped legs: promptChannel 'none' (CodeRabbit is fed no prompt) and outputChannel 'file-arg' (Codex writes via -o and discards stdout, #1698). Both are additive and closed; Phase 2 owns the validator. Closes #2690 Co-Authored-By: Claude Opus 5 * fix(#2794): make the parity checker total and pin the lane slug grammar Findings from the orthogonal review passes. Spec axis — the module claimed its vocabulary tracked ADR-2782 D1/D2 "verbatim" while diverging in three undisclosed ways, which is the translation layer Phase 2 was supposed to be spared: - `transport` moves from `invoke.transport` to the LANE level, a sibling of `probe`/`invoke`, exactly as D1's manifest example places it. The nested form read better as a TS discriminated union; the union is now discriminated at the lane level instead, which costs nothing. - The header and the CONTEXT.md glossary now enumerate all FOUR widenings (adding `outputArg` and `flags[]`), not two. Standards axis — CLAUDE.md requires a fast-check property test for a parser, and `checkReviewerLaneParity` parses markdown for markers and headings. Adding one found two real defects that the hand-written matrix missed: - NOT TOTAL: a malformed descriptor entry threw on `lane.flags` iteration, contradicting the module's own "never throws" claim. Every field is now narrowed from `unknown` at the trust boundary and reported as MALFORMED_LANE / INVALID_SLUG. This matters because Phase 2 feeds this function third-party overlay data, and a parity gate that crashes is indistinguishable from one never run. - SILENT GRAMMAR MISMATCH: LEG_MARKER_RE captures only [a-z0-9_-], so a slug outside that class was unmatchable — its marker could be present and correct and the scan would still report LEG_MARKER_MISSING forever. LANE_SLUG_RE now pins the grammar and a violating slug is reported INVALID_SLUG. A loud named violation beats a silent miss. Generators are document-shaped, not writer-seeded (CONTRIBUTING #2371): seeding from the module's own matchers could only produce documents those matchers already recognize. Co-Authored-By: Claude Opus 5 * fix(#2794): register the new bin/lib module in the ESLint ignore list The remote runner caught this; lint:ci did not, because the invariant lives in the test suite rather than the lint chain: tests/repo-invariants.test.cjs "each bin/lib/*.cjs is linted xor ignored according to migration state" -> tsc-generated bin/lib modules not yet added to ESLint ignore list: review-lane-descriptor.cjs Adding a src/*.cts module ripples to six surfaces (.gitignore, the ESLint ignore list, docs/INVENTORY-MANIFEST.json, the CONTEXT.md glossary, the capability/inventory manifests, and any size baseline). The other five were covered; this was the miss. Co-Authored-By: Claude Opus 5 * docs(#2794): amend ADR-2782 D1/D2/D8 with the vocabulary Phase 1 surfaced Building the Phase 1 descriptor table against all eleven shipped legs is the first time every lane's contract was written in one place, and it surfaced four cases the ADR's original survey did not cover. Amending the design lock rather than diverging from it, so Phase 2 (#2795) implements the manifest validator against the amended vocabulary instead of rediscovering the gaps. All four are additive widenings of closed enums; no decision reverses: - D2 promptChannel gains `none` — coderabbit is fed no prompt at all, it reviews the working-tree diff. - D2 outputChannel gains `file-arg` — the ADR called a file-writing lane a shape a real CLI *could* take; codex already is one, writing via -o/--output-last-message and discarding stdout (#1698). - D2 gains `outputArg`, required iff file-arg — knowing the review lands in a file is useless without the argument naming it. - D1 `flag` becomes `flags[]` and D8's uniqueness flattens across lanes — antigravity is selected by both --antigravity and --agy, which a single-valued field cannot express. This is the same evidence path that produced the openai-http transport: the vocabulary widens on a lane that exists, under review, never on speculation. Co-Authored-By: Claude Opus 5 * chore(#2794): backfill changeset pr number to 2820 --------- Co-authored-by: Claude Opus 5 --- .changeset/lively-yaks-frolic.md | 5 + .gitignore | 1 + CONTEXT.md | 3 + docs/COMMANDS.md | 6 +- docs/CONFIGURATION.md | 2 +- docs/INVENTORY-MANIFEST.json | 1 + .../2782-reviewer-lane-capability-surface.md | 74 +- eslint.config.mjs | 1 + gsd-core/workflows/review.md | 43 +- src/review-lane-descriptor.cts | 656 ++++++++++++++++ src/review-reviewer-selection.cts | 23 +- tests/emitted-drift-ack.json | 8 + .../fix-2794-review-qwen-empty-guard.test.cjs | 166 ++++ tests/review-lane-descriptor.test.cjs | 734 ++++++++++++++++++ tests/review-reviewer-selection.test.cjs | 163 ++++ 15 files changed, 1864 insertions(+), 22 deletions(-) create mode 100644 .changeset/lively-yaks-frolic.md create mode 100644 src/review-lane-descriptor.cts create mode 100644 tests/emitted-drift-ack.json create mode 100644 tests/fix-2794-review-qwen-empty-guard.test.cjs create mode 100644 tests/review-lane-descriptor.test.cjs 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__'))); + }); +});