bmad-fr-nfr-review-gate
Run a BMAD spec-driven post-implementation review gate that blocks completion until every requirement row scores 5/5 and the gate reports zero new findings. Use when a GitHub PR, feature, bugfix, or t
SKILL.mdplugins/php-backend-sdlc/skills/bmad-fr-nfr-review-gate/SKILL.md
GitHubBMAD FR/NFR Review Gate
Use this skill after implementation when a PR, feature, bugfix, or task has
BMAD specs under specs/. The gate checks whether the implementation
corresponds to every functional and non-functional requirement, verifies
expanded quality dimensions, every pinned system quality attribute, generated
positive/negative/edge test cases, automated test and CI coverage, flaky-test
risk, and related whole-codebase impact, then blocks completion until all
applicable rows score 5/5 and the gate run reports zero new findings.
Profile keys consumed
make.fr_nfr_gatemake.cimake.testsmake.e2emake.infectionmake.load_testsmake.deptracmake.pr_commentsmake.post_review_findingsquality.phpinsights.complexityquality.infection_msicapabilities.load_testingcapabilities.publish_pr_commentsci.required_checksreview.request_changes_blocking
Gate runner resolution
The gate invocation is resolved from the profile make map:
make.fr_nfr_gatenon-null: run the mapped target — the repository ships its own gate wrapper, which may accept richer toggles (publishing suppression, autonomous commit-and-push remediation, iteration caps).make.fr_nfr_gate: null(the shipped default): use the plugin script. Never assume a repo-local make target exists:"${CLAUDE_PLUGIN_ROOT}/scripts/fr-nfr-gate.sh" --spec-path "specs/<slug>" --impact-context "<one-line change summary>"
Inputs
- BMAD spec bundle or file:
--spec-path specs/<slug>(defaultspecs/). - Impact context:
--impact-contextcarries a one-line change summary plus graph/relationship conclusions (see below) and, when manual testing was performed, a pointer to the manual-evidence file. - Manual evidence: a markdown file the reviewer writes and cites in scorecard
rows. The shipped script does not ingest it directly — summarize its
conclusions in
--impact-contextand cite the file path in the report. - Publishing behavior (shipped script): always posts the
BMAD FR/NFR Review Gatecommit status for the PR HEAD; posts a PR comment carrying the findings only when the finding count is above zero — success stays comment-quiet, the status check is the durable signal. Exit 0 means zero new findings; findings, malformed output, or transport failure after one retry exit 1 with a failure status. - A repo wrapper mapped via
make.fr_nfr_gatemay expose extra inputs:
# Upstream reference wrapper invocation with its env-var contract:
BMAD_REVIEW_SPEC_PATH=specs/my-bundle \
BMAD_REVIEW_MANUAL_EVIDENCE=var/manual-test-evidence/<task>.md \
BMAD_REVIEW_AUTO_PUSH=true \
make bmad-fr-nfr-review-gate
Pinned NFR Catalog
The gate uses these NonFunctionals.com catalog categories:
- Performance
- Usability
- Maintainability
- Availability
- Interoperability
- Security
- Manageability
- Automatability
- Dependability
Do not add, remove, or rename categories during a review unless the skill is being intentionally updated.
Expanded Quality And Impact
The gate also requires an Expanded Quality Scorecard covering:
- Functional Suitability
- Performance Resource Sustainability
- Compatibility Coexistence
- Interaction Capability Accessibility
- Reliability Resilience
- Security Privacy Accountability
- Maintainability Testability
- Flexibility Portability
- Safety Harm Prevention
- Data Quality Integrity
- Operational Excellence Releaseability
- Observability Diagnosability
- Supply-Chain Integrity
- Compliance Governance
- Sustainability Resource Impact
- AI Automation Governance
It also requires a System Quality Attributes Scorecard covering every current attribute from https://en.wikipedia.org/wiki/List_of_system_quality_attributes. Each attribute must have a scored row with evidence, source, status, and an improvement recommendation. If an improvement, metric, guardrail, test, CI check, or operational control is missing, the row fails and the report must include a Required Fix.
The Whole-Codebase Impact Analysis must cover changed and related surfaces: runtime paths, architecture/layer boundaries, domain model, persistence, public API/schema, async events/queues, config/env, dependencies/lockfiles, CI/workflows, tests/fixtures, docs, operations/observability, security/privacy, and backward compatibility.
Graph/relationship evidence is required for whole-codebase impact scoring.
Supply it as impact context from the layer-dependency graph (the target mapped
by make.deptrac), codebase-memory MCP, CodeQL, SCIP, or similar tools. If no
context is supplied, build a bounded local graph/relationship context from
changed files and direct symbol references; the reviewer still has to inspect
related code rather than relying only on changed files. Every NFR catalog row,
expanded quality row, and system quality attribute row must cite
graph/relationship evidence, or give a concrete source-backed reason why graph
evidence is irrelevant for that row.
Repository Architecture And Design-Smell Gate
The gate must explicitly apply the sibling code-organization, implementing-ddd-architecture, deptrac-fixer, and complexity-management rules before assigning 5/5 to Maintainability, Maintainability Testability, Modularity, Simplicity, Testability, Data Quality Integrity, Dependability, or Architecture and layer-boundary impact rows.
Concrete changed-code blockers include:
- constructor default instantiation of collaborators, nullable service
collaborators with fallback
newobjects, optionalNullLoggerfallbacks, and manual internal resolver/service trees that should be injected or built by an explicit factory; - class placement that violates the class-type directory model, including new
or expanded
Service/,Helper/,Util/, orManagerbuckets, resolvers inEventListener/, normalizers inValidator/, and DTOs that own validators, normalizers, or environment policy; - Application code that leaks concrete external SDK/framework construction or raw vendor objects through use-case ports instead of Infrastructure adapters behind typed Application interfaces;
- sentinel result states encoded with empty strings, booleans, or nullable fields without explicit invariants or named state variants;
- factories or transformers that hide persistence, event publication, I/O, or broad orchestration;
- callback-by-reference control flow or
assert()used to prove business success state; - widening dedicated domain collections to bare
array,iterable, or unstructured payloads in production APIs; - multi-aggregate persistence across separate flushes without a transaction boundary, durable compensation/reconciliation design, or source-backed reason that best-effort rollback satisfies the stated FR/NFR;
- unbounded PR load-test workflows where a bounded smoke/subset check is the intended PR gate, or load-test jobs without clear timeout, log, and artifact evidence.
If any item applies, the gate must fail and include the exact Required Fix.
A generic statement that DDD layout or maintainability "looks good" is not
enough for a 5/5 score. Remediation is always root-cause: no suppression
annotations, never edit deptrac.yaml, and never lower quality.*
thresholds — quality.phpinsights.complexity (canonical floor 94) and
quality.infection_msi (canonical floor 100) are raise-only.
Scoring Contract
| Score | Meaning |
|---|---|
| 1/5 | Requirement not addressed or evidence absent |
| 2/5 | Partial implementation with major gaps |
| 3/5 | Implemented but missing tests, evidence, or important edge cases |
| 4/5 | Implemented and mostly verified with minor unresolved risk |
| 5/5 | Fully implemented, verified, traceable, and review-ready |
PASS requires all applicable FRs, NFRs, NFR catalog categories, expanded quality dimensions, system quality attributes, generated test-case matrix rows, automated test and CI coverage rows, flaky-test risk rows, whole-codebase impact surfaces, manual-test requirements, QA checkpoints, GitHub completion checks, and CI checks to score 5/5. It also requires review of vulnerabilities, bugs, regressions, defects, operational problems, and data-loss/privacy/security risks. A not-applicable row is allowed only with a concrete reason and source evidence. Missing evidence fails closed.
Mandatory QA Matrix
The reviewer must generate expected positive, negative, and edge/boundary/
race/timeout/error cases from every FR, NFR, acceptance criterion, story, and
quality requirement. It must map each repeatable case to automated tests and CI
checks: the suites behind make.tests and make.e2e, mutation testing behind
make.infection, load tests behind make.load_tests when
capabilities.load_testing is true (skip with a capability-absent note when
false), plus applicable static-analysis, security-scan, and contract/schema
checks. Manual evidence is supporting evidence only for behavior that cannot
be fully automated. Missing repeatable automated coverage, missing negative or
edge tests, unmitigated flaky-test risk, or unreviewed vulnerability/defect
risk blocks PASS.
Workflow
Read the BMAD spec bundle: PRD, architecture, epics/stories, research, and implementation-readiness files when present.
Extract every FR, NFR, acceptance criterion, story requirement, and readiness requirement with source path evidence.
Confirm expected positive, negative, and edge test classes from the spec so missing automated coverage can be treated as a blocker.
Run the gate via the resolved runner (see Gate runner resolution). With the shipped script:
"${CLAUDE_PLUGIN_ROOT}/scripts/fr-nfr-gate.sh" --spec-path "specs/<slug>" --impact-context "<one-line change summary>"If manual testing is required, record evidence in a markdown file (format below), cite it in the scorecard rows, summarize its conclusions in the impact context, and rerun the gate.
If the gate exits non-zero (new findings), apply PR-scoped root-cause refactors/fixes, rerun verification, and start the next gate iteration. A verification failure after a passing review is treated as another fix iteration, not completion. Bound the loop with an explicit iteration counter; an unbounded loop is acceptable only when intentionally chosen.
Fetch and address GitHub comments via the target mapped by
make.pr_commentswhen a PR exists; whenmake.pr_commentsisnull, use${CLAUDE_PLUGIN_ROOT}/scripts/get-pr-comments.sh.Do not mark the PR/task complete until the gate exits 0, the target mapped by
make.cipasses (capability-absent note whennull), GitHub comments are resolved, every check listed inci.required_checksis green, and — whenreview.request_changes_blockingis true — no requested-changes review remains. Human approval is not required before the gate runs or posts status updates.For PR work, leave the final result visible on the PR through the
BMAD FR/NFR Review Gatecommit status (and the findings comment on FAIL).
Publish (gated)
The gate's BMAD FR/NFR Review Gate commit status remains the durable success
signal; this Publish step is an additional consolidated view and does not
replace it. When capabilities.publish_pr_comments is true, project the gate
findings / per-requirement matrix to the canonical ledger JSON (schema in the
poster header) at ${SDLC_LEDGER_DIR:-.sdlc/review-ledgers}/fr-nfr.json, then
publish ONE consolidated, idempotent PR comment via the target mapped by
make.post_review_findings; when that key is null (the shipped default), the
plugin substitutes its script:
"${CLAUDE_PLUGIN_ROOT}/scripts/post-review-findings.sh" fr-nfr \
--file "${SDLC_LEDGER_DIR:-.sdlc/review-ledgers}/fr-nfr.json" --pr "$PR"
The poster is idempotent (hidden <!-- sdlc-review:fr-nfr --> marker — it
updates its prior comment, never spams), authorized (writes only to the resolved
repo's own PR), and DEGRADES (NFR-3): capabilities.publish_pr_comments
false/absent, gh absent, no PR, an empty ledger, a mismatched base repo, or a
gh write failure all skip-with-note and exit 0 — publishing NEVER fails this
gate. When the flag is false/absent, skip this step with a note.
Required PASS Markers
The reviewing agent authors the review report that carries these markers
— they are a contract on the report you write, not output emitted by the
shipped fr-nfr-gate.sh. The shipped script enforces only the mechanical
FR_NFR_NEW_FINDINGS: <n> contract (see the note after the marker list); it
never emits or validates any *_SCORECARD, *_MIN_SCORE, or STATUS: line.
So produce the marker-bearing scorecard report yourself from the scoring work
above; do not expect the script's stdout or its commit status to contain them.
(A repo wrapper mapped via make.fr_nfr_gate may additionally enforce these
markers on reviewer output — when one is mapped, follow its contract too.)
The review report produced for the gate must include:
FR_NFR_SCORECARD: PASS
NFR_CATALOG_SCORECARD: PASS
EXPANDED_QUALITY_SCORECARD: PASS
SYSTEM_QUALITY_ATTRIBUTES_SCORECARD: PASS
WHOLE_CODEBASE_IMPACT: PASS
GRAPH_IMPACT_CONTEXT: PASS
TEST_CASE_MATRIX: PASS
AUTO_TEST_COVERAGE: PASS
FLAKY_TEST_RISK: PASS
MANUAL_TEST_EVIDENCE: PASS
QA_BEST_PRACTICES: PASS
GITHUB_COMPLETION_GATE: PASS
CI_GATE: PASS
A STATUS: PASS without these markers is a failure, and PASS also requires
EXPANDED_QUALITY_MIN_SCORE: 5/5, IMPACT_ANALYSIS_MIN_SCORE: 5/5,
SYSTEM_QUALITY_ATTRIBUTES_MIN_SCORE: 5/5, TEST_CASE_COVERAGE_MIN_SCORE: 5/5,
AUTO_TEST_COVERAGE_MIN_SCORE: 5/5, and FLAKY_TEST_RISK_MIN_SCORE: 5/5
evidence markers. The shipped script enforces a mechanical contract on top:
the gate output's mandatory last line is FR_NFR_NEW_FINDINGS: <n>, and only
n = 0 exits 0 — a missing or malformed line fails closed with a failure
commit status.
Manual Evidence Format
Manual evidence must include:
- tester
- date
- scenario
- steps
- observed result
- linked artifacts or command output when available
- related FR/NFR IDs or NFR catalog categories
Do not fabricate manual evidence. If evidence is absent, leave the gate failing and report the exact manual action required.
Verification
Run focused checks for this skill change:
bash -n "${CLAUDE_PLUGIN_ROOT}/scripts/fr-nfr-gate.sh"
For production code changes, also run the target mapped by make.ci.