4f487e4e75276ad50fc13796cef8d23148cd8830
11 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
github-actions[bot]
|
b3906c66f6 | chore: sync next package version to 1.13.0 | ||
|
Behruz Nassre Esfahani
|
5ad9a36f35 |
fix(#4255): resolve reviewer-lane effort from the lane, not from gsd-plan-checker (#4275)
`review-lane plan` resolved every cross-AI reviewer lane's reasoning effort by spawning `query resolve-execution gsd-plan-checker --host <slug>`. 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. Every prompt-fed lane 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 its stub read as a crash. Effort is a property of the review, so the lane declares it. Two new fields on ReviewerLane — `effortConfigKey` (`review.effort.<slug>`) and `defaultEffort` — carried through each capability manifest and the generated registry, set on the three lanes with an argv effort channel and null on the other nine. A new pure `resolveLaneEffort()` resolves config key -> lane default -> nothing, where "nothing" emits no effort argument at all and the reviewer CLI's own configuration decides; `inherit` selects that path explicitly and an unrecognized level falls back to the lane default rather than being forwarded to a CLI that would reject it. The host's negotiated effortSurface still gates the rendering, so ADR-1239/#2481's trust boundary holds on this path too. Resolving in-process also removes up to twelve subprocess spawns per review. The empty-output stub now names the effort the lane ran at and distinguishes a clean exit from a timeout kill, a non-zero exit, and a process that never ran — `status` is null for both a timeout and a signal, so those were indistinguishable before. The hint is hedged: a clean empty exit is most often a model stopping short, but it is also consistent with a CLI writing its output elsewhere. Also: the capability validator now knows both fields, rejects a malformed key or an out-of-vocabulary default, and rejects a default declared without a config key (a level the operator could never override). An existing end-to-end row in tests/effort-surface-axis.test.cjs asserted the old coupling; it now configures the lane's own key and pins the decoupling in the same real spawn, with the agent execution tier set to a level that must not appear. Emitted-Drift-Ack-Growth: review.md — the effort/model resolution-order table this fix adds. The workflow is where an operator looks to find out which knob set a lane's model and effort; leaving the new key undocumented there is the same invisibility that made the plan-checker coupling survive this long. Emitted-Drift-Ack-Growth: review.md — the effort/model resolution-order table this fix adds. The workflow is where an operator looks to find out which knob set a lane's model and effort, so leaving the new key undocumented there is the same invisibility that let the plan-checker coupling survive. Claude-Session: https://claude.ai/code/session_01CRMEuzNMWn3gs5uUW2ghcF Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
62b0d939b6 |
feat(#3274): make reviewer-lane timeout configurable via timeoutConfigKey (#4083)
* 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> |
||
|
github-actions[bot]
|
4a705cf19f | chore: sync next package version to 1.12.0 | ||
|
Tom Boucher
|
aaf47c5fc2 |
fix(#3691): let every reviewer lane take a prompt cap, and make the documented global resolve (#3832)
* test(#3691): failing-first coverage for the reviewer prompt budget No prompt cap can reach any CLI reviewer lane, by any configuration. Two independent defects compound: all nine `transport: spawn` lanes declare `promptBudgetKey: null`, so `budgetFor` returns on its first line; and the documented global `review.max_prompt_tokens` is advertised in the schema manifest but declared nowhere, so the resolver never materializes it and `budgetFor`'s fallback is dead code. Adds to tests/reviewer-config-federation.test.cjs, which already owns the per-reviewer budget config-set/config-get idiom: - a CLI lane inherits the global cap (RED: reports null) - an http lane with the -1 sentinel inherits the global cap (RED: reports null) - the resolved review surface carries max_prompt_tokens at all (RED: absent) - per-lane overrides the global on a CLI lane - the sentinel boundary: -1 inherits, 0 means do-not-trim and must NOT read as unset, 1 is the smallest real budget — the regression budgetFor's own comment warns about - anti-tightening pins that must stay green: an empty config leaves every lane null, the three existing budgeted lanes are unchanged, and config-set still rejects a per-reviewer key naming something that is not a declared lane - a fast-check property over the resolution contract itself, with -1, 0 and non-finite inputs generated explicitly rather than left to chance Every row was reproduced by hand against the real CLI before being written, so the RED/GREEN split is observed rather than predicted. Refs #3691 * fix(#3691): let every reviewer lane take a prompt cap, and make the global resolve No prompt cap could reach any CLI reviewer lane, by any configuration. Two independent defects compounded. The nine spawn-transport lanes — claude, coderabbit, antigravity, cursor, gemini, codex, kimi-code, opencode, qwen — declared `promptBudgetKey: null`, so `budgetFor` returned on its first line and `review-lane plan` reported `promptBudget: null` no matter what was configured. Each now declares `review.max_prompt_tokens_per_reviewer.<slug>` with the same `-1`-is-unset sentinel the three local-server lanes already use. Separately, the central `review.max_prompt_tokens` was listed in the schema manifest's validKeys and documented as a supported setting, but declared nowhere — the resolved surface is built from capability declarations plus the defaults manifest, and neither carried it. `configGet` returned undefined and `budgetFor`'s documented fallback was dead code. It is now declared with a `null` default, exactly as docs/CONFIGURATION.md already specified, so the default behavior is unchanged: nothing configured means nothing trims. Two things the diagnosis had not predicted, found and fixed while implementing: - `REVIEWER_LANES` in src/review-lane-descriptor.cts is a second, hardcoded registration site that `mergeReviewerLanes` prefers over the capability registry on a slug collision. Editing only the capability files left every CLI lane still null. Both sites now agree. - The generated `gsd-core/bin/lib/capability-registry.cjs` was stale and masked the capability edits; regenerated with `npm run gen:capability-registry` rather than hand-edited. docs/CONFIGURATION.md said "Only lanes that declare a budget key accept one — today ollama, lm_studio and llama_cpp". That is false as of this change and is corrected rather than left to rot. The trim-versus-refuse question the issue raises is deliberately not taken up here: the refusal path already exists for the case that matters — a reviewer whose minimum set exceeds its budget is skipped rather than sent a misleading prompt — and trimming above that floor is the documented, shipped design of the feature. Changing it would alter behavior for the three lanes that already work, which is not what the issue asks for. Fixes #3691 * fix(#3691): document the new global and narrow an invariant this change obsoleted The full suite surfaced two consequences of giving every CLI lane a budget key. `review.max_prompt_tokens` entered CONFIG_DEFAULTS without a matching entry in the planning-config reference, which config-field-docs guards. Documented, including the sentinel semantics a reader needs: a per-lane value overrides the global, `-1` means unset and inherits it, and `0` means "do not trim that lane" and is not unset. The #2797 federation guard asserted that "a lane with no model flag and no host owns no config keys". That held only because budget keys existed solely on the three local-server lanes, all of which have hosts. A lane can now legitimately own a config key for a third reason, so qwen tripped it. The assertion is narrowed rather than weakened: such a lane must still own no model key and no host key, and may own at most its own `review.max_prompt_tokens_per_reviewer.<slug>` — never another lane's. That is strictly more specific in the dimensions that still matter. Proven to still bite: hypothetically giving qwen a `review.models.qwen` key fails it with `model/host: review.models.qwen`. The name and comment cite #3691 for why the premise changed, so a reader sees a deliberate narrowing, not erosion. Checked the sibling assertions in that describe block; the other three do not rest on the obsolete premise and are untouched. Refs #3691 * fix(#3685): port the write-flag content-change contract to its three sibling sites #3685 fixed `phase complete`'s `roadmap_updated` / `state_updated`, which reported `fs.existsSync(path)` rather than whether the transaction wrote anything. Three sibling sites carried the identical defect and are ported here. - `cmdPhaseRemove` reported `roadmap_updated: true`, hardcoded. `updateRoadmapAfterPhaseRemoval` now returns whether the content changed and the flag reports it. #2640/#2974 already fixed `state_updated` at this same call site and left this one behind, so the correct shape was adjacent. - `cmdMilestoneComplete` reported `state_updated: fs.existsSync(statePath)` — byte-identical to #3685's bug in a different command. - `cmdMilestoneComplete` reported `milestones_updated: true`, hardcoded, never consulting the MILESTONES.md write. `gsd-core/workflows/remove-phase.md:100` extracts `roadmap_updated` for display and never branches on it, so the flip from always-true to content-based changes no workflow behavior. Verified by reading the step, not assumed. One trap found while implementing: the obvious in-memory `finalContent !== originalStateContent` comparison — copying `cmdPhaseComplete`'s shipped shape verbatim — gives a FALSE POSITIVE for milestone completion. `platformWriteSync` normalizes Markdown at write time, and the milestone-closure transform regenerates `## Current Position` fresh on every call, so its pre-normalize output always differs from the already-normalized file on disk even when the persisted bytes are identical. The comparison is therefore made against the post-write on-disk content. `cmdPhaseComplete`'s own comparisons are left untouched — their repeat-no-op tests pass, so they are not exposed to this artifact. `milestones_updated` has no reachable no-op: the MILESTONES.md write unconditionally appends an entry every call. Only the true direction is pinned, documented inline rather than faked with a passing test. Refs #3685 * fix(#3685): compare write-flag content through the writer's own normalizer An independent reviewer disproved a claim made while porting #3685's contract to its sibling sites: that `cmdPhaseComplete`'s comparisons were not exposed to the Markdown-normalization artifact already diagnosed in `cmdMilestoneComplete`. `platformWriteSync` normalizes on write — CRLF stripped, blank-line runs collapsed, a blank line inserted after a heading, a single trailing newline enforced. Every flag that compares the PRE-normalization in-memory string against the on-disk pre-image can therefore report a change when the persisted bytes are identical. `cmdMilestoneComplete` had been worked around by re-reading the file after the write; the other sites compared raw strings. All of them now go through one exported seam, `contentChangedAfterNormalize(filePath, before, after)`, which normalizes both sides exactly as the writer does. That removes the extra disk read the milestone workaround needed, and makes the sites agree by construction rather than by four independent implementations of one rule — the divergence the repo names as an anti-pattern. Reachability, stated precisely rather than uniformly: the seam is load-bearing at `cmdPhaseComplete`'s `roadmapUpdated`, `requirementsUpdated` and `stateUpdated`, where section-rewrite logic genuinely regenerates content into a different-but-normalization-equivalent shape. At `updateRoadmapAfterPhaseRemoval` it is defense-in-depth: the no-match branch never reassigns `content`, so the raw comparison was already correct there. The first analysis claimed the reverse; this is the corrected finding. Also fixes an unsound test premise the remote suite caught. The byte-identity precondition in `roadmap_updated is false when ROADMAP.md comes out byte-identical` asserted against a hand-authored, un-normalized fixture — so the very first write reformatted it and the file could not come back identical. The fixture is now written already-normalized, so the assertion compares a normalized pre-image against a normalized post-image and still fails if the flag regresses to a hardcoded `true`. Not platform-specific; it reproduces on macOS too, and the earlier local check simply never exercised it. The sibling true-direction and milestone tests were checked for the same premise and do not share it — they assert `notEqual`, or compare two post-write states produced through the same normalizing seam. Refs #3685 * chore(changeset): backfill PR number for #3691 fragment --------- Co-authored-by: sim <sim@local> |
||
|
github-actions[bot]
|
97ca69a53e | chore: sync next package version to 1.11.0 | ||
|
github-actions[bot]
|
4f2a4932ce | chore: sync next package version to 1.10.0 | ||
|
github-actions[bot]
|
854c93533c | chore: sync next package version to 1.9.1 | ||
|
github-actions[bot]
|
4232a79396 | chore: sync next package version to 1.9.0 | ||
|
Tom Boucher
|
3f6b063fbb |
chore(#2799): invoke_reviewers and write_reviews iterate declared lanes (#2861)
* chore(#2799): resolve reviewer lanes into executable invocation plans Phase 5b of ADR-2782. Adds the resolver and runner that let invoke_reviewers iterate declared lanes instead of hand-authored per-CLI bash. Five additive descriptor amendments, each forced by a lane that ships today: - LaneHandler gains 'opencode' — the lane rebuilds its review from assistant text parts of a --format json stream; a plain stdout copy re-breaks #1936. - modelConfigKey — antigravity's key is review.models.agy, not .antigravity, so resolving by slug silently dropped a configured model. - defaultHost/fallbackModel — Phase 4 federated every *_host with a default of empty string; the real fallback only existed in the bash. - args becomes an argv template with a closed four-placeholder vocabulary. Positional splicing produced 'codex --model M -o F exec --ephemeral', which is not a valid invocation: codex injects in the middle, twice. - kimi-code lane, with the bounded command-capability probe (needle --output-format) that tells Kimi Code from the legacy python kimi-cli. Parity gate re-pointed: the workflow-text families it scanned are the text this phase deletes, so they are replaced by descriptor-to-registry parity plus an anti-parity check that no bespoke leg returns. jq, curl and external timeout/gtimeout all drop out of the review path. Refs #2782 * chore(#2799): add review-lane query surface and widen the manifest vocabulary Adds the gsd-tools 'review-lane' route (plan/invoke/sections) the workflow loops over, projects all twelve lanes into their capability manifests, and widens capability-validator for the amendments. opencode admitted to VALID_LANE_HANDLERS under the second arm of the enum's own admission rule: one lane, justified by a documented upstream defect data cannot express (#1936 — the agent can end its turn with zero output tokens and --format default then drops the assistant text entirely). Two bugs caught by an end-to-end stub run and fixed here: - loadConfigResolved returns a provenance wrapper, not the config; using it directly resolved every key to undefined, which reads as 'nothing configured' and silently dropped every model override. - hasBinary used shell:true with an args array (Node 26 DEP0190). Replaced with a PATH scan that spawns nothing at all. Refs #2782 * chore(#2799): iterate declared lanes in invoke_reviewers and write_reviews Replaces the eleven hand-authored per-CLI bash legs with a loop over resolved lanes, and renders REVIEWS.md sections from each lane's declared reviewsSection instead of thirteen hardcoded headings. review.md drops from 1104 lines to 507 (61KB to 28.7KB). Parity gate re-pointed, as agreed: the leg-marker and section-heading families scanned exactly the text this phase deletes, so they are replaced by descriptor-to-registry parity in both directions, plus an anti-parity check that fires if a bespoke leg is ever re-added. Enum, emitting sites and the Object.keys lock moved together. The budget-trim helper is hoisted out of the Ollama leg: it was always lane-agnostic, and any lane may now declare a promptBudgetKey. Refs #2782 * feat(#2799): bind the consented egress host and re-verify it at invocation Completes ADR-2782 D5. Rule 1 was recorded in the ADR as delivered by Phase 3 but was not implemented: ConsentRecord had no host field and nothing in the tree bound one, so this phase's rule-4 comparison had no baseline. ConsentRecord gains an OPTIONAL reviewerHost. Optional is the whole design: isValidConsentRecord does not require it, so every record already on disk stays valid and no re-consent storm fires (D4 rule 5). It is deliberately excluded from disclosureSignature — the loader has no config resolver, so folding a config-derived value in would make loader and lifecycle compute different signatures for the same manifest and re-prompt forever. Install resolves hostConfigKey (falling back to the lane's declared defaultHost, which is what the invocation path uses) and records it. Invocation re-resolves and blocks on mismatch rather than silently redirecting. Absence allows: no record, or a record predating the field, means nothing to compare — denying there would break every existing local-model user on upgrade. Refs #2782 * test(#2799): cover the resolver, runner and handlers; retarget the parity suites Adds the golden invocation-plan table (one row per shipped lane, derived from the bash legs rather than the descriptor types) plus runner coverage for the probe, empty-output policy, the three handlers and the egress check. Retargets the existing suites onto the new contract: descriptor-to-registry parity, the anti-parity check, the opencode handler, and the twelfth lane. Two corrections found by running them: - modelConfigKey was required; that breaks D4 rule 2, since a reviewer manifest authored before this phase would fail validation on upgrade. It is optional, read as null when absent. - the antigravity non-zero-exit test pre-seeded the transcript, which asserted that a STALE entry leaks through — the exact bug the watermark prevents. The spawn now appends, as the real tool does. Refs #2782 * fix(#2799): restore agy --add-dir and the self-report prompt in the handler Retargeting the three legacy reviewer suites off the deleted bash surfaced two real regressions in the port, both #2176: - --add-dir was dropped. Without it agy's permission context never receives the cwd repo, so the agent anchors on its own scratch dir and reviews the plan text in isolation — the exact failure the Review Instructions forbid. It is capability-probed, because an older agy rejects the unknown flag outright and a lane that fails to start is worse than one running on the prompt anchor. - the prompt lost the clause mandating a REVIEWED-WITHOUT-REPO-ACCESS self-report, which is what makes a blind review distinguishable from a grounded one. antigravity now builds its own prompt variant. Also ports the #2073 mode-2 cli.log diagnostic, which was dropped: a pinned model that 404s exits 0 with empty stdout AND an empty transcript, so agy's own log is the only evidence that anything failed. The three suites now assert against the plan and the handler instead of matching fence text, so they no longer need allow-test-rule exemptions. Refs #2782 * docs(#2799): document the declared lanes, the new flag, and dropped prerequisites COMMANDS.md gains --kimi-code and replaces the jq-prerequisite paragraph, which is now false: no lane requires jq, curl or an external timeout. Adds the changed-egress-destination behavior, since a blocked lane is something a user can hit. CONFIGURATION.md records that the model config key is declared per lane rather than derived from the flag — antigravity's is review.models.agy — and adds review.models.kimi-code. reviewer-instances.md now routes an instance through its lane's single invocation seam instead of a copied per-adapter bash block, which is what lets a cross-cutting fix reach instances for free. That required implementing the --model/--agent/--as flags it documents; --model re-resolves through the lane's argv template rather than splicing, so the flag lands where the lane declares it rather than ahead of a subcommand. CONTEXT.md glossary gains both new modules. Refs #2782 * chore(#2799): drop the stale emitted-drift acknowledgment The only entry was #2797's, acknowledging COMMENT-ONLY GROWTH in review.md. That file now shrinks by ~32KB and every emitted hash that moved is attributable to this diff, so the ack no longer explains anything. Removing the last entry means removing the file: its presence is the alarm, and an empty one signals nothing. Verified by deleting it and re-running the attribution and provenance gates plus lint:ci — all green without it. Refs #2782 * docs(#2799): record the Phase 5b vocabulary widenings in ADR-2782 Five additive amendments, each forced by a lane that ships today, plus two corrections the phase had to make rather than work around: - D5 rule 1 was recorded as delivered by Phase 3 and was not implemented, so this phase's rule-4 comparison had no baseline. Recorded because an ADR asserting a rule was delivered is exactly what stops a later phase checking. - The DEFECT.GENERATIVE-FIX gate is re-pointed: its workflow-text families scanned the text this phase deletes. Also records that D7's 'skip the probe where no bounding mechanism exists' carve-out is obsolete — in practice it meant the Antigravity lane ran unbounded on every stock macOS host, which ships neither timeout nor gtimeout. Refs #2782 * fix(#2799): close four defects found by adversarial review Two confirmed bugs, both reproduced before fixing: - resolveLanePlan was not total. An openai-http lane with a missing or non-object invoke dereferenced inv.hostConfigKey and threw, contradicting the module's own documented contract; the spawn branch guarded correctly and the http branch did not. The CLI seam resolves every selected lane in one map, so one malformed overlay manifest would have aborted the whole review rather than dropping its own lane. Guarded, plus a per-lane try/catch at the seam so a throw can never take down siblings. - A reviewer-instance model was silently dropped for any lane declaring modelConfigKey null (cursor, qwen, coderabbit). reviewer_instances validates that cli is a known slug but never that the slug accepts a model, so a user could configure one, get a clean run, and never learn a different model reviewed their plan. Now warns explicitly. Two hardening fixes: - The slug is concatenated into artifact paths, so LANE_SLUG_RE is enforced in the resolver rather than inherited from a validator that does not run on this path — the module documents itself as the overlay-manifest trust boundary, so it should not depend on someone else having checked. - normalizeHost mangled a scheme-less value: new URL('localhost:11434') parses with an empty hostname, so it became 'localhost://11434' and was compared and requested as if real. An empty hostname now means not-a-URL. Also documents the one gap that cannot be closed here: the antigravity watermark is keyed by workspace, so two concurrent reviews of the same repo share a transcript. agy exposes no per-invocation id to filter on, so the handler now states which half of its never-stale guarantee actually holds. Refs #2782 * test(#2799): retarget the remaining eight review.md-asserting suites The remote runner found 37 failures the local sweep missed (it hit the shell's two-minute cap before reaching these). All eight extract per-CLI bash from review.md that this phase deletes; each protects a real invariant, so each is retargeted onto the plan, the runner or the handler rather than removed. Three real defects surfaced by doing so: - effort args never reached ANY lane. model-resolver.cjs exports no resolveExecution, so effortFor silently returned [] every time. Restored by calling the same bounded resolve-execution query the bash legs used — and NOT with --raw, which prints the resolved effort rather than the picked field, so claude got 'low' instead of '--effort low'. - the timeout guidance lost 'a silent empty output is a timeout kill, not a crash' — the operator note that exists because of the Codex 0xc0000142 misdiagnosis. Restored. - the opencode handler dropped EMPTY assistant text parts. The shipped jq was , and only substitutes for false/null — an empty string is truthy in jq and contributed a blank line. Found by a property test shrinking to ['', '']. The opencode property suite no longer spawns jq at all, which deletes the #2099 hang mechanism it was architected around rather than mitigating it. Refs #2782 * fix(#2799): register the two new generated modules, and untrack them The remote runner caught build output committed to git. Both new modules compile from src/*.cts into gsd-core/bin/lib/*.cjs, and every sibling generated that way is gitignored and eslint-ignored (ADR-457) - including Phase 1's own review-lane-descriptor.cjs. Mine were neither, so repo-invariants' "each bin/lib/*.cjs is linted xor ignored according to migration state" failed. Registered both in .gitignore and eslint.config.mjs alongside the Phase 1 module, and dropped them from the index. Nothing about the shipped behaviour changes; the artifacts are rebuilt by build:lib. This is the new-.cts-module registration ripple, and it is the one part of it I had not completed - the CONTEXT.md glossary and the inventory manifest were already done. Refs #2782 * chore(#2799): backfill changeset pr number to 2861 * chore(#2799): backfill changeset pr number to 2861 --------- Co-authored-by: Test <test@example.com> |
||
|
Tom Boucher
|
6a9babda69 |
chore(#2798): declare the eleven reviewer lanes as manifest data (#2837)
* chore(#2798): declare the eleven reviewer lanes as manifest data Phase 5a of epic #2782, delivering ADR-2782 D9 (roster half) and D3. - Five reviewers GSD never installs into become lane-only role:reviewer capabilities with no runtime body, no runtimeCompat and no install surface: gemini, coderabbit, ollama, lm-studio, llama-cpp. Before this they had no descriptor at all and lived as a hardcoded NON_RUNTIME_REVIEWER_SLUGS tail, which is now deleted outright. - The six hosts that are ALSO reviewers gain a reviewer body alongside their runtime body. Their runtime bodies are byte-identical to next -- verified per capability against the git blob, not asserted -- so no install behaviour moves. - KNOWN_REVIEWER_SLUGS derives from declared bodies via an exported deriveReviewerSlugs(registry). hostBehaviors.reviewerCli survives as a derived legacy alias for one release; where a capability carries both, the body wins and the slug appears once. Alias removal is Phase 7 (#2801). THE KEYSTONE: the roster is the SAME ELEVEN SLUGS as before -- antigravity, claude, coderabbit, codex, cursor, gemini, llama_cpp, lm_studio, ollama, opencode, qwen. This phase changes HOW the roster is derived, not WHO is in it, and the test asserts that literal list rather than a count. kimi-code is deliberately NOT declared here. It is net-new with no invoke_reviewers leg, so declaring it now would make it selectable but not invocable -- present in --all, selected, emitting an empty section for the whole 5a-to-5b window -- and would break Phase 1's parity assertion. It lands in 5b alongside the iteration that can run it. Legacy kimi (the Python CLI) is not a reviewer at all and gains nothing. The highest-value test is declaredManifestLanesMatchThePhase1Descriptor: it deep-compares all eleven declared bodies against REVIEWER_LANES field-by-field, including probe and invoke sub-fields. All eleven are byte-identical, key order included. The epic's premise is that the manifest and the core descriptor describe the same lane with NO translation layer, and Phase 2's review already caught one divergence that every other test missed. Two ADR corrections folded in, as Phases 1-3 each did: 1. PHASE ORDER. The ADR runs Phase 4 (federated config) before 5a and #2798 claims a dependency on 4. That is inverted and makes Phase 4 unsatisfiable: D9 assigns review.<host>_host to lane capabilities that do not exist until THIS phase creates them, and a federated config slice must live inside capabilities/<id>/capability.json. Real graph: Phase 2 -> 5a -> 4. 2. #2798's INVENTORY acceptance item is vacuous. The inventory catalogs bin/lib/*.cjs modules, not capability directories -- antigravity, opencode and qwen appear zero times in it -- and gen-inventory-manifest --check passes with the five new dirs and no edit. Also corrected a stale line in Phase 2's own ADR amendment: it recorded the slug pattern as /^[a-z][a-z0-9_-]*$/, but Phase 2's security review widened the shipped pattern to /^[a-z0-9][a-z0-9_-]*$/ to match Phase 1's exported LANE_SLUG_RE. The prose had not followed the code. Closes #2798 * fix(#2798): catalogue reviewer capabilities in the generated matrix The capability matrix rendered exactly two tables, feature and runtime, via renderTable(caps, role) filtering on c.role === role. ADR-2782 D3 added a THIRD role, so every role:"reviewer" capability was silently dropped from the first-party catalogue. The drift guard did not catch it, and could not: --check compares generated output against the committed file, and both omitted the five lanes identically, so it reported "up to date" while five shipped capabilities were invisible in the one document that is supposed to list what ships. A guard blind to an entire role is not guarding. This phase is what exposed it -- it ships the first role:"reviewer" capabilities -- so it is fixed here rather than deferred (CLAUDE.md: a defect found while working is fixed in the current change, which overrides one-concern-per-PR). Verified red-before-green: with a lane row deleted from the matrix, --check now exits 1; restored, it exits 0. Before this fix the lanes were absent entirely, so there was nothing for the guard to compare. Phase 6 (#2800) still owns enriching the matrix with lane-specific detail (slug/flag/transport columns) and the locale parity gate. This is the narrower fix: the capabilities APPEAR at all. * fix(#2798): close two hardening gaps and record three limits durably Isolated security review (5 targets, no blockers) reproduced two gaps in the new deriveReviewerSlugs. Both are unreachable through the checked-in registry -- it is generated, JSON-sourced and code-reviewed -- but the function is EXPORTED for reuse and carries no other validation, so it must not depend on its caller. - A whitespace-only slug passed the length>0 test verbatim and occupied a roster entry it could never match. Slugs are now trimmed before the emptiness test. A blank body correctly falls through to the legacy alias rather than DROPPING the lane, which would have been worse than the blank slug. - KNOWN_REVIEWER_SLUGS is computed at require() time, so an uncaught throw there breaks import for EVERY consumer rather than degrading selection. It is now guarded, yielding an empty roster on a malformed registry. That is a visible degradation, not a silent one: under D4 an explicitly requested reviewer that is unavailable is an ERROR, so /gsd:review --claude against an empty roster fails loudly. This also removes an asymmetry -- the sibling capability-trust module documents its collectors as TOTAL and wraps them for exactly this reason. Also records three findings that previously existed ONLY in squash-merged PR bodies, which is not a durable record: - ADR-2782 D5 gains an implementation note explaining why the resolved host is deliberately EXCLUDED from the disclosure signature. Rule 1 says consent binds the resolved host; the loader has no config resolver, so folding it in would make the loader and lifecycle compute different signatures for one manifest and re-prompt forever. The binding is split: signature covers the SHA-pinned manifest fields, the consent record stores the resolved host, and Phase 5b re-resolves at invocation -- which is where rule 4 already puts the check. A reader comparing rule 1 to the code would otherwise conclude it is unimplemented. - CONTEXT.md's capability-trust entry still described THREE executable surfaces. Phase 3 added the fourth and made that false; corrected here, since it is drift this epic introduced rather than Phase 6's new-glossary-term work. - stableJson documents the NaN/Infinity/undefined -> null signature collision and why it is unreachable (JSON grammar has no such literal, so JSON.parse throws first). Reachability rests entirely on the ingest path staying JSON.parse-only, so the note lives where someone would break it. * chore(#2798): backfill changeset pr number to 2837 |