fix(nc-review): three severity levels, verdict derived not trusted

Run 5 produced a good review and then mislabelled it. Five real findings, all
marked advisory, verdict "clean" — so a PR with five things to fix got
agent:clean, which is the label a maintainer skims. Copilot read the same PR as
needing amends and was right.

Two causes, both in the rubric I wrote.

1. Confidence and severity were conflated. The old text put "anything you are
   less than confident about" in the advisory bucket. Those are orthogonal: a
   finding can be high-impact and uncertain. Severity is now rated by impact
   alone, with uncertainty expressed in the detail text instead — and an
   explicit instruction not to file anything it cannot assert at all.

2. Binary severity forced the bad verdict. Findings that individually do not
   block a merge collapsed into "clean". There is now a middle level,
   `important`: a human reviewer would ask for a change before approving. The
   test-that-passes-without-exercising-anything case is given as the worked
   example, since a green suite makes that more dangerous rather than less.

The verdict is now derived from the findings in the workflow rather than taken
from the model: any blocking -> needs-work, any findings -> comments, none ->
clean. A mismatch is logged as a notice. Three labels, mutually exclusive, with
the other two removed unconditionally so a re-review cannot leave a stale pair.

The heading now carries the tally ("comments — 2 important, 3 nits") so it
cannot disagree with the table beneath it.

Replaying run 5's findings through the new logic yields
"comments — 2 important, 3 nits" and agent:comments.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PZY52ePXLjwG9TaQgq2cHT

Author: will-lamerton

.github/nc-review/rubric.md

@@ -113,21 +113,75 @@ Do not demand tests for docs-only, comment-only or config-only changes.
 - **Do not speculate.** If you did not read the code, do not assert a bug in it.
   Where you are unsure, say so and mark it `advisory` — an honest "worth
   checking" is useful; a confident wrong claim is not.
-- **Do not pad.** A clean PR gets a short summary and an empty findings list.
-  That is a good outcome, not a failure to find something.
+- **Do not pad.** If a pull request genuinely has nothing worth raising, say so
+  in a sentence and return an empty findings list. Do not invent a `nit` to look
+  thorough. But do not use this as an excuse to skip real findings either — see
+  the calibration note under Severity.
 - **Do not moralise.** Many contributors here are new. Findings are about the
   code, never about the person.
 
-## Severity
+## Severity — rate by impact, not by your confidence
 
-- `blocking` — a maintainer should not merge until this is resolved. Correctness
-  bugs, security problems, broken contracts, duplicates, a new feature with no
-  meaningful test.
-- `advisory` — worth raising; a maintainer may reasonably merge anyway. Style of
-  approach, minor edge cases, suggestions, anything you are less than confident
-  about.
+These are two different things and must not be mixed:
 
