PR #780 follow-ups: the state-dir short-circuit, and two comments that overstate their tests nf-6mo ← Beads

open priority 2 task unassigned

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

Suggestions from ninefold-reviewer's review of #780 (design 006 piece 12 — the two health alarms rehomed to rebuild_f5.py --health and review_now.py). None of these blocked that PR; they are the residuals from a CONVERGED verdict, filed so the tension leaves the thread. Unclaimed.

1. The schedule gate opens, and dispatch_health closes it again when the state dir is absent

tools/pr/review_now.pydispatcher_line() consults scheduled_dispatcher_here(), which now asks the scheduler and is authoritative. It then calls dispatch_health(heartbeat_path(), …), whose first branch is:

```python if not path.exists(): if path.parent.is_dir(): ...report... return "" ```

So on a box where the job is deployed and ~/.ninefold-reviewer/ does not exist, the answer is silence.

That is the state scheduled_dispatcher_here's own docstring names as the reason the heartbeat gate was wrong: *"a job installed but never completing a first tick (a bad definition, an unresolvable python, a $HOME the writer cannot stamp) writes no heartbeat by construction."* Two of those three never create the directory either — the Linux crontab line runs flock -n /root/.ninefold-reviewer/dispatch.lock ..., and the deployment in tools/reviewer/README.md:307 redirects >> ~/.ninefold-reviewer/dispatch.log. Both fail *before* dispatch.py runs, so neither write_heartbeat's path.parent.mkdir (tools/reviewer/dispatch.py:470) nor the worktree mkdir is ever reached. DEVEL.md separately tells an operator not to hand-create that directory, so a fresh reviewer host lands in exactly this state.

Not a regression — poll.py behaved identically — and narrow enough that it did not gate #780. But path.parent.is_dir() is the same *"is a dispatcher deployed here?"* proxy the new gate now answers properly, one level down, and its "no" overrides the gate's "yes". DEV_PRINCIPLES B7: delete the proxy rather than stack a better heuristic in front of it. Note test_a_host_with_NO_heartbeat_EVER_still_reports passes only because setUp mkdirs self.home — the absent-dir case has no test.

2. MainDispatcherLineTest does stub the probe

tools/tests/test_review_now.py — the class docstring says *"⚠ The only place the probe is NOT stubbed, which is why every other main-driving class stubs it"*, and #780's body repeats it. _main() does R.dispatcher_line = lambda *a, **k: line in every case, so the probe is stubbed there too; what is unique is that it is stubbed to a non-empty value, which is what lets the print wiring be asserted.

Worth correcting because of what the wrong version invites: a reader who believes the real probe is exercised somewhere may drop the dispatcher_line stubs from InvocationTest/ReconcileOnlyTest, which puts every main case back on the live crontab and the live ~/.ninefold-reviewer/ — the finding those stubs were added for.

3. _quiet's except OSError is unreachable

tools/pr/review_now.py_quiet() wraps run(argv, check=False) in try/except OSError, but nflib.proc.run already converts both FileNotFoundError (→ code 127) and OSError (→ 126) into Results before returning (tools/nflib/proc.py:175-197), and check=False means it never raises. The behaviour is right; the guard is dead and its comment describes a path run already closed. B12.

4. "Check the schtasks job." reaches a Linux operator

dispatch_health's never-written outage line ends *"The dispatcher is not running. Check the schtasks job."* — moved verbatim, so pre-existing, but the live reviewer host is Linux and the same class was corrected in #780's newly-authored notes/operating.md prose, which now names crontab -l and schtasks per platform. Naming both here would finish the job.

5. Three files outside the Tier-1 perimeter now describe the old arrangement

Correctly left out of #780, recorded so they are not lost: tools/reviewer/README.md's heartbeat table still says *"digest says ..."*; tools/tests/test_dispatch.py's tearDownModule message still explains the leftover state dir in terms of the dev digest's alarm; tools/nflib/fakes.py's module docstring uses *"the digest said STALE-DISPATCH"* as its worked example of an outcome assertion. All three are GOVERNANCE-denied paths — a change for whoever owns those trees.

6. Cosmetic

FRESH is a very generic module-level name in a ~1900-line file (tools/pr/rebuild_f5.py); F5_FRESH or similar would read better beside F5_HEALTH_REF/HEALTH_REMEDY. Explicitly waived on the #780 thread — listed here only so it is written down once.

Dependencies

None.

Comments

No comments.

Add a comment