f7df920681f233ae0fe064ee659550bdf41ff708
4 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
0408276791 |
chore(#2797): federate reviewer config keys off the central schema (#2841)
* 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> |
||
|
Tom Boucher
|
dd21bb7451 |
fix(#2046): config-set <key> null unsets (removes) the key instead of writing "null"
`gsd-tools config-set <key> null` — the "Clear" action documented in
settings-integrations.md / settings-advanced.md — previously fell through the
value parser as the literal STRING "null" and persisted it. Consequences:
"cleared" keys stayed set (config-get returned truthy "null"), and for secret
keys (brave_search/firecrawl/exa_search) a masked success line hid a truthy
4-char value on disk that integrations could pass along as a real credential.
There was also no unset/delete verb at all.
Fix: parse a bare `null` to JS null and short-circuit to a real UNSET that
DELETES the key from config.json — the semantic the docs already describe
("Remove the stored key" / "remove the key by setting it to null"). Deleting
(not persisting JSON null) is the correct clear: a persisted null is still a
present value consumers must special-case.
- src/config.cts:
- parse block: `else if (val === 'null') parsedValue = null;`
- cmdConfigSet: when parsedValue === null, short-circuit BEFORE the typed
per-key validator gauntlet (so clearing an enum/boolean/number key removes
it rather than being rejected) and before the project_code special-case;
mask the previous value for secret keys in the output.
- new `unsetConfigValue()` + `_unsetNestedValue()` mirroring setConfigValue/
_setNestedValue: same prototype-pollution guard, but never creates missing
intermediates and never prunes empty parents; returns { previousValue,
existed }. Unsetting a never-set key is an idempotent no-op success.
- tests/config.test.cjs: new suite covering non-secret routing key, secret key,
typed-enum-key bypass (context), idempotent unset, literal-"null"-on-disk
guard, the unset-path prototype-pollution guard (alert #26 parity), and a
4-segment deep-nested unset.
- tests/review-model-config.test.cjs: update the stale round-trip test that
codified the bug (asserted config-set null → config-get returns "null") to the
fixed contract — the model key is removed; the review workflow's
`[ -n "$VAR" ] && [ "$VAR" != "null" ]` guard handles the empty read as
"no override → reviewer default", same as the old "null" sentinel.
The 4 documented "Clear" flows (settings-integrations.md, settings-advanced.md)
were verified — their prose already describes removal, so the fix makes them
accurate rather than aspirational; no doc wording change required.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Tom Boucher
|
bf73cbe1a4 |
test(#2688): add review.models.claude tests for per-runtime review model config (#2748)
Adds two tests to review-model-config.test.cjs: - isValidConfigKey accepts review.models.claude (schema validation) - round-trip: config-set then config-get for review.models.claude The dynamic key pattern (^review\.models\.[a-zA-Z0-9_-]+$), the workflow model-read logic in review.md, and the CONFIGURATION.md docs were already in place. Only the claude-specific test coverage was missing. Closes #2688 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
chudeemeke
|
7e2217186a |
feat(review): add per-CLI model selection via config (#1859)
* feat(review): add per-CLI model selection via config - Add review.models.<cli> dynamic config keys to VALID_CONFIG_KEYS - Update review.md to read model preferences via config-get at runtime - Null/missing values fall back to CLI defaults (backward compatible) - Add key suggestion for common typo (review.model) - Update planning-config reference doc Closes #1849 * fix(review): handle absent and null model config gracefully Address PR #1859 review feedback from @trek-e: 1. Add `|| true` to all four config-get subshell invocations in review.md so that an absent review.models.<cli> key does not produce a non-zero exit from the subshell. cmdConfigGet calls error() (process.exit(1)) when the key path is missing; the 2>/dev/null suppresses the message but the exit code was still discarded silently. The || true makes the fall-through explicit and survives future set -e adoption. 2. Add `&& [ "$VAR" != "null" ]` to all four if guards. cmdConfigSet does not parse the literal 'null' as JSON null — it stores the string 'null' — and cmdConfigGet --raw returns the literal text 'null' for that value. Without the extra guard the workflow would pass `-m "null"` to the CLI, which crashes. The issue spec documents null as the "fall back to CLI default" sentinel, so this restores the contract. 3. Add tests/review-model-config.test.cjs covering all five cases trek-e listed: - isValidConfigKey accepts review.models.gemini (via config-set) - isValidConfigKey accepts review.models.codex (via config-set) - review.model is rejected and suggests review.models.<cli-name> - config-set then config-get round-trip with a model ID - config-set then config-get round-trip with null (returns "null") Tests follow the node:test + node:assert/strict pattern from tests/agent-skills.test.cjs and use runGsdTools from helpers.cjs. Closes #1849 |