Skip to content

/advanced-code-review-report

Command Content

# Phase 5: Report Generation

## Invariant Principles

1. **Signal over noise**: Only verified findings appear in the final report. REFUTED findings are excluded. Quality of findings matters more than quantity.
2. **Actionable output**: Every finding must have clear next steps. Findings without suggestions or context are not actionable.
3. **Machine-readable artifacts for automation**: JSON summary enables CI/CD integration, automated triage, and tooling. Human-readable Markdown is not sufficient alone.

**Purpose:** Produce final deliverables including Markdown report and machine-readable JSON summary.

## 5.1 Finding Filtering

Filter to verified and inconclusive findings only:

```python
def filter_findings_for_report(findings: list[dict]) -> list[dict]:
    """Filter out REFUTED findings for final report."""
    return [
        f for f in findings
        if f.get("verification_status") != "REFUTED"
    ]
```

## 5.2 Severity Sorting

Sort findings by severity (most critical first):

```python
SEVERITY_ORDER = {
    "CRITICAL": 0,
    "HIGH": 1,
    "MEDIUM": 2,
    "LOW": 3,
    "NIT": 4,
    "QUESTION": 5,
    "PRAISE": 6
}
# QUESTION is a legal severity and MUST be present. Omitting it routes every
# QUESTION finding through the .get(..., 99) fallback, where it sorts last and
# disappears from by_severity. This dict must match the one in the
# advanced-code-review skill key for key.

def sort_by_severity(findings: list[dict]) -> list[dict]:
    """Sort findings by severity, most critical first."""
    return sorted(findings, key=lambda f: SEVERITY_ORDER.get(f["severity"], 99))
```

## 5.3 Verdict Determination

Determine overall review verdict:

```python
BLOCKING = {"CRITICAL", "HIGH"}
DISCUSS = {"MEDIUM"}
NON_BLOCKING = {"LOW", "NIT", "QUESTION", "PRAISE"}
KNOWN_SEVERITIES = BLOCKING | DISCUSS | NON_BLOCKING


def determine_verdict(findings: list[dict]) -> str:
    """
    Determine review verdict based on findings.

    This is a MERGE GATE, so it FAILS CLOSED. Two ways it used to fail open:
    exact-uppercase membership testing (a finding emitted as `Critical` matched
    nothing and fell through to APPROVE), and treating an UNRECOGNISED severity
    as non-blocking. Both let a blocking finding merge under
    "No blocking issues found."

    Returns: "APPROVE" | "REQUEST_CHANGES" | "COMMENT"
    """
    severities = {
        str(f.get("severity", "")).strip().upper()
        for f in findings
        if f.get("verification_status") != "REFUTED"
    }

    # An unrecognised severity is NOT evidence of harmlessness. It means a
    # producer is speaking a vocabulary this gate does not know, so the gate
    # cannot rank it -- block and make a human look.
    unknown = severities - KNOWN_SEVERITIES - {""}
    if unknown:
        return "REQUEST_CHANGES"

    if severities & BLOCKING:
        return "REQUEST_CHANGES"

    if severities & DISCUSS:
        return "COMMENT"

    return "APPROVE"

def verdict_rationale(verdict: str, findings: list[dict]) -> str:
    """Generate rationale for verdict."""
    by_severity = {}
    for f in findings:
        # Normalise the same way determine_verdict does. Counting raw strings
        # here would report "0 blocking issue(s)" alongside REQUEST_CHANGES.
        sev = str(f.get("severity", "")).strip().upper()
        by_severity[sev] = by_severity.get(sev, 0) + 1

    unknown = sorted(set(by_severity) - KNOWN_SEVERITIES - {""})

    if verdict == "REQUEST_CHANGES":
        if unknown:
            return (
                f"unrecognised severity value(s) {unknown} -- the gate cannot "
                "rank them and blocks rather than assuming they are harmless"
            )
        critical = by_severity.get("CRITICAL", 0)
        high = by_severity.get("HIGH", 0)
        return f"{critical + high} blocking issue(s) require attention"
    elif verdict == "COMMENT":
        medium = by_severity.get("MEDIUM", 0)
        return f"{medium} medium-severity issue(s) worth discussing"
    else:
        return "No blocking issues found"
```

## 5.4 Template Rendering

<CRITICAL>
Every review MUST report the base it used AND how that base was resolved. The
`merge_target`, `merge_base`, `base_ref`, `resolved_via` and `fetch` fields are
carried into `review-manifest.json` by Phase 1; this phase surfaces them at the
head of `review-report.md`:

```
Base: <merge_target> @ <merge_base[:12]> (resolved via <resolved_via>, fetch <fetch>)
Endpoint: <committed-only | includes working tree>
```

If `resolved_via` is `fallback-literal`, or `fetch` is not `ok`, flag it
prominently — the base may be wrong or stale. Silent fallback is the exact
failure this procedure exists to prevent.
</CRITICAL>

Use Python's `string.Template` for report generation:

```python
from string import Template

def render_report(manifest: dict, findings: list[dict], context: dict, snr: float) -> str:
    """Render final report using template."""
    with open("templates/report.md.tpl") as f:
        tpl = Template(f.read())

    # Count by severity
    by_severity = count_by_severity(findings)

    # Generate findings section
    findings_section = render_findings_section(findings)

    # Generate action items
    action_items = render_action_items(findings)

    # Generate previous context section
    previous_context = render_previous_context(context)

    return tpl.substitute(
        branch=manifest["target"]["branch"],
        base=manifest["target"]["base"],
        base_sha=manifest["target"]["merge_base_sha"][:8],
        # Provenance is REPORTED, never dropped. A silently wrong or stale base
        # is the failure this pipeline exists to prevent.
        resolved_via=manifest["target"]["resolved_via"],
        fetch_status=manifest["target"]["fetch"],
        endpoint=manifest["target"]["endpoint"],
        timestamp=datetime.now().strftime("%Y-%m-%d %H:%M UTC"),
        file_count=manifest["files"]["total"],
        finding_count=len(findings),
        snr=f"{snr:.2f}",
        critical_count=by_severity.get("CRITICAL", 0),
        high_count=by_severity.get("HIGH", 0),
        medium_count=by_severity.get("MEDIUM", 0),
        low_count=by_severity.get("LOW", 0),
        verdict=determine_verdict(findings),
        findings_section=findings_section,
        action_items=action_items,
        previous_context=previous_context
    )

def render_finding(finding: dict) -> str:
    """Render a single finding using template."""
    with open("templates/finding.md.tpl") as f:
        tpl = Template(f.read())

    line_str = str(finding["line"])
    if finding.get("end_line"):
        line_str = f"{finding['line']}-{finding['end_line']}"

    # Detect language from file extension
    ext = finding["file"].rsplit(".", 1)[-1] if "." in finding["file"] else ""
    lang_map = {"py": "python", "js": "javascript", "ts": "typescript", "rb": "ruby"}
    lang = lang_map.get(ext, ext)

    verification_flag = ""
    if finding.get("verification_status") == "INCONCLUSIVE":
        verification_flag = " [NEEDS VERIFICATION]"

    return tpl.substitute(
        severity=finding["severity"],
        id=finding["id"].replace("finding-", ""),
        summary=finding["summary"] + verification_flag,
        file=finding["file"],
        line=line_str,
        category=finding["category"].title(),
        reason=finding.get("reason", ""),
        lang=lang,
        evidence=finding.get("evidence", "N/A"),
        suggestion=finding.get("suggestion", "N/A")
    )
```

## 5.5 Action Items Generation

Generate actionable checklist:

```python
def render_action_items(findings: list[dict]) -> str:
    """Generate action items checklist."""
    items = []

    for f in findings:
        if f["severity"] in ("CRITICAL", "HIGH"):
            items.append(f"- [ ] Fix {f['id']}: {f['summary']}")
        elif f["severity"] == "MEDIUM":
            items.append(f"- [ ] Consider {f['id']}: {f['summary']}")

    return "\n".join(items) if items else "No blocking action items."
```

## 5.6 Previous Context Section

```python
def render_previous_context(context: dict) -> str:
    """Render previous review context section."""
    if not context.get("previous_review"):
        return "## Previous Review Context\n\nNo previous review found."

    lines = ["## Previous Review Context\n"]

    declined = len(context.get("declined_items", []))
    partial = len(context.get("partial_items", []))
    alternative = len(context.get("alternative_items", []))

    if declined:
        lines.append(f"- {declined} declined item(s) (not re-raised)")
    if partial:
        lines.append(f"- {partial} partial fix(es) (pending items noted)")
    if alternative:
        accepted = sum(1 for a in context["alternative_items"] if a.get("accepted"))
        lines.append(f"- {alternative} alternative(s) ({accepted} accepted)")

    return "\n".join(lines)
```

## 5.7 Output: review-report.md

The final report is rendered using `templates/report.md.tpl`:

```markdown
# Code Review Report

**Branch:** feature/auth-refactor
**Base:** <detected-base-branch> @ abc12345 (resolved via pr-base-ref, fetch ok)
**Endpoint:** committed-only (reviewing what will merge)
**Reviewed:** 2026-01-30 10:30 UTC
**Files:** 12 | **Findings:** 6 | **Signal/Noise:** 0.75

---

## Summary

| Severity | Count |
|----------|-------|
| Critical | 0 |
| High | 2 |
| Medium | 3 |
| Low | 1 |

**Verdict:** REQUEST_CHANGES (2 blocking issue(s) require attention)

---

## High Severity

### [HIGH-001] SQL injection via string interpolation

**File:** auth.py:45-47
**Category:** Security

User input from request directly concatenated into SQL query.

```python
# Current
query = f"SELECT * FROM users WHERE id = {user_id}"

