From 5bf77a527f3406e73437b8aa43f52ecee61bedf8 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 8 Jun 2026 23:32:34 -0400 Subject: [PATCH 1/4] fix(#913): guard top-level Claude Code plan-phase against role collapse (#915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three-part fix for the top-level inline collapse bug: 1. plan-phase.md: add block after that makes the Agent-availability requirement explicit; workflow fails-closed (stops with a clear log) in genuinely Agent-less contexts. 2. plan-phase.md: rename 7 "ORCHESTRATOR RULE — CODEX RUNTIME" labels to "ALL RUNTIMES" so the spawn guard applies universally (not just when Codex is detected). 3. execute-phase.md: scope the existing "Other runtimes" inline- fallback prose to non-Claude contexts, preserving the #853 backgrounded-agent behaviour for Claude Code background agents. Co-authored-by: Claude Opus 4.8 --- .../913-plan-phase-toplevel-spawn-guard.md | 5 ++ gsd-core/workflows/execute-phase.md | 7 ++- gsd-core/workflows/plan-phase.md | 39 ++++++++++--- tests/plan-phase-drift-guard.test.cjs | 58 +++++++++++++++++++ tests/workflow-size-budget.test.cjs | 8 +-- 5 files changed, 104 insertions(+), 13 deletions(-) create mode 100644 .changeset/913-plan-phase-toplevel-spawn-guard.md diff --git a/.changeset/913-plan-phase-toplevel-spawn-guard.md b/.changeset/913-plan-phase-toplevel-spawn-guard.md new file mode 100644 index 000000000..3a4b1ade8 --- /dev/null +++ b/.changeset/913-plan-phase-toplevel-spawn-guard.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 913 +--- +**Top-level Claude Code `/gsd-plan-phase` now always spawns the researcher/planner/plan-checker agents instead of collapsing them inline** — a `` block after `` makes the Agent-availability requirement explicit and documents that the workflow fails-closed (stops with a clear log message) in genuinely Agent-less contexts; seven "ORCHESTRATOR RULE — CODEX RUNTIME" labels are renamed to "ALL RUNTIMES" so the guard applies universally; `execute-phase.md` scopes its existing "Other runtimes" inline-fallback prose to non-Claude contexts, preserving the #853 backgrounded-agent behaviour. (#913) diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 0253ab045..1a6d711c3 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -21,8 +21,11 @@ Orchestrator coordinates, not executes. Each subagent loads the full execute-pla instead of spawning parallel agents. Only attempt parallel spawning if the user explicitly requests it — and in that case, rely on the spot-check fallback in step 3 to detect completion. -- **Other runtimes:** If `Agent`/`agent` tool is unavailable, use sequential inline execution as the - fallback. Check for tool availability at runtime rather than assuming based on runtime name. +- **Other runtimes:** If `Agent`/`agent` tool is genuinely unavailable (e.g. a backgrounded + Claude Code agent per #853, or a non-Claude runtime), use sequential inline execution as + the fallback for executor parallelization only. If `Agent` IS available (top-level Claude + Code), you MUST spawn gsd-executor agents — inline execution is not authorized. Check for + actual tool availability, not runtime name. **Fallback rule:** If a spawned agent completes its work (commits visible, SUMMARY.md exists) but the orchestrator never receives the completion signal, treat it as successful based on spot-checks diff --git a/gsd-core/workflows/plan-phase.md b/gsd-core/workflows/plan-phase.md index bc52afd30..caf56c32a 100644 --- a/gsd-core/workflows/plan-phase.md +++ b/gsd-core/workflows/plan-phase.md @@ -27,6 +27,31 @@ Valid GSD subagent types (use exact names — do not fall back to 'general-purpo - gsd-plan-checker — Reviews plan quality before execution + +**Subagent spawning — top-level Claude Code:** +The Agent tool IS available in a top-level Claude Code session. Always spawn +gsd-phase-researcher, gsd-planner, and gsd-plan-checker as separate Agent() calls. +Never absorb these roles inline. Role separation is required regardless of `--chain` +or `--auto` — those options suppress interactive prompts only; they NEVER authorize +collapsing plan roles into the orchestrator context. + +**Backgrounded Claude Code (via manager/autonomous):** +The calling workflow (manager.md / autonomous.md) already runs plan-phase inline via +Skill() on Claude Code so that the plan-checker subagent can still spawn. plan-phase +itself does not need to detect this case. + +**#1009 caveat (discuss-phase early-exit):** +The "display the command and exit" instruction near `## 4` applies only to the +discuss-phase early-exit path. It does NOT authorize inline role performance for any +plan-phase agents. + +**Other runtimes:** +If the Agent tool is genuinely absent (e.g. a backgrounded Claude Code agent per +#853, or a non-Claude runtime that does not expose Agent/agent), log the gap and +stop — do NOT perform researcher/planner/checker roles inline. Independent agent +contexts are required for the plan-checker gate to be meaningful. + + ## 0. Git Branch Invariant @@ -537,7 +562,7 @@ Agent( ) ``` -> **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. +> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. ### Handle Researcher Return @@ -856,7 +881,7 @@ Agent( ) ``` -> **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. +> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. **Handle return:** - **`## PATTERN MAPPING COMPLETE`:** Update `PATTERNS_PATH` to the created file path, continue to step 8. @@ -1019,7 +1044,7 @@ Agent( ) ``` -> **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. +> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. **If `CHUNKED_MODE` is `true`:** Skip the Agent() call above — proceed to step 8.5 instead. @@ -1075,7 +1100,7 @@ Agent( ) ``` -> **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. +> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. Handle return: - **`## OUTLINE COMPLETE`:** Read `PLAN-OUTLINE.md`, extract plan list. Continue to 8.5.2. @@ -1119,7 +1144,7 @@ For each plan entry extracted from `PLAN-OUTLINE.md`: ) ``` - > **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. + > **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. 4. **Verify disk:** Check `${PHASE_DIR}/{plan_id}-PLAN.md` exists. If missing: offer 1) Retry, 2) Stop. @@ -1277,7 +1302,7 @@ Agent( ) ``` -> **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. +> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. ## 11. Handle Checker Return @@ -1392,7 +1417,7 @@ Agent( ) ``` -> **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. +> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. After planner returns -> spawn checker again (step 10), increment iteration_count. diff --git a/tests/plan-phase-drift-guard.test.cjs b/tests/plan-phase-drift-guard.test.cjs index 2e8b84794..9ef1e5bf3 100644 --- a/tests/plan-phase-drift-guard.test.cjs +++ b/tests/plan-phase-drift-guard.test.cjs @@ -135,3 +135,61 @@ describe('plan-phase workflow: Artifacts this phase produces section (#22)', () ); }); }); + +// ─── (C) Top-level spawn guard (#913) ──────────────────────────────────────── + +describe('plan-phase workflow: top-level spawn guard (#913)', () => { + // Extract the runtime_compatibility block for targeted assertions + const rtBlock = (() => { + const m = workflow.match(/([\s\S]*?)<\/runtime_compatibility>/); + return m ? m[1] : ''; + })(); + + test('workflow has a runtime_compatibility block asserting Agent is available at top-level', () => { + assert.ok( + rtBlock.length > 0, + 'plan-phase must have a block — prevents role-collapse regression (#913)' + ); + assert.ok( + rtBlock.includes('Agent tool IS available') || rtBlock.includes('Agent IS available'), + 'plan-phase runtime_compatibility must assert that the Agent tool IS available at top-level Claude Code (#913)' + ); + assert.ok( + rtBlock.toLowerCase().includes('top-level'), + 'plan-phase runtime_compatibility must scope the IS-available assertion to top-level Claude Code (#913)' + ); + assert.ok( + rtBlock.includes('Always spawn') || rtBlock.includes('always spawn'), + 'plan-phase runtime_compatibility must state that plan roles must always be spawned (#913)' + ); + assert.ok( + rtBlock.includes('Never absorb') || rtBlock.includes('never absorb'), + 'plan-phase runtime_compatibility must state that roles must never be absorbed inline (#913)' + ); + }); + + test('workflow states --chain/--auto suppress prompts only, not spawns', () => { + assert.ok( + rtBlock.includes('suppress') && + (rtBlock.includes('prompts only') || rtBlock.includes('interactive prompts only')), + 'plan-phase runtime_compatibility must document that --chain/--auto suppress prompts only, not spawns (#913)' + ); + }); + + test('workflow does not contain unscoped CODEX RUNTIME orchestrator rule labels', () => { + // All "wait for subagent" rules must apply to ALL RUNTIMES, not just Codex + assert.ok( + !workflow.includes('ORCHESTRATOR RULE — CODEX RUNTIME'), + 'plan-phase must not label orchestrator wait rules as "CODEX RUNTIME" — they apply to all runtimes including top-level Claude Code (#913)' + ); + }); + + test('workflow contains ALL RUNTIMES orchestrator rule labels (count preserved)', () => { + // Must have all 7 agent-spawn wait rules still present (none dropped during rename) + const allRuntimesCount = (workflow.match(/ORCHESTRATOR RULE — ALL RUNTIMES/g) || []).length; + assert.ok( + allRuntimesCount >= 7, + `plan-phase must have at least 7 "ORCHESTRATOR RULE — ALL RUNTIMES" labels (one per agent spawn site); found ${allRuntimesCount} (#913)` + ); + }); +}); diff --git a/tests/workflow-size-budget.test.cjs b/tests/workflow-size-budget.test.cjs index 1c75523f4..7713b2be7 100644 --- a/tests/workflow-size-budget.test.cjs +++ b/tests/workflow-size-budget.test.cjs @@ -81,8 +81,8 @@ const GRACE = 3000; // current high-water mark within GRACE (#597 tighten-only ratchet). // XL high-water mark is execute-phase.md — note that under LINES it was // plan-phase; bytes genuinely re-rank the tier, which is the point of #717. -// actualMax=91161 (execute-phase, #891 launcher shim expansion — added 17 runtime home arms); -// slack=1839 ≤ GRACE. plan-phase.md=88120, new-project.md=58110; both well under ceiling. +// actualMax=92525 (execute-phase, #913 inline-fallback scope clarification); +// slack=475 ≤ GRACE. plan-phase.md=90501 (#913 runtime_compatibility block + label rename), new-project.md=58110. const XL_BUDGET = 93000; // LARGE high-water mark is docs-update.md. actualMax=54410 (#891 launcher shim expansion); // slack=1590 ≤ GRACE. quick.md=45710, autonomous.md=38030. @@ -95,8 +95,8 @@ const DEFAULT_BUDGET = 40000; // Grandfathered at current sizes — see PR #2551 for the progressive-disclosure // pattern that future shrinks should follow. Byte counts noted for reference. const XL_WORKFLOWS = new Set([ - 'execute-phase', // 91161 bytes (tier high-water mark; grew in #891 launcher shim expansion) - 'plan-phase', // 85068 bytes + 'execute-phase', // 92525 bytes (tier high-water mark; grew in #913 inline-fallback scope clarification) + 'plan-phase', // 90501 bytes (grew in #913 runtime_compatibility block + label rename) 'new-project', // 55850 bytes ]); From 6dbd89502884af5423cf228bdb1b3b88ce3c8eb2 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 8 Jun 2026 23:37:41 -0400 Subject: [PATCH 2/4] feat(#910): federated config merge in config-loader (ADR-857 phase 3b) (#914) Build the federated config merge: each capability owns its config-key slice (ADR-857 decision 3 / ADR-894), and loadConfig merges them defensively. The registry now emits a full configSchema index ({key:{owner,type,default, description}}, generator-validated); a new src/federated-config.cts resolves federated keys defensively (skip central keys -> pending-migration warning, skip malformed slices -> warning never throw, else type-checked user override ?? default, with nested dotted-path lookup and enum validation); and loadConfig applies the overlay on every return path. Wired as a provably-empty no-op channel: every UI-pilot key is still central, so validKeys is empty and loadConfig returns byte-identical output on all paths (identity return when the overlay is empty; shared CONFIG_DEFAULTS never mutated). Registry-only; no key is cut over; nothing in the live loop changes. Closes #910 Co-authored-by: Claude Opus 4.8 --- .gitignore | 1 + CONTEXT.md | 5 +- docs/ARCHITECTURE.md | 3 +- docs/INVENTORY-MANIFEST.json | 1 + docs/INVENTORY.md | 3 +- eslint.config.mjs | 1 + gsd-core/bin/lib/capability-registry.cjs | 22 + scripts/gen-capability-registry.cjs | 124 ++++ src/config-loader.cts | 187 +++++- src/federated-config.cts | 230 +++++++ tests/capability-registry.test.cjs | 266 +++++++- tests/federated-config-loadconfig.test.cjs | 451 +++++++++++++ tests/federated-config.test.cjs | 696 +++++++++++++++++++++ 13 files changed, 1981 insertions(+), 9 deletions(-) create mode 100644 src/federated-config.cts create mode 100644 tests/federated-config-loadconfig.test.cjs create mode 100644 tests/federated-config.test.cjs diff --git a/.gitignore b/.gitignore index 66723ad00..4bb4bc1c3 100644 --- a/.gitignore +++ b/.gitignore @@ -132,6 +132,7 @@ build/ /gsd-core/bin/lib/phase-id.cjs /gsd-core/bin/lib/config-loader.cjs /gsd-core/bin/lib/model-resolver.cjs +/gsd-core/bin/lib/federated-config.cjs /gsd-core/bin/lib/phase-locator.cjs /gsd-core/bin/lib/roadmap-parser.cjs /gsd-core/bin/lib/drift.cjs diff --git a/CONTEXT.md b/CONTEXT.md index a4c408654..a7b03af7c 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -149,7 +149,10 @@ A bundle delivering one optional GSD feature, toggled as a unit at install or af Generated description of what the five-step loop (Discuss → Plan → Execute → Verify → Ship) exposes as extension points: per-step loop points, agent roles, and core artifacts. Sourced from structured `` HTML-comment markers embedded near the top of each of the five step workflow files (`discuss-phase.md`, `plan-phase.md`, `execute-phase.md`, `verify-work.md`, `ship.md`). Generated by `scripts/gen-loop-host-contract.cjs` → `gsd-core/bin/lib/loop-host-contract.cjs` (ADR-894 §3 phase 3a-impl-2). Covers exactly the 12 canonical points (discuss:pre/post, plan:pre/post, execute:pre/wave:pre/wave:post/post, verify:pre/post, ship:pre/post). The generator enforces a drift guard: every declared non-orchestrator agent role must correspond to an actual agent reference in the workflow file. Consumed by `gen-capability-registry.cjs` (replaces the former inline `LOOP_HOST_CONTRACT` constant). Run `node scripts/gen-loop-host-contract.cjs --write` after editing a workflow step marker. ### Capability Registry -Generated central manifest projecting all co-located Capability declarations into one validated artifact for runtime resolution and for the install, surface, config, and loop-extension adapters. Mirrors the research-profiles / package-identity generation pattern (co-located source → generated central file). Generated by `scripts/gen-capability-registry.cjs` → `gsd-core/bin/lib/capability-registry.cjs` (ADR-894 §5 phase 3a-impl). Role-partitioned indexes: `bySkill`, `byAgent`, `byLoopPoint` (hook ordering materialized), `configKeys`, `runtimes`, `requiresClosure(id)`. Validated against the Loop Host Contract (12 points; generated by `gen-loop-host-contract.cjs` from workflow markers, phase 3a-impl-2). Run `node scripts/gen-capability-registry.cjs --write` after editing any `capabilities//capability.json`. +Generated central manifest projecting all co-located Capability declarations into one validated artifact for runtime resolution and for the install, surface, config, and loop-extension adapters. Mirrors the research-profiles / package-identity generation pattern (co-located source → generated central file). Generated by `scripts/gen-capability-registry.cjs` → `gsd-core/bin/lib/capability-registry.cjs` (ADR-894 §5 phase 3a-impl). Role-partitioned indexes: `bySkill`, `byAgent`, `byLoopPoint` (hook ordering materialized), `configKeys` (ownership map: key→capId), `configSchema` (full per-key schema: key→{ owner, type, default, description }), `runtimes`, `requiresClosure(id)`. ADR-857 phase 3b adds `configSchema` with validated type/default/description per key, sourced from each capability's `.config` slice. Validated against the Loop Host Contract (12 points; generated by `gen-loop-host-contract.cjs` from workflow markers, phase 3a-impl-2). Run `node scripts/gen-capability-registry.cjs --write` after editing any `capabilities//capability.json`. + +### Federated Config +ADR-857 phase 3b seam that merges capability-declared config slices into the `loadConfig` return value. Implemented in `src/federated-config.cts` → `gsd-core/bin/lib/federated-config.cjs`. Exports `mergeFederatedConfig({ configSchema, isCentralKey, userConfig }) → { values, validKeys, warnings }`. Rules: central-schema keys are skipped with a `pending-migration` warning; malformed slices are skipped with a warning (never throws); valid federated keys (absent from the central schema) resolve to the user-supplied value (if type-matches) or the slice default. Object writes are guarded against prototype pollution with inline literal `__proto__`/`constructor`/`prototype` key checks. Wired into `loadConfig` as a true no-op today: every Capability config key is still in the central config-schema, so `isCentralKey()` returns true for all of them and `values` is always empty. The channel becomes live when a key is atomically removed from the central schema at cutover (the ADR-857 migration step). `loadConfig` exposes `_setFederatedRegistryForTests`/`_resetFederatedRegistryForTests` seams for injecting a synthetic registry in tests. ### Loop Extension Point [Planned] A named, stable site on a host loop step (per-step `pre`/`post` plus per-wave in Execute; ~12 total) where Capabilities register hooks. Three hook kinds: `step` (runs as its own sequenced unit), `contribution` (injects into the core step's prompt/context), and `gate` (checks and optionally blocks via a declared `blocking` flag). Each hook declares the artifacts it produces and consumes; hook order is derived by topological sort of that produces/consumes graph (capability-id tiebreak), which also defines data flow — file-artifact based, surviving `/clear` and fresh executor contexts. Hooks are surfaced by runtime resolution with concrete projection: the workflow calls a query (extending the `init.*` resolution seam) that resolves the active hooks and returns fully-rendered, ordered markdown for the executor. Failure is default-resilient — a non-gate hook that errors is skipped with a warning; a hook may opt into `onError: halt`. Part of the Capability system. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4d8f313be..2328a5fa0 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -342,7 +342,8 @@ Node.js CLI utility (`gsd-tools.cjs`) with domain modules split across `gsd-core | Module | Responsibility | | ---------------------- | --------------------------------------------------------------------------------------------------- | -| `config-loader.cjs` | Project config loading — defaults merge, legacy-key migration, workstream overlay, unknown-key/profile-override validation (extracted from `core.cjs`, ADR-857) | +| `config-loader.cjs` | Project config loading — defaults merge, legacy-key migration, workstream overlay, unknown-key/profile-override validation, and federated config overlay (ADR-857 phase 3b) (extracted from `core.cjs`, ADR-857) | +| `federated-config.cjs` | Defensive merge of capability-declared config slices (ADR-857 phase 3b); exports `mergeFederatedConfig`; no-op until capability keys are removed from the central config-schema at cutover | | `core-utils.cjs` | Shared low-level utility primitives — POSIX path normalization, sub-repo/subdirectory scanning, phase file stats, slug/one-liner/plan-id helpers, time-ago (extracted from `core.cjs`, ADR-857) | | `core.cjs` | Shared utilities; compatibility re-exports for planning, I/O (`io.cjs`), and phase-id helpers | | `io.cjs` | CLI I/O primitives — output/error emission, JSON-error mode, large-payload temp-file spillover | diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index db56b51f7..a7e3bbc41 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -293,6 +293,7 @@ "docs.cjs", "drift.cjs", "fallow-runner.cjs", + "federated-config.cjs", "frontmatter.cjs", "gap-checker.cjs", "graphify.cjs", diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 481e84a93..31def503e 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -370,7 +370,7 @@ The `gsd-planner` agent is decomposed into a core agent plus reference modules t --- -## CLI Modules (99 shipped) +## CLI Modules (100 shipped) Full listing: `gsd-core/bin/lib/*.cjs`. @@ -404,6 +404,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`. | `docs.cjs` | Docs-update workflow init, Markdown scanning, monorepo detection | | `drift.cjs` | Post-execute codebase structural drift detector (#2003): classifies file changes into new-dir/barrel/migration/route categories and round-trips `last_mapped_commit` frontmatter | | `fallow-runner.cjs` | Fallow audit adapter for `/gsd-code-review`: binary resolution (`PATH` then `node_modules/.bin`), actionable missing-binary errors, and structural findings normalization | +| `federated-config.cjs` | Defensive merge of capability-declared config slices into the loadConfig return value — ADR-857 phase 3b; exports `mergeFederatedConfig({ configSchema, isCentralKey, userConfig })` → `{ values, validKeys, warnings }`; no-op until a key is atomically removed from the central config-schema (the cutover step) | | `frontmatter.cjs` | YAML frontmatter CRUD operations | | `gap-checker.cjs` | Post-planning gap analysis (#2493): unified REQUIREMENTS.md + CONTEXT.md decisions vs PLAN.md coverage report (`gsd-tools gap-analysis`) | | `graphify.cjs` | Knowledge-graph build/query/status/diff for `/gsd-graphify` | diff --git a/eslint.config.mjs b/eslint.config.mjs index f6c404288..876d53be2 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -74,6 +74,7 @@ export default tseslint.config( 'gsd-core/bin/lib/config-schema.cjs', 'gsd-core/bin/lib/model-profiles.cjs', 'gsd-core/bin/lib/model-resolver.cjs', + 'gsd-core/bin/lib/federated-config.cjs', 'gsd-core/bin/lib/installer-migrations/002-codex-legacy-hooks-json.cjs', 'gsd-core/bin/lib/installer-migrations/003-rename-get-shit-done-to-gsd-core.cjs', 'gsd-core/bin/lib/observability/logger.cjs', diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index a8ebff59f..df7bd5e41 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -207,6 +207,27 @@ const configKeys = { "workflow.ui_safety_gate": "ui" }; +const configSchema = { + "workflow.ui_phase": { + "owner": "ui", + "type": "boolean", + "default": true, + "description": "Enable the UI design-contract gate during planning." + }, + "workflow.ui_review": { + "owner": "ui", + "type": "boolean", + "default": true, + "description": "Enable the retrospective UI audit." + }, + "workflow.ui_safety_gate": { + "owner": "ui", + "type": "boolean", + "default": true, + "description": "Block execution on unmet UI-SPEC contracts." + } +}; + const runtimes = {}; const _requiresGraph = { @@ -236,6 +257,7 @@ module.exports = { byAgent, byLoopPoint, configKeys, + configSchema, runtimes, requiresClosure, }; diff --git a/scripts/gen-capability-registry.cjs b/scripts/gen-capability-registry.cjs index 36b2755ad..a9cf1861d 100644 --- a/scripts/gen-capability-registry.cjs +++ b/scripts/gen-capability-registry.cjs @@ -105,6 +105,100 @@ function loadCentralConfigKeys() { } } +// ─── Config-slice validation ────────────────────────────────────────────────── + +const VALID_CONFIG_SLICE_TYPES = new Set(['boolean', 'string', 'number', 'enum']); + +/** + * Validate a single config-slice entry (one key's { type, default, description }). + * Returns an array of error strings. Empty = valid. + * + * @param {string} capId Capability id (for error messages) + * @param {string} key Config key (for error messages) + * @param {object} slice The slice object from cap.config[key] + * @returns {string[]} + */ +function validateConfigSliceEntry(capId, key, slice) { + const errors = []; + + if (typeof slice !== 'object' || slice === null || Array.isArray(slice)) { + errors.push('capability "' + capId + '" config["' + key + '"]: slice must be a non-null object'); + return errors; + } + + // type must be one of the allowed set + if (!VALID_CONFIG_SLICE_TYPES.has(slice.type)) { + errors.push( + 'capability "' + capId + '" config["' + key + '"]: type must be one of ' + + [...VALID_CONFIG_SLICE_TYPES].join(', ') + ' (got: ' + JSON.stringify(slice.type) + ')', + ); + } + + // default must be present + if (!Object.prototype.hasOwnProperty.call(slice, 'default')) { + errors.push( + 'capability "' + capId + '" config["' + key + '"]: default is required', + ); + } else { + // type-consistency check + const def = slice.default; + if (slice.type === 'boolean') { + if (typeof def !== 'boolean') { + errors.push( + 'capability "' + capId + '" config["' + key + '"]: default must be a boolean for type:"boolean" (got: ' + typeof def + ')', + ); + } + } else if (slice.type === 'string') { + if (typeof def !== 'string') { + errors.push( + 'capability "' + capId + '" config["' + key + '"]: default must be a string for type:"string" (got: ' + typeof def + ')', + ); + } + } else if (slice.type === 'number') { + if (typeof def !== 'number') { + errors.push( + 'capability "' + capId + '" config["' + key + '"]: default must be a number for type:"number" (got: ' + typeof def + ')', + ); + } else if (!Number.isFinite(def)) { + // FIX 6a: Reject NaN and non-finite number defaults + errors.push( + 'capability "' + capId + '" config["' + key + '"]: default for type:"number" must be a finite number (got: ' + String(def) + ')', + ); + } + } else if (slice.type === 'enum') { + // FIX 5a: enum REQUIRES a non-empty values array (all strings), and default must be in it + if (!Array.isArray(slice.values) || slice.values.length === 0) { + errors.push( + 'capability "' + capId + '" config["' + key + '"]: type:"enum" requires a non-empty "values" array of strings', + ); + } else if (!slice.values.every((v) => typeof v === 'string')) { + errors.push( + 'capability "' + capId + '" config["' + key + '"]: type:"enum" values array must contain only strings', + ); + } + if (typeof def !== 'string') { + errors.push( + 'capability "' + capId + '" config["' + key + '"]: default must be a string for type:"enum" (got: ' + typeof def + ')', + ); + } else if (Array.isArray(slice.values) && slice.values.length > 0 && !slice.values.includes(def)) { + errors.push( + 'capability "' + capId + '" config["' + key + '"]: default "' + def + + '" is not one of the declared enum values [' + slice.values.join(', ') + ']', + ); + } + } + } + + // description must be a non-empty string + if (typeof slice.description !== 'string' || slice.description.length === 0) { + errors.push( + 'capability "' + capId + '" config["' + key + '"]: description must be a non-empty string (got: ' + JSON.stringify(slice.description) + ')', + ); + } + + return errors; +} + // ─── Per-capability validation ──────────────────────────────────────────────── const KEBAB_RE = /^[a-z][a-z0-9-]*$/; @@ -951,6 +1045,7 @@ function buildRegistry(capMap) { const byAgent = Object.create(null); const byLoopPoint = Object.create(null); const configKeys = Object.create(null); + const configSchema = Object.create(null); const runtimes = Object.create(null); // Initialize byLoopPoint for all valid points @@ -989,6 +1084,29 @@ function buildRegistry(capMap) { // S2b: inline literal guard at each write site (CodeQL barrier) if (key === '__proto__' || key === 'constructor' || key === 'prototype') continue; configKeys[key] = capId; + + // Build configSchema entry — validate the slice first (throw on violation) + const slice = (cap.config || {})[key]; + const sliceErrors = validateConfigSliceEntry(capId, key, slice); + if (sliceErrors.length > 0) { + throw new Error( + 'configSchema validation failed during registry build:\n' + + sliceErrors.map((e) => ' ' + e).join('\n'), + ); + } + // S2b: inline literal guard for configSchema write site + if (key !== '__proto__' && key !== 'constructor' && key !== 'prototype') { + configSchema[key] = { + owner: capId, + type: slice.type, + default: slice.default, + description: slice.description, + }; + // Preserve values array for enum types if present + if (slice.type === 'enum' && Array.isArray(slice.values)) { + configSchema[key].values = slice.values; + } + } } for (const step of (cap.steps || [])) { @@ -1050,6 +1168,7 @@ function buildRegistry(capMap) { byAgent, byLoopPoint, configKeys, + configSchema, runtimes, }; } @@ -1085,6 +1204,8 @@ function serializeRegistry(registry, capMap) { lines.push(''); lines.push('const configKeys = ' + JSON.stringify(registry.configKeys, null, 2) + ';'); lines.push(''); + lines.push('const configSchema = ' + JSON.stringify(registry.configSchema, null, 2) + ';'); + lines.push(''); lines.push('const runtimes = ' + JSON.stringify(registry.runtimes, null, 2) + ';'); lines.push(''); @@ -1121,6 +1242,7 @@ function serializeRegistry(registry, capMap) { lines.push(' byAgent,'); lines.push(' byLoopPoint,'); lines.push(' configKeys,'); + lines.push(' configSchema,'); lines.push(' runtimes,'); lines.push(' requiresClosure,'); lines.push('};'); @@ -1280,6 +1402,8 @@ module.exports = { computeRequiresClosure, topoSortSteps, normalizeLineEndings, + validateConfigSliceEntry, + VALID_CONFIG_SLICE_TYPES, LOOP_HOST_CONTRACT, VALID_LOOP_POINTS, POINT_ORDER, diff --git a/src/config-loader.cts b/src/config-loader.cts index 67982afee..942c9fd3c 100644 --- a/src/config-loader.cts +++ b/src/config-loader.cts @@ -35,8 +35,30 @@ const { detectSubRepos } = coreUtilsModule; import { CONFIG_DEFAULTS as CANONICAL_CONFIG_DEFAULTS, normalizeLegacyKeys } from './configuration.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import configSchema = require('./config-schema.cjs'); -const { VALID_CONFIG_KEYS, DYNAMIC_KEY_PATTERNS } = configSchema; +const { VALID_CONFIG_KEYS, DYNAMIC_KEY_PATTERNS, isValidConfigKey: _isValidConfigKeyFn } = configSchema; import { KNOWN_RUNTIMES, KNOWN_PROVIDERS } from './model-catalog.cjs'; +// ─── Federated Config (ADR-857 phase 3b) ───────────────────────────────────── +// eslint-disable-next-line @typescript-eslint/no-require-imports +import federatedConfigModule = require('./federated-config.cjs'); +const { mergeFederatedConfig } = federatedConfigModule; +// The capability-registry.cjs is generated and lives in the same gsd-core/bin/lib/ output dir. +// Both config-loader.cjs and capability-registry.cjs land in gsd-core/bin/lib/ at build time. +// eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-unsafe-assignment +const _capabilityRegistryReal: { configSchema?: Record } = require('./capability-registry.cjs'); + +// Module-level registry reference. Defaults to the real generated registry. +// Overridable for tests via _setFederatedRegistryForTests. +let _capabilityRegistry: { configSchema?: Record } = _capabilityRegistryReal; + +/** Test-only seam: inject a synthetic registry. Call _resetFederatedRegistryForTests() to restore. */ +function _setFederatedRegistryForTests(reg: { configSchema?: Record }): void { + _capabilityRegistry = reg; +} + +/** Test-only seam: restore the real generated registry. */ +function _resetFederatedRegistryForTests(): void { + _capabilityRegistry = _capabilityRegistryReal; +} // ─── File & Config utilities ────────────────────────────────────────────────── @@ -272,6 +294,81 @@ function _resetRuntimeWarningCacheForTests(): void { _warnedConfigKeys.clear(); } +// ─── FIX 2: Federated overlay helpers ──────────────────────────────────────── + +/** + * Apply federated key values into a mutable config object. + * Handles N-level dotted keys (e.g. "a.b.c" → obj.a.b.c). + * Only adds keys that are not already present (does not clobber). + * Inline prototype-pollution guards at every segment. + */ +function _applyFederatedValues( + obj: Record, + values: Record, + validKeys: string[], +): void { + for (const dottedKey of validKeys) { + // S2: inline literal guard on full key + if (dottedKey === '__proto__' || dottedKey === 'constructor' || dottedKey === 'prototype') continue; + const parts = dottedKey.split('.'); + if (parts.length === 1) { + const topKey = parts[0]; + if (topKey !== '__proto__' && topKey !== 'constructor' && topKey !== 'prototype') { + if (!Object.prototype.hasOwnProperty.call(obj, topKey)) { + obj[topKey] = values[dottedKey]; + } + } + } else { + // N-level nested key: traverse/create intermediate objects + let cur: Record = obj; + let ok = true; + for (let i = 0; i < parts.length - 1; i++) { + const seg = parts[i]; + // S2: inline literal guard on each segment + if (seg === '__proto__' || seg === 'constructor' || seg === 'prototype') { ok = false; break; } + if (!Object.prototype.hasOwnProperty.call(cur, seg) || cur[seg] === null) { + cur[seg] = {}; + } + if (typeof cur[seg] !== 'object' || Array.isArray(cur[seg])) { ok = false; break; } + cur = cur[seg] as Record; + } + if (!ok) continue; + const leafKey = parts[parts.length - 1]; + // S2: inline literal guard on leaf + if (leafKey === '__proto__' || leafKey === 'constructor' || leafKey === 'prototype') continue; + if (!Object.prototype.hasOwnProperty.call(cur, leafKey)) { + cur[leafKey] = values[dottedKey]; + } + } + } +} + +/** + * FIX 2: Apply the federated overlay to a base config object. + * When validKeys is empty (current registry — all keys are central), + * returns the baseConfig UNCHANGED (true no-op, preserves reference identity). + * When validKeys is non-empty, applies values into a shallow clone to avoid + * mutating shared CONFIG_DEFAULTS/module constants. + */ +function _applyFederatedOverlay( + baseConfig: Record, + userConfig: Record, +): Record { + const _fedRegistrySchema = _capabilityRegistry.configSchema; + if (!_fedRegistrySchema || typeof _fedRegistrySchema !== 'object') return baseConfig; + const _fedOverlay = mergeFederatedConfig({ + configSchema: _fedRegistrySchema, + isCentralKey: (key: string) => _isValidConfigKeyFn(key), + userConfig, + }); + // True no-op: if no federated keys, return UNCHANGED (byte-identical, no clone) + if (_fedOverlay.validKeys.length === 0) return baseConfig; + // Clone shallowly to avoid mutating shared constants, then apply nested values + const cloned: Record = { ...baseConfig }; + _applyFederatedValues(cloned, _fedOverlay.values, _fedOverlay.validKeys); + return cloned; +} + function loadConfig(cwd: string, options: Record = {}): Record { const activeWorkstream = Object.prototype.hasOwnProperty.call(options, 'workstream') ? options['workstream'] @@ -397,6 +494,31 @@ function loadConfig(cwd: string, options: Record = {}): Record< // Deprecated keys (still accepted for migration, not in config-set) 'depth', 'multiRepo', 'branching_strategy', ]); + + // FIX 3: Compute federated overlay BEFORE the unknown-key warning, so that + // federated top-level keys are added to KNOWN_TOP_LEVEL before the check runs. + // This is hoisted out of the try-catch below so validKeys are available here. + let _preWarningFedValidKeys: string[] = []; + try { + const _fedRegistrySchemaEarly = _capabilityRegistry.configSchema; + if (_fedRegistrySchemaEarly && typeof _fedRegistrySchemaEarly === 'object') { + const _earlyOverlay = mergeFederatedConfig({ + configSchema: _fedRegistrySchemaEarly, + isCentralKey: (key: string) => _isValidConfigKeyFn(key), + userConfig: parsed, + }); + _preWarningFedValidKeys = _earlyOverlay.validKeys; + for (const dottedKey of _preWarningFedValidKeys) { + const topKey = dottedKey.split('.')[0]; + if (topKey !== '__proto__' && topKey !== 'constructor' && topKey !== 'prototype') { + KNOWN_TOP_LEVEL.add(topKey); + } + } + } + } catch { + // Defensive: if registry access fails here, proceed without pre-warning keys + } + const unknownKeys = Object.keys(parsed).filter(k => !KNOWN_TOP_LEVEL.has(k)); if (unknownKeys.length > 0) { const warnKey = unknownKeys.join(','); @@ -429,7 +551,7 @@ function loadConfig(cwd: string, options: Record = {}): Record< return defaults.parallelization; })(); - return { + const _baseConfig: Record = { model_profile: get('model_profile') ?? defaults.model_profile, commit_docs: (() => { const explicit = get('commit_docs', { section: 'planning', field: 'commit_docs' }); @@ -492,14 +614,54 @@ function loadConfig(cwd: string, options: Record = {}): Record< claude_md_path: get('claude_md_path') || null, claude_md_assembly: (parsed['claude_md_assembly']) || null, }; + + // ─── ADR-857 phase 3b: federated config overlay ─────────────────────────── + // FIX 2: Use the pre-computed _preWarningFedValidKeys (from the FIX 3 block above) + // plus a fresh overlay call to get values. The KNOWN_TOP_LEVEL was already updated. + // TODAY: every UI key is still in the central config-schema, so isCentralKey() + // returns true for all of them → validKeys is empty → _baseConfig is returned UNCHANGED + // (true no-op: no clone, no reorder, byte-identical output). + // This becomes a live channel once a key is atomically removed from the central schema. + try { + if (_preWarningFedValidKeys.length > 0) { + // There are actual federated values — re-use the already-computed overlay + // (we run mergeFederatedConfig again here to get the values map; the validKeys + // are guaranteed identical since it's the same inputs). + const _fedRegistrySchema = _capabilityRegistry.configSchema; + if (_fedRegistrySchema && typeof _fedRegistrySchema === 'object') { + const _fedOverlay = mergeFederatedConfig({ + configSchema: _fedRegistrySchema, + isCentralKey: (key: string) => _isValidConfigKeyFn(key), + userConfig: parsed, + }); + // Apply dotted-path values (e.g. "workflow.ui_phase" → _baseConfig.workflow.ui_phase) + // WITHOUT clobbering existing keys. N-level nesting supported. + _applyFederatedValues(_baseConfig, _fedOverlay.values, _fedOverlay.validKeys); + } + } + // Pending-migration warnings are suppressed at load time to avoid noisy output on + // every loadConfig call. They are surfaced at registry-generation time (--check/--write). + } catch { + // Defensive: if the federated overlay throws for any reason, return the base config unchanged. + // This keeps loadConfig's no-throw contract intact regardless of capability registry state. + } + return _baseConfig; } catch { // Fall back to ~/.gsd/defaults.json only for truly pre-project contexts (#1683) if (fs.existsSync(planningDir(cwd, ws))) { if (rootParsed) { // Workstream has no config.json: re-parse using root config as the sole source. + // (FIX 2: overlay is applied recursively in the re-entrant loadConfig call) return loadConfig(cwd, { workstream: null }); } - return defaults; + // FIX 2: Apply the federated overlay on the no-config path. + // With the current registry (all keys central), _applyFederatedOverlay returns + // `defaults` UNCHANGED (true no-op, preserves byte-identical output). + try { + return _applyFederatedOverlay(defaults, {}); + } catch { + return defaults; + } } try { const home = process.env['GSD_HOME'] || os.homedir(); @@ -507,7 +669,7 @@ function loadConfig(cwd: string, options: Record = {}): Record< const raw = platformReadSync(globalDefaultsPath); if (raw === null) throw new Error('missing'); const globalDefaults = JSON.parse(raw) as Record; - return { + const _globalBaseCfg: Record = { ...defaults, model_profile: (globalDefaults['model_profile']) ?? defaults.model_profile, commit_docs: (globalDefaults['commit_docs']) ?? defaults.commit_docs, @@ -534,8 +696,21 @@ function loadConfig(cwd: string, options: Record = {}): Record< agent_skills: (globalDefaults['agent_skills']) || {}, response_language: (globalDefaults['response_language']) || null, }; + // FIX 2: Apply federated overlay on global-defaults path. + // With the current registry this is a true no-op (returns _globalBaseCfg unchanged). + try { + return _applyFederatedOverlay(_globalBaseCfg, globalDefaults); + } catch { + return _globalBaseCfg; + } } catch { - return defaults; + // FIX 2: Apply federated overlay on the final fallback path. + // With the current registry this is a true no-op (returns `defaults` unchanged). + try { + return _applyFederatedOverlay(defaults, {}); + } catch { + return defaults; + } } } } @@ -553,4 +728,6 @@ export = { _warnedConfigKeys, _gitIgnoredCache, RUNTIME_OVERRIDE_TIERS, + _setFederatedRegistryForTests, + _resetFederatedRegistryForTests, }; diff --git a/src/federated-config.cts b/src/federated-config.cts new file mode 100644 index 000000000..8ca0bac49 --- /dev/null +++ b/src/federated-config.cts @@ -0,0 +1,230 @@ +/** + * Federated Config — Defensive merge of capability-declared config keys + * + * ADR-857 phase 3b: wires the Capability Registry's configSchema into + * loadConfig as a provably-empty no-op channel until capability keys are + * migrated out of the central config-schema. + * + * Exported function: + * mergeFederatedConfig({ configSchema, isCentralKey, userConfig }) + * → { values, validKeys, warnings } + * + * Design: + * - For each key in configSchema: + * If isCentralKey(key) → SKIP; push a pending-migration warning. + * Else if slice is malformed → SKIP; push a warning. Never throw. + * Else (valid federated key absent from central): + * resolvedValue = nested userConfig lookup if present & type-matches; else slice.default. + * Add key→resolvedValue to values; add key to validKeys. + * - Guard all object writes with inline literal __proto__/constructor/prototype checks. + * - Zero external dependencies; no ajv; hand-rolled type checks only. + * + * ADR-857 no-op guarantee: + * With the current registry, every UI key is still present in the central + * config-schema, so isCentralKey() returns true for all of them and values + * is always empty. The channel is live but carries no traffic until a key + * is atomically removed from the central schema (the cutover step). + * + * Dependencies: none (zero-dep module). + */ + +// ─── Types ───────────────────────────────────────────────────────────────────── + +/** Shape of one entry in the capability registry's configSchema index. */ +interface ConfigSliceEntry { + owner: string; + type: string; + default: unknown; + description: string; + values?: string[]; // required for enum type + [key: string]: unknown; +} + +interface MergeFederatedConfigInput { + /** configSchema index from the capability registry: { [key]: ConfigSliceEntry } */ + configSchema: Record; + /** Returns true if the given key is owned by the central config-schema. */ + isCentralKey: (key: string) => boolean; + /** The raw/merged user config object already loaded in loadConfig. */ + userConfig: Record; +} + +interface MergeFederatedConfigResult { + /** Resolved values for federated (non-central) keys: { key → resolvedValue } */ + values: Record; + /** Array of keys that are now valid federated keys (i.e. were added to values). */ + validKeys: string[]; + /** Human-readable diagnostic strings (pending-migration, malformed-slice, type-mismatch). */ + warnings: string[]; +} + +// ─── Allowed slice types (mirrors gen-capability-registry.cjs VALID_CONFIG_SLICE_TYPES) ── + +const VALID_SLICE_TYPES = new Set(['boolean', 'string', 'number', 'enum']); + +// ─── Internal helpers ────────────────────────────────────────────────────────── + +/** + * Returns true if `slice` has a non-empty type, a `default` property, and a + * non-empty string description. Does NOT throw. + */ +function _isWellFormedSlice(slice: unknown): slice is ConfigSliceEntry { + if (typeof slice !== 'object' || slice === null || Array.isArray(slice)) return false; + const s = slice as Record; + if (typeof s['type'] !== 'string' || s['type'].length === 0) return false; + if (!VALID_SLICE_TYPES.has(s['type'])) return false; + if (!Object.prototype.hasOwnProperty.call(s, 'default')) return false; + return true; +} + +/** + * Returns true if `value` matches the declared type in the slice. + * For enum, also validates against slice.values if present. + */ +function _typeMatches(value: unknown, slice: ConfigSliceEntry): boolean { + switch (slice.type) { + case 'boolean': return typeof value === 'boolean'; + case 'string': return typeof value === 'string'; + case 'number': return typeof value === 'number'; + case 'enum': + // Must be a string AND, if values list is present, must be in it + if (typeof value !== 'string') return false; + if (Array.isArray(slice.values) && slice.values.length > 0) { + return slice.values.includes(value); + } + return true; + default: return false; + } +} + +/** + * Traverse a dotted key path through a nested config object. + * E.g. key="workflow.ui_phase", obj={workflow:{ui_phase:false}} → {found:true, value:false} + * Returns {found:false} if any segment is missing or not an own property. + * Handles 1, 2, or N segments generically. + */ +function _getNestedValue(obj: Record, key: string): { found: boolean; value: unknown } { + const segments = key.split('.'); + let current: unknown = obj; + for (let i = 0; i < segments.length; i++) { + const seg = segments[i]; + // Inline literal prototype-pollution guard + if (seg === '__proto__' || seg === 'constructor' || seg === 'prototype') { + return { found: false, value: undefined }; + } + if (typeof current !== 'object' || current === null) { + return { found: false, value: undefined }; + } + const cur = current as Record; + if (!Object.prototype.hasOwnProperty.call(cur, seg)) { + return { found: false, value: undefined }; + } + current = cur[seg]; + } + return { found: true, value: current }; +} + +// ─── Public API ──────────────────────────────────────────────────────────────── + +/** + * Defensive merge of capability-declared config slices into the loadConfig + * return value. + * + * DEFENSIVE contract (never throws, even on bad capability data): + * - Null/undefined/non-object input → returns empty result. + * - Null/undefined/non-object userConfig → treated as {} (no overrides). + * - Central keys are skipped with a pending-migration warning. + * - Malformed slices are skipped with a warning. + * - User-supplied values with wrong types (or out-of-enum values) fall back + * to the slice default (a type-mismatch warning is pushed but the key is + * still federated with its default; this is best-effort degraded operation). + */ +function mergeFederatedConfig(input: MergeFederatedConfigInput): MergeFederatedConfigResult { + // FIX 4: Guard null/undefined/non-object input + if (input === null || input === undefined || typeof input !== 'object') { + return { values: Object.create(null) as Record, validKeys: [], warnings: [] }; + } + + const { configSchema, isCentralKey } = input; + + // FIX 4: Guard null/undefined/non-object userConfig — treat as {} + const userConfig: Record = + (input.userConfig !== null && input.userConfig !== undefined && typeof input.userConfig === 'object' && !Array.isArray(input.userConfig)) + ? input.userConfig + : {}; + + // FIX 6b: Use null-prototype object for all return paths + const values: Record = Object.create(null) as Record; + const validKeys: string[] = []; + const warnings: string[] = []; + + if (typeof configSchema !== 'object' || configSchema === null) { + return { values: Object.create(null) as Record, validKeys: [], warnings: [] }; + } + + for (const key of Object.keys(configSchema)) { + // S2: inline literal prototype-pollution guard (CodeQL barrier) + // Guard both the full key AND all dotted-path segments + if (key === '__proto__' || key === 'constructor' || key === 'prototype') continue; + const _keySegments = key.split('.'); + if (_keySegments.some((s) => s === '__proto__' || s === 'constructor' || s === 'prototype')) continue; + + const slice = configSchema[key]; + + // If this key is still in the central schema → pending migration, skip + try { + if (isCentralKey(key)) { + warnings.push( + 'federated-config: key "' + key + '" is still in the central config-schema (pending-migration); ' + + 'skipping federated resolution until the central schema entry is removed', + ); + continue; + } + } catch { + // isCentralKey threw — treat as unknown, skip defensively + warnings.push('federated-config: isCentralKey("' + key + '") threw; skipping key'); + continue; + } + + // Validate slice shape — skip malformed entries + if (!_isWellFormedSlice(slice)) { + warnings.push( + 'federated-config: config slice for key "' + key + '" is malformed (missing or invalid type/default); skipping', + ); + continue; + } + + const sliceEntry = slice; + + // FIX 1: Resolve value using NESTED dotted-path lookup through userConfig + let resolvedValue: unknown = sliceEntry.default; + const { found: userHasKey, value: userValue } = _getNestedValue(userConfig, key); + if (userHasKey && userValue !== undefined) { + // FIX 5b: For enum, validate against slice.values if present; otherwise check type + if (_typeMatches(userValue, sliceEntry)) { + resolvedValue = userValue; + } else { + const typeDesc = sliceEntry.type === 'enum' && Array.isArray(sliceEntry.values) + ? 'enum(' + sliceEntry.values.join('|') + ')' + : sliceEntry.type; + warnings.push( + 'federated-config: user-supplied value for "' + key + '" has wrong type or invalid enum value ' + + '(expected ' + typeDesc + ', got ' + typeof userValue + + (typeof userValue === 'string' ? ' "' + String(userValue) + '"' : '') + + '); falling back to slice default', + ); + // resolvedValue stays as slice default + } + } + + // S2: inline literal guard before writing to values + if (key !== '__proto__' && key !== 'constructor' && key !== 'prototype') { + values[key] = resolvedValue; + validKeys.push(key); + } + } + + return { values, validKeys, warnings }; +} + +export = { mergeFederatedConfig }; diff --git a/tests/capability-registry.test.cjs b/tests/capability-registry.test.cjs index 7c2994d5f..fb204587a 100644 --- a/tests/capability-registry.test.cjs +++ b/tests/capability-registry.test.cjs @@ -29,6 +29,8 @@ const { computeRequiresClosure, topoSortSteps, normalizeLineEndings, + validateConfigSliceEntry, + VALID_CONFIG_SLICE_TYPES, SCHEMA_VERSION, } = require('../scripts/gen-capability-registry.cjs'); @@ -101,10 +103,33 @@ describe('UI pilot capability', () => { assert.strictEqual(uiGate.capId, 'ui'); assert.strictEqual(uiGate.blocking, true); - // configKeys maps the 3 UI keys to 'ui' + // configKeys maps the 3 UI keys to 'ui' (ownership map — preserved) assert.strictEqual(registry.configKeys['workflow.ui_phase'], 'ui'); assert.strictEqual(registry.configKeys['workflow.ui_review'], 'ui'); assert.strictEqual(registry.configKeys['workflow.ui_safety_gate'], 'ui'); + + // configSchema index — new in phase 3b + assert.ok(registry.configSchema, 'registry.configSchema should exist'); + + // workflow.ui_phase + assert.ok(registry.configSchema['workflow.ui_phase'], 'configSchema should have workflow.ui_phase'); + assert.strictEqual(registry.configSchema['workflow.ui_phase'].owner, 'ui'); + assert.strictEqual(registry.configSchema['workflow.ui_phase'].type, 'boolean'); + assert.strictEqual(registry.configSchema['workflow.ui_phase'].default, true); + assert.strictEqual(typeof registry.configSchema['workflow.ui_phase'].description, 'string'); + assert.ok(registry.configSchema['workflow.ui_phase'].description.length > 0); + + // workflow.ui_review + assert.ok(registry.configSchema['workflow.ui_review'], 'configSchema should have workflow.ui_review'); + assert.strictEqual(registry.configSchema['workflow.ui_review'].owner, 'ui'); + assert.strictEqual(registry.configSchema['workflow.ui_review'].type, 'boolean'); + assert.strictEqual(registry.configSchema['workflow.ui_review'].default, true); + + // workflow.ui_safety_gate + assert.ok(registry.configSchema['workflow.ui_safety_gate'], 'configSchema should have workflow.ui_safety_gate'); + assert.strictEqual(registry.configSchema['workflow.ui_safety_gate'].owner, 'ui'); + assert.strictEqual(registry.configSchema['workflow.ui_safety_gate'].type, 'boolean'); + assert.strictEqual(registry.configSchema['workflow.ui_safety_gate'].default, true); }); test('requiresClosure("ui") returns empty set (no requires)', () => { @@ -1200,6 +1225,7 @@ describe('FIX 1: self-consume rejection in validateConsumesGlobal', () => { ); }); + // (this test follows the series above) test('a step produces:["SELF.md"] and consumes:["SELF.md"] and another capability produces SELF.md at the SAME point is accepted (different hook)', () => { const producerCap = { id: 'producer-cap', role: 'feature', title: 'Producer', description: 'Produces SELF.md', @@ -1240,3 +1266,241 @@ describe('FIX 1: self-consume rejection in validateConsumesGlobal', () => { ); }); }); + +// ─── 14. configSchema emission (ADR-857 phase 3b) ──────────────────────────── + +describe('configSchema emission (ADR-857 phase 3b)', () => { + test('buildRegistry emits configSchema with correct shape for UI pilot', () => { + const capDir = makeTempCapDir({ ui: UI_CAP }); + const { capMap, errors } = loadAndValidate(new Set(), capDir); + assert.deepEqual(errors, [], 'No errors expected'); + + const registry = buildRegistry(capMap); + assert.ok(registry.configSchema, 'registry.configSchema must exist'); + + const uiPhase = registry.configSchema['workflow.ui_phase']; + assert.ok(uiPhase, 'configSchema must have workflow.ui_phase'); + assert.strictEqual(uiPhase.owner, 'ui'); + assert.strictEqual(uiPhase.type, 'boolean'); + assert.strictEqual(uiPhase.default, true); + assert.ok(typeof uiPhase.description === 'string' && uiPhase.description.length > 0); + + const uiReview = registry.configSchema['workflow.ui_review']; + assert.ok(uiReview, 'configSchema must have workflow.ui_review'); + assert.strictEqual(uiReview.owner, 'ui'); + assert.strictEqual(uiReview.type, 'boolean'); + + const uiSafetyGate = registry.configSchema['workflow.ui_safety_gate']; + assert.ok(uiSafetyGate, 'configSchema must have workflow.ui_safety_gate'); + assert.strictEqual(uiSafetyGate.type, 'boolean'); + }); + + test('serializeRegistry emits a configSchema block in the generated .cjs', () => { + const capDir = makeTempCapDir({ ui: UI_CAP }); + const { capMap } = loadAndValidate(new Set(), capDir); + const registry = buildRegistry(capMap); + const content = serializeRegistry(registry, capMap); + + assert.ok(content.includes('const configSchema'), 'Generated file must contain "const configSchema"'); + assert.ok(content.includes('"workflow.ui_phase"'), 'Generated file must contain "workflow.ui_phase"'); + assert.ok(content.includes('"owner"'), 'Generated file must contain "owner" field'); + assert.ok(content.includes('"type"'), 'Generated file must contain "type" field'); + assert.ok(content.includes('"default"'), 'Generated file must contain "default" field'); + assert.ok(content.includes('"description"'), 'Generated file must contain "description" field'); + assert.ok(content.includes('configSchema,'), 'Generated module.exports must include configSchema'); + }); + + test('committed capability-registry.cjs has configSchema with correct shape', () => { + const registry = require('../gsd-core/bin/lib/capability-registry.cjs'); + assert.ok(registry.configSchema, 'capability-registry.cjs must export configSchema'); + + const uiPhase = registry.configSchema['workflow.ui_phase']; + assert.ok(uiPhase, 'committed registry configSchema must have workflow.ui_phase'); + assert.strictEqual(uiPhase.owner, 'ui', 'owner must be "ui"'); + assert.strictEqual(uiPhase.type, 'boolean', 'type must be "boolean"'); + assert.strictEqual(uiPhase.default, true, 'default must be true'); + assert.ok(typeof uiPhase.description === 'string' && uiPhase.description.length > 0); + }); +}); + +// ─── 15. validateConfigSliceEntry adversarial tests ─────────────────────────── + +describe('validateConfigSliceEntry adversarial cases (ADR-857 phase 3b)', () => { + const CAP_ID = 'test-cap'; + const KEY = 'test.key'; + + test('VALID_CONFIG_SLICE_TYPES exports expected types', () => { + const types = [...VALID_CONFIG_SLICE_TYPES]; + assert.ok(types.includes('boolean'), 'Must include boolean'); + assert.ok(types.includes('string'), 'Must include string'); + assert.ok(types.includes('number'), 'Must include number'); + assert.ok(types.includes('enum'), 'Must include enum'); + assert.strictEqual(types.length, 4, 'Must have exactly 4 types'); + }); + + test('valid boolean slice passes validation', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'boolean', default: true, description: 'ok' }); + assert.deepEqual(errors, [], 'Valid boolean slice should produce no errors, got: ' + JSON.stringify(errors)); + }); + + test('valid string slice passes validation', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'string', default: 'x', description: 'ok' }); + assert.deepEqual(errors, []); + }); + + test('valid number slice passes validation', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'number', default: 5, description: 'ok' }); + assert.deepEqual(errors, []); + }); + + test('REJECTED: enum slice without values list → error (FIX 5a: values required)', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'enum', default: 'x', description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for enum without values list, got: ' + JSON.stringify(errors)); + assert.ok( + errors.some((e) => e.includes('values') || e.includes('enum')), + 'Error should mention values or enum, got: ' + JSON.stringify(errors), + ); + }); + + test('valid enum slice (with values list, default in values) passes', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { + type: 'enum', default: 'b', values: ['a', 'b', 'c'], description: 'ok', + }); + assert.deepEqual(errors, []); + }); + + test('REJECTED: bad type ("xml") → error mentioning type', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'xml', default: '', description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for bad type'); + assert.ok(errors.some((e) => e.includes('type')), 'Error should mention type, got: ' + JSON.stringify(errors)); + }); + + test('REJECTED: missing type → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { default: true, description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for missing type'); + assert.ok(errors.some((e) => e.includes('type'))); + }); + + test('REJECTED: missing default → error mentioning default', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'boolean', description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for missing default'); + assert.ok(errors.some((e) => e.includes('default')), 'Error should mention default, got: ' + JSON.stringify(errors)); + }); + + test('REJECTED: boolean type with string default → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'boolean', default: 'true', description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for boolean type with string default'); + assert.ok(errors.some((e) => e.includes('boolean') || e.includes('default'))); + }); + + test('REJECTED: string type with boolean default → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'string', default: false, description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for string type with boolean default'); + }); + + test('REJECTED: number type with string default → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'number', default: 'five', description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for number type with string default'); + }); + + test('REJECTED: enum type with values list, default not in values → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { + type: 'enum', default: 'z', values: ['a', 'b', 'c'], description: 'ok', + }); + assert.ok(errors.length > 0, 'Expected rejection for enum default not in values'); + assert.ok(errors.some((e) => e.includes('enum') || e.includes('values') || e.includes('z'))); + }); + + test('REJECTED: enum type with non-string default → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'enum', default: 42, description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for enum with non-string default'); + }); + + test('REJECTED: empty description string → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'boolean', default: true, description: '' }); + assert.ok(errors.length > 0, 'Expected rejection for empty description'); + assert.ok(errors.some((e) => e.includes('description'))); + }); + + test('REJECTED: non-string description (number) → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'boolean', default: true, description: 42 }); + assert.ok(errors.length > 0, 'Expected rejection for non-string description'); + assert.ok(errors.some((e) => e.includes('description'))); + }); + + test('REJECTED: missing description → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'boolean', default: true }); + assert.ok(errors.length > 0, 'Expected rejection for missing description'); + assert.ok(errors.some((e) => e.includes('description'))); + }); + + test('REJECTED: null slice → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, null); + assert.ok(errors.length > 0, 'Expected rejection for null slice'); + }); + + test('REJECTED: array slice → error', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, []); + assert.ok(errors.length > 0, 'Expected rejection for array slice'); + }); + + // FIX 5a: enum-without-values and default-not-in-values + test('REJECTED: enum with empty values array → error (FIX 5a)', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'enum', default: 'x', values: [], description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for enum with empty values, got: ' + JSON.stringify(errors)); + assert.ok(errors.some((e) => e.includes('values') || e.includes('enum'))); + }); + + test('REJECTED: enum with non-string values array entries → error (FIX 5a)', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'enum', default: 'x', values: ['a', 42], description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for enum with non-string values, got: ' + JSON.stringify(errors)); + assert.ok(errors.some((e) => e.includes('values') || e.includes('string'))); + }); + + test('REJECTED: enum default not in values → error (FIX 5a)', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'enum', default: 'z', values: ['a', 'b'], description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for enum default not in values, got: ' + JSON.stringify(errors)); + assert.ok(errors.some((e) => e.includes('z') || e.includes('values') || e.includes('default'))); + }); + + // FIX 6a: NaN and non-finite number defaults + test('REJECTED: NaN number default → error (FIX 6a)', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'number', default: NaN, description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for NaN default, got: ' + JSON.stringify(errors)); + assert.ok(errors.some((e) => e.includes('finite') || e.includes('NaN') || e.includes('number'))); + }); + + test('REJECTED: Infinity number default → error (FIX 6a)', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'number', default: Infinity, description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for Infinity default, got: ' + JSON.stringify(errors)); + assert.ok(errors.some((e) => e.includes('finite') || e.includes('number'))); + }); + + test('REJECTED: -Infinity number default → error (FIX 6a)', () => { + const errors = validateConfigSliceEntry(CAP_ID, KEY, { type: 'number', default: -Infinity, description: 'ok' }); + assert.ok(errors.length > 0, 'Expected rejection for -Infinity default, got: ' + JSON.stringify(errors)); + }); + + test('buildRegistry throws on malformed config slice in capability', () => { + // A capability with a config slice that has a missing default — buildRegistry must throw + const cap = { + ...UI_CAP, + config: { + ...UI_CAP.config, + 'workflow.bad_key': { type: 'boolean', description: 'missing default' }, + }, + }; + const capMap = new Map([['ui', cap]]); + assert.throws( + () => buildRegistry(capMap), + (err) => { + assert.ok(err instanceof Error, 'Must throw an Error'); + assert.ok( + err.message.includes('configSchema') || err.message.includes('default') || err.message.includes('validation'), + 'Error must mention configSchema validation, got: ' + err.message, + ); + return true; + }, + ); + }); +}); diff --git a/tests/federated-config-loadconfig.test.cjs b/tests/federated-config-loadconfig.test.cjs new file mode 100644 index 000000000..36e7f8134 --- /dev/null +++ b/tests/federated-config-loadconfig.test.cjs @@ -0,0 +1,451 @@ +'use strict'; + +/** + * federated-config-loadconfig.test.cjs — Tests for the federated config overlay + * wired into loadConfig (ADR-857 phase 3b). + * + * Tests: + * 1. EQUIVALENCE/no-op: with the real registry, loadConfig output has NO unexpected + * extra keys (the UI keys are central so the overlay is empty). + * 2. FIXTURE federated key: inject a synthetic configSchema with a key NOT in + * central schema → loadConfig surfaces it with its default. + * 3. FIXTURE federated key with user override: user config sets the federated key + * to a valid value → that value is used. + * 4. MALFORMED registry: configSchema with bad slices → loadConfig returns a valid + * config without throwing. + */ + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); + +const { cleanup } = require('./helpers.cjs'); + +// ─── Module under test ──────────────────────────────────────────────────────── + +const configLoader = require('../gsd-core/bin/lib/config-loader.cjs'); +const { + loadConfig, + _setFederatedRegistryForTests, + _resetFederatedRegistryForTests, +} = configLoader; + +// ─── Helpers ────────────────────────────────────────────────────────────────── + +function makeTempProject() { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-fed-cfg-test-')); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases'), { recursive: true }); + return tmpDir; +} + +function writeConfig(tmpDir, obj) { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify(obj, null, 2), + 'utf-8', + ); +} + +// Keep track of temp dirs for cleanup +let tmpDirs = []; + +beforeEach(() => { + tmpDirs = []; + _resetFederatedRegistryForTests(); +}); + +afterEach(() => { + _resetFederatedRegistryForTests(); + for (const d of tmpDirs) { + try { cleanup(d); } catch { /* ignore */ } + } +}); + +function mkTemp() { + const d = makeTempProject(); + tmpDirs.push(d); + return d; +} + +// ─── 1. Equivalence / no-op with real registry ─────────────────────────────── + +describe('EQUIVALENCE: real registry is a no-op overlay', () => { + test('loadConfig with an empty config.json returns base config without extra federated keys', () => { + const tmpDir = mkTemp(); + // Write an empty config to trigger the try-branch (federated overlay path) + writeConfig(tmpDir, {}); + const result = loadConfig(tmpDir); + + // The result must be an object + assert.ok(typeof result === 'object' && result !== null, 'loadConfig must return an object'); + + // Known result keys that loadConfig always provides (from the main try-branch) + const knownKeys = [ + 'model_profile', 'commit_docs', 'search_gitignored', 'branching_strategy', + 'research', 'plan_checker', 'verifier', 'parallelization', 'brave_search', + 'firecrawl', 'exa_search', 'text_mode', 'auto_advance', + 'mode', 'sub_repos', 'resolve_model_ids', 'context_window', 'phase_naming', + 'project_code', 'subagent_timeout', 'model_overrides', 'models', 'granularity', + 'granularities', 'planning', 'dynamic_routing', 'runtime', 'model_profile_overrides', + 'model_policy', 'effort', 'fast_mode', 'agent_skills', 'manager', + ]; + + for (const key of knownKeys) { + assert.ok( + Object.prototype.hasOwnProperty.call(result, key), + 'Expected result to have key: ' + key, + ); + } + + // UI-capability keys must NOT appear as new top-level keys (they're still central + // and the overlay is empty — so these keys should not be added) + // Note: 'workflow' IS an existing top-level key concept via VALID_CONFIG_KEYS, + // but the nested keys like 'ui_phase' must not be present. + const workflowSection = result['workflow']; + if (workflowSection && typeof workflowSection === 'object') { + // workflow section may already exist from user config but should not have ui_phase + // in the default no-config case + assert.ok( + !Object.prototype.hasOwnProperty.call(workflowSection, 'ui_phase'), + 'workflow.ui_phase should not be injected by the federated overlay (key is still central)', + ); + assert.ok( + !Object.prototype.hasOwnProperty.call(workflowSection, 'ui_review'), + 'workflow.ui_review should not be injected by the federated overlay (key is still central)', + ); + assert.ok( + !Object.prototype.hasOwnProperty.call(workflowSection, 'ui_safety_gate'), + 'workflow.ui_safety_gate should not be injected by the federated overlay (key is still central)', + ); + } + }); + + test('loadConfig with a real config.json returns expected values + no unexpected keys from overlay', () => { + const tmpDir = mkTemp(); + writeConfig(tmpDir, { model_profile: 'balanced', research: true }); + const result = loadConfig(tmpDir); + + assert.strictEqual(result['model_profile'], 'balanced', 'model_profile from config'); + assert.strictEqual(result['research'], true, 'research from config'); + + // The overlay must not have added any unexpected keys from the registry + // (all UI keys are central → skipped → no additions) + // Verify a spot-check: 'ui_phase' should not exist anywhere + assert.strictEqual(result['ui_phase'], undefined, 'ui_phase should not appear as top-level key'); + }); +}); + +// ─── 2. Fixture federated key — value = default ────────────────────────────── + +describe('FIXTURE federated key: key not in central schema', () => { + test('injected configSchema with non-central key → appears in loadConfig result with default', () => { + const tmpDir = mkTemp(); + // Write an empty config.json so loadConfig enters the try-branch (federated overlay path) + writeConfig(tmpDir, {}); + + // Inject a synthetic registry with a key not in the central schema + _setFederatedRegistryForTests({ + configSchema: { + 'mytool.enabled': { + owner: 'mytool', + type: 'boolean', + default: true, + description: 'Enable mytool.', + }, + }, + }); + + const result = loadConfig(tmpDir); + + // mytool is not in the central schema, so the overlay should surface it + // The key 'mytool.enabled' is dotted → result should have result.mytool.enabled = true + const myToolSection = result['mytool']; + assert.ok(typeof myToolSection === 'object' && myToolSection !== null, + 'mytool section must be created for dotted federated key'); + assert.strictEqual( + (myToolSection)['enabled'], + true, + 'mytool.enabled must default to true from slice', + ); + }); + + test('injected top-level (non-dotted) federated key → appears in result', () => { + const tmpDir = mkTemp(); + // Write an empty config.json to enter the try-branch + writeConfig(tmpDir, {}); + + _setFederatedRegistryForTests({ + configSchema: { + 'mytool_flag': { + owner: 'mytool', + type: 'boolean', + default: false, + description: 'Top-level mytool flag.', + }, + }, + }); + + const result = loadConfig(tmpDir); + // Top-level key: result['mytool_flag'] = false (the default) + // BUT: only added if NOT already present in _baseConfig + // 'mytool_flag' is not in the central schema, so it should be added + assert.strictEqual(result['mytool_flag'], false, 'mytool_flag should be set to default false'); + }); +}); + +// ─── 3. Fixture federated key — user override ──────────────────────────────── + +describe('FIXTURE federated key: user config sets the key', () => { + test('user sets a federated key to a valid value → loadConfig uses user value', () => { + const tmpDir = mkTemp(); + + // Write a user config with a synthetic federated key + // The user config uses flat notation (mytool_flag: false) + writeConfig(tmpDir, { mytool_flag: true }); + + _setFederatedRegistryForTests({ + configSchema: { + 'mytool_flag': { + owner: 'mytool', + type: 'boolean', + default: false, + description: 'Top-level mytool flag.', + }, + }, + }); + + const result = loadConfig(tmpDir); + // The user set mytool_flag=true, which matches the type (boolean), so user value wins + assert.strictEqual(result['mytool_flag'], true, 'User-supplied true should override default false'); + }); + + test('user sets a federated key to wrong type → loadConfig falls back to default', () => { + const tmpDir = mkTemp(); + // Write a user config with the wrong type for the federated key + writeConfig(tmpDir, { mytool_flag: 'not-a-bool' }); + + _setFederatedRegistryForTests({ + configSchema: { + 'mytool_flag': { + owner: 'mytool', + type: 'boolean', + default: false, + description: 'Top-level mytool flag.', + }, + }, + }); + + const result = loadConfig(tmpDir); + // Wrong type → fallback to default (false) + assert.strictEqual(result['mytool_flag'], false, 'Should fall back to default on type mismatch'); + }); +}); + +// ─── FIX 1: Nested dotted-path user-override in loadConfig ─────────────────── + +describe('FIX 1: nested user config drives federated overlay in loadConfig', () => { + test('user config { mytool: { enabled: false } } (NESTED) → loadConfig surfaces false', () => { + const tmpDir = mkTemp(); + // Write config.json with the nested structure users actually write + writeConfig(tmpDir, { mytool: { enabled: false } }); + + _setFederatedRegistryForTests({ + configSchema: { + 'mytool.enabled': { + owner: 'mytool', + type: 'boolean', + default: true, + description: 'Enable mytool.', + }, + }, + }); + + const result = loadConfig(tmpDir); + const myToolSection = result['mytool']; + assert.ok(typeof myToolSection === 'object' && myToolSection !== null, + 'mytool section must be in result'); + assert.strictEqual( + (myToolSection)['enabled'], + false, + 'Nested user override of false should override the default of true', + ); + }); +}); + +// ─── FIX 2: Overlay applied on no-config path ──────────────────────────────── + +describe('FIX 2: overlay applied on the no-config path', () => { + test('project with NO config.json → federated default is surfaced (non-central key)', () => { + // Create a project dir WITHOUT a .planning/config.json + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-fed-noconfig-')); + tmpDirs.push(tmpDir); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases'), { recursive: true }); + // Intentionally do NOT write a config.json + + _setFederatedRegistryForTests({ + configSchema: { + 'mytool.enabled': { + owner: 'mytool', + type: 'boolean', + default: false, + description: 'Enable mytool (default false).', + }, + }, + }); + + const result = loadConfig(tmpDir); + // The overlay must be applied on the no-config path: mytool.enabled should be false (the default) + const myToolSection = result['mytool']; + assert.ok( + typeof myToolSection === 'object' && myToolSection !== null, + 'mytool section must be created by overlay even on no-config path, got: ' + JSON.stringify(result['mytool']), + ); + assert.strictEqual( + (myToolSection)['enabled'], + false, + 'mytool.enabled must default to false on no-config path', + ); + }); + + test('no-config path with REAL registry (all keys central) → output is byte-identical to defaults (no-op)', () => { + // No config.json — use real registry which has all keys central + _resetFederatedRegistryForTests(); + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-fed-noconfig-real-')); + tmpDirs.push(tmpDir); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases'), { recursive: true }); + // No config.json + + const result = loadConfig(tmpDir); + assert.ok(typeof result === 'object' && result !== null, 'result must be an object'); + + // With real registry (all keys central → overlay is empty → no-op), the result + // should be the defaults object WITHOUT any extra injected keys. + // Spot-check: ui_phase / ui_review / ui_safety_gate must NOT be injected + const workflowSection = result['workflow']; + if (workflowSection && typeof workflowSection === 'object') { + assert.ok( + !Object.prototype.hasOwnProperty.call(workflowSection, 'ui_phase'), + 'workflow.ui_phase must not be injected on no-config path (no-op)', + ); + } + // model_profile must be present (it comes from defaults) + assert.ok(Object.prototype.hasOwnProperty.call(result, 'model_profile'), 'model_profile must be present'); + }); +}); + +// ─── FIX 3: Federated key in config.json → no unknown-key warning ───────────── + +describe('FIX 3: federated key present in config.json → no unknown-key warning', () => { + test('synthetic federated key in config.json → no "unknown config key" warning on stderr', () => { + const tmpDir = mkTemp(); + // Write a config.json that contains a key matching our synthetic federated key's top-level segment + writeConfig(tmpDir, { mytool: { enabled: true } }); + + _setFederatedRegistryForTests({ + configSchema: { + 'mytool.enabled': { + owner: 'mytool', + type: 'boolean', + default: false, + description: 'Enable mytool.', + }, + }, + }); + + // Capture stderr to check for unknown-key warning + const stderrChunks = []; + const origWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk, ...args) => { + stderrChunks.push(typeof chunk === 'string' ? chunk : String(chunk)); + return origWrite(chunk, ...args); + }; + + try { + const result = loadConfig(tmpDir); + // mytool.enabled is in the federated registry → KNOWN_TOP_LEVEL should include 'mytool' + // → no "unknown config key(s)" warning for 'mytool' + const stderrOutput = stderrChunks.join(''); + assert.ok( + !stderrOutput.includes('unknown config key') || !stderrOutput.includes('mytool'), + 'Should NOT warn about mytool as an unknown key when it is a registered federated key. stderr: ' + stderrOutput, + ); + // The value should be set from user config + const myToolSection = result['mytool']; + assert.ok( + typeof myToolSection === 'object' && myToolSection !== null, + 'mytool section should be in result', + ); + } finally { + process.stderr.write = origWrite; + } + }); +}); + +// ─── 4. Malformed registry — loadConfig still works ────────────────────────── + +describe('MALFORMED registry: loadConfig does not throw', () => { + test('configSchema with all malformed slices → loadConfig returns valid config, no throw', () => { + const tmpDir = mkTemp(); + + _setFederatedRegistryForTests({ + configSchema: { + 'bad.key1': null, + 'bad.key2': 'just-a-string', + 'bad.key3': { type: 'xml', default: '' }, // invalid type + 'bad.key4': { type: 'boolean', description: 'ok' }, // missing default + 'bad.key5': {}, // missing both + }, + }); + + let result; + assert.doesNotThrow(() => { + result = loadConfig(tmpDir); + }, 'loadConfig must not throw even with all-malformed configSchema'); + + assert.ok(typeof result === 'object' && result !== null, 'result must be an object'); + // None of the bad keys should appear in the result + assert.strictEqual(result['bad.key1'], undefined); + assert.strictEqual(result['bad.key2'], undefined); + const badSection = result['bad']; + if (badSection && typeof badSection === 'object') { + assert.strictEqual((badSection)['key1'], undefined, 'bad.key1 must not be set'); + } + }); + + test('configSchema is a string (completely unexpected) → loadConfig still works', () => { + const tmpDir = mkTemp(); + + _setFederatedRegistryForTests({ + configSchema: 'not-an-object', + }); + + let result; + assert.doesNotThrow(() => { + result = loadConfig(tmpDir); + }, 'loadConfig must not throw with non-object configSchema'); + + assert.ok(typeof result === 'object' && result !== null, 'result must be an object'); + }); + + test('registry throws during configSchema access → loadConfig still returns base config', () => { + const tmpDir = mkTemp(); + + // Create a registry proxy that throws when configSchema is accessed + const throwingRegistry = { + get configSchema() { throw new Error('registry exploded'); }, + }; + + _setFederatedRegistryForTests(throwingRegistry); + + let result; + assert.doesNotThrow(() => { + result = loadConfig(tmpDir); + }, 'loadConfig must not throw even if registry access throws'); + + assert.ok(typeof result === 'object' && result !== null, 'result must still be an object'); + // The base config keys must be present + assert.ok(Object.prototype.hasOwnProperty.call(result, 'model_profile'), 'model_profile must be present'); + }); +}); diff --git a/tests/federated-config.test.cjs b/tests/federated-config.test.cjs new file mode 100644 index 000000000..f6275badb --- /dev/null +++ b/tests/federated-config.test.cjs @@ -0,0 +1,696 @@ +'use strict'; + +/** + * federated-config.test.cjs — Behavioral tests for the federated-config module. + * + * ADR-857 phase 3b. Tests cover: + * - empty configSchema → empty result + * - key still in central schema → skipped + pending-migration warning + NOT in values + * - malformed slice (each variant) → skipped + warning, no throw + * - valid federated key → value = default + * - valid federated key with correct-type user override → user value used + * - valid federated key with wrong-type user override → falls back to default + warning + * - __proto__/constructor/prototype keys → ignored, no Object.prototype pollution + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); + +const { mergeFederatedConfig } = require('../gsd-core/bin/lib/federated-config.cjs'); + +// ─── Helper fixtures ────────────────────────────────────────────────────────── + +/** A minimal well-formed boolean config slice entry. */ +const BOOLEAN_SLICE = { + owner: 'test-cap', + type: 'boolean', + default: true, + description: 'A test boolean key.', +}; + +/** A minimal well-formed string config slice entry. */ +const STRING_SLICE = { + owner: 'test-cap', + type: 'string', + default: 'hello', + description: 'A test string key.', +}; + +/** A minimal well-formed number config slice entry. */ +const NUMBER_SLICE = { + owner: 'test-cap', + type: 'number', + default: 42, + description: 'A test number key.', +}; + +/** A minimal well-formed enum config slice entry (no values list). */ +const ENUM_SLICE_NO_VALUES = { + owner: 'test-cap', + type: 'enum', + default: 'medium', + description: 'A test enum key without values list.', +}; + +/** A minimal well-formed enum config slice entry (with values list). */ +const ENUM_SLICE_WITH_VALUES = { + owner: 'test-cap', + type: 'enum', + default: 'low', + values: ['low', 'medium', 'high'], + description: 'A test enum key with values list.', +}; + +/** A never-central isCentralKey that always returns false. */ +const neverCentral = (_key) => false; + +/** An always-central isCentralKey. */ +const alwaysCentral = (_key) => true; + +// ─── 1. Empty configSchema ──────────────────────────────────────────────────── + +describe('empty configSchema', () => { + test('empty object → empty result', () => { + const result = mergeFederatedConfig({ + configSchema: {}, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.strictEqual(Object.keys(result.values).length, 0, 'values should be empty'); + assert.deepEqual(result.validKeys, [], 'validKeys should be empty'); + assert.deepEqual(result.warnings, [], 'warnings should be empty'); + }); + + test('null configSchema → empty result (defensive)', () => { + const result = mergeFederatedConfig({ + configSchema: null, + isCentralKey: neverCentral, + userConfig: {}, + }); + // FIX 6b: values uses null-prototype object; check it's empty + assert.strictEqual(Object.keys(result.values).length, 0, 'values should be empty for null configSchema'); + assert.deepEqual(result.validKeys, [], 'validKeys should be empty'); + assert.deepEqual(result.warnings, [], 'warnings should be empty'); + }); +}); + +// ─── 2. Central-key skipping ────────────────────────────────────────────────── + +describe('central-key skipping', () => { + test('key in central schema → skipped + pending-migration warning + NOT in values', () => { + const result = mergeFederatedConfig({ + configSchema: { 'workflow.ui_phase': BOOLEAN_SLICE }, + isCentralKey: alwaysCentral, + userConfig: {}, + }); + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'workflow.ui_phase'), + 'central key must NOT appear in values'); + assert.deepEqual(result.validKeys, [], 'validKeys must be empty for central keys'); + assert.ok(result.warnings.length >= 1, 'Must produce at least one warning'); + assert.ok( + result.warnings.some((w) => w.includes('pending-migration') || w.includes('central config-schema')), + 'Warning must mention pending-migration or central config-schema, got: ' + JSON.stringify(result.warnings), + ); + }); + + test('key NOT in central schema → appears in values', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.enabled': BOOLEAN_SLICE }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.ok(Object.prototype.hasOwnProperty.call(result.values, 'mytool.enabled'), + 'Non-central key must appear in values'); + assert.strictEqual(result.validKeys.length, 1); + }); + + test('mixed central + non-central: central skipped, non-central included', () => { + const result = mergeFederatedConfig({ + configSchema: { + 'central.key': BOOLEAN_SLICE, + 'mytool.enabled': BOOLEAN_SLICE, + }, + isCentralKey: (key) => key === 'central.key', + userConfig: {}, + }); + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'central.key'), + 'central.key must not be in values'); + assert.ok(Object.prototype.hasOwnProperty.call(result.values, 'mytool.enabled'), + 'mytool.enabled must be in values'); + assert.strictEqual(result.validKeys.length, 1); + assert.ok(result.warnings.some((w) => w.includes('pending-migration') || w.includes('central'))); + }); +}); + +// ─── 3. Malformed slice handling ────────────────────────────────────────────── + +describe('malformed slice handling — no throw, warning emitted', () => { + test('slice missing type → skipped + warning, no throw', () => { + const result = mergeFederatedConfig({ + configSchema: { 'tool.key': { owner: 'x', default: true, description: 'x' } }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'tool.key'), 'malformed key must not be in values'); + assert.ok(result.warnings.length >= 1, 'Must warn about malformed slice'); + assert.ok(result.warnings.some((w) => w.includes('malformed') || w.includes('tool.key')), + 'Warning must mention the key, got: ' + JSON.stringify(result.warnings)); + }); + + test('slice with invalid type ("xml") → skipped + warning, no throw', () => { + const result = mergeFederatedConfig({ + configSchema: { 'tool.key': { owner: 'x', type: 'xml', default: '', description: 'xml key' } }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'tool.key')); + assert.ok(result.warnings.length >= 1); + }); + + test('slice missing default → skipped + warning, no throw', () => { + const result = mergeFederatedConfig({ + configSchema: { 'tool.key': { owner: 'x', type: 'boolean', description: 'x' } }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'tool.key')); + assert.ok(result.warnings.length >= 1); + }); + + test('slice is null → skipped + warning, no throw', () => { + const result = mergeFederatedConfig({ + configSchema: { 'tool.key': null }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'tool.key')); + assert.ok(result.warnings.length >= 1); + }); + + test('slice is a string scalar → skipped + warning, no throw', () => { + const result = mergeFederatedConfig({ + configSchema: { 'tool.key': 'just-a-string' }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'tool.key')); + assert.ok(result.warnings.length >= 1); + }); + + test('slice is a number → skipped + warning, no throw', () => { + const result = mergeFederatedConfig({ + configSchema: { 'tool.key': 42 }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'tool.key')); + assert.ok(result.warnings.length >= 1); + }); +}); + +// ─── 4. Valid federated key — default resolution ────────────────────────────── + +describe('valid federated key — default resolution', () => { + test('boolean key absent from userConfig → value = default (true)', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.enabled': BOOLEAN_SLICE }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.strictEqual(result.values['mytool.enabled'], true, 'Should use slice default (true)'); + assert.ok(result.validKeys.includes('mytool.enabled')); + assert.deepEqual(result.warnings, []); + }); + + test('string key absent from userConfig → value = default ("hello")', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.name': STRING_SLICE }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.strictEqual(result.values['mytool.name'], 'hello'); + assert.ok(result.validKeys.includes('mytool.name')); + }); + + test('number key absent from userConfig → value = default (42)', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.timeout': NUMBER_SLICE }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.strictEqual(result.values['mytool.timeout'], 42); + }); + + test('enum key absent from userConfig → value = default ("medium")', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.level': ENUM_SLICE_NO_VALUES }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.strictEqual(result.values['mytool.level'], 'medium'); + }); +}); + +// ─── 5. Valid federated key — correct-type user override ───────────────────── + +describe('valid federated key — user override with correct type', () => { + test('boolean key with boolean user override (nested) → user value used', () => { + // FIX 1: users write nested objects, not flat dotted keys + const result = mergeFederatedConfig({ + configSchema: { 'mytool.enabled': BOOLEAN_SLICE }, + isCentralKey: neverCentral, + userConfig: { mytool: { enabled: false } }, + }); + assert.strictEqual(result.values['mytool.enabled'], false, 'Should use user-supplied false'); + assert.deepEqual(result.warnings, []); + }); + + test('string key with string user override (nested) → user value used', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.name': STRING_SLICE }, + isCentralKey: neverCentral, + userConfig: { mytool: { name: 'custom' } }, + }); + assert.strictEqual(result.values['mytool.name'], 'custom'); + assert.deepEqual(result.warnings, []); + }); + + test('number key with number user override (nested) → user value used', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.timeout': NUMBER_SLICE }, + isCentralKey: neverCentral, + userConfig: { mytool: { timeout: 99 } }, + }); + assert.strictEqual(result.values['mytool.timeout'], 99); + assert.deepEqual(result.warnings, []); + }); + + test('enum key with in-values string user override (nested) → user value used', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.level': ENUM_SLICE_WITH_VALUES }, + isCentralKey: neverCentral, + userConfig: { mytool: { level: 'high' } }, + }); + assert.strictEqual(result.values['mytool.level'], 'high'); + assert.deepEqual(result.warnings, []); + }); +}); + +// ─── 6. Valid federated key — wrong-type user override ─────────────────────── + +describe('valid federated key — wrong-type user override', () => { + test('boolean key with string user override (nested) → falls back to default + warning', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.enabled': BOOLEAN_SLICE }, + isCentralKey: neverCentral, + userConfig: { mytool: { enabled: 'not-a-bool' } }, + }); + // Key IS in validKeys (degraded resolution — default used) + assert.ok(Object.prototype.hasOwnProperty.call(result.values, 'mytool.enabled'), + 'Key should still appear in values (degraded)'); + assert.strictEqual(result.values['mytool.enabled'], true, 'Should fall back to default (true)'); + assert.ok(result.warnings.length >= 1, 'Should warn about type mismatch'); + assert.ok( + result.warnings.some((w) => w.includes('wrong type') || w.includes('type')), + 'Warning should mention type mismatch, got: ' + JSON.stringify(result.warnings), + ); + }); + + test('string key with boolean user override (nested) → falls back to default + warning', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.name': STRING_SLICE }, + isCentralKey: neverCentral, + userConfig: { mytool: { name: true } }, + }); + assert.strictEqual(result.values['mytool.name'], 'hello', 'Should fall back to default'); + assert.ok(result.warnings.length >= 1); + }); + + test('number key with string user override (nested) → falls back to default + warning', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.timeout': NUMBER_SLICE }, + isCentralKey: neverCentral, + userConfig: { mytool: { timeout: 'fast' } }, + }); + assert.strictEqual(result.values['mytool.timeout'], 42, 'Should fall back to default'); + assert.ok(result.warnings.length >= 1); + }); +}); + +// ─── 7. Prototype pollution guard ──────────────────────────────────────────── + +describe('prototype pollution guard', () => { + test('__proto__ key in configSchema → ignored, no Object.prototype pollution', () => { + // We can't pass __proto__ as an own-enumerable property via object literal, + // so we use Object.create + defineProperty to simulate what a capability registry + // might hand us if prototype pollution had been attempted upstream. + const poisonedSchema = Object.create(null); + Object.defineProperty(poisonedSchema, '__proto__', { + value: { polluted: true }, + enumerable: true, + configurable: true, + writable: true, + }); + // Note: 'constructor' and 'prototype' CAN be passed via plain object literals + const schemaWithReservedKeys = { + 'constructor': BOOLEAN_SLICE, + 'prototype': STRING_SLICE, + }; + + const result = mergeFederatedConfig({ + configSchema: schemaWithReservedKeys, + isCentralKey: neverCentral, + userConfig: {}, + }); + + // Reserved keys must not appear in values + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'constructor'), 'constructor must not be in values'); + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'prototype'), 'prototype must not be in values'); + assert.ok(!result.validKeys.includes('constructor'), 'constructor must not be in validKeys'); + assert.ok(!result.validKeys.includes('prototype'), 'prototype must not be in validKeys'); + + // Object.prototype must not be polluted + assert.strictEqual(({}).polluted, undefined, 'Object.prototype must not be polluted'); + assert.strictEqual(({}).constructor, Object, 'Object.prototype.constructor must be Object (not overwritten)'); + }); + + test('buildRegistry with poisoned keys does not pollute Object.prototype', () => { + // Even with isCentralKey always returning false, reserved keys are guarded + const result = mergeFederatedConfig({ + configSchema: { 'legitimate.key': BOOLEAN_SLICE }, + isCentralKey: neverCentral, + userConfig: {}, + }); + assert.strictEqual(({}).polluted, undefined, 'Object.prototype.polluted must be undefined after merge'); + assert.ok(Object.prototype.hasOwnProperty.call(result.values, 'legitimate.key'), 'legitimate key must be in values'); + }); +}); + +// ─── FIX 1: Nested dotted-path user-override lookup ────────────────────────── + +describe('FIX 1: nested dotted-path user-override lookup', () => { + test('user sets mytool.enabled via NESTED object → user value used', () => { + // Nested config: { mytool: { enabled: false } } — NOT flat {"mytool.enabled": false} + const result = mergeFederatedConfig({ + configSchema: { + 'mytool.enabled': { + owner: 'mytool', + type: 'boolean', + default: true, + description: 'Enable mytool.', + }, + }, + isCentralKey: neverCentral, + userConfig: { mytool: { enabled: false } }, // NESTED + }); + assert.strictEqual(result.values['mytool.enabled'], false, 'Nested user override should be used (false overrides true)'); + assert.deepEqual(result.warnings, [], 'No warnings for valid nested override'); + assert.ok(result.validKeys.includes('mytool.enabled')); + }); + + test('flat {"mytool.enabled": false} does NOT match nested path lookup', () => { + // Flat key string lookup is intentionally NOT supported per FIX 1 spec + const result = mergeFederatedConfig({ + configSchema: { + 'mytool.enabled': { + owner: 'mytool', + type: 'boolean', + default: true, + description: 'Enable mytool.', + }, + }, + isCentralKey: neverCentral, + userConfig: { 'mytool.enabled': false }, // FLAT — not found via nested traversal + }); + // Flat key is not found by nested traversal, so default is used + assert.strictEqual(result.values['mytool.enabled'], true, 'Flat key not found by nested traversal → default used'); + }); + + test('user sets nested 3-segment key correctly', () => { + // Key: "a.b.c", user config: { a: { b: { c: 'override' } } } + const result = mergeFederatedConfig({ + configSchema: { + 'a.b.c': { + owner: 'test', + type: 'string', + default: 'default-val', + description: 'Three-segment key.', + }, + }, + isCentralKey: neverCentral, + userConfig: { a: { b: { c: 'override' } } }, + }); + assert.strictEqual(result.values['a.b.c'], 'override'); + assert.deepEqual(result.warnings, []); + }); + + test('partial nested path (a.b exists but a.b.c missing) → uses default', () => { + const result = mergeFederatedConfig({ + configSchema: { + 'a.b.c': { + owner: 'test', + type: 'string', + default: 'default-val', + description: 'Three-segment key.', + }, + }, + isCentralKey: neverCentral, + userConfig: { a: { b: {} } }, // c is missing + }); + assert.strictEqual(result.values['a.b.c'], 'default-val', 'Missing leaf should use default'); + }); +}); + +// ─── FIX 4: null/undefined/non-object input guards ─────────────────────────── + +describe('FIX 4: null/undefined/non-object input guards', () => { + test('null input → no throw, empty result', () => { + assert.doesNotThrow(() => { + const result = mergeFederatedConfig(null); + assert.ok(result.validKeys.length === 0); + }); + }); + + test('undefined input → no throw, empty result', () => { + assert.doesNotThrow(() => { + const result = mergeFederatedConfig(undefined); + assert.ok(result.validKeys.length === 0); + }); + }); + + test('non-object input (string) → no throw, empty result', () => { + assert.doesNotThrow(() => { + const result = mergeFederatedConfig('not-an-object'); + assert.ok(result.validKeys.length === 0); + }); + }); + + test('null userConfig → treated as {} (no overrides), no throw', () => { + assert.doesNotThrow(() => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.enabled': BOOLEAN_SLICE }, + isCentralKey: neverCentral, + userConfig: null, + }); + // Should use the default since userConfig is null + assert.strictEqual(result.values['mytool.enabled'], true, 'Should use default when userConfig is null'); + assert.deepEqual(result.warnings, []); + }); + }); + + test('undefined userConfig → treated as {} (no overrides), no throw', () => { + assert.doesNotThrow(() => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.enabled': BOOLEAN_SLICE }, + isCentralKey: neverCentral, + userConfig: undefined, + }); + assert.strictEqual(result.values['mytool.enabled'], true, 'Should use default when userConfig is undefined'); + }); + }); + + test('non-object userConfig → treated as {} (no overrides), no throw', () => { + assert.doesNotThrow(() => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.enabled': BOOLEAN_SLICE }, + isCentralKey: neverCentral, + userConfig: 42, + }); + assert.strictEqual(result.values['mytool.enabled'], true, 'Should use default when userConfig is non-object'); + }); + }); +}); + +// ─── FIX 5b: enum user override validation against values list ──────────────── + +describe('FIX 5b: enum out-of-values user override → falls back to default', () => { + test('enum user override IN values list → accepted', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.level': ENUM_SLICE_WITH_VALUES }, + isCentralKey: neverCentral, + userConfig: { mytool: { level: 'high' } }, + }); + assert.strictEqual(result.values['mytool.level'], 'high', 'In-values override should be accepted'); + assert.deepEqual(result.warnings, []); + }); + + test('enum user override OUT OF values list → falls back to default + warning', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.level': ENUM_SLICE_WITH_VALUES }, + isCentralKey: neverCentral, + userConfig: { mytool: { level: 'extreme' } }, // 'extreme' not in ['low', 'medium', 'high'] + }); + assert.strictEqual(result.values['mytool.level'], 'low', 'Out-of-values override should fall back to default'); + assert.ok(result.warnings.length >= 1, 'Should warn about out-of-values override'); + assert.ok( + result.warnings.some((w) => w.includes('type') || w.includes('enum') || w.includes('invalid')), + 'Warning should mention type/enum issue, got: ' + JSON.stringify(result.warnings), + ); + }); + + test('enum user override is non-string → falls back to default + warning', () => { + const result = mergeFederatedConfig({ + configSchema: { 'mytool.level': ENUM_SLICE_WITH_VALUES }, + isCentralKey: neverCentral, + userConfig: { mytool: { level: 42 } }, // number, not string + }); + assert.strictEqual(result.values['mytool.level'], 'low'); + assert.ok(result.warnings.length >= 1); + }); +}); + +// ─── FIX 6b: null-proto consistency in early-return paths ──────────────────── + +describe('FIX 6b: null-proto consistency on early-return paths', () => { + test('null configSchema → values uses Object.create(null) (no __proto__ chain)', () => { + const result = mergeFederatedConfig({ + configSchema: null, + isCentralKey: neverCentral, + userConfig: {}, + }); + // Object.create(null) has no __proto__ — prototype is null + assert.strictEqual(Object.getPrototypeOf(result.values), null, 'values must use null prototype on null configSchema path'); + }); + + test('null input → values uses Object.create(null)', () => { + const result = mergeFederatedConfig(null); + assert.strictEqual(Object.getPrototypeOf(result.values), null, 'values must use null prototype on null input path'); + }); +}); + +// ─── FIX 6c: N-level nested write + prototype pollution via dotted keys ────── + +describe('FIX 6c: N-level nested write and prototype-pollution via dotted keys', () => { + test('3-segment federated key is correctly nested in loadConfig overlay', () => { + // This test verifies _getNestedValue works correctly for 3-segment keys. + // The values object should store the key as "a.b.c" → value mapping. + const result = mergeFederatedConfig({ + configSchema: { + 'tool.section.flag': { + owner: 'test', + type: 'boolean', + default: true, + description: 'Three-segment boolean key.', + }, + }, + isCentralKey: neverCentral, + userConfig: { tool: { section: { flag: false } } }, + }); + assert.strictEqual(result.values['tool.section.flag'], false, '3-segment override should be picked up'); + assert.ok(result.validKeys.includes('tool.section.flag')); + assert.deepEqual(result.warnings, []); + }); + + test('__proto__ segment in dotted key does NOT pollute Object.prototype', () => { + const schema = Object.create(null); + // Create a key with __proto__ in the path via defineProperty + Object.defineProperty(schema, '__proto__.x', { + value: { owner: 'test', type: 'boolean', default: true, description: 'bad key' }, + enumerable: true, configurable: true, writable: true, + }); + assert.doesNotThrow(() => { + mergeFederatedConfig({ + configSchema: schema, + isCentralKey: neverCentral, + userConfig: {}, + }); + }); + // Object.prototype must not be polluted + assert.strictEqual(({}).x, undefined, 'Object.prototype.x must not be polluted via __proto__ key'); + }); + + test('a.__proto__.b segment in dotted key does NOT pollute Object.prototype', () => { + // The key "a.__proto__.b" should be skipped at the __proto__ segment + const result = mergeFederatedConfig({ + configSchema: { + // We can't define 'a.__proto__.b' as an OWN property normally; skip test via defensive path + 'a.constructor.b': { + owner: 'test', + type: 'boolean', + default: true, + description: 'constructor key', + }, + }, + isCentralKey: neverCentral, + userConfig: {}, + }); + // 'a.constructor.b' contains 'constructor' segment — must be skipped + assert.ok(!result.validKeys.includes('a.constructor.b'), 'Key with constructor segment must be skipped'); + // Object.prototype.constructor must still be Object + assert.strictEqual(({}).constructor, Object, 'Object.prototype.constructor must not be modified'); + }); +}); + +// ─── 8. Real registry — all UI keys are central (no-op guarantee) ──────────── + +describe('real registry: all UI keys are central → no-op channel', () => { + test('with real capability-registry, all configSchema keys are skipped (pending-migration)', () => { + const capRegistry = require('../gsd-core/bin/lib/capability-registry.cjs'); + const configSchemaFromRegistry = capRegistry.configSchema; + + // Import the real isValidConfigKey + const configSchemaModule = require('../gsd-core/bin/lib/config-schema.cjs'); + const { isValidConfigKey } = configSchemaModule; + + if (!configSchemaFromRegistry || Object.keys(configSchemaFromRegistry).length === 0) { + // Registry has no configSchema keys — no-op by definition + return; + } + + const result = mergeFederatedConfig({ + configSchema: configSchemaFromRegistry, + isCentralKey: isValidConfigKey, + userConfig: {}, + }); + + // Every key should be skipped (pending-migration) because UI keys are still in central schema + assert.strictEqual(Object.keys(result.values).length, 0, 'values must be empty — all keys are central (pending-migration)'); + assert.deepEqual(result.validKeys, [], 'validKeys must be empty'); + assert.ok(result.warnings.length > 0, 'Should have pending-migration warnings'); + + // Confirm each UI key specifically + const uiKeys = ['workflow.ui_phase', 'workflow.ui_review', 'workflow.ui_safety_gate']; + for (const key of uiKeys) { + assert.ok( + result.warnings.some((w) => w.includes(key)), + 'Should have a warning for ' + key + ', got: ' + JSON.stringify(result.warnings), + ); + } + }); +}); + +// ─── 9. isCentralKey throwing defensively ──────────────────────────────────── + +describe('isCentralKey defensive behavior', () => { + test('isCentralKey that throws → key is skipped with warning, no throw from mergeFederatedConfig', () => { + const throwingCentralKey = () => { throw new Error('internal error'); }; + const result = mergeFederatedConfig({ + configSchema: { 'mytool.key': BOOLEAN_SLICE }, + isCentralKey: throwingCentralKey, + userConfig: {}, + }); + // Key skipped due to isCentralKey throwing + assert.ok(!Object.prototype.hasOwnProperty.call(result.values, 'mytool.key'), 'Key must be skipped when isCentralKey throws'); + assert.ok(result.warnings.length >= 1, 'Must produce a warning when isCentralKey throws'); + }); +}); From bf954e4b443abb4c702cdff3deb55bb7d53b770f Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 8 Jun 2026 23:42:24 -0400 Subject: [PATCH 3/4] fix(#916): deterministic phase-complete subprocess in regression tests (#917) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bug #1998 subtest "checkbox updated when archived milestones exist in
" flaked under the high-concurrency docker run (~672 test files in parallel): the current-milestone checkbox was left unchecked. Root cause: `gsd-tools phase complete` writes ROADMAP.md as its LAST step, after a read-heavy parse/lock sequence. Under heavy parallel CPU/IO contention the test's tight `timeout: 10000` fired mid-parse and SIGTERM'd the subprocess before that write landed, leaving ROADMAP.md pristine (both phases `- [ ]`). The bare `catch {}` silently swallowed the kill, so a timeout masqueraded as a "checkbox not checked" assertion failure. All I/O is scoped to each test's tmpDir, so there is no cross-process race — the timeout was the sole cause. Consolidate all 7 duplicated `phase complete` call sites (suites #1998, #2005, #2526) into a shared runPhaseComplete() helper that: 1. raises the timeout to 60s so the test's own timer never kills the subprocess under load; 2. never silently swallows a signal/timeout kill (rethrows loudly with captured output) while still tolerating a clean non-zero exit for the ROADMAP-asserting tests via { tolerateExit: true }. No retry loop. Verified with 3x `gsd-test --reset` full docker runs (13207 tests / 2311 suites each, 0 failures, flaky subtest green every round). Closes #916 Co-authored-by: Claude Opus 4.8 --- tests/phase.test.cjs | 122 ++++++++++++++++++++----------------------- 1 file changed, 56 insertions(+), 66 deletions(-) diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 08f0e0a7b..5820a9998 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -3364,6 +3364,55 @@ describe('bug #1962: normalizePhaseName preserves letter suffix case', () => { // (consolidated from tests/bug-1998-phase-complete-checkbox.test.cjs) // ───────────────────────────────────────────────────────────────────────────── +/** + * Run `gsd-tools phase complete ` for the phase-complete regression + * suites and return its stdout. + * + * `phase complete` writes ROADMAP.md as its LAST step, after a read-heavy + * parse/lock sequence (ROADMAP read, two extractCurrentMilestone STATE.md + * parses, REQUIREMENTS read, phase-dir scan, STATE read — all before the single + * writePlanningFileSet flush). Under the high-concurrency docker run (~672 test + * files in parallel), a tight 10s timeout could fire mid-parse and SIGTERM the + * subprocess BEFORE that write landed, leaving ROADMAP.md untouched. Call sites + * that used a bare `catch {}` then silently proceeded to assert on the pristine + * file — an intermittent "checkbox not checked" failure (bug #1998 flake). + * + * Two-part fix, no retry loop: + * 1. A generous timeout so the test's own timer never kills the subprocess + * under load (10s cold-node startup × 672-way CPU/IO contention was the + * real culprit — all I/O is scoped to tmpDir, so there is no cross-process + * race to blame). + * 2. Never silently swallow a signal/timeout kill: it means the process was + * terminated before completing its writes, so we surface it loudly with + * context instead of letting it masquerade as an assertion failure. A + * *clean* non-zero exit is still tolerated when `tolerateExit` is set, + * because the ROADMAP write has already landed before any post-write step + * that may exit non-zero in these minimal fixtures. + */ +function runPhaseComplete(tmpDir, { phase = '1', tolerateExit = false } = {}) { + try { + return execFileSync('node', [GSD_TOOLS_BIN, 'phase', 'complete', phase], { + cwd: tmpDir, + timeout: 60000, + encoding: 'utf-8', + }); + } catch (err) { + // A signal/timeout kill terminated the process before it finished writing — + // never tolerate it; surface it with whatever output was captured. + if (err.killed || err.signal != null || err.code === 'ETIMEDOUT') { + throw new Error( + `gsd-tools phase complete ${phase} was killed before completion ` + + `(signal=${err.signal}, code=${err.code}). ` + + `stdout=${err.stdout || ''} stderr=${err.stderr || ''}` + ); + } + if (tolerateExit) { + return `${err.stdout || ''}${err.stderr || ''}`; + } + throw err; + } +} + describe('bug #1998: phase complete updates overview checkbox', () => { let tmpDir; let planningDir; @@ -3419,11 +3468,7 @@ describe('bug #1998: phase complete updates overview checkbox', () => { '| 2. Features | 0/1 | Pending | - |', ].join('\n')); - try { - execFileSync('node', [GSD_TOOLS_BIN, 'phase', 'complete', '1'], { cwd: tmpDir, timeout: 10000 }); - } catch { - // Command may exit non-zero if STATE.md update fails, but ROADMAP.md update happens first - } + runPhaseComplete(tmpDir, { tolerateExit: true }); const result = fs.readFileSync(roadmapPath, 'utf-8'); assert.match(result, /- \[x\] \*\*Phase 1: Foundation\*\*/, 'overview checkbox should be checked'); @@ -3468,11 +3513,7 @@ describe('bug #1998: phase complete updates overview checkbox', () => { '
', ].join('\n')); - try { - execFileSync('node', [GSD_TOOLS_BIN, 'phase', 'complete', '1'], { cwd: tmpDir, timeout: 10000 }); - } catch { - // May exit non-zero - } + runPhaseComplete(tmpDir, { tolerateExit: true }); const result = fs.readFileSync(roadmapPath, 'utf-8'); assert.match(result, /- \[x\] \*\*Phase 1: Setup\*\*/, 'current milestone checkbox should be checked'); @@ -3559,11 +3600,7 @@ describe('bug #2005: phase complete updates plan count when milestone is inside '', ].join('\n')); - try { - execFileSync('node', [GSD_TOOLS_BIN, 'phase', 'complete', '1'], { cwd: tmpDir, timeout: 10000 }); - } catch { - // May exit non-zero if STATE.md update fails, but ROADMAP.md update is the target - } + runPhaseComplete(tmpDir, { tolerateExit: true }); const result = fs.readFileSync(roadmapPath, 'utf-8'); @@ -3619,9 +3656,7 @@ describe('bug #2005: phase complete updates plan count when milestone is inside '', ].join('\n')); - try { - execFileSync('node', [GSD_TOOLS_BIN, 'phase', 'complete', '1'], { cwd: tmpDir, timeout: 10000 }); - } catch {} + runPhaseComplete(tmpDir, { tolerateExit: true }); const result = fs.readFileSync(roadmapPath, 'utf-8'); @@ -3709,22 +3744,7 @@ describe('bug #2526: phase complete warns about unregistered REQ-IDs', () => { '| REQ-001 | 1 | Pending |', ].join('\n')); - let stdout = ''; - let stderr = ''; - try { - const result = execFileSync('node', [GSD_TOOLS_BIN, 'phase', 'complete', '1'], { - cwd: tmpDir, - timeout: 10000, - encoding: 'utf-8', - }); - stdout = result; - } catch (err) { - stdout = err.stdout || ''; - stderr = err.stderr || ''; - throw err; - } - - const combined = stdout + stderr; + const combined = runPhaseComplete(tmpDir); assert.match(combined, /REQ-002/, 'output should mention REQ-002 as missing from Traceability table'); assert.match(combined, /REQ-003/, 'output should mention REQ-003 as missing from Traceability table'); }); @@ -3776,22 +3796,7 @@ describe('bug #2526: phase complete warns about unregistered REQ-IDs', () => { '| REQ-002 | 1 | Pending |', ].join('\n')); - let stdout = ''; - let stderr = ''; - try { - const result = execFileSync('node', [GSD_TOOLS_BIN, 'phase', 'complete', '1'], { - cwd: tmpDir, - timeout: 10000, - encoding: 'utf-8', - }); - stdout = result; - } catch (err) { - stdout = err.stdout || ''; - stderr = err.stderr || ''; - throw err; - } - - const combined = stdout + stderr; + const combined = runPhaseComplete(tmpDir); assert.doesNotMatch( combined, /unregistered|missing.*traceability|not in.*traceability/i, @@ -3845,22 +3850,7 @@ describe('bug #2526: phase complete warns about unregistered REQ-IDs', () => { '| REQ-001 | 1 | Pending |', ].join('\n')); - let stdout = ''; - let stderr = ''; - try { - const result = execFileSync('node', [GSD_TOOLS_BIN, 'phase', 'complete', '1'], { - cwd: tmpDir, - timeout: 10000, - encoding: 'utf-8', - }); - stdout = result; - } catch (err) { - stdout = err.stdout || ''; - stderr = err.stderr || ''; - throw err; - } - - const combined = stdout + stderr; + const combined = runPhaseComplete(tmpDir); assert.match(combined, /REQ-002/, 'should warn about REQ-002'); assert.match(combined, /REQ-003/, 'should warn about REQ-003'); assert.match(combined, /REQ-004/, 'should warn about REQ-004'); From 5670feaef5f5399a13c05b727e5ef4d8fbe50570 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 9 Jun 2026 00:24:12 -0400 Subject: [PATCH 4/4] =?UTF-8?q?feat(#918):=20loop.render-hooks=20resolver?= =?UTF-8?q?=20=E2=80=94=20consume=20the=20Capability=20Registry=20(ADR-857?= =?UTF-8?q?=20phase=203c)=20(#920)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the loop.render-hooks resolver: the first registry-consuming query. `gsd-tools loop render-hooks ` validates the point against the authoritative canonical 12, reads the registry's materialized byLoopPoint hooks, filters them by activation, and emits a JSON envelope {point, activeHooks, rendered} with ordered markdown. Activation resolves each hook's `when` key by precedence: loadConfig value (post-cutover federated) -> raw config.json workstream/root single-key lookup (pre-cutover central override) -> registry configSchema default (so a default:true capability hook is active out-of-the-box) -> inactive. Guarded single-value reads only (no merged object built from untrusted keys). Registry-only: no workflow calls the resolver yet (wiring is the phase-6 cutover). Completes the phase-3 trio. Closes #918 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- .gitignore | 1 + CONTEXT.md | 4 +- docs/ARCHITECTURE.md | 1 + docs/INVENTORY-MANIFEST.json | 1 + docs/INVENTORY.md | 3 +- eslint.config.mjs | 1 + gsd-core/bin/gsd-tools.cjs | 23 +- src/loop-resolver.cts | 533 +++++++++++++++++++++ tests/loop-render-hooks.test.cjs | 777 +++++++++++++++++++++++++++++++ 9 files changed, 1340 insertions(+), 4 deletions(-) create mode 100644 src/loop-resolver.cts create mode 100644 tests/loop-render-hooks.test.cjs diff --git a/.gitignore b/.gitignore index 4bb4bc1c3..b431ccedb 100644 --- a/.gitignore +++ b/.gitignore @@ -132,6 +132,7 @@ build/ /gsd-core/bin/lib/phase-id.cjs /gsd-core/bin/lib/config-loader.cjs /gsd-core/bin/lib/model-resolver.cjs +/gsd-core/bin/lib/loop-resolver.cjs /gsd-core/bin/lib/federated-config.cjs /gsd-core/bin/lib/phase-locator.cjs /gsd-core/bin/lib/roadmap-parser.cjs diff --git a/CONTEXT.md b/CONTEXT.md index a7b03af7c..18699859e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -154,8 +154,8 @@ Generated central manifest projecting all co-located Capability declarations int ### Federated Config ADR-857 phase 3b seam that merges capability-declared config slices into the `loadConfig` return value. Implemented in `src/federated-config.cts` → `gsd-core/bin/lib/federated-config.cjs`. Exports `mergeFederatedConfig({ configSchema, isCentralKey, userConfig }) → { values, validKeys, warnings }`. Rules: central-schema keys are skipped with a `pending-migration` warning; malformed slices are skipped with a warning (never throws); valid federated keys (absent from the central schema) resolve to the user-supplied value (if type-matches) or the slice default. Object writes are guarded against prototype pollution with inline literal `__proto__`/`constructor`/`prototype` key checks. Wired into `loadConfig` as a true no-op today: every Capability config key is still in the central config-schema, so `isCentralKey()` returns true for all of them and `values` is always empty. The channel becomes live when a key is atomically removed from the central schema at cutover (the ADR-857 migration step). `loadConfig` exposes `_setFederatedRegistryForTests`/`_resetFederatedRegistryForTests` seams for injecting a synthetic registry in tests. -### Loop Extension Point [Planned] -A named, stable site on a host loop step (per-step `pre`/`post` plus per-wave in Execute; ~12 total) where Capabilities register hooks. Three hook kinds: `step` (runs as its own sequenced unit), `contribution` (injects into the core step's prompt/context), and `gate` (checks and optionally blocks via a declared `blocking` flag). Each hook declares the artifacts it produces and consumes; hook order is derived by topological sort of that produces/consumes graph (capability-id tiebreak), which also defines data flow — file-artifact based, surviving `/clear` and fresh executor contexts. Hooks are surfaced by runtime resolution with concrete projection: the workflow calls a query (extending the `init.*` resolution seam) that resolves the active hooks and returns fully-rendered, ordered markdown for the executor. Failure is default-resilient — a non-gate hook that errors is skipped with a warning; a hook may opt into `onError: halt`. Part of the Capability system. +### Loop Extension Point +A named, stable site on a host loop step (per-step `pre`/`post` plus per-wave in Execute; 12 total) where Capabilities register hooks. Three hook kinds: `step` (runs as its own sequenced unit), `contribution` (injects into the core step's prompt/context), and `gate` (checks and optionally blocks via a declared `blocking` flag). Each hook declares the artifacts it produces and consumes; hook order is derived by topological sort of that produces/consumes graph (capability-id tiebreak), which also defines data flow — file-artifact based, surviving `/clear` and fresh executor contexts. Hooks are surfaced by runtime resolution with concrete projection: the workflow calls a query that resolves the active hooks and returns fully-rendered, ordered markdown for the executor. Failure is default-resilient — a non-gate hook that errors is skipped with a warning; a hook may opt into `onError: halt`. Part of the Capability system. ADR-857 phase 3c ships the registry-consuming query layer: `gsd-core/bin/lib/loop-resolver.cjs` exposes `resolveLoopHooks({ point, registry, config })` (pure, no I/O), `renderLoopHooks(resolved)` (pure markdown renderer), and `cmdLoopRenderHooks(cwd, point, raw, opts)` (I/O entry point); activated via `gsd-tools loop render-hooks ` which emits `{ point, activeHooks[], rendered }`. Activation is driven by `when` (dotted config key resolved against `loadConfig`), with inline literal `__proto__`/`constructor`/`prototype` prototype-pollution guard. Wiring a workflow to call this query is the ADR-857 phase-6 cutover (out of scope here). ### Runtime Capability [Planned] A `role: runtime` variant of a Capability (a Capability carries `role: feature | runtime`) that projects GSD's produced artifacts (skills/agents/hooks/commands) onto one host CLI's conventions — config-surface format, artifact-layout kinds, command template, hooks manifest, sandbox tier. It is a declarative descriptor over a fixed first-party primitive vocabulary (not a code adapter); install composes active Feature Capabilities × the chosen Runtime Capability at the InstallPlan seam (ADR-0058). First-party runtimes are authored through the same descriptor a third party would write (dogfooding the interface); tier-1 (Claude Code, Codex, Antigravity) is fully tested, the other existing runtimes ship lower-tier, none dropped. Third-party runtime loading is deferred to a purely additive external loader + trust gate. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2328a5fa0..145f7208f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -372,6 +372,7 @@ Node.js CLI utility (`gsd-tools.cjs`) with domain modules split across `gsd-core | `profile-output.cjs` | Profile rendering, USER-PROFILE.md and dev-preferences.md generation | | `loop-host-contract.cjs` | Generated Loop Host Contract — 12 loop points, per-step agent roles, and core artifacts; emitted by `scripts/gen-loop-host-contract.cjs` from workflow markers (ADR-894 §3); consumed by `gen-capability-registry.cjs` | | `capability-registry.cjs` | Generated central Capability Registry — role-partitioned index of all co-located capability declarations; emitted by `scripts/gen-capability-registry.cjs` (ADR-894 §5) | +| `loop-resolver.cjs` | Loop Extension Point resolver — ADR-857 phase 3c registry-consuming query; filters `byLoopPoint` by config activation, renders active hooks as markdown, emits `{ point, activeHooks, rendered }` envelope; `gsd-tools loop render-hooks ` | --- diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index a7e3bbc41..e44086525 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -309,6 +309,7 @@ "learnings.cjs", "legacy-cleanup.cjs", "loop-host-contract.cjs", + "loop-resolver.cjs", "milestone.cjs", "model-catalog.cjs", "model-profiles.cjs", diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 31def503e..01bd82715 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -370,7 +370,7 @@ The `gsd-planner` agent is decomposed into a core agent plus reference modules t --- -## CLI Modules (100 shipped) +## CLI Modules (101 shipped) Full listing: `gsd-core/bin/lib/*.cjs`. @@ -420,6 +420,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`. | `learnings.cjs` | Cross-phase learnings extraction for `/gsd-extract-learnings` | | `legacy-cleanup.cjs` | Detect and remove leftover get-shit-done-cc artifacts; exports `planLegacyCleanup` (pure scan) and `applyLegacyCleanup` (thin IO applier) that root out stale files from the old package across every GSD-managed runtime config directory (#607) | | `loop-host-contract.cjs` | Generated Loop Host Contract — 12 loop points, per-step agent roles, and core artifacts for the five-step pipeline (discuss/plan/execute/verify/ship); emitted by `scripts/gen-loop-host-contract.cjs --write` (ADR-894 §3); consumed by `gen-capability-registry.cjs` | +| `loop-resolver.cjs` | Loop Extension Point resolver — ADR-857 phase 3c registry-consuming query; given a canonical loop point, filters `byLoopPoint` by config activation (`when` key traversal with prototype-pollution guard), returns `{ point, activeHooks, rendered }` envelope; `resolveLoopHooks` and `renderLoopHooks` are pure (no I/O); command surface: `gsd-tools loop render-hooks ` | | `milestone.cjs` | Milestone archival, requirements marking | | `model-catalog.cjs` | CJS adapter over the shared model catalog JSON; exports canonical runtime tier defaults, agent profile maps, alias maps, and routing metadata for all CLI consumers | | `model-profiles.cjs` | Backward-compatible profile helpers derived from `model-catalog.cjs`; no longer owns its own model table | diff --git a/eslint.config.mjs b/eslint.config.mjs index 876d53be2..644bf64aa 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -74,6 +74,7 @@ export default tseslint.config( 'gsd-core/bin/lib/config-schema.cjs', 'gsd-core/bin/lib/model-profiles.cjs', 'gsd-core/bin/lib/model-resolver.cjs', + 'gsd-core/bin/lib/loop-resolver.cjs', 'gsd-core/bin/lib/federated-config.cjs', 'gsd-core/bin/lib/installer-migrations/002-codex-legacy-hooks-json.cjs', 'gsd-core/bin/lib/installer-migrations/003-rename-get-shit-done-to-gsd-core.cjs', diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 7d50dffcd..33907e867 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -163,6 +163,12 @@ * learnings prune --older-than Remove entries older than duration (e.g. 90d) * learnings delete Delete a learning by ID * + * Loop Extension Point Queries (ADR-857 phase 3c): + * loop render-hooks Resolve + render active Capability hooks at a loop point + * Returns JSON envelope { point, activeHooks, rendered } + * Valid points: discuss:pre/post, plan:pre/post, + * execute:pre/wave:pre/wave:post/post, verify:pre/post, ship:pre/post + * * GSD-2 Migration: * from-gsd2 [--path ] [--force] [--dry-run] * Import a GSD-2 (.gsd/) project back to GSD v1 (.planning/) format @@ -201,6 +207,7 @@ const { routeVerifyCommand } = require('./lib/verify-command-router.cjs'); const { routeVerificationCommand } = require('./lib/verification-command-router.cjs'); const verification = require('./lib/verification.cjs'); const { routeInitCommand } = require('./lib/init-command-router.cjs'); +const loopResolver = require('./lib/loop-resolver.cjs'); const { routePhaseCommand } = require('./lib/phase-command-router.cjs'); const { routePhasesCommand } = require('./lib/phases-command-router.cjs'); const { routeValidateCommand } = require('./lib/validate-command-router.cjs'); @@ -379,7 +386,7 @@ async function main() { 'current-timestamp, detect-custom-files, docs-init, effort, extract-messages, find-phase, ' + 'from-gsd2, frontmatter, gap-analysis, generate-claude-md, generate-claude-profile, ' + 'generate-dev-preferences, generate-slug, graphify, history-digest, init, intel, ' + - 'classify-confidence, learnings, list-todos, milestone, package-legitimacy, phase, phase-plan-index, phases, profile-questionnaire, ' + + 'classify-confidence, learnings, list-todos, loop, milestone, package-legitimacy, phase, phase-plan-index, phases, profile-questionnaire, ' + 'profile-sample, progress, prompt-budget, requirements, research-plan, research-store, resolve-granularity, resolve-model, roadmap, scaffold, state, ' + 'task, template, validate, verify, verify-path-exists, verify-summary, workstream, worktree\n\n' + 'Global flags:\n' + @@ -1118,6 +1125,20 @@ async function runCommand(command, args, cwd, raw, defaultValue, originalCommand break; } + case 'loop': { + // loop render-hooks + const loopSubcommand = args[1]; + if (loopSubcommand === 'render-hooks') { + loopResolver.cmdLoopRenderHooks(cwd, args[2], raw, {}); + } else { + error( + `Unknown loop subcommand: ${loopSubcommand}. Available: render-hooks`, + core.ERROR_REASON ? core.ERROR_REASON.SDK_UNKNOWN_COMMAND : undefined, + ); + } + break; + } + case 'phase-plan-index': { phase.cmdPhasePlanIndex(cwd, args[1], raw); break; diff --git a/src/loop-resolver.cts b/src/loop-resolver.cts new file mode 100644 index 000000000..6dbedee3e --- /dev/null +++ b/src/loop-resolver.cts @@ -0,0 +1,533 @@ +/** + * Loop Resolver — ADR-857 phase 3c registry-consuming query + * + * Given a loop point (one of the 12 canonical points from loop-host-contract.cjs), + * filters the materialized Capability Registry by config activation and returns + * the active hooks as a JSON envelope with a rendered-markdown field. + * + * REGISTRY-ONLY: no workflow calls this yet (phase-6 cutover is out of scope). + * + * Command surface: gsd-tools loop render-hooks + * + * Exports (three things): + * resolveLoopHooks({ point, registry, config }) → { point, activeHooks } + * renderLoopHooks(resolved) → markdown string + * cmdLoopRenderHooks(cwd, point, raw, options) — I/O entry point + * + * Both pure functions (resolveLoopHooks, renderLoopHooks) take explicit + * registry/config arguments so they are trivially testable without I/O. + * + * Dependencies (leaf modules only — no core.cjs circular risk): + * - node:fs / node:path (raw config.json read for capability-key activation) + * - ./config-loader.cjs (loadConfig) + * - ./planning-workspace.cjs (planningDir — to locate config.json) + * - ./core.cjs (output, error) + * - loop-host-contract.cjs (CANONICAL_POINTS via LOOP_HOST_CONTRACT) + * - capability-registry.cjs (byLoopPoint, consumed at call time) + */ + +import fs from 'node:fs'; +import path from 'node:path'; + +// eslint-disable-next-line @typescript-eslint/no-require-imports +import core = require('./core.cjs'); +const { output: coreOutput, error: coreError } = core; + +// eslint-disable-next-line @typescript-eslint/no-require-imports +import configLoaderModule = require('./config-loader.cjs'); +const { loadConfig } = configLoaderModule; + +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planningWorkspaceMod = require('./planning-workspace.cjs'); +const { planningDir, planningRoot } = planningWorkspaceMod; + +// ─── Canonical points (derived from LOOP_HOST_CONTRACT — authoritative 12) ─── + +// FIX 2: Derive the authoritative canonical set from LOOP_HOST_CONTRACT so it +// cannot drift from the host contract. CANONICAL_POINTS_FALLBACK is kept as an +// alias for backward compatibility in tests and exports. +// eslint-disable-next-line @typescript-eslint/no-require-imports +const _loopHostContract = require('./loop-host-contract.cjs') as { LOOP_HOST_CONTRACT: Array<{ points: string[] }> }; +const CANONICAL_POINTS: ReadonlyArray = (() => { + try { + const contract = _loopHostContract.LOOP_HOST_CONTRACT; + if (Array.isArray(contract)) { + const pts: string[] = []; + for (const step of contract) { + if (step && Array.isArray(step.points)) { + for (const p of step.points) { + if (typeof p === 'string') pts.push(p); + } + } + } + if (pts.length > 0) return pts; + } + } catch { /* fall through to hardcoded fallback */ } + return [ + 'discuss:pre', + 'discuss:post', + 'plan:pre', + 'plan:post', + 'execute:pre', + 'execute:wave:pre', + 'execute:wave:post', + 'execute:post', + 'verify:pre', + 'verify:post', + 'ship:pre', + 'ship:post', + ]; +})(); + +// Alias for backward compatibility (tests import this name) +const CANONICAL_POINTS_FALLBACK: ReadonlyArray = CANONICAL_POINTS; + +// FIX 2: _getCanonicalPoints now returns the authoritative CANONICAL_POINTS set +// derived from LOOP_HOST_CONTRACT — not the registry's byLoopPoint keys. +// The registry's byLoopPoint is only used to READ hooks, not to define valid points. +function _getCanonicalPoints(_registry: Record): ReadonlyArray { + return CANONICAL_POINTS; +} + +// ─── Prototype-pollution guard (inline literal, CodeQL barrier) ─────────────── + +/** + * Traverse a dotted config key through a nested config object. + * E.g. "workflow.ui_phase" in { workflow: { ui_phase: true } } → { found: true, value: true } + * Returns { found: false } if any segment is a forbidden key or not an own property. + */ +function _getNestedConfigValue( + config: Record, + dotKey: string, +): { found: boolean; value: unknown } { + const segments = dotKey.split('.'); + let current: unknown = config; + for (const seg of segments) { + // Inline literal prototype-pollution guard (CodeQL barrier) + if (seg === '__proto__' || seg === 'constructor' || seg === 'prototype') { + return { found: false, value: undefined }; + } + if (typeof current !== 'object' || current === null) { + return { found: false, value: undefined }; + } + const cur = current as Record; + if (!Object.prototype.hasOwnProperty.call(cur, seg)) { + return { found: false, value: undefined }; + } + current = cur[seg]; + } + return { found: true, value: current }; +} + +// ─── Single-key activation resolver (FIX 1) ─────────────────────────────────── + +/** + * Warn-once set for raw config.json parse errors. + * Avoids noisy per-call stderr from a single malformed file. + */ +const _warnedRawConfigPaths = new Set(); + +/** + * Read a raw config.json file and perform a guarded nested-lookup of a single + * dotted key. Returns { found: false } if the file is missing (ENOENT) or if + * the key is absent/forbidden. On a genuine JSON parse error: warns once to + * stderr and returns { found: false } — never throws. + */ +function _readRawConfigKey( + filePath: string, + dotKey: string, +): { found: boolean; value: unknown } { + try { + const raw = fs.readFileSync(filePath, 'utf8'); + let parsed: Record; + try { + parsed = JSON.parse(raw) as Record; + } catch { + if (!_warnedRawConfigPaths.has(filePath)) { + _warnedRawConfigPaths.add(filePath); + try { + process.stderr.write( + `gsd-tools: warning: failed to parse ${filePath} as JSON — skipping for activation resolution\n`, + ); + } catch { /* stderr might be closed */ } + } + return { found: false, value: undefined }; + } + return _getNestedConfigValue(parsed, dotKey); + } catch { + // ENOENT (missing file) is expected → skip silently. All other errors → also skip (defensive). + return { found: false, value: undefined }; + } +} + +/** + * FIX 1: Resolve the effective value for a hook's `when` key using the + * four-level precedence: + * + * 1. loadConfig result (`config` arg) — guarded nested-lookup of the dotted key. + * This is the post-cutover federated path (covers keys that loadConfig now exposes). + * 2. Raw workstream `.planning/.../config.json` — guarded single-key lookup. + * Workstream wins over root (mirrors loadConfig inheritance). + * 3. Raw root `.planning/config.json` — guarded single-key lookup. + * 4. `registry.configSchema[when]?.default` — schema default. + * A `default: true` hook is active out-of-the-box without any config. + * 5. Absent → inactive (return false). + * + * Never constructs a merged object from raw JSON keys — only reads the single + * leaf value at the guarded dotted path. Prototype-pollution sink is eliminated. + */ +function _resolveActivationValue( + dotKey: string, + config: Record, + cwd: string | undefined, + registry: Record, +): boolean { + // Level 1: loadConfig result + const fromConfig = _getNestedConfigValue(config, dotKey); + if (fromConfig.found) return Boolean(fromConfig.value); + + // Level 2 + 3: raw config.json files (only when cwd is available) + if (cwd) { + // Level 2: workstream config (planningDir respects GSD_WORKSTREAM env) + const wsConfigPath = path.join(planningDir(cwd), 'config.json'); + // Level 3: root config (planningRoot = cwd/.planning always) + const rootConfigPath = path.join(planningRoot(cwd), 'config.json'); + + // Workstream wins over root (mirroring loadConfig root→workstream precedence: + // workstream overlays root, so workstream value takes precedence). + const fromWs = _readRawConfigKey(wsConfigPath, dotKey); + if (fromWs.found) return Boolean(fromWs.value); + + // Only read root if it differs from the workstream path (avoids double-read + // when no workstream is active and both paths resolve to the same file). + if (wsConfigPath !== rootConfigPath) { + const fromRoot = _readRawConfigKey(rootConfigPath, dotKey); + if (fromRoot.found) return Boolean(fromRoot.value); + } + } + + // Level 4: registry configSchema default + const schemaEntry = (registry['configSchema'] as Record | undefined)?.[dotKey]; + if (schemaEntry && typeof schemaEntry === 'object' && schemaEntry !== null) { + const def = (schemaEntry as Record)['default']; + if (def !== undefined) return Boolean(def); + } + + // Level 5: absent → inactive + return false; +} + +// ─── Types ──────────────────────────────────────────────────────────────────── + +interface HookRef { + skill?: string; + [key: string]: unknown; +} + +interface RawHook { + capId?: unknown; + point?: unknown; + ref?: unknown; + into?: unknown; + produces?: unknown; + consumes?: unknown; + when?: unknown; + onError?: unknown; + blocking?: unknown; + check?: unknown; +} + +type HookKind = 'step' | 'contribution' | 'gate'; + +interface ActiveHook { + capId: string; + kind: HookKind; + ref?: HookRef; + into?: string; + when?: string; + produces?: string[]; + consumes?: string[]; + blocking?: boolean; + check?: unknown; + onError?: string; +} + +interface ResolveLoopHooksInput { + point: string; + registry: Record; + config: Record; + /** Optional cwd — enables raw config.json fallback reads (FIX 1 precedence level 2). */ + cwd?: string; +} + +interface ResolveLoopHooksResult { + point: string; + activeHooks: ActiveHook[]; +} + +// ─── Pure resolver ───────────────────────────────────────────────────────────── + +/** + * Pure resolver: given a point, registry, and config, returns the active hooks. + * + * Throws if `point` is not one of the 12 canonical points (caller converts to + * core.error). Never throws for malformed registry/hook entries — skips and + * continues. + * + * Ordering: steps first, then contributions, then gates. Within each array, + * the materialized registry order is preserved. + * + * Activation: a hook with no `when` is always active. With `when` (dotted key), + * resolved against `config`; active iff truthy. Inactive hooks are filtered out. + */ +function resolveLoopHooks(input: ResolveLoopHooksInput): ResolveLoopHooksResult { + const { point, registry, config, cwd } = input; + + // Validate point + const canonicalPoints = _getCanonicalPoints(registry); + if (!canonicalPoints.includes(point)) { + throw new Error( + `Invalid loop point: "${point}". Valid points: ${canonicalPoints.join(', ')}`, + ); + } + + // Guard: registry missing byLoopPoint + const byLoopPoint = registry['byLoopPoint']; + if (!byLoopPoint || typeof byLoopPoint !== 'object' || Array.isArray(byLoopPoint)) { + return { point, activeHooks: [] }; + } + const byLoopPointMap = byLoopPoint as Record; + + // Guard: point missing in registry + const entry = byLoopPointMap[point]; + if (!entry || typeof entry !== 'object' || Array.isArray(entry)) { + return { point, activeHooks: [] }; + } + const entryMap = entry as Record; + + const activeHooks: ActiveHook[] = []; + + // 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); + } + + // Helper: safe string array + function toStringArray(v: unknown): string[] { + if (!Array.isArray(v)) return []; + return v.filter((x): x is string => typeof x === 'string'); + } + + // Process steps + const stepsRaw = entryMap['steps']; + const steps: RawHook[] = Array.isArray(stepsRaw) ? (stepsRaw as RawHook[]) : []; + for (const hook of steps) { + if (!hook || typeof hook !== 'object') continue; + if (!isActive(hook)) continue; + const capId = typeof hook['capId'] === 'string' ? hook['capId'] : ''; + const ref = (typeof hook['ref'] === 'object' && hook['ref'] !== null) + ? (hook['ref'] as HookRef) + : undefined; + const when = typeof hook['when'] === 'string' ? hook['when'] : undefined; + const produces = toStringArray(hook['produces']); + const consumes = toStringArray(hook['consumes']); + const onError = typeof hook['onError'] === 'string' ? hook['onError'] : undefined; + const active: ActiveHook = { capId, kind: 'step' }; + if (ref !== undefined) active.ref = ref; + if (when !== undefined) active.when = when; + if (produces.length > 0) active.produces = produces; + if (consumes.length > 0) active.consumes = consumes; + if (onError !== undefined) active.onError = onError; + activeHooks.push(active); + } + + // Process contributions + const contributionsRaw = entryMap['contributions']; + const contributions: RawHook[] = Array.isArray(contributionsRaw) ? (contributionsRaw as RawHook[]) : []; + for (const hook of contributions) { + if (!hook || typeof hook !== 'object') continue; + if (!isActive(hook)) continue; + const capId = typeof hook['capId'] === 'string' ? hook['capId'] : ''; + const into = typeof hook['into'] === 'string' ? hook['into'] : undefined; + const when = typeof hook['when'] === 'string' ? hook['when'] : undefined; + const produces = toStringArray(hook['produces']); + const consumes = toStringArray(hook['consumes']); + const onError = typeof hook['onError'] === 'string' ? hook['onError'] : undefined; + const active: ActiveHook = { capId, kind: 'contribution' }; + if (into !== undefined) active.into = into; + if (when !== undefined) active.when = when; + if (produces.length > 0) active.produces = produces; + if (consumes.length > 0) active.consumes = consumes; + if (onError !== undefined) active.onError = onError; + activeHooks.push(active); + } + + // Process gates + const gatesRaw = entryMap['gates']; + const gates: RawHook[] = Array.isArray(gatesRaw) ? (gatesRaw as RawHook[]) : []; + for (const hook of gates) { + if (!hook || typeof hook !== 'object') continue; + if (!isActive(hook)) continue; + const capId = typeof hook['capId'] === 'string' ? hook['capId'] : ''; + const when = typeof hook['when'] === 'string' ? hook['when'] : undefined; + const check = hook['check'] !== undefined ? hook['check'] : undefined; + const blocking = typeof hook['blocking'] === 'boolean' ? hook['blocking'] : undefined; + const onError = typeof hook['onError'] === 'string' ? hook['onError'] : undefined; + const active: ActiveHook = { capId, kind: 'gate' }; + if (when !== undefined) active.when = when; + if (check !== undefined) active.check = check; + if (blocking !== undefined) active.blocking = blocking; + if (onError !== undefined) active.onError = onError; + activeHooks.push(active); + } + + return { point, activeHooks }; +} + +// ─── Pure renderer ───────────────────────────────────────────────────────────── + +/** + * Pure renderer: given a resolved result, returns a deterministic markdown string. + * + * Empty active set → returns a "no active hooks" placeholder line. + * Steps: heading with ordinal + skill ref + capId, produces/consumes lines. + * Contributions: labeled block. + * Gates: check name, blocking flag, onError. + */ +function renderLoopHooks(resolved: ResolveLoopHooksResult): string { + const { point, activeHooks } = resolved; + + if (activeHooks.length === 0) { + return `_No active hooks at ${point}._`; + } + + const lines: string[] = []; + let stepOrdinal = 0; + + for (const hook of activeHooks) { + if (hook.kind === 'step') { + stepOrdinal += 1; + const refStr = hook.ref?.skill + ? `skill:${hook.ref.skill}` + : JSON.stringify(hook.ref ?? {}); + lines.push(`### Step ${stepOrdinal}: ${refStr} (${hook.capId})`); + if (hook.produces && hook.produces.length > 0) { + lines.push(`- produces: ${hook.produces.join(', ')}`); + } + if (hook.consumes && hook.consumes.length > 0) { + lines.push(`- consumes: ${hook.consumes.join(', ')}`); + } + if (hook.when) { + lines.push(`- when: \`${hook.when}\``); + } + if (hook.onError) { + lines.push(`- onError: ${hook.onError}`); + } + lines.push(''); + } else if (hook.kind === 'contribution') { + lines.push(``); + if (hook.produces && hook.produces.length > 0) { + lines.push(`- produces: ${hook.produces.join(', ')}`); + } + if (hook.consumes && hook.consumes.length > 0) { + lines.push(`- consumes: ${hook.consumes.join(', ')}`); + } + if (hook.when) { + lines.push(`- when: \`${hook.when}\``); + } + lines.push(''); + } else if (hook.kind === 'gate') { + let checkStr = '(none)'; + if (hook.check !== undefined && hook.check !== null) { + checkStr = typeof hook.check === 'object' + ? JSON.stringify(hook.check) + : typeof hook.check === 'string' || typeof hook.check === 'number' || typeof hook.check === 'boolean' + ? String(hook.check) + : '(complex)'; + } + lines.push(`**Gate** (${hook.capId}): check=${checkStr}, blocking=${String(hook.blocking ?? false)}, onError=${hook.onError ?? 'skip'}`); + if (hook.when) { + lines.push(`- when: \`${hook.when}\``); + } + lines.push(''); + } + } + + // Trim trailing blank line + while (lines.length > 0 && lines[lines.length - 1] === '') { + lines.pop(); + } + + return lines.join('\n'); +} + +// ─── I/O command handler ─────────────────────────────────────────────────────── + +/** + * Command entry point: load registry + config, resolve + render, emit envelope. + * + * Envelope: { point, activeHooks, rendered } + * On invalid point, emits core.error instead of throwing. + * + * Config note: FIX 1 replaced _loadMergedConfig (whole-config deep-merge) with a + * per-hook single-key activation resolver (_resolveActivationValue). The resolver + * checks loadConfig result first, then raw config.json files directly (workstream + * then root), then the registry's configSchema default. This eliminates the + * merged-object-from-untrusted-keys security concern and correctly handles + * pre-cutover keys like `workflow.ui_phase` that live in config.json but are not + * yet exposed through loadConfig's whitelist. + */ +function cmdLoopRenderHooks( + cwd: string, + point: string, + raw: boolean, + _options: Record = {}, +): void { + if (!point) { + coreError('loop render-hooks requires a argument. Valid points: ' + CANONICAL_POINTS.join(', ')); + return; + } + + // Load registry at call time (generated file, not at module load time) + // eslint-disable-next-line @typescript-eslint/no-require-imports + const registry = require('./capability-registry.cjs') as Record; + // FIX 1: Pass loadConfig result as `config` (level 1 of precedence); + // raw config.json reads (levels 2+3) happen per-hook inside _resolveActivationValue + // via the `cwd` argument passed to resolveLoopHooks. + const config = loadConfig(cwd); + + let resolved: ResolveLoopHooksResult; + try { + resolved = resolveLoopHooks({ point, registry, config, cwd }); + } catch (err: unknown) { + const msg = (err instanceof Error) ? err.message : String(err); + coreError(msg); + return; + } + + const rendered = renderLoopHooks(resolved); + const envelope = { + point: resolved.point, + activeHooks: resolved.activeHooks, + rendered, + }; + + coreOutput(envelope, raw); +} + +export = { + resolveLoopHooks, + renderLoopHooks, + cmdLoopRenderHooks, + // Exported for tests + _getNestedConfigValue, + _resolveActivationValue, + _readRawConfigKey, + CANONICAL_POINTS_FALLBACK, + CANONICAL_POINTS, +}; diff --git a/tests/loop-render-hooks.test.cjs b/tests/loop-render-hooks.test.cjs new file mode 100644 index 000000000..8e75784e9 --- /dev/null +++ b/tests/loop-render-hooks.test.cjs @@ -0,0 +1,777 @@ +'use strict'; + +/** + * loop-render-hooks.test.cjs — behavioral tests for loop-resolver.cjs. + * + * ADR-857 phase 3c. + * Uses node:test + node:assert/strict. + * Pure-function tests (resolveLoopHooks, renderLoopHooks) pass registry+config + * directly — no I/O. End-to-end tests use cmdLoopRenderHooks + a temp project. + */ + +const { describe, test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const { cleanup } = require('./helpers.cjs'); + +const { + resolveLoopHooks, + renderLoopHooks, + _getNestedConfigValue, + _resolveActivationValue, + _readRawConfigKey, + CANONICAL_POINTS_FALLBACK, + CANONICAL_POINTS, +} = require('../gsd-core/bin/lib/loop-resolver.cjs'); + +// The real registry for integration tests +const realRegistry = require('../gsd-core/bin/lib/capability-registry.cjs'); + +// ─── Synthetic registry fixtures ───────────────────────────────────────────── + +/** + * Build a minimal synthetic registry with a single step hook at a given point. + * Optionally include a configSchema for testing default-based activation. + */ +function makeRegistry({ point = 'plan:pre', steps = [], contributions = [], gates = {}, configSchema = {} } = {}) { + const byLoopPoint = {}; + for (const p of CANONICAL_POINTS_FALLBACK) { + byLoopPoint[p] = { steps: [], contributions: [], gates: [] }; + } + if (steps.length) byLoopPoint[point].steps = steps; + if (contributions.length) byLoopPoint[point].contributions = contributions; + if (gates[point]) byLoopPoint[point].gates = gates[point]; + return { byLoopPoint, configSchema }; +} + +// ─── Temp project helpers ───────────────────────────────────────────────────── + +let tmpProjectDir; +// A project with NO .planning/config.json — relies on schema defaults +let tmpEmptyProjectDir; +// A project where ui_phase is explicitly false in root config +let tmpFalseConfigProjectDir; + +before(() => { + tmpProjectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'loop-resolver-test-')); + const planningDir = path.join(tmpProjectDir, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + // Write minimal config.json with all UI flags enabled + fs.writeFileSync( + path.join(planningDir, 'config.json'), + JSON.stringify({ workflow: { ui_phase: true, ui_review: true, ui_safety_gate: true } }), + 'utf8', + ); + + // Empty project — no config.json: schema defaults drive activation + tmpEmptyProjectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'loop-resolver-empty-')); + fs.mkdirSync(path.join(tmpEmptyProjectDir, '.planning'), { recursive: true }); + + // False config project — ui_phase explicitly false in root config + tmpFalseConfigProjectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'loop-resolver-false-')); + const falseConfigPlanningDir = path.join(tmpFalseConfigProjectDir, '.planning'); + fs.mkdirSync(falseConfigPlanningDir, { recursive: true }); + fs.writeFileSync( + path.join(falseConfigPlanningDir, 'config.json'), + JSON.stringify({ workflow: { ui_phase: false, ui_review: false, ui_safety_gate: false } }), + 'utf8', + ); +}); + +after(() => { + if (tmpProjectDir) cleanup(tmpProjectDir); + if (tmpEmptyProjectDir) cleanup(tmpEmptyProjectDir); + if (tmpFalseConfigProjectDir) cleanup(tmpFalseConfigProjectDir); +}); + +// ─── 1. Canonical-point validation ─────────────────────────────────────────── + +describe('canonical point validation', () => { + test('all 12 canonical points are accepted by resolveLoopHooks with empty registry', () => { + const emptyRegistry = makeRegistry(); + const config = {}; + for (const p of CANONICAL_POINTS_FALLBACK) { + const result = resolveLoopHooks({ point: p, registry: emptyRegistry, config }); + assert.strictEqual(result.point, p); + assert.deepEqual(result.activeHooks, []); + } + }); + + test('12 canonical points total', () => { + assert.strictEqual(CANONICAL_POINTS_FALLBACK.length, 12); + }); + + test('invalid point throws with a clear message', () => { + const emptyRegistry = makeRegistry(); + assert.throws( + () => resolveLoopHooks({ point: 'plan:mid', registry: emptyRegistry, config: {} }), + (err) => { + assert.ok(err instanceof Error); + assert.match(err.message, /Invalid loop point/); + assert.match(err.message, /plan:mid/); + return true; + }, + ); + }); + + test('empty string point throws', () => { + const emptyRegistry = makeRegistry(); + assert.throws( + () => resolveLoopHooks({ point: '', registry: emptyRegistry, config: {} }), + /Invalid loop point/, + ); + }); + + test('close typo throws', () => { + const emptyRegistry = makeRegistry(); + assert.throws( + () => resolveLoopHooks({ point: 'plan:pre ', registry: emptyRegistry, config: {} }), + /Invalid loop point/, + ); + }); + + // FIX 2: non-canonical point rejected even if the registry has it as a byLoopPoint key + test('non-canonical point in registry byLoopPoint is still rejected', () => { + // Craft a registry that has a synthetic non-canonical key in byLoopPoint + const registry = { + byLoopPoint: { + // All canonical points (needed so the registry is well-formed) + ...Object.fromEntries(CANONICAL_POINTS_FALLBACK.map(p => [p, { steps: [], contributions: [], gates: [] }])), + // A non-canonical key that a malformed registry might inject + 'inject:arbitrary': { steps: [{ capId: 'evil', ref: { skill: 'bad' } }], contributions: [], gates: [] }, + }, + }; + assert.throws( + () => resolveLoopHooks({ point: 'inject:arbitrary', registry, config: {} }), + /Invalid loop point/, + ); + }); + + // FIX 2: all 12 canonical points are listed in the error message + test('invalid point error lists the canonical 12', () => { + const emptyRegistry = makeRegistry(); + assert.throws( + () => resolveLoopHooks({ point: 'not:real', registry: emptyRegistry, config: {} }), + (err) => { + assert.ok(err instanceof Error); + for (const p of CANONICAL_POINTS_FALLBACK) { + assert.ok(err.message.includes(p), `Expected "${p}" in error message: ${err.message}`); + } + return true; + }, + ); + }); + + // CANONICAL_POINTS is derived from LOOP_HOST_CONTRACT, not from registry keys + test('CANONICAL_POINTS and CANONICAL_POINTS_FALLBACK are the same 12 points', () => { + assert.deepEqual(CANONICAL_POINTS, CANONICAL_POINTS_FALLBACK); + assert.strictEqual(CANONICAL_POINTS.length, 12); + }); +}); + +// ─── 2. Activation tests ───────────────────────────────────────────────────── + +describe('activation filter', () => { + test('hook with no "when" is always active', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' } }], + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 1); + assert.strictEqual(result.activeHooks[0].capId, 'test-cap'); + }); + + test('hook with when="mytool.on", config{mytool:{on:true}} → active', () => { + 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); + assert.strictEqual(result.activeHooks[0].kind, 'step'); + }); + + test('hook with when="mytool.on", config{mytool:{on:false}} → filtered', () => { + 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('hook with when="mytool.on", config{} (absent key) → filtered', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on' }], + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0); + }); + + test('hook with when="mytool.on", config{mytool:{}} → filtered (key absent)', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on' }], + }); + const config = { mytool: {} }; + const result = resolveLoopHooks({ point: 'plan:pre', registry, config }); + assert.strictEqual(result.activeHooks.length, 0); + }); + + // FIX 3: non-string `when` → INACTIVE (not always-active) + test('hook with when=true (boolean) → inactive (FIX 3: malformed non-string when)', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: true }], + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0, 'non-string when=true must be treated as inactive'); + }); + + test('hook with when=42 (number) → inactive (FIX 3)', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 42 }], + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0, 'non-string when=42 must be inactive'); + }); + + test('hook with when={} (object) → inactive (FIX 3)', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: {} }], + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0, 'non-string when={} must be inactive'); + }); + + // FIX 4: configSchema default=true → active with absent config (no cwd → level 4 applies) + test('configSchema default=true + absent config → active', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on' }], + configSchema: { + 'mytool.on': { type: 'boolean', default: true, description: 'Enable mytool.' }, + }, + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 1, 'schema default=true should activate the hook'); + assert.strictEqual(result.activeHooks[0].capId, 'test-cap'); + }); + + // FIX 4: configSchema default=false → inactive with absent config + test('configSchema default=false + absent config → inactive', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on' }], + configSchema: { + 'mytool.on': { type: 'boolean', default: false, description: 'Disabled by default.' }, + }, + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0, 'schema default=false should keep hook inactive'); + }); + + // FIX 4: configSchema default=true but explicit config override=false → inactive (config wins) + test('configSchema default=true but config override false → inactive (config wins)', () => { + const registry = makeRegistry({ + steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on' }], + configSchema: { + 'mytool.on': { type: 'boolean', default: true, description: 'Enabled by default.' }, + }, + }); + const config = { mytool: { on: false } }; + const result = resolveLoopHooks({ point: 'plan:pre', registry, config }); + assert.strictEqual(result.activeHooks.length, 0, 'explicit config=false overrides schema default=true'); + }); +}); + +// ─── 3. UI pilot integration tests ─────────────────────────────────────────── + +describe('UI pilot integration', () => { + test('plan:pre with workflow.ui_phase=true → ui-phase step active', () => { + const config = { workflow: { ui_phase: true, ui_review: true, ui_safety_gate: true } }; + const result = resolveLoopHooks({ point: 'plan:pre', registry: realRegistry, config }); + const uiStep = result.activeHooks.find(h => h.capId === 'ui' && h.kind === 'step'); + assert.ok(uiStep, 'Expected ui step at plan:pre'); + assert.deepEqual(uiStep.ref, { skill: 'ui-phase' }); + assert.ok(Array.isArray(uiStep.produces)); + assert.ok(uiStep.produces.includes('UI-SPEC.md')); + }); + + test('plan:pre with workflow.ui_phase=false → ui-phase step filtered', () => { + const config = { workflow: { ui_phase: false, ui_review: true, ui_safety_gate: true } }; + const result = resolveLoopHooks({ point: 'plan:pre', registry: realRegistry, config }); + const uiStep = result.activeHooks.find(h => h.capId === 'ui' && h.kind === 'step'); + assert.strictEqual(uiStep, undefined, 'Expected ui step to be filtered'); + }); + + // FIX 4 INVERSION: empty config + real registry → ui-phase IS active (schema default=true) + test('plan:pre with empty config + real registry → ui-phase step active by default (FIX 4)', () => { + // realRegistry has configSchema['workflow.ui_phase'].default === true + // So with no config and no cwd, the schema default kicks in → active + const result = resolveLoopHooks({ point: 'plan:pre', registry: realRegistry, config: {} }); + const uiStep = result.activeHooks.find(h => h.capId === 'ui' && h.kind === 'step'); + assert.ok( + uiStep, + 'Expected ui step to be active by default (configSchema.default=true). Got: ' + + JSON.stringify(result.activeHooks), + ); + assert.strictEqual(uiStep.when, 'workflow.ui_phase'); + }); + + test('execute:wave:post with workflow.ui_safety_gate=true → ui gate active', () => { + const config = { workflow: { ui_phase: true, ui_review: true, ui_safety_gate: true } }; + const result = resolveLoopHooks({ point: 'execute:wave:post', registry: realRegistry, config }); + const uiGate = result.activeHooks.find(h => h.capId === 'ui' && h.kind === 'gate'); + assert.ok(uiGate, 'Expected ui gate at execute:wave:post'); + assert.strictEqual(uiGate.blocking, true); + assert.strictEqual(uiGate.onError, 'halt'); + }); + + test('execute:wave:post with workflow.ui_safety_gate=false → ui gate filtered', () => { + const config = { workflow: { ui_phase: true, ui_review: true, ui_safety_gate: false } }; + const result = resolveLoopHooks({ point: 'execute:wave:post', registry: realRegistry, config }); + const uiGate = result.activeHooks.find(h => h.capId === 'ui' && h.kind === 'gate'); + assert.strictEqual(uiGate, undefined, 'Expected ui gate to be filtered'); + }); + + // FIX 4: execute:wave:post with empty config → ui gate active by schema default + test('execute:wave:post with empty config → ui gate active by schema default', () => { + const result = resolveLoopHooks({ point: 'execute:wave:post', registry: realRegistry, config: {} }); + const uiGate = result.activeHooks.find(h => h.capId === 'ui' && h.kind === 'gate'); + assert.ok(uiGate, 'Expected ui gate active by default (configSchema.default=true)'); + assert.strictEqual(uiGate.blocking, true); + }); +}); + +// ─── 4. Ordering tests ──────────────────────────────────────────────────────── + +describe('hook ordering', () => { + test('steps appear before contributions before gates', () => { + const registry = makeRegistry({ + point: 'plan:pre', + steps: [{ capId: 'c1', point: 'plan:pre', ref: { skill: 'sk1' } }], + contributions: [{ capId: 'c2', point: 'plan:pre', into: 'planner' }], + gates: { 'plan:pre': [{ capId: 'c3', point: 'plan:pre', check: { query: 'some-gate' }, blocking: false }] }, + }); + const config = {}; + const result = resolveLoopHooks({ point: 'plan:pre', registry, config }); + assert.strictEqual(result.activeHooks.length, 3); + assert.strictEqual(result.activeHooks[0].kind, 'step'); + assert.strictEqual(result.activeHooks[1].kind, 'contribution'); + assert.strictEqual(result.activeHooks[2].kind, 'gate'); + }); + + test('within steps, registry order is preserved', () => { + const registry = makeRegistry({ + point: 'plan:pre', + steps: [ + { capId: 'cap-a', point: 'plan:pre', ref: { skill: 'a' } }, + { capId: 'cap-b', point: 'plan:pre', ref: { skill: 'b' } }, + { capId: 'cap-c', point: 'plan:pre', ref: { skill: 'c' } }, + ], + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.deepEqual(result.activeHooks.map(h => h.capId), ['cap-a', 'cap-b', 'cap-c']); + }); +}); + +// ─── 5. Envelope shape ──────────────────────────────────────────────────────── + +describe('envelope shape', () => { + test('envelope has point, activeHooks, rendered from renderLoopHooks', () => { + const registry = makeRegistry({ + steps: [{ capId: 'cap-a', point: 'plan:pre', ref: { skill: 'my-skill' }, produces: ['A.md'], consumes: ['B.md'] }], + }); + const resolved = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + const rendered = renderLoopHooks(resolved); + assert.strictEqual(resolved.point, 'plan:pre'); + assert.ok(Array.isArray(resolved.activeHooks)); + assert.strictEqual(typeof rendered, 'string'); + }); + + test('empty activeHooks → rendered is non-empty placeholder string', () => { + const registry = makeRegistry(); // all empty + const resolved = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + const rendered = renderLoopHooks(resolved); + assert.strictEqual(resolved.activeHooks.length, 0); + assert.ok(rendered.length > 0, 'rendered should be a non-empty placeholder'); + assert.match(rendered, /plan:pre/); + }); + + test('rendered contains hook content when hooks are active', () => { + const registry = makeRegistry({ + steps: [{ capId: 'ui', point: 'plan:pre', ref: { skill: 'ui-phase' }, produces: ['UI-SPEC.md'], consumes: ['CONTEXT.md'], when: 'workflow.ui_phase', onError: 'skip' }], + }); + const config = { workflow: { ui_phase: true } }; + const resolved = resolveLoopHooks({ point: 'plan:pre', registry, config }); + const rendered = renderLoopHooks(resolved); + assert.match(rendered, /ui-phase/); + assert.match(rendered, /ui/); + assert.match(rendered, /UI-SPEC\.md/); + }); + + test('rendered for UI pilot at plan:pre with all flags on', () => { + const config = { workflow: { ui_phase: true, ui_review: true, ui_safety_gate: true } }; + const resolved = resolveLoopHooks({ point: 'plan:pre', registry: realRegistry, config }); + const rendered = renderLoopHooks(resolved); + assert.match(rendered, /ui-phase/); + assert.match(rendered, /UI-SPEC\.md/); + }); +}); + +// ─── 6. Malformed registry resilience ──────────────────────────────────────── + +describe('malformed registry resilience', () => { + test('missing byLoopPoint → no throw, empty activeHooks', () => { + const badRegistry = {}; // no byLoopPoint + // No throw — but point validation falls back to CANONICAL_POINTS_FALLBACK + const result = resolveLoopHooks({ point: 'plan:pre', registry: badRegistry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0); + }); + + test('null hook in steps array → skipped', () => { + const registry = makeRegistry({ + steps: [null, { capId: 'ok', point: 'plan:pre', ref: { skill: 'ok-skill' } }, undefined], + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 1); + assert.strictEqual(result.activeHooks[0].capId, 'ok'); + }); + + test('byLoopPoint[point] missing arrays → no throw, empty result', () => { + const registry = { byLoopPoint: { 'plan:pre': {} } }; // no steps/contributions/gates keys + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0); + }); + + test('byLoopPoint[point] has non-array steps → treated as empty', () => { + const registry = { byLoopPoint: { 'plan:pre': { steps: 'bad', contributions: [], gates: [] } } }; + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0); + }); + + test('byLoopPoint[point] is null → no throw, empty result', () => { + const registry = { byLoopPoint: { 'plan:pre': null } }; + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0); + }); +}); + +// ─── 7. Prototype-pollution guard ──────────────────────────────────────────── + +describe('prototype-pollution guard', () => { + test('when="__proto__.x" does not pollute Object.prototype', () => { + const registry = makeRegistry({ + steps: [{ capId: 'attacker', point: 'plan:pre', ref: { skill: 'evil' }, when: '__proto__.x' }], + }); + const config = { x: 'injected' }; + // Should not throw and should not activate (guard returns found:false) + const result = resolveLoopHooks({ point: 'plan:pre', registry, config }); + assert.strictEqual(result.activeHooks.length, 0); + // Object.prototype must not be polluted + assert.strictEqual(({}).x, undefined); + }); + + test('when="constructor.x" does not pollute', () => { + const registry = makeRegistry({ + steps: [{ capId: 'attacker', point: 'plan:pre', ref: { skill: 'evil' }, when: 'constructor.x' }], + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0); + }); + + test('when="prototype.x" does not pollute', () => { + const registry = makeRegistry({ + steps: [{ capId: 'attacker', point: 'plan:pre', ref: { skill: 'evil' }, when: 'prototype.x' }], + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(result.activeHooks.length, 0); + }); + + test('_getNestedConfigValue: __proto__ segment returns found:false', () => { + const r = _getNestedConfigValue({}, '__proto__.x'); + assert.strictEqual(r.found, false); + }); + + test('_getNestedConfigValue: constructor segment returns found:false', () => { + const r = _getNestedConfigValue({}, 'constructor.toString'); + assert.strictEqual(r.found, false); + }); + + test('_getNestedConfigValue: normal dotted key traversal works', () => { + const config = { workflow: { ui_phase: true } }; + const r = _getNestedConfigValue(config, 'workflow.ui_phase'); + assert.strictEqual(r.found, true); + assert.strictEqual(r.value, true); + }); + + // FIX 4: raw config.json with __proto__ key does not pollute via _readRawConfigKey + test('raw config.json with "__proto__" key does not pollute Object.prototype', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'loop-resolver-proto-')); + try { + // Write a raw config.json containing __proto__ at top level and nested + // (JSON.parse of {"__proto__":{"x":"polluted"}} does NOT set prototype in modern Node, + // but we verify our guarded traversal returns found:false for such keys) + const maliciousConfig = '{"__proto__":{"x":"polluted"},"workflow":{"ui_phase":true}}'; + fs.writeFileSync(path.join(tmpDir, 'config.json'), maliciousConfig, 'utf8'); + // _readRawConfigKey with '__proto__.x' should return found:false (guard) + const r1 = _readRawConfigKey(path.join(tmpDir, 'config.json'), '__proto__.x'); + assert.strictEqual(r1.found, false, '__proto__ lookup must be guarded'); + // Normal key should work + const r2 = _readRawConfigKey(path.join(tmpDir, 'config.json'), 'workflow.ui_phase'); + assert.strictEqual(r2.found, true); + assert.strictEqual(r2.value, true); + // Object.prototype must not be polluted + assert.strictEqual(({}).x, undefined); + } finally { + cleanup(tmpDir); + } + }); + + // FIX 4: _resolveActivationValue with cwd pointing to project with __proto__ config key + test('_resolveActivationValue: raw config with __proto__ key does not pollute', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'loop-resolver-proto2-')); + try { + const planningDir = path.join(tmpDir, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + fs.writeFileSync( + path.join(planningDir, 'config.json'), + '{"__proto__":{"y":"polluted2"}}', + 'utf8', + ); + const registry = makeRegistry({ + steps: [{ capId: 'test', point: 'plan:pre', ref: { skill: 'sk' }, when: '__proto__.y' }], + }); + const result = resolveLoopHooks({ point: 'plan:pre', registry, config: {}, cwd: tmpDir }); + assert.strictEqual(result.activeHooks.length, 0, '__proto__ when must be inactive'); + assert.strictEqual(({}).y, undefined, 'Object.prototype.y must not be polluted'); + } finally { + cleanup(tmpDir); + } + }); +}); + +// ─── 7b. Raw config.json override paths (FIX 4) ────────────────────────────── + +describe('raw config.json override paths (FIX 4)', () => { + // FIX 4: user sets workflow.ui_phase=false in root config.json → hook filtered + test('root config.json with ui_phase=false overrides schema default=true → inactive', () => { + // tmpFalseConfigProjectDir has .planning/config.json { workflow: { ui_phase: false } } + const result = resolveLoopHooks({ + point: 'plan:pre', + registry: realRegistry, + config: {}, // empty loadConfig result (simulating pre-cutover) + cwd: tmpFalseConfigProjectDir, + }); + const uiStep = result.activeHooks.find(h => h.capId === 'ui' && h.kind === 'step'); + assert.strictEqual( + uiStep, + undefined, + 'root config.json override false must beat schema default=true', + ); + }); + + // FIX 4: root config.json with ui_phase=true (explicit) → hook active + test('root config.json with ui_phase=true → active (raw config read path)', () => { + // tmpProjectDir has .planning/config.json { workflow: { ui_phase: true } } + const result = resolveLoopHooks({ + point: 'plan:pre', + registry: realRegistry, + config: {}, // empty loadConfig result (simulating pre-cutover) + cwd: tmpProjectDir, + }); + const uiStep = result.activeHooks.find(h => h.capId === 'ui' && h.kind === 'step'); + assert.ok(uiStep, 'root config.json ui_phase=true should activate hook'); + }); + + // FIX 4: no config.json at all → falls through to schema default=true → active + test('no config.json → schema default=true → hook active', () => { + // tmpEmptyProjectDir has .planning/ directory but no config.json + const result = resolveLoopHooks({ + point: 'plan:pre', + registry: realRegistry, + config: {}, // empty loadConfig result + cwd: tmpEmptyProjectDir, + }); + const uiStep = result.activeHooks.find(h => h.capId === 'ui' && h.kind === 'step'); + assert.ok(uiStep, 'no config.json → schema default=true → hook should be active'); + }); + + // FIX 4: _readRawConfigKey returns found:false for missing file (ENOENT — silent) + test('_readRawConfigKey: missing file → found:false, no throw', () => { + const result = _readRawConfigKey('/nonexistent/path/config.json', 'workflow.ui_phase'); + assert.strictEqual(result.found, false); + }); + + // FIX 4: _readRawConfigKey returns found:false for malformed JSON, warns once + test('_readRawConfigKey: malformed JSON → found:false, no throw', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'loop-resolver-malformed-')); + try { + const malformedPath = path.join(tmpDir, 'config.json'); + fs.writeFileSync(malformedPath, '{ invalid json }', 'utf8'); + const result = _readRawConfigKey(malformedPath, 'workflow.ui_phase'); + assert.strictEqual(result.found, false, 'malformed JSON should return found:false'); + } finally { + cleanup(tmpDir); + } + }); +}); + +// ─── 8. Renderer tests ──────────────────────────────────────────────────────── + +describe('renderLoopHooks', () => { + test('step hook renders skill ref, capId, produces, consumes', () => { + const resolved = { + point: 'plan:pre', + activeHooks: [{ + capId: 'ui', + kind: 'step', + ref: { skill: 'ui-phase' }, + when: 'workflow.ui_phase', + produces: ['UI-SPEC.md'], + consumes: ['CONTEXT.md'], + onError: 'skip', + }], + }; + const rendered = renderLoopHooks(resolved); + assert.match(rendered, /Step 1/); + assert.match(rendered, /skill:ui-phase/); + assert.match(rendered, /\(ui\)/); + assert.match(rendered, /UI-SPEC\.md/); + assert.match(rendered, /CONTEXT\.md/); + assert.match(rendered, /workflow\.ui_phase/); + assert.match(rendered, /skip/); + }); + + test('contribution hook renders into role', () => { + const resolved = { + point: 'plan:pre', + activeHooks: [{ + capId: 'contrib-cap', + kind: 'contribution', + into: 'planner', + }], + }; + const rendered = renderLoopHooks(resolved); + assert.match(rendered, /contribution/); + assert.match(rendered, /contrib-cap/); + assert.match(rendered, /planner/); + }); + + test('gate hook renders check, blocking, onError', () => { + const resolved = { + point: 'execute:wave:post', + activeHooks: [{ + capId: 'ui', + kind: 'gate', + check: { query: 'ui.safety-gate' }, + blocking: true, + onError: 'halt', + }], + }; + const rendered = renderLoopHooks(resolved); + assert.match(rendered, /Gate/); + assert.match(rendered, /ui/); + assert.match(rendered, /blocking=true/); + assert.match(rendered, /halt/); + }); + + test('multiple hooks in order render with correct ordinals', () => { + const resolved = { + point: 'plan:pre', + activeHooks: [ + { capId: 'cap-a', kind: 'step', ref: { skill: 'a' }, produces: ['A.md'], consumes: [] }, + { capId: 'cap-b', kind: 'step', ref: { skill: 'b' }, produces: ['B.md'], consumes: ['A.md'] }, + ], + }; + const rendered = renderLoopHooks(resolved); + assert.match(rendered, /Step 1/); + assert.match(rendered, /Step 2/); + const idx1 = rendered.indexOf('Step 1'); + const idx2 = rendered.indexOf('Step 2'); + assert.ok(idx1 < idx2, 'Step 1 should appear before Step 2'); + }); + + test('empty hooks returns placeholder containing the point name', () => { + const rendered = renderLoopHooks({ point: 'ship:post', activeHooks: [] }); + assert.match(rendered, /ship:post/); + assert.ok(rendered.length > 0); + }); + + test('rendered is deterministic (same input → same output)', () => { + const config = { workflow: { ui_phase: true, ui_review: true, ui_safety_gate: true } }; + const resolved = resolveLoopHooks({ point: 'plan:pre', registry: realRegistry, config }); + const r1 = renderLoopHooks(resolved); + const r2 = renderLoopHooks(resolved); + assert.strictEqual(r1, r2); + }); +}); + +// ─── 9. End-to-end cmdLoopRenderHooks (via gsd-tools subprocess) ───────────── + +const { spawnSync } = require('node:child_process'); +const ROOT = path.resolve(__dirname, '..'); +const GSD_TOOLS = path.join(ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs'); + +describe('cmdLoopRenderHooks end-to-end (via gsd-tools)', () => { + test('loop render-hooks plan:pre returns JSON envelope with ui-phase step active', () => { + const result = spawnSync( + process.execPath, + [GSD_TOOLS, 'loop', 'render-hooks', 'plan:pre', '--cwd', tmpProjectDir], + { cwd: ROOT, encoding: 'utf8' }, + ); + assert.strictEqual(result.status, 0, 'Expected exit 0. stderr: ' + (result.stderr || '')); + const envelope = JSON.parse(result.stdout.trim()); + assert.strictEqual(envelope.point, 'plan:pre'); + assert.ok(Array.isArray(envelope.activeHooks)); + assert.strictEqual(typeof envelope.rendered, 'string'); + // With ui_phase=true in tmpProjectDir config, ui-phase step should be active + const uiStep = envelope.activeHooks.find(h => h.capId === 'ui' && h.kind === 'step'); + assert.ok(uiStep, 'Expected ui step in activeHooks. Got: ' + JSON.stringify(envelope.activeHooks)); + assert.match(envelope.rendered, /ui-phase/); + }); + + // FIX 4: schema-default activation — no config.json in project → ui-phase step active by default + test('loop render-hooks plan:pre with no config.json → ui-phase step active by schema default', () => { + const result = spawnSync( + process.execPath, + [GSD_TOOLS, 'loop', 'render-hooks', 'plan:pre', '--cwd', tmpEmptyProjectDir], + { cwd: ROOT, encoding: 'utf8' }, + ); + assert.strictEqual(result.status, 0, 'Expected exit 0. stderr: ' + (result.stderr || '')); + const envelope = JSON.parse(result.stdout.trim()); + const uiStep = envelope.activeHooks.find(h => h.capId === 'ui' && h.kind === 'step'); + assert.ok( + uiStep, + 'Expected ui step active by default. Got: ' + JSON.stringify(envelope.activeHooks), + ); + assert.match(envelope.rendered, /ui-phase/); + }); + + // FIX 4: explicit false in config.json overrides schema default + test('loop render-hooks plan:pre with ui_phase=false in config.json → ui-phase step absent', () => { + const result = spawnSync( + process.execPath, + [GSD_TOOLS, 'loop', 'render-hooks', 'plan:pre', '--cwd', tmpFalseConfigProjectDir], + { cwd: ROOT, encoding: 'utf8' }, + ); + assert.strictEqual(result.status, 0, 'Expected exit 0. stderr: ' + (result.stderr || '')); + const envelope = JSON.parse(result.stdout.trim()); + const uiStep = envelope.activeHooks.find(h => h.capId === 'ui' && h.kind === 'step'); + assert.strictEqual( + uiStep, + undefined, + 'ui-phase step should be absent when config.json sets ui_phase=false', + ); + }); + + test('loop render-hooks invalid-point exits non-zero', () => { + const result = spawnSync( + process.execPath, + [GSD_TOOLS, 'loop', 'render-hooks', 'plan:mid', '--cwd', tmpProjectDir], + { cwd: ROOT, encoding: 'utf8' }, + ); + assert.notStrictEqual(result.status, 0, 'Expected non-zero exit for invalid point'); + assert.match(result.stderr, /plan:mid|Invalid loop point/); + }); +});