diff --git a/.changeset/clever-eagles-wake.md b/.changeset/clever-eagles-wake.md new file mode 100644 index 000000000..0c5ac28b1 --- /dev/null +++ b/.changeset/clever-eagles-wake.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3253 +--- +**Capability skills are now named at the install consent prompt** — installing a third-party capability whose only contribution was skills printed "ships no executable surfaces (declarative only)" and listed nothing, even though each `SKILL.md` body lands verbatim in your agent's instruction context. The pre-install disclosure now names every contributed skill in its own section and states plainly that the bodies are not content-scanned. Values interpolated into the prompt are escaped across every disclosed surface, so a crafted name can no longer forge additional lines of disclosure text. No stored consent is disturbed and no re-consent prompt fires. (#3248) diff --git a/CONTEXT.md b/CONTEXT.md index 12a006a8f..e7356ab9a 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -274,7 +274,7 @@ Issue #1459 user-owned consent seam (`gsd-core/bin/lib/capability-consent.cjs`, Issue #1459 finding 4 shared cross-process lock primitive (`gsd-core/bin/lib/capability-lock.cjs`, generated from `src/capability-lock.cts`). Leaf module (`node:fs`/`node:path`/`node:os`/`node:crypto` + the ledger's bounded `readSmallRegularFile` + `shell-command-projection`'s `execTool` for the rare start-time shell-out). THE single hardened lockfile protocol shared by BOTH `capability-lifecycle` (the `.gsd/capabilities/.lock` mutation lock) and `capability-consent` (the consent-store `.consent.lock`) — extracted so the two locks cannot diverge (mirrors the shared-validator / shared bounded-reader lessons). Exports: `acquireLock(lockPath, opts?)` (O_EXCL create with a JSON `{token,pid,hostname,startTime,ts}` body; steal protocol binds age to the body's own `ts`, never stale-steals a VERIFIED-LIVE same-host holder — pid alive AND recorded start-time matches the pid's current start-time, defeating pid-reuse without ever stealing a live holder — and reclaims only a dead/unverifiable holder via the dead-pid fast path or the hard `LOCK_DEADMAN_MS` deadman; `opts.maxAttempts` raises the bounded retry budget and `opts.waitForFresh` makes a contended fresh/live holder be WAITED FOR rather than failed-fast so genuinely-racing consent writers serialize), `releaseLock(handle)` (token + inode owner-safe — never deletes a successor's lock), `getProcessStartTime`, and the `_setLockProbes`/`_resetLockProbes` test seams. Carries the #1462 lifecycle-lock invariants (process-start-time liveness, TOCTOU-safe pre-rename identity recheck, bounded iterative loop). ### Capability Trust Gate -ADR-1244 Phase 4 (D5) PURE policy module (`gsd-core/bin/lib/capability-trust.cjs`). Computes *what* a capability would do and *whether* policy permits it; performs no mutation and no I/O beyond existence-checking declared artifacts. Exports: `discloseExecutableSurfaces(manifest, stagedDir?, resolveHost?)` (enumerates the four executable surfaces — `hooks`, command modules, `mcpServers`, and reviewer lanes (ADR-2782 D5) — and flags `hasExecutable`; a reviewer lane is the one class that *receives* data, so it discloses its binary + full args (spawn) or destination host + `hostConfigKey` (openai-http) together with the egress payload classes); `evaluateInstallTrust(args)` (composes source policy + reserved-namespace + engines gate + disclosure into `{ allowed, requiresConsent, disclosure, engines, blockReasons }`); `evaluateSourceAllowed(parsed, strictKnownRegistries)` enforcing `capabilities.strict_known_registries` (unset/null → permissive-with-consent; `[]` → block all external; non-empty → host-based allowlist, never substring); `checkEngines(manifest, hostVersion)` (engines.gsd hard gate via `semverSatisfies` + `compatVersions` graceful-downgrade picking the newest working version); `executableSetChanged(old, new)` (auto-update re-consent trigger); `checkReservedNamespace` (`gsd-`/`gsd-core-`/`anthropic-`). The MCP disclosure also captures each server's `env` (string→string, filtered) and `cwd` (#1459) — `disclosureSignature` folds them in as STABLE SORTED JSON so any env/cwd add/change forces re-consent while a key reorder does not; `signatureForManifest(manifest, stagedDir?)` is the single source of truth for that signature (consumed by the loader's consent check and the lifecycle's consent binding). #1459 finding 5: each MCP surface also carries `rawConfig` — the FULL declared server config the writer persists (`{...config}`), prototype-pollution-cleaned — folded into the signature as STABLE SORTED JSON so a change to ANY persisted field (not just the explicit whitelist — a future `envFile`/`workingDir`/launch option) forces re-consent, while a pure key reorder does not; the human summary stays readable via the key fields only. The barrier is consent + integrity + reversibility, NOT a sandbox — see `docs/explanation/capability-trust-model.md`. +ADR-1244 Phase 4 (D5) PURE policy module (`gsd-core/bin/lib/capability-trust.cjs`). Computes *what* a capability would do and *whether* policy permits it; performs no mutation and no I/O beyond existence-checking declared artifacts. Exports: `discloseExecutableSurfaces(manifest, stagedDir?, resolveHost?)` (enumerates the four executable surfaces — `hooks`, command modules, `mcpServers`, and reviewer lanes (ADR-2782 D5) — plus a fifth, non-executable class, instruction surfaces (`skills` stems only, ADR-2363 D5, #3248), returned as `instructionSurfaces`; flags `hasExecutable` from the four executable classes only — instruction surfaces deliberately do NOT contribute to it; a reviewer lane is the one class that *receives* data, so it discloses its binary + full args (spawn) or destination host + `hostConfigKey` (openai-http) together with the egress payload classes); `evaluateInstallTrust(args)` (composes source policy + reserved-namespace + engines gate + disclosure into `{ allowed, requiresConsent, disclosure, engines, blockReasons }`); `evaluateSourceAllowed(parsed, strictKnownRegistries)` enforcing `capabilities.strict_known_registries` (unset/null → permissive-with-consent; `[]` → block all external; non-empty → host-based allowlist, never substring); `checkEngines(manifest, hostVersion)` (engines.gsd hard gate via `semverSatisfies` + `compatVersions` graceful-downgrade picking the newest working version); `executableSetChanged(old, new)` (auto-update re-consent trigger); `checkReservedNamespace` (`gsd-`/`gsd-core-`/`anthropic-`); `collectInstructionSurfaces(manifest)` (the instruction-surface collector — `skills` stems only, independently testable, same total/`safeCollect` contract as the four executable collectors; ADR-2363 D3 classifies `agents` as an instruction surface too, but a third-party capability's declared `agents[]` are never staged into the agent's instruction context — `stageAgentsForRuntimeWithConverter` (`src/install-profiles.cts`) has no registry-aware third-party path the way `readInstalledCapabilitySkill` gives skills — so disclosing them would name a surface that does not exist; agents stay unimplemented pending a maintainer decision, and are NOT thereby safe or inert, only undisclosed); `summarizeInstructionSurfaces(disclosure)` (renders the instruction-surface section of the consent summary; called from BOTH branches of `summarizeDisclosure` because a skill-only capability has `hasExecutable === false` and takes the early return, so a section appended only at the end would never render for exactly the capabilities that need it). The MCP disclosure also captures each server's `env` (string→string, filtered) and `cwd` (#1459) — `disclosureSignature` folds them in as STABLE SORTED JSON so any env/cwd add/change forces re-consent while a key reorder does not; `signatureForManifest(manifest, stagedDir?)` is the single source of truth for that signature (consumed by the loader's consent check and the lifecycle's consent binding). #1459 finding 5: each MCP surface also carries `rawConfig` — the FULL declared server config the writer persists (`{...config}`), prototype-pollution-cleaned — folded into the signature as STABLE SORTED JSON so a change to ANY persisted field (not just the explicit whitelist — a future `envFile`/`workingDir`/launch option) forces re-consent, while a pure key reorder does not; the human summary stays readable via the key fields only. Instruction surfaces are deliberately EXCLUDED from `disclosureSignature` (ADR-2363 D4, #3248) — a manifest gaining, losing, or changing `skills` produces a byte-identical signature and disturbs no stored consent record; any future binding arrives as a versioned v2, never an in-place re-encoding of v1. The barrier is consent + integrity + reversibility, NOT a sandbox — see `docs/explanation/capability-trust-model.md`. ### Capability Lifecycle ADR-1244 Phase 4 (D5+D6) orchestration seam (`gsd-core/bin/lib/capability-lifecycle.cjs`) composing the source resolver, ledger, and trust gate into the mutating operations. Exports: `installCapability` (pre-fetch source gate → resolve copy-only with `promote:false` → trust verdict → promote + apply marker-stamped shared edits → **ledger commit**; nothing written on block/abort), `upgradeCapability` (atomic stage-then-swap: old set aside, new swapped in, shared edits re-derived, **ledger committed**, backup dropped; re-prompts when the executable set changed), `removeCapability` (strip only `_gsdCapability`-marked shared-config entries — user hand-edits preserved — delete exactly the ledger-recorded files, then drop the entry; `CAPABILITY_DATA` preserved unless `removeData`), `reconcileCapabilities` (crash recovery driven by the ledger's `_pending {kind,backupName,sharedFiles}` INTENT — not a version comparison: roll an uncommitted upgrade back by restoring the backup, an uncommitted fresh install away entirely, and re-sync shared config from the winning bundle, guaranteeing no half-state), plus `applyCapabilitySharedEdits`/`stripCapabilitySharedEdits` (marker-isolated JSON edits, prototype-pollution-guarded). All four mutating ops + reconcile take a cross-process lock (`.gsd/capabilities/.lock`, atomic stale-steal) so a concurrent reconcile can't clear a live intent. Capability code never executes during any operation. The source resolver's `promote:false`/`skipEnginesGate` options are the seams that let this module own the swap/commit ordering and the engines gate (with `compatVersions` downgrade hint). diff --git a/docs/adr/2363-capability-instruction-surface-trust.md b/docs/adr/2363-capability-instruction-surface-trust.md index 7fcbcb3c1..859ca67d7 100644 --- a/docs/adr/2363-capability-instruction-surface-trust.md +++ b/docs/adr/2363-capability-instruction-surface-trust.md @@ -1,6 +1,6 @@ # ADR-2363: A capability's skill body is an instruction surface — trusted, unscanned, and disclosed -- **Status:** Proposed — D1–D4 are decided; **D5's mechanism is unshipped** and is tracked by [#3248](https://github.com/open-gsd/gsd-core/issues/3248) (Phase 1). Per the corpus lifecycle rule, an ADR with an outstanding phase stays `Proposed`. Ratify when #3248 has merged and the consent summary renders the instruction surface. +- **Status:** Proposed — D1–D4 are decided. D5's mechanism ships in [#3248](https://github.com/open-gsd/gsd-core/issues/3248), which is open and unmerged; this ADR ratifies to `Accepted` when that PR merges (see the ratification bar under Consequences). - **Date:** 2026-08-09 - **Issue:** [#2363](https://github.com/open-gsd/gsd-core/issues/2363) (epic); Phase 0 tracked by [#3247](https://github.com/open-gsd/gsd-core/issues/3247), Phase 1 by [#3248](https://github.com/open-gsd/gsd-core/issues/3248) - **Amends (prospective — this ADR is `Proposed`, so no back-link is owed yet):** [ADR-1244](1244-capability-ecosystem.md) — D5's disclosure gains a **fifth** class, and it is the first that is *not* an executable surface. [ADR-2782](2782-reviewer-lane-capability-surface.md) added the fourth (reviewer lanes); this adds the first non-executable one, which is why it needs its own classification rather than a fifth entry in the same list. @@ -91,15 +91,17 @@ Point 3 is the part that makes this a decision rather than a punt: the door stay **The residual gap, stated plainly — and it is smaller than it first looks.** At **project** scope there is no gap at all: `bundleContentHash` walks every entry under the bundle with no exclusions and hashes every regular file (failing closed on symlinks and non-regular files), so a single changed byte in a skill body already deactivates a project-scoped capability until re-consent. At **global** scope there is no consent record in the first place — a global install is trusted because it sits under the user's own home ([ADR-1244](1244-capability-ecosystem.md) D5) — so a skill-body change on upgrade is not consent-gated there, and would not have been even if instruction surfaces were signature-bound. **The gap global scope has is the one it already had for code, and D4 neither widens nor narrows it.** What D4 declines to add is a re-consent *prompt* on skill-body change during an interactive upgrade; the v2 path above is where that would land if it is ever wanted. -### D5 — The mechanism *(Phase 1, [#3248](https://github.com/open-gsd/gsd-core/issues/3248) — not shipped by this ADR)* +### D5 — The mechanism *(Phase 1, implemented in [#3248](https://github.com/open-gsd/gsd-core/issues/3248), open and unmerged at time of writing — not shipped by this ADR itself)* `discloseExecutableSurfaces` gains an `instructionSurfaces` collector, enumerating each skill stem the manifest contributes, collected through the same `safeCollect` wrapper as the existing four classes so a hostile value degrades only that class and the function stays total for any manifest shape. The pre-install consent summary names those skills as an instruction surface. The signature behavior implements D4 exactly, pinned by a test that asserts what happens to a pre-existing consent record rather than leaving it incidental. The design is deliberately **additive** — a new independent collector and a new field, with no change to the four existing collectors and none to `hasExecutable` — because `get_impact` rates `discloseExecutableSurfaces` **CRITICAL** at 196 affected symbols. +**Implementation note (Phase 1 scope).** The mechanism above discloses `skills` only. D3's class table names `skills, agents` as instruction surfaces, and that classification stands unchanged. `agents[]` are not disclosed by this mechanism, because third-party `agents[]` are never actually staged into the agent's instruction context: `stageSkillsForRuntimeAsSkills` (`src/install-profiles.cts`) unions third-party skills in via `readInstalledCapabilitySkill`, whereas `stageAgentsForRuntimeWithConverter` (same file) takes only a source directory and has no registry-aware third-party path. Disclosing agents here would have named a surface that does not exist. Whether third-party `agents[]` should be staged at all — and, if so, whether D3's agents half becomes reachable — is an open question for the maintainer, not a silent omission. Agents remain classified as an instruction surface either way; they are simply not yet a staged one. + ## Consequences -**What improves.** The boundary is written down on both sides: a capability author reading [`docs/how-to/develop-a-capability.md`](../how-to/develop-a-capability.md) learns their skill body ships verbatim and unscanned, and a user reading [`capability-trust-model.md`](../explanation/capability-trust-model.md) learns what installing a skill-bearing capability grants. After Phase 1, a skill-only capability — which today discloses nothing at all — names its instruction surface at the consent moment. +**What improves.** The boundary is written down on both sides: a capability author reading [`docs/how-to/develop-a-capability.md`](../how-to/develop-a-capability.md) learns their skill body ships verbatim and unscanned, and a user reading [`capability-trust-model.md`](../explanation/capability-trust-model.md) learns what installing a skill-bearing capability grants. A skill-only capability — which used to disclose nothing at all — now names its instruction surface at the consent moment. **What does not change.** No behavior changes in this ADR's phase. No stored consent record is perturbed and no spurious re-consent fires, now or under Phase 1 (D4). Skills remain the intended, low-friction contribution path; this is disclosure, not discouragement. @@ -109,7 +111,7 @@ The design is deliberately **additive** — a new independent collector and a ne - **First-party skills are equally unscanned.** Their assurance is provenance — they are the shipped package, and the GSD Core release process is their control — not content inspection. No content control exists on the first-party side either, and no reader should infer one. - **Global-scope skill-body change on upgrade is not consent-gated.** That is true of a global install's code too, and D4 does not change it either way. See D4. -**Ratification bar.** This ADR flips to `Accepted` when #3248 has merged, the consent summary renders instruction surfaces, and the D4 signature behavior is pinned by a passing test. Until then it is `Proposed` for a recorded reason, not through neglect. +**Ratification bar.** This ADR flips to `Accepted` when #3248 has merged, the consent summary renders instruction surfaces, and the D4 signature behavior is pinned by a passing test. #3248, while open and unmerged, already delivers the second and third of these: the pre-install consent summary renders skill instruction surfaces by name, and a passing test pins the D4 signature behavior — instruction surfaces do not perturb `disclosureSignature`. The first condition, #3248's merge, has not yet occurred. ## Alternatives considered diff --git a/docs/explanation/capability-trust-model.md b/docs/explanation/capability-trust-model.md index f57a57ef3..479eb5e1c 100644 --- a/docs/explanation/capability-trust-model.md +++ b/docs/explanation/capability-trust-model.md @@ -236,12 +236,21 @@ So there are three classes, not two: | **Instruction surface** | skills, agents | Instructions that will reach the agent. Reach bounded only by what the agent will do when told. | | **Inert artifact** | everything else in the bundle | Note only. | -**Not yet itemized at the prompt.** Naming a capability's individual skills in the -pre-install consent summary lands with -[#3248](https://github.com/open-gsd/gsd-core/issues/3248); today a skill-bearing -capability is covered by the bundle's integrity and by your consent to install it, -but its skills are not listed for you one by one. The classification above is -what GSD has decided; the itemized prompt is what it has not yet built. +Both `skills` and `agents` are classified as instruction surfaces, but only +`skills` are disclosed today. A third-party capability's declared `agents[]` +are never staged into the agent's instruction context — the staging path that +unions third-party skills into a runtime's skills directory has no equivalent +for agents — so naming them at the consent prompt would claim a surface that +does not exist. This is not a claim that agents are safe or inert: they are +still classified as an instruction surface, they are simply not staged for +third-party capabilities today, which is why they are not itemized below. + +**Itemized at the prompt.** [#3248](https://github.com/open-gsd/gsd-core/issues/3248) +made the pre-install consent summary name each contributed skill in its own +section. A capability whose only contribution is skills — which used to +disclose nothing at all beyond the bundle's integrity — is included: you see its +skills listed before you consent to install it. The listing names the surface; +it does not assert anything about what the surface contains. Installing a capability that ships skills grants it **instruction reach**. That is a real grant, and it is the same bargain this document already describes for code: @@ -255,11 +264,11 @@ Two things worth stating so you do not infer them: - **First-party skills are equally unscanned.** Their assurance is provenance — they are the shipped package — not content inspection. There is no content control on either side. -- **Correcting this document did not disturb any consent you have already - given.** No re-consent prompt follows from it. ADR-2363 D4 keeps instruction - surfaces out of the v1 disclosure signature precisely so that recording the - boundary honestly does not fire a spurious re-consent prompt on every - skill-bearing capability you have installed. +- **Naming the instruction surface did not disturb any consent you have + already given.** No re-consent prompt follows from it. ADR-2363 D4 keeps + instruction surfaces out of the v1 disclosure signature precisely so that + disclosing the boundary honestly does not fire a spurious re-consent prompt + on every skill-bearing capability you have installed. ### Integrity pinning diff --git a/docs/how-to/develop-a-capability.md b/docs/how-to/develop-a-capability.md index 927704dfb..20ab4a3e7 100644 --- a/docs/how-to/develop-a-capability.md +++ b/docs/how-to/develop-a-capability.md @@ -128,7 +128,7 @@ That makes a skill body an **instruction surface**, and it carries author respon - **Write instructions you would be comfortable defending.** Your reach is bounded only by what the agent will do when told. Consent, integrity pinning and reversibility are the user's protections; content review is not among them. - **Do not embed anything that tries to redirect the agent away from the user's task** — reframing its role, overriding host instructions, or persisting directives past the skill's own scope. That is indistinguishable from a prompt-injection payload, and the fact that it is not scanned is not permission. - **Treat anything your skill tells the agent to read as data, not instructions.** If your skill has the agent ingest a file, a URL, or tool output, say so explicitly in the body. See [the untrusted-input boundary](../adr/1577-untrusted-input-boundary-and-injection-blocking.md). -- **Expect the surface to be disclosed.** Naming the skills your Capability contributes in the pre-install consent summary lands with [#3248](https://github.com/open-gsd/gsd-core/issues/3248) — it does not happen today. Write your skill body on the assumption that a user will see it listed before they accept. +- **The surface is disclosed.** The skills your Capability contributes are named, by stem, in their own section of the pre-install consent summary ([#3248](https://github.com/open-gsd/gsd-core/issues/3248)). Write your skill body on the assumption that a user sees it listed before they accept. Declared `agents[]` are not named there today — a third-party capability's agents are never staged into the agent's instruction context, so there is nothing to disclose — but they remain classified as an instruction surface, not an inert or safe one. The reasoning behind this posture — including why scanning was considered and rejected — is recorded in [ADR-2363](../adr/2363-capability-instruction-surface-trust.md), and the user-facing side is [the capability trust model](../explanation/capability-trust-model.md). diff --git a/docs/reference/capability-manifest.md b/docs/reference/capability-manifest.md index 60b186f61..c22012256 100644 --- a/docs/reference/capability-manifest.md +++ b/docs/reference/capability-manifest.md @@ -45,6 +45,10 @@ Feature capabilities declare owned artefacts, lifecycle hooks, a federated confi | `skills` | string[] | Owned skill stems. Exactly one capability may own each stem across the entire merged registry (first-party ∪ overlay). | | `agents` | string[] | Owned agent stems. Same uniqueness constraint as skills. | +The `skills` stems declared here are disclosed by name as **instruction surfaces** in the pre-install consent summary ([ADR-2363](../adr/2363-capability-instruction-surface-trust.md)). Bodies are installed verbatim and are not content-scanned — see [the capability trust model](../explanation/capability-trust-model.md). + +`agents` are classified as an instruction surface too (ADR-2363 D3), but the stems declared here are **not** disclosed at the prompt: a third-party capability's `agents[]` are never staged into the agent's instruction context — the staging path that unions third-party skills into a runtime's skills directory has no equivalent for agents — so naming them would claim a surface that does not exist. This does not make agents safe or inert; it means the mechanism does not yet reach them. + ### `hooks` Non-loop lifecycle hooks. diff --git a/docs/reference/gsd-capability-command.md b/docs/reference/gsd-capability-command.md index 3abaecaeb..7b312fbf6 100644 --- a/docs/reference/gsd-capability-command.md +++ b/docs/reference/gsd-capability-command.md @@ -37,7 +37,7 @@ gsd capability install [--integrity sha512-] [--scope global|projec **Behaviour** -Resolves `` to a versioned, staged capability bundle. The pipeline is: fetch → verify integrity or SHA pin → check `engines.gsd` against the installed GSD version → disclose executable surfaces (hooks, command modules, MCP servers) → obtain consent (a declarative capability needs none; an executable one requires `--yes`) → validate the incoming manifest against the trust invariants → extract to the scope root → write the ledger entry atomically. +Resolves `` to a versioned, staged capability bundle. The pipeline is: fetch → verify integrity or SHA pin → check `engines.gsd` against the installed GSD version → disclose executable surfaces (hooks, command modules, MCP servers) and instruction surfaces (skills) → obtain consent (a declarative capability needs none; an executable one requires `--yes` — disclosed instruction surfaces do not by themselves trigger this requirement) → validate the incoming manifest against the trust invariants → extract to the scope root → write the ledger entry atomically. An overlay whose `id` uses a reserved first-party prefix (`gsd-`, `gsd-core-`, `anthropic-`) is rejected before extraction. Install never executes capability code; staging is copy-only. A declined install (executable surface, no `--yes`) writes **nothing** — no bundle, no ledger entry, no shared-file edits. @@ -304,6 +304,18 @@ An empty (or missing) ledger reports nothing: `--json` emits `[]`; the table not --- +### Instruction surfaces (skills) + +A capability's declared `skills` stems are **instruction surfaces**: each stem's body — its `SKILL.md` — is copied verbatim into the runtime's instruction context, where it reaches the agent directly. `install` and `update` list every such stem by name in the disclosure printed before the pipeline proceeds, alongside the executable-surface disclosure. + +ADR-2363 D3 classifies `agents` as an instruction surface too, but a third-party capability's declared `agents[]` are not disclosed here: they are never staged into the agent's instruction context — the staging path that unions third-party skills into a runtime's skills directory has no equivalent for agents — so naming them would name a surface that does not exist. This is not a claim that agents are safe or inert, only that they are not currently staged for third-party capabilities. + +An instruction surface is disclosed but is **not** an executable surface: naming it does not set `hasExecutable` and it does not enter the `disclosureSignature`, so adding, removing, or changing skills between versions does not by itself require `--yes` or force re-consent on `update`. See [ADR-2363](../adr/2363-capability-instruction-surface-trust.md) D3 (the instruction-surface classification) and D4 (why it is excluded from the disclosure signature). + +Bodies are not content-scanned, at install, update, or any later point — disclosure names the surface; it never inspects what it contains. + +--- + ### `trust` Manage the **user-owned consent store** (#1459) that gates project-scope third-party capability activation. The store lives at `${GSD_HOME||homedir()}/.gsd/consent.json` — **outside any repository** — and records, per `(realpath(projectRoot), capability id)`, the bundle integrity and disclosure signature you consented to **on this machine**. A project-scope overlay is inactive until such a record exists (so a forged or cloned in-repo project ledger activates nothing on its own); installing a project-scope capability through the lifecycle writes the record, and removing it revokes the record. diff --git a/src/capability-trust.cts b/src/capability-trust.cts index 43756127e..3765c8bc3 100644 --- a/src/capability-trust.cts +++ b/src/capability-trust.cts @@ -1,6 +1,7 @@ /** * Capability trust gate — ADR-1244 Phase 4 (Decision D5 + the compatibility half of D6), extended - * by ADR-2782 Phase 3 (#2796) with a FOURTH executable-surface class: the reviewer lane. + * by ADR-2782 Phase 3 (#2796) with a FOURTH executable-surface class (the reviewer lane), and by + * ADR-2363 Phase 1 (#3248) with a FIFTH, NON-executable class: the instruction surface. * * PURE module. It computes *what* a capability would do and *whether* policy allows it; it * never mutates the filesystem and never performs I/O beyond reading staged files to confirm @@ -20,16 +21,33 @@ * the signature — the loader has no config resolver and must compute the same signature as the * lifecycle (constraint 2, `.gsd/phase/chore-2796-reviewer-trust-disclosure/40-design.md`). * + * ADR-2363 D5 (#3248): a capability's declared `skills` are INSTRUCTION surfaces — their bodies are + * copied verbatim into the user's agent instruction context, so their reach is bounded only by what + * the agent will do when told. They are disclosed BY NAME and never content-scanned (D2 — + * Kerckhoffs: a shipped rule set is readable by the adversary who installs it). Unlike the four + * executable classes they never set `hasExecutable` (D3) and never enter `disclosureSignature` (D4): + * folding them in would perturb the stored signature of every already-consented skill-bearing + * capability and fire a spurious re-consent on its next upgrade — the harm ADR-2782 D4 rule 5 + * already forbids. Any future signature binding arrives as a versioned v2, never an in-place + * re-encoding of v1. ADR-2363 D3's class table names "skills, agents", but third-party `agents[]` + * are deliberately EXCLUDED here: `stageAgentsForRuntimeWithConverter` (`src/install-profiles.cts`) + * takes only a source directory, with no registry-aware third-party staging path the way + * `readInstalledCapabilitySkill` gives skills — so a declared agent is never actually staged into + * the instruction context, and disclosing it would name a surface that does not exist. Agents stay + * unimplemented pending a maintainer decision. + * * Exports: * RESERVED_NAMESPACES — id prefixes third parties may not claim - * discloseExecutableSurfaces(...) — enumerate hooks / command modules / mcpServers / reviewer lanes + * discloseExecutableSurfaces(...) — enumerate the four executable classes + instruction surfaces * collectReviewerLaneSurfaces(...) — the reviewer-lane collector, independently testable + * collectInstructionSurfaces(...) — the instruction-surface collector, independently testable * checkReservedNamespace(id) — is this id in a reserved namespace? * evaluateSourceAllowed(parsed,...) — strictKnownRegistries enforcement * checkEngines(manifest, host) — engines.gsd hard gate + compatVersions downgrade * evaluateInstallTrust(args) — compose: source + namespace + engines + disclosure * executableSetChanged(old, new) — did the executable surface set change between versions? * summarizeDisclosure(disclosure) — human-readable consent-prompt lines + * summarizeInstructionSurfaces(d) — the instruction-surface section of the consent summary * UNRESOLVED_HOST_MARKER — the non-blank marker for an unresolved openai-http host * EGRESS_PAYLOAD_CLASSES — the named data classes every reviewer lane receives */ @@ -222,6 +240,25 @@ interface ReviewerLaneSurface { egressPayloadClasses: string[]; } +/** + * ADR-2363 D3 (#3248): an INSTRUCTION surface — an artifact whose body is copied verbatim into the + * user's agent instruction context. Peer to the four executable-surface classes, and deliberately + * NOT one of them: a skill body does not execute code, it instructs the thing that does. + * + * Disclosure NAMES the surface; it never inspects the body. Content scanning is rejected outright + * by ADR-2363 D2 (Kerckhoffs — a shipped rule set is readable by the adversary who installs it). + */ +interface InstructionSurface { + /** + * Which declaration array the name came from — still a discriminator even with one member: a + * future addition (see `INSTRUCTION_SURFACE_FIELDS`) is why this stays a field rather than being + * dropped now. + */ + kind: 'skill'; + /** The declared stem/name, VERBATIM — never normalized, truncated, or deduped. */ + name: string; +} + interface Disclosure { /** Hook scripts the capability registers (each runs as a runtime hook command). */ hooks: HookSurface[]; @@ -235,6 +272,14 @@ interface Disclosure { * above; a standing egress channel to an external reviewer. */ reviewerLanes: ReviewerLaneSurface[]; + /** + * ADR-2363 D5 (#3248): the skills this capability contributes to the agent's instruction context + * (agents are excluded — see the module header). A FIFTH disclosed class that is deliberately NOT + * executable: it never contributes to `hasExecutable` (D3) and never enters `disclosureSignature` + * (D4 — folding it in would perturb the stored signature of every already-consented skill-bearing + * capability and fire a spurious re-consent on its next upgrade, which ADR-2782 D4 rule 5 forbids). + */ + instructionSurfaces: InstructionSurface[]; /** True when the capability ships ANY executable surface (=> consent required). */ hasExecutable: boolean; /** @@ -688,6 +733,54 @@ function collectReviewerLaneSurfaces( }, []); } +/** + * The manifest fields whose declared names become instruction surfaces. Ordered data rather than a + * hand-rolled loop per field, so a future second member of the class is one row, not a second copy + * of the same filter. Deliberately a ONE-row table today: third-party `agents[]` are never staged + * into the instruction context — `stageAgentsForRuntimeWithConverter` (`src/install-profiles.cts`) + * takes only a source directory, with no registry-aware staging path the way + * `readInstalledCapabilitySkill` gives skills — so disclosing them would name a surface that does + * not exist. ADR-2363 D3's class table says "skills, agents"; the agents half is therefore + * deliberately unimplemented pending a maintainer decision. + */ +const INSTRUCTION_SURFACE_FIELDS: ReadonlyArray<{ field: string; kind: InstructionSurface['kind'] }> = [ + { field: 'skills', kind: 'skill' }, +]; + +/** + * Collect the instruction surfaces a manifest declares (ADR-2363 D5, #3248) — peer to the four + * executable-surface collectors, and invoked through the same `safeCollect` wrapper so a hostile + * value here degrades ONLY this class to empty rather than losing the other four. + * + * Liberal in what it accepts, exactly like the existing collectors: a non-object manifest, an + * absent field, a non-array field, and a non-string/blank member each degrade quietly. A non-array + * `skills` is NOT a partial success — it yields nothing, because a scalar declares no set. + * + * Deliberately does NOT: + * - dedup (a manifest declaring a stem twice discloses it twice — disclosure reports what the + * manifest SAYS; collapsing would misreport it, and dedup is the registry's job); + * - normalize or truncate a name (it is disclosed verbatim so the user sees what was declared); + * - existence-check the stem against `stagedDir`. A stem is a REGISTRY name, not a bundle-relative + * artifact path — checking it would repeat the reviewer-lane `binary` mistake (matrix C6) and + * put registry names into `missingArtifacts`, which is for declared bundle FILES only. + */ +function collectInstructionSurfaces(manifest: CapabilityManifest): InstructionSurface[] { + const surfaces: InstructionSurface[] = []; + if (typeof manifest !== 'object' || manifest === null) return surfaces; + for (const { field, kind } of INSTRUCTION_SURFACE_FIELDS) { + const declared = (manifest as Record)[field]; + if (!Array.isArray(declared)) continue; + for (const entry of declared) { + // A non-string or blank member is dropped INDIVIDUALLY — the valid siblings around it still + // disclose, mirroring how collectHookSurfaces skips a malformed entry rather than the array. + if (typeof entry !== 'string') continue; + if (entry.trim() === '') continue; + surfaces.push({ kind, name: entry }); + } + } + return surfaces; +} + /** * Enumerate every executable surface a capability manifest declares. * @@ -697,6 +790,13 @@ function collectReviewerLaneSurfaces( * - `mcpServers`: { : {...} } | [{ name }] — servers spawned by the host runtime * - `reviewer`: { slug, transport, invoke, ... } — an external reviewer lane (ADR-2782 D5, #2796) * + * plus ONE non-executable class (ADR-2363 D5, #3248): + * - `skills`: string[] of owned stems — INSTRUCTION surfaces, whose bodies land in the agent's + * instruction context. Disclosed by name; they never set `hasExecutable` (D3) and never enter + * `disclosureSignature` (D4). Stems are registry names, so they are never existence-checked + * against `stagedDir` and never appear in `missingArtifacts`. `agents` is excluded — see the + * module header. + * * `mcpServers` is not a first-party capability.json field today, but a third-party manifest may * declare it, so the trust gate discloses it whenever present (honest disclosure over the * narrower first-party schema). Pure: when `stagedDir` is provided, declared hook/command-module @@ -709,7 +809,7 @@ function collectReviewerLaneSurfaces( * throwing traps, or a property with a throwing getter (matrix C5, E2). Disclosure runs BEFORE * Phase 2's validation, on a manifest validation would reject outright, so it must tolerate what * validation does not. Each surface class is collected independently (`safeCollect`) so a hostile - * value in ONE class degrades only that class to empty rather than losing the other three. + * value in ONE class degrades only that class to empty rather than losing the others. * * `resolveHost` (optional, #2796) is forwarded to `collectReviewerLaneSurfaces` so a caller with * config access (the lifecycle, never the loader — see `signatureForManifest`) can disclose the REAL @@ -731,10 +831,16 @@ function discloseExecutableSurfaces( () => collectReviewerLaneSurfaces(manifest, resolveHost), [] as ReviewerLaneSurface[], ); + const instructionSurfaces = safeCollect( + () => collectInstructionSurfaces(manifest), + [] as InstructionSurface[], + ); + // ADR-2363 D3: instruction surfaces are deliberately ABSENT from this expression. Adding them + // would silently change `executableSetChanged` and the auto-update re-consent trigger. const hasExecutable = hooks.length > 0 || commandModules.length > 0 || mcpServers.length > 0 || reviewerLanes.length > 0; - return { hooks, commandModules, mcpServers, reviewerLanes, hasExecutable, missingArtifacts }; + return { hooks, commandModules, mcpServers, reviewerLanes, instructionSurfaces, hasExecutable, missingArtifacts }; } /** @@ -1126,21 +1232,127 @@ function truncateEnvValue(v: string): string { return v.length > ENV_VALUE_MAX ? `${v.slice(0, ENV_VALUE_MAX)}… (${v.length} chars)` : v; } +/** + * Characters that must never reach the consent prompt unescaped. `summarizeDisclosure`'s lines are + * joined with `\n` and written RAW to stderr on the needs-consent path, and every value in them is + * attacker-controlled manifest data. A raw newline forges a line indistinguishable from genuine + * disclosure text; a raw ESC lets a value rewrite or clear lines already printed; a bidi override + * visually reorders one. C0, DEL, C1, the bidi/isolate controls, and the line/paragraph separators + * are all escaped to a visible `\uXXXX`, so the value stays identifiable and cannot forge output. + */ +const UNSAFE_PROMPT_CHARS = /[\u0000-\u001f\u007f-\u009f\u200e\u200f\u2028\u2029\u202a-\u202e\u2066-\u2069]/g; + +/** Max characters of any single manifest-supplied value rendered into the consent prompt. */ +const PROMPT_VALUE_MAX = 200; + +/** + * Render one manifest-supplied value safely into a consent-prompt line: escape every character that + * could forge or rewrite output, then bound the length so one oversized value cannot flood the + * prompt and push the rest off screen. Escaping is IDENTITY for ordinary names, so this changes no + * existing rendered output for any well-formed manifest — only the disclosure OBJECT is verbatim; + * the rendered LINE is always escaped. + */ +function renderValueForPrompt(v: unknown): string { + // `String(v)` on an arbitrary `unknown` risks Object's default `[object Object]` stringification + // (@typescript-eslint/no-base-to-string) for a non-primitive; every call site here passes a string + // in practice, but the parameter stays `unknown` for the same total-collector discipline as + // `renderArgForHuman`, so a non-primitive is JSON-stringified instead of coerced. + let s: string; + if (typeof v === 'string') { + s = v; + } else if (v === null || v === undefined) { + s = ''; + } else if (typeof v === 'number' || typeof v === 'boolean' || typeof v === 'bigint') { + s = String(v); + } else { + try { + s = JSON.stringify(v) ?? ''; + } catch { + s = ''; + } + } + const escaped = s.replace(UNSAFE_PROMPT_CHARS, (c) => `\\u${c.charCodeAt(0).toString(16).padStart(4, '0')}`); + return escaped.length > PROMPT_VALUE_MAX + ? `${escaped.slice(0, PROMPT_VALUE_MAX)}… (${escaped.length} chars)` + : escaped; +} + +/** + * Render the instruction-surface section of a consent summary (ADR-2363 D3/D5, #3248). + * + * Extracted as its own exported function for two reasons. It is called from BOTH branches of + * `summarizeDisclosure` — a skill-only capability has `hasExecutable === false` and takes the early + * return, so a section appended only at the end would never render for exactly the capabilities + * that need it. And it gives tests a typed surface to assert on, instead of regex-matching prose out + * of `summarizeDisclosure` (CONTRIBUTING — "Prohibited: Raw Text Matching on Test Outputs"). + * + * Returns `[]` when nothing is declared, so either caller can append unconditionally without + * emitting an empty header. + * + * TOTAL for a partial disclosure object: the CLI edge calls `summarizeDisclosure(res.disclosure || {})` + * (`capability-command-router.cjs`), so a bare `{}` — carrying no `instructionSurfaces` at all — + * reaches this function whenever a lifecycle result has no disclosure. + * + * #3248: every manifest-supplied value rendered here (`kind`, `name`) goes through + * `renderValueForPrompt` first — the CLI edge writes these lines RAW to stderr on the needs-consent + * path, and an unescaped name could forge a line or rewrite output already printed (see that + * function's comment). + */ +function summarizeInstructionSurfaces(disclosure: Disclosure): string[] { + const declared = (disclosure as Partial | null | undefined)?.instructionSurfaces; + const surfaces = Array.isArray(declared) ? declared : []; + if (surfaces.length === 0) return []; + const lines: string[] = [ + ` instruction surfaces (${surfaces.length}): installed into your agent's instruction context`, + ]; + for (const s of surfaces) { + // #3248: kind/name are manifest-supplied — escape+bound before rendering (see `renderValueForPrompt`). + const kind = s?.kind ? renderValueForPrompt(s.kind) : '(kind?)'; + const name = s?.name ? renderValueForPrompt(s.name) : '(name?)'; + lines.push(` - ${kind}: ${name}`); + } + // ADR-2363 D1/D2, and Kerckhoffs: say plainly that nothing inspected these bodies. A summary that + // named the surface while implying review would be worse than silence — a "looks checked" line + // displaces the judgement this prompt exists to provoke (Goodhart, D2). + lines.push(' these bodies are installed verbatim and are NOT content-scanned'); + return lines; +} + /** * Render a disclosure as consent-prompt lines. Returned as an array so the CLI/runtime edge can * format it; the lib never writes to stdout. + * + * #3248: every manifest-supplied value interpolated into a line (hook event/script, command + * family/module/router, MCP name/transport/url/command/argv/header-keys/env-keys+values/cwd, + * reviewer-lane slug/hostConfigKey/resolvedHost/binary/rawArgs/handler, missingArtifacts entries) + * goes through `renderValueForPrompt` first, which escapes forging/rewriting control characters and + * bounds the length. These lines are joined with `\n` and written RAW to stderr on the + * needs-consent path (`capability-command-router.cjs`), so an unescaped value could forge a line or + * rewrite/clear output already printed — defeating the informed-consent guarantee this function + * exists to provide. GSD-authored literals (fallback placeholders, headings, ``) are never + * escaped — only manifest-supplied data is. */ function summarizeDisclosure(disclosure: Disclosure): string[] { const lines: string[] = []; + const instructionLines = summarizeInstructionSurfaces(disclosure); if (!disclosure.hasExecutable) { - lines.push('This capability ships no executable surfaces (declarative only).'); + // ADR-2363 D3: "declarative only" is true ONLY when there is no instruction surface either. + // Claiming it unconditionally told a user their capability contributes nothing to weigh while + // it was contributing agent instructions — the exact category error ADR-2363 was written to end. + if (instructionLines.length === 0) { + lines.push('This capability ships no executable surfaces (declarative only).'); + return lines; + } + lines.push('This capability ships no executable surfaces, but contributes agent instructions:'); + for (const line of instructionLines) lines.push(line); return lines; } lines.push('This capability ships executable surfaces that will run in your agent runtime:'); if (disclosure.hooks.length > 0) { lines.push(` hooks (${disclosure.hooks.length}): run as runtime hook commands`); for (const h of disclosure.hooks) { - lines.push(` - ${h.event || '(event?)'} -> ${h.script}`); + const event = h.event ? renderValueForPrompt(h.event) : '(event?)'; + lines.push(` - ${event} -> ${renderValueForPrompt(h.script)}`); } } if (disclosure.commandModules.length > 0) { @@ -1149,8 +1361,9 @@ function summarizeDisclosure(disclosure: Disclosure): string[] { ); for (const m of disclosure.commandModules) { // TRUST2-3 (#1459): show the router (which exported fn runs) so the user consents to the exact entry point. - const routerSuffix = m.router ? ` [router: ${m.router}]` : ''; - lines.push(` - ${m.family || '(family?)'} -> ${m.module}${routerSuffix}`); + const routerSuffix = m.router ? ` [router: ${renderValueForPrompt(m.router)}]` : ''; + const family = m.family ? renderValueForPrompt(m.family) : '(family?)'; + lines.push(` - ${family} -> ${renderValueForPrompt(m.module)}${routerSuffix}`); } } if (disclosure.mcpServers.length > 0) { @@ -1159,26 +1372,36 @@ function summarizeDisclosure(disclosure: Disclosure): string[] { // TRUST2-2 (#1459): a non-stdio (http/sse) server connects to a URL; disclose the endpoint, not // a (nonexistent) command. A stdio server discloses command + args as before. const isRemote = (s.transport === 'http' || s.transport === 'sse') || (!s.command && !!s.url); + const name = renderValueForPrompt(s.name); if (isRemote) { - const t = s.transport || 'http'; - lines.push(` - ${s.name} -> [${t}] ${s.url || '(no url declared)'}`); + const t = s.transport ? renderValueForPrompt(s.transport) : 'http'; + const url = s.url ? renderValueForPrompt(s.url) : '(no url declared)'; + lines.push(` - ${name} -> [${t}] ${url}`); // Header VALUES are redacted in the human summary (they may carry secrets); only the KEY set // is shown. The full values ARE in the signature, so a value change forces re-consent. const hdrKeys = s.headers ? Object.keys(s.headers) : []; if (hdrKeys.length > 0) { - lines.push(` headers: ${hdrKeys.map((k) => `${k}=`).join(', ')}`); + lines.push(` headers: ${hdrKeys.map((k) => `${renderValueForPrompt(k)}=`).join(', ')}`); } } else { - const cmd = [s.command, ...s.argv].filter(Boolean).join(' '); - lines.push(` - ${s.name} -> ${cmd || '(no command declared)'}`); + // Command + args are each escaped individually (not the joined string) so a value that + // embeds a newline cannot forge a line even when it lands mid-argv. + const cmd = [s.command, ...s.argv].filter(Boolean).map(renderValueForPrompt).join(' '); + lines.push(` - ${name} -> ${cmd || '(no command declared)'}`); } // TRUST-2 (#1459): env can change WHAT runs without touching the command, so show each env key - // and its (truncated) value — the user is consenting to this exact environment. + // and its (truncated) value — the user is consenting to this exact environment. Truncate first + // (keeps the prompt readable at the existing 60-char bound), then escape the result (#3248) so + // the truncated value can still not forge or rewrite output. const envKeys = s.env ? Object.keys(s.env) : []; if (envKeys.length > 0) { - lines.push(` env: ${envKeys.map((k) => `${k}=${truncateEnvValue(s.env[k])}`).join(', ')}`); + lines.push( + ` env: ${envKeys + .map((k) => `${renderValueForPrompt(k)}=${renderValueForPrompt(truncateEnvValue(s.env[k]))}`) + .join(', ')}`, + ); } - if (s.cwd) lines.push(` cwd: ${s.cwd}`); + if (s.cwd) lines.push(` cwd: ${renderValueForPrompt(s.cwd)}`); } } if (disclosure.reviewerLanes.length > 0) { @@ -1193,26 +1416,33 @@ function summarizeDisclosure(disclosure: Disclosure): string[] { // declared)" for a lane that in fact egresses to a live remote host — // understating the disclosure precisely when it matters. Disclosure runs // BEFORE validation, so a non-canonical transport does reach this code. + const slug = l.slug ? renderValueForPrompt(l.slug) : '(slug?)'; if (l.transport === 'openai-http' || (!l.binary && l.hostConfigKey)) { const localTag = l.isLocalDestination ? ' [local]' : ''; - lines.push(` - ${l.slug || '(slug?)'} -> [openai-http] ${l.hostConfigKey || '(hostConfigKey?)'} => ${l.resolvedHost}${localTag}`); + const hostConfigKey = l.hostConfigKey ? renderValueForPrompt(l.hostConfigKey) : '(hostConfigKey?)'; + lines.push(` - ${slug} -> [openai-http] ${hostConfigKey} => ${renderValueForPrompt(l.resolvedHost)}${localTag}`); } else { // Render the RAW declared args, not the string-filtered view. The raw // array is what the host receives and what the consent signature binds, // so a non-string member that is invisible here is a surface the user // consented to without being shown — the opposite of the disclosure's - // whole purpose. - const cmd = [l.binary, ...l.rawArgs.map(renderArgForHuman)].filter(Boolean).join(' '); - lines.push(` - ${l.slug || '(slug?)'} -> ${cmd || '(no binary declared)'}`); + // whole purpose. `renderArgForHuman` stringifies a non-string member; that + // string is equally attacker-controlled, so it is escaped too (#3248). + const cmd = [l.binary, ...l.rawArgs.map(renderArgForHuman)] + .filter(Boolean) + .map(renderValueForPrompt) + .join(' '); + lines.push(` - ${slug} -> ${cmd || '(no binary declared)'}`); } - if (l.handler) lines.push(` handler: ${l.handler}`); + if (l.handler) lines.push(` handler: ${renderValueForPrompt(l.handler)}`); lines.push(` sends: ${l.egressPayloadClasses.join(', ')}`); } } + for (const line of instructionLines) lines.push(line); if (disclosure.missingArtifacts.length > 0) { lines.push(' WARNING — declared artifacts not found in the staged bundle:'); for (const a of disclosure.missingArtifacts) { - lines.push(` - ${a}`); + lines.push(` - ${renderValueForPrompt(a)}`); } } return lines; @@ -1228,12 +1458,21 @@ export = { // #2796: the reviewer-lane collector, exported for independent testability (ADR-2782's own // argument for extracting per-class collectors rather than growing the switch inline). collectReviewerLaneSurfaces, + // ADR-2363 D5 (#3248): the instruction-surface collector, exported for independent testability — + // same rationale as `collectReviewerLaneSurfaces` above. + collectInstructionSurfaces, checkReservedNamespace, evaluateSourceAllowed, checkEngines, evaluateInstallTrust, executableSetChanged, summarizeDisclosure, + // ADR-2363 D3/D5 (#3248): the instruction-surface section of the consent summary, exported so + // callers/tests can assert on it directly — see `summarizeInstructionSurfaces`'s own JSDoc. + summarizeInstructionSurfaces, + // #3248: the consent-prompt escaping/bounding helper, exported so tests can assert directly that + // control characters (newline, ESC, bidi overrides, etc.) never reach a rendered prompt line. + renderValueForPrompt, // #1459: the consent-binding signature (single source of truth for loader + lifecycle consent). disclosureSignature, signatureForManifest, diff --git a/tests/instruction-surface-disclosure.security.test.cjs b/tests/instruction-surface-disclosure.security.test.cjs new file mode 100644 index 000000000..d58c20f2e --- /dev/null +++ b/tests/instruction-surface-disclosure.security.test.cjs @@ -0,0 +1,901 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +/** + * instruction-surface-disclosure.security.test.cjs — behavioral tests for a FIFTH disclosed + * class inside the capability trust gate (ADR-2363 D5, #3248): `instructionSurfaces` — the + * skill stems a capability manifest declares — added to `discloseExecutableSurfaces`. + * + * Instruction surfaces are SKILLS ONLY (`InstructionSurface.kind` is the literal `'skill'`). + * ADR-2363 D3's class table names "skills, agents", but a declared `agents[]` is deliberately + * NOT collected: third-party `agents[]` are never staged into the agent's instruction context + * (there is no registry-aware agent staging path, unlike `readInstalledCapabilitySkill` for + * skills), so disclosing them would be a false claim in a consent prompt. Tests below that used + * to assert agent disclosure now assert the NEGATIVE — that a declared `agents` array yields no + * instruction surface at all. + * + * Implements every row carrying a Test name in + * `.gsd/phase/feat-3248-disclose-instruction-surfaces/50-test-matrix.md`, derived from + * `40-design.md`'s behavior table. Rows 18-20 and 23-25 are the load-bearing ones: they encode + * ADR-2363 D4 — instruction surfaces must never perturb `disclosureSignature`/`hasExecutable`, and + * no pre-existing consent record may be disturbed by a manifest gaining a `skills`/`agents` array. + * + * FAILING-FIRST: at the time this file was written, `Disclosure.instructionSurfaces` does not + * exist. `discloseExecutableSurfaces` is called directly (the cheapest unit that proves the + * behavior), matching `tests/reviewer-trust-disclosure.test.cjs`'s own established idiom. + * + * Suite: `security` (filename `.security.` infix) — this is a trust-gate surface. + */ + +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 trust = require('../gsd-core/bin/lib/capability-trust.cjs'); + +// ─── Fixture builders ────────────────────────────────────────────────────── +// House convention (tests/reviewer-manifest-body.test.cjs, tests/reviewer-trust-disclosure.test.cjs): +// builder functions return a VALID fixture; an optional `mutator` callback is applied to the FRESH +// object before it is returned. Every call builds a brand-new object — no shared mutable state. + +/** + * A minimal, valid capability manifest declaring both skills and agents. Kept declaring BOTH + * deliberately — it is now valuable precisely because it proves `agents` is ignored: every + * assertion against this fixture's `instructionSurfaces` must show the skills only, never the + * declared agent name. + */ +function skillsAndAgentsManifest(mutator) { + const manifest = { + id: 'test-cap', + role: 'feature', + title: 'Test Capability', + description: 'A test capability for the instruction-surface disclosure test suite.', + tier: 'standard', + requires: [], + version: '1.0.0', + skills: ['ui-phase', 'ui-review'], + agents: ['gsd-ui-checker'], + }; + if (mutator) mutator(manifest); + return manifest; +} + +/** A manifest carrying one hook, one command module, and one mcpServer — no skills/agents/reviewer. */ +function executableSurfaceManifest(mutator) { + const manifest = { + id: 'x', + hooks: [{ event: 'PostToolUse', script: 'hooks/x.js' }], + commands: [{ family: 'demo', module: 'demo.cjs', router: 'run' }], + mcpServers: { srv: { command: 'node', args: ['s.js'], env: { A: '1' } } }, + }; + if (mutator) mutator(manifest); + return manifest; +} + +// ─── A. Happy path (rows 1-3) ─────────────────────────────────────────────── + +describe('A. Happy path', () => { + test('discloses declared skill stems in order', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: ['a', 'b'] }); + assert.deepEqual(d.instructionSurfaces, [ + { kind: 'skill', name: 'a' }, + { kind: 'skill', name: 'b' }, + ]); + }); + + // A declared `agents` array is classified as an instruction surface by ADR-2363 D3's class + // table, but is deliberately NOT staged into the instruction context for third-party + // capabilities (no registry-aware agent staging path — see the module header on + // src/capability-trust.cts). Disclosing it would name a surface that does not exist, so it + // must yield NO instruction surfaces at all. + test('declared agent names yield no instruction surfaces', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x', agents: ['gsd-ui-checker'] }); + assert.deepEqual(d.instructionSurfaces, []); + }); + + test('a manifest declaring both skills and agents discloses only its skills', () => { + const d = trust.discloseExecutableSurfaces(skillsAndAgentsManifest()); + assert.deepEqual(d.instructionSurfaces, [ + { kind: 'skill', name: 'ui-phase' }, + { kind: 'skill', name: 'ui-review' }, + ]); + }); +}); + +// ─── B. Boundary (rows 4-7) ───────────────────────────────────────────────── + +describe('B. Boundary', () => { + test('absent skills yields an empty instruction surface list', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x' }); + assert.deepEqual(d.instructionSurfaces, []); + }); + + test('empty skills array yields empty list', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: [] }); + assert.deepEqual(d.instructionSurfaces, []); + }); + + test('single skill', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: ['only'] }); + assert.deepEqual(d.instructionSurfaces, [{ kind: 'skill', name: 'only' }]); + }); + + test('many skills are not truncated', () => { + const stems = Array.from({ length: 64 }, (_, i) => `skill-${i}`); + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: stems }); + assert.deepEqual( + d.instructionSurfaces, + stems.map((name) => ({ kind: 'skill', name })), + ); + }); +}); + +// ─── C. Negative / malformed (rows 8-11, 15) ──────────────────────────────── + +describe('C. Negative / malformed', () => { + test('non-array skills yields empty list', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: 'a' }); + assert.deepEqual(d.instructionSurfaces, []); + }); + + test('object skills yields empty list', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: { 0: 'a' } }); + assert.deepEqual(d.instructionSurfaces, []); + }); + + test('non-string and empty stems are dropped individually', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: ['ok', 42, null, {}, '', true] }); + assert.deepEqual(d.instructionSurfaces, [{ kind: 'skill', name: 'ok' }]); + }); + + test('whitespace-only stem is dropped', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: [' '] }); + assert.deepEqual(d.instructionSurfaces, []); + }); + + test('non-object manifest is total', () => { + for (const manifest of [null, 42, []]) { + assert.doesNotThrow(() => trust.discloseExecutableSurfaces(manifest)); + const d = trust.discloseExecutableSurfaces(manifest); + assert.deepEqual(d.instructionSurfaces, [], `manifest=${JSON.stringify(manifest)} must disclose no instruction surfaces`); + assert.deepEqual(d.hooks, []); + assert.deepEqual(d.commandModules, []); + assert.deepEqual(d.mcpServers, []); + assert.deepEqual(d.reviewerLanes, []); + assert.equal(d.hasExecutable, false); + } + }); +}); + +// ─── D. Hostile (rows 12-14, 16) ───────────────────────────────────────────── + +describe('D. Hostile', () => { + test('a throwing skills getter degrades only its own class', () => { + const manifest = executableSurfaceManifest(); + Object.defineProperty(manifest, 'skills', { + enumerable: true, + get() { + throw new Error('boom: throwing skills getter'); + }, + }); + assert.doesNotThrow(() => trust.discloseExecutableSurfaces(manifest)); + const d = trust.discloseExecutableSurfaces(manifest); + assert.deepEqual(d.instructionSurfaces, [], 'a throwing skills getter must degrade to no instruction surfaces'); + assert.deepEqual(d.hooks, [{ event: 'PostToolUse', script: 'hooks/x.js' }], 'hooks must still populate'); + assert.deepEqual( + d.commandModules, + [{ family: 'demo', module: 'demo.cjs', router: 'run' }], + 'command modules must still populate', + ); + assert.equal(d.mcpServers.length, 1, 'mcp servers must still populate'); + assert.deepEqual(d.reviewerLanes, [], 'lane-free manifest still discloses no lane (unaffected either way)'); + assert.equal(d.hasExecutable, true, 'the other three classes still set hasExecutable'); + }); + + test('a hostile Proxy manifest never throws', () => { + const proxyManifest = new Proxy( + {}, + { + get() { + throw new Error('boom: get trap'); + }, + has() { + throw new Error('boom: has trap'); + }, + ownKeys() { + throw new Error('boom: ownKeys trap'); + }, + }, + ); + assert.doesNotThrow(() => trust.discloseExecutableSurfaces(proxyManifest)); + const d = trust.discloseExecutableSurfaces(proxyManifest); + assert.equal(d.hasExecutable, false); + assert.deepEqual(d.instructionSurfaces, []); + assert.deepEqual(d.hooks, []); + assert.deepEqual(d.commandModules, []); + assert.deepEqual(d.mcpServers, []); + assert.deepEqual(d.reviewerLanes, []); + }); + + test('prototype-polluting stem names do not mutate Object.prototype', () => { + const beforeProps = Object.getOwnPropertyNames(Object.prototype).sort(); + const d = trust.discloseExecutableSurfaces({ + id: 'x', + skills: ['__proto__', 'constructor', 'prototype'], + }); + assert.deepEqual(d.instructionSurfaces, [ + { kind: 'skill', name: '__proto__' }, + { kind: 'skill', name: 'constructor' }, + { kind: 'skill', name: 'prototype' }, + ], 'the literal names are disclosed, not interpreted as prototype keys'); + const afterProps = Object.getOwnPropertyNames(Object.prototype).sort(); + assert.deepEqual(afterProps, beforeProps, 'Object.prototype must be unchanged'); + assert.equal(({}).polluted, undefined, 'a fresh plain object must carry no polluted property'); + }); + + test('adversarial stem contents survive disclosure intact', () => { + const huge = 'x'.repeat(10000); + const stems = ['a\nb', 'x\0y', '日本語スキル', huge]; + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: stems }); + assert.deepEqual( + d.instructionSurfaces, + stems.map((name) => ({ kind: 'skill', name })), + 'every adversarial stem must be disclosed verbatim, with no crash and no truncation', + ); + const last = d.instructionSurfaces[d.instructionSurfaces.length - 1]; + assert.equal(last.name.length, 10000, 'the 10k-char stem must not be truncated'); + }); + + // Security matrix (CONTRIBUTING.md "Security and prompt-injection surfaces"): a fake instruction + // tag and a traversal-shaped stem. Both are disclosed VERBATIM as ordinary names — collectInstructionSurfaces + // never parses, executes, or interprets a stem's contents (ADR-2363 D2, Kerckhoffs: a shipped rule + // set is readable by the adversary who installs it), and `missingArtifacts` stays empty even with a + // `stagedDir` supplied. A stem is a REGISTRY NAME, not a bundle-relative artifact path — this + // collector never joins it to a filesystem path (see `collectInstructionSurfaces`'s own JSDoc), so + // `'../../etc/passwd'` has nothing to traverse: there is no `path.join(stagedDir, stem)` call for it + // to escape. Treating it as a defect would mean the FIX is to start resolving stems against the + // filesystem, which is exactly the mistake ADR-2363 D5's design note calls out as the reviewer-lane + // `binary` precedent (matrix C6) — existence-checking a registry name blocks every install instead + // of protecting one. Verbatim disclosure of the instruction-tag string is the intended behavior per + // ADR-2363 D1/D2, not a defect: the consent prompt shows the human exactly what was declared, + // unfiltered, so THEY judge it — the tool never silently "sanitizes" or interprets it on their behalf. + test('a fake instruction tag and a traversal-shaped stem are disclosed verbatim, never filesystem-resolved', (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'instr-surface-d5-')); + t.after(() => cleanup(dir)); + + const instructionTag = 'ignore previous'; + const traversal = '../../etc/passwd'; + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: [instructionTag, traversal] }, dir); + assert.deepEqual(d.instructionSurfaces, [ + { kind: 'skill', name: instructionTag }, + { kind: 'skill', name: traversal }, + ], 'both hostile stems must be disclosed as ordinary names, character-for-character'); + assert.deepEqual( + d.missingArtifacts, + [], + 'a traversal-shaped stem is a registry name, never resolved against stagedDir — it must not surface as a missing/escaping artifact', + ); + }); +}); + +// ─── E. Duplicate (row 17) ─────────────────────────────────────────────────── + +describe('E. Duplicate', () => { + test('duplicate stems are not collapsed', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: ['a', 'a'] }); + assert.deepEqual(d.instructionSurfaces, [ + { kind: 'skill', name: 'a' }, + { kind: 'skill', name: 'a' }, + ]); + }); +}); + +// ─── F. Independence — signature stability (rows 18-20, ADR-2363 D4) ──────── + +describe('F. Independence — signature stability', () => { + test('skills do not perturb the disclosure signature', () => { + const withSkills = { id: 'x', role: 'feature', version: '1.0.0', skills: ['a', 'b'] }; + const withoutSkills = { id: 'x', role: 'feature', version: '1.0.0' }; + assert.equal(trust.signatureForManifest(withSkills), trust.signatureForManifest(withoutSkills)); + }); + + test('skills do not perturb the disclosure signature of an executable-surface-bearing manifest', () => { + const withSkills = executableSurfaceManifest((m) => { + m.skills = ['a', 'b']; + }); + const withoutSkills = executableSurfaceManifest(); + assert.equal(trust.signatureForManifest(withSkills), trust.signatureForManifest(withoutSkills)); + }); + + // These two `agents` signature tests still assert a true and useful property — agents never + // perturb the signature — but are now TRIVIALLY true, since `agents` is not collected into + // `instructionSurfaces` at all (it never reaches `collectInstructionSurfaces`'s per-field + // loop). Kept so they guard the NARROWING (agents dropped entirely) rather than D4 + // specifically — a regression that made `agents` collected again would still need a separate + // D4 test to catch a signature perturbation. + test('agents do not perturb the disclosure signature', () => { + const withAgents = { id: 'x', role: 'feature', version: '1.0.0', agents: ['gsd-ui-checker'] }; + const withoutAgents = { id: 'x', role: 'feature', version: '1.0.0' }; + assert.equal(trust.signatureForManifest(withAgents), trust.signatureForManifest(withoutAgents)); + }); + + test('agents do not perturb the disclosure signature of an executable-surface-bearing manifest', () => { + const withAgents = executableSurfaceManifest((m) => { + m.agents = ['gsd-ui-checker']; + }); + const withoutAgents = executableSurfaceManifest(); + assert.equal(trust.signatureForManifest(withAgents), trust.signatureForManifest(withoutAgents)); + }); + + // A JS re-implementation of the PRE-#3248 (and pre-#2796-lane) discloseExecutableSurfaces + // (hooks/commands/mcpServers ONLY) + disclosureSignature + stableJson — copied verbatim from + // `tests/reviewer-trust-disclosure.test.cjs`'s own `refDiscloseExecutableSurfaces` / + // `refStableJson` oracle (itself copied from src/capability-trust.cts as it stood before ADR-2782 + // Phase 3), which satisfies the fixture-provenance rule (#2371): it was written by a source that + // does not know the `skills`/`agents`/`instructionSurfaces` class exists at all. + function refAsString(v) { + return typeof v === 'string' ? v : ''; + } + + function refStableJson(value) { + if (value === null || typeof value !== 'object') return JSON.stringify(value) ?? 'null'; + if (Array.isArray(value)) return `[${value.map(refStableJson).join(',')}]`; + const keys = Object.keys(value).sort(); + return `{${keys.map((k) => `${JSON.stringify(k)}:${refStableJson(value[k])}`).join(',')}}`; + } + + function refDiscloseExecutableSurfaces(manifest) { + const hooks = []; + const commandModules = []; + const mcpServers = []; + + if (Array.isArray(manifest.hooks)) { + for (const h of manifest.hooks) { + if (typeof h !== 'object' || h === null) continue; + const script = refAsString(h['script']); + const event = refAsString(h['event']); + if (script) hooks.push({ event, script }); + } + } + + if (Array.isArray(manifest.commands)) { + for (const c of manifest.commands) { + if (typeof c !== 'object' || c === null) continue; + const moduleName = refAsString(c['module']); + const family = refAsString(c['family']); + const router = refAsString(c['router']); + if (moduleName) commandModules.push({ family, module: moduleName, router }); + } + } + + if (manifest.mcpServers && typeof manifest.mcpServers === 'object') { + const pushServer = (name, config) => { + if (!name) return; + const cfg = typeof config === 'object' && config !== null ? config : {}; + const command = refAsString(cfg['command']); + const rawArgs = Array.isArray(cfg['args']) ? cfg['args'] : []; + const argv = rawArgs.filter((a) => typeof a === 'string'); + const transport = refAsString(cfg['type']) || refAsString(cfg['transport']); + const url = refAsString(cfg['url']); + const headers = {}; + const rawHeaders = cfg['headers']; + if (rawHeaders && typeof rawHeaders === 'object' && !Array.isArray(rawHeaders)) { + for (const [k, v] of Object.entries(rawHeaders)) { + if (k === '__proto__' || k === 'constructor' || k === 'prototype') continue; + if (typeof v === 'string') headers[k] = v; + } + } + const env = {}; + const rawEnv = cfg['env']; + if (rawEnv && typeof rawEnv === 'object' && !Array.isArray(rawEnv)) { + for (const [k, v] of Object.entries(rawEnv)) { + if (k === '__proto__' || k === 'constructor' || k === 'prototype') continue; + if (typeof v === 'string') env[k] = v; + } + } + const cwd = refAsString(cfg['cwd']); + const rawConfig = {}; + for (const [k, v] of Object.entries(cfg)) { + if (k === '__proto__' || k === 'constructor' || k === 'prototype') continue; + rawConfig[k] = v; + } + const surface = { name, transport, command, argv, rawArgs, url, headers, env, rawConfig }; + if (cwd) surface.cwd = cwd; + mcpServers.push(surface); + }; + if (Array.isArray(manifest.mcpServers)) { + for (const s of manifest.mcpServers) { + if (typeof s === 'object' && s !== null) pushServer(refAsString(s['name']), s['config'] ?? s); + } + } else { + for (const [name, config] of Object.entries(manifest.mcpServers)) pushServer(name, config); + } + } + + return { hooks, commandModules, mcpServers }; + } + + function refDisclosureSignature(d) { + const hooks = d.hooks.map((h) => refStableJson(['hook', h.event, h.script])).sort(); + const mods = d.commandModules.map((m) => refStableJson(['mod', m.family, m.module, m.router || ''])).sort(); + const mcp = d.mcpServers + .map((s) => + refStableJson([ + 'mcp', + s.name, + s.transport || '', + s.command, + s.rawArgs || [], + s.url || '', + s.headers || {}, + s.env || {}, + s.cwd || '', + s.rawConfig || {}, + ]), + ) + .sort(); + return JSON.stringify([hooks, mods, mcp]); + } + + function referenceLaneFreeSignature(manifest) { + return refDisclosureSignature(refDiscloseExecutableSurfaces(manifest)); + } + + test('signature matches the pre-change oracle for a skill-bearing manifest', () => { + const manifestWithSkills = executableSurfaceManifest((m) => { + m.skills = ['ui-phase', 'ui-review']; + m.agents = ['gsd-ui-checker']; + }); + // The oracle does not know `skills`/`agents`/`reviewer` exist at all — it only ever reads + // hooks/commands/mcpServers — so its output for the skill-bearing manifest IS the reference + // "sans skills" signature the matrix asks for. + assert.equal(trust.signatureForManifest(manifestWithSkills), referenceLaneFreeSignature(manifestWithSkills)); + }); +}); + +// ─── G. Independence — hasExecutable (rows 21-22) ─────────────────────────── + +describe('G. Independence — hasExecutable', () => { + test('an instruction surface alone does not set hasExecutable', () => { + const d = trust.discloseExecutableSurfaces(skillsAndAgentsManifest()); + assert.equal(d.hasExecutable, false); + }); + + test('hasExecutable still reflects executable surfaces only', () => { + const manifest = skillsAndAgentsManifest((m) => { + m.hooks = [{ event: 'PostToolUse', script: 'hooks/x.js' }]; + }); + const d = trust.discloseExecutableSurfaces(manifest); + assert.equal(d.hasExecutable, true, 'the hook, not the instruction surfaces, sets hasExecutable'); + assert.equal(d.hooks.length, 1); + // Skills-only count: `skillsAndAgentsManifest` declares 2 skills + 1 agent, but agents are + // not collected (see the module-header comment above), so only the 2 skills disclose. + assert.equal(d.instructionSurfaces.length, 2, 'instruction surfaces still disclosed alongside the hook'); + }); +}); + +// ─── H. Independence — executableSetChanged (row 23) ──────────────────────── + +describe('H. Independence — executableSetChanged', () => { + test('adding a skill is not an executable-set change', () => { + const before = trust.discloseExecutableSurfaces({ id: 'x' }); + const after = trust.discloseExecutableSurfaces({ id: 'x', skills: ['a'] }); + assert.equal(trust.executableSetChanged(before, after), false); + }); +}); + +// ─── I. Regression — pre-existing consent record (row 24) ─────────────────── + +describe('I. Regression — pre-existing consent record', () => { + const LOCAL_SPEC = { kind: 'local', raw: '.', target: '.' }; + + test('a pre-existing consent record survives instruction-surface disclosure', () => { + // Simulates a consent record written BEFORE this phase (a manifest with no skills/agents), + // then the capability being upgraded to a version that adds a skill — the stored signature + // must still match, so no re-consent prompt fires. + const preChangeManifest = { id: 'x', role: 'feature', version: '1.0.0' }; + const v1 = trust.evaluateInstallTrust({ parsed: LOCAL_SPEC, manifest: preChangeManifest, hostVersion: '1.0.0' }); + const storedSignature = trust.disclosureSignature(v1.disclosure); + + const upgradedManifest = { id: 'x', role: 'feature', version: '1.1.0', skills: ['ui-phase'] }; + const v2 = trust.evaluateInstallTrust({ parsed: LOCAL_SPEC, manifest: upgradedManifest, hostVersion: '1.0.0' }); + const upgradedSignature = trust.disclosureSignature(v2.disclosure); + + assert.equal(upgradedSignature, storedSignature, 'the stored consent signature must still match after the upgrade'); + assert.equal( + trust.executableSetChanged(v1.disclosure, v2.disclosure), + false, + 'gaining a skill must not force a re-consent prompt', + ); + assert.equal(v2.requiresConsent, false, 'a skill-only capability requires no consent at all'); + }); +}); + +// ─── J. Independence — missingArtifacts (row 25) ──────────────────────────── + +describe('J. Independence — missingArtifacts', () => { + test('skill stems are never existence-checked against stagedDir', (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'instr-surface-j1-')); + t.after(() => cleanup(dir)); + + const manifest = { id: 'x', skills: ['nonexistent-skill-stem', 'another-missing-one'] }; + const d = trust.discloseExecutableSurfaces(manifest, dir); + assert.deepEqual( + d.missingArtifacts, + [], + 'skill stems are registry names, not bundle-relative artifact paths — they must never contribute to missingArtifacts', + ); + assert.equal(d.instructionSurfaces.length, 2, 'the skills must still be disclosed'); + }); +}); + +// ─── K. Consent prompt (rows 26-27) ────────────────────────────────────────── +// +// `summarizeInstructionSurfaces(disclosure)` is the typed surface the implementation added for +// exactly this: CONTRIBUTING's "Prohibited: Raw Text Matching on Test Outputs" forbids regex-matching +// `summarizeDisclosure`'s rendered prose, so these rows assert on that function's structured output +// (its length against the declared surface count) and on ARRAY CONTAINMENT between the two +// renderers — never on the wording of a line. + +describe('K. Consent prompt', () => { + test('consent prompt names instruction surfaces separately', () => { + const manifest = { id: 'x', skills: ['ui-phase', 'ui-review'] }; + const d = trust.discloseExecutableSurfaces(manifest); + assert.deepEqual(d.instructionSurfaces, [ + { kind: 'skill', name: 'ui-phase' }, + { kind: 'skill', name: 'ui-review' }, + ]); + assert.deepEqual(d.hooks, [], 'instruction surfaces must not be folded into an executable-surface class'); + assert.equal(d.hasExecutable, false, 'a skill-only manifest never requires consent from this data'); + + // One header line + one line per surface + one "not content-scanned" line. + const section = trust.summarizeInstructionSurfaces(d); + assert.equal(section.length, d.instructionSurfaces.length + 2, 'every declared surface gets its own line'); + }); + + test('consent prompt omits the section when there is nothing to disclose', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x' }); + assert.deepEqual(d.instructionSurfaces, []); + assert.deepEqual( + trust.summarizeInstructionSurfaces(d), + [], + 'nothing declared => no lines at all, so no empty header can render', + ); + }); + + // Row 26a — the defect this phase is most likely to ship silently. A skill-only capability has + // hasExecutable === false and takes summarizeDisclosure's EARLY RETURN, so a section appended only + // at the end of the function would never render for precisely the capabilities that need it. + // Asserted as ARRAY CONTAINMENT of one renderer's output in the other's — a structural property, + // not a prose match. + test('a skill-only capability still renders its instruction surfaces in the consent summary', () => { + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: ['ui-phase'] }); + assert.equal(d.hasExecutable, false, 'precondition: this manifest takes the early-return branch'); + const section = trust.summarizeInstructionSurfaces(d); + const summary = trust.summarizeDisclosure(d); + assert.ok(section.length > 0, 'precondition: there is a section to render'); + for (const line of section) { + assert.ok(summary.includes(line), 'every instruction-surface line must reach the rendered summary'); + } + }); + + // Row 26b — the same containment property for a capability that ships BOTH, where the summary + // takes the executable branch instead. + test('a capability with both executable and instruction surfaces renders both', () => { + const manifest = executableSurfaceManifest((m) => { + m.skills = ['ui-phase']; + m.agents = ['gsd-ui-checker']; + }); + const d = trust.discloseExecutableSurfaces(manifest); + assert.equal(d.hasExecutable, true, 'precondition: this manifest takes the executable branch'); + const section = trust.summarizeInstructionSurfaces(d); + const summary = trust.summarizeDisclosure(d); + // Skills-only: the declared `agents` entry is not collected, so this is 1 surface (the + // skill), not 2 — header + 1 surface + the not-scanned line. + assert.equal(section.length, 3, 'header + 1 surface + the not-scanned line'); + for (const line of section) { + assert.ok(summary.includes(line), 'every instruction-surface line must reach the rendered summary'); + } + }); + + // Row 26c — the CLI edge calls `summarizeDisclosure(res.disclosure || {})` + // (gsd-core/bin/lib/capability-command-router.cjs), so a BARE `{}` carrying no arrays at all + // reaches both renderers whenever a lifecycle result has no disclosure. Reading + // `.instructionSurfaces.length` off that object unguarded would throw a TypeError at the consent + // prompt — a crash on the exact path that is supposed to inform the user. + test('a partial disclosure object from the CLI edge never throws', () => { + assert.doesNotThrow(() => trust.summarizeInstructionSurfaces({})); + assert.deepEqual(trust.summarizeInstructionSurfaces({}), []); + assert.doesNotThrow(() => trust.summarizeDisclosure({})); + assert.deepEqual(trust.summarizeDisclosure({}), ['This capability ships no executable surfaces (declarative only).']); + }); + + // Row 26d — a manifest may declare an unbounded number of stems. `lines.push(...section)` would + // exceed the engine's argument limit and throw RangeError here; the renderer must iterate. This + // guards a "simplification" back to spread, which no smaller fixture can catch. + test('an unbounded stem count does not break the renderer', () => { + const stems = Array.from({ length: 200000 }, (_, i) => `s${i}`); + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: stems }); + assert.equal(d.instructionSurfaces.length, 200000); + let summary; + assert.doesNotThrow(() => { + summary = trust.summarizeDisclosure(d); + }); + assert.equal(summary.length, 200000 + 3, 'intro line + header + one line per stem + the not-scanned line'); + }); +}); + +// ─── L. Cross-platform (row 28) ────────────────────────────────────────────── + +describe('L. Cross-platform', () => { + test('CRLF in a stem does not split the entry', () => { + const stem = 'ui-phase\r\nwith-crlf'; + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: [stem] }); + assert.deepEqual( + d.instructionSurfaces, + [{ kind: 'skill', name: stem }], + 'a CRLF inside a stem must be disclosed verbatim as ONE entry, never split into two', + ); + }); +}); + +// ─── M. Property-based (fast-check) ───────────────────────────────────────── +// +// Generalizes the hand-written A-L fixtures with an ADVERSARIAL manifest arbitrary: valid string +// stems mixed with non-strings, blanks, nested arrays, nested objects, nulls, and (via +// `instructionFieldArb`'s low-weight branch) `skills`/`agents` occasionally replaced wholesale by a +// non-array. `manifestArb` STILL GENERATES `agents` (good — hostile/adversarial input coverage), +// but `agents` is never collected into `instructionSurfaces`: only `skills` feeds +// `collectInstructionSurfaces`'s per-field loop (`INSTRUCTION_SURFACE_FIELDS` is a one-row table). +// `manifestArb` always yields a plain object (never array/null/Proxy — those totality +// cases are covered directly in section C/D) so P2/P3 can safely spread-and-delete `skills`/`agents` +// off the SAME generated manifest, matching this file's `refDiscloseExecutableSurfaces`/section D's +// established idiom of importing fast-check as `const fc = require('fast-check')` and calling +// `fc.assert(fc.property(...))` with no per-call seed/numRuns override. + +describe('M. Property-based (fast-check)', () => { + const stringArb = fc.string(); + const blankArb = fc.constantFrom('', ' ', '\n', '\t'); + const nonStringStemArb = fc.oneof( + fc.integer(), + fc.boolean(), + fc.constant(null), + fc.constant(undefined), + fc.array(stringArb, { maxLength: 3 }), + fc.object({ maxDepth: 1 }), + ); + const stemMemberArb = fc.oneof( + { weight: 3, arbitrary: stringArb }, + { weight: 1, arbitrary: blankArb }, + { weight: 1, arbitrary: nonStringStemArb }, + ); + const stemsArrayArb = fc.array(stemMemberArb, { maxLength: 6 }); + // Occasionally replace the whole field with a non-array (a scalar, null, or a plain object) — + // exercises `collectInstructionSurfaces`'s "a non-array field declares nothing" branch. + const instructionFieldArb = fc.oneof( + { weight: 5, arbitrary: stemsArrayArb }, + { weight: 1, arbitrary: fc.oneof(stringArb, fc.integer(), fc.constant(null), fc.object({ maxDepth: 1 })) }, + ); + + const hookArb = fc.record({ event: stringArb, script: stringArb }, { requiredKeys: [] }); + const commandArb = fc.record({ family: stringArb, module: stringArb, router: stringArb }, { requiredKeys: [] }); + const mcpConfigArb = fc.record( + { command: stringArb, args: fc.array(fc.oneof(stringArb, fc.integer())) }, + { requiredKeys: [] }, + ); + + // Always a plain object — P2/P3 rely on being able to spread it and delete skills/agents. + const manifestArb = fc.record( + { + id: stringArb, + hooks: fc.array(hookArb, { maxLength: 3 }), + commands: fc.array(commandArb, { maxLength: 3 }), + mcpServers: fc.dictionary(stringArb, mcpConfigArb), + skills: instructionFieldArb, + agents: instructionFieldArb, + }, + { requiredKeys: [] }, + ); + + /** `m` with `skills`/`agents` deleted — the D4 "sans instruction surfaces" comparison object. */ + function withoutInstructionFields(m) { + const m2 = { ...m }; + delete m2.skills; + delete m2.agents; + return m2; + } + + test('P1: discloseExecutableSurfaces is total and every instruction surface is well-shaped', () => { + fc.assert( + fc.property(manifestArb, (manifest) => { + let d; + assert.doesNotThrow(() => { + d = trust.discloseExecutableSurfaces(manifest); + }, `discloseExecutableSurfaces threw for manifest=${JSON.stringify(manifest)}`); + assert.ok(Array.isArray(d.instructionSurfaces), 'instructionSurfaces must always be an array'); + for (const surface of d.instructionSurfaces) { + // Skills-only: `agents` is generated by the arbitrary but never collected, so every + // disclosed instruction surface must be a skill. + assert.equal(surface.kind, 'skill', `unexpected kind ${JSON.stringify(surface.kind)}`); + assert.equal(typeof surface.name, 'string', `name must be a string, got ${typeof surface.name}`); + assert.ok(surface.name.length > 0, 'name must be non-empty'); + } + }), + ); + }); + + test('P2: ADR-2363 D4 — instruction surfaces never perturb the disclosure signature', () => { + fc.assert( + fc.property(manifestArb, (manifest) => { + const m2 = withoutInstructionFields(manifest); + assert.equal( + trust.signatureForManifest(manifest), + trust.signatureForManifest(m2), + `signature diverged for manifest=${JSON.stringify(manifest)}`, + ); + }), + ); + }); + + test('P3: ADR-2363 D3 — instruction surfaces never perturb hasExecutable', () => { + fc.assert( + fc.property(manifestArb, (manifest) => { + const m2 = withoutInstructionFields(manifest); + assert.equal( + trust.discloseExecutableSurfaces(manifest).hasExecutable, + trust.discloseExecutableSurfaces(m2).hasExecutable, + `hasExecutable diverged for manifest=${JSON.stringify(manifest)}`, + ); + }), + ); + }); + + test('P4: summarizeInstructionSurfaces is total and its length tracks instructionSurfaces.length', () => { + fc.assert( + fc.property(manifestArb, (manifest) => { + const d = trust.discloseExecutableSurfaces(manifest); + let section; + assert.doesNotThrow(() => { + section = trust.summarizeInstructionSurfaces(d); + }, `summarizeInstructionSurfaces threw for manifest=${JSON.stringify(manifest)}`); + if (d.instructionSurfaces.length === 0) { + assert.deepEqual(section, []); + } else { + assert.equal(section.length, d.instructionSurfaces.length + 2); + } + }), + ); + }); +}); + +// ─── N. Consent-prompt injection safety ───────────────────────────────────── +// +// #3248 BLOCKER finding: `summarizeDisclosure`'s lines are joined with `\n` and written RAW to +// stderr on the needs-consent path (`capability-command-router.cjs`). An unescaped newline in a +// manifest-supplied value forged lines indistinguishable from genuine GSD disclosure text, and an +// unescaped ANSI/control sequence could rewrite already-printed terminal lines. `renderValueForPrompt` +// is the fix: every manifest-supplied value rendered into a consent-prompt line is escaped and +// length-bounded first. The DISCLOSURE OBJECT itself still carries values VERBATIM (unchanged) — +// only the RENDERED line is escaped. +// +// Assertions here are on TYPED values and STRUCTURAL properties only (array length, character-class +// absence) — CONTRIBUTING.md forbids regex-matching rendered prose. Checking a rendered line for the +// ABSENCE of specific control characters is a structural safety property, not a prose match. + +describe('N. Consent-prompt injection safety', () => { + // Every character that must never survive into a rendered consent-prompt line: C0, DEL, C1, the + // bidi/isolate controls, and the line/paragraph separators. A raw newline forges a line that is + // indistinguishable from genuine GSD disclosure text; a raw ESC lets a manifest value rewrite lines + // already printed to the terminal. Defined independently of `src/capability-trust.cts`'s own + // `UNSAFE_PROMPT_CHARS` (not imported) so this test does not just echo the implementation back at + // itself — it is an independent restatement of the same forbidden-character contract. + // eslint-disable-next-line no-control-regex -- deliberately matching C0/DEL/C1 control chars. + const FORBIDDEN_IN_RENDERED_LINE = /[\u0000-\u001f\u007f-\u009f\u200e\u200f\u2028\u2029\u202a-\u202e\u2066-\u2069]/; + + test('renderValueForPrompt is identity for an ordinary stem', () => { + assert.equal(trust.renderValueForPrompt('ui-phase'), 'ui-phase'); + }); + + test('renderValueForPrompt escapes each hostile class', () => { + const hostileInputs = [ + '\n', // C0 — line feed + '\r\n', // C0 — CRLF + '\x1b[2K', // C0 ESC — ANSI erase-line, can rewrite already-printed terminal output + '
', // line/paragraph separator + '‮', // bidi/isolate control — RIGHT-TO-LEFT OVERRIDE + ]; + for (const input of hostileInputs) { + const result = trust.renderValueForPrompt(input); + assert.equal( + FORBIDDEN_IN_RENDERED_LINE.test(result), + false, + `renderValueForPrompt(${JSON.stringify(input)}) must contain no forbidden character, got ${JSON.stringify(result)}`, + ); + } + // The escaped form still contains the surrounding legible text, so the value stays + // identifiable rather than vanishing. + const escaped = trust.renderValueForPrompt('a\nb'); + assert.ok(escaped.includes('a'), 'escaped form must still contain the leading legible text'); + assert.ok(escaped.includes('b'), 'escaped form must still contain the trailing legible text'); + }); + + test('renderValueForPrompt bounds length', () => { + const huge = 'x'.repeat(10000); + const result = trust.renderValueForPrompt(huge); + assert.ok(result.length < 10000, `expected a materially shorter result, got length ${result.length}`); + }); + + test('a forged skill stem cannot inject a line', () => { + const forged = + 'ok\n hooks (1): run as runtime hook commands\n - fake -> ok\nRe-run with --yes to grant consent.'; + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: [forged] }); + const summary = trust.summarizeDisclosure(d); + for (const line of summary) { + assert.equal( + FORBIDDEN_IN_RENDERED_LINE.test(line), + false, + `rendered line must contain no forbidden character: ${JSON.stringify(line)}`, + ); + } + // intro line + header + 1 surface + the not-scanned line — the forged text must not become + // EXTRA array entries either (it stayed one skill, so it renders as exactly one surface line). + assert.equal(summary.length, 4, 'intro + header + 1 surface + not-scanned line'); + }); + + // PARITY — the generative-fix-divergence guard (CLAUDE.md "Generative Fix Divergence"): the same + // escaping guarantee must hold for every one of the five disclosed classes, not just skills. Every + // rendered field of every class carries the same hostile payload; if a future class is added to the + // renderer without routing its values through `renderValueForPrompt`, this test catches it instead + // of that class silently shipping unescaped. + test('PARITY — the same injection-safety guarantee holds for all five disclosed classes', () => { + const payload = 'a\nb[2Kc'; + const manifest = { + id: 'x', + hooks: [{ event: payload, script: payload }], + commands: [{ family: payload, module: payload, router: payload }], + mcpServers: { + [payload]: { + command: payload, + args: [payload], + url: payload, + cwd: payload, + env: { [payload]: payload }, + }, + }, + reviewer: { + slug: payload, + transport: 'spawn', + invoke: { + binary: payload, + args: [payload], + hostConfigKey: payload, + }, + handler: payload, + }, + skills: [payload], + }; + const d = trust.discloseExecutableSurfaces(manifest); + const summary = trust.summarizeDisclosure(d); + for (const line of summary) { + assert.equal( + FORBIDDEN_IN_RENDERED_LINE.test(line), + false, + `rendered line must contain no forbidden character: ${JSON.stringify(line)}`, + ); + } + }); + + test('the disclosure OBJECT stays verbatim', () => { + // Escaping is a RENDERING concern only. The object must stay verbatim because + // `disclosureSignature` and any consumer reasoning about identity depend on the declared + // value, not the escaped-for-display one. + const forged = 'ok\n hooks (1): run as runtime hook commands\nRe-run with --yes to grant consent.'; + const d = trust.discloseExecutableSurfaces({ id: 'x', skills: [forged] }); + assert.equal(d.instructionSurfaces[0].name, forged, 'the disclosure object must carry the exact unescaped value'); + }); +});