Twinned from https://github.com/cfsh/ninefold/issues/719 by tools/beads/import_github.py's reconcile pass.
Suggestions from ninefold-reviewer's review of #701 (poll: notice when the integration build goes stale). All four are advisory residuals left at CONVERGED — none blocked the merge, none is a behaviour bug today. Filed unclaimed.
Common thread: three of the four are the class tools/pr/poll.py names about itself in its own reviewer-liveness comment — *"a comment explaining WHY something is absent becomes false the moment someone adds it, and nothing type-checks prose"* — and #701 already fixed two instances of it across its own review rounds.
f5/integration ref" guard cannot fire on a normal clone, and its stated reason is falsetools/pr/poll.py f5_health():
```python if not git(["git", "fetch", "-q", "origin", "main", "f5/integration"], ...).ok: return "" if not git(["git", "rev-parse", "--verify", "-q", F5_REF], check=False, cwd=root).ok: return "" ```
The fetch runs first and names f5/integration explicitly. On any clone with the default +refs/heads/*:refs/remotes/origin/* refspec, git opportunistically writes refs/remotes/origin/f5/integration for a named branch it just fetched — so by the time rev-parse runs, the ref exists. If the *remote* has no such branch the fetch itself fails (couldn't find remote ref f5/integration) and the earlier guard has already returned "".
The residual reachable case is a --single-branch clone, which nothing in this repo documents using.
What is wrong is the justification, in three places:
f5_health docstring — *"a clone that has never built one. Every coder worktree here shares a .git that has it, but Aedan's workstation and a fresh clone do not"*. A fresh clone has it one line later.notes/f5.md — *"Every ambiguity (offline, no f5/integration ref, an unreadable log) answers silence"*. The middle one is not an ambiguity the code can meet.tools/tests/test_poll.py::test_a_clone_with_no_f5_ref_is_silent, which stubs a fetch-succeeds/rev-parse-fails combination production does not produce.Suggested: keep the guard (cheap, and it does cover the single-branch case) but correct the prose to say what actually makes it silent — the *fetch* failing when the remote has no f5/integration — and re-point the test at that.
_quiet_git's except OSError is dead code, and the test named for it cannot fail_quiet_git docstring: *"OSError is caught because check=False only covers a non-zero exit; a missing git on PATH raises instead."*
nflib.proc.run refutes that in its own docstring and its own code: with check=False it converts FileNotFoundError to Result(code=127) and OSError to Result(code=126) rather than raising, precisely so *"every caller [does not need] its own except-clause — which is the boilerplate this module exists to delete."* _quiet_git passes check=False unconditionally, so its except OSError is unreachable.
test_the_default_runner_never_raises_on_a_missing_binary asserts only assertFalse(got.ok), which holds via run's own 127 path — delete the except clause and the test stays green. Same shape as the -q minor already fixed on the thread.
Suggested: drop the clause and the claim, or keep it as explicit belt-and-braces and say *that* — but then the test should reach it (drive _quiet_git with a run stubbed to raise).
assertIn("fatal", got.err) is locale-dependenttest_the_default_runner_does_not_ECHO_git_s_stderr asserts git's literal English fatal. git translates that prefix under a non-C locale, so this is a machine-property assertion of the kind #565 is open about. assertTrue(got.err.strip()) proves the same thing — *git wrote something to stderr and we swallowed it* — without the premise.
--check one doestest_a_stale_f5_HOLDS_the_cadence_at_30m_NAMING_ITSELF catches F1 through assertNotIn("dispatcher looks dead", out). Its digest counterpart, test_the_FULL_DIGEST_holds_the_cadence_too_not_just_the_check, asserts only the interval (every 30m, not every 4h / (hourly)) — so reintroducing dispatch_bad or f5_bad at *that* call site would be caught at one site and not the other. Which is #437 F4's shape one level up, in the guard rather than the code.
Low risk now that cadence takes f5_bad as a required fourth parameter. One assertNotIn("dispatcher looks dead", out) closes it.
None.
No comments.