PR Council Review

pr-council-review — PR Council Review skill

Council-driven PR review. Dispatches 6 baseline + N adaptive parallel reviewers, each scoped to one dimension, each invoking the relevant Claude Code specialized skill (e.g. security-review, code-review, verify), then synthesizes a single consensus verdict.

This skill is the canonical body. The Claude Code slash command at .claude/commands/gflow/pr-council-review.md is a thin wrapper that invokes this skill. Non-Claude tools (Antigravity / Codex / Cursor / Aider) can consume this SKILL.md directly via their own skill loaders.

Three modes:

1. No argument → list open PRs ranked by review priority; user picks. (See § 1.)
2. PR# argument → run the full council on that PR. (See § 2 onward.)
3. Branch mode → run the full council on the current local feature branch (no PR yet). Invoked via the /gflow:branch-review wrapper. See § 8 for the PR→branch translation table and pre-flight.

Treat YELLOW as soft block — it is advisory in name only; clear it or dismiss it with a logged justification (§ 5 step 8).

0 · Pre-flight

All six checks are mandatory. Any failure (except step 6, which records a finding) halts before Phase 1/2.

1. gh authenticated — run gh auth status. Non-zero exit → stop with: "gh is not authenticated. Run gh auth login and re-invoke."
2. Inside the repo — assert AGENTS.md AND CLAUDE.md exist in the working directory.
3. Resolve the argument:
   • Empty → jump to Phase 1 (Prioritize).
   • PR number → validate with gh pr view <N> --json number. If error → stop with the error verbatim.
