* test(#3866): pin that verify:pre must dispatch every hook kind verify-work.md's verify_pre_hooks step dispatches only `kind == "gate"`, so getWiredKinds reports verify:pre -> {gate} and gen-capability-registry rejects any capability declaring a step or contribution there. The verify lane is therefore closed to capabilities that want to contribute to what UAT covers rather than refuse to let it start. Failing-first: the step, contribution, and exact-kind-set rows are RED; the pre-existing gate row is a green regression pin so the new arms cannot orphan the arm verify:pre already had. Refs #3866 * feat(#3866): dispatch step and contribution hooks at verify:pre verify_pre_hooks dispatched `kind == "gate"` only, so getWiredKinds reported verify:pre -> {gate} and gen-capability-registry's validateHooksWired rejected any capability declaring a step or contribution there. A capability could refuse to let UAT start; it could not contribute to what UAT covers. Add contribution and step arms mirroring execute:wave:post, deferring to references/loop-hook-dispatch.md and carrying its ref.command in-context validation guard ahead of any shell-use prose. A verify:pre step is advisory: it never blocks the start of UAT and an erroring step is routed by its own onError. The gate arm and its check guard are untouched. Give extract_tests an additive consumption seam for the artefacts those steps declare via the existing steps[].produces field -- no new registry field, no new ordering, no invented filename. Manifest-supplied artefact names are validated in-context against an allowlist and resolved only inside PHASE_DIR. With no producing step the derivation is unchanged, pinned by test rather than asserted in prose. Review findings folded in: the artefact-name allowlist (isolated adversarial pass), the artefact-shape contract and the seam-inertness tests (spec axis), and the reference/how-to split so one constraint has one source of truth (standards axis). Closes #3866 * chore(#3866): backfill changeset PR number --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/steady-foxes-rest.md
Normal file
5
.changeset/steady-foxes-rest.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Changed
|
||||
pr: 3869
|
||||
---
|
||||
**Capabilities can now do work before UAT, not only block it** — the `verify:pre` extension point dispatched gate hooks only, so a capability declaring a step or contribution there was rejected at registry-build time and the whole verify lane was closed to anything that wanted to contribute to what UAT covers. It now dispatches contribution, step, and gate hooks, and `extract_tests` additively consumes the artefacts those steps declare via `produces`. (#3866)
|
||||
@@ -119,6 +119,33 @@ Choose the hook kind that matches the behaviour:
|
||||
|
||||
Declare file artefact flow with `produces` and `consumes`. The registry uses those arrays to order hooks and to reject unsatisfied dependencies.
|
||||
|
||||
### Check the point dispatches your kind
|
||||
|
||||
A point having a call site does not mean it dispatches every kind. Each host workflow decides, per
|
||||
point, which kinds its dispatch text handles — the wire-on-demand model — and the registry build
|
||||
enforces the result: `gen-capability-registry.cjs` scans the host workflows with `getWiredKinds()`
|
||||
and rejects a manifest that declares a kind the point does not dispatch. You find out at
|
||||
`npm run gen:capability-registry`, not at runtime:
|
||||
|
||||
```
|
||||
capability "demo" steps[0].point "ship:pre" registers a step hook, but the host call site's
|
||||
dispatch text never covers `kind == "step"` (it covers: gate). A hand-rolled single-kind consumer
|
||||
silently never dispatches the other kinds — dispatch every registered kind per
|
||||
gsd-core/references/loop-hook-dispatch.md.
|
||||
```
|
||||
|
||||
That error is the answer, not a bug to route around. Either pick a point that dispatches your kind,
|
||||
express the behaviour as a kind the point does dispatch, or open an issue to wire the arm — that
|
||||
last route is what [#3866](https://github.com/open-gsd/gsd-core/issues/3866) did for `verify:pre`,
|
||||
which dispatched only `gate` and so let a capability refuse to let UAT start but never contribute
|
||||
to what UAT covers.
|
||||
|
||||
For which kinds a given point dispatches, and the `verify:pre` step / `produces` semantics, see
|
||||
[Valid `point` values](../reference/capability-manifest.md#valid-point-values) — that table is the
|
||||
single source of truth. One operational note it does not carry: a `verify:pre` `produces` name
|
||||
whose artefact is absent at UAT time is reported and skipped, so a step that was inactive, skipped,
|
||||
or failed never drops a deliverable and never blocks UAT.
|
||||
|
||||
## Ship skills — and know what you are shipping
|
||||
|
||||
A skill your Capability declares is not an inert asset. Its `SKILL.md` body is copied **verbatim** into the user's runtime skills directory at install, where it becomes an agent-invocable instruction file. GSD does **not** scan it: there is no content inspection of any kind, at install or at any later point. What you write is what reaches the agent.
|
||||
|
||||
@@ -123,6 +123,10 @@ Gates check a condition at a loop extension point and optionally block progressi
|
||||
|
||||
The 12 loop extension points are a **closed, additive-only vocabulary**. Every `steps`, `contributions`, and `gates` entry must use one of these identifiers exactly.
|
||||
|
||||
A valid point is necessary but not sufficient: each host workflow decides, per point, which hook **kinds** its dispatch text handles, and a point may dispatch a subset. The registry build derives the real answer from the host workflows (`getWiredKinds()` in `scripts/gen-loop-host-contract.cjs`) and rejects a manifest declaring a kind the point does not dispatch — so an unsupported combination is a build-time error naming the point, the kind, and the kinds that point does cover. It is never a hook that renders and is then silently dropped.
|
||||
|
||||
`verify:pre` dispatches all three kinds. A step there is **advisory**: it runs before UAT begins and never blocks it — a precondition that must halt verification is a `gate`. Its `produces` artefact names are consumed **additively** by the verify workflow's `extract_tests` step, which can deepen what UAT covers but cannot suppress a checkpoint. See [Develop a capability](../how-to/develop-a-capability.md#check-the-point-dispatches-your-kind) for the authoring workflow.
|
||||
|
||||
| Point | Phase | Position |
|
||||
|---|---|---|
|
||||
| `discuss:pre` | Discuss | Before the discuss step executes |
|
||||
|
||||
@@ -61,15 +61,26 @@ MVP_MODE=$(gsd_run query phase.mvp-mode "${phase_number}" ${GSD_WS} --pick activ
|
||||
</step>
|
||||
|
||||
<step name="verify_pre_hooks">
|
||||
**Verify:pre gate dispatch.** Before verification begins, dispatch every active
|
||||
gate hook registered at the `verify:pre` loop extension point. Each gate is
|
||||
data-driven — resolved from the capability registry, not hardcoded here.
|
||||
**Verify:pre capability dispatch.** Before verification begins, dispatch every
|
||||
active hook registered at the `verify:pre` loop extension point — of **every**
|
||||
kind, not gates alone. Each hook is data-driven — resolved from the capability
|
||||
registry, not hardcoded here.
|
||||
|
||||
```bash
|
||||
VERIFY_PRE_HOOKS_JSON=$(gsd_run loop render-hooks verify:pre --raw)
|
||||
PHASE_DIR=$(printf '%s' "$INIT" | jq -r '.phase_dir // empty')
|
||||
```
|
||||
|
||||
Read the `activeHooks` array from `VERIFY_PRE_HOOKS_JSON` in-context (do NOT pipe through a shell parser).
|
||||
|
||||
**If `activeHooks` is empty or absent:** skip silently to `check_active_session`.
|
||||
|
||||
**Contribution dispatch:** inject every `kind == "contribution"` fragment per @gsd-core/references/loop-hook-dispatch.md (skip when none), before the steps and gates below.
|
||||
|
||||
**Step dispatch:** dispatch every `kind == "step"` hook per @gsd-core/references/loop-hook-dispatch.md (skip when none) — not one shape of one. A step here is advisory: it never blocks the start of UAT, and a step that errors is routed by its own `onError` without failing verification. ⚠ **Validate `ref.command` in-context before any shell use** (third-party manifest input) — loop-hook-dispatch.md § `step`.
|
||||
|
||||
Record the union of `produces` artefact names declared by the active step entries as `VERIFY_PRE_PRODUCED` — `extract_tests` consumes it below. An entry declaring `produces: []` contributes nothing, which is the normal case.
|
||||
|
||||
⚠ **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"`.
|
||||
@@ -187,6 +198,35 @@ Read the JSON result (`mode`, `total`, `all_auto_covered`, `auto_passed[]`, `pre
|
||||
|
||||
The cold-start smoke test injection below still applies in `coverage` mode.
|
||||
|
||||
**Verify:pre produced-artefact seam (#3866).** If `VERIFY_PRE_PRODUCED` (recorded in
|
||||
`verify_pre_hooks`) is empty or absent, skip this paragraph entirely — derivation is unchanged.
|
||||
Otherwise, for each artefact name in it, locate the artefact the producing step wrote under
|
||||
`$PHASE_DIR` and merge its checkpoints into the test list **additively**.
|
||||
|
||||
**The artefact contract.** A consumable artefact is a Markdown file holding a list of checkpoint
|
||||
entries in the **same shape `extract_tests` already emits and `create_uat_file` already consumes** —
|
||||
each entry a `name` (brief test name) and an `expected` (specific, user-observable outcome).
|
||||
Nothing else is read: extra fields are ignored, not an error. There is no new schema and no new
|
||||
parser — a producing step writes what a checkpoint already looks like. An artefact that yields zero
|
||||
parseable entries is treated exactly like an absent one (see below).
|
||||
|
||||
Merge rules:
|
||||
|
||||
- ⚠ **Validate the artefact name in-context before resolving it** (third-party manifest input).
|
||||
An artefact name is a registry-declared name, **not** a path: check the value you read from
|
||||
`produces` against `^[A-Za-z0-9][A-Za-z0-9._-]*$` yourself — **never** by pasting it into a
|
||||
shell command to be tested there. A name carrying `/`, `..`, a leading `-`, a leading path
|
||||
separator, or any shell metacharacter is a malformed manifest: record a warning, skip that
|
||||
name, continue. Only a validated name is resolved, and only against what the step wrote inside
|
||||
`$PHASE_DIR` — never above it, and never through a symlink that leaves it.
|
||||
- A name with no artefact on disk means that step was inactive, skipped, or failed. Note it to the
|
||||
user and derive normally — **never drop a deliverable and never block UAT over it.**
|
||||
- Merged entries are added to, never subtracted from, what `coverage:` classification and the
|
||||
prose fallback produce. An `auto_passed[]` entry stays un-presented; a `present[]` entry stays a
|
||||
human checkpoint. This seam can deepen UAT, not suppress it.
|
||||
- Deduplicate against already-derived checkpoints by `name`, keeping the earlier entry's
|
||||
`expected` text so a produced artefact cannot silently rewrite a criterion.
|
||||
|
||||
**Extract testable deliverables from SUMMARY.md (legacy fallback — used when `mode: legacy`):**
|
||||
|
||||
Parse for:
|
||||
|
||||
@@ -4997,6 +4997,59 @@ describe('#1196 — discuss loop wiring + wired-point guard', () => {
|
||||
);
|
||||
});
|
||||
|
||||
// ─── #3866: the verify lane must be open to every hook kind ────────────────
|
||||
//
|
||||
// verify-work.md's verify_pre_hooks step historically dispatched only
|
||||
// `kind == "gate"`, so getWiredKinds reported verify:pre → {gate} and any
|
||||
// capability wanting to DO something before UAT (rather than block it) was
|
||||
// rejected at registry-build time. The rows below are the machine-observable
|
||||
// contract: what a capability may declare at verify:pre.
|
||||
|
||||
test('boundary: cap declaring a verify:pre step is accepted against real getWiredKinds(ROOT) (#3866)', () => {
|
||||
const cap = makeCapWithStep('verify:pre');
|
||||
const { getWiredKinds } = require('../scripts/gen-loop-host-contract.cjs');
|
||||
const errs = validateHooksWired(cap, getWiredKinds(ROOT));
|
||||
assert.deepEqual(
|
||||
errs, [],
|
||||
'verify:pre must dispatch step hooks — a capability can contribute to what UAT covers, ' +
|
||||
`not only refuse to let it start. Errors: ${errs.join('; ')}`,
|
||||
);
|
||||
});
|
||||
|
||||
test('boundary: cap declaring a verify:pre contribution is accepted against real getWiredKinds(ROOT) (#3866)', () => {
|
||||
const cap = makeCapWithContribution('verify:pre');
|
||||
const { getWiredKinds } = require('../scripts/gen-loop-host-contract.cjs');
|
||||
const errs = validateHooksWired(cap, getWiredKinds(ROOT));
|
||||
assert.deepEqual(
|
||||
errs, [],
|
||||
`verify:pre must dispatch contribution hooks. Errors: ${errs.join('; ')}`,
|
||||
);
|
||||
});
|
||||
|
||||
test('regression: cap declaring a verify:pre gate stays accepted after the step/contribution arms land (#3866)', () => {
|
||||
// The pre-existing gate arm is the one behavior verify:pre already had.
|
||||
// Adding arms above it must not orphan or narrow it.
|
||||
const cap = makeCapWithGate('verify:pre');
|
||||
const { getWiredKinds } = require('../scripts/gen-loop-host-contract.cjs');
|
||||
const errs = validateHooksWired(cap, getWiredKinds(ROOT));
|
||||
assert.deepEqual(
|
||||
errs, [],
|
||||
`the verify:pre gate arm must survive the new kind arms. Errors: ${errs.join('; ')}`,
|
||||
);
|
||||
});
|
||||
|
||||
test('verify:pre dispatch text covers exactly the three hook kinds, no more (#3866)', () => {
|
||||
// Asserts the VALUE, not `.has(...)`: a scanner that over-credits (or a
|
||||
// future fourth token creeping into HOOK_KINDS) fails here too.
|
||||
const { getWiredKinds, HOOK_KINDS } = require('../scripts/gen-loop-host-contract.cjs');
|
||||
const covered = getWiredKinds(ROOT).get('verify:pre');
|
||||
assert.ok(covered, 'verify:pre must have a render-hooks call site in the host loop');
|
||||
assert.deepEqual(
|
||||
[...covered].sort(), [...HOOK_KINDS].sort(),
|
||||
`verify:pre must cover every hook kind; got ${JSON.stringify([...covered].sort())}`,
|
||||
);
|
||||
});
|
||||
|
||||
test('invalid points (not in VALID_LOOP_POINTS) are not flagged as "unwired" (already caught by schema validator)', () => {
|
||||
const cap = {
|
||||
id: 'test-cap',
|
||||
|
||||
@@ -0,0 +1,9 @@
|
||||
{
|
||||
"$comment": "Growth ack (#2914 fragment). Reason: #3866 opens the verify lane to capabilities. verify-work.md's verify_pre_hooks step dispatched `kind == \"gate\"` only, so getWiredKinds reported verify:pre -> {gate} and gen-capability-registry's validateHooksWired rejected any capability declaring a step or contribution there — a capability could refuse to let UAT start but never contribute to what UAT covers. The growth is the two new dispatch arms (contribution + step, deferring to gsd-core/references/loop-hook-dispatch.md and carrying its ref.command in-context validation guard) plus the additive extract_tests consumption seam for the produces[] artefact names those steps declare. Prose is the product here: the arms ARE the dispatch contract an executing agent reads, so there is no smaller form. Includes the in-context allowlist for manifest-supplied produces names (isolated security review) and the artefact-shape contract (spec review), both of which a capability author must be able to read at the point of use. verify-work.md 35973 -> 39212 LF bytes (+3239).",
|
||||
"version": 1,
|
||||
"paths": {
|
||||
"verify-work.md": {
|
||||
"reason": "#3866: verify:pre gains contribution + step dispatch arms and extract_tests gains the produces[] consumption seam with its in-context name allowlist and artefact-shape contract; the dispatch contract is executable prose, so the arms cannot be expressed shorter; +3239 bytes"
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -297,6 +297,81 @@ describe('ADR-857 phase 6 — capabilities must not bake install paths into the
|
||||
}
|
||||
});
|
||||
|
||||
// ─── #3866: the verify:pre produced-artefact seam must be strictly additive ──
|
||||
//
|
||||
// The lane opened at verify:pre lets a capability step produce an artefact that
|
||||
// extract_tests consumes. The contract that makes that safe is that the seam is
|
||||
// INERT when nothing is produced: derivation must be unchanged for every project
|
||||
// that has no such capability — which is every project on `next` today. These
|
||||
// assert the workflow prose an executing agent actually reads (verify-work.md is
|
||||
// Markdown, not a source path, so local/no-source-grep does not apply).
|
||||
|
||||
test('the verify:pre produced-artefact seam is conditional, and the pre-existing derivation paths are not nested inside it (#3866)', () => {
|
||||
const wf = readRepoFile('gsd-core/workflows/verify-work.md');
|
||||
|
||||
const seamIdx = wf.indexOf('Verify:pre produced-artefact seam');
|
||||
assert.ok(seamIdx > 0, 'verify-work.md must carry the verify:pre produced-artefact seam');
|
||||
|
||||
// The seam must open with its own skip-when-absent guard, so an agent reading
|
||||
// it top-down never falls into the merge on a project with no producing step.
|
||||
const seamHead = wf.slice(seamIdx, seamIdx + 400);
|
||||
assert.match(
|
||||
seamHead, /VERIFY_PRE_PRODUCED/,
|
||||
'the seam must name the variable it is conditional on',
|
||||
);
|
||||
assert.match(
|
||||
seamHead, /empty or absent/,
|
||||
'the seam must state the empty/absent case before describing any merge',
|
||||
);
|
||||
|
||||
// Both pre-existing derivation paths must still exist, and must sit OUTSIDE the
|
||||
// seam: the coverage classifier before it, the legacy prose fallback after it.
|
||||
// If either migrated inside the seam it would become conditional on a producing
|
||||
// step existing — the exact regression "byte-identical when no artefact exists"
|
||||
// rules out.
|
||||
const coverageIdx = wf.indexOf('uat.classify-coverage');
|
||||
const legacyIdx = wf.indexOf('Extract testable deliverables from SUMMARY.md');
|
||||
assert.ok(coverageIdx > 0, 'the #1602 coverage classifier must still be invoked');
|
||||
assert.ok(legacyIdx > 0, 'the legacy prose-extraction fallback must still exist');
|
||||
assert.ok(
|
||||
coverageIdx < seamIdx,
|
||||
'coverage classification must run before the seam, not inside it',
|
||||
);
|
||||
assert.ok(
|
||||
legacyIdx > seamIdx,
|
||||
'the legacy fallback must follow the seam and stay unguarded by it',
|
||||
);
|
||||
});
|
||||
|
||||
test('the verify:pre produced-artefact seam validates manifest-supplied artefact names in-context (#3866)', () => {
|
||||
const wf = readRepoFile('gsd-core/workflows/verify-work.md');
|
||||
const seamIdx = wf.indexOf('Verify:pre produced-artefact seam');
|
||||
assert.ok(seamIdx > 0, 'verify-work.md must carry the verify:pre produced-artefact seam');
|
||||
const seam = wf.slice(seamIdx, legacyEnd(wf, seamIdx));
|
||||
|
||||
// `produces` names come from a third-party capability manifest. The seam must
|
||||
// carry an explicit in-context allowlist, the same shape loop-hook-dispatch.md
|
||||
// requires of `ref.command` — not a vague "it is a name, not a path".
|
||||
assert.match(
|
||||
seam, /\^\[A-Za-z0-9\]\[A-Za-z0-9\._-\]\*\$/,
|
||||
'the seam must pin an explicit allowlist regex for artefact names',
|
||||
);
|
||||
assert.match(
|
||||
seam, /never\*{0,2}\s*by pasting it into a\s*\n?\s*shell command|never\*{0,2} by pasting it into a shell/,
|
||||
'the seam must forbid shell-side validation of the manifest value',
|
||||
);
|
||||
assert.match(
|
||||
seam, /\$PHASE_DIR/,
|
||||
'the seam must confine resolution to the phase directory',
|
||||
);
|
||||
});
|
||||
|
||||
/** End of the seam region: the next top-level bold heading after it. */
|
||||
function legacyEnd(wf, seamIdx) {
|
||||
const next = wf.indexOf('**Extract testable deliverables', seamIdx);
|
||||
return next > seamIdx ? next : Math.min(wf.length, seamIdx + 3000);
|
||||
}
|
||||
|
||||
test('every declared gate check.query returns a uniform boolean `block` field', () => {
|
||||
// FIX A regression guard: every gate check command must return a top-level
|
||||
// boolean `block` field so the host-loop dispatch can read a single consistent
|
||||
|
||||
Reference in New Issue
Block a user