From 7f13ee53736786fb5c23195b274e63b3dc721df7 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 28 Jul 2026 22:26:26 -0400 Subject: [PATCH] =?UTF-8?q?enhance(#2793):=20ADR-2782=20=E2=80=94=20review?= =?UTF-8?q?er=20lane=20becomes=20a=20declared=20capability=20surface=20(#2?= =?UTF-8?q?809)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs(#2793): add ADR-2782 — reviewer lane capability surface Design lock for epic #2782. Declares a reviewer lane as capability data rather than a core patch across three unrelated surfaces. Key decisions: - D2 transport discriminator (spawn | openai-http) — a survey of all twelve lanes found three that are HTTP endpoints with no binary, which invalidated the single-invoke-shape draft. - D4 the reviewer body is optional and absent-safe at every layer. - D5 a fourth executable-surface disclosure class covering the lane binary or host AND its egress payload classes. - D6 handler is a closed first-party enum, upholding ADR-1016; the consequence — third-party lanes are data-only — is stated plainly. - D7 probe kinds wider than existence, and every probe bounded. Amends ADR-857, ADR-894, ADR-1016, ADR-1244. Also records the D7/D8-extended-by-ADR-1244 marker on ADR-857 that ADR-1244 D8 promised but never added. Closes #2793 * docs(#2793): address orthogonal review findings on ADR-2782 Two blockers from the isolated adversarial pass: - D5 disclosed the spawn binary but not its args, reopening the #1459 bug class already fixed for MCP servers (binary python3 + args -c ). args are now disclosed and signature-bound. - hostConfigKey resolves from .planning/config.json, which is mutable after consent with no integrity check, so a lane consented against localhost could be silently redirected to a remote host by an ordinary PR. The resolved host is now consent-bound and re-verified on the invocation path; a mismatch blocks the lane. Majors and spec gaps: - D4 gains an explicit-selection carve-out. Absent-safe governs discovery, never a lane the user named; the current selector records that as info, which Phase 1 now corrects. - D4 gains a warning delivery channel. - D6 enumerates the handler closed-enum members; a closed enum whose membership is left to the implementing phase is not closed. - D6 records aider and plandex as concrete lanes the vocabulary cannot express, rather than claiming sufficiency it did not verify. - D2 gains evidenceClass, requiresBinaries, promptBudgetKey for per-lane divergence that was only prose, and motivates the one-member outputChannel enum. - reviewer.requires renamed requiresBinaries — it collided with the envelope requires (capability deps) at a different nesting depth. - Antigravity two-level timeout: Context cited it then dropped it; now explicitly delegated to the handler. - D9 gains a per-key ownership table, including three keys that stay central because they are policy across lanes, not lane properties. - Phase table maps every decision D1-D9 to a delivering phase; D6 handler modules and D5 invocation-time re-verification were previously unclaimed. - American English per house style. --- .../adr/1016-runtime-capability-descriptor.md | 1 + docs/adr/1244-capability-ecosystem.md | 1 + .../2782-reviewer-lane-capability-surface.md | 558 ++++++++++++++++++ docs/adr/857-capability-system.md | 2 + docs/adr/894-capability-declaration-format.md | 1 + docs/adr/README.md | 5 +- 6 files changed, 566 insertions(+), 2 deletions(-) create mode 100644 docs/adr/2782-reviewer-lane-capability-surface.md diff --git a/docs/adr/1016-runtime-capability-descriptor.md b/docs/adr/1016-runtime-capability-descriptor.md index 0cfb373ef..fb6ea8c8e 100644 --- a/docs/adr/1016-runtime-capability-descriptor.md +++ b/docs/adr/1016-runtime-capability-descriptor.md @@ -8,6 +8,7 @@ - **Materializes:** [ADR-58](58-runtime-install-policy-module.md) (the typed `InstallPlan` projection) - **Builds on:** [ADR-3660](3660-runtime-artifact-layout-module.md) (artifact layout), [ADR-894](894-capability-declaration-format.md) (the `role: runtime` body, already validated) - **Subsumed by:** [ADR-1239](1239-gsd-embeddable-orchestration-engine.md) (GSD as an Embeddable Orchestration Engine) — read it first; see the amendment below +- **Amended by:** [ADR-2782](2782-reviewer-lane-capability-surface.md) (Reviewer Lane capability surface) — a `role: "runtime"` capability may now carry a `reviewer` body **alongside** its runtime body. The runtime body itself remains closed and unchanged, and no feature-only field becomes permissible on it. ADR-2782 D6 **upholds** this ADR's closed-vocabulary principle: the lane's `handler` is a closed enum of first-party names (the `ConverterName` construction of Decision 3), never an open escape hatch, so §Alternatives #2 stands unreversed. ## Amendment (2026-07-16): subsumed by ADR-1239 (EoS) — this ADR is the *declarative adapter*, not the whole architecture diff --git a/docs/adr/1244-capability-ecosystem.md b/docs/adr/1244-capability-ecosystem.md index 01cd57aeb..a58aeaae0 100644 --- a/docs/adr/1244-capability-ecosystem.md +++ b/docs/adr/1244-capability-ecosystem.md @@ -2,6 +2,7 @@ - **Status:** Accepted — ratified 2026-07-17 (originally Proposed 2026-06-14); see "Ratification" below - **Date:** 2026-06-14 +- **Amended by:** [ADR-2782](2782-reviewer-lane-capability-surface.md) (Reviewer Lane capability surface) — **D5 gains a fourth executable-surface disclosure class**, the reviewer lane, which is the first disclosed surface that *receives* data rather than only executing: a lane is piped plan text, requirements, research findings, and `CONTEXT.md` decisions. D9's capability matrix gains a reviewer-lane column. > **Relationship to other ADRs.** This ADR **amends and extends ADR-857 Decisions 7 and 8** — it does not reverse them. ADR-857 D7 deferred third-party code-loading "to its own ADR"; D8 deferred third-party CLI support "to an external loader + trust/validation gate, no rework because runtimes are already descriptors." This *is* that ADR, and it *delivers* that gate. It builds on **ADR-894** (capability declaration format), **ADR-1016** (runtime capability descriptor), and **ADR-58** (InstallPlan seam). Tracked by [#1244](https://github.com/open-gsd/gsd-core/issues/1244). Target release: **1.6.0**. diff --git a/docs/adr/2782-reviewer-lane-capability-surface.md b/docs/adr/2782-reviewer-lane-capability-surface.md new file mode 100644 index 000000000..ea71eae1d --- /dev/null +++ b/docs/adr/2782-reviewer-lane-capability-surface.md @@ -0,0 +1,558 @@ +# ADR-2782: Reviewer Lane — the cross-AI reviewer handoff becomes a declared capability surface + +- **Status:** Accepted +- **Date:** 2026-07-28 +- **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) +- **Subsumes:** [#2690](https://github.com/open-gsd/gsd-core/issues/2690) (core single-sourcing — lands as Phase 1 under this ADR rather than as its own design) + +## Context + +A cross-AI reviewer lane — one external CLI or model endpoint that `/gsd:review` hands a plan to +for independent review — is declared today in **three** unrelated places, none of which is the +capability system, and **none** of which a third party can extend. + +**1. The roster is half registry-derived, half hardcoded.** `src/review-reviewer-selection.cts` +derives slugs from `runtime.hostBehaviors.reviewerCli === true` (`:40-49`), then concatenates a +hardcoded `NON_RUNTIME_REVIEWER_SLUGS` tail (`:32-38`) for five reviewers that have no +`capabilities//` directory at all. The module's own comment says exactly this. Six capabilities +carry the flag; five reviewers have no descriptor of any kind. + +**2. The invocation contract is prose.** `gsd-core/workflows/review.md` is 1070 lines; +`invoke_reviewers` spans roughly 60% of it as hand-authored per-CLI bash. Each leg re-implements +probe, argv shape, model lookup, effort channel, timeout, stderr capture, and empty-output policy. + +**3. The output contract is prose.** `write_reviews` hardcodes per-reviewer section headings, +including two literal instance names. + +Three structural consequences follow, and they are why this is a capability question rather than +only a refactor question. + +**(a) `reviewerCli` is a bare boolean in an undocumented, unvalidated bag.** `hostBehaviors` +appears **zero times** in `docs/reference/capability-manifest.md` — not in the envelope table, not +in the runtime-body axis table — and `scripts/gen-capability-registry.cjs` does not validate its +keys. The one field that decides reviewer membership is unspecified, unvalidated, and carries no +invocation data. A capability author can discover it only by reading +`src/review-reviewer-selection.cts:47`. + +**(b) Reviewer-ness is welded to runtime-ness, and the runtime body structurally cannot hold a lane +contract.** `capability-manifest.md:141` states the runtime body is "a closed 8-axis (plus 4 +install-surface) vocabulary; no feature-only fields (`skills`, `agents`, `steps`, `contributions`, +`gates`, `hooks`) are permitted," and `gen-capability-registry.cjs:505` enforces the consequence — a +`role: "runtime"` capability is stored whole into `runtimes[]` and its `config`/`steps`/ +`contributions`/`gates` are never harvested. A reviewer lane therefore cannot own its own federated +config keys. That is why `review.models.*`, `review.ollama_host`, `review.lm_studio_host`, +`review.llama_cpp_host`, and `review.max_prompt_tokens_per_reviewer.*` all live in the central +schema instead of with the lane that uses them — the exact half-migrated shape the config-key +exclusivity invariant exists to prevent. + +**(c) A reviewer that is not a GSD install target has nowhere to live.** `gemini`, `coderabbit`, +`ollama`, `lm_studio`, and `llama_cpp` are review or model CLIs GSD never installs into. There is no +`capabilities//` for them, so they are a hardcoded tail by necessity, not by choice. + +**Net: adding a reviewer lane is a core patch.** It means editing the roster module or a runtime +descriptor, hand-authoring a bash leg, hand-adding a `write_reviews` heading, adding central config +keys, and updating five prose surfaces. #2718 was that patch in flight (PR #2776, closed in favor +of this design); #2781 is the documentation drift it produced. Cross-cutting fixes land per-leg: +#2494 and #2605 were the same empty-output defect filed twice; #2475 (effort channel), #2589 (model +lookup), #2295 (resolved-model recording), and #2272 (flag parity) are the same shape. + +### What a survey of the twelve lanes actually shows + +The design was drafted assuming one lane shape. Reading all twelve legs disproved that, and the +correction is the most consequential decision in this ADR (D2). + +| Family | Lanes | Shape | +|---|---|---| +| **Spawned CLI** | `gemini`, `claude`, `codex`, `coderabbit`, `opencode`, `qwen`, `cursor`, `antigravity`, `kimi-code` | Binary + argv; prompt via stdin or argv; stdout captured, stderr to a `.err` sidecar | +| **OpenAI-compatible HTTP** | `ollama`, `lm_studio`, `llama_cpp` | **No binary.** `curl` to `/v1/chat/completions` on a user-configured host; model discovered via `GET /v1/models` piped through `jq` | + +Three of twelve lanes are not spawned binaries at all. Timeout floors genuinely diverge — a measured +~570 s for Codex at `xhigh` effort and ~525 s for headless Claude drive a 900 000 ms floor with +1 200 000 ms for those two, while the Antigravity leg runs a 600 s external cap over a 540 s native +`--print-timeout`, and the HTTP lanes use 120 s. Five lanes require `jq` on `PATH`. The Antigravity +leg carries a deliberate three-layer fallback for an upstream stdout bug. + +**Divergence between lanes is real and frequently correct.** The value of a descriptor is therefore +*one place where divergence is declared*, not one behavior imposed on every lane. + +## Decisions + +### D1 — A `reviewer` body on the capability manifest, admissible on two roles + +A reviewer lane is declared as data in a `reviewer` body: + +```json +{ + "id": "acme-reviewer", + "role": "reviewer", + "version": "1.0.0", + "title": "Acme Review CLI", + "description": "Cross-AI plan review lane backed by the Acme CLI.", + "tier": "full", + "requires": [], + "engines": { "gsd": ">=1.9.0" }, + + "reviewer": { + "slug": "acme", + "flag": "--acme", + "transport": "spawn", + "probe": { "kind": "command-exists", "binary": "acme" }, + "invoke": { + "binary": "acme", + "args": ["review", "--format", "text"], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": "--model", + "effortChannel": "argv" + }, + "timeoutFloorMs": 900000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Acme Review", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null + }, + + "config": { + "review.models.acme": { + "type": "string", + "default": "", + "description": "Model passed to the Acme reviewer lane." + } + } +} +``` + +The body is admissible on **`role: "runtime"`** — so the six capabilities that are both install +targets and reviewers (`claude`, `codex`, `cursor`, `opencode`, `qwen`, `antigravity`) keep exactly +one manifest — and on a **new `role: "reviewer"`** (D3) for lanes that are not install targets. + +This is the amendment to ADR-1016: a `role: "runtime"` capability may now carry a `reviewer` body +alongside its runtime body. The runtime body itself remains closed and unchanged; no feature-only +field becomes permissible on it. A lane body is a third thing, not a relaxation of the second. + +Because a lane may own a federated `config` slice, `gen-capability-registry.cjs` must harvest +`config` from a lane-bearing capability of **either** role — the specific limitation at `:505` that +context (b) describes. + +### D2 — `transport` is a closed discriminator, and it selects the invoke sub-shape + +`reviewer.transport` is a closed enum: **`spawn` | `openai-http`**. + +| | `spawn` | `openai-http` | +|---|---|---| +| `invoke.binary` | required | **forbidden** | +| `invoke.args` | required (array) | forbidden | +| `invoke.promptChannel` | `stdin` \| `argv` \| `argv-file-ref` | forbidden | +| `invoke.outputChannel` | `stdout` | 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` | +| `invoke.modelArg` | optional | forbidden (model travels in the JSON body) | +| `invoke.effortChannel` | closed enum: `none` \| `argv` \| `env` | `none` | + +A manifest declaring fields from both sub-shapes, or neither, **fails validation**. The +discriminator is explicit rather than inferred from field presence: inference leaves a manifest with +both — or with neither — carrying undefined meaning, which is precisely what a closed vocabulary +exists to prevent. + +`promptChannel: "argv-file-ref"` exists because two lanes (`cursor`, `kimi-code`) take the prompt as +an argv argument, and passing a full plan set inline would approach the 32 767-character Windows +`execFileSync` ceiling. The file-reference form passes a short instruction naming a prompt file in +the run directory. That instruction must also carry the **absolute repository root**, because an +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. + +Three further declared fields carry per-lane divergence that would otherwise live only in prose: + +| Field | Values | Why it exists | +|---|---|---| +| `evidenceClass` | `source-grounded` \| `diff-only` | CodeRabbit reviews a diff, not the source tree, and its findings are deliberately down-weighted in synthesis (`review.md:367`). Today that caveat is a prose annotation a reader may miss; declaring it lets `write_reviews` render the caveat from data | +| `requiresBinaries` | string[] | External tools the lane needs on `PATH` — `jq` for five lanes. A missing prerequisite reports the lane unavailable with an install hint rather than running it into an empty review | +| `promptBudgetKey` | dotted config key \| `null` | Per-lane prompt trimming (`prepare_trimmed_prompt_for_reviewer`, `review.md:646-704`) is keyed per slug today; the key becomes the lane's own federated config (D9) | + +**Naming note:** the field is `requiresBinaries`, **not** `requires`. The envelope already carries a +`requires` (capability-id dependencies, ADR-1244). Two fields named `requires` at different nesting +depths with unrelated semantics is a defect waiting to happen; the collision was caught in review of +this ADR and renamed here rather than left for a downstream phase to trip over. + +### D3 — A third role, `role: "reviewer"`, for lanes that are not install targets + +`gemini`, `coderabbit`, `ollama`, `lm_studio`, and `llama_cpp` become first-party capabilities with +a `reviewer` body, **no runtime body, and no install surface** — which is the honest description of +what they are. `runtimeCompat` is not required for this role (it declares which host runtimes a +*feature* surfaces through; a lane surfaces through none). + +`tier` remains required, because it is the source of truth for install-profile membership. A +`role: "reviewer"` capability therefore receives profile membership from `deriveProfileMembership` +(`gen-capability-registry.cjs:201-213`) like any other. **That membership is inert**: the capability +contributes no artifacts, so there is nothing to install. This is stated explicitly because a reader +encountering a lane in an install profile would otherwise reasonably assume it installs something. + +Rejected: one role for every lane, splitting `codex` into `codex` + `codex-reviewer`. It is the +cleaner discriminator and was rejected for churn — six manifests would each fragment into two +capabilities and two ids, complicating roster derivation for no gain. + +### D4 — The `reviewer` body is optional and absent-safe at every layer + +**This is a normative MUST, and it governs every downstream phase.** + +1. A capability with **no** `reviewer` body is simply not a lane. This is **never** a validation + error. Most runtime capabilities are install targets only; a validator that errors on an absent + body would break the majority of the registry. +2. An overlay declaring a `role` or a field this GSD version does not know is **skipped with a + warning** via the existing `engines.gsd` hard gate (ADR-1244 D6) — never a crash. This is the + forward half: a capability built for a newer GSD degrades to discovered-but-inactive. +3. An unknown field *inside* a `reviewer` body is ignored with a warning rather than failing + validation. +4. A lane naming an unknown `handler` **fails closed** — the lane is unavailable; the registry does + not crash. +5. A capability with no `reviewer` body must not perturb its **disclosure signature** (D5). An + absent body that changed the signature would force spurious re-consent across every installed + capability. + +The asymmetry is deliberate and is Postel's Law applied with a boundary: liberal in what a manifest +may **omit**, strict in what it **asserts**. Permissiveness about absence is forward compatibility; +permissiveness about assertions would be an untyped escape hatch. + +#### Absent-safe governs discovery, never explicit selection + +Rules 1–5 describe what happens when the system is *looking for* lanes. They do **not** apply once a +user has named one. If a user runs `/gsd:review --acme` and the `acme` lane is unavailable — because +its capability was skipped under rule 2, because its `handler` failed closed under rule 4, because a +prerequisite binary is missing, or because its egress destination changed (D5) — that is an +**error**, surfaced and non-silent. It is not an informational note, and the run does not quietly +proceed with a thinner reviewer set. + +This is called out because the current implementation does the opposite: an unavailable +explicitly-requested reviewer is recorded as an `info` (`review-reviewer-selection.cts:246-248`). +The workflow's own guidance already names why that is wrong — "a cross-AI review that silently drops +a lane is blind in one eye" (`review.md:304`) — and a design whose whole premise is *more* lanes from +*less* trusted sources must not inherit a silently-degrading selector. **Correcting this is Phase 1's +responsibility**, because Phase 1 is where the selector is single-sourced. + +In one line: *not finding* a lane nobody asked for is normal; *failing to run* a lane somebody asked +for is an error. + +#### Where warnings surface + +"Skipped with a warning" means nothing unless a human sees it. Warnings arising at **build time** +(registry generation over first-party capabilities) are emitted by `gen-capability-registry.cjs` on +stderr, and fail the build only where D8's uniqueness invariants are breached. Warnings arising at +**load time** — an overlay skipped by `engines.gsd`, an unknown field, a `handler` that failed +closed — surface on the `/gsd:review` run that would have used the lane, and in +`gsd capability list`, which is where a user goes to ask why a capability is inactive. A warning +written only to a build log nobody reads is not a warning. + +### D5 — A fourth executable-surface disclosure class: the reviewer lane + +ADR-1244 D5 rule 2 requires that executable surfaces be disclosed and consented at install, and +names three classes: `hooks`, command modules, and `mcpServers`. A reviewer lane is a fourth, and it +is materially different from the other three: **it receives data**. A lane is piped the plan text, +the requirements, the research findings, and the `CONTEXT.md` decisions, and its output is read back +into `REVIEWS.md`. That is an egress channel for the most sensitive artifacts GSD produces. + +Making lanes pluggable **without** a disclosure class would open a data-exfiltration path behind a +manifest field. The trust work is therefore the gating requirement of this design, not polish. + +`discloseExecutableSurfaces` gains a reviewer-lane surface that discloses, by transport: + +- **`spawn`** — the **binary and its full declared `args`**, in both rendered and raw form, exactly + as MCP servers already disclose `argv`/`rawArgs` (`capability-trust.cts:688-690`). +- **`openai-http`** — the **destination host URL** resolved from `hostConfigKey`, plus the + `hostConfigKey` itself. Disclosing `curl` would be technically true and practically meaningless; + the destination is the disclosure that matters. A `localhost` destination is still disclosed, and + is distinguished from a remote one. + +Both forms additionally disclose the **egress payload classes** — plan text, requirements, research +findings, `CONTEXT.md` decisions — rather than an unhelpful "sends data to the tool". + +**Disclosing the binary without its `args` is insufficient, and this is not hypothetical.** A lane +declaring `binary: "python3"` with innocuous `args` could, in a later version, change `args` to +`["-c", ""]` without the binary changing at all. That is precisely the bug class +#1459 already fixed for MCP servers, and a binary-only disclosure would reopen it. `args` is +therefore disclosed **and** signature-bound. + +The lane folds into `disclosureSignature` / `signatureForManifest` as stable sorted JSON, exactly as +`env`/`cwd` do for MCP servers (#1459). `executableSetChanged` treats **any** of the following as an +executable-set change for the auto-update re-consent trigger (ADR-1244 D5 rule 4): adding or +removing a lane, or changing its `binary`, `args`, `hostConfigKey`, `promptChannel`, or `handler`. + +#### The egress destination is re-verified at invocation, not only at install + +`hostConfigKey` is the one consent-bound value that does **not** live in the SHA-pinned bundle. It +names a key in `.planning/config.json`, which is user- and CI-editable at any time with no +re-install and no integrity check — unlike every existing consent-bound field (`command`, `args`, +`env`, `cwd`, `url`), all of which come from the manifest itself (`capability-trust.cts:74-125`). + +Left unaddressed, this is a real hole: a lane consented against `http://localhost:8080` could be +silently redirected to a remote host by a later config edit — including one arriving through an +ordinary pull request touching `.planning/config.json` — and every subsequent review run would +egress plans, requirements, research, and decisions to the new destination with no re-prompt. + +Therefore, normatively: + +1. The consent record binds the **resolved host**, not merely the config key. +2. Before invoking an `openai-http` lane the runtime **re-resolves** `hostConfigKey` and compares the + result against the consented host. +3. On mismatch the lane is **blocked, not silently redirected**; the user is told the destination + changed and must re-consent. A blocked lane reports like any other unavailable lane — it never + degrades to running against the new host. +4. This check lives on the **invocation** path (Phase 5b), not only in the install path. + +A host change is a change of *who receives the user's plans*. It is the most security-relevant +mutation in this design, and it must not be reachable by editing a JSON file. + +**Stated honestly, and consistent with ADR-1244 D5's own acknowledgment that there is no sandbox:** +even with the above, consent-at-install remains a weaker gate for a *standing egress channel* than +for a hook. A user consents once; the lane thereafter receives every plan on every review run. +Disclosure plus destination re-verification makes the channel **visible, pinned, and revocable** — it +does not make it safe. A per-run egress prompt was considered and rejected as consent fatigue that +trains users to approve blindly. + +### D6 — `handler` is a closed enum of first-party names; third-party lanes are data-only + +Lane divergence is real (context above), so the descriptor must not promise uniformity. Where a lane +needs genuinely imperative behavior — the Antigravity three-layer fallback is the canary — +`reviewer.handler` names an imperative module **by closed first-party name**, rather than growing +conditionals inside data. + +The enum ships with exactly these members. `null` is the default and covers eight of the twelve +lanes: + +| `handler` | Lanes | What it owns that data cannot express | +|---|---|---| +| `null` | the other eight | Nothing — the declared vocabulary suffices | +| `"antigravity"` | `antigravity` | The three-layer fallback for an upstream stdout bug; the **two-level timeout** (a 600 s external `timeout`/`gtimeout` cap wrapping a 540 s native `--print-timeout`, `review.md:560`); and the stale-response watermark guard that rejects a cached conversation from a prior run | +| `"openai-compatible"` | `ollama`, `lm_studio`, `llama_cpp` | Model discovery against `/v1/models`, the JSON request/response shape, and the **served-model mismatch warning** raised when the responding model differs from the one requested (`review.md:794-797`) | + +Enumerating the members here is deliberate. A "closed enum" whose membership is left to the +implementing phase is not closed, and three separate phases would each have invented a different +list. + +**On `timeoutFloorMs` and the Antigravity two-level timeout.** The descriptor carries **one** scalar, +`timeoutFloorMs` — the outer wall-clock bound every lane gets. A lane whose tool has its own +*internal* timeout (Antigravity's `--print-timeout` is the only current case) expresses that inner +bound in its **handler**, not in the descriptor. Adding a second declarative timeout field to serve +one first-party lane would be speculative generality; the handler seam exists for exactly this. The +delegation is stated here so a reader does not wonder where the measured 540 s went. + +This upholds rather than relaxes ADR-1016. That ADR's core principle is that "a runtime that needs a +shape no existing primitive expresses is supported by adding a first-party primitive … never by +embedding arbitrary code or an open escape hatch in the descriptor," and its §Alternatives #2 +explicitly **rejected** an open escape hatch. `handler` is the same construction as ADR-1016 +Decision 3's closed `ConverterName`: the descriptor references a first-party function by name and +never embeds it. + +**The consequence must be stated plainly, because it caps this epic's headline claim.** Third-party +lanes are **data-only**. A third-party CLI needing a shape the closed vocabulary lacks is blocked on +a first-party PR. The honest claim is *most* lanes, declaratively — not *any* lane. + +The escalation path is the ADR-1016 model, and it is documented rather than implied: file an issue +naming the primitive the vocabulary lacks; it is reviewed and added first-party. **D2 is the worked +example of that path already functioning** — the `openai-http` transport exists precisely because a +survey produced evidence that three real lanes did not fit, and the vocabulary widened on evidence +rather than on speculation. + +**Two real CLIs that would NOT fit today**, named so the boundary is a known quantity rather than a +surprise for the first third-party author who hits it: + +- **Aider** mutates the repository by default — it edits files and commits. The vocabulary has no way + to declare "this tool must be invoked read-only", and the existing lanes achieve that only by + *asking politely inside the prompt text* (`review.md:448`: "Do not edit any files"), which a + coding-agent CLI is under no obligation to honor. A repo-mutating reviewer is a materially + different safety posture from a read-only one, and the descriptor does not currently express it. +- **Plandex** requires a stateful session (`plandex new`) before a review turn. D2's single + `binary` + `args` + prompt-channel shape describes one invocation; it cannot express a two-phase + setup-then-invoke sequence. + +Neither is a reason to reject this design — both are exactly the "file an issue naming the primitive" +case, and both would likely be served by two future primitives: a declared invocation-safety posture, +and a `setup` phase on the descriptor. They are recorded because an ADR claiming a closed vocabulary +is sufficient, without naming what it excludes, is claiming more than it verified. + +Revisiting this to permit a third-party `handler` module confined to the capability install root +(the ADR-1244 D7 model, which does allow third-party command modules) would genuinely deliver "any +plugin can ship a lane." It is rejected **here** because it reverses an ADR-1016 rejection rather +than amending it, and because D7 itself calls third-party code execution the highest-risk surface +and sequences it last. It should be revisited only with its own ADR and its own evidence. + +### D7 — `probe.kind` is a closed enum wider than existence, and every probe is bounded + +`probe.kind` is a closed enum: + +| Kind | Fields | Semantics | +|---|---|---| +| `command-exists` | `binary` | `command -v ` | +| `command-capability` | `binary`, `needle`, `timeoutMs` | ` --help` bounded, matched against `needle` | +| `http-reachable` | `hostConfigKey`, `path`, `timeoutMs` | Bounded GET; reachable ⇒ available | + +`command-exists` alone is **structurally insufficient**, and the evidence is concrete: `kimi` is +claimed by both Kimi Code CLI (Node) and the legacy Python kimi-cli — which is a separate, +first-party, non-reviewer runtime capability in this repo. An existence-only probe registers the +wrong tool. This was found in review of PR #2776 and is the reason the vocabulary ships wider than +one member. + +**Every probe that starts a process or a connection MUST be bounded.** This repo carries a named +*Unbounded Subprocesses* defect class, and the original Kimi probe was a live instance of it: an +unbounded `kimi --help | grep` that ran on **every** `/gsd:review` invocation regardless of which +flags were passed, so a user whose Kimi binary waited on a first-run consent or auth prompt would +hang every future review — including reviews that never asked for that lane. + +`command-capability` bounds via external `timeout`, falling back to `gtimeout` (the precedent +already set by the Antigravity block at `review.md:560`). **Stock macOS ships neither**; where no +bounding mechanism is available the probe is **skipped and the lane reported unavailable**, which +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` +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. + +An overlay lane colliding with a first-party lane is rejected, first-party winning — the existing +`id`-uniqueness precedent (`capability-manifest.md:167`). + +**Reviewer instances are not lanes.** `review.reviewer_instances. = {cli, model?, agent?}` +(ADR-1517) lets one model-capable adapter run as several reviewer identities. Instances resolve +*through* a lane and continue to; they do not participate in the roster, the flag set, or this +uniqueness check. + +### D9 — Reviewer config keys become federated, and the roster derives from declared lanes + +`review.models.*`, `review._host`, and `review.max_prompt_tokens_per_reviewer.*` move from the +central schema to federated `config` slices owned by their lane capabilities. Key **names** and +existing `.planning/config.json` files are unchanged; only validation provenance moves, so no user +migration is required. Per the config-key exclusivity invariant (`capability-manifest.md:173`), the +central-schema removal and the federated addition **must land in the same commit** or the build gate +fails on a key present in both. + +Ownership is per-key and per-lane, so that no key is owned twice — Phase 4 implements this table +rather than re-deriving it: + +| Key | Owner | Notes | +|---|---|---| +| `review.models.` | the lane whose `slug` it names | One key per lane; a lane with no model override declares none | +| `review.ollama_host` | `ollama` | The `hostConfigKey` its own descriptor points at (D2) | +| `review.lm_studio_host` | `lm_studio` | ditto | +| `review.llama_cpp_host` | `llama_cpp` | ditto | +| `review.max_prompt_tokens_per_reviewer.` | the lane whose `slug` it names | The lane's `promptBudgetKey` (D2) resolves to this | +| `review.max_prompt_tokens` | **stays central** | A global default across all lanes; owned by no single lane, so federating it would be wrong | +| `review.default_reviewers` | **stays central** | Selection policy over lanes (ADR-0011), not a property of any lane | +| `review.reviewer_instances` | **stays central** | Instance→lane mapping (ADR-1517); an instance is not a lane (D8) | + +The last three rows matter as much as the first five: a key that describes *policy across* lanes must +not be federated *into* one, and the exclusivity invariant would not catch that error — it only +catches a key owned twice, not a key owned by the wrong side. + +`KNOWN_REVIEWER_SLUGS` derives from declared reviewer bodies. `hostBehaviors.reviewerCli` survives +as a **derived legacy alias for one release** and is then removed. Where both a body and the alias +are present, the body wins. The field is undocumented, so external users are unlikely — but +"undocumented" is not "unused", which is why it gets a deprecation window and a changeset note +rather than a silent removal. **The removal is owned by a named phase** (#2801), not left implicit. + +## Consequences + +**Positive.** + +- Adding a reviewer becomes one manifest installed through `gsd capability install ` — no core + patch, no workflow edit, no release cycle — for any lane the vocabulary expresses. +- A cross-cutting fix (empty output, effort channel, model lookup) becomes a single-site change + covering every lane, retiring the #2494 → #2605 cadence. +- The roster gets one generated source, which makes the `DEFECT.GENERATIVE-FIX` parity assertion for + #2781 mechanical rather than per-lane. +- Third-party lanes arrive behind the existing trust gate — disclosure, consent, SHA pin, + `engines.gsd`, reserved namespaces — instead of as an unreviewable prose block. +- A lane owns its own configuration, closing a half-migrated config surface. + +**Negative, and accepted.** + +- The closed vocabulary must grow, under review, when a genuinely new lane shape appears. This is + intentional friction and it is the trust boundary. D2 shows the cost is real: the first survey + already forced one widening. +- Third-party lanes are data-only (D6). "Any plugin can ship a reviewer lane" overstates what this + delivers; the ADR and the epic should both say *most*. +- Consent-at-install is a weaker gate for a standing egress channel than for a hook (D5). There is + no sandbox. +- `discloseExecutableSurfaces` is already cyclomatic 51 / cognitive 99 with five dependents. Adding + a fourth class lands in an existing hotspot; the implementing phase should extract per-class + helpers rather than grow the switch, and should expect the mutation gate to bite. +- Two declaration mechanisms coexist for one release (D9). +- Normalizing empty-output handling is observable on lanes that previously returned nothing + silently. That is a bug fix that breaks a workaround, and it needs a changeset note rather than a + silent correction. + +**Explicitly unchanged:** reviewer selection precedence (ADR-0011), the `REVIEWS.md` contract +(ADR-1517), and every existing lane's observable command shape. + +## Implementation phases (dependency-ordered) + +Verified with `/adr-phase-coverage`: every deliverable is claimed by exactly one phase, every +hand-off lands, and every user-facing capability has a phase that wires its entry point. + +Every decision is mapped to the phase that delivers it, so no decision is left to be "handled +somewhere". + +| Phase | Issue | Delivers | Deliverable | +|---|---|---|---| +| 0 | [#2793](https://github.com/open-gsd/gsd-core/issues/2793) | — | This ADR | +| 1 | [#2794](https://github.com/open-gsd/gsd-core/issues/2794) | D4 (explicit-selection carve-out only) | Core single-sourced invocation descriptor + `DEFECT.GENERATIVE-FIX` parity assertion; corrects the silently-degrading selector — **closes #2690** | +| 2 | [#2795](https://github.com/open-gsd/gsd-core/issues/2795) | **D1, D2, D3, D4, D7, D8** | Manifest `reviewer` body and the third role; `transport` and `probe.kind` closed enums; registry harvest, validation, uniqueness; the absent-safe invariant and its warning channel | +| 3 | [#2796](https://github.com/open-gsd/gsd-core/issues/2796) | **D5** | The fourth trust-disclosure class: binary + `args` / host + `hostConfigKey`, egress payload classes, signature binding | +| 4 | [#2797](https://github.com/open-gsd/gsd-core/issues/2797) | **D9** (config half) | Federated config migration per the ownership table, same-commit | +| 5a | [#2798](https://github.com/open-gsd/gsd-core/issues/2798) | **D9** (roster half) | The **11 existing** lanes declare reviewer bodies; roster derives; hardcoded tail deleted | +| 5b | [#2799](https://github.com/open-gsd/gsd-core/issues/2799) | **D6**, D5 (invocation-time host re-verification) | `invoke_reviewers` / `write_reviews` iterate lanes; the `antigravity` and `openai-compatible` **handler modules** ported from the existing bash legs; the **`kimi-code`** lane — **closes #2718** | +| 6 | [#2800](https://github.com/open-gsd/gsd-core/issues/2800) | — | Docs, `hostBehaviors` documentation gap, capability matrix, locale parity gate — **closes #2781** | +| 7 | [#2801](https://github.com/open-gsd/gsd-core/issues/2801) | D9 (alias removal) | Remove the `hostBehaviors.reviewerCli` alias, the release *after* 5a | + +Two mappings are worth calling out because a reader would otherwise assume the wrong phase. **D6's +handler modules are code**, not data — porting Antigravity's ~100-line three-layer fallback and the +three OpenAI-compatible lanes into named first-party modules is Phase 5b's work, delivered alongside +the iteration that calls them. And **D5 splits across two phases**: the disclosure itself is Phase 3, +but the invocation-time destination re-verification necessarily lands in Phase 5b, because that is +where the invocation path is built. + +**Why `kimi-code` lands in 5b and not 5a.** 5a makes the roster derive from declared bodies, but 5b +is what makes `invoke_reviewers` iterate them. The eleven existing lanes already have hand-authored +legs, so declaring them in 5a changes nothing observable. `kimi-code` is net-new with no leg — +declaring it in 5a would make it **selectable but not invocable**: present in `--all`, selected, and +producing an empty section for the whole 5a → 5b window. Landing it with the iteration keeps the +Phase 1 parity assertion green across the entire migration. + +## Alternatives considered + +1. **A single unified `invoke` shape.** The design this ADR started from. Rejected on evidence: a + read of all twelve legs found three that are HTTP endpoints with no binary (see Context). Had it + shipped, Phase 2 would have bolted on an implicit second shape or stranded three lanes in the + hardcoded tail this epic exists to delete. +2. **Transport inferred from field presence** (`binary` ⇒ spawn, `hostConfigKey` ⇒ http). Fewer + fields; rejected because a manifest with both or neither has undefined meaning. +3. **A spawn-only body, leaving the three HTTP lanes in core.** Smaller and sooner; rejected because + it preserves a hardcoded tail and permanently bars a third party from shipping a local-model + lane — the epic's own problem statement in miniature. +4. **Core descriptor table only** (#2690 as filed). Single-sources invocation inside + `review-reviewer-selection.cts` and collapses the eleven blocks. Cheaper and lands sooner, and it + does fix the cross-cutting-defect cadence — but it does not make lanes installable: still a core + patch, still no trust gate, still no federated config. **Not discarded — adopted as Phase 1**, so + the descriptor shape is designed once under this ADR rather than twice. +5. **Keep `hostBehaviors.reviewerCli`, just document and validate it.** Cheapest, and it does close + the documentation gap. Rejected because it leaves problems (b) and (c) intact: a lane still + cannot own its config, and the five non-installable reviewers still have nowhere to live. +6. **A third-party `handler` module confined to the install root.** See D6 — the only option that + genuinely delivers "any plugin"; rejected here as reversing rather than amending ADR-1016, and as + the surface ADR-1244 D7 sequences last. Revisit with its own ADR. +7. **Route lanes through MCP.** Rejected: reviewers are batch, single-shot, ten-to-twenty-minute + invocations. An MCP server lifecycle adds nothing, and `mcpServers` disclosure already covers the + cases that genuinely are servers. +8. **One `role: "reviewer"` for every lane**, splitting the six dual-purpose runtimes. Cleaner + discriminator; rejected for churn (D3). diff --git a/docs/adr/857-capability-system.md b/docs/adr/857-capability-system.md index 1094419af..8d5a1b04a 100644 --- a/docs/adr/857-capability-system.md +++ b/docs/adr/857-capability-system.md @@ -6,6 +6,8 @@ - **Subsumes (generalizes):** Skill Surface Budget Module ([ADR-0011](0011-skill-surface-budget-module.md)), Runtime Install Policy Module ([ADR-58](58-runtime-install-policy-module.md)) — both remain **Accepted and live**; this ADR generalizes them, it does not replace them. See "Relation to ADR-0011 and ADR-58" below. - **Builds on:** CommandRoutingHub (ADR-0012), Runtime Artifact Layout Module (ADR-3660), generated-cjs single source (ADR-457) - **Amended:** 2026-06-12 — phase-6 boundary settled before the Migrate phase freezes it: the **verifier↔predicate contract** is classified as core verification substrate (not an off-by-default Feature Capability). See *Verification substrate vs. plug-in tier (the predicate boundary)* below. Prompted by @davesienkowski's boundary analysis on #857; coordinates with ADR-550 (spec-phase probe contract). +- **Decisions 7 and 8 are extended by** [ADR-1244](1244-capability-ecosystem.md) (Capability Ecosystem) — amend, not reverse. D7 deferred third-party code-loading "to its own ADR" and D8 deferred third-party CLI support to "an external loader + trust/validation gate"; ADR-1244 is that ADR and delivers that gate. *(This marker is required by ADR-1244 D8, which states ADR-857 "is updated to mark D7 and D8 'extended by ADR-1244'". The marker was never actually added; recorded here 2026-07-28 while amending this ADR for ADR-2782.)* +- **Amended by:** [ADR-2782](2782-reviewer-lane-capability-surface.md) (Reviewer Lane capability surface) — extends "extension points as data" to the cross-AI reviewer handoff: a reviewer lane becomes a declared capability body rather than a core patch. ## Ratification (2026-07-17): Proposed → Accepted diff --git a/docs/adr/894-capability-declaration-format.md b/docs/adr/894-capability-declaration-format.md index 67f5e9849..1dd42f8b4 100644 --- a/docs/adr/894-capability-declaration-format.md +++ b/docs/adr/894-capability-declaration-format.md @@ -6,6 +6,7 @@ - **Parent:** [ADR-857](857-capability-system.md) (Capability system) — resolves its Open question #1 - **Phase:** ADR-857 rollout phase 3a (design-only) - **Subsumed by:** [ADR-1239](1239-gsd-embeddable-orchestration-engine.md) (GSD as an Embeddable Orchestration Engine) — read it first; see the amendment below +- **Amended by:** [ADR-2782](2782-reviewer-lane-capability-surface.md) (Reviewer Lane capability surface) — adds a `reviewer` role-typed body and a third `role` value (`"reviewer"`) to the declaration format. ## Amendment (2026-07-16): subsumed by ADR-1239 (EoS); status is stale diff --git a/docs/adr/README.md b/docs/adr/README.md index 6481e8823..4470f4786 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -116,7 +116,7 @@ This replaces a hand-maintained table that had drifted to **40 of 65 ADRs** — -### Active decisions (52) +### Active decisions (53) These govern the system as it stands. Cite these. @@ -173,6 +173,7 @@ These govern the system as it stands. Cite these. | [ADR-2346](2346-command-dispatch-completion.md) | Command Dispatch Completion | Accepted | — | | [ADR-2629](2629-phase-effort-estimation-calibration.md) | Phase effort is estimated against a calibrated smart-zone budget, not a static heuristic | Accepted | — | | [ADR-2719](2719-emitted-artifact-attribution.md) | Emitted-artifact attribution — replace the committed parity fixtures with a computed conservation law | Accepted | — | +| [ADR-2782](2782-reviewer-lane-capability-surface.md) | Reviewer Lane — the cross-AI reviewer handoff becomes a declared capability surface | Accepted | — | | [ADR-3660](3660-runtime-artifact-layout-module.md) | Runtime Artifact Layout Module owns per-runtime artifact placement | Accepted | [ADR-1239](1239-gsd-embeddable-orchestration-engine.md) | ### Proposed (8) @@ -205,7 +206,7 @@ Historical record. **Do not follow these** — each names what replaced it, or w | [ADR-2264](2264-golden-parity-redesign.md) | Redesign golden-install-parity — single-source manifest builder + split invariant | Superseded | [ADR-2719](2719-emitted-artifact-attribution.md) | | [ADR-3524](3524-cjs-sdk-hard-seam.md) | CJS↔SDK hard seam — one source of truth per Shared Module | Superseded | [ADR-0174](0174-retire-gsd-sdk-package-boundary.md) | -_68 ADRs. Generated by `scripts/gen-adr-index.cjs` — run `--write` after adding or restatusing an ADR._ +_69 ADRs. Generated by `scripts/gen-adr-index.cjs` — run `--write` after adding or restatusing an ADR._