* 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 <!-- reviewer-lane: slug --> 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * chore(#2794): backfill changeset pr number to 2820 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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).
|
||||
|
||||
@@ -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:
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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 <FILE>` 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.
|
||||
|
||||
Reference in New Issue
Block a user