* 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 <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/graceful-rams-dance.md
Normal file
5
.changeset/graceful-rams-dance.md
Normal file
@@ -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)
|
||||
@@ -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.<agent-type>` and
|
||||
`review.models.<cli>` keys are matched against `^[a-zA-Z0-9_-]+$`. Inputs
|
||||
- **Agent-type and CLI slug validation.** `agent_skills.<agent-type>` 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.<cli>` 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.
|
||||
</security>
|
||||
|
||||
<required_reading>
|
||||
@@ -148,9 +153,23 @@ gsd_run query config-set brave_search null
|
||||
|
||||
<step name="section_2_review_models">
|
||||
|
||||
`review.models.<cli>` 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.<cli>` 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.<cli>` 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.<slug> 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.<cli> "<model id>"
|
||||
|
||||
After each update, return to the "Review model CLI mapping — what next?" question.
|
||||
Loop until the user selects "Done".
|
||||
|
||||
The `review.models.<cli>` 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.
|
||||
</step>
|
||||
|
||||
<step name="section_3_agent_skills">
|
||||
@@ -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.<slug> "<skill-a,skill-b,skill-c>"
|
||||
gsd_run query config-set agent_skills.<slug> '["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".
|
||||
</step>
|
||||
@@ -278,12 +315,10 @@ Search Integrations
|
||||
| search_gitignored | true | false |
|
||||
|
||||
Code Review CLI Routing
|
||||
| CLI | Command |
|
||||
| Lane | Model id |
|
||||
|-------------|--------------------------------------|
|
||||
| claude | <value or (session model default)> |
|
||||
| codex | <value or (unset)> |
|
||||
| gemini | <value or (unset)> |
|
||||
| opencode | <value or (unset)> |
|
||||
| <lane> | <value or (unset)> |
|
||||
| ... | ... 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 `****<last-4>` for set keys and `(unset)` for null
|
||||
- [ ] `review.models.<cli>` and `agent_skills.<agent-type>` keys validated against `[a-zA-Z0-9_-]+` before write
|
||||
- [ ] `agent_skills.<agent-type>` slugs validated against `[a-zA-Z0-9_-]+` before write; `review.models.<cli>` 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
|
||||
</success_criteria>
|
||||
|
||||
@@ -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)"
|
||||
}
|
||||
}
|
||||
@@ -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.<agent-type> 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.<cli> matches the dynamic pattern validator', () => {
|
||||
test('review.models.<cli> 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('"<skill-a,skill-b,skill-c>"'),
|
||||
'the comma-joined string write prescription must be gone — the resolver never splits it'
|
||||
);
|
||||
assert.ok(
|
||||
/config-set agent_skills\.<slug> '\["[^"]{0,80}"(?:,\s*"[^"]{0,80}"){0,20}\]'/.test(src),
|
||||
'workflow must show the JSON array write form (config-set agent_skills.<slug> \'["…","…"]\')'
|
||||
);
|
||||
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.<agent-type>', (t) => {
|
||||
test('config-set round-trips agent_skills.<agent-type> (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'])}`
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user