Review: keep every finding, and print the agent's error on a failed run

.code-review/run-review.sh

@@ -90,21 +90,23 @@ fi
 prior=()
 if [ -s "$prior_file" ]; then prior=(--prior-review "$prior_file"); fi
 
+# The run's credentials: blanked in any agent error the review prints, refused in what it posts.
+guard=(--secret-env OPENAI_API_KEY)
+if [ -n "${CODEX_HOME:-}" ]; then
+  guard+=(--secret-file "$CODEX_HOME/auth.json" --secret-file "$work/auth.seed.json")
+fi
+
 # The review never sees the GitHub token; only the post below gets it.
 status=0
 (cd "$here" && env -u GITHUB_TOKEN "${py[@]}" -m review_core.cli review --repo "$repo" --base "$base" \
   --head "$CR_HEAD" --runner "$runner" --model "${CR_MODEL:-gpt-6-luna}" \
-  --effort "${CR_EFFORT:-high}" --out "$out" --runs "$work/runs" "${registry[@]}" \
+  --effort "${CR_EFFORT:-high}" --out "$out" --runs "$work/runs" "${registry[@]}" "${guard[@]}" \
   ${prior[@]+"${prior[@]}"} "${paths[@]}") || status=$?
 
 # A failed run still has a review.json; post it so the pull request shows the failure.
 # The agents could read the login; the poster refuses to post any of its values.
 post=("${py[@]}" -m review_core.ci.github --runs "$work/runs" --repo-dir "$repo" --base "$base"
-      --head "$CR_HEAD" --repo "$CR_REPO" --pr "$CR_PR" --summary "$out/pr-summary.md"
-      --secret-env OPENAI_API_KEY)
-if [ -n "${CODEX_HOME:-}" ]; then
-  post+=(--secret-file "$CODEX_HOME/auth.json" --secret-file "$work/auth.seed.json")
-fi
+      --head "$CR_HEAD" --repo "$CR_REPO" --pr "$CR_PR" --summary "$out/pr-summary.md" "${guard[@]}")
 if [ -n "${CR_DRY_RUN:-}" ]; then post+=(--dry-run "$CR_DRY_RUN"); fi
 if [ -n "${CR_TITLE:-}" ]; then post+=(--title "$CR_TITLE"); fi
 if [ -n "${CR_BOT:-}" ]; then post+=(--bot "$CR_BOT"); fi

.code-review/src/plugin/skills/review-orchestrator/SKILL.md

@@ -8,7 +8,7 @@ description: Use when asked to run a full code review of the change between two
 You run a code review by directing other agents. You do not review code yourself, you do not write
 or edit any findings file, and you do not edit the repository. Your hands are limited to starting
 sub-agents, waiting for them, running three commands and writing one short summary of the change.
-Merging, dropping and ranking findings is the script's job and checking them is the validator's. You
+Grouping, dropping and ordering findings is the script's job and checking them is the validator's. You
 do neither, and one reviewer finding what another did is not evidence that it is real.
 
 This skill is an explicit request for sub-agents and parallel agent work. Start the sub-agents

.code-review/src/plugin/skills/review-orchestrator/scripts/review_core/aggregator/cli.py

@@ -20,7 +20,7 @@ def _parser() -> argparse.ArgumentParser:
     """Build the argument parser for both commands."""
     parser = argparse.ArgumentParser(prog="review_core.aggregator.cli")
     sub = parser.add_subparsers(dest="command", required=True)
-    first = sub.add_parser("first-pass", help="gate, filter and dedup the reviewers' findings")
+    first = sub.add_parser("first-pass", help="gate, filter and group the reviewers' findings")
     first.add_argument("--out", required=True)
     first.add_argument("--repo", required=True)
     first.add_argument("--base", required=True)

.code-review/src/plugin/skills/review-orchestrator/scripts/review_core/aggregator/confidence.py

