Skip to content

fix(pr-review): require attribution on every finding - #86

Merged
bborbe merged 1 commit into
masterfrom
feature/pr-review-finding-attribution
Aug 8, 2026
Merged

fix(pr-review): require attribution on every finding#86
bborbe merged 1 commit into
masterfrom
feature/pr-review-finding-attribution

Conversation

@bborbe

@bborbe bborbe commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Closes the blocker measured by the review benchmark: /coding:pr-review non-deterministically emits findings carrying neither a file reference nor a rule tag, so they cannot be keyed, matched or scored.

The measurement

Three benchmark runs over the 5-PR dev fixture (coding v0.35.4, sonnet/medium/short), review cache cleared between each:

                        run 1     run 2              run 3
tts-mcp#20              0         0                  0
github-pr-review-agent  1         FAIL (6 unattr.)   0
quant#109               FAIL (2)  0                  FAIL (3)
node-skeleton#2         0         1                  FAIL (1)
python-skeleton#3       FAIL (2)  1                  FAIL (1)
                        3 ok/2✗   4 ok/1✗            2 ok/3✗

6 of 15 attempts (40%) produced no row, rejected by the harvester's UNATTRIBUTABLE FINDING gate. No PR fails consistently — quant#109 failed, succeeded, failed. The missingness is not ignorable: it correlates with the runs that found the most (the github-pr-review-agent#11 rejection happened on the run where it produced six items), so averaging the survivors understates variance invisibly.

Why the fix belongs here and not in the harvester

Relaxing the harvester to accept body-only findings would restore exactly the silent acceptance that produced defects D2, D4 and D7 in the benchmark runner — a layer emitting a plausible wrong number instead of refusing. bench/README.md § "Reading review output" is the contract; the command simply never asked for what that contract requires. Step 5 mandated the three severity headers and described what kinds of issue belong under each, but said nothing about the shape of an individual finding item.

The change

commands/pr-review.md Step 5 only. Fifteen lines, no rule content touched, no harvester change.

  • The item's first bold run must be the file reference — not a summary phrase. The counter-example is quoted from a real capture: - **Inconsistent due_date choice** is wrong; - **task/x.yaml:24** — inconsistent due_date choice is right.
  • Accepted shapes only: **`path:LINE`**, **`path`**, **`path:START-END`** — the four shapes the harvester actually parses.
  • A path appearing only in the prose does not count, and no path may be inferred that was not in the diff — the reviewer-side mirror of the harvester's "no path is ever inferred by searching the repository" invariant. Having required a path on every finding, the tempting failure is to invent one.
  • General remarks go in a trailing **Notes:** block, never as a bullet inside a severity section.

Verification

Two live runs against the modified command over the full 5-PR fixture:

run A:  5 ok, 0 failed    path 7/7
run B:  5 ok, 0 failed    path 3/3

10 of 10 findings carry a real path; zero rejections across 10 PR attempts. Run A also exercised the range shape (.github/workflows/ci.yml:27-34) and a deep path (task/recurring-schedules/prod/plan-next-week.yaml:24).

Run A exposed a residual gap that run B closed: a finding led with a bold summary phrase rather than a path, satisfying "bold run first" while remaining unattributable. That is why the instruction now requires the bold run to be the reference, with the failing shape quoted verbatim.

Not evidenced: rule_id is 0/10 — grep '(rule:' finds no tag in any capture. Every finding in these runs was a CI/YAML/Makefile issue with no matching entry in rules/index.json, so omission may be correct per the instruction. Separating "correctly omitted" from "ignored" needs a run over a PR where code rules actually apply, and is deliberately not claimed here.

make precommit 30/30, 103 tests. Note these prove nothing about this change — no test reads the command's instructions; the live runs are the evidence.

@ben-s-pull-request-reviewer ben-s-pull-request-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
  "verdict": "approve",
  "summary": "PR adds a MANDATORY block to `commands/pr-review.md` Step 5 requiring every finding to open with a bold file reference and carry an inline rule tag, plus a corresponding `fix:` CHANGELOG entry. All judgment-rule triggers (changelog rules, agent-cmd/command-thin) are satisfied — the command addition is procedural documentation, not leaked business logic; the CHANGELOG bullet uses the correct conventional prefix and sits correctly under ## Unreleased. No violations found.",
  "comments": [],
  "concerns_addressed": [
    "correctness: commands/pr-review.md — the anti-example `**Inconsistent due_date choice**` is correctly quoted; first bold = summary phrase is correctly identified as wrong",
    "correctness: commands/pr-review.md — the three accepted shapes (**`path:LINE`**, **`path`**, **`path:START-END`**) are documented as specified; consistency with harvester implementation is an implementation detail beyond rule scope",
    "tests: commands/pr-review.md — the new finding format is a command output specification, not new code logic; no test gap introduced by this change"
  ]
}

@bborbe
bborbe merged commit d0c4175 into master Aug 8, 2026
1 check passed
@bborbe
bborbe deleted the feature/pr-review-finding-attribution branch August 8, 2026 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant