diff --git a/.changeset/lane-effort-from-review-config.md b/.changeset/lane-effort-from-review-config.md new file mode 100644 index 000000000..4d20a9d04 --- /dev/null +++ b/.changeset/lane-effort-from-review-config.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4275 +--- +**Cross-AI reviewer lanes no longer take their reasoning effort from the plan checker** — the three lanes that carry a reasoning level on their command line (`codex`, `claude`, `opencode`) resolved it by querying the `gsd-plan-checker` agent through a hardcoded agent id, so under every shipped model profile they ran at that structural verifier's `low`, and because the rendered argument is a command-line config override it silently beat the effort configured for the reviewer CLI itself. At `low` a large plan set could end the model's turn with no final message, leaving an empty lane whose stub read as a crash. Effort is now declared per lane: set `review.effort.codex` (or `.claude`/`.opencode`), leave it unset for the lane's `high` review default, or set `inherit` to emit no argument at all and let your own CLI configuration decide. An unrecognized level falls back to the lane default instead of being forwarded, and the host still clamps the result to what it supports. The empty-output stub now names the effort the lane ran at and distinguishes a clean exit from a timeout, a crash, and a binary that never started. The nine lanes with no effort channel are unchanged: they declared no key before and emit no argument now. Resolving effort in-process also removes up to twelve subprocess spawns per review. (#4255) diff --git a/capabilities/antigravity/capability.json b/capabilities/antigravity/capability.json index dd1a58071..269beeb99 100644 --- a/capabilities/antigravity/capability.json +++ b/capabilities/antigravity/capability.json @@ -139,6 +139,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.antigravity", "modelConfigKey": "review.models.agy", + "effortConfigKey": null, + "defaultEffort": null, "handler": "antigravity" }, "config": { diff --git a/capabilities/claude/capability.json b/capabilities/claude/capability.json index 6b9474d61..b73233255 100644 --- a/capabilities/claude/capability.json +++ b/capabilities/claude/capability.json @@ -152,6 +152,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.claude", "modelConfigKey": "review.models.claude", + "effortConfigKey": "review.effort.claude", + "defaultEffort": "high", "handler": null }, "config": { @@ -169,6 +171,11 @@ "type": "number", "default": -1, "description": "Outer wall-clock timeout override (seconds) for the Claude 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.effort.claude": { + "type": "string", + "default": "", + "description": "Reasoning effort for the Claude reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." } } } diff --git a/capabilities/coderabbit/capability.json b/capabilities/coderabbit/capability.json index fdc8c649a..70c49949a 100644 --- a/capabilities/coderabbit/capability.json +++ b/capabilities/coderabbit/capability.json @@ -38,6 +38,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.coderabbit", "modelConfigKey": null, + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { diff --git a/capabilities/codex/capability.json b/capabilities/codex/capability.json index 77dd23b59..487d1304d 100644 --- a/capabilities/codex/capability.json +++ b/capabilities/codex/capability.json @@ -147,6 +147,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.codex", "modelConfigKey": "review.models.codex", + "effortConfigKey": "review.effort.codex", + "defaultEffort": "high", "handler": null }, "config": { @@ -164,6 +166,11 @@ "type": "number", "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.effort.codex": { + "type": "string", + "default": "", + "description": "Reasoning effort for the Codex reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." } } } diff --git a/capabilities/cursor/capability.json b/capabilities/cursor/capability.json index 3929cc2d0..f862d6b55 100644 --- a/capabilities/cursor/capability.json +++ b/capabilities/cursor/capability.json @@ -150,6 +150,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.cursor", "modelConfigKey": "review.models.cursor", + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { diff --git a/capabilities/gemini/capability.json b/capabilities/gemini/capability.json index c16bbe3af..af0f4b36c 100644 --- a/capabilities/gemini/capability.json +++ b/capabilities/gemini/capability.json @@ -39,6 +39,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.gemini", "modelConfigKey": "review.models.gemini", + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { diff --git a/capabilities/kimi-code/capability.json b/capabilities/kimi-code/capability.json index ddb1e7299..d313371d6 100644 --- a/capabilities/kimi-code/capability.json +++ b/capabilities/kimi-code/capability.json @@ -143,6 +143,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.kimi-code", "modelConfigKey": "review.models.kimi-code", + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { diff --git a/capabilities/llama-cpp/capability.json b/capabilities/llama-cpp/capability.json index 1f3300564..9f8c03bbb 100644 --- a/capabilities/llama-cpp/capability.json +++ b/capabilities/llama-cpp/capability.json @@ -37,6 +37,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.llama_cpp", "modelConfigKey": "review.models.llama_cpp", + "effortConfigKey": null, + "defaultEffort": null, "handler": "openai-compatible" }, "config": { diff --git a/capabilities/lm-studio/capability.json b/capabilities/lm-studio/capability.json index 0f88dc863..c2ff8647c 100644 --- a/capabilities/lm-studio/capability.json +++ b/capabilities/lm-studio/capability.json @@ -37,6 +37,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.lm_studio", "modelConfigKey": "review.models.lm_studio", + "effortConfigKey": null, + "defaultEffort": null, "handler": "openai-compatible" }, "config": { diff --git a/capabilities/ollama/capability.json b/capabilities/ollama/capability.json index c6c3a9a3c..460c0cf75 100644 --- a/capabilities/ollama/capability.json +++ b/capabilities/ollama/capability.json @@ -37,6 +37,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.ollama", "modelConfigKey": "review.models.ollama", + "effortConfigKey": null, + "defaultEffort": null, "handler": "openai-compatible" }, "config": { diff --git a/capabilities/opencode/capability.json b/capabilities/opencode/capability.json index 55c54795e..22e2ed158 100644 --- a/capabilities/opencode/capability.json +++ b/capabilities/opencode/capability.json @@ -166,6 +166,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.opencode", "modelConfigKey": "review.models.opencode", + "effortConfigKey": "review.effort.opencode", + "defaultEffort": "high", "handler": "opencode" }, "config": { @@ -183,6 +185,11 @@ "type": "number", "default": -1, "description": "Outer wall-clock timeout override (seconds) for the OpenCode 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.effort.opencode": { + "type": "string", + "default": "", + "description": "Reasoning effort for the OpenCode reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." } } } diff --git a/capabilities/qwen/capability.json b/capabilities/qwen/capability.json index ae7ed4ad1..07ef831c6 100644 --- a/capabilities/qwen/capability.json +++ b/capabilities/qwen/capability.json @@ -136,6 +136,8 @@ "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.qwen", "modelConfigKey": null, + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 82f39a2d9..454cc984d 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -350,6 +350,41 @@ timeout key either, matching the same narrow key-ownership invariant their `revi 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 lane reasoning effort (`review.effort.*`, #4255) + +The three lanes that can carry a reasoning level on their command line — `codex`, `claude`, +`opencode` — federate a `review.effort.` key, owned by that lane's capability manifest like +its model and timeout keys. Accepted values are the usual effort levels (`minimal`, `low`, +`medium`, `high`, `xhigh`, `max`) plus `inherit`. + +**Resolution order for a lane's effort, highest first:** + +| # | Source | Result | +|---|---|---| +| 1 | `review.effort.` | the level you set, rendered in the host's own effort syntax and clamped to what that host supports | +| 2 | the lane's declared review default | `high` on all three lanes today | +| 3 | nothing declared | **no effort argument is emitted** — the reviewer CLI's own configuration decides | + +Row 1 is the level you asked for, not always the level that runs: each host clamps to its own +supported set. Verified against the shipped catalog, `minimal` reaches Codex and Claude as `low` +while OpenCode takes it as-is; every other level passes through on all three. `REVIEWS.md` records +the level that actually ran, not the one requested. + +`inherit` selects row 3 explicitly: use it when you want your own `~/.codex/config.toml` (or the +equivalent for another CLI) to be the authority, because the argument GSD renders is a +command-line config override and beats that file for the invocation. A value that is not a +recognized level falls back to row 2 rather than being forwarded, since an argument the CLI +rejects kills the lane outright. + +Before #4255 there was no review-specific source at all: every lane's level came from the +`gsd-plan-checker` agent's installed frontmatter — `low` under every shipped model profile — so a +prompt-fed, source-grounded review ran at the level chosen for a fast structural verifier, and a +large plan set could come back as an empty lane. Effort is now a property of the review. + +The lanes with no effort channel (`gemini`, `cursor`, `antigravity`, `qwen`, `coderabbit`, +`kimi-code`, `ollama`, `lm_studio`, `llama_cpp`) federate no key and emit no argument, matching the +same narrow key-ownership invariant their model and timeout keys already follow. + ### Reviewer defaults for `/gsd-review` Use `review.default_reviewers` to scope the no-flag `/gsd-review` run to a subset of detected reviewers. diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 65a45fc72..ab9a5601a 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -1322,7 +1322,8 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load const fsx = require('node:fs'); const os = require('node:os'); const { REVIEWER_LANES, mergeReviewerLanes } = require('./lib/review-lane-descriptor.cjs'); - const { resolveLanePlan } = require('./lib/review-lane-invocation.cjs'); + const { resolveLanePlan, resolveLaneEffort } = require('./lib/review-lane-invocation.cjs'); + const modelCatalog = require('./lib/model-catalog.cjs'); const runner = require('./lib/review-lane-runner.cjs'); const cfgLoader = require('./lib/config-loader.cjs'); const capabilityLoader = require('./lib/capability-loader.cjs'); @@ -1334,13 +1335,15 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load const sub = args[1]; // Fail fast on an unrecognized subcommand. Without this check, `sub` fell through // to the `sub !== 'invoke'` usage-error branch far below (after loading the - // capability registry AND building a per-lane plan for every lane — which itself - // spawns one child `query resolve-execution` process per lane via `effortFor`, - // up to 12 subprocess spawns for the default lane set) before ever reporting the - // error. That made an invalid subcommand slow instead of instant, and under bench - // load (many sequential node spawns) `review-lane bogus` could exceed a caller's - // spawn timeout and be killed before writing anything to stderr — the CI-observed - // failure was empty stdout AND stderr, not the expected usage message (#3148). + // capability registry AND building a per-lane plan for every lane) before ever + // reporting the error. That made an invalid subcommand slow instead of instant, and + // under bench load `review-lane bogus` could exceed a caller's spawn timeout and be + // killed before writing anything to stderr — the CI-observed failure was empty stdout + // AND stderr, not the expected usage message (#3148). The plan path used to be far + // heavier still: it spawned one child `query resolve-execution` process PER LANE to + // fetch effort, up to 12 subprocess spawns for the default set. #4255 resolves effort + // in-process from the lane's own declaration, so that cost is gone; the fail-fast + // check stays because building 12 plans is still work an unrecognized sub should skip. // `plan`/`invoke` are the only subs that need the expensive plan-building path // below; `sections`/`flags` return earlier still. Anything else errors here, before // any of that work starts. @@ -1427,46 +1430,29 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load return; } - // Effort argv is resolved per lane by the host's own execution policy, through the SAME - // `resolve-execution` surface the bash legs used (`--host `), so the host's negotiated - // effortSurface still decides whether an argument is emitted and the catalog still owns the - // syntax (ADR-1239 #2481, ADR-443's escalation ladder). `cmdResolveExecution` writes to - // stdout and exits, so it cannot be called in-process for a value — this spawns the same - // bounded query the legs did, once per selected lane. A lane whose slug is not a known host - // resolves to no effort argument at all. + // Effort argv is resolved from the LANE's own review configuration (#4255), then rendered + // through the host's negotiated `effortSurface` so ADR-1239/#2481's trust-boundary invariant + // still decides whether an argument is emitted at all and the catalog still owns the syntax. // - // NOT `--raw` and NOT `--pick` (#2295). `--raw` prints only the resolved EFFORT ('low') with - // no host-specific rendering at all. `--pick effort_argv_string` used to be the answer — the - // rendered array re-joined into a string ('-c model_reasoning_effort=low') — but the caller - // then had to `.split(/\s+/)` that string back apart to get an argv array, and re-splitting a - // string the callee just joined is a lossy round trip: any argv element that legitimately - // contains a space would come back split into two argv elements, corrupting the very argv it - // was rendered to preserve. Reading the UNPICKED object instead gives both `effort_argv` (a - // real string array, used verbatim, no re-splitting) and `effort_argv_value` (the bare level, - // #2295's `plan.effort`) from the one spawn. - const EMPTY_EFFORT = { argv: [], value: null }; - const effortFor = (slug) => { + // What this replaced: a `query resolve-execution gsd-plan-checker --host ` spawn per + // lane. The agent id was a hardcoded literal, so the `--host` argument chose only the argv + // RENDERING while the LEVEL always came from the installed plan-checker's frontmatter — `low` + // under every shipped model profile. Every prompt-fed reviewer therefore ran at a fast + // structural verifier's effort, and because the rendered argument is a CLI config override it + // silently beat the effort the operator had configured for that CLI. At `low` a large + // source-grounded prompt makes a model end its turn with no final message, so the lane came + // back empty and the stub read as a crash. + // + // `resolveLaneEffort` is pure and lives beside the other lane resolution; this closure only + // injects the rendering, which needs the registry and the catalog. + const renderLaneEffort = (host, level) => { try { - const r = cp.spawnSync( - process.execPath, - [__filename, 'query', 'resolve-execution', 'gsd-plan-checker', '--host', slug], - { cwd, encoding: 'utf8', timeout: 15000, killSignal: 'SIGKILL', maxBuffer: 1024 * 1024 }, - ); - if (r.status !== 0) return EMPTY_EFFORT; - let parsed; - try { - parsed = JSON.parse(String(r.stdout || '')); - } catch { return EMPTY_EFFORT; } - if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) return EMPTY_EFFORT; - const argv = Array.isArray(parsed.effort_argv) - ? parsed.effort_argv.filter((a) => typeof a === 'string' && a !== '') - : []; - const value = typeof parsed.effort_argv_value === 'string' && parsed.effort_argv_value - ? parsed.effort_argv_value - : null; - return { argv, value }; - } catch { return EMPTY_EFFORT; } + const surface = commands.effortSurfaceForHost(cwd, host); + const r = modelCatalog.renderEffortArgv(host, level, surface); + return { argv: Array.isArray(r && r.argv) ? r.argv : [], value: (r && r.value) || null }; + } catch { return { argv: [], value: null }; } }; + const effortFor = (lane) => resolveLaneEffort(lane, configGet, renderLaneEffort); /** * Per-lane prompt budget (#2797 semantics, preserved exactly). @@ -1498,7 +1484,7 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load // so losing all of them to one bad manifest is strictly worse. Belt and braces on purpose. let r; try { - const effort = effortFor(slug); + const effort = effortFor(lane); r = resolveLanePlan({ lane, configGet, runDir, repoRoot, effortArgs: effort.argv, effortValue: effort.value }); } catch (e) { return { slug, ok: false, reason: 'malformed_lane', detail: `resolver threw: ${e && e.message ? e.message : String(e)}` }; @@ -1658,7 +1644,7 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load `model override (it declares no modelConfigKey). The review will use the CLI's own default.\n`, ); } - const instanceEffort = effortFor(entry.slug); + const instanceEffort = effortFor(lane); const overridden = resolveLanePlan({ lane, configGet: (k) => (key && k === key ? instanceModel : configGet(k)), diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index 58413b523..338372f8c 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -233,6 +233,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.antigravity", "modelConfigKey": "review.models.agy", + "effortConfigKey": null, + "defaultEffort": null, "handler": "antigravity" }, "config": { @@ -650,6 +652,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.claude", "modelConfigKey": "review.models.claude", + "effortConfigKey": "review.effort.claude", + "defaultEffort": "high", "handler": null }, "config": { @@ -667,6 +671,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Outer wall-clock timeout override (seconds) for the Claude 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.effort.claude": { + "type": "string", + "default": "", + "description": "Reasoning effort for the Claude reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." } } }, @@ -1093,6 +1102,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.coderabbit", "modelConfigKey": null, + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { @@ -1252,6 +1263,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.codex", "modelConfigKey": "review.models.codex", + "effortConfigKey": "review.effort.codex", + "defaultEffort": "high", "handler": null }, "config": { @@ -1269,6 +1282,11 @@ const capabilities = { "type": "number", "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.effort.codex": { + "type": "string", + "default": "", + "description": "Reasoning effort for the Codex reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." } } }, @@ -1524,6 +1542,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.cursor", "modelConfigKey": "review.models.cursor", + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { @@ -1805,6 +1825,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.gemini", "modelConfigKey": "review.models.gemini", + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { @@ -2409,6 +2431,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.kimi-code", "modelConfigKey": "review.models.kimi-code", + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { @@ -2521,6 +2545,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.llama_cpp", "modelConfigKey": "review.models.llama_cpp", + "effortConfigKey": null, + "defaultEffort": null, "handler": "openai-compatible" }, "config": { @@ -2585,6 +2611,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.lm_studio", "modelConfigKey": "review.models.lm_studio", + "effortConfigKey": null, + "defaultEffort": null, "handler": "openai-compatible" }, "config": { @@ -2873,6 +2901,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.ollama", "modelConfigKey": "review.models.ollama", + "effortConfigKey": null, + "defaultEffort": null, "handler": "openai-compatible" }, "config": { @@ -3066,6 +3096,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.opencode", "modelConfigKey": "review.models.opencode", + "effortConfigKey": "review.effort.opencode", + "defaultEffort": "high", "handler": "opencode" }, "config": { @@ -3083,6 +3115,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Outer wall-clock timeout override (seconds) for the OpenCode 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.effort.opencode": { + "type": "string", + "default": "", + "description": "Reasoning effort for the OpenCode reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." } } }, @@ -3425,6 +3462,8 @@ const capabilities = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.qwen", "modelConfigKey": null, + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { @@ -4869,6 +4908,7 @@ const configKeys = { "review.models.claude": "claude", "review.max_prompt_tokens_per_reviewer.claude": "claude", "review.timeouts.claude": "claude", + "review.effort.claude": "claude", "claude_orchestration.enabled": "claude-orchestration", "claude_orchestration.execution_backend": "claude-orchestration", "claude_orchestration.min_agent_sdk_version": "claude-orchestration", @@ -4879,6 +4919,7 @@ const configKeys = { "review.models.codex": "codex", "review.max_prompt_tokens_per_reviewer.codex": "codex", "review.timeouts.codex": "codex", + "review.effort.codex": "codex", "review.models.cursor": "cursor", "review.max_prompt_tokens_per_reviewer.cursor": "cursor", "workflow.drift_threshold": "drift", @@ -4928,6 +4969,7 @@ const configKeys = { "review.models.opencode": "opencode", "review.max_prompt_tokens_per_reviewer.opencode": "opencode", "review.timeouts.opencode": "opencode", + "review.effort.opencode": "opencode", "workflow.pattern_mapper": "pattern-mapper", "profile-pipeline.enabled": "profile-pipeline", "review.max_prompt_tokens_per_reviewer.qwen": "qwen", @@ -5007,6 +5049,12 @@ const configSchema = { "default": -1, "description": "Outer wall-clock timeout override (seconds) for the Claude 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.effort.claude": { + "owner": "claude", + "type": "string", + "default": "", + "description": "Reasoning effort for the Claude reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." + }, "claude_orchestration.enabled": { "owner": "claude-orchestration", "type": "boolean", @@ -5081,6 +5129,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.effort.codex": { + "owner": "codex", + "type": "string", + "default": "", + "description": "Reasoning effort for the Codex reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." + }, "review.models.cursor": { "owner": "cursor", "type": "string", @@ -5391,6 +5445,12 @@ const configSchema = { "default": -1, "description": "Outer wall-clock timeout override (seconds) for the OpenCode 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.effort.opencode": { + "owner": "opencode", + "type": "string", + "default": "", + "description": "Reasoning effort for the OpenCode reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." + }, "workflow.pattern_mapper": { "owner": "pattern-mapper", "type": "boolean", @@ -5638,6 +5698,8 @@ const runtimes = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.antigravity", "modelConfigKey": "review.models.agy", + "effortConfigKey": null, + "defaultEffort": null, "handler": "antigravity" }, "config": { @@ -5926,6 +5988,8 @@ const runtimes = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.claude", "modelConfigKey": "review.models.claude", + "effortConfigKey": "review.effort.claude", + "defaultEffort": "high", "handler": null }, "config": { @@ -5943,6 +6007,11 @@ const runtimes = { "type": "number", "default": -1, "description": "Outer wall-clock timeout override (seconds) for the Claude 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.effort.claude": { + "type": "string", + "default": "", + "description": "Reasoning effort for the Claude reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." } } }, @@ -6306,6 +6375,8 @@ const runtimes = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.codex", "modelConfigKey": "review.models.codex", + "effortConfigKey": "review.effort.codex", + "defaultEffort": "high", "handler": null }, "config": { @@ -6323,6 +6394,11 @@ const runtimes = { "type": "number", "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.effort.codex": { + "type": "string", + "default": "", + "description": "Reasoning effort for the Codex reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." } } }, @@ -6578,6 +6654,8 @@ const runtimes = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.cursor", "modelConfigKey": "review.models.cursor", + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { @@ -7084,6 +7162,8 @@ const runtimes = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.kimi-code", "modelConfigKey": "review.models.kimi-code", + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { @@ -7272,6 +7352,8 @@ const runtimes = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.opencode", "modelConfigKey": "review.models.opencode", + "effortConfigKey": "review.effort.opencode", + "defaultEffort": "high", "handler": "opencode" }, "config": { @@ -7289,6 +7371,11 @@ const runtimes = { "type": "number", "default": -1, "description": "Outer wall-clock timeout override (seconds) for the OpenCode 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.effort.opencode": { + "type": "string", + "default": "", + "description": "Reasoning effort for the OpenCode reviewer lane: minimal, low, medium, high, xhigh, max, or inherit. Unset falls back to the lane's declared review default (high); inherit emits no effort argument so the CLI's own configuration decides. An unrecognized value falls back to the lane default rather than being forwarded." } } }, @@ -7500,6 +7587,8 @@ const runtimes = { "requiresBinaries": [], "promptBudgetKey": "review.max_prompt_tokens_per_reviewer.qwen", "modelConfigKey": null, + "effortConfigKey": null, + "defaultEffort": null, "handler": null }, "config": { diff --git a/gsd-core/bin/lib/capability-validator.cjs b/gsd-core/bin/lib/capability-validator.cjs index 4a6e69666..3966b22c0 100644 --- a/gsd-core/bin/lib/capability-validator.cjs +++ b/gsd-core/bin/lib/capability-validator.cjs @@ -1849,9 +1849,26 @@ const KNOWN_REVIEWER_FIELDS = new Set([ // `timeoutConfigKey` added by #3274, same optional/backward-compatible shape as // `modelConfigKey` above: a manifest authored before this field existed must keep validating. 'timeoutConfigKey', + // `effortConfigKey` / `defaultEffort` added by #4255, same optional/backward-compatible shape + // as the two above. Lane effort used to be resolved by querying the `gsd-plan-checker` AGENT + // through a hardcoded id, so every prompt-fed lane ran at that verifier's frontmatter effort; + // it is a property of the review, so the lane declares it. A manifest authored before these + // fields existed must keep validating — absent is read as "this lane declares no review + // effort", which emits no effort argument at all. + 'effortConfigKey', + 'defaultEffort', 'handler', ]); +/** + * The effort levels a lane may declare as its review default (#4255, #3533 vocabulary). + * + * `inherit` is deliberately NOT a member. It is a legitimate CONFIGURED value — it selects "emit + * no argument, the CLI's own configuration decides" — but as a DECLARED default it would be a + * second spelling of `null` and split one behaviour across two shapes. + */ +const REVIEWER_EFFORT_LEVELS = new Set(['minimal', 'low', 'medium', 'high', 'xhigh', 'max']); + /** * The closed `runtime.hostBehaviors` vocabulary (ADR-1016, closed via #2801). * @@ -2407,6 +2424,39 @@ function validateReviewerBodyFields(cap) { ); } + // OPTIONAL, mirroring the two keys above (#4255). Absent/null means "this lane declares no + // review effort", which is the shape that emits no effort argument at all — the correct state + // for the nine lanes with no effort channel to feed. An empty string is neither. + if (r.effortConfigKey !== undefined && r.effortConfigKey !== null && + (typeof r.effortConfigKey !== 'string' || r.effortConfigKey.length === 0)) { + errors.push( + ctx + ' reviewer.effortConfigKey must be a dotted config key or null ' + + '(got: ' + describeValue(r.effortConfigKey) + ')', + ); + } + + // A declared default must be a level the effort axis knows, or the lane would render an + // argument the reviewer CLI rejects and kill itself on every run. Closed vocabulary, checked + // here rather than at resolution time so a malformed manifest fails at the trust boundary. + if (r.defaultEffort !== undefined && r.defaultEffort !== null + && !(typeof r.defaultEffort === 'string' && REVIEWER_EFFORT_LEVELS.has(r.defaultEffort))) { + errors.push( + ctx + ' reviewer.defaultEffort must be one of ' + + Array.from(REVIEWER_EFFORT_LEVELS).join('/') + ' or null ' + + '(got: ' + describeValue(r.defaultEffort) + ')', + ); + } + + // A default with nothing to configure it through is a value the operator cannot change — the + // shape #4255 exists to end. Declared together or not at all. + if ((r.defaultEffort !== undefined && r.defaultEffort !== null) + && (r.effortConfigKey === undefined || r.effortConfigKey === null)) { + errors.push( + ctx + ' reviewer.defaultEffort is declared without an effortConfigKey, so the level ' + + 'could never be overridden', + ); + } + // `null` is the declared "no per-lane budget"; an empty string is not. if (r.promptBudgetKey !== null && (typeof r.promptBudgetKey !== 'string' || r.promptBudgetKey.length === 0)) { errors.push( diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index 0b5a4222d..b38cc666f 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -354,6 +354,20 @@ declared in the manifest — timeout floor, probe, prompt/output channel, empty- behaviour that data genuinely cannot express is a named first-party `handler` (ADR-2782 D6), never a bespoke block here. +**Effort and model resolution (#4255).** A lane's reasoning effort and model each resolve through +their own declared key, and the resolution order is inspectable rather than implicit: + +| piece | order, highest first | +|---|---| +| model | pinned reviewer-instance `--model` → the lane's `modelConfigKey` (`review.models.`) → the CLI's own default | +| effort | the lane's `effortConfigKey` (`review.effort.`) → the lane's declared `defaultEffort` → **nothing emitted**, so the CLI's own configuration applies | + +Both come from the LANE. Effort in particular is never read from an agent's execution settings: +until #4255 it was resolved by querying `gsd-plan-checker`, so every prompt-fed lane ran at that +verifier's `low` and, because the rendered argument is a CLI config override, it silently beat the +effort the operator had configured for the reviewer CLI itself. A lane that declares no effort +emits no argument at all — a value borrowed from an unrelated agent is worse than no value. + **Timeout guidance (#2194):** prompt-fed source-grounded reviews are slow — measured ~570 s for Codex at `xhigh` effort and ~525 s for headless Claude on a large plan set. Each lane declares its own `timeoutFloorMs` and the runner enforces it internally, but the **Bash tool call wrapping the diff --git a/src/commands.cts b/src/commands.cts index 565de44bf..fb746f75c 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -721,6 +721,11 @@ function cmdResolveExecution(cwd: string, agentType: string | undefined, raw: bo * applies here exactly as everywhere else: an unknown host, a missing axis, or the * `undocumented` sentinel all degrade to the safe floor rather than being trusted. * Never throws — a lookup failure yields `'none'`, which renders no argument. + * + * On the module's export surface for #4255: the reviewer-lane effort resolver renders a lane's own + * configured level through this same negotiation, so a lane can never emit an argument for a host + * whose negotiated surface does not accept one. One negotiation, both channels — a second copy in + * the lane path is exactly how the two would drift. */ function effortSurfaceForHost(cwd: string, host: string): string { void cwd; @@ -3458,6 +3463,7 @@ function cmdCommitDocsGuardDisable(cwd: string, raw: boolean): void { } export = { + effortSurfaceForHost, groupFilesBySubrepo, determinePhaseStatus, foldPhaseStatus, diff --git a/src/review-lane-descriptor.cts b/src/review-lane-descriptor.cts index c12ec21bd..753b37b8c 100644 --- a/src/review-lane-descriptor.cts +++ b/src/review-lane-descriptor.cts @@ -224,6 +224,30 @@ interface ReviewerLaneCommon { * the lane, and a naming convention that one shipped lane already breaks is not a contract. */ modelConfigKey: string | null; + /** + * Dotted config key holding this lane's REVIEW reasoning effort, or null when the lane has no + * effort channel to feed. + * + * #4255. Lane effort used to be resolved by querying `gsd-plan-checker`'s execution settings + * with a hardcoded agent id, so every prompt-fed lane ran at the plan CHECKER's frontmatter + * effort — `low` under every shipped profile. A cross-AI review is the opposite workload from a + * fast structural verifier, and `low` is where a large prompt makes a model end its turn with no + * final message: the lane came back empty and the stub read as a crash. Effort is a property of + * the REVIEW, so it is declared here beside the lane's other keys and never inherited from an + * agent that has nothing to do with reviewing. + */ + effortConfigKey: string | null; + /** + * The effort this lane runs at when `effortConfigKey` is unset — the review-specific default, + * declared per lane rather than centrally so a lane that wants a different floor can say so. + * + * #4255. `'high'` for the prompt-fed, source-grounded lanes: they read the repository against a + * plan set, which is the one place effort is load-bearing. `null` means GSD has NO + * review-specific value for this lane and the invocation emits no effort argument at all, so the + * CLI's own configuration decides — the correct behaviour when GSD has nothing of its own to + * say. A configured `'inherit'` selects that same no-argument path explicitly (#3533). + */ + defaultEffort: string | null; handler: LaneHandler; } @@ -280,6 +304,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ requiresBinaries: [], promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.gemini', modelConfigKey: 'review.models.gemini', + effortConfigKey: null, + defaultEffort: null, handler: null, }, { @@ -313,6 +339,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ requiresBinaries: [], promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.claude', modelConfigKey: 'review.models.claude', + effortConfigKey: 'review.effort.claude', + defaultEffort: 'high', handler: null, }, { @@ -342,6 +370,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ requiresBinaries: [], promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.codex', modelConfigKey: 'review.models.codex', + effortConfigKey: 'review.effort.codex', + defaultEffort: 'high', handler: null, }, { @@ -369,6 +399,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.coderabbit', // Accepts no model flag at all (review.md:367) — not merely "none configured". modelConfigKey: null, + effortConfigKey: null, + defaultEffort: null, handler: null, }, { @@ -396,6 +428,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ requiresBinaries: [], promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.opencode', modelConfigKey: 'review.models.opencode', + effortConfigKey: 'review.effort.opencode', + defaultEffort: 'high', // Phase 5b (#2799): was `null`. The review is REBUILT from assistant `text` parts; a plain // stdout copy would write the raw JSON envelope as the review (#1936). See LaneHandler. handler: 'opencode', @@ -420,6 +454,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ requiresBinaries: [], promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.qwen', modelConfigKey: null, + effortConfigKey: null, + defaultEffort: null, handler: null, }, { @@ -447,6 +483,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.cursor', // #3653: cursor-agent exposes --model (204 selectable models); wired the same as codex. modelConfigKey: 'review.models.cursor', + effortConfigKey: null, + defaultEffort: null, handler: null, }, { @@ -482,6 +520,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ // NOT `review.models.antigravity` — the shipped key is `review.models.agy` (review.md:291) and // Phase 4 federated it under that name. This lane is why the key is declared, not derived. modelConfigKey: 'review.models.agy', + effortConfigKey: null, + defaultEffort: null, handler: 'antigravity', }, { @@ -512,6 +552,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ requiresBinaries: [], promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.ollama', modelConfigKey: 'review.models.ollama', + effortConfigKey: null, + defaultEffort: null, handler: 'openai-compatible', }, { @@ -540,6 +582,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ requiresBinaries: [], promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.lm_studio', modelConfigKey: 'review.models.lm_studio', + effortConfigKey: null, + defaultEffort: null, handler: 'openai-compatible', }, { @@ -568,6 +612,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ requiresBinaries: [], promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.llama_cpp', modelConfigKey: 'review.models.llama_cpp', + effortConfigKey: null, + defaultEffort: null, handler: 'openai-compatible', }, { @@ -614,6 +660,8 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ requiresBinaries: [], promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.kimi-code', modelConfigKey: 'review.models.kimi-code', + effortConfigKey: null, + defaultEffort: null, handler: null, }, ].map((lane) => Object.freeze(lane)) as ReviewerLane[]); diff --git a/src/review-lane-invocation.cts b/src/review-lane-invocation.cts index 65c72e67e..a4e8f4937 100644 --- a/src/review-lane-invocation.cts +++ b/src/review-lane-invocation.cts @@ -265,6 +265,70 @@ export function normalizeHost(raw: string): string { * negative values are deliberately treated as unset too — a timeout has no legitimate zero or * negative value, so no second sentinel (unlike the prompt-budget keys, which use -1) is needed. */ +/** + * The reasoning effort a reviewer lane runs at, and its host-rendered argv (#4255). + * + * `argv` is spliced into `{{effort}}`; `value` is the bare level the runner folds into the + * recorded model designation (`gpt-5.6-sol (reasoning=high)`, #2295). Both are empty/null when + * this lane emits no effort argument, which is a real and correct outcome — see `resolveLaneEffort`. + */ +export interface LaneEffort { + argv: readonly string[]; + value: string | null; + /** Where `value` came from, for diagnostics: the config key, the lane default, or nothing. */ + source: 'config' | 'lane-default' | 'none'; +} + +/** Levels GSD's effort axis accepts (#3533). `inherit` selects the no-argument path. */ +const EFFORT_LEVELS: ReadonlySet = new Set([ + 'minimal', 'low', 'medium', 'high', 'xhigh', 'max', 'inherit', +]); + +/** + * Resolve one lane's reasoning effort from REVIEW configuration (#4255). + * + * Resolution order, highest first: + * 1. `lane.effortConfigKey` — the per-lane review effort the operator set + * 2. `lane.defaultEffort` — the lane's declared review default (`high` for prompt-fed, + * source-grounded lanes) + * 3. nothing — no effort argument is emitted and the reviewer CLI's own configuration decides + * + * A configured `'inherit'` selects (3) explicitly. An unrecognized level is REFUSED rather than + * passed to the host: it falls back to the lane default, because forwarding a typo would render an + * argument the CLI rejects and kill the lane outright. + * + * What this function deliberately does NOT do is consult any agent's execution settings. Before + * #4255 the level came from `gsd-plan-checker`'s installed frontmatter through a hardcoded agent + * id, so every lane ran at a fast structural verifier's `low` — and, because the rendered argument + * is a CLI config override, it silently beat the effort the operator had configured for that CLI + * itself. A value inherited from an unrelated agent is worse than no value at all, which is why + * (3) emits nothing rather than falling back to some other agent's number. + * + * `renderArgv` is injected (the host table and the ADR-2481 surface negotiation live in + * `model-catalog` / `commands`, above this module's layer) so this stays a pure function of its + * inputs and the golden lane table can assert it without a spawn. + */ +export function resolveLaneEffort( + lane: ReviewerLane, + configGet: (key: string) => unknown, + renderArgv: (host: string, level: string) => { argv: readonly string[]; value: string | null }, +): LaneEffort { + const none: LaneEffort = { argv: [], value: null, source: 'none' }; + if (!lane || typeof lane !== 'object') return none; + const configured = lane.effortConfigKey ? configString(configGet(lane.effortConfigKey)) : null; + const valid = configured !== null && EFFORT_LEVELS.has(configured) ? configured : null; + const level = valid ?? configString(lane.defaultEffort); + if (level === null || level === 'inherit') return none; + const rendered = renderArgv(lane.slug, level); + const argv = (rendered.argv ?? []).filter((a): a is string => typeof a === 'string' && a !== ''); + if (argv.length === 0) return none; + return { + argv, + value: configString(rendered.value) ?? level, + source: valid !== null ? 'config' : 'lane-default', + }; +} + export function resolveTimeoutMs( timeoutConfigKey: string | null | undefined, floorMs: number, diff --git a/src/review-lane-runner.cts b/src/review-lane-runner.cts index 24d22415f..6c41aff60 100644 --- a/src/review-lane-runner.cts +++ b/src/review-lane-runner.cts @@ -420,12 +420,20 @@ export async function probeLane( * `extraDiagnostics` carries the raw HTTP response body for the OpenAI-compatible lanes: an error * from such a server arrives with HTTP 4xx/5xx and the JSON in the BODY, so stderr alone is empty * and the body is the only evidence. + * + * `outcome` (#4255) carries the spawn's exit status so the stub can say WHICH empty it is. The + * header alone cannot: a crash, a timeout kill and a model that ended its turn without writing a + * final message all reach here as the same zero bytes, and the third is what a too-low reasoning + * effort produces on a large source-grounded prompt. A clean exit inside the timeout with no + * output is a stopped-short model, and the stub now says so — with the effort it ran at, which is + * the value the operator would change. */ export function writeReviewOrStub( plan: LanePlan, content: string, deps: RunnerDeps, extraDiagnostics?: string, + outcome?: { status: number | null; errorCode?: string }, ): { stubbed: boolean } { if (!isEmptyReview(content)) { deps.writeFile(plan.reviewPath, content.endsWith('\n') ? content : `${content}\n`); @@ -434,10 +442,57 @@ export function writeReviewOrStub( const stderr = deps.exists(plan.errPath) ? deps.readFile(plan.errPath) : ''; const parts = [`${plan.slug} review failed or returned empty output. stderr:`, stderr]; if (extraDiagnostics) parts.push('Raw response body:', extraDiagnostics); + parts.push(emptyOutputDiagnosis(plan, outcome)); deps.writeFile(plan.reviewPath, `${parts.join('\n')}\n`); return { stubbed: true }; } +/** + * One line naming the effort the lane ran at and how the process ended (#4255). + * + * Kept out of the header so the `failed or returned empty output` string every downstream reader + * greps for is untouched — this is an added line, not a reworded one. + */ +export function emptyOutputDiagnosis( + plan: LanePlan, + outcome?: { status: number | null; errorCode?: string }, +): string { + // `effort` lives on the spawn plan only. An HTTP lane reaches a server directly and has no + // reviewer CLI at all, so naming one there would be a lie about what ran (Codex review of + // #4255) — the two transports get different, accurate wording. + const spawned = plan.transport === 'spawn'; + const level = spawned ? plan.effort : null; + const effort = level + ? `ran at effort=${level}` + : spawned + ? "ran with no effort argument, so the reviewer CLI's own configuration applied" + : 'is an HTTP lane and carries no reasoning-effort setting'; + if (!outcome) return `Diagnosis: ${plan.slug} ${effort}.`; + // Four endings, not two. `status` is null for BOTH a timeout kill and a process that never + // ran or died on a signal (ENOENT, SIGKILL) — reporting the latter as "status null" said + // nothing, and folding them together would attach the stopped-short hint to a crash. + const timedOut = outcome.errorCode === 'ETIMEDOUT'; + const neverRan = !timedOut && outcome.status === null; + const cleanExit = outcome.status === 0; + const ending = timedOut + ? 'was killed by the outer timeout' + : neverRan + ? `did not exit normally (${outcome.errorCode ?? 'killed by a signal'})` + : cleanExit + ? 'exited cleanly inside the timeout' + : `exited with status ${String(outcome.status)}`; + // Hedged deliberately. A clean exit with no output is CONSISTENT with a model ending its turn + // without a final message — which is what too low an effort produces on a large prompt — but it + // is equally consistent with the CLI writing its output somewhere this lane did not read. The + // line points at the likeliest cause without asserting it. + const tail = cleanExit + ? ' — a clean exit that produced no output is most often a model ending its turn without' + + ' writing a final message rather than a crash; if this lane carries a reasoning effort,' + + ' raising it is the usual fix.' + : '.'; + return `Diagnosis: ${plan.slug} ${effort} and ${ending}${tail}`; +} + /* ------------------------------------------------------------------ * * Handlers (D6) — named first-party code, never conditionals in data * ------------------------------------------------------------------ */ @@ -1218,7 +1273,7 @@ function runSpawnLane(plan: SpawnPlan, deps: RunnerDeps, repoRoot: string): Lane // folded in as a diff observation, and the citation check must not change that surface. if (plan.evidenceClass !== 'diff-only') review = stampUngroundedReview(review); - const { stubbed } = writeReviewOrStub(plan, review, deps, extra); + const { stubbed } = writeReviewOrStub(plan, review, deps, extra, out); return { slug: plan.slug, ok: true, stubbed, model }; } diff --git a/tests/effort-surface-axis.test.cjs b/tests/effort-surface-axis.test.cjs index ce19f5cba..3b7c5f6c0 100644 --- a/tests/effort-surface-axis.test.cjs +++ b/tests/effort-surface-axis.test.cjs @@ -580,14 +580,20 @@ describe('#2481 — ADR-443 mechanism callers, as they actually exist', () => { describe('#2481 review workflow resolves effort per reviewer', () => { test('shipped orchestration: the live claude lane genuinely receives --effort in its spawned argv', () => { // Phase 5b (#2799) moved the call out of review.md's per-lane bash and into the review-lane - // route's `effortFor()`, which SPAWNS `query resolve-execution … --pick effort_argv_string` - // once per selected lane and folds the result into that lane's argv template. A text grep for - // the string "resolve-execution" in gsd-tools.cjs would pass even if effortFor's result were - // silently dropped before reaching the spawned reviewer, or if the call were dead code. This - // drives the REAL `review-lane invoke` route end-to-end — real cp.spawnSync, a real project - // config, a real claude-shaped shim on PATH — and inspects the argv the shim actually received, - // which is the only way to prove the resolved effort reaches the invocation rather than merely - // that some file mentions the command name. + // route's `effortFor()`, which folds a resolved level into that lane's argv template. A text + // grep in gsd-tools.cjs would pass even if the result were silently dropped before reaching + // the spawned reviewer, or if the call were dead code. This drives the REAL `review-lane + // invoke` route end-to-end — real cp.spawnSync, a real project config, a real claude-shaped + // shim on PATH — and inspects the argv the shim actually received, which is the only way to + // prove the resolved effort reaches the invocation rather than merely that some file mentions + // the command name. + // + // #4255 changed WHERE the level comes from, not whether it must arrive. It used to be read + // from the `gsd-plan-checker` AGENT's execution settings via a hardcoded id, so this row + // configured `effort.routing_tier_defaults` and expected that value in the reviewer's argv. + // A reviewer lane is not that agent, and the coupling is the bug. The row now configures the + // lane's OWN `review.effort.claude` and pins the decoupling in the same spawn: the execution + // routing tier is set to a DIFFERENT level, and leaking it into the reviewer is a failure. const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2481-orchestration-e2e-')); const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2481-orchestration-project-')); try { @@ -637,9 +643,14 @@ describe('#2481 review workflow resolves effort per reviewer', () => { fs.mkdirSync(path.join(projectDir, '.planning'), { recursive: true }); fs.writeFileSync( path.join(projectDir, '.planning', 'config.json'), - // #3531: pin every tier so the expected value is agent-independent — - // the reviewer lane's tier decides, not effort.default. - JSON.stringify({ effort: { routing_tier_defaults: { light: 'xhigh', standard: 'xhigh', heavy: 'xhigh' } } }, null, 2), + // Two levels, deliberately different (#4255). `review.effort.claude` is the reviewer + // lane's own key and is what must reach the shim. `effort.routing_tier_defaults` drives + // the AGENT execution axis and is pinned across every tier to a level that must NOT + // appear — before #4255 it, through gsd-plan-checker, was the only thing that could. + JSON.stringify({ + review: { effort: { claude: 'xhigh' } }, + effort: { routing_tier_defaults: { light: 'minimal', standard: 'minimal', heavy: 'minimal' } }, + }, null, 2), ); const r = cp.spawnSync( @@ -663,7 +674,15 @@ describe('#2481 review workflow resolves effort per reviewer', () => { const argv = fs.readFileSync(seenArgv, 'utf8').trim().split(/\r?\n/); assert.ok( argv.includes('--effort') && argv.includes('xhigh'), - `resolved effort ("xhigh") did not reach the spawned claude reviewer's argv: ${JSON.stringify(argv)}`, + `the lane's own review effort ("xhigh") did not reach the spawned claude reviewer's argv: ${JSON.stringify(argv)}`, + ); + // The decoupling half (#4255), and the reason this row is worth a real spawn: the agent + // execution tier is pinned to `minimal` above. Seeing it here would mean a reviewer lane is + // still taking its effort from an agent's execution settings. + assert.ok( + !argv.includes('minimal'), + 'the AGENT execution routing tier leaked into the reviewer lane\'s argv: ' + + `${JSON.stringify(argv)} — a lane's effort must come from its own review key`, ); } finally { cleanup(dir); diff --git a/tests/review-lane-descriptor.test.cjs b/tests/review-lane-descriptor.test.cjs index 594555b31..9bd6444f4 100644 --- a/tests/review-lane-descriptor.test.cjs +++ b/tests/review-lane-descriptor.test.cjs @@ -1045,3 +1045,49 @@ describe('review-lane CLI overlay invocation (#2927, rows 9–10)', () => { }); }); } + +describe('#4255 — every lane declares where its review effort comes from', () => { + // The two fields exist so a lane's effort is DATA about the lane, resolvable without asking any + // agent for its execution settings. These rows pin the declaration itself: a new lane that adds + // an argv effort channel without saying what effort to run at would otherwise silently inherit + // whatever the resolver's fallback happens to be — the exact shape of the original bug. + test('a lane with an argv effort channel declares BOTH a config key and a default', () => { + for (const lane of REVIEWER_LANES) { + if (lane.invoke.effortChannel !== 'argv') continue; + assert.equal(typeof lane.effortConfigKey, 'string', + `${lane.slug} renders an effort argument but declares no effortConfigKey`); + assert.equal(lane.effortConfigKey, `review.effort.${lane.slug}`, + `${lane.slug}: the key is read from config by this exact name`); + assert.equal(typeof lane.defaultEffort, 'string', + `${lane.slug} renders an effort argument but declares no review default`); + } + }); + + test('a lane with no effort channel declares neither', () => { + for (const lane of REVIEWER_LANES) { + if (lane.invoke.effortChannel === 'argv') continue; + assert.equal(lane.effortConfigKey, null, `${lane.slug} has no channel to feed`); + assert.equal(lane.defaultEffort, null, `${lane.slug} has no channel to feed`); + } + }); + + test('the prompt-fed, source-grounded lanes default no lower than high', () => { + // These lanes read the repository against a plan set. Effort is load-bearing exactly here: + // it is where a large prompt at a low level ends the turn with no final message. + const RANK = ['minimal', 'low', 'medium', 'high', 'xhigh', 'max']; + for (const lane of REVIEWER_LANES) { + if (lane.defaultEffort === null) continue; + assert.equal(lane.evidenceClass, 'source-grounded', `${lane.slug}: unexpected class for a default`); + assert.ok(RANK.indexOf(lane.defaultEffort) >= RANK.indexOf('high'), + `${lane.slug} defaults to ${lane.defaultEffort}, below the review floor`); + } + }); + + test('no declared default is the `inherit` sentinel — that is what a null key means', () => { + // `inherit` is a legitimate CONFIGURED value (it selects "emit nothing"), but as a declared + // default it would be a second spelling for `null` and split one behaviour across two shapes. + for (const lane of REVIEWER_LANES) { + assert.notEqual(lane.defaultEffort, 'inherit', `${lane.slug}: use null, not 'inherit'`); + } + }); +}); diff --git a/tests/review-lane-invocation.test.cjs b/tests/review-lane-invocation.test.cjs index 19f85cc04..d8bc9be93 100644 --- a/tests/review-lane-invocation.test.cjs +++ b/tests/review-lane-invocation.test.cjs @@ -26,6 +26,7 @@ const { } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs'); const { resolveLanePlan, + resolveLaneEffort, resolveTimeoutMs, isEmptyReview, normalizeHost, @@ -722,3 +723,142 @@ describe('#2358 design principle: run-scoped temp dirs never collide across proj ); }); }); + +describe('#4255 — lane effort comes from REVIEW config, never from another agent', () => { + // Before this fix `review-lane plan` resolved every lane's effort by spawning + // `query resolve-execution gsd-plan-checker --host `. The agent id was a hardcoded + // literal, so `--host` chose only the argv RENDERING while the LEVEL always came from the + // installed plan-checker's frontmatter — `low` under every shipped model profile. A cross-AI + // review is the opposite workload from a fast structural verifier, the rendered argument is a + // CLI config OVERRIDE (so it beat the effort the operator had set for that CLI), and at `low` a + // large source-grounded prompt makes the model end its turn with no final message: the lane + // came back empty and its stub read as a crash. + // + // These rows pin the replacement. `renderArgv` is injected, so they assert the RESOLUTION — + // which level wins, and whether an argument is emitted at all — independently of any host's + // argv syntax. The syntax itself stays pinned by the GOLDEN table above. + + /** Stand-in for the host renderer: echoes the level back in a recognisable shape. */ + const render = (host, level) => ({ argv: ['-c', `effort=${level}`], value: level }); + /** A host that declares no argv effort surface — renders nothing, as ADR-2481 requires. */ + const renderNone = () => ({ argv: [], value: null }); + const lane = (slug) => REVIEWER_LANES.find((l) => l.slug === slug); + + test('the declared default is used when nothing is configured — high on the prompt-fed lanes', () => { + for (const slug of ['codex', 'claude', 'opencode']) { + const r = resolveLaneEffort(lane(slug), () => undefined, render); + assert.equal(r.value, 'high', `${slug} must default to high, not to another agent's level`); + assert.equal(r.source, 'lane-default'); + assert.deepEqual(r.argv, ['-c', 'effort=high']); + } + }); + + test("a lane's own config key wins over the declared default", () => { + const r = resolveLaneEffort(lane('codex'), (k) => (k === 'review.effort.codex' ? 'xhigh' : undefined), render); + assert.equal(r.value, 'xhigh'); + assert.equal(r.source, 'config'); + }); + + test('each lane reads ONLY its own key — one lane’s effort never leaks into another', () => { + const configGet = (k) => (k === 'review.effort.codex' ? 'minimal' : undefined); + assert.equal(resolveLaneEffort(lane('codex'), configGet, render).value, 'minimal'); + assert.equal(resolveLaneEffort(lane('claude'), configGet, render).value, 'high', + 'claude must fall back to its OWN default, not pick up the codex key'); + }); + + test('a configured `inherit` emits NO argument, so the reviewer CLI’s own config decides', () => { + const r = resolveLaneEffort(lane('codex'), () => 'inherit', render); + assert.deepEqual(r.argv, []); + assert.equal(r.value, null); + assert.equal(r.source, 'none'); + }); + + test('a lane declaring no review effort at all emits no argument', () => { + // The non-vacuity half of the row above: `none` must be reachable from the DECLARATION too, + // not only from an explicit `inherit`. Nine shipped lanes have no effort channel to feed. + for (const l of REVIEWER_LANES.filter((x) => x.effortConfigKey === null)) { + const r = resolveLaneEffort(l, () => 'high', render); + assert.deepEqual(r.argv, [], `${l.slug} declares no effort key and must emit nothing`); + assert.equal(r.source, 'none'); + } + }); + + test('an unrecognized level is REFUSED, not forwarded to the CLI', () => { + // Forwarding a typo renders an argument the CLI rejects, which kills the lane outright — a + // strictly worse outcome than the level the operator meant. Fall back to the declared default. + for (const bogus of ['hihg', 'HIGH ', 'very-high', '', ' ', 'null']) { + const r = resolveLaneEffort(lane('codex'), () => bogus, render); + assert.equal(r.value, 'high', `'${bogus}' must fall back to the lane default`); + assert.equal(r.source, 'lane-default'); + } + }); + + test('a non-string configured value is ignored rather than rendered', () => { + for (const bogus of [42, true, null, {}, ['high']]) { + const r = resolveLaneEffort(lane('codex'), () => bogus, render); + assert.equal(r.value, 'high', `${JSON.stringify(bogus)} must not reach the renderer`); + } + }); + + test('the host’s negotiated surface still decides — a renderer that emits nothing yields none', () => { + // ADR-1239/#2481's trust-boundary invariant: a lane cannot talk a host into accepting an + // argument its negotiated effortSurface does not declare, however the lane is configured. + const r = resolveLaneEffort(lane('codex'), () => 'xhigh', renderNone); + assert.deepEqual(r.argv, []); + assert.equal(r.source, 'none'); + }); + + test('the documented clamp is what the catalog actually does', () => { + // The configuration reference tells the operator which levels survive per host, and a prose + // claim about behaviour is a claim that can rot. Pin it against the real renderer so the two + // cannot drift. (No file is read here — the lane's clamp table IS the thing being asserted.) + const mc = require('../gsd-core/bin/lib/model-catalog.cjs'); + const seen = {}; + for (const host of ['codex', 'claude', 'opencode']) { + seen[host] = Object.fromEntries( + ['minimal', 'low', 'medium', 'high', 'xhigh', 'max'] + .map((l) => [l, mc.renderEffortArgv(host, l, 'argv').value]), + ); + } + assert.equal(seen.codex.minimal, 'low', 'codex clamps minimal to low'); + assert.equal(seen.claude.minimal, 'low', 'claude clamps minimal to low'); + assert.equal(seen.opencode.minimal, 'minimal', 'opencode accepts minimal as-is'); + for (const host of ['codex', 'claude', 'opencode']) { + for (const l of ['low', 'medium', 'high', 'xhigh', 'max']) { + assert.equal(seen[host][l], l, `${host} must pass ${l} through unchanged`); + } + } + }); + + test('the renderer’s clamped value is recorded, not the level asked for', () => { + // #2295 records the effort in REVIEWS.md as `model (reasoning=LEVEL)`. When a host clamps + // (`max` -> `xhigh` on codex), the recorded level must be what actually ran. + const clamping = () => ({ argv: ['-c', 'effort=xhigh'], value: 'xhigh' }); + const r = resolveLaneEffort(lane('codex'), () => 'max', clamping); + assert.equal(r.value, 'xhigh', 'the clamped level is what ran, so it is what is recorded'); + }); + + test('a malformed lane object degrades to no effort instead of throwing', () => { + // The resolver is called from gsd-tools.cjs (untyped) with a lane looked up by slug, which + // can miss. Losing one lane's effort is recoverable; a throw there takes down every lane. + for (const bad of [null, undefined, {}, { slug: 'x' }]) { + assert.deepEqual(resolveLaneEffort(bad, () => 'high', render).argv, []); + } + }); + + test('resolved effort reaches argv only for lanes declaring effortChannel argv', () => { + for (const l of REVIEWER_LANES) { + const eff = resolveLaneEffort(l, () => undefined, render); + const r = resolveLanePlan({ + lane: l, configGet: () => undefined, runDir: RUN, repoRoot: ROOT, + effortArgs: eff.argv, effortValue: eff.value, + }); + if (!r.ok || r.plan.transport !== 'spawn') continue; + const carriesEffort = r.plan.argv.some((a) => String(a).startsWith('effort=')); + assert.equal(carriesEffort, l.invoke.effortChannel === 'argv', + `${l.slug}: effort argv must appear exactly when the lane declares the channel`); + assert.equal(r.plan.effort, l.invoke.effortChannel === 'argv' ? 'high' : null, + `${l.slug}: the recorded effort must match what actually expanded`); + } + }); +}); diff --git a/tests/review-lane-runner.test.cjs b/tests/review-lane-runner.test.cjs index b2e1d22ce..82c2985cc 100644 --- a/tests/review-lane-runner.test.cjs +++ b/tests/review-lane-runner.test.cjs @@ -14,7 +14,7 @@ const path = require('node:path'); const fc = require('fast-check'); const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs'); -const { resolveLanePlan, LANE_UNAVAILABLE } = require('../gsd-core/bin/lib/review-lane-invocation.cjs'); +const { resolveLanePlan, resolveLaneEffort, LANE_UNAVAILABLE } = require('../gsd-core/bin/lib/review-lane-invocation.cjs'); const { checkEgressHost, probeLane, @@ -233,6 +233,108 @@ describe('runner — empty-output policy (#2494 / #2605 / #2794)', () => { } }); + /** + * A plan built through the SAME two steps the CLI runs (#4255): resolve the lane's review + * effort, then expand it into argv. `plan()` above passes no effort at all, which is the right + * default for rows that do not care — but a stub row asserting the effort must not invent it. + */ + function planWithEffort(slug) { + const lane = REVIEWER_LANES.find((l) => l.slug === slug); + const eff = resolveLaneEffort(lane, () => undefined, (host, level) => ({ + argv: ['-c', `model_reasoning_effort=${level}`], value: level, + })); + const r = resolveLanePlan({ + lane, configGet: () => undefined, runDir: RUN, repoRoot: ROOT, + effortArgs: eff.argv, effortValue: eff.value, + }); + assert.equal(r.ok, true, `${slug} failed to resolve`); + return r.plan; + } + + test('#4255 — the stub names the effort and says the exit was clean', () => { + // A crash, a timeout kill and a model that ended its turn without writing a final message all + // reach the stub as the same zero bytes. The third is what a too-low reasoning effort produces + // on a large source-grounded prompt, and it was indistinguishable from the first two — the + // operator read "Codex failed" and went looking for a broken CLI. The stub now names the level + // it ran at (the value they would change) and how the process actually ended. + const p = planWithEffort('codex'); + const d = deps(); + writeReviewOrStub(p, '', d, undefined, { status: 0 }); + const out = d.files[p.reviewPath]; + assert.ok(out.includes('failed or returned empty output'), + 'the header every downstream reader greps for must be untouched'); + assert.ok(/ran at effort=high/.test(out), 'the stub must name the effort the lane ran at'); + assert.ok(/exited cleanly inside the timeout/.test(out)); + assert.ok(/ending its turn without writing a final message/.test(out), + 'a clean exit with no output must be named as the likeliest cause, not left reading as a crash'); + assert.ok(/most often/.test(out), + 'the cause is hedged on purpose: a clean empty exit is consistent with a stopped-short model ' + + 'AND with a CLI writing its output somewhere this lane did not read'); + }); + + test('#4255 — a timeout kill and a crash are NOT reported as stopping short', () => { + // The non-vacuity half: the stopped-short hint must be earned by a clean exit, or it is + // advice that sends the operator to raise effort on a lane that was killed or crashed. + const timedOut = deps(); + writeReviewOrStub(planWithEffort('codex'), '', timedOut, undefined, { status: null, errorCode: 'ETIMEDOUT' }); + const t = timedOut.files[planWithEffort('codex').reviewPath]; + assert.ok(/killed by the outer timeout/.test(t)); + assert.ok(!/most often/.test(t), 'a timeout is not a model stopping short'); + + const crashed = deps(); + writeReviewOrStub(planWithEffort('codex'), '', crashed, undefined, { status: 127 }); + const c = crashed.files[planWithEffort('codex').reviewPath]; + assert.ok(/exited with status 127/.test(c)); + assert.ok(!/most often/.test(c), 'a non-zero exit is not a model stopping short'); + + // `status` is null for a process that never started or died on a signal, exactly as it is for + // a timeout kill. Reporting "status null" named nothing the operator could act on, and the + // two must not collapse into one another (Codex review of #4255). + for (const [label, out, expect] of [ + ['binary missing', { status: null, errorCode: 'ENOENT' }, /did not exit normally \(ENOENT\)/], + ['killed by a signal', { status: null }, /did not exit normally \(killed by a signal\)/], + ]) { + const d = deps(); + writeReviewOrStub(planWithEffort('codex'), '', d, undefined, out); + const text = d.files[planWithEffort('codex').reviewPath]; + assert.match(text, expect, `${label} must be named, not reported as "status null"`); + assert.ok(!/status null/.test(text), `${label}: "status null" tells the operator nothing`); + assert.ok(!/most often/.test(text), `${label} is not a model stopping short`); + } + }); + + test('#4255 — an HTTP lane is not described as having a reviewer CLI', () => { + // ollama is reached directly over HTTP: there is no CLI, so "the CLI's own configuration + // applied" would be a lie about what ran (Codex review of #4255). + const p = plan('ollama'); + const d = deps(); + writeReviewOrStub(p, '', d, undefined, { status: 0 }); + const out = d.files[p.reviewPath]; + assert.ok(/HTTP lane/.test(out)); + assert.ok(!/reviewer CLI/.test(out), 'an HTTP lane has no reviewer CLI to attribute anything to'); + assert.ok(!/effort=/.test(out), 'no level may be claimed for a transport that carries none'); + }); + + test('#4255 — a lane that sent no effort argument says so, rather than naming a level', () => { + // gemini declares no effort channel, so `plan.effort` is null. Printing a level there would + // be a lie about what reached the CLI; the stub says the CLI's own configuration applied. + const p = plan('gemini'); + const d = deps(); + writeReviewOrStub(p, '', d, undefined, { status: 0 }); + const out = d.files[p.reviewPath]; + assert.ok(/no effort argument, so the reviewer CLI's own configuration applied/.test(out)); + assert.ok(!/effort=/.test(out), 'no level may be claimed for a lane that sent none'); + }); + + test('#4255 — the diagnosis is added even when the outcome is unknown', () => { + // The HTTP path has no spawn outcome to pass. The effort half still applies, so the line is + // still written — just without the exit clause it cannot honestly make. + const p = planWithEffort('codex'); + const d = deps(); + writeReviewOrStub(p, '', d); + assert.ok(/ran at effort=high/.test(d.files[p.reviewPath])); + }); + test('the stub is distinguishable from a real review', () => { // The ambiguity between "failed" and "ran cleanly with nothing to report" IS the defect. const p = plan('gemini');