@@ -1,21 +1,14 @@
-"""Severity order and the confidence floor.
+"""The confidence floor.
 
 Confidence is never raised because several reviewers agree: they share one model family, so
 agreement is weak evidence.
 """
 
 from __future__ import annotations
 
-from review_core.contracts.models import SEVERITIES
-
 DEFAULT_FLOOR = 50
 
 
-def severity_rank(severity: str) -> int:
-    """Position of a severity, 0 for the worst (P0)."""
-    return SEVERITIES.index(severity)
-
-
 def passes_floor(finding: dict, floor: int) -> bool:
     """True when the finding's confidence is at least the floor."""
     return finding["confidence"] >= floor

.code-review/src/plugin/skills/review-orchestrator/scripts/review_core/aggregator/dedup.py

@@ -1,59 +1,60 @@
-"""Merge findings that point at the same code.
+"""Group findings that point at the same code; no finding is dropped for its place.
 
-Two findings are duplicates when they share a file and side and their line ranges overlap or
+Two findings share a place when they share a file and side and their line ranges overlap or
 lie within `window` lines. Groups are anchored on their first member, so a long run of close
 findings never chains into one group. A finding that claims more than MAX_SPAN lines stands
-alone: otherwise one reviewer could claim a whole file and absorb everyone else's findings.
+alone, so one reviewer claiming a whole file cannot gather everyone else's findings.
 """
 
 from __future__ import annotations
 
-from review_core.aggregator.confidence import severity_rank
+from collections.abc import Callable, Sequence
+from typing import Any
+
 from review_core.contracts.models import MAX_SPAN
 from review_core.envelope import MERGE_WINDOW
 
+Place = tuple[str, str, int, int]
+
 
-def _preference(finding: dict) -> tuple:
-    """Sort key that puts the finding to keep first: severity, confidence, line, reviewer."""
-    return (
-        severity_rank(finding["severity"]),
-        -finding["confidence"],
-        finding["line_start"],
-        finding["reviewer"],
-    )
+def _at(finding: dict) -> Place:
+    """A finding document's file, side, first line and last line."""
+    return finding["file"], finding["side"], finding["line_start"], finding["line_end"]
 
 
-def _groups(findings: list[dict], window: int) -> list[list[dict]]:
-    """Split findings into duplicate groups."""
-    by_place: dict[tuple[str, str], list[dict]] = {}
+def place_groups(
+    findings: Sequence[Any], window: int = MERGE_WINDOW, at: Callable[[Any], Place] = _at
+) -> list[list[Any]]:
+    """Split findings into groups at one place, in file, side and line order; `at` reads a
+    finding's place, so CI groups its `Finding` objects by the same rule."""
+    by_place: dict[tuple[str, str], list] = {}
     for finding in findings:
-        by_place.setdefault((finding["file"], finding["side"]), []).append(finding)
+        by_place.setdefault(at(finding)[:2], []).append(finding)
     groups = []
     for place in sorted(by_place):
-        ordered = sorted(by_place[place], key=lambda x: (x["line_start"], x["line_end"]))
-        open_group: list[dict] | None = None
+        ordered = sorted(by_place[place], key=lambda x: at(x)[2:])
+        open_group: list | None = None
         for finding in ordered:
-            if _too_wide(finding):
+            if _too_wide(at(finding)):
                 groups.append([finding])
-            elif open_group and finding["line_start"] <= open_group[0]["line_end"] + window:
+            elif open_group and at(finding)[2] <= at(open_group[0])[3] + window:
                 open_group.append(finding)
             else:
                 open_group = [finding]
                 groups.append(open_group)
     return groups
 
 
-def _too_wide(finding: dict) -> bool:
+def _too_wide(place: Place) -> bool:
     """True when the finding claims more lines than any single place holds."""
