diff --git a/.changeset/merry-wasps-sprint.md b/.changeset/merry-wasps-sprint.md new file mode 100644 index 000000000..2da8a0e96 --- /dev/null +++ b/.changeset/merry-wasps-sprint.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 4160 +--- +**`review.models.cursor` now pins the Cursor reviewer lane's model** — the lane previously discarded any configured model because it declared no `--model` flag; it now injects one exactly like the `codex` lane. (#3653) diff --git a/capabilities/cursor/capability.json b/capabilities/cursor/capability.json index 78659a301..3929cc2d0 100644 --- a/capabilities/cursor/capability.json +++ b/capabilities/cursor/capability.json @@ -129,6 +129,7 @@ "binary": "cursor-agent", "args": [ "-p", + "{{model}}", "--mode", "ask", "--trust", @@ -138,7 +139,7 @@ ], "promptChannel": "argv-file-ref", "outputChannel": "stdout", - "modelArg": null, + "modelArg": "--model", "effortChannel": "none" }, "timeoutFloorMs": 900000, @@ -148,10 +149,15 @@ "evidenceClass": "source-grounded", "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.cursor", - "modelConfigKey": null, + "modelConfigKey": "review.models.cursor", "handler": null }, "config": { + "review.models.cursor": { + "type": "string", + "default": "", + "description": "Model passed to the Cursor reviewer lane." + }, "review.max_prompt_tokens_per_reviewer.cursor": { "type": "number", "default": -1, diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 14516348f..bba87936d 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -240,6 +240,7 @@ The key suffix is **not** always the lane slug. Each lane declares the config ke | `review.models.codex` | string | `null` | Model id for Codex review (injected into --model), e.g. `"gpt-5"` | | `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"` | +| `review.models.cursor` | string | `null` | Model id for Cursor review (injected into --model), e.g. `"cursor-grok-4.5-high"` | | `review.models.kimi-code` | string | `null` | Model id for Kimi Code review (injected into -m) | ### Resolved model recording (#2295) @@ -269,7 +270,7 @@ which schema validates them moved. One consequence follows: `` must now name a **declared reviewer lane**. Previously any slug matching `[a-zA-Z0-9_-]+` was accepted, so a typo or a key left over from a removed reviewer validated silently and was never read. Such a key is now rejected by `config-set`. The declared -lanes are `gemini`, `claude`, `codex`, `opencode`, `agy` (the Antigravity lane — its key suffix is +lanes are `gemini`, `claude`, `codex`, `opencode`, `cursor`, `agy` (the Antigravity lane — its key suffix is the CLI's own name, not the lane slug), `ollama`, `lm_studio` and `llama_cpp`. The same applies to `review.max_prompt_tokens_per_reviewer.`. `review.max_prompt_tokens` @@ -288,9 +289,10 @@ falls back to that lane's built-in floor. For the `antigravity` lane specificall derives its native `agy --print-timeout` flag (roughly 60 seconds under the configured outer value), so raising `review.timeouts.antigravity` raises both bounds together — this is how to fix a reviewer lane being killed mid-run on a large plan set: `gsd config-set review.timeouts.antigravity 900`. -Three lanes — `qwen`, `cursor`, and `coderabbit` — take neither a model flag nor a host and do not -federate a timeout key either, matching the same narrow key-ownership invariant their -`review.models.*`/host keys already follow (each owns only its own prompt-budget key). +Two lanes — `qwen` and `coderabbit` — take neither a model flag nor a host and do not federate a +timeout key either, matching the same narrow key-ownership invariant their `review.models.*`/host +keys already follow (each owns only its own prompt-budget key). `cursor` gained a model flag +(`review.models.cursor`, #3653) but still owns no federated timeout key of its own. ### Reviewer defaults for `/gsd-review` @@ -1280,6 +1282,7 @@ 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.cursor` | string | (CLI default) | Model used when the `--cursor` reviewer is invoked (injected into `--model`) | | `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 — the lane declares which key it reads, so the two need not match | | `review.models.kimi-code` | string | (CLI default) | Model used when the `--kimi-code` reviewer is invoked (injected into `-m`) | | `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` | diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index 351a84c94..80481365a 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -1480,6 +1480,7 @@ const capabilities = { "binary": "cursor-agent", "args": [ "-p", + "{{model}}", "--mode", "ask", "--trust", @@ -1489,7 +1490,7 @@ const capabilities = { ], "promptChannel": "argv-file-ref", "outputChannel": "stdout", - "modelArg": null, + "modelArg": "--model", "effortChannel": "none" }, "timeoutFloorMs": 900000, @@ -1499,10 +1500,15 @@ const capabilities = { "evidenceClass": "source-grounded", "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.cursor", - "modelConfigKey": null, + "modelConfigKey": "review.models.cursor", "handler": null }, "config": { + "review.models.cursor": { + "type": "string", + "default": "", + "description": "Model passed to the Cursor reviewer lane." + }, "review.max_prompt_tokens_per_reviewer.cursor": { "type": "number", "default": -1, @@ -4833,6 +4839,7 @@ const configKeys = { "review.models.codex": "codex", "review.max_prompt_tokens_per_reviewer.codex": "codex", "review.timeouts.codex": "codex", + "review.models.cursor": "cursor", "review.max_prompt_tokens_per_reviewer.cursor": "cursor", "workflow.drift_threshold": "drift", "workflow.drift_action": "drift", @@ -5024,6 +5031,12 @@ const configSchema = { "default": -1, "description": "Outer wall-clock timeout override (seconds) for the Codex reviewer lane. Unset is -1, a sentinel: 0 or a negative number is also treated as unset (a timeout has no legitimate zero/negative value), so no second sentinel is needed. Falls back to the lane's built-in timeoutFloorMs when unset." }, + "review.models.cursor": { + "owner": "cursor", + "type": "string", + "default": "", + "description": "Model passed to the Cursor reviewer lane." + }, "review.max_prompt_tokens_per_reviewer.cursor": { "owner": "cursor", "type": "number", @@ -6494,6 +6507,7 @@ const runtimes = { "binary": "cursor-agent", "args": [ "-p", + "{{model}}", "--mode", "ask", "--trust", @@ -6503,7 +6517,7 @@ const runtimes = { ], "promptChannel": "argv-file-ref", "outputChannel": "stdout", - "modelArg": null, + "modelArg": "--model", "effortChannel": "none" }, "timeoutFloorMs": 900000, @@ -6513,10 +6527,15 @@ const runtimes = { "evidenceClass": "source-grounded", "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.cursor", - "modelConfigKey": null, + "modelConfigKey": "review.models.cursor", "handler": null }, "config": { + "review.models.cursor": { + "type": "string", + "default": "", + "description": "Model passed to the Cursor reviewer lane." + }, "review.max_prompt_tokens_per_reviewer.cursor": { "type": "number", "default": -1, diff --git a/gsd-core/workflows/settings-integrations.md b/gsd-core/workflows/settings-integrations.md index 0d042cdf9..332ef60ba 100644 --- a/gsd-core/workflows/settings-integrations.md +++ b/gsd-core/workflows/settings-integrations.md @@ -163,12 +163,13 @@ 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`. +`review.models.cursor`, `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). +Reviewer lanes `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 @@ -208,8 +209,8 @@ 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.) +keys are enumerated above can be configured here. (qwen and coderabbit have no +per-lane model key.) ``` and re-prompt. diff --git a/src/review-lane-descriptor.cts b/src/review-lane-descriptor.cts index 25877ee15..c12ec21bd 100644 --- a/src/review-lane-descriptor.cts +++ b/src/review-lane-descriptor.cts @@ -195,10 +195,11 @@ interface ReviewerLaneCommon { * config value that is not a positive finite number, falls back to `timeoutFloorMs` unchanged. * For a lane whose `args` template ALSO carries a native tool-side timeout (antigravity's * `--print-timeout`, via the `{{nativeTimeout}}` ARGV_PLACEHOLDER), the resolved value feeds both - * levels — see `resolveLanePlan`'s expansion of that token. Three lanes — `qwen`, `cursor`, - * `coderabbit` — accept neither a model flag nor a host and own no `review.timeouts.` key - * either, matching the same narrow key-ownership invariant `modelConfigKey` already follows for - * them (#3691 narrows #2797). + * levels — see `resolveLanePlan`'s expansion of that token. Two lanes — `qwen` and `coderabbit` — + * accept neither a model flag nor a host and own no `review.timeouts.` key either, matching + * the same narrow key-ownership invariant `modelConfigKey` already follows for them (#3691 narrows + * #2797). `cursor` gained a model flag (`review.models.cursor`, #3653) but still owns no + * `review.timeouts.cursor` key of its own. */ timeoutConfigKey: string | null; emptyOutput: EmptyOutputPolicy; @@ -431,10 +432,10 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ probe: { kind: 'command-exists', binary: 'cursor-agent' }, invoke: { binary: 'cursor-agent', - args: ['-p', '--mode', 'ask', '--trust', '--output-format', 'text', '{{prompt}}'], + args: ['-p', '{{model}}', '--mode', 'ask', '--trust', '--output-format', 'text', '{{prompt}}'], promptChannel: 'argv-file-ref', outputChannel: 'stdout', - modelArg: null, + modelArg: '--model', effortChannel: 'none', }, timeoutFloorMs: 900_000, @@ -444,7 +445,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ evidenceClass: 'source-grounded', requiresBinaries: [], promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.cursor', - modelConfigKey: null, + // #3653: cursor-agent exposes --model (204 selectable models); wired the same as codex. + modelConfigKey: 'review.models.cursor', handler: null, }, { diff --git a/tests/review-lane-invocation.test.cjs b/tests/review-lane-invocation.test.cjs index 55e237ac4..19f85cc04 100644 --- a/tests/review-lane-invocation.test.cjs +++ b/tests/review-lane-invocation.test.cjs @@ -46,6 +46,7 @@ const FULL_CONFIG = { 'review.models.claude': 'C', 'review.models.codex': 'X', 'review.models.opencode': 'O', + 'review.models.cursor': 'U', 'review.models.agy': 'A', 'review.models.kimi-code': 'K', 'review.models.ollama': 'M', @@ -85,7 +86,7 @@ const GOLDEN = [ { slug: 'coderabbit', binary: 'coderabbit', argv: ['review', '--prompt-only'], stdin: false, out: 'stdout', timeout: 360000 }, { slug: 'opencode', binary: 'opencode', argv: ['run', '--model', 'O', '--effort', 'high', '--format', 'json', '-'], stdin: true, out: 'stdout', timeout: 660000 }, { slug: 'qwen', binary: 'qwen', argv: ['-'], stdin: true, out: 'stdout', timeout: 900000 }, - { slug: 'cursor', binary: 'cursor-agent', argv: ['-p', '--mode', 'ask', '--trust', '--output-format', 'text', FILE_REF], stdin: false, out: 'stdout', timeout: 900000 }, + { slug: 'cursor', binary: 'cursor-agent', argv: ['-p', '--model', 'U', '--mode', 'ask', '--trust', '--output-format', 'text', FILE_REF], stdin: false, out: 'stdout', timeout: 900000 }, // resolveLanePlan fully resolves {{nativeTimeout}} itself (#3274) — this row proves the // unconfigured default reproduces the original literal exactly. { slug: 'antigravity', binary: 'agy', argv: ['--print-timeout', '540s', '--model', 'A', '-p', FILE_REF], stdin: false, out: 'stdout', timeout: 600000 }, @@ -277,14 +278,35 @@ describe('reviewer lane invocation — model resolution', () => { }); test('a lane declaring no model key emits no model argument', () => { - for (const slug of ['qwen', 'cursor', 'coderabbit']) { + for (const slug of ['qwen', 'coderabbit']) { const lane = REVIEWER_LANES.find((l) => l.slug === slug); assert.equal(lane.modelConfigKey, null, `${slug} should declare no model key`); - const r = resolve(slug, { config: { 'review.models.qwen': 'X', 'review.models.cursor': 'X' } }); + const r = resolve(slug, { config: { 'review.models.qwen': 'X' } }); assert.ok(!r.plan.argv.includes('X')); } }); + test('cursor now declares a model key and arg (#3653) — no longer null', () => { + const lane = REVIEWER_LANES.find((l) => l.slug === 'cursor'); + assert.equal(lane.modelConfigKey, 'review.models.cursor'); + assert.equal(lane.invoke.modelArg, '--model'); + }); + + test('an unconfigured cursor lane invokes exactly as it does today (#3653, byte-identical)', () => { + const r = resolve('cursor', { config: {} }); + assert.deepStrictEqual( + r.plan.argv, + ['-p', '--mode', 'ask', '--trust', '--output-format', 'text', r.plan.argv[r.plan.argv.length - 1]], + ); + }); + + test('a configured review.models.cursor reaches --model, positioned right after -p (#3653)', () => { + const r = resolve('cursor', { config: { 'review.models.cursor': 'cursor-grok-4.5-high' } }); + const i = r.plan.argv.indexOf('-p'); + assert.equal(r.plan.argv[i + 1], '--model'); + assert.equal(r.plan.argv[i + 2], 'cursor-grok-4.5-high'); + }); + test('unset, empty, whitespace and the literal string "null" all mean unconfigured', () => { // `"null"` is the four literal characters `config-get --raw` prints for a missing key — every // bash leg tested for it. A config written by an older workflow can still contain it. diff --git a/tests/reviewer-config-federation.test.cjs b/tests/reviewer-config-federation.test.cjs index 7d7e2d5fb..cb3b3ab90 100644 --- a/tests/reviewer-config-federation.test.cjs +++ b/tests/reviewer-config-federation.test.cjs @@ -102,17 +102,20 @@ describe('reviewer config federation — provenance actually moved (#2797)', () }); test('a lane with no model flag and no host owns no MODEL or HOST key (#3691 narrows #2797)', () => { - // Absent-safe (ADR-2782 D4): qwen, cursor and coderabbit take neither a model + // Absent-safe (ADR-2782 D4): qwen and coderabbit take neither a model // argument nor a host. Under #2797 that meant "declares nothing" — the only // way a lane owned a config key was via a model flag or a host. #3691 gave // every CLI lane a `review.max_prompt_tokens_per_reviewer.` key, a - // third legitimate reason to own a key, so qwen/cursor/coderabbit now - // legitimately own their own budget key. The part of the #2797 invariant - // that still holds — a lane must never own a MODEL or HOST key, or another - // lane's budget key, it has no use for — is what this asserts directly. + // third legitimate reason to own a key, so qwen/coderabbit now legitimately + // own their own budget key. `cursor` gained a real model flag (#3653) and + // now legitimately owns `review.models.cursor` too, so it is excluded from + // this loop. The part of the #2797 invariant that still holds — a lane must + // never own a MODEL or HOST key it has no use for, or another lane's budget + // key — is what this asserts directly for the two lanes that still have + // neither. for (const [key, entry] of Object.entries(registry.configSchema || {})) { const owner = entry && entry.owner; - if (!['qwen', 'cursor', 'coderabbit'].includes(owner)) continue; + if (!['qwen', 'coderabbit'].includes(owner)) continue; assert.ok( !key.startsWith('review.models.') && !key.endsWith('_host'), `${owner} must not own a model or host key, but owns "${key}"`, diff --git a/tests/settings-integrations.test.cjs b/tests/settings-integrations.test.cjs index 6c955df6a..bd355adeb 100644 --- a/tests/settings-integrations.test.cjs +++ b/tests/settings-integrations.test.cjs @@ -211,7 +211,8 @@ describe('#3651 workflow — review.models settable-set rule', () => { 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']) { + // `cursor` gained a real modelConfigKey (#3653) and is no longer keyless. + for (const keyless of ['qwen', 'coderabbit']) { assert.ok( !isValidConfigKey(`review.models.${keyless}`), `review.models.${keyless} must not validate (lane declares no modelConfigKey)` @@ -227,7 +228,7 @@ describe('#3651 workflow — review.models settable-set rule', () => { const tmp = createTempProject(); t.after(() => cleanup(tmp)); runGsdTools(['config-ensure-section'], tmp); - const r = runGsdTools(['config-set', 'review.models.cursor', 'cursor-model'], tmp); + const r = runGsdTools(['config-set', 'review.models.qwen', 'qwen-model'], tmp); assert.ok( !r.success, 'config-set must reject a keyless lane — the exact error the old workflow steered users into'