amirbena / amirbena/code-review-skill

Add a benchmark fixture proving blocking findings never render a clean verdict

Open
#350 0 comments 0 reactions 1 assignee Claimed by @amirbena View on GitHub
area:review-quality maintainer-led priority:P1 type:quality
Dominant language
Python
Stars
7
Forks
2
Avg merge
26m
Merged PRs (30d)
200

Description

## Type

Quality

## Area

Review Quality

## Priority

P1 — High

## Problem

The mechanical severity → decision derivation in `shared/policies/severity.md` is well-specified, but nothing exercises it end-to-end against real reviewer output: the benchmark corpus has no case that specifically asserts a blocking finding never renders a clean verdict. This is the cheapest, highest-signal check available for the verdict-drift failure mode raised by a review-reliability audit, and it needs no new capability to build.

## Goal

Add a benchmark/regression fixture that runs an unambiguous blocking (P0/P1) defect through the real packaged Skill end-to-end and asserts the rendered outcome is never the clean/approved value.

## Scope

- One or more corpus fixtures with an unambiguous, unarguable blocking defect (reusing or extending an existing corpus case where one already qualifies).
- A metric/assertion that the rendered Result/Decision from a real run is the blocking value whenever the produced findings include a P0/P1.
- Cover both Skills where practical: `local-code-review`'s `CHANGES REQUIRED` and `github-pr-review`'s non-approve review event.
- Document the fixture in the existing corpus README/catalog alongside other cases.

## Non-Goals

- Does not build any enforcement mechanism — proof only, via the existing benchmark harness.
- Does not touch `severity.md`'s derivation or `shared/templates/review-summary.md`'s rendering rules.
- Does not require the output schema (#67/#71) — parses the existing rendered markdown/GitHub event.

## Acceptance Criteria

- [ ] At least one corpus fixture with an unambiguous blocking defect exists and is runnable through `run_benchmark.py`.
- [ ] A real run's rendered outcome is asserted to be non-clean whenever a P0/P1 finding is produced.
- [ ] The check fails loudly (not silently skipped) if the runtime is unavailable, consistent with existing `insufficient-coverage`/`runtime-unavailable` handling.
- [ ] Existing corpus cases are unaffected.

## Dependencies

Depends on: none
Relates: #338, #339 (if this fixture is folded into the standing nightly corpus rather than run ad hoc)

## Validation

- New fixture runs clean through the existing benchmark tooling.
- Existing `tests/unit/benchmark/` suite passes unchanged.

## Canonical design

Part of the architecture defined in
[`docs/benchmark-measurement-architecture/benchmark-measurement-architecture-model.md`](docs/benchmark-measurement-architecture/benchmark-measurement-architecture-model.md),
§12.4 ("Verdict integrity: benchmark proof and the consistency-boundary
research" — first bullet, "Benchmark proof"). Read the canonical design
before implementation.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.