* 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/<id>/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.<slug>, review.max_prompt_tokens_per_reviewer.<slug>) 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.<slug> 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <sha> 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 <noreply@anthropic.com> * 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 <sha> 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Test <test@example.com>
This commit is contained in:
5
.changeset/lucky-voles-click.md
Normal file
5
.changeset/lucky-voles-click.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Changed
|
||||
pr: 2841
|
||||
---
|
||||
**Reviewer config keys are now owned by their reviewer-lane capabilities** — `review.models.<lane>`, `review.<lane>_host` and `review.max_prompt_tokens_per_reviewer.<lane>` 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.<x>` or `review.max_prompt_tokens_per_reviewer.<x>` 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)
|
||||
@@ -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."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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\"."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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\"."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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\"."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 `<cli>` 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: `<cli>` 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.<slug>`. `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. |
|
||||
|
||||
@@ -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": {
|
||||
|
||||
@@ -2552,7 +2552,7 @@ const TIER_RANK = { core: 0, standard: 1, full: 2 };
|
||||
* @param {Set<string>} 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.<slug>`
|
||||
// and `review.max_prompt_tokens_per_reviewer.<slug>`) 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',
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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.<agent-type>"
|
||||
},
|
||||
{
|
||||
"topLevel": "review",
|
||||
"source": "^review\\.models\\.[a-zA-Z0-9_-]+$",
|
||||
"description": "review.models.<cli-name>"
|
||||
},
|
||||
{
|
||||
"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.<agent-id>"
|
||||
},
|
||||
{
|
||||
"topLevel": "review",
|
||||
"source": "^review\\.max_prompt_tokens_per_reviewer\\.[a-zA-Z0-9_-]+$",
|
||||
"description": "review.max_prompt_tokens_per_reviewer.<reviewer-slug>"
|
||||
},
|
||||
{
|
||||
"topLevel": "review",
|
||||
"source": "^review\\.reviewer_instances\\.[a-zA-Z0-9_-]+\\.(cli|model|agent)$",
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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.<slug>` 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' },
|
||||
|
||||
@@ -769,12 +769,20 @@ describe('config-set <key> null — unset/clear (#2046)', () => {
|
||||
'review.models.gemini must be absent on disk after unset, not the string "null"'
|
||||
);
|
||||
|
||||
// #2797: review.models.<slug> 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', () => {
|
||||
|
||||
8
tests/emitted-drift-ack.json
Normal file
8
tests/emitted-drift-ack.json
Normal file
@@ -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."
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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 <sha> 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 <sha> 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 ------
|
||||
|
||||
@@ -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) ───────────────────────────────────
|
||||
|
||||
@@ -146,15 +146,26 @@ describe('review.models.<cli> 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');
|
||||
});
|
||||
|
||||
364
tests/reviewer-config-federation.test.cjs
Normal file
364
tests/reviewer-config-federation.test.cjs
Normal file
@@ -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',
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -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)');
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user