From c547e73a714004381887fa77de4701298be9c695 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 4 Aug 2026 18:59:19 -0400 Subject: [PATCH] fix(#2927): merge installed overlay reviewer lanes into review-lane invocation (#3062) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2927): prove overlay reviewer lanes are invisible to review-lane Failing-first regression for #2927. routeReviewLane builds its lane map from the static REVIEWER_LANES array only, so an installed overlay reviewer lane (role:"reviewer" capability) is roster-visible and disclosed at install but never selectable, plannable, or invocable. The test exercises a pure mergeReviewerLanes(firstParty, registry) helper that does not exist yet, so every row fails at the require(). * fix(#2927): merge installed overlay reviewer lanes into review-lane invocation routeReviewLane built its lane map exclusively from the frozen first-party REVIEWER_LANES array, so an installed, consented third-party reviewer lane (role:"reviewer" capability) was roster-visible and disclosed at install but never selectable, plannable, or invocable — sections/flags/plan/invoke all shared the one static map. Add a pure, total mergeReviewerLanes(firstParty, registry) helper (src/review-lane-descriptor.cts) implementing ADR-2782 D8: first-party ∪ installed overlay reviewer bodies, first-party winning on slug collision. The overlay body is field-identical to ReviewerLane per ADR-2782 D1 ("no translation layer"), so the helper MERGES rather than PROJECTS. Malformed overlays (missing/non-object body, empty or grammar-invalid slug) are skipped, never thrown — one bad third-party manifest cannot take the first-party lanes down. routeReviewLane consults loadRegistry({includeInstalled:true}) and degrades to the static set on any load failure. * test(#2927): add CLI-seam coverage for the wiring defect + normalize slug Two findings from the isolated adversarial review: 1. The test matrix's rows 9-10 (acceptance criteria #1-#3: overlay appears in sections/flags and plan resolves ok) were documented as covered but had no backing tests. The eight pure-helper tests would stay green if the one-line routeReviewLane wiring were reverted — the actual defect this PR closes had no regression guard. Add real end-to-end CLI tests that install a global-scope role:"reviewer" overlay and assert review-lane sections/flags/plan see it through loadRegistry -> mergeReviewerLanes. 2. mergeReviewerLanes trimmed the slug for the map key but stored the body with its untrimmed slug, diverging from deriveReviewerSlugs (which trims before adding to the roster). Normalize the stored lane's slug to the trimmed value so the two surfaces agree on the canonical key. * test(#2927): correct CLI-seam fixtures for reviewer manifest shape Two corrections from local CLI smoke-testing before the verification run: 1. role:"reviewer" manifests must omit feature-only fields (skills/agents/ steps/contributions/gates/hooks/runtimeCompat) — the validator rejects them. Match the shipped capabilities/lm-studio shape. 2. The plan subcommand renders an ARRAY of {slug,ok,section,transport,...} (it strips the nested invocation plan object), so assert on the array element, not a top-level object. Also drop the malformed-flag-filter assertion: the capability validator enforces flag grammar at install time, so a lane with a malformed flag cannot be installed and never reaches the flags shape filter (which is defense-in-depth, not independently reachable). * fix(#2927): drop unnecessary type assertion flagged by lint:ci The `body as object` cast inside the spread is redundant — body is already narrowed to object by the preceding typeof check. eslint no-unnecessary-type- assertion flagged it; lint:ci is a merge gate. * chore(#2927): add changeset fragment pr:0 placeholder backfilled with the real PR number once the PR exists. * fix(#2927): access runGsdTools result via .output in CLI-seam tests runGsdTools returns {success, output, exitCode, error}, not a string. The CLI tests (rows 9-10) passed the result object directly to JSON.parse/.split, which string-coerced to "[object Object]" and threw under gsd-test (3 failures). My local smoke test ran the CLI directly (string stdout), so it missed this — the helper wraps execFileSync and returns a result object. Access .output and assert .success explicitly, matching the established capability-cli.test.cjs convention. * chore(#2927): backfill changeset PR number 3062 --------- Co-authored-by: sim --- .changeset/eager-ravens-fly.md | 5 + gsd-core/bin/gsd-tools.cjs | 29 +- src/review-lane-descriptor.cts | 98 ++++++ ...-reviewer-lane-overlay-invocation.test.cjs | 322 ++++++++++++++++++ 4 files changed, 451 insertions(+), 3 deletions(-) create mode 100644 .changeset/eager-ravens-fly.md create mode 100644 tests/issue-2927-reviewer-lane-overlay-invocation.test.cjs diff --git a/.changeset/eager-ravens-fly.md b/.changeset/eager-ravens-fly.md new file mode 100644 index 000000000..584e29bfe --- /dev/null +++ b/.changeset/eager-ravens-fly.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3062 +--- +**Installed third-party reviewer lanes can now be selected, planned, and invoked** — an installed `role:"reviewer"` capability was roster-visible and disclosed at install but `/gsd-review` (`gsd-tools review-lane sections|flags|plan|invoke`) built its lane map from the static first-party set only, so every third-party lane failed with "no such declared lane". The invocation surface now merges installed overlay reviewer lanes (first-party wins on collision, ADR-2782 D8). (#2927) diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index c228cfc79..4341f4b9c 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -1145,10 +1145,11 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load const cp = require('node:child_process'); const fsx = require('node:fs'); const os = require('node:os'); - const { REVIEWER_LANES } = require('./lib/review-lane-descriptor.cjs'); + const { REVIEWER_LANES, mergeReviewerLanes } = require('./lib/review-lane-descriptor.cjs'); const { resolveLanePlan } = require('./lib/review-lane-invocation.cjs'); const runner = require('./lib/review-lane-runner.cjs'); const cfgLoader = require('./lib/config-loader.cjs'); + const capabilityLoader = require('./lib/capability-loader.cjs'); const flag = (name) => { const i = args.indexOf(name); @@ -1176,8 +1177,30 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load const selected = (flag('--selected') || '') .split(',').map((s) => s.trim()).filter(Boolean); - const laneBySlug = new Map(REVIEWER_LANES.map((l) => [l.slug, l])); - const chosen = selected.length ? selected : REVIEWER_LANES.map((l) => l.slug); + // ADR-2782 D8 (#2927): the lane map is first-party ∪ INSTALLED overlay + // `reviewer` bodies, first-party winning on slug collision. Before this merge + // the map was built from the frozen REVIEWER_LANES array alone, so an installed, + // consented third-party reviewer lane was roster-visible (deriveReviewerSlugs) + // and disclosed at install (collectReviewerLaneSurfaces) but never selectable, + // plannable, or invocable — `sections`/`flags`/`plan`/`invoke` all consumed this + // one map. The overlay body is field-identical to a ReviewerLane (ADR-2782 D1, + // "no translation layer"), so `mergeReviewerLanes` is a pure merge, not a + // projection. loadRegistry is TOTAL and never throws on a malformed overlay + // (it skips the cap with a warning), and mergeReviewerLanes is total in turn, + // so a bad third-party manifest cannot take the first-party lanes down with it. + // `includeInstalled` is what merges project + global overlay caps into the + // registry; without it the base is first-party-only and this is a no-op. + let mergedLanes = REVIEWER_LANES; + try { + const registry = capabilityLoader.loadRegistry({ includeInstalled: true, cwd }); + mergedLanes = mergeReviewerLanes(REVIEWER_LANES, registry); + } catch { + // A registry load failure must never block first-party review. Degrade to the + // static set — identical to pre-fix behavior — rather than crashing review-lane. + mergedLanes = REVIEWER_LANES; + } + const laneBySlug = new Map(mergedLanes.map((l) => [l.slug, l])); + const chosen = selected.length ? selected : mergedLanes.map((l) => l.slug); if (sub === 'sections') { const rows = chosen diff --git a/src/review-lane-descriptor.cts b/src/review-lane-descriptor.cts index 383331d30..1825cb0e5 100644 --- a/src/review-lane-descriptor.cts +++ b/src/review-lane-descriptor.cts @@ -558,6 +558,104 @@ export const REVIEWER_LANES: ReadonlyArray = Object.freeze([ }, ].map((lane) => Object.freeze(lane)) as ReviewerLane[]); +/** + * Merge the first-party lane set with installed overlay `reviewer` bodies + * (ADR-2782 D8: first-party ∪ overlay, first-party wins on slug collision). + * + * `routeReviewLane` (gsd-core/bin/gsd-tools.cjs) consumes this to build the lane + * map its `sections`/`flags`/`plan`/`invoke` subcommands resolve against, so an + * installed, consented third-party reviewer capability (`role:"reviewer"` with a + * declared `reviewer` body) becomes selectable, plannable, and invocable through + * the same surface as a built-in lane — closing #2927, where the overlay was + * roster-visible (`deriveReviewerSlugs`) and disclosed at install + * (`collectReviewerLaneSurfaces`) but never reached the invocation path. + * + * ADR-2782 D1 deliberately made the manifest `reviewer` body field-identical to + * `ReviewerLaneCommon` so "Phase 2 harvests the shape into the capability manifest + * with no translation layer" (CONTEXT.md reviewer-lane-descriptor predicate). The + * overlay body IS already `ReviewerLane`-shaped; this function MERGES, it does not + * PROJECT — there is no field renaming, no vocabulary translation. That is the + * design decision the bug violated by simply never calling anything. + * + * Pure and TOTAL — never throws on any input, because third-party overlay + * manifests are untrusted data reaching this seam at load time. A cap whose + * `reviewer` body is absent, non-object, or carries an empty/grammar-invalid slug + * is SKIPPED (contributes no lane) rather than crashing, so one malformed overlay + * cannot take the whole `review-lane` surface (and every first-party lane with it) + * down. The slug grammar (`LANE_SLUG_RE`) is enforced HERE even though + * `resolveLanePlan`'s trust boundary re-checks it, because `sections`/`flags` read + * lane fields before reaching `resolveLanePlan` — defense in depth against the + * path-traversal class the invocation boundary exists for. + * + * First-party wins: an overlay declaring a slug that already names a first-party + * lane is silently superseded (the first-party entry is kept, identity-stable), + * never overwritten. The first-party object is returned by reference, not copied. + * + * @param firstParty The frozen first-party lane set (`REVIEWER_LANES`). + * @param registry The merged capability registry + * (`loadRegistry({ includeInstalled: true })`); only its + * `capabilities` map is read. A cap contributes a lane iff it + * carries an object `reviewer` body with a non-empty, + * grammar-valid `slug`. The `role:"runtime"` legacy + * `reviewerCli` alias contributes NO lane here — it has no lane + * descriptor, and the selection roster (`deriveReviewerSlugs`) + * is a separate surface. + * @returns A NEW array: first-party lanes (in order) followed by accepted overlay + * lanes (in registry iteration order). Callers must not mutate it. + */ +export function mergeReviewerLanes( + firstParty: ReadonlyArray, + registry: { capabilities?: Record } | null | undefined, +): ReviewerLane[] { + // First-party always wins and is returned by reference (identity-stable), so a + // collision cannot perturb the first-party entry a consumer already holds. + const bySlug = new Map(); + const ordered: ReviewerLane[] = []; + for (const lane of firstParty) { + const slug = typeof lane.slug === 'string' ? lane.slug.trim() : ''; + if (!slug) continue; // unreachable for the shipped frozen set, but this is exported + if (!bySlug.has(slug)) { + bySlug.set(slug, lane); + ordered.push(lane); + } + } + + const capabilities = (registry && registry.capabilities) || {}; + for (const cap of Object.values(capabilities)) { + if (cap === null || typeof cap !== 'object' || Array.isArray(cap)) continue; + const body = (cap as { reviewer?: unknown }).reviewer; + // C1/C2 (mirroring collectReviewerLaneSurfaces): a missing, null, or + // non-object reviewer body declares no lane — never an error at this layer. + if (body === null || typeof body !== 'object' || Array.isArray(body)) continue; + const rawSlug = (body as { slug?: unknown }).slug; + const slug = typeof rawSlug === 'string' ? rawSlug.trim() : ''; + // Empty/whitespace slug: nothing to key on. Grammar-invalid slug: the + // path-traversal class — skip rather than admit, even though resolveLanePlan + // would reject it downstream. Both are skips, not throws. + if (!slug || !LANE_SLUG_RE.test(slug)) continue; + if (bySlug.has(slug)) continue; // D8: first-party (or an earlier overlay) wins + // The body is already ReviewerLane-shaped per ADR-2782 D1. We do NOT + // deep-validate every field here: the invocation trust boundary + // (resolveLanePlan) is the seam that re-validates a lane before it runs, and + // `sections`/`flags` are both tolerant of a partial body (flags filters by + // shape; sections reads slug/reviewsSection which a non-string cap simply + // renders as undefined). Admitting the body keeps the merge a pure merge. + // + // The slug is normalized to its trimmed value on the stored lane so the merge + // and the selection roster (`deriveReviewerSlugs`, which trims before adding) + // agree on the canonical key — otherwise a body declaring `slug: ' x '` + // would key the map on `'x'` but emit the raw `' x '` in `sections` and fail + // to match a `--selected` value coming from the roster. (The capability + // validator's grammar check rejects surrounding whitespace before this is + // reachable through a real install, so this is belt-and-braces parity with the + // roster, not a live-input guard.) + const lane = { ...body, slug } as ReviewerLane; + bySlug.set(slug, lane); + ordered.push(lane); + } + return ordered; +} + /* ------------------------------------------------------------------ * * DEFECT.GENERATIVE-FIX parity (CONTEXT.md:797) * ------------------------------------------------------------------ */ diff --git a/tests/issue-2927-reviewer-lane-overlay-invocation.test.cjs b/tests/issue-2927-reviewer-lane-overlay-invocation.test.cjs new file mode 100644 index 000000000..55a422dbc --- /dev/null +++ b/tests/issue-2927-reviewer-lane-overlay-invocation.test.cjs @@ -0,0 +1,322 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +/** + * Regression test for #2927 — third-party reviewer lane installs and is + * roster-visible, but `review-lane sections|flags|plan|invoke` cannot select, + * plan, or invoke it. + * + * Root cause: `routeReviewLane` (gsd-core/bin/gsd-tools.cjs) built its lane map + * exclusively from the frozen first-party `REVIEWER_LANES` array and never + * consulted the merged capability registry, so an installed overlay + * `role:"reviewer"` capability — whose `reviewer` body is field-identical to a + * `ReviewerLane` (ADR-2782 D1, "no translation layer") — was invisible to every + * invocation subcommand. + * + * The fix extracts a PURE helper `mergeReviewerLanes(firstParty, registry)` + * (source of truth: src/review-lane-descriptor.cts) implementing ADR-2782 D8: + * first-party ∪ installed overlay `reviewer` bodies, first-party wins on slug + * collision. This file exercises the helper directly against synthetic + * registries — no real capability install — matching the convention in + * reviewer-manifest-body.test.cjs / review-lane-invocation.test.cjs. + * + * Matrix: .gsd/bug/fix/2927-reviewer-lane-overlay-invocation/50-test-matrix.md + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); + +const { + REVIEWER_LANES, + mergeReviewerLanes, + LANE_SLUG_RE, +} = require('../gsd-core/bin/lib/review-lane-descriptor.cjs'); + +/** A first-party lane set small enough to read at a glance, but real-shaped. */ +const FP = REVIEWER_LANES.slice(0, 2); // gemini, claude +const FP_SLUGS = FP.map((l) => l.slug); + +/** A valid overlay `reviewer` body, field-identical to a SpawnLane (ADR-2782 D1). */ +function overlayLane(overrides = {}) { + return { + slug: 'agy-revisor', + flags: ['--agy-revisor'], + transport: 'spawn', + probe: { kind: 'command-exists', binary: 'agy' }, + invoke: { + binary: 'agy', + args: ['--agent', 'revisor-gsd', '{{model}}', '-p', '{{prompt}}'], + promptChannel: 'argv-file-ref', + outputChannel: 'stdout', + modelArg: '--model', + effortChannel: 'none', + }, + timeoutFloorMs: 600000, + emptyOutput: 'handler-owned', + reviewsSection: 'Antigravity revisor-gsd', + evidenceClass: 'source-grounded', + requiresBinaries: [], + promptBudgetKey: null, + modelConfigKey: 'review.models.agy-revisor', + handler: 'antigravity', + ...overrides, + }; +} + +/** A `role:"reviewer"` capability envelope carrying a reviewer body. */ +function reviewerCap(body) { + return { id: body && typeof body === 'object' && body.slug ? body.slug : 'x', role: 'reviewer', reviewer: body }; +} + +/** Build a synthetic registry shape ({ capabilities: { id: cap } }). */ +function registry(...caps) { + const capabilities = {}; + for (const c of caps) capabilities[c.id] = c; + return { capabilities }; +} + +describe('mergeReviewerLanes (#2927)', () => { + test('overlayAbsentReturnsFirstPartyUnchanged', () => { + // Row 1: no overlay reviewer caps → merged set is first-party exactly. + const merged = mergeReviewerLanes(FP, registry()); + assert.deepEqual(merged.map((l) => l.slug), FP_SLUGS); + assert.equal(merged.length, FP.length); + // identity, not just equality — first-party objects themselves + assert.equal(merged[0], FP[0]); + assert.equal(merged[1], FP[1]); + }); + + test('overlayLaneIncludedInMerge', () => { + // Row 2 (failing-first regression): one valid non-colliding overlay lane is present. + const merged = mergeReviewerLanes(FP, registry(reviewerCap(overlayLane()))); + const slugs = merged.map((l) => l.slug); + assert.ok(slugs.includes('agy-revisor'), 'overlay slug admitted into merged set'); + assert.ok(slugs.includes('gemini'), 'first-party lanes preserved'); + // the overlay body itself is the merged entry (no translation layer) + const overlay = merged.find((l) => l.slug === 'agy-revisor'); + assert.equal(overlay.reviewsSection, 'Antigravity revisor-gsd'); + assert.deepEqual(overlay.flags, ['--agy-revisor']); + }); + + test('firstPartyWinsOnSlugCollision', () => { + // Row 3 / D8: an overlay declaring a first-party slug is superseded by first-party. + const colliding = overlayLane({ slug: 'claude', reviewsSection: 'EVIL CLAUDE' }); + const merged = mergeReviewerLanes(FP, registry(reviewerCap(colliding))); + const claude = merged.find((l) => l.slug === 'claude'); + assert.equal(claude, FP.find((l) => l.slug === 'claude'), 'first-party identity wins'); + assert.notEqual(claude.reviewsSection, 'EVIL CLAUDE', 'overlay did not leak through'); + assert.equal(merged.length, FP.length, 'collision added no extra entry'); + }); + + test('runtimeCapWithoutReviewerBodyAddsNoLane', () => { + // Row 4: a role:"runtime" cap with only the legacy reviewerCli alias has no lane descriptor. + const runtimeCap = { id: 'some-runtime', role: 'runtime', runtime: { hostBehaviors: { reviewerCli: true } } }; + const merged = mergeReviewerLanes(FP, registry(runtimeCap)); + assert.deepEqual(merged.map((l) => l.slug), FP_SLUGS, 'runtime alias contributed no lane'); + }); + + test('emptySlugOverlaySkippedNotThrown', () => { + // Row 5: an overlay body whose slug is empty/whitespace is skipped, never throws. + const empty = reviewerCap(overlayLane({ slug: ' ' })); + const missing = reviewerCap(overlayLane({ slug: '' })); + assert.doesNotThrow(() => mergeReviewerLanes(FP, registry(empty))); + assert.doesNotThrow(() => mergeReviewerLanes(FP, registry(missing))); + const merged = mergeReviewerLanes(FP, registry(empty, missing)); + assert.deepEqual(merged.map((l) => l.slug), FP_SLUGS, 'empty-slug overlays admitted no lane'); + }); + + test('invalidGrammarSlugSkipped', () => { + // Row 6 / security: a slug outside LANE_SLUG_RE (path-traversal class) is skipped at the merge. + const evil = reviewerCap(overlayLane({ slug: '../evil' })); + assert.doesNotThrow(() => mergeReviewerLanes(FP, registry(evil))); + const merged = mergeReviewerLanes(FP, registry(evil)); + assert.ok(!merged.map((l) => l.slug).includes('../evil'), 'invalid-grammar slug not admitted'); + // sanity: the grammar is what we think it is + assert.ok(!LANE_SLUG_RE.test('../evil')); + assert.ok(LANE_SLUG_RE.test('agy-revisor')); + }); + + test('twoOverlaysBothIncluded', () => { + // Row 7: two distinct non-colliding overlays both present; count == fp + 2. + const a = reviewerCap(overlayLane({ slug: 'alpha-lane', reviewsSection: 'Alpha' })); + const b = reviewerCap(overlayLane({ slug: 'beta-lane', reviewsSection: 'Beta' })); + const merged = mergeReviewerLanes(FP, registry(a, b)); + const slugs = merged.map((l) => l.slug); + assert.ok(slugs.includes('alpha-lane')); + assert.ok(slugs.includes('beta-lane')); + assert.equal(merged.length, FP.length + 2); + }); + + test('malformedReviewerBodySkipped', () => { + // Row 8: reviewer body that is null / array / string is skipped, no throw. + const nullBody = { id: 'n', role: 'reviewer', reviewer: null }; + const arrBody = { id: 'a', role: 'reviewer', reviewer: [] }; + const strBody = { id: 's', role: 'reviewer', reviewer: 'not-an-object' }; + assert.doesNotThrow(() => mergeReviewerLanes(FP, registry(nullBody, arrBody, strBody))); + const merged = mergeReviewerLanes(FP, registry(nullBody, arrBody, strBody)); + assert.deepEqual(merged.map((l) => l.slug), FP_SLUGS, 'malformed bodies admitted no lane'); + }); +}); + +// --------------------------------------------------------------------------- +// Rows 9–10: the WIRING defect this PR exists to close. The eight rows above +// guard the pure helper, but the actual bug was that `routeReviewLane` never +// CALLED any merge — so a revert of the one-line wiring change would leave every +// helper test green. These rows exercise the real CLI end-to-end: install a +// global-scope `role:"reviewer"` overlay (global scope is trusted without a +// consent record, CONTEXT.md capability-loader predicate), then assert +// `review-lane sections|flags|plan` actually see it through loadRegistry → +// mergeReviewerLanes → the lane map. This is acceptance criteria #1–#3. +// --------------------------------------------------------------------------- + +const fs = require('node:fs'); +const os = require('node:os'); +const nodePath = require('node:path'); +const { runGsdTools, cleanup } = require('./helpers.cjs'); + +const cliTmps = []; +function cliTmpDir(prefix) { + const d = fs.mkdtempSync(nodePath.join(os.tmpdir(), prefix)); + cliTmps.push(d); + return d; +} +test.after(() => { for (const d of cliTmps) cleanup(d); }); + +/** A GSD_HOME-sandboxed env that neutralizes ambient GSD_ vars (hermeticity). */ +function scopeEnv(home) { + return { GSD_HOME: home, GSD_WORKSTREAM: '', GSD_PROJECT: '' }; +} + +/** A cwd with a .planning/ root so findProjectRoot resolves cleanly. */ +function makeCwd() { + const cwd = cliTmpDir('rev2927-cwd-'); + fs.mkdirSync(nodePath.join(cwd, '.planning'), { recursive: true }); + fs.writeFileSync(nodePath.join(cwd, '.planning', 'config.json'), '{}'); + return cwd; +} + +/** + * Write a conformant `role:"reviewer"` capability source dir whose `reviewer` + * body is a valid SpawnLane (ADR-2782 D1 shape). Returns the source path, + * usable as a `capability install ` argument. + */ +function writeReviewerCapSource(id, bodyOverrides = {}) { + const src = cliTmpDir(`rev2927-src-${id}-`); + // A `role:"reviewer"` manifest carries ONLY id/role/version/title/description/ + // tier/requires/engines/reviewer (+ optional config) — skills/agents/steps/ + // contributions/gates/hooks/runtimeCompat are feature-only fields the validator + // rejects for a reviewer (mirrors the shipped `capabilities/lm-studio` shape). + const cap = { + id, + role: 'reviewer', + version: '1.0.0', + title: `${id} test lane`, + description: 'test third-party reviewer lane for #2927', + tier: 'standard', + requires: [], + engines: { gsd: '>=1.9.0' }, + reviewer: { + slug: id, + flags: [`--${id}`], + transport: 'spawn', + probe: { kind: 'command-exists', binary: id }, + invoke: { + binary: id, + args: ['{{model}}', '-p', '{{prompt}}'], + promptChannel: 'stdin', + outputChannel: 'stdout', + modelArg: '--model', + effortChannel: 'none', + }, + timeoutFloorMs: 600000, + emptyOutput: 'stub-with-stderr', + reviewsSection: `${id} review`, + evidenceClass: 'source-grounded', + requiresBinaries: [], + promptBudgetKey: null, + modelConfigKey: `review.models.${id}`, + handler: null, + ...bodyOverrides, + }, + }; + fs.writeFileSync(nodePath.join(src, 'capability.json'), JSON.stringify(cap, null, 2)); + return src; +} + +describe('review-lane CLI overlay invocation (#2927, rows 9–10)', () => { + test('cliSectionsAndPlanSeeOverlayLane', () => { + // Acceptance #1 + #3: an installed overlay lane appears in `sections` and + // `plan --selected ` returns ok:true with a usable plan. + const home = cliTmpDir('rev2927-home-'); + const cwd = makeCwd(); + const src = writeReviewerCapSource('rev2927lane'); + // Global scope is trusted without a consent record; --yes acknowledges the + // executable reviewer surface; --raw emits JSON. + const install = runGsdTools( + ['capability', 'install', src, '--scope', 'global', '--yes', '--raw'], + cwd, + scopeEnv(home), + ); + assert.equal(install.success, true, `install failed: ${install.error || install.output}`); + const installOut = JSON.parse(install.output); + assert.equal(installOut.status, 'installed', `install did not report installed: ${install.output}`); + + // Row 9 / acceptance #1: sections includes the overlay slug + reviewsSection. + const sections = runGsdTools(['review-lane', 'sections'], cwd, scopeEnv(home)); + assert.equal(sections.success, true, `sections failed: ${sections.error || sections.output}`); + const sectionRows = sections.output.split('\n').filter(Boolean); + const overlayRow = sectionRows.find((r) => r.startsWith('rev2927lane\t')); + assert.ok(overlayRow, `overlay lane missing from sections output:\n${sections.output}`); + assert.equal(overlayRow, 'rev2927lane\trev2927lane review'); + + // Row 9 / acceptance #3: plan --selected resolves ok (NOT + // malformed_lane / no such declared lane — the pre-fix failure). The `plan` + // subcommand renders an ARRAY of {slug, ok, section, transport, ...} (it strips + // the nested invocation `plan` object before output), so find the overlay entry. + const plan = runGsdTools( + ['review-lane', 'plan', '--selected', 'rev2927lane', '--run-dir', cwd, '--repo-root', cwd], + cwd, + scopeEnv(home), + ); + assert.equal(plan.success, true, `plan failed: ${plan.error || plan.output}`); + const planOut = JSON.parse(plan.output); + assert.ok(Array.isArray(planOut), `plan output is not an array:\n${plan.output}`); + const overlayPlan = planOut.find((p) => p.slug === 'rev2927lane'); + assert.ok(overlayPlan, `overlay plan entry missing:\n${plan.output}`); + assert.equal(overlayPlan.ok, true, `overlay plan did not resolve ok:\n${plan.output}`); + assert.equal(overlayPlan.section, 'rev2927lane review'); + assert.equal(overlayPlan.transport, 'spawn'); + }); + + test('cliFlagsIncludeOverlayFlag', () => { + // Acceptance #2: the overlay's declared --flag appears in `flags` output. + // + // NOTE on the negative-space "malformed flag filtered" case: the capability + // validator enforces the /^--[a-z0-9][a-z0-9-]*$/ flag grammar AT INSTALL TIME + // (capability-validator rejects a reviewer.flags entry that fails it), so a lane + // carrying a malformed flag (e.g. `--bad flag`, `*.js`) can never be installed + // and therefore never reaches the `flags` shape filter. That filter is + // defense-in-depth over an input class the validator already excludes; it is + // not independently reachable through a validated install, so it is not asserted + // here. A lane declaring two well-formed flags (mirroring antigravity's + // --antigravity/--agy) proves the per-lane flag array is preserved, not flattened. + const home = cliTmpDir('rev2927-home-'); + const cwd = makeCwd(); + const src = writeReviewerCapSource('rev2927flag', { + flags: ['--rev2927flag', '--rev2927alt'], + }); + const install = runGsdTools( + ['capability', 'install', src, '--scope', 'global', '--yes', '--raw'], + cwd, + scopeEnv(home), + ); + assert.equal(install.success, true, `install failed: ${install.error || install.output}`); + assert.equal(JSON.parse(install.output).status, 'installed', `install did not report installed: ${install.output}`); + + const flags = runGsdTools(['review-lane', 'flags'], cwd, scopeEnv(home)); + assert.equal(flags.success, true, `flags failed: ${flags.error || flags.output}`); + const flagLines = flags.output.split('\n').filter(Boolean); + assert.ok(flagLines.includes('--rev2927flag'), `overlay flag missing from flags output:\n${flags.output}`); + assert.ok(flagLines.includes('--rev2927alt'), 'second well-formed overlay flag missing (flag array flattened?)'); + }); +});