-    return finding["line_end"] - finding["line_start"] + 1 > MAX_SPAN
-
-
-def dedup(findings: list[dict], window: int = MERGE_WINDOW) -> tuple[list[dict], list[dict]]:
-    """Return (kept, removed). Each kept finding lists the other reviewers of its group."""
-    kept, removed = [], []
-    for group in _groups(findings, window):
-        winner = min(group, key=_preference)
-        others = sorted({x["reviewer"] for x in group if x is not winner} - {winner["reviewer"]})
-        kept.append({**winner, "also_raised_by": others})
-        removed.extend(x for x in group if x is not winner)
-    return kept, removed
+    return place[3] - place[2] + 1 > MAX_SPAN
+
+
+def group_by_place(findings: list[dict], window: int = MERGE_WINDOW) -> list[dict]:
+    """Every finding, in the order given, each listing the other reviewers with a finding at its
+    place; run again after verdicts so a declined finding's reviewer is no longer listed."""
+    others: dict[int, list[str]] = {}
+    for group in place_groups(findings, window):
+        reviewers = {x["reviewer"] for x in group}
+        others.update({id(x): sorted(reviewers - {x["reviewer"]}) for x in group})
+    return [{**x, "also_raised_by": others[id(x)]} for x in findings]

.code-review/src/plugin/skills/review-orchestrator/scripts/review_core/aggregator/pipeline.py

@@ -1,4 +1,4 @@
-"""The two aggregator passes: gate and merge, then fold in the validator's verdicts."""
+"""The two aggregator passes: gate and group by place, then fold in the validator's verdicts."""
 
 from __future__ import annotations
 
@@ -7,7 +7,7 @@
 from pathlib import Path
 
 from review_core.aggregator.confidence import DEFAULT_FLOOR, passes_floor
