PR #578 follow-ups: review_now.py failure reporting and the armed STALE-DISPATCH boxes nf-9zv1 ← Beads

open priority 2 task unassigned

Twinned from https://github.com/cfsh/ninefold/issues/581 by tools/beads/import_github.py's reconcile pass.

Suggestions from ninefold-reviewer's review of #578. All advisory — #578 converged and none of these blocked it. Unclaimed.

1. conclusion() can never report a fresh spawn failure (tools/pr/review_now.py:290)

The "backoff" in out branch is tested before f"#{number}: FAILED", and the dispatcher's failure line itself contains the word (tools/reviewer/dispatch.py:755):

> #N: FAILED — see <log> (backoff 45m; rev:pending stays on; --force retries)

So a spawn that just failed is summarised as *"a previous spawn FAILED and this PR is in backoff"* — wrong tense, wrong cause, and it drops the log pointer the FAILED branch exists to give. That branch is unreachable in practice, and untested, which is how it survived.

Fix: swap the two branches, or match skip ( on the backoff arm. Add the missing ConclusionTest case.

2. review_now.py exits 0 when the review spawn failed

sweep_one turns a spawn failure into a printed line and main() still returns 0, so result.ok is true and review_now.py returns 0. Only the text is truthful — anything keying on the exit code reads a failed review as a successful one.

3. Boxes already carrying a stray ~/.ninefold-reviewer/ stay armed

tearDownModule's _REAL_STATE_DIR_PREEXISTED short-circuit makes the new guard inert on exactly the population #578 identifies as affected (the WSL office box, and any box that ran --suite python before the fix). There the false ⚠ STALE-DISPATCH continues indefinitely and says nothing about its cause, while notes/poll-loop.md:22 routes the session to a schtasks job that was never deployed there.

Two lines close it:

4. ✓ verdict posted is asserted from #N: done, which can lie

fresh_token()'s docstring documents the case: an expired or revoked token 401s every gh call while claude -p still exits 0, so the sweep prints done, drops the lock, and posts nothing. preflight() checks pem *presence*, not validity — correctly, since minting per invocation was rejected — so it does not cover this, and the wrapper then upgrades an ambiguous line into a definite claim.

Cheap confirmation if wanted: after done, one gh pr view <n> for a bot verdict at the head sha.

5. Self-sync's hard reset is now routine and concurrent, not a rare hand-run

review_now.py:324's comment rates the exposure unchanged, and it is — but the frequency is not. git reset --hard in the reviewer's clone now happens at dev-agent cadence, and can land while a scheduled tick is mid-review reading that tree. Real-world delta is usually nil (both ends reset to origin/main; the CHARTER is read into the prompt before the review starts), so this is a watch item rather than a defect. A fetch-then-compare skip when the clone is already at origin/main would remove it entirely.

---

Not filed, deliberately: the _drive helper sitting above its class's first test means the meta-guard scans it as part of the *previous* class's last test block. It passes either way and causes no false result.

Dependencies

None.

Comments

No comments.

Add a comment