diff --git a/.changeset/proud-ibex-snooze.md b/.changeset/proud-ibex-snooze.md new file mode 100644 index 000000000..da938655c --- /dev/null +++ b/.changeset/proud-ibex-snooze.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 2823 +--- +**Reviewer lanes can be declared as capability manifest data** — a capability may now carry a `reviewer` body describing a cross-AI review lane (slug, flags, transport, probe, invocation shape, timeout floor, output policy), and a new `role: "reviewer"` declares a lane that is not an install target. The registry validates the body against closed vocabularies and enforces slug, flag, and section uniqueness across first-party and installed capabilities, so two lanes can no longer silently share a REVIEWS.md heading. A capability with no reviewer body is unaffected. (#2795) diff --git a/docs/adr/2782-reviewer-lane-capability-surface.md b/docs/adr/2782-reviewer-lane-capability-surface.md index be5b3c609..dcf3c61aa 100644 --- a/docs/adr/2782-reviewer-lane-capability-surface.md +++ b/docs/adr/2782-reviewer-lane-capability-surface.md @@ -610,3 +610,55 @@ discriminator selecting the invoke sub-shape; `probe.kind` and `handler` are unt absent-safe invariant (D4), the disclosure class (D5), and the config-ownership table (D9) are unaffected. Phase 2 (#2795) implements the manifest validator against the vocabulary **as amended here**, which is the point of amending rather than leaving it for Phase 2 to rediscover. + +### 2026-07-29 — three factual corrections from Phase 2 (#2795) + +Implementing the validator required reading the code each claim rests on. Three statements above +did not survive that reading. None changes a decision; each would have misdirected a later phase, +which is precisely why they are corrected here rather than worked around in code. + +**1. The cause of the stranded config keys was misattributed (Context (b), Scope of changes, D9).** + +The ADR attributes reviewer config keys living centrally to the runtime body forbidding feature-only +fields. That is not the mechanism. `FEATURE_FIELDS_FORBIDDEN_ON_RUNTIME` is +`['skills','agents','steps','contributions','gates','hooks','activationKey']` — **`config` is not in +it**, and a `role: "runtime"` capability carrying a `config` slice passes validation today. The real +cause is two *harvest* sites that never read it: + +- `gen-capability-registry.cjs` nested its config-harvest loop inside the `role === 'feature'` + branch, so a non-feature capability's `config` was silently dropped from `configKeys`/`configSchema`. +- `validateCrossCapability` opened its config-key ownership loop with `if (cap.role !== 'feature') … + continue`, so a non-feature capability was exempt from both single-ownership **and** the + central-schema collision check. + +Phase 2 fixes both by filtering on the *presence of a `config` slice* rather than on the role. +This matters for Phase 4 (#2797), which would otherwise have been designed against a constraint that +does not exist — and it means the exclusivity invariant was, until now, unenforced for every +non-feature capability rather than merely unused. + +**2. D3's profile-membership claim is inverted.** + +D3 states that a `role: "reviewer"` capability "receives profile membership from +`deriveProfileMembership` (`gen-capability-registry.cjs:201-213`) like any other" and that the +membership is inert. It receives **no** membership: that function skips any capability without a +non-empty `skills` array, and a lane-only capability has none. The *intended outcome* — a reviewer +capability installs nothing — holds exactly as D3 wanted, and `tier` remains required as the source +of truth for the `requires`-closure. Only the stated mechanism was wrong, and a Phase 5a author +following D3 would have gone looking for membership that is not there. + +**3. The specified capability folder names for two lanes would fail the build.** + +The Scope-of-changes section and #2798 both name `capabilities/lm_studio/` and +`capabilities/llama_cpp/`. Both would be rejected: `id` must equal the folder name **and** match +`KEBAB_RE` (`/^[a-z][a-z0-9-]*$/`), which does not admit `_`. Three namespaces are in play for one +lane and they are deliberately not the same string: + +| | value | casing | fixed by | +|---|---|---|---| +| capability `id` / folder | `lm-studio` | kebab | the `id` conformance invariant | +| `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. diff --git a/gsd-core/bin/lib/capability-validator.cjs b/gsd-core/bin/lib/capability-validator.cjs index 2593966ca..b59106b01 100644 --- a/gsd-core/bin/lib/capability-validator.cjs +++ b/gsd-core/bin/lib/capability-validator.cjs @@ -173,7 +173,10 @@ function validateConfigSliceEntry(capId, key, slice) { // ─── Per-capability validation ──────────────────────────────────────────────── const KEBAB_RE = /^[a-z][a-z0-9-]*$/; -const VALID_ROLES = new Set(['feature', 'runtime']); +// ADR-2782 D3: a third role for reviewer lanes that are NOT install targets +// (gemini, coderabbit, ollama, lm_studio, llama_cpp). A `role: "reviewer"` +// capability carries a reviewer body, no runtime body, and no install surface. +const VALID_ROLES = new Set(['feature', 'runtime', 'reviewer']); const VALID_TIERS = new Set(['core', 'standard', 'full']); const VALID_ON_ERROR = new Set(['skip', 'halt']); const RUNTIME_COMPAT_WILDCARD = '*'; @@ -321,7 +324,7 @@ function validateCapability(cap, folderId) { } if (!VALID_ROLES.has(cap.role)) { - errors.push('role must be one of: feature, runtime (got: ' + cap.role + ')'); + errors.push('role must be one of: ' + [...VALID_ROLES].join(', ') + ' (got: ' + cap.role + ')'); } if (typeof cap.title !== 'string' || cap.title.length === 0) { @@ -354,8 +357,33 @@ function validateCapability(cap, folderId) { if (cap.role === 'feature') { errors.push(...validateFeatureBody(cap)); + // ADR-2782 D1 admits the reviewer body on `runtime` and `reviewer` ONLY. A + // feature capability owns loop artefacts, not an external review CLI. This is + // an ERROR rather than an ignored field because declaring a body is an + // ASSERTION of lane-ness (D4's Postel boundary), and a manifest built for a + // GSD that admits it declares `engines.gsd` and is gated before reaching here. + if (cap.reviewer !== undefined) { + errors.push('role:feature capability must not have a "reviewer" body (admissible on role runtime or reviewer only)'); + } } else if (cap.role === 'runtime') { errors.push(...validateRuntimeBody(cap)); + // A host that is ALSO a reviewer keeps exactly one manifest (ADR-2782 D1). + errors.push(...validateReviewerBody(cap)); + } else if (cap.role === 'reviewer') { + // ADR-2782 D3 — a lane that is not an install target. No runtime body, no + // install surface, no runtimeCompat (it surfaces through no host runtime). + if (cap.reviewer === undefined) { + errors.push('role:reviewer capability must have a "reviewer" body — the role asserts a lane'); + } + if (cap.runtime !== undefined) { + errors.push('role:reviewer capability must not have a "runtime" body (it is not an install target)'); + } + for (const field of FEATURE_FIELDS_FORBIDDEN_ON_REVIEWER) { + if (cap[field] !== undefined) { + errors.push('role:reviewer capability must not have "' + field + '" (feature-only field)'); + } + } + errors.push(...validateReviewerBody(cap)); } return errors; @@ -743,6 +771,85 @@ const VALID_EFFORT_SURFACES = new Set(['argv', 'none']); // same-wave executors — a dispatch sub-field, not a top-level axis. const VALID_DISPATCH_ISOLATION = new Set(['harness-worktree', 'orchestrator-worktree', 'none']); +// ─── Reviewer lane body (ADR-2782 D1/D2/D3/D7/D8) ──────────────────────────── +// +// A reviewer lane is one external CLI or model endpoint that /gsd:review hands a +// plan to. The body is admissible on `role: "runtime"` (a host that is ALSO a +// reviewer) and on `role: "reviewer"` (a lane that is not an install target). +// +// The vocabulary below tracks src/review-lane-descriptor.cts (Phase 1, #2794) +// field-for-field INCLUDING nesting, so no translation layer exists between the +// core descriptor and the manifest. Four members were added by Phase 1's ADR +// amendment, each forced by a lane that ships today: `promptChannel: 'none'` +// (CodeRabbit is fed no prompt), `outputChannel: 'file-arg'` + `outputArg` +// (Codex writes via -o and discards stdout, #1698), and `flags[]` (Antigravity +// answers to both --antigravity and --agy). +// +// EVERY error message below enumerates its valid members. That is deliberate: +// the prose reference lands in Phase 6 (#2800), so until then the validator's +// own errors ARE the documentation — the same gap that left `hostBehaviors` +// discoverable only by grepping a source line is not repeated here. + +// A slug is NOT a capability id. Ids are kebab (KEBAB_RE, which rejects "_"); +// slugs carry the shipped roster's snake forms (`lm_studio`, `llama_cpp`) and +// name the config keys (`review.lm_studio_host`) that ADR-2782 D9 leaves +// unchanged. Reusing KEBAB_RE here would reject two shipped lanes. +// +// ⚠ DEFECT.GENERATIVE-FIX — this grammar is DUPLICATED, by necessity, from +// `LANE_SLUG_RE` in src/review-lane-descriptor.cts (Phase 1, #2794). It cannot be +// imported: that module compiles to gsd-core/bin/lib/review-lane-descriptor.cjs, +// which is gitignored build output, and THIS file is a committed plain .cjs that +// must load on a fresh worktree before `npm run build:lib` has ever run (see the +// header). Two surfaces sharing one parser therefore require a parity assertion +// that fails when they diverge — `laneSlugGrammarMatchesPhase1Descriptor` in +// tests/reviewer-manifest-body.test.cjs. +// +// A LEADING DIGIT IS PERMITTED. Phase 1 allows it and a manifest validator that +// did not would reject a slug the core descriptor accepts — a model-named lane +// such as `4o-mini` — which is exactly the translation layer ADR-2782 exists to +// delete. Keep the two grammars byte-identical. +const LANE_SLUG_RE = /^[a-z0-9][a-z0-9_-]*$/; +// Flags are kebab even when the slug is snake: `lm_studio` → `--lm-studio`. +// Phase 1 declares flags but does not constrain their grammar, so this is the +// first and only definition — no parity partner to track. +const LANE_FLAG_RE = /^--[a-z0-9][a-z0-9-]*$/; + +const VALID_LANE_TRANSPORTS = new Set(['spawn', 'openai-http']); +const VALID_LANE_PROBE_KINDS = new Set(['command-exists', 'command-capability', 'http-reachable']); +const VALID_PROMPT_CHANNELS = new Set(['stdin', 'argv', 'argv-file-ref', 'none']); +const VALID_OUTPUT_CHANNELS = new Set(['stdout', 'file-arg']); +const VALID_LANE_EFFORT_CHANNELS = new Set(['none', 'argv', 'env']); +const VALID_MODEL_DISCOVERY = new Set(['none', 'first-from-models-endpoint']); +const VALID_EMPTY_OUTPUT = new Set(['stub-with-stderr', 'handler-owned']); +const VALID_EVIDENCE_CLASSES = new Set(['source-grounded', 'diff-only']); + +// ADR-2782 D6 — a CLOSED enum of FIRST-PARTY module names, never a path and +// never third-party code. `null` is the default and covers 7 of the 11 shipped +// lanes; it is checked separately because a Set cannot hold the "declared +// absent" case distinctly from an unknown string. +// +// ADMISSION RULE for a new member — this enum is the pressure valve that keeps +// the descriptor from becoming an ad-hoc interpreter, and it only works while +// membership stays scarce. A new member requires EITHER >=2 lanes that share the +// behaviour, OR a documented upstream defect that data provably cannot express. +// Today: `openai-compatible` serves 3 lanes (healthy); `antigravity` serves 1, +// justified solely by a documented upstream stdout bug. An enum that grows one +// member per lane has stopped being a vocabulary and become a dispatch table for +// bespoke code — at which point the descriptor is a plugin system wearing a +// manifest, and that is a decision for an ADR, not for a downstream phase. +const VALID_LANE_HANDLERS = new Set(['antigravity', 'openai-compatible']); + +// D2 — `transport` selects the invoke sub-shape. A manifest carrying fields from +// BOTH sub-shapes, or from NEITHER, has undefined meaning and fails validation. +// The discriminator is explicit rather than inferred from field presence, which +// is precisely the ambiguity these two sets exist to detect. +const SPAWN_ONLY_INVOKE_FIELDS = ['binary', 'args', 'promptChannel', 'outputChannel', 'outputArg', 'modelArg']; +const HTTP_ONLY_INVOKE_FIELDS = ['hostConfigKey', 'path', 'modelDiscovery']; + +// Feature-only fields are as forbidden on a lane-only capability as on a runtime +// one; a `role: "reviewer"` capability owns no artefacts and wires no loop point. +const FEATURE_FIELDS_FORBIDDEN_ON_REVIEWER = ['skills', 'agents', 'steps', 'contributions', 'gates', 'hooks', 'activationKey']; + // GATE A: installSurface → allowed hooksSurface values (DEFECT.GENERATIVE-FIX: parity invariant) // Derived from the actual pairings in the 16 real runtime descriptors. const INSTALL_SURFACE_TO_ALLOWED_HOOKS_SURFACES = new Map([ @@ -1430,6 +1537,503 @@ function validateRuntimeBody(cap) { return errors; } +/** + * ADR-2782 D2/D7 — every declared reviewer field is a known key. Anything else + * inside the body is IGNORED WITH A WARNING (D4.3), never a validation error: + * a capability authored for a newer GSD must degrade to discovered-but-inactive + * rather than failing the build of a repo that merely reads it. + */ +const KNOWN_REVIEWER_FIELDS = new Set([ + 'slug', 'flags', 'transport', 'probe', 'invoke', 'timeoutFloorMs', 'emptyOutput', + 'reviewsSection', 'evidenceClass', 'requiresBinaries', 'promptBudgetKey', 'handler', +]); + +const KNOWN_PROBE_FIELDS = new Set(['kind', 'binary', 'needle', 'timeoutMs', 'hostConfigKey', 'path']); + +/** A bounded probe timeout must be a finite, positive INTEGER of milliseconds. */ +function isPositiveIntegerMs(v) { + return typeof v === 'number' && Number.isInteger(v) && v > 0; +} + +/** + * Render any value for an error message WITHOUT ever throwing. + * + * `JSON.stringify` throws on a BigInt and on a circular structure, and a value + * carrying a throwing `toJSON` propagates that throw. Interpolating a rejected + * value into its own rejection message must never itself become the failure — + * these validators are contracted to RETURN errors, and #1461 OVL-1 records a + * validator that threw and would have crashed every consumer of loadRegistry. + * + * @param {*} v + * @returns {string} + */ +function describeValue(v) { + if (typeof v === 'bigint') return String(v) + 'n'; + if (typeof v === 'symbol') return String(v); + if (typeof v === 'function') return '[function]'; + try { + const json = JSON.stringify(v); + // stringify returns undefined for undefined and for non-serializable roots. + return json === undefined ? String(v) : json; + } catch { + // Circular structure, a nested BigInt, or a throwing toJSON. + try { + return Object.prototype.toString.call(v); + } catch { + return '[unserializable]'; + } + } +} + +/** Extract a message from an unknown thrown value without throwing again. */ +function safeErrorMessage(err) { + try { + if (err instanceof Error && typeof err.message === 'string') return err.message; + return describeValue(err); + } catch { + return '[unprintable error]'; + } +} + +/** House CodeQL barrier — inline literal guard at every key-derived read/write site. */ +function isReservedName(v) { + return v === '__proto__' || v === 'constructor' || v === 'prototype'; +} + +/** + * One closed-enum membership check, with the members always enumerated in the + * error. + * + * The enumeration is the point, not the deduplication. The prose reference for + * the reviewer body lands in Phase 6 (#2800), so until then these errors are the + * only documentation of the vocabulary — exactly the gap that left + * `hostBehaviors` discoverable solely by grepping a source line. Routing every + * enum through one helper makes "the error names the valid members" structural + * rather than a convention repeated at nine call sites, where it would drift. + * + * No reserved-name pre-check: a `VALID_*` set never contains `__proto__`, + * `constructor` or `prototype`, so membership alone already rejects them, and + * "must be one of: …" tells an author more than "is a reserved name". The + * literal guards stay where they do real work — the key-derived write sites. + * + * @param {string} ctx Error-message prefix. + * @param {string} label Dotted field path, e.g. "reviewer.transport". + * @param {*} value The declared value. + * @param {Set} validSet The closed vocabulary. + * @returns {string[]} + */ +function validateEnumField(ctx, label, value, validSet) { + if (validSet.has(value)) return []; + return [ + ctx + ' ' + label + ' must be one of: ' + [...validSet].join(', ') + + ' (got: ' + describeValue(value) + ')', + ]; +} + +/** + * Collect NON-FATAL diagnostics for a reviewer body (ADR-2782 D4.3). + * + * Kept separate from validateReviewerBody so validateCapability's contract + * (`=> string[]` of ERRORS) is unchanged for its two existing callers. The + * build-time generator writes these to stderr; the overlay loader surfaces them + * through OverlayMeta.warnings. A warning written only to a build log nobody + * reads is not a warning (ADR-2782 D4, "Where warnings surface"). + * + * @param {object} cap A capability manifest. + * @returns {string[]} Warning strings; empty when there is nothing to say. + */ +function collectReviewerWarnings(cap) { + // Same totality contract as validateReviewerBody, for the same reason: this is + // called from loadRegistry's accept path, and a diagnostic that throws must + // never cost the user a working lane. + try { + return collectReviewerWarningFields(cap); + } catch { + return []; + } +} + +function collectReviewerWarningFields(cap) { + const warnings = []; + if (typeof cap !== 'object' || cap === null || Array.isArray(cap)) return warnings; + const r = cap.reviewer; + if (typeof r !== 'object' || r === null || Array.isArray(r)) return warnings; + + const capId = typeof cap.id === 'string' ? cap.id : '(unknown)'; + for (const key of Object.keys(r)) { + if (isReservedName(key) || KNOWN_REVIEWER_FIELDS.has(key)) continue; + warnings.push( + '⚠ capability "' + capId + '" reviewer.' + key + ' is not a known reviewer field ' + + 'in this GSD version — ignored. Known fields: ' + [...KNOWN_REVIEWER_FIELDS].join(', '), + ); + } + return warnings; +} + +/** + * Validate a `reviewer` lane body (ADR-2782 D1/D2/D3/D6/D7). + * + * TOTAL: returns an array of error strings for ANY input and never throws. The + * overlay loader contracts every validator to RETURN errors — #1461 OVL-1 + * records a validator that THREW and would have crashed every consumer of + * loadRegistry. That contract is load-bearing, not stylistic. + * + * ABSENT-SAFE (D4.1): a capability with no `reviewer` key is simply not a lane. + * That is NEVER an error — 39 of 39 shipped capabilities are in this state, and + * a validator that errors here breaks the entire registry. `undefined` is the + * ONLY permissive case: `null`, `{}`, `[]`, `false` and `0` are all assertions + * of a body, and a malformed assertion is an error (Postel's Law with a + * boundary — liberal in what a manifest may OMIT, strict in what it ASSERTS). + * + * Totality is enforced STRUCTURALLY by the wrapper below, not by auditing every + * field read. Serialization is made safe via describeValue(), but that alone is + * not enough: a value carrying a throwing getter, or a Proxy with a throwing + * `get`/`ownKeys` trap, throws on the READ itself, before any message is built. + * A caller cannot be asked to re-derive today's reachability analysis — the + * contract says "any input", so the guarantee is absolute rather than argued. + * + * @param {object} cap The parsed capability manifest. + * @returns {string[]} Array of error strings; empty = valid. + */ +function validateReviewerBody(cap) { + try { + return validateReviewerBodyFields(cap); + } catch (err) { + // A malformed body must degrade to a validation ERROR, never to a crash of + // every consumer of loadRegistry (#1461 OVL-1). + return ['capability reviewer body could not be validated: ' + safeErrorMessage(err)]; + } +} + +function validateReviewerBodyFields(cap) { + const errors = []; + if (typeof cap !== 'object' || cap === null || Array.isArray(cap)) return errors; + + const r = cap.reviewer; + if (r === undefined) return errors; // D4.1 — not a lane. Never an error. + + const ctx = 'capability "' + (typeof cap.id === 'string' ? cap.id : '(unknown)') + '"'; + + if (typeof r !== 'object' || r === null || Array.isArray(r)) { + const got = r === null ? 'null' : Array.isArray(r) ? 'array' : typeof r; + errors.push( + ctx + ' reviewer must be an object (got: ' + got + '). ' + + 'Omit the key entirely to declare no lane — an explicit null is not an omission.', + ); + return errors; // cannot validate fields of a non-object + } + + // ── slug ─────────────────────────────────────────────────────────────────── + // NOT a capability id: ids are kebab, slugs carry the roster's snake forms. + if (typeof r.slug !== 'string' || r.slug.length === 0) { + errors.push(ctx + ' reviewer.slug must be a non-empty string'); + } else if (isReservedName(r.slug)) { + errors.push(ctx + ' reviewer.slug "' + r.slug + '" is a reserved name'); + } else if (!LANE_SLUG_RE.test(r.slug)) { + errors.push( + ctx + ' reviewer.slug "' + r.slug + '" must match ' + String(LANE_SLUG_RE) + + ' (lower-case; "_" and "-" permitted — a slug is not a capability id and not a flag)', + ); + } + + // ── flags ────────────────────────────────────────────────────────────────── + if (!Array.isArray(r.flags)) { + errors.push(ctx + ' reviewer.flags must be an array of CLI flags'); + } else if (r.flags.length === 0) { + errors.push( + ctx + ' reviewer.flags must declare at least one flag — a lane nobody can name ' + + 'cannot be explicitly selected, so ADR-2782 D4\'s explicit-selection rule is unreachable for it', + ); + } else { + const seen = new Set(); + for (const flag of r.flags) { + if (typeof flag !== 'string' || !LANE_FLAG_RE.test(flag)) { + errors.push( + ctx + ' reviewer.flags entry ' + describeValue(flag) + + ' must match ' + String(LANE_FLAG_RE) + ' (e.g. "--lm-studio")', + ); + continue; + } + if (seen.has(flag)) { + errors.push(ctx + ' reviewer.flags lists "' + flag + '" more than once'); + } + seen.add(flag); + } + } + + // ── transport (D2) — explicit discriminator, never inferred ──────────────── + errors.push(...validateEnumField(ctx, 'reviewer.transport', r.transport, VALID_LANE_TRANSPORTS)); + + errors.push(...validateLaneProbe(ctx, r.probe)); + errors.push(...validateLaneInvoke(ctx, r.transport, r.invoke)); + + // ── lane scalars ─────────────────────────────────────────────────────────── + if (!isPositiveIntegerMs(r.timeoutFloorMs)) { + errors.push( + ctx + ' reviewer.timeoutFloorMs must be a positive integer of milliseconds ' + + '(got: ' + describeValue(r.timeoutFloorMs) + ')', + ); + } + + errors.push(...validateEnumField(ctx, 'reviewer.emptyOutput', r.emptyOutput, VALID_EMPTY_OUTPUT)); + + if (typeof r.reviewsSection !== 'string' || r.reviewsSection.length === 0) { + errors.push(ctx + ' reviewer.reviewsSection must be a non-empty string'); + } + + errors.push(...validateEnumField(ctx, 'reviewer.evidenceClass', r.evidenceClass, VALID_EVIDENCE_CLASSES)); + + if (!Array.isArray(r.requiresBinaries)) { + errors.push( + ctx + ' reviewer.requiresBinaries must be an array (use [] when the lane needs no ' + + 'external tool on PATH). Note the name: the envelope\'s "requires" is capability ids', + ); + } else { + for (const bin of r.requiresBinaries) { + if (typeof bin !== 'string' || bin.length === 0) { + errors.push(ctx + ' reviewer.requiresBinaries entry ' + describeValue(bin) + ' must be a non-empty string'); + } + } + } + + // `null` is the declared "no per-lane budget"; an empty string is not. + if (r.promptBudgetKey !== null && (typeof r.promptBudgetKey !== 'string' || r.promptBudgetKey.length === 0)) { + errors.push( + ctx + ' reviewer.promptBudgetKey must be a dotted config key or null ' + + '(got: ' + describeValue(r.promptBudgetKey) + ')', + ); + } + + // ── handler (D6) — closed first-party enum; null is the default ──────────── + if (r.handler !== null && !VALID_LANE_HANDLERS.has(r.handler)) { + errors.push( + ctx + ' reviewer.handler must be null or one of: ' + [...VALID_LANE_HANDLERS].join(', ') + + ' (got: ' + describeValue(r.handler) + '). Handlers are first-party module NAMES, ' + + 'never paths and never third-party code — a lane needing a shape the vocabulary lacks ' + + 'files an issue naming the missing primitive (ADR-2782 D6)', + ); + } + + return errors; +} + +/** + * ADR-2782 D7 — probe.kind is a closed enum WIDER than existence, and every + * probe that starts a process or a connection MUST be bounded. + * + * `command-exists` alone is structurally insufficient: `kimi` is claimed by both + * Kimi Code CLI and the legacy Python kimi-cli (a separate first-party runtime + * capability in this repo), so an existence-only probe registers the wrong tool. + * The unbounded form of that probe was a live instance of this repo's named + * Unbounded Subprocesses defect — it ran on EVERY /gsd:review invocation + * regardless of which flags were passed, so a binary waiting on a first-run auth + * prompt hung every future review, including reviews that never asked for it. + * + * @param {string} ctx Error-message prefix. + * @param {*} probe The probe value. + * @returns {string[]} + */ +function validateLaneProbe(ctx, probe) { + const errors = []; + + if (typeof probe !== 'object' || probe === null || Array.isArray(probe)) { + errors.push(ctx + ' reviewer.probe must be an object with a "kind" from: ' + [...VALID_LANE_PROBE_KINDS].join(', ')); + return errors; + } + + const kindErrors = validateEnumField(ctx, 'reviewer.probe.kind', probe.kind, VALID_LANE_PROBE_KINDS); + if (kindErrors.length > 0) { + errors.push(...kindErrors); + return errors; // sub-shape is meaningless without a known kind + } + + for (const key of Object.keys(probe)) { + if (!isReservedName(key) && !KNOWN_PROBE_FIELDS.has(key)) { + errors.push(ctx + ' reviewer.probe.' + key + ' is not a known probe field'); + } + } + + const needsBound = probe.kind === 'command-capability' || probe.kind === 'http-reachable'; + + if (probe.kind === 'command-exists' || probe.kind === 'command-capability') { + if (typeof probe.binary !== 'string' || probe.binary.length === 0) { + errors.push(ctx + ' reviewer.probe.binary must be a non-empty string for kind "' + probe.kind + '"'); + } + } else if (probe.binary !== undefined) { + errors.push(ctx + ' reviewer.probe.binary is not permitted for kind "' + probe.kind + '"'); + } + + if (probe.kind === 'command-capability') { + if (typeof probe.needle !== 'string' || probe.needle.length === 0) { + errors.push(ctx + ' reviewer.probe.needle must be a non-empty string for kind "command-capability"'); + } + } else if (probe.needle !== undefined) { + errors.push(ctx + ' reviewer.probe.needle is not permitted for kind "' + probe.kind + '"'); + } + + if (probe.kind === 'http-reachable') { + if (typeof probe.hostConfigKey !== 'string' || probe.hostConfigKey.length === 0) { + errors.push(ctx + ' reviewer.probe.hostConfigKey must be a non-empty string for kind "http-reachable"'); + } + if (typeof probe.path !== 'string' || probe.path.length === 0) { + errors.push(ctx + ' reviewer.probe.path must be a non-empty string for kind "http-reachable"'); + } + } else { + if (probe.hostConfigKey !== undefined) { + errors.push(ctx + ' reviewer.probe.hostConfigKey is not permitted for kind "' + probe.kind + '"'); + } + if (probe.path !== undefined) { + errors.push(ctx + ' reviewer.probe.path is not permitted for kind "' + probe.kind + '"'); + } + } + + if (needsBound) { + if (!isPositiveIntegerMs(probe.timeoutMs)) { + errors.push( + ctx + ' reviewer.probe.timeoutMs must be a positive integer of milliseconds for kind "' + + probe.kind + '" — an unbounded probe hangs every /gsd:review invocation ' + + '(got: ' + describeValue(probe.timeoutMs) + ')', + ); + } + } else if (probe.timeoutMs !== undefined) { + errors.push( + ctx + ' reviewer.probe.timeoutMs is not permitted for kind "command-exists" — ' + + 'no process is started, so there is nothing to bound', + ); + } + + return errors; +} + +/** + * ADR-2782 D2 — the invoke sub-shape is selected by `transport`. A manifest + * declaring fields from BOTH sub-shapes, or from NEITHER, fails validation: + * inference from field presence leaves those two cases carrying undefined + * meaning, which is exactly what a closed vocabulary exists to prevent. + * + * @param {string} ctx Error-message prefix. + * @param {*} transport The (already enum-checked) transport value. + * @param {*} invoke The invoke value. + * @returns {string[]} + */ +function validateLaneInvoke(ctx, transport, invoke) { + const errors = []; + + if (typeof invoke !== 'object' || invoke === null || Array.isArray(invoke)) { + errors.push(ctx + ' reviewer.invoke must be an object'); + return errors; + } + + const hasSpawnField = SPAWN_ONLY_INVOKE_FIELDS.some((f) => invoke[f] !== undefined); + const hasHttpField = HTTP_ONLY_INVOKE_FIELDS.some((f) => invoke[f] !== undefined); + + if (hasSpawnField && hasHttpField) { + errors.push( + ctx + ' reviewer.invoke mixes spawn-only fields (' + SPAWN_ONLY_INVOKE_FIELDS.join(', ') + + ') with openai-http-only fields (' + HTTP_ONLY_INVOKE_FIELDS.join(', ') + + ') — a lane is one transport or the other', + ); + } + + if (transport === 'spawn') { + for (const f of HTTP_ONLY_INVOKE_FIELDS) { + if (invoke[f] !== undefined) { + errors.push(ctx + ' reviewer.invoke.' + f + ' is not permitted for transport "spawn"'); + } + } + errors.push(...validateSpawnInvoke(ctx, invoke)); + } else if (transport === 'openai-http') { + for (const f of SPAWN_ONLY_INVOKE_FIELDS) { + if (invoke[f] !== undefined) { + errors.push(ctx + ' reviewer.invoke.' + f + ' is not permitted for transport "openai-http"'); + } + } + errors.push(...validateHttpInvoke(ctx, invoke)); + } + // transport already reported as invalid upstream — do not double-report here. + + return errors; +} + +function validateSpawnInvoke(ctx, invoke) { + const errors = []; + + if (typeof invoke.binary !== 'string' || invoke.binary.length === 0) { + errors.push(ctx + ' reviewer.invoke.binary must be a non-empty string for transport "spawn"'); + } + + if (!Array.isArray(invoke.args)) { + errors.push(ctx + ' reviewer.invoke.args must be an array (use [] when the lane takes no arguments)'); + } else { + for (const a of invoke.args) { + if (typeof a !== 'string') { + errors.push(ctx + ' reviewer.invoke.args entry ' + describeValue(a) + ' must be a string'); + } + } + } + + errors.push(...validateEnumField(ctx, 'reviewer.invoke.promptChannel', invoke.promptChannel, VALID_PROMPT_CHANNELS)); + + const outputChannelErrors = validateEnumField(ctx, 'reviewer.invoke.outputChannel', invoke.outputChannel, VALID_OUTPUT_CHANNELS); + if (outputChannelErrors.length > 0) { + errors.push(...outputChannelErrors); + } else if (invoke.outputChannel === 'file-arg') { + // Knowing the review lands in a file is useless without the argument naming it. + if (typeof invoke.outputArg !== 'string' || invoke.outputArg.length === 0) { + errors.push( + ctx + ' reviewer.invoke.outputArg is required (non-empty string) when outputChannel is "file-arg"', + ); + } + } else if (invoke.outputArg !== undefined) { + // Forbidden rather than ignored: a manifest carrying an outputArg it does not + // use is data a later reader may honour. + errors.push( + ctx + ' reviewer.invoke.outputArg is only permitted when outputChannel is "file-arg" ' + + '(got outputChannel: ' + describeValue(invoke.outputChannel) + ')', + ); + } + + // `null` declares "this lane accepts no model override". An empty string does not. + if (invoke.modelArg !== null && (typeof invoke.modelArg !== 'string' || invoke.modelArg.length === 0)) { + errors.push( + ctx + ' reviewer.invoke.modelArg must be a non-empty string or null ' + + '(got: ' + describeValue(invoke.modelArg) + ')', + ); + } + + errors.push(...validateEnumField(ctx, 'reviewer.invoke.effortChannel', invoke.effortChannel, VALID_LANE_EFFORT_CHANNELS)); + + return errors; +} + +function validateHttpInvoke(ctx, invoke) { + const errors = []; + + if (typeof invoke.hostConfigKey !== 'string' || invoke.hostConfigKey.length === 0) { + errors.push( + ctx + ' reviewer.invoke.hostConfigKey must be a non-empty dotted config key ' + + 'for transport "openai-http" (it names the config key holding the base URL)', + ); + } + + if (typeof invoke.path !== 'string' || invoke.path.length === 0) { + errors.push(ctx + ' reviewer.invoke.path must be a non-empty string for transport "openai-http" (e.g. "/v1/chat/completions")'); + } + + errors.push(...validateEnumField(ctx, 'reviewer.invoke.modelDiscovery', invoke.modelDiscovery, VALID_MODEL_DISCOVERY)); + + // D2 fixes effortChannel to 'none' for this transport — an HTTP lane has no + // argv to carry an effort flag and no env of its own. + if (invoke.effortChannel !== 'none') { + errors.push( + ctx + ' reviewer.invoke.effortChannel must be "none" for transport "openai-http" ' + + '(got: ' + describeValue(invoke.effortChannel) + ')', + ); + } + + return errors; +} + // #1459 CONVERGENCE finding 1(b) — GENEROUS DoS backstop on a (possibly project-plantable) hook // fragment file. A real fragment is a few KiB of markdown; 8 MiB is wildly more than any legitimate // fragment. The bounded reader refuses a non-regular (FIFO/device/symlink-to-nonregular) or oversized @@ -1991,10 +2595,19 @@ function validateCrossCapability(capMap, centralKeys) { } } - // Config key ownership: exclusive AND absent from central schema + // Config key ownership: exclusive AND absent from central schema. + // + // ADR-2782 D1/D9: the role filter was `role !== 'feature'`, which silently + // exempted every non-feature capability from ownership AND from the + // central-schema collision check — the reason reviewer config keys were + // stranded centrally. Ownership is a property of DECLARING a config slice, not + // of being a feature, so the filter is now purely on the slice's presence. + // Verified inert at introduction: no shipped capability declares `config` on a + // non-feature role, so this widening changes no existing key — it stops a + // latent silent drop and unblocks Phase 4 (#2797). const configKeyOwner = new Map(); // key → capId for (const [capId, cap] of capMap) { - if (cap.role !== 'feature' || typeof cap.config !== 'object' || cap.config === null) continue; + if (typeof cap.config !== 'object' || cap.config === null) continue; for (const key of Object.keys(cap.config)) { if (configKeyOwner.has(key)) { errors.push( @@ -2013,6 +2626,78 @@ function validateCrossCapability(capMap, centralKeys) { } } + // ── Reviewer lane uniqueness (ADR-2782 D8) ───────────────────────────────── + // + // slug, every flag, and reviewsSection are each unique across the MERGED + // first-party ∪ overlay set. reviewsSection uniqueness is not cosmetic: two + // lanes sharing a heading silently merge their output in REVIEWS.md, producing + // a review that appears to have consensus it does not have. + // + // This runs in validateCrossCapability rather than in the generator because + // BOTH callers reach it: the build-time generator over first-party, and + // capability-loader's loadRegistry over `acceptedMap` (first-party ∪ accepted + // overlays) per candidate. First-party is already in the map when an overlay + // candidate is added, so the OVERLAY is the collider that gets dropped — + // which is exactly D8's "first-party wins", with no provenance check here. + // + // Reviewer INSTANCES (review.reviewer_instances., ADR-1517) resolve + // THROUGH a lane and are not lanes; they never enter these sets. + const laneSlugClaims = new Map(); // slug → capId[] + const laneFlagClaims = new Map(); // flag → capId[] + const laneSectionClaims = new Map(); // reviewsSection → capId[] + + // Claims are ACCUMULATED and reported after the sweep, never reported on the + // second claimant. Reporting pairwise-on-collision looks equivalent and is not: + // with three lanes on one key it names whichever pair happened to arrive first, + // so the message text depends on Map insertion order — which is readdir order + // at build time and candidate order at load time. A cross-platform CI lane + // would then disagree with a local run about the text of the same failure. + // Accumulating makes the output a pure function of the input set for ANY N. + const claim = (claims, key, capId) => { + if (typeof key !== 'string' || key.length === 0) return; + if (isReservedName(key)) return; + let claimants = claims.get(key); + if (claimants === undefined) { + claimants = []; + claims.set(key, claimants); + } + if (!claimants.includes(capId)) claimants.push(capId); + }; + + for (const [capId, cap] of capMap) { + const r = cap.reviewer; + // A capability with no lane contributes to no uniqueness set. A MALFORMED + // body was already reported by validateCapability — do not double-report. + if (typeof r !== 'object' || r === null || Array.isArray(r)) continue; + claim(laneSlugClaims, r.slug, capId); + claim(laneSectionClaims, r.reviewsSection, capId); + if (Array.isArray(r.flags)) { + // Flattened across arrays: Antigravity answers to --antigravity AND --agy, + // so uniqueness is per-flag, not per-lane. + for (const flag of r.flags) claim(laneFlagClaims, flag, capId); + } + } + + // One error per colliding key naming EVERY claimant, ids sorted; the whole + // block is sorted before it is appended, so both the messages and their order + // are independent of how the capabilities were enumerated. + const laneCollisions = []; + for (const [claims, label] of [ + [laneSlugClaims, 'slug'], + [laneFlagClaims, 'flag'], + [laneSectionClaims, 'reviewsSection'], + ]) { + for (const [key, claimants] of claims) { + if (claimants.length < 2) continue; + laneCollisions.push( + 'reviewer ' + label + ' "' + key + '" is declared by ' + + [...claimants].sort().map((i) => '"' + i + '"').join(' and '), + ); + } + } + laneCollisions.sort(); + errors.push(...laneCollisions); + // requires: all ids exist for (const [capId, cap] of capMap) { if (!Array.isArray(cap.requires)) continue; @@ -2407,6 +3092,22 @@ module.exports = { VALID_ARTIFACT_KIND_NAMES, VALID_ARTIFACT_NESTINGS, FEATURE_FIELDS_FORBIDDEN_ON_RUNTIME, + // ADR-2782 D1/D2/D3/D6/D7/D8 — reviewer lane body + FEATURE_FIELDS_FORBIDDEN_ON_REVIEWER, + LANE_SLUG_RE, + LANE_FLAG_RE, + VALID_LANE_TRANSPORTS, + VALID_LANE_PROBE_KINDS, + VALID_PROMPT_CHANNELS, + VALID_OUTPUT_CHANNELS, + VALID_LANE_EFFORT_CHANNELS, + VALID_MODEL_DISCOVERY, + VALID_EMPTY_OUTPUT, + VALID_EVIDENCE_CLASSES, + VALID_LANE_HANDLERS, + KNOWN_REVIEWER_FIELDS, + validateReviewerBody, + collectReviewerWarnings, VALID_INSTALL_SURFACES, VALID_PERMISSION_WRITERS, VALID_EXTENDED_HOOK_EVENTS, diff --git a/scripts/gen-capability-registry.cjs b/scripts/gen-capability-registry.cjs index d16805058..db70d9756 100644 --- a/scripts/gen-capability-registry.cjs +++ b/scripts/gen-capability-registry.cjs @@ -71,6 +71,7 @@ const { validateArtifactKindEntry, validateArtifactLayout, validateRuntimeBody, + collectReviewerWarnings, materializeHookFragments, validateAgainstContract, validateConsumesGlobal, @@ -340,9 +341,13 @@ function loadAndValidate(centralKeys, capabilitiesDir) { const resolvedCapDir = capabilitiesDir !== undefined ? capabilitiesDir : CAPABILITIES_DIR; const errors = []; const capMap = new Map(); + // ADR-2782 D4 — non-fatal diagnostics (e.g. an unknown field inside a reviewer + // body). These NEVER fail the build; they surface on stderr so a forward-built + // manifest degrades visibly instead of silently. + const warnings = []; if (!fs.existsSync(resolvedCapDir)) { - return { capMap, errors }; + return { capMap, errors, warnings }; } // Compute wired points ONCE before iterating capabilities so the filesystem @@ -366,6 +371,10 @@ function loadAndValidate(centralKeys, capabilitiesDir) { continue; } + // Collected BEFORE the error short-circuit below so a manifest that is both + // forward-built and invalid still reports why it looked unfamiliar. + for (const w of collectReviewerWarnings(cap)) warnings.push(folderId + '/capability.json: ' + w); + const capErrors = validateCapability(cap, folderId); if (capErrors.length > 0) { for (const e of capErrors) errors.push(folderId + '/capability.json: ' + e); @@ -406,7 +415,7 @@ function loadAndValidate(centralKeys, capabilitiesDir) { const consumesErrors = validateConsumesGlobal(capMap); errors.push(...consumesErrors); - return { capMap, errors }; + return { capMap, errors, warnings }; } /** @@ -446,6 +455,46 @@ function buildRegistry(capMap) { if (capId === '__proto__' || capId === 'constructor' || capId === 'prototype') continue; capabilities[capId] = cap; + // Federated config slice — harvested from ANY role that declares one. + // + // ADR-2782 D1/D9: this loop was nested inside the `role === 'feature'` branch, + // so a `role: "runtime"` capability's `config` was read by nothing and dropped + // in silence — the actual reason reviewer config keys are stranded in the + // central schema. (The often-cited reason, that the runtime body forbids + // feature-only fields, does not apply: `config` is NOT in + // FEATURE_FIELDS_FORBIDDEN_ON_RUNTIME.) Owning a config slice is a property of + // DECLARING one, not of being a feature. Verified inert at introduction — no + // shipped capability declares `config` on a non-feature role — so this changes + // no existing key; it stops a latent silent drop and unblocks Phase 4 (#2797). + for (const key of Object.keys(cap.config || {})) { + // S2b: inline literal guard at each write site (CodeQL barrier) + if (key === '__proto__' || key === 'constructor' || key === 'prototype') continue; + configKeys[key] = capId; + + // Build configSchema entry — validate the slice first (throw on violation) + const slice = (cap.config || {})[key]; + const sliceErrors = validateConfigSliceEntry(capId, key, slice); + if (sliceErrors.length > 0) { + throw new Error( + 'configSchema validation failed during registry build:\n' + + sliceErrors.map((e) => ' ' + e).join('\n'), + ); + } + // S2b: inline literal guard for configSchema write site + if (key !== '__proto__' && key !== 'constructor' && key !== 'prototype') { + configSchema[key] = { + owner: capId, + type: slice.type, + default: slice.default, + description: slice.description, + }; + // Preserve values array for enum types if present + if (slice.type === 'enum' && Array.isArray(slice.values)) { + configSchema[key].values = slice.values; + } + } + } + if (cap.role === 'feature') { for (const skill of (cap.skills || [])) { // S2b: inline literal guard at each write site (CodeQL barrier) @@ -457,34 +506,6 @@ function buildRegistry(capMap) { if (agent === '__proto__' || agent === 'constructor' || agent === 'prototype') continue; byAgent[agent] = capId; } - for (const key of Object.keys(cap.config || {})) { - // S2b: inline literal guard at each write site (CodeQL barrier) - if (key === '__proto__' || key === 'constructor' || key === 'prototype') continue; - configKeys[key] = capId; - - // Build configSchema entry — validate the slice first (throw on violation) - const slice = (cap.config || {})[key]; - const sliceErrors = validateConfigSliceEntry(capId, key, slice); - if (sliceErrors.length > 0) { - throw new Error( - 'configSchema validation failed during registry build:\n' + - sliceErrors.map((e) => ' ' + e).join('\n'), - ); - } - // S2b: inline literal guard for configSchema write site - if (key !== '__proto__' && key !== 'constructor' && key !== 'prototype') { - configSchema[key] = { - owner: capId, - type: slice.type, - default: slice.default, - description: slice.description, - }; - // Preserve values array for enum types if present - if (slice.type === 'enum' && Array.isArray(slice.values)) { - configSchema[key].values = slice.values; - } - } - } for (const step of (cap.steps || [])) { if (VALID_LOOP_POINTS.has(step.point)) { @@ -741,7 +762,11 @@ function main() { if (flag === '--check') { // Fix #3: read the REAL central config keys so collision detection fires and is visible. const centralKeys = loadCentralConfigKeys(); - const { capMap, errors } = loadAndValidate(centralKeys); + const { capMap, errors, warnings } = loadAndValidate(centralKeys); + + // ADR-2782 D4 — non-fatal manifest diagnostics. Emitted BEFORE the hard-error + // exit so a forward-built manifest still explains itself on a failing build. + for (const w of warnings) process.stderr.write(w + '\n'); // Separate pending-migration warnings from hard errors const { hardErrors, pendingMigrationWarnings } = classifyCrossErrors(errors); @@ -778,7 +803,11 @@ function main() { } else if (flag === '--write') { // Fix #3: read the REAL central config keys so collision detection fires and is visible. const centralKeys = loadCentralConfigKeys(); - const { capMap, errors } = loadAndValidate(centralKeys); + const { capMap, errors, warnings } = loadAndValidate(centralKeys); + + // ADR-2782 D4 — non-fatal manifest diagnostics. Emitted BEFORE the hard-error + // exit so a forward-built manifest still explains itself on a failing build. + for (const w of warnings) process.stderr.write(w + '\n'); // Separate pending-migration warnings from hard errors const { hardErrors, pendingMigrationWarnings } = classifyCrossErrors(errors); diff --git a/src/capability-loader.cts b/src/capability-loader.cts index 2ba118ccc..7b469bab3 100644 --- a/src/capability-loader.cts +++ b/src/capability-loader.cts @@ -60,6 +60,14 @@ interface ValidatorModule { validateAgainstContract: (cap: unknown, capId: string) => string[]; validateConsumesGlobal: (capMap: Map) => string[]; validateCrossCapability: (capMap: Map, centralKeys: Set) => string[]; + /** + * ADR-2782 D4.3 — NON-FATAL diagnostics for an ACCEPTED capability (e.g. an + * unknown field inside a `reviewer` body, which is ignored with a warning + * rather than failing validation so a manifest built for a newer GSD degrades + * visibly instead of being rejected). Returns warnings; never throws. + * Optional so an older built validator without it still loads. + */ + collectReviewerWarnings?: (cap: unknown) => string[]; } interface SemverModule { semverSatisfies: (version: unknown, range: unknown) => boolean; @@ -138,6 +146,15 @@ export interface BlockedGate { export interface OverlayMeta { /** Capabilities skipped at load, with the reason (surfaced to the user). */ warnings: OverlaySkip[]; + /** + * ADR-2782 D4.3 — non-fatal diagnostics for capabilities that were ACCEPTED. + * Distinct from `warnings`, which records capabilities that were SKIPPED: a + * consumer that treats every `warnings` entry as "inactive" would mislabel an + * active capability if these were folded in. Today this carries unknown-field + * notices from a `reviewer` body built for a newer GSD — the case D4.3 exists + * for, which validates cleanly and so would otherwise surface nowhere at all. + */ + diagnostics: string[]; /** Skipped capabilities that declared a gate — the loop must fail CLOSED for these. */ incompatibleGateCapIds: string[]; /** @@ -508,6 +525,8 @@ export function loadRegistry(options: LoadRegistryOptions = {}): Registry { const gsdHome = options.gsdHome || process.env['GSD_HOME'] || os.homedir(); const warnings: OverlaySkip[] = []; + // ADR-2782 D4.3 — non-fatal notices for capabilities that are ACCEPTED (see OverlayMeta). + const diagnostics: string[] = []; const incompatibleGateCapIds: string[] = []; const blockedGates: BlockedGate[] = []; const commandRoots: Record = {}; @@ -790,6 +809,25 @@ export function loadRegistry(options: LoadRegistryOptions = {}): Registry { } // Accepted. + // + // ADR-2782 D4.3: an unknown field inside a `reviewer` body is IGNORED WITH A + // WARNING rather than failing validation, so a lane built for a newer GSD + // degrades to accepted-but-partially-understood instead of being rejected. + // That case validates cleanly, so without this call it would surface + // nowhere at runtime — the build-time generator only ever sees first-party + // in-repo manifests, never an installed third-party overlay. Guarded on + // presence so an older built validator without the function still loads, + // and wrapped because the never-crash contract (ADR-1244 D2) outranks a + // diagnostic: a throwing collector must not cost the user a working lane. + if (typeof validator.collectReviewerWarnings === 'function') { + try { + for (const w of validator.collectReviewerWarnings(cap) || []) { + diagnostics.push(`${root.scope}:${id}: ${w}`); + } + } catch { + // A diagnostic that cannot be produced is not worth failing an install over. + } + } overlayCaps.push(cap); acceptedIds.add(id); for (const s of skills) claimedSkills.add(s); @@ -818,7 +856,7 @@ export function loadRegistry(options: LoadRegistryOptions = {}): Registry { } } - const meta: OverlayMeta = { warnings, incompatibleGateCapIds, blockedGates, commandRoots }; + const meta: OverlayMeta = { warnings, diagnostics, incompatibleGateCapIds, blockedGates, commandRoots }; if (overlayCaps.length === 0) { // Nothing to compose. Return the frozen registry unchanged when there is diff --git a/tests/reviewer-manifest-body.test.cjs b/tests/reviewer-manifest-body.test.cjs new file mode 100644 index 000000000..27b4a30e8 --- /dev/null +++ b/tests/reviewer-manifest-body.test.cjs @@ -0,0 +1,1682 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +/** + * reviewer-manifest-body.test.cjs — behavioral tests for the reviewer lane body + * (ADR-2782, chore #2795 Phase 2): `validateReviewerBody`, `collectReviewerWarnings`, + * the `role:'reviewer'` dispatch branch of `validateCapability`, the reviewer-lane + * uniqueness rules inside `validateCrossCapability`, and the harvest widening in + * `buildRegistry` / `loadAndValidate`. + * + * Implements every row in `.gsd/phase/chore-2795-reviewer-manifest-body/50-test-matrix.md` + * that carries a Test name (sections A–J). Test names are copied verbatim from the + * matrix. See `.gsd/phase/chore-2795-reviewer-manifest-body/40-design.md` for the + * behavior table the matrix derives from. + * + * Level choice: rows describing the SHAPE of the `reviewer` body in isolation + * (A1–A3, A7–A11, and all of B–F) call `validateReviewerBody` directly — the + * cheapest unit that proves the behavior, per the matrix's own "Units and level" + * table. Rows describing ROLE-CONDITIONED admissibility of the body (A4–A6, + * A12–A15) call `validateCapability(cap, folderId)`, since that dispatch only + * exists there. Section G calls `validateCrossCapability(Map, Set)` — the real + * caller shape, never a plain object. Section H splits across `buildRegistry` + * (the harvest itself) and `validateCrossCapability` (the config-key ownership + * loop it also widened) per where each behavior actually lives in the source. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const fc = require('fast-check'); + +const { cleanup } = require('./helpers.cjs'); + +const { + LANE_SLUG_RE, + validateReviewerBody, + collectReviewerWarnings, + validateCapability, + validateCrossCapability, + VALID_LANE_EFFORT_CHANNELS, + VALID_MODEL_DISCOVERY, + VALID_EMPTY_OUTPUT, + VALID_EVIDENCE_CLASSES, + VALID_LANE_HANDLERS, + KNOWN_REVIEWER_FIELDS, +} = require('../gsd-core/bin/lib/capability-validator.cjs'); + +const { loadAndValidate, buildRegistry } = require('../scripts/gen-capability-registry.cjs'); + +// ─── Fixture builders ────────────────────────────────────────────────────── +// House convention (tests/gen-registry.test.cjs): builder functions return a +// VALID fixture, which each test then mutates. Every call returns a FRESH +// object — no module-level mutable shared state, no execution-order dependence. + +/** A valid `spawn`-transport reviewer lane body. */ +function validLane() { + return { + slug: 'my-lane', + flags: ['--my-lane'], + transport: 'spawn', + probe: { kind: 'command-exists', binary: 'my-lane' }, + invoke: { + binary: 'my-lane', + args: [], + promptChannel: 'stdin', + outputChannel: 'stdout', + modelArg: null, + effortChannel: 'none', + }, + timeoutFloorMs: 5000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'My Lane', + evidenceClass: 'source-grounded', + requiresBinaries: ['my-lane'], + promptBudgetKey: null, + handler: null, + }; +} + +/** A valid `openai-http`-transport reviewer lane body. */ +function validHttpLane() { + return { + slug: 'lm-studio-http', + flags: ['--lm-studio'], + transport: 'openai-http', + probe: { + kind: 'http-reachable', + hostConfigKey: 'lmStudio.baseUrl', + path: '/v1/models', + timeoutMs: 2000, + }, + invoke: { + hostConfigKey: 'lmStudio.baseUrl', + path: '/v1/chat/completions', + modelDiscovery: 'none', + effortChannel: 'none', + }, + timeoutFloorMs: 5000, + emptyOutput: 'stub-with-stderr', + reviewsSection: 'LM Studio', + evidenceClass: 'source-grounded', + requiresBinaries: [], + promptBudgetKey: null, + handler: null, + }; +} + +/** A fresh, well-formed spawn lane, mutated in place by `mutator` before return. */ +function laneOverride(mutator) { + const lane = validLane(); + mutator(lane); + return lane; +} + +/** A fresh, well-formed http lane, mutated in place by `mutator` before return. */ +function httpOverride(mutator) { + const lane = validHttpLane(); + mutator(lane); + return lane; +} + +/** + * A valid `role:'reviewer'` capability envelope (id/title/description/tier/ + * requires/version all satisfy `validateCapability`'s common envelope) carrying + * a well-formed lane body. `overrides` is shallow-merged last, so passing + * `{ reviewer: undefined }` represents "no reviewer key" (property present, + * value undefined — behaviorally identical to an absent key for every check + * in this file, and it is what `JSON.stringify` drops when a fixture is + * written to disk in section I). + */ +function capWith(overrides) { + return Object.assign( + { + id: 'test-cap', + role: 'reviewer', + title: 'Test Capability', + description: 'A test capability for the reviewer manifest body test suite.', + tier: 'standard', + requires: [], + version: '1.0.0', + reviewer: validLane(), + }, + overrides || {}, + ); +} + +// ─── A. Body presence / shape ────────────────────────────────────────────── + +describe('A. Body presence / shape', () => { + test('runtimeCapWithoutReviewerBodyIsValidAndNotALane', () => { + const errs = validateReviewerBody({ id: 'x', role: 'runtime' }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('featureCapWithoutReviewerBodyIsValid', () => { + const errs = validateReviewerBody({ id: 'x', role: 'feature' }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('runtimeCapMayCarryReviewerBody', () => { + const errs = validateReviewerBody({ id: 'x', role: 'runtime', reviewer: validLane() }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('reviewerRoleWithLaneBodyIsValid', () => { + const cap = capWith(); + const errs = validateCapability(cap, cap.id); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('reviewerRoleWithoutLaneBodyIsRejected', () => { + const cap = capWith({ reviewer: undefined }); + const errs = validateCapability(cap, cap.id); + assert.ok( + errs.some((e) => e.includes('role:reviewer capability must have a "reviewer" body')), + `expected the lane-ness assertion error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('featureRoleRejectsReviewerBody', () => { + const cap = capWith({ role: 'feature', reviewer: validLane() }); + const errs = validateCapability(cap, cap.id); + assert.ok( + errs.some((e) => e.includes('role:feature capability must not have a "reviewer" body')), + `expected the feature-forbids-reviewer-body error, got: ${JSON.stringify(errs)}`, + ); + }); + + // A7/A1 are the highest-consequence pair: `typeof null === 'object'`, so an + // explicit `null` must NOT be read as "absent" (A1), yet must still error (A7). + test('reviewerNullIsRejectedNotTreatedAsAbsent', () => { + const errs = validateReviewerBody({ id: 'x', reviewer: null }); + assert.ok( + errs.some((e) => e.includes('reviewer must be an object (got: null)')), + `expected a null-is-not-absent error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('reviewerBooleanIsRejected', () => { + const errs = validateReviewerBody({ id: 'x', reviewer: true }); + assert.ok( + errs.some((e) => e.includes('reviewer must be an object (got: boolean)')), + `expected a must-be-an-object error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('reviewerZeroIsRejected', () => { + // Falsy-but-present: 0 must not be misread as absent (only `undefined` is). + const errs = validateReviewerBody({ id: 'x', reviewer: 0 }); + assert.ok( + errs.some((e) => e.includes('reviewer must be an object (got: number)')), + `expected a must-be-an-object error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('reviewerStringIsRejected', () => { + const errs = validateReviewerBody({ id: 'x', reviewer: 'spawn' }); + assert.ok( + errs.some((e) => e.includes('reviewer must be an object (got: string)')), + `expected a must-be-an-object error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('reviewerArrayIsRejected', () => { + const errs = validateReviewerBody({ id: 'x', reviewer: [] }); + assert.ok( + errs.some((e) => e.includes('reviewer must be an object (got: array)')), + `expected an array-is-not-a-body error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('emptyReviewerBodyReportsAllMissingFields', () => { + const errs = validateReviewerBody({ id: 'x', reviewer: {} }); + const requiredFieldMarkers = [ + 'reviewer.slug', + 'reviewer.flags', + 'reviewer.transport', + 'reviewer.probe', + 'reviewer.invoke', + 'reviewer.timeoutFloorMs', + 'reviewer.emptyOutput', + 'reviewer.reviewsSection', + 'reviewer.evidenceClass', + 'reviewer.requiresBinaries', + 'reviewer.promptBudgetKey', + 'reviewer.handler', + ]; + for (const marker of requiredFieldMarkers) { + assert.ok( + errs.some((e) => e.includes(marker)), + `expected an error mentioning ${marker}, got: ${JSON.stringify(errs)}`, + ); + } + assert.equal( + errs.length, + requiredFieldMarkers.length, + `expected exactly one error per required field (not just the first), got: ${JSON.stringify(errs)}`, + ); + }); + + test('unknownFieldInsideReviewerBodyWarnsButValidates', () => { + const lane = validLane(); + lane.futureField = 'from-a-newer-gsd'; + const cap = { id: 'cap-x', reviewer: lane }; + + const errs = validateReviewerBody(cap); + assert.deepEqual(errs, [], `unknown field must not be a validation error, got: ${JSON.stringify(errs)}`); + + const warnings = collectReviewerWarnings(cap); + assert.ok( + warnings.some( + (w) => w.includes('cap-x') && w.includes('reviewer.futureField') + && w.includes([...KNOWN_REVIEWER_FIELDS].join(', ')), + ), + `expected a warning naming reviewer.futureField and the known-fields list, got: ${JSON.stringify(warnings)}`, + ); + }); + + test('unknownRoleIsRejectedWithEnumeratedMembers', () => { + const cap = capWith({ role: 'wat' }); + const errs = validateCapability(cap, cap.id); + assert.ok( + errs.some((e) => e.includes('role must be one of: feature, runtime, reviewer')), + `expected the role error to enumerate all three members, got: ${JSON.stringify(errs)}`, + ); + }); + + test('reviewerRoleDoesNotRequireRuntimeCompat', () => { + const cap = capWith(); + assert.equal(cap.runtimeCompat, undefined, 'fixture must not declare runtimeCompat'); + const errs = validateCapability(cap, cap.id); + assert.deepEqual(errs, [], `expected no errors (runtimeCompat is a runtime-only concern), got: ${JSON.stringify(errs)}`); + }); + + test('reviewerRoleRejectsRuntimeBody', () => { + const cap = capWith({ runtime: { configFormat: 'toml' } }); + const errs = validateCapability(cap, cap.id); + assert.ok( + errs.some((e) => e.includes('role:reviewer capability must not have a "runtime" body')), + `expected a no-runtime-body error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('reviewerRoleRejectsFeatureOnlyFields', () => { + const cap = capWith({ skills: ['s'], agents: ['a'], steps: [{ point: 'plan:pre' }] }); + const errs = validateCapability(cap, cap.id); + for (const field of ['skills', 'agents', 'steps']) { + assert.ok( + errs.some((e) => e.includes(`role:reviewer capability must not have "${field}"`)), + `expected a feature-only-field error for "${field}", got: ${JSON.stringify(errs)}`, + ); + } + }); +}); + +// ─── B. transport discriminator (D2) ─────────────────────────────────────── + +describe('B. transport discriminator (D2)', () => { + test('transportIsRequiredAndNeverInferred', () => { + const lane = laneOverride((l) => { delete l.transport; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.transport must be one of: spawn, openai-http')), + `expected a transport-required error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('spawnTransportAcceptsSpawnInvoke', () => { + const errs = validateReviewerBody({ id: 'x', reviewer: validLane() }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('httpTransportAcceptsHttpInvoke', () => { + const errs = validateReviewerBody({ id: 'x', reviewer: validHttpLane() }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('spawnTransportRejectsHttpOnlyInvokeFields', () => { + const lane = laneOverride((l) => { l.invoke.hostConfigKey = 'x.y'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.hostConfigKey is not permitted for transport "spawn"')), + `expected a forbidden-field error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('httpTransportRejectsSpawnOnlyInvokeFields', () => { + const lane = httpOverride((l) => { l.invoke.binary = 'lm-studio'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.binary is not permitted for transport "openai-http"')), + `expected a forbidden-field error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('invokeWithBothSubShapesIsRejected', () => { + const lane = laneOverride((l) => { l.invoke.hostConfigKey = 'x.y'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke mixes spawn-only fields') && e.includes('openai-http-only fields')), + `expected D2's exact both-subshapes error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('invokeWithNeitherSubShapeIsRejected', () => { + const lane = laneOverride((l) => { l.invoke = {}; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.binary must be a non-empty string for transport "spawn"')), + `expected the spawn sub-shape to be validated against an empty invoke, got: ${JSON.stringify(errs)}`, + ); + assert.ok( + !errs.some((e) => e.includes('mixes spawn-only fields')), + `neither-subshape must not ALSO report the mixed-subshape error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('unknownTransportEnumeratesValidMembers', () => { + const lane = laneOverride((l) => { l.transport = 'grpc'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.transport must be one of: spawn, openai-http') && e.includes('"grpc"')), + `expected an unknown-transport error enumerating valid members, got: ${JSON.stringify(errs)}`, + ); + }); + + test('invokeIsRequired', () => { + const lane = laneOverride((l) => { delete l.invoke; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke must be an object')), + `expected an invoke-required error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('invokeNullIsRejected', () => { + const lane = laneOverride((l) => { l.invoke = null; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke must be an object')), + `expected an invoke-must-be-object error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('invokeArrayIsRejected', () => { + const lane = laneOverride((l) => { l.invoke = []; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke must be an object')), + `expected an invoke-must-be-object error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('reservedNameAsTransportIsRejected', () => { + // A reserved JS name is rejected by closed-enum membership itself — a + // VALID_* set never contains __proto__/constructor/prototype, so no separate + // reserved-name branch is needed here and the enum error is the more useful + // one because it names the valid members. The inline literal guards stay + // where they do real work: the key-derived write sites. + for (const reserved of ['__proto__', 'constructor', 'prototype']) { + const lane = laneOverride((l) => { l.transport = reserved; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.transport must be one of: spawn, openai-http')), + `expected ${reserved} to be rejected by enum membership, got: ${JSON.stringify(errs)}`, + ); + } + }); +}); + +// ─── C. spawn invoke fields ───────────────────────────────────────────────── + +describe('C. spawn invoke fields', () => { + test('spawnBinaryIsRequired', () => { + const lane = laneOverride((l) => { delete l.invoke.binary; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.binary must be a non-empty string for transport "spawn"')), + `expected a binary-required error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('spawnEmptyBinaryIsRejected', () => { + const lane = laneOverride((l) => { l.invoke.binary = ''; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.binary must be a non-empty string for transport "spawn"')), + `expected an empty-binary error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('spawnArgsArrayIsRequired', () => { + const lane = laneOverride((l) => { delete l.invoke.args; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.args must be an array')), + `expected an args-required error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('spawnEmptyArgsArrayIsValid', () => { + const lane = laneOverride((l) => { l.invoke.args = []; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors (a lane may take no args), got: ${JSON.stringify(errs)}`); + }); + + test('spawnArgsRejectsNonStringElement', () => { + const lane = laneOverride((l) => { l.invoke.args = ['-p', 7]; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.args entry 7 must be a string')), + `expected a non-string-element error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('promptChannelStdinIsValid', () => { + const lane = laneOverride((l) => { l.invoke.promptChannel = 'stdin'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('promptChannelNoneIsValidPerAmendment', () => { + const lane = laneOverride((l) => { l.invoke.promptChannel = 'none'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors (A1 amendment), got: ${JSON.stringify(errs)}`); + }); + + test('promptChannelArgvFileRefIsValid', () => { + const lane = laneOverride((l) => { l.invoke.promptChannel = 'argv-file-ref'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + // Zero shipped lanes use promptChannel:'argv' — this row is its only coverage + // until Phase 5b ships one (40-design.md C6 / Known Limits). + test('promptChannelArgvIsValidDespiteNoShippedLane', () => { + const lane = laneOverride((l) => { l.invoke.promptChannel = 'argv'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('outputChannelStdoutIsValid', () => { + const lane = laneOverride((l) => { l.invoke.outputChannel = 'stdout'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('fileArgOutputWithOutputArgIsValid', () => { + const lane = laneOverride((l) => { + l.invoke.outputChannel = 'file-arg'; + l.invoke.outputArg = 'out.txt'; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors (A2/A3 amendment), got: ${JSON.stringify(errs)}`); + }); + + test('fileArgOutputWithoutOutputArgIsRejected', () => { + const lane = laneOverride((l) => { l.invoke.outputChannel = 'file-arg'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.outputArg is required') && e.includes('"file-arg"')), + `expected a required-iff error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('stdoutOutputWithOutputArgIsRejected', () => { + const lane = laneOverride((l) => { + l.invoke.outputChannel = 'stdout'; + l.invoke.outputArg = 'out.txt'; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.outputArg is only permitted when outputChannel is "file-arg"')), + `expected a forbidden-field error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('modelArgNullMeansNoOverride', () => { + const lane = laneOverride((l) => { l.invoke.modelArg = null; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('modelArgEmptyStringIsRejected', () => { + const lane = laneOverride((l) => { l.invoke.modelArg = ''; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.modelArg must be a non-empty string or null')), + `expected an empty-string-is-not-none error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('effortChannelAcceptsAllThreeMembers', () => { + for (const effortChannel of VALID_LANE_EFFORT_CHANNELS) { + const lane = laneOverride((l) => { l.invoke.effortChannel = effortChannel; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `effortChannel=${effortChannel} expected no errors, got: ${JSON.stringify(errs)}`); + } + }); +}); + +// ─── D. openai-http invoke fields ────────────────────────────────────────── + +describe('D. openai-http invoke fields', () => { + test('httpHostConfigKeyIsRequired', () => { + const lane = httpOverride((l) => { delete l.invoke.hostConfigKey; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.hostConfigKey must be a non-empty dotted config key')), + `expected a hostConfigKey-required error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('httpEmptyHostConfigKeyIsRejected', () => { + const lane = httpOverride((l) => { l.invoke.hostConfigKey = ''; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.hostConfigKey must be a non-empty dotted config key')), + `expected an empty-hostConfigKey error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('httpPathIsRequiredAndNonEmpty', () => { + for (const badPath of [undefined, '']) { + const lane = httpOverride((l) => { + if (badPath === undefined) delete l.invoke.path; + else l.invoke.path = badPath; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.path must be a non-empty string for transport "openai-http"')), + `path=${JSON.stringify(badPath)}: expected a path-required error, got: ${JSON.stringify(errs)}`, + ); + } + }); + + test('modelDiscoveryAcceptsBothMembers', () => { + for (const modelDiscovery of VALID_MODEL_DISCOVERY) { + const lane = httpOverride((l) => { l.invoke.modelDiscovery = modelDiscovery; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `modelDiscovery=${modelDiscovery} expected no errors, got: ${JSON.stringify(errs)}`); + } + }); + + test('httpTransportRejectsNonNoneEffortChannel', () => { + const lane = httpOverride((l) => { l.invoke.effortChannel = 'argv'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.invoke.effortChannel must be "none" for transport "openai-http"')), + `expected an effortChannel-fixed-to-none error, got: ${JSON.stringify(errs)}`, + ); + }); +}); + +// ─── E. probe (D7) — bounded-probe control ───────────────────────────────── + +describe('E. probe (D7) — bounded-probe control', () => { + test('probeIsRequiredObject', () => { + const laneAbsent = laneOverride((l) => { delete l.probe; }); + const errsAbsent = validateReviewerBody({ id: 'x', reviewer: laneAbsent }); + assert.ok( + errsAbsent.some((e) => e.includes('reviewer.probe must be an object with a "kind" from:')), + `absent probe: expected a probe-required error, got: ${JSON.stringify(errsAbsent)}`, + ); + + const laneNonObject = laneOverride((l) => { l.probe = 'not-an-object'; }); + const errsNonObject = validateReviewerBody({ id: 'x', reviewer: laneNonObject }); + assert.ok( + errsNonObject.some((e) => e.includes('reviewer.probe must be an object with a "kind" from:')), + `non-object probe: expected a probe-required error, got: ${JSON.stringify(errsNonObject)}`, + ); + }); + + test('commandExistsProbeIsValid', () => { + const lane = laneOverride((l) => { l.probe = { kind: 'command-exists', binary: 'my-lane' }; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + // Zero shipped lanes use probe.kind:'command-capability' — this row is its + // only coverage until Phase 5b ships one. + test('commandCapabilityProbeIsValidDespiteNoShippedLane', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'command-capability', binary: 'my-lane', needle: 'v1.2', timeoutMs: 3000 }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('httpReachableProbeIsValid', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'http-reachable', hostConfigKey: 'lmStudio.baseUrl', path: '/v1/models', timeoutMs: 2000 }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('commandCapabilityProbeRequiresTimeout', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'command-capability', binary: 'my-lane', needle: 'v1.2' }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.probe.timeoutMs must be a positive integer of milliseconds for kind "command-capability"')), + `expected a timeout-required error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('httpReachableProbeRequiresTimeout', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'http-reachable', hostConfigKey: 'lmStudio.baseUrl', path: '/v1/models' }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.probe.timeoutMs must be a positive integer of milliseconds for kind "http-reachable"')), + `expected a timeout-required error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('probeTimeoutZeroIsRejected', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: 0 }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.probe.timeoutMs must be a positive integer of milliseconds') && e.includes('(got: 0)')), + `expected a boundary (limit-1) rejection, got: ${JSON.stringify(errs)}`, + ); + }); + + test('probeTimeoutOneIsAccepted', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: 1 }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors (boundary: limit), got: ${JSON.stringify(errs)}`); + }); + + test('probeNegativeTimeoutIsRejected', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: -1 }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.probe.timeoutMs must be a positive integer of milliseconds') && e.includes('(got: -1)')), + `expected a negative-timeout rejection, got: ${JSON.stringify(errs)}`, + ); + }); + + test('probeFractionalTimeoutIsRejected', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: 1.5 }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.probe.timeoutMs must be a positive integer of milliseconds') && e.includes('(got: 1.5)')), + `expected a fractional-timeout rejection (integer only), got: ${JSON.stringify(errs)}`, + ); + }); + + test('probeNumericStringTimeoutIsRejected', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: '900' }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.probe.timeoutMs must be a positive integer of milliseconds') && e.includes('(got: "900")')), + `expected a numeric-string rejection, got: ${JSON.stringify(errs)}`, + ); + }); + + test('probeNaNTimeoutIsRejected', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: NaN }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.probe.timeoutMs must be a positive integer of milliseconds')), + `expected a NaN rejection, got: ${JSON.stringify(errs)}`, + ); + }); + + test('probeInfiniteTimeoutIsRejected', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: Infinity }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + // Note: JSON.stringify(Infinity) renders as "null" in the message's "(got: …)" + // suffix — that's a property of JSON.stringify, not of this assertion; the + // load-bearing check is that Infinity (literally unbounded, the exact defect + // D7 exists to prevent) is rejected by the same branch as every other + // non-positive-integer value. + assert.ok( + errs.some((e) => e.includes('reviewer.probe.timeoutMs must be a positive integer of milliseconds for kind "http-reachable"')), + `expected an unbounded-timeout rejection, got: ${JSON.stringify(errs)}`, + ); + }); + + test('commandExistsProbeRejectsTimeout', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'command-exists', binary: 'my-lane', timeoutMs: 500 }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.probe.timeoutMs is not permitted for kind "command-exists"')), + `expected a forbidden-timeout error (no process is started), got: ${JSON.stringify(errs)}`, + ); + }); + + test('unknownProbeKindEnumeratesValidMembers', () => { + const lane = laneOverride((l) => { l.probe = { kind: 'nope' }; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.probe.kind must be one of: command-exists, command-capability, http-reachable')), + `expected an unknown-kind error enumerating all three kinds, got: ${JSON.stringify(errs)}`, + ); + }); + + test('commandCapabilityProbeRequiresNeedle', () => { + const lane = laneOverride((l) => { + l.probe = { kind: 'command-capability', binary: 'my-lane', timeoutMs: 3000 }; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.probe.needle must be a non-empty string for kind "command-capability"')), + `expected a needle-required error, got: ${JSON.stringify(errs)}`, + ); + }); +}); + +// ─── F. Lane scalars ──────────────────────────────────────────────────────── + +describe('F. Lane scalars', () => { + test('slugIsRequiredNonEmptyString', () => { + for (const badSlug of [undefined, '', 42]) { + const lane = laneOverride((l) => { + if (badSlug === undefined) delete l.slug; + else l.slug = badSlug; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.slug must be a non-empty string')), + `slug=${JSON.stringify(badSlug)}: expected a slug-required error, got: ${JSON.stringify(errs)}`, + ); + } + }); + + test('snakeCaseSlugIsAcceptedUnlikeCapabilityId', () => { + const lane = laneOverride((l) => { l.slug = 'lm_studio'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors (slug is not KEBAB_RE-checked), got: ${JSON.stringify(errs)}`); + }); + + // DEFECT.GENERATIVE-FIX parity assertion. The slug grammar exists in TWO places + // and cannot be reduced to one: Phase 1 owns it in src/review-lane-descriptor.cts, + // but that module compiles to gitignored build output, and the manifest validator + // is a committed plain .cjs that must load on a fresh worktree before build:lib. + // A divergence here silently reintroduces the translation layer ADR-2782 exists + // to delete — a slug the core descriptor accepts would be rejected by the + // manifest validator, and only for lanes nobody has shipped yet, so no other + // test would notice. This caught a real divergence in review: the validator + // required a leading LETTER while the descriptor allows a leading digit, which + // would have rejected a model-named lane such as `4o-mini`. + test('laneSlugGrammarMatchesPhase1Descriptor', () => { + // Built artifact — present after `npm run build:lib`, which CI runs before tests. + const descriptor = require('../gsd-core/bin/lib/review-lane-descriptor.cjs'); + assert.ok( + descriptor.LANE_SLUG_RE instanceof RegExp, + 'Phase 1 must export LANE_SLUG_RE; if it moved, this parity assertion needs updating, not deleting', + ); + assert.equal( + String(LANE_SLUG_RE), String(descriptor.LANE_SLUG_RE), + 'reviewer.slug grammar has diverged between the manifest validator and the Phase 1 core descriptor', + ); + + // Behavioural parity, not just source equality: the same inputs must get the + // same verdict from both surfaces. + for (const slug of ['gemini', 'lm_studio', 'llama_cpp', '4o-mini', '2b-local', 'kimi-code']) { + const lane = laneOverride((l) => { l.slug = slug; }); + const accepted = validateReviewerBody({ id: 'x', reviewer: lane }).length === 0; + assert.equal( + accepted, descriptor.LANE_SLUG_RE.test(slug), + `slug "${slug}": manifest validator and core descriptor disagree`, + ); + } + }); + + test('slugRejectsUppercase', () => { + const lane = laneOverride((l) => { l.slug = 'Lm-Studio'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.slug "Lm-Studio" must match')), + `expected an uppercase-rejection error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('slugRejectsWhitespace', () => { + const lane = laneOverride((l) => { l.slug = 'lm studio'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.slug "lm studio" must match')), + `expected a whitespace-rejection error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('slugRejectsFlagSyntax', () => { + const lane = laneOverride((l) => { l.slug = '--x'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.slug "--x" must match')), + `expected a flag-syntax-rejection error (a slug is not a flag), got: ${JSON.stringify(errs)}`, + ); + }); + + test('flagsArrayIsRequired', () => { + for (const badFlags of [undefined, 'not-an-array']) { + const lane = laneOverride((l) => { + if (badFlags === undefined) delete l.flags; + else l.flags = badFlags; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.flags must be an array of CLI flags')), + `flags=${JSON.stringify(badFlags)}: expected a flags-required error, got: ${JSON.stringify(errs)}`, + ); + } + }); + + test('emptyFlagsArrayIsRejected', () => { + const lane = laneOverride((l) => { l.flags = []; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.flags must declare at least one flag')), + `expected an unnameable-lane rejection, got: ${JSON.stringify(errs)}`, + ); + }); + + test('duplicateFlagWithinOneLaneIsRejected', () => { + const lane = laneOverride((l) => { l.flags = ['--x', '--x']; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.flags lists "--x" more than once')), + `expected a self-duplicate-flag error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('flagWithoutDoubleDashIsRejected', () => { + const lane = laneOverride((l) => { l.flags = ['x']; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.flags entry "x" must match')), + `expected a missing-double-dash error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('multipleFlagsPerLaneAreValidPerAmendment', () => { + const lane = laneOverride((l) => { l.flags = ['--antigravity', '--agy']; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors (A4 amendment), got: ${JSON.stringify(errs)}`); + }); + + test('timeoutFloorIsRequired', () => { + const lane = laneOverride((l) => { delete l.timeoutFloorMs; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.timeoutFloorMs must be a positive integer of milliseconds')), + `expected a timeoutFloorMs-required error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('timeoutFloorZeroIsRejected', () => { + const lane = laneOverride((l) => { l.timeoutFloorMs = 0; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.timeoutFloorMs must be a positive integer of milliseconds') && e.includes('(got: 0)')), + `expected a boundary (limit-1) rejection, got: ${JSON.stringify(errs)}`, + ); + }); + + test('timeoutFloorOneIsAccepted', () => { + const lane = laneOverride((l) => { l.timeoutFloorMs = 1; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors (boundary: limit), got: ${JSON.stringify(errs)}`); + }); + + test('emptyOutputAcceptsBothMembers', () => { + for (const emptyOutput of VALID_EMPTY_OUTPUT) { + const lane = laneOverride((l) => { l.emptyOutput = emptyOutput; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `emptyOutput=${emptyOutput} expected no errors, got: ${JSON.stringify(errs)}`); + } + }); + + test('reviewsSectionIsRequiredNonEmpty', () => { + for (const bad of [undefined, '']) { + const lane = laneOverride((l) => { + if (bad === undefined) delete l.reviewsSection; + else l.reviewsSection = bad; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.reviewsSection must be a non-empty string')), + `reviewsSection=${JSON.stringify(bad)}: expected a reviewsSection-required error, got: ${JSON.stringify(errs)}`, + ); + } + }); + + test('evidenceClassAcceptsBothMembers', () => { + for (const evidenceClass of VALID_EVIDENCE_CLASSES) { + const lane = laneOverride((l) => { l.evidenceClass = evidenceClass; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `evidenceClass=${evidenceClass} expected no errors, got: ${JSON.stringify(errs)}`); + } + }); + + test('requiresBinariesArrayIsRequired', () => { + for (const bad of [undefined, 'not-an-array']) { + const lane = laneOverride((l) => { + if (bad === undefined) delete l.requiresBinaries; + else l.requiresBinaries = bad; + }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.requiresBinaries must be an array')), + `requiresBinaries=${JSON.stringify(bad)}: expected a required-array error, got: ${JSON.stringify(errs)}`, + ); + } + }); + + test('emptyRequiresBinariesIsValid', () => { + const lane = laneOverride((l) => { l.requiresBinaries = []; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors (no external tool required), got: ${JSON.stringify(errs)}`); + }); + + test('promptBudgetKeyNullIsValid', () => { + const lane = laneOverride((l) => { l.promptBudgetKey = null; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('promptBudgetKeyEmptyStringIsRejected', () => { + const lane = laneOverride((l) => { l.promptBudgetKey = ''; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.promptBudgetKey must be a dotted config key or null') && e.includes('(got: "")')), + `expected an empty-string-is-not-none error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('handlerNullIsValidDefault', () => { + const lane = laneOverride((l) => { l.handler = null; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors (default), got: ${JSON.stringify(errs)}`); + }); + + test('handlerAcceptsEachFirstPartyMember', () => { + for (const handler of VALID_LANE_HANDLERS) { + const lane = laneOverride((l) => { l.handler = handler; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `handler=${handler} expected no errors, got: ${JSON.stringify(errs)}`); + } + }); + + test('unknownHandlerIsRejectedAtBuildTime', () => { + const lane = laneOverride((l) => { l.handler = 'acme'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.handler must be null or one of: antigravity, openai-compatible') && e.includes('"acme"')), + `expected an unknown-handler error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('pathTraversalHandlerIsRejected', () => { + const lane = laneOverride((l) => { l.handler = '../evil'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.handler must be null or one of: antigravity, openai-compatible') && e.includes('"../evil"')), + `expected a path-shaped-handler rejection, got: ${JSON.stringify(errs)}`, + ); + }); +}); + +// ─── G. Uniqueness (D8) — validateCrossCapability(Map, Set) ──────────────── + +describe('G. Uniqueness (D8) — validateCrossCapability(Map, Set)', () => { + // Each lane defaults to a slug/flags/reviewsSection derived from its own id, + // so distinct caps never accidentally collide — a test forces exactly ONE + // axis to collide via `reviewerOverrides`, isolating what it claims to test. + function laneCap(id, reviewerOverrides) { + return { + id, + role: 'reviewer', + reviewer: Object.assign( + validLane(), + { slug: `${id}-slug`, flags: [`--${id}`], reviewsSection: `Section for ${id}` }, + reviewerOverrides || {}, + ), + }; + } + + test('duplicateSlugAcrossCapabilitiesIsRejected', () => { + const capA = laneCap('cap-a'); + const capB = laneCap('cap-b', { slug: capA.reviewer.slug }); + const errs = validateCrossCapability(new Map([['cap-a', capA], ['cap-b', capB]]), new Set()); + assert.ok( + errs.some((e) => e.includes(`reviewer slug "${capA.reviewer.slug}" is declared by "cap-a" and "cap-b"`)), + `expected a slug-collision error naming both ids, got: ${JSON.stringify(errs)}`, + ); + }); + + // Regression guard for a real order-dependence defect in the first cut of the + // lane-uniqueness check. That version reported a collision when the SECOND + // claimant arrived, so with three lanes on one key it named whichever pair + // happened to arrive first: forward order blamed {cap-0,cap-1}+{cap-0,cap-2}, + // reversed blamed {cap-1,cap-2}+{cap-0,cap-2}. Map order is readdir order at + // build time and candidate order at load time, so a cross-platform CI lane + // could disagree with a local run about the text of the same failure. The + // pairwise case (G7) is order-independent either way, which is exactly why it + // did not catch this. Claims are now accumulated and reported after the sweep: + // ONE error per colliding key naming EVERY claimant, ids sorted. + test('threeWayCollisionIsReportedOnceAndOrderIndependently', () => { + const ids = ['cap-0', 'cap-1', 'cap-2']; + const caps = ids.map((id) => laneCap(id, { slug: 'shared-slug' })); + const laneErrs = (entries) => validateCrossCapability(new Map(entries), new Set()) + .filter((e) => e.startsWith('reviewer slug ')); + + const forward = laneErrs(ids.map((id, i) => [id, caps[i]])); + const reversed = laneErrs(ids.map((id, i) => [id, caps[i]]).reverse()); + + assert.equal( + forward.length, 1, + `a 3-way collision is ONE error naming all three, got: ${JSON.stringify(forward)}`, + ); + assert.deepEqual( + forward, reversed, + `error text must not depend on Map insertion order, got forward=${JSON.stringify(forward)} reversed=${JSON.stringify(reversed)}`, + ); + for (const id of ids) { + assert.ok(forward[0].includes(`"${id}"`), `every claimant must be named; ${id} missing from: ${forward[0]}`); + } + }); + + test('overlappingFlagsAcrossCapabilitiesAreRejected', () => { + const capA = laneCap('cap-a'); + const capB = laneCap('cap-b', { flags: [...capA.reviewer.flags] }); + const errs = validateCrossCapability(new Map([['cap-a', capA], ['cap-b', capB]]), new Set()); + assert.ok( + errs.some((e) => e.includes(`reviewer flag "${capA.reviewer.flags[0]}" is declared by "cap-a" and "cap-b"`)), + `expected a flag-collision error (flattened across arrays), got: ${JSON.stringify(errs)}`, + ); + }); + + test('duplicateReviewsSectionIsRejected', () => { + const capA = laneCap('cap-a'); + const capB = laneCap('cap-b', { reviewsSection: capA.reviewer.reviewsSection }); + const errs = validateCrossCapability(new Map([['cap-a', capA], ['cap-b', capB]]), new Set()); + assert.ok( + errs.some((e) => e.includes(`reviewer reviewsSection "${capA.reviewer.reviewsSection}" is declared by "cap-a" and "cap-b"`)), + `expected a reviewsSection-collision error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('reviewsSectionUniquenessIsCaseSensitiveByDesign', () => { + const capA = laneCap('cap-a'); + const capB = laneCap('cap-b', { reviewsSection: capA.reviewer.reviewsSection.toUpperCase() }); + const errs = validateCrossCapability(new Map([['cap-a', capA], ['cap-b', capB]]), new Set()); + assert.deepEqual( + errs, [], + `case-differing reviewsSection must NOT collide (Rejected #4, Known Limit), got: ${JSON.stringify(errs)}`, + ); + }); + + test('singleLaneHasNoUniquenessError', () => { + const capA = laneCap('cap-a'); + const errs = validateCrossCapability(new Map([['cap-a', capA]]), new Set()); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('capabilitiesWithoutLanesContributeNoUniquenessErrors', () => { + const capA = { id: 'cap-a', role: 'runtime' }; + const capB = { id: 'cap-b', role: 'runtime' }; + const errs = validateCrossCapability(new Map([['cap-a', capA], ['cap-b', capB]]), new Set()); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('nonLaneCapabilityIdMayMatchALaneSlug', () => { + const capA = laneCap('cap-a', { slug: 'shared-name' }); + const capB = { id: 'shared-name', role: 'runtime' }; + const errs = validateCrossCapability(new Map([['cap-a', capA], ['shared-name', capB]]), new Set()); + assert.deepEqual( + errs, [], + `a capability id matching a lane slug string is not a collision (instances/ids are not lanes), got: ${JSON.stringify(errs)}`, + ); + }); + + test('uniquenessErrorsAreOrderIndependent', () => { + const capA = laneCap('cap-a'); + const capB = laneCap('cap-b', { slug: capA.reviewer.slug }); + const forward = new Map([['cap-a', capA], ['cap-b', capB]]); + const reversed = new Map([['cap-b', capB], ['cap-a', capA]]); + const errsForward = validateCrossCapability(forward, new Set()); + const errsReversed = validateCrossCapability(reversed, new Set()); + assert.ok(errsForward.length > 0, `expected a real collision to exercise, got: ${JSON.stringify(errsForward)}`); + assert.deepEqual( + errsForward, errsReversed, + `error set must not depend on Map insertion order: forward=${JSON.stringify(errsForward)} reversed=${JSON.stringify(errsReversed)}`, + ); + }); + + test('threeLanesWithOneCollisionReportOnce', () => { + const capA = laneCap('cap-a'); + const capB = laneCap('cap-b', { slug: capA.reviewer.slug }); + const capC = laneCap('cap-c'); + const errs = validateCrossCapability( + new Map([['cap-a', capA], ['cap-b', capB], ['cap-c', capC]]), + new Set(), + ); + const slugErrors = errs.filter((e) => e.startsWith('reviewer slug')); + assert.equal(slugErrors.length, 1, `expected exactly one collision error, not two, got: ${JSON.stringify(errs)}`); + assert.equal(errs.length, 1, `expected no unrelated errors from the non-colliding third lane, got: ${JSON.stringify(errs)}`); + }); +}); + +// ─── H. Harvest widening — buildRegistry(capMap) ─────────────────────────── + +describe('H. Harvest widening — buildRegistry(capMap)', () => { + test('runtimeCapConfigIsHarvestedNotDropped', () => { + const capMap = new Map([['my-runtime', { + id: 'my-runtime', + role: 'runtime', + config: { 'myRuntime.enabled': { type: 'boolean', default: false, description: 'Enable my runtime feature.' } }, + }]]); + const registry = buildRegistry(capMap); + assert.equal(registry.configKeys['myRuntime.enabled'], 'my-runtime'); + assert.deepEqual(registry.configSchema['myRuntime.enabled'], { + owner: 'my-runtime', + type: 'boolean', + default: false, + description: 'Enable my runtime feature.', + }); + }); + + test('reviewerCapConfigIsHarvested', () => { + const capMap = new Map([['my-reviewer', { + id: 'my-reviewer', + role: 'reviewer', + config: { 'myReviewer.enabled': { type: 'boolean', default: true, description: 'Enable my reviewer lane.' } }, + }]]); + const registry = buildRegistry(capMap); + assert.equal(registry.configKeys['myReviewer.enabled'], 'my-reviewer'); + assert.deepEqual(registry.configSchema['myReviewer.enabled'], { + owner: 'my-reviewer', + type: 'boolean', + default: true, + description: 'Enable my reviewer lane.', + }); + }); + + test('runtimeCapWithoutConfigIsUnchanged', () => { + const cap = { id: 'my-runtime', role: 'runtime' }; + const capMap = new Map([['my-runtime', cap]]); + const registry = buildRegistry(capMap); + assert.equal(registry.runtimes['my-runtime'], cap); + // registry.configKeys is Object.create(null) (S2b prototype-pollution guard), + // so comparing against a plain `{}` literal would fail on prototype alone — + // compare via Object.keys() instead of asserting shape equality. + assert.deepEqual(Object.keys(registry.configKeys), []); + }); + + test('reviewerCapIsNotStoredAsRuntime', () => { + const cap = { id: 'my-reviewer', role: 'reviewer' }; + const capMap = new Map([['my-reviewer', cap]]); + const registry = buildRegistry(capMap); + assert.equal(registry.capabilities['my-reviewer'], cap); + assert.equal(registry.runtimes['my-reviewer'], undefined); + }); + + // Lives in validateCrossCapability's config-key ownership loop, not + // buildRegistry — that loop is the OTHER half of the harvest widening + // (40-design.md: "the role filter was `role !== 'feature'`"), so this row + // and H6 exercise validateCrossCapability directly rather than the harvest. + test('nonFeatureConfigKeyCollidingWithCentralSchemaIsFlagged', () => { + const capMap = new Map([['my-runtime', { + id: 'my-runtime', + role: 'runtime', + config: { 'shared.key': { type: 'string', default: 'x', description: 'y' } }, + }]]); + const errs = validateCrossCapability(capMap, new Set(['shared.key'])); + assert.ok( + errs.some((e) => e.includes('shared.key') && e.includes('central config-schema')), + `expected a central-schema collision error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('configKeyOwnedByTwoRolesIsRejected', () => { + const capMap = new Map([ + ['cap-a', { id: 'cap-a', role: 'runtime', config: { 'dup.key': { type: 'string', default: 'x', description: 'y' } } }], + ['cap-b', { id: 'cap-b', role: 'reviewer', config: { 'dup.key': { type: 'string', default: 'z', description: 'w' } } }], + ]); + const errs = validateCrossCapability(capMap, new Set()); + assert.ok( + errs.some((e) => e.includes('dup.key') && e.includes('owned by both "cap-a" and "cap-b"')), + `expected a two-role ownership-collision error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('shippedRegistryOutputIsUnchangedByHarvestWidening', () => { + const { capMap, errors } = loadAndValidate(new Set()); + assert.deepEqual(errors, [], `expected the real shipped capability set to validate cleanly, got: ${JSON.stringify(errors)}`); + + const registry = buildRegistry(capMap); + for (const [key, ownerId] of Object.entries(registry.configKeys)) { + const owner = capMap.get(ownerId); + assert.ok(owner, `configKeys owner "${ownerId}" for key "${key}" must exist in capMap`); + assert.equal( + owner.role, + 'feature', + `harvest widening must not change the shipped set: config key "${key}" is owned by non-feature ` + + `capability "${ownerId}" (role: ${owner.role}) — today only feature-role capabilities own config keys`, + ); + } + }); +}); + +// ─── I. Integration — loadAndValidate(centralKeys, tmpCapDir) ────────────── + +describe('I. Integration — loadAndValidate(centralKeys, tmpCapDir)', () => { + function withCapDir(foldersToContent, fn) { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-reviewer-body-')); + try { + for (const [folder, contentOrCap] of Object.entries(foldersToContent)) { + const dir = path.join(tmp, folder); + fs.mkdirSync(dir, { recursive: true }); + const content = typeof contentOrCap === 'string' ? contentOrCap : `${JSON.stringify(contentOrCap, null, 2)}\n`; + fs.writeFileSync(path.join(dir, 'capability.json'), content, 'utf8'); + } + return fn(tmp); + } finally { + cleanup(tmp); + } + } + + test('reviewerCapabilityLoadsFromDisk', () => { + const cap = capWith({ + id: 'lm-studio', + reviewer: Object.assign(validLane(), { slug: 'lm-studio', flags: ['--lm-studio'] }), + }); + withCapDir({ 'lm-studio': cap }, (tmp) => { + const { capMap, errors } = loadAndValidate(new Set(), tmp); + assert.deepEqual(errors, [], `expected no errors, got: ${JSON.stringify(errors)}`); + assert.ok(capMap.has('lm-studio')); + }); + }); + + test('malformedReviewerBodyReportsFolderId', () => { + const cap = capWith({ id: 'lm-studio', reviewer: {} }); + withCapDir({ 'lm-studio': cap }, (tmp) => { + const { errors } = loadAndValidate(new Set(), tmp); + assert.ok( + errors.some((e) => e.startsWith('lm-studio/capability.json:') && e.includes('reviewer.slug')), + `expected the folder id to prefix the reported error, got: ${JSON.stringify(errors)}`, + ); + }); + }); + + test('emptyCapabilityFileIsReportedNotCrashed', () => { + withCapDir({ 'lm-studio': '' }, (tmp) => { + let errors; + assert.doesNotThrow(() => { ({ errors } = loadAndValidate(new Set(), tmp)); }); + assert.ok( + errors.some((e) => e.includes('lm-studio/capability.json') && e.includes('JSON parse error')), + `expected a JSON-parse error naming the folder, got: ${JSON.stringify(errors)}`, + ); + }); + }); + + test('crlfCapabilityFileParsesIdentically', () => { + const cap = capWith({ + id: 'lm-studio', + reviewer: Object.assign(validLane(), { slug: 'lm-studio', flags: ['--lm-studio'] }), + }); + const lfContent = `${JSON.stringify(cap, null, 2)}\n`; + const crlfContent = lfContent.replace(/\n/g, '\r\n'); + withCapDir({ 'lm-studio': lfContent }, (tmpLf) => { + withCapDir({ 'lm-studio': crlfContent }, (tmpCrlf) => { + const lfResult = loadAndValidate(new Set(), tmpLf); + const crlfResult = loadAndValidate(new Set(), tmpCrlf); + assert.deepEqual(lfResult.errors, [], `LF variant should validate cleanly, got: ${JSON.stringify(lfResult.errors)}`); + assert.deepEqual(crlfResult.errors, [], `CRLF variant should validate cleanly, got: ${JSON.stringify(crlfResult.errors)}`); + assert.deepEqual( + crlfResult.capMap.get('lm-studio'), + lfResult.capMap.get('lm-studio'), + 'a CRLF-authored manifest must parse identically to its LF counterpart', + ); + }); + }); + }); + + test('unreadableCapabilityFileIsReportedNotCrashed', () => { + const cap = capWith({ + id: 'lm-studio', + reviewer: Object.assign(validLane(), { slug: 'lm-studio', flags: ['--lm-studio'] }), + }); + withCapDir({ 'lm-studio': cap }, (tmp) => { + const capPath = path.join(tmp, 'lm-studio', 'capability.json'); + // House rule: inject a deterministic IO failure by monkeypatching the fs + // method (save the original, override it to throw, restore in `finally`). + // NEVER `chmod 0o000` — root bypasses mode bits, so that trick silently + // passes with zero coverage in root Docker/CI. The override is scoped to + // the exact capability.json path under test and delegates every other + // read (e.g. loadAndValidate's own wired-loop-point scan) to the real + // implementation, so it fails only the read this test cares about. + const originalReadFileSync = fs.readFileSync; + let result; + try { + fs.readFileSync = (p, ...rest) => { + if (p === capPath) { + throw new Error('injected unreadable-file failure (simulated EACCES)'); + } + return originalReadFileSync.call(fs, p, ...rest); + }; + assert.doesNotThrow(() => { result = loadAndValidate(new Set(), tmp); }); + } finally { + fs.readFileSync = originalReadFileSync; + } + assert.ok( + result.errors.some((e) => e.includes('lm-studio') && e.includes('injected unreadable-file failure')), + `expected the unreadable file to be reported, not crashed, got: ${JSON.stringify(result.errors)}`, + ); + }); + }); + + test('snakeCaseIdIsRejectedEvenWhenSlugIsSnakeCase', () => { + // The Phase 5a trap: folder is kebab ("lm-studio") and reviewer.slug is + // snake ("lm_studio", accepted per F2) — but `id` itself must ALSO be kebab. + const badCap = capWith({ + id: 'lm_studio', + reviewer: Object.assign(validLane(), { slug: 'lm_studio', flags: ['--lm-studio'] }), + }); + withCapDir({ 'lm-studio': badCap }, (tmp) => { + const { errors } = loadAndValidate(new Set(), tmp); + assert.ok( + errors.some((e) => e.includes('lm-studio/capability.json') && e.includes('kebab-case')), + `expected a snake_case id to be rejected, got: ${JSON.stringify(errors)}`, + ); + }); + + const goodCap = capWith({ + id: 'lm-studio', + reviewer: Object.assign(validLane(), { slug: 'lm_studio', flags: ['--lm-studio'] }), + }); + withCapDir({ 'lm-studio': goodCap }, (tmp) => { + const { capMap, errors } = loadAndValidate(new Set(), tmp); + assert.deepEqual( + errors, [], + `a kebab id with a snake_case reviewer.slug must be accepted, got: ${JSON.stringify(errors)}`, + ); + assert.ok(capMap.has('lm-studio')); + }); + }); + + // ADR-2782 D4.3, LOAD-TIME half. The build-time generator only ever sees + // first-party in-repo manifests, so an unknown field on an INSTALLED + // third-party lane — the case D4.3 exists for — surfaced nowhere at runtime + // until the loader was wired to collect diagnostics. A global-scope overlay is + // used because project scope additionally requires a consent record; the + // subject here is the diagnostic channel, not the consent gate. + test('overlayLaneWithUnknownFieldIsAcceptedAndDiagnosed', () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-reviewer-overlay-')); + try { + const capDir = path.join(home, '.gsd', 'capabilities', 'acme-reviewer'); + fs.mkdirSync(capDir, { recursive: true }); + fs.writeFileSync(path.join(capDir, 'capability.json'), JSON.stringify({ + id: 'acme-reviewer', + role: 'reviewer', + version: '1.0.0', + title: 'Acme', + description: 'Third-party reviewer lane.', + tier: 'full', + requires: [], + engines: { gsd: '>=1.0.0' }, + reviewer: Object.assign(validLane(), { + slug: 'acme', + flags: ['--acme'], + reviewsSection: 'Acme', + // The field a NEWER GSD would understand and this one does not. + futureFieldFromNewerGsd: true, + }), + })); + + const { loadRegistry } = require('../gsd-core/bin/lib/capability-loader.cjs'); + const registry = loadRegistry({ + includeInstalled: true, cwd: process.cwd(), gsdHome: home, hostVersion: '1.8.0', + }); + + // Accepted, NOT skipped — an unknown field must never cost the user the lane. + assert.ok( + registry.capabilities && registry.capabilities['acme-reviewer'], + 'a third-party lane with an unknown reviewer field must still be accepted', + ); + const overlay = registry._overlay; + assert.ok(overlay, 'overlay meta must be attached when an overlay is composed'); + assert.deepEqual( + overlay.warnings, [], + `an unknown field is a diagnostic, not a skip, got: ${JSON.stringify(overlay.warnings)}`, + ); + assert.ok( + overlay.diagnostics.some((d) => d.includes('futureFieldFromNewerGsd')), + `expected a diagnostic naming the unknown field, got: ${JSON.stringify(overlay.diagnostics)}`, + ); + } finally { + cleanup(home); + } + }); +}); + +// ─── J. Property-based (fast-check) ──────────────────────────────────────── + +describe('J. Property-based (fast-check)', () => { + // The first version of this property used a bare `fc.anything()` as the whole + // reviewer value, and it was FALSE CONFIDENCE: at default constraints + // fc.anything() emits no BigInt, no circular reference, no getter and no + // Proxy — 20,000 sampled draws produced zero of each — which is precisely the + // value space that broke the contract. Worse, even enabling withBigInt is not + // enough under whole-value fuzzing, because the bug needs an exotic value in a + // SPECIFICALLY NAMED field and random key names essentially never land on one. + // So the generator is field-targeted, and the value space is widened by hand + // to include the shapes JSON cannot express but a JS caller can still pass. + const assertTotal = (cap, label) => { + let result; + try { + result = validateReviewerBody(cap); + } catch (err) { + assert.fail(`validateReviewerBody threw for ${label}: ${err && err.message}`); + } + assert.ok(Array.isArray(result), `must return an array for ${label}`); + assert.ok(result.every((e) => typeof e === 'string'), `every entry must be a string for ${label}`); + }; + + test('validateReviewerBodyNeverThrowsOnArbitraryInput', () => { + // Whole-value fuzzing — the original property, kept as the broad sweep. + fc.assert( + fc.property(fc.anything({ withBigInt: true }), (reviewerValue) => { + assertTotal({ id: 'prop-test', reviewer: reviewerValue }, 'whole-body fuzz'); + return true; + }), + { numRuns: 500 }, + ); + + // Field-targeted fuzzing — this is the variant that actually falsifies the + // pre-fix implementation, on roughly the first generated case. + const TARGET_FIELDS = [ + 'slug', 'flags', 'transport', 'probe', 'invoke', 'timeoutFloorMs', + 'emptyOutput', 'reviewsSection', 'evidenceClass', 'requiresBinaries', + 'promptBudgetKey', 'handler', + ]; + fc.assert( + fc.property( + fc.constantFrom(...TARGET_FIELDS), + fc.anything({ withBigInt: true }), + (field, value) => { + const lane = laneOverride((l) => { l[field] = value; }); + assertTotal({ id: 'prop-test', reviewer: lane }, `${field} = ${String(field)}`); + return true; + }, + ), + { numRuns: 1000 }, + ); + }); + + // The shapes fast-check cannot generate but a JS caller can still hand us. + // These are enumerated rather than fuzzed because a generator that produced + // them would be reimplementing the enumeration anyway. + test('validateReviewerBodyNeverThrowsOnValuesJsonCannotExpress', () => { + const circular = () => { const o = {}; o.self = o; return o; }; + const throwingGetter = () => { + const o = {}; + Object.defineProperty(o, 'valueOf', { get() { throw new Error('boom'); } }); + Object.defineProperty(o, 'toJSON', { get() { throw new Error('boom'); } }); + return o; + }; + const hostile = [ + ['bigint', 10n], + ['circular', circular()], + ['throwing-getter', throwingGetter()], + ['symbol', Symbol('s')], + ['function', () => {}], + ['null-prototype', Object.create(null)], + ]; + + for (const field of ['slug', 'transport', 'timeoutFloorMs', 'handler', 'promptBudgetKey', 'probe', 'invoke']) { + for (const [name, value] of hostile) { + const lane = laneOverride((l) => { l[field] = value; }); + assertTotal({ id: 'prop-test', reviewer: lane }, `${field} = ${name}`); + } + } + + // Array-element positions, which take a different code path from scalars. + for (const field of ['flags', 'requiresBinaries']) { + for (const [name, value] of hostile) { + const lane = laneOverride((l) => { l[field] = [value]; }); + assertTotal({ id: 'prop-test', reviewer: lane }, `${field}[0] = ${name}`); + } + } + const argsLane = laneOverride((l) => { l.invoke.args = [circular()]; }); + assertTotal({ id: 'prop-test', reviewer: argsLane }, 'invoke.args[0] = circular'); + + // Read-time throws: a getter or Proxy trap fires BEFORE any message is built, + // so describeValue() alone cannot save these — only the structural wrapper can. + assertTotal( + Object.defineProperty({ reviewer: {} }, 'id', { get() { throw new Error('boom'); } }), + 'throwing getter on cap.id', + ); + const slugThrows = laneOverride(() => {}); + Object.defineProperty(slugThrows, 'slug', { get() { throw new Error('boom'); } }); + assertTotal({ id: 'x', reviewer: slugThrows }, 'throwing getter on reviewer.slug'); + assertTotal({ id: 'x', reviewer: new Proxy({}, { get() { throw new Error('boom'); } }) }, 'Proxy get trap'); + assertTotal({ id: 'x', reviewer: new Proxy({}, { ownKeys() { throw new Error('boom'); } }) }, 'Proxy ownKeys trap'); + + // collectReviewerWarnings carries the same contract — it runs on + // loadRegistry's ACCEPT path, so a throwing diagnostic would cost a user a + // lane that is otherwise perfectly valid. + for (const cap of [ + Object.defineProperty({ reviewer: {} }, 'id', { get() { throw new Error('boom'); } }), + { id: 'x', reviewer: new Proxy({}, { ownKeys() { throw new Error('boom'); } }) }, + ]) { + let warnings; + try { + warnings = collectReviewerWarnings(cap); + } catch (err) { + assert.fail(`collectReviewerWarnings threw: ${err && err.message}`); + } + assert.ok(Array.isArray(warnings), 'collectReviewerWarnings must always return an array'); + } + }); + + test('uniquenessIsInvariantUnderMapOrder', () => { + // Groups of ARBITRARY size (1..4 capabilities sharing one slug), so N-way + // collisions are exercised, not just pairwise ones. An earlier cut of the + // uniqueness check reported a collision the moment a second claimant arrived, + // which named a different pair depending on insertion order once three lanes + // shared a key; claims are now accumulated and reported after the sweep, so + // the guarantee holds for any N. Errors are compared UNSORTED and filtered to + // the lane errors, so this asserts deterministic emission ORDER too — sorting + // both sides first would hide exactly the defect this guards. + fc.assert( + fc.property( + fc.array(fc.integer({ min: 1, max: 4 }), { minLength: 1, maxLength: 4 }), + fc.array(fc.integer({ min: 0, max: 999 }), { minLength: 0, maxLength: 8 }), + (groupSizes, shuffleSeed) => { + const entries = []; + groupSizes.forEach((size, groupIdx) => { + for (let member = 0; member < size; member += 1) { + const id = `cap-${groupIdx}-${member}`; + entries.push([id, { + id, + role: 'reviewer', + // Every member of a group shares one slug; flags and sections stay + // distinct so the slug is the only colliding dimension. + reviewer: Object.assign(validLane(), { + slug: `group-slug-${groupIdx}`, + flags: [`--${id}`], + reviewsSection: `Section ${id}`, + }), + }]); + } + }); + + const laneErrors = (ordered) => validateCrossCapability(new Map(ordered), new Set()) + .filter((e) => e.startsWith('reviewer ')); + + // A deterministic permutation derived from the generated seed, so the + // property covers arbitrary orderings rather than only forward/reverse. + const shuffled = [...entries]; + shuffleSeed.forEach((n, i) => { + const j = n % shuffled.length; + const k = (i + n) % shuffled.length; + [shuffled[j], shuffled[k]] = [shuffled[k], shuffled[j]]; + }); + + const forward = laneErrors(entries); + const reversed = laneErrors([...entries].reverse()); + const permuted = laneErrors(shuffled); + + assert.deepEqual(reversed, forward, 'lane errors must not depend on Map insertion order (reversed)'); + assert.deepEqual(permuted, forward, 'lane errors must not depend on Map insertion order (permuted)'); + + // One error per colliding group, naming every claimant. + const expectedCollisions = groupSizes.filter((s) => s > 1).length; + assert.equal( + forward.length, expectedCollisions, + `expected one error per colliding group, got: ${JSON.stringify(forward)}`, + ); + return true; + }, + ), + { numRuns: 200 }, + ); + }); + + test('absentReviewerBodyNeverContributesErrors', () => { + fc.assert( + fc.property( + fc.dictionary(fc.string(), fc.anything()), + (dict) => { + delete dict.reviewer; // guarantee absence regardless of low-probability random key collision + const capUndefinedKey = { ...dict, reviewer: undefined }; + const capDeletedKey = { ...dict }; + const errsUndefinedKey = validateReviewerBody(capUndefinedKey); + const errsDeletedKey = validateReviewerBody(capDeletedKey); + assert.deepEqual(errsUndefinedKey, errsDeletedKey, 'an explicit undefined must behave identically to an absent key'); + assert.deepEqual(errsUndefinedKey, [], 'D4.1 — absence must never be an error'); + return true; + }, + ), + { numRuns: 200 }, + ); + }); +});