4. Draft check (PR# mode only) — if gh pr view <N> --json isDraft returns true, surface a banner citing memory [[draft-pr-merge-trap]]: "PR #N is DRAFT. Reviewing is fine, but do NOT merge a draft (the merge API can close it + delete the head ref). Run gh pr ready N first if you intend to merge. Continue review? (yes/no)". Ask the user before dispatching.
5. Capture PR head ref + SHA (pin the review) — head_branch=$(gh pr view <N> --json headRefName --jq '.headRefName') and head_sha=$(gh pr view <N> --json headRefOid --jq '.headRefOid'). Pin both to a REVIEWED_SHA variable and pass to every dispatched agent so the council's verdict is anchored to one commit. The local working tree is NOT on the PR head; all file reads must go through git show $REVIEWED_SHA:<path> (or git show origin/$head_branch:<path> if you fetched first). If the author pushes new commits during the review, the council still reports against REVIEWED_SHA; the synthesizer notes any divergence in Phase 5 step 5.
6. Mechanical CI gate (D0 — non-LLM, runs BEFORE dispatch). The LLM dimensions reason about the diff; none of them run the repo's actual lint/format/link gates, so a whole-tree failure sails past the council (this happened on PR #269 — a latent ruff format failure in a file the diff only touched went green through 8 agents, then reddened CI and dragged SonarCloud new_coverage to 0). Run the exact CI gate commands (.github/workflows/ci.yml → Lint / Format check / Documentation links / Repo hygiene) against the reviewed tree:

   # Prefer running at REVIEWED_SHA. If HEAD is already there (reviewing your own
   # just-pushed PR, or branch-review mode), run in place:
   if [ "$(git rev-parse HEAD)" = "$REVIEWED_SHA" ]; then dir=.; else \
     dir=$(mktemp -d); git worktree add --detach "$dir" "$REVIEWED_SHA"; fi
   ( cd "$dir" && uv run ruff check src tests \
       && uv run ruff format --check src tests \
       && uv run python scripts/ci/check_doc_links.py \
       && uv run python scripts/ci/check_repo_hygiene.py )
   # if a worktree was created: git worktree remove --force "$dir" (Windows: prune later if locked)

   • Any non-zero → record a D0 — CI-mechanical RED. This is a hard blocker regardless of the LLM dimensions' verdicts; surface the failing command + output verbatim in the report and do NOT call the PR merge-ready. (Mirrors the SonarCloud-gate rule in the wrapper: the council must not bless a tree CI will reject.)
   • If running the gate is impractical (no uv, worktree add fails), fall back to gh pr checks <N> and inspect the test job's Lint/Format steps; if they are pending or failing, flag D0 as UNVERIFIED — must be confirmed green before merge, never as GREEN.
   • On a fork PR, gh pr checks is not sufficient and the fallback above is blind. GitHub holds pull_request workflows from forks at conclusion=action_required until a maintainer clicks Approve and run, and such a run does not appear in statusCheckRollup at all — so the PR reports every check green while nothing ran. Measured on 2026-09-15: #781 (approved) showed 16 checks; #793 (held) and #787 (no run) each showed 2, all green. Run:

     uv run python scripts/ci/check_fork_pr_ci.py --pr <N>

     Non-zero → D0 is RED — CI never ran. Approve the workflow run, or gate the head locally, before trusting any green on that PR.
   • Unlike steps 1–5, a D0 failure does not halt — dispatch the LLM council anyway so its findings are gathered in one pass, then fold D0 into the Phase 5 verdict.

1 · Prioritize (no-argument mode)

gh pr list --state open --json number,title,author,isDraft,headRefName,updatedAt,additions,deletions,labels,reviewDecision,statusCheckRollup

Empty-list short-circuit: if the result is [], print "No open PRs to review." and exit.

Rank with these heuristics (highest priority first):

Present a numbered table, then stop and ask the user to pick a PR number. Do NOT auto-start on Rank 1 — recommend, do not pre-select.

2 · Gather context (PR# mode)

Pull in parallel via ctx_batch_execute:

• PR_META → gh pr view <N> --json title,body,author,baseRefName,headRefName,headRefOid,state,isDraft,additions,deletions,changedFiles,labels,files,statusCheckRollup
• PR_DIFF → gh pr diff <N>
• PR_CHECKS → gh pr checks <N>
• TOUCHED_PATHS → gh pr view <N> --json files --jq '.files[].path' | sort -u
• RECENT_COMMITS → gh pr view <N> --json commits --jq '.commits[-5:] | .[] | "\(.oid[:7]) \(.messageHeadline)"'
• PR_COMMENTS → gh pr view <N> --json comments --jq '.comments[] | "\(.createdAt) \(.author.login): \(.body)"'

Read the thread before you flag "no evidence". PR_COMMENTS is not optional colour: a prior council verdict, a maintainer's counter-capture, and the contributor's reply all live there and none of them appear in the diff. On PR #650 the autonomous run posted "reverses a confirmed-live finding with no live evidence attached" as its headline must-fix — 50 minutes after the contributor had posted a machine-generated capability matrix with a SHA-256 and a screenshot in that same thread. Truncate long bodies if you must, but never review a contested PR without reading what has already been said on it.

Reference files (read via git show origin/$head_branch:<path> — NOT local Read, because the working tree is on develop):

• CLAUDE.md, AGENTS.md, docs/INDEX.md

Spike traversal (NEW v2.2) — do this BEFORE dispatch, and hand the result to the reviewers. The council reasons about the diff. A measurement that refutes the diff's premise does not appear in the diff, so no dimension can find it by reading well. Sweep docs/superpowers/spikes/ for the surface the PR touches and pass every match into the prompts of the dimensions that own that surface, as required reading:

total=$(ls docs/superpowers/spikes/*.md | wc -l)
gh pr diff <N> | grep -ohiE '/about|batchexecute|recaptcha|networkidle|SNlM0e|aisandbox[a-z-]*|agent-mode|referenceEntit[a-z]*|SignOutOptions|flow-[a-z-]+|ya29|SAPISID' \
| tr 'A-Z' 'a-z' | sort -u | while read -r t; do
    hits=$(grep -rli -- "$t" docs/superpowers/spikes/ 2>/dev/null)
    n=$(printf '%s\n' "$hits" | grep -c . )
    # A term matching most of the corpus is a topic, not a lead. Skip it.
    [ "$n" -gt 0 ] && [ "$n" -le $(( total / 3 )) ] && printf '## %s (%s)\n%s\n' "$t" "$n" "$hits"
  done

Do not pipe the loop through sort -u — it separates the headers from their paths, and the grouping is the readable part.

Selectivity is the whole trick, and it is measurable rather than a matter of taste. Counted against the 30-spike corpus on 2026-09-16: flow.google.com hits 23 — a topic, useless as a lead — while /about hits 9, including all three about-redirect spikes. So filter out any term matching more than a third of the corpus; what remains is short enough to actually read.

Read the verdict section of each hit, not the whole spike. If one contradicts something the PR asserts, that is a blocking finding before a single agent is dispatched — and it is cheaper than every dimension that would have failed to notice.

Written from PR #835. It added a migrated-host auth oracle that read the rendered DOM and claimed the signal was server-attested. Eight dimensions passed it — D1, D3, D6, D10 all read the auth path closely and none objected to the premise. The refutation was five days old and already in this repository: 2026-09-11-about-redirect-is-decided-client-side.md measured that Flow's /about hop is decided client-side with zero requests to Flow, and states outright that "the backend grants access while the frontend declines to open it." That single sentence invalidates the oracle. The diff touched flow.google.com in fourteen places; a grep would have put the spike in front of D6 and D10 as required reading. The e2e caught it instead — by going red on a live account whose state moved mid-session. This is a routing failure, not a reviewer failure, and routing is fixable.

Memory traversal: for each TOUCHED_PATH, look up relevant slugs:

• transports/ → [[migrated-host-driver-wire-lessons]], [[pr-must-verify-on-affected-surface]], [[flow-locale-leak-icon-ligatures]], [[ligature-carrier-differs-by-host]], [[playwright-click-no-downstream-event-signature]], [[rest-transports-drop-ui-fields]], [[image-video-mode-switch-symmetry]], [[ui-selector-drift-error-exit-23]]
• data/ → [[data-layer-overview]], [[data-layer-test-pollution-trap]], [[exit-code-16-data-store]], [[on-started-callback-recorder-safety]]
• auth/ → [[real-browser-auth-mandatory]], [[release-signing]]
• cli → [[release-back-merge-gap-recovery]], [[wheel-build-sanity-gate]]
• tests/ (any) → [[bdd-stubs-mirror-runtime-signatures]], [[background-e2e-pytest-pattern]], [[full-test-suite-ooms]], [[stale-test-discovery]], [[structlog-cache-logger-off-for-tests]]
• tests/features/ (BDD) → also [[bdd-stubs-mirror-runtime-signatures]]
• scripts/ → [[wheel-build-sanity-gate]], [[release-back-merge-gap-recovery]]
• .planning/, docs/superpowers/ → [[release-spec-plan-memory-consolidation]]
• docs/, *.md → [[readme-hybrid-router-pattern]], [[doc-examples-are-untested-fixtures]], [[agents-md-vs-llms-txt]], [[pypi-readme-staleness-fix]]
• pyproject.toml, .github/ → [[release-spec-plan-memory-consolidation]], [[pr-hygiene-revert-and-multi-commit]], [[draft-pr-merge-trap]], [[pypi-rejected-filename-reusable]]

3 · Detect adaptive dimensions

D15 specifics. gflow ships every capability twice, and no automated gate can see the two copies drift: tests/mcp/test_cli_parity.py is command-level (a new leaf needs a mapping), so an unmirrored option, an unread queued-payload key, or a docstring asserting removed behaviour is green everywhere. Walk the six mirror axes in skills/check/SKILL.md step 1b against the diff and report each as satisfied or drifted. Highest-yield check: for every param the PR touches, confirm the key mcp/tools.py writes into the queue payload is the key worker/codec.py reads — they are matched by string, so a mismatch type-checks and silently no-ops. This dimension exists because #626 unlocked a CLI combination while mcp/tools.py and docs/MCP.md went on telling agents it was rejected, through a fully green pipeline.

Baseline floor is non-negotiable. D1–D5 and D14 ALWAYS run. Docs-only PRs (100% paths under *.md, docs/**, CHANGELOG.md, README.md, LICENSE, AUTHORS) → D4 reframes from "test code coverage" to "docs-verification"; D5 still runs unchanged.

4 · Dispatch the council

Use the superpowers:dispatching-parallel-agents skill. Send all agents in one message — they must run concurrently.

Per-dimension specialized-skill mapping (NEW v2)

Each agent is a general-purpose agent (only subagent type that supports arbitrary parallel dispatch), but the prompt instructs it to invoke the relevant Claude Code skill inside the agent for specialized capability. Mapping:

On the D14 soft dependency: ponytail:ponytail-review is a user-local plugin, not shipped with this repo, so it is optional (same pattern as the agy extra reviewer in issue-resolve). The over-engineering lens is owned by this skill (the rubric below); the plugin only accelerates it. An agent without the plugin applies the rubric directly and still produces a D14 verdict — never skip D14 because the plugin is absent.

Per-agent prompt skeleton (mandatory v2 changes in bold)

You are one of <N> parallel reviewers on a council reviewing PR #<N> of `gflow-cli` at C:\development\github\gflow-cli.

Your dimension is **<DIMENSION NAME>**. Other agents handle <other dimensions> — do NOT duplicate their work.

**PR head branch:** `<head_branch>` (head SHA: `<head_sha>`).
**Base branch:** `<base_branch>`.

**🚨 CRITICAL — file reading + verification rules (v2 stale-tree-reads fix + v2.1 verify-before-claim):**

The orchestrator's working tree is on `<base_branch>` (typically `develop`), NOT the PR head. If you `Read` a file in `C:\development\github\gflow-cli\`, you get the PRE-PR copy and will produce FALSE POSITIVES like "file X doesn't exist" or "claim Y not applied" when in fact X and Y are present on the PR head.

**Mandatory rules:**

1. **For file inspection** — ALWAYS use `ctx_execute(language="shell", code="git show <REVIEWED_SHA>:<path>")` (or `git show origin/<head_branch>:<path>`). For the diff itself, `gh pr diff <N>`. For metadata, `gh pr view <N> --json ...`. NEVER use `Read` on a repo file unless you have verified `git branch --show-current` returns `<head_branch>`.

2. **Verify-before-claim — applies to BEHAVIOR claims, not just file existence (v2.1 NEW):** any "feature/setting/marker/env-var is NOT present" or "is missing" or "is wrong" claim MUST be backed by an explicit `git show <REVIEWED_SHA>:<path> | grep <expected>` (or equivalent) that you ran. Quote the exact command + its output in your report. Do NOT rely on memory or summary; the v2 council had a real false-negative where D5 claimed "PR doesn't add addopts filter" because the agent assumed-not-verified — the addopts WAS added but the agent never ran `git show <SHA>:pyproject.toml`. Treat your own claims like a code reviewer: would this assertion survive a hostile re-review? If yes, ship it; if uncertain, re-verify.

3. **SHA pinning verification (v2.1 NEW):** before reporting findings, run `git rev-parse $REVIEWED_SHA` (or `git ls-remote origin <head_branch>`) and confirm your reads were against `<REVIEWED_SHA>`. If the author has pushed new commits during your dispatch, your findings still apply to `<REVIEWED_SHA>` — the synthesizer will note any divergence at Phase 5 step 5.

**Specialized skill (if listed for your dimension):** before deep analysis, invoke the Skill tool for `<skill_name>` to load specialized review guidance. Apply that skill's checklists in addition to the dimension-specific questions below.

Assess specifically:
1. <dimension-specific question 1, with code citation hooks>
2. <…>

**Mandatory memory you MUST consult and cite if relevant:** <fixed slug list from the Dimension → Slugs table>.

Output a structured report under 500 words:
- Verdict: GREEN / YELLOW / RED
- Must-fix (numbered, file:line refs)
- Nice-to-have (numbered)
- Confirmed-<good/safe/correct> (1-line bullets)

If you have nothing to flag, say so explicitly and state GREEN with a one-line justification — do NOT manufacture findings.

If you are NOT sure a finding is real because it depends on file content, VERIFY via `git show origin/<head_branch>:<path>` before reporting it. Stale-tree false positives are a documented v1 council bug.

Dimension → mandatory memory slugs table

Slugs resolve directly: [[<slug>]] → docs/superpowers/memory/<slug>.md. Open exactly the files your dimension's row names — no searching, no judgement call about what is relevant. The directory is in the repo, so it is available to every agent that can read the tree, including the sandboxed autonomous runs that have no access to a maintainer's local store.

scripts/ci/check_council_memory.py enforces the round trip both ways: a citation with no file fails CI, and a file no dimension cites fails CI too. So a gap announces itself instead of quietly degrading routing back into a search — which is what this table previously did, when the slugs were "conceptual anchors" that resolved to nothing.

These files are a published subset of the maintainer's working memory, not a mirror of it: review-relevant facts only, with private identifiers stripped. If a slug's file is missing, say so in your report and move on — never fabricate its contents.

Per-dimension specifics

• D1 Correctness & completeness: root cause vs symptom? Edge cases? CHANGELOG matches diff? PR-body compliance (test-plan checkboxes + Closes #N acceptance criteria met)?
• D2 Code quality: style consistency? Comments WHY-not-WHAT (per CLAUDE.md)? SRP intact? Type annotations on every new signature?
• D3 Security: injection vectors? Env-var trust boundary? Shell interpolation? Path traversal? Secret-shaped literals? --no-verify / signature-bypass?
• D4 Tests:

  • Mandatory affected-surface check: identify the runtime surface (T2V / I2V / data / CLI / auth / etc.). If the suite doesn't exercise that exact surface → automatically YELLOW. Cite the test file:line proving coverage, or state explicitly no such test exists.

  • Mandatory e2e-evidence check (since PR #675). If the PR touches a Flow surface, it owes an e2e test — one the author ran, with its result, or a new one under tests/e2e/. Offline-green is structurally incapable of proving a blackbox still behaves as captured ([[e2e-evidence-is-a-contributor-deliverable]]). Score it:

    • Absent, with no reason given → D4 RED, which forces consensus RED under § 5 step 3. Deliberately not YELLOW: a YELLOW is dismissable under § 5 step 8 with a one-line justification, and "the live runs in the PR body are good enough" is exactly the justification someone will write — which reopens the substitution this check exists to close, through the escape valve rather than through the wording.
    • Absent, but the author states why they could not run it (no Flow account, no credits for a video path) → YELLOW, blocked on a maintainer run. That is the documented exemption, not an evasion; name it in the report so the maintainer knows what to run.
    • Present → verify it, do not take it on trust. Cite the tests/e2e/ file:line, confirm it exists at REVIEWED_SHA (git show $REVIEWED_SHA:<path>), and confirm it exercises the surface this PR changed. A named test that passes but never touches the changed code path is the [[pr-must-verify-on-affected-surface]] failure with an e2e label on it.

    Three traps to avoid:

    • A live-verify ledger is not an e2e test. A PR body describing live runs satisfies [[verification-ledger-5-layer]], not this. It is a narrative record of one run; an e2e test is a re-runnable regression. Never record live evidence as satisfying the e2e rule — that substitution was attempted on #669 within an hour of the rule landing.
    • A BDD .feature file is not an e2e test. It runs offline against fakes.
    • Scope it honestly. Docs-only, help-text and exit-code changes touch no Flow surface and are exempt; say so rather than demanding an impossible run.
    • PRs opened before #675 merged (e7a09d8, 2026-09-05 19:15Z) predate the rule — flag it forward-looking, do not hold it against them. Anchor on the merge, not the calendar day: #669 (11:28Z) and #671 (13:38Z) were both opened that same day, hours before the rule existed, and are exactly the PRs this clause protects.

  • Test pyramid placement? Behavior vs shape? Coverage delta meaningful vs dead?

• D5 Memory hygiene & consolidation (NEW v2, refined v2.1):
  • Inspect ~/.claude/projects/C--development-github-gflow-cli/memory/ (file-based memory — primary source).
  • Concrete MCP memory-server probe (v2.1): in the current session, look at the tool list for any tool whose name matches mcp__*memory*, mcp__*mempalace*, mcp__*mem0*, mcp__*context-mode* (for indexed KB), or any tool the user explicitly mentioned. If found, invoke its "list" / "search" / "stats" tool to enumerate stored items. If no such tool is loaded in-session, state explicitly "no MCP memory server loaded; D5 scope = file-based memory only".
  • An empty or absent memory directory is UNAVAILABLE, never GREEN. Verify you can actually read it (ls the path, quote the file count) before any verdict. Memory is keyed by working-directory path, not repo identity, so a fresh clone — every autonomous sandbox run — starts with an empty namespace unless the store was synced to that host. Reporting "no memory entry contradicts this PR" from a directory you could not read is a false negative dressed as a pass: it is the assumed-not-verified failure this dimension already forbids, and it happened on PR #650, where video-model-capability-matrix.md recorded that exact PR as REJECTED while D5 reported GREEN from an empty mount.
  • For each memory entry potentially relevant to the PR's surface: does the PR's content CONTRADICT or SUPERSEDE it? If so, the PR must include a memory edit/delete in the same change. Flag missing edits.
  • Does the PR introduce a new durable pattern / failure mode / decision that should be captured as a NEW memory entry? Flag missing additions.
  • Is MEMORY.md index still in sync (entries listed correspond to existing files)? Flag drift.
  • Per [[release-spec-plan-memory-consolidation]]: any spec/plan in docs/superpowers/ or .planning/ that's now post-ship should be deleted on merge, with durable bits extracted to memory. When D5 fires RED on consolidation, D9 (Docs drift) defers to D5 — do not double-list the same plan-file finding in both dimensions.
  • Verify-before-claim discipline applies here too (v2.1): any "this memory entry is missing X" or "this PR doesn't update memory Y" claim must cite the exact memory file you read (git show or filesystem cat) and the line range. The v2 run had a real false-claim here.
• D6 UI/live-verify: selector matches real DOM on non-EN? Are selectors strictly locale-invariant (Tier 1 structural/ARIA/href/icon anchors + Tier 2 multi-locale text cascades across EN, PT, ES, DE, FR, IT, JA, ZH, KO)? Single-language English text additions WITHOUT Tier 1 structural anchors or Tier 2 cascades MUST be flagged RED/YELLOW! Runtime evidence beyond static-string invariants? Live-verify evidence (file count + magic bytes + Pillow dims + structlog events: new_project_clicked/submit_clicked/frame_attached/image_mode_entered/count_setter_completed/reference_attached) in PR body? Absent → YELLOW per [[pr-must-verify-on-affected-surface]].
• D7 Data-migration: on_started callback safety (every callsite wrapped in try/except DataStoreError — bare callback in paid-generation path = RED). Schema compat. Migration idempotent. DataStoreError vs DataMigrationError vs DataIntegrityError semantics.
• D8 CLI UX: help text matches? Flag names --kebab-case? --help golden-snapshot tests updated? Docstring examples runnable?
• D9 Docs drift: cross-references mutually consistent? Code examples run? Version strings consistent?
• D10 Auth: Chrome-strategy profile required (memory [[real-browser-auth-mandatory]])? Profile-dir SecurityError boundary intact? No new secret-shaped strings?
• D11 Release-gate: version match pyproject.toml ↔ src/gflow_cli/__init__.py (RED if drift). [Unreleased] emptied + new version added. Wheel build clean. Back-merge gap addressed.
• D12 BDD: new _run_* kwarg → mirror in tests/features/_fake_* stubs (silent TypeError trap).
• D13 Scripts: Windows-clean (memory [[windows-dev-quirks]]); release scripts include wheel-build sanity gate.
• D14 Over-engineering / YAGNI: should this code exist at all? — the lens D1 (correctness) and D2 (quality) don't ask. If ponytail:ponytail-review is installed, invoke it; otherwise apply this rubric directly to the added lines:
  • Dead / orphaned: a constant, helper, field, or branch with no live caller after the change (e.g. a module constant only its own definition + a docstring reference now use). Cut it.
  • Speculative flexibility: an interface/factory/abstraction with one implementation, a config knob or flag nobody sets, a parameter only ever passed one value, a layer with one caller. Inline until a second caller exists.
  • Reinvented stdlib / platform: a hand-rolled parser/validator/date-format the standard library or the platform already ships. Name the built-in.
  • One-caller indirection: a wrapper function whose only value is a test seam, when the callee could be patched directly (match how sibling code does it).
  • Scaffolding "for later": boilerplate, placeholder modules, or generality added for a use case not in this PR.
  • Output the concrete cut per finding (file:Lline: delete/inline/shrink — what replaces it) and, when meaningful, net: -N lines possible. A single smoke test / assert-based self-check is the minimum, never flag it as bloat. RED only for genuinely dead/unreachable code; over-generality is Nice-to-have unless it's load-bearing.