diff --git a/.github/nc-review/rubric.md b/.github/nc-review/rubric.md
index 50ca1109..631d916b 100644
--- a/.github/nc-review/rubric.md
+++ b/.github/nc-review/rubric.md
@@ -4,8 +4,9 @@ You are reviewing a pull request to Nanocoder on behalf of the maintainers.
Do a **real code review**. Read the diff properly, read the surrounding source
to understand what the changed code interacts with, and judge whether the change
-is correct, safe, and well-made. Then also judge the contribution around it —
-duplicates, scope, tests, changeset.
+is correct, safe, and well-made. Then judge the contribution around it: whether
+it actually resolves the issue it claims to close, whether it was warranted at
+all, and whether it duplicates other work in flight.
You never merge, close, or push. You produce one JSON verdict.
@@ -89,7 +90,59 @@ regressed. If not, say so.
Do not demand tests for docs-only, comment-only or config-only changes.
-### 5. Contribution hygiene
+### 5. Does it actually resolve the linked issue?
+
+If the PR closes an issue, the issue text is in the context. Read it and compare
+it to the diff. Ask: **if this merges, is the problem described in that issue
+gone?**
+
+This is not the same as scope. Scope asks whether it did *more* than the issue;
+this asks whether it did *all* of it.
+
+- A partial fix on a PR that says "Closes #N" is `important` at minimum. Merging
+ it closes the issue, and whatever is left over is silently lost — nobody
+ returns to a closed issue.
+- A fix that addresses the symptom in the issue's reproduction steps but not the
+ underlying cause the issue describes is worth raising.
+- If the issue lists several distinct problems and the PR handles one, say which
+ ones remain.
+- If the diff does something *other* than what the issue asked, that is a
+ correctness problem in the framing, not just scope creep.
+
+Where the issue is vague and several readings are defensible, say so rather than
+inventing a standard the author could not have known.
+
+### 6. Was the pull request warranted?
+
+Does this change deserve to exist? Maintainer attention is the scarcest resource
+in this project, and a PR that should not have been opened costs it whether or
+not the code is correct.
+
+Consider:
+
+- **Is there a real problem here?** A linked issue is good evidence. Without
+ one, is the change self-evidently worth making — a genuine bug, a clear
+ improvement — or is it speculative?
+- **Does the codebase already do this?** Search before concluding. A
+ reimplementation of something that already exists is worth flagging even when
+ the new code is fine.
+- **Is it churn?** Renaming, reformatting, reorganising or "improving" working
+ code with no behavioural benefit costs review time and creates conflicts with
+ real work in flight. CONTRIBUTING asks contributors to discuss substantial
+ changes in an issue first.
+- **Does it fight the project's direction?** Judge against `CLAUDE.md` and
+ `CONTRIBUTING.md`, not against your own preferences.
+
+Be careful and fair here. Small PRs from first-time contributors are how people
+start, and "this is small" is not a reason to reject anything. The question is
+whether the change is *justified*, not whether it is large. If it is justified
+but simply undiscussed, say that — the fix is a conversation, not a rejection.
+
+If you are unsure, do not file this as a finding. An unwarranted-PR claim is the
+most discouraging thing you can say to a contributor, and you should only say it
+when the case is clear.
+
+### 7. Contribution hygiene
- **Duplicates.** The open PR list is provided. Two PRs touching the same files
are not necessarily duplicates — two PRs *solving the same problem* are. Name
@@ -113,21 +166,77 @@ 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
+
+These are two different things and must not be mixed:
+
+- **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
+- A PR that claims to close an issue but leaves part of it unfixed, where the
+ remainder would be lost when the issue auto-closes
+
+**`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:
-- `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.
+| Verdict | When |
+|---|---|
+| `clean` | **no findings at all** |
+| `comments` | at least one finding, none `blocking` |
+| `needs-work` | at least one `blocking` finding |
-If nothing is `blocking`, the verdict is `clean`.
+`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,14 +245,14 @@ 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."
}
],
@@ -151,10 +260,11 @@ before or after, no markdown fences. Schema:
}
```
-- `verdict` — `"clean"` or `"needs-work"`. `needs-work` if and only if at least
- one finding is `blocking`.
-- `area` — one of `correctness`, `security`, `design`, `tests`, `duplicate`,
- `scope`, `changeset`, `contributing`.
+- `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`, `completeness`,
+ `warranted`, `duplicate`, `scope`, `changeset`, `contributing`.
- `file` / `line` — where the finding is. Omit both if it is not tied to a
specific location. Never guess a line number; omit it instead.
- `duplicate_of` — PR number as an integer, or `null`. Only when confident.
diff --git a/.github/workflows/nc-review.yml b/.github/workflows/nc-review.yml
index 6f29779a..d5a17b57 100644
--- a/.github/workflows/nc-review.yml
+++ b/.github/workflows/nc-review.yml
@@ -176,6 +176,26 @@ jobs:
| jq --argjson me "$PR" 'map(select(.number != $me))' \
> /tmp/nc-review/open-prs.json
+ # Issues this PR claims to close, with their full bodies — the agent
+ # cannot judge whether the issue was actually addressed without them.
+ # Only GraphQL exposes closingIssuesReferences; `gh pr view --json`
+ # does not have the field.
+ OWNER="${REPO%%/*}"; NAME="${REPO##*/}"
+ gh api graphql -f query="
+ {
+ repository(owner: \"$OWNER\", name: \"$NAME\") {
+ pullRequest(number: $PR) {
+ closingIssuesReferences(first: 10) {
+ nodes { number title state body labels(first: 10) { nodes { name } } }
+ }
+ }
+ }
+ }" --jq '.data.repository.pullRequest.closingIssuesReferences.nodes' \
+ > /tmp/nc-review/issues.json 2>/dev/null || echo '[]' > /tmp/nc-review/issues.json
+ [ -s /tmp/nc-review/issues.json ] || echo '[]' > /tmp/nc-review/issues.json
+ N_ISSUES=$(jq 'length' /tmp/nc-review/issues.json 2>/dev/null || echo 0)
+ echo "linked issues: $N_ISSUES"
+
{
echo "# Pull request under review"
echo
@@ -210,6 +230,22 @@ jobs:
echo
jq -r '.body // "(no description provided)"' /tmp/nc-review/pr.json
echo
+ echo "## Linked issue(s) — what this PR claims to close"
+ echo
+ if [ "$N_ISSUES" -eq 0 ]; then
+ echo "**This pull request closes no issue.** It may still be warranted —"
+ echo "a typo fix or a small obvious improvement does not need one — but you"
+ echo "should judge whether the change is justified on its own terms, and"
+ echo "whether CONTRIBUTING's guidance to discuss first applies given its size."
+ else
+ echo "Judge whether the diff actually resolves what is described below."
+ echo "A PR that says it closes an issue and only partly does is a problem:"
+ echo "merging it closes the issue and the remainder is silently lost."
+ echo
+ jq -r '.[] | "### Issue #\(.number) — \(.title)\n\nState: \(.state)\nLabels: \([.labels.nodes[].name] | join(", "))\n\n\(.body // "(no body)")\n"' \
+ /tmp/nc-review/issues.json
+ fi
+ echo
echo "## Other open pull requests (duplicate-detection corpus)"
echo '```json'
cat /tmp/nc-review/open-prs.json
@@ -230,8 +266,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 +394,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,7 +441,7 @@ 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"
@@ -386,13 +449,21 @@ jobs:
fi
echo "---"
echo
- echo "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\`."
+ echo "🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional"
+ echo
+ echo "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\`."
} > /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