* feat(#3274): make reviewer-lane timeout configurable via timeoutConfigKey Add an optional `timeoutConfigKey` field to the reviewer lane descriptor, resolved in `resolveLanePlan` at invocation time and falling back to the frozen `timeoutFloorMs` when unset or invalid, in the same spirit as the existing `promptBudgetKey`/`modelConfigKey` fields. All 12 shipped lanes declare `review.timeouts.<slug>` on both surfaces (the descriptor and their capability.json manifest), validated by capability-validator.cjs. For the antigravity lane, the native `agy --print-timeout` flag — previously a second hardcoded literal (`540s`) independent of the outer cap — is now derived from the same resolved outer timeout in `antigravityArgv`, preserving the existing 60-second buffer relationship (ADR-2782 D6: the outer bound is declared data, the inner one is handler-owned). The antigravity default timeoutFloorMs stays at 600s per the maintainer's disposition; users raise it through the new config key instead. * docs(#3274): document review.timeouts.* and extract resolveTimeoutMs helper Address code-review findings on the timeoutConfigKey change: extract the inline timeout-resolution logic into a named, exported, directly-tested resolveTimeoutMs helper (matching the file's existing configString/ normalizeHost convention); document the new review.timeouts.* federated config keys in docs/CONFIGURATION.md, docs/reference/capability-manifest.md, and docs/how-to/ship-a-reviewer-lane.md; add the changeset fragment. * fix(#3274): resolve native antigravity timeout in resolveLanePlan, not the runner gsd-test caught two design mistakes in the prior commits: 1. SpawnPlan.argv is documented and tested as fully resolved by resolveLanePlan (model/effort/output/prompt already folded in) — leaving the antigravity '{{nativeTimeout}}' marker unresolved until the runner's antigravityArgv violated that contract and broke tests that read plan.argv directly (tests/antigravity-reviewer.test.cjs, tests/review-default-reviewers-workflow.test.cjs). Fix: '{{nativeTimeout}}' is now a fifth ARGV_PLACEHOLDER member, resolved by resolveLanePlan itself via the new nativeTimeoutToken() helper, exactly like the other four. antigravityArgv reverts to its pre-#3274 four-argument form. Also missed updating capabilities/antigravity/capability.json's invoke.args to match the descriptor, which broke the manifest/descriptor parity test. 2. tests/reviewer-config-federation.test.cjs enforces a deliberate, narrow invariant (#3691 narrows #2797): qwen, cursor, and coderabbit — the three lanes with neither a model flag nor a host — may own no config key beyond their own prompt-budget key. Adding review.timeouts.<slug> to all 12 lanes violated it. Fix: those three keep timeoutConfigKey: null and own no review.timeouts.* key, matching their existing modelConfigKey: null. The other 9 lanes are unaffected. * chore(#3274): backfill changeset PR number (pr:0 -> 4083) --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
@@ -276,6 +276,22 @@ The same applies to `review.max_prompt_tokens_per_reviewer.<slug>`. `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.<slug>` 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.
|
||||
|
||||
@@ -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`:
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user