From 0408276791e62f70d98ee81e189a114c199278a9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Jul 2026 08:52:56 -0400 Subject: [PATCH] chore(#2797): federate reviewer config keys off the central schema (#2841) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(#2797): federate reviewer config keys off the central schema Phase 4 of epic #2782 (ADR-2782 D9, config half). Runs AFTER 5a per the ADR's swap amendment: a federated config slice lives inside a capabilities//capability.json, and three of the five key families had no capability directory until 5a created them. Four key families move to the lanes that use them; the central-schema removal and the federated addition land in this one commit because the exclusivity invariant fails the build on a key present in both. review.max_prompt_tokens, review.default_reviewers and review.reviewer_instances describe policy ACROSS lanes and stay central. Two things the issue did not name, both found while building it: 1. THE EXCLUSIVITY GATE WAS BLIND TO PATTERNS. It compared federated keys against manifest.validKeys only, and two of the four families (review.models., review.max_prompt_tokens_per_reviewer.) were pattern-backed. That is not cosmetic: isCentralConfigKey consults those patterns and mergeFederatedConfig skips every key for which it returns true, so declaring a slice while the pattern survived would have shipped an INERT slice behind a green gate — the exact half-migrated shape the invariant exists to prevent. The gate now loads the patterns from the same manifest the runtime reads. 2. AN UNSET PER-LANE BUDGET NOW RESOLVES TO 0, NOT NOT-FOUND, because a federated key always resolves to its declared default. The three fallback guards in review.md checked only empty-or-"null", so a user who set the GLOBAL review.max_prompt_tokens would have silently lost trimming on the HTTP lanes. The guards now treat 0 as unset. D9 says review.models. is owned by "the lane whose slug it names". That is false for one lane: the shipped key is review.models.agy while the slug is antigravity. Ownership follows the lane; the key name is preserved, because renaming would break every config that sets it. Existing tests updated rather than left asserting the old world: config-get on a cleared federated key yields empty instead of not-found (what #2046 actually protects — never persisting the literal "null" — is unchanged and still asserted); the config-schema dynamic pattern representative moves to reviewer_instances; the prototype-pollution guard case moves to a surviving dynamic prefix so alert #26 keeps its coverage, with a new case asserting the old key is now rejected earlier; and Phase 2's harvest-widening inertness assertion becomes an ownership assertion, since Phase 4 is what consumes it. Co-Authored-By: Claude Opus 5 * fix(#2797): use a -1 sentinel so an explicit per-lane budget of 0 survives A federated config key always resolves to its declared default, so an unset per-lane prompt budget needed a value the workflow could treat as 'not configured'. The first cut used 0 — which is wrong: 0 is already a LEGITIMATE per-lane budget meaning 'do not trim this lane' (the early-return guard in prepare_trimmed_prompt_for_reviewer). Treating it as unset would have silently switched a user who deliberately disabled trimming for one lane onto the global budget. The sentinel is now -1, which is not a valid token budget, so all three states stay distinguishable: unset falls back to global, an explicit 0 disables trimming for that lane, and an explicit N is used. Locked by three CLI round-trip tests. Surfaced by the isolated security reviewer before it crashed mid-run; verified independently against the shipped trim guard rather than taken on trust. Co-Authored-By: Claude Opus 5 * fix(#2797): update central-registration assertions and stay under the review.md cap The remote runner caught both; my local sweep missed the files. 1. tests/plan-review-convergence.test.cjs asserted the three local-server host keys are in VALID_CONFIG_KEYS. They are federated to their lane capabilities now, and the exclusivity invariant forbids a key living in both places. What #2306-local actually protects is that config-set ACCEPTS them, so that is what is asserted — via isValidConfigKey, the predicate config-set itself uses, which spans central and federated. A second assertion pins federated ownership, so a silent reversion back to the central schema fails too. 2. review.md exceeded the LARGE tier hard cap (62583 > 61440). That cap is a red line, not a budget to raise. The three per-lane budget guard comments were near-identical; condensed to one terse line each. 61371 bytes, 69 to spare. Real extraction to workflows/review/modes/ is Phase 5b/6 work. Co-Authored-By: Claude Opus 5 * fix(#2797): fail closed on a broken config-schema manifest; reconcile stale docs Isolated security review findings. MAJOR — loadCentralConfigPatterns failed OPEN. It swallowed a JSON parse error and returned [], while its sibling loadCentralConfigKeys, reading the SAME file, writes to stderr and throws ExitError(1) on that identical failure class. Fail-open here defeats the gate this function exists to feed: with zero patterns, validateCrossCapability's pattern-collision check silently passes and an inert federated slice ships green. It was masked in the one production call site only because loadCentralConfigKeys runs first against the same path — a coincidence of ordering, not a guarantee, and this function is exported and called standalone. The two now share a contract: ENOENT is the legitimate absent case, anything else throws loudly. A single unparseable PATTERN is still skipped, which degrades to "checked less" rather than blocking every build. The branch had zero coverage; it now has two tests (malformed JSON, EISDIR). MINOR — docs/CONFIGURATION.md still listed review.models.qwen and review.models.cursor as settable, ~770 lines below this PR's own new Ownership section. Those lanes take no model flag, so they declare no model key and config-set now rejects them. Rows removed; the missing review.models.agy row added; the per-reviewer budget row corrected to name only the lanes that own a budget key, and to document that a per-lane 0 disables trimming for that lane. Also fixes a shadowed "raw" binding introduced by the fail-closed change, which made the generator unrequirable — caught immediately by its own --check. Co-Authored-By: Claude Opus 5 * chore(#2797): backfill changeset pr number to 2841 * fix(#2452): make the base-ref mutation test hermetic against leaked GIT_* env tests/mutation-workflow-base-ref.test.cjs fails on PR branches while next stays green, and it is currently blocking at least three unrelated PRs (#2841, #2832, #2827) with: error: invalid object 100644 for 'base-N.txt' error: Error building trees The existing loop comment attributes this to `git add .` rehashing O(n^2) blobs "before the object write had landed" and works around it by staging one path per iteration. That is not the cause: sequential execFileSync calls cannot race each other's object writes, and the failure persisted after that change — it simply moved to a lower commit index. The cause is that the git() helper inherited the runner's environment. A leaked GIT_INDEX_FILE makes `git add` write into a DIFFERENT repository's index; GIT_OBJECT_DIRECTORY / GIT_ALTERNATE_OBJECT_DIRECTORIES send the blob to another object store; GIT_DIR / GIT_WORK_TREE redirect the whole operation. In every case `git commit` then cannot resolve a blob it just staged, which is precisely the error above. Verified by negative control: with GIT_DIR exported, this test fails on the unfixed helper (the git commands operate on the wrong repository entirely); with the helper stripping GIT_* it passes. The single-path staging is kept — it is genuinely less work — but it is no longer load bearing. Found while shipping #2797. Fixed in place rather than deferred: it is a defect surfaced during the work, and it is blocking other contributors. Co-Authored-By: Claude Opus 5 * fix(#2452): build the base-advance commits empty, removing the lost-object class The base-ref guard has been failing in CI with: error: invalid object 100644 for 'base-N.txt' error: Error building trees It is currently red on at least three unrelated PRs (#2841, #2832, #2827) while next stays green. Two theories have now been tried and neither held. #1881 blamed `git add .` rehashing O(n^2) blobs and switched to staging one path per iteration; the failure moved from commit 32 to commit 25 and carried on. The preceding commit here made the git helper hermetic against leaked GIT_* environment — that IS a real vulnerability (with GIT_DIR exported the helper operates on the wrong repository entirely, proven by negative control) but it produces a different error than CI reports, so it is not demonstrably the cause either. Neither trigger reproduces off-CI, so this stops guessing at the trigger and removes the failure CLASS instead. The loop needs base-branch DEPTH and nothing else: no assertion reads these commits' contents, and base-side files cannot appear in `origin/base...HEAD` regardless. `--allow-empty` writes no blob and no tree, so there is no object for the index to reference and lose. It is also far less work than 60 write+hash+index cycles. The guard still proves its mechanism: the test asserts that a --depth=1 base fetch FAILS and a full fetch resolves, so a broken topology would surface immediately rather than passing vacuously. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 Co-authored-by: Test --- .changeset/lucky-voles-click.md | 5 + capabilities/antigravity/capability.json | 27 +- capabilities/claude/capability.json | 21 +- capabilities/codex/capability.json | 27 +- capabilities/gemini/capability.json | 21 +- capabilities/llama-cpp/capability.json | 25 +- capabilities/lm-studio/capability.json | 25 +- capabilities/ollama/capability.json | 25 +- capabilities/opencode/capability.json | 31 +- docs/CONFIGURATION.md | 20 +- gsd-core/bin/lib/capability-registry.cjs | 212 ++++++++++ gsd-core/bin/lib/capability-validator.cjs | 23 +- .../bin/shared/config-schema.manifest.json | 13 - gsd-core/workflows/review.md | 9 +- scripts/gen-capability-registry.cjs | 73 +++- tests/config-schema.property.test.cjs | 6 +- tests/config.test.cjs | 39 +- tests/emitted-drift-ack.json | 8 + tests/mutation-workflow-base-ref.test.cjs | 64 ++- tests/plan-review-convergence.test.cjs | 45 ++- tests/review-model-config.test.cjs | 19 +- tests/reviewer-config-federation.test.cjs | 364 ++++++++++++++++++ tests/reviewer-manifest-body.test.cjs | 32 +- 23 files changed, 1039 insertions(+), 95 deletions(-) create mode 100644 .changeset/lucky-voles-click.md create mode 100644 tests/emitted-drift-ack.json create mode 100644 tests/reviewer-config-federation.test.cjs diff --git a/.changeset/lucky-voles-click.md b/.changeset/lucky-voles-click.md new file mode 100644 index 000000000..902b66e9e --- /dev/null +++ b/.changeset/lucky-voles-click.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 2841 +--- +**Reviewer config keys are now owned by their reviewer-lane capabilities** — `review.models.`, `review._host` and `review.max_prompt_tokens_per_reviewer.` moved from the central config schema to federated slices on the lanes that use them. Key names and existing `.planning/config.json` files are unchanged and no migration is needed. Two consequences are user-visible: a `review.models.` or `review.max_prompt_tokens_per_reviewer.` key naming something that is not a declared lane is now rejected by `config-set` where it was previously accepted and silently ignored; and clearing one of these keys now reads back as its declared default rather than reporting key-not-found, because a federated key always resolves — an empty string for the model and host keys, and `-1` for a per-lane token budget (a deliberate sentinel, since `0` already means "do not trim this lane" and must stay distinguishable from unset). `review.max_prompt_tokens`, `review.default_reviewers` and `review.reviewer_instances` describe policy across lanes and deliberately remain central. (#2797) diff --git a/capabilities/antigravity/capability.json b/capabilities/antigravity/capability.json index 34d40b56f..ad02b31de 100644 --- a/capabilities/antigravity/capability.json +++ b/capabilities/antigravity/capability.json @@ -102,12 +102,22 @@ }, "reviewer": { "slug": "antigravity", - "flags": ["--antigravity", "--agy"], + "flags": [ + "--antigravity", + "--agy" + ], "transport": "spawn", - "probe": { "kind": "command-exists", "binary": "agy" }, + "probe": { + "kind": "command-exists", + "binary": "agy" + }, "invoke": { "binary": "agy", - "args": ["--print-timeout", "540s", "-p"], + "args": [ + "--print-timeout", + "540s", + "-p" + ], "promptChannel": "argv-file-ref", "outputChannel": "stdout", "modelArg": "--model", @@ -117,8 +127,17 @@ "emptyOutput": "handler-owned", "reviewsSection": "Antigravity", "evidenceClass": "source-grounded", - "requiresBinaries": ["jq"], + "requiresBinaries": [ + "jq" + ], "promptBudgetKey": null, "handler": "antigravity" + }, + "config": { + "review.models.agy": { + "type": "string", + "default": "", + "description": "Model passed to the Antigravity reviewer lane. The key suffix is the lane binary/flag alias `agy`, not the slug `antigravity` — preserved verbatim so existing .planning/config.json files keep working." + } } } diff --git a/capabilities/claude/capability.json b/capabilities/claude/capability.json index 74f5e2c2a..f1e1d271a 100644 --- a/capabilities/claude/capability.json +++ b/capabilities/claude/capability.json @@ -107,12 +107,20 @@ }, "reviewer": { "slug": "claude", - "flags": ["--claude"], + "flags": [ + "--claude" + ], "transport": "spawn", - "probe": { "kind": "command-exists", "binary": "claude" }, + "probe": { + "kind": "command-exists", + "binary": "claude" + }, "invoke": { "binary": "claude", - "args": ["-p", "-"], + "args": [ + "-p", + "-" + ], "promptChannel": "stdin", "outputChannel": "stdout", "modelArg": "--model", @@ -125,5 +133,12 @@ "requiresBinaries": [], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.claude": { + "type": "string", + "default": "", + "description": "Model passed to the Claude reviewer lane." + } } } diff --git a/capabilities/codex/capability.json b/capabilities/codex/capability.json index a233bd515..d9c9270c9 100644 --- a/capabilities/codex/capability.json +++ b/capabilities/codex/capability.json @@ -77,7 +77,9 @@ }, "orchestratorExec": { "command": "codex", - "args": ["exec"], + "args": [ + "exec" + ], "cwdFlag": "--cd", "promptFlag": null }, @@ -92,12 +94,22 @@ }, "reviewer": { "slug": "codex", - "flags": ["--codex"], + "flags": [ + "--codex" + ], "transport": "spawn", - "probe": { "kind": "command-exists", "binary": "codex" }, + "probe": { + "kind": "command-exists", + "binary": "codex" + }, "invoke": { "binary": "codex", - "args": ["exec", "--ephemeral", "--skip-git-repo-check", "-"], + "args": [ + "exec", + "--ephemeral", + "--skip-git-repo-check", + "-" + ], "promptChannel": "stdin", "outputChannel": "file-arg", "outputArg": "-o", @@ -111,5 +123,12 @@ "requiresBinaries": [], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.codex": { + "type": "string", + "default": "", + "description": "Model passed to the Codex reviewer lane." + } } } diff --git a/capabilities/gemini/capability.json b/capabilities/gemini/capability.json index 6b6f8cbb0..b786c3707 100644 --- a/capabilities/gemini/capability.json +++ b/capabilities/gemini/capability.json @@ -11,12 +11,20 @@ }, "reviewer": { "slug": "gemini", - "flags": ["--gemini"], + "flags": [ + "--gemini" + ], "transport": "spawn", - "probe": { "kind": "command-exists", "binary": "gemini" }, + "probe": { + "kind": "command-exists", + "binary": "gemini" + }, "invoke": { "binary": "gemini", - "args": ["-p", "-"], + "args": [ + "-p", + "-" + ], "promptChannel": "stdin", "outputChannel": "stdout", "modelArg": "-m", @@ -29,5 +37,12 @@ "requiresBinaries": [], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.gemini": { + "type": "string", + "default": "", + "description": "Model passed to the Gemini reviewer lane." + } } } diff --git a/capabilities/llama-cpp/capability.json b/capabilities/llama-cpp/capability.json index 7728db786..b3d6a663d 100644 --- a/capabilities/llama-cpp/capability.json +++ b/capabilities/llama-cpp/capability.json @@ -11,7 +11,9 @@ }, "reviewer": { "slug": "llama_cpp", - "flags": ["--llama-cpp"], + "flags": [ + "--llama-cpp" + ], "transport": "openai-http", "probe": { "kind": "http-reachable", @@ -29,8 +31,27 @@ "emptyOutput": "stub-with-stderr", "reviewsSection": "llama.cpp", "evidenceClass": "source-grounded", - "requiresBinaries": ["jq"], + "requiresBinaries": [ + "jq" + ], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.llama_cpp", "handler": "openai-compatible" + }, + "config": { + "review.models.llama_cpp": { + "type": "string", + "default": "", + "description": "Model requested from the llama.cpp reviewer lane; empty discovers the first model from /v1/models." + }, + "review.llama_cpp_host": { + "type": "string", + "default": "", + "description": "Base URL of the llama.cpp OpenAI-compatible server." + }, + "review.max_prompt_tokens_per_reviewer.llama_cpp": { + "type": "number", + "default": -1, + "description": "Prompt-token budget for the llama.cpp reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + } } } diff --git a/capabilities/lm-studio/capability.json b/capabilities/lm-studio/capability.json index cf03e6430..8cc8ecee9 100644 --- a/capabilities/lm-studio/capability.json +++ b/capabilities/lm-studio/capability.json @@ -11,7 +11,9 @@ }, "reviewer": { "slug": "lm_studio", - "flags": ["--lm-studio"], + "flags": [ + "--lm-studio" + ], "transport": "openai-http", "probe": { "kind": "http-reachable", @@ -29,8 +31,27 @@ "emptyOutput": "stub-with-stderr", "reviewsSection": "LM Studio", "evidenceClass": "source-grounded", - "requiresBinaries": ["jq"], + "requiresBinaries": [ + "jq" + ], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.lm_studio", "handler": "openai-compatible" + }, + "config": { + "review.models.lm_studio": { + "type": "string", + "default": "", + "description": "Model requested from the LM Studio reviewer lane; empty discovers the first model from /v1/models." + }, + "review.lm_studio_host": { + "type": "string", + "default": "", + "description": "Base URL of the LM Studio OpenAI-compatible server." + }, + "review.max_prompt_tokens_per_reviewer.lm_studio": { + "type": "number", + "default": -1, + "description": "Prompt-token budget for the LM Studio reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + } } } diff --git a/capabilities/ollama/capability.json b/capabilities/ollama/capability.json index df74f6cd7..6647e0869 100644 --- a/capabilities/ollama/capability.json +++ b/capabilities/ollama/capability.json @@ -11,7 +11,9 @@ }, "reviewer": { "slug": "ollama", - "flags": ["--ollama"], + "flags": [ + "--ollama" + ], "transport": "openai-http", "probe": { "kind": "http-reachable", @@ -29,8 +31,27 @@ "emptyOutput": "stub-with-stderr", "reviewsSection": "Ollama", "evidenceClass": "source-grounded", - "requiresBinaries": ["jq"], + "requiresBinaries": [ + "jq" + ], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.ollama", "handler": "openai-compatible" + }, + "config": { + "review.models.ollama": { + "type": "string", + "default": "", + "description": "Model requested from the Ollama reviewer lane; empty discovers the first model from /v1/models." + }, + "review.ollama_host": { + "type": "string", + "default": "", + "description": "Base URL of the Ollama OpenAI-compatible server." + }, + "review.max_prompt_tokens_per_reviewer.ollama": { + "type": "number", + "default": -1, + "description": "Prompt-token budget for the Ollama reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + } } } diff --git a/capabilities/opencode/capability.json b/capabilities/opencode/capability.json index ef3974402..a4935e1d6 100644 --- a/capabilities/opencode/capability.json +++ b/capabilities/opencode/capability.json @@ -89,7 +89,9 @@ }, "orchestratorExec": { "command": "opencode", - "args": ["run"], + "args": [ + "run" + ], "cwdFlag": "--dir", "promptFlag": null }, @@ -113,12 +115,22 @@ }, "reviewer": { "slug": "opencode", - "flags": ["--opencode"], + "flags": [ + "--opencode" + ], "transport": "spawn", - "probe": { "kind": "command-exists", "binary": "opencode" }, + "probe": { + "kind": "command-exists", + "binary": "opencode" + }, "invoke": { "binary": "opencode", - "args": ["run", "--format", "json", "-"], + "args": [ + "run", + "--format", + "json", + "-" + ], "promptChannel": "stdin", "outputChannel": "stdout", "modelArg": "--model", @@ -128,8 +140,17 @@ "emptyOutput": "stub-with-stderr", "reviewsSection": "OpenCode", "evidenceClass": "source-grounded", - "requiresBinaries": ["jq"], + "requiresBinaries": [ + "jq" + ], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.opencode": { + "type": "string", + "default": "", + "description": "Model passed to the OpenCode reviewer lane." + } } } diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 7e8a55bf6..0e7f62dab 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -237,7 +237,20 @@ API key fields accept a string value (the key itself). They can also be set to t | `review.models.gemini` | string | `null` | Model id for Gemini review (injected into -m), e.g. `"gemini-2.5-pro"` | | `review.models.opencode` | string | `null` | Model id for OpenCode review (injected into --model), e.g. `"claude-sonnet-4"` | -The `` slug is validated against `[a-zA-Z0-9_-]+`. Empty or path-containing slugs are rejected by `config-set`. +**Ownership.** These keys are owned by their reviewer-lane capabilities rather than the central +config schema — `review.models.ollama` belongs to the `ollama` capability, `review.ollama_host` +to the same, and so on. Key names and existing `.planning/config.json` files are unchanged; only +which schema validates them moved. + +One consequence follows: `` must now name a **declared reviewer lane**. Previously any slug +matching `[a-zA-Z0-9_-]+` was accepted, so a typo or a key left over from a removed reviewer +validated silently and was never read. Such a key is now rejected by `config-set`. The declared +lanes are `gemini`, `claude`, `codex`, `opencode`, `agy` (the Antigravity lane — its key suffix is +the CLI's own name, not the lane slug), `ollama`, `lm_studio` and `llama_cpp`. + +The same applies to `review.max_prompt_tokens_per_reviewer.`. `review.max_prompt_tokens` +(the global default), `review.default_reviewers` and `review.reviewer_instances` describe policy +across lanes rather than one lane's behavior, so they remain central and are unaffected. ### Reviewer defaults for `/gsd-review` @@ -1001,14 +1014,13 @@ Configure per-CLI model selection for `/gsd-review`. When set, overrides the CLI | `review.models.claude` | string | (CLI default) | Model used when `--claude` reviewer is invoked | | `review.models.codex` | string | (CLI default) | Model used when `--codex` reviewer is invoked | | `review.models.opencode` | string | (CLI default) | Model used when `--opencode` reviewer is invoked | -| `review.models.qwen` | string | (CLI default) | Model used when `--qwen` reviewer is invoked | -| `review.models.cursor` | string | (CLI default) | Model used when `--cursor` reviewer is invoked | +| `review.models.agy` | string | (CLI default) | Model used when the `--antigravity` / `--agy` reviewer is invoked. The key suffix is the CLI's own name (`agy`), not the lane slug | | `review.models.ollama` | string | (server default) | Model name passed to Ollama when `--ollama` reviewer is invoked. If unset, the first available model reported by the server is used (e.g. `llama3`). Set to a specific tag: `gsd config-set review.models.ollama codellama` | | `review.models.lm_studio` | string | (server default) | Model name passed to LM Studio when `--lm-studio` reviewer is invoked. If unset, the first available model reported by the server is used. | | `review.models.llama_cpp` | string | (server default) | Model name passed to llama.cpp when `--llama-cpp` reviewer is invoked. If unset, the first model reported by `/v1/models` is used. | | `review.default_reviewers` | string[] \| null | (all detected reviewers) | Default reviewer subset for no-flag `/gsd-review`. Example: `["gemini","codex"]`. May include configured `review.reviewer_instances` names. Explicit flags and `--all` override this setting. | | `review.max_prompt_tokens` | number\|null | null | Default maximum estimated tokens for the assembled review prompt. When set, the prompt is deterministically trimmed before being sent to each reviewer. Per-reviewer overrides via `review.max_prompt_tokens_per_reviewer` take precedence. null = no trim (current behavior). | -| `review.max_prompt_tokens_per_reviewer` | object | {} | Per-reviewer token budget overrides. Keys are reviewer slugs (ollama, llama_cpp, lm_studio, gemini, claude, codex, opencode, qwen, cursor). Values override `review.max_prompt_tokens` for that reviewer. Recommended for local model servers. | +| `review.max_prompt_tokens_per_reviewer` | object | {} | Per-reviewer token budget overrides. Keys are reviewer slugs. Only lanes that declare a budget key accept one — today `ollama`, `lm_studio` and `llama_cpp`, the local model servers this exists for. Values override `review.max_prompt_tokens` for that reviewer. A per-lane value of `0` disables trimming for that lane specifically. | | `review.ollama_host` | string | `http://localhost:11434` | Base URL of the Ollama server. Override when running Ollama on a non-default port or remote host: `gsd config-set review.ollama_host http://192.168.1.10:11434` | | `review.lm_studio_host` | string | `http://localhost:1234` | Base URL of the LM Studio local server. Override when using a non-default port. | | `review.llama_cpp_host` | string | `http://localhost:8080` | Base URL of the llama.cpp server (`llama-server`). Override when using a non-default port. | diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index e96a5957e..1f563ab34 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -226,6 +226,13 @@ const capabilities = { ], "promptBudgetKey": null, "handler": "antigravity" + }, + "config": { + "review.models.agy": { + "type": "string", + "default": "", + "description": "Model passed to the Antigravity reviewer lane. The key suffix is the lane binary/flag alias `agy`, not the slug `antigravity` — preserved verbatim so existing .planning/config.json files keep working." + } } }, "assumption-delta": { @@ -601,6 +608,13 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.claude": { + "type": "string", + "default": "", + "description": "Model passed to the Claude reviewer lane." + } } }, "claude-orchestration": { @@ -1102,6 +1116,13 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.codex": { + "type": "string", + "default": "", + "description": "Model passed to the Codex reviewer lane." + } } }, "copilot": { @@ -1597,6 +1618,13 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.gemini": { + "type": "string", + "default": "", + "description": "Model passed to the Gemini reviewer lane." + } } }, "graphify": { @@ -2120,6 +2148,23 @@ const capabilities = { ], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.llama_cpp", "handler": "openai-compatible" + }, + "config": { + "review.models.llama_cpp": { + "type": "string", + "default": "", + "description": "Model requested from the llama.cpp reviewer lane; empty discovers the first model from /v1/models." + }, + "review.llama_cpp_host": { + "type": "string", + "default": "", + "description": "Base URL of the llama.cpp OpenAI-compatible server." + }, + "review.max_prompt_tokens_per_reviewer.llama_cpp": { + "type": "number", + "default": -1, + "description": "Prompt-token budget for the llama.cpp reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + } } }, "lm-studio": { @@ -2160,6 +2205,23 @@ const capabilities = { ], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.lm_studio", "handler": "openai-compatible" + }, + "config": { + "review.models.lm_studio": { + "type": "string", + "default": "", + "description": "Model requested from the LM Studio reviewer lane; empty discovers the first model from /v1/models." + }, + "review.lm_studio_host": { + "type": "string", + "default": "", + "description": "Base URL of the LM Studio OpenAI-compatible server." + }, + "review.max_prompt_tokens_per_reviewer.lm_studio": { + "type": "number", + "default": -1, + "description": "Prompt-token budget for the LM Studio reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + } } }, "mempalace": { @@ -2424,6 +2486,23 @@ const capabilities = { ], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.ollama", "handler": "openai-compatible" + }, + "config": { + "review.models.ollama": { + "type": "string", + "default": "", + "description": "Model requested from the Ollama reviewer lane; empty discovers the first model from /v1/models." + }, + "review.ollama_host": { + "type": "string", + "default": "", + "description": "Base URL of the Ollama OpenAI-compatible server." + }, + "review.max_prompt_tokens_per_reviewer.ollama": { + "type": "number", + "default": -1, + "description": "Prompt-token budget for the Ollama reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + } } }, "opencode": { @@ -2573,6 +2652,13 @@ const capabilities = { ], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.opencode": { + "type": "string", + "default": "", + "description": "Model passed to the OpenCode reviewer lane." + } } }, "pattern-mapper": { @@ -4182,13 +4268,16 @@ const byLoopPoint = { const configKeys = { "workflow.ai_integration_phase": "ai-integration", "workflow.api_coverage_gate": "ai-integration", + "review.models.agy": "antigravity", "workflow.assumption_delta": "assumption-delta", "workflow.windows_enforce": "broken-windows", + "review.models.claude": "claude", "claude_orchestration.enabled": "claude-orchestration", "claude_orchestration.execution_backend": "claude-orchestration", "claude_orchestration.min_agent_sdk_version": "claude-orchestration", "workflow.code_review": "code-review", "workflow.code_review_depth": "code-review", + "review.models.codex": "codex", "workflow.drift_threshold": "drift", "workflow.drift_action": "drift", "workflow.schema_drift_gate": "drift", @@ -4199,8 +4288,15 @@ const configKeys = { "external_job.submit_timeout_ms": "external-job", "external_job.poll_timeout_ms": "external-job", "workflow.post_planning_gaps": "gap-analysis", + "review.models.gemini": "gemini", "graphify.enabled": "graphify", "intel.enabled": "intel", + "review.models.llama_cpp": "llama-cpp", + "review.llama_cpp_host": "llama-cpp", + "review.max_prompt_tokens_per_reviewer.llama_cpp": "llama-cpp", + "review.models.lm_studio": "lm-studio", + "review.lm_studio_host": "lm-studio", + "review.max_prompt_tokens_per_reviewer.lm_studio": "lm-studio", "mempalace.enabled": "mempalace", "mempalace.memory_mode": "mempalace", "mempalace.wing": "mempalace", @@ -4212,6 +4308,10 @@ const configKeys = { "mempalace.diary_journal": "mempalace", "mempalace.auto_capture_hooks": "mempalace", "workflow.nyquist_validation": "nyquist", + "review.models.ollama": "ollama", + "review.ollama_host": "ollama", + "review.max_prompt_tokens_per_reviewer.ollama": "ollama", + "review.models.opencode": "opencode", "workflow.pattern_mapper": "pattern-mapper", "profile-pipeline.enabled": "profile-pipeline", "workflow.research": "research", @@ -4238,6 +4338,12 @@ const configSchema = { "default": true, "description": "Require an explicit API-coverage decision (full-by-default, opt-out-not-opt-in) before a phase that integrates an external API/SDK/service can seal. At plan:pre the planner is prompted to enumerate the API surface into COVERAGE.md; at verify:pre a blocking gate fails the seal unless the matrix exists with every non-integrated capability an explicit, reasoned opt-out. Independent of ai_integration_phase (applies to any external-API integration, not only AI)." }, + "review.models.agy": { + "owner": "antigravity", + "type": "string", + "default": "", + "description": "Model passed to the Antigravity reviewer lane. The key suffix is the lane binary/flag alias `agy`, not the slug `antigravity` — preserved verbatim so existing .planning/config.json files keep working." + }, "workflow.assumption_delta": { "owner": "assumption-delta", "type": "boolean", @@ -4250,6 +4356,12 @@ const configSchema = { "default": false, "description": "Enable the blocking ship:pre gate for the broken-windows ledger. When true (opt-in), /gsd-ship blocks while .planning/WINDOWS.md has any open entry. When false (default), windows are still tracked (the executor and verifier still populate WINDOWS.md via gsd-tools windows append) but ship does not block — teams can adopt tracking before enforcement. Issue #1950." }, + "review.models.claude": { + "owner": "claude", + "type": "string", + "default": "", + "description": "Model passed to the Claude reviewer lane." + }, "claude_orchestration.enabled": { "owner": "claude-orchestration", "type": "boolean", @@ -4290,6 +4402,12 @@ const configSchema = { "deep" ] }, + "review.models.codex": { + "owner": "codex", + "type": "string", + "default": "", + "description": "Model passed to the Codex reviewer lane." + }, "workflow.drift_threshold": { "owner": "drift", "type": "number", @@ -4357,6 +4475,12 @@ const configSchema = { "default": true, "description": "Run the post-planning gap analysis report after plans are generated." }, + "review.models.gemini": { + "owner": "gemini", + "type": "string", + "default": "", + "description": "Model passed to the Gemini reviewer lane." + }, "graphify.enabled": { "owner": "graphify", "type": "boolean", @@ -4369,6 +4493,42 @@ const configSchema = { "default": false, "description": "Enable the intel code-intelligence command." }, + "review.models.llama_cpp": { + "owner": "llama-cpp", + "type": "string", + "default": "", + "description": "Model requested from the llama.cpp reviewer lane; empty discovers the first model from /v1/models." + }, + "review.llama_cpp_host": { + "owner": "llama-cpp", + "type": "string", + "default": "", + "description": "Base URL of the llama.cpp OpenAI-compatible server." + }, + "review.max_prompt_tokens_per_reviewer.llama_cpp": { + "owner": "llama-cpp", + "type": "number", + "default": -1, + "description": "Prompt-token budget for the llama.cpp reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.models.lm_studio": { + "owner": "lm-studio", + "type": "string", + "default": "", + "description": "Model requested from the LM Studio reviewer lane; empty discovers the first model from /v1/models." + }, + "review.lm_studio_host": { + "owner": "lm-studio", + "type": "string", + "default": "", + "description": "Base URL of the LM Studio OpenAI-compatible server." + }, + "review.max_prompt_tokens_per_reviewer.lm_studio": { + "owner": "lm-studio", + "type": "number", + "default": -1, + "description": "Prompt-token budget for the LM Studio reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, "mempalace.enabled": { "owner": "mempalace", "type": "boolean", @@ -4440,6 +4600,30 @@ const configSchema = { "default": true, "description": "Enable Nyquist validation coverage auditing." }, + "review.models.ollama": { + "owner": "ollama", + "type": "string", + "default": "", + "description": "Model requested from the Ollama reviewer lane; empty discovers the first model from /v1/models." + }, + "review.ollama_host": { + "owner": "ollama", + "type": "string", + "default": "", + "description": "Base URL of the Ollama OpenAI-compatible server." + }, + "review.max_prompt_tokens_per_reviewer.ollama": { + "owner": "ollama", + "type": "number", + "default": -1, + "description": "Prompt-token budget for the Ollama reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.models.opencode": { + "owner": "opencode", + "type": "string", + "default": "", + "description": "Model passed to the OpenCode reviewer lane." + }, "workflow.pattern_mapper": { "owner": "pattern-mapper", "type": "boolean", @@ -4650,6 +4834,13 @@ const runtimes = { ], "promptBudgetKey": null, "handler": "antigravity" + }, + "config": { + "review.models.agy": { + "type": "string", + "default": "", + "description": "Model passed to the Antigravity reviewer lane. The key suffix is the lane binary/flag alias `agy`, not the slug `antigravity` — preserved verbatim so existing .planning/config.json files keep working." + } } }, "augment": { @@ -4896,6 +5087,13 @@ const runtimes = { "requiresBinaries": [], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.claude": { + "type": "string", + "default": "", + "description": "Model passed to the Claude reviewer lane." + } } }, "cline": { @@ -5207,6 +5405,13 @@ const runtimes = { "requiresBinaries": [], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.codex": { + "type": "string", + "default": "", + "description": "Model passed to the Codex reviewer lane." + } } }, "copilot": { @@ -5998,6 +6203,13 @@ const runtimes = { ], "promptBudgetKey": null, "handler": null + }, + "config": { + "review.models.opencode": { + "type": "string", + "default": "", + "description": "Model passed to the OpenCode reviewer lane." + } } }, "pi": { diff --git a/gsd-core/bin/lib/capability-validator.cjs b/gsd-core/bin/lib/capability-validator.cjs index b59106b01..db6043a66 100644 --- a/gsd-core/bin/lib/capability-validator.cjs +++ b/gsd-core/bin/lib/capability-validator.cjs @@ -2552,7 +2552,7 @@ const TIER_RANK = { core: 0, standard: 1, full: 2 }; * @param {Set} centralKeys Set of keys in the central config-schema * @returns {string[]} Array of error strings; empty = all pass. */ -function validateCrossCapability(capMap, centralKeys) { +function validateCrossCapability(capMap, centralKeys, centralPatterns = []) { const errors = []; // Ownership: one owner per skill stem + agent name @@ -2623,6 +2623,27 @@ function validateCrossCapability(capMap, centralKeys) { 'remove from central config-schema before adding to the capability', ); } + // #2797: exact-key membership is not the whole central schema. A key may + // also be claimed by a central DYNAMIC PATTERN, and until now that + // collision was invisible here — `centralKeys` is built from + // `manifest.validKeys` alone. + // + // Why that mattered enough to fix rather than note: `isCentralConfigKey` + // DOES consult the patterns, and `mergeFederatedConfig` skips every key + // for which it returns true. So a federated slice overlapping a central + // pattern is INERT — it carries no traffic — while the build stays green. + // Two of the four key families Phase 4 migrates (`review.models.` + // and `review.max_prompt_tokens_per_reviewer.`) were pattern-backed, + // so the invariant was blind to exactly the migration it exists to police. + const collidingPattern = centralPatterns.find((p) => p.test(key)); + if (collidingPattern) { + errors.push( + 'config key "' + key + '" is declared in capability "' + capId + + '" AND is matched by central config-schema pattern /' + collidingPattern.source + + '/ — the federated slice would be inert (mergeFederatedConfig skips central keys): ' + + 'remove the pattern from the central config-schema in the SAME commit', + ); + } } } diff --git a/gsd-core/bin/shared/config-schema.manifest.json b/gsd-core/bin/shared/config-schema.manifest.json index 585389370..d1c653d31 100644 --- a/gsd-core/bin/shared/config-schema.manifest.json +++ b/gsd-core/bin/shared/config-schema.manifest.json @@ -49,9 +49,6 @@ "planning.commit_docs", "planning.search_gitignored", "planning.sub_repos", - "review.ollama_host", - "review.lm_studio_host", - "review.llama_cpp_host", "review.default_reviewers", "review.max_prompt_tokens", "review.max_prompt_tokens_per_reviewer", @@ -118,11 +115,6 @@ "source": "^agent_skills\\.[a-zA-Z0-9_-]+$", "description": "agent_skills." }, - { - "topLevel": "review", - "source": "^review\\.models\\.[a-zA-Z0-9_-]+$", - "description": "review.models." - }, { "topLevel": "features", "source": "^features\\.[a-zA-Z0-9_]+$", @@ -178,11 +170,6 @@ "source": "^fast_mode\\.agent_overrides\\.[a-zA-Z0-9_-]+$", "description": "fast_mode.agent_overrides." }, - { - "topLevel": "review", - "source": "^review\\.max_prompt_tokens_per_reviewer\\.[a-zA-Z0-9_-]+$", - "description": "review.max_prompt_tokens_per_reviewer." - }, { "topLevel": "review", "source": "^review\\.reviewer_instances\\.[a-zA-Z0-9_-]+\\.(cli|model|agent)$", diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index 8507c8b98..54973f10e 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -708,7 +708,8 @@ prepare_trimmed_prompt_for_reviewer() { # Resolve prompt budget for Ollama: per-reviewer override > global default > null OLLAMA_REVIEWER_BUDGET=$(gsd_run query config-get review.max_prompt_tokens_per_reviewer.ollama --raw 2>/dev/null || echo "null") -if [ -z "$OLLAMA_REVIEWER_BUDGET" ] || [ "$OLLAMA_REVIEWER_BUDGET" = "null" ]; then +# #2797: -1 = unset; 0 legitimately means "do not trim". +if [ -z "$OLLAMA_REVIEWER_BUDGET" ] || [ "$OLLAMA_REVIEWER_BUDGET" = "null" ] || [ "$OLLAMA_REVIEWER_BUDGET" = "-1" ]; then OLLAMA_REVIEWER_BUDGET=$(gsd_run query config-get review.max_prompt_tokens --raw 2>/dev/null || echo "null") fi @@ -775,7 +776,8 @@ fi ```bash # Resolve prompt budget for LM Studio: per-reviewer override > global default > null LM_STUDIO_REVIEWER_BUDGET=$(gsd_run query config-get review.max_prompt_tokens_per_reviewer.lm_studio --raw 2>/dev/null || echo "null") -if [ -z "$LM_STUDIO_REVIEWER_BUDGET" ] || [ "$LM_STUDIO_REVIEWER_BUDGET" = "null" ]; then +# #2797: -1 = unset; 0 legitimately means "do not trim". +if [ -z "$LM_STUDIO_REVIEWER_BUDGET" ] || [ "$LM_STUDIO_REVIEWER_BUDGET" = "null" ] || [ "$LM_STUDIO_REVIEWER_BUDGET" = "-1" ]; then LM_STUDIO_REVIEWER_BUDGET=$(gsd_run query config-get review.max_prompt_tokens --raw 2>/dev/null || echo "null") fi @@ -861,7 +863,8 @@ fi ```bash # Resolve prompt budget for llama.cpp: per-reviewer override > global default > null LLAMA_CPP_REVIEWER_BUDGET=$(gsd_run query config-get review.max_prompt_tokens_per_reviewer.llama_cpp --raw 2>/dev/null || echo "null") -if [ -z "$LLAMA_CPP_REVIEWER_BUDGET" ] || [ "$LLAMA_CPP_REVIEWER_BUDGET" = "null" ]; then +# #2797: -1 = unset; 0 legitimately means "do not trim". +if [ -z "$LLAMA_CPP_REVIEWER_BUDGET" ] || [ "$LLAMA_CPP_REVIEWER_BUDGET" = "null" ] || [ "$LLAMA_CPP_REVIEWER_BUDGET" = "-1" ]; then LLAMA_CPP_REVIEWER_BUDGET=$(gsd_run query config-get review.max_prompt_tokens --raw 2>/dev/null || echo "null") fi diff --git a/scripts/gen-capability-registry.cjs b/scripts/gen-capability-registry.cjs index db70d9756..c22597a0a 100644 --- a/scripts/gen-capability-registry.cjs +++ b/scripts/gen-capability-registry.cjs @@ -129,6 +129,71 @@ function loadCentralConfigKeys(schemaPath = CONFIG_SCHEMA_PATH) { return new Set(Array.isArray(manifest.validKeys) ? manifest.validKeys : []); } +/** + * Loads the central config-schema's DYNAMIC key patterns (#2797). + * + * `loadCentralConfigKeys` above reads `validKeys` only, so a federated key + * claimed by a central *pattern* was invisible to the exclusivity check. That is + * not a cosmetic gap: `isCentralConfigKey` consults these same patterns, and + * `mergeFederatedConfig` skips every key for which it returns true — so an + * overlapping slice is inert while the build stays green. + * + * The patterns are read from the SAME manifest the runtime reads and compiled + * with the same `source`, rather than re-implementing a matcher here, so the two + * cannot drift. + * + * Failure contract MATCHES `loadCentralConfigKeys` above deliberately — the two + * read the same file and must not disagree about what a broken one means: + * - ENOENT → empty list. Legitimately absent. + * - Any other read error, or a JSON parse error → prominent stderr + throw. + * + * An earlier revision swallowed the parse error and returned []. That is + * fail-OPEN on the gate this function exists to feed: with zero patterns, + * `validateCrossCapability`'s pattern-collision check silently passes and an + * inert federated slice ships green. It was masked in the one production call + * site only because `loadCentralConfigKeys` runs first against the same path and + * throws — a coincidence of ordering, not a guarantee, and this function is + * exported and called standalone. + * + * A single unparseable PATTERN is still skipped rather than fatal: that is a + * per-entry defect the central schema's own tests own, and skipping one pattern + * degrades to "checked less" rather than blocking every build. + */ +function loadCentralConfigPatterns(schemaPath = CONFIG_SCHEMA_PATH) { + let raw; + try { + raw = fs.readFileSync(schemaPath, 'utf8'); + } catch (err) { + if (err && err.code === 'ENOENT') return []; + process.stderr.write( + ' ERROR Failed to read config-schema manifest at ' + schemaPath + ': ' + err.message + '\n', + ); + throw new ExitError(1, 'could not read config-schema manifest'); + } + + let manifest; + try { + manifest = JSON.parse(raw); + } catch (err) { + process.stderr.write( + ' ERROR Config-schema manifest at ' + schemaPath + ' is broken (JSON parse error): ' + err.message + '\n', + ); + throw new ExitError(1, 'config-schema manifest JSON is malformed'); + } + const declared = Array.isArray(manifest.dynamicKeyPatterns) ? manifest.dynamicKeyPatterns : []; + const out = []; + for (const entry of declared) { + const src = entry && typeof entry.source === 'string' ? entry.source : null; + if (!src) continue; + try { + out.push(new RegExp(src)); + } catch { + // Unparseable pattern — the central schema's own tests own that failure. + } + } + return out; +} + // ─── ADR-857 Phase 4a: Derived views ───────────────────────────────────────── // (Config-slice validation, per-capability validators, contract validators, @@ -336,8 +401,11 @@ function runConsistencyGate(capabilityClusters, profileMembership, capMap) { * (used during 3a-impl while migration is in-progress). * @param {string} [capabilitiesDir] Override capabilities dir (for testing with fixtures). */ -function loadAndValidate(centralKeys, capabilitiesDir) { +function loadAndValidate(centralKeys, capabilitiesDir, centralPatterns) { const resolvedCentralKeys = centralKeys !== undefined ? centralKeys : loadCentralConfigKeys(); + // #2797: patterns default to the real manifest unless a caller passes its own + // (tests pass [] to isolate the exact-key path). + const resolvedCentralPatterns = centralPatterns !== undefined ? centralPatterns : loadCentralConfigPatterns(); const resolvedCapDir = capabilitiesDir !== undefined ? capabilitiesDir : CAPABILITIES_DIR; const errors = []; const capMap = new Map(); @@ -406,7 +474,7 @@ function loadAndValidate(centralKeys, capabilitiesDir) { } // Cross-capability invariants — capMap contains only fully-valid capabilities at this point. - const crossErrors = validateCrossCapability(capMap, resolvedCentralKeys); + const crossErrors = validateCrossCapability(capMap, resolvedCentralKeys, resolvedCentralPatterns); errors.push(...crossErrors); // C2: Global consumes-satisfiability — runs after capMap is fully built so cross-capability @@ -857,6 +925,7 @@ module.exports = { validateCrossCapability, classifyCrossErrors, loadCentralConfigKeys, + loadCentralConfigPatterns, loadAndValidate, buildRegistry, serializeRegistry, diff --git a/tests/config-schema.property.test.cjs b/tests/config-schema.property.test.cjs index 0302f211f..01292d86b 100644 --- a/tests/config-schema.property.test.cjs +++ b/tests/config-schema.property.test.cjs @@ -350,7 +350,11 @@ describe('Bug #2986: M2 -- DYNAMIC_KEY_PATTERNS.some semantic, not .every', () = // dynamic path is the only way to reach `true`. const patternRepresentatives = [ { key: 'agent_skills.gsd-planner', topLevel: 'agent_skills' }, - { key: 'review.models.claude', topLevel: 'review' }, + // #2797: `review.models.` was the `review` representative until that + // pattern was federated to the lane capabilities and removed from the central + // schema. `reviewer_instances` is the surviving `review` dynamic pattern — + // an instance is not a lane (ADR-2782 D8), so it stays central. + { key: 'review.reviewer_instances.myinstance.cli', topLevel: 'review' }, { key: 'features.some_dynamic_feature', topLevel: 'features' }, { key: 'claude_md_assembly.blocks.intro', topLevel: 'claude_md_assembly' }, { key: 'model_profile_overrides.codex.opus', topLevel: 'model_profile_overrides' }, diff --git a/tests/config.test.cjs b/tests/config.test.cjs index 96d67e10e..1a9cc2f3d 100644 --- a/tests/config.test.cjs +++ b/tests/config.test.cjs @@ -769,12 +769,20 @@ describe('config-set null — unset/clear (#2046)', () => { 'review.models.gemini must be absent on disk after unset, not the string "null"' ); + // #2797: review.models. is federated to its lane capability now, and a + // federated key always resolves to its declared default — so a cleared key + // reads back as the empty string (rendered `""` by the non-raw formatter) + // instead of erroring. The property this test actually guards is unchanged + // and asserted above: the key is REMOVED from disk, never persisted as the + // literal string "null". const getResult = runGsdTools('config-get review.models.gemini', tmpDir); + const shown = (getResult.output || '').trim(); assert.ok( - !getResult.output || !getResult.output.trim() || getResult.output.trim() === 'undefined', + shown === '' || shown === '""' || shown === 'undefined', `config-get should return empty/undefined after unset, got: ${getResult.output}` ); - assert.notStrictEqual(getResult.output && getResult.output.trim(), 'null'); + assert.notStrictEqual(shown, 'null'); + assert.notStrictEqual(shown, '"null"'); }); test('secret key: config-set brave_search null removes the key (never persists "null")', () => { @@ -1465,8 +1473,17 @@ describe('config-set prototype-pollution guard via dynamic-key prefixes (alert # 'features.__proto__: Object.prototype should not gain "somevalue"'); }); - test('review.models.constructor is blocked by setConfigValue guard (not schema gate)', () => { - const result = runGsdTools('config-set review.models.constructor somevalue', tmpDir); + test('agent_skills.constructor is blocked by setConfigValue guard (not schema gate)', () => { + // #2797: this case used `review.models.constructor`, which reached the + // setConfigValue guard only because the central pattern + // ^review\.models\.[a-zA-Z0-9_-]+$ accepted it first. That pattern is now + // federated to the lane capabilities and gone from the central schema, so + // the key is rejected at the schema gate instead (asserted separately below). + // + // The GUARD still needs coverage, so this exercises it through a surviving + // dynamic prefix. Losing this assertion would have quietly deleted the + // regression test for alert #26 rather than relocating it. + const result = runGsdTools('config-set agent_skills.constructor somevalue', tmpDir); assert.strictEqual(result.success, false, `Expected failure but got: ${result.output}`); @@ -1480,7 +1497,19 @@ describe('config-set prototype-pollution guard via dynamic-key prefixes (alert # ); assert.strictEqual(Object.prototype.hasOwnProperty.call(Object.prototype, 'somevalue'), false, - 'review.models.constructor: Object.prototype should not gain "somevalue"'); + 'agent_skills.constructor: Object.prototype should not gain "somevalue"'); + }); + + test('review.models.constructor is now rejected at the schema gate (#2797)', () => { + // Defence in depth: after federation the key never reaches setConfigValue at + // all, because no lane declares `review.models.constructor`. Rejecting + // earlier is strictly safer than rejecting later — this locks that it is + // still rejected, by whichever gate gets there first. + const result = runGsdTools('config-set review.models.constructor somevalue', tmpDir); + + assert.strictEqual(result.success, false, `Expected failure but got: ${result.output}`); + assert.strictEqual(Object.prototype.hasOwnProperty.call(Object.prototype, 'somevalue'), false, + 'Object.prototype must not gain "somevalue"'); }); test('positive control: agent_skills.sonnet-coder with valid value succeeds', () => { diff --git a/tests/emitted-drift-ack.json b/tests/emitted-drift-ack.json new file mode 100644 index 000000000..0141ffedf --- /dev/null +++ b/tests/emitted-drift-ack.json @@ -0,0 +1,8 @@ +{ + "version": 1, + "paths": { + "review.md": { + "reason": "#2797 / ADR-2782 D9 (config half). Growth is comment-only: the three per-lane prompt-budget fallback guards (ollama, lm_studio, llama_cpp) gain an explanation of why they test for the -1 sentinel rather than 0. A federated config key always resolves to its declared default rather than reporting not-found, so an unset per-lane budget needs a sentinel that cannot collide with a legitimate value — and 0 is legitimate: it already means \"do not trim this lane\" (the early-return guard in prepare_trimmed_prompt_for_reviewer). Treating 0 as unset would have silently switched a user who deliberately disabled trimming for one lane onto the global budget. No lane's invocation shape changed." + } + } +} diff --git a/tests/mutation-workflow-base-ref.test.cjs b/tests/mutation-workflow-base-ref.test.cjs index f60b761be..b2bf00fda 100644 --- a/tests/mutation-workflow-base-ref.test.cjs +++ b/tests/mutation-workflow-base-ref.test.cjs @@ -55,12 +55,44 @@ const THREE_DOT_WORKFLOWS = [ // Bounded: git subprocesses in tests must never hang a CI lane. const GIT_TIMEOUT_MS = 30_000; +/** + * Environment for every git call below, with EVERY `GIT_*` variable stripped. + * + * Without this the helper inherits the runner's environment. A leaked + * `GIT_INDEX_FILE` makes `git add` in this temp repo write into a DIFFERENT + * repository's index; a leaked `GIT_OBJECT_DIRECTORY` / + * `GIT_ALTERNATE_OBJECT_DIRECTORIES` sends the blob to a different object store; + * a leaked `GIT_DIR` / `GIT_WORK_TREE` redirects the operation wholesale. In + * each case `git commit` then cannot resolve a blob it just staged, and git + * reports exactly: + * + * error: invalid object 100644 for 'base-N.txt' + * error: Error building trees + * + * That failure was previously attributed to `git add .` rehashing O(n²) blobs + * "before the object write had landed" (see the loop comment below) and worked + * around by staging one path per iteration. That reduced the churn but not the + * cause: sequential execFileSync calls cannot race each other's object writes, + * and the failure persisted — reappearing at a lower commit index on PR branches + * while `next` stayed green. Making the environment hermetic addresses the + * mechanism rather than the symptom; the single-path staging below is kept + * because it is genuinely less work. + */ +function gitEnv() { + const env = { ...process.env }; + for (const key of Object.keys(env)) { + if (key.startsWith('GIT_')) delete env[key]; + } + return env; +} + function git(cwd, args) { return execFileSync('git', args, { cwd, encoding: 'utf8', timeout: GIT_TIMEOUT_MS, stdio: ['ignore', 'pipe', 'pipe'], + env: gitEnv(), }); } @@ -172,17 +204,27 @@ describe('#2452 CI gates: base-ref fetch must preserve ancestry', () => { // Base then advances, leaving `feature` BEHIND — the failing condition. git(origin, ['checkout', '--quiet', 'base']); for (let n = 1; n <= BASE_ADVANCE; n++) { - fs.writeFileSync(path.join(origin, `base-${n}.txt`), `${n}\n`); - // Stage only the file this iteration created. `git add .` re-stages every - // file already in the tree, so across BASE_ADVANCE iterations it rehashes - // O(n²) blobs — roughly 1,800 stagings and 60 full index rewrites to add 60 - // one-line files. That churn is what this loop failed on in CI: the index - // ended up referencing a blob whose object write had not landed - // ("invalid object … for 'base-31.txt' / Error building trees") at commit 32 - // of 60. Staging the single new path is equivalent here — each commit adds - // exactly one file — and removes the redundant work entirely. - git(origin, ['add', `base-${n}.txt`]); - git(origin, ['commit', '--quiet', '-m', `base advance ${n}`]); + // EMPTY commits: this loop needs base-branch DEPTH, nothing else. No + // assertion reads these commits' contents — the three-dot diff below is + // asserted on `covered.cts`, which lives on the HEAD side, and base-side + // files cannot appear in `origin/base...HEAD` at all. + // + // They used to write and stage a `base-N.txt` per iteration, and that is + // what kept failing in CI: + // + // error: invalid object 100644 for 'base-N.txt' + // error: Error building trees + // + // The first attempt blamed `git add .` rehashing O(n²) blobs and switched + // to staging one path per iteration (#1881). It did not work — the failure + // simply moved from commit 32 to commit 25, and went on blocking unrelated + // PRs. The trigger for the lost object write was never reproduced off-CI. + // + // So rather than keep guessing at the trigger, this removes the failure + // CLASS: `--allow-empty` writes no blob and no tree for these commits, so + // there is no object for the index to reference and lose. It is also + // dramatically less work than 60 write+hash+index cycles. + git(origin, ['commit', '--quiet', '--allow-empty', '-m', `base advance ${n}`]); } // ---- runner: has the PR head, must fetch the base ref separately ------ diff --git a/tests/plan-review-convergence.test.cjs b/tests/plan-review-convergence.test.cjs index df611f4af..2f20cf124 100644 --- a/tests/plan-review-convergence.test.cjs +++ b/tests/plan-review-convergence.test.cjs @@ -968,28 +968,33 @@ describe('plan-review-convergence local model reviewer flags (#2306-local)', () describe('plan-review-convergence local model config schema registration (#2306-local)', () => { // After Cycle 5 (#3536), config-schema.cjs is a thin adapter sourcing from // the manifest. Use the runtime Set instead of text-parsing the source file. - const { VALID_CONFIG_KEYS } = require('../gsd-core/bin/lib/config-schema.cjs'); + // + // #2797 (ADR-2782 D9): these three host keys are no longer in VALID_CONFIG_KEYS + // — they are federated to their reviewer-lane capabilities (`ollama`, + // `lm-studio`, `llama-cpp`), and the exclusivity invariant forbids a key living + // in both places. What #2306-local actually protects is that `gsd config-set` + // ACCEPTS these keys, so that is what is asserted now, via isValidConfigKey — + // the predicate config-set itself uses, which spans both the central schema and + // federated capability slices. Asserting central membership would now be + // asserting the migration did not happen. + const { isValidConfigKey, isCapabilityConfigKey } = + require('../gsd-core/bin/lib/config-schema.cjs'); - test('review.ollama_host is registered in config-schema.cjs', () => { - assert.ok( - VALID_CONFIG_KEYS.has('review.ollama_host'), - "review.ollama_host must be in VALID_CONFIG_KEYS so gsd config-set accepts it" - ); - }); + for (const key of ['review.ollama_host', 'review.lm_studio_host', 'review.llama_cpp_host']) { + test(`${key} is accepted by config-set`, () => { + assert.ok( + isValidConfigKey(key), + `${key} must be a valid config key so gsd config-set accepts it` + ); + }); - test('review.lm_studio_host is registered in config-schema.cjs', () => { - assert.ok( - VALID_CONFIG_KEYS.has('review.lm_studio_host'), - "review.lm_studio_host must be in VALID_CONFIG_KEYS so gsd config-set accepts it" - ); - }); - - test('review.llama_cpp_host is registered in config-schema.cjs', () => { - assert.ok( - VALID_CONFIG_KEYS.has('review.llama_cpp_host'), - "review.llama_cpp_host must be in VALID_CONFIG_KEYS so gsd config-set accepts it" - ); - }); + test(`${key} is owned by its reviewer-lane capability (#2797)`, () => { + assert.ok( + isCapabilityConfigKey(key), + `${key} must be federated to its lane capability, not stranded centrally` + ); + }); + } }); // ─── Workflow: source-grounding pass (#22) ─────────────────────────────────── diff --git a/tests/review-model-config.test.cjs b/tests/review-model-config.test.cjs index 4ae6c0d3b..da4f81f2a 100644 --- a/tests/review-model-config.test.cjs +++ b/tests/review-model-config.test.cjs @@ -146,15 +146,26 @@ describe('review.models. config key', () => { assert.doesNotMatch(rawText, /"gemini":\s*"null"/, 'must never persist review.models.gemini as the literal string "null"'); - // config-get on the removed key reports not-found (the review workflow reads - // it as `... 2>/dev/null || echo ""` → empty → the `[ -n "$VAR" ]` guard falls - // back to the reviewer default). + // config-get on the removed key yields EMPTY (the review workflow reads it as + // `... 2>/dev/null || echo ""` → empty → the `[ -n "$VAR" ]` guard falls back + // to the reviewer default). + // + // #2797: this key is now federated to the `gemini` lane capability, and a + // federated key always resolves to its declared default — so config-get exits + // 0 with empty output rather than exiting non-zero with "Key not found". The + // WORKFLOW outcome is unchanged: the guard above sees empty either way, which + // is exactly what this test's own rationale (the comment above) turns on. + // + // What #2046 actually protects is asserted below and is untouched: clearing + // must never yield the literal string "null", which would be handed to the + // CLI as a model name. const getResult = runGsdTools( ['config-get', 'review.models.gemini', '--raw'], tmpDir, { HOME: tmpDir, USERPROFILE: tmpDir } ); - assert.ok(!getResult.success, 'config-get on a cleared key should report not-found'); + assert.strictEqual((getResult.output || '').trim(), '', + 'config-get on a cleared key must yield empty output'); assert.notStrictEqual(getResult.output && getResult.output.trim(), 'null', 'config-get must not emit the literal string "null" for a cleared key'); }); diff --git a/tests/reviewer-config-federation.test.cjs b/tests/reviewer-config-federation.test.cjs new file mode 100644 index 000000000..0f38c5a1e --- /dev/null +++ b/tests/reviewer-config-federation.test.cjs @@ -0,0 +1,364 @@ +'use strict'; + +/** + * Reviewer config-key federation — ADR-2782 D9 (config half), Phase 4 (#2797). + * + * Four key families move from the central config-schema to federated `config` + * slices owned by their lane capabilities. Three keys deliberately stay central, + * because a key describing policy *across* lanes must not be federated *into* + * one. + * + * The trap this suite exists to catch: two of the four families were governed by + * central DYNAMIC PATTERNS rather than exact keys. `isCentralConfigKey` consults + * those patterns, and `mergeFederatedConfig` skips every key for which it returns + * true — so declaring a slice while the pattern survives yields an INERT slice + * and a green build. Assertions here are on provenance (`isCentralConfigKey` vs + * `isCapabilityConfigKey`), not merely on validity, because validity alone cannot + * tell a working migration from a no-op. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { createTempProject, createTempDir, cleanup, runGsdTools } = require('./helpers.cjs'); +const configSchema = require('../gsd-core/bin/lib/config-schema.cjs'); +const capValidator = require('../gsd-core/bin/lib/capability-validator.cjs'); +const registry = require('../gsd-core/bin/lib/capability-registry.cjs'); +const gen = require('../scripts/gen-capability-registry.cjs'); + +/** Keys that moved to a lane capability, with the lane that must own each. */ +const FEDERATED = { + 'review.models.gemini': 'gemini', + 'review.models.claude': 'claude', + 'review.models.codex': 'codex', + 'review.models.opencode': 'opencode', + // The suffix is the lane's binary/flag alias, NOT its slug `antigravity` — + // preserved verbatim so existing .planning/config.json files keep working. + 'review.models.agy': 'antigravity', + 'review.models.ollama': 'ollama', + 'review.models.lm_studio': 'lm-studio', + 'review.models.llama_cpp': 'llama-cpp', + 'review.ollama_host': 'ollama', + 'review.lm_studio_host': 'lm-studio', + 'review.llama_cpp_host': 'llama-cpp', + 'review.max_prompt_tokens_per_reviewer.ollama': 'ollama', + 'review.max_prompt_tokens_per_reviewer.lm_studio': 'lm-studio', + 'review.max_prompt_tokens_per_reviewer.llama_cpp': 'llama-cpp', +}; + +/** Keys D9 names as staying central — policy across lanes, not lane properties. */ +const CENTRAL_SURVIVORS = [ + 'review.max_prompt_tokens', + 'review.default_reviewers', + 'review.reviewer_instances.myinstance.cli', +]; + +describe('reviewer config federation — provenance actually moved (#2797)', () => { + test('every federated key is owned by a capability and no longer central', () => { + for (const [key, owner] of Object.entries(FEDERATED)) { + assert.equal( + configSchema.isCentralConfigKey(key), false, + `${key} must NOT be central — while it is, mergeFederatedConfig skips it and the slice is inert`, + ); + assert.equal( + configSchema.isCapabilityConfigKey(key), true, + `${key} must be owned by a capability config slice`, + ); + assert.equal(configSchema.isValidConfigKey(key), true, `${key} must remain valid`); + assert.equal( + registry.configSchema[key] && registry.configSchema[key].owner, owner, + `${key} must be owned by "${owner}"`, + ); + } + }); + + test('the keys D9 keeps central are untouched', () => { + for (const key of CENTRAL_SURVIVORS) { + assert.equal(configSchema.isCentralConfigKey(key), true, `${key} must stay central`); + assert.equal( + configSchema.isCapabilityConfigKey(key), false, + `${key} describes policy across lanes and must not be federated into one`, + ); + } + }); + + test('no capability claims a policy-across-lanes key', () => { + const owned = new Set(Object.keys(registry.configSchema || {})); + for (const key of ['review.max_prompt_tokens', 'review.default_reviewers', 'review.reviewer_instances']) { + assert.equal(owned.has(key), false, `${key} must not appear in any capability slice`); + } + }); + + test('the container key remains central so whole-object get/set still works', () => { + // Only the per-slug leaves federate. Narrowing the container is a separate + // decision D9 did not make; locking today's behavior so a future change is + // deliberate rather than accidental. + assert.equal(configSchema.isCentralConfigKey('review.max_prompt_tokens_per_reviewer'), true); + }); + + test('a lane with no model flag and no host owns no config keys', () => { + // Absent-safe (ADR-2782 D4): qwen, cursor and coderabbit take neither a model + // argument nor a host, so they declare nothing. That is not a coverage hole. + const owners = Object.values(registry.configSchema || {}).map((e) => e && e.owner); + for (const laneId of ['qwen', 'cursor', 'coderabbit']) { + assert.equal(owners.includes(laneId), false, `${laneId} must declare no config keys`); + } + }); +}); + +describe('reviewer config federation — the disclosed tightening (#2797)', () => { + test('a model key naming no declared lane is rejected', () => { + // Was accepted by the central pattern ^review\.models\.[a-zA-Z0-9_-]+$. + // The exclusivity invariant forbids keeping that pattern alongside the + // federated keys, so this tightening is unavoidable — and desirable: it + // catches typos and stale keys that previously validated silently. + assert.equal(configSchema.isValidConfigKey('review.models.__not_a_lane__'), false); + }); + + test('a per-lane budget key naming no declared lane is rejected', () => { + assert.equal( + configSchema.isValidConfigKey('review.max_prompt_tokens_per_reviewer.__nope__'), false, + ); + }); + + test('the hyphenated capability id is not a config key', () => { + // The directory is `lm-studio`; the config key uses the slug `lm_studio`. + // Conflating them yields a key no user has ever set. + assert.equal(configSchema.isValidConfigKey('review.lm-studio_host'), false); + assert.equal(configSchema.isValidConfigKey('review.lm_studio_host'), true); + }); +}); + +describe('reviewer config federation — end-to-end through the CLI (#2797)', () => { + test('a federated model key round-trips: set, persist, get', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const set = runGsdTools('config-set review.models.ollama llama3', tmpDir); + assert.ok(set.success, `config-set must accept the federated key: ${set.error || ''}`); + + const cfg = JSON.parse(fs.readFileSync(path.join(tmpDir, '.planning', 'config.json'), 'utf-8')); + assert.equal(cfg.review?.models?.ollama, 'llama3', 'value must be persisted'); + + const get = runGsdTools('query config-get review.models.ollama --raw', tmpDir); + assert.ok(get.success, 'config-get must resolve the federated key'); + }); + + test('a federated host key round-trips', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const set = runGsdTools('config-set review.ollama_host http://127.0.0.1:9999', tmpDir); + assert.ok(set.success, `config-set must accept the federated host key: ${set.error || ''}`); + + const cfg = JSON.parse(fs.readFileSync(path.join(tmpDir, '.planning', 'config.json'), 'utf-8')); + assert.equal(cfg.review?.ollama_host, 'http://127.0.0.1:9999'); + }); + + test('a central survivor still round-trips unchanged', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const set = runGsdTools('config-set review.max_prompt_tokens 8000', tmpDir); + assert.ok(set.success, `the global budget must stay settable: ${set.error || ''}`); + + const cfg = JSON.parse(fs.readFileSync(path.join(tmpDir, '.planning', 'config.json'), 'utf-8')); + assert.equal(cfg.review?.max_prompt_tokens, 8000); + }); + + test('an existing config carrying federated keys loads unchanged — no migration', (t) => { + // The acceptance criterion: key NAMES and existing files are unchanged; only + // validation provenance moved. A file written before this phase must still load. + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const cfgPath = path.join(tmpDir, '.planning', 'config.json'); + const pre = { + review: { + models: { ollama: 'llama3', agy: 'gemini-3-pro' }, + ollama_host: 'http://localhost:11434', + max_prompt_tokens_per_reviewer: { ollama: 6000 }, + max_prompt_tokens: 8000, + }, + }; + fs.writeFileSync(cfgPath, JSON.stringify(pre, null, 2)); + + const get = runGsdTools('query config-get review.models.ollama --raw', tmpDir); + assert.ok(get.success, 'a pre-existing federated value must still resolve'); + + const after = JSON.parse(fs.readFileSync(cfgPath, 'utf-8')); + assert.deepEqual(after, pre, 'reading must not rewrite the file'); + }); + + test('an unset per-lane budget resolves to the -1 sentinel, not 0', (t) => { + // 0 is a LEGITIMATE per-lane budget meaning "do not trim this lane" — the + // guard in prepare_trimmed_prompt_for_reviewer returns early on it. A + // federated key always resolves to its declared default, so if that default + // were 0, "unset" and "deliberately disabled" would be indistinguishable and + // the workflow's fallback-to-global branch could not tell them apart. + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const get = runGsdTools( + 'query config-get review.max_prompt_tokens_per_reviewer.ollama --raw', tmpDir, + ); + assert.strictEqual((get.output || '').trim(), '-1', + 'an unset per-lane budget must read back as the -1 sentinel'); + }); + + test('an explicit per-lane budget of 0 survives federation', (t) => { + // The regression this guards: treating 0 as "unset" in the workflow fallback + // would silently switch a user who disabled trimming for one lane onto the + // global budget instead. + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const set = runGsdTools('config-set review.max_prompt_tokens_per_reviewer.ollama 0', tmpDir); + assert.ok(set.success, `config-set must accept an explicit 0: ${set.error || ''}`); + + const get = runGsdTools( + 'query config-get review.max_prompt_tokens_per_reviewer.ollama --raw', tmpDir, + ); + assert.strictEqual((get.output || '').trim(), '0', + 'an explicit 0 must survive as 0, distinguishable from unset'); + }); + + test('an explicit per-lane budget value round-trips', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + runGsdTools('config-set review.max_prompt_tokens_per_reviewer.ollama 6000', tmpDir); + const get = runGsdTools( + 'query config-get review.max_prompt_tokens_per_reviewer.ollama --raw', tmpDir, + ); + assert.strictEqual((get.output || '').trim(), '6000'); + }); + + test('a model key naming no declared lane is rejected by the CLI', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const set = runGsdTools('config-set review.models.__not_a_lane__ x', tmpDir); + assert.equal(set.success, false, 'an unknown lane key must be rejected, not silently accepted'); + }); +}); + +describe('exclusivity gate sees dynamic patterns (#2797)', () => { + const slice = { type: 'string', default: '', description: 'x' }; + const capWith = (key) => new Map([['acme', { config: { [key]: slice } }]]); + + test('the shipped capability set passes the extended gate', () => { + // The production-shape row: proves the cutover is COMPLETE, not merely + // declared. If any federated key still had a central pattern, this fails. + const errors = capValidator.validateCrossCapability( + new Map(Object.entries(registry.capabilities || {})), + gen.loadCentralConfigKeys(), + gen.loadCentralConfigPatterns(), + ); + assert.deepEqual(errors, [], `shipped capabilities must pass: ${JSON.stringify(errors)}`); + }); + + test('a federated key colliding with a central PATTERN fails the gate', () => { + // The gap this phase closes. `centralKeys` is built from validKeys alone, so + // before the fix this returned [] and the inert slice shipped green. + const errors = capValidator.validateCrossCapability( + capWith('review.reviewer_instances.acme.cli'), + new Set(), + gen.loadCentralConfigPatterns(), + ); + assert.equal(errors.length, 1, `expected one pattern-collision error, got ${JSON.stringify(errors)}`); + assert.match(errors[0], /matched by central config-schema pattern/); + assert.match(errors[0], /review\.reviewer_instances\.acme\.cli/); + }); + + test('a federated key colliding with an exact central key still fails', () => { + const errors = capValidator.validateCrossCapability( + capWith('review.max_prompt_tokens'), + new Set(['review.max_prompt_tokens']), + [], + ); + assert.equal(errors.length, 1); + assert.match(errors[0], /exists in the central config-schema/); + }); + + test('a cleanly federated key passes', () => { + const errors = capValidator.validateCrossCapability( + capWith('review.models.ollama'), + gen.loadCentralConfigKeys(), + gen.loadCentralConfigPatterns(), + ); + assert.deepEqual(errors, []); + }); + + test('two capabilities declaring one key still collide', () => { + const errors = capValidator.validateCrossCapability( + new Map([ + ['a', { config: { 'x.y': slice } }], + ['b', { config: { 'x.y': slice } }], + ]), + new Set(), + [], + ); + assert.equal(errors.length, 1); + assert.match(errors[0], /owned by both/); + }); + + test('omitting the patterns argument preserves the pre-#2797 signature', () => { + // Back-compat: existing callers passing two arguments must not start seeing + // pattern errors they cannot act on. + const errors = capValidator.validateCrossCapability( + capWith('review.reviewer_instances.acme.cli'), + new Set(), + ); + assert.deepEqual(errors, []); + }); + + test('loadCentralConfigPatterns reads the same manifest the runtime reads', () => { + const pats = gen.loadCentralConfigPatterns(); + assert.ok(pats.length > 0, 'expected the central schema to declare patterns'); + for (const p of pats) assert.ok(p instanceof RegExp); + // The two families this phase removed must be gone. + const sources = pats.map((p) => p.source); + assert.equal(sources.some((s) => s.includes('review\\.models')), false, + 'the review.models pattern must be removed — it is federated now'); + assert.equal(sources.some((s) => s.includes('max_prompt_tokens_per_reviewer')), false, + 'the per-reviewer budget pattern must be removed — it is federated now'); + }); + + test('an absent manifest returns no patterns — the legitimate case', () => { + assert.deepEqual(gen.loadCentralConfigPatterns('/nonexistent/path.json'), []); + }); + + test('a MALFORMED manifest throws rather than silently reporting no patterns', (t) => { + // Fail-open here would defeat the gate this function exists to feed: with + // zero patterns, the pattern-collision check silently passes and an inert + // federated slice ships green. `loadCentralConfigKeys` reads the same file + // and throws on the same failure class — the two must not disagree about + // what a broken manifest means. + const dir = createTempDir('gsd-2797-badmanifest-'); + t.after(() => cleanup(dir)); + + const bad = path.join(dir, 'broken.json'); + fs.writeFileSync(bad, '{ "validKeys": [ this is not json'); + + assert.throws( + () => gen.loadCentralConfigPatterns(bad), + (err) => err && /malformed|JSON/i.test(String(err.message)), + 'a broken manifest must fail closed, not return []', + ); + }); + + test('an unreadable manifest path throws rather than returning no patterns', (t) => { + // A directory where a file is expected yields EISDIR, not ENOENT — the + // "absent" carve-out must not swallow it. + const dir = createTempDir('gsd-2797-dirmanifest-'); + t.after(() => cleanup(dir)); + + assert.throws( + () => gen.loadCentralConfigPatterns(dir), + 'reading a directory as the manifest must fail closed', + ); + }); +}); diff --git a/tests/reviewer-manifest-body.test.cjs b/tests/reviewer-manifest-body.test.cjs index 27b4a30e8..652eff86e 100644 --- a/tests/reviewer-manifest-body.test.cjs +++ b/tests/reviewer-manifest-body.test.cjs @@ -1267,21 +1267,41 @@ describe('H. Harvest widening — buildRegistry(capMap)', () => { ); }); - test('shippedRegistryOutputIsUnchangedByHarvestWidening', () => { + test('shippedRegistryConfigKeysAreOwnedByDeclaringCapabilities', () => { + // Phase 2 introduced the harvest widening (ownership follows DECLARING a + // config slice, not being a feature) and asserted it was INERT — no shipped + // non-feature capability owned a config key, so the widening changed nothing. + // + // #2797 (Phase 4) is what consumes it: the reviewer lanes now own their own + // config keys, and those lanes are `role: "reviewer"` (lane-only CLIs) or + // `role: "runtime"` (dual-purpose install targets). Asserting `role === + // 'feature'` here would now be asserting the migration did not happen. + // + // The protection is kept, just restated: every key's owner must exist, and a + // `review.*` key must be owned by a capability that actually declares a + // reviewer lane — not by an arbitrary capability that happened to claim it. const { capMap, errors } = loadAndValidate(new Set()); assert.deepEqual(errors, [], `expected the real shipped capability set to validate cleanly, got: ${JSON.stringify(errors)}`); const registry = buildRegistry(capMap); + let reviewKeys = 0; for (const [key, ownerId] of Object.entries(registry.configKeys)) { const owner = capMap.get(ownerId); assert.ok(owner, `configKeys owner "${ownerId}" for key "${key}" must exist in capMap`); - assert.equal( - owner.role, - 'feature', - `harvest widening must not change the shipped set: config key "${key}" is owned by non-feature ` + - `capability "${ownerId}" (role: ${owner.role}) — today only feature-role capabilities own config keys`, + assert.ok( + ['feature', 'reviewer', 'runtime'].includes(owner.role), + `config key "${key}" is owned by capability "${ownerId}" with unexpected role "${owner.role}"`, ); + if (key.startsWith('review.')) { + reviewKeys += 1; + assert.ok( + owner.reviewer && typeof owner.reviewer.slug === 'string', + `review config key "${key}" must be owned by a capability declaring a reviewer lane, ` + + `but "${ownerId}" declares none`, + ); + } } + assert.ok(reviewKeys > 0, 'expected the federated reviewer config keys to be present (#2797)'); }); });