-If nothing is `blocking`, the verdict is `clean`.
+- **Severity** is how much the finding matters if it is true.
+- **Confidence** is how sure you are that it is true.
+
+Rate severity by **impact alone**. If you are unsure whether something is real,
+say so in the `detail` ("I could not verify whether X handles Y") — do not
+downgrade the severity to hedge. And if you are not confident enough to assert a
+finding at all, do not file it. A quiet omission is better than a confident
+error, but a hedged real finding is better than a silent one.
+
+Three levels:
+
+**`blocking`** — do not merge until this is resolved.
+
+- A correctness bug that will misbehave for real inputs
+- Any security problem
+- A broken public contract: CLI flags, config schema, tool interfaces, session
+  or `RunRecord` formats
+- A duplicate of another open PR
+- A new feature with no test at all
+
+**`important`** — a human reviewer would ask for a change before approving. Not
+catastrophic, but it should not merge as-is without a reason.
+
+- A test that does not actually exercise what it claims to — e.g. it asserts a
+  failure path that silently succeeds under some environments, so it passes
+  while proving nothing. The suite going green makes this *more* dangerous, not
+  less.
+- Removing existing coverage without replacing it
+- An unhandled edge case that a plausible user will hit
+- Logic that is correct today but fragile against a likely near-term change
+- Duplicated logic that must now be kept in sync in two places, where drift
+  would cause a real bug
+
+**`nit`** — genuinely optional. The author may ignore it.
+
+- Naming, comment wording, a documentation inconsistency with no behavioural
+  effect
+- A self-healing race with no security or correctness impact
+- Preference about structure where the current approach is defensible
+
+**Calibration.** If you find yourself marking everything `nit`, you are
+under-calling. Ask of each finding: *would a careful human reviewer ask for a
+change before approving?* If yes, it is at least `important`. "The maintainer
+could merge this anyway" is true of almost everything and is not the test.
+
+## Verdict
+
+Derived mechanically from the findings — do not set it by feel:
+
+| Verdict | When |
+|---|---|
+| `clean` | **no findings at all** |
+| `comments` | at least one finding, none `blocking` |
+| `needs-work` | at least one `blocking` finding |
+
+`clean` means you have nothing to say. A pull request with five things worth
+fixing is **not** clean, even if none of them block the merge — labelling it
+clean tells a maintainer to skim past findings you spent the run producing.
 
 ## Output
 
@@ -136,23 +190,24 @@ before or after, no markdown fences. Schema:
 
 ```json
 {
-  "verdict": "clean",
+  "verdict": "comments",
   "summary": "Two or three sentences. What the change does, whether it is correct, and whether it is ready.",
   "findings": [
     {
-      "area": "correctness",
-      "severity": "blocking",
-      "file": "source/tools/execute-bash.ts",
-      "line": 142,
+      "area": "tests",
+      "severity": "important",
+      "file": "source/vscode/discovery.spec.ts",
+      "line": 509,
       "detail": "Specific and actionable. What is wrong, why it matters, and what would fix it."
     }
   ],
   "duplicate_of": null
 }
 ```
 
-- `verdict` — `"clean"` or `"needs-work"`. `needs-work` if and only if at least
-  one finding is `blocking`.
+- `verdict` — `"clean"`, `"comments"` or `"needs-work"`, derived from the
+  findings per the table above.
+- `severity` — `"blocking"`, `"important"` or `"nit"`.
 - `area` — one of `correctness`, `security`, `design`, `tests`, `duplicate`,
   `scope`, `changeset`, `contributing`.
 - `file` / `line` — where the finding is. Omit both if it is not tied to a

.github/workflows/nc-review.yml

@@ -230,8 +230,9 @@ jobs:
         env:
           GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         run: |
-          gh label create "agent:clean"      --color "0e8a16" --description "nc-review found no blocking issues" --force
-          gh label create "agent:needs-work" --color "d93f0b" --description "nc-review found blocking issues"    --force
+          gh label create "agent:clean"      --color "0e8a16" --description "nc-review had nothing to raise"          --force
+          gh label create "agent:comments"   --color "fbca04" --description "nc-review left non-blocking findings"    --force
+          gh label create "agent:needs-work" --color "d93f0b" --description "nc-review found blocking findings"       --force
 
       # Nanocoder resolves providers from `agents.config.json` in the working
       # directory first. This is written at run time rather than committed: a
@@ -357,16 +358,42 @@ jobs:
             exit 0
           fi
 
-          VERDICT=$(jq -r '.verdict // "needs-work"' "$V")
           SUMMARY=$(jq -r '.summary // "No summary provided."' "$V")
           DUP=$(jq -r '.duplicate_of // empty' "$V")
 
