golem15/msd-core
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

chore(#2798): declare the eleven reviewer lanes as manifest data (#2837)

Browse Source 
* chore(#2798): declare the eleven reviewer lanes as manifest data

Phase 5a of epic #2782, delivering ADR-2782 D9 (roster half) and D3.

- Five reviewers GSD never installs into become lane-only role:reviewer
  capabilities with no runtime body, no runtimeCompat and no install surface:
  gemini, coderabbit, ollama, lm-studio, llama-cpp. Before this they had no
  descriptor at all and lived as a hardcoded NON_RUNTIME_REVIEWER_SLUGS tail,
  which is now deleted outright.
- The six hosts that are ALSO reviewers gain a reviewer body alongside their
  runtime body. Their runtime bodies are byte-identical to next -- verified per
  capability against the git blob, not asserted -- so no install behaviour moves.
- KNOWN_REVIEWER_SLUGS derives from declared bodies via an exported
  deriveReviewerSlugs(registry). hostBehaviors.reviewerCli survives as a derived
  legacy alias for one release; where a capability carries both, the body wins
  and the slug appears once. Alias removal is Phase 7 (#2801).

THE KEYSTONE: the roster is the SAME ELEVEN SLUGS as before -- antigravity,
claude, coderabbit, codex, cursor, gemini, llama_cpp, lm_studio, ollama,
opencode, qwen. This phase changes HOW the roster is derived, not WHO is in it,
and the test asserts that literal list rather than a count.

kimi-code is deliberately NOT declared here. It is net-new with no
invoke_reviewers leg, so declaring it now would make it selectable but not
invocable -- present in --all, selected, emitting an empty section for the whole
5a-to-5b window -- and would break Phase 1's parity assertion. It lands in 5b
alongside the iteration that can run it. Legacy kimi (the Python CLI) is not a
reviewer at all and gains nothing.

The highest-value test is declaredManifestLanesMatchThePhase1Descriptor: it
deep-compares all eleven declared bodies against REVIEWER_LANES field-by-field,
including probe and invoke sub-fields. All eleven are byte-identical, key order
included. The epic's premise is that the manifest and the core descriptor
describe the same lane with NO translation layer, and Phase 2's review already
caught one divergence that every other test missed.

Two ADR corrections folded in, as Phases 1-3 each did:

1. PHASE ORDER. The ADR runs Phase 4 (federated config) before 5a and #2798
   claims a dependency on 4. That is inverted and makes Phase 4 unsatisfiable:
   D9 assigns review.<host>_host to lane capabilities that do not exist until
   THIS phase creates them, and a federated config slice must live inside
   capabilities/<id>/capability.json. Real graph: Phase 2 -> 5a -> 4.
2. #2798's INVENTORY acceptance item is vacuous. The inventory catalogs
   bin/lib/*.cjs modules, not capability directories -- antigravity, opencode
   and qwen appear zero times in it -- and gen-inventory-manifest --check passes
   with the five new dirs and no edit.

Also corrected a stale line in Phase 2's own ADR amendment: it recorded the slug
pattern as /^[a-z][a-z0-9_-]*$/, but Phase 2's security review widened the
shipped pattern to /^[a-z0-9][a-z0-9_-]*$/ to match Phase 1's exported
LANE_SLUG_RE. The prose had not followed the code.

Closes #2798

* fix(#2798): catalogue reviewer capabilities in the generated matrix

The capability matrix rendered exactly two tables, feature and runtime, via
renderTable(caps, role) filtering on c.role === role. ADR-2782 D3 added a THIRD
role, so every role:"reviewer" capability was silently dropped from the
first-party catalogue.

The drift guard did not catch it, and could not: --check compares generated
output against the committed file, and both omitted the five lanes identically,
so it reported "up to date" while five shipped capabilities were invisible in
the one document that is supposed to list what ships. A guard blind to an entire
role is not guarding.

This phase is what exposed it -- it ships the first role:"reviewer"
capabilities -- so it is fixed here rather than deferred (CLAUDE.md: a defect
found while working is fixed in the current change, which overrides
one-concern-per-PR).

Verified red-before-green: with a lane row deleted from the matrix, --check now
exits 1; restored, it exits 0. Before this fix the lanes were absent entirely, so
there was nothing for the guard to compare.

Phase 6 (#2800) still owns enriching the matrix with lane-specific detail
(slug/flag/transport columns) and the locale parity gate. This is the narrower
fix: the capabilities APPEAR at all.

* fix(#2798): close two hardening gaps and record three limits durably

Isolated security review (5 targets, no blockers) reproduced two gaps in the new
deriveReviewerSlugs. Both are unreachable through the checked-in registry -- it is
generated, JSON-sourced and code-reviewed -- but the function is EXPORTED for
reuse and carries no other validation, so it must not depend on its caller.

- A whitespace-only slug passed the length>0 test verbatim and occupied a roster
  entry it could never match. Slugs are now trimmed before the emptiness test. A
  blank body correctly falls through to the legacy alias rather than DROPPING the
  lane, which would have been worse than the blank slug.
- KNOWN_REVIEWER_SLUGS is computed at require() time, so an uncaught throw there
  breaks import for EVERY consumer rather than degrading selection. It is now
  guarded, yielding an empty roster on a malformed registry. That is a visible
  degradation, not a silent one: under D4 an explicitly requested reviewer that
  is unavailable is an ERROR, so /gsd:review --claude against an empty roster
  fails loudly. This also removes an asymmetry -- the sibling capability-trust
  module documents its collectors as TOTAL and wraps them for exactly this reason.

Also records three findings that previously existed ONLY in squash-merged PR
bodies, which is not a durable record:

- ADR-2782 D5 gains an implementation note explaining why the resolved host is
  deliberately EXCLUDED from the disclosure signature. Rule 1 says consent binds
  the resolved host; the loader has no config resolver, so folding it in would
  make the loader and lifecycle compute different signatures for one manifest and
  re-prompt forever. The binding is split: signature covers the SHA-pinned
  manifest fields, the consent record stores the resolved host, and Phase 5b
  re-resolves at invocation -- which is where rule 4 already puts the check. A
  reader comparing rule 1 to the code would otherwise conclude it is unimplemented.
- CONTEXT.md's capability-trust entry still described THREE executable surfaces.
  Phase 3 added the fourth and made that false; corrected here, since it is drift
  this epic introduced rather than Phase 6's new-glossary-term work.
- stableJson documents the NaN/Infinity/undefined -> null signature collision and
  why it is unreachable (JSON grammar has no such literal, so JSON.parse throws
  first). Reachability rests entirely on the ingest path staying JSON.parse-only,
  so the note lives where someone would break it.

* chore(#2798): backfill changeset pr number to 2837
This commit is contained in:
Tom Boucher
2026-07-29 16:58:02 -04:00
committed by GitHub
parent 982e83f0f7
commit 6a9babda69
20 changed files with 1740 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
src/capability-trust.cts
View File

@@ -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<unknown>): string {
if (typeof value === 'bigint') return JSON.stringify(`${value.toString()}n`);

135
src/review-reviewer-selection.cts
View File

@@ -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/<id>/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/<id>/ 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/<id>/` 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<string> = [
'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<string, { runtime?: { hostBehaviors?: { reviewerCli?: boolean } } }>;
};
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<string> = [
...deriveRuntimeReviewerSlugs(),
...NON_RUNTIME_REVIEWER_SLUGS,
];
interface ReviewerCapabilityRegistry {
capabilities?: Record<string, RegistryReviewerCapability>;
}
/**
* 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<string>();
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<string> = (() => {
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-]*$/;