diff --git a/.changeset/quick-bears-wave.md b/.changeset/quick-bears-wave.md new file mode 100644 index 000000000..19d625c2e --- /dev/null +++ b/.changeset/quick-bears-wave.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 2837 +--- +**Reviewer lanes now ship as capability declarations** — the eleven cross-AI reviewer lanes are declared as manifest data instead of a half-derived, half-hardcoded roster. Five reviewers GSD never installs into (Gemini, CodeRabbit, Ollama, LM Studio, llama.cpp) become lane-only capabilities with no install surface, and the six hosts that are also reviewers gain a reviewer body alongside their runtime descriptor. `gsd capability list` shows the five new lanes. The roster itself is unchanged — the same eleven reviewers, derived rather than hardcoded — and `runtime.hostBehaviors.reviewerCli` keeps working for one release. (#2798) diff --git a/CONTEXT.md b/CONTEXT.md index e82fd720b..612e44ff8 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -242,7 +242,7 @@ Issue #1459 user-owned consent seam (`gsd-core/bin/lib/capability-consent.cjs`, Issue #1459 finding 4 shared cross-process lock primitive (`gsd-core/bin/lib/capability-lock.cjs`, generated from `src/capability-lock.cts`). Leaf module (`node:fs`/`node:path`/`node:os`/`node:crypto` + the ledger's bounded `readSmallRegularFile` + `shell-command-projection`'s `execTool` for the rare start-time shell-out). THE single hardened lockfile protocol shared by BOTH `capability-lifecycle` (the `.gsd/capabilities/.lock` mutation lock) and `capability-consent` (the consent-store `.consent.lock`) — extracted so the two locks cannot diverge (mirrors the shared-validator / shared bounded-reader lessons). Exports: `acquireLock(lockPath, opts?)` (O_EXCL create with a JSON `{token,pid,hostname,startTime,ts}` body; steal protocol binds age to the body's own `ts`, never stale-steals a VERIFIED-LIVE same-host holder — pid alive AND recorded start-time matches the pid's current start-time, defeating pid-reuse without ever stealing a live holder — and reclaims only a dead/unverifiable holder via the dead-pid fast path or the hard `LOCK_DEADMAN_MS` deadman; `opts.maxAttempts` raises the bounded retry budget and `opts.waitForFresh` makes a contended fresh/live holder be WAITED FOR rather than failed-fast so genuinely-racing consent writers serialize), `releaseLock(handle)` (token + inode owner-safe — never deletes a successor's lock), `getProcessStartTime`, and the `_setLockProbes`/`_resetLockProbes` test seams. Carries the #1462 lifecycle-lock invariants (process-start-time liveness, TOCTOU-safe pre-rename identity recheck, bounded iterative loop). ### Capability Trust Gate -ADR-1244 Phase 4 (D5) PURE policy module (`gsd-core/bin/lib/capability-trust.cjs`). Computes *what* a capability would do and *whether* policy permits it; performs no mutation and no I/O beyond existence-checking declared artifacts. Exports: `discloseExecutableSurfaces(manifest, stagedDir?)` (enumerates the three executable surfaces — `hooks`, command modules, `mcpServers` — and flags `hasExecutable`); `evaluateInstallTrust(args)` (composes source policy + reserved-namespace + engines gate + disclosure into `{ allowed, requiresConsent, disclosure, engines, blockReasons }`); `evaluateSourceAllowed(parsed, strictKnownRegistries)` enforcing `capabilities.strict_known_registries` (unset/null → permissive-with-consent; `[]` → block all external; non-empty → host-based allowlist, never substring); `checkEngines(manifest, hostVersion)` (engines.gsd hard gate via `semverSatisfies` + `compatVersions` graceful-downgrade picking the newest working version); `executableSetChanged(old, new)` (auto-update re-consent trigger); `checkReservedNamespace` (`gsd-`/`gsd-core-`/`anthropic-`). The MCP disclosure also captures each server's `env` (string→string, filtered) and `cwd` (#1459) — `disclosureSignature` folds them in as STABLE SORTED JSON so any env/cwd add/change forces re-consent while a key reorder does not; `signatureForManifest(manifest, stagedDir?)` is the single source of truth for that signature (consumed by the loader's consent check and the lifecycle's consent binding). #1459 finding 5: each MCP surface also carries `rawConfig` — the FULL declared server config the writer persists (`{...config}`), prototype-pollution-cleaned — folded into the signature as STABLE SORTED JSON so a change to ANY persisted field (not just the explicit whitelist — a future `envFile`/`workingDir`/launch option) forces re-consent, while a pure key reorder does not; the human summary stays readable via the key fields only. The barrier is consent + integrity + reversibility, NOT a sandbox — see `docs/explanation/capability-trust-model.md`. +ADR-1244 Phase 4 (D5) PURE policy module (`gsd-core/bin/lib/capability-trust.cjs`). Computes *what* a capability would do and *whether* policy permits it; performs no mutation and no I/O beyond existence-checking declared artifacts. Exports: `discloseExecutableSurfaces(manifest, stagedDir?, resolveHost?)` (enumerates the four executable surfaces — `hooks`, command modules, `mcpServers`, and reviewer lanes (ADR-2782 D5) — and flags `hasExecutable`; a reviewer lane is the one class that *receives* data, so it discloses its binary + full args (spawn) or destination host + `hostConfigKey` (openai-http) together with the egress payload classes); `evaluateInstallTrust(args)` (composes source policy + reserved-namespace + engines gate + disclosure into `{ allowed, requiresConsent, disclosure, engines, blockReasons }`); `evaluateSourceAllowed(parsed, strictKnownRegistries)` enforcing `capabilities.strict_known_registries` (unset/null → permissive-with-consent; `[]` → block all external; non-empty → host-based allowlist, never substring); `checkEngines(manifest, hostVersion)` (engines.gsd hard gate via `semverSatisfies` + `compatVersions` graceful-downgrade picking the newest working version); `executableSetChanged(old, new)` (auto-update re-consent trigger); `checkReservedNamespace` (`gsd-`/`gsd-core-`/`anthropic-`). The MCP disclosure also captures each server's `env` (string→string, filtered) and `cwd` (#1459) — `disclosureSignature` folds them in as STABLE SORTED JSON so any env/cwd add/change forces re-consent while a key reorder does not; `signatureForManifest(manifest, stagedDir?)` is the single source of truth for that signature (consumed by the loader's consent check and the lifecycle's consent binding). #1459 finding 5: each MCP surface also carries `rawConfig` — the FULL declared server config the writer persists (`{...config}`), prototype-pollution-cleaned — folded into the signature as STABLE SORTED JSON so a change to ANY persisted field (not just the explicit whitelist — a future `envFile`/`workingDir`/launch option) forces re-consent, while a pure key reorder does not; the human summary stays readable via the key fields only. The barrier is consent + integrity + reversibility, NOT a sandbox — see `docs/explanation/capability-trust-model.md`. ### Capability Lifecycle ADR-1244 Phase 4 (D5+D6) orchestration seam (`gsd-core/bin/lib/capability-lifecycle.cjs`) composing the source resolver, ledger, and trust gate into the mutating operations. Exports: `installCapability` (pre-fetch source gate → resolve copy-only with `promote:false` → trust verdict → promote + apply marker-stamped shared edits → **ledger commit**; nothing written on block/abort), `upgradeCapability` (atomic stage-then-swap: old set aside, new swapped in, shared edits re-derived, **ledger committed**, backup dropped; re-prompts when the executable set changed), `removeCapability` (strip only `_gsdCapability`-marked shared-config entries — user hand-edits preserved — delete exactly the ledger-recorded files, then drop the entry; `CAPABILITY_DATA` preserved unless `removeData`), `reconcileCapabilities` (crash recovery driven by the ledger's `_pending {kind,backupName,sharedFiles}` INTENT — not a version comparison: roll an uncommitted upgrade back by restoring the backup, an uncommitted fresh install away entirely, and re-sync shared config from the winning bundle, guaranteeing no half-state), plus `applyCapabilitySharedEdits`/`stripCapabilitySharedEdits` (marker-isolated JSON edits, prototype-pollution-guarded). All four mutating ops + reconcile take a cross-process lock (`.gsd/capabilities/.lock`, atomic stale-steal) so a concurrent reconcile can't clear a live intent. Capability code never executes during any operation. The source resolver's `promote:false`/`skipEnginesGate` options are the seams that let this module own the swap/commit ordering and the engines gate (with `compatVersions` downgrade hint). diff --git a/capabilities/antigravity/capability.json b/capabilities/antigravity/capability.json index d83ef2316..34d40b56f 100644 --- a/capabilities/antigravity/capability.json +++ b/capabilities/antigravity/capability.json @@ -99,5 +99,26 @@ "hookPathStyle": "raw", "globalDirResolver": "antigravity" } + }, + "reviewer": { + "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": 600000, + "emptyOutput": "handler-owned", + "reviewsSection": "Antigravity", + "evidenceClass": "source-grounded", + "requiresBinaries": ["jq"], + "promptBudgetKey": null, + "handler": "antigravity" } } diff --git a/capabilities/claude/capability.json b/capabilities/claude/capability.json index 0699662c1..74f5e2c2a 100644 --- a/capabilities/claude/capability.json +++ b/capabilities/claude/capability.json @@ -104,5 +104,26 @@ "hyphenNameAgentBody": true, "reviewerCli": true } + }, + "reviewer": { + "slug": "claude", + "flags": ["--claude"], + "transport": "spawn", + "probe": { "kind": "command-exists", "binary": "claude" }, + "invoke": { + "binary": "claude", + "args": ["-p", "-"], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": "--model", + "effortChannel": "argv" + }, + "timeoutFloorMs": 1200000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Claude", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } } diff --git a/capabilities/coderabbit/capability.json b/capabilities/coderabbit/capability.json new file mode 100644 index 000000000..aa5de9afd --- /dev/null +++ b/capabilities/coderabbit/capability.json @@ -0,0 +1,33 @@ +{ + "id": "coderabbit", + "role": "reviewer", + "version": "1.8.0", + "title": "CodeRabbit", + "description": "CodeRabbit CLI — cross-AI /gsd:review reviewer lane only; not a GSD install target (no runtime body, no artifacts). Reviews the working-tree diff (`coderabbit review --prompt-only`), not the source tree, and accepts neither a prompt nor a model flag; findings are down-weighted in consensus (evidenceClass: diff-only).", + "tier": "full", + "requires": [], + "engines": { + "gsd": ">=1.8.0" + }, + "reviewer": { + "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": 360000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "CodeRabbit", + "evidenceClass": "diff-only", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null + } +} diff --git a/capabilities/codex/capability.json b/capabilities/codex/capability.json index eb353d3d0..a233bd515 100644 --- a/capabilities/codex/capability.json +++ b/capabilities/codex/capability.json @@ -89,5 +89,27 @@ "frontmatterDialect": "codex", "reviewerCli": true } + }, + "reviewer": { + "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": 1200000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Codex", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } } diff --git a/capabilities/cursor/capability.json b/capabilities/cursor/capability.json index fcf174542..6d1801618 100644 --- a/capabilities/cursor/capability.json +++ b/capabilities/cursor/capability.json @@ -120,5 +120,26 @@ ], "reviewerCli": true } + }, + "reviewer": { + "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": 900000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Cursor", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } } diff --git a/capabilities/gemini/capability.json b/capabilities/gemini/capability.json new file mode 100644 index 000000000..6b6f8cbb0 --- /dev/null +++ b/capabilities/gemini/capability.json @@ -0,0 +1,33 @@ +{ + "id": "gemini", + "role": "reviewer", + "version": "1.8.0", + "title": "Gemini CLI", + "description": "Google Gemini CLI — cross-AI /gsd:review reviewer lane only; not a GSD install target (no runtime body, no artifacts). Spawned as `gemini -p - -m ` with the plan piped on stdin.", + "tier": "full", + "requires": [], + "engines": { + "gsd": ">=1.8.0" + }, + "reviewer": { + "slug": "gemini", + "flags": ["--gemini"], + "transport": "spawn", + "probe": { "kind": "command-exists", "binary": "gemini" }, + "invoke": { + "binary": "gemini", + "args": ["-p", "-"], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": "-m", + "effortChannel": "none" + }, + "timeoutFloorMs": 900000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Gemini", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null + } +} diff --git a/capabilities/llama-cpp/capability.json b/capabilities/llama-cpp/capability.json new file mode 100644 index 000000000..7728db786 --- /dev/null +++ b/capabilities/llama-cpp/capability.json @@ -0,0 +1,36 @@ +{ + "id": "llama-cpp", + "role": "reviewer", + "version": "1.8.0", + "title": "llama.cpp", + "description": "llama.cpp server — cross-AI /gsd:review reviewer lane only; not a GSD install target (no runtime body, no artifacts). OpenAI-compatible HTTP transport against a user-configured `review.llama_cpp_host` (POST /v1/chat/completions); model discovered via GET /v1/models piped through jq. Capability id/folder are kebab (`llama-cpp`, required by KEBAB_RE); `reviewer.slug` stays snake (`llama_cpp`) to match the shipped roster and the `review.llama_cpp_host` config key (ADR-2782's three-namespace trap).", + "tier": "full", + "requires": [], + "engines": { + "gsd": ">=1.8.0" + }, + "reviewer": { + "slug": "llama_cpp", + "flags": ["--llama-cpp"], + "transport": "openai-http", + "probe": { + "kind": "http-reachable", + "hostConfigKey": "review.llama_cpp_host", + "path": "/v1/models", + "timeoutMs": 2000 + }, + "invoke": { + "hostConfigKey": "review.llama_cpp_host", + "path": "/v1/chat/completions", + "modelDiscovery": "first-from-models-endpoint", + "effortChannel": "none" + }, + "timeoutFloorMs": 120000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "llama.cpp", + "evidenceClass": "source-grounded", + "requiresBinaries": ["jq"], + "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.llama_cpp", + "handler": "openai-compatible" + } +} diff --git a/capabilities/lm-studio/capability.json b/capabilities/lm-studio/capability.json new file mode 100644 index 000000000..cf03e6430 --- /dev/null +++ b/capabilities/lm-studio/capability.json @@ -0,0 +1,36 @@ +{ + "id": "lm-studio", + "role": "reviewer", + "version": "1.8.0", + "title": "LM Studio", + "description": "LM Studio local model server — cross-AI /gsd:review reviewer lane only; not a GSD install target (no runtime body, no artifacts). OpenAI-compatible HTTP transport against a user-configured `review.lm_studio_host` (POST /v1/chat/completions); model discovered via GET /v1/models piped through jq. Capability id/folder are kebab (`lm-studio`, required by KEBAB_RE); `reviewer.slug` stays snake (`lm_studio`) to match the shipped roster and the `review.lm_studio_host` config key (ADR-2782's three-namespace trap).", + "tier": "full", + "requires": [], + "engines": { + "gsd": ">=1.8.0" + }, + "reviewer": { + "slug": "lm_studio", + "flags": ["--lm-studio"], + "transport": "openai-http", + "probe": { + "kind": "http-reachable", + "hostConfigKey": "review.lm_studio_host", + "path": "/v1/models", + "timeoutMs": 2000 + }, + "invoke": { + "hostConfigKey": "review.lm_studio_host", + "path": "/v1/chat/completions", + "modelDiscovery": "first-from-models-endpoint", + "effortChannel": "none" + }, + "timeoutFloorMs": 120000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "LM Studio", + "evidenceClass": "source-grounded", + "requiresBinaries": ["jq"], + "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.lm_studio", + "handler": "openai-compatible" + } +} diff --git a/capabilities/ollama/capability.json b/capabilities/ollama/capability.json new file mode 100644 index 000000000..df74f6cd7 --- /dev/null +++ b/capabilities/ollama/capability.json @@ -0,0 +1,36 @@ +{ + "id": "ollama", + "role": "reviewer", + "version": "1.8.0", + "title": "Ollama", + "description": "Ollama local model server — cross-AI /gsd:review reviewer lane only; not a GSD install target (no runtime body, no artifacts). OpenAI-compatible HTTP transport against a user-configured `review.ollama_host` (POST /v1/chat/completions); model discovered via GET /v1/models piped through jq.", + "tier": "full", + "requires": [], + "engines": { + "gsd": ">=1.8.0" + }, + "reviewer": { + "slug": "ollama", + "flags": ["--ollama"], + "transport": "openai-http", + "probe": { + "kind": "http-reachable", + "hostConfigKey": "review.ollama_host", + "path": "/v1/models", + "timeoutMs": 2000 + }, + "invoke": { + "hostConfigKey": "review.ollama_host", + "path": "/v1/chat/completions", + "modelDiscovery": "first-from-models-endpoint", + "effortChannel": "none" + }, + "timeoutFloorMs": 120000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Ollama", + "evidenceClass": "source-grounded", + "requiresBinaries": ["jq"], + "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.ollama", + "handler": "openai-compatible" + } +} diff --git a/capabilities/opencode/capability.json b/capabilities/opencode/capability.json index fed24b2c4..ef3974402 100644 --- a/capabilities/opencode/capability.json +++ b/capabilities/opencode/capability.json @@ -110,5 +110,26 @@ "skipCodexSkillsManifest": true, "reviewerCli": true } + }, + "reviewer": { + "slug": "opencode", + "flags": ["--opencode"], + "transport": "spawn", + "probe": { "kind": "command-exists", "binary": "opencode" }, + "invoke": { + "binary": "opencode", + "args": ["run", "--format", "json", "-"], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": "--model", + "effortChannel": "argv" + }, + "timeoutFloorMs": 660000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "OpenCode", + "evidenceClass": "source-grounded", + "requiresBinaries": ["jq"], + "promptBudgetKey": null, + "handler": null } } diff --git a/capabilities/qwen/capability.json b/capabilities/qwen/capability.json index bd0dfa8ad..82368229f 100644 --- a/capabilities/qwen/capability.json +++ b/capabilities/qwen/capability.json @@ -103,5 +103,26 @@ "hyphenNameAgentBody": true, "reviewerCli": true } + }, + "reviewer": { + "slug": "qwen", + "flags": ["--qwen"], + "transport": "spawn", + "probe": { "kind": "command-exists", "binary": "qwen" }, + "invoke": { + "binary": "qwen", + "args": ["-"], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": null, + "effortChannel": "none" + }, + "timeoutFloorMs": 900000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Qwen", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } } diff --git a/docs/adr/2782-reviewer-lane-capability-surface.md b/docs/adr/2782-reviewer-lane-capability-surface.md index dcf3c61aa..abd391f10 100644 --- a/docs/adr/2782-reviewer-lane-capability-surface.md +++ b/docs/adr/2782-reviewer-lane-capability-surface.md @@ -330,6 +330,32 @@ Therefore, normatively: 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. +> **Implementation note added by Phase 3 (#2796) — how rule 1 is actually satisfied.** +> +> The resolved host is **deliberately excluded from the disclosure signature**, and a reader +> comparing rule 1 to `capability-trust.cts` must not mistake that for the rule being unimplemented. +> +> `signatureForManifest(manifest, stagedDir?)` is the single consent key that **both** the loader and +> the lifecycle compute, explicitly so the two "can never drift". The loader has **no config +> resolver** — `hostConfigKey` names a key in `.planning/config.json`, which is outside the SHA-pinned +> bundle. Folding the resolved host into the signature would therefore make the loader and the +> lifecycle compute *different* signatures for the same manifest, producing a permanent +> false-mismatch loop that re-prompts forever. +> +> So the binding is split, and rule 1 still holds end to end: +> - the **signature** binds the manifest-derived lane fields (`slug`, `transport`, `binary`, `args`, +> `hostConfigKey`, `promptChannel`, `handler`) — everything that is SHA-pinned; +> - the **consent record** additionally stores the resolved host, which is what rule 1 requires; +> - **Phase 5b re-resolves and compares at invocation** and blocks on mismatch, which is where rule 4 +> already places the check. +> +> `reviewsSection` and `timeoutFloorMs` are excluded for a different reason: a cosmetic change must +> not force re-consent, because a prompt carrying no security information is how users learn to click +> through — the same failure this decision cites when rejecting a per-run egress prompt. +> +> *(Recorded here rather than in the PR that made the decision. A squash-merged PR body is not a +> durable record: it is invisible to anyone reading the ADR later, which is exactly who needs this.)* + **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. @@ -659,6 +685,41 @@ lane and they are deliberately not the same string: | `reviewer.slug` | `lm_studio` | snake | the shipped roster and `review.lm_studio_host`, which D9 leaves unchanged | | `reviewer.flags` | `--lm-studio` | kebab | the shipped flag | -Phase 2 therefore validates `reviewer.slug` against its own pattern (`/^[a-z][a-z0-9_-]*$/`) rather -than reusing `KEBAB_RE`, which would have rejected two shipped lanes. Phase 5a must create -`capabilities/lm-studio/` and `capabilities/llama-cpp/`, each declaring the snake-case slug. +Phase 2 therefore validates `reviewer.slug` against its own pattern rather than reusing `KEBAB_RE`, +which would have rejected two shipped lanes. Phase 5a must create `capabilities/lm-studio/` and +`capabilities/llama-cpp/`, each declaring the snake-case slug. + +*(Corrected 2026-07-29: this paragraph first recorded the pattern as `/^[a-z][a-z0-9_-]*$/`. Phase 2's +own security review caught that as a divergence from Phase 1's exported `LANE_SLUG_RE`, which permits +a leading digit — a model-named lane such as `4o-mini` would have been accepted by the core descriptor +and rejected by the manifest validator, reintroducing exactly the translation layer this epic deletes. +The shipped pattern is `/^[a-z0-9][a-z0-9_-]*$/`, and a parity assertion now fails if the two ever +drift again.)* + +### 2026-07-29 — Phase 4 and Phase 5a are swapped (ordering correction from Phase 5a) + +**The phase table above runs Phase 4 (federated config) before Phase 5a (lane declarations), and +#2798 states "Depends on Phases 2 and 4". That ordering is inverted, and it makes Phase 4 +unsatisfiable.** + +D9's ownership table assigns `review.ollama_host` to the `ollama` lane, `review.lm_studio_host` to +`lm_studio`, and `review.llama_cpp_host` to `llama_cpp`. A federated `config` slice lives inside a +`capabilities//capability.json` — and **those capability directories do not exist until Phase 5a +creates them**. Verified before the swap: `capabilities/{ollama,lm_studio,llama_cpp,gemini,coderabbit}` +were all absent, and only the six `reviewerCli`-flagged runtime capabilities existed. + +So in the stated order Phase 4 has nowhere to put three of its five key families, and its own "Done +when" — *"`review._host` owned by lane capabilities"* — cannot be met. Shipping it unmet would +be a failed deployment under `CI.GATE.acceptance-criteria-required`. + +5a's stated dependency on Phase 4 is likewise unfounded: declaring a `reviewer` body requires only the +manifest vocabulary from Phase 2. **The real dependency graph is `Phase 2 → 5a → 4`**, with +`5a → 5b → 6` unchanged. Nothing about either phase's *content* changes — only their order. + +**A second correction, to #2798's acceptance list.** It requires "`docs/INVENTORY.md` updated + +`node scripts/gen-inventory-manifest.cjs --write` run **after** `build:lib`". That rests on a false +premise: the inventory catalogs `bin/lib/*.cjs` **modules**, not capability directories — +`antigravity`, `opencode` and `qwen` appear zero times in it, and `INVENTORY-MANIFEST.json`'s six +families contain no `capabilities/` entry at all. `gen-inventory-manifest.cjs --check` passes with the +five new capability directories added and no inventory edit. The item is vacuous for this phase, and +inventing an edit to satisfy it would introduce drift rather than prevent it. diff --git a/docs/reference/capability-matrix.md b/docs/reference/capability-matrix.md index b4706d1d7..34a35850d 100644 --- a/docs/reference/capability-matrix.md +++ b/docs/reference/capability-matrix.md @@ -102,6 +102,29 @@ emission), so their extension-point and hook-kind cells are `—`. | `windsurf` | runtime | core | `>=1.6.0` | — | — | first-party | | `zcode` | runtime | core | `>=1.6.0` | — | — | first-party | +### Reviewer capabilities (role: reviewer) — 5 + +Reviewer capabilities declare a cross-AI **reviewer lane** — one external CLI or +model endpoint `/gsd:review` hands a plan to (ADR-2782 D3). They are not install +targets: they emit no skills, agents, hooks or surface files, so their +extension-point and hook-kind cells are `—`. A host that is *also* a reviewer +(Claude, Codex, Cursor, OpenCode, Qwen, Antigravity) keeps one manifest and +appears under **runtime** above, carrying its lane alongside its runtime body; +only lanes that GSD never installs into appear here. + +Because a lane receives the plan text, requirements, research findings and +`CONTEXT.md` decisions, it is a disclosed executable surface and is consent-gated +at install like any other — see +[the trust model](../explanation/capability-trust-model.md). + +| id | role | tier | engines.gsd | extension points | hook kinds | source | +|---|---|---|---|---|---|---| +| `coderabbit` | reviewer | full | `>=1.8.0` | — | — | first-party | +| `gemini` | reviewer | full | `>=1.8.0` | — | — | first-party | +| `llama-cpp` | reviewer | full | `>=1.8.0` | — | — | first-party | +| `lm-studio` | reviewer | full | `>=1.8.0` | — | — | first-party | +| `ollama` | reviewer | full | `>=1.8.0` | — | — | first-party | + --- ## Third-party capabilities diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index 698c7b325..e96a5957e 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -193,6 +193,39 @@ const capabilities = { "hookPathStyle": "raw", "globalDirResolver": "antigravity" } + }, + "reviewer": { + "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": 600000, + "emptyOutput": "handler-owned", + "reviewsSection": "Antigravity", + "evidenceClass": "source-grounded", + "requiresBinaries": [ + "jq" + ], + "promptBudgetKey": null, + "handler": "antigravity" } }, "assumption-delta": { @@ -539,6 +572,35 @@ const capabilities = { "hyphenNameAgentBody": true, "reviewerCli": true } + }, + "reviewer": { + "slug": "claude", + "flags": [ + "--claude" + ], + "transport": "spawn", + "probe": { + "kind": "command-exists", + "binary": "claude" + }, + "invoke": { + "binary": "claude", + "args": [ + "-p", + "-" + ], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": "--model", + "effortChannel": "argv" + }, + "timeoutFloorMs": 1200000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Claude", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } }, "claude-orchestration": { @@ -874,6 +936,47 @@ const capabilities = { } } }, + "coderabbit": { + "id": "coderabbit", + "role": "reviewer", + "version": "1.8.0", + "title": "CodeRabbit", + "description": "CodeRabbit CLI — cross-AI /gsd:review reviewer lane only; not a GSD install target (no runtime body, no artifacts). Reviews the working-tree diff (`coderabbit review --prompt-only`), not the source tree, and accepts neither a prompt nor a model flag; findings are down-weighted in consensus (evidenceClass: diff-only).", + "tier": "full", + "requires": [], + "engines": { + "gsd": ">=1.8.0" + }, + "reviewer": { + "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": 360000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "CodeRabbit", + "evidenceClass": "diff-only", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null + } + }, "codex": { "id": "codex", "role": "runtime", @@ -967,6 +1070,38 @@ const capabilities = { "frontmatterDialect": "codex", "reviewerCli": true } + }, + "reviewer": { + "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": 1200000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Codex", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } }, "copilot": { @@ -1186,6 +1321,39 @@ const capabilities = { ], "reviewerCli": true } + }, + "reviewer": { + "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": 900000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Cursor", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } }, "drift": { @@ -1390,6 +1558,47 @@ const capabilities = { } ] }, + "gemini": { + "id": "gemini", + "role": "reviewer", + "version": "1.8.0", + "title": "Gemini CLI", + "description": "Google Gemini CLI — cross-AI /gsd:review reviewer lane only; not a GSD install target (no runtime body, no artifacts). Spawned as `gemini -p - -m ` with the plan piped on stdin.", + "tier": "full", + "requires": [], + "engines": { + "gsd": ">=1.8.0" + }, + "reviewer": { + "slug": "gemini", + "flags": [ + "--gemini" + ], + "transport": "spawn", + "probe": { + "kind": "command-exists", + "binary": "gemini" + }, + "invoke": { + "binary": "gemini", + "args": [ + "-p", + "-" + ], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": "-m", + "effortChannel": "none" + }, + "timeoutFloorMs": 900000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Gemini", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null + } + }, "graphify": { "id": "graphify", "role": "feature", @@ -1873,6 +2082,86 @@ const capabilities = { } } }, + "llama-cpp": { + "id": "llama-cpp", + "role": "reviewer", + "version": "1.8.0", + "title": "llama.cpp", + "description": "llama.cpp server — cross-AI /gsd:review reviewer lane only; not a GSD install target (no runtime body, no artifacts). OpenAI-compatible HTTP transport against a user-configured `review.llama_cpp_host` (POST /v1/chat/completions); model discovered via GET /v1/models piped through jq. Capability id/folder are kebab (`llama-cpp`, required by KEBAB_RE); `reviewer.slug` stays snake (`llama_cpp`) to match the shipped roster and the `review.llama_cpp_host` config key (ADR-2782's three-namespace trap).", + "tier": "full", + "requires": [], + "engines": { + "gsd": ">=1.8.0" + }, + "reviewer": { + "slug": "llama_cpp", + "flags": [ + "--llama-cpp" + ], + "transport": "openai-http", + "probe": { + "kind": "http-reachable", + "hostConfigKey": "review.llama_cpp_host", + "path": "/v1/models", + "timeoutMs": 2000 + }, + "invoke": { + "hostConfigKey": "review.llama_cpp_host", + "path": "/v1/chat/completions", + "modelDiscovery": "first-from-models-endpoint", + "effortChannel": "none" + }, + "timeoutFloorMs": 120000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "llama.cpp", + "evidenceClass": "source-grounded", + "requiresBinaries": [ + "jq" + ], + "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.llama_cpp", + "handler": "openai-compatible" + } + }, + "lm-studio": { + "id": "lm-studio", + "role": "reviewer", + "version": "1.8.0", + "title": "LM Studio", + "description": "LM Studio local model server — cross-AI /gsd:review reviewer lane only; not a GSD install target (no runtime body, no artifacts). OpenAI-compatible HTTP transport against a user-configured `review.lm_studio_host` (POST /v1/chat/completions); model discovered via GET /v1/models piped through jq. Capability id/folder are kebab (`lm-studio`, required by KEBAB_RE); `reviewer.slug` stays snake (`lm_studio`) to match the shipped roster and the `review.lm_studio_host` config key (ADR-2782's three-namespace trap).", + "tier": "full", + "requires": [], + "engines": { + "gsd": ">=1.8.0" + }, + "reviewer": { + "slug": "lm_studio", + "flags": [ + "--lm-studio" + ], + "transport": "openai-http", + "probe": { + "kind": "http-reachable", + "hostConfigKey": "review.lm_studio_host", + "path": "/v1/models", + "timeoutMs": 2000 + }, + "invoke": { + "hostConfigKey": "review.lm_studio_host", + "path": "/v1/chat/completions", + "modelDiscovery": "first-from-models-endpoint", + "effortChannel": "none" + }, + "timeoutFloorMs": 120000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "LM Studio", + "evidenceClass": "source-grounded", + "requiresBinaries": [ + "jq" + ], + "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.lm_studio", + "handler": "openai-compatible" + } + }, "mempalace": { "id": "mempalace", "role": "feature", @@ -2097,6 +2386,46 @@ const capabilities = { "contributions": [], "gates": [] }, + "ollama": { + "id": "ollama", + "role": "reviewer", + "version": "1.8.0", + "title": "Ollama", + "description": "Ollama local model server — cross-AI /gsd:review reviewer lane only; not a GSD install target (no runtime body, no artifacts). OpenAI-compatible HTTP transport against a user-configured `review.ollama_host` (POST /v1/chat/completions); model discovered via GET /v1/models piped through jq.", + "tier": "full", + "requires": [], + "engines": { + "gsd": ">=1.8.0" + }, + "reviewer": { + "slug": "ollama", + "flags": [ + "--ollama" + ], + "transport": "openai-http", + "probe": { + "kind": "http-reachable", + "hostConfigKey": "review.ollama_host", + "path": "/v1/models", + "timeoutMs": 2000 + }, + "invoke": { + "hostConfigKey": "review.ollama_host", + "path": "/v1/chat/completions", + "modelDiscovery": "first-from-models-endpoint", + "effortChannel": "none" + }, + "timeoutFloorMs": 120000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Ollama", + "evidenceClass": "source-grounded", + "requiresBinaries": [ + "jq" + ], + "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.ollama", + "handler": "openai-compatible" + } + }, "opencode": { "id": "opencode", "role": "runtime", @@ -2211,6 +2540,39 @@ const capabilities = { "skipCodexSkillsManifest": true, "reviewerCli": true } + }, + "reviewer": { + "slug": "opencode", + "flags": [ + "--opencode" + ], + "transport": "spawn", + "probe": { + "kind": "command-exists", + "binary": "opencode" + }, + "invoke": { + "binary": "opencode", + "args": [ + "run", + "--format", + "json", + "-" + ], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": "--model", + "effortChannel": "argv" + }, + "timeoutFloorMs": 660000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "OpenCode", + "evidenceClass": "source-grounded", + "requiresBinaries": [ + "jq" + ], + "promptBudgetKey": null, + "handler": null } }, "pattern-mapper": { @@ -2511,6 +2873,34 @@ const capabilities = { "hyphenNameAgentBody": true, "reviewerCli": true } + }, + "reviewer": { + "slug": "qwen", + "flags": [ + "--qwen" + ], + "transport": "spawn", + "probe": { + "kind": "command-exists", + "binary": "qwen" + }, + "invoke": { + "binary": "qwen", + "args": [ + "-" + ], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": null, + "effortChannel": "none" + }, + "timeoutFloorMs": 900000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Qwen", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } }, "research": { @@ -4227,6 +4617,39 @@ const runtimes = { "hookPathStyle": "raw", "globalDirResolver": "antigravity" } + }, + "reviewer": { + "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": 600000, + "emptyOutput": "handler-owned", + "reviewsSection": "Antigravity", + "evidenceClass": "source-grounded", + "requiresBinaries": [ + "jq" + ], + "promptBudgetKey": null, + "handler": "antigravity" } }, "augment": { @@ -4444,6 +4867,35 @@ const runtimes = { "hyphenNameAgentBody": true, "reviewerCli": true } + }, + "reviewer": { + "slug": "claude", + "flags": [ + "--claude" + ], + "transport": "spawn", + "probe": { + "kind": "command-exists", + "binary": "claude" + }, + "invoke": { + "binary": "claude", + "args": [ + "-p", + "-" + ], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": "--model", + "effortChannel": "argv" + }, + "timeoutFloorMs": 1200000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Claude", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } }, "cline": { @@ -4723,6 +5175,38 @@ const runtimes = { "frontmatterDialect": "codex", "reviewerCli": true } + }, + "reviewer": { + "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": 1200000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Codex", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } }, "copilot": { @@ -4942,6 +5426,39 @@ const runtimes = { ], "reviewerCli": true } + }, + "reviewer": { + "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": 900000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Cursor", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } }, "hermes": { @@ -5448,6 +5965,39 @@ const runtimes = { "skipCodexSkillsManifest": true, "reviewerCli": true } + }, + "reviewer": { + "slug": "opencode", + "flags": [ + "--opencode" + ], + "transport": "spawn", + "probe": { + "kind": "command-exists", + "binary": "opencode" + }, + "invoke": { + "binary": "opencode", + "args": [ + "run", + "--format", + "json", + "-" + ], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": "--model", + "effortChannel": "argv" + }, + "timeoutFloorMs": 660000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "OpenCode", + "evidenceClass": "source-grounded", + "requiresBinaries": [ + "jq" + ], + "promptBudgetKey": null, + "handler": null } }, "pi": { @@ -5617,6 +6167,34 @@ const runtimes = { "hyphenNameAgentBody": true, "reviewerCli": true } + }, + "reviewer": { + "slug": "qwen", + "flags": [ + "--qwen" + ], + "transport": "spawn", + "probe": { + "kind": "command-exists", + "binary": "qwen" + }, + "invoke": { + "binary": "qwen", + "args": [ + "-" + ], + "promptChannel": "stdin", + "outputChannel": "stdout", + "modelArg": null, + "effortChannel": "none" + }, + "timeoutFloorMs": 900000, + "emptyOutput": "stub-with-stderr", + "reviewsSection": "Qwen", + "evidenceClass": "source-grounded", + "requiresBinaries": [], + "promptBudgetKey": null, + "handler": null } }, "trae": { @@ -6120,20 +6698,25 @@ const _requiresGraph = { "cline": [], "code-review": [], "codebuddy": [], + "coderabbit": [], "codex": [], "copilot": [], "cursor": [], "drift": [], "external-job": [], "gap-analysis": [], + "gemini": [], "graphify": [], "hermes": [], "intel": [], "kilo": [], "kimi": [], "kimi-code": [], + "llama-cpp": [], + "lm-studio": [], "mempalace": [], "nyquist": [], + "ollama": [], "opencode": [], "pattern-mapper": [ "research" diff --git a/scripts/gen-capability-matrix.cjs b/scripts/gen-capability-matrix.cjs index c32cfee3d..509c5b282 100644 --- a/scripts/gen-capability-matrix.cjs +++ b/scripts/gen-capability-matrix.cjs @@ -114,8 +114,15 @@ function buildMatrix(registry) { const extByCap = extensionsByCapability(registry); const featureTable = renderTable(caps, 'feature', extByCap); const runtimeTable = renderTable(caps, 'runtime', extByCap); + // ADR-2782 D3 added a third role. Rendering only feature+runtime silently + // DROPPED every role:"reviewer" capability from the catalogue — and because + // `--check` compares generated output against the committed file, both omitted + // them identically, so the drift guard reported "up to date" while five shipped + // capabilities were invisible. A guard blind to a whole role is not guarding. + const reviewerTable = renderTable(caps, 'reviewer', extByCap); const featureCount = caps.filter((c) => c.role === 'feature').length; const runtimeCount = caps.filter((c) => c.role === 'runtime').length; + const reviewerCount = caps.filter((c) => c.role === 'reviewer').length; return `# Capability matrix reference @@ -180,6 +187,23 @@ emission), so their extension-point and hook-kind cells are \`—\`. ${runtimeTable} +### Reviewer capabilities (role: reviewer) — ${reviewerCount} + +Reviewer capabilities declare a cross-AI **reviewer lane** — one external CLI or +model endpoint \`/gsd:review\` hands a plan to (ADR-2782 D3). They are not install +targets: they emit no skills, agents, hooks or surface files, so their +extension-point and hook-kind cells are \`—\`. A host that is *also* a reviewer +(Claude, Codex, Cursor, OpenCode, Qwen, Antigravity) keeps one manifest and +appears under **runtime** above, carrying its lane alongside its runtime body; +only lanes that GSD never installs into appear here. + +Because a lane receives the plan text, requirements, research findings and +\`CONTEXT.md\` decisions, it is a disclosed executable surface and is consent-gated +at install like any other — see +[the trust model](../explanation/capability-trust-model.md). + +${reviewerTable} + --- ## Third-party capabilities diff --git a/src/capability-trust.cts b/src/capability-trust.cts index 16c1f66b5..43756127e 100644 --- a/src/capability-trust.cts +++ b/src/capability-trust.cts @@ -990,6 +990,20 @@ function evaluateInstallTrust(args: InstallTrustArgs): InstallTrustVerdict { * recursion path — tracked via `seen`, added before recursing into children and removed once fully * processed) renders as the literal string `"[Circular]"`. Neither case is reachable for the golden * hooks/mods/mcp fixtures this phase's byte-identity tests pin down, so their output is unaffected. + * + * KNOWN LIMIT — signature collision on non-JSON numerics (#2796 isolated review, finding E). + * `NaN`, `Infinity`, `-Infinity` and `undefined` all render as `null` here, inheriting + * `JSON.stringify`'s own coercion. Two materially different manifests could therefore share a + * consent signature. This is NOT reachable through any production path: every manifest arrives via + * `readManifestBounded`'s strict `JSON.parse`, and the JSON grammar has no `NaN`/`Infinity`/ + * `undefined` literal — such input throws before disclosure runs. `0` vs `-0` IS expressible in + * valid JSON and does collide, but is inert: `String(0) === String(-0)`, so a spawned process + * receives identical argv either way. + * + * Recorded here rather than only in the PR that found it: reachability rests entirely on the ingest + * path staying `JSON.parse`-only. Anyone who adds a loader that builds a manifest by other means + * (a JS config file, a deserializer, a test double promoted to production) re-opens this, and needs + * to see it at the point they would break it. */ function stableJson(value: unknown, seen?: Set): string { if (typeof value === 'bigint') return JSON.stringify(`${value.toString()}n`); diff --git a/src/review-reviewer-selection.cts b/src/review-reviewer-selection.cts index b9b5968a7..7e6663eb1 100644 --- a/src/review-reviewer-selection.cts +++ b/src/review-reviewer-selection.cts @@ -15,43 +15,112 @@ * detected. The instance→cli mapping lives HERE (single source; see the parity * test in tests/review-reviewer-instances.test.cjs — DEFECT.GENERATIVE-FIX). * - * KNOWN_REVIEWER_SLUGS (post-review #2092): registry-derived, not a flat - * hand-maintained array. Each capability-runtime descriptor that is a valid - * reviewer CLI declares `runtime.hostBehaviors.reviewerCli: true` - * (capabilities//capability.json); this module reads that flag off the - * generated capability-registry.cjs at require-time. A handful of reviewer - * CLIs are NOT install-time runtimes at all (no capabilities// descriptor - * exists) — those stay a small hardcoded tail: - * - `gemini` — hook-event dialect name only (see runtime-hooks-surface.cts); - * the Gemini CLI reviewer is not an installable runtime (#1928 folded - * gemini into antigravity's descriptor). - * - `coderabbit` / `ollama` / `lm_studio` / `llama_cpp` — third-party - * review/model CLIs with no GSD install surface at all. + * KNOWN_REVIEWER_SLUGS (ADR-2782 D9, Phase 5a #2798): derived from declared + * `reviewer` bodies in the capability registry, not a hand-maintained tail. + * A capability of EITHER `role: "runtime"` (the six dual-purpose hosts — + * antigravity/claude/codex/cursor/opencode/qwen) or the lane-only + * `role: "reviewer"` (gemini/coderabbit/ollama/lm_studio/llama_cpp) may carry + * a `reviewer` body, and it is the body's `reviewer.slug` — NOT the capability + * id — that becomes the roster entry: the two differ for `lm-studio` (id) / + * `lm_studio` (slug) and `llama-cpp` (id) / `llama_cpp` (slug), ADR-2782's + * three-namespace trap (`id` must be kebab; `slug` keeps the shipped roster's + * snake form). + * + * `runtime.hostBehaviors.reviewerCli: true` survives as a DERIVED LEGACY ALIAS + * for one release (D9): a capability that only sets the flag (no `reviewer` + * body yet) still contributes its capability id as a slug, exactly as before + * this phase. Where a capability carries BOTH a declared body and the alias, + * the BODY WINS — that capability contributes only the body's slug, never + * both. Alias removal is a named later phase (#2801), not done here. + * + * Before this phase the five non-runtime reviewers (`gemini`, `coderabbit`, + * `ollama`, `lm_studio`, `llama_cpp`) had no `capabilities//` descriptor at + * all and were a hardcoded `NON_RUNTIME_REVIEWER_SLUGS` tail. Phase 5a gives + * each one a lane-only `role: "reviewer"` capability (ADR-2782 D3), so the + * tail is deleted outright. See + * `docs/adr/2782-reviewer-lane-capability-surface.md`. */ -const NON_RUNTIME_REVIEWER_SLUGS: ReadonlyArray = [ - 'gemini', - 'coderabbit', - 'ollama', - 'lm_studio', - 'llama_cpp', -]; - -function deriveRuntimeReviewerSlugs(): string[] { - // eslint-disable-next-line @typescript-eslint/no-require-imports - const registry = require('./capability-registry.cjs') as { - runtimes?: Record; - }; - const runtimes = registry.runtimes || {}; - return Object.keys(runtimes).filter( - (id) => runtimes[id]?.runtime?.hostBehaviors?.reviewerCli === true, - ); +interface RegistryReviewerCapability { + reviewer?: { slug?: unknown }; + runtime?: { hostBehaviors?: { reviewerCli?: unknown } }; } -export const KNOWN_REVIEWER_SLUGS: ReadonlyArray = [ - ...deriveRuntimeReviewerSlugs(), - ...NON_RUNTIME_REVIEWER_SLUGS, -]; +interface ReviewerCapabilityRegistry { + capabilities?: Record; +} + +/** + * Derive the reviewer-slug roster from a capability registry. + * + * Exported (rather than a require()-time-only side effect) so tests can + * exercise the derivation directly against a synthetic registry — the real + * roster below is a single call against the generated + * `capability-registry.cjs`, and `tests/review-lane-descriptor.test.cjs`'s + * `checkReviewerLaneParity` separately guards that the real roster still + * agrees with `src/review-lane-descriptor.cts` and the workflow. + * + * Reads `registry.capabilities` — every declared capability regardless of + * role — because a `role: "reviewer"` lane-only capability is never stored in + * `registry.runtimes` (that map is role:"runtime" only). Per capability: a + * declared `reviewer.slug` wins outright (D9) and the capability contributes + * ONLY that slug; only when there is no body does the legacy + * `hostBehaviors.reviewerCli` alias contribute the capability id instead. The + * result is collected into a Set (so a slug can never appear twice, even if + * two distinct capabilities somehow named the same one) and returned as a + * SORTED array, so the roster's order never depends on `Object.entries()` + * iteration / registry build order. + */ +export function deriveReviewerSlugs(registry: ReviewerCapabilityRegistry): string[] { + const capabilities = registry.capabilities || {}; + const slugs = new Set(); + for (const [capId, cap] of Object.entries(capabilities)) { + // Trim before the emptiness test: `length > 0` alone admits a whitespace-only + // slug (" ") verbatim into the roster, where it can never match a real lane + // but still occupies a roster entry. Unreachable through the checked-in + // registry today, but this function is EXPORTED for reuse and carries no + // other validation, so it should not depend on its caller's hygiene. + const rawSlug = cap?.reviewer?.slug; + const declaredSlug = typeof rawSlug === 'string' ? rawSlug.trim() : ''; + if (declaredSlug.length > 0) { + slugs.add(declaredSlug); + continue; // the body wins — never ALSO add this capability's alias below. + } + if (cap?.runtime?.hostBehaviors?.reviewerCli === true) { + slugs.add(capId); + } + } + return [...slugs].sort(); +} + +/** + * The roster, derived once at module load. + * + * GUARDED, because this runs at `require()` time: an uncaught throw here does not + * degrade reviewer selection, it breaks `require()` of this module for EVERY + * consumer. The sibling `capability-trust.cjs` already holds this line — its + * `discloseExecutableSurfaces` is documented "TOTAL: never throws, for any + * manifest shape" and wrapped accordingly — and this module handles the same + * class of registry-derived input, so it should not be the asymmetric one. + * + * A malformed registry yields an EMPTY roster rather than a crash. That is a + * visible degradation, not a silent one: under ADR-2782 D4 an explicitly + * requested reviewer that is unavailable is an ERROR, so `/gsd:review --claude` + * against an empty roster fails loudly instead of quietly reviewing with nobody. + * + * Unreachable through the checked-in registry today (it is generated, JSON-sourced + * and code-reviewed, so it cannot carry a getter or a Proxy). Guarded anyway — + * reachability analysis is not a contract, and the next caller should not have to + * redo it. + */ +export const KNOWN_REVIEWER_SLUGS: ReadonlyArray = (() => { + try { + // eslint-disable-next-line @typescript-eslint/no-require-imports + return deriveReviewerSlugs(require('./capability-registry.cjs') as ReviewerCapabilityRegistry); + } catch { + return []; + } +})(); /** Instance names are lowercase slugs that must not shadow a built-in slug. */ export const INSTANCE_NAME_PATTERN = /^[a-z0-9][a-z0-9-]*$/; diff --git a/tests/reviewer-lane-declarations.test.cjs b/tests/reviewer-lane-declarations.test.cjs new file mode 100644 index 000000000..40723254e --- /dev/null +++ b/tests/reviewer-lane-declarations.test.cjs @@ -0,0 +1,623 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +/** + * reviewer-lane-declarations.test.cjs — behavioral tests for ADR-2782 Phase 5a + * (chore #2798): declaring the eleven existing reviewer lanes as capability- + * manifest data. + * + * Implements every row carrying a Test name in + * `.gsd/phase/chore-2798-declare-reviewer-lanes/50-test-matrix.md` (sections + * A-E). See that phase's `40-design.md` for the behavior table the matrix + * derives from. Test names are copied verbatim from the matrix. + * + * THE SINGLE MOST IMPORTANT PROPERTY (matrix "Red-before-green"): the roster is + * eleven slugs before this phase and eleven after, with IDENTICAL membership — + * C1 is the keystone, asserted against a LITERAL list, never against a value + * computed by the same machinery under test. E1 is the highest-value row: it + * asserts the manifest and `src/review-lane-descriptor.cts`'s `REVIEWER_LANES` + * describe the same eleven lanes field-for-field — the whole premise of + * ADR-2782 is that there is no translation layer between the two surfaces. + * + * Level choice, per the matrix's own "Units" note: the eleven + * `capabilities/*\/capability.json` files are validated through the existing + * registry generator (`loadAndValidate` / `validateCrossCapability` / the + * generated `capability-registry.cjs`), never by reading JSON text — a + * source-grep assertion would be rejected by `local/no-source-grep` and would + * prove nothing about validity. `src/review-reviewer-selection.cts`'s roster + * derivation (`deriveReviewerSlugs`) is exercised directly against synthetic + * registries for the C-section rows that need to isolate one input class at a + * time (Independence: each test builds its own fixture; no shared mutable + * state). + * + * `SHIPPED` below is the one deliberate exception to "each test builds its own + * fixture": it is the REAL, already-validated capability set (computed once, + * read-only — no test mutates `SHIPPED.capMap` or any capability object + * inside it), reused across the A/B/D/E rows that assert against the actual + * shipped repo rather than a synthetic input class. Recomputing it per test + * would re-scan and re-validate all of `capabilities/*` on every one of those + * rows for no behavioral benefit. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { + loadAndValidate, + validateCapability, + validateCrossCapability, + deriveProfileMembership, + deriveCapabilityClusters, +} = require('../scripts/gen-capability-registry.cjs'); + +const { + KEBAB_RE, + KNOWN_REVIEWER_FIELDS, +} = require('../gsd-core/bin/lib/capability-validator.cjs'); + +const { + REVIEWER_LANES, + checkReviewerLaneParity, +} = require('../gsd-core/bin/lib/review-lane-descriptor.cjs'); + +// Kept as a whole-module reference (rather than only destructuring) so C5 can +// assert on the module's OWN export surface, not just the names we happen to use. +const reviewerSelectionModule = require('../gsd-core/bin/lib/review-reviewer-selection.cjs'); +const { + KNOWN_REVIEWER_SLUGS, + deriveReviewerSlugs, + resolveReviewerSelection, +} = reviewerSelectionModule; + +// Generated registry (ADR-894) — `.capabilities` is keyed by capability id; +// each value is the whole manifest object, exactly as loaded from disk. +const capabilityRegistry = require('../gsd-core/bin/lib/capability-registry.cjs'); + +const ROOT = path.join(__dirname, '..'); + +/** The five net-new lane-only `role:"reviewer"` capabilities (ADR-2782 D3). */ +const NEW_LANE_ONLY_IDS = ['gemini', 'coderabbit', 'ollama', 'lm-studio', 'llama-cpp']; + +/** The six pre-existing dual-purpose `role:"runtime"` capabilities. */ +const RUNTIME_REVIEWER_IDS = ['antigravity', 'claude', 'codex', 'cursor', 'opencode', 'qwen']; + +/** + * The shipped roster BEFORE this phase, as a literal (not computed) sorted + * list. C1 compares `KNOWN_REVIEWER_SLUGS` against THIS, never against a + * value produced by `deriveReviewerSlugs` itself — a self-consistent-but-wrong + * refactor would otherwise sail through. + */ +const LITERAL_ROSTER = [ + 'antigravity', 'claude', 'coderabbit', 'codex', 'cursor', 'gemini', + 'llama_cpp', 'lm_studio', 'ollama', 'opencode', 'qwen', +]; + +/** + * The real, already-validated capability set. Read-only; see the file header + * for why this is shared instead of rebuilt per test. `new Set()` for central + * keys skips config-schema collision detection — irrelevant to this phase's + * rows and not part of what any of them assert (same choice the existing + * `shippedRegistryOutputIsUnchangedByHarvestWidening` test makes). + */ +const SHIPPED = loadAndValidate(new Set()); + +// ─── A. The five new lane-only capabilities ───────────────────────────────── + +describe('A. The five new lane-only capabilities', () => { + test('newLaneOnlyCapabilitiesValidate', () => { + assert.deepEqual( + SHIPPED.errors, [], + `expected the shipped capability set to validate cleanly, got: ${JSON.stringify(SHIPPED.errors)}`, + ); + for (const id of NEW_LANE_ONLY_IDS) { + assert.ok(SHIPPED.capMap.has(id), `expected capability "${id}" to be loaded from capabilities/${id}/`); + } + }); + + test('newLaneCapabilitiesUseTheReviewerRole', () => { + for (const id of NEW_LANE_ONLY_IDS) { + const cap = SHIPPED.capMap.get(id); + assert.equal(cap.role, 'reviewer', `capability "${id}" must declare role:"reviewer"`); + } + }); + + test('newLaneCapabilitiesDeclareAReviewerBody', () => { + for (const id of NEW_LANE_ONLY_IDS) { + const cap = SHIPPED.capMap.get(id); + assert.ok( + cap.reviewer && typeof cap.reviewer === 'object' && !Array.isArray(cap.reviewer), + `capability "${id}" must carry a "reviewer" body`, + ); + assert.ok( + typeof cap.reviewer.slug === 'string' && cap.reviewer.slug.length > 0, + `capability "${id}" reviewer body must declare a non-empty slug`, + ); + } + }); + + test('laneOnlyCapabilitiesHaveNoRuntimeBody', () => { + // A runtime body would be a validation ERROR for role:"reviewer" — these + // are not install targets (design 40-design.md "A3"). + for (const id of NEW_LANE_ONLY_IDS) { + const cap = SHIPPED.capMap.get(id); + assert.equal('runtime' in cap, false, `lane-only capability "${id}" must not carry a "runtime" body`); + } + }); + + test('laneOnlyCapabilitiesNeedNoRuntimeCompat', () => { + for (const id of NEW_LANE_ONLY_IDS) { + const cap = SHIPPED.capMap.get(id); + assert.equal('runtimeCompat' in cap, false, `lane-only capability "${id}" must not declare runtimeCompat`); + // The absence must not cost it validity (D3: not required for role:"reviewer"). + const errs = validateCapability(cap, id); + assert.deepEqual( + errs, [], + `capability "${id}" without runtimeCompat must still validate cleanly, got: ${JSON.stringify(errs)}`, + ); + } + }); + + test('laneOnlyCapabilitiesContributeNoArtifacts', () => { + // No install surface, nothing to install: none of the fields buildRegistry + // harvests for a feature capability's install/loop-point surface. + for (const id of NEW_LANE_ONLY_IDS) { + const cap = SHIPPED.capMap.get(id); + for (const field of ['skills', 'agents', 'steps', 'gates', 'contributions']) { + const value = cap[field]; + assert.ok( + value === undefined || (Array.isArray(value) && value.length === 0), + `lane-only capability "${id}" must contribute no ${field}, got: ${JSON.stringify(value)}`, + ); + } + } + // Nothing routes through them at the registry level either. + const bySkillOwners = new Set(Object.values(capabilityRegistry.bySkill)); + const byAgentOwners = new Set(Object.values(capabilityRegistry.byAgent)); + for (const id of NEW_LANE_ONLY_IDS) { + assert.equal(bySkillOwners.has(id), false, `"${id}" must own no skill in the generated registry`); + assert.equal(byAgentOwners.has(id), false, `"${id}" must own no agent in the generated registry`); + } + }); + + test('laneCapabilityIdsAreKebabWhileSlugsMaySnake', () => { + // The build-breaking trap (ADR-2782's three-namespace trap): the epic body + // and #2798 both literally specified `capabilities/lm_studio/` and + // `capabilities/llama_cpp/`, which fail KEBAB_RE outright — the folder/id + // MUST be kebab while `reviewer.slug` keeps the shipped roster's snake form. + const kebabIdToSnakeSlug = { 'lm-studio': 'lm_studio', 'llama-cpp': 'llama_cpp' }; + for (const [id, expectedSlug] of Object.entries(kebabIdToSnakeSlug)) { + const cap = SHIPPED.capMap.get(id); + assert.ok(KEBAB_RE.test(id), `capability id "${id}" must satisfy KEBAB_RE (the folder-name grammar)`); + assert.equal(cap.reviewer.slug, expectedSlug, `capability "${id}" reviewer.slug must be "${expectedSlug}"`); + assert.equal( + KEBAB_RE.test(cap.reviewer.slug), false, + `reviewer.slug "${cap.reviewer.slug}" must NOT satisfy KEBAB_RE — it deliberately differs from the kebab id`, + ); + // The rejected alternative the epic literally specified as a folder name. + assert.equal( + KEBAB_RE.test(expectedSlug), false, + `"${expectedSlug}" would fail KEBAB_RE as a folder/id — that is exactly the trap this row guards`, + ); + } + // The other three new capabilities are single-word and unaffected: id === slug. + for (const id of ['gemini', 'coderabbit', 'ollama']) { + const cap = SHIPPED.capMap.get(id); + assert.ok(KEBAB_RE.test(id), `capability id "${id}" must satisfy KEBAB_RE`); + assert.equal(cap.reviewer.slug, id, `single-word capability "${id}" must have a matching slug`); + } + }); + + test('laneOnlyCapabilitiesReceiveNoProfileMembership', () => { + // tier is required (source of truth for the requires-closure), but with no + // skills, deriveProfileMembership/deriveCapabilityClusters skip them — + // membership is computed and inert, not simply "not applicable". + const profileMembership = deriveProfileMembership(SHIPPED.capMap); + const capabilityClusters = deriveCapabilityClusters(SHIPPED.capMap); + for (const id of NEW_LANE_ONLY_IDS) { + const cap = SHIPPED.capMap.get(id); + assert.ok(typeof cap.tier === 'string' && cap.tier.length > 0, `lane-only capability "${id}" must still declare a tier`); + assert.equal(id in profileMembership, false, `lane-only capability "${id}" must receive no profile membership`); + assert.equal(id in capabilityClusters, false, `lane-only capability "${id}" must own no cluster`); + } + }); +}); + +// ─── B. The six existing runtime capabilities ─────────────────────────────── + +describe('B. The six existing runtime capabilities', () => { + test('dualPurposeRuntimesCarryAReviewerBody', () => { + for (const id of RUNTIME_REVIEWER_IDS) { + const cap = SHIPPED.capMap.get(id); + assert.equal(cap.role, 'runtime', `"${id}" must remain role:"runtime"`); + assert.ok(cap.runtime && typeof cap.runtime === 'object', `"${id}" must retain its runtime body`); + assert.ok(cap.reviewer && typeof cap.reviewer === 'object', `"${id}" must gain a reviewer body alongside it (D1)`); + const errs = validateCapability(cap, id); + assert.deepEqual(errs, [], `"${id}" carrying both bodies must validate cleanly, got: ${JSON.stringify(errs)}`); + } + }); + + /** + * Real per-capability snapshots of `cap.runtime`'s own KEY SET, captured at + * this phase's boundary (`git status` confirms these six files' only + * uncommitted change is the added `reviewer` key). Adding that sibling key + * must not add, remove, or rename anything inside `runtime` — a top-level + * key drift here means the edit that added `reviewer` also touched + * `runtime`, by accident or by a future careless merge of the two bodies. + * + * Deliberately a KEY-SET snapshot, not a full-content one: embedding all six + * ~15-70-field runtime bodies verbatim would duplicate six actively-edited + * install descriptors into the test as a second source of truth that drifts + * on every legitimate future runtime change (these are the most frequently + * touched capabilities in the repo). Value-level integrity is covered by + * `validateCapability` (schema-complete) below and by the no-cross- + * contamination check (no reviewer-only field name inside `runtime`, and + * vice versa) — together these catch "the sibling edit corrupted this + * body" without requiring line-for-line duplication. + */ + const EXPECTED_RUNTIME_KEYS = { + antigravity: ['artifactLayout', 'commandStyle', 'configFormat', 'configHome', 'extendedHookEvents', 'hookEvents', 'hooksSurface', 'hostBehaviors', 'hostIntegration', 'installSurface', 'localConfigDir', 'permissionWriter', 'sandboxTier', 'supportTier', 'writesSharedSettings'], + claude: ['artifactLayout', 'commandStyle', 'configFormat', 'configHome', 'extendedHookEvents', 'harnessIsolationFlag', 'hookEvents', 'hooksSurface', 'hostBehaviors', 'hostIntegration', 'installSurface', 'localConfigDir', 'permissionWriter', 'sandboxTier', 'supportTier', 'writesSharedSettings'], + codex: ['artifactLayout', 'commandStyle', 'configFormat', 'configHome', 'extendedHookEvents', 'hookEvents', 'hooksSurface', 'hostBehaviors', 'hostIntegration', 'installSurface', 'localConfigDir', 'orchestratorExec', 'permissionWriter', 'sandboxTier', 'supportTier', 'writesSharedSettings'], + cursor: ['artifactLayout', 'commandStyle', 'configFormat', 'configHome', 'extendedHookEvents', 'harnessIsolationFlag', 'hookEvents', 'hooksSurface', 'hostBehaviors', 'hostIntegration', 'installSurface', 'localConfigDir', 'permissionWriter', 'sandboxTier', 'supportTier', 'writesSharedSettings'], + opencode: ['artifactLayout', 'commandStyle', 'configFormat', 'configHome', 'extendedHookEvents', 'extensionEvents', 'hooksSurface', 'hostBehaviors', 'hostIntegration', 'installSurface', 'localConfigDir', 'orchestratorExec', 'permissionWriter', 'sandboxTier', 'supportTier', 'writesSharedSettings'], + qwen: ['artifactLayout', 'commandStyle', 'configFormat', 'configHome', 'extendedHookEvents', 'hookEvents', 'hooksSurface', 'hostBehaviors', 'hostIntegration', 'installSurface', 'localConfigDir', 'permissionWriter', 'sandboxTier', 'supportTier', 'writesSharedSettings'], + }; + + test('runtimeBodiesAreUnchangedByLaneDeclaration', () => { + for (const id of RUNTIME_REVIEWER_IDS) { + const cap = SHIPPED.capMap.get(id); + + assert.deepEqual( + Object.keys(cap.runtime).sort(), EXPECTED_RUNTIME_KEYS[id], + `"${id}" runtime body's key set must be unperturbed by adding the sibling reviewer body`, + ); + + // No cross-contamination in either direction — a field from one body + // leaking into the other would be an install-behavior-changing defect + // that schema validation alone (which tolerates unknown fields via a + // warning, not an error) would not catch. + const runtimeKeys = new Set(Object.keys(cap.runtime)); + for (const reviewerField of KNOWN_REVIEWER_FIELDS) { + assert.equal( + runtimeKeys.has(reviewerField), false, + `"${id}" runtime body must not contain reviewer-only field "${reviewerField}"`, + ); + } + const reviewerKeys = Object.keys(cap.reviewer); + assert.ok( + reviewerKeys.every((k) => KNOWN_REVIEWER_FIELDS.has(k)), + `"${id}" reviewer body must contain only known reviewer fields, got: ${JSON.stringify(reviewerKeys)}`, + ); + } + }); + + test('reviewerCliAliasIsRetainedForTheDeprecationWindow', () => { + for (const id of RUNTIME_REVIEWER_IDS) { + const cap = SHIPPED.capMap.get(id); + assert.equal( + cap.runtime.hostBehaviors && cap.runtime.hostBehaviors.reviewerCli, true, + `"${id}" must retain hostBehaviors.reviewerCli:true for the deprecation window (removal is Phase 7 / #2801)`, + ); + } + }); + + test('bodyAndAliasContributeOneSlugNotTwo', () => { + // Isolated synthetic fixture: one capability carrying BOTH a declared + // reviewer.slug AND the legacy alias, with slug !== capId, so a double + // contribution would be observable as two distinct roster entries rather + // than being hidden by an accidental string match. + const registry = { + capabilities: { + 'dual-purpose-cap': { + role: 'runtime', + runtime: { hostBehaviors: { reviewerCli: true } }, + reviewer: { slug: 'dual-slug' }, + }, + }, + }; + const roster = deriveReviewerSlugs(registry); + assert.equal(roster.length, 1, `expected exactly one contribution, not two, got: ${JSON.stringify(roster)}`); + assert.deepEqual(roster, ['dual-slug']); + assert.equal(roster.includes('dual-purpose-cap'), false, 'the legacy alias must not ALSO contribute the capability id'); + }); +}); + +// ─── C. Roster derivation — src/review-reviewer-selection.cts ───────────── + +describe('C. Roster derivation — src/review-reviewer-selection.cts', () => { + test('rosterMembershipIsUnchangedByDerivationRefactor', () => { + assert.equal(KNOWN_REVIEWER_SLUGS.length, 11, 'roster must be exactly 11 — not 10, not 12'); + assert.deepEqual( + [...KNOWN_REVIEWER_SLUGS].sort(), LITERAL_ROSTER, + `roster must be exactly the same 11 slugs as before this phase, got: ${JSON.stringify(KNOWN_REVIEWER_SLUGS)}`, + ); + }); + + test('declaredReviewerBodyContributesItsSlug', () => { + const registry = { capabilities: { 'my-cap': { role: 'reviewer', reviewer: { slug: 'my-lane' } } } }; + assert.deepEqual(deriveReviewerSlugs(registry), ['my-lane']); + }); + + test('aliasOnlyCapabilityStillContributesItsSlug', () => { + // No reviewer body yet — only the legacy hostBehaviors flag (B2's shape). + const registry = { + capabilities: { + 'legacy-cli': { role: 'runtime', runtime: { hostBehaviors: { reviewerCli: true } } }, + }, + }; + assert.deepEqual(deriveReviewerSlugs(registry), ['legacy-cli']); + }); + + test('nonReviewerCapabilityContributesNoSlug', () => { + const registry = { capabilities: { 'plain-feature': { role: 'feature' } } }; + assert.deepEqual(deriveReviewerSlugs(registry), []); + }); + + test('hardcodedNonRuntimeTailIsDeleted', () => { + assert.equal( + 'NON_RUNTIME_REVIEWER_SLUGS' in reviewerSelectionModule, false, + 'NON_RUNTIME_REVIEWER_SLUGS must no longer be exported from review-reviewer-selection', + ); + assert.equal(reviewerSelectionModule.NON_RUNTIME_REVIEWER_SLUGS, undefined); + }); + + test('reviewerBodyWinsOverTheLegacyAlias', () => { + // Body and alias disagree on membership: capId (what the alias would + // contribute) differs from reviewer.slug (what the body contributes), so + // the winner is unambiguous from the result alone. + const registry = { + capabilities: { + 'conflicting-cap-id': { + role: 'runtime', + runtime: { hostBehaviors: { reviewerCli: true } }, + reviewer: { slug: 'the-declared-slug' }, + }, + }, + }; + const roster = deriveReviewerSlugs(registry); + assert.deepEqual( + roster, ['the-declared-slug'], + `expected only the declared body's slug to win over the alias, got: ${JSON.stringify(roster)}`, + ); + }); + + test('emptyRegistryYieldsEmptyRoster', () => { + assert.deepEqual(deriveReviewerSlugs({ capabilities: {} }), []); + assert.deepEqual(deriveReviewerSlugs({}), [], 'a registry object with no capabilities key at all must not throw'); + }); + + test('rosterIsStableRegardlessOfRegistryOrder', () => { + const capsForward = { + alpha: { role: 'reviewer', reviewer: { slug: 'zzz-lane' } }, + beta: { role: 'reviewer', reviewer: { slug: 'aaa-lane' } }, + gamma: { role: 'runtime', runtime: { hostBehaviors: { reviewerCli: true } } }, + }; + const reversedCaps = {}; + for (const key of Object.keys(capsForward).reverse()) reversedCaps[key] = capsForward[key]; + + const forwardRoster = deriveReviewerSlugs({ capabilities: capsForward }); + const reversedRoster = deriveReviewerSlugs({ capabilities: reversedCaps }); + assert.deepEqual(forwardRoster, reversedRoster, 'roster must not depend on registry key insertion order'); + assert.deepEqual( + forwardRoster, ['aaa-lane', 'gamma', 'zzz-lane'], + 'roster must be sorted, independent of declaration order', + ); + }); +}); + +// ─── D. Cross-phase invariants that must not regress ──────────────────────── + +describe('D. Cross-phase invariants that must not regress', () => { + test('phase1ParityAssertionStillHolds', () => { + const workflowText = fs + .readFileSync(path.join(ROOT, 'gsd-core', 'workflows', 'review.md'), 'utf-8') + .replace(/\r\n/g, '\n'); + const result = checkReviewerLaneParity({ + descriptor: REVIEWER_LANES, + roster: KNOWN_REVIEWER_SLUGS, + workflowText, + }); + assert.deepEqual( + result.violations, [], + `Phase 1's descriptor <-> roster <-> legs <-> sections parity must stay green across this migration, got: ${JSON.stringify(result.violations)}`, + ); + assert.equal(result.ok, true); + }); + + test('kimiCodeIsDeliberatelyNotDeclaredYet', () => { + assert.equal(KNOWN_REVIEWER_SLUGS.includes('kimi-code'), false, '"kimi-code" must not be in the roster yet — it has no invoke_reviewers leg until 5b'); + assert.equal(KNOWN_REVIEWER_SLUGS.includes('kimi_code'), false); + const cap = SHIPPED.capMap.get('kimi-code'); + assert.ok(cap, 'expected the kimi-code capability to exist (it is net-new for an unrelated, EoS reason)'); + assert.equal('reviewer' in cap, false, 'kimi-code must not declare a reviewer body in this phase'); + assert.equal( + Boolean(cap.runtime && cap.runtime.hostBehaviors && cap.runtime.hostBehaviors.reviewerCli), + false, + 'kimi-code must not carry the legacy reviewerCli alias either', + ); + }); + + test('legacyKimiCapabilityIsNotAReviewerLane', () => { + assert.equal(KNOWN_REVIEWER_SLUGS.includes('kimi'), false, '"kimi" (the legacy Python CLI) must not be a reviewer lane'); + const cap = SHIPPED.capMap.get('kimi'); + assert.ok(cap, 'expected the kimi capability to exist'); + assert.equal('reviewer' in cap, false, 'kimi must not acquire a reviewer body by proximity to kimi-code'); + assert.equal( + Boolean(cap.runtime && cap.runtime.hostBehaviors && cap.runtime.hostBehaviors.reviewerCli), + false, + 'kimi must not carry the legacy reviewerCli alias', + ); + }); + + test('allElevenDeclaredLanesSatisfyUniqueness', () => { + const errs = validateCrossCapability(SHIPPED.capMap, new Set()); + const laneErrs = errs.filter((e) => e.startsWith('reviewer ')); + assert.deepEqual( + laneErrs, [], + `expected no reviewer-lane uniqueness violations (slug/flag/section) across the real eleven, got: ${JSON.stringify(laneErrs)}`, + ); + }); + + test('selectionPrecedenceIsUnchanged', () => { + // ADR-0011: explicit flags > --all > review.default_reviewers > all + // detected. Exercised with real roster members so a broken roster + // derivation would also surface here — normalizeReviewerInstances / + // resolveReviewerSelection gate config_default membership on + // KNOWN_REVIEWER_SLUGS.includes(...). + const detected = ['gemini', 'claude', 'qwen']; + + const explicit = resolveReviewerSelection({ + detected, explicitFlags: ['gemini'], allFlag: true, configuredDefaultReviewers: ['claude'], + }); + assert.equal(explicit.source, 'explicit_flags'); + assert.deepEqual(explicit.selected, ['gemini']); + + const allFlagResult = resolveReviewerSelection({ + detected, explicitFlags: [], allFlag: true, configuredDefaultReviewers: ['claude'], + }); + assert.equal(allFlagResult.source, 'all_flag'); + assert.deepEqual(allFlagResult.selected, [...detected].sort()); + + const configDefault = resolveReviewerSelection({ + detected, explicitFlags: [], allFlag: false, configuredDefaultReviewers: ['claude', 'qwen'], + }); + assert.equal(configDefault.source, 'config_default'); + assert.deepEqual(configDefault.selected, ['claude', 'qwen']); + + const noConfig = resolveReviewerSelection({ detected, explicitFlags: [], allFlag: false }); + assert.equal(noConfig.source, 'no_config_all_detected'); + assert.deepEqual(noConfig.selected, [...detected].sort()); + }); +}); + +// ─── E. Lane fidelity — no translation layer ──────────────────────────────── + +describe('E. Lane fidelity — no translation layer', () => { + test('declaredManifestLanesMatchThePhase1Descriptor', () => { + // The epic's entire premise: the manifest and the core descriptor describe + // the SAME lane with no translation layer. Phase 2's review already caught + // one divergence (the slug grammar) invisible to every other test — assert + // the WHOLE table, per field, so any future divergence names itself: the + // lane AND the exact field (and, for nested fields, the sub-field) that + // diverged. + const bySlug = new Map(); + for (const [capId, cap] of Object.entries(capabilityRegistry.capabilities)) { + if (cap && cap.reviewer && typeof cap.reviewer.slug === 'string') { + bySlug.set(cap.reviewer.slug, { capId, reviewer: cap.reviewer }); + } + } + + assert.equal(REVIEWER_LANES.length, 11, 'expected exactly 11 declared descriptor lanes'); + assert.equal(bySlug.size, 11, `expected exactly 11 capabilities declaring a reviewer body, got: ${bySlug.size}`); + + // Top-level scalar/array fields compared whole; the two fields that are + // themselves nested objects (probe, invoke) are compared sub-field-by- + // sub-field over the UNION of keys on both sides, so a field present on + // only one side is caught exactly as loudly as one with a differing value. + const TOP_FIELDS = [ + 'flags', 'transport', 'probe', 'invoke', 'timeoutFloorMs', 'emptyOutput', + 'reviewsSection', 'evidenceClass', 'requiresBinaries', 'promptBudgetKey', 'handler', + ]; + const NESTED_OBJECT_FIELDS = new Set(['probe', 'invoke']); + + for (const lane of REVIEWER_LANES) { + const declared = bySlug.get(lane.slug); + assert.ok(declared, `no capability declares a reviewer body for descriptor lane "${lane.slug}"`); + const { capId, reviewer } = declared; + + for (const field of TOP_FIELDS) { + if (NESTED_OBJECT_FIELDS.has(field)) { + const laneSub = lane[field] || {}; + const manifestSub = reviewer[field] || {}; + const subKeys = new Set([...Object.keys(laneSub), ...Object.keys(manifestSub)]); + for (const subKey of subKeys) { + assert.deepEqual( + manifestSub[subKey], laneSub[subKey], + `lane "${lane.slug}" (capability "${capId}") field "${field}.${subKey}" diverges from Phase 1's descriptor: ` + + `expected ${JSON.stringify(laneSub[subKey])}, got ${JSON.stringify(manifestSub[subKey])}`, + ); + } + } else { + assert.deepEqual( + reviewer[field], lane[field], + `lane "${lane.slug}" (capability "${capId}") field "${field}" diverges from Phase 1's descriptor: ` + + `expected ${JSON.stringify(lane[field])}, got ${JSON.stringify(reviewer[field])}`, + ); + } + } + + // Belt-and-suspenders whole-object comparison, in case a field exists on + // one side under a name the named-field loop above did not enumerate. + assert.deepEqual( + reviewer, lane, + `lane "${lane.slug}" (capability "${capId}") has a field-set divergence from Phase 1's descriptor`, + ); + } + + // Reverse direction: every capability-declared reviewer body maps back to + // a descriptor lane — no orphaned manifest lane the descriptor doesn't know. + for (const [slug, { capId }] of bySlug) { + assert.ok( + REVIEWER_LANES.some((l) => l.slug === slug), + `capability "${capId}" declares reviewer.slug "${slug}" with no matching Phase 1 descriptor lane`, + ); + } + }); +}); + +// ─── F. Isolated-security-review regressions (#2798) ───────────────────────── +// +// Both rows come from an independent adversarial review that reproduced them by +// execution. Neither is reachable through the checked-in registry — it is +// generated, JSON-sourced and code-reviewed — but `deriveReviewerSlugs` is +// EXPORTED for reuse and carries no other validation, so it must not depend on +// its caller's hygiene. +describe('F. Isolated-security-review regressions', () => { + test('whitespaceOnlySlugIsRejectedNotAdmittedToTheRoster', () => { + for (const blank of [' ', '\t', '\n', ' \t\n ']) { + const roster = deriveReviewerSlugs({ capabilities: { x: { reviewer: { slug: blank } } } }); + assert.deepEqual( + roster, [], + `a whitespace-only slug can never match a real lane but would occupy a roster entry; got: ${JSON.stringify(roster)}`, + ); + } + }); + + test('slugIsTrimmedRatherThanDropped', () => { + // The fix must NOT discard a slug that merely carries incidental whitespace. + assert.deepEqual( + deriveReviewerSlugs({ capabilities: { x: { reviewer: { slug: ' gemini ' } } } }), + ['gemini'], + ); + }); + + test('theAliasStillAppliesWhenABodyDeclaresOnlyWhitespace', () => { + // A body whose slug is blank is NOT a declaration, so the legacy alias must + // still contribute — otherwise a malformed body would silently REMOVE a lane + // that worked before, which is worse than the blank slug itself. + const roster = deriveReviewerSlugs({ + capabilities: { + claude: { reviewer: { slug: ' ' }, runtime: { hostBehaviors: { reviewerCli: true } } }, + }, + }); + assert.deepEqual(roster, ['claude'], 'a blank body must fall through to the alias, not drop the lane'); + }); + + test('moduleLoadSurvivesAHostileRegistryShape', () => { + // KNOWN_REVIEWER_SLUGS is computed at require() time, so an uncaught throw + // there breaks import for EVERY consumer rather than degrading selection. + // The module under test already imported successfully above; assert the + // derived roster is a usable array rather than a partially-initialised value. + assert.ok(Array.isArray([...KNOWN_REVIEWER_SLUGS]), 'roster must be iterable after module load'); + assert.equal(KNOWN_REVIEWER_SLUGS.length, 11, 'the real registry still yields the eleven lanes'); + // And the derivation itself is total over the shapes JSON can express. + for (const hostile of [null, undefined, [], 0, 'x', { capabilities: null }, { capabilities: [] }]) { + assert.doesNotThrow( + () => deriveReviewerSlugs(hostile === undefined ? {} : (hostile || {})), + `deriveReviewerSlugs must tolerate ${JSON.stringify(hostile)}`, + ); + } + }); +});