bf-pr-review-flow

name: bf-pr-review-flow description: "Part of the Blueprintflow methodology. Use when a task PR or milestone breakdown PR is open, under review, blocked, ready for merge, or needs standard Blueprintflow merge-gate handling."

PR Review Flow

The standard flow from PR open to merged.

Direct Invocation Guard

If using-plueprint is not active, STOP here. Load using-plueprint with the user's input; do nothing else in this skill until it routes back.

Pre-requisite: implementation design ✅

Code tasks must have passed the four-role design review before the PR opens. Design doc lives in the task leaf folder as design.md. Task baseline docs come from bf-task-fourpiece; design review comes from bf-implementation-design. Non-code tasks skip this step.

🚫 Permanently forbidden (hard red line)

No scenario, no excuse. Set on 2026-04-29:

• ❌ gh pr merge --admin
• ❌ Ruleset disable / restore (even "≤10s window")
• ❌ Any way of bypassing required CI checks
• ❌ Phrases like "ruleset stopgap" / "admin merge agent" / "temporary transition"

Admin bypass hides bugs. History: e2e failures bypassed into main → each needed a hotfix.

PR template

Blueprint: docs/blueprint/next/<file>.md §X.Y
Touches: <packages or docs>
Current sync: <docs/current path + bf-current-doc-standard check, or N/A — reason>
Stage: v0|v1

## Summary
...
## Acceptance
- [x] ...
## Test plan
- [x] ...

Missing fields → lint red. Fix through lint patch flow, don't bypass.

Required reviews

Every execution PR is a task PR. Role artifacts are commits inside that task PR, not separate PR types.

Milestone breakdown PRs are the narrow planning/boundary exception: they publish reviewed task folders and boundary-level task.md files for one selected milestone. They must not create task-0-breakdown-*, four-piece files, design.md, progress.md, implementation, worktree state, or task acceptance evidence.

Verification: QA uses bf-verification before LGTM. UI/frontend PRs load bf-verification/references/ui-e2e.md; backend/data/CLI/background jobs load the matching bf-verification references.

Security review: walks references/security-checklist.md (12 categories). LGTM must cite specific items.

Remote-agent / dangerous-command review: when scope touches remote agents, host automation, command execution, approval flows, or filesystem/network delegation, Security also walks references/remote-agent-dangerous-commands.md.

LGTM command (author cannot self-approve):

gh pr comment <num> --body "LGTM (reason ≤30 chars)"

Three-gate merge rule

For code changes, task completeness includes docs/current sync. QA verifies existence; Architect verifies boundary/state/anchor quality with bf-current-doc-standard.

All three pass → gh pr merge <N> --squash --delete-branch. Any missing reviewer, red CI, or unchecked item → don't merge.

Merge-gate decisions stay in this skill. Fast-cron may route back here; it does not own a separate merge protocol.

Blocked PR Routing

Red CI blocks merge even when it looks unrelated. Track flakes separately, but merge only after required checks pass.

Flaky tests

Principle: fix, don't rerun.

• PR didn't change related code but CI fails → flaky signal
• Found flaky → fix root cause immediately
• Truly flaky → fix the real cause (race / timing / env dependency)
• Lint false positive → fix the lint rule
• ❌ Rerun and pray / "not my change" / "merge first, fix later" / "3 reruns green = passed"

Review subagent

Parallel review subagents handle machine-checkable verification. Template in references/review-subagent.md. Batch merge in references/batch-merge.md.

Subagents are read-only — they verify, they don't author (spec / stance / content-lock). NOT-LGTM → escalate to persistent role.

Review checklist

• Section breakdown lines up 1:1 with spec brief
• Counts add up
• Byte-identical anchors aligned to sources (list PR # / commit SHA)
• Anti-constraint grep (list specific pattern)

Review do's

• Read the whole file, then the diff
• Put yourself in a first-time reader's shoes
• Each role reviews from their specific angle:
  • Architect: architectural consistency + cost reasonableness + cross-skill conflicts
  • PM: user experience + cognitive load + can a new team member understand it
  • Dev: executability + ambiguity + would I get stuck following this
  • QA: verifiability + how do we know it's done right + enough examples
  • Security: injection / XSS / SSRF + auth and least-privilege + sensitive data + dependency security
  • Performance: algorithmic complexity + hot path + unnecessary IO/network + memory and concurrency
• Before LGTM, find 3 things to challenge
• Verify technical details against docs — don't trust the author
• Check for unintended side effects (bulk replace breaking formatting, edits breaking references)
• Check that existing logic isn't broken — regression thinking
• Status flips (acceptance / REG / PROGRESS) land in the same PR, no follow-up

Anti-patterns

• ❌ LGTM without reading the PR body
• ❌ Diff-only review (miss broken context)
• ❌ "No obvious bug" = LGTM (review is "good enough", not "no errors")
• ❌ Not questioning design reasonableness
• ❌ Self-LGTM as dual approval (same GH account doesn't count)
• ❌ Subagent review replacing persistent-role authoring
• ❌ Merge report containing "admin" / "ruleset" / "bypass"

Real example (Borgee): While reviewing acceptance, QA noticed a field name didn't match the Architect's spec brief (a rename hadn't been propagated). Patched on the spot. This is dual-track review working — the spec was written in shape A, acceptance naturally written against shape A, and the drift surfaced.

How to invoke

follow skill bf-pr-review-flow
dispatch review for PR #<N>