Skip to content

nc-review: severity calibration, issue resolution, and warranted checks #1209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
will-lamerton merged 2 commits into from
Sep 6, 2026
Merged

nc-review: severity calibration, issue resolution, and warranted checks #1209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06d716a
fix(nc-review): three severity levels, verdict derived not trusted
will-lamerton Sep 6, 2026
d4ba093
feat(nc-review): judge issue resolution and whether the PR was warranted
will-lamerton Sep 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 132 additions & 22 deletions .github/nc-review/rubric.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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
Expand All @@ -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

Expand All @@ -136,25 +245,26 @@ 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`.
- `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.
101 changes: 86 additions & 15 deletions .github/workflows/nc-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -378,21 +441,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
Loading