diff --git a/.changeset/curious-zebras-squeak.md b/.changeset/curious-zebras-squeak.md new file mode 100644 index 000000000..75c79f811 --- /dev/null +++ b/.changeset/curious-zebras-squeak.md @@ -0,0 +1,5 @@ +--- +type: Removed +pr: 3272 +--- +**The undocumented `runtime.hostBehaviors.reviewerCli` capability field has been removed** — it was superseded by the declared `reviewer` body in 1.9.0 and kept working for one release as a derived alias. A manifest that still sets it contributes no reviewer lane and now reports a non-fatal warning naming the capability, at build time on stderr and at install time through the overlay loader; nothing crashes and no other behavior changes. Every shipped reviewer lane already declares a `reviewer` body, so the roster is unchanged — if you maintain an out-of-tree runtime descriptor that relied on the flag, declare a `reviewer` body to restore the lane. (#2801) diff --git a/.changeset/nimble-orcas-roam.md b/.changeset/nimble-orcas-roam.md new file mode 100644 index 000000000..bf2211edc --- /dev/null +++ b/.changeset/nimble-orcas-roam.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3272 +--- +**`runtime.hostBehaviors` is now a closed vocabulary** — the capability-manifest field that carries per-host install and adaptation switches was validated by nothing, so a typo'd or invented key was silently ignored forever. Its 59 keys are now enumerated, and a key outside the vocabulary is ignored with a non-fatal warning naming the capability and the key. It is never a validation error: a manifest authored against a newer GSD degrades visibly rather than failing the build, and an out-of-tree runtime descriptor carrying a bespoke key keeps installing. No shipped capability is affected. (#2801) diff --git a/capabilities/antigravity/capability.json b/capabilities/antigravity/capability.json index 4ef3e5310..c7f955e60 100644 --- a/capabilities/antigravity/capability.json +++ b/capabilities/antigravity/capability.json @@ -93,7 +93,6 @@ "effortSurface": "undocumented" }, "hostBehaviors": { - "reviewerCli": true, "projectInstructionFile": "GEMINI.md", "noPathRewrite": true, "hookPathStyle": "raw", diff --git a/capabilities/claude/capability.json b/capabilities/claude/capability.json index 43260dfd5..65c444e13 100644 --- a/capabilities/claude/capability.json +++ b/capabilities/claude/capability.json @@ -101,8 +101,7 @@ "skillsGlobalOnboarding": true, "legacyCommandsGsdInstallMigration": true, "legacyCommandsGsdUninstall": "global", - "hyphenNameAgentBody": true, - "reviewerCli": true + "hyphenNameAgentBody": true } }, "reviewer": { diff --git a/capabilities/codex/capability.json b/capabilities/codex/capability.json index 01d596c4c..ed5b6f4e9 100644 --- a/capabilities/codex/capability.json +++ b/capabilities/codex/capability.json @@ -87,8 +87,7 @@ "tomlConfigInstall": true, "cleanupSkillSidecars": true, "agentTomlFiles": true, - "frontmatterDialect": "codex", - "reviewerCli": true + "frontmatterDialect": "codex" } }, "reviewer": { diff --git a/capabilities/cursor/capability.json b/capabilities/cursor/capability.json index fdc2f4134..1f14ed480 100644 --- a/capabilities/cursor/capability.json +++ b/capabilities/cursor/capability.json @@ -107,8 +107,7 @@ "stop", "subagentStart", "subagentStop" - ], - "reviewerCli": true + ] } }, "reviewer": { diff --git a/capabilities/opencode/capability.json b/capabilities/opencode/capability.json index 05707e008..f5167d763 100644 --- a/capabilities/opencode/capability.json +++ b/capabilities/opencode/capability.json @@ -109,8 +109,7 @@ "skipHomePrefixSubstitution": true, "skipSettingsUi": true, "skipUpdateBannerCommand": true, - "skipCodexSkillsManifest": true, - "reviewerCli": true + "skipCodexSkillsManifest": true } }, "reviewer": { diff --git a/capabilities/qwen/capability.json b/capabilities/qwen/capability.json index edfbad23b..8e8206632 100644 --- a/capabilities/qwen/capability.json +++ b/capabilities/qwen/capability.json @@ -100,8 +100,7 @@ "legacyCommandsGsdCleanup": true, "legacyCommandsGsdInstallMigration": true, "legacyCommandsGsdUninstall": true, - "hyphenNameAgentBody": true, - "reviewerCli": true + "hyphenNameAgentBody": true } }, "reviewer": { diff --git a/docs/adr/1016-runtime-capability-descriptor.md b/docs/adr/1016-runtime-capability-descriptor.md index 15916f8a2..bc98676de 100644 --- a/docs/adr/1016-runtime-capability-descriptor.md +++ b/docs/adr/1016-runtime-capability-descriptor.md @@ -10,6 +10,29 @@ - **Subsumed by:** [ADR-1239](1239-gsd-embeddable-orchestration-engine.md) (GSD as an Embeddable Orchestration Engine) — read it first; see the amendment below - **Amended by:** [ADR-2866](2866-install-surface-resolution.md) (Install-surface resolution) — **one axis is added to this ADR's closed descriptor vocabulary: host trigger precedence.** ADR-2866 is the review this ADR's closed-vocabulary friction exists to force. The axis is *required-with-default*, so descriptors authored against today's schema keep working and [ADR-894](894-capability-declaration-format.md)'s additive-only contract holds; the registry generator and validator move with it. **Timing:** the decision is recorded and `Accepted`; the descriptor schema itself changes at epic [#2866](https://github.com/open-gsd/gsd-core/issues/2866) Phase 2 ([#2871](https://github.com/open-gsd/gsd-core/issues/2871)), not before. Nothing else in this ADR's vocabulary opens — precedence is a fact about the *host*, which is exactly why it belongs on the descriptor rather than in a per-runtime branch. - **Amended by:** [ADR-2782](2782-reviewer-lane-capability-surface.md) (Reviewer Lane capability surface) — a `role: "runtime"` capability may now carry a `reviewer` body **alongside** its runtime body. The runtime body itself remains closed and unchanged, and no feature-only field becomes permissible on it. ADR-2782 D6 **upholds** this ADR's closed-vocabulary principle: the lane's `handler` is a closed enum of first-party names (the `ConverterName` construction of Decision 3), never an open escape hatch, so §Alternatives #2 stands unreversed. +- **Amended by:** [#2801](https://github.com/open-gsd/gsd-core/issues/2801) (closes `hostBehaviors`) — the one hole in this ADR's closure is closed; see the amendment below. + +## Amendment (2026-08-09): `hostBehaviors` is closed (#2801) + +This ADR closed twelve axes and rejected "an open escape hatch in the descriptor" (§Alternatives #2). It never mentioned `runtime.hostBehaviors`, and that silence was read as permission: the field accumulated **59 keys across 18 manifests — 39 of them set by a single capability** — validated by nothing. `docs/reference/capability-manifest.md` went further and described it as *"the deliberate open seam"* sanctioned by this ADR. That attribution was never true. + +**Decision.** `hostBehaviors` is a closed vocabulary, enumerated in `KNOWN_HOST_BEHAVIORS` (`gsd-core/bin/lib/capability-validator.cjs`). A key outside it is **ignored with a non-fatal warning**, surfaced on both paths a manifest arrives through — build-time registry generation to stderr, and overlay install through `OverlayMeta.warnings`. + +**Why warning and not error.** Two reasons, and the second is the load-bearing one: + +1. It matches the [ADR-2782](2782-reviewer-lane-capability-surface.md) D4.3 treatment of an unknown `reviewer` field, so one manifest surface does not contradict its neighbor. +2. An error would hard-break any out-of-tree runtime descriptor carrying a bespoke key, with **no deprecation window** — the exact failure mode the change that closed this (#2801, ADR-2782 D9) spent a full release avoiding for `reviewerCli`. Escalating to an error is a separate decision and needs its own window. + +So this closure is real but soft: the vocabulary is enumerated, drift is visible, and adding a key is deliberate — while nothing installed today breaks. + +**Consequences.** + +- Adding a host behavior is now a reviewed change: the key must be declared in the vocabulary. That is this ADR's intended friction (§Consequences, "the closed vocabulary must grow (reviewed) … intentional friction, the trust boundary"), now applied to the surface that was escaping it. +- A parity test asserts the vocabulary equals the set of keys the shipped manifests declare, in **both** directions — an undeclared key warns on every build, and a key left behind after its last manifest drops it is dead vocabulary. Neither can rot silently. +- Zero shipped capability draws a warning at the time of closure; the change is inert for everything in-tree, and a test asserts that too. +- Third-party descriptors carrying bespoke keys now see a warning where they previously saw silence. That is the intended signal, not a regression — but it is the reason enforcement stops at warning. + +**Not decided here.** Whether the 39 single-use keys should be consolidated, promoted to real axes, or retired. Closing the vocabulary makes that question answerable; it does not answer it. ## Amendment (2026-07-16): subsumed by ADR-1239 (EoS) — this ADR is the *declarative adapter*, not the whole architecture diff --git a/docs/how-to/ship-a-reviewer-lane.md b/docs/how-to/ship-a-reviewer-lane.md index 335658e67..a01b5c2c6 100644 --- a/docs/how-to/ship-a-reviewer-lane.md +++ b/docs/how-to/ship-a-reviewer-lane.md @@ -219,6 +219,46 @@ For the reasoning behind consent-plus-integrity rather than a sandbox, see [The --- +## Migrate off the removed `reviewerCli` flag + +Before 1.9.0, a runtime capability declared itself a reviewer with a boolean in the open host-behaviors bag: + +```json +"runtime": { "hostBehaviors": { "reviewerCli": true } } +``` + +That flag carried no invocation data — it only added the capability id to the roster, leaving the probe, argv shape, timeout, and output policy hardcoded in GSD core. It was superseded by the `reviewer` body in 1.9.0, kept working for one release as a derived alias, and **was removed in the release after that**. + +**Symptom.** Your capability installs and validates exactly as before, but `/gsd-review` no longer offers your flag and your lane never runs. On a registry build or a capability install you will see: + +``` +⚠ capability "your-cap" runtime.hostBehaviors.reviewerCli was removed (ADR-2782 D9) + — ignored, and it contributes no reviewer lane. Declare a `reviewer` body instead; + see docs/how-to/ship-a-reviewer-lane.md +``` + +**Fix.** Delete the flag and declare a `reviewer` body, following [Declare a spawned-CLI lane](#declare-a-spawned-cli-lane) above. Your `reviewer.slug` should be whatever the flag used to contribute — your **capability id** — so existing `review.default_reviewers` entries and `--` flags keep working: + +```json +{ + "id": "your-cap", + "role": "runtime", + "runtime": { "hostBehaviors": { } }, + "reviewer": { + "slug": "your-cap", + "flags": ["--your-cap"] + } +} +``` + +Then rebuild and re-verify with the steps in [Build and install](#build-and-install) and [Verify the lane resolves](#verify-the-lane-resolves). + +Two things the migration buys you beyond restoring the lane: your invocation shape becomes declared data rather than something GSD core has to know about, and your lane can own its own config keys (see [Own your lane's config keys](#own-your-lanes-config-keys)). + +> **Nothing else about your capability changes.** A manifest still carrying the removed key parses, validates, and installs exactly as before — it simply contributes no lane, and says so. The key is inert, not fatal. + +--- + ## Conditionals: when the vocabulary does not fit your tool Third-party lanes are **data-only**. `handler` is a closed enum of first-party names (`antigravity`, `openai-compatible`, `opencode`, or `null`) — you may reference an existing member, but you cannot ship your own handler module. @@ -238,6 +278,7 @@ Filing the issue is the supported route, not a workaround. The `openai-http` tra ## Related - [Capability manifest](../reference/capability-manifest.md) — the full `reviewer` body field table and validation rules +- [Capability manifest → `hostBehaviors`](../reference/capability-manifest.md#hostbehaviors) — why the bag is unvalidated, and the one removal notice inside it - [Set up cross-AI review](set-up-cross-ai-review.md) — the user-facing side: choosing, configuring, and running reviewers - [Develop a Capability for GSD 1.5+](develop-a-capability.md) — manifests, registry generation, and federated config - [Publish a capability](publish-a-capability.md) — versioning, `engines.gsd`, and distribution diff --git a/docs/reference/capability-manifest.md b/docs/reference/capability-manifest.md index c22012256..bc16c15d4 100644 --- a/docs/reference/capability-manifest.md +++ b/docs/reference/capability-manifest.md @@ -161,24 +161,31 @@ Runtime capabilities describe how GSD projects its artefacts onto one host CLI. ### `hostBehaviors` -`runtime.hostBehaviors` is an **open, unvalidated bag** of per-host behavior switches consumed directly by installer and runtime-adaptation code. Unlike every axis in the table above, it is **not covered by any schema**: the key `hostBehaviors` appears zero times in `scripts/gen-capability-registry.cjs` and zero times in `scripts/registry-schema.cjs`. An unknown key inside `hostBehaviors` is neither rejected nor warned about — it is simply ignored by any code path that does not look for it by name. +`runtime.hostBehaviors` is a **closed vocabulary** of per-host behavior switches consumed directly by installer and runtime-adaptation code. A key outside the vocabulary is **ignored, with a non-fatal warning** naming the capability and the key; it is never a validation error, so a manifest authored against a newer GSD degrades visibly instead of failing the build of a repo that merely reads it. -58 distinct keys are declared across the shipped runtime manifests; most are set by exactly one capability. This table is not exhaustive — it lists the keys with the widest reuse so a reader can pattern-match new ones against the same shape: +Adding a key is a reviewed first-party change, which is [ADR-1016](../adr/1016-runtime-capability-descriptor.md)'s intended friction rather than an obstacle: the runtime descriptor expresses every per-host difference as a value over a closed vocabulary, and a host needing a new shape gets a named primitive rather than an open escape hatch. + +> **History.** `hostBehaviors` went unvalidated until [#2801](https://github.com/open-gsd/gsd-core/issues/2801), and this page previously described it as a deliberate open seam sanctioned by ADR-1016. That attribution was wrong — ADR-1016 does not mention `hostBehaviors` at all. See the [ADR-1016 amendment](../adr/1016-runtime-capability-descriptor.md#amendment-2026-08-09-hostbehaviors-is-closed-2801). + +The vocabulary holds 59 keys; 39 of them are set by exactly one capability. This table is not exhaustive — it lists the keys with the widest reuse so a reader can pattern-match new ones against the same shape: | Key | Capabilities declaring it | |---|---| | `reapplyCommand` | 9 | | `skipSharedHooksInstall` | 8 | -| `reviewerCli` | 6 | | `frontmatterDialect` | 5 | | `hyphenNameAgentBody` | 3 | | `legacyCommandsGsdInstallMigration` | 3 | +| `legacyCommandsGsdUninstall` | 3 | +| `nativePlugin` | 3 | | `skipUpdateBannerCommand` | 3 | | `verificationStyle` | 3 | -**`reviewerCli` is deprecated.** It is a boolean that historically marked a runtime capability as also being a reviewer lane. It is now a **derived legacy alias**, retained for one release so an out-of-tree runtime descriptor that still sets it keeps working. A declared `reviewer` body (see below) takes precedence over the alias, and a capability declaring both contributes **one** slug, not two. `reviewerCli` is superseded by the `reviewer` body; its removal is tracked by issue #2801. It is currently set by 6 capabilities: `antigravity`, `claude`, `codex`, `cursor`, `opencode`, `qwen`. +**`reviewerCli` has been removed.** It was a boolean that marked a runtime capability as also being a reviewer lane. [ADR-2782](../adr/2782-reviewer-lane-capability-surface.md) replaced it with the [`reviewer` body](#reviewer-body-role-reviewer-or-on-any-role); it survived one release (1.9.0 → 1.10.0) as a derived legacy alias and was deleted in Phase 7 ([#2801](https://github.com/open-gsd/gsd-core/issues/2801)). No shipped capability declares it. -See [ADR-1016](../adr/1016-runtime-capability-descriptor.md) (the runtime body is a closed 8-axis plus 4 install-surface vocabulary; `hostBehaviors` is the deliberate open seam beside it) and [ADR-2782](../adr/2782-reviewer-lane-capability-surface.md) (introduces the `reviewer` body and the `reviewerCli` alias's deprecation). +**If your out-of-tree manifest still sets it:** nothing crashes and nothing else about your capability changes — it simply contributes no reviewer lane, and the registry reports a non-fatal warning naming the capability. The warning reaches you at build time on stderr, and at install time through the overlay loader's diagnostics. To restore the lane, declare a `reviewer` body; [Ship a reviewer lane in your capability](../how-to/ship-a-reviewer-lane.md) is the migration path, and the field reference is below. + +See [ADR-1016](../adr/1016-runtime-capability-descriptor.md) (the runtime descriptor is a closed vocabulary; its 2026-08-09 amendment closes `hostBehaviors` too) and [ADR-2782](../adr/2782-reviewer-lane-capability-surface.md) (introduces the `reviewer` body, and D9 retires the `reviewerCli` alias). For a minimal `role: "runtime"` example, see [ADR-1016 §Decision 8](../adr/1016-runtime-capability-descriptor.md). diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index 38bffd9d1..cb543a721 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -187,7 +187,6 @@ const capabilities = { "effortSurface": "undocumented" }, "hostBehaviors": { - "reviewerCli": true, "projectInstructionFile": "GEMINI.md", "noPathRewrite": true, "hookPathStyle": "raw", @@ -577,8 +576,7 @@ const capabilities = { "skillsGlobalOnboarding": true, "legacyCommandsGsdInstallMigration": true, "legacyCommandsGsdUninstall": "global", - "hyphenNameAgentBody": true, - "reviewerCli": true + "hyphenNameAgentBody": true } }, "reviewer": { @@ -1085,8 +1083,7 @@ const capabilities = { "tomlConfigInstall": true, "cleanupSkillSidecars": true, "agentTomlFiles": true, - "frontmatterDialect": "codex", - "reviewerCli": true + "frontmatterDialect": "codex" } }, "reviewer": { @@ -1337,8 +1334,7 @@ const capabilities = { "stop", "subagentStart", "subagentStop" - ], - "reviewerCli": true + ] } }, "reviewer": { @@ -2661,8 +2657,7 @@ const capabilities = { "skipHomePrefixSubstitution": true, "skipSettingsUi": true, "skipUpdateBannerCommand": true, - "skipCodexSkillsManifest": true, - "reviewerCli": true + "skipCodexSkillsManifest": true } }, "reviewer": { @@ -3005,8 +3000,7 @@ const capabilities = { "legacyCommandsGsdCleanup": true, "legacyCommandsGsdInstallMigration": true, "legacyCommandsGsdUninstall": true, - "hyphenNameAgentBody": true, - "reviewerCli": true + "hyphenNameAgentBody": true } }, "reviewer": { @@ -4853,7 +4847,6 @@ const runtimes = { "effortSurface": "undocumented" }, "hostBehaviors": { - "reviewerCli": true, "projectInstructionFile": "GEMINI.md", "noPathRewrite": true, "hookPathStyle": "raw", @@ -5114,8 +5107,7 @@ const runtimes = { "skillsGlobalOnboarding": true, "legacyCommandsGsdInstallMigration": true, "legacyCommandsGsdUninstall": "global", - "hyphenNameAgentBody": true, - "reviewerCli": true + "hyphenNameAgentBody": true } }, "reviewer": { @@ -5431,8 +5423,7 @@ const runtimes = { "tomlConfigInstall": true, "cleanupSkillSidecars": true, "agentTomlFiles": true, - "frontmatterDialect": "codex", - "reviewerCli": true + "frontmatterDialect": "codex" } }, "reviewer": { @@ -5683,8 +5674,7 @@ const runtimes = { "stop", "subagentStart", "subagentStop" - ], - "reviewerCli": true + ] } }, "reviewer": { @@ -6264,8 +6254,7 @@ const runtimes = { "skipHomePrefixSubstitution": true, "skipSettingsUi": true, "skipUpdateBannerCommand": true, - "skipCodexSkillsManifest": true, - "reviewerCli": true + "skipCodexSkillsManifest": true } }, "reviewer": { @@ -6477,8 +6466,7 @@ const runtimes = { "legacyCommandsGsdCleanup": true, "legacyCommandsGsdInstallMigration": true, "legacyCommandsGsdUninstall": true, - "hyphenNameAgentBody": true, - "reviewerCli": true + "hyphenNameAgentBody": true } }, "reviewer": { diff --git a/gsd-core/bin/lib/capability-validator.cjs b/gsd-core/bin/lib/capability-validator.cjs index be6598b9b..0f4a50b02 100644 --- a/gsd-core/bin/lib/capability-validator.cjs +++ b/gsd-core/bin/lib/capability-validator.cjs @@ -1578,6 +1578,122 @@ const KNOWN_REVIEWER_FIELDS = new Set([ 'handler', ]); +/** + * The closed `runtime.hostBehaviors` vocabulary (ADR-1016, closed via #2801). + * + * ADR-1016's core principle is that every per-runtime difference is a value over + * a closed primitive vocabulary, and that a host needing a new shape gets a + * reviewed first-party primitive rather than "an open escape hatch in the + * descriptor" (§Alternatives #2 rejects exactly that). `hostBehaviors` was the + * one hole left in that closure: 59 keys across 18 manifests, 39 of them set by + * a single capability, validated by nothing. It was described in the reference + * docs as a deliberate open seam sanctioned by ADR-1016 — ADR-1016 does not + * mention `hostBehaviors` at all, and its stated principle is the opposite. + * + * Adding a key here is deliberate and reviewed. That friction IS the decision + * (ADR-1016 §Consequences: "the closed vocabulary must grow (reviewed) when a + * genuinely new host shape appears — intentional friction, the trust boundary"). + * + * WARNING, never error. Two reasons: + * 1. It matches the ADR-2782 D4.3 treatment of an unknown `reviewer` field, so + * a manifest built against a newer GSD degrades visibly instead of failing + * the build of a repo that merely reads it. + * 2. An error would hard-break an out-of-tree descriptor carrying a bespoke + * key, with no deprecation window — the exact mistake #2801's own alias + * removal spent a full release avoiding. Escalating to an error is a later + * step and needs its own window. + * + * Kept in sorted order, and `tests/reviewer-manifest-body.test.cjs` asserts this + * set equals the keys the shipped manifests actually declare, so the list cannot + * silently rot away from reality (DEFECT.GENERATIVE-FIX). + */ +const KNOWN_HOST_BEHAVIORS = new Set([ + 'agentFileExtension', + 'agentFrontmatterExtensions', + 'agentManifestStyle', + 'agentTomlFiles', + 'attributionConfigResolver', + 'attributionSource', + 'authorsCanonicalWorkflow', + 'brandingRewrites', + 'cleanupSkillSidecars', + 'clineRulesSurface', + 'combinedFamilyInstall', + 'commandBodyConverter', + 'doneBannerStyle', + 'flatCommandDir', + 'frontmatterDialect', + 'globalDirResolver', + 'hookPathStyle', + 'hooksJsonSurface', + 'hyphenNameAgentBody', + 'installsCommandBodiesForWorkflowDelegation', + 'legacyCommandsGsdCleanup', + 'legacyCommandsGsdInstallMigration', + 'legacyCommandsGsdUninstall', + 'legacyDevinSkillsCleanup', + 'localCommandsViaRules', + 'localInstallDeferred', + 'localInstallStyle', + 'localTargetIsProjectRoot', + 'managedHookEvents', + 'mcpCompanion', + 'namedSubagentsSupported', + 'nativeModelAliases', + 'nativePlugin', + 'noPathRewrite', + 'ownsClaudePaths', + 'permissionsSchema', + 'pluginOnlyInstall', + 'projectInstructionFile', + 'reapplyCommand', + 'reportCommandsDir', + 'reportSkillsCount', + 'retiredArtifacts', + 'settingsFileByScope', + 'sharedHooksDirName', + 'skillFrontmatterVersion', + 'skillPriorityFrontmatter', + 'skillsGlobalOnboarding', + 'skillsManifestPrefix', + 'skipCodexSkillsManifest', + 'skipHomePrefixSubstitution', + 'skipSettingsUi', + 'skipSharedHooksInstall', + 'skipUpdateBannerCommand', + 'soloStageMetadata', + 'sourceMarkerFile', + 'tomlConfigInstall', + 'trackCategoryDescription', + 'verificationStyle', + 'writeCategoryDescription', +]); + +/** + * Frozen reason codes for the non-fatal reviewer diagnostics (ADR-2782 D4.3). + * + * The IR behind `collectReviewerWarnings`' rendered strings. Tests assert on + * these codes; the rendered `message` is operator console output and tests must + * not depend on it (CONTRIBUTING.md, "Prohibited: Raw Text Matching on Test + * Outputs"), which is the same split `bin/verify-reapply-patches.cjs` uses. + * + * Adding a code is THREE coordinated changes: this enum, the emitting site in + * `collectReviewerWarningRecordFields`, and the test that locks + * `Object.keys(REVIEWER_WARNING).sort()`. That coupling is the point — it stops + * the code surface drifting from the test surface. + */ +const REVIEWER_WARNING = Object.freeze({ + /** A key inside a `reviewer` body that this GSD version does not know. */ + UNKNOWN_REVIEWER_FIELD: 'unknown_reviewer_field', + /** A `runtime.hostBehaviors` key that was removed from the vocabulary. */ + REMOVED_HOST_BEHAVIOR: 'removed_host_behavior', + /** A `runtime.hostBehaviors` key outside the closed vocabulary. */ + UNKNOWN_HOST_BEHAVIOR: 'unknown_host_behavior', +}); + +/** Dotted path of the field removed by ADR-2782 D9 / #2801. */ +const REMOVED_REVIEWER_CLI_FIELD = 'runtime.hostBehaviors.reviewerCli'; + const KNOWN_PROBE_FIELDS = new Set(['kind', 'binary', 'needle', 'timeoutMs', 'hostConfigKey', 'path']); /** A bounded probe timeout must be a finite, positive INTEGER of milliseconds. */ @@ -1615,6 +1731,41 @@ function describeValue(v) { } } +/** + * Ceiling on how many undeclared-key diagnostics one capability may produce. + * + * The loops below iterate MANIFEST-SUPPLIED keys, and an installed third-party + * manifest is bounded only by MANIFEST_MAX_BYTES (8MB). Unbounded, one manifest + * of 800k keys yields 800k records and ~139MB of message text, retained for the + * registry's lifetime in OverlayMeta.diagnostics. Ten is enough to act on; the + * rest are summarized. Mirrors the existing truncation idiom in + * `capability-loader.cts` (`crossErrs.slice(0, 3)`). + */ +const MAX_REPORTED_UNKNOWN_KEYS = 10; + +/** Ceiling on how much of one manifest-supplied key name a diagnostic repeats. */ +const MAX_REPORTED_KEY_CHARS = 80; + +/** + * Render a manifest-supplied KEY for a diagnostic: control-safe and bounded. + * + * Key names carry no grammar anywhere — unlike `cap.id`, which `validateCapability` + * gates on KEBAB_RE before these diagnostics run — so a key is fully + * attacker-controlled text heading for stderr and OverlayMeta.warnings. C0/C1 + * controls (ESC, CR, LF) become U+FFFD so a key cannot emit terminal escapes or + * forge a log line, and the result is clipped so one key cannot carry megabytes + * into a retained diagnostic. + * + * @param {*} key + * @returns {string} + */ +function describeKey(key) { + const raw = typeof key === 'string' ? key : String(key); + // eslint-disable-next-line no-control-regex + const safe = raw.replace(/[\x00-\x1f\x7f-\x9f]/g, '�'); + return safe.length > MAX_REPORTED_KEY_CHARS ? safe.slice(0, MAX_REPORTED_KEY_CHARS) + '…' : safe; +} + /** Extract a message from an unknown thrown value without throwing again. */ function safeErrorMessage(err) { try { @@ -1661,7 +1812,39 @@ function validateEnumField(ctx, label, value, validSet) { } /** - * Collect NON-FATAL diagnostics for a reviewer body (ADR-2782 D4.3). + * Collect NON-FATAL reviewer diagnostics for a capability manifest as TYPED + * RECORDS (ADR-2782 D4.3, plus the D9 `hostBehaviors.reviewerCli` removal + * notice, #2801). + * + * This is the IR. `collectReviewerWarnings` below renders it to strings for the + * two production consumers; tests assert on these records instead of matching + * the rendered prose (CONTRIBUTING.md, "Prohibited: Raw Text Matching on Test + * Outputs"). + * + * Record shape — `code` and `capId` are always present; the rest is per-code: + * { code: REVIEWER_WARNING.UNKNOWN_REVIEWER_FIELD, + * capId, field: 'reviewer.', knownFields: string[], message } + * { code: REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR, + * capId, field: 'runtime.hostBehaviors.reviewerCli', + * replacement: 'reviewer', docs: '', message } + * + * TOTAL: returns an array for ANY input and never throws. It runs on + * loadRegistry's ACCEPT path, so a diagnostic that throws would cost the user a + * lane that is otherwise perfectly valid (#1461 OVL-1). + * + * @param {object} cap A capability manifest. + * @returns {Array} Warning records; empty when there is nothing to say. + */ +function collectReviewerWarningRecords(cap) { + try { + return collectReviewerWarningRecordFields(cap); + } catch { + return []; + } +} + +/** + * Render the IR for operator console output. * * Kept separate from validateReviewerBody so validateCapability's contract * (`=> string[]` of ERRORS) is unchanged for its two existing callers. The @@ -1669,35 +1852,135 @@ function validateEnumField(ctx, label, value, validSet) { * through OverlayMeta.warnings. A warning written only to a build log nobody * reads is not a warning (ADR-2782 D4, "Where warnings surface"). * + * The `string[]` shape is load-bearing for both consumers and does not change. + * * @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 []; - } + return collectReviewerWarningRecords(cap).map((record) => record.message); } -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; +function collectReviewerWarningRecordFields(cap) { + const records = []; + if (typeof cap !== 'object' || cap === null || Array.isArray(cap)) return records; const capId = typeof cap.id === 'string' ? cap.id : '(unknown)'; + + // ADR-2782 D9 / #2801 — the REMOVED `runtime.hostBehaviors.reviewerCli` alias. + // + // Emitted BEFORE the `reviewer`-body early-return below, deliberately. The + // manifest this notice exists for is the ALIAS-ONLY one, which by definition + // carries no body; after that guard the check would fire only for + // capabilities that already declare a lane — exactly the set that does not + // need telling. + // + // Presence-based, not `=== true`: the key is unknown at ANY value now, which + // is the same rule the unknown-`reviewer.*`-field loop below applies. Own-key + // read, so a polluted prototype cannot manufacture this warning on every + // otherwise-innocent manifest. This is ONE keyed removal notice, not general + // `hostBehaviors` validation — the closed vocabulary below handles that (#2801). + const runtimeBody = cap.runtime; + if (typeof runtimeBody === 'object' && runtimeBody !== null && !Array.isArray(runtimeBody)) { + const hostBehaviors = runtimeBody.hostBehaviors; + if ( + typeof hostBehaviors === 'object' + && hostBehaviors !== null + && !Array.isArray(hostBehaviors) + && Object.prototype.hasOwnProperty.call(hostBehaviors, 'reviewerCli') + ) { + records.push({ + code: REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR, + capId, + field: REMOVED_REVIEWER_CLI_FIELD, + replacement: 'reviewer', + docs: 'docs/how-to/ship-a-reviewer-lane.md', + message: + '⚠ capability "' + capId + '" ' + REMOVED_REVIEWER_CLI_FIELD + ' was removed (ADR-2782 D9) ' + + '— ignored, and it contributes no reviewer lane. Declare a `reviewer` body instead; see ' + + 'docs/how-to/ship-a-reviewer-lane.md', + }); + } + + // #2801 — the closed `hostBehaviors` vocabulary (ADR-1016). `reviewerCli` is + // excluded here: it already drew its own removal notice above, and a second, + // generic "unknown key" record for the same key would be noise, not signal. + if (typeof hostBehaviors === 'object' && hostBehaviors !== null && !Array.isArray(hostBehaviors)) { + let reported = 0; + let omitted = 0; + for (const key of Object.keys(hostBehaviors)) { + if (isReservedName(key) || key === 'reviewerCli' || KNOWN_HOST_BEHAVIORS.has(key)) continue; + if (reported >= MAX_REPORTED_UNKNOWN_KEYS) { + omitted += 1; + continue; + } + reported += 1; + const safeKey = describeKey(key); + records.push({ + code: REVIEWER_WARNING.UNKNOWN_HOST_BEHAVIOR, + capId, + field: 'runtime.hostBehaviors.' + safeKey, + message: + '⚠ capability "' + capId + '" runtime.hostBehaviors.' + safeKey + ' is not a known host behavior ' + + 'in this GSD version — ignored. Adding one is a reviewed first-party change (ADR-1016).', + }); + } + if (omitted > 0) { + records.push({ + code: REVIEWER_WARNING.UNKNOWN_HOST_BEHAVIOR, + capId, + field: 'runtime.hostBehaviors', + truncated: true, + omittedCount: omitted, + message: + '⚠ capability "' + capId + '" declares ' + omitted + ' further unknown runtime.hostBehaviors ' + + 'key(s), not listed. A manifest this far outside the vocabulary is likely built for a ' + + 'different GSD version.', + }); + } + } + } + + const r = cap.reviewer; + if (typeof r !== 'object' || r === null || Array.isArray(r)) return records; + + // Same ceiling and the same key sanitization as the hostBehaviors sweep above. + // This loop predates #2801 and carried both defects; fixing only the new copy + // would leave the identical defect one screen away from its own fix. + let reportedFields = 0; + let omittedFields = 0; 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(', '), - ); + if (reportedFields >= MAX_REPORTED_UNKNOWN_KEYS) { + omittedFields += 1; + continue; + } + reportedFields += 1; + const safeKey = describeKey(key); + records.push({ + code: REVIEWER_WARNING.UNKNOWN_REVIEWER_FIELD, + capId, + field: 'reviewer.' + safeKey, + knownFields: [...KNOWN_REVIEWER_FIELDS], + message: + '⚠ capability "' + capId + '" reviewer.' + safeKey + ' is not a known reviewer field ' + + 'in this GSD version — ignored. Known fields: ' + [...KNOWN_REVIEWER_FIELDS].join(', '), + }); } - return warnings; + if (omittedFields > 0) { + records.push({ + code: REVIEWER_WARNING.UNKNOWN_REVIEWER_FIELD, + capId, + field: 'reviewer', + knownFields: [...KNOWN_REVIEWER_FIELDS], + truncated: true, + omittedCount: omittedFields, + message: + '⚠ capability "' + capId + '" declares ' + omittedFields + ' further unknown reviewer field(s), ' + + 'not listed.', + }); + } + return records; } /** @@ -3170,8 +3453,14 @@ module.exports = { VALID_EVIDENCE_CLASSES, VALID_LANE_HANDLERS, KNOWN_REVIEWER_FIELDS, + KNOWN_HOST_BEHAVIORS, + MAX_REPORTED_UNKNOWN_KEYS, + MAX_REPORTED_KEY_CHARS, validateReviewerBody, collectReviewerWarnings, + collectReviewerWarningRecords, + REVIEWER_WARNING, + REMOVED_REVIEWER_CLI_FIELD, VALID_INSTALL_SURFACES, VALID_PERMISSION_WRITERS, VALID_EXTENDED_HOOK_EVENTS, diff --git a/src/review-lane-descriptor.cts b/src/review-lane-descriptor.cts index 1825cb0e5..89d288f81 100644 --- a/src/review-lane-descriptor.cts +++ b/src/review-lane-descriptor.cts @@ -596,10 +596,10 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ * (`loadRegistry({ includeInstalled: true })`); only its * `capabilities` map is read. A cap contributes a lane iff it * carries an object `reviewer` body with a non-empty, - * grammar-valid `slug`. The `role:"runtime"` legacy - * `reviewerCli` alias contributes NO lane here — it has no lane - * descriptor, and the selection roster (`deriveReviewerSlugs`) - * is a separate surface. + * grammar-valid `slug`. The `role:"runtime"` `reviewerCli` + * alias never contributed a lane here, and as of #2801 it no + * longer contributes to the selection roster + * (`deriveReviewerSlugs`) either — the two surfaces now agree. * @returns A NEW array: first-party lanes (in order) followed by accepted overlay * lanes (in registry iteration order). Callers must not mutate it. */ diff --git a/src/review-reviewer-selection.cts b/src/review-reviewer-selection.cts index 7e6663eb1..8ddaa8788 100644 --- a/src/review-reviewer-selection.cts +++ b/src/review-reviewer-selection.cts @@ -26,12 +26,14 @@ * three-namespace trap (`id` must be kebab; `slug` keeps the shipped roster's * snake form). * - * `runtime.hostBehaviors.reviewerCli: true` survives as a DERIVED LEGACY ALIAS - * for one release (D9): a capability that only sets the flag (no `reviewer` - * body yet) still contributes its capability id as a slug, exactly as before - * this phase. Where a capability carries BOTH a declared body and the alias, - * the BODY WINS — that capability contributes only the body's slug, never - * both. Alias removal is a named later phase (#2801), not done here. + * `runtime.hostBehaviors.reviewerCli` is GONE (Phase 7, #2801). It survived one + * release as a derived legacy alias — a capability setting only the flag + * contributed its capability id as a slug — and that window closed when 1.10.0 + * shipped after Phase 5a's 1.9.0. A declared `reviewer` body is now the ONLY + * route onto the roster. A manifest still carrying the key contributes no lane + * and is told so: `collectReviewerWarnings` + * (`gsd-core/bin/lib/capability-validator.cjs`) emits a removal notice on both + * the build-time registry generation and the third-party overlay load path. * * Before this phase the five non-runtime reviewers (`gemini`, `coderabbit`, * `ollama`, `lm_studio`, `llama_cpp`) had no `capabilities//` descriptor at @@ -43,7 +45,6 @@ interface RegistryReviewerCapability { reviewer?: { slug?: unknown }; - runtime?: { hostBehaviors?: { reviewerCli?: unknown } }; } interface ReviewerCapabilityRegistry { @@ -62,19 +63,17 @@ interface ReviewerCapabilityRegistry { * * Reads `registry.capabilities` — every declared capability regardless of * role — because a `role: "reviewer"` lane-only capability is never stored in - * `registry.runtimes` (that map is role:"runtime" only). Per capability: a - * declared `reviewer.slug` wins outright (D9) and the capability contributes - * ONLY that slug; only when there is no body does the legacy - * `hostBehaviors.reviewerCli` alias contribute the capability id instead. The + * `registry.runtimes` (that map is role:"runtime" only). A capability + * contributes exactly one slug — its declared `reviewer.slug` — or none. The * result is collected into a Set (so a slug can never appear twice, even if * two distinct capabilities somehow named the same one) and returned as a - * SORTED array, so the roster's order never depends on `Object.entries()` + * SORTED array, so the roster's order never depends on `Object.values()` * iteration / registry build order. */ export function deriveReviewerSlugs(registry: ReviewerCapabilityRegistry): string[] { const capabilities = registry.capabilities || {}; const slugs = new Set(); - for (const [capId, cap] of Object.entries(capabilities)) { + for (const cap of Object.values(capabilities)) { // Trim before the emptiness test: `length > 0` alone admits a whitespace-only // slug (" ") verbatim into the roster, where it can never match a real lane // but still occupies a roster entry. Unreachable through the checked-in @@ -82,13 +81,7 @@ export function deriveReviewerSlugs(registry: ReviewerCapabilityRegistry): strin // other validation, so it should not depend on its caller's hygiene. const rawSlug = cap?.reviewer?.slug; const declaredSlug = typeof rawSlug === 'string' ? rawSlug.trim() : ''; - if (declaredSlug.length > 0) { - slugs.add(declaredSlug); - continue; // the body wins — never ALSO add this capability's alias below. - } - if (cap?.runtime?.hostBehaviors?.reviewerCli === true) { - slugs.add(capId); - } + if (declaredSlug.length > 0) slugs.add(declaredSlug); } return [...slugs].sort(); } diff --git a/tests/declarative-reference-antigravity.test.cjs b/tests/declarative-reference-antigravity.test.cjs index 7803f3d2d..7a0f69bc7 100644 --- a/tests/declarative-reference-antigravity.test.cjs +++ b/tests/declarative-reference-antigravity.test.cjs @@ -174,7 +174,10 @@ test('capabilities/antigravity/capability.json validates — subagentToolkit "fu test('antigravity descriptor declares runtime.hostBehaviors (the folded-in behaviors) + the subagentToolkit upgrade', () => { const hb = ANTIGRAVITY_CAP.runtime.hostBehaviors; assert.ok(hb && typeof hb === 'object'); - assert.equal(hb.reviewerCli, true); + // `reviewerCli` was one of the folded-in behaviors originally; ADR-2782 + // Phase 7 (#2801) removed the alias — antigravity's reviewer lane is declared + // by its `reviewer` body now, not by a boolean in the open hostBehaviors bag. + assert.equal(Object.prototype.hasOwnProperty.call(hb, 'reviewerCli'), false); assert.equal(hb.projectInstructionFile, 'GEMINI.md'); assert.equal(hb.noPathRewrite, true); assert.equal(hb.hookPathStyle, 'raw'); diff --git a/tests/reviewer-lane-declarations.test.cjs b/tests/reviewer-lane-declarations.test.cjs index 73a8f0d46..a2a0f0732 100644 --- a/tests/reviewer-lane-declarations.test.cjs +++ b/tests/reviewer-lane-declarations.test.cjs @@ -11,6 +11,13 @@ process.env.GSD_TEST_MODE = '1'; * A-E). See that phase's `40-design.md` for the behavior table the matrix * derives from. Test names are copied verbatim from the matrix. * + * AMENDED by ADR-2782 Phase 7 (chore #2801), which removes the + * `runtime.hostBehaviors.reviewerCli` derived legacy alias. Rows that asserted + * the alias still contributed a slug are inverted here rather than deleted, so + * the file keeps a guard against reintroduction. See + * `.gsd/phase/chore-2801-remove-reviewercli-alias/50-test-matrix.md` rows + * B1 and C1-C10, P1. + * * THE SINGLE MOST IMPORTANT PROPERTY (matrix "Red-before-green"): the roster is * eleven slugs before this phase and eleven after, with IDENTICAL membership — * C1 is the keystone, asserted against a LITERAL list, never against a value @@ -43,6 +50,7 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); +const fc = require('fast-check'); const { loadAndValidate, @@ -296,21 +304,27 @@ describe('B. The six existing runtime capabilities', () => { } }); - test('reviewerCliAliasIsRetainedForTheDeprecationWindow', () => { + test('reviewerCliAliasIsRemovedFromEveryShippedManifest', () => { + // Phase 7 (#2801): the deprecation window opened in 1.9.0 (Phase 5a) and + // 1.9.1 + 1.10.0 have since shipped, so the alias goes. Each of these six + // already declares a `reviewer` body whose slug equals its capability id, so + // removing the key costs none of them a lane — C1 is the proof. for (const id of RUNTIME_REVIEWER_IDS) { const cap = SHIPPED.capMap.get(id); + const hb = cap.runtime.hostBehaviors || {}; assert.equal( - cap.runtime.hostBehaviors && cap.runtime.hostBehaviors.reviewerCli, true, - `"${id}" must retain hostBehaviors.reviewerCli:true for the deprecation window (removal is Phase 7 / #2801)`, + Object.prototype.hasOwnProperty.call(hb, 'reviewerCli'), false, + `"${id}" must no longer declare hostBehaviors.reviewerCli (removed in Phase 7 / #2801); declare a reviewer body instead`, ); } }); - test('bodyAndAliasContributeOneSlugNotTwo', () => { - // Isolated synthetic fixture: one capability carrying BOTH a declared - // reviewer.slug AND the legacy alias, with slug !== capId, so a double - // contribution would be observable as two distinct roster entries rather - // than being hidden by an accidental string match. + test('vestigialAliasKeyDoesNotAddASecondSlug', () => { + // Post-#2801 this is no longer a PRECEDENCE rule (body beats alias) — the + // key is simply never read. Kept, with slug !== capId so a stray + // contribution would be observable as a distinct roster entry rather than + // hidden by an accidental string match, because an out-of-tree manifest can + // still carry the vestigial key for years. const registry = { capabilities: { 'dual-purpose-cap': { @@ -323,14 +337,17 @@ describe('B. The six existing runtime capabilities', () => { const roster = deriveReviewerSlugs(registry); assert.equal(roster.length, 1, `expected exactly one contribution, not two, got: ${JSON.stringify(roster)}`); assert.deepEqual(roster, ['dual-slug']); - assert.equal(roster.includes('dual-purpose-cap'), false, 'the legacy alias must not ALSO contribute the capability id'); + assert.equal(roster.includes('dual-purpose-cap'), false, 'the removed alias key must not contribute the capability id'); }); }); // ─── C. Roster derivation — src/review-reviewer-selection.cts ───────────── describe('C. Roster derivation — src/review-reviewer-selection.cts', () => { - test('rosterMembershipIsUnchangedByDerivationRefactor', () => { + test('rosterMembershipIsUnchangedByAliasRemoval', () => { + // KEYSTONE. This row is GREEN before and after #2801 — it is the invariant + // the phase must not break, not a red row. The literal list is never + // computed by the machinery under test. assert.equal(KNOWN_REVIEWER_SLUGS.length, 12, 'roster must be exactly 12 — not 11, not 13'); assert.deepEqual( [...KNOWN_REVIEWER_SLUGS].sort(), LITERAL_ROSTER, @@ -343,14 +360,34 @@ describe('C. Roster derivation — src/review-reviewer-selection.cts', () => { assert.deepEqual(deriveReviewerSlugs(registry), ['my-lane']); }); - test('aliasOnlyCapabilityStillContributesItsSlug', () => { - // No reviewer body yet — only the legacy hostBehaviors flag (B2's shape). + test('aliasOnlyCapabilityContributesNoSlug', () => { + // #2801 core inversion: no reviewer body, only the removed hostBehaviors + // flag. Before Phase 7 this contributed `legacy-cli`; now it contributes + // nothing, and `collectReviewerWarnings` says so (see section K of + // tests/reviewer-manifest-body.test.cjs). const registry = { capabilities: { 'legacy-cli': { role: 'runtime', runtime: { hostBehaviors: { reviewerCli: true } } }, }, }; - assert.deepEqual(deriveReviewerSlugs(registry), ['legacy-cli']); + assert.deepEqual(deriveReviewerSlugs(registry), []); + }); + + test('aliasWithAnyValueContributesNoSlug', () => { + // Value sweep: the old branch was a strict `=== true`, so `false`/`"true"`/`1` + // never contributed even before. Locking all of them at once means a partial + // revert that restores only the truthy branch is still caught. + for (const value of [true, false, 'true', 0, 1, null, {}, []]) { + const registry = { + capabilities: { + 'legacy-cli': { role: 'runtime', runtime: { hostBehaviors: { reviewerCli: value } } }, + }, + }; + assert.deepEqual( + deriveReviewerSlugs(registry), [], + `reviewerCli = ${JSON.stringify(value)} must contribute no slug`, + ); + } }); test('nonReviewerCapabilityContributesNoSlug', () => { @@ -366,10 +403,9 @@ describe('C. Roster derivation — src/review-reviewer-selection.cts', () => { assert.equal(reviewerSelectionModule.NON_RUNTIME_REVIEWER_SLUGS, undefined); }); - test('reviewerBodyWinsOverTheLegacyAlias', () => { - // Body and alias disagree on membership: capId (what the alias would - // contribute) differs from reviewer.slug (what the body contributes), so - // the winner is unambiguous from the result alone. + test('declaredBodyIsUnaffectedByAVestigialAliasKey', () => { + // capId (what the removed alias used to contribute) differs from + // reviewer.slug, so the result alone shows which surface was read. const registry = { capabilities: { 'conflicting-cap-id': { @@ -382,7 +418,7 @@ describe('C. Roster derivation — src/review-reviewer-selection.cts', () => { const roster = deriveReviewerSlugs(registry); assert.deepEqual( roster, ['the-declared-slug'], - `expected only the declared body's slug to win over the alias, got: ${JSON.stringify(roster)}`, + `only the declared body's slug may contribute, got: ${JSON.stringify(roster)}`, ); }); @@ -392,10 +428,13 @@ describe('C. Roster derivation — src/review-reviewer-selection.cts', () => { }); test('rosterIsStableRegardlessOfRegistryOrder', () => { + // `gamma` was an alias-only capability before #2801. It is now body-declared + // so this row still proves a THREE-way sort; shrinking it to two entries + // would quietly weaken the ordering guarantee it exists to protect. const capsForward = { alpha: { role: 'reviewer', reviewer: { slug: 'zzz-lane' } }, beta: { role: 'reviewer', reviewer: { slug: 'aaa-lane' } }, - gamma: { role: 'runtime', runtime: { hostBehaviors: { reviewerCli: true } } }, + gamma: { role: 'runtime', runtime: { hostBehaviors: {} }, reviewer: { slug: 'gamma' } }, }; const reversedCaps = {}; for (const key of Object.keys(capsForward).reverse()) reversedCaps[key] = capsForward[key]; @@ -408,6 +447,70 @@ describe('C. Roster derivation — src/review-reviewer-selection.cts', () => { 'roster must be sorted, independent of declaration order', ); }); + + test('derivedRosterNeverAdmitsAnAliasOnlyCapability', () => { + // P1 — property over arbitrary registries. Three invariants at once: + // (1) no capability that declares ONLY the removed alias ever reaches the + // roster, (2) the roster is sorted and duplicate-free, and (3) it does not + // depend on key insertion order. A partial revert of the alias branch is + // caught by (1) at any registry shape, not just the handful enumerated above. + fc.assert( + fc.property( + fc.array( + fc.record({ + capId: fc.string({ minLength: 1, maxLength: 8 }).filter((s) => s.trim().length > 0), + declaresBody: fc.boolean(), + slug: fc.string({ minLength: 1, maxLength: 8 }).filter((s) => s.trim().length > 0), + aliasValue: fc.constantFrom(true, false, 'true', 1, 0, null, undefined), + }), + { minLength: 0, maxLength: 12 }, + ), + (specs) => { + const capabilities = {}; + const aliasOnlyIds = new Set(); + const declaredSlugs = new Set(); + for (const s of specs) { + if (s.capId === '__proto__' || s.capId === 'constructor' || s.capId === 'prototype') continue; + const cap = { role: 'runtime', runtime: { hostBehaviors: {} } }; + if (s.aliasValue !== undefined) cap.runtime.hostBehaviors.reviewerCli = s.aliasValue; + if (s.declaresBody) { + cap.reviewer = { slug: s.slug }; + declaredSlugs.add(s.slug.trim()); + } else if (s.aliasValue !== undefined) { + aliasOnlyIds.add(s.capId); + } + capabilities[s.capId] = cap; + } + + const roster = deriveReviewerSlugs({ capabilities }); + + // (1) An alias-only capability's id may appear ONLY if some other + // capability legitimately declared it as a body slug. + for (const id of aliasOnlyIds) { + if (declaredSlugs.has(id)) continue; + assert.equal( + roster.includes(id), false, + `alias-only capability "${id}" must not reach the roster; roster=${JSON.stringify(roster)}`, + ); + } + + // (2) sorted + unique + assert.deepEqual(roster, [...roster].sort(), `roster must be sorted, got: ${JSON.stringify(roster)}`); + assert.equal(new Set(roster).size, roster.length, `roster must be duplicate-free, got: ${JSON.stringify(roster)}`); + + // (3) order-independent + const reversed = {}; + for (const key of Object.keys(capabilities).reverse()) reversed[key] = capabilities[key]; + assert.deepEqual( + deriveReviewerSlugs({ capabilities: reversed }), roster, + 'roster must not depend on registry key insertion order', + ); + return true; + }, + ), + { numRuns: 200, seed: 2801 }, + ); + }); }); // ─── D. Cross-phase invariants that must not regress ──────────────────────── @@ -528,7 +631,7 @@ describe('E. Lane fidelity — no translation layer', () => { } assert.equal(REVIEWER_LANES.length, 12, 'expected exactly 12 declared descriptor lanes'); - assert.equal(bySlug.size, 12, `expected exactly 11 capabilities declaring a reviewer body, got: ${bySlug.size}`); + assert.equal(bySlug.size, 12, `expected exactly 12 capabilities declaring a reviewer body, got: ${bySlug.size}`); // Top-level scalar/array fields compared whole; the two fields that are // themselves nested objects (probe, invoke) are compared sub-field-by- @@ -611,16 +714,27 @@ describe('F. Isolated-security-review regressions', () => { ); }); - test('theAliasStillAppliesWhenABodyDeclaresOnlyWhitespace', () => { - // A body whose slug is blank is NOT a declaration, so the legacy alias must - // still contribute — otherwise a malformed body would silently REMOVE a lane - // that worked before, which is worse than the blank slug itself. + test('aBlankBodyContributesNothingAndNoAliasRescuesIt', () => { + // INVERTED by Phase 7 (#2801). While the alias existed, a blank slug fell + // through to it, so a malformed body could not silently remove a lane that + // worked before — that was the point of the original row. With the alias + // gone there is nothing to fall through TO: a blank body is not a + // declaration, and a declaration is now the only route onto the roster. + // + // The row is inverted rather than deleted because it is the combination the + // single-variable rows miss, and because it is the security-review + // provenance for the trim: `deriveReviewerSlugs` is exported and carries no + // other validation, so a whitespace slug must never occupy a roster entry + // it can never match. const roster = deriveReviewerSlugs({ capabilities: { claude: { reviewer: { slug: ' ' }, runtime: { hostBehaviors: { reviewerCli: true } } }, }, }); - assert.deepEqual(roster, ['claude'], 'a blank body must fall through to the alias, not drop the lane'); + assert.deepEqual( + roster, [], + 'a blank body is not a declaration, and the removed alias cannot rescue it', + ); }); test('moduleLoadSurvivesAHostileRegistryShape', () => { @@ -629,7 +743,7 @@ describe('F. Isolated-security-review regressions', () => { // The module under test already imported successfully above; assert the // derived roster is a usable array rather than a partially-initialised value. assert.ok(Array.isArray([...KNOWN_REVIEWER_SLUGS]), 'roster must be iterable after module load'); - assert.equal(KNOWN_REVIEWER_SLUGS.length, 12, 'the real registry still yields the eleven lanes'); + assert.equal(KNOWN_REVIEWER_SLUGS.length, 12, 'the real registry still yields the twelve lanes'); // And the derivation itself is total over the shapes JSON can express. for (const hostile of [null, undefined, [], 0, 'x', { capabilities: null }, { capabilities: [] }]) { assert.doesNotThrow( diff --git a/tests/reviewer-manifest-body.test.cjs b/tests/reviewer-manifest-body.test.cjs index 652eff86e..7e8141e5e 100644 --- a/tests/reviewer-manifest-body.test.cjs +++ b/tests/reviewer-manifest-body.test.cjs @@ -3,7 +3,7 @@ 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`, + * (ADR-2782, chore #2795 Phase 2): `validateReviewerBody`, `collectReviewerWarnings` / `collectReviewerWarningRecords`, * the `role:'reviewer'` dispatch branch of `validateCapability`, the reviewer-lane * uniqueness rules inside `validateCrossCapability`, and the harvest widening in * `buildRegistry` / `loadAndValidate`. @@ -37,6 +37,9 @@ const { LANE_SLUG_RE, validateReviewerBody, collectReviewerWarnings, + collectReviewerWarningRecords, + REVIEWER_WARNING, + REMOVED_REVIEWER_CLI_FIELD, validateCapability, validateCrossCapability, VALID_LANE_EFFORT_CHANNELS, @@ -45,6 +48,9 @@ const { VALID_EVIDENCE_CLASSES, VALID_LANE_HANDLERS, KNOWN_REVIEWER_FIELDS, + KNOWN_HOST_BEHAVIORS, + MAX_REPORTED_UNKNOWN_KEYS, + MAX_REPORTED_KEY_CHARS, } = require('../gsd-core/bin/lib/capability-validator.cjs'); const { loadAndValidate, buildRegistry } = require('../scripts/gen-capability-registry.cjs'); @@ -268,14 +274,19 @@ describe('A. Body presence / shape', () => { 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)}`, - ); + // Asserted on the typed IR, not the rendered prose (CONTRIBUTING.md, + // "Prohibited: Raw Text Matching on Test Outputs"). The `message` field + // exists for operator console output only. + const records = collectReviewerWarningRecords(cap); + assert.equal(records.length, 1, `expected exactly one record, got: ${JSON.stringify(records)}`); + assert.equal(records[0].code, REVIEWER_WARNING.UNKNOWN_REVIEWER_FIELD); + assert.equal(records[0].capId, 'cap-x'); + assert.equal(records[0].field, 'reviewer.futureField'); + assert.deepEqual(records[0].knownFields, [...KNOWN_REVIEWER_FIELDS]); + + // The renderer still produces one string per record for the two production + // consumers (gen-capability-registry -> stderr, capability-loader -> OverlayMeta.warnings). + assert.equal(collectReviewerWarnings(cap).length, records.length); }); test('unknownRoleIsRejectedWithEnumeratedMembers', () => { @@ -1700,3 +1711,420 @@ describe('J. Property-based (fast-check)', () => { ); }); }); + +// ─── K. Removed `hostBehaviors.reviewerCli` alias (ADR-2782 D9, chore #2801) ── +// +// Phase 7 deletes the derived legacy alias. `collectReviewerWarnings` is the +// channel the removal announces itself on, because it is already wired to BOTH +// surfaces a manifest can arrive through: the build-time generator +// (`gen-capability-registry.cjs` -> stderr) and the overlay loader +// (`capability-loader.cts` -> OverlayMeta.warnings, on the ACCEPT path for every +// accepted capability). An out-of-tree manifest still setting the alias reaches +// the second one. +// +// Rows K1-K9 implement W1-W9 of +// `.gsd/phase/chore-2801-remove-reviewercli-alias/50-test-matrix.md`. +// +// The load-bearing structural fact these rows pin down: the removal check must +// run BEFORE `collectReviewerWarningRecordFields`' `reviewer`-body early-return. An +// alias-only manifest — precisely the case the deprecation window existed for — +// has no `reviewer` body, so a check placed after that guard would fire only for +// capabilities that do not need it. K1 is the row that fails if it is misplaced. + +/** A whole runtime manifest — the shape production passes to this function. */ +function runtimeCapWithHostBehaviors(hostBehaviors, extra = {}) { + return { + id: 'legacy-cli', + role: 'runtime', + runtime: { hostBehaviors }, + ...extra, + }; +} + +describe('K. Removed hostBehaviors.reviewerCli alias (#2801)', () => { + /** Records for the removal notice only, keyed on the typed code. */ + function removalRecords(cap) { + return collectReviewerWarningRecords(cap) + .filter((rec) => rec.code === REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR); + } + + test('reviewerWarningCodeSurfaceIsLocked', () => { + // The third of the three coordinated changes a new code requires. Without + // this, a code can be added or renamed with no test noticing. + assert.deepEqual( + Object.keys(REVIEWER_WARNING).sort(), + ['REMOVED_HOST_BEHAVIOR', 'UNKNOWN_HOST_BEHAVIOR', 'UNKNOWN_REVIEWER_FIELD'], + ); + assert.equal(Object.isFrozen(REVIEWER_WARNING), true, 'the code enum must be frozen'); + assert.equal(REMOVED_REVIEWER_CLI_FIELD, 'runtime.hostBehaviors.reviewerCli'); + }); + + test('removedReviewerCliAliasWarnsWhenPresentWithoutABody', () => { + // No `reviewer` body at all — the alias-only manifest. This is the row that + // proves the check runs before the body early-return. + const cap = runtimeCapWithHostBehaviors({ reviewerCli: true }); + const records = removalRecords(cap); + assert.equal( + records.length, 1, + `expected exactly one removal record for an alias-only manifest, got: ${JSON.stringify(collectReviewerWarningRecords(cap))}`, + ); + assert.equal(records[0].capId, 'legacy-cli'); + assert.equal(records[0].field, REMOVED_REVIEWER_CLI_FIELD); + }); + + test('removedReviewerCliAliasWarnsAlongsideADeclaredBody', () => { + const cap = runtimeCapWithHostBehaviors({ reviewerCli: true }, { reviewer: validLane() }); + assert.equal(removalRecords(cap).length, 1, 'a declared body must not suppress the removal notice'); + assert.deepEqual( + validateReviewerBody(cap), [], + 'the vestigial key must stay a WARNING — never a validation error (Postel: liberal in what we accept)', + ); + }); + + test('removedReviewerCliAliasWarnsRegardlessOfItsValue', () => { + // Presence-based, deliberately: after removal the key is unknown at ANY + // value, exactly as an unknown `reviewer.*` field is. A value-sensitive + // warning would tell an author carrying `reviewerCli: false` that their + // stale key is fine, when it is simply dead. + // (40-design.md -> Rejected 3.) + for (const value of [true, false, 'true', 0, 1, null, {}, []]) { + const cap = runtimeCapWithHostBehaviors({ reviewerCli: value }); + assert.equal( + removalRecords(cap).length, 1, + `expected a removal record for reviewerCli = ${JSON.stringify(value)}, got: ${JSON.stringify(collectReviewerWarningRecords(cap))}`, + ); + } + }); + + test('similarlyNamedHostBehaviorKeysAreNotTheRemovedField', () => { + // Exact own-key match only: a near-miss name must never be reported as the + // removed `reviewerCli`. Since #2801 closed the vocabulary these names DO + // now draw an unknown-host-behavior notice, which is correct — they are not + // declared behaviors — but they must not draw the removal notice. + const cap = runtimeCapWithHostBehaviors({ + reviewerCliPath: '/usr/bin/thing', + reviewer_cli: true, + reviewerCLI: true, + reapplyCommand: 'x', + }); + const records = collectReviewerWarningRecords(cap); + assert.deepEqual( + records.filter((rec) => rec.code === REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR), [], + 'only the exact own key `reviewerCli` is the removed field', + ); + assert.deepEqual( + records.map((rec) => rec.field).sort(), + [ + 'runtime.hostBehaviors.reviewerCLI', + 'runtime.hostBehaviors.reviewer_cli', + 'runtime.hostBehaviors.reviewerCliPath', + ].sort(), + 'the three undeclared names draw an unknown-host-behavior notice; the declared reapplyCommand does not', + ); + }); + + test('malformedHostBehaviorsNeitherWarnsNorThrows', () => { + const shapes = [ + ['null', { id: 'c', role: 'runtime', runtime: { hostBehaviors: null } }], + ['array', { id: 'c', role: 'runtime', runtime: { hostBehaviors: [] } }], + ['string', { id: 'c', role: 'runtime', runtime: { hostBehaviors: 'reviewerCli' } }], + ['number', { id: 'c', role: 'runtime', runtime: { hostBehaviors: 42 } }], + ['empty object', { id: 'c', role: 'runtime', runtime: { hostBehaviors: {} } }], + ['no hostBehaviors', { id: 'c', role: 'runtime', runtime: {} }], + ['no runtime', { id: 'c', role: 'reviewer', reviewer: validLane() }], + ['runtime null', { id: 'c', role: 'runtime', runtime: null }], + ]; + for (const [name, cap] of shapes) { + let records; + try { + records = collectReviewerWarningRecords(cap); + } catch (err) { + assert.fail(`collectReviewerWarningRecords threw for ${name}: ${err && err.message}`); + } + assert.deepEqual( + records.filter((rec) => rec.code === REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR), [], + `${name} must not produce a removal record`, + ); + } + }); + + test('removalWarningAndUnknownFieldWarningCoexist', () => { + // Two independent diagnostics on one manifest. Neither may swallow the other + // — an early `return` after the first would hide the second. + const lane = validLane(); + lane.futureField = 'from-a-newer-gsd'; + const cap = runtimeCapWithHostBehaviors({ reviewerCli: true }, { id: 'both-cap', reviewer: lane }); + + const records = collectReviewerWarningRecords(cap); + assert.deepEqual( + records.map((rec) => rec.code).sort(), + [REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR, REVIEWER_WARNING.UNKNOWN_REVIEWER_FIELD].sort(), + `expected exactly one of each code, got: ${JSON.stringify(records)}`, + ); + }); + + test('inheritedReviewerCliFromPrototypeDoesNotWarn', () => { + // Own-key read: a polluted prototype must not manufacture a removal record + // on every otherwise-innocent manifest. + const polluted = Object.create({ reviewerCli: true }); + polluted.reapplyCommand = 'x'; + const cap = runtimeCapWithHostBehaviors(polluted); + assert.deepEqual( + collectReviewerWarningRecords(cap), [], + 'an inherited reviewerCli is not a declared field', + ); + }); + + test('removalWarningNamesTheReviewerBodyReplacement', () => { + // A removal notice that does not say what to do instead is not a migration + // path. The field was undocumented for its whole life and only documented at + // 1.9.0 as ALREADY deprecated, so we cannot enumerate who depends on it + // (Hyrum) — the exit has to carry its own instructions. Asserted on the + // typed fields, never on the rendered sentence. + const [record] = removalRecords(runtimeCapWithHostBehaviors({ reviewerCli: true })); + assert.ok(record, 'expected a removal record'); + assert.equal(record.replacement, 'reviewer'); + assert.equal(record.docs, 'docs/how-to/ship-a-reviewer-lane.md'); + }); + + test('renderedStringsStayOneToOneWithRecords', () => { + // The two production consumers still receive strings; the renderer must not + // drop or duplicate a diagnostic. + const lane = validLane(); + lane.futureField = 'x'; + for (const cap of [ + runtimeCapWithHostBehaviors({ reviewerCli: true }), + runtimeCapWithHostBehaviors({ reviewerCli: true }, { reviewer: lane }), + runtimeCapWithHostBehaviors({ reapplyCommand: 'x' }), + ]) { + const records = collectReviewerWarningRecords(cap); + const strings = collectReviewerWarnings(cap); + assert.equal(strings.length, records.length); + assert.deepEqual(strings, records.map((rec) => rec.message)); + } + }); + + test('collectReviewerWarningsStaysTotalOverTheNewHostBehaviorsReadPath', () => { + // W9 — the totality contract (#1461 OVL-1) now covers a second read path. + // A throwing getter or Proxy trap fires on the READ, before any message is + // built, so only the structural wrapper can save these. Both the IR and the + // renderer must survive, since the renderer maps over the IR. + const throwing = () => { throw new Error('boom'); }; + + const hostBehaviorsGetterThrows = { id: 'x', role: 'runtime', runtime: {} }; + Object.defineProperty(hostBehaviorsGetterThrows.runtime, 'hostBehaviors', { get: throwing }); + + const reviewerCliGetterThrows = { id: 'x', role: 'runtime', runtime: { hostBehaviors: {} } }; + Object.defineProperty(reviewerCliGetterThrows.runtime.hostBehaviors, 'reviewerCli', { get: throwing }); + + const cases = [ + ['runtime getter throws', Object.defineProperty({ id: 'x' }, 'runtime', { get: throwing })], + ['hostBehaviors getter throws', hostBehaviorsGetterThrows], + ['reviewerCli getter throws', reviewerCliGetterThrows], + ['hostBehaviors Proxy traps throw', { + id: 'x', + role: 'runtime', + runtime: { hostBehaviors: new Proxy({}, { has: throwing, get: throwing, getOwnPropertyDescriptor: throwing, ownKeys: throwing }) }, + }], + ]; + + for (const [name, cap] of cases) { + let records; + let strings; + try { + records = collectReviewerWarningRecords(cap); + strings = collectReviewerWarnings(cap); + } catch (err) { + assert.fail(`${name}: threw ${err && err.message}`); + } + assert.ok(Array.isArray(records), `${name}: records must always be an array`); + assert.ok(Array.isArray(strings), `${name}: strings must always be an array`); + } + }); +}); + +// ─── L. Closed `hostBehaviors` vocabulary (ADR-1016, closed by #2801) ──────── + +describe('L. Closed hostBehaviors vocabulary (#2801)', () => { + const ROOT = path.resolve(__dirname, '..'); + + /** Every hostBehaviors key the shipped manifests actually declare. */ + function shippedHostBehaviorKeys() { + const keys = new Set(); + const capsDir = path.join(ROOT, 'capabilities'); + for (const dir of fs.readdirSync(capsDir, { withFileTypes: true })) { + if (!dir.isDirectory()) continue; + const file = path.join(capsDir, dir.name, 'capability.json'); + if (!fs.existsSync(file)) continue; + const cap = JSON.parse(fs.readFileSync(file, 'utf8')); + const hb = cap && cap.runtime && cap.runtime.hostBehaviors; + if (hb && typeof hb === 'object' && !Array.isArray(hb)) { + for (const key of Object.keys(hb)) keys.add(key); + } + } + return keys; + } + + test('vocabularyExactlyMatchesWhatTheShippedManifestsDeclare', () => { + // DEFECT.GENERATIVE-FIX: two surfaces, one truth. A key added to a manifest + // without being declared here would warn on every build; a key left here + // after its last manifest drops it is dead vocabulary. Both directions fail. + const shipped = shippedHostBehaviorKeys(); + assert.deepEqual( + [...shipped].sort(), [...KNOWN_HOST_BEHAVIORS].sort(), + 'the closed vocabulary and the shipped manifests must name the same keys', + ); + }); + + test('noShippedCapabilityDrawsAHostBehaviorWarning', () => { + // The closure must be inert for everything that ships today. If this fails, + // closing the vocabulary broke a real capability rather than a hypothetical one. + const capsDir = path.join(ROOT, 'capabilities'); + const offenders = []; + for (const dir of fs.readdirSync(capsDir, { withFileTypes: true })) { + if (!dir.isDirectory()) continue; + const file = path.join(capsDir, dir.name, 'capability.json'); + if (!fs.existsSync(file)) continue; + const cap = JSON.parse(fs.readFileSync(file, 'utf8')); + for (const rec of collectReviewerWarningRecords(cap)) { + if (rec.code === REVIEWER_WARNING.UNKNOWN_HOST_BEHAVIOR + || rec.code === REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR) { + offenders.push(`${dir.name}: ${rec.field}`); + } + } + } + assert.deepEqual(offenders, [], `no shipped capability may draw a hostBehaviors notice, got: ${JSON.stringify(offenders)}`); + }); + + test('anUndeclaredHostBehaviorWarnsAndIsNotAnError', () => { + const cap = runtimeCapWithHostBehaviors({ someFutureSwitch: true }); + const records = collectReviewerWarningRecords(cap); + assert.equal(records.length, 1, `expected one record, got: ${JSON.stringify(records)}`); + assert.equal(records[0].code, REVIEWER_WARNING.UNKNOWN_HOST_BEHAVIOR); + assert.equal(records[0].field, 'runtime.hostBehaviors.someFutureSwitch'); + // Forward-compat invariant: a warning, never a validation error. + assert.deepEqual(validateCapability({ ...cap, version: '1.0.0' }, cap.id).filter((e) => e.includes('someFutureSwitch')), []); + }); + + test('aDeclaredHostBehaviorIsSilentAtAnyValue', () => { + for (const value of [true, false, 'x', 0, null, {}, []]) { + const cap = runtimeCapWithHostBehaviors({ reapplyCommand: value }); + assert.deepEqual( + collectReviewerWarningRecords(cap), [], + `a declared key must be silent regardless of value, got value ${JSON.stringify(value)}`, + ); + } + }); + + test('theRemovedAliasDrawsItsOwnNoticeNotTheGenericOne', () => { + // reviewerCli is excluded from the unknown-key sweep on purpose: it has a + // migration pointer the generic notice does not carry, and two records for + // one key would be noise. + const records = collectReviewerWarningRecords(runtimeCapWithHostBehaviors({ reviewerCli: true })); + assert.equal(records.length, 1, `expected exactly one record, got: ${JSON.stringify(records)}`); + assert.equal(records[0].code, REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR); + assert.equal(records[0].replacement, 'reviewer'); + }); + + test('reservedKeysInTheBagAreIgnoredNotWarned', () => { + const hostile = JSON.parse('{"__proto__": {"polluted": true}, "constructor": 1, "prototype": 2, "reapplyCommand": "x"}'); + const cap = runtimeCapWithHostBehaviors(hostile); + let records; + try { + records = collectReviewerWarningRecords(cap); + } catch (err) { + assert.fail(`collectReviewerWarningRecords threw: ${err && err.message}`); + } + assert.deepEqual(records, [], 'reserved names are skipped, not reported as unknown behaviors'); + assert.equal({}.polluted, undefined, 'Object.prototype must not be polluted'); + }); +}); + +// ─── M. Diagnostics are bounded and control-safe (#2801 review findings) ───── +// +// Both loops iterate MANIFEST-SUPPLIED keys. An installed third-party manifest +// is attacker-controlled and bounded only by MANIFEST_MAX_BYTES (8MB), so the +// record count and each key's rendered length must both have a ceiling, and a +// key must not be able to carry terminal escapes or a forged newline into +// stderr / OverlayMeta.warnings. + +describe('M. Diagnostics are bounded and control-safe (#2801)', () => { + function manyUnknownHostBehaviors(n) { + const hb = {}; + for (let i = 0; i < n; i += 1) hb['undeclaredKey' + i] = true; + return runtimeCapWithHostBehaviors(hb); + } + + test('unknownHostBehaviorRecordsAreCappedWithASummary', () => { + const n = MAX_REPORTED_UNKNOWN_KEYS + 25; + const records = collectReviewerWarningRecords(manyUnknownHostBehaviors(n)); + assert.equal( + records.length, MAX_REPORTED_UNKNOWN_KEYS + 1, + `expected ${MAX_REPORTED_UNKNOWN_KEYS} records plus one summary, got ${records.length}`, + ); + const summary = records[records.length - 1]; + assert.equal(summary.truncated, true); + assert.equal(summary.omittedCount, 25); + assert.equal(summary.field, 'runtime.hostBehaviors'); + }); + + test('exactlyAtTheCapThereIsNoSummaryRecord', () => { + // limit-1 / limit / limit+1 around the ceiling. + const below = collectReviewerWarningRecords(manyUnknownHostBehaviors(MAX_REPORTED_UNKNOWN_KEYS - 1)); + assert.equal(below.length, MAX_REPORTED_UNKNOWN_KEYS - 1); + assert.equal(below.some((rec) => rec.truncated), false); + + const at = collectReviewerWarningRecords(manyUnknownHostBehaviors(MAX_REPORTED_UNKNOWN_KEYS)); + assert.equal(at.length, MAX_REPORTED_UNKNOWN_KEYS); + assert.equal(at.some((rec) => rec.truncated), false, 'no summary when nothing was omitted'); + + const above = collectReviewerWarningRecords(manyUnknownHostBehaviors(MAX_REPORTED_UNKNOWN_KEYS + 1)); + assert.equal(above.length, MAX_REPORTED_UNKNOWN_KEYS + 1); + assert.equal(above[above.length - 1].omittedCount, 1); + }); + + test('unknownReviewerFieldRecordsAreCappedTheSameWay', () => { + const lane = validLane(); + for (let i = 0; i < MAX_REPORTED_UNKNOWN_KEYS + 5; i += 1) lane['futureField' + i] = 1; + const records = collectReviewerWarningRecords({ id: 'cap-x', reviewer: lane }); + assert.equal(records.length, MAX_REPORTED_UNKNOWN_KEYS + 1); + assert.equal(records[records.length - 1].truncated, true); + assert.equal(records[records.length - 1].omittedCount, 5); + }); + + test('controlCharactersInAKeyNeverReachTheDiagnostic', () => { + // ESC-based colour sequence, a CR overwrite, and an embedded newline that + // would forge a second log line. + const hostile = '\x1b[31mred\x1b[0m\r\nforged: everything is fine'; + for (const cap of [ + runtimeCapWithHostBehaviors({ [hostile]: true }), + { id: 'cap-x', reviewer: { slug: 'x', [hostile]: true } }, + ]) { + for (const rec of collectReviewerWarningRecords(cap)) { + // eslint-disable-next-line no-control-regex + assert.equal(/[\x00-\x1f\x7f-\x9f]/.test(rec.field), false, `control char survived into field: ${JSON.stringify(rec.field)}`); + // eslint-disable-next-line no-control-regex + assert.equal(/[\x00-\x1f\x7f-\x9f]/.test(rec.message), false, `control char survived into message: ${JSON.stringify(rec.message)}`); + } + } + }); + + test('anEnormousKeyNameIsClipped', () => { + const huge = 'k'.repeat(5000); + const [record] = collectReviewerWarningRecords(runtimeCapWithHostBehaviors({ [huge]: true })); + assert.ok(record, 'expected a record'); + assert.ok( + record.field.length < MAX_REPORTED_KEY_CHARS + 40, + `field must be bounded, got length ${record.field.length}`, + ); + assert.ok(record.field.endsWith('…'), 'a clipped key is marked as clipped'); + }); + + test('aDeclaredKeyIsNeverClippedOrAltered', () => { + // The sanitizer must not perturb the ordinary case: declared keys are silent, + // and an undeclared but well-formed key is reported verbatim. + const records = collectReviewerWarningRecords(runtimeCapWithHostBehaviors({ someFutureSwitch: true })); + assert.equal(records.length, 1); + assert.equal(records[0].field, 'runtime.hostBehaviors.someFutureSwitch'); + }); +});