-from review_core.aggregator.dedup import dedup
+from review_core.aggregator.dedup import group_by_place
 from review_core.aggregator.gates import (
     GitReader,
     Reader,
@@ -49,10 +49,10 @@ def gate_and_merge(
     floor: int = DEFAULT_FLOOR,
     window: int = MERGE_WINDOW,
 ) -> tuple[list[dict], dict, list[dict]]:
-    """Apply the path gate, the quote gate, the confidence floor and the merge.
+    """Apply the path gate, the quote gate and the confidence floor, then group by place.
 
     `collected` is what `collect_findings` returns: the sound findings, and one reviewer name
-    per finding already dropped as malformed. Returns the merged findings in a fixed order,
+    per finding already dropped as malformed. Returns the kept findings in a fixed order,
     the count of drops by reason, and one row per dropped finding. Pure: it reads code and
     writes nothing.
     """
@@ -77,14 +77,13 @@ def drop(items: list[dict], reason: str) -> None:
         drop([x for x in gated if x["reason"] == reason], reason)
     strong = [x for x in kept if passes_floor(x, floor)]
     drop([x for x in kept if not passes_floor(x, floor)], "confidence_floor")
-    merged, removed = dedup(strong, window)
-    drop(removed, "duplicate")
+    merged = group_by_place(strong, window)
     merged.sort(key=lambda x: (x["file"], x["line_start"], x["side"], x["reviewer"]))
     return merged, dropped, detail
 
 
 def validator_input(merged: list[dict]) -> dict:
-    """What the validator is handed: the merged findings severe enough to need a verdict."""
+    """What the validator is handed: the kept findings severe enough to need a verdict."""
     to_check = [x for x in merged if x["severity"] in VALIDATED_SEVERITIES]
     return {
         "findings": [{"id": verdict_id(x), **{k: x[k] for k in VALIDATOR_FIELDS}} for x in to_check]
@@ -110,7 +109,7 @@ def first_pass(
     floor: int = DEFAULT_FLOOR,
     window: int = MERGE_WINDOW,
 ) -> dict:
-    """Gate, filter and dedup the findings; write candidates.json and validator-input.json.
+    """Gate, filter and group the findings; write candidates.json and validator-input.json.
 
     Raises ContractError when the output folder, repository or a ref is unusable.
     """
@@ -166,7 +165,8 @@ def rebuild(
         collect_findings(out, list(planned), ran), reader, floor, window
     )
     verdicts = (read_verdicts(out / "validation.json") if validator_ran else None) or {}
-    findings, declined = apply_verdicts(merged, verdicts)
+    kept, declined = apply_verdicts(merged, verdicts)
+    findings = group_by_place(kept, window)
     dropped["validator_declined"] += len(declined)
     run = build_envelope(out, list(planned), run_meta, ran)
     loose = unvalidated(findings, verdicts)
@@ -193,7 +193,7 @@ def _read_candidates(out: Path) -> dict:
     return doc
 
 
-def finalize(out_dir: Path | str, run_meta: dict) -> dict:
+def finalize(out_dir: Path | str, run_meta: dict, window: int = MERGE_WINDOW) -> dict:
     """Apply validation.json to the candidates and write the final findings.json.
 
     A P0 or P1 finding stays unless it is declined. If any is left without an upheld verdict
@@ -208,7 +208,8 @@ def finalize(out_dir: Path | str, run_meta: dict) -> dict:
     findings = candidates["findings"]
     verdicts = read_verdicts(out / "validation.json") or {}
     dropped = dict(candidates["dropped"])
-    findings, declined = apply_verdicts(findings, verdicts)
+    kept, declined = apply_verdicts(findings, verdicts)
+    findings = group_by_place(kept, window)
     dropped["validator_declined"] += len(declined)
     if declined:
         append_declined(out / "declined.json", declined)

.code-review/src/plugin/skills/review-orchestrator/scripts/review_core/aggregator/report.py

@@ -135,9 +135,9 @@ def _order(finding: Finding) -> tuple:
 
 
 def _raised_by(finding: Finding) -> str:
-    """The reviewer that kept the finding, then every other that raised it, in roster order."""
+    """The finding's reviewer, then the others with a finding on its lines, in roster order."""
     others = [r for r in REVIEWERS if r in finding.also_raised_by and r != finding.reviewer]
-    return ", ".join([finding.reviewer, *others])
+    return finding.reviewer + (f" (also on these lines: {', '.join(others)})" if others else "")
 
 
 def _finding_block(finding: Finding) -> list[str]:

.code-review/src/plugin/skills/review-orchestrator/scripts/review_core/aggregator/validate.py

@@ -29,12 +29,15 @@ def plain_file(path: Path) -> bool:
 
 
 def verdict_id(finding: dict) -> str:
-    """The name a verdict uses for a finding: its fingerprint and its lines.
+    """The name a verdict uses for a finding: its fingerprint, its lines and its reviewer.
 
     The fingerprint alone is not enough: the same line quoted at two places in a file gives two
-    findings one fingerprint. Two findings with the same lines are merged before this point.
+    findings one fingerprint, and two reviewers can report two problems on the same lines.
     """
-    return f"{finding['fingerprint']}:{finding['line_start']}-{finding['line_end']}"
+    # Review (pre-push), shown not to occur: one reviewer's two findings on one quote and lines
+    # would share an id, and none of 545 findings in 50 real trials did.
+    lines = f"{finding['line_start']}-{finding['line_end']}"
+    return f"{finding['fingerprint']}:{lines}:{finding['reviewer']}"
 
 
 def read_verdicts(path: Path | str) -> dict[str, dict] | None:
@@ -78,9 +81,9 @@ def apply_verdicts(
     """Return (kept, declined). A verdict acts only on the finding it names, and only on one
     the validator was given; anything else is ignored.
 
-    Findings over the span limit are never merged, so two can share an id. A decline that fits
-    more than one finding removes none: a finding is not dropped on a verdict that might have
-    been about another.
+    Two findings share an id only when one reviewer reports the same lines twice. A decline that
+    fits more than one finding removes none: a finding is not dropped on a verdict that might
+    have been about another.
     """
     given = [verdict_id(x) for x in findings if x["severity"] in VALIDATED_SEVERITIES]
     shared = {i for i in given if given.count(i) > 1}

.code-review/src/plugin/skills/review-orchestrator/scripts/review_core/contracts/models.py

@@ -48,7 +48,7 @@
     "malformed",
 )
 # A finding that claims more lines than this points at no particular place: it is not matched
-# against the key and it is never merged with a neighbour.
+# against the key and it is never grouped with a neighbour.
 MAX_SPAN = 30
 TIERS = (1, 2, 3)
 TAGS = ("visible_in_diff", "needs_other_files")
@@ -161,7 +161,7 @@ class FindingsDoc:
 
 def _finding(doc, where: str, *, need_fingerprint: bool) -> Finding:
     """Validate one finding object."""
-    final = need_fingerprint  # only the final document's findings carry the merge list
+    final = need_fingerprint  # only the final document's findings name reviewers at their place
     d = _fields(doc, (*FINDING_FIELDS, "also_raised_by") if final else FINDING_FIELDS, where)
     merged = _list(d, "also_raised_by", where) if final else []
     if any(not isinstance(name, str) or name not in REVIEWERS for name in merged):

.code-review/src/plugin/skills/review-orchestrator/scripts/review_core/envelope.py

@@ -26,9 +26,9 @@
 MAX_REVIEWER_FILE_BYTES = 5_000_000
 # No reviewer has this many real findings on one change; more is a flood, not a review.
 MAX_FINDINGS_PER_REVIEWER = 500
-# Findings in the same file and side count as one report when the later one starts no more than
-# this many lines after the earlier one ends: 0 means their line ranges share a line. A wider
-# window merged distinct defects a few lines apart, and bought almost nothing on tuning runs.
+# Findings in the same file and side share a place (one CI comment, never merged) when the later
+# one starts no more than this many lines after the earlier one ends: 0 means their line ranges
+# share a line. A wider window joined distinct defects a few lines apart.
 MERGE_WINDOW = 0
 
 DROP_COUNTS = DROP_REASONS
@@ -80,7 +80,7 @@ def fingerprint(finding: dict) -> str:
 
     Nothing a model words freely and no line number goes in, so the same quoted code gives the
     same fingerprint whoever raises it and however a later push moves it. Two places that quote
-    identical lines share a fingerprint; callers that need identity add the line range.
+    identical lines share a fingerprint; callers that need identity add the lines or reviewer.
     """
     path = safe_relpath(finding["file"]) or finding["file"]
     parts = [path, finding["side"], *quote_lines(finding["quote"])]

.code-review/src/plugin/skills/review-validator/SKILL.md

@@ -6,7 +6,7 @@ description: Tries to disprove each high-severity code review finding by reading
 # Review validator
 
 You are the second look at a code review. Seven reviewers have already written their findings
-and a script has merged them. Your job is to try to **disprove** the strongest claims, not to
+and a script has collected them. Your job is to try to **disprove** the strongest claims, not to
 find new problems. A false alarm that survives costs the reader more than a missed nit, so
 read each claim sceptically, but only decline one when you can say exactly why it is wrong.
 Work alone: do not read the reviewers' own findings files or `candidates.json`, which could

.code-review/src/review_core/aggregator/cli.py

@@ -20,7 +20,7 @@ def _parser() -> argparse.ArgumentParser:
     """Build the argument parser for both commands."""
     parser = argparse.ArgumentParser(prog="review_core.aggregator.cli")
     sub = parser.add_subparsers(dest="command", required=True)
-    first = sub.add_parser("first-pass", help="gate, filter and dedup the reviewers' findings")
+    first = sub.add_parser("first-pass", help="gate, filter and group the reviewers' findings")
     first.add_argument("--out", required=True)
     first.add_argument("--repo", required=True)
     first.add_argument("--base", required=True)

.code-review/src/review_core/aggregator/confidence.py

@@ -1,21 +1,14 @@
-"""Severity order and the confidence floor.
+"""The confidence floor.
 
 Confidence is never raised because several reviewers agree: they share one model family, so
 agreement is weak evidence.
 """
 
 from __future__ import annotations
 
-from review_core.contracts.models import SEVERITIES
-
 DEFAULT_FLOOR = 50
 
 
-def severity_rank(severity: str) -> int:
-    """Position of a severity, 0 for the worst (P0)."""
-    return SEVERITIES.index(severity)
-
-
 def passes_floor(finding: dict, floor: int) -> bool:
     """True when the finding's confidence is at least the floor."""
     return finding["confidence"] >= floor