diff --git a/.changeset/quick-finches-greet.md b/.changeset/quick-finches-greet.md new file mode 100644 index 000000000..12206f0b4 --- /dev/null +++ b/.changeset/quick-finches-greet.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 4083 +--- +**Reviewer lane timeouts can now be configured per-lane** — declare `timeoutConfigKey` on a reviewer lane manifest (nine of the twelve shipped lanes now do, via `review.timeouts.`) to override its frozen wall-clock timeout floor from `.planning/config.json`, instead of being stuck with a value that was right for one repository and wrong for another. For the antigravity lane, its native `agy --print-timeout` flag now derives from the same configured value instead of a second hardcoded literal. (#3274) diff --git a/capabilities/antigravity/capability.json b/capabilities/antigravity/capability.json index 9a36450c4..5a4a1aead 100644 --- a/capabilities/antigravity/capability.json +++ b/capabilities/antigravity/capability.json @@ -120,7 +120,7 @@ "binary": "agy", "args": [ "--print-timeout", - "540s", + "{{nativeTimeout}}", "{{model}}", "-p", "{{prompt}}" @@ -131,6 +131,7 @@ "effortChannel": "none" }, "timeoutFloorMs": 600000, + "timeoutConfigKey": "review.timeouts.antigravity", "emptyOutput": "handler-owned", "reviewsSection": "Antigravity", "evidenceClass": "source-grounded", @@ -149,6 +150,11 @@ "type": "number", "default": -1, "description": "Prompt-token budget for the Antigravity reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\". Keyed on the reviewer slug `antigravity`, not the `agy` binary alias used by review.models.agy." + }, + "review.timeouts.antigravity": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Antigravity 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." } } } diff --git a/capabilities/claude/capability.json b/capabilities/claude/capability.json index 06cd2e01e..db757b6ed 100644 --- a/capabilities/claude/capability.json +++ b/capabilities/claude/capability.json @@ -144,6 +144,7 @@ } }, "timeoutFloorMs": 1200000, + "timeoutConfigKey": "review.timeouts.claude", "emptyOutput": "stub-with-stderr", "reviewsSection": "Claude", "evidenceClass": "source-grounded", @@ -162,6 +163,11 @@ "type": "number", "default": -1, "description": "Prompt-token budget for the Claude reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.claude": { + "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." } } } diff --git a/capabilities/coderabbit/capability.json b/capabilities/coderabbit/capability.json index 885e072e7..fdc8c649a 100644 --- a/capabilities/coderabbit/capability.json +++ b/capabilities/coderabbit/capability.json @@ -31,6 +31,7 @@ "effortChannel": "none" }, "timeoutFloorMs": 360000, + "timeoutConfigKey": null, "emptyOutput": "stub-with-stderr", "reviewsSection": "CodeRabbit", "evidenceClass": "diff-only", diff --git a/capabilities/codex/capability.json b/capabilities/codex/capability.json index 7f6c319bc..2ce75523a 100644 --- a/capabilities/codex/capability.json +++ b/capabilities/codex/capability.json @@ -139,6 +139,7 @@ "effortChannel": "argv" }, "timeoutFloorMs": 1200000, + "timeoutConfigKey": "review.timeouts.codex", "emptyOutput": "stub-with-stderr", "reviewsSection": "Codex", "evidenceClass": "source-grounded", @@ -157,6 +158,11 @@ "type": "number", "default": -1, "description": "Prompt-token budget for the Codex reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.codex": { + "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." } } } diff --git a/capabilities/cursor/capability.json b/capabilities/cursor/capability.json index b8495ad8c..58762e8ae 100644 --- a/capabilities/cursor/capability.json +++ b/capabilities/cursor/capability.json @@ -141,6 +141,7 @@ "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": null, "emptyOutput": "stub-with-stderr", "reviewsSection": "Cursor", "evidenceClass": "source-grounded", diff --git a/capabilities/gemini/capability.json b/capabilities/gemini/capability.json index f6da034f6..c16bbe3af 100644 --- a/capabilities/gemini/capability.json +++ b/capabilities/gemini/capability.json @@ -32,6 +32,7 @@ "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": "review.timeouts.gemini", "emptyOutput": "stub-with-stderr", "reviewsSection": "Gemini", "evidenceClass": "source-grounded", @@ -50,6 +51,11 @@ "type": "number", "default": -1, "description": "Prompt-token budget for the Gemini reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.gemini": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Gemini 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." } } } diff --git a/capabilities/kimi-code/capability.json b/capabilities/kimi-code/capability.json index f257de2d0..57826e99c 100644 --- a/capabilities/kimi-code/capability.json +++ b/capabilities/kimi-code/capability.json @@ -135,6 +135,7 @@ "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": "review.timeouts.kimi-code", "emptyOutput": "stub-with-stderr", "reviewsSection": "Kimi Code", "evidenceClass": "source-grounded", @@ -153,6 +154,11 @@ "type": "number", "default": -1, "description": "Prompt-token budget for the Kimi Code reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.kimi-code": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Kimi Code 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." } } } diff --git a/capabilities/llama-cpp/capability.json b/capabilities/llama-cpp/capability.json index 23856f506..1f3300564 100644 --- a/capabilities/llama-cpp/capability.json +++ b/capabilities/llama-cpp/capability.json @@ -30,6 +30,7 @@ "effortChannel": "none" }, "timeoutFloorMs": 120000, + "timeoutConfigKey": "review.timeouts.llama_cpp", "emptyOutput": "stub-with-stderr", "reviewsSection": "llama.cpp", "evidenceClass": "source-grounded", @@ -53,6 +54,11 @@ "type": "number", "default": -1, "description": "Prompt-token budget for the llama.cpp reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.llama_cpp": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the llama.cpp 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." } } } diff --git a/capabilities/lm-studio/capability.json b/capabilities/lm-studio/capability.json index ab156c8df..0f88dc863 100644 --- a/capabilities/lm-studio/capability.json +++ b/capabilities/lm-studio/capability.json @@ -30,6 +30,7 @@ "effortChannel": "none" }, "timeoutFloorMs": 120000, + "timeoutConfigKey": "review.timeouts.lm_studio", "emptyOutput": "stub-with-stderr", "reviewsSection": "LM Studio", "evidenceClass": "source-grounded", @@ -53,6 +54,11 @@ "type": "number", "default": -1, "description": "Prompt-token budget for the LM Studio reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.lm_studio": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the LM Studio 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." } } } diff --git a/capabilities/ollama/capability.json b/capabilities/ollama/capability.json index 5aa890daa..c6c3a9a3c 100644 --- a/capabilities/ollama/capability.json +++ b/capabilities/ollama/capability.json @@ -30,6 +30,7 @@ "effortChannel": "none" }, "timeoutFloorMs": 120000, + "timeoutConfigKey": "review.timeouts.ollama", "emptyOutput": "stub-with-stderr", "reviewsSection": "Ollama", "evidenceClass": "source-grounded", @@ -53,6 +54,11 @@ "type": "number", "default": -1, "description": "Prompt-token budget for the Ollama reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.ollama": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Ollama 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." } } } diff --git a/capabilities/opencode/capability.json b/capabilities/opencode/capability.json index 92521d9e7..e588a991f 100644 --- a/capabilities/opencode/capability.json +++ b/capabilities/opencode/capability.json @@ -158,6 +158,7 @@ "effortChannel": "argv" }, "timeoutFloorMs": 660000, + "timeoutConfigKey": "review.timeouts.opencode", "emptyOutput": "stub-with-stderr", "reviewsSection": "OpenCode", "evidenceClass": "source-grounded", @@ -176,6 +177,11 @@ "type": "number", "default": -1, "description": "Prompt-token budget for the OpenCode reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.opencode": { + "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." } } } diff --git a/capabilities/qwen/capability.json b/capabilities/qwen/capability.json index 6cf2943fd..7fdc69e05 100644 --- a/capabilities/qwen/capability.json +++ b/capabilities/qwen/capability.json @@ -128,6 +128,7 @@ "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": null, "emptyOutput": "stub-with-stderr", "reviewsSection": "Qwen", "evidenceClass": "source-grounded", diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 5b8ef11f9..a18cf91b0 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -276,6 +276,22 @@ The same applies to `review.max_prompt_tokens_per_reviewer.`. `review.max_ (the global default), `review.default_reviewers` and `review.reviewer_instances` describe policy across lanes rather than one lane's behavior, so they remain central and are unaffected. +### Reviewer lane timeouts (`review.timeouts.*`, #3274) + +Nine of the twelve declared reviewer lanes accept an outer wall-clock timeout override, federated +per-lane exactly like `review.max_prompt_tokens_per_reviewer.` above — the key is owned by +that lane's own capability manifest, not a central schema. Keys are seconds: `review.timeouts.gemini`, +`review.timeouts.claude`, `review.timeouts.codex`, `review.timeouts.opencode`, +`review.timeouts.antigravity`, `review.timeouts.kimi-code`, `review.timeouts.ollama`, +`review.timeouts.lm_studio`, `review.timeouts.llama_cpp`. Unset (or `0`/negative/non-numeric) +falls back to that lane's built-in floor. For the `antigravity` lane specifically, this value also +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). + ### 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/docs/how-to/ship-a-reviewer-lane.md b/docs/how-to/ship-a-reviewer-lane.md index a01b5c2c6..8f6cdcaac 100644 --- a/docs/how-to/ship-a-reviewer-lane.md +++ b/docs/how-to/ship-a-reviewer-lane.md @@ -51,6 +51,7 @@ Most lanes are `transport: "spawn"` — GSD runs a binary and reads its output. "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": "review.timeouts.acme", "emptyOutput": "stub-with-stderr", "reviewsSection": "Acme", "evidenceClass": "source-grounded", @@ -65,6 +66,11 @@ Most lanes are `transport: "spawn"` — GSD runs a binary and reads its output. "type": "string", "default": "", "description": "Model passed to the Acme reviewer lane." + }, + "review.timeouts.acme": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Acme reviewer lane." } } } @@ -107,6 +113,7 @@ If your reviewer is a served model endpoint rather than a CLI, use `transport: " "effortChannel": "none" }, "timeoutFloorMs": 120000, + "timeoutConfigKey": "review.timeouts.acme_local", "emptyOutput": "stub-with-stderr", "reviewsSection": "Acme Local", "evidenceClass": "source-grounded", @@ -127,7 +134,7 @@ If your reviewer is a served model endpoint rather than a CLI, use `transport: " Declare the lane's keys in your own manifest `config` block, never in the central schema. A key present in both is a build failure, not a warning — federated ownership is exclusive. -Name `modelConfigKey` and `promptBudgetKey` to match keys you actually declare. A lane pointing at a key nobody owns resolves to nothing, which reads to the user as "my model override is being ignored." +Name `modelConfigKey`, `promptBudgetKey`, and `timeoutConfigKey` to match keys you actually declare. A lane pointing at a key nobody owns resolves to nothing, which reads to the user as "my model override is being ignored." Users then set them the ordinary way, in `.planning/config.json`: diff --git a/docs/reference/capability-manifest.md b/docs/reference/capability-manifest.md index 79eae7a7c..0cd895fdc 100644 --- a/docs/reference/capability-manifest.md +++ b/docs/reference/capability-manifest.md @@ -232,7 +232,7 @@ The shape is **hybrid**: Current role counts across `capabilities/`: `feature` 20, `runtime` 19, `reviewer` 5. -All 12 shipped lane declarations carry all 13 fields below. +All 12 shipped lane declarations carry all 14 fields below. | Field | Type | Notes | |---|---|---| @@ -242,6 +242,7 @@ All 12 shipped lane declarations carry all 13 fields below. | `probe` | object | Availability check. `probe.kind` is a closed enum: `command-exists` \| `command-capability` \| `http-reachable`. `command-capability` additionally takes `binary`, `needle`, and a **required** `timeoutMs` — it exists because a bare binary name can be ambiguous (`kimi` is claimed by both the Kimi Code CLI and the legacy Python `kimi-cli`), and the timeout bound is mandatory because an unbounded `--help \| grep` probe is this repo's named Unbounded Subprocesses defect. | | `invoke` | object | Shape is selected by `transport`. For `spawn`: `binary`, `args[]`, `promptChannel` (`stdin` \| `argv` \| `argv-file-ref` \| `none`), `outputChannel` (`stdout` \| `file-arg`), `outputArg` (required when `outputChannel` is `file-arg`), `modelArg` (string or `null`), `effortChannel` (`none` \| `argv` \| `env`), `env` (optional; an object of environment name/value pairs, string values only, merged over the inherited environment for that one spawn — keys must match the portable environment-name grammar `[A-Za-z_][A-Za-z0-9_]*`, which is a portability policy rather than an OS limit, and `__proto__` is refused because it would be dropped before reaching the child). For `openai-http`: `hostConfigKey`, `defaultHost`, `path`, `modelDiscovery` (`none` \| `first-from-models-endpoint`), `fallbackModel`, `effortChannel`. `args` supports the `{{model}}`, `{{prompt}}`, `{{effort}}`, and `{{output}}` placeholders. **Every field in this object is disclosed at install and bound to the consent signature** — `env` and `defaultHost` by name in the consent prompt, the rest through a residual, so any change to a declared `invoke` field forces re-consent. `env` additionally **refuses execution-primitive names** — `PATH`, `NODE_OPTIONS`, `LD_PRELOAD`, `DYLD_INSERT_LIBRARIES`, `BASH_ENV`, `PYTHONPATH`, `PERL5OPT`, `RUBYOPT`, `GIT_SSH_COMMAND`, `JAVA_TOOL_OPTIONS` and siblings, matched case-insensitively (Windows environment lookup is). A lane needing a specific executable declares an absolute `binary` rather than reshaping the child's `PATH`. That denylist is defence in depth and not the boundary: it cannot be complete against an arbitrary child, and disclosure runs before validation, so install-time consent — which shows every declared pair and warns on execution-primitive names — is what actually gates them. | | `timeoutFloorMs` | number | Measured per-lane floor. Lane divergence here is real and correct — the descriptor's job is to declare divergence in one place, not to promise uniformity. | +| `timeoutConfigKey` | string or `null` | Federated config key holding this lane's outer timeout override, in SECONDS, e.g. `review.timeouts.antigravity`. Falls back to `timeoutFloorMs` when unset or invalid (#3274). | | `emptyOutput` | closed enum | `stub-with-stderr` \| `handler-owned`. | | `reviewsSection` | string | The `REVIEWS.md` heading this lane renders under. Must be unique across the merged roster. | | `evidenceClass` | closed enum | `source-grounded` \| `diff-only` (diff-only findings are down-weighted in consensus). | @@ -282,6 +283,7 @@ An unknown field inside a `reviewer` body is a **non-fatal warning on stderr, ne "effortChannel": "none" }, "timeoutFloorMs": 360000, + "timeoutConfigKey": null, "emptyOutput": "stub-with-stderr", "reviewsSection": "CodeRabbit", "evidenceClass": "diff-only", diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index 4913c3d92..9948c80ea 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -214,7 +214,7 @@ const capabilities = { "binary": "agy", "args": [ "--print-timeout", - "540s", + "{{nativeTimeout}}", "{{model}}", "-p", "{{prompt}}" @@ -225,6 +225,7 @@ const capabilities = { "effortChannel": "none" }, "timeoutFloorMs": 600000, + "timeoutConfigKey": "review.timeouts.antigravity", "emptyOutput": "handler-owned", "reviewsSection": "Antigravity", "evidenceClass": "source-grounded", @@ -243,6 +244,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Prompt-token budget for the Antigravity reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\". Keyed on the reviewer slug `antigravity`, not the `agy` binary alias used by review.models.agy." + }, + "review.timeouts.antigravity": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Antigravity 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." } } }, @@ -634,6 +640,7 @@ const capabilities = { } }, "timeoutFloorMs": 1200000, + "timeoutConfigKey": "review.timeouts.claude", "emptyOutput": "stub-with-stderr", "reviewsSection": "Claude", "evidenceClass": "source-grounded", @@ -652,6 +659,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Prompt-token budget for the Claude reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.claude": { + "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." } } }, @@ -1046,6 +1058,7 @@ const capabilities = { "effortChannel": "none" }, "timeoutFloorMs": 360000, + "timeoutConfigKey": null, "emptyOutput": "stub-with-stderr", "reviewsSection": "CodeRabbit", "evidenceClass": "diff-only", @@ -1203,6 +1216,7 @@ const capabilities = { "effortChannel": "argv" }, "timeoutFloorMs": 1200000, + "timeoutConfigKey": "review.timeouts.codex", "emptyOutput": "stub-with-stderr", "reviewsSection": "Codex", "evidenceClass": "source-grounded", @@ -1221,6 +1235,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Prompt-token budget for the Codex reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.codex": { + "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." } } }, @@ -1466,6 +1485,7 @@ const capabilities = { "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": null, "emptyOutput": "stub-with-stderr", "reviewsSection": "Cursor", "evidenceClass": "source-grounded", @@ -1718,6 +1738,7 @@ const capabilities = { "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": "review.timeouts.gemini", "emptyOutput": "stub-with-stderr", "reviewsSection": "Gemini", "evidenceClass": "source-grounded", @@ -1736,6 +1757,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Prompt-token budget for the Gemini reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.gemini": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Gemini 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." } } }, @@ -2311,6 +2337,7 @@ const capabilities = { "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": "review.timeouts.kimi-code", "emptyOutput": "stub-with-stderr", "reviewsSection": "Kimi Code", "evidenceClass": "source-grounded", @@ -2329,6 +2356,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Prompt-token budget for the Kimi Code reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.kimi-code": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Kimi Code 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." } } }, @@ -2417,6 +2449,7 @@ const capabilities = { "effortChannel": "none" }, "timeoutFloorMs": 120000, + "timeoutConfigKey": "review.timeouts.llama_cpp", "emptyOutput": "stub-with-stderr", "reviewsSection": "llama.cpp", "evidenceClass": "source-grounded", @@ -2440,6 +2473,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Prompt-token budget for the llama.cpp reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.llama_cpp": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the llama.cpp 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." } } }, @@ -2475,6 +2513,7 @@ const capabilities = { "effortChannel": "none" }, "timeoutFloorMs": 120000, + "timeoutConfigKey": "review.timeouts.lm_studio", "emptyOutput": "stub-with-stderr", "reviewsSection": "LM Studio", "evidenceClass": "source-grounded", @@ -2498,6 +2537,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Prompt-token budget for the LM Studio reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.lm_studio": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the LM Studio 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." } } }, @@ -2757,6 +2801,7 @@ const capabilities = { "effortChannel": "none" }, "timeoutFloorMs": 120000, + "timeoutConfigKey": "review.timeouts.ollama", "emptyOutput": "stub-with-stderr", "reviewsSection": "Ollama", "evidenceClass": "source-grounded", @@ -2780,6 +2825,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Prompt-token budget for the Ollama reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.ollama": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Ollama 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." } } }, @@ -2943,6 +2993,7 @@ const capabilities = { "effortChannel": "argv" }, "timeoutFloorMs": 660000, + "timeoutConfigKey": "review.timeouts.opencode", "emptyOutput": "stub-with-stderr", "reviewsSection": "OpenCode", "evidenceClass": "source-grounded", @@ -2961,6 +3012,11 @@ const capabilities = { "type": "number", "default": -1, "description": "Prompt-token budget for the OpenCode reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.opencode": { + "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." } } }, @@ -3294,6 +3350,7 @@ const capabilities = { "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": null, "emptyOutput": "stub-with-stderr", "reviewsSection": "Qwen", "evidenceClass": "source-grounded", @@ -4709,10 +4766,12 @@ const configKeys = { "workflow.api_coverage_gate": "ai-integration", "review.models.agy": "antigravity", "review.max_prompt_tokens_per_reviewer.antigravity": "antigravity", + "review.timeouts.antigravity": "antigravity", "workflow.assumption_delta": "assumption-delta", "workflow.windows_enforce": "broken-windows", "review.models.claude": "claude", "review.max_prompt_tokens_per_reviewer.claude": "claude", + "review.timeouts.claude": "claude", "claude_orchestration.enabled": "claude-orchestration", "claude_orchestration.execution_backend": "claude-orchestration", "claude_orchestration.min_agent_sdk_version": "claude-orchestration", @@ -4721,6 +4780,7 @@ const configKeys = { "review.max_prompt_tokens_per_reviewer.coderabbit": "coderabbit", "review.models.codex": "codex", "review.max_prompt_tokens_per_reviewer.codex": "codex", + "review.timeouts.codex": "codex", "review.max_prompt_tokens_per_reviewer.cursor": "cursor", "workflow.drift_threshold": "drift", "workflow.drift_action": "drift", @@ -4734,17 +4794,21 @@ const configKeys = { "workflow.post_planning_gaps": "gap-analysis", "review.models.gemini": "gemini", "review.max_prompt_tokens_per_reviewer.gemini": "gemini", + "review.timeouts.gemini": "gemini", "graphify.enabled": "graphify", "intel.enabled": "intel", "review.models.kimi-code": "kimi-code", "review.max_prompt_tokens_per_reviewer.kimi-code": "kimi-code", + "review.timeouts.kimi-code": "kimi-code", "workflow.live_dom_uat": "live-dom-uat", "review.models.llama_cpp": "llama-cpp", "review.llama_cpp_host": "llama-cpp", "review.max_prompt_tokens_per_reviewer.llama_cpp": "llama-cpp", + "review.timeouts.llama_cpp": "llama-cpp", "review.models.lm_studio": "lm-studio", "review.lm_studio_host": "lm-studio", "review.max_prompt_tokens_per_reviewer.lm_studio": "lm-studio", + "review.timeouts.lm_studio": "lm-studio", "mempalace.enabled": "mempalace", "mempalace.memory_mode": "mempalace", "mempalace.wing": "mempalace", @@ -4759,8 +4823,10 @@ const configKeys = { "review.models.ollama": "ollama", "review.ollama_host": "ollama", "review.max_prompt_tokens_per_reviewer.ollama": "ollama", + "review.timeouts.ollama": "ollama", "review.models.opencode": "opencode", "review.max_prompt_tokens_per_reviewer.opencode": "opencode", + "review.timeouts.opencode": "opencode", "workflow.pattern_mapper": "pattern-mapper", "profile-pipeline.enabled": "profile-pipeline", "review.max_prompt_tokens_per_reviewer.qwen": "qwen", @@ -4804,6 +4870,12 @@ const configSchema = { "default": -1, "description": "Prompt-token budget for the Antigravity reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\". Keyed on the reviewer slug `antigravity`, not the `agy` binary alias used by review.models.agy." }, + "review.timeouts.antigravity": { + "owner": "antigravity", + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Antigravity 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." + }, "workflow.assumption_delta": { "owner": "assumption-delta", "type": "boolean", @@ -4828,6 +4900,12 @@ const configSchema = { "default": -1, "description": "Prompt-token budget for the Claude reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." }, + "review.timeouts.claude": { + "owner": "claude", + "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." + }, "claude_orchestration.enabled": { "owner": "claude-orchestration", "type": "boolean", @@ -4886,6 +4964,12 @@ const configSchema = { "default": -1, "description": "Prompt-token budget for the Codex reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." }, + "review.timeouts.codex": { + "owner": "codex", + "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.max_prompt_tokens_per_reviewer.cursor": { "owner": "cursor", "type": "number", @@ -4971,6 +5055,12 @@ const configSchema = { "default": -1, "description": "Prompt-token budget for the Gemini reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." }, + "review.timeouts.gemini": { + "owner": "gemini", + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Gemini 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." + }, "graphify.enabled": { "owner": "graphify", "type": "boolean", @@ -4995,6 +5085,12 @@ const configSchema = { "default": -1, "description": "Prompt-token budget for the Kimi Code reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." }, + "review.timeouts.kimi-code": { + "owner": "kimi-code", + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Kimi Code 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." + }, "workflow.live_dom_uat": { "owner": "live-dom-uat", "type": "boolean", @@ -5019,6 +5115,12 @@ const configSchema = { "default": -1, "description": "Prompt-token budget for the llama.cpp reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." }, + "review.timeouts.llama_cpp": { + "owner": "llama-cpp", + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the llama.cpp 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.lm_studio": { "owner": "lm-studio", "type": "string", @@ -5037,6 +5139,12 @@ const configSchema = { "default": -1, "description": "Prompt-token budget for the LM Studio reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." }, + "review.timeouts.lm_studio": { + "owner": "lm-studio", + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the LM Studio 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." + }, "mempalace.enabled": { "owner": "mempalace", "type": "boolean", @@ -5126,6 +5234,12 @@ const configSchema = { "default": -1, "description": "Prompt-token budget for the Ollama reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." }, + "review.timeouts.ollama": { + "owner": "ollama", + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Ollama 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.opencode": { "owner": "opencode", "type": "string", @@ -5138,6 +5252,12 @@ const configSchema = { "default": -1, "description": "Prompt-token budget for the OpenCode reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." }, + "review.timeouts.opencode": { + "owner": "opencode", + "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." + }, "workflow.pattern_mapper": { "owner": "pattern-mapper", "type": "boolean", @@ -5366,7 +5486,7 @@ const runtimes = { "binary": "agy", "args": [ "--print-timeout", - "540s", + "{{nativeTimeout}}", "{{model}}", "-p", "{{prompt}}" @@ -5377,6 +5497,7 @@ const runtimes = { "effortChannel": "none" }, "timeoutFloorMs": 600000, + "timeoutConfigKey": "review.timeouts.antigravity", "emptyOutput": "handler-owned", "reviewsSection": "Antigravity", "evidenceClass": "source-grounded", @@ -5395,6 +5516,11 @@ const runtimes = { "type": "number", "default": -1, "description": "Prompt-token budget for the Antigravity reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\". Keyed on the reviewer slug `antigravity`, not the `agy` binary alias used by review.models.agy." + }, + "review.timeouts.antigravity": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Antigravity 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." } } }, @@ -5657,6 +5783,7 @@ const runtimes = { } }, "timeoutFloorMs": 1200000, + "timeoutConfigKey": "review.timeouts.claude", "emptyOutput": "stub-with-stderr", "reviewsSection": "Claude", "evidenceClass": "source-grounded", @@ -5675,6 +5802,11 @@ const runtimes = { "type": "number", "default": -1, "description": "Prompt-token budget for the Claude reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.claude": { + "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." } } }, @@ -6028,6 +6160,7 @@ const runtimes = { "effortChannel": "argv" }, "timeoutFloorMs": 1200000, + "timeoutConfigKey": "review.timeouts.codex", "emptyOutput": "stub-with-stderr", "reviewsSection": "Codex", "evidenceClass": "source-grounded", @@ -6046,6 +6179,11 @@ const runtimes = { "type": "number", "default": -1, "description": "Prompt-token budget for the Codex reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.codex": { + "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." } } }, @@ -6291,6 +6429,7 @@ const runtimes = { "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": null, "emptyOutput": "stub-with-stderr", "reviewsSection": "Cursor", "evidenceClass": "source-grounded", @@ -6786,6 +6925,7 @@ const runtimes = { "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": "review.timeouts.kimi-code", "emptyOutput": "stub-with-stderr", "reviewsSection": "Kimi Code", "evidenceClass": "source-grounded", @@ -6804,6 +6944,11 @@ const runtimes = { "type": "number", "default": -1, "description": "Prompt-token budget for the Kimi Code reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.kimi-code": { + "type": "number", + "default": -1, + "description": "Outer wall-clock timeout override (seconds) for the Kimi Code 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." } } }, @@ -6967,6 +7112,7 @@ const runtimes = { "effortChannel": "argv" }, "timeoutFloorMs": 660000, + "timeoutConfigKey": "review.timeouts.opencode", "emptyOutput": "stub-with-stderr", "reviewsSection": "OpenCode", "evidenceClass": "source-grounded", @@ -6985,6 +7131,11 @@ const runtimes = { "type": "number", "default": -1, "description": "Prompt-token budget for the OpenCode reviewer lane. Unset is -1, a sentinel: 0 is a legitimate value meaning \"do not trim this lane\", so it cannot double as \"not configured\"." + }, + "review.timeouts.opencode": { + "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." } } }, @@ -7187,6 +7338,7 @@ const runtimes = { "effortChannel": "none" }, "timeoutFloorMs": 900000, + "timeoutConfigKey": null, "emptyOutput": "stub-with-stderr", "reviewsSection": "Qwen", "evidenceClass": "source-grounded", diff --git a/gsd-core/bin/lib/capability-validator.cjs b/gsd-core/bin/lib/capability-validator.cjs index c3ad61a28..3bfdc6ccf 100644 --- a/gsd-core/bin/lib/capability-validator.cjs +++ b/gsd-core/bin/lib/capability-validator.cjs @@ -1824,6 +1824,9 @@ const KNOWN_REVIEWER_FIELDS = new Set([ // missed a configured model and silently disabled the pinned-model escape hatch // #2073 added. A convention one shipped lane already violates is not a contract. 'modelConfigKey', + // `timeoutConfigKey` added by #3274, same optional/backward-compatible shape as + // `modelConfigKey` above: a manifest authored before this field existed must keep validating. + 'timeoutConfigKey', 'handler', ]); @@ -2371,6 +2374,17 @@ function validateReviewerBodyFields(cap) { ); } + // OPTIONAL, mirroring modelConfigKey's D4 forward/backward-compat treatment (#3274): a manifest + // authored before this field existed must keep validating. Absent/null means "no override for + // this lane, use timeoutFloorMs". An empty string is neither absent nor a key, and is rejected. + if (r.timeoutConfigKey !== undefined && r.timeoutConfigKey !== null && + (typeof r.timeoutConfigKey !== 'string' || r.timeoutConfigKey.length === 0)) { + errors.push( + ctx + ' reviewer.timeoutConfigKey must be a dotted config key or null ' + + '(got: ' + describeValue(r.timeoutConfigKey) + ')', + ); + } + // `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/src/review-lane-descriptor.cts b/src/review-lane-descriptor.cts index 9d04d3f92..25877ee15 100644 --- a/src/review-lane-descriptor.cts +++ b/src/review-lane-descriptor.cts @@ -44,6 +44,9 @@ * 4. `flags: string[]` — Antigravity is selected by BOTH `--antigravity` and * `--agy`, which a single-valued field cannot express. This also flattens * D8's uniqueness invariant across every lane's flags. + * 5. `NATIVE_TIMEOUT` — a lane whose CLI takes its own native inner timeout flag (today only + * antigravity's `--print-timeout`) declares where the resolved value goes; `resolveLanePlan` + * computes what it is from the same resolved outer `timeoutMs` (#3274). * * Phase 2 (#2795) implements the manifest validator against the amended * vocabulary, which is the point of amending rather than leaving it to be @@ -110,7 +113,7 @@ export type LaneProbe = * and vanishes when it has nothing to contribute (no model configured, no effort channel, prompt on * stdin), which is what lets one template serve the configured and unconfigured cases. * - * This is a closed four-member vocabulary with no expressions, no nesting and no conditionals — a + * This is a closed five-member vocabulary with no expressions, no nesting and no conditionals — a * placeholder set, deliberately not a template language. The moment it needs a conditional, the * lane wants a `handler` instead (D6). */ @@ -123,6 +126,9 @@ export const ARGV_PLACEHOLDER = Object.freeze({ OUTPUT: '{{output}}', /** The argv-borne prompt, or nothing unless `promptChannel` is `argv`/`argv-file-ref`. */ PROMPT: '{{prompt}}', + /** A lane's own CLI-native inner timeout duration, derived from the resolved outer `timeoutMs` + * (never independently configured) — see `resolveLanePlan`'s expansion of this token. */ + NATIVE_TIMEOUT: '{{nativeTimeout}}', } as const); export interface SpawnInvoke { @@ -178,6 +184,23 @@ interface ReviewerLaneCommon { probe: LaneProbe; /** Outer wall-clock bound. An inner tool-native timeout lives in the handler (D6). */ timeoutFloorMs: number; + /** + * Dotted config key holding this lane's outer timeout override, in SECONDS, or null when the + * lane accepts none. + * + * Added by #3274 in the same spirit as `promptBudgetKey`/`modelConfigKey`: the frozen + * `timeoutFloorMs` table has no reachable override, and a review duration is a property of the + * user's repository and model, not of the lane — a cap right for a three-plan phase is wrong + * for a fifteen-plan one. Resolved at invocation time (`resolveLanePlan`); an unset key, or a + * 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). + */ + timeoutConfigKey: string | null; emptyOutput: EmptyOutputPolicy; /** The `## Review` heading in write_reviews. Unique (D8). */ reviewsSection: string; @@ -249,6 +272,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ effortChannel: 'none', }, timeoutFloorMs: 900_000, + timeoutConfigKey: 'review.timeouts.gemini', emptyOutput: 'stub-with-stderr', reviewsSection: 'Gemini', evidenceClass: 'source-grounded', @@ -281,6 +305,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ env: { CLAUDE_CODE_DISABLE_CLAUDE_MDS: '1', CLAUDE_CODE_DISABLE_AUTO_MEMORY: '1' }, }, timeoutFloorMs: 1_200_000, + timeoutConfigKey: 'review.timeouts.claude', emptyOutput: 'stub-with-stderr', reviewsSection: 'Claude', evidenceClass: 'source-grounded', @@ -309,6 +334,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ effortChannel: 'argv', }, timeoutFloorMs: 1_200_000, + timeoutConfigKey: 'review.timeouts.codex', emptyOutput: 'stub-with-stderr', reviewsSection: 'Codex', evidenceClass: 'source-grounded', @@ -334,6 +360,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ effortChannel: 'none', }, timeoutFloorMs: 360_000, + timeoutConfigKey: null, emptyOutput: 'stub-with-stderr', reviewsSection: 'CodeRabbit', evidenceClass: 'diff-only', @@ -359,6 +386,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ effortChannel: 'argv', }, timeoutFloorMs: 660_000, + timeoutConfigKey: 'review.timeouts.opencode', emptyOutput: 'stub-with-stderr', reviewsSection: 'OpenCode', evidenceClass: 'source-grounded', @@ -384,6 +412,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ effortChannel: 'none', }, timeoutFloorMs: 900_000, + timeoutConfigKey: null, emptyOutput: 'stub-with-stderr', reviewsSection: 'Qwen', evidenceClass: 'source-grounded', @@ -409,6 +438,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ effortChannel: 'none', }, timeoutFloorMs: 900_000, + timeoutConfigKey: null, emptyOutput: 'stub-with-stderr', reviewsSection: 'Cursor', evidenceClass: 'source-grounded', @@ -428,13 +458,19 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ probe: { kind: 'command-exists', binary: 'agy' }, invoke: { binary: 'agy', - args: ['--print-timeout', '540s', '{{model}}', '-p', '{{prompt}}'], + // `{{nativeTimeout}}` is the fifth ARGV_PLACEHOLDER member (#3274) — `resolveLanePlan` + // (review-lane-invocation.cts) expands it to a value DERIVED from this same lane's resolved + // outer `timeoutMs`, so the native `--print-timeout` and the outer wall-clock cap can never + // drift apart. No other shipped lane's `args` template contains this token, so the expansion + // is inert everywhere else. + args: ['--print-timeout', '{{nativeTimeout}}', '{{model}}', '-p', '{{prompt}}'], promptChannel: 'argv-file-ref', outputChannel: 'stdout', modelArg: '--model', effortChannel: 'none', }, timeoutFloorMs: 600_000, + timeoutConfigKey: 'review.timeouts.antigravity', emptyOutput: 'handler-owned', reviewsSection: 'Antigravity', evidenceClass: 'source-grounded', @@ -465,6 +501,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ effortChannel: 'none', }, timeoutFloorMs: 120_000, + timeoutConfigKey: 'review.timeouts.ollama', emptyOutput: 'stub-with-stderr', reviewsSection: 'Ollama', evidenceClass: 'source-grounded', @@ -494,6 +531,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ effortChannel: 'none', }, timeoutFloorMs: 120_000, + timeoutConfigKey: 'review.timeouts.lm_studio', emptyOutput: 'stub-with-stderr', reviewsSection: 'LM Studio', evidenceClass: 'source-grounded', @@ -521,6 +559,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ effortChannel: 'none', }, timeoutFloorMs: 120_000, + timeoutConfigKey: 'review.timeouts.llama_cpp', emptyOutput: 'stub-with-stderr', reviewsSection: 'llama.cpp', evidenceClass: 'source-grounded', @@ -566,6 +605,7 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ effortChannel: 'none', }, timeoutFloorMs: 900_000, + timeoutConfigKey: 'review.timeouts.kimi-code', emptyOutput: 'stub-with-stderr', reviewsSection: 'Kimi Code', evidenceClass: 'source-grounded', diff --git a/src/review-lane-invocation.cts b/src/review-lane-invocation.cts index 4ddd7f8fa..65c72e67e 100644 --- a/src/review-lane-invocation.cts +++ b/src/review-lane-invocation.cts @@ -254,6 +254,44 @@ export function normalizeHost(raw: string): string { return `${scheme}//${host}${port ? `:${port}` : ''}${pathPart}`; } +/** + * Resolve a lane's outer wall-clock timeout in milliseconds (#3274). + * + * `timeoutConfigKey` resolves in SECONDS — the user-facing convention this repo already uses for + * timeout-shaped config keys (`workflow.cross_ai_timeout`, `graphify.build_timeout`), distinct from + * the internal millisecond unit `timeoutFloorMs` carries. Anything that is not a positive finite + * number is treated as unset and falls back to `floorMs`, never coerced: a wrong-typed config value + * silently becoming a wrong-but-plausible timeout is worse than falling back cleanly. `0` and + * 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. + */ +export function resolveTimeoutMs( + timeoutConfigKey: string | null | undefined, + floorMs: number, + configGet: (key: string) => unknown, +): number { + const configuredSeconds = typeof timeoutConfigKey === 'string' ? configGet(timeoutConfigKey) : undefined; + return typeof configuredSeconds === 'number' && Number.isFinite(configuredSeconds) && configuredSeconds > 0 + ? configuredSeconds * 1000 + : floorMs; +} + +/** Buffer (seconds) a lane's native inner timeout sits under its resolved outer wall-clock cap + * (#3274). Matches the shipped 600s outer / 540s native relationship exactly when unconfigured: + * floor(600000/1000) - 60 = 540. */ +const NATIVE_TIMEOUT_BUFFER_SECONDS = 60; + +/** + * Render the `{{nativeTimeout}}` argv placeholder from a lane's resolved outer timeout (#3274). + * + * Clamped to a 1-second floor so a very small configured (or, today, only-ever-default) outer + * timeout never produces a zero or negative duration string a CLI would reject or misinterpret. + */ +export function nativeTimeoutToken(timeoutMs: number): string { + const seconds = Math.max(1, Math.floor(timeoutMs / 1000) - NATIVE_TIMEOUT_BUFFER_SECONDS); + return `${seconds}s`; +} + /** * Classify a lane's output as a review or as empty. * @@ -362,10 +400,11 @@ export function resolveLanePlan(input: ResolveInput): ResolveResult { } const { promptPath, reviewPath, errPath } = artifactPaths(input.runDir, slug); - const timeoutMs = + const floorMs = typeof lane.timeoutFloorMs === 'number' && Number.isFinite(lane.timeoutFloorMs) && lane.timeoutFloorMs > 0 ? lane.timeoutFloorMs : 900_000; + const timeoutMs = resolveTimeoutMs(lane.timeoutConfigKey, floorMs, input.configGet); const emptyOutput: EmptyOutputPolicy = lane.emptyOutput === 'handler-owned' ? 'handler-owned' : 'stub-with-stderr'; // #3194: only an EXACT 'diff-only' declaration exempts a lane from evidence verification. // Anything else — including a missing or garbage value on a third-party overlay body — @@ -503,6 +542,7 @@ export function resolveLanePlan(input: ResolveInput): ResolveResult { '{{effort}}': effortExpansion, '{{output}}': outputExpansion, '{{prompt}}': promptExpansion, + '{{nativeTimeout}}': [nativeTimeoutToken(timeoutMs)], }; const template = Array.isArray(inv.args) ? inv.args.filter((a): a is string => typeof a === 'string') diff --git a/src/review-lane-runner.cts b/src/review-lane-runner.cts index 5756890e5..491bd89a8 100644 --- a/src/review-lane-runner.cts +++ b/src/review-lane-runner.cts @@ -817,9 +817,13 @@ export function antigravityPrompt(promptPath: string, repoRoot: string): string * lane that fails to start is worse than one that runs on the prompt anchor alone. * 2. The self-report prompt variant above, swapped in for the standard file-ref text. * - * Both are argv shape, so they belong here rather than in the descriptor: expressing "add this flag - * only if the binary's --help mentions it" as data would need a conditional, which is precisely - * what the named-handler seam exists to absorb (ADR-2782 D6). + * Both are argv shape, so they belong here rather than in the descriptor: expressing "add this + * flag only if the binary's --help mentions it" as data would need a conditional, which is + * precisely what the named-handler seam exists to absorb (ADR-2782 D6). The native + * `--print-timeout` VALUE (#3274) is NOT this handler's job — `resolveLanePlan` + * (review-lane-invocation.cts) resolves the `{{nativeTimeout}}` ARGV_PLACEHOLDER itself, exactly + * like `{{model}}`/`{{effort}}`/`{{output}}`/`{{prompt}}`, so `plan.argv` arrives here already fully + * resolved. */ export function antigravityArgv( argv: readonly string[], diff --git a/tests/review-lane-invocation.test.cjs b/tests/review-lane-invocation.test.cjs index 81d4a9979..55e237ac4 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, + resolveTimeoutMs, isEmptyReview, normalizeHost, fileRefPrompt, @@ -85,6 +86,8 @@ const GOLDEN = [ { 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 }, + // 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 }, { slug: 'kimi-code', binary: 'kimi', argv: ['-m', 'K', '-p', FILE_REF], stdin: false, out: 'stdout', timeout: 900000 }, ]; @@ -137,6 +140,130 @@ describe('reviewer lane invocation — golden plans (the strangler-fig contract) }); }); +describe('#3274 — timeoutConfigKey resolves the outer wall-clock cap', () => { + const AGY_FLOOR = REVIEWER_LANES.find((l) => l.slug === 'antigravity').timeoutFloorMs; + const AGY_KEY = REVIEWER_LANES.find((l) => l.slug === 'antigravity').timeoutConfigKey; + + test('an absent config key falls back to timeoutFloorMs (row 1)', () => { + const r = resolve('antigravity', { config: {} }); + assert.equal(r.plan.timeoutMs, AGY_FLOOR); + }); + + test('a configured positive number overrides timeoutFloorMs, seconds -> ms (row 2)', () => { + const r = resolve('antigravity', { config: { [AGY_KEY]: 900 } }); + assert.equal(r.plan.timeoutMs, 900_000); + }); + + test('0 is treated as unset, not a zero-length timeout (row 3)', () => { + const r = resolve('antigravity', { config: { [AGY_KEY]: 0 } }); + assert.equal(r.plan.timeoutMs, AGY_FLOOR); + }); + + test('a negative configured timeout is treated as unset (row 4)', () => { + const r = resolve('antigravity', { config: { [AGY_KEY]: -5 } }); + assert.equal(r.plan.timeoutMs, AGY_FLOOR); + }); + + test('NaN and Infinity are treated as unset (row 5)', () => { + assert.equal(resolve('antigravity', { config: { [AGY_KEY]: NaN } }).plan.timeoutMs, AGY_FLOOR); + assert.equal(resolve('antigravity', { config: { [AGY_KEY]: Infinity } }).plan.timeoutMs, AGY_FLOOR); + }); + + test('a non-number configured value is never coerced, falls back (row 6)', () => { + for (const bad of ['900', true, {}, [], 'null']) { + const r = resolve('antigravity', { config: { [AGY_KEY]: bad } }); + assert.equal(r.plan.timeoutMs, AGY_FLOOR, `value ${JSON.stringify(bad)} must not resolve to a timeout`); + } + }); + + test('a lane with no timeoutConfigKey field falls back like an unset key (row 7)', () => { + const lane = { ...REVIEWER_LANES.find((l) => l.slug === 'gemini') }; + delete lane.timeoutConfigKey; + const r = resolveLanePlan({ lane, configGet: () => 900, runDir: RUN, repoRoot: ROOT }); + assert.equal(r.ok, true); + assert.equal(r.plan.timeoutMs, lane.timeoutFloorMs); + }); + + test('resolveTimeoutMs: direct unit — unset falls back to floorMs', () => { + assert.equal(resolveTimeoutMs(null, 5000, () => undefined), 5000); + assert.equal(resolveTimeoutMs('some.key', 5000, () => undefined), 5000); + }); + + test('resolveTimeoutMs: direct unit — configured value overrides, seconds -> ms', () => { + assert.equal(resolveTimeoutMs('some.key', 5000, (k) => (k === 'some.key' ? 30 : undefined)), 30000); + }); + + test('boundary: 0 vs 1 vs a fractional second (row 8)', () => { + assert.equal(resolve('antigravity', { config: { [AGY_KEY]: 0 } }).plan.timeoutMs, AGY_FLOOR); + assert.equal(resolve('antigravity', { config: { [AGY_KEY]: 1 } }).plan.timeoutMs, 1000); + assert.equal(resolve('antigravity', { config: { [AGY_KEY]: 0.5 } }).plan.timeoutMs, 500); + }); + + test("a non-antigravity lane's configured timeout does not touch argv (row 13)", () => { + const key = REVIEWER_LANES.find((l) => l.slug === 'gemini').timeoutConfigKey; + const unset = resolve('gemini', { config: {} }); + const configured = resolve('gemini', { config: { [key]: 300 } }); + assert.deepStrictEqual(configured.plan.argv, unset.plan.argv); + assert.notEqual(configured.plan.timeoutMs, unset.plan.timeoutMs); + }); + + test('property: any positive-second config resolves to exactly seconds * 1000 ms (row 20)', () => { + fc.assert( + fc.property(fc.integer({ min: 1, max: 1_000_000 }), (seconds) => { + const r = resolve('antigravity', { config: { [AGY_KEY]: seconds } }); + assert.equal(r.plan.timeoutMs, seconds * 1000); + }), + FC, + ); + }); + + test('property: any hostile config value degrades to timeoutFloorMs, never throws (row 21)', () => { + const hostile = fc.oneof( + fc.string(), + fc.boolean(), + fc.object(), + fc.array(fc.anything()), + fc.constant(0), + fc.integer({ max: 0 }), + fc.constant(NaN), + fc.constant(Infinity), + fc.constant(-Infinity), + ); + fc.assert( + fc.property(hostile, (value) => { + const r = resolve('antigravity', { config: { [AGY_KEY]: value } }); + assert.equal(r.ok, true); + assert.equal(r.plan.timeoutMs, AGY_FLOOR); + }), + FC, + ); + }); + + test('a configured antigravity timeout derives both the outer cap and the native flag (row 10)', () => { + const r = resolve('antigravity', { config: { [AGY_KEY]: 900 } }); + assert.equal(r.plan.timeoutMs, 900_000); + const i = r.plan.argv.indexOf('--print-timeout'); + assert.equal(r.plan.argv[i + 1], '840s'); + }); + + test('a native timeout below the 60s buffer clamps to 1s, never 0 or negative (row 11)', () => { + const r = resolve('antigravity', { config: { [AGY_KEY]: 30 } }); + const i = r.plan.argv.indexOf('--print-timeout'); + assert.equal(r.plan.argv[i + 1], '1s'); + }); + + test('boundary around the 60-second native buffer (row 12)', () => { + const nativeFor = (seconds) => { + const r = resolve('antigravity', { config: { [AGY_KEY]: seconds } }); + return r.plan.argv[r.plan.argv.indexOf('--print-timeout') + 1]; + }; + assert.equal(nativeFor(59), '1s'); // floor(59)-60 = -1 -> clamped + assert.equal(nativeFor(60), '1s'); // floor(60)-60 = 0 -> clamped + assert.equal(nativeFor(61), '1s'); // floor(61)-60 = 1 + assert.equal(nativeFor(121), '61s'); // floor(121)-60 = 61 + }); +}); + describe('reviewer lane invocation — model resolution', () => { test("antigravity resolves its model from review.models.agy, not the slug", () => { // The regression this locks: antigravity's slug is `antigravity` but its shipped key is diff --git a/tests/reviewer-manifest-body.test.cjs b/tests/reviewer-manifest-body.test.cjs index db4c74cae..d1dab0182 100644 --- a/tests/reviewer-manifest-body.test.cjs +++ b/tests/reviewer-manifest-body.test.cjs @@ -1178,6 +1178,49 @@ describe('F. Lane scalars', () => { }); }); +// ─── F2. timeoutConfigKey (#3274) — mirrors modelConfigKey's D4 forward/backward-compat shape ── + +describe('F2. timeoutConfigKey (#3274)', () => { + test('reviewer.timeoutConfigKey is optional — absent is valid (row 14)', () => { + const lane = laneOverride(() => {}); + assert.equal(lane.timeoutConfigKey, undefined, 'fixture must not declare timeoutConfigKey'); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('reviewer.timeoutConfigKey: null is valid (row 15)', () => { + const lane = laneOverride((l) => { l.timeoutConfigKey = null; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('reviewer.timeoutConfigKey: a dotted string is valid (row 16)', () => { + const lane = laneOverride((l) => { l.timeoutConfigKey = 'review.timeouts.acme'; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`); + }); + + test('reviewer.timeoutConfigKey: empty string is rejected (row 17)', () => { + const lane = laneOverride((l) => { l.timeoutConfigKey = ''; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.timeoutConfigKey must be a dotted config key or null') && e.includes('(got: "")')), + `expected an empty-string-is-not-none error, got: ${JSON.stringify(errs)}`, + ); + }); + + test('reviewer.timeoutConfigKey: non-string, non-null is rejected (row 18)', () => { + for (const bad of [42, {}, []]) { + const lane = laneOverride((l) => { l.timeoutConfigKey = bad; }); + const errs = validateReviewerBody({ id: 'x', reviewer: lane }); + assert.ok( + errs.some((e) => e.includes('reviewer.timeoutConfigKey must be a dotted config key or null')), + `timeoutConfigKey=${JSON.stringify(bad)}: expected a type-rejection error, got: ${JSON.stringify(errs)}`, + ); + } + }); +}); + // ─── G. Uniqueness (D8) — validateCrossCapability(Map, Set) ──────────────── describe('G. Uniqueness (D8) — validateCrossCapability(Map, Set)', () => {