Fix max-effort review findings in lock protocol, workflows, and docs

Confirmed by a 10-finder / 28-verifier adversarial review pass:

board_lock.py — the flock is now the sole authority: drop cmd_hold's
pid-liveness pre-gate (a live hil_test.py pool worker's stale record no
longer blocks a genuinely free board); cmd_release probes the flock and
only signals a verified holder, refuses to kill hil_test.py holders
(CI mid-test), handles PermissionError; the holder daemon truncates its
lock records on SIGTERM and keeps the success pipe clear of fds 0-2
(closed-stdio hold used to leave an orphan holder while reporting
failure); --config default resolves beside the script.

hil_test.py — truncate the lock record on per-board release (pool
workers outlive their flocks); warn instead of silently failing open
when the lock dir is unusable; error out on -b names absent from the
config (was a silent zero-test exit 0, readable as a green HIL run);
drop an emptied board row in accumulate_report (variant boards left a
blank ghost row).

workflows — remove the stray positional arg that made the validate size
stage exit 2 on every run; wrap JSON.parse(args) in all six scripts;
factor pr-babysit's drifted reply recipe into postReplyRecipe and dedup
refutation replies across cycles; validate args.pr and maxCycles;
driver-review rejects an empty dimensions list; hil-validate drops a
dead guard clause and retries diagnostics with -v -r 1.

agents/docs — port-dev scopes git clang-format to its own files
(concurrent workers reformatted each other in shared checkouts);
hil-operator/hil skill wording matches actual fail-fast output; the
implementation plan is now a DO-NOT-EXECUTE historical record (banner +
checked boxes) so plan-executing agents cannot revert shipped files.

Verified: lock storm 1-winner-in-10, stale-record hold, closed-stdio
hold, dead-pid cleanup, CI-holder refusal, ghost-row 4-scenario merge,
unknown-board exit 1, py_compile + check.sh on all six workflows,
pre-commit clean.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Upj4hta5TNoAbidqeC1zZ6

2 files changed, 94 insertions, 21 deletions

diff --git a/test/hil/board_lock.py b/test/hil/board_lock.py
index 42df80b35..c35e13705 100755
--- a/test/hil/board_lock.py
+++ b/test/hil/board_lock.py
@@ -3,12 +3,13 @@
 
 Arbitrates board access between dev sessions and CI's hil_test.py without
 stopping the actions-runner. Locks are kernel flocks: the kernel releases
-them automatically when the holder process dies, so stale locks are
-impossible (/tmp also clears on reboot).
+them automatically when the holder process dies, and holders clear their
+lock-file record on release so records stay truthful (/tmp also clears on
+reboot).
 
 Usage:
   board_lock.py hold BOARD [BOARD...] --reason TEXT
-  board_lock.py hold --all [--config test/hil/tinyusb.json] --reason TEXT
+  board_lock.py hold --all [--config CONFIG.json] --reason TEXT
   board_lock.py release BOARD [BOARD...] | release --all
   board_lock.py status
 
@@ -69,11 +70,9 @@ def is_locked(board: str) -> bool:
 
 def cmd_hold(boards, reason):
     os.makedirs(LOCK_DIR, exist_ok=True)
-    already = [b for b in boards if is_locked(b)]
-    if already:
-        for b in already:
-            print(f'ERROR: {b} already locked: {read_info(b)}', file=sys.stderr)
-        return 1
+    # No pre-check: the holder's own LOCK_NB flock is the only authority — a
+    # recorded pid may be stale or recycled (e.g. a live hil_test.py worker
+    # that already released this board's flock but not its record).
     # The holder signals success through this pipe. A generic is_locked()
     # poll would be fooled by a RIVAL invocation's flock — only the holder
     # itself knows whether it won every board.
@@ -88,7 +87,11 @@ def cmd_hold(boards, reason):
         if ok:
             print(f'held: {", ".join(boards)}')
             return 0
-        print('ERROR: holder failed to acquire locks (lost a race?)', file=sys.stderr)
+        for b in boards:
+            info = read_info(b)
+            if info:
+                print(f'ERROR: {b} locked: {info}', file=sys.stderr)
+        print('ERROR: holder failed to acquire locks', file=sys.stderr)
         return 1
     # intermediate child: detach, then spawn the actual holder
     os.setsid()
@@ -96,6 +99,10 @@ def cmd_hold(boards, reason):
         os._exit(0)
     # holder (grandchild): acquire all flocks, signal the parent, sleep until killed
     os.close(r_fd)
+    # Keep the success pipe clear of fds 0-2: invoked with stdio closed,
+    # os.pipe() can land there and the dup2 loop below would clobber it.
+    if w_fd <= 2:
+        w_fd = fcntl.fcntl(w_fd, fcntl.F_DUPFD, 3)
     # Detach stdio: a `hold` whose output is captured must see EOF when the
     # front-end exits — the immortal holder must not keep that pipe open.
     devnull = os.open(os.devnull, os.O_RDWR)
@@ -125,31 +132,71 @@ def cmd_hold(boards, reason):
         os._exit(1)  # lost a race; parent reports the failure
     os.write(w_fd, b'1')
     os.close(w_fd)
-    signal.signal(signal.SIGTERM, lambda *_: os._exit(0))
+
+    def _bow_out(*_):
+        # clear the records before dying so read_info/status stay truthful
+        # (the kernel drops the flocks themselves on exit either way)
+        for h in handles:
+            try:
+                h.truncate(0)
+            except OSError:
+                pass
+        os._exit(0)
+
+    signal.signal(signal.SIGTERM, _bow_out)
     while True:
         signal.pause()
 
 
 def cmd_release(boards):
-    pids = set()
+    rc = 0
+    victims = set()
     for b in boards:
-        if not is_locked(b):
+        try:
+            fd = os.open(lock_path(b), os.O_RDWR)
+        except OSError:
+            continue  # no lock file (or another user's): nothing we can release
+        fh = os.fdopen(fd, 'r+')
+        try:
+            fcntl.flock(fh, fcntl.LOCK_EX | fcntl.LOCK_NB)
+        except OSError:
+            # flock genuinely held — never SIGTERM on a mere pid record: the
+            # pid may be recycled, or a live worker that already moved on.
+            fh.close()
+            info = read_info(b) or {}
+            pid = info.get('pid')
+            if info.get('reason') == 'hil_test.py':
+                print(f'ERROR: {b} is mid-test by hil_test.py (pid {pid}) — not killing a '
+                      'CI run; wait for it to finish', file=sys.stderr)
+                rc = 1
+            elif isinstance(pid, int) and pid > 0:
+                victims.add(pid)
+            else:
+                print(f'ERROR: {b} is held but its record is unreadable', file=sys.stderr)
+                rc = 1
             continue
-        info = read_info(b) or {}
-        if info.get('pid'):
-            pids.add(info['pid'])
-    for holder in sorted(pids):
+        # flock was free: only a stale record remained — clear it
+        try:
+            fh.truncate(0)
+        except OSError:
+            pass
+        fh.close()
+    for holder in sorted(victims):
         try:
             os.kill(holder, signal.SIGTERM)
             print(f'released holder pid {holder}')
         except ProcessLookupError:
             pass
+        except PermissionError:
+            print(f'ERROR: holder pid {holder} belongs to another user — cannot signal it',
+                  file=sys.stderr)
+            rc = 1
     time.sleep(0.3)
     still = [b for b in boards if is_locked(b)]
     if still:
         print(f'ERROR: still locked: {", ".join(still)}', file=sys.stderr)
         return 1
-    return 0
+    return rc
 
 
 def cmd_status():
@@ -176,7 +223,10 @@ def main():
     p_hold = sub.add_parser('hold')
     p_hold.add_argument('boards', nargs='*')
     p_hold.add_argument('--all', action='store_true')
-    p_hold.add_argument('--config', default='test/hil/tinyusb.json')
+    p_hold.add_argument('--config',
+                        default=os.path.join(os.path.dirname(os.path.abspath(__file__)),
+                                             'tinyusb.json'),
+                        help='board roster JSON (default: tinyusb.json beside this script)')
     p_hold.add_argument('--reason', required=True)
     p_rel = sub.add_parser('release')
     p_rel.add_argument('boards', nargs='*')
diff --git a/test/hil/hil_test.py b/test/hil/hil_test.py
index 2e5085079..4b87154b6 100755
--- a/test/hil/hil_test.py
+++ b/test/hil/hil_test.py
@@ -75,8 +75,12 @@ def acquire_board_lock(board_name):
         fd = os.open(os.path.join(BOARD_LOCK_DIR, f'{board_name}.lock'),
                      os.O_RDWR | os.O_CREAT, 0o666)
         fh = os.fdopen(fd, 'r+')
-    except OSError:
-        return None  # odd lock dir (perms, path collision): proceed unlocked
+    except OSError as e:
+        # odd lock dir (perms, path collision): proceed unlocked, but say so —
+        # a silent fail-open is indistinguishable from the intentional bypass
+        print(f'warning: board lock unavailable for {board_name} ({e}); proceeding unlocked',
+              flush=True)
+        return None
     try:
         fcntl.flock(fh, fcntl.LOCK_EX | fcntl.LOCK_NB)
     except OSError:
@@ -1767,6 +1771,14 @@ def test_board(board: Board) -> tuple[str, int, list[str], list]:
         return name, err_count, sorted(set(failed_tests)), rows
     finally:
         if _lock_fh:
+            try:
+                # clear our pid record before dropping the flock: this worker
+                # process lives on (pool reuse), so a stale record would make
+                # board_lock.py's pid-liveness checks report a freed board as
+                # still locked for the rest of the run
+                _lock_fh.truncate(0)
+            except OSError:
+                pass
             _lock_fh.close()
 
 
@@ -1837,7 +1849,13 @@ def accumulate_report(mret: list, report_dir: Path, fresh: bool) -> str:
         if rows and not any('board-locked' in cells for _, cells in rows):
             # board ran for real this time: clear a stale lock-failure cell
             # (its row is keyed by board name; test rows may be variant names)
-            acc.get(name, {}).pop('board-locked', None)
+            stale = acc.get(name)
+            if stale is not None:
+                stale.pop('board-locked', None)
+                if not stale:
+                    # variant-keyed boards never repopulate the board-name row —
+                    # drop it or it renders as a blank ghost row
+                    del acc[name]
         for row_label, cells in rows:
             acc.setdefault(row_label, {}).update(cells)
 
@@ -1900,6 +1918,11 @@ def main() -> None:
     if len(boards) == 0:
         config_boards = [e for e in config['boards'] if e['name'] not in skip_boards]
     else:
+        unknown = [b for b in boards if b not in {e['name'] for e in config['boards']}]
+        if unknown:
+            # exiting 0 with 'No tests were run.' would read as a green HIL run
+            print(f'ERROR: board(s) not in {config_file.name}: {", ".join(unknown)}')
+            sys.exit(1)
         config_boards = [e for e in config['boards'] if e['name'] in boards]
 
     build_err = 0