feat(code-review): render a two-part Technical + Functional review on every CR run

CHANGELOG.md

@@ -4,6 +4,8 @@ All notable changes to `cursor-rules` will be documented in this file.
 
 ## [Unreleased]
 
+- ✨ **Added** `feat(code-review)`: every full-lens CR wrapper (`code-review`, `code-review-github`, `code-review-jira`, `code-review-bugsnag`) now produces an explicit **two-part output — Technical review & Functional review** (#737). The **Technical review** is the existing strict-rule-compliance analysis, unchanged, published on the PR comment. The **Functional review** — a full checklist of every extracted acceptance criterion with its status (Met / Not met / Partial / Divergent) plus an explicit `Goal met: Yes/No` verdict — is built by `@skills/assignment-compliance-check/SKILL.md`, now rendered on **every** run with a linked tracker (including the fully affirmative report on a clean run), published on the linked-tracker comment via `@skills/pr-summary/SKILL.md`, with the one-line verdict mirrored onto the PR comment's `Summary` line. This is a deliberate, narrow **affirmative exception**, scoped to the Functional-review verdict alone, to the project's long-standing "report only what needs action; never render a positive banner; omit when clean" convention — every other CR section keeps that convention unchanged. `@skills/api-review` is a documented carve-out: instead of the full `assignment-compliance-check`, it renders a **light** `API contract matches assignment: Yes/No` verdict derived from its own Core Checks walk. Canonical contract in `@rules/code-review/general.mdc` *Two-part CR output — Technical & Functional review*; thin references from `skills/code-review/SKILL.md` (5000-token budget preserved) and the four wrapper output templates. Pinned by `tests/Installer/CodeReviewContentTest.php` and `tests/Installer/SkillsContentTest.php`.
+
 - ✨ **Added** `feat(compound-engineering)`: every agent/skill that creates a tracker issue now **selects and assigns the most relevant existing label** as part of issue creation (#734). New canonical section `@rules/compound-engineering/general.mdc` *Assign the most relevant existing label when creating a tracker issue* — enumerate the tracker's existing labels (`gh label list --json name,description` on GitHub), match by type + domain, never invent a new label or fall back to a random one, and create the issue unlabelled when nothing genuinely fits; the sanctioned `EPIC` label stays the sole documented exception. Referenced from `skills/create-issue/SKILL.md`, `skills/create-issues-from-text/SKILL.md`, `skills/blueprint/SKILL.md`, and `skills/product-capability/SKILL.md`. Pinned by `tests/Installer/CompoundEngineeringContentTest.php` and `tests/Installer/SkillsContentTest.php`.
 
 - ✨ **Added** `feat(code-review)`: every code review now walks a new **Self-Documenting Code — Comment & Doc Hygiene** lens (#733) — for every comment / PHPDoc / inline doc the diff adds or modifies, flag redundant *what*-narration that restates what the code already says and require its deletion, while never flagging the two preserved exceptions: rationale / considered alternatives / domain language (belongs in ADRs & glossaries) and navigation pointers (`@rules/…`, `@skills/…`, links to collaborating classes/methods). Documented in `@rules/code-review/general.mdc` *Self-Documenting Code — Comment & Doc Hygiene Analysis*, referenced from `skills/code-review/SKILL.md`'s Core Analysis walk (inherited by every CR wrapper), pinned by `tests/Installer/CodeReviewContentTest.php`.

rules/code-review/general.mdc

@@ -23,10 +23,14 @@ globs: []
 ## Scope
 - Focus only on relevant changes in the PR unless broader context is required.
 
-## Assignment Conformance
-- Every code-review wrapper skill — one that produces a full PR review against a linked issue / task (`@skills/code-review`, `@skills/code-review-github`, `@skills/code-review-jira`, `@skills/code-review-bugsnag`) — must run `@skills/assignment-compliance-check/SKILL.md` as an **always-run** step on every CR run, so the implementation is always checked against what the assignment actually asked for. Any unmet requirement is a **Critical** finding.
-- This invariant is mandatory and inheritable: any new CR wrapper added later must wire the same always-run assignment check before it is considered complete.
-- Single-lens specialized review skills (`@skills/api-review`, `@skills/security-review`) intentionally do **not** run it — functional / assignment conformance is owned by the wrapper. Duplicating the check inside a lens is a defect per `@rules/compound-engineering/general.mdc`, not a safeguard.
+## Two-part CR output — Technical & Functional review (issue #737)
+- Every code-review wrapper skill — one that produces a full PR review against a linked issue / task (`@skills/code-review`, `@skills/code-review-github`, `@skills/code-review-jira`, `@skills/code-review-bugsnag`) — produces a two-part output on every run:
+  1. **Technical review** — the existing CR analysis (Core Analysis, Strict rule compliance, Architecture, security, coverage, and every other walk-through in this file and `@skills/code-review/SKILL.md`), unchanged in behavior: it strictly checks the written code against every applicable project rule. Published on the **PR comment**.
+  2. **Functional review** — an explicit report answering whether the change meets the assignment's goal: a `Goal met: Yes/No` verdict, plus a named checklist of every acceptance criterion / requirement extracted from the linked assignment with its status — **Met**, **Not met**, **Partial**, or **Divergent**. `Goal met` is `No` whenever at least one criterion is not Met. Published on the **linked-tracker comment** authored by `@skills/pr-summary/SKILL.md`; the one-line verdict is mirrored on the PR comment's `Summary` line so a reader never has to leave the PR to know the outcome.
+- The Functional review is built by `@skills/assignment-compliance-check/SKILL.md` — every code-review wrapper must run it as an **always-run** step on every CR run that has a linked tracker, so the implementation is always checked against what the assignment actually asked for. This section defines the contract the skill must satisfy; the skill's own file owns the mechanics. Every **Not met** / **Partial** / **Divergent** criterion is additionally a **Critical** finding folded into the Technical review's severity buckets, per the Assignment Conformance Gate in `@skills/code-review/SKILL.md`.
+- **Affirmative exception — scoped to the Functional review only.** The project's default convention is "report only what needs action; never render a positive banner; omit when clean" (see `@rules/reports/general.mdc` and this file's Output Rules). The Functional review is the deliberate, narrow exception to that convention: it renders on **every** run that has a linked tracker, including the fully affirmative report (`Goal met: Yes — all N acceptance criteria satisfied`) when every criterion is Met — it is never omitted on a clean run. Do not extend this exception to any other surface: the Technical review, the `## Architecture` section, the `## Coverage` section, and every other CR section keep the omit-when-clean convention unchanged.
+- This invariant is mandatory and inheritable: any new CR wrapper added later must wire the same always-run assignment check and render both parts before it is considered complete.
+- `@skills/security-review` intentionally does **not** run the full assignment-compliance-check — functional / assignment conformance is owned by the wrapper, and duplicating the check inside a lens is a defect per `@rules/compound-engineering/general.mdc`, not a safeguard. `@skills/api-review` is a documented carve-out: instead of invoking the full `assignment-compliance-check`, it renders a **light** functional verdict — `API contract matches assignment: Yes/No` — derived from its own Core Checks walk against the diff's API surface (routes, status codes, payload shapes measured against what the assignment describes), so API-focused issues still get a functional signal without running two full assignment-conformance engines over the same diff. **Render target — standalone runs only.** The light verdict renders only when `api-review` is the review being invoked (a standalone API review, not wrapped by another CR skill). When it runs **inline** as a sub-lens inside a full wrapper (`@skills/code-review/SKILL.md` Specialized Reviews → Always run), that wrapper's own `assignment-compliance-check` already owns the Functional review for the run, so the light verdict line is suppressed — never render two functional signals for one CR run.
 
 ## Reuse Existing Logic
 - **Reuse-first gate — before judging *how* new logic is written, decide whether it should exist at all.** For every block of newly added or modified logic in the diff, ask the two questions in order:

skills/api-review/SKILL.md

@@ -78,8 +78,13 @@ Minor findings may omit these fields when no behavior change is implied.
 
 These fields exist so `@skills/process-code-review/SKILL.md` can turn each finding into a reproducer test and apply the fix without re-deriving context.
 
+## Functional cross-check (light — issue #737 carve-out)
+`@rules/code-review/general.mdc` *Two-part CR output — Technical & Functional review* requires every CR wrapper to render a Functional review, normally built by `@skills/assignment-compliance-check/SKILL.md`. `api-review` is a documented carve-out: it does **not** invoke the full `assignment-compliance-check` — running a second full assignment-conformance engine over the same diff would duplicate the wrapper's checklist. Instead, derive a **light** functional verdict from the Core Checks walk already performed above: does the diff's API surface (routes, status codes, payload shapes, idempotency) match what the linked assignment describes for the API contract? Render one line at the end of the returned markdown — `API contract matches assignment: Yes/No` — where `No` cites whichever Core Checks finding above traces to an unmet API requirement (no duplicated detail). When the diff touches no API surface (per Scope), omit the line entirely — there is nothing to cross-check.
+
+**Render target — standalone runs only.** Render the light verdict line only when `api-review` is invoked as a **standalone** review (not wrapped by another CR skill). When invoked **inline** as a sub-lens inside a full CR wrapper (`@skills/code-review/SKILL.md` Specialized Reviews → Always run), the wrapper's own `assignment-compliance-check` already owns the Functional review for that run — suppress the light verdict line entirely so the two-part output carries exactly one functional signal, not a redundant second one that the wrapper would discard.
+
 ## Output Format
-Use the template defined in `templates/review-output.md`. Omit any severity section that has no findings; never emit `None.` / `n/a` placeholders.
+Use the template defined in `templates/review-output.md`, appended with the **Functional cross-check** verdict line above only on a standalone run that touches an API surface; suppress the line on an inline sub-lens invocation per the Render target rule above. Omit any severity section that has no findings; never emit `None.` / `n/a` placeholders.
 
 ## Done when
 - Every API-surface change on the diff has been walked against the six Core Checks.

skills/api-review/templates/review-output.md

@@ -23,3 +23,5 @@
 - [file:line] Description — Rule: @rules/api/general.mdc#<section> — concrete fix.
 
 > Omit any section that has no findings. Faulty Example, Expected Behavior, Test Hint, and Suggested Fix are mandatory for every Critical and Moderate finding so `@skills/process-code-review/SKILL.md` can turn each into a reproducer test and apply the fix. Minor findings may omit them when no behavior change is implied.
+
+**API contract matches assignment:** Yes | No — <one-line light functional cross-check per `@skills/api-review/SKILL.md` Functional cross-check; omit this line entirely when the diff touches no API surface, or when this review runs inline as a sub-lens inside a full CR wrapper (that wrapper's `assignment-compliance-check` already owns the Functional review)>