msd-core/commands/gsd/plan-review-convergence.md at fe5731185eebc4a48573893357909418121a7f42

Files

Tom Boucher 352876ff0c fix(#2315 ): respect review.default_reviewers in bare convergence invocation (#2451 )

* test(#2315): regression test for review.default_reviewers precedence

A bare /gsd-plan-review-convergence invocation (no reviewer flags) is supposed
to let users configure a persistent reviewer lineup via review.default_reviewers
and just run the loop. Instead, the orchestrator's argument parser silently
discards that configuration and forces --codex on every no-flag invocation —
with no warning that the configured reviewers were ignored.

This commit adds a regression test that fails against the pre-fix workflow
(the buggy unconditional --codex fallback is still present at this commit)
and passes after the fix lands:

- Structural: the workflow must NOT contain an unconditional
  'if [ -z "$REVIEWER_FLAGS" ]; then REVIEWER_FLAGS="--codex"; fi' line
  before the workflow.plan_review_convergence config gate.
- Structural: the workflow must query review.default_reviewers AFTER the config
  gate and document that empty REVIEWER_FLAGS lets gsd-review apply the default.
- Behavioral: matrix across {configured, unset, empty-array, explicit-flag}
  invoking the actual deployed parse + resolution blocks with a stubbed gsd_run.

Also updates two existing tests whose assertions the fix makes stale:
- #2293 behavioral: endMarker was the buggy unconditional fallback line; the
  bare invocation assertion was 'run("5") === "--codex"'. Both flip post-fix
  (endMarker is now the last --all grep line; bare invocation returns empty
  from the parse block, default applied later in step 1.5).
- command-default-claim: the pre-fix command documented '--codex (default if
  no reviewer specified)' which was the user-facing mirror of the bug. The
  assertion now requires the command to document the review.default_reviewers
  precedence.

* fix(#2315): respect review.default_reviewers in bare convergence invocation

Root cause: plan-review-convergence.md step 1 (Parse and Normalize Arguments)
contained an unconditional fallback that set REVIEWER_FLAGS=\"--codex\" whenever
no explicit reviewer flag was supplied. This value was then interpolated
verbatim into the gsd-review args, so gsd-review saw --codex as an explicit
flag (precedence rule 1) and never reached rule 3 (review.default_reviewers).
The same path silently dropped any configured review.reviewer_instances
(instances participate ONLY via review.default_reviewers per ADR-1517).

Fix:
- Remove the unconditional --codex fallback from step 1.
- Add a config-gated resolution in step 1.5 (after CONVERGENCE_ENABLED check)
  that queries review.default_reviewers and either leaves REVIEWER_FLAGS empty
  (letting gsd-review apply its own rule-3 default) or falls back to --codex
  when no default is configured — preserving the pre-fix default for
  unconfigured users (AC3).
- Replace the banner {REVIEWER_FLAGS} token with {REVIEWER_DISPLAY} so the
  startup banner reflects what will actually run (AC4), not a hardcoded value.

The fix upholds the documented precedence contract (ADR-0011, ADR-0015) that
the bug was actively violating. Explicit-flag invocations (--gemini, --all,
etc.) are unaffected (AC5).

References: #2315; ADR-0011 (review.default_reviewers precedence);
ADR-0015 (autonomous cross-AI convergence); ADR-1517 (reviewer instances).

* chore(#2315): bump plan-review-convergence.md baseline + changeset

- Bump plan-review-convergence.md size baseline 23713 → 25536 (the new
  step-1.5 default-resolution block).
- Add .changeset/plucky-yaks-roar.md documenting the user-visible change.

* fix(#2315): restore /gsd: colon syntax + skip behavioral test when jq missing

Two follow-ups to the #2315 fix discovered by gsd-test:

1. The fix commit accidentally regressed the slash-command namespace in the
   disabled-feature exit message: /gsd:plan-review-convergence (correct,
   from PR #3452) became /gsd-plan-review-convergence (retired dash syntax).
   The slash-command-namespace invariant test caught this. Restored the
   colon form.

2. The behavioral test exercises the deployed reviewer-resolution block,
   which pipes through jq. jq is a documented production dependency
   (review.md:244 "install jq if missing") and is present in every
   production deployment, but is NOT on PATH in the gsd-test linux-node{22,24}
   containers (same constraint as tests/opencode-review-reconstruction.
   property.test.cjs). Without jq, the printf|jq pipeline fails silently,
   the ||echo 0 fallback yields DEFAULT_REVIEWERS_COUNT=0, and the resolution
   falls through to the --codex branch — producing a false negative.
   Added a jqAvailable guard at module load (matching the existing pattern)
   and skip the behavioral test when jq is absent. The structural tests
   (no bash execution) still run and validate the fix.

* chore(#2315): regenerate golden-install-parity fixtures

Source changes to plan-review-convergence.md (workflow + skill mirror +
command doc) changed the install-tree hashes. Regenerated via
'npm run gen:golden' after rebuilding gsd-core/bin/lib/install-engine.cjs
('npm run build:lib') — the on-disk lib was stale relative to
src/install-engine.cts (isSymlinkedDestOptIn) and blocked fixture gen.

* test(#2315): address review findings — strengthen structural tests + property test

Code-review + security-review (isolated subagent passes) surfaced Low/Nit
findings; this commit addresses the test-side findings:

- Structural test 1 ("unconditional --codex one-liner") now asserts the
  buggy line is absent EVERYWHERE, not just before the config gate. The
  earlier assertion allowed a maintainer to re-add the line after the gate
  (passing the structural test) while the bug would still bite at runtime
  before the gate runs.
- Structural test 4 ("banner uses REVIEWER_DISPLAY") now asserts the
  LITERAL banner placeholder "Reviewers: {REVIEWER_DISPLAY}" and forbids
  "Reviewers: {REVIEWER_FLAGS}". The earlier workflow.includes(
  "REVIEWER_DISPLAY") was satisfied by a comment mention.
- New structural test for command/skill content parity: both files must
  document the review.default_reviewers precedence on the --codex flag
  (catches a manual edit to one that the gen:plugin-skills mirror misses).
- Behavioral test stub now passes default_reviewers via env var
  ($GSD_TEST_DEFAULT_REVIEWERS) instead of inline-interpolating into a
  bash single-quoted string. Removes the (currently-safe) fragility where
  a future test input containing a single quote would close the bash
  quote and execute as bash under execFileSync.
- New property test (fast-check, numRuns=25) for the JSON-classification
  contract: non-empty arrays of slugs -> empty REVIEWER_FLAGS; empty
  array / scalar JSON / malformed JSON -> --codex fallback. Locks the
  parser contract per CLAUDE.md mandate.

* fix(#2315): address review findings — defensive jq-missing warning + banner cleanup

Code-review surfaced two Low-severity workflow-side findings:

- Defensive jq-missing warning: if jq is not on PATH in production (it is
  a documented dependency per review.md:244, but the dependency can be
  absent in degraded environments), the printf|jq pipeline fails silently
  to "0" and a user with review.default_reviewers configured gets --codex
  with no indication their configured default was unreadable. Added a
  command -v jq guard at the top of the resolution that falls back to
  --codex AND emits a stderr warning explaining the reason. This makes the
  failure diagnosable instead of silently reproducing the #2315 override.

- Banner leading-space cleanup: REVIEWER_FLAGS accumulates with a leading
  space ("$REVIEWER_FLAGS --gemini" from ""), so the explicit-flag branch
  of REVIEWER_DISPLAY="$REVIEWER_FLAGS" rendered "Reviewers:  --gemini"
  (double space). Pre-existing but worth fixing alongside the AC4 banner
  work. Strip one leading space with the ${VAR# } parameter expansion in
  the explicit-flag branch only (the configured-default and --codex
  branches already produce clean strings).

* chore(#2315): bump plan-review-convergence.md baseline + regen golden fixtures

The defensive jq-missing warning grew plan-review-convergence.md
(25536 -> 26285 bytes). Bumps the per-file baseline snapshot and
regenerates the golden-install-parity fixtures for the resulting
install-tree hash changes.

* chore(#2315): backfill pr:2451 in .changeset/plucky-yaks-roar.md

2026-07-20 10:55:28 -04:00

3.5 KiB

Raw Blame History

name, description, argument-hint, allowed-tools, requires

name

description

argument-hint

allowed-tools

requires

gsd:plan-review-convergence

Cross-AI plan convergence - replan until review concerns are resolved.

<phase> [--codex] [--gemini] [--claude] [--opencode] [--ollama] [--lm-studio] [--llama-cpp] [--agy] [--text] [--ws <name>] [--all] [--max-cycles N]

Read

Write

Bash

Glob

Grep

Agent

Skill

AskUserQuestion

phase

review

Cross-AI plan convergence loop — an outer revision gate around gsd-review and gsd-planner. Repeatedly: review plans with external AI CLIs → if HIGH or actionable non-HIGH concerns remain → replan with --reviews feedback → re-review. Stops when no unresolved HIGH concerns or actionable MEDIUM/LOW findings remain outside PLAN.md, or when max cycles is reached.

Flow: Skill("gsd-plan-phase") → Agent→Skill("gsd-review") → check unresolved HIGH + actionable non-HIGH → Skill("gsd-plan-phase --reviews") → Agent→Skill("gsd-review") → ... → Converge or escalate

Replaces gsd-plan-phase's internal gsd-plan-checker with external AI reviewers (codex, gemini, etc.). Plan-phase runs inline (bare Skill at depth 0) so it can spawn gsd-planner/gsd-plan-checker at depth 1. Review runs inside an isolated Agent (gsd-review is a Bash leaf — no sub-agents needed). Orchestrator only does loop control.

Orchestrator role: Parse arguments, validate phase, run plan-phase inline (Skill at depth 0), spawn an Agent for gsd-review, check unresolved HIGH and actionable non-HIGH counts, stall detection, escalation gate.

<execution_context> @$HOME/.claude/gsd-core/workflows/plan-review-convergence.md @$HOME/.claude/gsd-core/references/revision-loop.md @$HOME/.claude/gsd-core/references/gates.md @$HOME/.claude/gsd-core/references/agent-contracts.md </execution_context>

<runtime_note> Copilot (VS Code): Use vscode_askquestions wherever this workflow calls AskUserQuestion. They are equivalent — vscode_askquestions is the VS Code Copilot implementation of the same interactive question API. Do not skip questioning steps because AskUserQuestion appears unavailable; use vscode_askquestions instead. </runtime_note>

Phase number: extracted from $ARGUMENTS (required)

Flags:

--codex — Use Codex CLI as reviewer (default if no reviewer flag given AND review.default_reviewers is unset; otherwise review.default_reviewers wins per ADR-0011 — #2315)
--gemini — Use Gemini CLI as reviewer
--agy / --antigravity — Use Antigravity CLI as reviewer (successor to the discontinued Gemini CLI)
--claude — Use Claude CLI as reviewer (separate session)
--opencode — Use OpenCode as reviewer
--ollama — Use local Ollama server as reviewer (OpenAI-compatible, default host http://localhost:11434; configure model via review.models.ollama)
--lm-studio — Use local LM Studio server as reviewer (OpenAI-compatible, default host http://localhost:1234; configure model via review.models.lm_studio)
--llama-cpp — Use local llama.cpp server as reviewer (OpenAI-compatible, default host http://localhost:8080; configure model via review.models.llama_cpp)
--all — Use all available CLIs and running local model servers
--max-cycles N — Maximum replan→review cycles (default: 3)

Feature gate: This command requires workflow.plan_review_convergence=true. Enable with: gsd config-set workflow.plan_review_convergence true

Execute end-to-end. Preserve all workflow gates (pre-flight, revision loop, stall detection, escalation).

3.5 KiB Raw Blame History

3.5 KiB

Raw Blame History