Skip to content

fix(nc-review): stop set -e killing the step on exactly one nit - #1214

Merged
will-lamerton merged 1 commit into
mainfrom
fix/nc-review-counts-set-e
Sep 6, 2026
Merged

fix(nc-review): stop set -e killing the step on exactly one nit#1214
will-lamerton merged 1 commit into
mainfrom
fix/nc-review-counts-set-e

Conversation

@will-lamerton

Copy link
Copy Markdown
Member

The review job failed on #1213 after producing a correct verdict. The agent wrote the file, the workflow read it, then the step exited 1 having printed nothing at all.

Cause

COUNTS="...${N_NIT} nit$([ "$N_NIT" -gt 1 ] && echo s)"

With exactly one nit, the inner [ 1 -gt 1 ] returns 1, the command substitution inherits that status, and under set -e an assignment fails with the status of its substitution. The step died on that line.

Nothing appeared in the log because every echo after that point is redirected into comment.md — so a hard failure looked like a step that silently did nothing.

It fires only when N_NIT == 1. Zero nits short-circuits at the [ "$N_NIT" -gt 0 ] guard before reaching the substitution, which is why this survived six earlier runs and then broke on the first PR that produced a single nit.

Fix

Plain if blocks, with explicit nit / nits selection. The CLAIMED mismatch notice above it had the identical hazard ([ ... ] && echo) and got the same treatment.

Verified

Extracted the real step from the workflow and ran it against eight combinations — 0/0/0, 1/0/0, 0/1/0, 0/0/1, 0/0/2, 1/1/1, 2/3/4, 0/2/1. All exit 0 with the expected heading and correct pluralisation.

The review job failed on nanocoder#1213 after producing a correct verdict. The
agent wrote the file, the workflow read it, and then the step exited 1 having
printed nothing at all.

Cause:

  COUNTS="...${N_NIT} nit$([ "$N_NIT" -gt 1 ] && echo s)"

With exactly one nit the inner test returns 1, the command substitution inherits
that status, and under `set -e` an assignment fails with the status of its
substitution. The step died there. Nothing appeared in the log because every
echo after that point is redirected into comment.md, so the failure looked like
a step that did nothing.

It only ever fires on N_NIT == 1. Runs with zero nits short-circuit at the guard
before reaching the substitution, which is why this survived six earlier runs.

Replaced with plain if-blocks, and the same shape applied to the CLAIMED notice
above it, which had the identical hazard.

Verified across eight count combinations: 0/0/0, 1/0/0, 0/1/0, 0/0/1, 0/0/2,
1/1/1, 2/3/4, 0/2/1. All exit 0 and render the expected heading, including
correct nit/nits pluralisation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PZY52ePXLjwG9TaQgq2cHT
@github-actions github-actions Bot added the area:ci GitHub Actions and CI label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

No changeset found

This PR does not add a changeset, so it will not appear in the changelog or trigger a release.

If the change is user-facing, add one:

pnpm changeset

Pick a bump (patch / minor / major) and write the changelog entry in our usual voice ("Added X... Thanks to @you. Closes #123."), then commit the generated .changeset/*.md file.

If this PR is docs-only or a chore that needs no release note, you can ignore this - or run pnpm changeset --empty to record that intentionally.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

nc-review: nothing to raise

Fixes a real set -e hazard in the review workflow: when exactly one nit was emitted, the inner [ 1 -gt 1 ] in the N_NIT pluralisation returned 1, the command substitution inherited that status, and the outer assignment failed under set -e — silently, because every later echo is redirected into comment.md. The fix replaces the [ ... ] && VAR=... patterns with plain if/then blocks and uses explicit nit/nits selection. It also covers the analogous CLAIMED mismatch notice, which had the same shape. Author's diagnosis is correct, fix is idiomatic, and no other similar hazards remain in the file.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

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.

@github-actions github-actions Bot added the agent:clean nc-review had nothing to raise label Sep 6, 2026
@will-lamerton
will-lamerton merged commit 0b8f978 into main Sep 6, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:clean nc-review had nothing to raise area:ci GitHub Actions and CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant