From c82355972d51d0769c475a6e2f481c67caef8fc3 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 7 Jul 2026 19:38:01 -0400 Subject: [PATCH 1/4] =?UTF-8?q?test(#2009):=20fail-first=20=E2=80=94=20loa?= =?UTF-8?q?d-failed=20capability=20gate=20must=20fail=20open=20loudly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asserts the loop resolver injects NO gate for a load-failed overlay capability (fail open) and instead emits a loud warning — to stderr and the envelope 'warnings' array — carrying the 'gsd capability remove ' remediation, and that an invalid (non-kebab) capability id is withheld from the runnable command. Fails against origin/next (which injects a blocking fail-closed gate). --- tests/loop-render-hooks.test.cjs | 147 +++++++++++++++++++++---------- 1 file changed, 102 insertions(+), 45 deletions(-) diff --git a/tests/loop-render-hooks.test.cjs b/tests/loop-render-hooks.test.cjs index dc82b982a..ef91f7313 100644 --- a/tests/loop-render-hooks.test.cjs +++ b/tests/loop-render-hooks.test.cjs @@ -1039,79 +1039,136 @@ describe('Phase 4 regression: capabilityStatesById gates on active (not enabled) }); }); -// ─── ADR-1244 D2 fail-closed gate injection ──────────────────────────────────── +// ─── ADR-1244 D2: load-failed capability gates FAIL OPEN with a loud warning (#2009) ── -describe('ADR-1244 D2: fail-closed gate injection for skipped overlay caps with gates', () => { - // Verifies that cmdLoopRenderHooks injects a BLOCKING synthetic gate at the - // declared point when an overlay capability that declares a gate is skipped at - // load time due to an incompatible engines.gsd version constraint. - // - // Fixture: overlay cap declares a gate at execute:wave:post with engines.gsd: ">=99.0.0" - // → loadRegistry skips it → records it in _overlay.blockedGates - // → cmdLoopRenderHooks injects a blocking=true, onError=halt gate at execute:wave:post +describe('ADR-1244 D2: load-failed capability gates fail OPEN with a loud warning (#2009)', () => { + // Decision (#2009): a capability that fails to LOAD must not block the loop. + // cmdLoopRenderHooks injects NO gate (the loop proceeds — fail open) and emits a + // loud warning naming the load reason and the exact `gsd capability remove ` + // remediation, both to STDERR (the channel host workflows actually surface) and + // in the envelope's `warnings` array. Previously it injected a blocking=true, + // onError=halt gate that halted every declared point project-wide. - test('skipped gate-kind overlay cap → BLOCKING synthetic gate at its declared point', (t) => { - const overlayHome = fs.mkdtempSync(path.join(os.tmpdir(), 'loop-fail-closed-')); - t.after(() => cleanup(overlayHome)); - - // Write an overlay capability that: - // - declares a gate at execute:wave:post - // - has engines.gsd: ">=99.0.0" (incompatible → will be skipped at load) - const capId = 'fail-closed-gate-cap'; + // Helper: write an overlay cap with an incompatible engines.gsd (→ skipped at load) + // that declares gate(s) at the given point(s). `capId` may be an invalid id to + // exercise the sanitization path. + function writeSkippedGateCap(overlayHome, capId, gatePoints) { const capDir = path.join(overlayHome, '.gsd', 'capabilities', capId); fs.mkdirSync(capDir, { recursive: true }); const capManifest = { id: capId, role: 'feature', version: '1.0.0', - title: 'Fail Closed Gate Cap', - description: 'ADR-1244 D2 fail-closed wiring test', + title: 'Load-Failed Gate Cap', + description: 'ADR-1244 D2 fail-open wiring test', tier: 'standard', requires: [], - engines: { gsd: '>=99.0.0' }, // intentionally incompatible → always skipped + engines: { gsd: '>=99.0.0' }, // intentionally incompatible → always skipped at load runtimeCompat: { supported: ['*'], unsupported: [] }, skills: [], agents: [], hooks: [], config: {}, steps: [], contributions: [], - gates: [{ point: 'execute:wave:post', check: 'always-pass', blocking: true, onError: 'halt' }], + gates: gatePoints.map((point) => ({ point, check: { query: 'always-pass' }, blocking: true, onError: 'halt' })), }; fs.writeFileSync(path.join(capDir, 'capability.json'), JSON.stringify(capManifest), 'utf8'); + } - // Invoke gsd-tools via subprocess so stdout is the real fd-1 (io.cjs writes via writeSync). - // Set GSD_HOME to the overlay home so loadRegistry picks up the incompatible cap. + function renderHooks(overlayHome, point, extraArgs = []) { const result = spawnSync( process.execPath, - [GSD_TOOLS, 'loop', 'render-hooks', 'execute:wave:post', '--cwd', overlayHome], - { - cwd: ROOT, - encoding: 'utf8', - env: { ...process.env, GSD_HOME: overlayHome }, - }, + [GSD_TOOLS, 'loop', 'render-hooks', point, '--cwd', overlayHome, ...extraArgs], + { cwd: ROOT, encoding: 'utf8', env: { ...process.env, GSD_HOME: overlayHome } }, ); + assert.strictEqual(result.status, 0, `Expected exit 0 at ${point}. stderr: ` + (result.stderr || '')); + return result; + } - assert.strictEqual(result.status, 0, 'Expected exit 0. stderr: ' + (result.stderr || '')); - - let envelope; + function parseEnvelope(result) { try { - envelope = JSON.parse(result.stdout.trim()); + return JSON.parse(result.stdout.trim()); } catch { assert.fail('loop render-hooks output must be valid JSON; got: ' + result.stdout.slice(0, 300)); } + } - // The synthetic blocking gate must be present in activeHooks - const syntheticGate = Array.isArray(envelope.activeHooks) - ? envelope.activeHooks.find((h) => h.capId === capId && h.kind === 'gate') + test('load-failed gate-kind overlay cap → NO gate injected (fail open) + loud warning on stderr and in envelope', (t) => { + const overlayHome = fs.mkdtempSync(path.join(os.tmpdir(), 'loop-fail-open-')); + t.after(() => cleanup(overlayHome)); + + const capId = 'load-failed-gate-cap'; + writeSkippedGateCap(overlayHome, capId, ['execute:wave:post']); + + const result = renderHooks(overlayHome, 'execute:wave:post'); + const envelope = parseEnvelope(result); + + // AC2 / AC-a: fail OPEN — NO gate (blocking or otherwise) is injected for the + // load-failed cap, so the loop proceeds. + const anyGate = Array.isArray(envelope.activeHooks) + ? envelope.activeHooks.find((h) => h.capId === capId) : undefined; - assert.ok( - syntheticGate !== undefined, - `activeHooks must contain a synthetic gate attributed to ${capId} (fail-closed injection). ` + - 'Got: ' + JSON.stringify(envelope.activeHooks), + assert.strictEqual( + anyGate, undefined, + 'no hook must be injected for a load-failed cap (fail open). Got: ' + JSON.stringify(envelope.activeHooks), ); - assert.strictEqual(syntheticGate.blocking, true, 'synthetic gate must be blocking=true'); - assert.strictEqual(syntheticGate.onError, 'halt', 'synthetic gate must have onError=halt'); - // The rendered markdown must also reference the gate cap + // AC-b: a loud warning is surfaced in the envelope `warnings` channel... + const warnEnv = (envelope.warnings || []).find((w) => w.includes(capId)); + assert.ok(warnEnv, 'envelope.warnings must name the load-failed cap. Got: ' + JSON.stringify(envelope.warnings)); + // AC1: ...carrying the exact remediation, and making clear the gate is not enforced. assert.ok( - typeof envelope.rendered === 'string' && envelope.rendered.includes(capId), - 'rendered output must reference the fail-closed gate cap. Got: ' + envelope.rendered, + warnEnv.includes(`gsd capability remove ${capId}`), + 'warning must include the `gsd capability remove ` remediation. Got: ' + warnEnv, + ); + assert.match(warnEnv, /skipped|not enforced|failing open/i, 'warning must say the gate is not enforced. Got: ' + warnEnv); + + // ...and ALSO to stderr (the channel host workflows actually see). + assert.ok( + result.stderr.includes(`gsd capability remove ${capId}`), + 'stderr must carry the loud fail-open warning with remediation. Got: ' + result.stderr, + ); + }); + + test('load-failed cap declaring gates at ship:pre AND verify:post → neither point blocks (project can ship & verify)', (t) => { + const overlayHome = fs.mkdtempSync(path.join(os.tmpdir(), 'loop-fail-open-2pt-')); + t.after(() => cleanup(overlayHome)); + + const capId = 'load-failed-two-point-cap'; + writeSkippedGateCap(overlayHome, capId, ['ship:pre', 'verify:post']); + + for (const point of ['ship:pre', 'verify:post']) { + const result = renderHooks(overlayHome, point); + const envelope = parseEnvelope(result); + const injected = envelope.activeHooks.find((h) => h.capId === capId); + assert.strictEqual( + injected, undefined, + `${point} must NOT inject any hook for a load-failed cap (fail open). Got: ` + JSON.stringify(envelope.activeHooks), + ); + const warn = (envelope.warnings || []).find((w) => w.includes(`gsd capability remove ${capId}`)); + assert.ok(warn, `${point} must surface a fail-open warning with remediation. Got: ` + JSON.stringify(envelope.warnings)); + } + }); + + test('security: an invalid (non-kebab) capability id is withheld — no runnable remove command is rendered (#2009 review)', (t) => { + const overlayHome = fs.mkdtempSync(path.join(os.tmpdir(), 'loop-fail-open-evil-')); + t.after(() => cleanup(overlayHome)); + + // A directory name that is a valid POSIX filename but NOT a valid capability + // id, and contains a shell metacharacter. It must never appear inside a + // runnable `gsd capability remove ` command in the surfaced warning. + const evilId = 'Bad;Cap'; + writeSkippedGateCap(overlayHome, evilId, ['ship:pre']); + + const result = renderHooks(overlayHome, 'ship:pre'); + const envelope = parseEnvelope(result); + + const combined = (envelope.warnings || []).join('\n') + '\n' + result.stderr; + // The raw metacharacter id must not be embedded in a runnable remove command. + assert.ok( + !combined.includes(`gsd capability remove ${evilId}`), + 'invalid capability id must NOT be placed in a runnable remove command. Got: ' + combined, + ); + // A warning is still surfaced (fail-open is loud), using the withheld-id form. + assert.match( + combined, /invalid id \(withheld\)/, + 'an invalid id must be reported via the withheld-id placeholder. Got: ' + combined, ); }); }); From 46f8d3814c9a1ddf0e479e4de99548c9d3f983fc Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 7 Jul 2026 19:38:01 -0400 Subject: [PATCH 2/4] fix(#2009): load-failed capability gates fail open with a loud warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously a capability that failed to LOAD (e.g. incompatible engines.gsd) but declared a gate-kind loop hook caused the loop resolver to inject a BLOCKING synthetic gate (blocking:true, onError:halt) at every declared point, halting every ship:pre / verify:post project-wide over an unrelated load error, with no remediation surfaced. Per maintainer decision (#2009) it now fails OPEN: no gate is injected (the loop proceeds; --active-cap correctly reports the failed cap inactive) and a loud warning is emitted — to stderr (the channel host workflows/agents actually see) and in the envelope 'warnings' array — naming the load reason and the exact 'gsd capability remove ' remediation. The loader still records blockedGates; only the consequence changes from block to warn. Security (review): capId and reason originate from a third-party manifest / directory name. capId is validated against the canonical kebab-case id shape before it is placed in the runnable remediation command (withheld otherwise); reason is stripped of control chars and backticks. This closes an argument/ prompt-injection vector in the surfaced message. Docs updated to the fail-open-warning posture (ARCHITECTURE, INVENTORY, CONFIGURATION, README, capability-overlay-model). Also removes a dead 'before' import surfaced by lint in the issue-2045 test. --- docs/ARCHITECTURE.md | 2 +- docs/CONFIGURATION.md | 4 +- docs/INVENTORY.md | 2 +- docs/README.md | 2 +- docs/explanation/capability-overlay-model.md | 56 +++++++----- src/capability-loader.cts | 20 +++-- src/loop-resolver.cts | 88 +++++++++++++++---- ...e-2045-third-party-skills-surface.test.cjs | 2 +- 8 files changed, 123 insertions(+), 53 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 760409bcd..59ef61380 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -381,7 +381,7 @@ Node.js CLI utility (`gsd-tools.cjs`) with domain modules split across `gsd-core | `profile-pipeline.cjs` | User behavioral profiling data pipeline, session file scanning | | `profile-output.cjs` | Profile rendering, USER-PROFILE.md and dev-preferences.md generation | | `loop-host-contract.cjs` | Generated Loop Host Contract — 12 loop points, per-step agent roles, and core artifacts; emitted by `scripts/gen-loop-host-contract.cjs` from workflow markers (ADR-894 §3); consumed by `gen-capability-registry.cjs` | -| `capability-loader.cjs` | Runtime registry overlay loader (ADR-1244 D2) — `loadRegistry({ includeInstalled })` composes the frozen first-party registry with a validated installed overlay of third-party capability manifests read from global `$GSD_HOME/.gsd/capabilities/` and project `/.gsd/capabilities/`; first-party always wins; load-time `engines.gsd` re-gate skips incompatible overlays with a warning; gate-kind hooks on skipped capabilities fail CLOSED | +| `capability-loader.cjs` | Runtime registry overlay loader (ADR-1244 D2) — `loadRegistry({ includeInstalled })` composes the frozen first-party registry with a validated installed overlay of third-party capability manifests read from global `$GSD_HOME/.gsd/capabilities/` and project `/.gsd/capabilities/`; first-party always wins; load-time `engines.gsd` re-gate skips incompatible overlays with a warning; gate-kind hooks on skipped capabilities fail OPEN — no gate is injected; a loud warning (stderr + envelope `warnings`) names the load failure and the `gsd capability remove ` remediation (#2009) | | `capability-registry.cjs` | Generated central Capability Registry — role-partitioned index of all co-located capability declarations; emitted by `scripts/gen-capability-registry.cjs` (ADR-894 §5) | | `loop-resolver.cjs` | Loop Extension Point resolver — ADR-857 phase 3c registry-consuming query; consumes resolved Capability State, filters `byLoopPoint` by capability enablement plus config activation, renders active hooks as markdown, emits `{ point, activeHooks, rendered }` envelope; `gsd-tools loop render-hooks [--config-dir ]` | | `capability-state.cjs` | Unified capability-state resolver — ADR-857 phase 4b/6; composes install profile, runtime surface, and config activation into one per-capability view consumed by workflow hook rendering; pure `resolveCapabilityState`, reusable `resolveCapabilityRuntimeState`, I/O `cmdCapabilityState`, and convenience predicate `isCapabilityActive(capId, cwd)`; `gsd-tools capability state [--config-dir ]` emits `{ runtimeConfigDir, capabilities[] }` where each entry carries `enabled` (installed && surfaced) and `active` (enabled && configActivation via the capability's `activationKey`; absent key → active===enabled) | diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index f1c4357a8..eb6f36b6a 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -769,9 +769,9 @@ Installed overlay capabilities are merged via the same `buildRegistry` pipeline Each overlay manifest may declare an `engines.gsd` semver range. At load time GSD evaluates this range against the running GSD version. An overlay that does not satisfy the range is **skipped with a warning** — it is never loaded and never crashes the loop. Manifests without an `engines.gsd` field are accepted unconditionally. -### Gate-kind fail-closed policy +### Gate-kind fail-open policy (#2009) -If a skipped overlay capability declared a `gate`-kind loop hook, the loop resolver **injects a blocking gate** at that hook point (fail CLOSED). Skipped capabilities whose hooks are `step` or `contribution` kind skip open — the loop proceeds without them. +If a skipped or load-failed overlay capability (for example, one whose `engines.gsd` range is incompatible) declared a `gate`-kind loop hook, the loop resolver does **not** inject a gate at that hook point (fail OPEN): the loop proceeds. Instead it emits a loud warning — to stderr and in the `loop render-hooks` envelope's `warnings` array — naming the load-failure reason and the exact remediation, `gsd capability remove `, so the operator is loudly told how to clear it. Skipped capabilities whose hooks are `step` or `contribution` kind skip open too, as before — the loop proceeds without them. ### Overlay config federation diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 8743b4305..6562c72c1 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -405,7 +405,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`. | `capability-lock.cjs` | Shared cross-process lock primitive (#1459 finding 4) — the SINGLE hardened lockfile protocol used by BOTH capability-lifecycle (`.gsd/capabilities/.lock`) and capability-consent (`.consent.lock`); exports `acquireLock(lockPath, opts?)`/`releaseLock(handle)` with pid + process-start-time liveness identity, a hard deadman, and token+inode owner-safe release — NEVER stale-steals a verified-live same-host holder, reclaims only a provably-dead/unverifiable holder, never deadlocks; `opts.maxAttempts`/`opts.waitForFresh` let the consent store serialize genuinely-contended writers; `_setLockProbes`/`_resetLockProbes` are test seams | | `capability-ledger.cjs` | Per-runtime install ledger (ADR-1244 D4) — atomic read/write of `.gsd-capabilities.json` recording `{ id, version, source, integrity, files[], sharedEdits[] }` per installed capability; exports `readLedger`/`writeLedger`/`recordInstall`/`removeEntry`/`reconcile` (orphan detection)/`readSmallRegularFile` (utf8) + `readSmallRegularFileBuffer` (raw bytes, the byte-exact consent-hash reader, #1459 finding 1); atomic commit point and reconciliation basis for Phase-4 upgrade/remove | | `capability-lifecycle.cjs` | Capability lifecycle orchestration (ADR-1244 Phase 4, D5+D6) — composes the source resolver + ledger + trust gate into `installCapability`/`upgradeCapability`/`removeCapability`/`reconcileCapabilities`; ledger write is the commit point; upgrade is atomic stage-then-swap (old set aside, new swapped in, ledger committed, backup dropped) with deterministic crash recovery (`reconcileCapabilities` rolls forward/back to a fully-old-or-fully-new state); remove surgically strips only marker-stamped (`_gsdCapability`) shared-config entries, preserving user hand-edits; never executes capability code | -| `capability-loader.cjs` | Runtime Capability Registry overlay (ADR-1244 D2) — `loadRegistry({ includeInstalled })` composes the frozen first-party registry with a validated installed overlay read from `$GSD_HOME/.gsd/capabilities//` (global) and `/.gsd/capabilities//` (project); first-party-wins on id/skill/agent/config collisions, reserved-namespace rejection, load-time `engines.gsd` re-gate (skip-with-warning), and gate-kind fail-closed via `_overlay.blockedGates`; composes through the canonical `buildRegistry` so derived views never drift | +| `capability-loader.cjs` | Runtime Capability Registry overlay (ADR-1244 D2) — `loadRegistry({ includeInstalled })` composes the frozen first-party registry with a validated installed overlay read from `$GSD_HOME/.gsd/capabilities//` (global) and `/.gsd/capabilities//` (project); first-party-wins on id/skill/agent/config collisions, reserved-namespace rejection, load-time `engines.gsd` re-gate (skip-with-warning), and gate-kind fail-open via `_overlay.blockedGates` — a loud warning (stderr + envelope `warnings`) naming the load failure and `gsd capability remove ` remediation; no gate injected (#2009); composes through the canonical `buildRegistry` so derived views never drift | | `capability-registry.cjs` | Generated central Capability Registry — role-partitioned index of all co-located capability declarations (`capabilities//capability.json`); emitted by `scripts/gen-capability-registry.cjs --write` (ADR-894 §5) | | `capability-source.cjs` | Capability source resolver (ADR-1244 D3) — `resolveCapabilitySource(spec, opts)` fetches and stages a capability from local path, git (https/ssh/git transports only), npm pack (no lifecycle scripts), tarball (sha512 integrity verify before extraction), or registry (stub); tar-slip/symlink rejection; atomic staging to `$GSD_HOME/.gsd/capabilities//`; no capability code executes during install | | `capability-state.cjs` | Unified capability-state resolver (ADR-857 phase 4b/6) — composes install profile, runtime surface, and config activation into one per-capability view consumed by workflow hook rendering; exports pure `resolveCapabilityState`, reusable `resolveCapabilityRuntimeState`, and I/O handler `cmdCapabilityState`; command surface: `gsd-tools capability state [--config-dir ]` emitting `{ runtimeConfigDir, capabilities[] }` | diff --git a/docs/README.md b/docs/README.md index f05fce2cd..c78509ffa 100644 --- a/docs/README.md +++ b/docs/README.md @@ -72,7 +72,7 @@ Language versions: [English](README.md) · [Português (pt-BR)](pt-BR/README.md) - [Multi-agent orchestration](explanation/multi-agent-orchestration.md) — how subagents are spawned, scoped, and coordinated - [Security model](explanation/security-model.md) — trust boundaries, permissions, and safe automation - [The capability trust model](explanation/capability-trust-model.md) — why third-party capabilities are gated by consent + integrity + reversibility, not a sandbox -- [How overlay capabilities compose](explanation/capability-overlay-model.md) — why first-party always wins and how the loader resolves precedence, conflicts, and fail-closed gates +- [How overlay capabilities compose](explanation/capability-overlay-model.md) — why first-party always wins and how the loader resolves precedence, conflicts, and fail-open load-failure warnings - [Architecture](ARCHITECTURE.md) — system architecture, agent model, and data flow - [Discuss modes](workflow-discuss-mode.md) — assumptions mode vs interview mode for `/gsd-discuss-phase` - [Context monitoring](context-monitor.md) — context window monitoring hook architecture diff --git a/docs/explanation/capability-overlay-model.md b/docs/explanation/capability-overlay-model.md index a39326644..b1d95f956 100644 --- a/docs/explanation/capability-overlay-model.md +++ b/docs/explanation/capability-overlay-model.md @@ -181,9 +181,10 @@ candidate is processed. A single broken overlay cannot poison the rest of the se --- -## The one place where skipping is dangerous: gates +## The one place where a skip must be loud: gates -Skipping a broken overlay is the safe default for most surfaces — but not for *gates*. +Skipping a broken overlay is the safe default for every surface — including gates, though +gates get special treatment. A capability's loop hooks come in three kinds: @@ -195,21 +196,28 @@ For steps and contributions, skipping a capability means the loop simply proceed **without** that addition. That is *fail-open*, and it is correct: the loop is missing an optional step, not doing something unsafe. -A gate is the opposite. The whole purpose of a gate is to *stop* the loop when a -condition is not met — a deploy gate, a house-style verification gate, a safety check. If -GSD skipped a broken gate-declaring capability and proceeded, it would behave exactly as -if the gate had *passed* — silently waving through the very thing the gate existed to -block. That is a fail-open on a security-relevant control, and it is unacceptable. +A gate looks different at first glance. The whole purpose of a gate is to *stop* the loop +when a condition is not met — a deploy gate, a house-style verification gate, a safety +check. Silently skipping a broken gate-declaring capability and proceeding as if the gate +had *passed* would wave through the very thing the gate existed to block, with no signal +to the operator at all. -So composition treats gates asymmetrically from steps and contributions. When a -capability that declares a gate is skipped, GSD records its gate points in -`_overlay.incompatibleGateCapIds` and `_overlay.blockedGates`, and the loop resolver -**injects a synthetic blocking gate** at each of those extension points. The loop -**fails closed**: rather than proceed as if the gate passed, it halts with a message -naming the skipped capability and why its gate could not be evaluated. +So, per the maintainer decision on [#2009](https://github.com/open-gsd/gsd-core/issues/2009), +composition treats gates like steps and contributions for control flow — the loop always +proceeds — but never silently. When a capability that declares a gate is skipped, GSD +records its gate points in `_overlay.incompatibleGateCapIds` and `_overlay.blockedGates`, +and the loop resolver **injects no gate** at each of those extension points. The loop +**fails open**, but loudly: it emits a warning through two channels — stderr (the channel +host workflows/agents see when they run `gsd_run loop render-hooks `) and the +`loop render-hooks` JSON envelope's top-level `warnings` array. The warning names the +skipped capability, why it could not be loaded (for example, an incompatible +`engines.gsd` range), and the exact remediation — `gsd capability remove ` — so the +operator sees the missing control on every pass through the loop until they act on it, +instead of the loop halting project-wide over a single incompatible overlay. The discriminator is therefore *not* "is this overlay broken?" but "what does failing -to load it mean?" — and for a gate, failing to load it means you must not proceed. +to load it mean?" — and for a gate, failing to load it means the operator must be told, +unmistakably, until they resolve it. --- @@ -230,10 +238,12 @@ details make this safe rather than merely convenient: behind a path that a runtime dispatcher might `require()` a command module from. - Every dropped overlay's **gates are recorded as blocked** — using the same extraction as the per-candidate path — so a gate-declaring overlay that vanishes in the fallback - still **fails closed**, never open. + still **surfaces a loud warning** (stderr + envelope `warnings`) at its gate points + rather than vanishing silently (#2009). The principle is the same at every layer: when GSD cannot compose an overlay, it removes -the overlay's *additions* but never weakens a *control*. +the overlay's *additions* but never silences a *control* — a missing gate always +surfaces, even though, per #2009, it no longer blocks the loop. --- @@ -262,16 +272,20 @@ The overlay model rests on a few rules applied consistently: - **First-party always wins** every collision — id, skill/agent stem, config key, command family, reserved prefix. An overlay can only add, never override. - A bad overlay is **skipped, not crashed** — the loop always gets a usable registry. -- Skipping **fails open** for steps and contributions (a missing optional addition) but - **fails closed** for gates (a missing control must block, not pass). +- Skipping **fails open** for steps and contributions (a missing optional addition) and, + per [#2009](https://github.com/open-gsd/gsd-core/issues/2009), **fails open** for gates + too (a missing control, no gate injected) — but loudly, via a warning (stderr + the + envelope's `warnings` array) that names the load failure and its + `gsd capability remove ` remediation. - A whole-set compose failure **falls back to first-party**, clearing command roots and - still blocking dropped gates. + still surfacing dropped gates as loud warnings. - One canonical builder materialises both first-party and overlay views, so an accepted overlay has true parity with a shipped capability. Every one of these choices answers the same question — *what does it mean if this -composition step fails?* — and resolves it in favour of first-party authority and a -fail-closed security posture. +composition step fails?* — and resolves it in favour of first-party authority and, +per #2009, a loud fail-open posture: never silent, never a project-wide halt over a +single incompatible overlay. --- diff --git a/src/capability-loader.cts b/src/capability-loader.cts index 673570770..2ba118ccc 100644 --- a/src/capability-loader.cts +++ b/src/capability-loader.cts @@ -17,10 +17,12 @@ * `gsd-core-` / `anthropic-` id prefix) is rejected. * - Load-time re-gate (default-resilient): an overlay that fails validation or * whose `engines.gsd` does not satisfy the running GSD version is SKIPPED - * with a warning — it never crashes the loop. EXCEPTION (per-hook-kind - * policy): a skipped capability that declares a `gate` is recorded in - * `_overlay.incompatibleGateCapIds` so the loop resolver can fail CLOSED for - * that gate rather than silently proceeding as if it had passed. + * with a warning — it never crashes the loop. A skipped capability that + * declares a `gate` is additionally recorded in + * `_overlay.incompatibleGateCapIds` / `_overlay.blockedGates` so the loop + * resolver can surface a loud fail-OPEN advisory for that gate (#2009): the + * un-evaluable gate is skipped (not enforced) with a remediation message, + * rather than silently vanishing. * * The merged registry is materialized by the canonical `buildRegistry` * (re-exported from the generator, which ships) over a cap-map reconstructed @@ -845,12 +847,12 @@ export function loadRegistry(options: LoadRegistryOptions = {}): Registry { // to load. Clear the map (the first-party base never lists overlay commandRoots — first-party // command modules ship in bin/lib/, not via _overlay.commandRoots). meta.commandRoots = {}; - // #1461 OVL-2 fail-CLOSED on compose failure (HIGH): the fallback DROPS every accepted overlay, - // so any accepted overlay that DECLARED a gate would have its gate silently vanish → a blocking - // gate FAILS OPEN, violating ADR-1244 (a skipped capability declaring a gate must FAIL CLOSED). + // #1461 OVL-2 (HIGH): on compose failure the fallback DROPS every accepted overlay, so any + // accepted overlay that DECLARED a gate would have its gate silently vanish with no trace. // Record each dropped gate-declaring overlay's gate as blocked using the SAME extraction the - // per-candidate `skip()` closure uses (gatePointsOf), so loop-resolver injects the synthetic - // blocking gate at each declared point exactly as it would for a per-candidate skip. + // per-candidate `skip()` closure uses (gatePointsOf), so loop-resolver surfaces the loud + // fail-OPEN advisory (#2009) at each declared point exactly as it would for a per-candidate + // skip — the gate does not silently disappear. for (const cap of overlayCaps) { const gatePoints = gatePointsOf(cap); if (gatePoints.length === 0) continue; diff --git a/src/loop-resolver.cts b/src/loop-resolver.cts index 1b0d3b1aa..ff86175a7 100644 --- a/src/loop-resolver.cts +++ b/src/loop-resolver.cts @@ -454,6 +454,27 @@ function renderLoopHooks(resolved: ResolveLoopHooksResult): string { * Missing value → coreError + non-zero exit. * Unknown/inactive capId → `false` (not an error). */ +// #2009: a capability id surfaced inside the runnable `gsd capability remove ` +// remediation must match the canonical kebab-case id shape (identical to +// capability-consent.cts / capability-ledger.cts) before it is embedded — a raw +// overlay directory name is attacker-controlled and can carry shell/markdown +// metacharacters (backticks, ';', '|', '$()'). An id that fails this check is +// withheld and no runnable command is rendered for it. +const LOAD_FAIL_CAP_ID_RE = /^[a-z][a-z0-9-]*$/; + +// #2009: neutralize control chars, newlines, and backticks from a third-party +// load-failure reason so a malicious manifest cannot break out of the warning +// line or inject markdown / prompt content into the surfaced message. +function sanitizeLoadFailReason(reason: unknown): string { + const cleaned = String(reason) + // Strip C0 control chars, DEL, and backticks; collapse remaining whitespace. + .replace(/[\x00-\x1F\x7F`]/g, ' ') + .replace(/\s+/g, " ") + .trim() + .slice(0, 300); + return cleaned || '(no reason given)'; +} + function cmdLoopRenderHooks( cwd: string, point: string, @@ -519,26 +540,55 @@ function cmdLoopRenderHooks( return; } - // ── ADR-1244 D2 fail-closed gate injection ──────────────────────────────────── - // For every skipped overlay capability that declared a gate at this point, - // inject a synthetic BLOCKING gate into the resolved output so the loop HALTS - // rather than silently proceeding as if the gate had passed. step/contribution - // overlays that were skipped are left open (skip-open is correct for them). + // ── ADR-1244 D2: load-failed capability gates FAIL OPEN with a loud warning ──── + // Decision (#2009): a capability that failed to LOAD must not block the loop. + // The prior behavior injected a BLOCKING synthetic gate (blocking:true, + // onError:'halt') at every point where the skipped cap declared a gate, so a + // single incompatible capability halted every ship:pre / verify:post + // project-wide for a load error unrelated to what the gate checked — with no + // remediation surfaced. We now fail OPEN: no gate is injected (the loop proceeds + // and `--active-cap ` correctly reports it inactive), and a loud + // warning is emitted instead — to STDERR (which the operator, or the agent + // running the command, actually sees regardless of how the host workflow + // consumes stdout) AND in the envelope's `warnings` channel for structured + // consumers. The warning names the load reason and the exact + // `gsd capability remove ` remediation so the operator can clear the broken + // capability. blockedGates is still recorded by the loader; only the consequence + // changes from block to warn. step/contribution overlays were already skip-open. + // + // The gate injection was dropped rather than made non-blocking because no host + // workflow generically surfaces an arbitrary gate's message at ship:pre / + // verify:post (consumers dispatch on specific capIds / ref.skills), and the + // generic gate consumers expect an object-shaped `check`, not a prose string — + // so an injected advisory gate would be silently dropped or mis-dispatched. A + // stderr warning is the channel that is actually surfaced. (See #2009 review.) const overlayMeta = (registry as { _overlay?: { blockedGates?: Array<{ point: string; capId: string; reason: string }> } })['_overlay']; + const loadFailWarnings: string[] = []; if (overlayMeta && Array.isArray(overlayMeta.blockedGates)) { for (const blocked of overlayMeta.blockedGates) { - if (blocked.point === point) { - const syntheticGate: ActiveHook = { - capId: blocked.capId, - kind: 'gate', - blocking: true, - onError: 'halt', - check: `capability "${blocked.capId}" was skipped at load (${blocked.reason}); its gate at ${point} cannot be evaluated — failing closed`, - }; - resolved.activeHooks.push(syntheticGate); - } + if (blocked.point !== point) continue; + // Security (#2009 review): capId/reason come from a third-party manifest or + // directory name. Validate capId before embedding it in the runnable + // remediation command; withhold it (no runnable command) if it is not a + // canonical id. Strip control chars/backticks from reason. + const idValid = LOAD_FAIL_CAP_ID_RE.test(String(blocked.capId)); + const capLabel = idValid + ? `"${blocked.capId}"` + : 'with an invalid id (withheld) under .gsd/capabilities/'; + const remediation = idValid + ? `Run \`gsd capability remove ${blocked.capId}\` to remove it, or fix the load error.` + : 'Remove the offending capability directory under .gsd/capabilities/, or fix the load error.'; + loadFailWarnings.push( + `capability ${capLabel} failed to load (${sanitizeLoadFailReason(blocked.reason)}); ` + + `its gate at ${point} is SKIPPED and NOT enforced (failing open). ${remediation}`, + ); } } + // Emit loudly to stderr in EVERY output mode (including --active-cap), so a + // skipped gate is never silently invisible to the operator/agent. + for (const w of loadFailWarnings) { + process.stderr.write(`gsd: warning — ${w}\n`); + } // --active-cap mode: print exactly 'true' or 'false' with no envelope if (activeCapId !== undefined) { @@ -558,8 +608,12 @@ function cmdLoopRenderHooks( activeHooks: resolved.activeHooks, rendered, }; - if (state.warnings && state.warnings.length > 0) { - envelope.warnings = state.warnings; + // Surface capability-state warnings and the #2009 load-failure fail-open + // warnings together in the structured `warnings` channel (in addition to the + // stderr emission above, which is the channel host workflows actually see). + const combinedWarnings = [...(state.warnings || []), ...loadFailWarnings]; + if (combinedWarnings.length > 0) { + envelope.warnings = combinedWarnings; } coreOutput(envelope, raw); diff --git a/tests/issue-2045-third-party-skills-surface.test.cjs b/tests/issue-2045-third-party-skills-surface.test.cjs index 09917e620..3e5ccd1cb 100644 --- a/tests/issue-2045-third-party-skills-surface.test.cjs +++ b/tests/issue-2045-third-party-skills-surface.test.cjs @@ -29,7 +29,7 @@ * AC5: first-party caps unaffected; writer still rejects truly-unknown ids. */ -const { describe, test, before, after } = require('node:test'); +const { describe, test, after } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); From f51ccceaf747b5ca211968f261e3560805906af4 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 7 Jul 2026 20:25:06 -0400 Subject: [PATCH 3/4] fix(test): make perf-316 lock-contention deterministic via release handshake The perf-316 state-lock test released the held lock on a fixed 1000ms timer, then asserted the SUT had contended (lockAttempts >= 2). On a slow/loaded runner the SUT worker's spawn+init exceeded the timer, so it acquired the lock on the first try (lockAttempts:1) and the test failed intermittently (observed on linux-node22 while linux-node24 passed). The holder now releases only when the writer signals its first FAILED lock attempt (via a shared SharedArrayBuffer + Atomics), so a retry is guaranteed regardless of worker-spawn latency; holdMs becomes a safety cap. Surfaced by the #2009 gsd-test run. --- .../perf-316-state-lock-buffer-alloc.test.cjs | 50 +++++++++++++------ 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/tests/perf-316-state-lock-buffer-alloc.test.cjs b/tests/perf-316-state-lock-buffer-alloc.test.cjs index 3924cc5ef..4e0e04328 100644 --- a/tests/perf-316-state-lock-buffer-alloc.test.cjs +++ b/tests/perf-316-state-lock-buffer-alloc.test.cjs @@ -57,9 +57,13 @@ const fs = require('fs'); // Write pid to lock file so acquireStateLock sees a live pid and retries. fs.writeFileSync(workerData.lockPath, String(process.pid)); parentPort.postMessage({ pid: process.pid }); -// Synchronous sleep — blocks this worker thread for holdMs ms. -const buf = new Int32Array(new SharedArrayBuffer(4)); -Atomics.wait(buf, 0, 0, workerData.holdMs); +// Hold the lock until the writer signals it has made its first FAILED lock +// attempt (deterministic contention), or until holdMs elapses as a safety cap. +// Atomics.wait blocks this thread while releaseFlag[0] === 0. A fixed timer here +// was racy: on a slow/loaded runner the writer's spawn+init could exceed the +// timer, so the lock released before the writer ever contended (lockAttempts:1). +const releaseFlag = new Int32Array(workerData.releaseSab); +Atomics.wait(releaseFlag, 0, 0, workerData.holdMs); // Release the lock. try { fs.unlinkSync(workerData.lockPath); } catch { /* already gone */ } parentPort.postMessage({ done: true }); @@ -88,17 +92,29 @@ global.SharedArrayBuffer = StubSAB; // path — without this witness, a no-retry success would yield sabCount === 1 // from BOTH pre-fix and post-fix code (the SAB is allocated unconditionally // post-fix, and exactly once for the single successful open pre-fix), giving -// a false-pass against the bug. The 1000ms holdMs + 200ms SUT retry delay -// guarantees >=4 attempts even on the slowest CI runners. +// a false-pass against the bug. Contention is deterministic here: this worker +// signals the holder to release only after its first failed attempt, so a +// retry always occurs (lockAttempts >= 2) regardless of runner speed. const realOpenSync = fs.openSync.bind(fs); +const releaseFlag = new Int32Array(workerData.releaseSab); let lockAttempts = 0; fs.openSync = function(filePath, flags, mode) { - if (typeof filePath === 'string' && filePath.endsWith('.lock') && + const isLockCreate = typeof filePath === 'string' && filePath.endsWith('.lock') && typeof flags === 'number' && - (flags & fs.constants.O_CREAT) && (flags & fs.constants.O_EXCL)) { - lockAttempts++; + (flags & fs.constants.O_CREAT) && (flags & fs.constants.O_EXCL); + if (isLockCreate) lockAttempts++; + try { + return realOpenSync(filePath, flags, mode); + } catch (e) { + // On the FIRST failed atomic-create (lock is held → contention proven), + // signal the holder worker to release so the next retry succeeds. Makes the + // retry path deterministic regardless of worker-spawn latency. + if (isLockCreate && lockAttempts === 1) { + Atomics.store(releaseFlag, 0, 1); + Atomics.notify(releaseFlag, 0); + } + throw e; } - return realOpenSync(filePath, flags, mode); }; // Delete cache entry to ensure a fresh require picks up the stubbed constructor. @@ -160,17 +176,18 @@ describe('perf #316: acquireStateLock hoists sleep buffer — exactly one SAB pe { timeout: 8000 }, async () => { // ── Worker A: hold the lock for 1000ms ───────────────────────────────── - // state.cjs retry delay = 200ms + 0-50ms jitter; 1000ms hold guarantees - // >=4 retries even on the slowest CI worker (~200ms spawn + 4 retry - // intervals ~1000ms ≈ hold duration). The lockAttempts assertion below - // proves the retry path was exercised end-to-end. + // The holder releases the lock only when the writer signals its first + // failed lock attempt (see WRITER_WORKER_CODE), so the writer is guaranteed + // to contend at least once regardless of worker-spawn latency. holdMs is now + // only a safety cap in case that signal never arrives. const holdMs = 1000; + const releaseSab = new SharedArrayBuffer(4); let resolveLockWritten; const lockWritten = new Promise((resolve) => { resolveLockWritten = resolve; }); const holderDone = new Promise((resolve, reject) => { holderWorker = new Worker(HOLDER_WORKER_CODE, { eval: true, - workerData: { lockPath, holdMs }, + workerData: { lockPath, holdMs, releaseSab }, }); holderWorker.on('message', (msg) => { if (msg.pid !== undefined) resolveLockWritten(); @@ -223,6 +240,7 @@ describe('perf #316: acquireStateLock hoists sleep buffer — exactly one SAB pe statePath, content: MINIMAL_STATE_MD, tmpDir, + releaseSab, }, }); writerWorker.on('message', resolve); @@ -259,8 +277,8 @@ describe('perf #316: acquireStateLock hoists sleep buffer — exactly one SAB pe assert.ok( writeResult.lockAttempts >= 2, 'SUT must have entered the retry path (>=1 failed lock attempt before success). ' + - 'Got lockAttempts: ' + writeResult.lockAttempts + '. The 1000ms holdMs + 200ms ' + - 'SUT retry delay guarantees >=2 attempts on any CI runner.' + 'Got lockAttempts: ' + writeResult.lockAttempts + '. The holder releases only ' + + 'after the writer signals its first failed attempt, so contention is deterministic.' ); // THE KEY INVARIANT: From 487ba4ddd13e25749e13f354ccef1107be4a046e Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 7 Jul 2026 22:09:26 -0400 Subject: [PATCH 4/4] docs(#2009): add changeset fragment (PR #2075) --- .changeset/2009-load-failed-capability-fail-open.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/2009-load-failed-capability-fail-open.md diff --git a/.changeset/2009-load-failed-capability-fail-open.md b/.changeset/2009-load-failed-capability-fail-open.md new file mode 100644 index 000000000..22ce740cf --- /dev/null +++ b/.changeset/2009-load-failed-capability-fail-open.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2075 +--- +**Load-failed capability gates now fail open with a loud warning instead of blocking the whole project** — when an installed overlay (third-party) capability failed to load (e.g. an incompatible `engines.gsd` range) but had declared a `gate`-kind loop hook, the loop resolver injected a blocking synthetic gate (`blocking:true`, `onError:halt`) at every point where that capability declared a gate. A single incompatible capability therefore halted every `ship:pre` and `verify:post` in the project — unrelated to what the gate would have checked, and with no remediation surfaced. The resolver now injects no gate and instead emits a loud warning — to stderr and in the `loop render-hooks` envelope's `warnings` array — naming the load-failure reason and the exact `gsd capability remove ` remediation, and the loop proceeds (fail open). The capability id embedded in that remediation is validated against the canonical id shape first, so a malformed overlay directory name cannot inject shell metacharacters into the surfaced command. The loader still records `_overlay.blockedGates`; only the consequence changes from block to warn. `step`/`contribution` overlays were already skip-open. (#2009)