From 65d2839b111d02e9ffe2be5a5c4cfc4143c9baa5 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 21 Aug 2026 02:20:53 -0400 Subject: [PATCH] fix(#3651): prescribe only writes config-set accepts in integrations flow (#3732) * test(#3651): regression rows for workflow config-write prescriptions * fix(#3651): prescribe only writes config-set accepts in integrations flow * fix(#3651): review fixes - single-source lane list, configSchema-derived test set * fix(#3651): canonical cite, review wording fixes, one-element array pin * chore(#3651): backfill changeset pr number --------- Co-authored-by: sim --- .changeset/graceful-rams-dance.md | 5 + gsd-core/workflows/settings-integrations.md | 79 +++++-- ...1-settings-integrations-prescriptions.json | 7 + tests/settings-integrations.test.cjs | 220 +++++++++++++++++- 4 files changed, 281 insertions(+), 30 deletions(-) create mode 100644 .changeset/graceful-rams-dance.md create mode 100644 tests/emitted-drift-acks/3651-settings-integrations-prescriptions.json diff --git a/.changeset/graceful-rams-dance.md b/.changeset/graceful-rams-dance.md new file mode 100644 index 000000000..e222c9b6d --- /dev/null +++ b/.changeset/graceful-rams-dance.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3732 +--- +**`/gsd:config --integrations` no longer prescribes writes that fail** — the review-models section states the real rule (only reviewer lanes whose capability declares a modelConfigKey are settable; the nine settable lanes are enumerated; cursor/qwen/coderabbit named as keyless) instead of a validation pattern that never existed, and agent-skill lists are now written as JSON arrays instead of a comma-joined string that resolves as one broken skill path. (#3651) diff --git a/gsd-core/workflows/settings-integrations.md b/gsd-core/workflows/settings-integrations.md index 956f2096b..a1315e7d1 100644 --- a/gsd-core/workflows/settings-integrations.md +++ b/gsd-core/workflows/settings-integrations.md @@ -26,10 +26,15 @@ log the plaintext value. The workflow follows these rules: - **`config-set` output is masked** for keys in the secret set (`brave_search`, `firecrawl`, `exa_search`) — see `gsd-core/bin/lib/secrets.cjs`. -- **Agent-type and CLI slug validation.** `agent_skills.` and - `review.models.` keys are matched against `^[a-zA-Z0-9_-]+$`. Inputs +- **Agent-type and CLI slug validation.** `agent_skills.` slug + inputs are checked against `^[a-zA-Z0-9_-]+$` before any write; inputs containing path separators (`/`, `\`, `..`), whitespace, or shell - metacharacters are rejected. This closes off skill-injection attacks. + metacharacters are rejected. This closes off skill-injection attacks on + that open namespace (dynamic key pattern). For `review.models.` no + slug-shape check is needed or performed: the gate is membership in the + closed, registry-derived settable set (see the review-models section + below), which subsumes slug shape — slug shape alone never makes a + `review.models.*` key writable. @@ -148,9 +153,23 @@ gsd_run query config-set brave_search null -`review.models.` is a map that tells the code-review workflow which -shell command to invoke for a given reviewer flavor. Supported flavors: -`claude`, `codex`, `gemini`, `opencode`. +`review.models.` is a closed, registry-derived map that tells the review +workflow which model id a reviewer lane uses. It is not an open namespace: a +`review.models.` key is settable only when that lane's capability +declares a `modelConfigKey`, and `config-set` accepts exactly those keys +(federated from the capability registry). No dynamic-key regex governs this +namespace — any other slug fails with `Unknown config key`. + +Settable keys (the shipped registry's model-bearing lanes): + +`review.models.agy` (Antigravity), `review.models.claude`, `review.models.codex`, +`review.models.gemini`, `review.models.kimi-code`, `review.models.llama_cpp`, +`review.models.lm_studio`, `review.models.ollama`, `review.models.opencode`. + +Reviewer lanes `cursor`, `qwen`, and `coderabbit` declare no `modelConfigKey` — +there is nothing to configure for them here (whether they should have a +per-lane model key is a separate question, out of scope for this workflow). +If the user asks for one of those, say exactly that and skip. ```text AskUserQuestion([ @@ -159,7 +178,7 @@ AskUserQuestion([ header: "Review", multiSelect: false, options: [ - { label: "Configure CLI", description: "Pick a reviewer flavor and set/clear its command" }, + { label: "Configure CLI", description: "Pick a reviewer lane and set/clear its model id" }, { label: "Done", description: "Finish this section" } ] } @@ -171,7 +190,7 @@ If "Configure CLI" is selected, ask: ```text AskUserQuestion([ { - question: "Which reviewer CLI do you want to configure?", + question: "Which reviewer lane do you want to configure? (Common lanes below; any settable lane from the list above works — or type its slug)", header: "CLI", multiSelect: false, options: [ @@ -184,7 +203,18 @@ AskUserQuestion([ ]) ``` -For the selected CLI, show the current value (or `(unset)`) and offer +For a slug received as free text, check it against the settable set above. +If it is not one of the settable keys, print: + +```text +Rejected: review.models. is not settable — only the reviewer lanes whose +keys are enumerated above can be configured here. (cursor, qwen, and +coderabbit have no per-lane model key.) +``` + +and re-prompt. + +For the selected lane, show the current value (or `(unset)`) and offer Leave / Replace / Clear, followed by a text-input prompt for the model id string. Write via: @@ -194,10 +224,6 @@ gsd_run query config-set review.models. "" After each update, return to the "Review model CLI mapping — what next?" question. Loop until the user selects "Done". - -The `review.models.` key is validated by the dynamic pattern -`^review\.models\.[a-zA-Z0-9_-]+$`. Empty CLI slugs and path-containing slugs -are rejected by `config-set` before any write. @@ -249,13 +275,24 @@ spaces, or shell metacharacters). and re-prompt. -For a selected slug, prompt for the comma-separated skill list (text input). -Show the current value if any, offer Leave / Replace / Clear. Write via: +For a selected slug, prompt for the skill list (text input; a comma-separated +reply is fine). Show the current value if any, offer Leave / Replace / Clear. + +Split the reply before writing: the resolver never splits strings, so a +comma-joined string would be stored as ONE skill path that silently fails +resolution at spawn time (#3651 — `gsd-core/references/planning-config.md`: +"Paths cannot be comma-joined into one string; each path must be its own +array element"). Split on commas, trim each entry, drop empty entries, reject +any entry containing a quote character (`'` or `"` — it cannot be written +safely through the single-quoted form), and write the JSON array form: ```bash -gsd_run query config-set agent_skills. "" +gsd_run query config-set agent_skills. '["skills/alpha","skills/beta"]' ``` +A single skill may be written as one bare string or a one-element array — +both resolve identically. + After each update, return to the "Agent skills mapping — what next?" question. Loop until "Done". @@ -278,12 +315,10 @@ Search Integrations | search_gitignored | true | false | Code Review CLI Routing -| CLI | Command | +| Lane | Model id | |-------------|--------------------------------------| -| claude | | -| codex | | -| gemini | | -| opencode | | +| | | +| ... | ... one row per lane the user set | Agent Skills Injection | Agent Type | Skills | @@ -310,6 +345,6 @@ Quick commands: - [ ] User presented with three sections: Search Integrations, Review CLI Routing, Agent Skills Injection - [ ] API keys written plaintext only to `config.json`; never echoed, never logged, never displayed - [ ] Masked confirmation table uses `****` for set keys and `(unset)` for null -- [ ] `review.models.` and `agent_skills.` keys validated against `[a-zA-Z0-9_-]+` before write +- [ ] `agent_skills.` slugs validated against `[a-zA-Z0-9_-]+` before write; `review.models.` slugs accepted only from the registry-derived settable set; skill lists written as JSON arrays (never comma-joined strings) - [ ] Config merge preserves all keys outside the three sections this workflow owns diff --git a/tests/emitted-drift-acks/3651-settings-integrations-prescriptions.json b/tests/emitted-drift-acks/3651-settings-integrations-prescriptions.json new file mode 100644 index 000000000..94d1ed9d7 --- /dev/null +++ b/tests/emitted-drift-acks/3651-settings-integrations-prescriptions.json @@ -0,0 +1,7 @@ +{ + "$comment": "Growth ack (#2914 fragment). Reason: #3651 corrects the two broken prescriptions in settings-integrations.md — the review.models section states the registry-derived settable rule (per-lane modelConfigKey, nine lanes enumerated once as full review.models.* keys, keyless lanes cursor/qwen/coderabbit named) instead of a nonexistent dynamic-key pattern claim, and the agent_skills section prescribes the JSON array write form plus the split-on-commas instruction (quote-bearing entries rejected). settings-integrations.md 16257 -> 18309 LF bytes (+2052, DEFAULT cap 40960). Pinned by the #3651 rows in tests/settings-integrations.test.cjs (workflow text must match the registry's settable set).", + "version": 1, + "paths": { + "settings-integrations.md": "gsd-core/workflows/settings-integrations.md +2052B: registry-derived review.models settable rule + agent_skills JSON-array prescription (#3651)" + } +} diff --git a/tests/settings-integrations.test.cjs b/tests/settings-integrations.test.cjs index 0dc3e7396..6c955df6a 100644 --- a/tests/settings-integrations.test.cjs +++ b/tests/settings-integrations.test.cjs @@ -12,9 +12,14 @@ * - Workflow references the four search API key fields * - Workflow exposes review.models.{claude,codex,gemini,opencode} routing * - Workflow exposes agent_skills. injection input + * - #3651: workflow states the registry-derived review.models settable rule (no + * dynamic-pattern claim) and enumerates exactly the registry's settable lanes + * - #3651: workflow prescribes the JSON array agent_skills write form (never a + * comma-joined string); behavioral pins for array/comma/single shapes * - Masking convention (****last4) is documented in the workflow and the displayed * confirmation pattern does not echo plaintext - * - config-set round-trips all integration keys through VALID_CONFIG_KEYS + dynamic patterns + * - config-set round-trips all integration keys through VALID_CONFIG_KEYS, + * dynamic patterns, and the federated capability registry * - Config merge preserves unrelated keys * - /gsd:settings confirmation output mentions /gsd:settings-integrations * - Negative: invalid agent-type name (path traversal / special char) is rejected @@ -106,7 +111,10 @@ describe('#2529 workflow — review.models routing', () => { } }); - test('review.models. matches the dynamic pattern validator', () => { + test('review.models. keys validate via the federated capability registry', () => { + // #3651: these pass because the capability registry federates each lane's + // modelConfigKey into the valid-key set — NOT via a dynamicKeyPatterns regex + // (no such pattern exists; see the #3651 describe below). for (const cli of ['claude', 'codex', 'gemini', 'opencode']) { assert.ok( isValidConfigKey(`review.models.${cli}`), @@ -136,6 +144,199 @@ describe('#2529 workflow — agent_skills injection', () => { }); }); +// ─── #3651: prescribed writes must match the real config-set contract ─────── + +// The set `config-set` actually accepts for review.models.*: the frozen +// first-party registry's configSchema — the exact map isCapabilityConfigKey +// consults (hasOwnProperty), so this helper cannot drift from the validator. +function collectReviewerModelConfigKeys() { + const registry = require('../gsd-core/bin/lib/capability-registry.cjs'); + const schema = registry.configSchema || {}; + return new Set( + Object.keys(schema) + .filter((k) => k.startsWith('review.models.')) + .map((k) => k.slice('review.models.'.length)) + ); +} + +// Shared shape for the #3651 behavioral rows: write agent_skills for a slug, +// then read back the resolver's structured diagnostic. +function resolveSkillsCount(tmp, slug) { + const diag = runGsdTools(['agent-skills', slug, '--json'], tmp); + assert.ok(diag.success, `agent-skills failed: ${diag.error}`); + return JSON.parse(diag.output); +} + +describe('#3651 workflow — review.models settable-set rule', () => { + test('workflow states the registry-derived review.models rule, not a dynamic-pattern claim (#3651)', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + assert.ok( + !src.includes('^review\\.models\\.'), + 'workflow must not claim a review.models dynamic-key pattern — dynamicKeyPatterns has no review.models entry' + ); + assert.ok( + /modelConfigKey/.test(src), + 'workflow must name the per-lane modelConfigKey rule' + ); + assert.ok( + /capability registry/i.test(src), + 'workflow must state that the settable set is derived from the capability registry' + ); + }); + + test("workflow enumerates exactly the registry's settable review.models lanes (#3651)", () => { + const registryKeys = collectReviewerModelConfigKeys(); + assert.ok( + registryKeys.size >= 9, + `expected the shipped model-bearing lane set from the registry, found ${registryKeys.size}` + ); + const src = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const mentioned = new Set( + [...src.matchAll(/review\.models\.([a-zA-Z0-9_-]+)/g)].map((m) => m[1]) + ); + for (const key of registryKeys) { + assert.ok( + mentioned.has(key), + `workflow must enumerate settable lane review.models.${key} (registry truth)` + ); + } + for (const slug of mentioned) { + assert.ok( + registryKeys.has(slug), + `workflow mentions review.models.${slug} but the registry declares no such settable key` + ); + } + }); + + test('keyless reviewer lanes have no settable review.models key (#3651)', (t) => { + // Lanes whose capability declares modelConfigKey: null — the workflow used + // to walk users into writing these keys, and config-set rejects them. + for (const keyless of ['cursor', 'qwen', 'coderabbit']) { + assert.ok( + !isValidConfigKey(`review.models.${keyless}`), + `review.models.${keyless} must not validate (lane declares no modelConfigKey)` + ); + } + for (const settable of collectReviewerModelConfigKeys()) { + assert.ok( + isValidConfigKey(`review.models.${settable}`), + `review.models.${settable} must validate` + ); + } + + const tmp = createTempProject(); + t.after(() => cleanup(tmp)); + runGsdTools(['config-ensure-section'], tmp); + const r = runGsdTools(['config-set', 'review.models.cursor', 'cursor-model'], tmp); + assert.ok( + !r.success, + 'config-set must reject a keyless lane — the exact error the old workflow steered users into' + ); + }); +}); + +describe('#3651 workflow — agent_skills array-form write', () => { + test('workflow prescribes the JSON array agent_skills write, not a comma-joined string (#3651)', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + assert.ok( + !src.includes('""'), + 'the comma-joined string write prescription must be gone — the resolver never splits it' + ); + assert.ok( + /config-set agent_skills\. '\["[^"]{0,80}"(?:,\s*"[^"]{0,80}"){0,20}\]'/.test(src), + 'workflow must show the JSON array write form (config-set agent_skills. \'["…","…"]\')' + ); + assert.ok( + /[Ss]plit/.test(src) && /comma/i.test(src), + 'workflow must instruct the driving agent to split comma-separated input before the write' + ); + }); + + test('JSON array agent_skills write round-trips and resolves per-element (#3651)', (t) => { + const tmp = createTempProject(); + t.after(() => cleanup(tmp)); + runGsdTools(['config-ensure-section'], tmp); + + const r = runGsdTools( + ['config-set', 'agent_skills.gsd-planner', '["skills/alpha","skills/beta"]'], + tmp + ); + assert.ok(r.success, `array-form set failed: ${r.error}`); + + const cfg = JSON.parse(fs.readFileSync(path.join(tmp, '.planning', 'config.json'), 'utf-8')); + assert.deepStrictEqual( + cfg.agent_skills?.['gsd-planner'], + ['skills/alpha', 'skills/beta'], + 'stored value must be the JSON array, element per skill' + ); + + const parsed = resolveSkillsCount(tmp, 'gsd-planner'); + assert.strictEqual( + parsed.skills_count, + 2, + `array form must resolve as 2 skill paths, got ${parsed.skills_count}` + ); + }); + + test('comma-joined agent_skills value resolves as ONE path — the hazard the array form avoids (#3651)', (t) => { + const tmp = createTempProject(); + t.after(() => cleanup(tmp)); + runGsdTools(['config-ensure-section'], tmp); + + const r = runGsdTools( + ['config-set', 'agent_skills.gsd-planner', 'skills/alpha,skills/beta'], + tmp + ); + assert.ok(r.success, `comma-string set is accepted by config-set (shape is legal): ${r.error}`); + + const parsed = resolveSkillsCount(tmp, 'gsd-planner'); + assert.strictEqual( + parsed.skills_count, + 1, + `a comma-joined string is ONE path (never split) — got ${parsed.skills_count}; if this changes, the workflow prescription and this pin must change together` + ); + }); + + test('single bare-string agent_skills path keeps working (#3651)', (t) => { + const tmp = createTempProject(); + t.after(() => cleanup(tmp)); + runGsdTools(['config-ensure-section'], tmp); + + const r = runGsdTools( + ['config-set', 'agent_skills.gsd-planner', 'skills/solo'], + tmp + ); + assert.ok(r.success, `single-string set failed: ${r.error}`); + + const parsed = resolveSkillsCount(tmp, 'gsd-planner'); + assert.strictEqual(parsed.configured, true, 'a single string path is a configured entry'); + assert.strictEqual(parsed.skills_count, 1, 'one string = one skill path'); + }); + + test('one-element array agent_skills write resolves identically to the bare string (#3651)', (t) => { + const tmp = createTempProject(); + t.after(() => cleanup(tmp)); + runGsdTools(['config-ensure-section'], tmp); + + const r = runGsdTools( + ['config-set', 'agent_skills.gsd-planner', '["skills/solo"]'], + tmp + ); + assert.ok(r.success, `one-element-array set failed: ${r.error}`); + + const cfg = JSON.parse(fs.readFileSync(path.join(tmp, '.planning', 'config.json'), 'utf-8')); + assert.deepStrictEqual( + cfg.agent_skills?.['gsd-planner'], + ['skills/solo'], + 'one-element array must persist verbatim' + ); + + const parsed = resolveSkillsCount(tmp, 'gsd-planner'); + assert.strictEqual(parsed.configured, true); + assert.strictEqual(parsed.skills_count, 1, 'one-element array = one skill path, same as the bare string'); + }); +}); + // ─── Content: masking ──────────────────────────────────────────────────────── describe('#2529 workflow — API key masking', () => { @@ -214,21 +415,24 @@ describe('#2529 config-set round-trip', () => { assert.strictEqual(cfg.review?.models?.codex, 'codex exec --model gpt-5'); }); - test('config-set round-trips agent_skills.', (t) => { + test('config-set round-trips agent_skills. (array form — the shape the workflow prescribes, #3651)', (t) => { const tmp = createTempProject(); t.after(() => cleanup(tmp)); runGsdTools(['config-ensure-section'], tmp); const r = runGsdTools( - ['config-set', 'agent_skills.gsd-executor', 'skill-a,skill-b'], + ['config-set', 'agent_skills.gsd-executor', '["skill-a","skill-b"]'], tmp ); assert.ok(r.success, `agent_skills.gsd-executor set failed: ${r.error}`); const cfg = JSON.parse(fs.readFileSync(path.join(tmp, '.planning', 'config.json'), 'utf-8')); - // Accept either array or string — validator accepts both shapes today. - const v = cfg.agent_skills?.['gsd-executor']; - assert.ok(v === 'skill-a,skill-b' || (Array.isArray(v) && v.join(',') === 'skill-a,skill-b'), - `expected agent_skills.gsd-executor to contain both skills, got ${JSON.stringify(v)}`); + // #3651: the prescribed write form must persist as a real JSON array — the + // either-shape acceptance this row used to allow hid the comma-string hazard. + assert.deepStrictEqual( + cfg.agent_skills?.['gsd-executor'], + ['skill-a', 'skill-b'], + `expected the array form to persist verbatim, got ${JSON.stringify(cfg.agent_skills?.['gsd-executor'])}` + ); }); });