PR #472 follow-ups: a comment that outlived its code, unused import, same-sha assumptions nf-5uy6 ← Beads

open priority 2 task unassigned

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

Suggestions from ninefold-reviewer's review of #472 (result=CONVERGED at 1175c80). Unclaimed and unlabelled — inert backlog, per CLAUDE.md. None of these blocked the merge; they are what is left after F1/F2/F3, N1 and G1–G3 were all closed on the branch.

1. A comment claims a saving the code does not make

tools/pr/probe_gate.py:432–434, added when load_baseline moved above the sweep:

> ⚠ The baseline is read BEFORE the sweep now, because the skip below needs it. It also means a failed fetch stops us paying for 3-4 minutes of probes whose result we have already decided not to trust.

The second sentence is not what happens. A failed fetch clears record_ok and leaves have_baseline False, so the already-recorded skip cannot fire (:459) and the sweep runs to completion — correctly, since load_baseline's own docstring says the report direction is best-effort and an unreachable remote must not turn a successful merge into a scary failure.

So the behaviour is right and the comment describes a different program. Worth fixing rather than ignoring because prose that outlives the condition it was written under is exactly #412 body item 1, and this file is the one that fixed it.

Either drop the sentence, or make it true by returning early when not may_record — but that second option is a real behaviour change and probably the wrong one.

2. env_flag is imported and unused

tools/pr/probe_gate.py:77:

```python from nflib import env_flag, repo_root # noqa: E402 (env_flag: SKIP_PROBE_GATE) ```

env_flag is never called in this file. SKIP_PROBE_GATE is handled by merge_pr.py:486, before the gate is invoked at all. The noqa covers E402 (import position), so no linter flags the unused name — and the python lane runs unittest, not a linter, so nothing else will either.

Minor, but noting it because #472's reply listed env_flag among the unused imports dropped; the other three (os, re, dataclasses.field) were.

3. The already-recorded skip assumes "same sha ⇒ same tree"

probe_gate.py:459:

```python if record_ok and have_baseline and baseline.get("sha") == sha: ```

True of a committed tree, false of a dirty one. Run bare on a main checkout with uncommitted changes, the gate prints already recorded — skipping the sweep and exits 0 having tested nothing.

Low reachability: merge_pr hard-resets to origin/main before invoking the gate, CLAUDE.md forbids working on main, and the documented manual invocation is --dry-run, which clears record_ok and so bypasses the skip entirely. A git status --porcelain check on the skip path would close it.

Same shape as the dirty-tree item on presubmit's same-sha path in #446 — worth solving once for the tier rather than twice.

4. The baseline is read 3–4 minutes before the delta uses it

load_baseline now runs before the sweep (it must, for the skip). A concurrent session that records a baseline *during* our sweep is therefore invisible to us: we diff against the pre-record map, so a failure it already recorded can surface as ████ NEW FAILURES — this merge regressed main ████ against an innocent merge, and our own record then stamps an older sha over its newer one.

Not introduced by #472probe_gate.sh:106 fetches before :150 sweeps, so this is parity with what main ships today. Filing it here only so it is written down somewhere.

It is the same "can this banner be trusted" question as #478 (span attribution) and #471 (probe flake), and the cheapest fix composes with #478 rather than standing alone: keep the early read for the skip decision, re-read the baseline after the sweep for the comparison and the record. Two fetches, about a second, and the window closes.

Related: #478, #471, #446, #450.

Dependencies

None.

Comments

No comments.

Add a comment