diff --git a/.changeset/noble-otters-wander.md b/.changeset/noble-otters-wander.md new file mode 100644 index 000000000..bb5517634 --- /dev/null +++ b/.changeset/noble-otters-wander.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 4159 +--- +**`workflow.code_review_point` config to run code review per-wave instead of once per phase** — set it to `execute:wave:post` and the automatic code-review step registers at each completed wave instead of at the end of the phase, scoped to what changed since the phase's prior review. Defaults to `execute:post` (today's behavior, unchanged). diff --git a/CONTEXT.md b/CONTEXT.md index 6098d1239..8e9f186af 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -415,7 +415,7 @@ A Capability whose integration shape brings its own external process, service, o `RULESET.CAPABILITY.step-additive-gate-blocks=a `step` hook is purely additive (invoke skill + produce artifacts, NEVER halts the host); host-blocking preconditions are `gate`s (blocking:true, onError:halt); runtime/mode context (auto/chain vs manual) self-gates IN THE SKILL, not via `when` (config-only). §5.6 = plan:pre step (ui-phase; skill self-gates on frontend+pipeline, auto-fires only in pipelines) + a NEW plan:pre gate (frontend-and-no-UI-SPEC → halt, when:workflow.ui_safety_gate); the loop.render-hooks dispatch template handles steps AND gates. Resolves #1022.` -`RULESET.CAPABILITY.precedence-engine-single-owner=the config-key four-level precedence walk (loadConfig result → workstream config.json → root config.json → registry.configSchema default → absent) is owned solely by src/capability-activation.cts: raw-value primitive resolveConfigKey(dotKey, {config,cwd,registry}) and boolean wrapper _resolveActivationValue(dotKey,config,cwd,registry); loop-resolver.cts imports the engine (no duplicate); resolveConfigValues in loop-resolver.cts delegates to resolveConfigKey; resolveCapabilityRuntimeState does NOT return registry/config — callers import capability-registry.cjs and call loadConfig(cwd) directly.` +`RULESET.CAPABILITY.precedence-engine-single-owner=the config-key four-level precedence walk (loadConfig result → workstream config.json → root config.json → registry.configSchema default → absent) is owned solely by src/capability-activation.cts: raw-value primitive resolveConfigKey(dotKey, {config,cwd,registry}) and boolean wrapper _resolveActivationValue(dotKey,config,cwd,registry); loop-resolver.cts imports the engine (no duplicate); resolveConfigValues in loop-resolver.cts delegates to resolveConfigKey; resolveCapabilityRuntimeState does NOT return registry/config — callers import capability-registry.cjs and call loadConfig(cwd) directly. #3661 adds a THIRD export from the same engine, _resolvePointGate(pointFrom,point,config,cwd,registry): a step's optional pointFrom field names a dotted enum config key; the step is active for its own `point` only when that key resolves to a value === point (found:false or a type/value mismatch → false). loop-resolver.cts's isActive and capability-state.cts's processHooks both call it (ANDed with the existing when gate) so a capability can register the same logical step at more than one loop point with config selecting which registration is live — see capabilities/code-review/capability.json's execute:post/execute:wave:post pair for the reference shape. capability-validator.cjs's validateAgainstContract requires pointFrom to reference an enum cap.config key whose values include the declaring step's own point (mirrors the pre-existing when-must-be-a-cap.config-key check for `when`).` `SEAM.capability-activation-precedence-owner.owns=the config-key four-level precedence walk (loadConfig result → workstream config.json → root config.json → registry.configSchema default → absent), owned solely by src/capability-activation.cts` `SEAM.capability-activation-precedence-owner.enforced-by=test:tests/capability-precedence-parity.test.cjs` diff --git a/capabilities/code-review/capability.json b/capabilities/code-review/capability.json index 194286f8f..1857a8a07 100644 --- a/capabilities/code-review/capability.json +++ b/capabilities/code-review/capability.json @@ -38,6 +38,15 @@ ], "default": "standard", "description": "Default depth for code review when no --depth override is supplied." + }, + "workflow.code_review_point": { + "type": "enum", + "values": [ + "execute:post", + "execute:wave:post" + ], + "default": "execute:post", + "description": "Loop point at which the code-review step registers — execute:post reviews once per phase (default); execute:wave:post reviews once per completed wave, scoped to that wave's diff." } }, "steps": [ @@ -53,6 +62,20 @@ "SUMMARY.md" ], "when": "workflow.code_review", + "pointFrom": "workflow.code_review_point", + "onError": "skip" + }, + { + "point": "execute:wave:post", + "ref": { + "skill": "code-review" + }, + "produces": [ + "REVIEW.md" + ], + "consumes": [], + "when": "workflow.code_review", + "pointFrom": "workflow.code_review_point", "onError": "skip" } ], diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index bba87936d..00d348b26 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -46,6 +46,7 @@ GSD stores project settings in `.planning/config.json`. Created during `/gsd-new "text_mode": false, "use_worktrees": true, "code_review": true, + "code_review_point": "execute:post", "code_review_depth": "standard", "code_review_depth_overrides": [], "plan_bounce": false, @@ -428,6 +429,7 @@ All workflow toggles follow the **absent = enabled** pattern. If a key is missin | `workflow.agent_hint_routing` | boolean | `true` | Per-plan specialist executor routing (#1689). When `true`, a plan whose `agent_hint:` frontmatter names a subagent that resolves on the active runtime is dispatched to that specialist instead of `gsd-executor`. Default `true` — a no-op for plans without `agent_hint:`, so existing dispatch is unchanged. Set `false` to disable. See [PLAN.md `agent_hint`](reference/plan-md.md#per-plan-executor-routing). | | `workflow.worktree_skip_hooks` | boolean | `false` | When `true`, executor agents in worktree mode pass `--no-verify` (skipping pre-commit hooks) and post-wave hook validation runs against the merged result instead. Opt-in escape hatch for projects whose hooks cannot run in agent worktrees. Default `false` runs hooks on every commit (#2924). | | `workflow.code_review` | boolean | `true` | Enable `/gsd-code-review` and `/gsd-code-review --fix` commands. When `false`, the commands exit with a configuration gate message. Added in v1.34 | +| `workflow.code_review_point` | string | `execute:post` | Loop point at which the code-review capability's step registers: `execute:post` reviews once, after every wave in a phase has landed (default — unchanged behavior); `execute:wave:post` reviews once per completed wave instead, scoped to what changed since the phase's prior review (the whole phase's diff on the first wave, each subsequent wave's own diff thereafter). Manual `/gsd-code-review ` invocation is unaffected by this key — it is gated by `workflow.code_review` alone and runs regardless of which point is configured. `/gsd-autonomous` and `/gsd-quick` have no wave granularity of their own, so setting this to `execute:wave:post` means code review does not run automatically inside those two flows (consistent with how every other `execute:wave:post`-only capability already behaves for them). Added in #3661 | | `workflow.code_review_depth` | string | `standard` | Default review depth for `/gsd-code-review`: `quick` (pattern-matching only), `standard` (per-file analysis), or `deep` (cross-file with import graphs). Can be overridden per-run with `--depth=`. Added in v1.34 | | `workflow.code_review_depth_overrides` | array | `[]` | Ordered list of `{ paths: string[], depth }` rules that escalate `/gsd-code-review` depth for specific directories, e.g. `[{ "paths": ["src/auth"], "depth": "deep" }]`. Each rule's `paths` are matched against the review's changed-file set by whole-segment directory-path prefix (`src/auth` matches `src/auth/token.ts`, never `src/authfoo/x.ts` or `docs/src/auth/x.ts`); matching is case-sensitive, following git. Glob syntax (`*`, `?`) is a configuration error, not sugar for a prefix. One matched file escalates the entire review — depth is not applied per file. Resolution order: `--depth=` flag → strongest matching rule → `workflow.code_review_depth` → `standard`; a matching rule wins even when its tier is weaker than the global default. A malformed rule (bad `depth`, glob syntax, absolute path, `..` segment, empty path, non-array `overrides`, non-object rule, malformed `paths`) is a configuration error and the review halts rather than falling back silently. The resolved depth and the matching rule are printed in the review output. Added in #2554 | | `workflow.plan_bounce` | boolean | `false` | Run external validation script against generated plans. When enabled, the plan-phase orchestrator pipes each PLAN.md through the script specified by `plan_bounce_script` and blocks on non-zero exit. Added in v1.36 | diff --git a/docs/CONTEXT-INDEX.json b/docs/CONTEXT-INDEX.json index d0eb0cf0f..deb0ccba6 100644 --- a/docs/CONTEXT-INDEX.json +++ b/docs/CONTEXT-INDEX.json @@ -964,7 +964,7 @@ { "id": "RULESET.CAPABILITY.precedence-engine-single-owner", "klass": "RULESET", - "value": "the config-key four-level precedence walk (loadConfig result → workstream config.json → root config.json → registry.configSchema default → absent) is owned solely by src/capability-activation.cts: raw-value primitive resolveConfigKey(dotKey, {config,cwd,registry}) and boolean wrapper _resolveActivationValue(dotKey,config,cwd,registry); loop-resolver.cts imports the engine (no duplicate); resolveConfigValues in loop-resolver.cts delegates to resolveConfigKey; resolveCapabilityRuntimeState does NOT return registry/config — callers import capability-registry.cjs and call loadConfig(cwd) directly." + "value": "the config-key four-level precedence walk (loadConfig result → workstream config.json → root config.json → registry.configSchema default → absent) is owned solely by src/capability-activation.cts: raw-value primitive resolveConfigKey(dotKey, {config,cwd,registry}) and boolean wrapper _resolveActivationValue(dotKey,config,cwd,registry); loop-resolver.cts imports the engine (no duplicate); resolveConfigValues in loop-resolver.cts delegates to resolveConfigKey; resolveCapabilityRuntimeState does NOT return registry/config — callers import capability-registry.cjs and call loadConfig(cwd) directly. #3661 adds a THIRD export from the same engine, _resolvePointGate(pointFrom,point,config,cwd,registry): a step's optional pointFrom field names a dotted enum config key; the step is active for its own `point` only when that key resolves to a value === point (found:false or a type/value mismatch → false). loop-resolver.cts's isActive and capability-state.cts's processHooks both call it (ANDed with the existing when gate) so a capability can register the same logical step at more than one loop point with config selecting which registration is live — see capabilities/code-review/capability.json's execute:post/execute:wave:post pair for the reference shape. capability-validator.cjs's validateAgainstContract requires pointFrom to reference an enum cap.config key whose values include the declaring step's own point (mirrors the pre-existing when-must-be-a-cap.config-key check for `when`)." }, { "id": "RULESET.CAPABILITY.step-additive-gate-blocks", diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 41d818277..58ac51dd3 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -2247,14 +2247,31 @@ Test suite that scans all agent, workflow, and command files for embedded inject - REQ-REVIEW-05: Each fix MUST be committed atomically with a descriptive message - REQ-REVIEW-06: `--auto` flag MUST enable fix + re-review iteration loop, capped at 3 iterations - REQ-REVIEW-07: Feature MUST be gated by `workflow.code_review` config flag +- REQ-REVIEW-08: `workflow.code_review_point` MUST select which loop point the automatic review step registers at (`execute:post` default, or `execute:wave:post`), independent of the `workflow.code_review` on/off gate and of manual `/gsd-code-review` invocation (#3661) **Config:** | Setting | Type | Default | Description | |---------|------|---------|-------------| | `workflow.code_review` | boolean | `true` | Enable code review commands | +| `workflow.code_review_point` | string | `execute:post` | Loop point for the automatic review: `execute:post` (once per phase, default) or `execute:wave:post` (once per completed wave, scoped to what changed since the phase's prior review). See below. | | `workflow.code_review_depth` | string | `standard` | Default review depth: `quick`, `standard`, or `deep` | | `workflow.code_review_depth_overrides` | array | `[]` | Ordered `{ paths, depth }` rules that escalate depth for directories matched by path prefix against the changed-file set (#2554). See below. | +**Reviewing per wave instead of per phase (#3661)** + +Setting `workflow.code_review_point` to `execute:wave:post` moves the automatic review from +"once, after the whole phase's waves have all landed" to "once per completed wave." Each +wave's review scopes to what changed since the phase's *previous* review — the whole phase's +diff on the first wave, then just that wave's diff on every wave after — so review batches +stay small instead of growing with the phase. A finding introduced early is caught after the +wave that introduced it, not after the last wave of the phase. + +This only affects the *automatic* dispatch inside a wave-based phase execution. Manual +`/gsd-code-review ` runs are gated by `workflow.code_review` alone and are unaffected +by this key. `/gsd-autonomous` and `/gsd-quick` have no wave granularity of their own, so +setting this to `execute:wave:post` means automatic review does not run inside those two +flows — the same way every other wave-scoped capability step already behaves for them. + **Path-scoped code review depth overrides** `workflow.code_review_depth_overrides` matches rules against the review's changed-file set by whole-segment directory-path prefix — `src/auth` matches `src/auth/token.ts` and `src/auth` itself, never `src/authfoo/x.ts` or `docs/src/auth/x.ts` — and is case-sensitive, following git. diff --git a/docs/features/code-review-pipeline.md b/docs/features/code-review-pipeline.md index 1a22a9ea6..7ff845628 100644 --- a/docs/features/code-review-pipeline.md +++ b/docs/features/code-review-pipeline.md @@ -16,14 +16,31 @@ group: v1.34.0 Features - REQ-REVIEW-05: Each fix MUST be committed atomically with a descriptive message - REQ-REVIEW-06: `--auto` flag MUST enable fix + re-review iteration loop, capped at 3 iterations - REQ-REVIEW-07: Feature MUST be gated by `workflow.code_review` config flag +- REQ-REVIEW-08: `workflow.code_review_point` MUST select which loop point the automatic review step registers at (`execute:post` default, or `execute:wave:post`), independent of the `workflow.code_review` on/off gate and of manual `/gsd-code-review` invocation (#3661) **Config:** | Setting | Type | Default | Description | |---------|------|---------|-------------| | `workflow.code_review` | boolean | `true` | Enable code review commands | +| `workflow.code_review_point` | string | `execute:post` | Loop point for the automatic review: `execute:post` (once per phase, default) or `execute:wave:post` (once per completed wave, scoped to what changed since the phase's prior review). See below. | | `workflow.code_review_depth` | string | `standard` | Default review depth: `quick`, `standard`, or `deep` | | `workflow.code_review_depth_overrides` | array | `[]` | Ordered `{ paths, depth }` rules that escalate depth for directories matched by path prefix against the changed-file set (#2554). See below. | +**Reviewing per wave instead of per phase (#3661)** + +Setting `workflow.code_review_point` to `execute:wave:post` moves the automatic review from +"once, after the whole phase's waves have all landed" to "once per completed wave." Each +wave's review scopes to what changed since the phase's *previous* review — the whole phase's +diff on the first wave, then just that wave's diff on every wave after — so review batches +stay small instead of growing with the phase. A finding introduced early is caught after the +wave that introduced it, not after the last wave of the phase. + +This only affects the *automatic* dispatch inside a wave-based phase execution. Manual +`/gsd-code-review ` runs are gated by `workflow.code_review` alone and are unaffected +by this key. `/gsd-autonomous` and `/gsd-quick` have no wave granularity of their own, so +setting this to `execute:wave:post` means automatic review does not run inside those two +flows — the same way every other wave-scoped capability step already behaves for them. + **Path-scoped code review depth overrides** `workflow.code_review_depth_overrides` matches rules against the review's changed-file set by whole-segment directory-path prefix — `src/auth` matches `src/auth/token.ts` and `src/auth` itself, never `src/authfoo/x.ts` or `docs/src/auth/x.ts` — and is case-sensitive, following git. diff --git a/docs/reference/capability-matrix.md b/docs/reference/capability-matrix.md index c7047be05..561f9c52d 100644 --- a/docs/reference/capability-matrix.md +++ b/docs/reference/capability-matrix.md @@ -57,7 +57,7 @@ points. | `audit` | feature | full | `>=1.6.0` | — | — | first-party | | `broken-windows` | feature | full | `>=1.7.0` | `ship:pre` | gate | first-party | | `claude-orchestration` | feature | full | `>=1.7.0` | `plan:post`, `execute:wave:pre` | contribution | first-party | -| `code-review` | feature | full | `>=1.6.0` | `execute:post` | step | first-party | +| `code-review` | feature | full | `>=1.6.0` | `execute:wave:post`, `execute:post` | step | first-party | | `drift` | feature | full | `>=1.6.0` | `plan:pre`, `execute:wave:post` | gate | first-party | | `external-job` | feature | full | `>=1.7.0` | `plan:post`, `execute:wave:post` | contribution | first-party | | `gap-analysis` | feature | standard | `>=1.6.0` | `plan:post` | gate | first-party | diff --git a/examples/dynamic-context-management/CONTEXT-INDEX.json b/examples/dynamic-context-management/CONTEXT-INDEX.json index 69d0ae331..7e7841a4b 100644 --- a/examples/dynamic-context-management/CONTEXT-INDEX.json +++ b/examples/dynamic-context-management/CONTEXT-INDEX.json @@ -1151,7 +1151,7 @@ { "id": "RULESET.CAPABILITY.precedence-engine-single-owner", "klass": "RULESET", - "value": "the config-key four-level precedence walk (loadConfig result → workstream config.json → root config.json → registry.configSchema default → absent) is owned solely by src/capability-activation.cts: raw-value primitive resolveConfigKey(dotKey, {config,cwd,registry}) and boolean wrapper _resolveActivationValue(dotKey,config,cwd,registry); loop-resolver.cts imports the engine (no duplicate); resolveConfigValues in loop-resolver.cts delegates to resolveConfigKey; resolveCapabilityRuntimeState does NOT return registry/config — callers import capability-registry.cjs and call loadConfig(cwd) directly.", + "value": "the config-key four-level precedence walk (loadConfig result → workstream config.json → root config.json → registry.configSchema default → absent) is owned solely by src/capability-activation.cts: raw-value primitive resolveConfigKey(dotKey, {config,cwd,registry}) and boolean wrapper _resolveActivationValue(dotKey,config,cwd,registry); loop-resolver.cts imports the engine (no duplicate); resolveConfigValues in loop-resolver.cts delegates to resolveConfigKey; resolveCapabilityRuntimeState does NOT return registry/config — callers import capability-registry.cjs and call loadConfig(cwd) directly. #3661 adds a THIRD export from the same engine, _resolvePointGate(pointFrom,point,config,cwd,registry): a step's optional pointFrom field names a dotted enum config key; the step is active for its own `point` only when that key resolves to a value === point (found:false or a type/value mismatch → false). loop-resolver.cts's isActive and capability-state.cts's processHooks both call it (ANDed with the existing when gate) so a capability can register the same logical step at more than one loop point with config selecting which registration is live — see capabilities/code-review/capability.json's execute:post/execute:wave:post pair for the reference shape. capability-validator.cjs's validateAgainstContract requires pointFrom to reference an enum cap.config key whose values include the declaring step's own point (mirrors the pre-existing when-must-be-a-cap.config-key check for `when`).", "line": 415 }, { diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index 80481365a..ed5e372eb 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -891,6 +891,15 @@ const capabilities = { ], "default": "standard", "description": "Default depth for code review when no --depth override is supplied." + }, + "workflow.code_review_point": { + "type": "enum", + "values": [ + "execute:post", + "execute:wave:post" + ], + "default": "execute:post", + "description": "Loop point at which the code-review step registers — execute:post reviews once per phase (default); execute:wave:post reviews once per completed wave, scoped to that wave's diff." } }, "steps": [ @@ -906,6 +915,20 @@ const capabilities = { "SUMMARY.md" ], "when": "workflow.code_review", + "pointFrom": "workflow.code_review_point", + "onError": "skip" + }, + { + "point": "execute:wave:post", + "ref": { + "skill": "code-review" + }, + "produces": [ + "REVIEW.md" + ], + "consumes": [], + "when": "workflow.code_review", + "pointFrom": "workflow.code_review_point", "onError": "skip" } ], @@ -4552,6 +4575,20 @@ const byLoopPoint = { }, "execute:wave:post": { "steps": [ + { + "capId": "code-review", + "point": "execute:wave:post", + "ref": { + "skill": "code-review" + }, + "produces": [ + "REVIEW.md" + ], + "consumes": [], + "when": "workflow.code_review", + "pointFrom": "workflow.code_review_point", + "onError": "skip" + }, { "capId": "live-dom-uat", "point": "execute:wave:post", @@ -4652,6 +4689,7 @@ const byLoopPoint = { "SUMMARY.md" ], "when": "workflow.code_review", + "pointFrom": "workflow.code_review_point", "onError": "skip" }, { @@ -4835,6 +4873,7 @@ const configKeys = { "claude_orchestration.min_agent_sdk_version": "claude-orchestration", "workflow.code_review": "code-review", "workflow.code_review_depth": "code-review", + "workflow.code_review_point": "code-review", "review.max_prompt_tokens_per_reviewer.coderabbit": "coderabbit", "review.models.codex": "codex", "review.max_prompt_tokens_per_reviewer.codex": "codex", @@ -5007,6 +5046,16 @@ const configSchema = { "deep" ] }, + "workflow.code_review_point": { + "owner": "code-review", + "type": "enum", + "default": "execute:post", + "description": "Loop point at which the code-review step registers — execute:post reviews once per phase (default); execute:wave:post reviews once per completed wave, scoped to that wave's diff.", + "values": [ + "execute:post", + "execute:wave:post" + ] + }, "review.max_prompt_tokens_per_reviewer.coderabbit": { "owner": "coderabbit", "type": "number", diff --git a/gsd-core/bin/lib/capability-validator.cjs b/gsd-core/bin/lib/capability-validator.cjs index 3a549b60d..4a6e69666 100644 --- a/gsd-core/bin/lib/capability-validator.cjs +++ b/gsd-core/bin/lib/capability-validator.cjs @@ -2894,6 +2894,10 @@ function validateStep(step, prefix, declaredSkills, declaredAgents) { errors.push(prefix + '.when must be a string if present'); } + if (step.pointFrom !== undefined && typeof step.pointFrom !== 'string') { + errors.push(prefix + '.pointFrom must be a string if present'); + } + if (step.fragment !== undefined) { errors.push(...validateFragment(step.fragment, prefix + '.fragment')); } @@ -3033,6 +3037,31 @@ function validateAgainstContract(cap, capId) { } } + // pointFrom (#3661): selects which of possibly several same-capability steps is + // active for its own `point`, based on an enum config key. Require it references + // an enum key in cap.config whose values include THIS step's own point — otherwise + // the step could never activate (a silently-dead step). + for (const step of cap.steps) { + if (step.pointFrom !== undefined) { + if (typeof step.pointFrom !== 'string') continue; // already reported above + const slice = typeof cap.config === 'object' && cap.config !== null ? cap.config[step.pointFrom] : undefined; + if (!slice || typeof slice !== 'object') { + errors.push( + prefix + ' step.pointFrom "' + step.pointFrom + '" is not defined in capability config keys', + ); + } else if (slice.type !== 'enum') { + errors.push( + prefix + ' step.pointFrom "' + step.pointFrom + '" must reference an enum config key (got type: ' + slice.type + ')', + ); + } else if (!Array.isArray(slice.values) || !slice.values.includes(step.point)) { + errors.push( + prefix + ' step.pointFrom "' + step.pointFrom + '" enum values do not include this step\'s own point "' + + step.point + '" — the step could never activate', + ); + } + } + } + for (const contrib of cap.contributions) { if (contrib.when !== undefined) { if (typeof contrib.when !== 'string') continue; diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index 3d8856c22..6d33ce4ae 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -81,26 +81,36 @@ fi -Check if code review is active via the capability registry: +Check if code review is active via `workflow.code_review` (the capability's on/off toggle — independent of `workflow.code_review_point`, the loop-point selector; a manual invocation must work regardless of which automatic point is currently configured): ```bash -EXECUTE_POST_HOOKS_JSON=$(gsd_run loop render-hooks execute:post --raw) +CODE_REVIEW_ENABLED=$(gsd_run query config-get workflow.code_review --raw 2>/dev/null || echo "true") ``` -Resolve active step hooks from `EXECUTE_POST_HOOKS_JSON` where `kind == "step"` and `ref.skill == "code-review"`. - -If no active code-review step hook exists: +If `CODE_REVIEW_ENABLED` is not `"true"`: ``` Code review skipped (code-review capability inactive) ``` Exit workflow. -Default is active through the Capability Registry schema — only skip when the registry resolves no active code-review step hook. This check runs AFTER phase validation so invalid phase errors are shown first. +Default is active (`workflow.code_review` schema default is `true`) — only skip when explicitly disabled. This check runs AFTER phase validation so invalid phase errors are shown first. Three-tier scoping with explicit precedence: +Compute the phase's last review commit, if any. This narrows Tiers 2 and 3 below to what +changed since that review (wave-scoped reviews under `workflow.code_review_point=execute:wave:post`): + +```bash +# #3661: incremental scoping — when this phase has a prior review, later tiers +# narrow to what changed since it (wave-scoped reviews under +# workflow.code_review_point=execute:wave:post). Empty on a phase's first review +# (the entire execute:post-default path), in which case Tiers 2 and 3 below are +# unchanged from today. +LAST_REVIEW_COMMIT=$(git log --format=%H -1 -- "${PHASE_DIR}/${PADDED_PHASE}-REVIEW.md" 2>/dev/null) +``` + **Tier 1 — --files override (highest precedence per D-08):** If FILES_OVERRIDE is set (from --files flag): @@ -144,6 +154,14 @@ if [ -z "$FILES_OVERRIDE" ]; then # `$VAR` word-splits under bash but not zsh, collapsing every element onto # one iteration there. for summary in $(printf '%s' "$SUMMARIES"); do + # #3661: skip a SUMMARY.md unchanged since the phase's last review — this + # summary's plan was already reviewed. No-op (every summary is "changed") when + # LAST_REVIEW_COMMIT is empty. Fails OPEN on any git error (file stays in scope) + # — never silently drop a file because a git command errored. + if [ -n "$LAST_REVIEW_COMMIT" ] && git diff --quiet "${LAST_REVIEW_COMMIT}" HEAD -- "$summary" 2>/dev/null; then + continue + fi + # Extract key_files.created and key_files.modified using node for reliable YAML parsing # This avoids fragile awk parsing that breaks on indentation differences EXTRACTED=$(node -e " @@ -241,7 +259,11 @@ surface — so a partial SUMMARY result can no longer silently mask the rest of # move under milestones/ on archive) is closed. PHASE_START=$(git log --format="%H" --diff-filter=A -- "${PHASE_DIR}" 2>/dev/null | tail -1) DIFF_BASE="" -if [ -n "$PHASE_START" ]; then +if [ -n "$LAST_REVIEW_COMMIT" ]; then + # #3661: a prior review exists — narrow the diff base to since that review + # (wave-scoped) instead of the whole phase. + DIFF_BASE="$LAST_REVIEW_COMMIT" +elif [ -n "$PHASE_START" ]; then if git rev-parse "${PHASE_START}^" >/dev/null 2>&1; then DIFF_BASE="${PHASE_START}^" else @@ -774,7 +796,7 @@ If `--files` validation fails unexpectedly on macOS, install coreutils or use ab - [ ] Phase validated before config gate check -- [ ] Capability gate checked (execute:post code-review hook) +- [ ] Capability gate checked (`workflow.code_review` config key) - [ ] --fix/--all/--auto flags parsed via code-review-flags.cjs typed IR (not ad-hoc bash) - [ ] Depth resolved with validation (quick|standard|deep) - [ ] File scope computed with 3 tiers: --files > SUMMARY.md > git diff diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 02b716dc9..e7e6def40 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -1025,7 +1025,7 @@ increases monotonically across waves. `{status}` is `complete` (success), **Contribution dispatch:** inject every `kind == "contribution"` fragment per @gsd-core/references/loop-hook-dispatch.md (skip when none), before the 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 wave completion. ⚠ **Validate `ref.command` in-context before any shell use** (third-party manifest input) — loop-hook-dispatch.md § `step`. + **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 wave completion. ⚠ **Validate `ref.command` in-context before any shell use** (third-party manifest input) — loop-hook-dispatch.md § `step`. **`ref.skill == "code-review"` (#3661):** the generic contract's bare skill dispatch carries no phase argument, but `code-review.md`'s `initialize` step requires one (`PHASE_ARG="${1}"`) or it reports "Phase not found" and exits — pass it explicitly, mirroring step `code_review_gate` below: `Skill(skill="gsd-code-review", args="${PHASE_NUMBER}")`. **For each active entry where `kind == "gate"`** (process in array order): read and execute `gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md` for the full evaluation contract (check validation, `onError`, blocking semantics, mapper spawn). When all active gates are processed without a blocking halt, continue to step 5.8. diff --git a/src/capability-activation.cts b/src/capability-activation.cts index 265f73a49..69db8d878 100644 --- a/src/capability-activation.cts +++ b/src/capability-activation.cts @@ -124,9 +124,41 @@ function _resolveActivationValue( return r.found ? Boolean(r.value) : false; } +/** + * Resolve a step's optional `pointFrom` gate (#3661): a step declaring + * `pointFrom: ""` is only active for ITS OWN `point` when the + * resolved value of that config key equals `point` exactly. This lets a + * capability register the same logical step at more than one loop point and + * have config select which registration is live — without teaching the + * shared `when` grammar an equality operator (`when` stays a plain boolean + * gate on a dotted key everywhere else). + * + * No `pointFrom` → true (unconditional; every existing step is unaffected). + * Present but not a non-empty string → false (malformed, mirrors `when`). + * Present and resolved → true only on an exact match against `point`. + * + * Single-owner precedence engine: called from both `loop-resolver.cts` + * (`isActive`) and `capability-state.cts` (`processHooks`) so the two + * consumers can never diverge on activation (see + * `tests/capability-precedence-parity.test.cjs`). + */ +function _resolvePointGate( + pointFrom: unknown, + point: string, + config: Record, + cwd: string | undefined, + registry: Record, +): boolean { + if (pointFrom === undefined || pointFrom === null) return true; + if (typeof pointFrom !== 'string' || pointFrom.length === 0) return false; + const r = resolveConfigKey(pointFrom, { config, cwd, registry }); + return r.found && r.value === point; +} + export = { _getNestedConfigValue, _readRawConfigKey, _resolveActivationValue, + _resolvePointGate, resolveConfigKey, }; diff --git a/src/capability-state.cts b/src/capability-state.cts index dc987e978..464f03854 100644 --- a/src/capability-state.cts +++ b/src/capability-state.cts @@ -38,7 +38,7 @@ const { output: coreOutput, error: coreError } = ioMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import activationMod = require('./capability-activation.cjs'); -const { _resolveActivationValue } = activationMod; +const { _resolveActivationValue, _resolvePointGate } = activationMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import configLoaderMod = require('./config-loader.cjs'); @@ -279,6 +279,12 @@ function resolveCapabilityState(input: ResolveCapabilityStateInput): ResolveCapa // (mirrors loop-resolver.isActive: `typeof when !== 'string' || when.length === 0` → false) configured = false; } + // #3661: optional point-selection gate, ANDed in — mirrors loop-resolver.isActive + // EXACTLY (see capability-activation.cts's _resolvePointGate doc comment; this + // parity is load-bearing, see tests/capability-precedence-parity.test.cjs). + if (configured) { + configured = _resolvePointGate(h['pointFrom'], point, config, cwd, registry); + } // Hook active = capability-level active AND hook's own config gate. // The capability's `active` constant (= enabled && configActivation) is // used here so that a config-disabled capability (active=false) cannot diff --git a/src/loop-resolver.cts b/src/loop-resolver.cts index ff86175a7..e9ef111a4 100644 --- a/src/loop-resolver.cts +++ b/src/loop-resolver.cts @@ -43,7 +43,7 @@ const { resolveCapabilityRuntimeState } = capabilityStateModule; // ─── Capability-activation engine (single owner for config-key precedence) ──── // eslint-disable-next-line @typescript-eslint/no-require-imports import capabilityActivationModule = require('./capability-activation.cjs'); -const { _getNestedConfigValue, _readRawConfigKey, _resolveActivationValue, resolveConfigKey } = capabilityActivationModule; +const { _getNestedConfigValue, _readRawConfigKey, _resolveActivationValue, _resolvePointGate, resolveConfigKey } = capabilityActivationModule; // ─── Canonical points (derived from LOOP_HOST_CONTRACT — authoritative 12) ─── @@ -203,11 +203,13 @@ function resolveLoopHooks(input: ResolveLoopHooksInput): ResolveLoopHooksResult // Helper: check activation using single-key precedence resolver (FIX 1 + FIX 3) function isActive(hook: RawHook): boolean { const when = hook['when']; - // No `when` → unconditional hook, always active - if (when === undefined || when === null) return true; - // FIX 3: `when` present but not a non-empty string → malformed registry data → INACTIVE - if (typeof when !== 'string' || when.length === 0) return false; - return _resolveActivationValue(when, config, cwd, registry); + if (when !== undefined && when !== null) { + // FIX 3: `when` present but not a non-empty string → malformed registry data → INACTIVE + if (typeof when !== 'string' || when.length === 0) return false; + if (!_resolveActivationValue(when, config, cwd, registry)) return false; + } + // #3661: optional point-selection gate — see capability-activation.cts. + return _resolvePointGate((hook as Record)['pointFrom'], point, config, cwd, registry); } function isCapabilityActive(capId: string): boolean { @@ -630,6 +632,10 @@ export = { // Re-exported for identity parity guard (FIX 2: resolveConfigValues in this module // calls resolveConfigKey; exporting it here makes the single-owner contract testable). resolveConfigKey, + // #3661: re-exported for the same identity parity guard — isActive calls + // _resolvePointGate; exporting it here makes the single-owner contract testable + // (see tests/capability-precedence-parity.test.cjs's identity guard describe block). + _resolvePointGate, CANONICAL_POINTS_FALLBACK, CANONICAL_POINTS, }; diff --git a/tests/capability-precedence-parity.test.cjs b/tests/capability-precedence-parity.test.cjs index c6ece69aa..200ed37cd 100644 --- a/tests/capability-precedence-parity.test.cjs +++ b/tests/capability-precedence-parity.test.cjs @@ -38,6 +38,7 @@ const LIB = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib'); const capabilityActivation = require(path.join(LIB, 'capability-activation.cjs')); const loopResolver = require(path.join(LIB, 'loop-resolver.cjs')); +const capabilityStateMod = require(path.join(LIB, 'capability-state.cjs')); // ─── (a) Identity guard ─────────────────────────────────────────────────────── @@ -65,6 +66,128 @@ describe('capability-precedence-parity: identity guard — loop-resolver re-expo '_readRawConfigKey: loop-resolver must re-export the capability-activation.cjs function, not a local copy', ); }); + + // #3661 + test('_resolvePointGate is the same function in both modules', () => { + assert.strictEqual( + loopResolver._resolvePointGate, + capabilityActivation._resolvePointGate, + '_resolvePointGate: loop-resolver must re-export the capability-activation.cjs function, not a local copy', + ); + }); +}); + +// ─── (a2) #3661 — _resolvePointGate pure-function matrix (50-test-matrix.md Section A) ───── +// +// A1-A10: pure, no-I/O behavioral coverage of _resolvePointGate directly (happy / +// boundary / negative / hostile), mirroring the style of the _resolveActivationValue +// matrix below but scoped to the new pointFrom gate. + +describe('capability-precedence-parity: _resolvePointGate matrix (#3661, matrix Section A)', () => { + const { _resolvePointGate } = capabilityActivation; + + test('A1: resolvePointGateTrueWhenPointFromAbsent — pointFrom undefined → true', () => { + const registry = makeRegistry('workflow.point_key', undefined); + assert.strictEqual( + _resolvePointGate(undefined, 'execute:post', {}, undefined, registry), + true, + ); + }); + + test('A2: resolvePointGateTrueWhenPointFromNull — pointFrom null → true (mirrors undefined)', () => { + const registry = makeRegistry('workflow.point_key', undefined); + assert.strictEqual( + _resolvePointGate(null, 'execute:post', {}, undefined, registry), + true, + ); + }); + + test('A3: resolvePointGateTrueOnExactMatch — config resolves to the SAME value as point → true', () => { + const key = 'workflow.point_key'; + const config = { workflow: { point_key: 'execute:wave:post' } }; + const registry = makeRegistry(key, undefined); + assert.strictEqual( + _resolvePointGate(key, 'execute:wave:post', config, undefined, registry), + true, + ); + }); + + test('A4: resolvePointGateFalseOnMismatch — config resolves to a DIFFERENT value than point → false', () => { + const key = 'workflow.point_key'; + const config = { workflow: { point_key: 'execute:post' } }; + const registry = makeRegistry(key, undefined); + assert.strictEqual( + _resolvePointGate(key, 'execute:wave:post', config, undefined, registry), + false, + ); + }); + + test('A5: resolvePointGateFalseWhenKeyUnresolvable — key absent from config and no schema default → false', () => { + const key = 'workflow.nonexistent_point_key'; + const registry = makeRegistry(key, undefined); + assert.strictEqual( + _resolvePointGate(key, 'execute:post', {}, undefined, registry), + false, + ); + }); + + test('A6: resolvePointGateFalseOnEmptyString — pointFrom="" → false (malformed, mirrors when)', () => { + const registry = makeRegistry('workflow.point_key', undefined); + assert.strictEqual( + _resolvePointGate('', 'execute:post', {}, undefined, registry), + false, + ); + }); + + test('A7: resolvePointGateFalseOnNonStringType — pointFrom is a number/object/array → false', () => { + const registry = makeRegistry('workflow.point_key', undefined); + for (const malformed of [42, { key: 'workflow.point_key' }, ['workflow.point_key']]) { + assert.strictEqual( + _resolvePointGate(malformed, 'execute:post', {}, undefined, registry), + false, + `pointFrom=${JSON.stringify(malformed)} must resolve to false`, + ); + } + }); + + test('A8: resolvePointGateUsesSchemaDefaultPrecedence — resolves via registry.configSchema default', () => { + const key = 'workflow.point_key'; + // No explicit config value — only the schema default, matching point. + const registryMatch = makeRegistry(key, 'execute:post'); + assert.strictEqual( + _resolvePointGate(key, 'execute:post', {}, undefined, registryMatch), + true, + 'schema default equal to point must match', + ); + // Schema default present but NOT equal to point → false. + const registryMismatch = makeRegistry(key, 'execute:wave:post'); + assert.strictEqual( + _resolvePointGate(key, 'execute:post', {}, undefined, registryMismatch), + false, + 'schema default not equal to point must not match', + ); + }); + + test('A9: resolvePointGateDoesNotMatchOnDoubleEmptyUnlessFound — point="" and unresolved key must not spuriously match', () => { + const key = 'workflow.nonexistent_point_key'; + const registry = makeRegistry(key, undefined); // no schema default → found:false + assert.strictEqual( + _resolvePointGate(key, '', {}, undefined, registry), + false, + 'an unresolved (found:false) key must never match, even against an empty point string', + ); + }); + + test('A10: resolvePointGateRejectsPrototypePollutionKey — pointFrom colliding with a prototype key segment → false', () => { + const key = 'a.__proto__.polluted'; + const config = { a: { real: 1 } }; + const registry = makeRegistry(key, undefined); // no schema default for this exact dotted key + assert.strictEqual( + _resolvePointGate(key, 'execute:post', config, undefined, registry), + false, + '__proto__ path segment must be rejected by the shared nested-traversal guard, yielding found:false', + ); + }); }); // ─── (b) Behavioral matrix ──────────────────────────────────────────────────── @@ -594,3 +717,169 @@ describe('capability-precedence-parity: behavioral — loop-resolver resolveConf ); }); }); + +// ─── (e) #3661 — loop-resolver and capability-state agree on pointFrom selection ── +// (50-test-matrix.md Section D) +// +// A single synthetic two-point capability fixture is run through BOTH resolvers +// (resolveLoopHooks / resolveCapabilityState) sharing the same enum config key, +// proving the two consumers of _resolvePointGate can never diverge. + +describe('capability-precedence-parity: loop-resolver + capability-state agree on pointFrom selection (#3661, matrix Section D)', () => { + const { resolveCapabilityState } = capabilityStateMod; + const { resolveLoopHooks, CANONICAL_POINTS_FALLBACK } = loopResolver; + + /** + * Build one synthetic capability declared twice (point A / point B) sharing one + * enum `pointFrom` key, plus a third control step at point A with NO `pointFrom` + * (governed by `when` alone). Returns both a loop-resolver-shaped registry + * (byLoopPoint) and a capability-state-shaped registry (capabilities), built from + * the SAME step objects, so both resolvers see byte-identical hook declarations. + */ + function makeTwoPointFixture() { + const capId = 'test-two-point-cap'; + const pointA = 'execute:post'; + const pointB = 'execute:wave:post'; + const enumKey = 'workflow.test_point_select'; + const whenAKey = 'workflow.test_step_a_enabled'; + const whenBKey = 'workflow.test_step_b_enabled'; + const whenControlKey = 'workflow.test_control_enabled'; + + const stepA = { + capId, point: pointA, ref: { skill: 'test-skill' }, + produces: [], consumes: [], when: whenAKey, pointFrom: enumKey, onError: 'skip', + }; + const stepB = { + capId, point: pointB, ref: { skill: 'test-skill' }, + produces: [], consumes: [], when: whenBKey, pointFrom: enumKey, onError: 'skip', + }; + // Control: no pointFrom at all — must be unaffected by the enum's value (D4). + const controlStep = { + capId, point: pointA, ref: { skill: 'control-skill' }, + produces: [], consumes: [], when: whenControlKey, onError: 'skip', + }; + + const configSchema = { + [enumKey]: { default: pointA }, + [whenAKey]: { default: true }, + [whenBKey]: { default: true }, + [whenControlKey]: { default: true }, + }; + + const byLoopPoint = {}; + for (const p of CANONICAL_POINTS_FALLBACK) { + byLoopPoint[p] = { steps: [], contributions: [], gates: [] }; + } + byLoopPoint[pointA].steps = [stepA, controlStep]; + byLoopPoint[pointB].steps = [stepB]; + const loopRegistry = { byLoopPoint, configSchema }; + + const capStateRegistry = { + capabilities: { + [capId]: { + id: capId, tier: 'standard', skills: [], steps: [stepA, stepB, controlStep], + gates: [], contributions: [], config: {}, + }, + }, + configSchema, + }; + + const capabilityStatesById = new Map([[capId, { enabled: true, active: true }]]); + + return { + capId, pointA, pointB, enumKey, whenAKey, whenBKey, whenControlKey, + loopRegistry, capStateRegistry, capabilityStatesById, + }; + } + + function capStateHooks(fixture, config) { + const result = resolveCapabilityState({ + registry: fixture.capStateRegistry, + installedSkills: '*', + surfacedSkills: new Set(), + config, + }); + assert.strictEqual(result.capabilities.length, 1, 'fixture declares exactly one capability'); + return result.capabilities[0].hooks; + } + + test('D1: loopResolverAndCapabilityStateAgreeOnDefaultPointFromSelection', () => { + const f = makeTwoPointFixture(); + const config = {}; // everything resolves via schema default: enumKey -> pointA + + const resultA = resolveLoopHooks({ point: f.pointA, registry: f.loopRegistry, config, capabilityStatesById: f.capabilityStatesById }); + const resultB = resolveLoopHooks({ point: f.pointB, registry: f.loopRegistry, config, capabilityStatesById: f.capabilityStatesById }); + const loopStepAActive = resultA.activeHooks.some((h) => h.when === f.whenAKey); + const loopStepBActive = resultB.activeHooks.some((h) => h.when === f.whenBKey); + assert.strictEqual(loopStepAActive, true, 'loop-resolver: point A step must be active by default'); + assert.strictEqual(loopStepBActive, false, 'loop-resolver: point B step must be inactive by default'); + + const hooks = capStateHooks(f, config); + const stateStepA = hooks.find((h) => h.when === f.whenAKey); + const stateStepB = hooks.find((h) => h.when === f.whenBKey); + assert.strictEqual(stateStepA.active, true, 'capability-state: point A step must be active by default'); + assert.strictEqual(stateStepB.active, false, 'capability-state: point B step must be inactive by default'); + + assert.strictEqual(loopStepAActive, stateStepA.active, 'resolvers must agree on point A'); + assert.strictEqual(loopStepBActive, stateStepB.active, 'resolvers must agree on point B'); + }); + + test('D2: loopResolverAndCapabilityStateAgreeOnFlippedPointFromSelection', () => { + const f = makeTwoPointFixture(); + const config = { workflow: { test_point_select: f.pointB } }; + + const resultA = resolveLoopHooks({ point: f.pointA, registry: f.loopRegistry, config, capabilityStatesById: f.capabilityStatesById }); + const resultB = resolveLoopHooks({ point: f.pointB, registry: f.loopRegistry, config, capabilityStatesById: f.capabilityStatesById }); + const loopStepAActive = resultA.activeHooks.some((h) => h.when === f.whenAKey); + const loopStepBActive = resultB.activeHooks.some((h) => h.when === f.whenBKey); + assert.strictEqual(loopStepAActive, false, 'loop-resolver: point A step must flip to inactive'); + assert.strictEqual(loopStepBActive, true, 'loop-resolver: point B step must flip to active'); + + const hooks = capStateHooks(f, config); + const stateStepA = hooks.find((h) => h.when === f.whenAKey); + const stateStepB = hooks.find((h) => h.when === f.whenBKey); + assert.strictEqual(stateStepA.active, false, 'capability-state: point A step must flip to inactive'); + assert.strictEqual(stateStepB.active, true, 'capability-state: point B step must flip to active'); + + assert.strictEqual(loopStepAActive, stateStepA.active, 'resolvers must agree on point A after flip'); + assert.strictEqual(loopStepBActive, stateStepB.active, 'resolvers must agree on point B after flip'); + }); + + test('D3: loopResolverAndCapabilityStateAgreeOnOutOfEnumPointFromValue', () => { + const f = makeTwoPointFixture(); + const config = { workflow: { test_point_select: 'bogus' } }; + + const resultA = resolveLoopHooks({ point: f.pointA, registry: f.loopRegistry, config, capabilityStatesById: f.capabilityStatesById }); + const resultB = resolveLoopHooks({ point: f.pointB, registry: f.loopRegistry, config, capabilityStatesById: f.capabilityStatesById }); + const loopStepAActive = resultA.activeHooks.some((h) => h.when === f.whenAKey); + const loopStepBActive = resultB.activeHooks.some((h) => h.when === f.whenBKey); + assert.strictEqual(loopStepAActive, false, 'loop-resolver: point A step must be inactive on an out-of-enum value'); + assert.strictEqual(loopStepBActive, false, 'loop-resolver: point B step must be inactive on an out-of-enum value'); + + const hooks = capStateHooks(f, config); + const stateStepA = hooks.find((h) => h.when === f.whenAKey); + const stateStepB = hooks.find((h) => h.when === f.whenBKey); + assert.strictEqual(stateStepA.active, false, 'capability-state: point A step must be inactive on an out-of-enum value'); + assert.strictEqual(stateStepB.active, false, 'capability-state: point B step must be inactive on an out-of-enum value'); + + assert.strictEqual(loopStepAActive, stateStepA.active, 'resolvers must agree: both points inactive'); + assert.strictEqual(loopStepBActive, stateStepB.active, 'resolvers must agree: both points inactive'); + }); + + test('D4: loopResolverAndCapabilityStateAgreeWhenPointFromAbsent', () => { + // Reuse D3's out-of-enum config — the strongest proof that the control step + // (no pointFrom at all) is UNAFFECTED by the enum's value in either resolver. + const f = makeTwoPointFixture(); + const config = { workflow: { test_point_select: 'bogus' } }; + + const resultA = resolveLoopHooks({ point: f.pointA, registry: f.loopRegistry, config, capabilityStatesById: f.capabilityStatesById }); + const loopControlActive = resultA.activeHooks.some((h) => h.when === f.whenControlKey); + assert.strictEqual(loopControlActive, true, 'loop-resolver: control step (no pointFrom) must remain active, governed by when alone'); + + const hooks = capStateHooks(f, config); + const stateControl = hooks.find((h) => h.when === f.whenControlKey); + assert.strictEqual(stateControl.active, true, 'capability-state: control step (no pointFrom) must remain active, governed by when alone'); + + assert.strictEqual(loopControlActive, stateControl.active, 'resolvers must agree the control step is unaffected'); + }); +}); diff --git a/tests/capability-registry.test.cjs b/tests/capability-registry.test.cjs index cee06d76f..45ef1e50a 100644 --- a/tests/capability-registry.test.cjs +++ b/tests/capability-registry.test.cjs @@ -363,6 +363,167 @@ describe('validateAgainstContract adversarial cases', () => { }); }); +// ─── validateStep / validateAgainstContract: step.pointFrom (#3661, matrix Section E) ── + +describe('capability-validator: step.pointFrom (#3661, matrix Section E)', () => { + /** + * Minimal synthetic capability, independent of UI_CAP, carrying one enum + * config key (usable as a pointFrom target) and one boolean key (usable as + * a non-enum negative fixture). + */ + function makePointFromCap(overrides = {}) { + return { + id: 'test-point-from-cap', + role: 'feature', + version: '1.0.0', + title: 'Test PointFrom Cap', + description: 'Synthetic fixture for pointFrom (#3661) validator tests.', + tier: 'standard', + requires: [], + runtimeCompat: { supported: ['*'], unsupported: [] }, + skills: ['test-skill'], + agents: [], + hooks: [], + config: { + 'workflow.test_point': { + type: 'enum', + values: ['execute:post', 'execute:wave:post'], + default: 'execute:post', + description: 'Test enum key for pointFrom.', + }, + 'workflow.test_bool': { + type: 'boolean', + default: true, + description: 'Test non-enum key.', + }, + }, + steps: [], + contributions: [], + gates: [], + ...overrides, + }; + } + + test('E1: validateStepRejectsNonStringPointFrom', () => { + const cap = makePointFromCap({ + steps: [{ + point: 'execute:post', ref: { skill: 'test-skill' }, produces: [], consumes: [], + pointFrom: 42, onError: 'skip', + }], + }); + const errors = validateCapability(cap, 'test-point-from-cap'); + assert.ok( + errors.some((e) => e.includes('.pointFrom must be a string if present')), + 'Expected a pointFrom type error, got: ' + JSON.stringify(errors), + ); + }); + + test('E2: validateStepAcceptsMissingPointFrom — fully optional', () => { + const cap = makePointFromCap({ + steps: [{ + point: 'execute:post', ref: { skill: 'test-skill' }, produces: [], consumes: [], + onError: 'skip', + }], + }); + const capErrors = validateCapability(cap, 'test-point-from-cap'); + assert.deepEqual(capErrors, [], 'Expected no validateCapability errors: ' + JSON.stringify(capErrors)); + const contractErrors = validateAgainstContract(cap, 'test-point-from-cap'); + assert.deepEqual(contractErrors, [], 'Expected no validateAgainstContract errors: ' + JSON.stringify(contractErrors)); + }); + + test('E3: validateAgainstContractRejectsUndefinedPointFromKey', () => { + const cap = makePointFromCap({ + steps: [{ + point: 'execute:post', ref: { skill: 'test-skill' }, produces: [], consumes: [], + pointFrom: 'workflow.nonexistent_key', onError: 'skip', + }], + }); + const errors = validateAgainstContract(cap, 'test-point-from-cap'); + assert.ok( + errors.some((e) => e.includes('pointFrom "workflow.nonexistent_key" is not defined in capability config keys')), + 'Expected an undefined-key pointFrom error, got: ' + JSON.stringify(errors), + ); + }); + + test('E4: validateAgainstContractRejectsNonEnumPointFromKey', () => { + const cap = makePointFromCap({ + steps: [{ + point: 'execute:post', ref: { skill: 'test-skill' }, produces: [], consumes: [], + pointFrom: 'workflow.test_bool', onError: 'skip', + }], + }); + const errors = validateAgainstContract(cap, 'test-point-from-cap'); + assert.ok( + errors.some((e) => e.includes('must reference an enum config key')), + 'Expected a non-enum pointFrom error, got: ' + JSON.stringify(errors), + ); + }); + + test('E5: validateAgainstContractRejectsPointFromEnumMissingOwnPoint', () => { + const cap = makePointFromCap({ + steps: [{ + point: 'verify:post', ref: { skill: 'test-skill' }, produces: [], consumes: [], + pointFrom: 'workflow.test_point', onError: 'skip', + }], + }); + const errors = validateAgainstContract(cap, 'test-point-from-cap'); + assert.ok( + errors.some((e) => e.includes('enum values do not include this step') && e.includes('verify:post')), + 'Expected an enum-missing-own-point error, got: ' + JSON.stringify(errors), + ); + }); + + test('E6: validateAgainstContractAcceptsWellFormedTwoPointDeclaration — mirrors real code-review shape', () => { + const cap = makePointFromCap({ + steps: [ + { + point: 'execute:post', ref: { skill: 'test-skill' }, produces: ['REVIEW.md'], consumes: [], + when: 'workflow.test_bool', pointFrom: 'workflow.test_point', onError: 'skip', + }, + { + point: 'execute:wave:post', ref: { skill: 'test-skill' }, produces: ['REVIEW.md'], consumes: [], + when: 'workflow.test_bool', pointFrom: 'workflow.test_point', onError: 'skip', + }, + ], + }); + const capErrors = validateCapability(cap, 'test-point-from-cap'); + assert.deepEqual(capErrors, [], 'Expected no validateCapability errors: ' + JSON.stringify(capErrors)); + const contractErrors = validateAgainstContract(cap, 'test-point-from-cap'); + assert.deepEqual(contractErrors, [], 'Expected no validateAgainstContract errors: ' + JSON.stringify(contractErrors)); + }); + + test('E7: validateAgainstContractSkipsAlreadyReportedMalformedPointFrom — non-string pointFrom does not double-report or crash', () => { + // Uses a non-string pointFrom (not an empty string): validateStep's own type + // check is `typeof step.pointFrom !== 'string'`, which an empty string PASSES + // (typeof '' === 'string') — so an empty string is not actually "already + // reported" by validateStep. A non-string value (here: an array) is the + // fixture that genuinely exercises the `continue` / "already reported above" + // skip-pattern inside validateAgainstContract's own pointFrom loop, mirroring + // the identical pattern already used for `when`. + const cap = makePointFromCap({ + steps: [{ + point: 'execute:post', ref: { skill: 'test-skill' }, produces: [], consumes: [], + pointFrom: ['workflow.test_point'], onError: 'skip', + }], + }); + const capErrors = validateCapability(cap, 'test-point-from-cap'); + assert.ok( + capErrors.some((e) => e.includes('.pointFrom must be a string if present')), + 'validateStep must flag the non-string pointFrom, got: ' + JSON.stringify(capErrors), + ); + + // validateAgainstContract must not crash and must not ALSO emit a + // "not defined in capability config keys" / "must reference an enum" / + // "enum values do not include" error for the same malformed field — it + // silently skips (continue) because the type error was already reported. + const contractErrors = validateAgainstContract(cap, 'test-point-from-cap'); + assert.ok( + !contractErrors.some((e) => e.includes('pointFrom')), + 'validateAgainstContract must not double-report an already-malformed pointFrom, got: ' + JSON.stringify(contractErrors), + ); + }); +}); + describe('validateCrossCapability adversarial cases', () => { test('duplicate skill ownership across two capabilities rejected', () => { const cap1 = { ...UI_CAP }; diff --git a/tests/capability-state.test.cjs b/tests/capability-state.test.cjs index a09dda7e8..bb53614db 100644 --- a/tests/capability-state.test.cjs +++ b/tests/capability-state.test.cjs @@ -603,6 +603,63 @@ describe('resolveCapabilityState — hook activation details', () => { }); }); +// ─── resolveCapabilityState — pointFrom gate (#3661, 50-test-matrix.md Section C) ── + +describe('resolveCapabilityState — pointFrom gate (#3661, matrix Section C)', () => { + test('C1: capabilityStateConfiguredTrueWhenBothGatesPass — same fixture as B3', () => { + const registry = makeRegistry({ + steps: [{ point: 'plan:pre', when: 'mytool.on', pointFrom: 'mytool.point' }], + configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } }, + }); + const result = resolveCapabilityState({ + registry, + installedSkills: '*', + surfacedSkills: new Set(), + config: { mytool: { on: true } }, + }); + const hook = result.capabilities[0].hooks.find((h) => h.kind === 'step'); + assert.ok(hook, 'step hook should be present'); + assert.strictEqual(hook.configured, true, 'configured=true when both when and pointFrom pass'); + assert.strictEqual(hook.active, true, 'active reflects capability-level active AND configured'); + }); + + test('C2: capabilityStateConfiguredFalseOnPointFromMismatch — same fixture as B4', () => { + const registry = makeRegistry({ + steps: [{ point: 'plan:pre', when: 'mytool.on', pointFrom: 'mytool.point' }], + configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:post', description: 'x' } }, + }); + const result = resolveCapabilityState({ + registry, + installedSkills: '*', + surfacedSkills: new Set(), + config: { mytool: { on: true } }, + }); + const hook = result.capabilities[0].hooks.find((h) => h.kind === 'step'); + assert.ok(hook, 'step hook should be present'); + assert.strictEqual(hook.configured, false, 'pointFrom mismatch must set configured=false even though when is truthy'); + assert.strictEqual(hook.active, false); + }); + + test('C3: capabilityStateCarriesRawWhenThrough — hook.when unchanged even with pointFrom also present', () => { + const registry = makeRegistry({ + steps: [{ point: 'plan:pre', when: 'mytool.on', pointFrom: 'mytool.point' }], + configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } }, + }); + const result = resolveCapabilityState({ + registry, + installedSkills: '*', + surfacedSkills: new Set(), + config: { mytool: { on: true } }, + }); + const hook = result.capabilities[0].hooks.find((h) => h.kind === 'step'); + assert.ok(hook, 'step hook should be present'); + assert.strictEqual( + hook.when, 'mytool.on', + 'raw when value must still be carried through unchanged for diagnostic visibility (pre-existing contract)', + ); + }); +}); + // ─── resolveCapabilityState — determinism ───────────────────────────────────── describe('resolveCapabilityState — determinism', () => { diff --git a/tests/code-review.test.cjs b/tests/code-review.test.cjs index c470b40c9..1eebbd570 100644 --- a/tests/code-review.test.cjs +++ b/tests/code-review.test.cjs @@ -38,9 +38,13 @@ function extractBashBlocks(content) { return blocks; } const os = require('os'); -const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const { runGsdTools, createTempProject, createTempGitProject, cleanup } = require('./helpers.cjs'); +const { runNode } = require('./helpers/process-seam.cjs'); const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); +const REPO_ROOT = path.join(__dirname, '..'); +const GSD_TOOLS_BIN = path.join(REPO_ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs'); + // --- Test Environment Setup --- const AGENTS_DIR = path.join(__dirname, '..', 'agents'); @@ -449,6 +453,103 @@ describe('CR-WORKFLOW: code review workflow structure', () => { assert.ok(content.includes('while IFS= read -r'), 'code-review-fix.md should use portable while-read loop instead of mapfile'); }); + + // #3661: configurable code-review hook point (see .gsd/phase/feat-3661-code-review-hook-point/ + // 40-design.md and 50-test-matrix.md Section G) — manual invocation reads + // workflow.code_review directly (independent of the automatic loop-point + // selector workflow.code_review_point) instead of gating on registry + // presence at the hardcoded execute:post point; and Tier 2/3 file scoping + // narrows to what changed since the phase's last review. + describe('#3661: configurable code-review hook point (test matrix Section G)', () => { + function extractStepBody(content, stepName) { + const re = new RegExp(`([\\s\\S]*?)<\\/step>`); + const m = content.match(re); + return m ? m[1] : null; + } + + test('G1: checkConfigGateReadsCodeReviewConfigDirectly', () => { + const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8'); + const stepContent = extractStepBody(content, 'check_config_gate'); + assert.ok(stepContent, 'code-review.md missing check_config_gate step'); + + assert.ok(/gsd_run query config-get workflow\.code_review\b/.test(stepContent), + 'check_config_gate must read workflow.code_review via gsd_run query config-get'); + }); + + test('G2: checkConfigGateNoLongerProbesExecutePostHooks', () => { + const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8'); + const stepContent = extractStepBody(content, 'check_config_gate'); + assert.ok(stepContent, 'code-review.md missing check_config_gate step'); + + assert.ok(!stepContent.includes('render-hooks execute:post'), + 'check_config_gate must no longer gate on render-hooks execute:post — manual invocation must work regardless of workflow.code_review_point'); + }); + + test('G3: computeFileScopeDerivesLastReviewCommit', () => { + const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8'); + const stepContent = extractStepBody(content, 'compute_file_scope'); + assert.ok(stepContent, 'code-review.md missing compute_file_scope step'); + + assert.ok( + /LAST_REVIEW_COMMIT=\$\(git log --format=%H -1 -- "\$\{PHASE_DIR\}\/\$\{PADDED_PHASE\}-REVIEW\.md"/.test(stepContent), + 'compute_file_scope must derive LAST_REVIEW_COMMIT from the phase REVIEW.md git history', + ); + }); + + test('G4: tier2SkipsUnchangedSummariesSinceLastReview', () => { + const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8'); + const codeBlocks = extractBashBlocks(content); + const tier2Block = codeBlocks.find(block => + block.includes('for summary in $(printf') && block.includes('LAST_REVIEW_COMMIT')); + assert.ok(tier2Block, 'code-review.md Tier 2 SUMMARY loop must reference LAST_REVIEW_COMMIT'); + + assert.ok( + /git diff --quiet "\$\{LAST_REVIEW_COMMIT\}" HEAD -- "\$summary"/.test(tier2Block), + 'Tier 2 must contain a `git diff --quiet "${LAST_REVIEW_COMMIT}" HEAD -- "$summary"` skip-conditional', + ); + assert.ok(/\bcontinue\b/.test(tier2Block), + 'Tier 2 unchanged-since-last-review guard must `continue` (skip) the summary, not just log'); + }); + + test('G5: tier3PrefersLastReviewCommitOverPhaseStartDerivation', () => { + const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8'); + const codeBlocks = extractBashBlocks(content); + // #3995 replaced the old commit-message-grep PHASE_COMMITS derivation with + // PHASE_START (git log --diff-filter=A -- "${PHASE_DIR}") — a phase number is + // only unique within a milestone, not the whole repo, so #3661's fallback + // chain rides on whichever derivation is current rather than pinning the old name. + const tier3Block = codeBlocks.find(block => + block.includes('DIFF_BASE=""') && block.includes('PHASE_START')); + assert.ok(tier3Block, 'code-review.md Tier 3 DIFF_BASE derivation block not found'); + + const lastReviewIdx = tier3Block.indexOf('if [ -n "$LAST_REVIEW_COMMIT" ]; then'); + const phaseStartElifIdx = tier3Block.indexOf('elif [ -n "$PHASE_START" ]; then'); + assert.ok(lastReviewIdx !== -1, 'Tier 3 DIFF_BASE must check LAST_REVIEW_COMMIT'); + assert.ok(phaseStartElifIdx !== -1, 'Tier 3 DIFF_BASE must fall back to PHASE_START via elif (#3995 derivation unchanged)'); + assert.ok(lastReviewIdx < phaseStartElifIdx, + 'LAST_REVIEW_COMMIT must be checked BEFORE the PHASE_START fallback, so a prior review narrows the diff base'); + assert.ok(/DIFF_BASE="\$LAST_REVIEW_COMMIT"/.test(tier3Block), + 'Tier 3 must set DIFF_BASE directly from LAST_REVIEW_COMMIT when present (no ^ parent offset)'); + }); + + test('G6: tier2GuardIsNoOpWhenLastReviewCommitEmpty', () => { + const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8'); + const codeBlocks = extractBashBlocks(content); + const tier2Block = codeBlocks.find(block => + block.includes('for summary in $(printf') && block.includes('LAST_REVIEW_COMMIT')); + assert.ok(tier2Block, 'code-review.md Tier 2 SUMMARY loop must reference LAST_REVIEW_COMMIT'); + + // The skip-conditional's guard must require LAST_REVIEW_COMMIT to be + // non-empty (`[ -n "$LAST_REVIEW_COMMIT" ]`) as the FIRST operand of an + // `&&` chain, so on a phase's first review (LAST_REVIEW_COMMIT="") the + // conditional is structurally a no-op — bash short-circuits `&&` before + // ever reaching `git diff --quiet`, so nothing can be skipped. + assert.ok( + /if \[ -n "\$LAST_REVIEW_COMMIT" \] && git diff --quiet/.test(tier2Block), + 'Tier 2 skip-conditional must guard on `[ -n "$LAST_REVIEW_COMMIT" ]` as the first `&&` operand, so it is a structural no-op when LAST_REVIEW_COMMIT is empty (first review)', + ); + }); + }); }); // --- CR-CONFIG: config key registration --- @@ -499,6 +600,71 @@ describe('CR-CONFIG: config key registration', () => { assert.strictEqual(getResult.output, '"standard"', `workflow.code_review_depth should return '"standard"', got ${getResult.output}`); }); + + // ── #3661: workflow.code_review_point — CLI-behavioral (50-test-matrix.md + // Section F, rows F4-F7). Uses createTempGitProject + runGsdTools + the real + // gsd-tools subprocess (via runNode), not source-grep, per the matrix's + // coverage-strategy note. + + function renderHooksEnvelope(tmpDir, point) { + const result = runNode( + [GSD_TOOLS_BIN, 'loop', 'render-hooks', point, '--cwd', tmpDir], + { cwd: REPO_ROOT, timeoutMs: 15000 }, + ); + assert.strictEqual(result.exitCode, 0, `Expected exit 0 for render-hooks ${point}. stderr: ` + (result.stderr || '')); + return JSON.parse(result.stdout.trim()); + } + + test('F4: renderHooksExecutePostActiveByDefault', (t) => { + const tmpDir = createTempGitProject(); + t.after(() => cleanup(tmpDir)); + + const envelope = renderHooksEnvelope(tmpDir, 'execute:post'); + const step = envelope.activeHooks.find((h) => h.capId === 'code-review' && h.kind === 'step'); + assert.ok(step, 'Expected an active code-review step at execute:post by default. Got: ' + JSON.stringify(envelope.activeHooks)); + }); + + test('F5: renderHooksExecuteWavePostInactiveByDefault', (t) => { + const tmpDir = createTempGitProject(); + t.after(() => cleanup(tmpDir)); + + const envelope = renderHooksEnvelope(tmpDir, 'execute:wave:post'); + const step = envelope.activeHooks.find((h) => h.capId === 'code-review' && h.kind === 'step'); + assert.strictEqual(step, undefined, 'code-review step must be inactive at execute:wave:post by default (point not selected). Got: ' + JSON.stringify(envelope.activeHooks)); + }); + + test('F6: renderHooksFlipsWithConfigSetCodeReviewPoint', (t) => { + const tmpDir = createTempGitProject(); + t.after(() => cleanup(tmpDir)); + + const setResult = runGsdTools(['config-set', 'workflow.code_review_point', 'execute:wave:post'], tmpDir); + assert.ok(setResult.success, `config-set workflow.code_review_point failed: ${setResult.error}`); + + const postEnvelope = renderHooksEnvelope(tmpDir, 'execute:post'); + const wavePostEnvelope = renderHooksEnvelope(tmpDir, 'execute:wave:post'); + const postStep = postEnvelope.activeHooks.find((h) => h.capId === 'code-review' && h.kind === 'step'); + const wavePostStep = wavePostEnvelope.activeHooks.find((h) => h.capId === 'code-review' && h.kind === 'step'); + assert.strictEqual(postStep, undefined, 'execute:post code-review step must be inactive once flipped to execute:wave:post'); + assert.ok(wavePostStep, 'execute:wave:post code-review step must become active once the point is flipped'); + }); + + test('F7: configSetRejectsOutOfEnumCodeReviewPoint', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const result = runGsdTools(['config-set', 'workflow.code_review_point', 'bogus'], tmpDir); + assert.strictEqual(result.success, false, 'config-set must reject an out-of-enum workflow.code_review_point value'); + assert.match( + result.error || '', + /Invalid workflow\.code_review_point/, + `Expected an enum-rejection error, got: ${result.error}`, + ); + + // The rejected value must not have been persisted. + const getResult = runGsdTools(['config-get', 'workflow.code_review_point'], tmpDir); + assert.ok(getResult.success, `config-get workflow.code_review_point failed: ${getResult.error}`); + assert.notStrictEqual(getResult.output, '"bogus"', 'out-of-enum value must not be silently accepted/persisted'); + }); }); // --- CR-INTEGRATION: workflow integration points --- @@ -526,6 +692,25 @@ describe('CR-INTEGRATION: workflow integration points', () => { 'execute-phase.md code_review_gate must not read workflow.code_review directly'); }); + // #3661: the generic execute:wave:post step-dispatch contract + // (loop-hook-dispatch.md § step) invokes `Skill(skill="gsd-")` with no + // phase argument, but code-review.md's `initialize` step requires one positionally + // (`PHASE_ARG="${1}"`) — without it the review reports "Phase not found" and exits. + // Caught by the orthogonal spec review; fixed with a precedented carve-out in step + // 5.75 mirroring code_review_gate's own `args="${PHASE_NUMBER}"` invocation. + test('execute-phase.md wave-post dispatch passes PHASE_NUMBER to the code-review skill', () => { + const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'execute-phase.md'), 'utf-8'); + // eslint-disable-next-line local/no-unbounded-quantifier -- parses this repo's own workflow markdown, bounded author-controlled prose + const stepMatch = content.match(/5\.75\.[\s\S]*?(?=\r?\n5\.8\.)/); + assert.ok(stepMatch, 'execute-phase.md missing step 5.75 (execute:wave:post capability dispatch)'); + const stepContent = stepMatch[0]; + + assert.ok(stepContent.includes('ref.skill == "code-review"'), + 'step 5.75 must carve out ref.skill == "code-review" from the generic step-dispatch contract'); + assert.ok(/Skill\(skill="gsd-code-review",\s*args="\$\{PHASE_NUMBER\}"\)/.test(stepContent), + 'step 5.75 must dispatch code-review with an explicit args="${PHASE_NUMBER}", matching code_review_gate\'s invocation — the bare generic Skill(skill="gsd-") form has no phase argument and code-review.md requires one'); + }); + test('execute-phase.md does NOT contain ls.*REVIEW.md.*head pattern', { skip: !PLUGIN_AVAILABLE ? 'Plugin dir not installed' : false }, () => { const content = fs.readFileSync(path.join(PLUGIN_WORKFLOWS_DIR, 'execute-phase.md'), 'utf-8'); diff --git a/tests/execute-wave-post-gate-pipeline-e2e.test.cjs b/tests/execute-wave-post-gate-pipeline-e2e.test.cjs index 7ffb43d29..a84c2be14 100644 --- a/tests/execute-wave-post-gate-pipeline-e2e.test.cjs +++ b/tests/execute-wave-post-gate-pipeline-e2e.test.cjs @@ -628,17 +628,29 @@ describe('F. Real registry execute:wave:post shape — guard against accidental `ui.safety-gate onError must be 'halt'; got ${uiGate.onError}`); }); - test('[happy] real registry: execute:wave:post has exactly 1 step (#2856 live-dom-uat gsd-dom-verifier, onError:skip) and 2 contributions (external-job executor + mempalace capture-problems)', () => { + test('[happy] real registry: execute:wave:post has exactly 2 steps (#3661 code-review ref.skill:code-review pointFrom:workflow.code_review_point onError:skip; #2856 live-dom-uat gsd-dom-verifier, onError:skip) and 2 contributions (external-job executor + mempalace capture-problems)', () => { const point = realRegistry.byLoopPoint['execute:wave:post']; - assert.strictEqual(point.steps.length, 1, - `execute:wave:post must have exactly 1 step; got ${point.steps.length}`); - const [step] = point.steps; - assert.strictEqual(step.capId, 'live-dom-uat', - `execute:wave:post step capId must be 'live-dom-uat'; got ${step.capId}`); - assert.deepStrictEqual(step.ref, { agent: 'gsd-dom-verifier' }, - `execute:wave:post step ref must be { agent: 'gsd-dom-verifier' }; got ${JSON.stringify(step.ref)}`); - assert.strictEqual(step.onError, 'skip', - `execute:wave:post step onError must be 'skip'; got ${step.onError}`); + assert.strictEqual(point.steps.length, 2, + `execute:wave:post must have exactly 2 steps; got ${point.steps.length}`); + const [codeReviewStep, domUatStep] = point.steps; + // #3661: code-review's execute:wave:post step is config-gated (inactive by default) + // and can also live at execute:post via workflow.code_review_point. + assert.strictEqual(codeReviewStep.capId, 'code-review', + `execute:wave:post first step capId must be 'code-review'; got ${codeReviewStep.capId}`); + assert.deepStrictEqual(codeReviewStep.ref, { skill: 'code-review' }, + `execute:wave:post code-review step ref must be { skill: 'code-review' }; got ${JSON.stringify(codeReviewStep.ref)}`); + assert.strictEqual(codeReviewStep.pointFrom, 'workflow.code_review_point', + `execute:wave:post code-review step pointFrom must be 'workflow.code_review_point'; got ${codeReviewStep.pointFrom}`); + assert.strictEqual(codeReviewStep.when, 'workflow.code_review', + `execute:wave:post code-review step when must be 'workflow.code_review'; got ${codeReviewStep.when}`); + assert.strictEqual(codeReviewStep.onError, 'skip', + `execute:wave:post code-review step onError must be 'skip'; got ${codeReviewStep.onError}`); + assert.strictEqual(domUatStep.capId, 'live-dom-uat', + `execute:wave:post step capId must be 'live-dom-uat'; got ${domUatStep.capId}`); + assert.deepStrictEqual(domUatStep.ref, { agent: 'gsd-dom-verifier' }, + `execute:wave:post step ref must be { agent: 'gsd-dom-verifier' }; got ${JSON.stringify(domUatStep.ref)}`); + assert.strictEqual(domUatStep.onError, 'skip', + `execute:wave:post step onError must be 'skip'; got ${domUatStep.onError}`); // #2285: claude-orchestration's dispatch-backend-selector contribution moved // from execute:wave:post to execute:wave:pre — wave:post fires AFTER the // wave already dispatched inline, too late to select a dispatch backend. diff --git a/tests/io.test.cjs b/tests/io.test.cjs index 90664a1b7..3b4b534fc 100644 --- a/tests/io.test.cjs +++ b/tests/io.test.cjs @@ -410,7 +410,9 @@ describe('bug #1008: io.output() tolerates a full / slow non-blocking pipe', () test('retries on EAGAIN and emits the full payload without throwing', (t) => { const written = []; let calls = 0; + const orig = fs.writeSync.bind(fs); t.mock.method(fs, 'writeSync', (fd, data, offset, length) => { + if (fd !== 1) return orig(fd, data, offset, length); calls += 1; if (calls === 1) throw bug1008WriteError('EAGAIN', -11); // pipe momentarily full const chunk = bug1008ChunkOf(data, offset, length); @@ -427,7 +429,9 @@ describe('bug #1008: io.output() tolerates a full / slow non-blocking pipe', () test('retries on EINTR (signal-interrupted write) too', (t) => { const written = []; let calls = 0; + const orig = fs.writeSync.bind(fs); t.mock.method(fs, 'writeSync', (fd, data, offset, length) => { + if (fd !== 1) return orig(fd, data, offset, length); calls += 1; if (calls === 1) throw bug1008WriteError('EINTR', -4); const chunk = bug1008ChunkOf(data, offset, length); @@ -442,7 +446,9 @@ describe('bug #1008: io.output() tolerates a full / slow non-blocking pipe', () test('handles short (partial) writes without truncating', (t) => { const written = []; const CAP = 3; // each writeSync accepts at most 3 bytes, like a draining pipe + const orig = fs.writeSync.bind(fs); t.mock.method(fs, 'writeSync', (fd, data, offset, length) => { + if (fd !== 1) return orig(fd, data, offset, length); const chunk = bug1008ChunkOf(data, offset, length); const part = chunk.slice(0, CAP); written.push(part); @@ -455,13 +461,35 @@ describe('bug #1008: io.output() tolerates a full / slow non-blocking pipe', () }); test('does NOT swallow a genuine, non-transient write error (EPIPE)', (t) => { - t.mock.method(fs, 'writeSync', () => { throw bug1008WriteError('EPIPE', -32); }); + const orig = fs.writeSync.bind(fs); + t.mock.method(fs, 'writeSync', (fd, data, offset, length) => { + if (fd !== 1) return orig(fd, data, offset, length); + throw bug1008WriteError('EPIPE', -32); + }); assert.throws( () => io.output({ ok: true }, false), (err) => err.code === 'EPIPE', 'real (non-transient) errors must still surface', ); }); + + test('regression: the fault-injection mock does not intercept writes to an unrelated fd (would corrupt node:test\'s own IPC otherwise)', (t) => { + const orig = fs.writeSync.bind(fs); + let sawOtherFd = false; + t.mock.method(fs, 'writeSync', (fd, data, offset, length) => { + if (fd !== 1) { + sawOtherFd = true; + return orig(fd, data, offset, length); + } + throw bug1008WriteError('EAGAIN', -11); + }); + // A real write to fd 2 (stderr) while the fd-1 mock is active must reach + // the real fs.writeSync untouched, not the injected fault. + const marker = 'fd-scope-regression-marker\n'; + const n = fs.writeSync(2, marker); + assert.equal(n, Buffer.byteLength(marker), 'write to an unrelated fd must succeed via passthrough, not be intercepted'); + assert.ok(sawOtherFd, 'the mock must observe the unrelated-fd call and delegate it'); + }); }); describe('bug #1008: io.error() tolerates a full non-blocking stderr pipe', () => { @@ -477,9 +505,9 @@ describe('bug #1008: io.error() tolerates a full non-blocking stderr pipe', () = let calls = 0; const restore = fs.writeSync; fs.writeSync = (fd, data, offset, length) => { + if (fd !== 2) return restore(fd, data, offset, length); calls += 1; if (calls === 1) throw bug1008WriteError('EAGAIN', -11); - assert.equal(fd, 2, 'error() must write to stderr'); const chunk = bug1008ChunkOf(data, offset, length); written.push(chunk); return Buffer.byteLength(chunk, 'utf8'); diff --git a/tests/loop-render-hooks.test.cjs b/tests/loop-render-hooks.test.cjs index d3d1df0d4..eca721bfe 100644 --- a/tests/loop-render-hooks.test.cjs +++ b/tests/loop-render-hooks.test.cjs @@ -299,6 +299,112 @@ describe('activation filter', () => { }); }); +// ─── 2b. pointFrom gate (#3661, 50-test-matrix.md Section B) ──────────────── + +describe('activation filter: pointFrom gate (#3661, matrix Section B)', () => { + test('B1: isActiveUnchangedForWhenOnlyStepTruthy — when-only step, when truthy → active (unchanged)', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on' }], + }); + const config = { mytool: { on: true } }; + const result = resolveLoopHooks({ point: 'plan:pre', registry, config }); + assert.strictEqual(result.activeHooks.length, 1); + }); + + test('B2: isActiveUnchangedForWhenOnlyStepFalsy — when-only step, when falsy → inactive (unchanged)', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on' }], + }); + const config = { mytool: { on: false } }; + const result = resolveLoopHooks({ point: 'plan:pre', registry, config }); + assert.strictEqual(result.activeHooks.length, 0); + }); + + test('B3: isActiveTrueWhenBothGatesPass — when truthy AND pointFrom matching point → active', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on', pointFrom: 'mytool.point' }], + configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } }, + }); + const config = { mytool: { on: true } }; + const result = resolveLoopHooks({ point: 'plan:pre', registry, config }); + assert.strictEqual(result.activeHooks.length, 1); + }); + + test('B4: isActiveFalseWhenPointFromMismatchesDespiteWhenTrue — AND semantics: pointFrom mismatch wins', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on', pointFrom: 'mytool.point' }], + configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:post', description: 'x' } }, + }); + const config = { mytool: { on: true } }; + const result = resolveLoopHooks({ point: 'plan:pre', registry, config }); + assert.strictEqual(result.activeHooks.length, 0); + }); + + test('B5: isActiveFalseWhenWhenFalseDespitePointFromMatch — AND semantics: when short-circuits', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on', pointFrom: 'mytool.point' }], + configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } }, + }); + const config = { mytool: { on: false } }; + const result = resolveLoopHooks({ point: 'plan:pre', registry, config }); + assert.strictEqual(result.activeHooks.length, 0); + }); + + test('B6: resolveLoopHooksSelectsExactlyOneOfTwoPointRegistrations — no cross-point leakage', () => { + const byLoopPoint = {}; + for (const p of CANONICAL_POINTS_FALLBACK) byLoopPoint[p] = { steps: [], contributions: [], gates: [] }; + byLoopPoint['plan:pre'].steps = [ + { capId: 'test-cap', point: 'plan:pre', ref: { skill: 'a' }, when: 'mytool.on', pointFrom: 'mytool.point' }, + ]; + byLoopPoint['plan:post'].steps = [ + { capId: 'test-cap', point: 'plan:post', ref: { skill: 'a' }, when: 'mytool.on', pointFrom: 'mytool.point' }, + ]; + const registry = { + byLoopPoint, + configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } }, + }; + const config = { mytool: { on: true } }; + const resultPre = resolveLoopHooks({ point: 'plan:pre', registry, config }); + const resultPost = resolveLoopHooks({ point: 'plan:post', registry, config }); + assert.strictEqual(resultPre.activeHooks.length, 1, 'point A must have exactly one active hook'); + assert.strictEqual(resultPost.activeHooks.length, 0, 'point B must have no active hooks — no leakage'); + }); + + test('B7: resolveLoopHooksBothPointsInactiveOnOutOfEnumValue — fail-closed on a typo', () => { + const byLoopPoint = {}; + for (const p of CANONICAL_POINTS_FALLBACK) byLoopPoint[p] = { steps: [], contributions: [], gates: [] }; + byLoopPoint['plan:pre'].steps = [ + { capId: 'test-cap', point: 'plan:pre', ref: { skill: 'a' }, when: 'mytool.on', pointFrom: 'mytool.point' }, + ]; + byLoopPoint['plan:post'].steps = [ + { capId: 'test-cap', point: 'plan:post', ref: { skill: 'a' }, when: 'mytool.on', pointFrom: 'mytool.point' }, + ]; + const registry = { + byLoopPoint, + configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } }, + }; + const badConfig = { mytool: { on: true, point: 'bogus' } }; + const resultPre = resolveLoopHooks({ point: 'plan:pre', registry, config: badConfig }); + const resultPost = resolveLoopHooks({ point: 'plan:post', registry, config: badConfig }); + assert.strictEqual(resultPre.activeHooks.length, 0, 'an out-of-enum value must not revive the step at either point'); + assert.strictEqual(resultPost.activeHooks.length, 0, 'an out-of-enum value must not revive the step at either point'); + }); + + test('B8: isActiveFalseWhenCapabilityInactiveDespitePointFromMatch — capability gate still cascades', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on', pointFrom: 'mytool.point' }], + configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } }, + }); + const config = { mytool: { on: true } }; + const capabilityStatesById = new Map([['test-cap', { enabled: true, active: false }]]); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config, capabilityStatesById }); + assert.strictEqual( + result.activeHooks.length, 0, + 'capability-level active=false must suppress the hook regardless of pointFrom match', + ); + }); +}); + // ─── 3. UI pilot integration tests ─────────────────────────────────────────── describe('UI pilot integration', () => { @@ -358,6 +464,42 @@ describe('UI pilot integration', () => { }); }); +// ─── 3b. #3661 — code-review capability.json + generated registry structural +// assertions (50-test-matrix.md Section F, rows F2-F3) ──────────────── +// +// Loads the REAL generated registry (built by `npm run build:lib` + +// `node scripts/gen-capability-registry.cjs --write`) — no source-grep, this +// is the same `realRegistry` object the "UI pilot integration" tests above +// already use for behavioral (not text) assertions. + +describe('#3661: code-review capability.json + generated registry (matrix Section F, F2-F3)', () => { + test('F2: registryConfigSchemaHasCodeReviewPointEnum', () => { + const slice = realRegistry.configSchema['workflow.code_review_point']; + assert.ok(slice, 'registry.configSchema must contain workflow.code_review_point'); + assert.strictEqual(slice.type, 'enum'); + assert.deepStrictEqual(slice.values, ['execute:post', 'execute:wave:post']); + assert.strictEqual(slice.default, 'execute:post'); + assert.strictEqual(slice.owner, 'code-review'); + }); + + test('F3: registryRegistersCodeReviewStepAtBothPoints', () => { + const postSteps = realRegistry.byLoopPoint['execute:post'].steps; + const wavePostSteps = realRegistry.byLoopPoint['execute:wave:post'].steps; + assert.ok(Array.isArray(postSteps), 'execute:post.steps must be an array'); + assert.ok(Array.isArray(wavePostSteps), 'execute:wave:post.steps must be an array'); + + const postStep = postSteps.find((s) => s.capId === 'code-review' && s.ref && s.ref.skill === 'code-review'); + const wavePostStep = wavePostSteps.find((s) => s.capId === 'code-review' && s.ref && s.ref.skill === 'code-review'); + assert.ok(postStep, 'execute:post.steps must contain a code-review step. Got: ' + JSON.stringify(postSteps)); + assert.ok(wavePostStep, 'execute:wave:post.steps must contain a code-review step. Got: ' + JSON.stringify(wavePostSteps)); + }); + + // F4-F7 (CLI-behavioral: gsd_run loop render-hooks / config-set through the real + // subprocess, against a temp git project) live in tests/code-review.test.cjs's + // 'CR-CONFIG: config key registration' describe block, alongside the sibling + // workflow.code_review / workflow.code_review_depth CLI round-trip tests. +}); + // ─── 4. Ordering tests ──────────────────────────────────────────────────────── describe('hook ordering', () => { diff --git a/tests/phase6-review-capabilities.test.cjs b/tests/phase6-review-capabilities.test.cjs index 493966f9b..48dc488d8 100644 --- a/tests/phase6-review-capabilities.test.cjs +++ b/tests/phase6-review-capabilities.test.cjs @@ -129,13 +129,13 @@ describe('ADR-857 phase 6 verification and review capability migration', () => { assert.ok(!section.includes('config-get workflow.code_review')); }); - test('code-review command self-gates through execute:post hooks', () => { + test('code-review command self-gates via workflow.code_review config directly, not execute:post hooks (#3661: manual invocation must work regardless of which automatic point — execute:post or execute:wave:post — is configured)', () => { const content = workflow('code-review.md'); const section = sectionBetween(content, '', ''); - assert.ok(section.includes('loop render-hooks execute:post')); - assert.ok(section.includes('ref.skill == "code-review"')); - assert.ok(!section.includes('config-get workflow.code_review')); + assert.ok(section.includes('gsd_run query config-get workflow.code_review --raw')); + assert.ok(!section.includes('loop render-hooks execute:post')); + assert.ok(!section.includes('ref.skill == "code-review"')); }); test('code-review-fix command self-gates through execute:post hooks', () => {