diff --git a/scripts/super-board-collect-state.py b/scripts/super-board-collect-state.py index 0b400a0d..b1acbb4e 100755 --- a/scripts/super-board-collect-state.py +++ b/scripts/super-board-collect-state.py @@ -3,15 +3,26 @@ The lock is inherited by the real shell filer and its synchronous children. A pending write survives interruption and blocks retry until explicitly reconciled. + +POSIX: flock, exec into the filer, and the child proves the lock by the inherited fd's inode. +Windows (no fcntl): msvcrt locking, the filer runs as a `bash` child while this process holds +the lock, and the child proves it by a per-hold random token. Needs bash and python3 on PATH. """ import argparse -import fcntl +try: + import fcntl +except ImportError: # Windows + fcntl = None + import msvcrt import hashlib import importlib.util import json import math import os from pathlib import Path +import secrets +import shutil +import subprocess import sys import time @@ -30,6 +41,51 @@ def paths(repo): return key, root / (key + '.lock'), root / (key + '.json') +def try_lock(fd): + """Take the exclusive lock without waiting; BlockingIOError when another holder has it.""" + if fcntl: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + return + os.lseek(fd, 0, os.SEEK_SET) + try: + msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) + except OSError as exc: + raise BlockingIOError(*exc.args) from exc + + +def run_locked(fd, key, lock_path, args): + """Run the filer holding the lock; returns only on the portable path.""" + if fcntl: + os.set_inheritable(fd, True) + env = dict(os.environ, SB_COLLECT_LOCK_HELD=key, SB_COLLECT_LOCK_FD=str(fd)) + os.execvpe(args[0], args, env) + # No shebang exec and no fd inheritance: run the filer as a child under a token file. + token_path = lock_path.with_suffix('.token') + token = secrets.token_hex(16) + token_path.write_text(token) + env = dict(os.environ, SB_COLLECT_LOCK_HELD=key, SB_COLLECT_LOCK_TOKEN=token) + env.pop('SB_COLLECT_LOCK_FD', None) + if args[0].endswith('.sh'): + args = [shutil.which('bash') or 'bash', *args] + try: + return subprocess.call(args, env=env) + finally: + token_path.unlink(missing_ok=True) + + +def lock_held(key, lock_path): + if os.environ.get('SB_COLLECT_LOCK_HELD') != key: + return False + if fcntl: + fd = int(os.environ.get('SB_COLLECT_LOCK_FD', '-1')) + return os.fstat(fd).st_ino == lock_path.stat().st_ino + token = os.environ.get('SB_COLLECT_LOCK_TOKEN', '') + try: + return bool(token) and secrets.compare_digest(lock_path.with_suffix('.token').read_text(), token) + except FileNotFoundError: + return False + + def load(path): if not path.exists(): return {'receipts': {}} @@ -71,7 +127,7 @@ def main(): while True: gh.check_halt() try: - fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + try_lock(fd) break except BlockingIOError: if time.monotonic() >= deadline: @@ -83,20 +139,15 @@ def main(): gh.halt('Collector write outcome unresolved; reconcile pending receipt before retrying', 1) if not args: raise ValueError('--lock needs the existing filer command') - os.set_inheritable(fd, True) - env = dict(os.environ, SB_COLLECT_LOCK_HELD=key, SB_COLLECT_LOCK_FD=str(fd)) - os.execvpe(args[0], args, env) + return run_locked(fd, key, lock_path, args) elif a.action: - fd = int(os.environ.get('SB_COLLECT_LOCK_FD', '-1')) - held = os.environ.get('SB_COLLECT_LOCK_HELD') == key - if held: - held = os.fstat(fd).st_ino == lock_path.stat().st_ino + held = lock_held(key, lock_path) # Explicit manual reconciliation acquires the same lock; never clears a live writer. stream = None if a.action == 'reconcile' and not held: stream = lock_path.open('a+') try: - fcntl.flock(stream, fcntl.LOCK_EX | fcntl.LOCK_NB) + try_lock(stream.fileno()) except BlockingIOError: return 4 held = True diff --git a/scripts/super-review-file-refactor.sh b/scripts/super-review-file-refactor.sh index 13d97c9c..c61a614f 100755 --- a/scripts/super-review-file-refactor.sh +++ b/scripts/super-review-file-refactor.sh @@ -7,8 +7,8 @@ # warning; unavailable required evidence or an unknown write outcome pauses the # run (79) while preserving any issue already created. No mutation is retried. # -# Cards land in Backlog, not Ready. A refactor the reviewer noticed has had no human -# eyes on it and no acceptance criteria; auto-promoting it to Ready would feed the +# A card with real acceptance criteria lands in Ready; one without lands in Backlog. +# A bare note has had no human eyes on it; auto-promoting it to Ready would feed the # build lane work nobody asked for. It waits for `super-board lint` like any other # unrefined ticket. # @@ -181,6 +181,10 @@ LABEL_ARGS=() for l in "${LABELS[@]}"; do LABEL_ARGS+=(--label "$l"); done # Title: `♻️ [refactor] : `; scope = --area, else the fingerprint's module. +# --title is the bare problem. A caller that followed the house format passes +# `<emoji> [<kind>] <scope>: <subject>`; drop that prefix so it is not doubled. Only a +# leading all-non-ASCII word (the emoji) triggers it, so `Map [x] fields: keep order` stays. +TITLE=$(printf '%s' "$TITLE" | LC_ALL=C sed -E 's/^[^ -~]{1,16} \[[a-z-]+\] [^:]{1,40}: //') SCOPE="${AREA:-${FINGERPRINT%%|*}}" SCOPE=$(echo "${SCOPE:-code}" | tr '[:upper:]' '[:lower:]' | sed -E 's#[^a-z0-9._/-]+#-#g; s#^-+|-+$##g') python3 "$GITHUB_READ" --check || exit $? diff --git a/skills/super-collect/references/scheduled-intake.md b/skills/super-collect/references/scheduled-intake.md index 302213aa..d5fe5200 100644 --- a/skills/super-collect/references/scheduled-intake.md +++ b/skills/super-collect/references/scheduled-intake.md @@ -32,8 +32,9 @@ access. No universal scheduler service is bundled. ## Same-host overlap and receipts -Collectors serialize by repository identity on one POSIX machine. They share an -inherited OS lock in project-local `.claude/super-board/collect/`, resolved through +Collectors serialize by repository identity on one machine. They share an OS lock +(flock on POSIX, msvcrt on Windows, where the filer runs as a `bash` child under a +per-hold token) in project-local `.claude/super-board/collect/`, resolved through the existing GitHub halt location across worktrees. `SB_COLLECT_STATE_DIR` can select another dedicated shared ignored directory. The helper creates a self-ignoring `.gitignore` there, including after an app install. Receipts contain diff --git a/skills/super-review/SKILL.md b/skills/super-review/SKILL.md index 669613fa..01c4e227 100644 --- a/skills/super-review/SKILL.md +++ b/skills/super-review/SKILL.md @@ -112,6 +112,7 @@ If the input is ambiguous, default to reviewing the current branch against its u - If a blocker is a functional regression, hand it to **Super QA**. - If a blocker is visual/design fidelity, file a UI ticket; a human can run `/ui-refine-loop` (the board never triggers it). - If it is a deepening opportunity, file it with `scripts/super-review-file-refactor.sh` and carry on to the merge decision. Do not open a PR thread for it; do not bounce the card. + `--title` is the bare problem: the filer adds the `♻️ [refactor] <scope>:` prefix, so never pass one. The `--body-file` uses the ticket format (writing-standard.md § 3): `## Problem`, `## Context` (`- **Where:**` the module and files), `## Fix` — the filer refuses a body without them. **Write real acceptance criteria too.** A card that carries an diff --git a/tests/test-file-refactor.sh b/tests/test-file-refactor.sh index 21b88911..d4c2370b 100755 --- a/tests/test-file-refactor.sh +++ b/tests/test-file-refactor.sh @@ -95,6 +95,17 @@ has "lands in Backlog, not Ready" "$(cat "$GH_LOG")" "opt_Backlog" has "labels the source" "$(cat "$GH_LOG")" "source:review" has "labels the strength" "$(cat "$GH_LOG")" "strength:strong" has "title in ticket format" "$(cat "$GH_LOG")" "♻️ [refactor] orderintake: OrderIntake is shallow" + +echo "── a pre-formatted title is not prefixed twice (#65)" +title_of() { grep '^issue create' "$GH_LOG" | sed -E 's/.*--title (.*) --body-file.*/\1/'; } +for case in "♻️ [refactor] images: one image-with-fallback component|♻️ [refactor] orderintake: one image-with-fallback component" \ + "🐛 [bug] people: buttons squeeze names|♻️ [refactor] orderintake: buttons squeeze names" \ + "Map [x] fields: keep order|♻️ [refactor] orderintake: Map [x] fields: keep order" \ + "drop the web root-path fallbacks: after the move|♻️ [refactor] orderintake: drop the web root-path fallbacks: after the move"; do + DEDUPE_HIT="" run --config "$WORK/config.json" --title "${case%%|*}" \ + --body-file "$WORK/body.md" --fingerprint "OrderIntake|title-${#case}" >/dev/null + is "title: ${case%%|*}" "${case#*|}" "$(title_of)" +done printf 'just a note\n' > "$WORK/nosections.md" is "refuses a body without Problem/Context/Fix" 66 "$( DEDUPE_HIT="" "$SCRIPT" --config "$WORK/config.json" --title T --fingerprint f --body-file "$WORK/nosections.md" >/dev/null 2>&1; echo $? )" diff --git a/tests/test_collect_windows.py b/tests/test_collect_windows.py new file mode 100644 index 00000000..cc24abea --- /dev/null +++ b/tests/test_collect_windows.py @@ -0,0 +1,100 @@ +"""Collector lock on a host without fcntl, fd inheritance or shebang exec (Windows, #64). + +The real scripts run under a shim that hides fcntl and supplies an msvcrt stand-in, so the +whole intake suite and the lock-token guards are exercised through the non-POSIX path. +""" +import json +import os +from pathlib import Path +import subprocess +import sys +import tempfile +import unittest + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import test_collect_intake_safety as intake # noqa: E402 + +ROOT = intake.ROOT +STATE = ROOT/'scripts/super-board-collect-state.py' +SHIM = r'''#!/usr/bin/env python3 +import fcntl as _real, os, runpy, sys, types +# Python 3.12+ stdlib (subprocess) probes `import msvcrt` to decide it is on Windows; load it +# before the stand-in exists so only the script under test sees msvcrt. +import subprocess # noqa: F401 +sys.modules['fcntl'] = None # import fcntl -> ImportError, as on Windows +m = types.ModuleType('msvcrt'); m.LK_NBLCK, m.LK_UNLCK = 2, 0 +def locking(fd, mode, nbytes): + if mode == m.LK_UNLCK: return _real.flock(fd, _real.LOCK_UN) + try: _real.flock(fd, _real.LOCK_EX | _real.LOCK_NB) + except BlockingIOError: raise PermissionError(13, 'Permission denied') # msvcrt's contention error +m.locking = locking; sys.modules['msvcrt'] = m +real = os.environ['SB_TEST_REAL_STATE'] +if os.environ.get('SB_TEST_LOG_FDS'): + with open(os.environ['SB_TEST_LOG_FDS'], 'a') as f: f.write(os.environ.get('SB_COLLECT_LOCK_FD', '') + '\n') +sys.argv[0] = real; runpy.run_path(real, run_name='__main__') +''' + + +def shim_bin(root): + bin_dir = root/'bin'; bin_dir.mkdir() + for f in (ROOT/'scripts').iterdir(): + if f.name != STATE.name: + (bin_dir/f.name).symlink_to(f) + (bin_dir/STATE.name).write_text(SHIM); (bin_dir/STATE.name).chmod(0o755) + return bin_dir + + +class WindowsIntakeTests(intake.IntakeTests): + """Every intake regression again, through the portable lock path.""" + def setUp(self): + super().setUp() + self.env.update(SUPER_BOARD_BIN=str(shim_bin(self.root)), SB_TEST_REAL_STATE=str(STATE)) + + def test_portable_path_never_passes_an_fd_token(self): + log = self.root/'fds.log' + result = self.run_filer(SB_TEST_LOG_FDS=str(log)) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertNotIn('Errno 8', result.stderr) + self.assertEqual(set(log.read_text().splitlines()), {''}) + + def test_receipt_action_refuses_forged_or_stale_token(self): + self.assertEqual(self.run_filer().returncode, 0) + key = __import__('hashlib').sha256(b'acme/app').hexdigest() + cmd = [sys.executable, str(self.root/'bin'/STATE.name), '--repo', 'acme/app', '--action', 'begin', + '--fingerprint', 'custom|demo|one'] + for token in ['', 'forged']: + with self.subTest(token=token): + env = dict(self.env, SB_COLLECT_LOCK_HELD=key, SB_COLLECT_LOCK_TOKEN=token) + result = subprocess.run(cmd, env=env, capture_output=True, text=True, timeout=15) + self.assertEqual(result.returncode, 70, result.stderr) + self.assertIn('require the held repository lock', result.stderr) + state = json.loads(next((self.root/'collector-state').glob('*.json')).read_text()) + self.assertNotIn('pending', state) + + def test_interrupted_write_still_blocks_retry(self): + first = self.run_filer(FAIL_CREATE_AFTER_STORE='1') + self.assertEqual(first.returncode, 79, first.stderr) + (self.root/'halt.json').unlink(missing_ok=True) + second = self.run_filer() + self.assertEqual(second.returncode, 79) + self.assertIn('Collector write outcome unresolved', second.stderr) + + +class ImportTests(unittest.TestCase): + def test_shim_loads_stdlib_before_the_msvcrt_stand_in(self): + # Regression guard (PR #68 CI): on 3.12 a stand-in msvcrt visible to subprocess's + # import flips it to its Windows branch (_winapi). Holds on every interpreter. + self.assertLess(SHIM.index('import subprocess'), SHIM.index("sys.modules['msvcrt']")) + + def test_module_imports_without_fcntl(self): + with tempfile.TemporaryDirectory() as tmp: + bin_dir = shim_bin(Path(tmp)) + env = dict(os.environ, SB_TEST_REAL_STATE=str(STATE)) + result = subprocess.run([sys.executable, str(bin_dir/STATE.name), '--help'], + env=env, capture_output=True, text=True, timeout=15) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertNotIn('ModuleNotFoundError', result.stderr) + + +if __name__ == '__main__': + unittest.main()