From 46e84d5e39a9d4ebe6e8dbbc4cb01ebbf6fb695b Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 29 Jul 2026 10:55:45 -0400 Subject: [PATCH] chore(#2795): reviewer manifest body + registry harvest, validation, forward-compat (#2823) * chore(#2795): reviewer manifest body + registry harvest, validation, forward-compat Phase 2 of epic #2782 under ADR-2782. Delivers D1, D2, D3, D7, D8 and the four Phase-1 vocabulary amendments (A1-A4). - VALID_ROLES gains "reviewer"; the reviewer body is admissible on role:runtime (a host that is also a reviewer keeps one manifest) and on the new role:reviewer (a lane that is not an install target). A reviewer body on role:feature is an error: declaring one is an assertion of lane-ness. - validateReviewerBody + validateLaneProbe + validateLaneInvoke: nine closed enums, a transport discriminator selecting mutually-exclusive invoke sub-shapes, bounded probes (D7), and outputArg required-iff outputChannel is file-arg and forbidden otherwise. - Absent-safe (D4.1): only `undefined` is absent. null/{}/[]/false/0 are malformed assertions and error. 39 of 39 shipped capabilities depend on this. - collectReviewerWarnings: an unknown field inside the body warns, never errors, so a forward-built manifest degrades visibly instead of failing the build. - D8 uniqueness (slug / flags / reviewsSection) lives in validateCrossCapability, so it is enforced at build time over first-party AND at load time over the merged first-party union overlay set, with first-party-wins falling out of the loader's existing ordering rather than a new provenance check. - Config harvest widened past the role==="feature" branch in both the generator and the ownership loop. The often-cited cause of the stranded reviewer config keys -- the runtime body forbidding feature-only fields -- is not the mechanism: `config` is not in FEATURE_FIELDS_FORBIDDEN_ON_RUNTIME. The cause is two harvest sites that never read it. Verified inert: no shipped capability declares config on a non-feature role, and the generated registry is unchanged. Three ADR corrections are folded in (Phase 1 set the precedent of amending in-phase): the misattributed config-stranding cause, D3's inverted profile-membership claim, and the specified capability folder names for lm_studio / llama_cpp, which would have failed the id kebab-case invariant. Closes #2795 * chore(#2795): collapse nine enum checks into one validateEnumField helper Standards-axis review findings, both applied: - Duplicated Code: the enum-membership + enumerate-the-members error shape repeated near-verbatim at nine call sites. Routing them through one helper makes "the error names its valid members" structural rather than a convention repeated nine times, where it would drift. That property is load-bearing until Phase 6 ships the prose reference, because these errors are currently the only documentation of the vocabulary. - Speculative Generality: the isReservedName() pre-check on every enum field was inert. A VALID_* set never contains __proto__/constructor/prototype, so membership alone already rejects them, and "must be one of: ..." is more actionable than "is a reserved name". The literal guards remain where they do real work -- the key-derived write sites in the registry generator and the claim() accumulator. The reserved-name test now asserts all three reserved names are rejected via enum membership, rather than one name via a branch that no longer exists. * fix(#2795): align lane slug grammar with Phase 1 and wire the load-time diagnostic channel Spec-axis review findings, both applied. (1) The slug grammar had diverged from Phase 1's core descriptor. Phase 1 exports LANE_SLUG_RE = /^[a-z0-9][a-z0-9_-]*$/ (leading digit permitted); the manifest validator required a leading LETTER. A slug the core descriptor accepts -- a model-named lane such as 4o-mini -- would have been rejected by the manifest validator, which is exactly the translation layer ADR-2782 exists to delete. It was inert only because all eleven shipped slugs begin with a letter, so nothing else would have caught it until a third party shipped such a lane. The grammar cannot be reduced to one definition: Phase 1's module compiles to gitignored build output, and capability-validator.cjs is a committed plain .cjs that must load on a fresh worktree before build:lib has ever run. That makes this the repo's DEFECT.GENERATIVE-FIX class, so the duplication now carries a parity assertion -- laneSlugGrammarMatchesPhase1Descriptor -- which compares both the source grammar and the accept/reject verdict for a shared input set, and fails if the two ever drift again. (2) collectReviewerWarnings had exactly one caller: the build-time generator, which only ever sees first-party in-repo manifests. The real third-party overlay loader never called it and ValidatorModule did not declare it, so ADR-2782 D4.3 -- an unknown field inside a reviewer body is ignored WITH A WARNING -- surfaced nowhere at runtime, which is precisely the case D4.3 exists for. loadRegistry now collects those diagnostics on the accept path, behind a typeof-guard (an older built validator without the function still loads) and a try/catch (ADR-1244 D2's never-crash contract outranks a diagnostic). They land in a NEW OverlayMeta.diagnostics field rather than OverlayMeta.warnings, because warnings records capabilities that were SKIPPED and a consumer treating every entry as inactive would mislabel a working lane. Covered end-to-end by overlayLaneWithUnknownFieldIsAcceptedAndDiagnosed, which drives a real global-scope overlay through loadRegistry and asserts the lane is accepted, produces no skip warning, and yields a diagnostic naming the field. * fix(#2795): make the reviewer validators honour their documented totality contract Isolated adversarial review finding (MAJOR), reproduced by execution. validateReviewerBody documents itself as "TOTAL: returns an array of error strings for ANY input and never throws", and 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. The contract was false at ten sites: JSON.stringify throws on a BigInt and on a circular structure, and every enum/scalar rejection path interpolated the rejected value into its own rejection message. Reading the value could throw too, before any message was built, via a throwing getter or a Proxy get/ownKeys trap. Not reachable through a capability.json today -- every ingestion path is a plain JSON.parse of file text, which cannot express any of those shapes. Fixed anyway: the contract is stated on an EXPORTED function, and a caller must not have to re-derive today's reachability analysis to know whether it holds. Two layers, because serialization safety alone is insufficient: - describeValue() renders any value without throwing, so messages stay useful (a BigInt now reads "got: 10n" rather than degrading to a generic fallback). - A structural try/catch around validateReviewerBody and collectReviewerWarnings makes the guarantee absolute rather than argued, covering read-time throws that fire before any message exists. The same review found the property test guarding this contract was FALSE CONFIDENCE, which is the more important half. fc.anything() at default constraints emits no BigInt, no circular reference, no getter and no Proxy -- 20,000 sampled draws produced zero of each -- so the test was named for a contract its generator could not reach. Even withBigInt is insufficient under whole-value fuzzing, because the defect needs an exotic value in a specifically NAMED field and random key names never land on one. The property is now field-targeted across all twelve reviewer fields, and a companion test enumerates the shapes fast-check cannot generate at all (BigInt, circular, throwing getter, symbol, function, null-prototype) across scalar positions, array-element positions, and read-time traps. Verified red-before-green: with the fix reverted both property tests fail; with it restored all 119 pass. * chore(#2795): backfill changeset pr number to 2823 --- .changeset/proud-ibex-snooze.md | 5 + .../2782-reviewer-lane-capability-surface.md | 52 + gsd-core/bin/lib/capability-validator.cjs | 709 ++++++- scripts/gen-capability-registry.cjs | 93 +- src/capability-loader.cts | 40 +- tests/reviewer-manifest-body.test.cjs | 1682 +++++++++++++++++ 6 files changed, 2544 insertions(+), 37 deletions(-) create mode 100644 .changeset/proud-ibex-snooze.md create mode 100644 tests/reviewer-manifest-body.test.cjs 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 }, + ); + }); +});