# Suggested
cursor.execute("SELECT * FROM users WHERE id = %s", (user_id,))
```

---

## Action Items

- [ ] Fix HIGH-001: SQL injection in auth.py
- [ ] Fix HIGH-002: Missing auth check in payment.py
- [ ] Consider MEDIUM-001: Add input validation

---

## Previous Review Context

- 1 declined item(s) (not re-raised)
- 1 partial fix(es) (pending items noted)
- 1 alternative(s) (1 accepted)
```

## 5.8 Output: review-summary.json

```json
{
  "version": "1.0",
  "generated_at": "2026-01-30T10:30:00Z",
  "target": {
    "branch": "feature/auth-refactor",
    "base": "<detected-base-branch>",
    "base_ref": "<remote>/<detected-base-branch>",
    "merge_base_sha": "abc12345",
    "head_sha": "def67890",
    "resolved_via": "pr-base-ref",
    "fetch": "ok",
    "endpoint": "committed-only"
  },
  "verdict": "REQUEST_CHANGES",
  "verdict_rationale": "2 blocking issue(s) require attention",
  "statistics": {
    "files_reviewed": 12,
    "total_findings": 8,
    "verified_findings": 6,
    "refuted_findings": 2,
    "by_severity": {
      "CRITICAL": 0,
      "HIGH": 2,
      "MEDIUM": 3,
      "LOW": 1,
      "NIT": 0,
      "QUESTION": 0,
      "PRAISE": 0
    },
    "signal_to_noise": 0.75
  },
  "action_items": [
    {"id": "HIGH-001", "summary": "SQL injection in auth.py", "priority": "blocking"},
    {"id": "HIGH-002", "summary": "Missing auth check in payment.py", "priority": "blocking"},
    {"id": "MEDIUM-001", "summary": "Add input validation", "priority": "suggested"}
  ],
  "artifacts": {
    "report_path": "~/.local/spellbook/docs/project/reviews/feature-auth-abc12345/review-report.md",
    "findings_path": "~/.local/spellbook/docs/project/reviews/feature-auth-abc12345/findings.json"
  }
}
```

## 5.9 File Output

Write all artifacts to the review directory:

```python
def write_review_artifacts(review_dir: Path, report: str, summary: dict):
    """Write all final artifacts."""
    review_dir.mkdir(parents=True, exist_ok=True)

    # Write Markdown report
    (review_dir / "review-report.md").write_text(report)

    # Write JSON summary
    (review_dir / "review-summary.json").write_text(
        json.dumps(summary, indent=2)
    )

    print(f"Review complete: {review_dir / 'review-report.md'}")
```

## Phase 5 Self-Check

Before declaring review complete:

- [ ] Findings filtered (REFUTED removed)
- [ ] Findings sorted by severity
- [ ] Verdict determined with rationale
- [ ] Report rendered from template
- [ ] Action items generated
- [ ] Previous context included
- [ ] review-report.md written
- [ ] review-summary.json written
- [ ] All artifacts in correct directory

## Final Self-Check (the whole review, not just this phase)

Phase 5 is where the review is declared complete, so the whole-run gate is
applied here.

### Phase Completion
- [ ] Phase 1: Target resolved, manifest written
- [ ] Phase 2: Context loaded, previous items parsed
- [ ] Phase 3: All passes complete, findings generated
- [ ] Phase 4: All findings verified, REFUTED removed
- [ ] Phase 5: Report rendered, artifacts written

### Quality Gates
- [ ] Every finding has: id, severity, category, file, line, evidence, **rule**
- [ ] **Every `rule` names a catalogued rule (document + id) or a named correctness/logic bug**
- [ ] **Base was DETECTED (no hardcoded literal), and base + `resolved_via` + fetch status are reported**
- [ ] **Endpoint (committed-only vs. working tree) chosen deliberately and stated**
- [ ] **Standards load completed; `rule-catalogue.json` written; if nothing was found, disclosed**
- [ ] **Coverage reconciled N-of-N at hunk level; gaps listed with reasons**
- [ ] No REFUTED findings in final report
- [ ] INCONCLUSIVE findings flagged with [NEEDS VERIFICATION]
- [ ] Declined items from previous review not re-raised
- [ ] Signal-to-noise ratio calculated and reported

### Output Verification
- [ ] Every artifact named in the advanced-code-review skill's Outputs table exists and is valid

<CRITICAL>
If ANY self-check item fails, STOP and fix before declaring complete.
</CRITICAL>