Skip to content

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

Merged
pekral merged 2 commits into
masterfrom
fix/cr-skills-two-part-review
Jul 16, 2026
Merged

feat(code-review): render a two-part Technical + Functional review on every CR run#738
pekral merged 2 commits into
masterfrom
fix/cr-skills-two-part-review

Conversation

@pekral

@pekral pekral commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #737.

Every full-lens CR wrapper (code-review, code-review-github, code-review-jira, code-review-bugsnag) now produces an explicit two-part output on every run:

  1. Technical review — the existing strict rule-compliance analysis, unchanged in behavior. Published on the PR comment.
  2. Functional review — a full acceptance-criteria checklist (Met / Not met / Partial / Divergent per criterion) plus an explicit Goal met: Yes/No verdict. Built by assignment-compliance-check, published on the linked-tracker comment via pr-summary, with the one-line verdict mirrored onto the PR comment's Summary line.

Key behavior change: assignment-compliance-check now renders the block on every run that has a linked tracker — including the fully affirmative report on a clean run — instead of the old "gaps-only, omit when clean" behavior. This is a deliberate, narrow exception to the project's "report only what needs action; never render a positive banner; omit when clean" convention, scoped to the Functional-review verdict alone; every other CR section (Technical review, Architecture, Coverage, …) keeps that convention unchanged. pr-summary's {assignment_verdict} top banner gained the matching affirmative variant (English + Czech).

api-review is a documented carve-out (per orchestrator decision): instead of invoking the full assignment-compliance-check, it renders a light API contract matches assignment: Yes/No verdict derived from its own Core Checks walk, so API-focused issues get a functional signal without running two full assignment-conformance engines over the same diff.

Files changed

  • rules/code-review/general.mdc — canonical Two-part CR output — Technical & Functional review contract (replaces the old Assignment Conformance section).
  • skills/code-review/SKILL.md — thin reference to the contract (stays under the 5000-token skill-check budget; verified str_word_count < 5000 in a pinning test).
  • skills/assignment-compliance-check/SKILL.md — core behavior change: full checklist + always-render + affirmative verdict.
  • skills/pr-summary/SKILL.md + templates/pr-summary-{github,jira}.md — affirmative {assignment_verdict} banner variant.
  • skills/code-review-{github,jira,bugsnag}/SKILL.md — updated wrapper prose describing the new always-render behavior.
  • 4 wrapper output templates (code-review, code-review-github, code-review-jira, code-review-bugsnag) — framed as "Technical review" with a pointer to the Functional review.
  • skills/api-review/SKILL.md + templates/review-output.md — light functional cross-check carve-out.
  • CHANGELOG.mdfeat(code-review) entry.
  • tests/Installer/CodeReviewContentTest.php, tests/Installer/SkillsContentTest.php — updated pins for the reversed convention (old "omit when clean" pins replaced) + new pins for the two-part contract, per the project's content-pin-tests-are-the-contract-enforcement convention.

How to test

  1. Run composer build — skill-check (0 errors/warnings, 62 skills), phpcs, Pint, Rector, phpstan, security-audit, and the full Pest suite (329 tests, 100% coverage) all pass.
  2. Read rules/code-review/general.mdc Two-part CR output — Technical & Functional review and confirm it defines both halves, the affirmative exception, and the api-review carve-out.
  3. Trigger a CR run on a PR linked to a GitHub issue (or JIRA ticket) and confirm: the PR comment is framed as the Technical review; the linked-tracker comment carries a full Goal met: Yes/No checklist (including Met entries) via pr-summary; a clean run renders ✅ Changes satisfy the assignment — all N acceptance criteria met instead of nothing.

TODO

  • code-review-bugsnag's PR-comment summary line does not carry an assignment conformance: … mirror slot the way the other three wrappers do (pre-existing asymmetry, unrelated to this change) — worth a follow-up if full parity across all four wrappers' summary lines is desired.

pekral added 2 commits July 16, 2026 11:21
… every CR run

Every full-lens CR wrapper (code-review, code-review-github,
code-review-jira, code-review-bugsnag) now produces an explicit
two-part output (#737): the Technical review (existing strict
rule-compliance analysis, unchanged, on the PR comment) and the
Functional review (a full acceptance-criteria checklist with a
Goal met: Yes/No verdict, Met/Not met/Partial/Divergent per
criterion) built by assignment-compliance-check and published on
the linked-tracker comment via pr-summary, mirrored as a one-line
verdict on the PR comment summary.

assignment-compliance-check now renders the block on every run with
a linked tracker instead of gaps-only/omit-when-clean, including the
fully affirmative report on a clean run -- a deliberate, narrow
exception to the project's report-only-what-needs-action convention,
scoped to the Functional-review verdict alone. pr-summary's
{assignment_verdict} banner gained the matching affirmative variant.
api-review is a documented carve-out: a light API contract matches
assignment: Yes/No verdict instead of the full compliance engine.

Canonical contract lives in rules/code-review/general.mdc; skills
carry thin references only (code-review/SKILL.md stays under its
5000-token budget). Content-pin tests updated/added in
CodeReviewContentTest.php and SkillsContentTest.php per the
project's content-pin-tests-are-the-contract-enforcement convention.
Addresses argos CR #738 iteration 1: pin the api-review template's
functional-cross-check row (M1), give code-review-bugsnag the same
`assignment conformance:` Summary mirror as the other three wrappers
(M2), scope the api-review light verdict to standalone runs only so
it self-suppresses on an inline sub-lens invocation (Minor 1), and
pin the Critical-fold clause in the canonical rule (Minor 2).
@pekral

pekral commented Jul 16, 2026

Copy link
Copy Markdown
Owner Author

Code Review

Technical review. This comment is the Technical review half of the two-part CR output (@rules/code-review/general.mdc Two-part CR output — Technical & Functional review) — strict rule compliance only. The Functional review (full acceptance-criteria checklist, Goal met: Yes/No) is published separately on the linked-issue comment via @skills/pr-summary/SKILL.md.

Status: Converged — 0 Critical / 0 Moderate (iteration 2, commit 42f8b7a).

All four iteration-1 findings are resolved and no regressions were introduced:

  • M1 (api-review template pin) — the two-part test now reads skills/api-review/templates/review-output.md and pins **API contract matches assignment:**.
  • M2 (code-review-bugsnag drift) — full parity: the bugsnag template's Summary line and header-note now carry the same assignment conformance: mirror as the other three wrappers, and the pin asserts the slot across all four templates.
  • Minor 1 (api-review light verdict render target) — a Render target — standalone runs only clause was added to the rule, the skill, and the template so the light verdict self-suppresses on an inline sub-lens invocation; pinned.
  • Minor 2 (Critical-fold clause) — pinned in rules/code-review/general.mdc.

Consolidated security review (athena, commit 42f8b7a): 0 Critical / 0 Moderate / 0 Minor — documentation / CR-infra prose only, no new sink.


Summary: 0 Critical · 0 Moderate · 0 Minor · 0 Refactoring · assignment conformance: conformant

@pekral
pekral marked this pull request as ready for review July 16, 2026 10:12
@pekral
pekral merged commit 7828b68 into master Jul 16, 2026
1 check passed
@pekral
pekral deleted the fix/cr-skills-two-part-review branch July 16, 2026 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FIX | CR skills

1 participant