Skip to content

fix(nc-review): make the verdict file the deliverable, retry once - #1211

Merged
will-lamerton merged 1 commit into
mainfrom
fix/nc-review-verdict-delivery
Sep 6, 2026
Merged

fix(nc-review): make the verdict file the deliverable, retry once#1211
will-lamerton merged 1 commit into
mainfrom
fix/nc-review-verdict-delivery

Conversation

@will-lamerton

Copy link
Copy Markdown
Member

Run 6 produced a thorough and accurate analysis, then threw it away.

The agent worked through nineteen numbered findings on stdout — including a correct walk through how umask interacts with mode: 0o600 — and then finished without ever calling write_file. It treated the chat as the output. Zero-byte verdict, safe fallback comment, run wasted.

Cause

The instruction to write the file was buried mid-prompt, behind the reading and analysis instructions. With the deeper rubric there is now a lot of analysis to do, so by the time the model finished thinking, "write the file" was a long way back.

Fix

The write instruction is now last and framed as the entire deliverable:

YOUR ONLY DELIVERABLE IS THE FILE .nc-review/verdict.json [...] Anything you write in chat is discarded and never reaches a human. Only the JSON file is read. Do not narrate your analysis; put your conclusions in the JSON. [...] You are not finished until write_file has succeeded.

The rubric's Output section says the same thing, since this is the one instruction whose failure costs a whole run.

Retry

Adds a single retry, scoped to exactly this failure: if verdict.json is missing or empty after the first attempt, run once more with a terse write-the-file prompt. Verified the guard fires on missing and empty, and not on valid JSON.

Deliberately not a general retry — a genuine agent error still falls through to the safe comment rather than burning another three minutes.

Stale labels

Unrelated bug spotted while reading the output. Applying no label on failure was the right call, but leaving the previous run's label was not: #1184 has carried agent:clean through two failed re-reviews, which reads as "an agent looked at this and was happy" when nothing of the kind happened.

The failure path now clears all three agent:* labels.

Run 6 produced a thorough, accurate analysis and threw it away. The agent
narrated nineteen numbered findings to stdout — including a correct walk through
umask interaction with mode 0600 — then finished without ever calling
write_file. It treated the chat as the output.

The instruction to write the file was buried mid-prompt behind the reading and
analysis instructions. It is now last, stated as the whole deliverable, and says
plainly that chat output is discarded and never reaches a human. The rubric's
Output section says the same, because that is the one instruction whose failure
costs an entire run.

Also adds a single retry, scoped to exactly this failure: if verdict.json is
missing or empty after the first attempt, run once more with a terse
write-the-file prompt. A genuine agent error still falls through to the safe
comment rather than being retried blindly. Roughly three minutes.

Separately, a failed run no longer leaves a stale label. Applying none on
failure was right, but #1184 has carried agent:clean through two failed
re-reviews, which reads as "an agent looked at this and was happy" when nothing
of the kind happened. The failure path now clears all three agent:* labels.

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.

@will-lamerton
will-lamerton merged commit 3a77f12 into main Sep 6, 2026
15 checks passed
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

nc-review: comments — 2 nits

The PR addresses a demonstrated failure mode of the review agent (running analysis in chat instead of writing the verdict file) by reordering the prompt to put the write instruction last, and adds a one-shot retry plus a stale-label cleanup in the same failure path. Both code changes are correct and tight; the prompt and rubric edits match the surrounding tone. One small inconsistency between the description and the code is worth flagging — see finding.

Area Where Finding
correctness .github/workflows/nc-review.yml The PR description claims 'a genuine agent error still falls through to the safe comment rather than burning another three minutes', but the guard [ ! -s .nc-review/verdict.json ] triggers on ANY non-empty-error / missing-file outcome — including auth failures, model outages, or tool errors that have nothing to do with the 'narrated instead of wrote' failure mode. In those cases the retry will re-run the exact same failing code path and consume another ~3 minutes before falling through. The trade-off is defensible (the demonstrated failure is the dominant one), but the description slightly misrepresents the retry's scope.
design .github/workflows/nc-review.yml The retry block duplicates the set +e / set -e / STATUS=$? / echo "agent_status=$STATUS" pattern from the first attempt. Extracting the run-and-capture into a small shell function would remove the duplication and make the two attempts easier to keep in sync (e.g., if the agent invocation flags ever change). Optional — the current code is clear enough.

🔴 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:comments nc-review left non-blocking findings label Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:comments nc-review left non-blocking findings area:ci GitHub Actions and CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant