PR #390 follow-ups: success-path hand-back, charter quoting rule, unpinned parsing nf-kdx7 ← Beads

closed priority 2 task unassigned

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

Suggestions from ninefold-reviewer's review of #390. All advisory — the PR reached result=CONVERGED with these deliberately left out rather than folded back in. None of them break anything today.

1. tools/pr/rebuild_f5.sh:132-135 — the success-path hand-back has no failure arm, and it is the path that runs on every rebuild. #379 item 3 hardened bail_to_start's checkout with an explicit else (:95-103). The final hand-back is still a bare git checkout -q "$START_REF" followed by an unconditional echo "→ Back on $START_REF". Under set -euo pipefail (:25) a failed checkout aborts there carrying only git's stderr — no ⚠ STILL ON f5/integration — and the line printed immediately before it is ✓ f5/integration rebuilt + pushed, which reads as done. Net effect after #390: the *rare* path says the #375 foot-gun is live, the *common* path does not. The fix is the same if/else shape already sitting 35 lines above, or hoisting that block into a shared helper so there is one hand-back rather than two.

2. tools/reviewer/CHARTER.md:83-89 — the new quoting-hazard note describes dispatch.sh's matcher inaccurately, which undercuts the rule. The note says both readers "take the newest bot post containing a line-start match" and prescribes keeping quoted verdicts indented. That holds for poll.sh — all three of its matchers are line-start (:78 (?m)^, :85 grep -m1 '^…', :199 startswith). tools/reviewer/dispatch.sh:143 is grep -oE 'REVIEWER-VERDICT sha=…', unanchored: it matches a quoted verdict inside an indented table cell just as readily, so indentation protects it not at all. What actually protects it is | head -1 plus the real verdict occupying the body's first line. #390's own dispatch.sh:144-146 note states the unanchored fact correctly, so the two artifacts disagree.

Nothing is broken today — but the note exists so a future ledger-format change trips over it there, and as written a change that dutifully keeps quotes indented while moving the verdict off line 1 satisfies the stated rule and still hands dispatch.sh a stale sha. Suggested wording: the invariant that covers both readers is "the verdict line must be the body's FIRST line", with indentation as the secondary rule for poll.sh.

3. tools/reviewer/dispatch.sh:403-406 — the new parsing is unpinned by --selftest. head_and_baselive_head/base_ref plus the [ "$live_head" = "$head_and_base" ] && base_ref="" unparseable guard is pure string work, and this file's own convention for pure string work is a selftest assertion — :145-147, on rewrite_verdict_sha, says so in as many words ("Pure string work, so --selftest can pin it"). Selftest is unchanged at 5/5; #390 verified the parsing by hand end-to-end instead, which holds for one pass but not the next edit. Pinning it means extracting the split into a small function (parse_head_and_base) and asserting the three cases the review probed: sha|main → both fields, garbagebase_ref empty, "" → both empty.

Worth noting for whoever picks these up: I verified under set -euo pipefail that the && guard in item 3 does not abort the sweep on the normal (test-fails) path — errexit exempts a non-final && operand. That was the one way the change could have broken every tick, and it doesn't. Item 3 is about future edits, not present behaviour.

Dependencies

None.

Comments

No comments.

Add a comment