From 4cabad8f5781370c72df84b3d2d70aabae3de467 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 11:23:55 +0200 Subject: [PATCH 01/11] [#228] feat: verdict-first code-review template (security + cost + architecture, 30s budget) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the review artifact to be verdict-first (D22, R6.6) with the seven required assessment sections, and align pair-process-review output to it. - code-review-template (dataset + root mirror): verdict-first body — top Verdict block (risk tier + cost class + decision + Classification-changed drift note), then seven 1-line-verdict +
sections: input validation, output handling, authentication, authorization, introduced vulnerabilities, cost, architecture (coupling). "not assessed" is explicit when a capability is absent. Declares it is the native review body, not a separate comment (Q5). ~30s reading budget. - review SKILL (dataset + mirror): compose /assess-cost (Step 2.5) + reference /assess-coupling (Step 2.6, "not assessed" until #263); Phase 5.1 report is verdict-first with Cost + Architecture sections + upward-only "Classification changed" drift note; Step 5.3 submits the report AS the native review body (no separate PR comment, AC2); review-time classification stays a floor (raise-only, D17) with tags re-applied on the PR (AC3 #226); graceful degradation + output format updated. - how-to-11 (dataset + mirror): native-review-body verdict, drop "post report as PR comment". - execution.mdx: document the verdict-first review body + reading budget. - Conformance guard code-review-verdict-first.test.ts (dataset + mirror, name-mapped) — 39 tests. Refs: #228, #226 (AC3 review-side cost + classify floor) Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/pair-process-review/SKILL.md | 78 +++-- .../templates/code-review-template.md | 311 +++++++----------- .../knowledge/how-to/11-how-to-code-review.md | 6 +- .../docs/developer-journey/execution.mdx | 12 +- .../templates/code-review-template.md | 311 +++++++----------- .../knowledge/how-to/11-how-to-code-review.md | 6 +- .../dataset/.skills/process/review/SKILL.md | 78 +++-- .../code-review-verdict-first.test.ts | 177 ++++++++++ 8 files changed, 515 insertions(+), 464 deletions(-) create mode 100644 packages/knowledge-hub/src/conformance/code-review-verdict-first.test.ts diff --git a/.claude/skills/pair-process-review/SKILL.md b/.claude/skills/pair-process-review/SKILL.md index 7275ad7a..8651bb31 100644 --- a/.claude/skills/pair-process-review/SKILL.md +++ b/.claude/skills/pair-process-review/SKILL.md @@ -19,11 +19,13 @@ Review a pull request through 6 sequential phases (5 review + 1 optional merge). | `/pair-capability-record-decision` | Capability | Yes | Any | Record missing ADR (HALT condition) | | `/pair-capability-analyze-debt` | Capability | Yes | 4 | Flag tech debt items | | `/pair-capability-assess-security` | Capability | Yes † | 2 | Security posture verdict + findings (D22) | +| `/pair-capability-assess-cost` | Capability | Yes † | 2 | Cost class verdict + signals (D22) | +| `/pair-capability-assess-coupling` | Capability | Optional | 2 | Architecture/coupling balance verdict (D22) | | `/pair-capability-verify-adoption` | Capability | Optional | 3 | Full adoption compliance | | `/pair-capability-assess-stack` | Capability | Optional | 3 | Tech-stack resolution | | `/pair-capability-execute-manual-tests` | Capability | Optional | 6 | Post-merge release validation (manual tests) | -† **Required _when installed_.** `/pair-capability-classify` and `/pair-capability-assess-security` carry Required = Yes because `/pair-process-review` composes them by default — but both **degrade gracefully**: `/pair-process-review` **warns and continues** when the skill is absent (`/pair-capability-classify` → Step 1.5 Skip; `/pair-capability-assess-security` → Step 2.4 / Graceful Degradation), never HALTing on their absence. "Required" here means _composed by default_, not _a hard prerequisite_, so the flag never contradicts the graceful-skip steps. +† **Required _when installed_.** `/pair-capability-classify`, `/pair-capability-assess-security` and `/pair-capability-assess-cost` carry Required = Yes because `/pair-process-review` composes them by default — but all **degrade gracefully**: `/pair-process-review` **warns and continues** when the skill is absent (`/pair-capability-classify` → Step 1.5 Skip; `/pair-capability-assess-security` → Step 2.4; `/pair-capability-assess-cost` → Step 2.5; each also under Graceful Degradation), the affected section reading **not assessed**, never HALTing on their absence. `/pair-capability-assess-coupling` (#263) is Optional — until it ships the Architecture (Coupling) section reads **not assessed**. "Required" here means _composed by default_, not _a hard prerequisite_, so the flag never contradicts the graceful-skip steps. ## Arguments @@ -98,7 +100,7 @@ Ask: _"Proceed with review?"_ 1. **Check**: Has `/pair-capability-classify` already run with `$context: review` on the current PR head commit? 2. **Skip**: If already run — reuse the matrix + tier, move to Phase 2. If `/pair-capability-classify` is not installed → warn (`/pair-capability-classify not installed — no review-time risk matrix`) and move to Phase 2. -3. **Act**: Compose `/pair-capability-classify` with `$context: review` against the PR diff. It applies the [quality model](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) to the diff footprint, reads the story's refinement-time tier, and produces the review matrix as a **floor** — it confirms or **raises** the tier, and **never lowers** it (D17). The Security relevance and Coupling balance dimensions are reconciled in Phase 2 (Step 2.4) as `/pair-capability-assess-security` / `/pair-capability-assess-coupling` verdicts land — raise-only. +3. **Act**: Compose `/pair-capability-classify` with `$context: review` against the PR diff. It applies the [quality model](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) to the diff footprint, reads the story's refinement-time tier, and produces the review matrix as a **floor** — it confirms or **raises** the tier, and **never lowers** it (D17). The Security relevance and Coupling balance dimensions are reconciled in Phase 2 (Steps 2.4 and 2.6) as `/pair-capability-assess-security` / `/pair-capability-assess-coupling` verdicts land — raise-only. 4. **Verify**: The review matrix + `risk:*` tier are recorded on the PR (matrix in the body as 1 line + `
`, D22; tags applied only when a `## Tag Projection` is declared). This body matrix is the single rendered artifact — if Phase 2 raises Security relevance or Coupling balance, `/pair-process-review` updates it **in place** (Step 2.4), it is not re-emitted by `/pair-capability-classify`. A raise to `risk:red` is carried into the Step 5.2 decision. `/pair-capability-classify` HALTs only if the quality model doc (#221) is absent. ## Phase 2: Technical Review @@ -137,9 +139,25 @@ Ask: _"Proceed with review?"_ ### Step 2.4: Security Assessment 1. **Check**: Has `/pair-capability-assess-security` already run with `$mode: review` on the current PR head commit? -2. **Skip**: If already run — reuse the verdict + findings, move to Phase 3. -3. **Act**: Compose `/pair-capability-assess-security` with `$mode: review` against the PR diff. It resolves the rule set (KB global + per-service + per-web-app + adoption project rules) and returns a 1-line verdict + collapsed findings, each tagged **introduced** or **pre-existing**. -4. **Verify**: Verdict + findings recorded — feeds the Security Review section (Step 5.1) and the **Security relevance** dimension of the Step 1.5 classification matrix (`/pair-capability-classify` folds this verdict in **raise-only** — it may raise the tier, never lower it). **Body re-render**: when the verdict raises Security relevance (or the Coupling verdict raises Coupling balance), `/pair-process-review` updates the already-written Step 1.5 body matrix **in place** — re-rendering the affected `
` row and the 1-line `risk:*` tier so the PR body reflects the final, raised tier; `/pair-capability-classify` is **not** re-invoked (its Phase-1 run stands, and a raise-only edit needs no recompute). **Tag re-apply**: when a `## Tag Projection` is declared (e.g. `Active: risk`), this in-place Phase-2-originated raise **also re-applies the projected chromatic tag on the PR** — swapping the stale label for the raised tier (e.g. `risk:yellow` → `risk:red`) via the same §5 projection `/pair-capability-classify` uses in its Step 5, applied here by `/pair-process-review` on the raise-only edit — so the PR label matches the raised body tier (AC3); when no projection is declared, only the body matrix is updated and no label is touched. If any **introduced** finding is red → flag explicitly: this is the AC4 signal that drives the CHANGES-REQUESTED decision in Step 5.2. Does not itself HALT — `/pair-capability-assess-security` has no merge authority, this skill's own decision step does. +2. **Skip**: If already run — reuse the verdict + findings, move to Step 2.5. +3. **Act**: Compose `/pair-capability-assess-security` with `$mode: review` against the PR diff. It resolves the rule set (KB global + per-service + per-web-app + adoption project rules) and returns a 1-line verdict + collapsed findings, each tagged **introduced** or **pre-existing**. The verdict feeds the five verdict-first Security sections of the review body — **input validation, output handling, authentication, authorization, introduced vulnerabilities** (Step 5.1, per the [code-review-template](../../../.pair/knowledge/guidelines/collaboration/templates/code-review-template.md)). +4. **Verify**: Verdict + findings recorded — feeds the Security sections (Step 5.1) and the **Security relevance** dimension of the Step 1.5 classification matrix (`/pair-capability-classify` folds this verdict in **raise-only** — it may raise the tier, never lower it). **Body re-render**: when the verdict raises Security relevance (or the Coupling verdict raises Coupling balance), `/pair-process-review` updates the already-written Step 1.5 body matrix **in place** — re-rendering the affected `
` row and the 1-line `risk:*` tier so the PR body reflects the final, raised tier; `/pair-capability-classify` is **not** re-invoked (its Phase-1 run stands, and a raise-only edit needs no recompute). **Tag re-apply**: when a `## Tag Projection` is declared (e.g. `Active: risk`), this in-place Phase-2-originated raise **also re-applies the projected chromatic tag on the PR** — swapping the stale label for the raised tier (e.g. `risk:yellow` → `risk:red`) via the same §5 projection `/pair-capability-classify` uses in its Step 5, applied here by `/pair-process-review` on the raise-only edit — so the PR label matches the raised body tier (AC3); when no projection is declared, only the body matrix is updated and no label is touched. If any **introduced** finding is red → flag explicitly: this is the AC4 signal that drives the CHANGES-REQUESTED decision in Step 5.2. Does not itself HALT — `/pair-capability-assess-security` has no merge authority, this skill's own decision step does. +5. **Degrade**: `/pair-capability-assess-security` not installed → the five Security sections read **not assessed** (never dropped); a manual security read of the diff is still expected. + +### Step 2.5: Cost Assessment + +1. **Check**: Has `/pair-capability-assess-cost` already run against the current PR head commit? +2. **Skip**: If already run — reuse the class + signals, move to Step 2.6. +3. **Act**: Compose `/pair-capability-assess-cost` against the PR diff. It resolves the cost-signal catalog from the project's stack/architecture/infrastructure adoption and returns the `cost:green|yellow|orange|red` class as a 1-line verdict + collapsed signals table (D22). This is a genuine re-assessment of the diff — never a restatement of the story's refinement-time `cost:*` tag. +4. **Verify**: Class + signals recorded — feed the **Cost** section of the review body (Step 5.1, 1 line + `
`). The `cost:*` class is carried as its own dimension of the matrix (never folded into the risk `max`). A **red** cost class surfaces the **blocking human sign-off** requirement in the Verdict area (Step 5.1) and is carried into the Step 5.2 decision. Tag re-apply on the PR follows the same rule as Step 2.4 when `cost` is in the declared `## Tag Projection`. Does not itself HALT — `/pair-capability-assess-cost` has no merge authority. +5. **Degrade**: `/pair-capability-assess-cost` not installed → the Cost section reads **not assessed** (never dropped). + +### Step 2.6: Architecture (Coupling) Assessment + +1. **Check**: Is `/pair-capability-assess-coupling` installed (ships with #263)? +2. **Skip**: If not installed → the **Architecture (Coupling)** section reads **not assessed** explicitly; move to Phase 3. +3. **Act**: Compose `/pair-capability-assess-coupling` with `$scope: diff`. It returns a 1-line balance verdict on the integrations the diff touches (integration strength, socio-technical distance, volatility) + collapsed findings (D22). +4. **Verify**: Verdict + findings recorded — feed the **Architecture (Coupling)** section of the review body (Step 5.1) and the **Coupling balance** dimension of the Step 1.5 matrix (`/pair-capability-classify` folds it in **raise-only**, same in-place body re-render + tag re-apply rule as Step 2.4). Does not itself HALT. ## Phase 3: Adoption Compliance @@ -191,17 +209,13 @@ Run the procedure for the level determined in Step 3.1 — see [degradation-leve ### Step 5.1: Compile Review Report -1. **Act**: Compile all findings into a review report following the [code-review-template.md](../../../.pair/knowledge/guidelines/collaboration/templates/code-review-template.md) (resolve override-first — [template resolution](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/template-resolution.md)): - - **Review Information**: PR number, author, reviewer, date, story, review type - - **Review Summary**: overall assessment, key changes, business value - - **Code Review Checklist**: functionality, code quality, technical standards (from Phase 2) - - **Security Review**: verdict + collapsed findings from `/pair-capability-assess-security` (Step 2.4) — 1-line + `
` (D22) - - **Testing Review**: test coverage and quality (from /pair-capability-verify-quality) - - **Documentation Review**: documentation completeness (from /pair-capability-verify-done) - - **Detailed Review Comments**: issues by severity, positive feedback - - **Risk Assessment**: technical and business risks - - **Tech Debt**: items flagged by /pair-capability-analyze-debt - - **Adoption Compliance**: results from Phase 3 (with degradation level noted) +1. **Act**: Compile all findings into a **verdict-first** review body following the [code-review-template.md](../../../.pair/knowledge/guidelines/collaboration/templates/code-review-template.md) (resolve override-first — [template resolution](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/template-resolution.md)). The body is ordered so verdict, tier and cost class read in **~30 seconds** (D22, R6.6): + - **Verdict** (top): classification tags (`risk:` · `cost:`) + the decision + a 1-line reason + PR/author/story metadata. Include the **`Classification changed:`** drift note **only** when the review-time tier/cost differs from the story's refinement-time classification — it fires **upward only** (e.g. `risk:yellow` → `risk:red`, raise-only per quality-model §3.2 / D17); a review that would lower a dimension records the reduction as a finding in the collapsed details, never as a silent downgrade. A **red** cost class states the **blocking human sign-off** requirement here. + - **Assessments** (each a 1-line verdict + `
`, "not assessed" when the capability is absent): + - **Security — input validation, output handling, authentication, authorization, introduced vulnerabilities** (five verdicts from `/pair-capability-assess-security`, Step 2.4) + - **Cost** — `cost:*` class + signals (from `/pair-capability-assess-cost`, Step 2.5) + - **Architecture (Coupling)** — balance verdict (from `/pair-capability-assess-coupling`, Step 2.6; "not assessed" until #263) + - **Details** (collapsed): findings by severity + positive feedback (Phase 2); functionality / AC coverage; testing & quality gates (from /pair-capability-verify-quality); adoption compliance with degradation level (Phase 3); tech debt (from /pair-capability-analyze-debt); documentation (from /pair-capability-verify-done). ### Step 5.2: Make Review Decision @@ -210,18 +224,20 @@ Based on compiled findings: | Decision | Condition | | --------------------- | ----------------------------------------------------------------------------------------- | | **APPROVED** | No critical or major issues. All AC met. Quality gates pass. | -| **CHANGES-REQUESTED** | Critical issues found, missing ADRs, any **introduced** red security finding from `/pair-capability-assess-security` (AC4), failing tests, AC not met. | +| **CHANGES-REQUESTED** | Critical issues found, missing ADRs, any **introduced** red security finding from `/pair-capability-assess-security` (AC4), failing tests, AC not met. A **red** `cost:*` class does not itself block — it surfaces a **blocking human sign-off** requirement in the Verdict (the human, not the skill, gates on cost). | | **TECH-DEBT** | Only minor issues or debt items. Approve current PR, track debt separately. | -### Step 5.3: Post Review +### Step 5.3: Submit Review + +The compiled report **is the body of the native GitHub review** — the verdict is the review action; there is **no separate PR comment** (decision Q5, AC2). -1. **Act**: Post the review report as a PR comment. -2. **Act**: Set PR review status using the PM tool (per [github-implementation.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md)): - - **APPROVED / TECH-DEBT**: Submit review with `event = APPROVE`. - - **CHANGES-REQUESTED**: Submit review with `event = REQUEST_CHANGES`. - - MCP-first: use `pull_request_review_write` with `method = create` and appropriate `event`. - - CLI fallback: `gh pr review --approve` or `--request-changes`. -3. **Verify**: Review posted and status updated. +1. **Act**: Submit the native review using the PM tool (per [github-implementation.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md)), passing the compiled verdict-first report as the review **body**: + - **APPROVED / TECH-DEBT**: `event = APPROVE`. + - **CHANGES-REQUESTED**: `event = REQUEST_CHANGES`. + - MCP-first: `pull_request_review_write` with `method = create`, the report as `body`, and the appropriate `event`. + - CLI fallback: `gh pr review --approve|--request-changes --body-file `. +2. **Act**: On re-review, **edit the existing review body in place** rather than posting a duplicate (idempotency). +3. **Verify**: The native review is submitted with the verdict-first body — no separate review-comment artifact exists. ### Step 5.4: Determine Next Action @@ -249,12 +265,14 @@ REVIEW COMPLETE: ├── Story: [#ID: Title | N/A] ├── Decision: [APPROVED | CHANGES-REQUESTED | TECH-DEBT] ├── Issues: [critical: N | major: N | minor: N] -├── Security: [green | yellow | red — N findings, N introduced | N/A — not installed] +├── Security: [green | yellow | red — N findings, N introduced | not assessed] +├── Cost: [green | yellow | orange | red | not assessed] +├── Coupling: [green | yellow | red | not assessed — until #263] ├── Quality: [PASS | FAIL — N gates] ├── DoD: [N/N criteria met] ├── Adoption: [Level N — summary] ├── Debt: [N items flagged] -└── Report: [Posted as PR comment] +└── Review: [Submitted as native review body — no separate comment] ``` At merge (Phase 6): see [merge-and-cascade.md](./merge-and-cascade.md). @@ -287,7 +305,9 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar - **/verify-adoption not installed**: Falls back to inline dependency checking against [tech-stack.md](../../../.pair/adoption/tech/tech-stack.md). Warning logged. See degradation cascade (Phase 3). - **/assess-stack not installed**: Unlisted dependencies flagged as warnings for manual verification. Does NOT HALT. - **/analyze-debt not available**: Skip debt assessment, note in report. -- **/assess-security not installed**: Skip Step 2.4. Security Review section notes "manual judgment only — /pair-capability-assess-security not installed". Does NOT HALT; a manual security read of the diff is still expected per [how-to-11](../../../.pair/knowledge/how-to/11-how-to-code-review.md). +- **/assess-security not installed**: Skip Step 2.4. The five Security sections (input validation, output handling, authentication, authorization, introduced vulnerabilities) read **not assessed** — never dropped. Does NOT HALT; a manual security read of the diff is still expected per [how-to-11](../../../.pair/knowledge/how-to/11-how-to-code-review.md). +- **/assess-cost not installed**: Skip Step 2.5. The Cost section reads **not assessed**. Does NOT HALT. +- **/assess-coupling not installed** (until #263): Skip Step 2.6. The Architecture (Coupling) section reads **not assessed**. Does NOT HALT. - **Story not found**: Review proceeds with PR-only validation (no AC check). Phase 6 skips parent cascade. - **Code review template not found**: **HALT** — cannot produce review without template (a required dependency, not optional). - **PM tool not accessible**: Phase 6 merge via CLI only. @@ -297,7 +317,7 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar ## Notes -- This skill **reads code, posts review comments, and optionally merges PRs** — it does not modify source code. +- This skill **reads code, submits the native review (verdict = the review action), and optionally merges PRs** — it does not modify source code and posts no separate review comment (AC2). - Review phases are sequential — each phase builds on findings from prior phases. - The reviewer can stop between phases; re-invoke to resume (see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md)). - Output follows [code-review-template.md](../../../.pair/knowledge/guidelines/collaboration/templates/code-review-template.md) — the template defines structure, /pair-process-review fills it with findings. diff --git a/.pair/knowledge/guidelines/collaboration/templates/code-review-template.md b/.pair/knowledge/guidelines/collaboration/templates/code-review-template.md index 7cacec41..84d53161 100644 --- a/.pair/knowledge/guidelines/collaboration/templates/code-review-template.md +++ b/.pair/knowledge/guidelines/collaboration/templates/code-review-template.md @@ -1,279 +1,192 @@ # Code Review Template -## Review Information +> **Verdict-first (D22, R6.6).** This is the **native GitHub review body** submitted with the review action (Approve / Request Changes / Comment) — it is **not a separate PR comment** (decision Q5). The top of the body — classification tags, tier, cost class and the 1-line verdict — reads in **~30 seconds**; every assessment is a 1-line verdict with the breakdown collapsed in `
`. A reader scanning the top understands verdict, tier and cost class inside the 30-second reading budget without opening a single `
`. +> +> **Not assessed is explicit.** When the backing capability is missing or failed (`/assess-security`, `/assess-cost`, `/assess-coupling`), the section verdict reads **not assessed** — never silently omitted. -**PR Number:** [#XXX] -**Author:** [Developer name] -**Reviewer:** [Your name] -**Review Date:** [YYYY-MM-DD] -**Story/Epic:** [US-XXX or EP-XXX] -**Review Type:** [Feature/Bug Fix/Refactor/Hotfix] -**Estimated Review Time:** [XX minutes] +## Verdict -## Review Summary +`risk:` · `cost:` — **** — -### Overall Assessment + +> **Classification changed:** `risk:yellow` → `risk:red` — -- [ ] **Approved** - Ready to merge -- [ ] **Approved with Comments** - Minor issues noted, can merge -- [ ] **Request Changes** - Issues must be addressed before merge -- [ ] **Comment Only** - Feedback provided, no blocking issues +**PR:** [#XXX] · **Author:** [name] · **Reviewer:** [name] · **Date:** [YYYY-MM-DD] · **Story:** [US-XXX] · **Type:** [feature | bug | refactor | docs | config] -### Key Changes Summary +
+Classification matrix — per dimension -[Brief summary of what was reviewed and the main changes] +| Dimension | Tier | Source | Note | +| --------------------------- | ------- | -------------------------- | ----------------- | +| Service/domain criticality | [color] | [KB default / table] | [note] | +| Change/diff risk | [color] | [diff footprint] | [note] | +| Business impact | [color] | [subdomain class] | [note] | +| Security relevance | [color] | `/assess-security` | [raise-only, D17] | +| Coupling balance | [color] | `/assess-coupling` | [not assessed until #263] | -### Business Value Validation +Tier = max(assessed). Cost = highest detected signal. Review value is a **floor** (D17): confirm or raise, never lower. -[Confirm the changes deliver the expected business value] +
-## Code Review Checklist +## Assessments -### Functionality Review +Each assessment is a **1-line verdict**; open the `
` for the breakdown. An unavailable capability shows **not assessed** (the section is never dropped). -- [ ] **Requirements Met** - Implementation matches acceptance criteria -- [ ] **Business Logic** - Logic is correct and handles edge cases -- [ ] **User Experience** - Changes improve or maintain user experience -- [ ] **Integration** - Works correctly with existing systems -- [ ] **Error Handling** - Appropriate error handling and recovery -- [ ] **Performance** - No performance regressions introduced +### Security — Input validation -### Code Quality Assessment +**Verdict:** [green | yellow | red | not assessed] — -- [ ] **Readability** - Code is clear and easy to understand -- [ ] **Maintainability** - Code is organized and well-structured -- [ ] **Reusability** - Common functionality properly abstracted -- [ ] **Naming** - Variables, functions, and classes well-named -- [ ] **Comments** - Appropriate documentation and comments -- [ ] **Complexity** - Code complexity is reasonable and justified +
+Details -### Technical Standards Compliance +- Inputs touched by the diff and whether each is validated / sanitized. +- Feeds from `/assess-security` (`$mode: review`). -- [ ] **Style Guide** - Follows established coding standards -- [ ] **Architecture** - Aligns with system architecture principles -- [ ] **Design Patterns** - Uses appropriate design patterns -- [ ] **Dependencies** - Dependencies are justified and minimal -- [ ] **API Design** - API changes are backward compatible -- [ ] **Database** - Database changes are optimized and indexed +
-## Security Review +### Security — Output handling -### Security Checklist +**Verdict:** [green | yellow | red | not assessed] — -- [ ] **Input Validation** - All user inputs properly validated -- [ ] **Output Encoding** - Data properly encoded for output -- [ ] **Authentication** - Authentication mechanisms properly implemented -- [ ] **Authorization** - Access controls correctly enforced -- [ ] **Data Protection** - Sensitive data properly handled -- [ ] **Dependency Security** - No known security vulnerabilities -- [ ] **Secrets Management** - No hardcoded secrets or credentials -- [ ] **HTTPS/TLS** - Secure communication protocols used +
+Details -### Security Concerns +- Output encoding / escaping on the touched surfaces (XSS, injection). -| Concern | Severity | Description | Recommendation | -| --------- | --------------- | ------------- | -------------- | -| [Issue 1] | High/Medium/Low | [Description] | [How to fix] | -| [Issue 2] | High/Medium/Low | [Description] | [How to fix] | +
-## Testing Review +### Security — Authentication -### Test Coverage Assessment +**Verdict:** [green | yellow | red | not assessed] — -- [ ] **Unit Tests** - Adequate unit test coverage (target: 80%+) -- [ ] **Integration Tests** - Integration scenarios covered -- [ ] **End-to-End Tests** - Critical user journeys tested -- [ ] **Edge Cases** - Boundary conditions and edge cases tested -- [ ] **Error Scenarios** - Error conditions and recovery tested -- [ ] **Performance Tests** - Performance requirements validated +
+Details -### Test Quality Review +- Authentication mechanisms on the changed paths. -- [ ] **Test Clarity** - Tests are clear and well-named -- [ ] **Test Independence** - Tests don't depend on each other -- [ ] **Test Data** - Test data is appropriate and realistic -- [ ] **Mocking** - External dependencies properly mocked -- [ ] **Assertions** - Assertions are specific and meaningful -- [ ] **Test Organization** - Tests are well-organized and grouped +
-### Testing Feedback +### Security — Authorization -```text -Current Coverage: XX% -New Coverage: XX% -Coverage Change: +/-XX% +**Verdict:** [green | yellow | red | not assessed] — -Test Results: ✅ All Passing / ❌ X Failing -Performance Tests: ✅ Within Limits / ⚠️ Degradation Detected -``` +
+Details -## Performance Review +- Access-control enforcement on the changed paths. -### Performance Analysis +
-- [ ] **Response Time** - API response times within acceptable limits -- [ ] **Memory Usage** - Memory consumption is reasonable -- [ ] **Database Performance** - Database queries are optimized -- [ ] **Caching** - Appropriate caching strategies implemented -- [ ] **Resource Usage** - CPU and I/O usage is efficient -- [ ] **Scalability** - Changes support system scalability +### Security — Introduced vulnerabilities -### Performance Metrics +**Verdict:** [green | yellow | red | not assessed] — -| Metric | Before | After | Change | Acceptable | -| ---------------- | ------ | ----- | ------ | ---------- | -| Response Time | XXXms | XXXms | +/-XX% | ✅/❌ | -| Memory Usage | XXXMb | XXXMb | +/-XX% | ✅/❌ | -| Database Queries | XX | XX | +/-XX | ✅/❌ | +
+Details -## Documentation Review +| Severity | Category | File:location | Introduced / pre-existing | Recommendation | +| -------- | -------- | ------------- | ------------------------- | -------------- | +| [P0/P1] | [OWASP] | [file:line] | [introduced] | [fix] | -### Documentation Checklist +Any **introduced** red finding drives **CHANGES-REQUESTED** (AC4). Pre-existing findings are surfaced, not blocking. -- [ ] **Code Comments** - Complex logic appropriately commented -- [ ] **API Documentation** - API changes documented -- [ ] **README Updates** - README reflects new functionality -- [ ] **User Documentation** - User-facing documentation updated -- [ ] **Technical Documentation** - Architecture/design docs updated -- [ ] **Change Log** - Changes documented in CHANGELOG +
-### Documentation Quality +### Cost -- [ ] **Accuracy** - Documentation matches implementation -- [ ] **Completeness** - All new features documented -- [ ] **Clarity** - Documentation is clear and understandable -- [ ] **Examples** - Appropriate examples provided -- [ ] **Up-to-date** - Existing documentation updated +**Verdict:** `cost:` — -## Detailed Review Comments +
+Details -### Positive Feedback +| Signal | Class | Provider | Note | +| ------ | ----- | -------- | ---- | +| [signal] | [color] | [stack] | [note] | -#### What's Done Well: +Feeds from `/assess-cost` against the diff. A **red** cost class surfaces a **blocking human sign-off** requirement in the Verdict area. Capability absent → not assessed. -- [Specific positive observations about code quality, approach, or implementation] -- [Recognition of good practices, clever solutions, or improvements] -- [Appreciation for thorough testing or documentation] +
-### Issues to Address +### Architecture (Coupling) -#### Critical Issues ⚠️ +**Verdict:** [green | yellow | red | not assessed] — -#### Must fix before merge: +
+Details -- [ ] **[File:Line]** - [Critical issue description and impact] -- [ ] **[File:Line]** - [Another critical issue requiring resolution] +- Integration strength / socio-technical distance / volatility on the integrations the diff touches. +- Feeds from `/assess-coupling` (`$scope: diff`). Capability absent (until #263) → not assessed. -#### Major Issues 🔍 +
-#### Should fix before merge: +## Details -- [ ] **[File:Line]** - [Major issue description and suggested solution] -- [ ] **[File:Line]** - [Another major issue with improvement recommendation] +
+Findings by severity -#### Minor Issues 💡 +**Critical (must fix before merge)** -#### Consider addressing: +- [ ] **[File:Line]** — [issue + impact] -- [ ] **[File:Line]** - [Minor improvement suggestion] -- [ ] **[File:Line]** - [Code style or best practice recommendation] +**Major (should fix before merge)** -#### Questions ❓ +- [ ] **[File:Line]** — [issue + suggested fix] -#### Clarification needed: +**Minor (consider)** -- [ ] **[File:Line]** - [Question about implementation approach or decision] -- [ ] **[File:Line]** - [Request for explanation of complex logic] +- [ ] **[File:Line]** — [suggestion] -## Suggestions and Improvements +**Questions** -### Code Improvements +- [ ] **[File:Line]** — [clarification] -```diff -[Suggested code change in diff format] -``` +**Positive** -### Architecture Suggestions +- [what's done well] -- [Suggestion for better design pattern or architectural approach] -- [Recommendation for improved separation of concerns] -- [Advice on better abstraction or interface design] +
-### Best Practices +
+Functionality & requirements (AC coverage) -- [Recommendation for following established team practices] -- [Suggestion for improved error handling patterns] -- [Advice on better testing strategies] +- [ ] Acceptance criteria met +- [ ] Business logic + edge cases correct +- [ ] Integrates with existing systems +- [ ] Error handling appropriate -## Risk Assessment +
-### Technical Risks +
+Testing & quality gates -| Risk | Impact | Probability | Mitigation | -| ------------------ | ------------ | ------------ | --------------------- | -| [Technical risk] | High/Med/Low | High/Med/Low | [Mitigation strategy] | -| [Integration risk] | High/Med/Low | High/Med/Low | [Risk reduction plan] | +- [ ] Adequate coverage (unit / integration / e2e as applicable) +- [ ] Edge + error scenarios tested +- [ ] Quality gates: [PASS | FAIL — which] -### Business Risks +
-| Risk | Impact | Probability | Mitigation | -| --------------- | ------------ | ------------ | ---------------------- | -| [Business risk] | High/Med/Low | High/Med/Low | [Business mitigation] | -| [User impact] | High/Med/Low | High/Med/Low | [User experience plan] | +
+Adoption compliance -## Deployment Considerations +- Degradation level: [1–4] +- Dependencies match `tech-stack.md`; patterns match `architecture.md` +- ADRs present for new technical decisions (missing ADR → CHANGES-REQUESTED) -### Deployment Checklist +
-- [ ] **Database Migration** - Migration scripts reviewed and tested -- [ ] **Configuration** - Environment configuration reviewed -- [ ] **Feature Flags** - Feature toggles properly configured -- [ ] **Rollback Plan** - Rollback procedure defined and tested -- [ ] **Monitoring** - Appropriate monitoring and alerting in place -- [ ] **Documentation** - Deployment documentation updated +
+Tech debt -### Post-Deployment Monitoring +- Items flagged by `/analyze-debt` (surfaced, never blocking on debt grounds alone). -- [ ] **Error Rates** - Monitor error rate changes -- [ ] **Performance Metrics** - Track performance indicators -- [ ] **User Behavior** - Monitor user interaction patterns -- [ ] **System Health** - Overall system health monitoring +
-## Follow-up Actions +
+Documentation -### Author Action Items +- [ ] Code / API / user / architecture docs updated as applicable. -- [ ] **[Priority]** - [Action item for the author to complete] -- [ ] **[Priority]** - [Another action item with deadline if applicable] -- [ ] **[Priority]** - [Follow-up task or improvement] - -### Reviewer Follow-up - -- [ ] **Re-review** - Schedule re-review after changes -- [ ] **Pair Programming** - Offer pairing session for complex issues -- [ ] **Knowledge Sharing** - Share relevant resources or documentation - -### Team Actions - -- [ ] **Process Improvement** - Identify process improvements from this review -- [ ] **Standards Update** - Update coding standards based on findings -- [ ] **Training Need** - Identify training opportunities for team - -## Review Timeline - -### Review Process - -- **Review Started:** [YYYY-MM-DD HH:MM] -- **Initial Review Completed:** [YYYY-MM-DD HH:MM] -- **Changes Requested:** [YYYY-MM-DD HH:MM] -- **Changes Made:** [YYYY-MM-DD HH:MM] -- **Final Approval:** [YYYY-MM-DD HH:MM] - -### Review Effort - -- **Time Spent Reviewing:** [XX hours] -- **Complexity Level:** [Low/Medium/High] -- **Review Thoroughness:** [Quick/Standard/Deep] +
--- diff --git a/.pair/knowledge/how-to/11-how-to-code-review.md b/.pair/knowledge/how-to/11-how-to-code-review.md index f6914ba6..c9561ffe 100644 --- a/.pair/knowledge/how-to/11-how-to-code-review.md +++ b/.pair/knowledge/how-to/11-how-to-code-review.md @@ -91,8 +91,8 @@ Follow [project management tool guidelines](../guidelines/collaboration/project- ### Phase 3: Review Report & Decision -1. **Generate review report** using [code-review-template.md](../guidelines/collaboration/templates/code-review-template.md) -2. **Post report as PR comment** with all findings by severity +1. **Generate a verdict-first review report** using [code-review-template.md](../guidelines/collaboration/templates/code-review-template.md) — classification tags + 1-line verdict at the top (~30-second reading budget, D22), assessment sections (security / cost / architecture) collapsed +2. **Submit the report as the native review body** (verdict = the review action: Approve / Request Changes / Comment) — no separate PR comment (Q5) 3. **Set PR status** based on findings: #### Review Decisions: @@ -120,7 +120,7 @@ Follow [project management tool guidelines](../guidelines/collaboration/project- - [ ] ADR and adoption compliance validated for all new technical decisions - [ ] All technical standards verified against adoption guidelines - [ ] Review report generated using [code-review-template.md](../guidelines/collaboration/templates/code-review-template.md) -- [ ] Report posted as PR comment +- [ ] Report submitted as the native review body (verdict-first, no separate comment) - [ ] PR status updated (pending/changes-requested/approved) - [ ] Review decision made diff --git a/apps/website/content/docs/developer-journey/execution.mdx b/apps/website/content/docs/developer-journey/execution.mdx index 118fb4bc..036130d0 100644 --- a/apps/website/content/docs/developer-journey/execution.mdx +++ b/apps/website/content/docs/developer-journey/execution.mdx @@ -74,9 +74,9 @@ Review a pull request through a structured 6-phase process. ### The Review Phases ```text -Phase 1 ── Validation ────── PR structure, linked story, template compliance +Phase 1 ── Validation ────── PR structure, linked story, classification (risk floor) │ -Phase 2 ── Technical ─────── Code quality, architecture match, security +Phase 2 ── Technical ─────── Code quality, security, cost, architecture (coupling) │ Phase 3 ── Adoption ──────── Libraries match tech-stack.md, patterns match architecture.md │ @@ -87,6 +87,14 @@ Phase 5 ── Decision ──────── APPROVED, CHANGES REQUESTED, or Phase 6 ── Merge ─────────── Merge PR, update story/epic/initiative status ``` +### The Verdict-First Review Body + +The review is submitted as the **native GitHub review** (the verdict *is* the Approve / Request Changes / Comment action) — there is no separate review comment. Its body is **verdict-first** so a reader understands the outcome in about 30 seconds (D22): + +- **Top:** classification tags (`risk:` · `cost:`), the decision, and a one-line reason. If the review-time classification differs from the story's refinement-time one, a **Classification changed** line states the change — it only ever fires *upward* (the review value is a floor: it may raise a tier, never lower it). +- **Assessments** (each a one-line verdict, details collapsed): security — input validation, output handling, authentication, authorization, introduced vulnerabilities (from `/pair-capability-assess-security`); **cost** class (from `/pair-capability-assess-cost`); **architecture / coupling** balance (from `/pair-capability-assess-coupling`). +- When a capability is not installed, its section reads **not assessed** — explicit, never silently dropped. + ### ADR Enforcement If the review detects a new library not listed in `tech-stack.md` or an architecture pattern not recorded in `architecture.md`, it **blocks** and requests an Architecture Decision Record before approval. This ensures every technical choice is deliberate and documented. diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/templates/code-review-template.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/templates/code-review-template.md index 7cacec41..84d53161 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/templates/code-review-template.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/templates/code-review-template.md @@ -1,279 +1,192 @@ # Code Review Template -## Review Information +> **Verdict-first (D22, R6.6).** This is the **native GitHub review body** submitted with the review action (Approve / Request Changes / Comment) — it is **not a separate PR comment** (decision Q5). The top of the body — classification tags, tier, cost class and the 1-line verdict — reads in **~30 seconds**; every assessment is a 1-line verdict with the breakdown collapsed in `
`. A reader scanning the top understands verdict, tier and cost class inside the 30-second reading budget without opening a single `
`. +> +> **Not assessed is explicit.** When the backing capability is missing or failed (`/assess-security`, `/assess-cost`, `/assess-coupling`), the section verdict reads **not assessed** — never silently omitted. -**PR Number:** [#XXX] -**Author:** [Developer name] -**Reviewer:** [Your name] -**Review Date:** [YYYY-MM-DD] -**Story/Epic:** [US-XXX or EP-XXX] -**Review Type:** [Feature/Bug Fix/Refactor/Hotfix] -**Estimated Review Time:** [XX minutes] +## Verdict -## Review Summary +`risk:` · `cost:` — **** — -### Overall Assessment + +> **Classification changed:** `risk:yellow` → `risk:red` — -- [ ] **Approved** - Ready to merge -- [ ] **Approved with Comments** - Minor issues noted, can merge -- [ ] **Request Changes** - Issues must be addressed before merge -- [ ] **Comment Only** - Feedback provided, no blocking issues +**PR:** [#XXX] · **Author:** [name] · **Reviewer:** [name] · **Date:** [YYYY-MM-DD] · **Story:** [US-XXX] · **Type:** [feature | bug | refactor | docs | config] -### Key Changes Summary +
+Classification matrix — per dimension -[Brief summary of what was reviewed and the main changes] +| Dimension | Tier | Source | Note | +| --------------------------- | ------- | -------------------------- | ----------------- | +| Service/domain criticality | [color] | [KB default / table] | [note] | +| Change/diff risk | [color] | [diff footprint] | [note] | +| Business impact | [color] | [subdomain class] | [note] | +| Security relevance | [color] | `/assess-security` | [raise-only, D17] | +| Coupling balance | [color] | `/assess-coupling` | [not assessed until #263] | -### Business Value Validation +Tier = max(assessed). Cost = highest detected signal. Review value is a **floor** (D17): confirm or raise, never lower. -[Confirm the changes deliver the expected business value] +
-## Code Review Checklist +## Assessments -### Functionality Review +Each assessment is a **1-line verdict**; open the `
` for the breakdown. An unavailable capability shows **not assessed** (the section is never dropped). -- [ ] **Requirements Met** - Implementation matches acceptance criteria -- [ ] **Business Logic** - Logic is correct and handles edge cases -- [ ] **User Experience** - Changes improve or maintain user experience -- [ ] **Integration** - Works correctly with existing systems -- [ ] **Error Handling** - Appropriate error handling and recovery -- [ ] **Performance** - No performance regressions introduced +### Security — Input validation -### Code Quality Assessment +**Verdict:** [green | yellow | red | not assessed] — -- [ ] **Readability** - Code is clear and easy to understand -- [ ] **Maintainability** - Code is organized and well-structured -- [ ] **Reusability** - Common functionality properly abstracted -- [ ] **Naming** - Variables, functions, and classes well-named -- [ ] **Comments** - Appropriate documentation and comments -- [ ] **Complexity** - Code complexity is reasonable and justified +
+Details -### Technical Standards Compliance +- Inputs touched by the diff and whether each is validated / sanitized. +- Feeds from `/assess-security` (`$mode: review`). -- [ ] **Style Guide** - Follows established coding standards -- [ ] **Architecture** - Aligns with system architecture principles -- [ ] **Design Patterns** - Uses appropriate design patterns -- [ ] **Dependencies** - Dependencies are justified and minimal -- [ ] **API Design** - API changes are backward compatible -- [ ] **Database** - Database changes are optimized and indexed +
-## Security Review +### Security — Output handling -### Security Checklist +**Verdict:** [green | yellow | red | not assessed] — -- [ ] **Input Validation** - All user inputs properly validated -- [ ] **Output Encoding** - Data properly encoded for output -- [ ] **Authentication** - Authentication mechanisms properly implemented -- [ ] **Authorization** - Access controls correctly enforced -- [ ] **Data Protection** - Sensitive data properly handled -- [ ] **Dependency Security** - No known security vulnerabilities -- [ ] **Secrets Management** - No hardcoded secrets or credentials -- [ ] **HTTPS/TLS** - Secure communication protocols used +
+Details -### Security Concerns +- Output encoding / escaping on the touched surfaces (XSS, injection). -| Concern | Severity | Description | Recommendation | -| --------- | --------------- | ------------- | -------------- | -| [Issue 1] | High/Medium/Low | [Description] | [How to fix] | -| [Issue 2] | High/Medium/Low | [Description] | [How to fix] | +
-## Testing Review +### Security — Authentication -### Test Coverage Assessment +**Verdict:** [green | yellow | red | not assessed] — -- [ ] **Unit Tests** - Adequate unit test coverage (target: 80%+) -- [ ] **Integration Tests** - Integration scenarios covered -- [ ] **End-to-End Tests** - Critical user journeys tested -- [ ] **Edge Cases** - Boundary conditions and edge cases tested -- [ ] **Error Scenarios** - Error conditions and recovery tested -- [ ] **Performance Tests** - Performance requirements validated +
+Details -### Test Quality Review +- Authentication mechanisms on the changed paths. -- [ ] **Test Clarity** - Tests are clear and well-named -- [ ] **Test Independence** - Tests don't depend on each other -- [ ] **Test Data** - Test data is appropriate and realistic -- [ ] **Mocking** - External dependencies properly mocked -- [ ] **Assertions** - Assertions are specific and meaningful -- [ ] **Test Organization** - Tests are well-organized and grouped +
-### Testing Feedback +### Security — Authorization -```text -Current Coverage: XX% -New Coverage: XX% -Coverage Change: +/-XX% +**Verdict:** [green | yellow | red | not assessed] — -Test Results: ✅ All Passing / ❌ X Failing -Performance Tests: ✅ Within Limits / ⚠️ Degradation Detected -``` +
+Details -## Performance Review +- Access-control enforcement on the changed paths. -### Performance Analysis +
-- [ ] **Response Time** - API response times within acceptable limits -- [ ] **Memory Usage** - Memory consumption is reasonable -- [ ] **Database Performance** - Database queries are optimized -- [ ] **Caching** - Appropriate caching strategies implemented -- [ ] **Resource Usage** - CPU and I/O usage is efficient -- [ ] **Scalability** - Changes support system scalability +### Security — Introduced vulnerabilities -### Performance Metrics +**Verdict:** [green | yellow | red | not assessed] — -| Metric | Before | After | Change | Acceptable | -| ---------------- | ------ | ----- | ------ | ---------- | -| Response Time | XXXms | XXXms | +/-XX% | ✅/❌ | -| Memory Usage | XXXMb | XXXMb | +/-XX% | ✅/❌ | -| Database Queries | XX | XX | +/-XX | ✅/❌ | +
+Details -## Documentation Review +| Severity | Category | File:location | Introduced / pre-existing | Recommendation | +| -------- | -------- | ------------- | ------------------------- | -------------- | +| [P0/P1] | [OWASP] | [file:line] | [introduced] | [fix] | -### Documentation Checklist +Any **introduced** red finding drives **CHANGES-REQUESTED** (AC4). Pre-existing findings are surfaced, not blocking. -- [ ] **Code Comments** - Complex logic appropriately commented -- [ ] **API Documentation** - API changes documented -- [ ] **README Updates** - README reflects new functionality -- [ ] **User Documentation** - User-facing documentation updated -- [ ] **Technical Documentation** - Architecture/design docs updated -- [ ] **Change Log** - Changes documented in CHANGELOG +
-### Documentation Quality +### Cost -- [ ] **Accuracy** - Documentation matches implementation -- [ ] **Completeness** - All new features documented -- [ ] **Clarity** - Documentation is clear and understandable -- [ ] **Examples** - Appropriate examples provided -- [ ] **Up-to-date** - Existing documentation updated +**Verdict:** `cost:` — -## Detailed Review Comments +
+Details -### Positive Feedback +| Signal | Class | Provider | Note | +| ------ | ----- | -------- | ---- | +| [signal] | [color] | [stack] | [note] | -#### What's Done Well: +Feeds from `/assess-cost` against the diff. A **red** cost class surfaces a **blocking human sign-off** requirement in the Verdict area. Capability absent → not assessed. -- [Specific positive observations about code quality, approach, or implementation] -- [Recognition of good practices, clever solutions, or improvements] -- [Appreciation for thorough testing or documentation] +
-### Issues to Address +### Architecture (Coupling) -#### Critical Issues ⚠️ +**Verdict:** [green | yellow | red | not assessed] — -#### Must fix before merge: +
+Details -- [ ] **[File:Line]** - [Critical issue description and impact] -- [ ] **[File:Line]** - [Another critical issue requiring resolution] +- Integration strength / socio-technical distance / volatility on the integrations the diff touches. +- Feeds from `/assess-coupling` (`$scope: diff`). Capability absent (until #263) → not assessed. -#### Major Issues 🔍 +
-#### Should fix before merge: +## Details -- [ ] **[File:Line]** - [Major issue description and suggested solution] -- [ ] **[File:Line]** - [Another major issue with improvement recommendation] +
+Findings by severity -#### Minor Issues 💡 +**Critical (must fix before merge)** -#### Consider addressing: +- [ ] **[File:Line]** — [issue + impact] -- [ ] **[File:Line]** - [Minor improvement suggestion] -- [ ] **[File:Line]** - [Code style or best practice recommendation] +**Major (should fix before merge)** -#### Questions ❓ +- [ ] **[File:Line]** — [issue + suggested fix] -#### Clarification needed: +**Minor (consider)** -- [ ] **[File:Line]** - [Question about implementation approach or decision] -- [ ] **[File:Line]** - [Request for explanation of complex logic] +- [ ] **[File:Line]** — [suggestion] -## Suggestions and Improvements +**Questions** -### Code Improvements +- [ ] **[File:Line]** — [clarification] -```diff -[Suggested code change in diff format] -``` +**Positive** -### Architecture Suggestions +- [what's done well] -- [Suggestion for better design pattern or architectural approach] -- [Recommendation for improved separation of concerns] -- [Advice on better abstraction or interface design] +
-### Best Practices +
+Functionality & requirements (AC coverage) -- [Recommendation for following established team practices] -- [Suggestion for improved error handling patterns] -- [Advice on better testing strategies] +- [ ] Acceptance criteria met +- [ ] Business logic + edge cases correct +- [ ] Integrates with existing systems +- [ ] Error handling appropriate -## Risk Assessment +
-### Technical Risks +
+Testing & quality gates -| Risk | Impact | Probability | Mitigation | -| ------------------ | ------------ | ------------ | --------------------- | -| [Technical risk] | High/Med/Low | High/Med/Low | [Mitigation strategy] | -| [Integration risk] | High/Med/Low | High/Med/Low | [Risk reduction plan] | +- [ ] Adequate coverage (unit / integration / e2e as applicable) +- [ ] Edge + error scenarios tested +- [ ] Quality gates: [PASS | FAIL — which] -### Business Risks +
-| Risk | Impact | Probability | Mitigation | -| --------------- | ------------ | ------------ | ---------------------- | -| [Business risk] | High/Med/Low | High/Med/Low | [Business mitigation] | -| [User impact] | High/Med/Low | High/Med/Low | [User experience plan] | +
+Adoption compliance -## Deployment Considerations +- Degradation level: [1–4] +- Dependencies match `tech-stack.md`; patterns match `architecture.md` +- ADRs present for new technical decisions (missing ADR → CHANGES-REQUESTED) -### Deployment Checklist +
-- [ ] **Database Migration** - Migration scripts reviewed and tested -- [ ] **Configuration** - Environment configuration reviewed -- [ ] **Feature Flags** - Feature toggles properly configured -- [ ] **Rollback Plan** - Rollback procedure defined and tested -- [ ] **Monitoring** - Appropriate monitoring and alerting in place -- [ ] **Documentation** - Deployment documentation updated +
+Tech debt -### Post-Deployment Monitoring +- Items flagged by `/analyze-debt` (surfaced, never blocking on debt grounds alone). -- [ ] **Error Rates** - Monitor error rate changes -- [ ] **Performance Metrics** - Track performance indicators -- [ ] **User Behavior** - Monitor user interaction patterns -- [ ] **System Health** - Overall system health monitoring +
-## Follow-up Actions +
+Documentation -### Author Action Items +- [ ] Code / API / user / architecture docs updated as applicable. -- [ ] **[Priority]** - [Action item for the author to complete] -- [ ] **[Priority]** - [Another action item with deadline if applicable] -- [ ] **[Priority]** - [Follow-up task or improvement] - -### Reviewer Follow-up - -- [ ] **Re-review** - Schedule re-review after changes -- [ ] **Pair Programming** - Offer pairing session for complex issues -- [ ] **Knowledge Sharing** - Share relevant resources or documentation - -### Team Actions - -- [ ] **Process Improvement** - Identify process improvements from this review -- [ ] **Standards Update** - Update coding standards based on findings -- [ ] **Training Need** - Identify training opportunities for team - -## Review Timeline - -### Review Process - -- **Review Started:** [YYYY-MM-DD HH:MM] -- **Initial Review Completed:** [YYYY-MM-DD HH:MM] -- **Changes Requested:** [YYYY-MM-DD HH:MM] -- **Changes Made:** [YYYY-MM-DD HH:MM] -- **Final Approval:** [YYYY-MM-DD HH:MM] - -### Review Effort - -- **Time Spent Reviewing:** [XX hours] -- **Complexity Level:** [Low/Medium/High] -- **Review Thoroughness:** [Quick/Standard/Deep] +
--- diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/how-to/11-how-to-code-review.md b/packages/knowledge-hub/dataset/.pair/knowledge/how-to/11-how-to-code-review.md index a1c1f3c8..de5cc305 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/how-to/11-how-to-code-review.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/how-to/11-how-to-code-review.md @@ -91,8 +91,8 @@ Follow [project management tool guidelines](../guidelines/collaboration/project- ### Phase 3: Review Report & Decision -1. **Generate review report** using [code-review-template.md](../guidelines/collaboration/templates/code-review-template.md) -2. **Post report as PR comment** with all findings by severity +1. **Generate a verdict-first review report** using [code-review-template.md](../guidelines/collaboration/templates/code-review-template.md) — classification tags + 1-line verdict at the top (~30-second reading budget, D22), assessment sections (security / cost / architecture) collapsed +2. **Submit the report as the native review body** (verdict = the review action: Approve / Request Changes / Comment) — no separate PR comment (Q5) 3. **Set PR status** based on findings: #### Review Decisions: @@ -120,7 +120,7 @@ Follow [project management tool guidelines](../guidelines/collaboration/project- - [ ] ADR and adoption compliance validated for all new technical decisions - [ ] All technical standards verified against adoption guidelines - [ ] Review report generated using [code-review-template.md](../guidelines/collaboration/templates/code-review-template.md) -- [ ] Report posted as PR comment +- [ ] Report submitted as the native review body (verdict-first, no separate comment) - [ ] PR status updated (pending/changes-requested/approved) - [ ] Review decision made diff --git a/packages/knowledge-hub/dataset/.skills/process/review/SKILL.md b/packages/knowledge-hub/dataset/.skills/process/review/SKILL.md index a4425d85..ffd572c5 100644 --- a/packages/knowledge-hub/dataset/.skills/process/review/SKILL.md +++ b/packages/knowledge-hub/dataset/.skills/process/review/SKILL.md @@ -19,11 +19,13 @@ Review a pull request through 6 sequential phases (5 review + 1 optional merge). | `/record-decision` | Capability | Yes | Any | Record missing ADR (HALT condition) | | `/analyze-debt` | Capability | Yes | 4 | Flag tech debt items | | `/assess-security` | Capability | Yes † | 2 | Security posture verdict + findings (D22) | +| `/assess-cost` | Capability | Yes † | 2 | Cost class verdict + signals (D22) | +| `/assess-coupling` | Capability | Optional | 2 | Architecture/coupling balance verdict (D22) | | `/verify-adoption` | Capability | Optional | 3 | Full adoption compliance | | `/assess-stack` | Capability | Optional | 3 | Tech-stack resolution | | `/execute-manual-tests` | Capability | Optional | 6 | Post-merge release validation (manual tests) | -† **Required _when installed_.** `/classify` and `/assess-security` carry Required = Yes because `/review` composes them by default — but both **degrade gracefully**: `/review` **warns and continues** when the skill is absent (`/classify` → Step 1.5 Skip; `/assess-security` → Step 2.4 / Graceful Degradation), never HALTing on their absence. "Required" here means _composed by default_, not _a hard prerequisite_, so the flag never contradicts the graceful-skip steps. +† **Required _when installed_.** `/classify`, `/assess-security` and `/assess-cost` carry Required = Yes because `/review` composes them by default — but all **degrade gracefully**: `/review` **warns and continues** when the skill is absent (`/classify` → Step 1.5 Skip; `/assess-security` → Step 2.4; `/assess-cost` → Step 2.5; each also under Graceful Degradation), the affected section reading **not assessed**, never HALTing on their absence. `/assess-coupling` (#263) is Optional — until it ships the Architecture (Coupling) section reads **not assessed**. "Required" here means _composed by default_, not _a hard prerequisite_, so the flag never contradicts the graceful-skip steps. ## Arguments @@ -98,7 +100,7 @@ Ask: _"Proceed with review?"_ 1. **Check**: Has `/classify` already run with `$context: review` on the current PR head commit? 2. **Skip**: If already run — reuse the matrix + tier, move to Phase 2. If `/classify` is not installed → warn (`/classify not installed — no review-time risk matrix`) and move to Phase 2. -3. **Act**: Compose `/classify` with `$context: review` against the PR diff. It applies the [quality model](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) to the diff footprint, reads the story's refinement-time tier, and produces the review matrix as a **floor** — it confirms or **raises** the tier, and **never lowers** it (D17). The Security relevance and Coupling balance dimensions are reconciled in Phase 2 (Step 2.4) as `/assess-security` / `/assess-coupling` verdicts land — raise-only. +3. **Act**: Compose `/classify` with `$context: review` against the PR diff. It applies the [quality model](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) to the diff footprint, reads the story's refinement-time tier, and produces the review matrix as a **floor** — it confirms or **raises** the tier, and **never lowers** it (D17). The Security relevance and Coupling balance dimensions are reconciled in Phase 2 (Steps 2.4 and 2.6) as `/assess-security` / `/assess-coupling` verdicts land — raise-only. 4. **Verify**: The review matrix + `risk:*` tier are recorded on the PR (matrix in the body as 1 line + `
`, D22; tags applied only when a `## Tag Projection` is declared). This body matrix is the single rendered artifact — if Phase 2 raises Security relevance or Coupling balance, `/review` updates it **in place** (Step 2.4), it is not re-emitted by `/classify`. A raise to `risk:red` is carried into the Step 5.2 decision. `/classify` HALTs only if the quality model doc (#221) is absent. ## Phase 2: Technical Review @@ -137,9 +139,25 @@ Ask: _"Proceed with review?"_ ### Step 2.4: Security Assessment 1. **Check**: Has `/assess-security` already run with `$mode: review` on the current PR head commit? -2. **Skip**: If already run — reuse the verdict + findings, move to Phase 3. -3. **Act**: Compose `/assess-security` with `$mode: review` against the PR diff. It resolves the rule set (KB global + per-service + per-web-app + adoption project rules) and returns a 1-line verdict + collapsed findings, each tagged **introduced** or **pre-existing**. -4. **Verify**: Verdict + findings recorded — feeds the Security Review section (Step 5.1) and the **Security relevance** dimension of the Step 1.5 classification matrix (`/classify` folds this verdict in **raise-only** — it may raise the tier, never lower it). **Body re-render**: when the verdict raises Security relevance (or the Coupling verdict raises Coupling balance), `/review` updates the already-written Step 1.5 body matrix **in place** — re-rendering the affected `
` row and the 1-line `risk:*` tier so the PR body reflects the final, raised tier; `/classify` is **not** re-invoked (its Phase-1 run stands, and a raise-only edit needs no recompute). **Tag re-apply**: when a `## Tag Projection` is declared (e.g. `Active: risk`), this in-place Phase-2-originated raise **also re-applies the projected chromatic tag on the PR** — swapping the stale label for the raised tier (e.g. `risk:yellow` → `risk:red`) via the same §5 projection `/classify` uses in its Step 5, applied here by `/review` on the raise-only edit — so the PR label matches the raised body tier (AC3); when no projection is declared, only the body matrix is updated and no label is touched. If any **introduced** finding is red → flag explicitly: this is the AC4 signal that drives the CHANGES-REQUESTED decision in Step 5.2. Does not itself HALT — `/assess-security` has no merge authority, this skill's own decision step does. +2. **Skip**: If already run — reuse the verdict + findings, move to Step 2.5. +3. **Act**: Compose `/assess-security` with `$mode: review` against the PR diff. It resolves the rule set (KB global + per-service + per-web-app + adoption project rules) and returns a 1-line verdict + collapsed findings, each tagged **introduced** or **pre-existing**. The verdict feeds the five verdict-first Security sections of the review body — **input validation, output handling, authentication, authorization, introduced vulnerabilities** (Step 5.1, per the [code-review-template](../../../.pair/knowledge/guidelines/collaboration/templates/code-review-template.md)). +4. **Verify**: Verdict + findings recorded — feeds the Security sections (Step 5.1) and the **Security relevance** dimension of the Step 1.5 classification matrix (`/classify` folds this verdict in **raise-only** — it may raise the tier, never lower it). **Body re-render**: when the verdict raises Security relevance (or the Coupling verdict raises Coupling balance), `/review` updates the already-written Step 1.5 body matrix **in place** — re-rendering the affected `
` row and the 1-line `risk:*` tier so the PR body reflects the final, raised tier; `/classify` is **not** re-invoked (its Phase-1 run stands, and a raise-only edit needs no recompute). **Tag re-apply**: when a `## Tag Projection` is declared (e.g. `Active: risk`), this in-place Phase-2-originated raise **also re-applies the projected chromatic tag on the PR** — swapping the stale label for the raised tier (e.g. `risk:yellow` → `risk:red`) via the same §5 projection `/classify` uses in its Step 5, applied here by `/review` on the raise-only edit — so the PR label matches the raised body tier (AC3); when no projection is declared, only the body matrix is updated and no label is touched. If any **introduced** finding is red → flag explicitly: this is the AC4 signal that drives the CHANGES-REQUESTED decision in Step 5.2. Does not itself HALT — `/assess-security` has no merge authority, this skill's own decision step does. +5. **Degrade**: `/assess-security` not installed → the five Security sections read **not assessed** (never dropped); a manual security read of the diff is still expected. + +### Step 2.5: Cost Assessment + +1. **Check**: Has `/assess-cost` already run against the current PR head commit? +2. **Skip**: If already run — reuse the class + signals, move to Step 2.6. +3. **Act**: Compose `/assess-cost` against the PR diff. It resolves the cost-signal catalog from the project's stack/architecture/infrastructure adoption and returns the `cost:green|yellow|orange|red` class as a 1-line verdict + collapsed signals table (D22). This is a genuine re-assessment of the diff — never a restatement of the story's refinement-time `cost:*` tag. +4. **Verify**: Class + signals recorded — feed the **Cost** section of the review body (Step 5.1, 1 line + `
`). The `cost:*` class is carried as its own dimension of the matrix (never folded into the risk `max`). A **red** cost class surfaces the **blocking human sign-off** requirement in the Verdict area (Step 5.1) and is carried into the Step 5.2 decision. Tag re-apply on the PR follows the same rule as Step 2.4 when `cost` is in the declared `## Tag Projection`. Does not itself HALT — `/assess-cost` has no merge authority. +5. **Degrade**: `/assess-cost` not installed → the Cost section reads **not assessed** (never dropped). + +### Step 2.6: Architecture (Coupling) Assessment + +1. **Check**: Is `/assess-coupling` installed (ships with #263)? +2. **Skip**: If not installed → the **Architecture (Coupling)** section reads **not assessed** explicitly; move to Phase 3. +3. **Act**: Compose `/assess-coupling` with `$scope: diff`. It returns a 1-line balance verdict on the integrations the diff touches (integration strength, socio-technical distance, volatility) + collapsed findings (D22). +4. **Verify**: Verdict + findings recorded — feed the **Architecture (Coupling)** section of the review body (Step 5.1) and the **Coupling balance** dimension of the Step 1.5 matrix (`/classify` folds it in **raise-only**, same in-place body re-render + tag re-apply rule as Step 2.4). Does not itself HALT. ## Phase 3: Adoption Compliance @@ -191,17 +209,13 @@ Run the procedure for the level determined in Step 3.1 — see [degradation-leve ### Step 5.1: Compile Review Report -1. **Act**: Compile all findings into a review report following the [code-review-template.md](../../../.pair/knowledge/guidelines/collaboration/templates/code-review-template.md) (resolve override-first — [template resolution](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/template-resolution.md)): - - **Review Information**: PR number, author, reviewer, date, story, review type - - **Review Summary**: overall assessment, key changes, business value - - **Code Review Checklist**: functionality, code quality, technical standards (from Phase 2) - - **Security Review**: verdict + collapsed findings from `/assess-security` (Step 2.4) — 1-line + `
` (D22) - - **Testing Review**: test coverage and quality (from /verify-quality) - - **Documentation Review**: documentation completeness (from /verify-done) - - **Detailed Review Comments**: issues by severity, positive feedback - - **Risk Assessment**: technical and business risks - - **Tech Debt**: items flagged by /analyze-debt - - **Adoption Compliance**: results from Phase 3 (with degradation level noted) +1. **Act**: Compile all findings into a **verdict-first** review body following the [code-review-template.md](../../../.pair/knowledge/guidelines/collaboration/templates/code-review-template.md) (resolve override-first — [template resolution](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/template-resolution.md)). The body is ordered so verdict, tier and cost class read in **~30 seconds** (D22, R6.6): + - **Verdict** (top): classification tags (`risk:` · `cost:`) + the decision + a 1-line reason + PR/author/story metadata. Include the **`Classification changed:`** drift note **only** when the review-time tier/cost differs from the story's refinement-time classification — it fires **upward only** (e.g. `risk:yellow` → `risk:red`, raise-only per quality-model §3.2 / D17); a review that would lower a dimension records the reduction as a finding in the collapsed details, never as a silent downgrade. A **red** cost class states the **blocking human sign-off** requirement here. + - **Assessments** (each a 1-line verdict + `
`, "not assessed" when the capability is absent): + - **Security — input validation, output handling, authentication, authorization, introduced vulnerabilities** (five verdicts from `/assess-security`, Step 2.4) + - **Cost** — `cost:*` class + signals (from `/assess-cost`, Step 2.5) + - **Architecture (Coupling)** — balance verdict (from `/assess-coupling`, Step 2.6; "not assessed" until #263) + - **Details** (collapsed): findings by severity + positive feedback (Phase 2); functionality / AC coverage; testing & quality gates (from /verify-quality); adoption compliance with degradation level (Phase 3); tech debt (from /analyze-debt); documentation (from /verify-done). ### Step 5.2: Make Review Decision @@ -210,18 +224,20 @@ Based on compiled findings: | Decision | Condition | | --------------------- | ----------------------------------------------------------------------------------------- | | **APPROVED** | No critical or major issues. All AC met. Quality gates pass. | -| **CHANGES-REQUESTED** | Critical issues found, missing ADRs, any **introduced** red security finding from `/assess-security` (AC4), failing tests, AC not met. | +| **CHANGES-REQUESTED** | Critical issues found, missing ADRs, any **introduced** red security finding from `/assess-security` (AC4), failing tests, AC not met. A **red** `cost:*` class does not itself block — it surfaces a **blocking human sign-off** requirement in the Verdict (the human, not the skill, gates on cost). | | **TECH-DEBT** | Only minor issues or debt items. Approve current PR, track debt separately. | -### Step 5.3: Post Review +### Step 5.3: Submit Review + +The compiled report **is the body of the native GitHub review** — the verdict is the review action; there is **no separate PR comment** (decision Q5, AC2). -1. **Act**: Post the review report as a PR comment. -2. **Act**: Set PR review status using the PM tool (per [github-implementation.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md)): - - **APPROVED / TECH-DEBT**: Submit review with `event = APPROVE`. - - **CHANGES-REQUESTED**: Submit review with `event = REQUEST_CHANGES`. - - MCP-first: use `pull_request_review_write` with `method = create` and appropriate `event`. - - CLI fallback: `gh pr review --approve` or `--request-changes`. -3. **Verify**: Review posted and status updated. +1. **Act**: Submit the native review using the PM tool (per [github-implementation.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md)), passing the compiled verdict-first report as the review **body**: + - **APPROVED / TECH-DEBT**: `event = APPROVE`. + - **CHANGES-REQUESTED**: `event = REQUEST_CHANGES`. + - MCP-first: `pull_request_review_write` with `method = create`, the report as `body`, and the appropriate `event`. + - CLI fallback: `gh pr review --approve|--request-changes --body-file `. +2. **Act**: On re-review, **edit the existing review body in place** rather than posting a duplicate (idempotency). +3. **Verify**: The native review is submitted with the verdict-first body — no separate review-comment artifact exists. ### Step 5.4: Determine Next Action @@ -249,12 +265,14 @@ REVIEW COMPLETE: ├── Story: [#ID: Title | N/A] ├── Decision: [APPROVED | CHANGES-REQUESTED | TECH-DEBT] ├── Issues: [critical: N | major: N | minor: N] -├── Security: [green | yellow | red — N findings, N introduced | N/A — not installed] +├── Security: [green | yellow | red — N findings, N introduced | not assessed] +├── Cost: [green | yellow | orange | red | not assessed] +├── Coupling: [green | yellow | red | not assessed — until #263] ├── Quality: [PASS | FAIL — N gates] ├── DoD: [N/N criteria met] ├── Adoption: [Level N — summary] ├── Debt: [N items flagged] -└── Report: [Posted as PR comment] +└── Review: [Submitted as native review body — no separate comment] ``` At merge (Phase 6): see [merge-and-cascade.md](merge-and-cascade.md). @@ -287,7 +305,9 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar - **/verify-adoption not installed**: Falls back to inline dependency checking against [tech-stack.md](../../../.pair/adoption/tech/tech-stack.md). Warning logged. See degradation cascade (Phase 3). - **/assess-stack not installed**: Unlisted dependencies flagged as warnings for manual verification. Does NOT HALT. - **/analyze-debt not available**: Skip debt assessment, note in report. -- **/assess-security not installed**: Skip Step 2.4. Security Review section notes "manual judgment only — /assess-security not installed". Does NOT HALT; a manual security read of the diff is still expected per [how-to-11](../../../.pair/knowledge/how-to/11-how-to-code-review.md). +- **/assess-security not installed**: Skip Step 2.4. The five Security sections (input validation, output handling, authentication, authorization, introduced vulnerabilities) read **not assessed** — never dropped. Does NOT HALT; a manual security read of the diff is still expected per [how-to-11](../../../.pair/knowledge/how-to/11-how-to-code-review.md). +- **/assess-cost not installed**: Skip Step 2.5. The Cost section reads **not assessed**. Does NOT HALT. +- **/assess-coupling not installed** (until #263): Skip Step 2.6. The Architecture (Coupling) section reads **not assessed**. Does NOT HALT. - **Story not found**: Review proceeds with PR-only validation (no AC check). Phase 6 skips parent cascade. - **Code review template not found**: **HALT** — cannot produce review without template (a required dependency, not optional). - **PM tool not accessible**: Phase 6 merge via CLI only. @@ -297,7 +317,7 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar ## Notes -- This skill **reads code, posts review comments, and optionally merges PRs** — it does not modify source code. +- This skill **reads code, submits the native review (verdict = the review action), and optionally merges PRs** — it does not modify source code and posts no separate review comment (AC2). - Review phases are sequential — each phase builds on findings from prior phases. - The reviewer can stop between phases; re-invoke to resume (see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md)). - Output follows [code-review-template.md](../../../.pair/knowledge/guidelines/collaboration/templates/code-review-template.md) — the template defines structure, /review fills it with findings. diff --git a/packages/knowledge-hub/src/conformance/code-review-verdict-first.test.ts b/packages/knowledge-hub/src/conformance/code-review-verdict-first.test.ts new file mode 100644 index 00000000..6272e1b1 --- /dev/null +++ b/packages/knowledge-hub/src/conformance/code-review-verdict-first.test.ts @@ -0,0 +1,177 @@ +import { describe, it, expect } from 'vitest' +import { readFileSync } from 'fs' +import { join } from 'path' + +// Conformance guard for #228: the code-review template is verdict-first (D22, R6.6) +// and carries the seven required assessment sections — input validation, output +// handling, authentication, authorization, introduced vulnerabilities, cost, +// architecture (coupling) — each a 1-line verdict +
. The review artifact is +// the NATIVE GitHub review body (verdict = the review action), not a separate comment +// (decision Q5, AC2). /pair-process-review composes /assess-cost, surfaces the +// architecture/coupling verdict (assess-coupling, "not assessed" until #263), keeps +// the review-time classification a floor (raise-only, D17) and emits the +// "Classification changed" drift note upward only. Asserted on BOTH the dataset +// (source of truth) and the installed root mirror, name-mapped — the drift class this +// repo's parity guards exist to catch. + +const DATASET_SKILLS = join(__dirname, '../../dataset/.skills') +const MIRROR_SKILLS = join(__dirname, '../../../../.claude/skills') +const DATASET_TEMPLATES = join( + __dirname, + '../../dataset/.pair/knowledge/guidelines/collaboration/templates', +) +const MIRROR_TEMPLATES = join( + __dirname, + '../../../../.pair/knowledge/guidelines/collaboration/templates', +) +const DATASET_HOWTO = join(__dirname, '../../dataset/.pair/knowledge/how-to') +const MIRROR_HOWTO = join(__dirname, '../../../../.pair/knowledge/how-to') + +const TEMPLATE_DATASET = readFileSync(join(DATASET_TEMPLATES, 'code-review-template.md'), 'utf-8') +const TEMPLATE_MIRROR = readFileSync(join(MIRROR_TEMPLATES, 'code-review-template.md'), 'utf-8') +const REVIEW_DATASET = readFileSync(join(DATASET_SKILLS, 'process/review/SKILL.md'), 'utf-8') +const REVIEW_MIRROR = readFileSync(join(MIRROR_SKILLS, 'pair-process-review/SKILL.md'), 'utf-8') +const HOWTO11_DATASET = readFileSync(join(DATASET_HOWTO, '11-how-to-code-review.md'), 'utf-8') +const HOWTO11_MIRROR = readFileSync(join(MIRROR_HOWTO, '11-how-to-code-review.md'), 'utf-8') + +// The seven required assessment sections (AC1), matched case-insensitively as `###` headings. +const REQUIRED_SECTIONS = [ + 'Input validation', + 'Output handling', + 'Authentication', + 'Authorization', + 'Introduced vulnerabilities', + 'Cost', + 'Architecture', +] as const + +const TEMPLATE_VARIANTS = [ + ['dataset', TEMPLATE_DATASET], + ['mirror', TEMPLATE_MIRROR], +] as const + +describe('code-review-template — verdict-first reading budget (AC3, D22) (#228)', () => { + for (const [label, content] of TEMPLATE_VARIANTS) { + it(`${label} opens with a Verdict section carrying risk tier + cost class + decision`, () => { + // Verdict must be the first `##` section so the reader hits it in ~30s. + const firstSection = content.slice(content.search(/^##\s+/m)).split(/\n## /)[0] + expect(firstSection).toMatch(/##\s+Verdict/) + expect(firstSection).toMatch(/risk:/) + expect(firstSection).toMatch(/cost:/) + expect(firstSection).toMatch(/APPROVED|CHANGES-REQUESTED|TECH-DEBT/) + }) + + it(`${label} states the ~30-second reading budget (D22, R6.6)`, () => { + expect(content).toMatch(/D22/) + expect(content).toMatch(/30\s*[- ]?\s*second|~?30s/i) + }) + + it(`${label} declares it is the native review body, not a separate comment (AC2, Q5)`, () => { + expect(content.toLowerCase()).toMatch(/native (github )?review/) + expect(content.toLowerCase()).toMatch(/no separate.*comment|not a separate.*comment/) + }) + } +}) + +describe('code-review-template — seven verdict-first assessment sections (AC1) (#228)', () => { + for (const [label, content] of TEMPLATE_VARIANTS) { + for (const section of REQUIRED_SECTIONS) { + it(`${label} has a "${section}" section that is 1-line verdict +
`, () => { + const re = new RegExp(`^###\\s+.*${section}`, 'im') + const idx = content.search(re) + expect(idx).toBeGreaterThan(-1) + // Slice to the next `###`/`##` heading — isolate this section only. + const body = content + .slice(idx) + .replace(re, '') + .split(/\n#{2,3}\s/)[0] + // A 1-line verdict marker... + expect(body).toMatch(/\*\*Verdict:\*\*|Verdict:/i) + // ...backed by a collapsed details block. + expect(body).toContain('
') + }) + } + + it(`${label} shows an unavailable assessment as "not assessed" explicitly (edge case)`, () => { + expect(content.toLowerCase()).toContain('not assessed') + }) + } +}) + +const REVIEW_VARIANTS = [ + { label: 'dataset', content: REVIEW_DATASET, cost: '/assess-cost', coupling: '/assess-coupling' }, + { + label: 'mirror', + content: REVIEW_MIRROR, + cost: '/pair-capability-assess-cost', + coupling: '/pair-capability-assess-coupling', + }, +] as const + +describe('review skill — composes assess-cost into the review (AC1 #228, AC3 #226)', () => { + for (const v of REVIEW_VARIANTS) { + it(`${v.label} lists assess-cost as a composed skill`, () => { + const row = v.content + .split('\n') + .find(line => line.includes(`\`${v.cost}\``) && line.includes('Capability')) + expect(row).toBeDefined() + }) + + it(`${v.label} composes assess-cost against the diff and shows the cost class 1-line + collapsed`, () => { + expect(v.content).toContain(`Compose \`${v.cost}\``) + expect(v.content).toMatch(/D22/) + }) + + it(`${v.label} surfaces the architecture/coupling verdict, "not assessed" until assess-coupling ships`, () => { + expect(v.content).toContain(v.coupling) + // Architecture/coupling degrades to "not assessed" when the capability is absent. + const gd = v.content.slice(v.content.indexOf('## Graceful Degradation')) + expect(gd.toLowerCase()).toContain('not assessed') + }) + } +}) + +describe('review skill — review-time classification is a floor, drift note fires upward (AC3 #226, #228)', () => { + for (const v of REVIEW_VARIANTS) { + it(`${v.label} keeps the review matrix raise-only (never lowers the tier, D17)`, () => { + expect(v.content).toMatch(/never\s+lower|raise-only|raise only/i) + expect(v.content).toMatch(/D17/) + }) + + it(`${v.label} emits a "Classification changed" drift note when review-time differs`, () => { + expect(v.content).toMatch(/Classification changed/) + }) + + it(`${v.label} applies the projected classification tags on the PR (not only the story)`, () => { + expect(v.content.toLowerCase()).toMatch(/tag.*on the pr|label.*on the pr|re-apply.*tag/) + }) + } +}) + +describe('review skill — review IS the native review body, no separate comment (AC2, Q5) (#228)', () => { + for (const v of REVIEW_VARIANTS) { + it(`${v.label} submits the report as the native review body, not a separate PR comment`, () => { + const post = v.content.slice(v.content.indexOf('### Step 5.3')) + expect(post.toLowerCase()).toMatch(/native (github )?review|review body/) + expect(post.toLowerCase()).not.toMatch(/post the review report as a pr comment/) + }) + } +}) + +describe('how-to-11 — aligned to native-review-body verdict (AC2) (#228)', () => { + for (const [label, content] of [ + ['dataset', HOWTO11_DATASET], + ['mirror', HOWTO11_MIRROR], + ] as const) { + it(`${label} no longer instructs a separate "post report as PR comment"`, () => { + expect(content.toLowerCase()).not.toMatch(/post report as pr comment/) + }) + } +}) + +describe('code-review-template — root/dataset structural parity (#228)', () => { + const headings = (content: string) => content.match(/^##+ .*$/gm) ?? [] + it('has the same number of section headings in root and dataset', () => { + expect(headings(TEMPLATE_MIRROR).length).toBe(headings(TEMPLATE_DATASET).length) + }) +}) From eae28d7fd83cc135ce4091d171b7e6a9a9f796ba Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 12:11:37 +0200 Subject: [PATCH 02/11] [#228] chore: resolve review round 1 on #376 (consumer sync, chip fallback, byte-eq guard, self-review path, scope ADL) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - contract-generator.md: parsing hints point to new anchors (## Verdict line, ## Details → Findings by severity) - reviewer.md: default verdict vocab → APPROVED | CHANGES-REQUESTED | TECH-DEBT - code-review-template.md (root+dataset): document unassessed-dimension top-line chip fallback (risk/cost:n/a, never dropped) - verdict-first conformance test: add byte-equality guard on the two template files (identical-mirror invariant) - review SKILL.md (mirror+dataset): self-authored PR fallback (event=COMMENT) in Step 5.3 + Graceful Degradation - decision-log: record #226 AC3 delivered in #228 PR (shared review-template lane) Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/agents/contract-generator.md | 2 +- .claude/agents/reviewer.md | 2 +- .claude/skills/pair-process-review/SKILL.md | 2 + ...-24-review-template-lane-226-228-one-pr.md | 57 +++++++++++++++++++ .../templates/code-review-template.md | 2 + .../templates/code-review-template.md | 2 + .../dataset/.skills/process/review/SKILL.md | 2 + .../code-review-verdict-first.test.ts | 7 +++ 8 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 .pair/adoption/decision-log/2026-07-24-review-template-lane-226-228-one-pr.md diff --git a/.claude/agents/contract-generator.md b/.claude/agents/contract-generator.md index d4baec2a..ae24a26e 100644 --- a/.claude/agents/contract-generator.md +++ b/.claude/agents/contract-generator.md @@ -17,7 +17,7 @@ You are the **contract generator**: you turn a human-friendly KB markdown templa 1. **Check the cache first**: `node .claude/workflows/contracts/ensure-contract.mjs check