+          N_BLOCK=$(jq '[.findings[]? | select(.severity == "blocking")] | length' "$V")
+          N_IMP=$(jq   '[.findings[]? | select(.severity == "important")] | length' "$V")
+          N_NIT=$(jq   '[.findings[]? | select(.severity == "nit")] | length' "$V")
+          N_ALL=$(jq   '.findings | length' "$V")
+
+          # Derive the verdict from the findings rather than trusting the model's
+          # own label. Run 5 returned verdict "clean" alongside five findings,
+          # which put an agent:clean label on a PR that had five things to fix —
+          # and the label is what a maintainer skims.
+          if [ "$N_BLOCK" -gt 0 ]; then
+            VERDICT="needs-work"
+          elif [ "$N_ALL" -gt 0 ]; then
+            VERDICT="comments"
+          else
+            VERDICT="clean"
+          fi
+
+          CLAIMED=$(jq -r '.verdict // "(none)"' "$V")
+          [ "$CLAIMED" != "$VERDICT" ] && \
+            echo "::notice::model said '$CLAIMED', findings imply '$VERDICT' — using '$VERDICT'"
+
+          # Human-readable tally, e.g. "1 blocking, 2 important, 2 nits".
+          COUNTS=""
+          [ "$N_BLOCK" -gt 0 ] && COUNTS="${N_BLOCK} blocking"
+          [ "$N_IMP" -gt 0 ] && COUNTS="${COUNTS:+$COUNTS, }${N_IMP} important"
+          [ "$N_NIT" -gt 0 ] && COUNTS="${COUNTS:+$COUNTS, }${N_NIT} nit$([ "$N_NIT" -gt 1 ] && echo s)"
+
           {
-            if [ "$VERDICT" = "clean" ]; then
-              echo "### nc-review: no blocking issues"
-            else
-              echo "### nc-review: needs work"
-            fi
+            case "$VERDICT" in
+              needs-work) echo "### nc-review: needs work — ${COUNTS}" ;;
+              comments)   echo "### nc-review: comments — ${COUNTS}" ;;
+              clean)      echo "### nc-review: nothing to raise" ;;
+            esac
             echo
             echo "$SUMMARY"
             echo
@@ -378,21 +405,29 @@ jobs:
               echo "| | Area | Where | Finding |"
               echo "|---|---|---|---|"
               jq -r '.findings[] |
-                "| \(if .severity == "blocking" then "🔴" else "🟡" end)"
+                "| \(if .severity == "blocking" then "🔴" elif .severity == "important" then "🟠" else "⚪" end)"
                 + " | `\(.area)`"
                 + " | \(if .file then "`\(.file)\(if .line then ":\(.line)" else "" end)`" else "—" end)"
                 + " | \(.detail | gsub("\n"; " ")) |"' "$V"
               echo
             fi
             echo "---"
             echo
-            echo "<sub>Automated code review — correctness, security, design, tests, plus duplicates and scope. Advisory: a human still decides. Not a substitute for review, and not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with \`/re-review\`.</sub>"
+            echo "<sub>🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional</sub>"
+            echo
+            echo "<sub>Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with \`/re-review\`.</sub>"
           } > /tmp/nc-review/comment.md
 
           gh pr comment "$PR" --repo "$REPO" --body-file /tmp/nc-review/comment.md
 
-          if [ "$VERDICT" = "clean" ]; then
-            gh pr edit "$PR" --repo "$REPO" --add-label "agent:clean" --remove-label "agent:needs-work" || true
-          else
-            gh pr edit "$PR" --repo "$REPO" --add-label "agent:needs-work" --remove-label "agent:clean" || true
-          fi
+          # Exactly one agent:* label should ever be present. Remove the other
+          # two unconditionally so a re-review cannot leave a stale pair.
+          case "$VERDICT" in
+            clean)      KEEP="agent:clean";      DROP="agent:comments agent:needs-work" ;;
+            comments)   KEEP="agent:comments";   DROP="agent:clean agent:needs-work" ;;
+            needs-work) KEEP="agent:needs-work"; DROP="agent:clean agent:comments" ;;
+          esac
+          ARGS="--add-label $KEEP"
+          for d in $DROP; do ARGS="$ARGS --remove-label $d"; done
+          # shellcheck disable=SC2086
+          gh pr edit "$PR" --repo "$REPO" $ARGS || true