Files
msd-core/src/review-lane-descriptor.cts
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>
2026-08-24 19:39:51 -04:00

1266 lines
56 KiB
TypeScript
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
/**
* Reviewer Lane Descriptor Module (ADR-2782 Phase 1, #2794 — closes #2690).
*
* ONE place where the cross-AI reviewer lane contract is declared as data.
*
* Before this module the contract lived in three unrelated surfaces: the roster
* in `review-reviewer-selection.cts`, ~640 lines of hand-authored per-CLI bash in
* `gsd-core/workflows/review.md` `invoke_reviewers`, and the hardcoded section
* headings in `write_reviews`. A cross-cutting fix therefore landed per-leg —
* #2494 and #2605 were the same empty-output defect filed twice, and #2475 /
* #2295 / #2272 are the same shape.
*
* SCOPE (ADR-2782's phase table). This module DECLARES; it does not execute.
* `invoke_reviewers` still runs hand-authored legs until Phase 5b (#2799) makes
* it iterate. What Phase 1 buys is that a leg can no longer be added, removed,
* or renamed without the table and the REVIEWS.md section moving with it —
* `checkReviewerLaneParity` below is the `DEFECT.GENERATIVE-FIX` assertion
* (`CONTEXT.md:797`) that the roster has never had.
*
* The descriptor deliberately does NOT promise uniformity. Lane divergence is
* real and frequently correct (measured timeout floors differ; three lanes are
* HTTP endpoints with no binary; Antigravity needs a three-layer fallback for an
* upstream stdout bug). The value is one place where divergence is DECLARED.
* Behaviour that data cannot express is delegated to a named `handler`
* (ADR-2782 D6 closed enum) — never to conditionals inside the table.
*
* Field names, nesting, and enum members track ADR-2782 D1/D2/D6/D7 so Phase 2
* (#2795) harvests this shape into the capability manifest without a translation
* layer — INCLUDING `transport`'s placement at the lane level (see SpawnLane).
*
* Building this table against all eleven shipped legs surfaced four cases the
* ADR's original survey did not cover. Rather than diverge silently — which is
* exactly the translation layer this module exists to avoid — ADR-2782 was
* AMENDED in the same PR (see its Amendments section, 2026-07-29). All four are
* additive widenings of closed enums, each forced by a lane that exists today:
*
* 1. `promptChannel: 'none'` — CodeRabbit reviews the working-tree diff and is
* fed no prompt at all.
* 2. `outputChannel: 'file-arg'` — Codex writes the review via its own
* `-o/--output-last-message <FILE>` and discards stdout (#1698), because on
* Windows it emits teardown noise to stdout after the final message.
* 3. `outputArg` — the companion to (2): knowing the review lands in a file is
* useless without the argument that names the file.
* 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.
*
* Phase 2 (#2795) implements the manifest validator against the amended
* vocabulary, which is the point of amending rather than leaving it to be
* rediscovered.
*/
/** ADR-2782 D2 — closed transport discriminator; selects the invoke sub-shape. */
export type LaneTransport = 'spawn' | 'openai-http';
/** ADR-2782 D2, widened by the CodeRabbit lane (see module header). */
export type PromptChannel = 'stdin' | 'argv' | 'argv-file-ref' | 'none';
/** ADR-2782 D2, widened by the Codex lane (see module header). */
export type OutputChannel = 'stdout' | 'file-arg';
/** ADR-2782 D2 — how reasoning effort reaches the tool. */
export type EffortChannel = 'none' | 'argv' | 'env';
/** ADR-2782 D2 — CodeRabbit reviews a diff, not the source tree (`review.md:367`). */
export type EvidenceClass = 'source-grounded' | 'diff-only';
/**
* ADR-2782 D6 — closed enum of first-party imperative modules. Ported in Phase 5b.
*
* `'opencode'` was added by Phase 5b (#2799) as an additive widening, forced by a lane that ships
* today. Phase 1 declared `opencode.handler = null`, but the lane's review is RECONSTRUCTED from
* assistant `text` parts of a `--format json` stream — `--format json` is the primary invocation,
* not a fallback, because the default formatter drops the text when the agent ends its turn with no
* final message (#1936). A data-driven `outputChannel: 'stdout'` copy would write the raw JSON
* envelope into REVIEWS.md as the review, straight back into #1936.
*
* The alternative — an `outputChannel: 'json-parts'` member — was rejected: it pushes a parsing
* language into the descriptor and would want a selector expression next. D6's whole point is that
* divergence escapes to NAMED FIRST-PARTY CODE rather than accreting inside data.
*/
export type LaneHandler = null | 'antigravity' | 'openai-compatible' | 'opencode';
/**
* What a lane does when it produces no usable output.
* `stub-with-stderr` is the normalized policy (#2494/#2605): write a diagnostic
* stub carrying the captured stderr, so `write_reviews` can tell a failed lane
* apart from a reviewer that ran cleanly with nothing to report.
* `handler-owned` means the lane's handler writes its own diagnostics.
*/
export type EmptyOutputPolicy = 'stub-with-stderr' | 'handler-owned';
/** ADR-2782 D7 — every probe that starts a process or connection MUST be bounded. */
export type LaneProbe =
| { kind: 'command-exists'; binary: string }
| { kind: 'command-capability'; binary: string; needle: string; timeoutMs: number }
| { kind: 'http-reachable'; hostConfigKey: string; path: string; timeoutMs: number };
/**
* Argv placeholders (Phase 5b, #2799).
*
* `args` is an argv TEMPLATE, not a prefix. The injected pieces — model, effort, output file,
* argv-borne prompt — do not all go in the same place, and no positional rule expresses that:
* `codex` injects the model in the MIDDLE (after the `exec --ephemeral` subcommand) and the output
* file later still, while `gemini` injects the model first and five lanes end with a bare `-` that
* must stay last. Splicing by position silently produced
* `codex --model M -o F exec --ephemeral …`, which is not a valid codex invocation.
*
* So each lane declares WHERE each piece goes. A placeholder expands to zero or more argv elements
* 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
* placeholder set, deliberately not a template language. The moment it needs a conditional, the
* lane wants a `handler` instead (D6).
*/
export const ARGV_PLACEHOLDER = Object.freeze({
/** `modelArg` + the resolved model, or nothing. */
MODEL: '{{model}}',
/** The host's effort argv, or nothing unless `effortChannel` is `argv`. */
EFFORT: '{{effort}}',
/** `outputArg` + the review path, or nothing unless `outputChannel` is `file-arg`. */
OUTPUT: '{{output}}',
/** The argv-borne prompt, or nothing unless `promptChannel` is `argv`/`argv-file-ref`. */
PROMPT: '{{prompt}}',
} as const);
export interface SpawnInvoke {
binary: string;
/** Argv template — see `ARGV_PLACEHOLDER`. Order here is the order the tool receives. */
args: ReadonlyArray<string>;
promptChannel: PromptChannel;
outputChannel: OutputChannel;
/** Present only when `outputChannel === 'file-arg'`. */
outputArg?: string;
/** `null` when the lane accepts no model override. */
modelArg: string | null;
effortChannel: EffortChannel;
/**
* Per-invocation environment pairs, merged over the inherited environment at spawn time (#2483).
* Declared data, not a handler (D6): the pairs are static per lane. Scoped to the one spawn —
* the orchestrating session's environment is never mutated. First-party lanes only today: the
* resolver sees exactly `REVIEWER_LANES`, so no third-party manifest can reach this field; if
* manifest lanes are ever wired to execute, `env` must join the trust disclosure
* (`capability-trust`) before it is honored there.
*/
env?: Readonly<Record<string, string>>;
}
export interface HttpInvoke {
/** Dotted config key holding the base URL. */
hostConfigKey: string;
/**
* Base URL used when `hostConfigKey` resolves to empty.
*
* Added by Phase 5b (#2799). Phase 4 federated every `*_host` key with a default of `""`, so the
* REAL fallback (`http://localhost:11434` and friends) only ever existed inside the bash leg. A
* data-driven lane with an empty host and no declared default would POST to a garbage URL, so
* the fallback has to be declared somewhere — and the lane is the only thing that knows it.
*/
defaultHost: string;
path: string;
modelDiscovery: 'none' | 'first-from-models-endpoint';
/** Model used when neither config nor discovery yields one. */
fallbackModel: string;
effortChannel: 'none';
}
export type LaneInvoke = SpawnInvoke | HttpInvoke;
interface ReviewerLaneCommon {
slug: string;
/**
* Every CLI flag that selects this lane. First entry is canonical.
* ADR-2782 D1 shows a singular `flag`; see the module header's widening (3).
*/
flags: ReadonlyArray<string>;
probe: LaneProbe;
/** Outer wall-clock bound. An inner tool-native timeout lives in the handler (D6). */
timeoutFloorMs: number;
emptyOutput: EmptyOutputPolicy;
/** The `## <reviewsSection> Review` heading in write_reviews. Unique (D8). */
reviewsSection: string;
evidenceClass: EvidenceClass;
/** External tools required on PATH — `jq` for five lanes. */
requiresBinaries: ReadonlyArray<string>;
/** Dotted config key for per-lane prompt trimming, or null. */
promptBudgetKey: string | null;
/**
* Dotted config key holding this lane's model override, or null when the lane accepts none.
*
* Added by Phase 5b (#2799). Phase 1 left the model key IMPLICIT (`review.models.<slug>`), and
* that convention is wrong for a lane that ships today: `antigravity`'s slug is `antigravity` but
* its key is `review.models.agy` (`review.md:291`, and Phase 4 federated it under that exact
* name). Resolving by slug would look up `review.models.antigravity`, miss, and silently ignore a
* configured model — the pinned-model escape hatch #2073 added precisely so a 404ing default can
* be overridden.
*
* Declared rather than derived, for the same reason `promptBudgetKey` is: the key is data about
* the lane, and a naming convention that one shipped lane already breaks is not a contract.
*/
modelConfigKey: string | null;
handler: LaneHandler;
}
/**
* `transport` sits at the LANE level, a sibling of `probe` and `invoke`, exactly
* as ADR-2782 D1's manifest example places it — not nested inside `invoke`.
* Nesting it would read more naturally as a TypeScript discriminated union, and
* that is precisely the convenience Phase 2 (#2795) would have to translate away
* when harvesting this shape into the capability manifest. The union is
* discriminated at the lane level instead, which costs nothing and keeps the two
* vocabularies identical.
*/
export interface SpawnLane extends ReviewerLaneCommon {
transport: 'spawn';
invoke: SpawnInvoke;
}
export interface HttpLane extends ReviewerLaneCommon {
transport: 'openai-http';
invoke: HttpInvoke;
}
export type ReviewerLane = SpawnLane | HttpLane;
const SPAWN_STDIN_STDOUT = {
promptChannel: 'stdin',
outputChannel: 'stdout',
} as const;
/**
* The twelve declared lanes, in `write_reviews` order.
*
* `kimi-code` joined in Phase 5b (#2799, closes #2718) — ADR-2782's phase table lands it here
* rather than in 5a precisely so it arrives together with the iteration that can invoke it.
*/
export const REVIEWER_LANES: ReadonlyArray<ReviewerLane> = Object.freeze([
{
slug: 'gemini',
flags: ['--gemini'],
transport: 'spawn',
probe: { kind: 'command-exists', binary: 'gemini' },
invoke: {
binary: 'gemini',
args: ['{{model}}', '-p', '-'],
...SPAWN_STDIN_STDOUT,
modelArg: '-m',
effortChannel: 'none',
},
timeoutFloorMs: 900_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'Gemini',
evidenceClass: 'source-grounded',
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.gemini',
modelConfigKey: 'review.models.gemini',
handler: null,
},
{
// 1_200_000 rather than the 900_000 floor: headless Claude measured ~525 s
// on a large plan set (review.md:304).
slug: 'claude',
flags: ['--claude'],
transport: 'spawn',
probe: { kind: 'command-exists', binary: 'claude' },
invoke: {
binary: 'claude',
args: ['{{model}}', '{{effort}}', '-p', '-'],
...SPAWN_STDIN_STDOUT,
modelArg: '--model',
effortChannel: 'argv',
// #2483: without these the claude leg is the only reviewer that additionally inherits the
// invoking user's global CLAUDE.md, the project CLAUDE.md, and Claude Code auto-memory —
// a context asymmetry against the independent-review premise (gemini sees only the
// assembled prompt; codex runs --ephemeral). Both flags, not just the first: CLAUDE.md
// loading and auto-memory are independently-toggled mechanisms, and an environment
// exporting CLAUDE_CODE_DISABLE_AUTO_MEMORY=0 forces auto-memory back ON — the explicit
// pair is robust against that. Applies when /gsd:review runs from a non-Claude-Code host;
// inside Claude Code the claude lane self-skips for independence.
env: { CLAUDE_CODE_DISABLE_CLAUDE_MDS: '1', CLAUDE_CODE_DISABLE_AUTO_MEMORY: '1' },
},
timeoutFloorMs: 1_200_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'Claude',
evidenceClass: 'source-grounded',
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.claude',
modelConfigKey: 'review.models.claude',
handler: null,
},
{
// Codex captures the review through its own `-o/--output-last-message` and
// discards stdout, because on Windows it writes process-teardown noise to
// stdout AFTER the final message, which a stdout redirect would append to a
// non-empty file and slip past the empty-output guard (#1698).
slug: 'codex',
flags: ['--codex'],
transport: 'spawn',
probe: { kind: 'command-exists', binary: 'codex' },
invoke: {
binary: 'codex',
// Leg order exactly: codex exec --ephemeral --model M $EFFORT --skip-git-repo-check -o F -
args: ['exec', '--ephemeral', '{{model}}', '{{effort}}', '--skip-git-repo-check', '{{output}}', '-'],
promptChannel: 'stdin',
outputChannel: 'file-arg',
outputArg: '-o',
modelArg: '--model',
effortChannel: 'argv',
},
timeoutFloorMs: 1_200_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'Codex',
evidenceClass: 'source-grounded',
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.codex',
modelConfigKey: 'review.models.codex',
handler: null,
},
{
// Fed no prompt: CodeRabbit reviews the working-tree diff and accepts
// neither a prompt nor a model flag (review.md:367). Its findings are
// deliberately down-weighted in the consensus step.
slug: 'coderabbit',
flags: ['--coderabbit'],
transport: 'spawn',
probe: { kind: 'command-exists', binary: 'coderabbit' },
invoke: {
binary: 'coderabbit',
args: ['review', '--prompt-only'],
promptChannel: 'none',
outputChannel: 'stdout',
modelArg: null,
effortChannel: 'none',
},
timeoutFloorMs: 360_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'CodeRabbit',
evidenceClass: 'diff-only',
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.coderabbit',
// Accepts no model flag at all (review.md:367) — not merely "none configured".
modelConfigKey: null,
handler: null,
},
{
// `--format json` is the primary invocation, not a fallback: the review text
// lives in assistant `text` parts, which the default formatter drops when the
// agent stops with no final message (#1936). Reconstruction needs jq.
slug: 'opencode',
flags: ['--opencode'],
transport: 'spawn',
probe: { kind: 'command-exists', binary: 'opencode' },
invoke: {
binary: 'opencode',
args: ['run', '{{model}}', '{{effort}}', '--format', 'json', '-'],
...SPAWN_STDIN_STDOUT,
modelArg: '--model',
effortChannel: 'argv',
},
timeoutFloorMs: 660_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'OpenCode',
evidenceClass: 'source-grounded',
// Phase 5b: the handler reconstructs from the JSON stream with JSON.parse, so `jq` — absent on
// stock Windows/Git-Bash (#2589) — is no longer a prerequisite for this lane.
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.opencode',
modelConfigKey: 'review.models.opencode',
// Phase 5b (#2799): was `null`. The review is REBUILT from assistant `text` parts; a plain
// stdout copy would write the raw JSON envelope as the review (#1936). See LaneHandler.
handler: 'opencode',
},
{
slug: 'qwen',
flags: ['--qwen'],
transport: 'spawn',
probe: { kind: 'command-exists', binary: 'qwen' },
invoke: {
binary: 'qwen',
args: ['-'],
...SPAWN_STDIN_STDOUT,
modelArg: null,
effortChannel: 'none',
},
timeoutFloorMs: 900_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'Qwen',
evidenceClass: 'source-grounded',
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.qwen',
modelConfigKey: null,
handler: null,
},
{
// `cursor-agent` is a SEPARATE binary from the `cursor` IDE launcher. Print
// mode takes the prompt as an ARGUMENT, so a full plan set is passed by file
// reference to stay clear of the 32,767-char Windows execFileSync ceiling.
slug: 'cursor',
flags: ['--cursor'],
transport: 'spawn',
probe: { kind: 'command-exists', binary: 'cursor-agent' },
invoke: {
binary: 'cursor-agent',
args: ['-p', '--mode', 'ask', '--trust', '--output-format', 'text', '{{prompt}}'],
promptChannel: 'argv-file-ref',
outputChannel: 'stdout',
modelArg: null,
effortChannel: 'none',
},
timeoutFloorMs: 900_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'Cursor',
evidenceClass: 'source-grounded',
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.cursor',
modelConfigKey: null,
handler: null,
},
{
// Handler-owned: a three-layer fallback for an upstream stdout bug, a
// two-level timeout (600 s external cap over a 540 s native --print-timeout),
// and a stale-response watermark guard. `timeoutFloorMs` carries the OUTER
// bound only; the inner one lives in the handler (ADR-2782 D6).
slug: 'antigravity',
flags: ['--antigravity', '--agy'],
transport: 'spawn',
probe: { kind: 'command-exists', binary: 'agy' },
invoke: {
binary: 'agy',
args: ['--print-timeout', '540s', '{{model}}', '-p', '{{prompt}}'],
promptChannel: 'argv-file-ref',
outputChannel: 'stdout',
modelArg: '--model',
effortChannel: 'none',
},
timeoutFloorMs: 600_000,
emptyOutput: 'handler-owned',
reviewsSection: 'Antigravity',
evidenceClass: 'source-grounded',
// Phase 5b: the handler reads the transcript with JSON.parse per line, not `jq`.
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.antigravity',
// NOT `review.models.antigravity` — the shipped key is `review.models.agy` (review.md:291) and
// Phase 4 federated it under that name. This lane is why the key is declared, not derived.
modelConfigKey: 'review.models.agy',
handler: 'antigravity',
},
{
slug: 'ollama',
flags: ['--ollama'],
transport: 'openai-http',
probe: {
kind: 'http-reachable',
hostConfigKey: 'review.ollama_host',
path: '/v1/models',
timeoutMs: 2_000,
},
invoke: {
hostConfigKey: 'review.ollama_host',
defaultHost: 'http://localhost:11434',
path: '/v1/chat/completions',
modelDiscovery: 'first-from-models-endpoint',
fallbackModel: 'llama3',
effortChannel: 'none',
},
timeoutFloorMs: 120_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'Ollama',
evidenceClass: 'source-grounded',
// Phase 5b: the openai-compatible handler speaks HTTP directly and parses with JSON.parse, so
// neither `jq` nor `curl` is a prerequisite any more (#2589's stated Windows hazard).
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.ollama',
modelConfigKey: 'review.models.ollama',
handler: 'openai-compatible',
},
{
slug: 'lm_studio',
flags: ['--lm-studio'],
transport: 'openai-http',
probe: {
kind: 'http-reachable',
hostConfigKey: 'review.lm_studio_host',
path: '/v1/models',
timeoutMs: 2_000,
},
invoke: {
hostConfigKey: 'review.lm_studio_host',
defaultHost: 'http://localhost:1234',
path: '/v1/chat/completions',
modelDiscovery: 'first-from-models-endpoint',
fallbackModel: 'local-model',
effortChannel: 'none',
},
timeoutFloorMs: 120_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'LM Studio',
evidenceClass: 'source-grounded',
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.lm_studio',
modelConfigKey: 'review.models.lm_studio',
handler: 'openai-compatible',
},
{
slug: 'llama_cpp',
flags: ['--llama-cpp'],
transport: 'openai-http',
probe: {
kind: 'http-reachable',
hostConfigKey: 'review.llama_cpp_host',
path: '/v1/models',
timeoutMs: 2_000,
},
invoke: {
hostConfigKey: 'review.llama_cpp_host',
defaultHost: 'http://localhost:8080',
path: '/v1/chat/completions',
modelDiscovery: 'first-from-models-endpoint',
fallbackModel: 'local-model',
effortChannel: 'none',
},
timeoutFloorMs: 120_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'llama.cpp',
evidenceClass: 'source-grounded',
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.llama_cpp',
modelConfigKey: 'review.models.llama_cpp',
handler: 'openai-compatible',
},
{
// Phase 5b (#2799) — closes #2718. Net-new in this phase BY DESIGN (ADR-2782's phase table):
// declaring it in 5a would have made it selectable but not invocable, producing an empty
// section for the whole 5a → 5b window.
//
// The probe is `command-capability`, not `command-exists`, and that is the entire reason D7's
// vocabulary ships wider than existence: `kimi` is claimed by BOTH the Kimi Code CLI (Node) and
// the legacy Python kimi-cli, which is a separate first-party runtime capability in this repo.
// An existence-only probe registers the wrong tool. The needle `--output-format` appears in
// Kimi Code's `--help` and is absent from the legacy CLI (whose headless flags are `--print` /
// `--work-dir`). Verified in both directions against Kimi Code CLI 0.29.2 and a stub legacy
// binary in closed PR #2776 — analysis carried forward with credit to @drungrin.
//
// The original probe there was an UNBOUNDED `kimi --help | grep` that ran on EVERY /gsd:review
// regardless of flags: a live instance of this repo's named Unbounded Subprocesses defect, and
// the review blocker. Here the bound is declared (`timeoutMs`) and the runner enforces it, and
// the probe runs only for a SELECTED lane.
slug: 'kimi-code',
flags: ['--kimi-code'],
transport: 'spawn',
probe: {
kind: 'command-capability',
binary: 'kimi',
needle: '--output-format',
timeoutMs: 5_000,
},
invoke: {
// Print mode takes the prompt as an ARGUMENT, so the full plan set goes by file reference to
// stay clear of the 32,767-char Windows execFileSync ceiling — same shape as cursor.
binary: 'kimi',
args: ['{{model}}', '-p', '{{prompt}}'],
promptChannel: 'argv-file-ref',
outputChannel: 'stdout',
modelArg: '-m',
effortChannel: 'none',
},
timeoutFloorMs: 900_000,
emptyOutput: 'stub-with-stderr',
reviewsSection: 'Kimi Code',
evidenceClass: 'source-grounded',
requiresBinaries: [],
promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.kimi-code',
modelConfigKey: 'review.models.kimi-code',
handler: null,
},
].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"` `reviewerCli`
* alias never contributed a lane here, and as of #2801 it no
* longer contributes to the selection roster
* (`deriveReviewerSlugs`) either — the two surfaces now agree.
* @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<ReviewerLane>,
registry: { capabilities?: Record<string, unknown> } | 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<string, ReviewerLane>();
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)
* ------------------------------------------------------------------ */
/**
* Frozen reason enum. Tests assert on these values, never on rendered prose —
* CONTRIBUTING.md "Tests assert on typed structured values". Adding a reason is
* three coordinated changes: this enum, the emitting site, and the test locking
* `Object.keys(...).sort()`.
*/
export const PARITY_VIOLATION = Object.freeze({
MALFORMED_LANE: 'malformed_lane',
INVALID_SLUG: 'invalid_slug',
ROSTER_SLUG_UNDECLARED: 'roster_slug_undeclared',
DESCRIPTOR_LANE_NOT_IN_ROSTER: 'descriptor_lane_not_in_roster',
REGISTRY_LANE_UNDECLARED: 'registry_lane_undeclared',
DESCRIPTOR_LANE_NOT_IN_REGISTRY: 'descriptor_lane_not_in_registry',
BESPOKE_LEG_PRESENT: 'bespoke_leg_present',
DUPLICATE_SLUG: 'duplicate_slug',
DUPLICATE_FLAG: 'duplicate_flag',
DUPLICATE_SECTION: 'duplicate_section',
} as const);
export type ParityViolationReason =
(typeof PARITY_VIOLATION)[keyof typeof PARITY_VIOLATION];
export interface ParityViolation {
reason: ParityViolationReason;
subject: string;
}
export interface ParityResult {
ok: boolean;
violations: ParityViolation[];
}
export interface ParityInput {
descriptor: ReadonlyArray<ReviewerLane>;
roster: ReadonlyArray<string>;
/**
* Reviewer slugs declared by `reviewer` bodies in the GENERATED capability registry.
*
* Phase 5b (#2799). Once `invoke_reviewers` iterates lanes, the registry — not the workflow text —
* is what actually decides which lanes exist at runtime, so this is the surface parity has to
* bind. A missing or malformed value degrades to violations rather than silence, exactly as
* `workflowText` does: a checker that cannot tell "no registry" from "registry agrees" is worse
* than no checker.
*/
registry: ReadonlyArray<string>;
/** Full text of gsd-core/workflows/review.md. */
workflowText: string;
}
/**
* The marker that used to make a hand-authored `invoke_reviewers` leg identifiable.
*
* Phase 5b (#2799) deleted every bespoke leg, so this regex flipped polarity: matching one is now
* the VIOLATION (`BESPOKE_LEG_PRESENT`) rather than the requirement. It is retained precisely
* because deleting it would leave nothing stopping a future contributor from quietly re-adding a
* per-CLI block — which is the drift (#2718 → #2781) this whole epic exists to end.
*/
const LEG_MARKER_RE = /<!--\s*reviewer-lane:\s*([a-z0-9_-]+)\s*-->/g;
/**
* The slug grammar, and the reason it is enforced rather than assumed.
*
* `LEG_MARKER_RE` can only capture `[a-z0-9_-]`. A lane whose slug falls outside
* that class is therefore UNMATCHABLE in the workflow: its marker can be present
* and correct and the scan will still never see it, so the lane reports
* `LEG_MARKER_MISSING` forever with no indication why. All twelve shipped slugs
* sit inside the class (`lm_studio`, `llama_cpp` use the underscore), so this
* never bites today — but Phase 2 (#2795) admits third-party overlay lanes, and
* a slug like `acme.reviewer` would silently vanish from a review.
*
* ADR-2782 does not specify a slug grammar. Rather than widen the marker regex —
* which would make an HTML comment scanned out of prose more ambiguous, not less
* — the grammar is pinned here and a violating slug is reported as
* `INVALID_SLUG`. A loud, named violation beats a silent miss; that is the whole
* design principle of this module.
*/
export const LANE_SLUG_RE = /^[a-z0-9][a-z0-9_-]*$/;
/** Bounds of the step a bespoke leg marker could appear inside. */
const INVOKE_STEP_RE = /<step name="invoke_reviewers">([\s\S]*?)<\/step>/;
function sliceStep(workflowText: string, re: RegExp): string {
const m = workflowText.match(re);
return m ? m[1] : '';
}
function countOccurrences(haystack: string, re: RegExp): Map<string, number> {
const counts = new Map<string, number>();
// Fresh lastIndex per call — a module-level /g regex carries state between
// calls and would silently skip matches on the second invocation.
const scanner = new RegExp(re.source, re.flags);
let m: RegExpExecArray | null;
while ((m = scanner.exec(haystack)) !== null) {
const key = m[1].trim();
counts.set(key, (counts.get(key) ?? 0) + 1);
}
return counts;
}
/**
* Bidirectional parity across three surfaces: the descriptor, the roster
* (`KNOWN_REVIEWER_SLUGS`), and the generated capability **registry** — plus one anti-parity
* assertion against the workflow.
*
* **Re-pointed by Phase 5b (#2799).** Through Phase 5a this function also required a literal
* `<!-- reviewer-lane: <slug> -->` per lane inside `invoke_reviewers` and a literal
* `## <Section> Review` per lane inside `write_reviews`. Phase 5b deletes exactly that text: the
* workflow now iterates declared lanes and renders sections from `reviewsSection`, so there is no
* per-lane text left to scan. Those two families could not be kept without keeping the
* hand-maintained per-lane blocks this epic exists to delete.
*
* What replaced them is the parity that is actually load-bearing once lanes are data: the registry
* is what decides which lanes exist at runtime, so `descriptor ↔ registry` is checked in both
* directions. That is also the mechanical single source #2781/Phase 6 needs for its docs and locale
* gate, which per-leg text could never provide.
*
* Bidirectional is still the point. A forward-only check ("does each declared lane resolve?")
* misses the failure this exists to catch: #2718 added a lane and #2781 was the documentation drift
* that followed. A registry lane nobody declared must fail, and so must a re-added bespoke leg.
*
* Pure and total: never reads the filesystem, never throws. Empty or malformed `workflowText` /
* `registry` degrades to violations, so a caller cannot mistake a read failure for a clean bill of
* health.
*
* CRLF-insensitive: `\r` is stripped before matching, because a Windows autocrlf checkout would
* otherwise leave every marker unmatched.
*/
export function checkReviewerLaneParity(input: ParityInput): ParityResult {
const { descriptor, roster } = input;
const workflowText = String(input.workflowText ?? '').replace(/\r\n/g, '\n');
const violations: ParityViolation[] = [];
const add = (reason: ParityViolationReason, subject: string): void => {
violations.push({ reason, subject });
};
// --- D8 uniqueness within the descriptor itself ---
//
// Every field is validated before use rather than trusted. In Phase 1 the
// descriptor is a frozen in-source table and none of these guards can fire,
// but Phase 2 (#2795) feeds this same function manifest-derived data from
// third-party overlays — which is precisely where a malformed entry arrives.
// A checker that throws on bad input cannot report on it, and a parity gate
// that crashes is indistinguishable from one that was never run.
const seenSlug = new Set<string>();
const seenFlag = new Set<string>();
const seenSection = new Set<string>();
// The declared parameter type says `ReviewerLane[]`, but this function is a
// trust boundary — narrow from `unknown` rather than believing the annotation.
const rawLanes: unknown[] = Array.isArray(descriptor) ? (descriptor as unknown[]) : [];
for (const raw of rawLanes) {
if (raw === null || typeof raw !== 'object') {
add(PARITY_VIOLATION.MALFORMED_LANE, String(raw));
continue;
}
const lane = raw as Record<string, unknown>;
const slug = lane.slug;
if (typeof slug !== 'string' || !LANE_SLUG_RE.test(slug)) {
add(PARITY_VIOLATION.INVALID_SLUG, String(slug));
continue;
}
const section = typeof lane.reviewsSection === 'string' ? lane.reviewsSection : null;
if (seenSlug.has(slug)) add(PARITY_VIOLATION.DUPLICATE_SLUG, slug);
seenSlug.add(slug);
const flags: unknown[] = Array.isArray(lane.flags) ? (lane.flags as unknown[]) : [];
for (const flag of flags) {
if (typeof flag !== 'string') continue;
if (seenFlag.has(flag)) add(PARITY_VIOLATION.DUPLICATE_FLAG, flag);
seenFlag.add(flag);
}
// Two lanes sharing a heading would silently MERGE their output in
// REVIEWS.md, producing a review that appears to have consensus it does
// not have (ADR-2782 D8).
if (section !== null) {
if (seenSection.has(section)) add(PARITY_VIOLATION.DUPLICATE_SECTION, section);
seenSection.add(section);
}
}
// --- descriptor <-> roster ---
const rosterSet = new Set(
(Array.isArray(roster) ? roster : []).filter((x): x is string => typeof x === 'string'),
);
for (const slug of rosterSet) {
if (!seenSlug.has(slug)) add(PARITY_VIOLATION.ROSTER_SLUG_UNDECLARED, slug);
}
for (const slug of seenSlug) {
if (!rosterSet.has(slug)) add(PARITY_VIOLATION.DESCRIPTOR_LANE_NOT_IN_ROSTER, slug);
}
// --- descriptor <-> registry ---
//
// The registry is what decides which lanes exist at runtime once the workflow iterates, so this
// replaces the per-leg text checks Phase 5b deleted. A non-array degrades to an empty set, which
// then reports every descriptor lane as missing — loud, not silent.
const registrySet = new Set(
(Array.isArray(input.registry) ? input.registry : []).filter(
(x): x is string => typeof x === 'string',
),
);
for (const slug of registrySet) {
if (!seenSlug.has(slug)) add(PARITY_VIOLATION.REGISTRY_LANE_UNDECLARED, slug);
}
for (const slug of seenSlug) {
if (!registrySet.has(slug)) add(PARITY_VIOLATION.DESCRIPTOR_LANE_NOT_IN_REGISTRY, slug);
}
// --- anti-parity: no bespoke leg may return ---
//
// Phase 5b deleted every hand-authored per-CLI block. Nothing in the type system stops a future
// contributor from adding one back, and a re-added block is invisible to every other check here
// (it would still be declared, still be in the roster, still be in the registry). Matching a leg
// marker is therefore now the violation.
const markerSlugs = countOccurrences(sliceStep(workflowText, INVOKE_STEP_RE), LEG_MARKER_RE);
for (const slug of markerSlugs.keys()) {
add(PARITY_VIOLATION.BESPOKE_LEG_PRESENT, slug);
}
return { ok: violations.length === 0, violations };
}
/* ------------------------------------------------------------------ *
* Documentation parity (ADR-2782 Phase 6, #2800 — closes #2781/#2272)
* ------------------------------------------------------------------ */
/**
* Frozen reason enum for the DOCUMENTATION arm.
*
* Deliberately separate from `PARITY_VIOLATION` rather than an extension of it. The two functions
* answer different questions — `checkReviewerLaneParity` asks *what runs* (descriptor ↔ roster ↔
* registry), this asks *what is documented*. Fusing them would change the signature of a function
* with seven dependents and would let a stale doc make the runtime checker look red.
*
* Same three-coordinated-changes rule as its sibling: this enum, the emitting site, and the test
* locking `Object.keys(...).sort()`.
*/
export const DOCS_PARITY_VIOLATION = Object.freeze({
MALFORMED_LANE: 'malformed_lane',
DOC_FLAG_MISSING: 'doc_flag_missing',
DOC_FLAG_UNDECLARED: 'doc_flag_undeclared',
DOC_TITLE_MISSING: 'doc_title_missing',
SIGNATURE_FLAG_MISSING: 'signature_flag_missing',
DOC_UNREADABLE: 'doc_unreadable',
TABLE_ROW_MISSING: 'table_row_missing',
} as const);
export type DocsParityViolationReason =
(typeof DOCS_PARITY_VIOLATION)[keyof typeof DOCS_PARITY_VIOLATION];
export interface DocsParityViolation {
reason: DocsParityViolationReason;
/** The document the violation was found in. */
doc: string;
/** The flag or section title at fault. */
subject: string;
}
export interface DocsParityResult {
ok: boolean;
violations: DocsParityViolation[];
/**
* Documents that carry no `/gsd-review` surface at all and were therefore skipped.
*
* Reported rather than silently dropped: `docs/pt-BR/FEATURES.md` is a 77-line stub, and a
* caller that cannot tell "this mirror does not document the command" from "this mirror agrees"
* has the same blindness this whole gate exists to remove.
*/
skipped: string[];
}
export interface DocsParityInput {
descriptor: ReadonlyArray<ReviewerLane>;
/** Map of document label (repo-relative path) to its full text. */
docs: Record<string, string>;
}
/**
* Non-lane tokens that legitimately share the `/gsd-review` signature line.
*
* `--all` is a selection control, not a reviewer lane. Without this allow-list the undeclared-flag
* arm would fire on correct documentation — the gate failing on a doc that is right is the fastest
* way to get a gate deleted.
*/
const NON_LANE_SIGNATURE_FLAGS: ReadonlySet<string> = new Set(['--all']);
/**
* Is `flag` documented in `text` under one of the two structural shapes docs actually use?
*
* Backticked is the `COMMANDS.md` table-cell shape; bracketed (`[--gemini]`) is the
* `FEATURES.md` signature shape. Requiring one of those two delimiters — rather than a bare
* substring — is what keeps prose and fenced examples from satisfying the gate, and it bounds the
* token for free: a backticked `--claude` demands its closing backtick, so a backticked
* `--claude-foo` cannot satisfy it.
*
* Literal `includes`, deliberately NOT a RegExp. `new RegExp` throws `SyntaxError` on a pattern
* beyond ~100k chars, and Phase 2 (#2795) admits third-party overlay lanes whose declared strings
* are untrusted in length — a parity gate that throws on its own input is indistinguishable from
* one that never ran. Literal matching also removes the need to escape metacharacters, which is
* what `llama.cpp` needed.
*/
function flagIsDocumented(text: string, flag: string): boolean {
return text.includes('`' + flag + '`') || text.includes('[' + flag + ']');
}
/**
* Strip fenced code blocks and HTML comments before parity matching.
*
* Both are places a flag can be *mentioned* without being *documented*: a fenced block showing
* markdown syntax, or a commented-out row left behind by an edit. Counting either is a false pass
* — the gate would report a lane as documented when a reader never sees it.
*
* Lines are replaced with empty strings rather than removed so that every downstream line index
* (the signature line, the Purpose paragraph beneath it) still refers to the same physical line.
*/
function stripNonProse(text: string): string {
const lines = text.split('\n');
const out: string[] = [];
let inFence = false;
let inComment = false;
for (const line of lines) {
const trimmed = line.trim();
if (!inComment && /^(`{3,}|~{3,})/.test(trimmed)) {
inFence = !inFence;
out.push('');
continue;
}
if (inFence) {
out.push('');
continue;
}
let working = line;
if (!inComment && working.includes('<!--') && !working.includes('-->')) {
inComment = true;
working = working.slice(0, working.indexOf('<!--'));
out.push(working);
continue;
}
if (inComment) {
if (working.includes('-->')) {
inComment = false;
working = working.slice(working.indexOf('-->') + 3);
out.push(working);
} else {
out.push('');
}
continue;
}
// Single-line comments: strip to a FIXED POINT, then honor any unterminated opener.
//
// One pass is not enough. `<!--<!---->-->` strips the inner span and leaves a live `<!--`
// behind, so a commented-out row could survive and be counted as documented — the exact
// false pass this helper exists to prevent. (CodeQL flags the single-pass form as
// js/incomplete-multi-character-sanitization; here the consequence is a wrong verdict rather
// than an injection, but the fix is the same.) Iterating to a fixed point terminates because
// every pass strictly shortens the string.
let previous: string;
do {
previous = working;
working = working.replace(/<!--[\s\S]*?-->/g, '');
} while (working !== previous);
// Whatever `<!--` remains after that is unterminated, so it opens a multi-line comment that
// the `inComment` branch above will close on a later line.
const danglingOpen = working.indexOf('<!--');
if (danglingOpen !== -1) {
inComment = true;
working = working.slice(0, danglingOpen);
}
out.push(working);
}
return out.join('\n');
}
/** Every bracketed `--token` on a signature line — the line's claimed reviewer flag set. */
const BRACKETED_FLAG_RE = /\[(--[a-z0-9][a-z0-9-]*)\]/g;
/**
* Declared flags that appear in the FIRST cell of a markdown table row.
*
* The distinction is load-bearing, not cosmetic. `docs/COMMANDS.md` and every locale mirror carry
* BOTH a per-lane reviewer table (flag in cell 1) and a forwarding row that lists all 13 flags in
* cell 3. A file-scoped check is therefore satisfied by the forwarding row alone, so deleting a
* lane's actual table row — the exact #2781 regression — passes. Keying on cell 1 separates the
* two: `| `--agy` / `--antigravity` | … |` is one lane row declaring two flags, while
* `| Reviewer flags | No | … `--gemini`, `--claude` … |` is not a lane row at all.
*/
function flagsInFirstTableCell(lines: string[], declared: ReadonlySet<string>): Set<string> {
const found = new Set<string>();
for (const line of lines) {
const trimmed = line.trim();
if (!trimmed.startsWith('|')) continue;
const cells = trimmed.split('|');
// split('|') on "| a | b |" yields ['', ' a ', ' b ', ''] — cell 1 is index 1.
if (cells.length < 2) continue;
const firstCell = cells[1];
for (const flag of declared) {
if (flagIsDocumented(firstCell, flag)) found.add(flag);
}
}
return found;
}
/**
* The `**Command:** ... /gsd-review ... [--flag]` signature line, if the doc carries one.
*
* Anchored on `/gsd-review` plus a bracketed flag rather than on the bolded label, because the
* label is translated in every mirror. Structure survives translation; prose does not.
*/
function findSignatureLine(lines: string[]): number {
return lines.findIndex((l) => /\/gsd[-:]review\b/.test(l) && l.includes('[--'));
}
/**
* Documentation parity for the declared reviewer roster (#2800; closes #2781, #2272).
*
* Gates `docs/COMMANDS.md`, its four locale mirrors, `docs/FEATURES.md` and its mirrors against
* the one declared source of truth. This is the `DEFECT.GENERATIVE-FIX` parity assertion
* `CONTEXT.md:806` requires for this roster and which has never existed for it — only the Cursor
* lane ever had one (`tests/cursor-reviewer.test.cjs`).
*
* Three arms, deliberately independent so that gaming one does not green the build:
*
* 1. **flag** — every declared flag appears, delimited, somewhere in the doc. This is the arm
* that catches #2781 (`--kimi-code` present in `docs/COMMANDS.md` and absent from all four
* mirrors).
* 2. **signature** — where a `Command:` signature line exists it is held to the FULL roster, and
* any bracketed non-lane token on it is reported. This restores per-site strictness the
* file-wide arm cannot provide.
* 3. **title** — every declared `reviewsSection` appears in the Purpose paragraph. This is the
* arm that catches the class #2800 names: the `Command:` line updated and the `Purpose:` line
* one row below not.
* 4. **table row** — a doc carrying a per-lane table must carry a row for EVERY lane, keyed on
* the FIRST cell. Without this the file-wide flag arm is satisfied by the forwarding row that
* lists all flags in cell 3, and deleting a lane's own row — the #2781 regression itself —
* goes undetected.
*
* A doc carrying no `/gsd-review` surface is **skipped, not failed** — a partial mirror that never
* claimed to document the command is not drift.
*
* Pure and total: no filesystem, no clock, never throws. A non-string document is reported as
* `DOC_UNREADABLE` rather than coerced or skipped; an empty or absent `docs` map degrades to
* violations rather than a clean bill of health, because a checker that cannot distinguish
* "nothing to read" from "everything agrees" is worse than no checker.
*
* CRLF-insensitive: a Windows `autocrlf` checkout must produce an identical verdict.
*/
export function checkReviewerDocsParity(input: DocsParityInput): DocsParityResult {
const violations: DocsParityViolation[] = [];
const skipped: string[] = [];
const add = (reason: DocsParityViolationReason, doc: string, subject: string): void => {
violations.push({ reason, doc, subject });
};
// Trust boundary: narrow from `unknown` rather than believing the annotation. Phase 2 (#2795)
// admits third-party overlay lanes into this same descriptor.
const declaredFlags: string[] = [];
const declaredTitles: string[] = [];
const rawLanes: unknown[] = Array.isArray(input?.descriptor)
? (input.descriptor as unknown[])
: [];
for (const raw of rawLanes) {
if (raw === null || typeof raw !== 'object') {
add(DOCS_PARITY_VIOLATION.MALFORMED_LANE, '(descriptor)', String(raw));
continue;
}
const lane = raw as Record<string, unknown>;
const flags: unknown[] = Array.isArray(lane.flags) ? (lane.flags as unknown[]) : [];
for (const flag of flags) {
if (typeof flag === 'string' && flag.length > 0) declaredFlags.push(flag);
}
if (typeof lane.reviewsSection === 'string' && lane.reviewsSection.length > 0) {
declaredTitles.push(lane.reviewsSection);
}
}
const declaredFlagSet = new Set(declaredFlags);
// An absent or non-object map is not "no docs to check" — it is every doc missing. Reported as
// such so the caller cannot read silence as success.
const rawDocs =
input?.docs !== null && typeof input?.docs === 'object'
? (input.docs as Record<string, unknown>)
: {};
const docLabels = Object.keys(rawDocs);
if (docLabels.length === 0) {
for (const flag of declaredFlagSet) {
add(DOCS_PARITY_VIOLATION.DOC_FLAG_MISSING, '(no documents supplied)', flag);
}
return { ok: violations.length === 0, violations, skipped };
}
for (const label of docLabels) {
// A document value that is not a string is REPORTED, not coerced and not skipped.
//
// The earlier version called `String(rawValue)`, which (a) throws outright on an object with
// an own non-callable `toString` and (b) — worse — turns any other object into
// `[object Object]`, a "document" with no `/gsd-review` marker that then lands in `skipped`.
// That is a silent pass: the gate would report success for input it never actually read.
// `@typescript-eslint/no-base-to-string` flags exactly this, and it is right to.
const rawValue = rawDocs[label];
if (typeof rawValue !== 'string') {
add(DOCS_PARITY_VIOLATION.DOC_UNREADABLE, label, typeof rawValue);
continue;
}
const text = rawValue.replace(/\r\n/g, '\n');
// Fenced examples and commented-out rows mention a flag without documenting it. Stripped
// BEFORE the `/gsd-review` skip check too, so a doc whose only mention is inside a fence is
// correctly treated as not documenting the command at all.
const prose = stripNonProse(text);
// Skip-if-absent. A mirror that does not document /gsd-review is a partial translation, not
// drift — gating it would make this change responsible for authoring one.
if (!/\/gsd[-:]review\b/.test(prose)) {
skipped.push(label);
continue;
}
const lines = prose.split('\n');
// --- arm 1: every declared flag is documented somewhere in the doc ---
for (const flag of declaredFlagSet) {
if (!flagIsDocumented(prose, flag)) {
add(DOCS_PARITY_VIOLATION.DOC_FLAG_MISSING, label, flag);
}
}
// --- arm 4: a doc that carries a per-lane table must carry a row for EVERY lane ---
//
// Only applies when the doc actually has a lane table (>=1 declared flag in a first cell);
// FEATURES-shaped docs carry a signature line instead and are covered by arms 2 and 3.
const rowFlags = flagsInFirstTableCell(lines, declaredFlagSet);
if (rowFlags.size > 0) {
for (const flag of declaredFlagSet) {
if (!rowFlags.has(flag)) {
add(DOCS_PARITY_VIOLATION.TABLE_ROW_MISSING, label, flag);
}
}
}
const sigIdx = findSignatureLine(lines);
if (sigIdx === -1) continue; // COMMANDS.md-shaped docs carry a table, not a signature.
// --- arm 2: the signature line carries the FULL roster and nothing undeclared ---
const sigLine = lines[sigIdx];
for (const flag of declaredFlagSet) {
if (!flagIsDocumented(sigLine, flag)) {
add(DOCS_PARITY_VIOLATION.SIGNATURE_FLAG_MISSING, label, flag);
}
}
// Fresh RegExp per line — a module-level /g literal carries `lastIndex` between calls and
// would silently skip matches on the second document scanned.
const scanner = new RegExp(BRACKETED_FLAG_RE.source, BRACKETED_FLAG_RE.flags);
const seenUndeclared = new Set<string>();
let m: RegExpExecArray | null;
while ((m = scanner.exec(sigLine)) !== null) {
const token = m[1];
if (typeof token !== 'string') continue;
if (declaredFlagSet.has(token) || NON_LANE_SIGNATURE_FLAGS.has(token)) continue;
if (seenUndeclared.has(token)) continue;
seenUndeclared.add(token);
add(DOCS_PARITY_VIOLATION.DOC_FLAG_UNDECLARED, label, token);
}
// --- arm 3: the Purpose paragraph names every declared section title ---
//
// The Purpose paragraph is the next non-blank line after the signature. Structural, so it
// survives every translated label. Absent (signature is the last line) => title arm skipped,
// the flag arms still stand.
let p = sigIdx + 1;
while (p < lines.length && lines[p].trim() === '') p += 1;
if (p >= lines.length) continue;
const purpose = lines[p];
for (const title of declaredTitles) {
if (!purpose.includes(title)) {
add(DOCS_PARITY_VIOLATION.DOC_TITLE_MISSING, label, title);
}
}
}
return { ok: violations.length === 0, violations, skipped };
}